Refactor DeleteSourceDialog so it accepts Set[Source]

[rocodes]

Status

Ready for review

Description

Refactor DeleteSourceDialog so it takes a set of sources instead of a single source. Display a user-friendly error message if the empty set is provided. Add parameterized unit tests to test empty, n=1, and n>1 conditions. This does not include any actual batch delete functionality, but it is in preparation for that change.

I was tempted to also refactor the DeleteSource and DeleteConversation dialogs into one dialog element, since there's no reason not to and is in line with what we did on the Print, Print Conversation, and Print Transcript dialogs, but I think it's too much scope creep. But it would be a straightforward refactor PR later on if anyone wants to take it on.

This PR makes minor string changes that will need il8n.

Towards #2160

Test Plan

• Visual review
• CI

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

• I have tested these changes in the appropriate Qubes environment
• I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
• These changes should not need testing in Qubes

If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable:

• I have updated the AppArmor profile
• No update to the AppArmor profile is required for these changes
• I don't know and would appreciate guidance

If these changes modify the database schema, you should include a database migration. Please check as applicable:

• I have written a migration and upgraded a test database based on main and confirmed that the migration is self-contained and applies cleanly
• I have written a migration but have not upgraded a test database based on main and would like the reviewer to do so
• I need help writing a database migration
• No database schema changes are needed

[rocodes]

(realized I want to add one more test before ready for review :) )

[deeplow]

Thanks for the PR. It generally makes sense. It took me some extra time to review due to the need to familiarize myself with the client code and find out was to test the code.
The code and the tests look fine to me in general. I only found that source names are not displayed.

I do have some more conceptual questions about how deletion could take place, but your approach is clearly simpler. It's just the kind of stuff where UX help would be nice to have 🙃 )

[deeplow]

Alternative approach suggestion

While reviewing the PR, I though of another alternative approach. Since this is such a destructive action I wonder, if source deletion should be done one by one. As in, one delete dialog per source (kind of like how deleting multiple qubes asks you for each).

Another idea which could be done on top of that would be for each source deletion dialog, the source view would be changed to the source about to be deleted, such that in the background the user can be reminded of the last interactions with the source. This would be source of a visual reminder of what they're about to delete.

This is one of those situations where some UX help would be great 🙂

[rocodes]

Alternative approach suggestion

This is one of those situations where some UX help would be great 🙂

Hey, thanks for your thoughtful comment. I do agree that ux help would be beneficial. Here're my initial thoughts; I'm open to discussion.

There are a few known UX patterns we can fall back on, specifically "Confirmation Modal for Destructive Operation", "Bulk Action best practices", and "Actions/affordances in a table like/scrolling list view". (These might not be exact names but give a decent idea of what to search for. Links are below.)

Confirmation modal for destructive operations, best practices:

• Dangerous option should be a different colour and not the default (so default is Cancel, not Delete)
• Dangerous option should explain precisely what's happening ("Are you sure you want to delete the following sources" instead of "Are you sure you want to do this/delete 3 sources?")
• ❌ Super dangerous actions should require more than a button click (eg github typing the project name to confirm deleting the project), but doing this too often frustrates users
• ❌ Consider Progressive Disclosure ("See more ...." / details of action hidden but visible before deletion)
• ❌ Consider "Don't ask me again" to dismiss confirmation dialog

Bulk action best practices:

• (this is the plan- checkbox) Clear affordance (if items have a checkbox, you know they can be checked/selected)
• (this is the plan) Visual indicator of which items are selected
• ❌ Usually, bulk actions are at the top or bottom of the table/row of items they will act on (think about the Gmail bar)

Destructive action best practices:

• Don't put it near a nondestructive or routine action, to avoid the risk of mis-clicks
• Launch a confirmation modal per above
• Don't overuse destructive action confirmations to avoid user fatigue

IMO repeated confirmation dialogs would not be better than the current DeleteSource implementation; the goal of batch operations is to save the user clicks. (This is also why I am advocating for a sidebar and not a dropdown menu for the Batch delete action). I would also worry about encouraging users to click through Destructive dialogs quickly in order to complete a task, and this feature is requested in the context of spam removal, which is a repetitive task. So I personally am in favour of one dialog for a batch operation, which I believe to be in line with batch operations ux practices generally, but I am open to other opinions.

The areas where I do feel we are departing from UX norms and should maybe discuss are:

• Batch action is not at the top of the source list, but is on a side panel. This is open for debate and was mostly me trying to a) ensure destructive/global actions are in a different place than other actions (avoid misclicks) and b) avoid visually crowded ui area (sourcelist). Traditionally, this would go at the top of the sourcelistwidget.
• We don't offer an Undo button and we aren't requiring an unusual action ("type "delete" to proceed"). I think we should not change anything on this front because of the frequency of this operation as an anti-spam measure.
• We aren't doing progressive disclosure (previews of messages). I feel like this would make the feature much more complex, but could be a possible(?) enhancement at some point down the line depending on capacity.
• Another possibility is, in addition to the checkbox, to highlight the entire sourcelist line when the source conversation is selected. Not sure if this is an enhancement, a do-now, or a needs discussion. I'm inclined to implement the simplest version of this feature (checkbox only) for now and revisit when we have more ux support, but open to other opinions.

(Links - based on a lot from nngroup, who are longstanding/practitioners in/creators of the ux field, see eg from them and others:
https://www.nngroup.com/articles/confirmation-dialog/
https://uxpsychology.substack.com/p/how-to-design-better-destructive
https://ux.folio.org/docs/guidelines/ux-patterns/using-the-confirmation-modal/
https://www.nngroup.com/articles/progressive-disclosure/
https://www.nngroup.com/articles/data-tables/ (The section on Batch Actions, although it's brief)
https://www.nngroup.com/articles/closeness-of-actions-and-objects-gui/ (where should the actions go?)

[deeplow]

Thanks a lot for all the reasoning. After reading it, I'm with you that it makes more sense to only make one dialog. But looking at this NNGroup post (point 7. in particular) gave me another idea...

This is mostly a spam mitigation mechanism. With spam, it means that you probably have not even replied. What if in case any source has had replies from a journalist, we highlight that fact. For example:

The following sources have had replies from journalists: source-one, source-two

Or in general any criteria / way to warn users that they may have clicked on the wrong source. This would add to the point of ☑️ Dangerous option should explain precisely what's happening. Or maybe just a table with the sources, number of replies and number of shared files.

What do you think? Maybe I'm complicating this feature too much.

[rocodes]

The following sources have had replies from journalists: source-one, source-two

I think this is a great idea in terms of differentiating what might be spam, and from an implementation perspective, it would not be very complex, but I think we should discuss if it's a must-do at this stage, or an enhancement. It also introduces a new UX condition (so would it be present for all deletion operations including single-source deletion, or just multi-source deletion?).

[legoktm]

I don't have an opinion on the UX question right now, I do think that if we're stuck or uncertain, sticking close to what the Guardian did is fine/what I'd recommend and then working on improvements afterwards.

(And a quick skim of the PR)

[cfm]

On Thu, Sep 05, 2024 at 09:27:31AM -0700, Kunal Mehta wrote: I don't have an opinion on the UX question right now, I do think that if we're stuck or uncertain, sticking close to what the Guardian did is fine/what I'd recommend and then working on improvements afterwards.

I second this.

[deeplow]

The following sources have had replies from journalists: source-one, source-two

I think this is a great idea in terms of differentiating what might be spam, and from an implementation perspective, it would not be very complex, but I think we should discuss if it's a must-do at this stage, or an enhancement. It also introduces a new UX condition (so would it be present for all deletion operations including single-source deletion, or just multi-source deletion?).

@cfm wrote:

On Thu, Sep 05, 2024 at 09:27:31AM -0700, Kunal Mehta wrote:
I don't have an opinion on the UX question right now, I do think that
if we're stuck or uncertain, sticking close to what the Guardian did
is fine/what I'd recommend and then working on improvements
afterwards.
I second this.

Given the opinions, it feels like we should merge this PR right away and discuss improvements afterwards.

[rocodes]

Thanks all for the review + comments. I've fixed the journalist designation display issue (see below) and also added a regression test to catch that test case.

[deeplow]

Thanks for fixing the word wrapping issue. However, I couldn't get the test to work for me.

[rocodes]

Marking again as ready for review.

• I've given up on the regression test for the dialog text cutoff size. However, in all of my manual testing, the text is resized correctly. Refactor DeleteSourceDialog so it accepts Set[Source] #2188 (comment)
• I've addressed other review feedback.

[legoktm]

Thanks, LGTM. I did a full review since deeplow's last review. The comments are just personal preference things in tests and, if desired, can be addressed in follow-ups.

changes addressed

[cfm]

Approving the test-only change in https://github.com/freedomofpress/securedrop-client/compare/b128e48cc04d7daa487b6c5890e38644872852cc..1e541a6694678839c20305ff9084bd3f7e0f7cf6 after the full review in #2188 (review).