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

update reply sender name during sync if it has changed #1137

Merged
merged 2 commits into from
Sep 10, 2020

Conversation

sssoleileraaa
Copy link
Contributor

@sssoleileraaa sssoleileraaa commented Aug 7, 2020

Description

Towards #76

This depends on freedomofpress/securedrop-sdk#125 which can be installed via pip install --upgrade git+https://github.com/freedomofpress/securedrop-sdk@add-journalist-name-to-reply#egg=securedrop-sdk but should not be merged until the sdk is updated.

Test Plan

  1. pip install git+https://github.com/freedomofpress/securedrop-sdk@add-journalist-name-to-reply#egg=securedrop-sdk
  2. run make test and see all tests pass locally (they won't pass here until the sdk is updated)
  3. log into the client
  4. log into the journalist UI with a new journalist account
  5. send a reply to a source as the new journalist.
  6. wait until next sync then check local client db
    • verify local db shows an account for that new journalist in the users table
  7. change first and last name of this new journalist account from the journalist UI
  8. wait until next sync then check local client db
    • verify local db shows the new first and last names for the journalist in the users table
  9. change first and last name to empty string from the journalist UI
  10. wait until next sync then check local client db
    • verify local db shows updated first and last name
  11. change username of the new journalist account from the admin UI
  12. wait until next sync then check local client db
    • verify local db shows updated username

@sssoleileraaa sssoleileraaa changed the title update reply journo first and last name during sync update reply sender name during sync if it has changed Aug 7, 2020
@sssoleileraaa sssoleileraaa force-pushed the update-reply-senders-during-sync branch from 866e831 to f0167ee Compare August 7, 2020 00:11
@sssoleileraaa
Copy link
Contributor Author

sssoleileraaa commented Aug 7, 2020

Note: We will see CI fail here with errors like the following until a new version of the sdk is published:

E               AttributeError: 'Reply' object has no attribute 'journalist_first_name'

But you can run through the test plan now and see that it works locally.

@sssoleileraaa sssoleileraaa added this to Ready for Review in SecureDrop Team Board Sep 2, 2020
@emkll emkll moved this from Ready for Review to Under Review in SecureDrop Team Board Sep 8, 2020
Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

Thanks @creviera test plan is passing for me using locally built sdk, and visual review of the diff looks good

  1. pip install --upgrade git+https://github.com/freedomofpress/securedrop-sdk@add-journalist-name-to-reply#egg=securedrop-sdk
  • run make test and see all tests pass locally (they won't pass here until the sdk is updated)
  1. log into the client
  2. log into the journalist UI with a new journalist account
  3. send a reply to a source as the new journalist.
  4. wait until next sync then check local client db
    • verify local db shows an account for that new journalist in the users table
  5. change first and last name of this new journalist account from the journalist UI
  6. wait until next sync then check local client db
    • verify local db shows the new first and last names for the journalist in the users table
  7. change first and last name to empty string from the journalist UI
  8. wait until next sync then check local client db
    • verify local db shows updated first and last name
  9. change username of the new journalist account from the admin UI
  10. wait until next sync then check local client db

As you mentioned in the PR description, marking as blocked until freedomofpress/securedrop-sdk#127 is closed, and we can include SDK 0.1.1 in the requirements file as part of this PR to get tests passing

@emkll emkll moved this from Under Review to In Development in SecureDrop Team Board Sep 8, 2020
@emkll emkll added the blocked label Sep 8, 2020
@emkll
Copy link
Contributor

emkll commented Sep 9, 2020

securedrop-sdk 0.1.1 has been released in freedomofpress/securedrop-builder#193 , this PR is now unblocked.

We should now be able to use this new version of the sdk in the requirements file, and after a rebase on latest main this will be ready for final review.

@emkll emkll removed the blocked label Sep 9, 2020
@sssoleileraaa sssoleileraaa moved this from In Development to Ready for Review in SecureDrop Team Board Sep 10, 2020
@sssoleileraaa
Copy link
Contributor Author

thanks for the review. sdk in the requirements files has been updated, so now you'll see ci pass and you can skip pip install --upgrade git+https://github.com/freedomofpress/securedrop-sdk@add-journalist-name-to-reply#egg=securedrop-sdk if you run through the test plan again.

@emkll emkll moved this from Ready for Review to Under Review in SecureDrop Team Board Sep 10, 2020
Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

Went through the test plan again using a locally built deb (including the sdk 0.1.1 wheel). Works as expected, visual diff also looks good to me. Thanks!

Test plan

  1. built client deb on this branch using the sdk wheel from pypi mirror
  2. log into the client
  3. log into the journalist UI with a new journalist account
  4. send a reply to a source as the new journalist.
  5. wait until next sync then check local client db
    • verify local db shows an account for that new journalist in the users table
  6. change first and last name of this new journalist account from the journalist UI
  7. wait until next sync then check local client db
    • verify local db shows the new first and last names for the journalist in the users table
  8. change first and last name to empty string from the journalist UI
  9. wait until next sync then check local client db
    • verify local db shows updated first and last name
  10. change username of the new journalist account from the admin UI
  11. wait until next sync then check local client db
    • verify local db shows updated username

@@ -9,7 +9,7 @@ pathlib2==2.3.2 --hash=sha256:460e67b14d0574b0529a0017b1eb05d10d9722681e303fec70
python-dateutil==2.7.5 --hash=sha256:f6eb9c17acd5a6954e1a5f2f999a41de3e7e25b6bc41baf6344bd053ec25ceeb
python-editor==1.0.3 --hash=sha256:e47dcec4ea883853b8196fbd425b875d7ec791d4ede2e20cfc70b9a25365c65b
requests==2.20.0 --hash=sha256:d87b2085783d31d874ac7bc62660e287932aaee7059e80b41b76462eb18d35cc
securedrop-sdk==0.1.0 --hash=sha256:488417f9f08e4c432c81348dfbd5da0e756ded1737ba58b2ffc8f0e703abc1cb
securedrop-sdk==0.1.1 --hash=sha256:a631495acd741ab568410287879c5a3af3ccd38e00a2f3a127cc6b27cba99392
Copy link
Contributor

Choose a reason for hiding this comment

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

✔️

securedrop-sdk==0.1.0 \
--hash=sha256:970fde25e6238e1808ac120951ee972549f4cd7952966dfe29f731bb308cc0d8 \
securedrop-sdk==0.1.1 \
--hash=sha256:138ce7a717db519c3c8d19b9475d7660fb7095d7608e8802723e682a7415e677 \
Copy link
Contributor

Choose a reason for hiding this comment

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

✔️

@emkll emkll merged commit d97c652 into main Sep 10, 2020
SecureDrop Team Board automation moved this from Under Review to Done Sep 10, 2020
@emkll emkll deleted the update-reply-senders-during-sync branch September 10, 2020 20:07
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.

None yet

2 participants