Add logging and exception handling to delete respondents

[pricem14pc]

What and why?

Introduces exception handling, db commits, db rollbacks, logging and unit tests for handling batch respondent deletions.

The current implementation terminates mid process if a single DB error is encountered when deleting records. This indiscriminately rolls back previous unrelated respondent record deletions.

As each respondent is unrelated there is no need to stop the process, only to roll back the failing respondent transaction and move onto the next respondent being deleted. We should commit successful deletion transactions before moving onto the next respondent being deleted.

How to test?

This can be complicated to test due to the permutations of data required, and the potential exceptions that can be thrown. However, an approach is detailed below;

• create four separate accounts
• transfer a survey for account numbers 1 and 3 (i.e create pending_surveys for them)
• edit the auth.user.last_login_date year of all four respondents from 2025 to 2021 to make them subject to deletion
• run the following CURL commands to action the deletions

curl -u admin:secret -X DELETE http://localhost:8041/api/batch/account/users/mark-for-deletion
curl -u admin:secret -X DELETE http://localhost:8041/api/batch/account/users
curl -u admin:secret -X DELETE http://localhost:8081/party-api/v1/batch/respondents

• test the behaviour of the current implementation to replicate the bug whereby processing terminates at the first db error (also note the rollback of previously successfully processed deletions)
• test the behaviour of this PR fix
• check the data to ensure partysvc.respondents 2 and 4 are deleted, and also their referenced child records
• check the data to ensure records for respondents 1 and 3 are NOT deleted, nor are their referenced child records
• check the party application logs to ensure all deletions, failed deletions, and counts are accurately logged

Jira

https://jira.ons.gov.uk/browse/RAS-1460

[pricem14pc]

/deploy pricem

[ras-rm-pr-bot]

Deploying to dev cluster with following parameters:

• namespace: pricem

• tag: pr-452

• configBranch: main

• paramKey: ``

• paramValue: ``

[SteveScorfield]

/deploy scorfs

[ras-rm-pr-bot]

Deploying to dev cluster with following parameters:

• namespace: scorfs

• tag: pr-452

• configBranch: main

• paramKey: ``

• paramValue: ``

[SteveScorfield]

Tested and works correctly. No more errors relating to rollbacks. Code looks good. Approved.

[LJBabbage]

The logging does work, although as the failed_deletion_count is shared and inside an info it kind of gets hidden, however you do have the "A data constraint violation occurred trying to delete the respondent records" as an error which helps. On that subject you don't have to punch out the party_id again as it is already in the error detail.

Just logging this is fine, but the process is not so good. I lose my respondent in auth regardless, so the account becomes unusable even though it doesn't get deleted in party.

I also lose my shared to target, which conflicts with the expectations in the instructions, its the pending_surveys_shared_by_fkey which causes the foreign key constraint which would make sense, that I keep the person that is sharing, but the one shared to is removed. This then will of course also leave a record in pending_surveys to an email that has been deleted.

I do get one email sent which is kind of right with how it is working, rather than the expected behaviour. However the email to the one that has been shared to will then force a new user generation, which might not be quite right

In summary logging is fine, the data that is left isn't great and we definitely need to priorities an alert if we want to merge

[pricem14pc]

I lose my respondent in auth regardless, so the account becomes unusable even though it doesn't get deleted in party.

This is intended existing functionality and is correct. The auth user account is deleted on night 1, sending a request to mark the respondent party for deletion. To all intents and purposes the user account has been correctly deleted and the user can no longer login. On night 2 an attempt is made to delete the respondent party records for the user who has had their account deleted on night 1. That may or may not be successful depending on the existence of dependent party records (e.g. pending_survey). In most scenarios that will correct itself in up to 72 hours until the pending_survey records are accepted or expired. As I have mentioned, there may be other scenarios we know nothing about yet that are causing the party deletion exceptions.

The only question I see still outstanding is whether the account deletion email should be sent on night 1 following the deletion of the auth user account, rather than on night 2 following the deletion of the respondent party records.

[pricem14pc]

I also lose my shared to target, which conflicts with the expectations in the instructions, its the pending_surveys_shared_by_fkey which causes the foreign key constraint which would make sense, that I keep the person that is sharing, but the one shared to is removed. This then will of course also leave a record in pending_surveys to an email that has been deleted.

I'm not sure what scenario you tested, but there was no instruction to share a survey with another account that you are also deleting. While this may be possible, this isn't a scenario I'm aware we need to deal with. The transfer target is intended to be an unrelated/different email address to those being deleted.

[pricem14pc]

failed_deletion_count is shared and inside an info it kind of gets hidden

Yes this is reasonable but its 100% better than we currently have, however the known Integrity Violation exception we are seeing are recoverable and not an exceptional scenario. Therefore logging it as info feels appropriate.

[LJBabbage]

I also lose my shared to target, which conflicts with the expectations in the instructions, its the pending_surveys_shared_by_fkey which causes the foreign key constraint which would make sense, that I keep the person that is sharing, but the one shared to is removed. This then will of course also leave a record in pending_surveys to an email that has been deleted.

I'm not sure what scenario you tested, but there was no instruction to share a survey with another account that you are also deleting. While this may be possible, this isn't a scenario I'm aware we need to deal with. The transfer target is intended to be an unrelated/different email address to those being deleted.

This bit, you are making them all viable for deletion
edit the auth.user.last_login_date year of all four respondents from 2025 to 2021 to make them subject to deletion

[pricem14pc]

I also lose my shared to target, which conflicts with the expectations in the instructions, its the pending_surveys_shared_by_fkey which causes the foreign key constraint which would make sense, that I keep the person that is sharing, but the one shared to is removed. This then will of course also leave a record in pending_surveys to an email that has been deleted.

I'm not sure what scenario you tested, but there was no instruction to share a survey with another account that you are also deleting. While this may be possible, this isn't a scenario I'm aware we need to deal with. The transfer target is intended to be an unrelated/different email address to those being deleted.

This bit, you are making them all viable for deletion edit the auth.user.last_login_date year of all four respondents from 2025 to 2021 to make them subject to deletion

That's correct. They should all be subject to deletion. But there's no instruction to share a survey from one of the four users with another one of the four users. Which I'm assuming from your comment is what you did.

[LJBabbage]

I also lose my shared to target, which conflicts with the expectations in the instructions, its the pending_surveys_shared_by_fkey which causes the foreign key constraint which would make sense, that I keep the person that is sharing, but the one shared to is removed. This then will of course also leave a record in pending_surveys to an email that has been deleted.

I'm not sure what scenario you tested, but there was no instruction to share a survey with another account that you are also deleting. While this may be possible, this isn't a scenario I'm aware we need to deal with. The transfer target is intended to be an unrelated/different email address to those being deleted.

This bit, you are making them all viable for deletion edit the auth.user.last_login_date year of all four respondents from 2025 to 2021 to make them subject to deletion

That's correct. They should all be subject to deletion. But there's no instruction to share a survey from one of the four users with another one of the four users. Which I'm assuming from your comment is what you did.

Transfer should be for both new and existing accounts, therefore you need an account that isn't being deleted to test as well, ergo there should be 5 in your instructions, with 4 deleted

[LJBabbage]

Just seen the pipfile.lock has been updated?

[pricem14pc]

I also lose my shared to target, which conflicts with the expectations in the instructions, its the pending_surveys_shared_by_fkey which causes the foreign key constraint which would make sense, that I keep the person that is sharing, but the one shared to is removed. This then will of course also leave a record in pending_surveys to an email that has been deleted.

I'm not sure what scenario you tested, but there was no instruction to share a survey with another account that you are also deleting. While this may be possible, this isn't a scenario I'm aware we need to deal with. The transfer target is intended to be an unrelated/different email address to those being deleted.

This bit, you are making them all viable for deletion edit the auth.user.last_login_date year of all four respondents from 2025 to 2021 to make them subject to deletion

That's correct. They should all be subject to deletion. But there's no instruction to share a survey from one of the four users with another one of the four users. Which I'm assuming from your comment is what you did.

Transfer should be for both new and existing accounts, therefore you need an account that isn't being deleted to test as well, ergo there should be 5 in your instructions, with 4 deleted

Ok, it depends what scenarios you're testing. This PR was to fix a very specific production bug whereby processing stopped when an exception occurred. Your scenario may be one way of generating that but is not needed to test the issue. You don't need an existing account to transfer surveys to someone else. You can simply transfer to me@here.com and this will be good enough.

[LJBabbage]

Agreed updating Jinga is ok this time