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

Rename journalist key routes #5651

Conversation

DrGFreeman
Copy link
Contributor

@DrGFreeman DrGFreeman commented Nov 25, 2020

Status

Ready for review

Description of Changes

Fixes #4421

Changes proposed in this pull request:

Rename why-journalist-key and journalist-key routes of the Source Interface to why-public-key and public-key respectively. This aligns the route names with the language used in the associated pages where "public key" is used to designate the SecureDrop Submission Key of the instance.

Also rename the functions associated with the routes.

Maintain the journalist-key and redirect it to public-key in case it is used or linked by instance owners or sources. Add a unit test to verify the redirect.

Testing

How should the reviewer test this PR?
Write out any special testing steps here.

  1. In the source interface, navigate to /lookup.
  2. Hover over the public key link.
    • Verify the link points to /public-key.
  3. Click the public key link.
    • Verify the key file downloads as expected.
  4. Click the learn more link.
    • Verify the /why-public-key page loads.
  5. Hover over the Download link.
    • Verify the link points to /public-key.
  6. Click the Download link.
    • Verify the key file downloads as expected.
  7. Using the browser address bar navigate to /journalist-key.
    • Verify the redirect works and the key file downloads as expected.

Deployment

Any special considerations for deployment? Consider both:

  1. Upgrading existing production instances.
  2. New installs.

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make test) pass in the development container

If you made changes to securedrop-admin:

  • Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

Choose one of the following:

If you added or updated a code dependency:

Choose one of the following:

  • I have performed a diff review and pasted the contents to the packaging wiki
  • I would like someone else to do the diff review

@emkll emkll added this to Ready for Review in SecureDrop Team Board Nov 25, 2020
@emkll emkll moved this from Ready for Review to In Development in SecureDrop Team Board Nov 25, 2020
@emkll
Copy link
Contributor

emkll commented Nov 25, 2020

Thanks @DrGFreeman for your contribution. This PR will need a rebase on latest develop to include the changes to from #5648 to get all CI tests passing.

@zenmonkeykstop zenmonkeykstop self-assigned this Nov 25, 2020
Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

Test plan passes, but it would be a good idea to include a redirect from the old /journalist-key to the new /public-key in case its in use anywhere. See patch in comment for simplest approach.

securedrop/source_app/info.py Show resolved Hide resolved
Rename why-journalist-key and journalist-key routes of the Source
Interface to why-public-key and public-key respectively. This aligns
the route names with the language used in the associated pages where
"public key" is used to designate the SecureDrop Submission Key of the
instance.
Restore /journalist-key and redirect it to public-key in case it may be
used or linked by instances or sources.

Add a test to verify the redirect.
@DrGFreeman DrGFreeman force-pushed the 4421-rename-journalist-key-routes branch from dc121a9 to 57c141a Compare November 26, 2020 02:11
@DrGFreeman
Copy link
Contributor Author

@zenmonkeykstop, thanks for the feedback. Good points that I'll consider in future PRs.

I restored the /journalist-key and redirected it to /public-key as suggested. I also added a test to verify the redirect.

Note that the default return type of flask.redirect is werkzeug.wrappers.Response instead of flask.Response. It can be changed via Response parameter however I don't see the purpose of changing it at this point. Let me know if you think otherwise.

I rebased the branch as requested by @emkll. I intended to do it before submitting the PR but I forgot. 😕

@codecov-io
Copy link

codecov-io commented Nov 26, 2020

Codecov Report

Merging #5651 (57c141a) into develop (95f0d7e) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5651      +/-   ##
===========================================
+ Coverage    53.10%   53.13%   +0.03%     
===========================================
  Files           50       50              
  Lines         5928     5932       +4     
  Branches       530      531       +1     
===========================================
+ Hits          3148     3152       +4     
  Misses        2678     2678              
  Partials       102      102              
Impacted Files Coverage Δ
securedrop/source_app/info.py 78.12% <100.00%> (+3.12%) ⬆️

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 95f0d7e...57c141a. Read the comment docs.

Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for adding in the 301 on the old path.

@zenmonkeykstop zenmonkeykstop merged commit 566a2df into freedomofpress:develop Nov 26, 2020
SecureDrop Team Board automation moved this from In Development to Done Nov 26, 2020
@eloquence eloquence added this to the 1.7.0 milestone Jan 5, 2021
@kushaldas kushaldas mentioned this pull request Jan 18, 2021
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

"Journalist key" in source interface is not (typically) a journalist key
5 participants