Skip to content
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

feat: [390] - Purge non-confirmed users periodically #402

Conversation

lucifercr07
Copy link
Contributor

@lucifercr07 lucifercr07 commented Jun 6, 2024

Changes:

  • Related issue: [FEATURE] Purge non-confirmed users periodically #390
  • Added new method in ScheduleService to purge non-confirmed users that haven't confirmed their email-address for 14 days
  • Added new method to scheduleLong()
  • 14 days as configurable in application.properties under app.schedule.delay.user.cleanup
  • Added unit test

@lucifercr07 lucifercr07 force-pushed the lucifercr07/390_purge_non_confirmed_users_periodically branch from aa6a090 to 5385758 Compare June 6, 2024 12:43
@lucifercr07 lucifercr07 changed the title [FEAT][390] - Purge non-confirmed users periodically feat: [390] - Purge non-confirmed users periodically Jun 6, 2024
ms -= nonConfirmedUsersCleanupDelay;
Date d = new Date(ms);

List<User> users = userRepo.findByConfirmedIsFalse().stream()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently this is just finding the users which have not confirmed and deleting all whose creation date is before 14 days.
@Nonononoki can you please confirm below points once:

  1. If we need to archive/soft-delete them rather than purge? Can see something as user-hide table shall we utilise it and purge after 90 days?
  2. Does any of the details need to be stored or all the details of user need to be purged?
  3. Also is there a mechanism already in place to notify these users before 14 days to confirm their email address else account would be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add unit-tests once above approach is confirmed.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Purge, no soft delete
  2. No, delete all data
  3. I don't think any email notification is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nonononoki please review once have added unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lucifercr07 You can use the date filtering directly in the JPA (e.g. `findByConfirmedIsFalseAndDatesCreationDateBefore(Date date))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nonononoki thanks have addressed the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Code is looking good, but why did you need to change the RegisterServiceTest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to create new mock users as only testUsers was getting used across all tests in that file. Also delete users method would throw error since few of the users would be purged and couldn't be found. Please let me know if you feel there's any other effective way to do this would change accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nonononoki please let me know if any changes required to be done here.

@lucifercr07 lucifercr07 force-pushed the lucifercr07/390_purge_non_confirmed_users_periodically branch 2 times, most recently from eaf77af to 257b7d0 Compare June 6, 2024 13:23
@lucifercr07 lucifercr07 force-pushed the lucifercr07/390_purge_non_confirmed_users_periodically branch from 257b7d0 to 957e667 Compare June 6, 2024 13:23
@lucifercr07 lucifercr07 requested a review from Nonononoki June 10, 2024 12:05
@lucifercr07 lucifercr07 force-pushed the lucifercr07/390_purge_non_confirmed_users_periodically branch from 423a171 to db7421e Compare June 10, 2024 18:16
@Nonononoki Nonononoki merged commit 2a51ca4 into Alovoa:master Jun 13, 2024
3 checks passed
@Dremor Dremor mentioned this pull request Jun 21, 2024

List<User> users = userRepo.findByConfirmedIsFalseAndDatesCreationDateBefore(d);

userRepo.deleteAll(users);
Copy link

Choose a reason for hiding this comment

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

Could be a good idea to double check before purging user... you know, to avoid risking purging the whole database in the case of "userRepo.findByConfirmedIsFalseAndDatesCreationDateBefore" not being restrictive enough. Btw, were is this query implemented? I can't find it on the PR diff.

@lucifercr07 lucifercr07 deleted the lucifercr07/390_purge_non_confirmed_users_periodically branch June 21, 2024 18:08
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