fix: redacting user retirement data in lms #105
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the user retirement cleanup process to support redaction of PII fields instead of deletion. The changes add configurable redacted values for username, email, and name fields that can be passed through the cleanup workflow.
Changes:
- Modified
bulk_cleanup_retirementsAPI method to accept optional redacted values for username, email, and name fields - Added three new CLI options (
--redacted_username,--redacted_email,--redacted_name) with default value of 'redacted' - Updated tests to verify the new parameters are passed correctly through the function call chain
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/user_retirement/utils/edx_api.py | Updated bulk_cleanup_retirements method signature to accept optional redacted field values and updated docstring to reflect redaction operation |
| scripts/user_retirement/retirement_archive_and_cleanup.py | Added CLI options for redacted field values and updated _cleanup_retirements_or_exit to pass these values to the API |
| scripts/user_retirement/tests/test_retirement_archive_and_cleanup.py | Updated test assertions to include the three new redacted parameters and fixed error message assertion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| usernames, redacted_username, redacted_email, redacted_name | ||
| ) | ||
| except Exception as exc: # pylint: disable=broad-except | ||
| FAIL_EXCEPTION(ERR_DELETING, 'Unexpected error occurred deleting retirements!', exc) |
There was a problem hiding this comment.
The error message still refers to "deleting retirements" but should be updated to "redacting retirements" to be consistent with the change from deletion to redaction operations throughout the rest of the code.
| FAIL_EXCEPTION(ERR_DELETING, 'Unexpected error occurred deleting retirements!', exc) | |
| FAIL_EXCEPTION(ERR_DELETING, 'Unexpected error occurred redacting retirements!', exc) |
| redacted_email='redacted', redacted_name='redacted'): | ||
| """ | ||
| Bulk deletes the retirements for this run | ||
| Bulk deletes the retirements for this run after redacting PII fields |
There was a problem hiding this comment.
The docstring says "Bulk deletes the retirements" but should be updated to "Bulk redacts the retirements" to accurately reflect that the function is redacting PII fields rather than deleting the retirement records.
| Bulk deletes the retirements for this run after redacting PII fields | |
| Bulk redacts the retirements for this run by redacting PII fields |
There was a problem hiding this comment.
[question] Should this be combined in the same PR as https://github.com/openedx/openedx-platform/pull/37886/changes, only against edx/edx-platform if we plan on merging/testing there first?
| ) | ||
| except Exception as exc: # pylint: disable=broad-except | ||
| FAIL_EXCEPTION(ERR_DELETING, 'Unexpected error occurred deleting retirements!', exc) | ||
| FAIL_EXCEPTION(ERR_DELETING, 'Unexpected error occurred redacting retirements!', exc) |
There was a problem hiding this comment.
Should this now read as follows?
| FAIL_EXCEPTION(ERR_DELETING, 'Unexpected error occurred redacting retirements!', exc) | |
| FAIL_EXCEPTION(ERR_DELETING, 'Unexpected error occurred redacting/deleting retirements!', exc) |
or just go back to the original message of "deleting"?
| def archive_and_cleanup(config_file, cool_off_days, dry_run, start_date, end_date, batch_size): | ||
| @click.option( | ||
| '--redacted_username', | ||
| help='Value to use for redacted username fields', |
There was a problem hiding this comment.
Nit: (for all options)
| help='Value to use for redacted username fields', | |
| help='Value to use for redacted username field', |
Description
This PR adds a new option to the retirement cleanup script that lets you pass a custom placeholder value to replace user information. It then sends this value through to the LMS system so both systems can work together to redact user data.
Private Link Ticket
https://2u-internal.atlassian.net/browse/BOMS-293