-
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
Prepare for a smooth transition for new Deleted User #1397
Conversation
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.
Just did a code review for now, I'll test it tomorrow.
273e2c6
to
6ceefeb
Compare
Signed-off-by: Allie Crevier <allie@freedom.press>
6ceefeb
to
1e226ae
Compare
Signed-off-by: Allie Crevier <allie@freedom.press>
1e226ae
to
7f3299f
Compare
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.
Works well for me! Tested against a server running on current head of develop, the syncing was accurate. Notably, both the username and uuid were set to "deleted".
After switching the server to use PR freedomofpress/securedrop#6225 and repeating local testing, the sync of all attributes was still accurate, with the notable difference that the original uuid for the first deleted user in the client db was preserved. Creating and deleting multiple users did not create additional "deleted" users, nor did it overwrite the uuid of of the special "deleted" user in the client db, as expected.
I've approved, but am intentionally not merging, so that @legoktm can take a close look in tandem with related functionality in freedomofpress/securedrop#6225. |
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.
Went through the test plan and everything LGTM! And it helped me find a bug in my PR: freedomofpress/securedrop#6225 (comment).
Description
Fixes #1143
Closes #1157
This PR removes the
/user
api call and instead uses the/users
endpoint to update and manage the current user as well as all other users. Since an API call is removed and a new one is introduced, the functional test cassettes needed to be regenerated (see the cassettes section in the README for more details if needed).Followup: Once freedomofpress/securedrop#6225 is released, we can remove the old way of creating users based on a reply's
journalist_uuid
. For now, I left this in to continue to support showing replies associated with no corresponding user account on the server (/users
doesn't actually return any user for replies with a "deleted" UUID)Test Plan
make dev
to create a dev server with a deleted user- look for the sparkle icon in the journalist badge next to replies)Future-proof test
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