-
-
Notifications
You must be signed in to change notification settings - Fork 105
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
feat: [390] - Purge non-confirmed users periodically #402
Conversation
aa6a090
to
5385758
Compare
ms -= nonConfirmedUsersCleanupDelay; | ||
Date d = new Date(ms); | ||
|
||
List<User> users = userRepo.findByConfirmedIsFalse().stream() |
There was a problem hiding this comment.
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:
- 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? - Does any of the details need to be stored or all the details of user need to be purged?
- 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Purge, no soft delete
- No, delete all data
- I don't think any email notification is needed.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
eaf77af
to
257b7d0
Compare
257b7d0
to
957e667
Compare
423a171
to
db7421e
Compare
|
||
List<User> users = userRepo.findByConfirmedIsFalseAndDatesCreationDateBefore(d); | ||
|
||
userRepo.deleteAll(users); |
There was a problem hiding this comment.
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.
Changes:
app.schedule.delay.user.cleanup