Download unread/ all selected sources #1467

psivesely · 2016-11-07T00:31:51Z

Adds buttons/ functionality for downloading either all submissions or just unread submissions from one or more selected sources.
Refactors download backend download functionality.
Chooses better icons for buttons.
Implements unit tests for this new functionality.

Since the git history is messy on this one, but I still want all 3 of us to get credit, I'm going to rebase this PR into 3 commits: one from each of us.

ajvb

Left one small comment, but besides that, this looks great!

ajvb · 2016-11-07T03:14:25Z

securedrop/tests/test_unit_journalist.py


 # Set environment variable so config.py uses a test environment
 os.environ['SECUREDROP_ENV'] = 'test'
+import config


Where is config being called? Or is this simply for making sure the flask settings are set?

Currently, the securedrop/config.py modules reads the environment variable SECUREDROP_ENV, and sets the configuration differently based on its value. We'd like to deprecate the reading/ setting of environment variables in our code entirely, and instead be read from static INI files.

Although I'm still considering how to do this, I believe that we need to create a separate module for each INI file, for each environment setting. This will involve breaking up securedrop/manage.py. Each new module will read only a single INI file. All functionality it provides will involve calls to generators of other modules with initialization arguments, so that no other modules besides these are actually reading from a configuration file.

Currently, the securedrop/config.py modules reads the environment variable SECUREDROP_ENV, and sets the configuration differently based on its value.

Ok awesome. Could I suggest leaving a comment explaining that its needed even though it is not directly called?

We'd like to deprecate the reading/ setting of environment variables in our code entirely, and instead be read from static INI files.

👍 makes complete sense. Only thing to consider is that environment variables are much easier to work with when it comes to Docker / containers than INI files in my experience.

Ah, I see now. We don't actually access any of the attributes in config directly in this module. I have to refresh my memory on what happens if we import some other modules which import config before setting this environment variable. I think that could be bad, but need to confirm.

psivesely · 2016-11-07T23:35:03Z

Want this one to get at least one more review @redshiftzero @garrettr

psivesely · 2016-11-08T05:28:13Z

Note: blocking on #1440.

redshiftzero

Looks good! Just rebase on current develop and then this should be ready to merge.

redshiftzero · 2016-11-09T01:28:05Z

securedrop/journalist_templates/col.html

@@ -85,7 +85,7 @@
    <input name="csrf_token" type="hidden" value="{{ csrf_token() }}"/>
    <input type="hidden" name="sid" value="{{ sid }}"/>
    <input type="hidden" name="col_name" value="{{ source.journalist_designation }}"/>
-    <button type="submit" class="danger"><i class="fa fa-times"></i> Delete collection</button>
+    <button type="submit" class="danger"><i class="fa fa-trash-o"></i> Delete Collection</button>


Good call on changing these icons!

redshiftzero · 2016-11-09T01:35:25Z

securedrop/tests/test_unit_journalist.py

        sid = 'EQZGCJBRGISGOTC2NZVWG6LILJBHEV3CINNEWSCLLFTUWZJPKJFECLS2NZ4G4U3QOZCFKTTPNZMVIWDCJBBHMUDBGFHXCQ3R'
        source = Source(sid, crypto_util.display_id())
        db_session.add(source)
        db_session.commit()
-        files = ['1-abc1-msg.gpg', '2-abc2-msg.gpg']
-        filenames = common.setup_test_docs(sid, files)
+        files = ['1-abc1-msg.gpg', '2-abc2-msg.gpg', '3-abc3-msg.gpg', '4-abc4-msg.gpg']


You can adapt add_source_and_submissions() in #1440 (once it's merged in) here

psivesely · 2016-11-14T06:29:39Z

Hmm, all tests were passing on my machine. I'll have to retry with a clean branch and VM.

Button type 1. Address comments for Button 1. Button 2. Might be buggy. Extend and improve bulk download unit test This improves the bulk download unit test by having it check that only selected files are downloaded and not unselected files. It extends the unit test to include a test for downloading all present files. Add unit test for download_all_unread

Clean up action checking and get tests passing Extend test coverage to all new code in download all PR Style cleanup in test_unit_journalist.py Fixes based on code review Fix test for removal of g.user.username from all_unread

psivesely · 2016-11-14T17:42:27Z

Okay, still everything passes locally. I rebased and pushed again--we'll see if it passes on Travis this time.

psivesely · 2016-11-14T18:35:11Z

@redshiftzero Can you pull this one down and test locally? I'm not sure why Travis is running into sqlite3.OperationalErrors. Thoughts?

redshiftzero · 2016-11-14T19:30:49Z

Hmm - all tests are passing locally for me as well...

redshiftzero · 2016-11-14T20:08:53Z

Also: I see you added a commit with a major refactor of the test suite - can we maybe put this refactor in a separate PR? Else the merging of this PR will be delayed as those changes definitely warrants a careful re-review... up to you

psivesely · 2016-11-14T20:19:20Z

@redshiftzero Problem is I'd have to rewrite the tests for download functionalities with the current test structures, which are now soon (hopefully) going to be outdated anyway. Alternatively, if you think this is an okay idea, we could also make an exception here and merge this without tests, since I've already written two passing* sets of tests for the functionality that you've been able to confirm--the ones I wrote pre-#1440 and the ones introduced in 0307265.

The latter only seems to be passing locally.

I really don't want to write a 3rd set of tests for this functionality, that is just going to be replaced days later by my 2nd set.

psivesely · 2016-11-14T20:19:39Z

Maybe let's figure out Travis first?

redshiftzero · 2016-11-14T20:33:56Z

Oh sorry - I wasn't suggesting writing of a third set of tests, instead just moving the last commit into a separate PR, rebase this one on develop as is since the first three commits are already reviewed and approved. Then we merge this one in (assuming we get a passing Travis build on the first three commits - which at least was the case when I reviewed this last week).

psivesely · 2016-11-14T22:16:56Z

I think rebasing will require me to rewrite tests because #1440 changed some things.

psivesely · 2016-11-14T22:22:27Z

Ah, nvm.

psivesely · 2016-11-14T22:29:03Z

Okay, that worked. My mistake. Split into two PRs.

redshiftzero · 2016-11-14T22:40:55Z

securedrop/tests/test_unit_journalist.py

-import datetime
-
-from flask_testing import TestCase
-from flask import url_for, escape

 # Set environment variable so config.py uses a test environment
 os.environ['SECUREDROP_ENV'] = 'test'


Need to move os import above this line

I swear I ran this locally after git reset HEAD~1 --hard, but before pushing, and it succeeded. Probably just stepped in to a parallel universe where that happened, or ran on another branch by accident.

Introduce download selection btns, better icons Improve document interface download functionality * User can now download all or just unread messages from a selection of sources. * Backend cleanup. * Small CSS bug introduced in last commit fixed. Minor fixes--fn to download single submission Add unit tests for new download functionality Remove unused g import

psivesely · 2016-11-15T02:34:55Z

@redshiftzero Ready for re-review.

psivesely · 2016-11-15T19:19:20Z

Okay, I've gotten 3 approved reviews, and all 3 checks have passed. Going to merge this one myself in this case because I don't want to block #1460 any longer, which has already been open for 10 days.

psivesely force-pushed the download-selections branch from 5f522a8 to df7a73d Compare November 7, 2016 00:37

redshiftzero added the hackathon label Nov 7, 2016

ajvb approved these changes Nov 7, 2016

View reviewed changes

ajvb mentioned this pull request Nov 7, 2016

Download All submissions (based on #1181) #1456

Closed

psivesely mentioned this pull request Nov 7, 2016

Create a more usable folder structure for downloads #1460

Closed

psivesely added the PR: Needs review label Nov 7, 2016

redshiftzero self-assigned this Nov 8, 2016

redshiftzero approved these changes Nov 9, 2016

View reviewed changes

redshiftzero added PR: Pending additional work and removed PR: Needs review labels Nov 9, 2016

redshiftzero assigned psivesely and unassigned redshiftzero Nov 9, 2016

psivesely force-pushed the download-selections branch from df7a73d to 0385b82 Compare November 13, 2016 09:07

pwplus and others added 2 commits November 14, 2016 08:35

Additional work on download all functionality

a896333

Clean up action checking and get tests passing Extend test coverage to all new code in download all PR Style cleanup in test_unit_journalist.py Fixes based on code review Fix test for removal of g.user.username from all_unread

psivesely force-pushed the download-selections branch from 0385b82 to cb1a028 Compare November 14, 2016 17:41

psivesely assigned redshiftzero and unassigned psivesely Nov 14, 2016

psivesely force-pushed the download-selections branch 2 times, most recently from 1f11a35 to 0307265 Compare November 14, 2016 18:15

psivesely mentioned this pull request Nov 14, 2016

Refactor tests/common.py #1474

Merged

psivesely force-pushed the download-selections branch from 0307265 to c0cf7da Compare November 14, 2016 22:28

redshiftzero reviewed Nov 14, 2016

View reviewed changes

redshiftzero assigned psivesely and unassigned redshiftzero Nov 14, 2016

psivesely force-pushed the download-selections branch from c0cf7da to 6aec0c0 Compare November 15, 2016 02:15

psivesely force-pushed the download-selections branch from 6aec0c0 to ac02336 Compare November 15, 2016 02:29

psivesely added PR: Needs review and removed PR: Pending additional work labels Nov 15, 2016

ajvb approved these changes Nov 15, 2016

View reviewed changes

Merge branch 'develop' into download-selections

6f8fd38

psivesely merged commit 38fb29c into develop Nov 15, 2016

psivesely deleted the download-selections branch November 15, 2016 19:19

redshiftzero mentioned this pull request Nov 23, 2016

Download submissions from the Sources view #1354

Closed

conorsch mentioned this pull request Jul 11, 2017

Download all submissions #1171

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download unread/ all selected sources #1467

Download unread/ all selected sources #1467

psivesely commented Nov 7, 2016

ajvb left a comment

ajvb Nov 7, 2016

psivesely Nov 7, 2016

ajvb Nov 7, 2016

psivesely Nov 8, 2016

psivesely commented Nov 7, 2016

psivesely commented Nov 8, 2016

redshiftzero left a comment

redshiftzero Nov 9, 2016

redshiftzero Nov 9, 2016

psivesely commented Nov 14, 2016

psivesely commented Nov 14, 2016

psivesely commented Nov 14, 2016

redshiftzero commented Nov 14, 2016

redshiftzero commented Nov 14, 2016

psivesely commented Nov 14, 2016

psivesely commented Nov 14, 2016

redshiftzero commented Nov 14, 2016

psivesely commented Nov 14, 2016

psivesely commented Nov 14, 2016

psivesely commented Nov 14, 2016

redshiftzero Nov 14, 2016

psivesely Nov 15, 2016

psivesely commented Nov 15, 2016

psivesely commented Nov 15, 2016

Download unread/ all selected sources #1467

Download unread/ all selected sources #1467

Conversation

psivesely commented Nov 7, 2016

ajvb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psivesely commented Nov 7, 2016

psivesely commented Nov 8, 2016

redshiftzero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psivesely commented Nov 14, 2016

psivesely commented Nov 14, 2016

psivesely commented Nov 14, 2016

redshiftzero commented Nov 14, 2016

redshiftzero commented Nov 14, 2016

psivesely commented Nov 14, 2016

psivesely commented Nov 14, 2016

redshiftzero commented Nov 14, 2016

psivesely commented Nov 14, 2016

psivesely commented Nov 14, 2016

psivesely commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psivesely commented Nov 15, 2016

psivesely commented Nov 15, 2016