-
Notifications
You must be signed in to change notification settings - Fork 68
Refactor first_login email to onboarded
#11766
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.
Looks good! Almost there. Do you have a plan on how to handle users who don't finish onboarding? See #11708
|
|
||
| after_update :update_stripe_cardholder, if: -> { phone_number_previously_changed? || email_previously_changed? } | ||
|
|
||
| after_update_commit :send_onboarded_email, if: -> { full_name_previously_changed? && full_name_before_last_save.blank? && full_name.present? } |
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.
This is a bit brittle in the case that we change what it means to be onboarded.
Can we abstract some of this logic out into a method that plays well with our existing onboarding? method?
Lines 315 to 318 in ab1d1c4
| def onboarding? | |
| # in_database to prevent a blank name update attempt from triggering onboarding. | |
| full_name_in_database.blank? | |
| end |
| if user.user_sessions.size == 1 | ||
| UserSessionMailer.first_login(user:).deliver_later | ||
| elsif fingerprint.present? && user.user_sessions.excluding(self).where(fingerprint:).none? | ||
| if user.user_sessions.size > 1 && fingerprint.present? && user.user_sessions.excluding(self).where(fingerprint:).none? |
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.
This comment is not directly related to this PR. However, maybe we should explicitly wrap this in a !impersonated?.
This isn't completely necessary since impersonated sessions do not have fingerprints. However, I do still believe it's good to have this condition explicitly documented in the code.
No, honestly, it doesn't make sense to me why we'd email them this email. We can have a job that does that every 10 minutes but just feels weird to send someone an email they don't seem to want. |
Closes #11708 by factoring this email to only go out when a user is "onboarded" which we define as the user having a full name.