Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add download conversation #1388

Merged

Conversation

gonzalo-bulnes
Copy link
Contributor

@gonzalo-bulnes gonzalo-bulnes commented Jan 3, 2022

🔮 Reviewers! The commits are built individually and are meant to do one thing at a time. I left a couple of quirks when splitting them, that you'll likely notice if you review commit by commit, but I think it's still worth [reviewing commit by commit] if the overall diff feels a bit overwhelming. 🙂

Once we're there, I'll rebase the branch one more time so that it's fully up to date before merging, just let me know.

Description

Closes #1354

Please refer to the issue for context.

Summary:

  • This PR enables a journalist to download all the files of the currently visible conversation at once, via the conversation menu (a.k.a "source menu", a.k.a "kebab menu") or a keyboard shortcut (Ctrl+D).
  • From that single user action, the files are downloaded one by one, the oldest file first. Progress is visible to the journalist in the same way as for individual downloads: files currently being downloaded display a spinning icon, and two "export" and "print" buttons replace that icon once the download is ready.
  • If a conversation has no files or all its files have already been downloaded (whether individually or all at once), the menu action is disabled. (That's also true to the keyboard shortcut.)

This feature is a prerequisite to introducing the possibility of exporting a entire conversation at once.
🍐 @creviera

Implementation Decisions

  1. The state of the files is tracked in Python, in a new structure called state.State.
  2. The state refers to files, source and conversations using identifiers that treat the server-generated identifiers as opaque strings. As far as the client state is concerned, it doesn't matter that files are identified by a UUID, a number, or something else as long as the identifiers are unique.
  3. GUI elements interact with that new state.State structure exclusively: both the controller and the database are abstracted away from the GUI. (Scope: only for this feature. We can propagate the pattern later if we like.)
  4. The GUI subscribes to changes of the state via Qt signals.
  5. A limited number of GUI elements update the state via Qt slots.
  6. Part of the state is persisted to the database so that it remains available across reboots. That state was already being persisted, and this PR doesn't change that (out of scope). That state is loaded when the application starts, and it is currently the only interaction of state.State with the database. (A database.Database object was introduced for clarity of the API and ease of testing.)
  7. The downloading of the files is performed using the same job queue, and the same job types as individual downloads. The only addition is making sure the state gets notice when downloads succeed.
  8. The action is called "All Files" within a "Download" menu section (see Enable a user or Export to "Download All" files to the Workstation for a given source #1354)
Reference diagram

           Sync --updates ---\
                (ongoing)    |
                             v
GUI  --communicates with--> State --abstracts--> Controller ------manages--> Queue of API Jobs --downloads from--> SDK | Server
                             |                        |
                      reads from                 persists state to*
                   (intialization)                    |
                              \---> Database  <------/

(*) State will do it in the future, but that change is out of scope for this PR.

Reference screenshots This is what things should look like. (These were taken in a development environment, the original mockups are in #1354).

Test Plan

Note: Thorough the test plan, please try to activate the "All Files" action menu by clicking on it, but also using the Ctlr+D shortcut.
Note on Qubes OS: Performing the testing in at least one Workstation is necessary, because some of the UI may behave differently on Qubes OS (e.g. the conversation menu will be framed by the label color of the sd-app qube.)

It's a fairly long test plan, here we go!

  • Make sure your server is up to date (see SecureDrop 2.1.0)
  • Start the server, start the client, select a source.
  • Confirm that the conversation and its files are shown as usual.
  • Confirm that the "conversation menu" (a.k.a "source menu", a.k.a "kebab menu") contains a sub-section "Download" and an action "All Files".
  • Confirm that the "All Files" action is enabled for conversations that have at least one file that hasn't been downloaded before.
  • Confirm that the files can be downloaded individually (by activating their "Download" buttons) as usual.
  • Confirm that downloading all the files of a conversation one by one (by activating their "Download" buttons) causes the "All Files" action to be disabled.
  • Confirm that activating the "All Files" menu action causes all the not-already-downloaded files of the conversation to start downloading.
  • Confirm that when downloading all files, the oldest are downloaded first.
  • Confirm that once all files are downloaded, the "All Files" action is disabled.
  • For later, leave some conversation with a few files to download, one file to download, no files to download, mixing those you download individually and those you download all at once.
  • Stop the client.

Take a breath!


  • Start the client.
  • Confirm that the "All Files" action is disabled for conversations that have no files to download, and enabled for all the other. (Independently of the fact that the files were or not downloaded individually or via "All Files").
  • Confirm that everything in the steps above is still true.

Next comes testing the sync.


  • Stop the server (or somehow disconnect the client, this could be done emulating a network issue)
  • Wait until the client displays the banner that indicates that the connection to the server was lost.
  • Upload new files for existing sources. (Ideally upload file to some sources that already have files to download and other that don't have any yet, so that you can confirm later that the action was enabled as expected.)
  • Create new sources, upload files from some sources, and leave other conversations without files
  • Start the server (or somehow fix the network issue)
  • Confirm that the banner is cleared as usual when the client is automatically sync'd
  • Confirm that the new sources are loaded
  • Confirm that the "All Files" action is enabled for the conversations of the new sources with files
  • Confirm that the "All Files" action is disabled for the conversations of the new sources with no files
  • Confirm that the new files are visible for the previously existing sources
  • Confirm that the "All Files" action is enabled for the conversations that were added files
  • Confirm that the "All Files" action is disabled for the conversations that didn't have files and weren't added files

Take a breath again : )


  • Start downloading "All Files"
  • Disconnect the server while the files are downloading (ideally, some were downloaded, others not yet)
  • Confirm that the client behaves as usual with respect of failed download tasks. (@creviera I need your help to detail this step!)
  • Stop the client

Remain some steps specific to using the keyboard shortcut.


  • Start the client, do not select any source.
  • Confirm that pressing Ctlr+D ("the shorcut") does not trigger any download (even when all conversation have files that could be downloaded)
  • Select a source
  • Confirm that pressing "the shortcut" causes all files to be downloaded for the conversation that's visible.
  • Confirm that the "All Files" action in the conversation menu is disabled once all files were downloaded using the shortcut.
  • Select a different source
  • Confirm that the shortcut only causes files to be downloaded for the currently selected conversation

Thanks for sticking through! Last but not least, please let me know of any scenarios you'd test that I didn't think of : ) ⭐
Bonus points for getting the client into an inconsistent state! 🌟

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 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

@@ -3918,6 +3930,26 @@ def __init__(self, source: Source, controller: Controller) -> None:

self.addAction(DeleteConversationAction(self.source, self, self.controller))
self.addAction(DeleteSourceAction(self.source, self, self.controller))
self.addAction(DownloadSelectedConversation(self, state, self.controller))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slight pref for a rename to: DownloadConversationAction, similar to DeleteConversationAction

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went for DownloadConversation (no ...Action suffix) because eventually I'd see the actions defined in gui/conversation/actions.py and used with a prefix:

from securedrop_client/gui/conversation import actions

#     prefix --v                          v-- suffix would be redundant in the 'actions' module
self.AddAction(actions.DownloadConversation(self, state, self.controller))

@gonzalo-bulnes gonzalo-bulnes force-pushed the add-download-conversation branch from d6bba42 to eacd78e Compare January 4, 2022 03:19
@gonzalo-bulnes
Copy link
Contributor Author

(rebase with no changes)

@gonzalo-bulnes gonzalo-bulnes force-pushed the add-download-conversation branch from eacd78e to d0468af Compare January 4, 2022 03:47
@eloquence
Copy link
Member

For the sprint starting 1/5, we will aim to get this PR fully to "Ready for review", including front-end changes and the test plan.

@gonzalo-bulnes gonzalo-bulnes force-pushed the add-download-conversation branch 3 times, most recently from 4a24b98 to ff26391 Compare January 10, 2022 20:50
@gonzalo-bulnes
Copy link
Contributor Author

(rebase)

@gonzalo-bulnes gonzalo-bulnes force-pushed the add-download-conversation branch 2 times, most recently from d0bcb6e to c35c68f Compare January 11, 2022 00:51
@gonzalo-bulnes gonzalo-bulnes force-pushed the add-download-conversation branch 5 times, most recently from c17f37d to 33b094a Compare January 18, 2022 04:39
@gonzalo-bulnes
Copy link
Contributor Author

Consolidated commits, added tests. The last four commits are still very draft-y.

@gonzalo-bulnes gonzalo-bulnes force-pushed the add-download-conversation branch 2 times, most recently from bfa9f9c to 8f1da1d Compare January 25, 2022 10:41
@sssoleileraaa
Copy link
Contributor

sssoleileraaa commented Mar 15, 2022

Hi @creviera ! I see there are a couple of conflicts with main, let me know if you want me to rebase the PR : )

Hello! Looks like a botched rebase, so I'll fix it up and finish the approval.

@gonzalo-bulnes
Copy link
Contributor Author

👍 (I've got a local backup too, if needed.)

@sssoleileraaa
Copy link
Contributor

@gonzalo-bulnes 💯 if you're able to work today, please feel free to fix this up and ping me when finished (sorry about the extra work)

@gonzalo-bulnes
Copy link
Contributor Author

gonzalo-bulnes commented Mar 15, 2022

👌 @creviera! No worries!

Except for user actions (QAction), the GUI widgets should not
need more than read-only access to the application state.

The user actions should become the interface for the GUI to
modify the application state.

On properties, see https://realpython.com/python-property
This action will eventually be moved out of widgets.py
into an actions.py module, that's why its name doesn't
contain the "*Action" suffix.
Add file_download_started signal to Controller
The action should be disabled when there are no files to download.
...from the persitance layer when the application is started.

Eventually, the GUI could depend exclusively on state.State,
and a method could be added to persist it to the database.

That would remove the necessity to setup a database in order to
use the GUI. When present, such a database would allow state to
persist across restarts, which is exactly its role.

In particular, the database would not be responsible for transporting
information between different parts of the GUI. That repsonsibility
would be that of state.State, which can have whatever internal structure
is best suited to the task, without any constraint related to database
data layout.

This can be done progressively, by expanding the Database collection
of methods as convenient.
If information about the conversation files is available,
the action should be enabled on disabled accordingly.
Ensure that the menu sub-section titles are closer to the sub-section
items than they are the last item is to the next sub-section title.

Ensure that the action items are clickable from one edge of the menu
to the other (like standard menu items).

Increase the menu items size (area).

See https://lawsofux.com/law-of-proximity
and https://lawsofux.com/fittss-law

🍐 @ninavizz
@gonzalo-bulnes gonzalo-bulnes force-pushed the add-download-conversation branch from 06a3102 to f5f11b4 Compare March 15, 2022 20:01
@gonzalo-bulnes
Copy link
Contributor Author

gonzalo-bulnes commented Mar 15, 2022

Rebased @creviera - I had a few core dumps from xvfb when running make check locally, I believe it's likely to be local, but if you can run it too I think it doesn't hurt to double check.

For info, the conflicts were:

  • some tests were removed that I had touched to add a parameter to the Controller constructor
  • one method signature changed that is used in one test (ConversationView.on_reply_sent)

@gonzalo-bulnes gonzalo-bulnes requested review from sssoleileraaa and removed request for ninavizz March 15, 2022 20:17
Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran through the test plan again, and this still looks good. I'm going to create a followup issue for something I'd rather be worked on outside of this PR (since the PR is already pretty large) and it's a minor issue around needing to disable the "Download All" option if all conversation items have been deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable a user or Export to "Download All" files to the Workstation for a given source
4 participants