Upstream "Delete Sources" (batch-delete) implementation

[rocodes]

Description

Refs #1569

Tracking issue to discuss work plan for upstreaming guardian#5 ; I didn't put this in a comment in the original issue since it might get into the weeds.

Here're my high-level thoughts:

• Keep the same structural approach (new multi-delete QAction that dispatches sequential DeleteSourceActions)
• Instead of adding a button at the top of the SourceList, add a new UI widget, a side panel for batch/multi-source actions, in the LeftPane edit: (new) Top Pane. The only batch action we will currently support is "Delete Sources" Batch Delete (name TBD).
• I don't think we need to conditionally enable/disable the batch delete button the way the fork does. For a first iteration let's not add more branching logic to update_sources in the controller, which is already a bit complex. If the user clicks "batch delete" and no sources are selected, the confirmation modal can simply say that no sources are selected, and we can revisit this later.
• Update and add tests so that we guard against race conditions. The current batch delete tests would pass if the wrong source was selected for deletion due to a race between dynamic reordering of the sourcelistwidget items and the deletion request - our tests should ensure that the uuid (not the list widget position) is being checked, and we should create a few different test conditions, basically, ensuring throughout that we never reference the list widget position and always reference the uuid:
  • business as usual (straightforward batch delete)
  • select items for batch delete, source list reorders one or more times (not all items must reorder), ensure items selected still are correct (by uuid)
  • select items for batch delete, and before batch delete happens, some items are remotely deleted (ensure no new items are selected)

For a work plan, I'm proposing (open to discussion):

• Initial small-scoped refactor PR that updates the DeleteSourceDialog to allow it to take a list of 0..n sources instead of a single source. If the list is empty, tell the user "no sources are selected". Refactor DeleteSourceDialog so it accepts Set[Source] #2188
• [new] A PR that moves the status bar to the bottom
• A PR that adds a new "Batch Action" widget to the LeftPane (new) Batch Action (Top) Pane. This is a UX change, so we need some consensus on it as well before we proceed. Update: thanks for feedback everyone! Add batch actions top bar element #2233
  • Another UX thing: naming the button. Update: "Delete Sources" [per below, thanks for feedback everyone!] "Batch Delete" ? "Multi-delete" ?
• A docs PR to update app screenshots with UI changes: Update sd-app screenshots for Client 0.14.0 release  securedrop-workstation-docs#266
• A separate PR that does most of the upstreaming, minus the parts we have changed above, and adds test cases as described above.
  • (edit) last nuance: worth investigating the difference between adding every source uuid to an in-memory list of selected uuids (and removing if unchecked) at the time its checkbox is clicked, vs the existing implementation, which gathers the list of checked widget items at the time the batch delete is requested. In an ideal world there should be no difference, but if there ever were a GUI reordering bug/race condition, the current implementation is a little more vulnerable than the one that grabs UUIDs at the time of the click. The tradeoff is complexity (unselect - need to remove from the list; remotely deleted; need to remove from the list)

[legoktm]

Overall your plan seems reasonable to me, I wonder if it'll be easier to pull in the upstream work first and then refactor it. I just tried naively merging the guardian delete-multiple-sources branch into our main and it threw up a number of conflicts, partly (or entirely?) because it was pre-monorepo; so not sure if it'll be even more work to pull it in after further changes.

[rocodes]

I tried the same thing :) I also tried some variants of squashing the upstream commits into per-file commits so that cherry-picking would be easier, but didn't get too far since I had other stuff on the go.

It's a pretty small changeset, and will be a fairly small diff overall, so I don't think we'll feel too much pain no matter what we do. I would suggest that we start with the refactor prep we may want to do regardless (ModalDialog), and then the ordering of the other two steps isn't that important (although I honestly think it may be easier to create the 'dummy' batch delete UI pane first, not have it hooked up to anything, and then hook it up in the last step, as I've suggested here, cause then the step where we pull tg work in is also the step where we have all the pieces in place to test it, and we'll have preexisting UI tests for the components we changed so we can ensure our changes conform to our expectations).

I usually try to prioritize directly cherry-picking commits, but I also think if that gives us any extra pain (because of monorepo), we can make sure we preserve attribution via commit messages, release acknowledgements, etc; my sense is that pragmatism should win the day here.

[philmcmahon]

Need to check with @zekehuntergreen as well but I think we (guardian fork maintainers) would also encourage pragmatism - if we wanted attribution we really ought to have opened a PR to upstream! Really happy you're doing the work for us 🙏🏻

Our main branch is in sync with the monorepo but I don't think that's very helpful when it comes to trying to pick the commits from the original batch delete PR. I found that the diff of our main with version 0.12.0 of freedomofpress/main was a bit more readable - just skip anything related to export or 'whistleflow'

Thanks again for upstreaming this ❤️

[rocodes]

Thank you @philmcmahon @zekehuntergreen - at minimum we'll be able to provide attribution in commit messages, changelog, and release notes/announcement; individual commits TBD. We'll be in touch before all of that to run everything attribution-related by you.

[rocodes]

Hey folks, I want to request some UX/UI input on two questions. There was some discussion in #2188 about ux best practices, and in this case I'm looking for feedback on where the batch action button/toolbar should go. This button, when clicked, will launch a confirmation modal dialog, as is done when a single source is deleted.

Questions:

• Where should the Batch Delete toolbar/button be located?
• What should the keyboard shortcut for batch delete (keyboard shortcut will still prompt confirmation dialog) be?

Batch delete location
Proposed guiding principles, no particular order:

• Don't put destructive action near routine/non-destructive actions (avoid misclicks)
• Put actions as conceptually near the elements they apply to as possible
• Don't crowd actions in the ui, especially those with different interaction types (typing, clicking, menu opening)
• Make common actions as quick as possible (toolbar ux) - so avoiding a hidden/dropdown menu for repetitive actions

Factors to consider:

• Bulk delete is one bulk action; future others may or may not include bulk export, bulk sanitize.
• Bulk actions (applying to records) are different than global actions (backup, check system health, tooltips, whatever)
• The space right above the source list is probably going to be needed for keyword filtering.

As far as I can tell, these are the 3 main places we could put the Bulk Delete button.

Option A: the least crowded, the easiest to implement (already done), the most space for other actions, BUT the least consistent with other good UI principles since it's kind of in a random place.

Option B: In a proposed top bar, which is more in line with traditional UX, but: smaller, won't be able to show lots of text without mouseover, won't be able to have "Batch Actions" title (can have mouseover but not as good), farther from sourcelist. Nothing to misclick in the top bar, but source overflow menu is nearby if a source is active.

Option C: Located closest to sourcelist - sensible UX wise, but very crowded, lots of things to misclick (user menu, stars) and visual crowding; also, will take pace that is probably better suited for keyword filtering searchbar.

Ultimately, I wonder if the best design for us would be a much smaller or horizontally-oriented LeftPane. I think it eats a lot of real estate. But I don't want to open that can of worms right now, so I'm kind of asking for what's the combination of clear/sensible and not too bad to implement.

Keyboard shortcut
@cfm proposed adding a keyboard shortcut, which I think is a good idea and has also been requested by users. This shortcut would trigger the delete confirmation modal dialog. (If no sources are selected, the dialog will display a "please select some sources" message per #2188). What should the sequence be? Currently Ctrl+D is Download (all) from within the source menu, which may or may not be the shortcut we want to stick to for download. We also don't want to interfere with any Qubes shortcuts. Suggestions welcome.

[zenmonkeykstop]

Here's my mobile-pattern-influenced recco (implementation might be hairy tho):

• add a small (no taller than an existing source list item) button panel just above the source list.
• use it to display the num of sources selected, along with buttons for some or all batch actions you can apply to 1+ sources (currently I'd say dwonload and delete)
• if you want to get really fancy, only display it when multiple sources are selected (slide it down with a nice little animation)

IMO this takes up an appropriate amount of screen real estate and keeps the actions associated with the thing they act upon. I'd keep the space in A for future tagging/organizing/spam management controls, we're not ready to do the filter
bar in C, and B has the problems you mentioned already
Here is some bad ascii (DWN and DEL would be icons with tooltips):

                                            -                                         
                                                                                      
+----------------+-------------------------------------------------------------------+
|                |   future filter box (not added yet)                               |
|                +-------------------------------------------------------------------+
|                | (# selected)  DWN DEL  |                                          |
|                |------------------------|                                          |
|                |   source name          |                                          |
|                |------------------------|                                          |
|                |  other source name     |                                          |
|                |------------------------|                                          |
|                |   source again         |                                          |
|                |------------------------|                                          |
|                |  a source name etc     |    CONVO BOX                             |
|                |------------------------|                                          |
|                |                        |                                          |
|                |                        |                                          |
|                |                        |                                          |
|                |                        |                                          |
|                |                        |                                          |
|                |                        |                                          |
+----------------+------------------------+------------------------------------------+

(I realise this puts the download (non-destructive) and delete (destructive) operations beside each other, violating your first principle - IMO that's ok because confirmation dialogs.)

[deeplow]

Another UX heuristic is to follow conventions. Since the workstation was modeled around a webmail client, it makes sense that bulk actions are done right above the selection mechanism. This would either be C or @zenmonkeykstop's suggestion.

Something else that we may or may not want to consider is the ability to "select all". For the client this may seem like a useless (and extremely risky operation -- I agree). The only situation where it would make sense for a "select all" option is if this is combined with search (e.g. a particular spammer sends always the same content). This is how gmail did it some years ago:

But this second bit is probably a bit out of scope for now until search becomes a thing. Just bringing it up because it may have implications when considering if ☑️ at the top is something we want.

[deeplow]

Options A and C I wouldn't consider because A) feels more for user-related actions and C for message-related ones. But I am no UX expert, so it could make sense.

[rocodes]

Thanks all, this has been super helpful. I want to continue talking about it a bit more, maybe at the team meeting, but for now here's some more thoughts.

@zenmonkeykstop's suggestion looks like the fork's implementation to me:

I like that the batch delete toolbar/button is over the area it interacts with. I don't love that it is only the width of the sourcelist, which means a) the conversation view (righthand panel) is higher up than the top of the sourcelist, which disconnects them from each other, and b) the toolbar can only be as wide as the sourcelist view (limits future batch options).

• Re @deeplow's comment about design patterns: The client was modeled more around a Slack client/IM app than a web client as far as I understand it (think: ReplybBox, LeftPane, etc). But I think the web client model (and your screenshot) are actually much better for us and are where our mental model should be/seems to be converging.
• I want to de-scope "select all" specifically as a question for now, but keep the broader question (other bulk actions) in scope. The screenshot you shared is very relevant; there are numerous bulk-context items (select all is one, archive/delete/move to etc are others). The priority ones are available with one click (so no clicking dropdown menu). In this case the search bar and the actions toolbar are both on top of the list view (makes sense, contextually relevant).

I think based on this feedback I'm leaning towards nixing A, nixing C, and finding a way to do ~B that still feels visually coherent. I'll put up some other mockups to play around with.

Things I think are important:

• Be able to access the option with just one click
• Lines up in some way with sourcelist view

thank you all for your feedback so far, super helpful

[zenmonkeykstop]

I didn't realize that the upstream version had that there already! The styling/UX needs work, but would it make it easier or faster to implement to go with the placement as is and just fix up the widget itself to be cleaner and more extensible?

[zenmonkeykstop]

Actually scratch that, I'm leaning more towards ~B/webclienty version now as well.

[rocodes]

There are 2 ~ B, web-client type variants:

• Combined toolbar/searchbar layout (one row, some of which is taken up by toolbar, some of which is taken up by searchbar)
• 2 header rows (Gmail screenshot, search and toolbar are in separate rows)

And there is one further point:

• Arguably, our Status Bar should be at the bottom of the page, or shouldn't take up permanent width across the top since it is noninteractive but in a prime visual area for the user. Desktop application Status Bars (noninteractive, progress or message reporting) are different than toolbars (interactive, at the top) and address/search bars. See eg https://www.wix.com/encyclopedia/definition/status-bar , also eg Office suite tools and other more traditionally desktop-first applications, also this good discussion: https://ux.stackexchange.com/questions/39265/placement-of-flash-growl-notifications-or-messages-in-web-application

I think that we should have at most 2 horizontal rows at the top of the app. So we could leave the status bar where it is, and add one row (combined search/toolbar), or we could move the status bar to the bottom (also could help with having a place to show download progress when we get there), and have 2 rows at the top, one that containing the toolbar elements and a space for flashed messages (the error messages), the one below containing the search field.

too many top bars :) (This is my argument for maybe moving the status bar to the bottom, but I don't know that I would rush to do this)

[rocodes]

Ok. These are my best proposed 3, ranked in order. I know it seems finicky but I think it's a good idea to play around with a few options.
In all cases, I'm kind of deferring the decision about whether the keyword search bar is combined with the batch actions bar, because keyword search is still a ways away.

Basic B ;) Button is close to logout button, misclicks are possible, but it's simplest.

Whitespace B: Align the start of the button with the journalist designation text. Leave some space, taper/shape the white background eg like a tab taper shape, or put a nondestructive element such as a counter or menu icon in the top left corner. (This also leaves a checkbox sized space re future Select All discussion, but there be dragons). Pros: Less crowded corner for misclicks. Cons: maybe overcomplicating things.

B to the top: Either of above, but move the status bar down. I ranked this at the bottom even though I think it's maybe the best-looking choice, because we don't need to make decisions about any changes like this til we do keyword filtering, and it's more work.

Last questions I promise:

• Do you like the first option? (Button justified left). Anyone want to argue for the second option (leave some space before the button and include some other element like a counter, icon, label that says "batch actions", etc)?
• Do you like the gray/off-white bar (used for the empty ConversationView), with a white button with the blue text, outline, and icon? Obviously the styling will be the right fonts etc. I don't really like the blue text, I might try our other darker blue since we use that elsewhere for other button text, but basically is the white background, darker text/outline alright with everyone.
• This bar is intended to have the same height as the status bar. (I'll fix the weird blue border part, I'm basically messing with a ms paint type tool, not a real prototyping tool, these are very rough just for example purposes). If anyone wants to argue for a different height, please say so now.
• There will be a hover tooltip for the button that explains what it does.
• Keyboard shortcut suggestions welcome

I don't think it's gonna win any design awards, but I think it will work well enough. Thanks for playing, y'all, your feedback's been great.

[hoyla]

My tuppence: I don't like being wordy, but should this button make it clear it's "Batch Delete Sources" rather than "Batch Delete Content"?

Ideally I'd like both options, but perhaps the distinction should be made clear even if only one batch operation is offered.

c.f. the menu syntax "Delete All Files and Messages" vs "Delete Source Account".

We use the two functions for very different purposes: we delete content for housekeeping, data privacy, disk space, additional security etc; we delete sources to end a conversation entirely (often in the hope it might dissuade someone from continuing). In practice deleting sources is far more common than retaining a source account but deleting their correspondence.

And fwiw I like the status bar at the bottom, but agree that that is perhaps a separate matter.

[zenmonkeykstop]

I think the eventual state of 2 rows up top (filter on top, toolbar underneath) makes sense, it gives a lot of room for doing interesting filter bar stuff.

For the initial implementation I'm into the last variant (toolbar up top, status bar moved to bottom). It feels very idiomatic which is no bad thing! If we're concerned about confusion about what actions apply to sources vs conversations we can just group them like

+---------------------------------------------------------------------------+
|   batch buttons here   ||   convo buttons here  ||                        |
+ --------------------------------------------------------------------------+

One thing to think about though, is how errors are displayed. They currently get shown centered in the status bar, so front and center for users. IMO that was a compromise tho, as it means they're not shown in context. For this it seems ok to keep them in the status bar even if it does move, but once we have some dedicated UX resources we should probably give errors some love.

[rocodes]

@hoyla: Thank you for weighing in :) Maybe "Delete Sources" instead of "Batch Delete"? I wasn't sold on the world "Batch"/"Bulk" anyway (idiomatic English, harder to translate). Typically, toolbar buttons don't have any text, but I think in our case it's warranted to have a good description, as well as a tooltip (visible on hover) that has a longer description. The dialog will also be the same confirmation modal that explains what will be deleted. Would that work?

Re: further options: yes, part of all this is so that it's not a big lift to add further options later on (including some options in a drop-down menu), but for now it will just be Delete Sources, due to an understanding that deleting files and messages only is a less common task. (Napkin sketch: the individual Source overflow menu ("3 dot menu"), where export, download, etc, are, could eventually migrate to this bulk actions toolbar, so that they can be applied to n>=1 sources, instead of accessed in an individual source conversation; and more options, such as sanitize/export, can eventually be integrated).

@zenmonkeykstop: Thank you again too, and agreed-- the error handling is on my mind too. I think the jury's out; some people say "Flashed" (aka toast, aka banner) messages should be on the bottom (https://spectrum.adobe.com/page/toast/#Placement) "unless they're too important and need to be on the top," some have flashed/banner messages on the top with the proviso that they are important/rare and need the users' attention, and this is also in line with the Flashed messages on the JI/SI. But for sure the "status" (last connection check, routine notifs) should be on the bottom.

I'll put up a draft PR next per @zenmonkeykstop's last, so folks can play around with it, and I'll attach some screenshots.