-
Notifications
You must be signed in to change notification settings - Fork 10
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
Deal with registration edge cases in UserCleaner #693
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #693 +/- ##
==========================================
+ Coverage 54.40% 54.45% +0.04%
==========================================
Files 160 160
Lines 6744 6751 +7
==========================================
+ Hits 3669 3676 +7
Misses 3075 3075 ☔ View full report in Codecov by Sentry. |
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.
LGTM, but I think we should find a solution concerning the scopes. My suggestion for now would be to place them in the User
model, make a comment there, and we will see how to deal with it when we are in the process of refactoring later on.
* Deal with registration edge cases in UserCleaner * Replace `last_sign_in_at` by `current_sign_in_at` * Simplify inactive_users method via usage of scopes * Add another scope for recently active users
In #647, we introduced the "UserCleaner" that deletes users that haven't logged in for too long. We now noticed that we don't deal with specific edge cases related to user registration. This means that our initial
where
clause(to determine which users are inactive) is not sufficient to handle all cases. The new special cases are described as docstrings in the
app/models/user_cleaner.rb
.Furthermore, we also replace
last_sign_in_at
bycurrent_sign_in_at
in this PR. See this Devise code to see the difference.current_sign_in_at
always holds the newest value of the last user sign in, so we want to use that instead oflast_sign_in_at
, which refers to the last sign in before thecurrent_sign_in_at
date. Yes, the names could have been chosen better by Devise 😅Reviewers
I admit that it was way more difficult than I originally thought to arrive at the new statement, which is a lot more complicated than the original one. Nevertheless, I think its worth it to also handle the edge cases, especially in order not to irritate users with deletion mails when they have just registered (and confirmed their account -> note that also in this case
last_sign_in_at
is stillnil
).inactive_users
method. I've done this multiple times to refine the statement and in the current state, I wasn't able to break it anymore. But maybe you can?:user
by:confirmed_user
in the tests such that they still work. Please make sure that this does not mean I forgot to handle cases where we should actually also test for non-confirmed users.where
statement in a more performant way, e.g. by resorting to direct SQL or by using some other tricks. I was happy that I arrived at any workable solution for all the edge cases in the first place and didn't measure anything related to performance. This is just to inform you about this.