fix: redacting user retirement data in lms

[ktyagiapphelix2u]

Description

Instead of deleting user retirement records completely, this PR updates the system to replace personal information (name, email, username) with safe placeholder values while keeping the records in the database.

Private Ticket

https://2u-internal.atlassian.net/browse/BOMS-293

[pdpinch]

Maybe this is out of scope for this PR, but I'm concerned about the explicit reference to Jenkins. I understand the desirability of recording the job id, but not everyone uses Jenkins as their runner.

[robrap]

@pdpinch: @ktyagiapphelix2u is out until Friday, but we will be removing references to 2U and Jenkins. Thanks.

[ktyagiapphelix2u]

@pdpinch I agreed I am removing jenkins name reference. Thanks.

[robrap]

@ktyagiapphelix2u: Also, I think we should change this to a breaking change PR. That means updating the title to fix!: .... Also, we should open a Fast Track DEPR (i.e automatically accepted) for this as an inform.

We should note that switching from deleting records to redacting records is not a breaking change from the point of view of user retirement, because in either case the sensitive data has been safely taken care of. The breaking change is that these records will no longer be deleted, so any operator scripts that run against the table after retirement, that also relied on the fact that data was being deleted, would need to be updated.

[ktyagiapphelix2u]

@robrap I was thinking of updating archive cleanup script for Open edX. Is it expected to update it there as well, or should it be left as is? as they have moved to CI workflows rather than jenkins

[bmtcril]

Hi all, can we get a description of the problem this is trying to address since we can't see the internal ticket? I have some (very small) amount of concern about this table growing given how often it's called, but more want to understand why this change is necessary.

Since this is a breaking change in the API I think it should be versioned to a V2 endpoint as part of the depr.

@fghaas I know you've been involved in tutor-contrib-retirement so wanted to give you a heads up as well

[robrap]

@bmtcril: In thinking through explaining the issue to you, I may have come up with a backward-compatible version of this change, so that is what I am going to propose we do.

The reason we wanted this change is because in Snowflake, we've used different technologies for sync, but all of them treat the status record deletes as soft-deletes, which then requires an additional custom job to clear out the soft deleted data to fully remove the sensitive data. We decided that redacting here, just like we do everywhere else, would avoid the custom jobs and ongoing maintenance.

I just realized that we could have the best of both worlds, and have this api first redact, and then delete. That way it will still be backward-compatible with the deletes, but we wouldn't have to clean up the soft-deletes, because those records would already be redacted.

@ktyagiapphelix2u: Can you please implement this. You'll need to ensure that the redacted data gets saved to the DB before the delete. I'm not certain exactly how to ensure this, but maybe working with Dave W. to ensure that the soft-deleted records contain the redacted data? Hopefully neither mysql nor Django nor the sync code tries to be too smart.

[robrap]

@ktyagiapphelix2u: Once we ensure we can get everything working with redact + delete, we'd be able to close the DEPR:

• [DEPR]: User Retirement Records Redaction #37921.

[robrap]

I'd start with either assertNumQueries (see other comment) or possibly the Django debug toolbar to ensure you are getting the update query before the delete query.

Once confirmed, you could also consider using https://www.mysqltutorial.org/mysql-administration/mysql-binary-logs/ locally to confirm the update is seen before the delete.

[robrap]

Looks great. Minor comments. Thank you.

[robrap]

I'm not sure of the exact syntax, but can you update custom_username to something like f"original_username='{custom_username}'" so we verify that the correct field is getting the correct value?

[robrap]

@ktyagiapphelix2u: You either missed this comment, or didn't fully understand it. If you updated the code to:

 for retirement in retirements:
                retirement.original_username = redacted_email
                retirement.original_email = redacted_name
                retirement.original_name = redacted_username

Where we are setting the wrong field with each input, I think your test would still pass, right? I was trying to get you to assert that the correct field was being set.

[ktyagiapphelix2u]

@robrap I've updated the test to check that each specific field original_username, original_email, original_name is being set with its correct corresponding value. If someone swaps the assignments as you suggested, the test will now fail immediately. Thanks.

[robrap]

I don't think there is any difference between what is being tested here, and what is tested in test_redaction_before_deletion, right? It's the same test, but you are just using two different names and asserting on different things. I think I would just delete test_simple_success and rename test_redaction_before_deletion to test_default_redacted_values. I think test_default_redacted_values already has all the assertions you need. Thoughts?

[ktyagiapphelix2u]

Updated thanks.

[robrap]

Also, I remove the breaking change "!" from the title, because this is no longer a breaking change. Thanks.

[robrap]

@ktyagiapphelix2u: I thought you had said this work was complete. I guess I misunderstood. Either way, please take care of this and all copilot comments. Thank you.

[robrap]

Oh. Is this a very outdated duplicate PR for openedx? We should discuss our process, but I would have imagined that this would be the PR we start with.