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
Cancel signup link #25201
Cancel signup link #25201
Conversation
- provider = t("auth.#{@user.provider}") | ||
= t('activerecord.attributes.user.finish_sign_up_subheader_provider', provider: provider, email: @user.email).html_safe | ||
- else | ||
= t('activerecord.attributes.user.finish_sign_up_subheader', email: @user.email).html_safe |
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.
I feel okay about relying on having @user.email
set at this point (the user shouldn't be able to get to this point if we don't have an email for their account), but would like to hear others' feedback on this.
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.
Now that we're not sending Clever through this path I suppose it's fine; we may end up with other providers that don't send over an email in the future. (Powerschool?)
That won't cause an error though, just an odd string - I'm okay if we want to cross that bridge when we come to it.
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.
I've got suggestions, but this looks great!
- provider = t("auth.#{@user.provider}") | ||
= t('activerecord.attributes.user.finish_sign_up_subheader_provider', provider: provider, email: @user.email).html_safe | ||
- else | ||
= t('activerecord.attributes.user.finish_sign_up_subheader', email: @user.email).html_safe |
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.
Now that we're not sending Clever through this path I suppose it's fine; we may end up with other providers that don't send over an email in the future. (Powerschool?)
That won't cause an error though, just an odd string - I'm okay if we want to cross that bridge when we come to it.
def self.cancel(session) | ||
SignUpTracking.end_sign_up_tracking(session) | ||
session.delete(USER_ATTRIBUTES_SESSION_KEY) | ||
session |
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.
Is there a particular reason to return session
here?
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.
for unit tests, which i feel kind of meh about. being able to create a fake session, pass it in to the method (which is necessary either way), and return it made testing this method more straightforward
@@ -47,6 +47,12 @@ def self.persist_attributes(session, user) | |||
session[USER_ATTRIBUTES_SESSION_KEY] = user.attributes | |||
end | |||
|
|||
def self.cancel(session) | |||
SignUpTracking.end_sign_up_tracking(session) |
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.
I know we haven't spec'd it, but it might be helpful to send a metrics event for cancel right before we end sign-up tracking here.
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.
good call. i'll add a cancel event that gets logged for the experiment group (which is currently the only group that sees the "cancel" link)
Note: change base before merging!
What it does
GET /users/cancel
route to cancel an in-progress partial registrationusers_cancel_path
in the finish signup formExamples
OAuth accounts:
Email accounts (i.e., user where provider is
nil
):