Aircloak / aircloak

This repository contains the Aircloak Air frontend as well as the code for our Cloak query and anonymization platform
2 stars 0 forks source link

Explorer tweaks #4816

Closed sebastian closed 3 years ago

sebastian commented 3 years ago

This pull-request does a number of things:

This is what the new setup looks like:

Screen Shot 2020-11-10 at 17 38 30
aircloak-robot commented 3 years ago

Standard tests have passed 😊

gampleman commented 3 years ago

On further reflection, I'm not convinced about the use here of checkboxes, given that clicking them immediately leads to some action (with consequences like launching an analysis of a large table for instance). Either there should be a way to confirm the selection before committing, or these should be buttons labeled with the action that will happen (Analyze/Cancel/Remove analysis).

aircloak-robot commented 3 years ago

Standard tests have passed 🎉

sebastian commented 3 years ago

On further reflection, I'm not convinced about the use here of checkboxes, given that clicking them immediately leads to some action (with consequences like launching an analysis of a large table for instance). Either there should be a way to confirm the selection before committing, or these should be buttons labeled with the action that will happen (Analyze/Cancel/Remove analysis).

How about making it a toggle instead? such as this: https://gitbrent.github.io/bootstrap4-toggle/ I don't think the damage or large scale analysis is all too bad. The explorer now issues cancel requests to halt explorations if you toggle it again.

gampleman commented 3 years ago

I think toggles would be better, yeah (although I still think explicitly labeled buttons might be the best, although I appreciate that aesthetically it might be suboptimal).

sebastian commented 3 years ago

I believe I have addressed your feedback. The new version now looks like this:

Screen Shot 2020-11-13 at 11 50 14

It's a bit of a cluster fuck of colors, but I hope the utility is still sufficiently OK.

aircloak-robot commented 3 years ago

Standard tests have passed 💯

sebastian commented 3 years ago

Ready for re-review

gampleman commented 3 years ago

Also on several readings of this PR I am a bit less convinced about the soft delete. It seems to me like the bug we saw was some sort of data integrity issue which this may or may not hide, but doesn't really address head on, while adding an additional state all the code has to consider (exists, doesn't exist, soft deleted).

sebastian commented 3 years ago

Also on several readings of this PR I am a bit less convinced about the soft delete. It seems to me like the bug we saw was some sort of data integrity issue which this may or may not hide, but doesn't really address head on, while adding an additional state all the code has to consider (exists, doesn't exist, soft deleted).

Yes, I agree that it is just a bandaid, at the cost of extra state and complexity. I do not however see another solution on the horizon. These empty data sources can happen for a number of reason. DB being offline or overloaded, or be a cause of a bug in cloak that we haven't yet found. Either way it is the case (I have observed it on a number of occasions) that analysis vanish (where the audit log makes no mention of changes). Given the high cost of regenerating them I would much rather take this added complexity, than lose state and hard earned analysis results!

The interface now looks like this (not pretty, but I guess OK):

Screen Shot 2020-11-16 at 11 13 41
aircloak-robot commented 3 years ago

Standard tests have passed 💯

aircloak-robot commented 3 years ago

Standard tests have passed 😊

sebastian commented 3 years ago

Ready for re-review

aircloak-robot commented 3 years ago

Pull request can be merged 😊