Skip to content

Fix link in email when confirming new email address for existing account#7019

Merged
mitchellhenke merged 1 commit intomainfrom
mitchellhenke/fix-email-confirmation
Sep 22, 2022
Merged

Fix link in email when confirming new email address for existing account#7019
mitchellhenke merged 1 commit intomainfrom
mitchellhenke/fix-email-confirmation

Conversation

@mitchellhenke
Copy link
Contributor

No description provided.

…irming new email address for existing account
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 38 to +39
<%= link_to add_email_confirmation_url(_request_id: @request_id, confirmation_token: @token, locale: @locale),
sign_up_create_email_confirmation_url(
add_email_confirmation_url(
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 what if we assigned @add_email_url in the mailer, and then re-used that variable here in the template instead of re-creating it 3 times, risking having different results? (I can also make a follow-up PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I'm good with that

Copy link
Contributor

Choose a reason for hiding this comment

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

@mitchellhenke mitchellhenke merged commit 0fa7516 into main Sep 22, 2022
@mitchellhenke mitchellhenke deleted the mitchellhenke/fix-email-confirmation branch September 22, 2022 23:04
mitchellhenke pushed a commit that referenced this pull request Sep 22, 2022
…irming new email address for existing account (#7019)
zachmargolis added a commit that referenced this pull request Sep 22, 2022
- Follow-up to #7019 to prevent similar bugs in the future

[skip changelog]
zachmargolis added a commit that referenced this pull request Sep 26, 2022
- Follow-up to #7019 to prevent similar bugs in the future

[skip changelog]
@solipet solipet mentioned this pull request Sep 26, 2022
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.

2 participants