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

Request account removal #447

Merged

Conversation

charles-cowart
Copy link
Contributor

API support for an account removal queue.

Added admin-level endpoints to delete an account from queue, or ignore
the request. All actions are logged and stored in a separate table.
Ignore functionality was removed as a user-accessible endpoint.
Confirming send_email is cause of test failure in CI.
Set disable_authentication = False for the purposes of testing in CI.
Removed disable_authentication from server_config.json.
Disabled account deleted send_email() for testing.
This reverts commit e1b35f9.
@charles-cowart charles-cowart force-pushed the request_account_removal branch from 37a1117 to 2954b83 Compare August 2, 2022 21:18
microsetta_private_api/admin/admin_impl.py Outdated Show resolved Hide resolved
microsetta_private_api/admin/admin_impl.py Outdated Show resolved Hide resolved
microsetta_private_api/api/_account.py Outdated Show resolved Hide resolved
microsetta_private_api/api/_account.py Outdated Show resolved Hide resolved
microsetta_private_api/api/microsetta_private_api.yaml Outdated Show resolved Hide resolved
microsetta_private_api/api/tests/test_api.py Outdated Show resolved Hide resolved
microsetta_private_api/api/tests/test_api.py Outdated Show resolved Hide resolved
microsetta_private_api/repo/account_repo.py Outdated Show resolved Hide resolved
@charles-cowart charles-cowart force-pushed the request_account_removal branch from 91c164a to fffbe33 Compare August 4, 2022 19:01
@charles-cowart
Copy link
Contributor Author

@cassidysymons Ready for review/approval/merge.

Copy link
Member

@wasade wasade left a comment

Choose a reason for hiding this comment

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

The API test was pretty good, but I was surprised to not see unit tests.

microsetta_private_api/api/_removal_queue.py Outdated Show resolved Hide resolved
microsetta_private_api/api/_removal_queue.py Outdated Show resolved Hide resolved
microsetta_private_api/api/_removal_queue.py Outdated Show resolved Hide resolved
microsetta_private_api/db/patches/0102.sql Outdated Show resolved Hide resolved
microsetta_private_api/db/patches/0102.sql Outdated Show resolved Hide resolved
microsetta_private_api/admin/admin_impl.py Outdated Show resolved Hide resolved
microsetta_private_api/repo/removal_queue_repo.py Outdated Show resolved Hide resolved
microsetta_private_api/repo/removal_queue_repo.py Outdated Show resolved Hide resolved

# delete the entry from queue. account_delete() will fail
# w/out this.
cur.execute("DELETE FROM delete_account_queue WHERE account_id"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I missed it, but shouldn't this method call the account deletion? If it does not, we create (possible) scenario, particularly if multiple transactions are involved, where the account is migrated to the account_removal_log but not actually deleted

Copy link
Contributor Author

@charles-cowart charles-cowart Dec 14, 2022

Choose a reason for hiding this comment

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

This is handled in allow_removal_request():

https://github.com/charles-cowart/microsetta-private-api/blob/4b8f9455cebaf0392c6eb7645704d2b049af7276/microsetta_private_api/admin/admin_impl.py#L882-L897

delete_user() is called immediately after update_queue() returns and we're sure the change was committed and no RepoExceptions occurred. I believe @cassidysymons and I discussed it previously and we thought better of touching the existing delete code. He might remember it better than I do.

@charles-cowart charles-cowart changed the base branch from master to master-overhaul December 13, 2022 17:34
@charles-cowart
Copy link
Contributor Author

make sure new repo has success and failure tests as needed.

@charles-cowart
Copy link
Contributor Author

@cassidysymons @wasade Unit tests for removal_queue_repo are in. I'm going to address the remaining issues now.

@charles-cowart
Copy link
Contributor Author

@wasade @cassidysymons Tentatively done. Comments appreciated.

return jsonify(code=200, message="Request Accepted"), 200


def cancel_request_remove_account(account_id, token_info):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

verify this has an endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Endpoint added. I apparently added cancel_request_remove_account () for completeness. The UI doesn't use this functionality because we didn't want the user to cancel a request once they made it, and do it again. The UI will instead show a 'Your account deletion request is being reviewed.' msg instead.

Endpoint added for cancel_request_remove_account(). This endpoint is not
currently used by the UI.
API test updated to distinguish between removing the user from the queue
using cancel_request_remove_account() vs (admin)
ignore_removal_request().
Disposition must now be either 'deleted' or 'ignored'.
@charles-cowart
Copy link
Contributor Author

@cassidysymons tentatively good. Please feel free to review.

-- intention is to record the admin who accepted or denied the request
-- here. This means that an account_id may appear more than once if the
-- user makes multiple requests.
disposition VARCHAR(8),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change to ENUM

@cassidysymons cassidysymons merged commit c620348 into biocore:master-overhaul Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants