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

feat: add LMS retirement listener for VerificationAttempts #35436

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

alangsto
Copy link
Contributor

@alangsto alangsto commented Sep 5, 2024

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 the USER_RETIRE_LMS_MISC signal, and deletes any VerificationAttempts 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

  • Create a VerificationAttempt via the Django admin interface.
  • Make a post request to localhost:18000/api/user/v1/accounts/retire_misc/, with a request body like { 'username': '<username for VerificationAttempt>'}
  • Assert that VerificationAttempt that was created for the user has been removed.

@alangsto alangsto force-pushed the alangsto/add_user_retirement_listener branch from 59c768d to 9da57c6 Compare September 5, 2024 14:44
@alangsto alangsto marked this pull request as ready for review September 5, 2024 15:51
@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)
Copy link
Contributor

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.

Copy link
Contributor

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.

@alangsto alangsto requested review from ormsbee and a team September 5, 2024 17:19
@kdmccormick kdmccormick removed the request for review from a team September 5, 2024 17:21
@alangsto alangsto force-pushed the alangsto/add_user_retirement_listener branch from 9da57c6 to 9530192 Compare September 10, 2024 18:33
@alangsto alangsto merged commit 2c28ef1 into master Sep 10, 2024
49 checks passed
@alangsto alangsto deleted the alangsto/add_user_retirement_listener branch September 10, 2024 18:57
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

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.

4 participants