Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
from django.contrib.sites.models import Site
from django.core import mail
from django.core.cache import cache
from django.db import connection
from django.test import TestCase
from django.test.utils import CaptureQueriesContext
from django.urls import reverse
from enterprise.models import (
EnterpriseCourseEnrollment,
Expand Down Expand Up @@ -1079,8 +1081,79 @@ def cleanup_and_assert_status(self, data=None, expected_status=status.HTTP_204_N
return response

def test_simple_success(self):
"""
Test basic cleanup with default redacted values.
"""
# Verify redaction happens (records exist before cleanup)
assert UserRetirementStatus.objects.count() == 9

# Make the cleanup request
self.cleanup_and_assert_status()
assert not UserRetirementStatus.objects.all()

# Records should be deleted after redaction
retirements = UserRetirementStatus.objects.all()
assert retirements.count() == 0

def test_redaction_before_deletion(self):
"""
Verify that redaction (UPDATE) happens before deletion (DELETE).
Captures actual SQL queries to ensure UPDATE queries contain redacted values.
"""
with CaptureQueriesContext(connection) as context:
self.cleanup_and_assert_status()

# Verify records are deleted after redaction
retirements = UserRetirementStatus.objects.all()
assert retirements.count() == 0

# Verify UPDATE queries exist with default 'redacted' value
queries = context.captured_queries
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']]

# Should have 9 UPDATE and 9 DELETE queries
assert len(update_queries) == 9, f"Expected 9 UPDATE queries, found {len(update_queries)}"
assert len(delete_queries) == 9, f"Expected 9 DELETE queries, found {len(delete_queries)}"

# Verify UPDATE queries contain the redacted values
for update_query in update_queries:
sql = update_query['sql']
assert "'redacted'" in sql, f"UPDATE query missing 'redacted' value: {sql}"
assert 'original_username' in sql, f"UPDATE query missing original_username field: {sql}"
assert 'original_email' in sql, f"UPDATE query missing original_email field: {sql}"
assert 'original_name' in sql, f"UPDATE query missing original_name field: {sql}"

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'
Comment on lines +1128 to +1130
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @ktyagiapphelix2u. I'm glad you were able to confirm the updates are happening with assertNumQueries. I know I'm being picky here, but I don't want to lose the assertions that the redacting is happening do the correct fields using the specific custom redacted values.

If you peak under the covers of assertNumQueries, it is using CaptureQueriesContext. You can see how our custom implementation of assertNumQueries reviews the queries to pull out queries for certain ignore-tables.

Could you try using CaptureQueriesContext in place of assertNumQueries, and ensure that the queries are found with the appropriate updates? I think assert retirements.count() == 0 is enough for asserting that the deletes happened, and we can assume they happen after the updates, or otherwise the updates would have failed (maybe)? You could try swapping the .save() and .delete() line to see what happens, and see if this needs its own assertion.

Hopefully this makes sense. Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah @robrap, that make sense so i have implemented using CaptureQueriesContext and tested them so these 3 cased now what i see is-
Case 1: Correct Order (SAVE then DELETE)
We call .save() to execute 9 UPDATE queries redacting PII, then .delete() to execute 9 DELETE queries. The MySQL binlog captures both: UPDATE with redacted data first, then DELETE, so Fivetran syncs safely redacted PII to Snowflake.

Case 2: Swapped Broken Order (DELETE then SAVE)
We call .delete() to remove records, then .save() fails because records no longer exist, resulting in 0 UPDATE queries. PII never gets redacted and Fivetran syncs original sensitive data to Snowflake.

Case 3: Why Tests Fail
Tests assert 9 UPDATE queries should execute. When order is swapped, they find 0 instead, failing with "assert 9 == 0" and proving the importance of redacting before deleting.


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 queries contain the custom redacted values
queries = context.captured_queries
update_queries = [q for q in queries if 'UPDATE' in q['sql'] and 'user_api_userretirementstatus' in q['sql']]

assert len(update_queries) == 9, f"Expected 9 UPDATE queries, found {len(update_queries)}"

for update_query in update_queries:
sql = update_query['sql']
assert custom_username in sql, f"UPDATE query missing custom username '{custom_username}': {sql}"
assert custom_email in sql, f"UPDATE query missing custom email '{custom_email}': {sql}"
assert custom_name in sql, f"UPDATE query missing custom name '{custom_name}': {sql}"

def test_leaves_other_users(self):
remaining_usernames = []
Expand Down
21 changes: 18 additions & 3 deletions openedx/core/djangoapps/user_api/accounts/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -1024,14 +1024,20 @@ def cleanup(self, request):

```
{
'usernames': ['user1', 'user2', ...]
'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'
}
```

Deletes a batch of retirement requests by username.
Redacts a batch of retirement requests by redacting PII fields.
"""
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.")
Expand All @@ -1045,7 +1051,16 @@ def cleanup(self, request):
if len(usernames) != len(retirements):
raise UserRetirementStatus.DoesNotExist("Not all usernames exist in the COMPLETE state.")

retirements.delete()
# Redact PII fields first, then delete. In case an ETL tool is syncing data
# to a downstream data warehouse, and treats the deletes as soft-deletes,
# the data will have first been redacted, protecting the sensitive PII.
for retirement in retirements:
retirement.original_username = redacted_username
retirement.original_email = redacted_email
retirement.original_name = redacted_name
retirement.save()
retirement.delete()

return Response(status=status.HTTP_204_NO_CONTENT)
except (RetirementStateError, UserRetirementStatus.DoesNotExist, TypeError) as exc:
return Response(str(exc), status=status.HTTP_400_BAD_REQUEST)
Expand Down
Loading