removed TODOs and documented them in ticket tracker #3615

heartsucker · 2018-06-29T14:33:10Z

Status

Ready for review

Description of Changes

(kinda sorta) Fixes #724

Just removed TODOs

Testing

Just let CI run.

codecov-io · 2018-06-29T15:22:20Z

Codecov Report

Merging #3615 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #3615   +/-   ##
========================================
  Coverage    85.12%   85.12%           
========================================
  Files           37       37           
  Lines         2367     2367           
  Branches       260      260           
========================================
  Hits          2015     2015           
  Misses         289      289           
  Partials        63       63

Impacted Files	Coverage Δ
crypto_util.py	`96% <0%> (ø)`	⬆️
journalist_app/decorators.py	`100% <0%> (ø)`	⬆️
models.py	`88.35% <0%> (ø)`	⬆️
config.py	`85.36% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3554e7...3909172. Read the comment docs.

redshiftzero

I'm a big fan of tracking these TODOs in the bug tracker 🤘 thanks @heartsucker

ghost · 2018-06-29T22:10:14Z

How do we relate the TODO's with their replacement in the list of issues?

redshiftzero

hey - I went through and wrote out my justifications for these line by line for @dachary to peruse. also: @dachary or @heartsucker, if either you disagree with any of my comments, feel free to add a note on the corresponding issue or file a followup

redshiftzero · 2018-06-30T00:10:20Z

docs/create_admin_account.rst

@@ -50,9 +50,6 @@ output like this:
 Passphrases include the spaces between the words, but not leading or trailing
 whitespace. Be sure to save this passphrase in the appropriate KeePassX database.

-.. todo:: Clarify how to set up TOTP/HOTP through ``./manage.py


issue #3594

redshiftzero · 2018-06-30T00:11:33Z

docs/development/setup_development.rst

@@ -164,15 +164,6 @@ from the `Vagrant Downloads page`_ and then install it.
            instructions in Vagrantfile that would enable vagrant-cachier are
            currently commented out.

-.. todo:: This warning is here because a common refrain during hackathons for


irrelevant, we use the Docker dev env at hackathons (rarely if ever to people wade into the waters 🌊 of the staging VMs at hackathons)

redshiftzero · 2018-06-30T00:11:55Z

docs/journalist.rst

@@ -40,13 +40,6 @@ key, used for decryption, stays on the *Journalist Workstation*. The
 public key, used for encryption, is copied to the *Secure Viewing
 Station*.

-.. todo:: This document recommends transferring documents from the


issue #3598

redshiftzero · 2018-06-30T00:12:13Z

docs/onboarding.rst

@@ -41,9 +41,6 @@ you have a lot of journalists who wish to access SecureDrop
 concurrently, you will need to provision multiple *Secure Viewing
 Stations*.

-.. todo:: Describe best practices for provisioning multiple Secure


issue #3596

redshiftzero · 2018-06-30T00:12:48Z

docs/overview.rst

@@ -58,9 +58,6 @@ the sources, and the journalists.

 |SecureDrop architecture overview diagram|

-.. todo:: A picture of an actual physical setup (e.g. the office


issue #3599

redshiftzero · 2018-06-30T00:25:27Z

securedrop/tests/test_journalist.py

@@ -161,7 +161,6 @@ def test_unauthorized_access_redirects_to_login(journalist_app):

 def test_login_throttle(journalist_app, test_journo):
    # Overwrite the default value used during testing
-    # TODO this may break other tests during parallel testing


covered by #2755 and #2535

redshiftzero · 2018-06-30T00:25:43Z

securedrop/tests/test_journalist.py

@@ -1130,9 +1129,6 @@ def tearDown(self):
    # making a point of this, we hope to avoid the introduction of new tests,
    # that do not truly prove their result because of this disconnect between
    # request context in Flask Testing and production.
-    #
-    # TODO: either ditch Flask Testing or subclass it as discussed in the
-    # aforementioned issue to fix the described problem.


issue #3607

redshiftzero · 2018-06-30T00:26:01Z

securedrop/tests/utils/env.py

@@ -16,7 +16,7 @@

 FILES_DIR = abspath(join(dirname(realpath(__file__)), '..', 'files'))

-# TODO: the PID file for the redis worker is hard-coded below.  Ideally this


this one is also issue #3606

redshiftzero · 2018-06-30T00:26:28Z

testinfra/app/test_tor_hidden_services.py

@@ -13,7 +13,6 @@ def test_tor_service_directories(File, Sudo, tor_service):
    with Sudo():
        f = File("/var/lib/tor/services/{}".format(tor_service['name']))
        assert f.is_directory
-        # TODO: tor might mark these dirs as setgid


issue #3602

redshiftzero · 2018-06-30T00:40:02Z

docs/backup_and_restore.rst

@@ -153,8 +153,6 @@ Once the restore is done, the Application Server will use the original Source an
 *Journalist Interface* Onion URLs. You will need to update the corresponding
 files on the Admin Workstation:

-.. todo:: We really should automate this process for Admins.


imho fine to remove, this is no longer a priority to implement as:

the correct behavior is documented for admins,

the restore part of the backup/restore is a rare action. as such, engineering effort spent on automating it does not produce benefits to end users as much as automating more common tasks e.g. the journalist provisioning process (issue Provisioning utility for journalists & Journalist Workstations #1177, a common request by admins)

ghost · 2018-06-30T11:52:29Z

Great reference, thanks! @redshiftzero

removed TODOs and documented them in ticket tracker

3909172

heartsucker requested a review from conorsch as a code owner June 29, 2018 14:33

heartsucker requested a review from a user June 29, 2018 14:33

heartsucker requested review from msheiny and redshiftzero as code owners June 29, 2018 14:33

redshiftzero approved these changes Jun 29, 2018

View reviewed changes

redshiftzero merged commit 25d988b into develop Jun 29, 2018

redshiftzero deleted the remove-todos branch June 29, 2018 21:27

redshiftzero reviewed Jun 30, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

removed TODOs and documented them in ticket tracker #3615

removed TODOs and documented them in ticket tracker #3615

heartsucker commented Jun 29, 2018

codecov-io commented Jun 29, 2018 •

edited

Loading

redshiftzero left a comment

ghost commented Jun 29, 2018

redshiftzero left a comment

redshiftzero Jun 30, 2018

redshiftzero Jun 30, 2018

redshiftzero Jun 30, 2018

redshiftzero Jun 30, 2018

redshiftzero Jun 30, 2018

redshiftzero Jun 30, 2018

redshiftzero Jun 30, 2018

redshiftzero Jun 30, 2018

redshiftzero Jun 30, 2018

redshiftzero Jun 30, 2018

ghost commented Jun 30, 2018

		@@ -58,9 +58,6 @@ the sources, and the journalists.

		\|SecureDrop architecture overview diagram\|

		.. todo:: A picture of an actual physical setup (e.g. the office

		@@ -16,7 +16,7 @@

		FILES_DIR = abspath(join(dirname(realpath(__file__)), '..', 'files'))

		# TODO: the PID file for the redis worker is hard-coded below. Ideally this

removed TODOs and documented them in ticket tracker #3615

removed TODOs and documented them in ticket tracker #3615

Conversation

heartsucker commented Jun 29, 2018

Status

Description of Changes

Testing

codecov-io commented Jun 29, 2018 • edited Loading

Codecov Report

redshiftzero left a comment

Choose a reason for hiding this comment

ghost commented Jun 29, 2018

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 30, 2018

codecov-io commented Jun 29, 2018 •

edited

Loading