Support deletion of multiple sources

[rocodes]

Status

Ready for review and testing; test suite is passing but new tests have not yet been added.

Description

Ready for review.

• Add support for selecting multiple sources in the SourceList
• Add pyqtSignal/Slot listener/requester so the top bar (actions/toolbar) can request updates on sourcelist selection status
• Add another view in EmptyConversationView that instructs the user that multiple sources are selected. This is to avoid ambiguity where a single source conversation is previewed, but multiple sources are selected in the sourcelist.
• "Naive"/straightforward multi delete implementation that sequentially sends DeleteSourceJob (this approach for now, pending server changes that allow for true batch delete will be an easy addition)

Closes #2160. Closes #1569.

Test Plan

• Visual review
• CI
• test scenarios below
• additional tests written for race condition regression testing

Test against a dev server with NUM_SOURCES=50 make dev (or some other large number.
I suggest adding replies to various sources once logged in that indicate which source conversation is expected, for example, the source nickname in a reply. (I'm realizing this is also a feature improvement to make dev that we can add, will track that separately).
Basic testing

• Ctrl+click selects multiple disjointed sources
• Shift+click selects range of sources
• Mouse drag on sourcelist selects range of sources
• When more than one source is selected, the "Multiple sources selected" context view replaces the Source Conversation view
• When sources are deselected, the correct conversation view appears
• Selecting Delete Sources from the toolbar prompts a dialog that shows each source name confirming deletion
• Closing or canceling the dialog has no effect and sources are still selected
• Accepting the dialog shows individualized deletion animation for each target source
• DeleteSources button is not visible when logged out. (Note than in addition there is a logical check that requires an authenticated session)
• Source Menu (3-dots menu by a single source) functionality is unchanged, including deleting a single source from the source menu
• Basic usage testing: ensure that successive clicks on different sources in the sourcelist render the correct expanded conversation in the conversation pane (right side). (Easiest way to do this is to reply with the sources nickname so that you can readily match the conversation to the source)
• Toggling between views (nothing selected, multi selected, single source selected) renders correct view

** Race condition testing**

• Tester (@cfm) can use some discretion here, but the goal is to ensure that when the sourcelist is reordered, the correct sources remain selected. One way to test this would be to select (programatically or manually) a number of sources while the client is loading lots of dev data, or to select a number of sources, and before any action has been taken, reorder the sourcelist and ensure the correct items remain selected.
• The best asessment though is via review, noting that when the Sources are identifiedis at the time of the mouseclick, not at the time the Delete Sources button is called, and the selection is based on a QListWidget item which is tied to a Source object.

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]

(When/if we add more options, this text will change. I'm not attached to it)

[rocodes]

Here are some screenshots of what I've implemented. If anyone wants to check out this branch and run it locally, you can do that too. I will have some followup comments as well but just sharing what it looks like at the moment.

Logged in, no source selected, delete button visible:

Select some sources

Tooltip/hover button state

Confirm deletion

Deletion in progress (note animation for individual sources; the animation is why some of these look odd in the screenshot)

Some other notes:

• the delete sources button is hidden when the user is logged out. The top bar toolbar pane is not hidden, because offline keyword search could still be a thing.
• I made use of the list widget selector property instead of adding checkboxes. Not sure how folks feel about it, I know that holding Control or Shift and then clicking isn't the most convenient gesture, and @cfm already said it might not be his preference. But I think it's going to be less visually confusing in the long run and it's less code complexity (leveraging built-in qt signal about list item selection), so I would like to know if anyone is strongly opposed to this approach, at least for now.
• I stuck to one DeleteSourceJob per deletion (for now, until we implement server-side batch delete) instead of refactoring DeleteSourceJob to take a set of sources and dispatch them sequentially; this is because it would have meant slightly more code changes to have individualized delete animations for each source. Not a big thing, could change it, just taking the approach of changing the fewest things possible.

[rocodes]

Updates:

I will write a longer comment explaining the latest changes (QStackedLayout decision), which I previewed to @cfm already.
Unit tests are passing; functional tests I am hitting #1880 (comment) consistently, but have made some progress. (The integration tests are failing but it's an easy fix and I'll pick it up this evening).

[rocodes]

Status:

• ready for code review and manual testing
• additional functional tests targeting the reordering of the sourcelist while items are selected, or any other corner cases, would be helpful (handoff on that was discussed last week)

More context/info about layout elements:

The QStackedLayout element that I was mentioning in the previous comment is an additional change that was not in the original work plan, here is a summary of the changes and why they were needed.

Originally, there was one view, the EmptyConversationView, that was a vertical layout that contained multiple child layout items that would represent contextual states (No sources available, No source selected) and would be shown depending on the user and sourcelist behaviour. The way to switch between views following this approach was to show and hide individual layout elements, which gets messy, can make for undefined ui states (if you forget to show/hide an element), and is less well-encapsulated for testing. However, there was a further issue, which is that the previous expectation was that the ConversationView would not be regularly shown and hidden (once you select a source, your option is to keep selecting other sources and rendering their conversations in the righthand pane, or, to delete the source, rendering the "no sources selected" view).

Originally I added a third state (multiple sources selected) to this layout, but I realized that showing and hiding the Conversation pane element was wiping out the layout widgets that were hidden and redrawing each time the user switched between selecting multiple sources and a single source.

A better approach than showing and hiding individual gui components is using separate layouts in QStackedWidget. This is like a tab view, where the context can 'flip' between Empty, Nothing Selected, Multiple Selected, and Single Conversation Rendered (and anything else we want on that stack), and can be thought of as an array of layouts, one of which can be seen at any given time. This is what is now implemented.

Another benefit of this approach is that we could deprecate main_view.source_conversations and just push SourceConversationWrappers onto the end of the QStackedWidget. This would also allow us to set a max number of widgets we want to cache, since right now, main_view.source_conversations is not size-constrained, and if a newsroom has 1000 sources and clicks each one sequentially, the app will try to hold a dict of 1000 {source.uuid: SourceConversationWrapper}s, and that's a big expensive dictionary that will probably crash the client. 🙃

(What would be needed for that approach is a clean/efficient way of searching through SourceConversationWrappers to see if the right one is present on the stack, and displaying it if so. It might be possible to do this in a more performant way with setting a custom object property in the QWidget that is the source uuid - I'm not sure, haven't looked into it. But either way I think it would be an improvement to the strategy we use now.)

[cfm]

@rocodes, I did an initial review today, which I'm posting WIP to maximize our back-and-forth this week. I'll both update this and comment further as I chip away at this checklist tomorrow and Friday. It's looking really good so far.

• Visual review: done except for:
  • securedrop_client.gui.widgets
  • tests/
• CI
• test scenarios below
• additional tests written for race condition regression testing

Basic testing

• See screenshot: text cropped (especially characters truncated vertically) in narrow window

Though this also happens on the Nothing to see yet screen, so it may not be anything you've introduced here! Feel free to file a separate ticket for this if you feel it's out of scope.

• Ctrl+click selects multiple disjointed sources
• Shift+click selects range of sources
• Mouse drag on sourcelist selects range of sources

Yes, although I am weakly opposed to introducing this UX pattern, which feels like a non-native touch gesture.

• When more than one source is selected, the "Multiple sources selected" context view replaces the Source Conversation view
• When all but one of the selected sources are is deselected, the correct conversation view appears
• When another (non-selected) source is selected, the correct conversation view appears
• Selecting Delete Sources from the toolbar prompts a dialog that shows each source name confirming deletion

Yes, but I've just realized that the use of a set means they're not presented in either presentation order (from top to bottom) or selection order (the order in which I clicked them), which makes it hard to compare "what I think I selected" with what the dialog lists. I think this can be a follow-up improvement, but I wanted to flag it.

• Closing or canceling the dialog has no effect and sources are still selected
• Accepting the dialog shows individualized deletion animation for each target source
• Clicking into a source pending deletion shows the deleted-source view for that that source
• DeleteSources button is not visible when logged out. (Note than in addition there is a logical check that requires an authenticated session)
• Delete Sources button does not display hover state when disabled because nothing is selected
• Source Menu (3-dots menu by a single source) functionality is unchanged, including deleting a single source from the source menu
• Basic usage testing: ensure that successive clicks on different sources in the sourcelist render the correct expanded conversation in the conversation pane (right side). (Easiest way to do this is to reply with the sources nickname so that you can readily match the conversation to the source)
• Toggling between views (nothing selected, multi selected, single source selected) renders correct view

There's no way to go directly back to "nothing selected", is there?

Race condition testing

• Tester (@cfm) can use some discretion here, but the goal is to ensure that when the sourcelist is reordered, the correct sources remain selected. One way to test this would be to select (programatically or manually) a number of sources while the client is loading lots of dev data, or to select a number of sources, and before any action has been taken, reorder the sourcelist and ensure the correct items remain selected.
• The best asessment though is via review, noting that when the Sources are identifiedis at the time of the mouseclick, not at the time the Delete Sources button is called, and the selection is based on a QListWidget item which is tied to a Source object.

[cfm]

Since this is a read-only getter with no side effects or dependencies, why not simplify it to:

Suggested change

	def get_selected_sources(self) -> set[db.Source] | None:
	return self._selected_sources
	@property
	def selected_sources(self) -> set[db.Source]:
	return self._selected_sources

[rocodes]

Depends on below - pending your feedback I can either adjust to instantiate with the empty set, and always return a set, or leave as is.

[cfm]

#2252 (comment) resolves the typing question. My other suggestion here was to go from a method called as .get_selected_sources() to a @property called as .selected_sources, but feel free to disregard that too if you feel it's too out of pattern with the rest of the current codebase.

[rocodes]

@cfm : Thank you for your testing, it's been very helpful and a few small changes coming presently. One question: What desktop environment /window manager /display server are you using for testing? Re the narrow window/cut off text, the client sets a minimum screen width and does not permit resizing below that width, and that's not a regression. (It's not a great solution, we should reflow/resize fluidly and support more screen widths, but that's not introduced here.) In my testing, I'm prevented from decreasing the width of the application below the mininum width we have specified. If you're on i3 we should definitely file a separate issue since there will be a significant number of changes to make to accommodate that. But that said, if you notice a difference in behaviour from our last tagged release, please let me know.

Other comments:

• I was wondering about set vs list and the ordering question too. I could: a) sort the source nicknames alphabetically in the dialog; b) switch from set to list to display them ordered by row position in the sourcelist. In neither case will they be sorted by "order clicked on" unless I take extra steps. (Which I could do but didn't think it was necessarily worth it). Do you have a preference?
• Testing in a non-Qubes environment, when I'm in offline mode I notice the DeleteSources button is still visible. I'm going to investigate this.

[rocodes]

Another update re cut-off text: Since the last release, the conversation view area has shrank a bit due to the top toolbar addition, so on smaller screen sizes the text can get cut off, as below. This isn't what you were reporting @cfm but it did cause me to find this, so thank you - will tack on a fix.

[cfm]

@rocodes in #2252 (comment):

@cfm : Thank you for your testing, it's been very helpful and a few small changes coming presently. One question: What desktop environment /window manager /display server are you using for testing? Re the narrow window/cut off text, the client sets a minimum screen width and does not permit resizing below that width, and that's not a regression. (It's not a great solution, we should reflow/resize fluidly and support more screen widths, but that's not introduced here.) In my testing, I'm prevented from decreasing the width of the application below the mininum width we have specified. If you're on i3 we should definitely file a separate issue since there will be a significant number of changes to make to accommodate that. But that said, if you notice a difference in behaviour from our last tagged release, please let me know.

Good call: I'm doing development testing in i3. Disregard these observations, and if we do notice them in release QA in Xfce we'll know it's a regression.

[cfm]

@rocodes in #2252 (comment):

I was wondering about set vs list and the ordering question too. I could: a) sort the source nicknames alphabetically in the dialog; b) switch from set to list to display them ordered by row position in the sourcelist. In neither case will they be sorted by "order clicked on" unless I take extra steps. (Which I could do but didn't think it was necessarily worth it). Do you have a preference?

My triaging instinct:

1. If (b) isn't much more of a refactoring than s/set/list/g, and the update signal is guaranteed to not give duplicates, then let's do that as part of this pull request.
2. Otherwise I think (a) would be just fine, either now or as a nice presentation-layer follow-up enhancement.

My assumption here is: either source-list ordering or lexical ordering will be obvious and intuitive to most folks; selection ordering will be neither obvious nor intuitive nor worth it to implement. :-)

[rocodes]

Yup, set -> list refactor coming presently-- I was originally thinking it would be a feed of selection data (and therefore a set was more important), but since we're using the built-in selectedItems, I feel fine about using a list

[cfm]

@rocodes's guidance was for testing races was:

• Tester (@cfm) can use some discretion here, but the goal is to ensure that when the sourcelist is reordered, the correct sources remain selected. One way to test this would be to select (programatically or manually) a number of sources while the client is loading lots of dev data, or to select a number of sources, and before any action has been taken, reorder the sourcelist and ensure the correct items remain selected.
• The best asessment though is via review, noting that when the Sources are identifiedis at the time of the mouseclick, not at the time the Delete Sources button is called, and the selection is based on a QListWidget item which is tied to a Source object.

I came up with three scenarios. Scenarios (2) and (3) should be automatable entirely within the Client test suite, which I'll do tomorrow. Scenario (1) is not automatable without a way to trigger server-side actions (even just loaddata.py) from the Client test suite, but I'll see how much of it I can simulate with Client-side operations.

Scenario 1: Addition

1. Select one or more sources to delete.
2. Add one or more new sources (e.g., via loaddata.py) and wait for the source list to sync.
3. The selected sources remain selected.
4. Click Delete Sources.
5. The list of sources is the set of sources selected in step (1).
6. Exactly this list of sources is deleted.

Scenario 2: Deletion, pre-dialog

1. Select one or more sources to delete.
2. Delete one or more of the selected sources out of band (e.g., via the Journalist Interface) and wait for the source list to sync.
3. The selected sources remain selected.
4. Click Delete Sources.
5. The list of sources is the set of sources selected in step (1).
6. Exactly this list of sources is deleted.

Scenario 3: Deletion, in-dialog

1. Select one or more sources to delete.
2. Click Delete Sources.
3. Delete one or more of the selected sources out of band (e.g., via the Journalist Interface) and wait for the source list to sync.
4. The selected sources remain selected, except for the deleted sources, which disappear from the source list.
5. Click Yes, Delete n Source Accounts. Yes, the list of sources will be out of date, but...
6. The remaining sources are deleted.

(I think this level of resilience is adequate, and trying to add more responsiveness might actually reduce resilience [i.e., introduce new data races].)

However, the Client crashes when it attempts to create a DeleteSourceJob for one of the deleted sources.

2024-10-24 16:28:49,778 - securedrop_client.queue:164(add_job) DEBUG: Added <securedrop_client.api_jobs.sources.DeleteSourceJob object at 0x76faadbfb370> to queue
2024-10-24 16:28:49,787 - securedrop_client.sdk:330(_send_json_request) DEBUG: Sending request to proxy: DELETE api/v1/sources/c70eea2a-1d54-4df3-8d4b-c76c3358020d (body=False, stream=False, timeout=60)
2024-10-24 16:28:49,802 - securedrop_client.queue:164(add_job) DEBUG: Added <securedrop_client.api_jobs.sources.DeleteSourceJob object at 0x76faadbfb130> to queue
2024-10-24 16:28:49,805 - securedrop_client.queue:164(add_job) DEBUG: Added <securedrop_client.api_jobs.sources.DeleteSourceJob object at 0x76faadbfb2e0> to queue
Traceback (most recent call last):
  File "/home/user/securedrop-client/client/securedrop_client/gui/widgets.py", line 558, in <lambda>
    dialog.accepted.connect(lambda: self.controller.delete_sources(targets))
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/securedrop-client/client/securedrop_client/logic.py", line 80, in decorated_function
    return f(self, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/securedrop-client/client/securedrop_client/logic.py", line 1038, in delete_sources
    job = DeleteSourceJob(source.uuid)
                          ^^^^^^^^^^^
  File "/home/user/.cache/pypoetry/virtualenvs/securedrop-client-KGGbhkP--py3.11/lib/python3.11/site-packages/sqlalchemy/orm/attributes.py", line 276, in __get__
2024-10-24 16:28:49,807 - root:66(excepthook) ERROR: Unrecoverable error
Traceback (most recent call last):
  File "/home/user/securedrop-client/client/securedrop_client/gui/widgets.py", line 558, in <lambda>
    dialog.accepted.connect(lambda: self.controller.delete_sources(targets))
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/securedrop-client/client/securedrop_client/logic.py", line 80, in decorated_function
    return f(self, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/securedrop-client/client/securedrop_client/logic.py", line 1038, in delete_sources
    job = DeleteSourceJob(source.uuid)
                          ^^^^^^^^^^^
  File "/home/user/.cache/pypoetry/virtualenvs/securedrop-client-KGGbhkP--py3.11/lib/python3.11/site-packages/sqlalchemy/orm/attributes.py", line 276, in __get__
    return self.impl.get(instance_state(instance), dict_)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.cache/pypoetry/virtualenvs/securedrop-client-KGGbhkP--py3.11/lib/python3.11/site-packages/sqlalchemy/orm/attributes.py", line 677, in get
    value = state._load_expired(state, passive)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.cache/pypoetry/virtualenvs/securedrop-client-KGGbhkP--py3.11/lib/python3.11/site-packages/sqlalchemy/orm/state.py", line 660, in _load_expired
    self.manager.deferred_scalar_loader(self, toload)
  File "/home/user/.cache/pypoetry/virtualenvs/securedrop-client-KGGbhkP--py3.11/lib/python3.11/site-packages/sqlalchemy/orm/loading.py", line 985, in load_scalar_attributes
    raise orm_exc.ObjectDeletedError(state)
sqlalchemy.orm.exc.ObjectDeletedError: Instance '<Source at 0x76faadbb1950>' has been deleted, or its row is otherwise not present.
    return self.impl.get(instance_state(instance), dict_)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.cache/pypoetry/virtualenvs/securedrop-client-KGGbhkP--py3.11/lib/python3.11/site-packages/sqlalchemy/orm/attributes.py", line 677, in get
    value = state._load_expired(state, passive)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.cache/pypoetry/virtualenvs/securedrop-client-KGGbhkP--py3.11/lib/python3.11/site-packages/sqlalchemy/orm/state.py", line 660, in _load_expired
    self.manager.deferred_scalar_loader(self, toload)
  File "/home/user/.cache/pypoetry/virtualenvs/securedrop-client-KGGbhkP--py3.11/lib/python3.11/site-packages/sqlalchemy/orm/loading.py", line 985, in load_scalar_attributes
    raise orm_exc.ObjectDeletedError(state)
sqlalchemy.orm.exc.ObjectDeletedError: Instance '<Source at 0x76faadbb1950>' has been deleted, or its row is otherwise not present.

What it should do in its iterating, and what the bulk DELETE endpoint in freedomofpress/securedrop#7228 will support in its batching, is silently discard a DeleteSourceJob for a Source (i.e., UUID) that doesn't exist, but probably logger.warning() on it.

[rocodes]

Hey @cfm, thank you and good find. I have pushed the most limited-diff fix, which is to try-catch accessing Source properties source.uuid in Controller.delete_sources, fixed the empty-list case for DeleteSourceDialog, and added comments to address your other questions.

(This for our future selves): I started down, then paused, the path of a more involved fix for the race condition issue, which I will describe now: avoid passing db models to GUI (and API) components, and instead pass a simplified ViewModel object (in this case for a Source) that is constructed on demand and is not dependent on the underlying db session. (The DeleteSourcesDialog doesn't need all Source attributes, only the journalist designator; the Controller / API don't need all source attributes, only the uuid; etc). This way, the business of whether or not the db object is valid is not part of the gui or api responsibility. But I think that's a larger-scoped idea so I am leaving it out for now.

I have rebased, pushed changes that I hope will fix the new FileWidget functional test, addressed changes, and squashed and signed commits except for the latest.

[cfm]

Thanks for the quick fixes, @rocodes. Your 4e20eee resolves the crash in the "deletion, in-dialog" scenario, which now succeeds with the expected warning:

2024-10-25 12:25:16,498 - securedrop_client.logic:1071(delete_sources) WARNING: DeleteSourceJob requested but source already deleted

I've amended that as c267ce7 to fix up test_deleted_file_filewidget.

Here's how I'd like to proceed: Because you've resolved this race (thanks!), and because testing for races thoroughly exercises this feature (and isn't completely automatable), I'm going to approve this now but not merge it. Instead, I'll push whatever tests I'm able to write this afternoon, so that next week the team can review just the tests and re-approve for merge on that basis.

[cfm]

f6bd2c9 rewrites test_delete_sources() to be parameterized for deletion of both a single source and multiple (currently three) sources. The details (in the commit message) are worth reviewing, because this test introduces one implementation-level change and one workaround (for #2273). Otherwise I'm happy for review of f6bd2c9 to stack on my last approval in #2252 (review), and this should be ready to go.

[deeplow]

I have taken a look at the missing f6bd2c9 and it looks reasonable to me. The test was really clear I especially appreciate the fact that we're confirming that we're now checking that the source which gets deleted does it fact (at least visually) match the one we want.

Here's how I'd like to proceed: Because you've resolved this race (thanks!), and because testing for races thoroughly exercises this feature (and isn't completely automatable), I'm going to approve this now but not merge it. Instead, I'll push whatever tests I'm able to write this afternoon, so that next week the team can review just the tests and re-approve for merge on that basis.

^{Originally posted by cfm October 25, 2024}

Based on the above, I'm approving it generally, piggybacking pn @cfm's approval.

[deeplow]

because this test introduces one implementation-level change and one workaround (for #2273).

As a side-note on this, I was able to reproduce the issue with exec() and your solution fixes it for me as well. But I did notice that dialog.exec() is also used in _on_print_clicked(). It could be that we'll be bitten by this in the future as well, but since tests are passing, I'll assume for the time being we can leave it as-is.