From 67e88cbd27b86832b90d5c89a03a8299422adf98 Mon Sep 17 00:00:00 2001 From: Krish Tyagi Date: Wed, 29 Jul 2026 06:59:57 +0000 Subject: [PATCH] feat: simplify retirement cleanup by removing custom redaction value parameters --- .../accounts/tests/test_retirement_views.py | 50 ++++--------------- .../djangoapps/user_api/accounts/views.py | 14 ++---- .../retirement_archive_and_cleanup.py | 32 ++---------- .../test_retirement_archive_and_cleanup.py | 45 ++--------------- scripts/user_retirement/utils/edx_api.py | 10 +--- 5 files changed, 24 insertions(+), 127 deletions(-) diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py b/openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py index 44128bddfe4b..b769dd238c94 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py @@ -1072,14 +1072,11 @@ def cleanup_and_assert_status(self, data=None, expected_status=status.HTTP_204_N assert response.status_code == expected_status return response - def _assert_redacted_update_delete_queries(self, queries, redacted_username, redacted_email, redacted_name): + def _assert_redacted_update_delete_queries(self, queries): """ Helper method to verify UPDATE and DELETE queries use ID-based filtering and correct field-value assignments. Args: queries: List of captured query dicts from CaptureQueriesContext - redacted_username: Expected redacted username value - redacted_email: Expected redacted email value - redacted_name: Expected redacted name value """ update_queries = [q for q in queries if 'UPDATE' in q['sql'] and 'user_api_userretirementstatus' in q['sql']] delete_queries = [q for q in queries if 'DELETE' in q['sql'] and 'user_api_userretirementstatus' in q['sql']] @@ -1090,14 +1087,14 @@ def _assert_redacted_update_delete_queries(self, queries, redacted_username, red update_query = update_queries[0] sql_lower = update_query['sql'] # Ensure original_username, original_email, and original_name are set to redacted values - assert f'"original_username" = \'{redacted_username}\'' in sql_lower, ( - f"UPDATE query missing '\"original_username\" = {redacted_username}': {sql_lower}" + assert '"original_username" = \'redacted\'' in sql_lower, ( + f"UPDATE query missing '\"original_username\" = redacted': {sql_lower}" ) - assert f'"original_email" = \'{redacted_email}\'' in sql_lower, ( - f"UPDATE query missing '\"original_email\" = {redacted_email}': {sql_lower}" + assert '"original_email" = \'redacted\'' in sql_lower, ( + f"UPDATE query missing '\"original_email\" = redacted': {sql_lower}" ) - assert f'"original_name" = \'{redacted_name}\'' in sql_lower, ( - f"UPDATE query missing '\"original_name\" = {redacted_name}': {sql_lower}" + assert '"original_name" = \'redacted\'' in sql_lower, ( + f"UPDATE query missing '\"original_name\" = redacted': {sql_lower}" ) # Ensure UPDATE uses ID-based filtering assert '"id" IN' in sql_lower or 'WHERE "id"' in sql_lower, ( @@ -1113,9 +1110,9 @@ def _assert_redacted_update_delete_queries(self, queries, redacted_username, red f"DELETE query should use ID filtering to prevent over-deletion, but got: {sql_lower}" ) - def test_default_redacted_values(self): + def test_redact_and_delete(self): """ - Test basic cleanup with default redacted values. + Test basic cleanup with redacted values. Verify that redaction (UPDATE) happens before deletion (DELETE). Captures actual SQL queries to ensure UPDATE queries contain correct field-value assignments. """ @@ -1126,33 +1123,8 @@ def test_default_redacted_values(self): retirements = UserRetirementStatus.objects.all() assert retirements.count() == 0 - # Verify UPDATE and DELETE queries with default 'redacted' value - self._assert_redacted_update_delete_queries(context.captured_queries, 'redacted', 'redacted', 'redacted') - - def test_custom_redacted_values(self): - """Test that custom redacted values are applied before deletion.""" - custom_username = 'username-redacted-12345' - custom_email = 'email-redacted-67890' - custom_name = 'name-redacted-abcde' - - data = { - 'usernames': self.usernames, - 'redacted_username': custom_username, - 'redacted_email': custom_email, - 'redacted_name': custom_name - } - - with CaptureQueriesContext(connection) as context: - self.cleanup_and_assert_status(data=data) - - # Verify records are deleted after redaction - retirements = UserRetirementStatus.objects.all() - assert retirements.count() == 0 - - # Verify UPDATE and DELETE queries with custom redacted values - self._assert_redacted_update_delete_queries( - context.captured_queries, custom_username, custom_email, custom_name - ) + # Verify UPDATE and DELETE queries with 'redacted' value + self._assert_redacted_update_delete_queries(context.captured_queries) def test_does_not_delete_unrelated_redacted_records(self): """ diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index e7ab64b0bfbe..759f32f08af8 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -1024,10 +1024,7 @@ def cleanup(self, request): ``` { - 'usernames': ['user1', 'user2', ...], - 'redacted_username': 'Value to store in username field', - 'redacted_email': 'Value to store in email field', - 'redacted_name': 'Value to store in name field' + 'usernames': ['user1', 'user2', ...] } ``` @@ -1035,9 +1032,6 @@ def cleanup(self, request): """ try: usernames = request.data["usernames"] - redacted_username = request.data.get("redacted_username", "redacted") - redacted_email = request.data.get("redacted_email", "redacted") - redacted_name = request.data.get("redacted_name", "redacted") if not isinstance(usernames, list): raise TypeError("Usernames should be an array.") @@ -1058,9 +1052,9 @@ def cleanup(self, request): retirement_ids = list(retirements.values_list('id', flat=True)) # Update by IDs UserRetirementStatus.objects.filter(id__in=retirement_ids).update( - original_username=redacted_username, - original_email=redacted_email, - original_name=redacted_name + original_username="redacted", + original_email="redacted", + original_name="redacted" ) # Delete by IDs UserRetirementStatus.objects.filter(id__in=retirement_ids, current_state=complete_state).delete() diff --git a/scripts/user_retirement/retirement_archive_and_cleanup.py b/scripts/user_retirement/retirement_archive_and_cleanup.py index e7211101b813..c73312e272fc 100644 --- a/scripts/user_retirement/retirement_archive_and_cleanup.py +++ b/scripts/user_retirement/retirement_archive_and_cleanup.py @@ -196,17 +196,14 @@ def _archive_retirements_or_exit(config, learners, dry_run=False): FAIL_EXCEPTION(ERR_ARCHIVING, 'Unexpected error occurred archiving retirements!', exc) -def _cleanup_retirements_or_exit(config, learners, redacted_username='redacted', - redacted_email='redacted', redacted_name='redacted'): +def _cleanup_retirements_or_exit(config, learners): """ Bulk deletes the retirements for this run after redacting PII fields """ LOG('Cleaning up retirements for {} learners'.format(len(learners))) # noqa: UP032 try: usernames = [l['original_username'] for l in learners] - config['LMS'].bulk_cleanup_retirements( - usernames, redacted_username, redacted_email, redacted_name - ) + config['LMS'].bulk_cleanup_retirements(usernames) except Exception as exc: # pylint: disable=broad-except FAIL_EXCEPTION(ERR_DELETING, 'Unexpected error occurred redacting/deleting retirements!', exc) @@ -262,26 +259,7 @@ def _get_utc_now(): help='Number of user retirements to process', type=int ) -@click.option( - '--redacted_username', - help='Value to use for redacted username field', - type=str, - default='redacted' -) -@click.option( - '--redacted_email', - help='Value to use for redacted email field', - type=str, - default='redacted' -) -@click.option( - '--redacted_name', - help='Value to use for redacted name field', - type=str, - default='redacted' -) -def archive_and_cleanup(config_file, cool_off_days, dry_run, start_date, end_date, batch_size, - redacted_username, redacted_email, redacted_name): +def archive_and_cleanup(config_file, cool_off_days, dry_run, start_date, end_date, batch_size): """ Cleans up UserRetirementStatus rows in LMS by: 1- Getting all rows currently in COMPLETE that were created --cool_off_days ago or more, @@ -336,9 +314,7 @@ def archive_and_cleanup(config_file, cool_off_days, dry_run, start_date, end_dat if dry_run: LOG('This is a dry-run. Exiting before any retirements are cleaned up') else: - _cleanup_retirements_or_exit( - config, batch, redacted_username, redacted_email, redacted_name - ) + _cleanup_retirements_or_exit(config, batch) LOG('Archive and cleanup complete for batch #{}'.format(str(index + 1))) # noqa: UP032 time.sleep(DELAY) else: diff --git a/scripts/user_retirement/tests/test_retirement_archive_and_cleanup.py b/scripts/user_retirement/tests/test_retirement_archive_and_cleanup.py index 59439de681f1..363a791e1dd3 100644 --- a/scripts/user_retirement/tests/test_retirement_archive_and_cleanup.py +++ b/scripts/user_retirement/tests/test_retirement_archive_and_cleanup.py @@ -28,8 +28,7 @@ FAKE_BUCKET_NAME = "fake_test_bucket" -def _call_script(cool_off_days=37, batch_size=None, dry_run=None, start_date=None, end_date=None, - redacted_username=None, redacted_email=None, redacted_name=None): +def _call_script(cool_off_days=37, batch_size=None, dry_run=None, start_date=None, end_date=None): """ Call the archive script with the given params and a generic config file. Returns the CliRunner.invoke results @@ -51,12 +50,6 @@ def _call_script(cool_off_days=37, batch_size=None, dry_run=None, start_date=Non base_args += ['--start_date', start_date] if end_date: base_args += ['--end_date', end_date] - if redacted_username: - base_args += ['--redacted_username', redacted_username] - if redacted_email: - base_args += ['--redacted_email', redacted_email] - if redacted_name: - base_args += ['--redacted_name', redacted_name] result = runner.invoke(archive_and_cleanup, args=base_args) print(result) @@ -113,7 +106,7 @@ def test_successful(*args, **kwargs): assert mock_get_access_token.call_count == 1 mock_get_learners.assert_called_once() mock_bulk_cleanup_retirements.assert_called_once_with( - ['test1', 'test2', 'test3'], 'redacted', 'redacted', 'redacted') + ['test1', 'test2', 'test3']) assert result.exit_code == 0 assert 'Archive and cleanup complete' in result.output @@ -141,44 +134,14 @@ def test_successful_with_batching(*args, **kwargs): # Called once to get the LMS token assert mock_get_access_token.call_count == 1 mock_get_learners.assert_called_once() - get_learner_calls = [call(['test1', 'test2'], 'redacted', 'redacted', 'redacted'), - call(['test3'], 'redacted', 'redacted', 'redacted')] + get_learner_calls = [call(['test1', 'test2']), + call(['test3'])] mock_bulk_cleanup_retirements.assert_has_calls(get_learner_calls) assert result.exit_code == 0 assert 'Archive and cleanup complete for batch #1' in result.output assert 'Archive and cleanup complete for batch #2' in result.output - -@patch('scripts.user_retirement.utils.edx_api.BaseApiClient.get_access_token', return_value=('THIS_IS_A_JWT', None)) -@patch.multiple( - 'scripts.user_retirement.utils.edx_api.LmsApi', - get_learners_by_date_and_status=DEFAULT, - bulk_cleanup_retirements=DEFAULT -) -@mock_aws -def test_successful_with_custom_redaction_values(*args, **kwargs): - conn = boto3.resource('s3') - conn.create_bucket(Bucket=FAKE_BUCKET_NAME) - - mock_get_access_token = args[0] - mock_get_learners = kwargs['get_learners_by_date_and_status'] - mock_bulk_cleanup_retirements = kwargs['bulk_cleanup_retirements'] - - mock_get_learners.return_value = fake_learners_to_retire() - - result = _call_script( - redacted_username='custom_user', - redacted_email='custom@example.com', - redacted_name='Custom Name' - ) - - # Called once to get the LMS token - assert mock_get_access_token.call_count == 1 - mock_get_learners.assert_called_once() - mock_bulk_cleanup_retirements.assert_called_once_with( - ['test1', 'test2', 'test3'], 'custom_user', 'custom@example.com', 'Custom Name') - assert result.exit_code == 0 assert 'Archive and cleanup complete' in result.output diff --git a/scripts/user_retirement/utils/edx_api.py b/scripts/user_retirement/utils/edx_api.py index f068b8416e6a..720b35b52528 100644 --- a/scripts/user_retirement/utils/edx_api.py +++ b/scripts/user_retirement/utils/edx_api.py @@ -352,19 +352,11 @@ def retirement_retire_proctoring_backend_data(self, learner): return self._request("POST", api_url) @_retry_lms_api() - def bulk_cleanup_retirements(self, usernames, redacted_username=None, - redacted_email=None, redacted_name=None): + def bulk_cleanup_retirements(self, usernames): """ Redacts and then deletes the retirements for all given usernames. - Optionally pass caller-defined redacted values for each PII field before deletion. """ data = {'usernames': usernames} - if redacted_username is not None: - data['redacted_username'] = redacted_username - if redacted_email is not None: - data['redacted_email'] = redacted_email - if redacted_name is not None: - data['redacted_name'] = redacted_name api_url = self.get_api_url("api/user/v1/accounts/retirement_cleanup") return self._request("POST", api_url, json=data)