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 DownloadJob #427

Merged
merged 3 commits into from
Jun 20, 2019
Merged

Add DownloadJob #427

merged 3 commits into from
Jun 20, 2019

Conversation

sssoleileraaa
Copy link
Contributor

@sssoleileraaa sssoleileraaa commented Jun 15, 2019

Description

Closes #406

This is part of breaking down a large PR #409

This and following smaller refactors are a way to help us avoid bugs around file management when we download and decrypt, which is tricky.

Summary of changes

  • Moved call_api, _download, and _check_file_integrity into a new base class called DownloadJob from FileDownloadJob and MessageDownloadJob. _decrypt will also be moved into DownloadJob from its derived classes in a followup PR.

  • This also introduces new behavior: checking file integrity of everything that gets downloaded. So now message downloads also get checked.

Test Plan

No need to test in Qubes, but always feel free. This PR doesn't modify how we decrypt or download, and the extra file integrity checks is not qubes-specific.

  1. Send messages and files as a source
  2. Refresh on the client to see messages downloaded and decrytped. Click on files to trigger a download and decrypt. When you see "Open" next to the filename you know it's decrypted.
  3. [optional] query the db to make sure statuses look correct

@sssoleileraaa sssoleileraaa changed the title Generalize more Add DownloadJob Jun 15, 2019
@sssoleileraaa sssoleileraaa changed the base branch from master to generalize-decrypt June 15, 2019 00:25
@sssoleileraaa sssoleileraaa force-pushed the generalize-more branch 2 times, most recently from 2c3b7af to 1fef1b4 Compare June 17, 2019 17:48
@redshiftzero
Copy link
Contributor

Confirming:

  1. This closes refactor: MessageDownloadJob and FileDownloadJob should inherit from new class DownloadJob #406 correct? If so let's add to the PR description such that the GitHub autoclose will happen when this PR is merged
  2. I think there should just be one commit here (rebased on latest master after merge of Version 0.0.8 #428 fixing CI). It looks like a merge commit made its way onto this branch

@redshiftzero
Copy link
Contributor

Base branch here should be master no?

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

took a look at this - I wasn't able to download files or messages on these latest changes, see comments inline

securedrop_client/api_jobs/downloads.py Show resolved Hide resolved
securedrop_client/api_jobs/downloads.py Outdated Show resolved Hide resolved
securedrop_client/api_jobs/downloads.py Outdated Show resolved Hide resolved
securedrop_client/api_jobs/downloads.py Outdated Show resolved Hide resolved
@sssoleileraaa
Copy link
Contributor Author

alembic tests are failing - will look into this tomorrow

@sssoleileraaa sssoleileraaa changed the base branch from generalize-decrypt to master June 19, 2019 02:45
@redshiftzero
Copy link
Contributor

heads up that the alembic tests fail here because the removal of constraints from existing tables is not supported in sqlite due to incomplete ALTER table support. However, we can still do these constraint removals via the following steps:

  1. Rename the table my_table to be modified to e.g. my_table_tmp
  2. Create a new table my_table with the constraints applied that we want
  3. Move each row from my_table_tmp into the new table (my_table)
  4. Once all rows are migrated, drop my_table_tmp

You can follow this example here which is from when I added the new journalist UUID column with a unique constraint on the server-side (also uses sqlite)

@sssoleileraaa
Copy link
Contributor Author

thanks @redshiftzero! worked like a charm 🍀

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

just tested this and read through the updated diff, all my comments were addressed, LGTM 🚀

@redshiftzero
Copy link
Contributor

note that we need packaging change freedomofpress/securedrop-builder#59 to land also, let's wait until that is approved before merging this (good to get in this practice as we always want to leave master in a state ready for nightly builds)

@redshiftzero redshiftzero merged commit 49470a7 into master Jun 20, 2019
@redshiftzero redshiftzero deleted the generalize-more branch June 20, 2019 00:58
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.

refactor: MessageDownloadJob and FileDownloadJob should inherit from new class DownloadJob
2 participants