Skip to content
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

Add dedicated reconfirmation mail #5087

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sakuraineed
Copy link

This PR adds :subject_key to devise_mail options for reconfirmation mail subject.

@tegon
Copy link
Member

tegon commented Aug 2, 2019

Hi @sakuraineed, thanks for your contribution.

I'm more inclined to include a dedicated reconfirmation email, as mentioned in #4145. Do you want to change this pull request to do that instead?

@sakuraineed sakuraineed force-pushed the add_dedicated_reconfirmation_mail_subject branch from 2115d29 to 33e921f Compare August 11, 2019 08:55
@sakuraineed sakuraineed changed the title Add dedicated reconfirmation mail subject Add dedicated reconfirmation mail Aug 13, 2019
@sakuraineed
Copy link
Author

Hi @tegon, thanks for the comment.
I agree to include a dedicated reconfirmation email to clarify the functionality of the reconfirmable, so I added an additional change.

@@ -117,8 +117,11 @@ def send_confirmation_instructions
generate_confirmation_token!
end

opts = pending_reconfirmation? ? { to: unconfirmed_email } : { }
send_devise_notification(:confirmation_instructions, @raw_confirmation_token, opts)
if pending_reconfirmation?

Choose a reason for hiding this comment

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

What do you think about change this ifto:

params = {}
params[:to] = unconformed_email if pending_reconfirmation?
send_devise_notification(:confirmation_instructions, @raw_confirmation_token, params)

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your review.
I think send_devise_notification 's first argument must use each for calling Devise::Mailer#confirmation_instructions, reconfirmation_instructions

@sowenjub
Copy link

Is there anything I can do to help get this branch merged?
I was about to start working on PR to tackle that same issue when I found this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants