Skip to content

feat: soft delete user #2776

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Apr 29, 2025
Merged

feat: soft delete user #2776

merged 12 commits into from
Apr 29, 2025

Conversation

capJavert
Copy link
Contributor

@capJavert capJavert commented Apr 18, 2025

  • preserve deleted user ids
  • preserve deleted user transactions under ghost
  • check for deleted user ids collision on user creation
  • add new handler for deleted user collision in safeInsertUser
  • also apply delete user logic to zombie user cron

@capJavert capJavert self-assigned this Apr 18, 2025
@capJavert capJavert requested a review from a team as a code owner April 18, 2025 09:49
@capJavert capJavert requested a review from idoshamun April 18, 2025 09:59
Comment on lines +9 to +13
await queryRunner.query(
`DROP RULE prototect_ghostuser_deletion on "user";`,
);
await queryRunner.query(`
CREATE OR REPLACE FUNCTION prevent_special_user_delete()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of RULE create trigger, because RULE was blocking usage of RETURNING in zombie cron.

@omBratteng omBratteng requested a review from Copilot April 22, 2025 12:56
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements soft deletion for users by preserving deleted user IDs, checking for collisions during user creation, and handling zombie user cleanup. Key changes include:

  • Adding a new telemetry metric counter for deleted user collisions.
  • Creating migrations for a DeletedUser table and a trigger to prevent deletion of special users.
  • Updating user logic and error handling to support soft deletion and collision detection.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/telemetry/metrics.ts Adds metric counter for tracking deleted user collisions.
src/migration/1744972478032-SpecialUserDeleteTrigger.ts Creates trigger to prevent deletion of special users; includes a potential condition inconsistency and naming typo.
src/migration/1744965354822-DeletedUser.ts Creates the DeletedUser table for storing soft deleted user records.
src/errors.ts Adds a new DeletedUserCollisionError and updates error enums.
src/entity/user/utils.ts Enhances safeInsertUser logic to check for deleted users and handle collisions.
src/entity/user/DeletedUser.ts Defines the DeletedUser entity for soft delete implementation.
src/cron/cleanZombieUsers.ts Updates zombie user cleanup to insert records into DeletedUser.
src/common/user.ts Modifies deleteUser to insert a DeletedUser record after deletion.
tests/private.ts Adds tests to verify handling of deleted user collisions during user creation.
tests/cron/cleanZombieUsers.ts Adds tests ensuring deleted user records are created during zombie cleanup.
Comments suppressed due to low confidence (1)

src/migration/1744972478032-SpecialUserDeleteTrigger.ts:10

  • [nitpick] The rule name 'prototect_ghostuser_deletion' may be a typo; consider renaming it to 'protect_ghostuser_deletion' for clarity.
`DROP RULE prototect_ghostuser_deletion on "user";`

Copy link
Member

@omBratteng omBratteng left a comment

Choose a reason for hiding this comment

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

What happens in the user_transaction table? Does the rows get deleted?

@capJavert
Copy link
Contributor Author

yes, as other user content, njord keeps the log though, although @idoshamun wdyt, should we move user_transaction items to ghost user instead of cascading when user is deleted?

@omBratteng
Copy link
Member

I think we should update it to ghost user, so the receiver still can see awards they've been given.

@capJavert
Copy link
Contributor Author

@omBratteng yeah I agree, see 9402bb6

@capJavert capJavert requested a review from omBratteng April 22, 2025 14:02
@capJavert capJavert requested review from idoshamun and Copilot April 28, 2025 15:04
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@capJavert
Copy link
Contributor Author

@idoshamun ready for review again 🙏

@capJavert capJavert enabled auto-merge (squash) April 29, 2025 11:26
@capJavert capJavert merged commit f24b2e3 into main Apr 29, 2025
8 checks passed
@capJavert capJavert deleted the soft-delete-user branch April 29, 2025 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants