-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat: add LMS retirement listener for VerificationAttempts #35436
Conversation
59c768d
to
9da57c6
Compare
@receiver(USER_RETIRE_LMS_MISC) | ||
def _listen_for_lms_retire_verification_attempts(sender, **kwargs): # pylint: disable=unused-argument | ||
user = kwargs.get('user') | ||
VerificationAttempt.retire_user(user.id) |
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.
Should we handle exception during deletion? I feel like we should at least log info here to let the logger know we are deleting users.
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.
I had this same thought as you on Alie's persona-integration
pull request: https://github.com/edx/persona-integration/pull/17, but my final opinion was that because this code runs as part of a separate retirement job that runs the retirement scripts, it would probably be better for it to fail loudly than to fail silently so that we know something went wrong. My thinking was that no one is going to be reviewing the output of that job to look for silent failures, but loud failures may get someone's attention.
9da57c6
to
9530192
Compare
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
Because the
VerificationAttempt
model contains PII, the data contained in that model should be removed upon user retirement. This PR adds an event listener for theUSER_RETIRE_LMS_MISC
signal, and deletes anyVerificationAttempt
s associated with the specified user upon receiving the event.Supporting information
https://2u-internal.atlassian.net/browse/COSMO-464
openedx/platform-roadmap#367
Testing instructions
localhost:18000/api/user/v1/accounts/retire_misc/
, with a request body like{ 'username': '<username for VerificationAttempt>'}