-
Notifications
You must be signed in to change notification settings - Fork 42
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 sender badge to replies #1142
Conversation
Update:
|
81e2eba
to
5f0a336
Compare
5f0a336
to
de27c31
Compare
this is ready for review, but should wait for #1147 to be merged first |
b0f013f
to
fe703fe
Compare
Functional testing, part 1: Sending a reply from the JI
Changing username
Sending a reply from the JI with a new username
Changing name
Multiple journalists responding to the same source
Deleting a journalistNote: #1143 is a known issue
Sending a reply from the client: success
Sending a reply from the client: failed
|
Part 2: Sending a reply from the client: pending
Switching journalist accounts in the client
Multiple deleted accountsNote: #1143 is a known issue
Failed replies associated with deleted account
Same authenticated journalist with new name across sources
|
Functional testing per test plan looks great! Will do a bit more exploratory testing tomorrow. >:) |
Did a bit more exploratory testing and wasn't able to break things (e.g. deleting individual replies via JI, using pseudo-delete functionality in Source UI, renaming user right after logging in and before sending first reply). :) |
fe703fe
to
9eaec46
Compare
I added one more test section to the test plan around offline mode and switching accounts, so please make sure to focus here upon second review:
|
9eaec46
to
60af203
Compare
just learned there is one more state around reply badges that we need to test: the failed-to-decrypt state. will update the test plan and push one more change so this is ready for the night crew :) |
60af203
to
9768bc7
Compare
This is ready for review again! I add a new test section to the test plan to account for changes made to handle decryption errors:
|
9768bc7
to
188fa92
Compare
188fa92
to
882024d
Compare
updated to include new artwork added from the issue this resolves (still ready for review) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(@emkyll oh friday mistakes... i accidentally edited your comment and now i'm unable to get your comment back with all of the formatting, so we can just look at the github comment history here to see what you said orignally :bushes:)
@emkyll - you'll see above that i accidentally edited your comment and lost all the nice formatting you did so... for efficiency, i captured just the part of the test plan that needs focus (the rest passed without issue):
Just to double check, you're saying that in step 3 and 4 the new reply from a journalist and message from a source did not show up in gray with the "cannot decrypt message", even though you deleted the submission key locally? If so I am unable to reproduce. In case it helps try this:
For test #3 above, you need to do the same thing but with a source message instead of a journalist reply. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @creviera confirmed the thorough test plan is now passing in an sd-app
VM in Qubes, including the failed to decrypt scenario. (see below). Thanks for your hard work on this, changes look great.
There are a couple of todo's still outstanding (one which is dependent on freedomofpress/securedrop-sdk#134) . Would you like to address these as part of this PR or open follow-up issues? IMO this PR if good to merge as-is, and the todo's are small incremental improvements that can be merged separately.
Sending a reply from the JI
- log into the JI as
dellsberg
and reply to the new source
- confirm that you see
de
in the badge next to the reply
Changing username
- log into the JI as
journalist
(an admin account) and change dellsberg's username tounknown
and wait until next sync
- confirm that you see the badge initials next to the reply change from
de
toun
Sending a reply from the JI with a new username
- log into the JI as
unknown
and reply to the source
- confirm that you see
un
in the badge next to the new reply
Changing name
- while logged in as
unknown
, change first name toPizza
and last name toCat
and wait until next sync
- confirm that you see the badge initials next to all existing replies change from
un
topc
- confirm that you see
pc
in the badge next to the new reply
Multiple journalists responding to the same source
- log into the JI as
journalist
and reply to the source
- confirm that you see
jo
in a purple badge next to the reply - confirm all replies from
Pizza Cat
remainpc
Deleting a journalist
Note: #1143 is a known issue
- log into the JI as
journalist
and delete unknown's account
- confirm that you see a deleted account icon in the badge next to all replies from
unknown
Sending a reply from the client: success
- log into the client as
alice
- confirm that you see
jo
in a blue badge now
- send a reply to the source
- confirm that you see
al
in a purple badge next to the new reply
Sending a reply from the client: failed
- close the client
- in the securedrop-client root dir, run
git clone https://gist.github.com/5ba70d50c12b6a6df6f98ed40ad09645.git
- run
git apply 5ba70d50c12b6a6df6f98ed40ad09645/failed-reply
- log in as alice and send a reply to the source
- confirm that you see
al
in a red badge next to the new reply - confirm that you see a red status bar
Sending a reply from the client: pending
- close the client
- in the securedrop-client root dir, run
git clone https://gist.github.com/7f19a7d10334359f40dbdbb2354cd13a.git
- stash your changes to
uploads.py
and rungit apply 7f19a7d10334359f40dbdbb2354cd13a/pending-reply
- log in as alice and send a reply to the source
- confirm that you see
al
in a purple badge next to the new reply - confirm that you see a purple status bar
- confirm the background color of the reply widget and the color of the text are faded/ more transparent
Offline mode and switching accounts in the client
- stash your changes and now log into the client as journalist
- confirm that journalist replies have purple badges
- logout so that you're in offline mode
- confirm that all reply badges, including badges for journalist are now blue
- click the login button and log in as alice
- confirm
al
badges are now purple
- close the client and log in in offline mode
- confirm all badges are blue
- click the login button and log in as journalist
- confirm all badges for journalist are purple
Multiple deleted accounts
Note: #1143 is a known issue
- log into the JI as journalist and delete alice's account and wait until next sync
- confirm that you now see a deleted account icon in badges next to alice's replies that are not failed replies (see next step for what to expect with failed replies that are asscociated with a deleted account)
Failed replies associated with deleted account
- confirm from you see an
al
in a red badge next to failed replies: once the known issue described in Deleted user is not fully removed from client database after a sync #1143 (comment) is fixed we can start showing the deleted account icon in the red badge
Same authenticated journalist with new name across sources
- reply to all sources with the same "journalist" account
- now change the username from the admin interface in the JI and wait until next sync
- confirm that you see the new initials in all the relevant reply badges across all the sources
- now change the first and/or last name from the admin interface in the JI and wait until next sync
- confirm that you see the new initials in all the relevant reply badges across all the sources
Failed-to-decrypt errors
- start the client, and before logging in, delete the submission key:
gpg --homedir ~/.securedrop_client/gpg --delete-secret-keys --yes 65A1B5FF195B56353CC63DFFCC40EF1228271441
gpg --homedir ~/.securedrop_client/gpg --delete-keys --yes 65A1B5FF195B56353CC63DFFCC40EF1228271441
- log in and send a reply
- confirm failed-to-send error with urgent coral badge
- Send a reply from the JI
- confirm reply appears with a gray badge
- Send a message from the JI
- confirm message appears with a gray badge
- Delete account from the JI
- confirm failed-to-decrypt reply badge is now gray with the deleted account icon
- log out
- confirm all failed-to-send and failed-to-decrypt status bars, badges, and other styling remain unchanged
- restart the client
- confirm all failed-to-decrypt messages and replies are now decrypted with normal styling
self.set_normal_styles() | ||
self.error.hide() | ||
if self.uuid == uuid: | ||
self.status = "SUCCEEDED" # TODO: Add and use success status in db.ReplySendStatusCodes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we aren't adding db.ReplySendStatusCodes as part of this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, db.ReplySendStatusCodes
only includes 'FAILED' and 'PENDING' statuses. I see other areas of the code that have to hardcode an implied 'SUCCEEDED' status, so I followed suit and made note of how we can improve this: we can add a success status to db.ReplySendStatusCodes
and start tracking successful replies with a non-null status. This will require several unrelated updates so I left it out of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Between Erik and myself, we'll add issues to address this TODO and the TODO here: https://github.com/freedomofpress/securedrop-client/pull/1142/files#diff-e9eabf3b8acf5ab72fd3a0881b7a8af395005334f8349fcc6965c385183e1b22R2956-R2957
""" | ||
Add a reply from a journalist to the source. | ||
""" | ||
try: | ||
send_status = reply.send_status.name | ||
except AttributeError: | ||
send_status = "SUCCEEDED" | ||
send_status = "SUCCEEDED" # TODO: Add and use success status in db.ReplySendStatusCodes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
@@ -1658,11 +1747,12 @@ def __init__( | |||
update_signal, | |||
download_error_signal, | |||
index: int, | |||
error: bool = False, | |||
failed_to_decrypt: bool = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious as to why this was renamed, unless there's no other error that could occur with replies (other than failure to decrypt)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there was a bug during implementation: error is also the name of the error message widget so when we checked to see if error was null, it always said, nope!
also there are a couple types of errors: decryption/download and failure to send.
Follow up is being tracked in #1156, and we will improve the journalist updating logic once the sdk is released with the changes in freedomofpress/securedrop-sdk#134. |
Description
Resolves #76
Test Plan
Init setup
make dev
journalist
Sending a reply from the JI
dellsberg
and reply to the new sourcede
in the badge next to the replyChanging username
journalist
(an admin account) and change dellsberg's username tounknown
and wait until next syncde
toun
Sending a reply from the JI with a new username
unknown
and reply to the sourceun
in the badge next to the new replyChanging name
unknown
, change first name toPizza
and last name toCat
and wait until next syncun
topc
pc
in the badge next to the new replyMultiple journalists responding to the same source
journalist
and reply to the sourcejo
in a purple badge next to the replyPizza Cat
remainpc
Deleting a journalist
Note: #1143 is a known issue
journalist
and delete unknown's accountunknown
Sending a reply from the client: success
alice
jo
in a blue badge nowal
in a purple badge next to the new replySending a reply from the client: failed
git clone https://gist.github.com/5ba70d50c12b6a6df6f98ed40ad09645.git
git apply 5ba70d50c12b6a6df6f98ed40ad09645/failed-reply
al
in a red badge next to the new replySending a reply from the client: pending
git clone https://gist.github.com/7f19a7d10334359f40dbdbb2354cd13a.git
uploads.py
and rungit apply 7f19a7d10334359f40dbdbb2354cd13a/pending-reply
al
in a purple badge next to the new replyOffline mode and switching accounts in the client
al
badges are now purpleMultiple deleted accounts
Note: #1143 is a known issue
Failed replies associated with deleted account
al
in a red badge next to failed replies: once the known issue described in Deleted user is not fully removed from client database after a sync #1143 (comment) is fixed we can start showing the deleted account icon in the red badgeSame authenticated journalist with new name across sources
Failed-to-decrypt errors
gpg --homedir ~/.securedrop_client/gpg --delete-secret-keys --yes 65A1B5FF195B56353CC63DFFCC40EF1228271441
gpg --homedir ~/.securedrop_client/gpg --delete-keys --yes 65A1B5FF195B56353CC63DFFCC40EF1228271441
Checklist
If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:
If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable:
If these changes modify the database schema, you should include a database migration. Please check as applicable:
main
and confirmed that the migration applies cleanlymain
and would like the reviewer to do so