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

Use a service class to send confirmation emails #2948

Merged
merged 8 commits into from
May 16, 2019
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/forms/register_user_email_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def valid_form?
def process_successful_submission(request_id, instructions)
@success = true
user.save!
user.send_custom_confirmation_instructions(request_id, instructions)
SendSignUpEmailConfirmation.new(user).call(request_id: request_id, instructions: instructions)
end

def extra_analytics_attributes
Expand All @@ -62,7 +62,7 @@ def process_errors(request_id)
# To prevent discovery of existing emails, we check to see if the email is
# already taken and if so, we act as if the user registration was successful.
if email_taken? && user_unconfirmed?
existing_user.send_custom_confirmation_instructions(request_id)
SendSignUpEmailConfirmation.new(existing_user).call(request_id: request_id)
true
elsif email_taken?
UserMailer.signup_with_your_email(email).deliver_later
Expand Down
2 changes: 1 addition & 1 deletion app/forms/resend_email_confirmation_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def user
def send_confirmation_email_if_necessary
return unless valid? && user.persisted? && !user.confirmed?

user.send_custom_confirmation_instructions(request_id)
SendSignUpEmailConfirmation.new(user).call(request_id: request_id)
end

def extra_analytics_attributes
Expand Down
2 changes: 1 addition & 1 deletion app/forms/update_user_email_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,6 @@ def extra_analytics_attributes

def update_user_email
UpdateUser.new(user: @user, attributes: { email: email }).call
@user.send_custom_confirmation_instructions
SendSignUpEmailConfirmation.new(user).call
end
end
9 changes: 0 additions & 9 deletions app/mailers/custom_devise_mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,4 @@ def reset_password_instructions(*)
@locale = locale_url_param
super
end

def confirmation_instructions(record, token, options = {})
presenter = ConfirmationEmailPresenter.new(record, view_context)
@first_sentence = options[:first_sentence] || presenter.first_sentence
@confirmation_period = presenter.confirmation_period
@request_id = options[:request_id]
@locale = locale_url_param
super
end
end
13 changes: 13 additions & 0 deletions app/mailers/user_mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,19 @@ def email_changed(old_email)
mail(to: old_email, subject: t('mailer.email_change_notice.subject'))
end

# :reek:ControlParameter
# :reek:LongParameterList
# :reek:TooManyStatements
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you want to leave in the 3 lines above. I am pointing this out in case you intended to remove them later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I meant to disable these. Normally I'd add a method for consuming the parameters, but this class is already pretty fat and I'd be worried about that getting lost in the concerns it has for the other mails it sends.

def email_confirmation_instructions(user, email, token, request_id:, instructions:)
presenter = ConfirmationEmailPresenter.new(user, view_context)
@first_sentence = instructions || presenter.first_sentence
@confirmation_period = presenter.confirmation_period
@request_id = request_id
@locale = locale_url_param
@token = token
mail(to: email, subject: t('user_mailer.email_confirmation_instructions.subject'))
end

def signup_with_your_email(email)
@root_url = root_url(locale: locale_url_param)
mail(to: email, subject: t('mailer.email_reuse_notice.subject'))
Expand Down
11 changes: 2 additions & 9 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ class User < ApplicationRecord

validates :x509_dn_uuid, uniqueness: true, allow_nil: true

skip_callback :create, :before, :generate_confirmation_token

attr_accessor :asserted_attributes

def confirmed_email_addresses
Expand Down Expand Up @@ -133,13 +135,4 @@ def strip_whitespace
def send_confirmation_instructions
# no-op
end

def send_custom_confirmation_instructions(id = nil, instructions = nil)
generate_confirmation_token! unless @raw_confirmation_token

opts = pending_reconfirmation? ? { to: unconfirmed_email, request_id: id } : { request_id: id }
opts[:first_sentence] = instructions if instructions
send_devise_notification(:confirmation_instructions,
@raw_confirmation_token, opts)
end
end
4 changes: 2 additions & 2 deletions app/presenters/confirmation_email_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ def initialize(user, view)
def first_sentence # rubocop:disable MethodLength
if user.confirmed_at.present?
I18n.t(
'mailer.confirmation_instructions.first_sentence.confirmed',
'user_mailer.email_confirmation_instructions.first_sentence.confirmed',
app: app_link, confirmation_period: confirmation_period,
)
else
I18n.t(
'mailer.confirmation_instructions.first_sentence.unconfirmed',
'user_mailer.email_confirmation_instructions.first_sentence.unconfirmed',
app: app_link, confirmation_period: confirmation_period,
)
end
Expand Down
2 changes: 1 addition & 1 deletion app/services/request_password_reset.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def perform
private

def instructions
I18n.t('mailer.confirmation_instructions.first_sentence.forgot_password')
I18n.t('user_mailer.email_confirmation_instructions.first_sentence.forgot_password')
end

def user_not_found?
Expand Down
69 changes: 69 additions & 0 deletions app/services/send_sign_up_email_confirmation.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
class SendSignUpEmailConfirmation
attr_reader :user

def initialize(user)
@user = user
end

def call(request_id: nil, instructions: nil)
update_user_record
update_email_address_record
send_confirmation_email(request_id, instructions)
end

private

def confirmation_token
return email_address.confirmation_token if valid_confirmation_token_exists?
@token ||= Devise.friendly_token
end

def confirmation_sent_at
return email_address.confirmation_sent_at if valid_confirmation_token_exists?
@confirmation_sent_at ||= Time.zone.now
end

def confirmation_period_expired?
@confirmation_period_expired ||= user.confirmation_period_expired?
end

# :reek:DuplicateMethodCall
def email_address
@email_address ||= begin
raise handle_multiple_email_address_error if user.email_addresses.count > 1
Copy link
Member Author

Choose a reason for hiding this comment

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

The raise here is redundant

user.email_addresses.take
end
end

def update_user_record
user.update!(
confirmation_token: confirmation_token,
confirmation_sent_at: confirmation_sent_at,
)
end

def valid_confirmation_token_exists?
email_address.confirmation_token.present? && !confirmation_period_expired?
end

def update_email_address_record
email_address.update!(
confirmation_token: confirmation_token,
confirmation_sent_at: confirmation_sent_at,
)
end

def send_confirmation_email(request_id, instructions)
UserMailer.email_confirmation_instructions(
user,
email_address.email,
confirmation_token,
request_id: request_id,
instructions: instructions,
).deliver_later
end

def handle_multiple_email_address_error
raise 'sign up user has multiple email address records'
end
end
2 changes: 1 addition & 1 deletion app/views/user_mailer/account_reset_granted.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ table.hr
tr
th
| &nbsp;
p = t('mailer.confirmation_instructions.footer', confirmation_period: '24 hours')
p = t('user_mailer.email_confirmation_instructions.footer', confirmation_period: '24 hours')
p == t('user_mailer.account_reset_granted.help',
cancel_account_reset: link_to(t('user_mailer.account_reset_granted.cancel_link_text'),
account_reset_cancel_url(token: @token)))
2 changes: 1 addition & 1 deletion app/views/user_mailer/confirm_email_and_reverify.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ table.button.expanded.large.radius
td
center
center
= link_to t('mailer.confirmation_instructions.link_text'), \
= link_to t('user_mailer.email_confirmation_instructions.link_text'), \
idv_recovery_step_url(step: :recover, token: @token, locale: @locale), \
target: '_blank', \
class: 'float-center', align: 'center'
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
p.lead == t('mailer.confirmation_instructions.header', intro: @first_sentence) + ' ' + \
t('mailer.confirmation_instructions.footer', confirmation_period: @confirmation_period)
p.lead == t('user_mailer.email_confirmation_instructions.header', intro: @first_sentence) + ' ' + \
t('user_mailer.email_confirmation_instructions.footer', confirmation_period: @confirmation_period)

table.button.expanded.large.radius
tbody
Expand All @@ -10,7 +10,7 @@ table.button.expanded.large.radius
tr
td
center
= link_to t('mailer.confirmation_instructions.link_text'), \
= link_to t('user_mailer.email_confirmation_instructions.link_text'), \
sign_up_create_email_confirmation_url(_request_id: \
@request_id, confirmation_token: @token, locale: @locale), \
target: '_blank', \
Expand Down
2 changes: 1 addition & 1 deletion config/i18n-tasks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ ignore_missing:
## any unused translations would fail erroneously.
ignore_unused:
- 'devise.failure.*'
- 'devise.mailer.confirmation_instructions.subject'
- 'user_mailer.email_confirmation_instructions.subject'
- 'devise.mailer.reset_password_instructions.subject'
- 'devise.sessions.signed_in'
- 'datetime.dotiw.two_words_connector'
Expand Down
6 changes: 5 additions & 1 deletion config/initializers/new_relic_tracers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,12 @@
User.class_eval do
include ::NewRelic::Agent::MethodTracer
add_method_tracer :send_devise_notification, "Custom/#{name}/send_devise_notification"
end

SendSignUpEmailConfirmation.class_eval do
include ::NewRelic::Agent::MethodTracer
add_method_tracer(
:send_custom_confirmation_instructions, "Custom/#{name}/send_custom_confirmation_instructions"
:call, "Custom/#{name}/call"
)
end

Expand Down
2 changes: 0 additions & 2 deletions config/locales/devise/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ en:
unauthenticated: Your session has expired. Please sign in again to continue.
unconfirmed: You need to confirm your email address before continuing.
mailer:
confirmation_instructions:
subject: Confirm your email
password_updated:
subject: Your password has been changed
reset_password_instructions:
Expand Down
2 changes: 0 additions & 2 deletions config/locales/devise/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ es:
unauthenticated: Su sesión ha caducado. Vuelva a iniciar la sesión para continuar.
unconfirmed: Debe confirmar su email antes de continuar.
mailer:
confirmation_instructions:
subject: Confirme su email
password_updated:
subject: Su contraseña ha sido cambiada.
reset_password_instructions:
Expand Down
2 changes: 0 additions & 2 deletions config/locales/devise/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@ fr:
pour continuer.
unconfirmed: Vous devez confirmer votre adresse courriel avant de continuer.
mailer:
confirmation_instructions:
subject: Confirmez votre adresse courriel
password_updated:
subject: Votre mot de passe a été modifié
reset_password_instructions:
Expand Down
11 changes: 0 additions & 11 deletions config/locales/mailer/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,6 @@
en:
mailer:
about: About %{app}
confirmation_instructions:
first_sentence:
confirmed: Trying to change your email address?
forgot_password: You tried to reset your login.gov password but we don't have
an account linked to this email address. If you'd like to set up a new
account linked to this email, confirm your email address below.
unconfirmed: Thanks for creating an account.
footer: This link will expire in %{confirmation_period}.
header: "%{intro} Please click the link below or copy and paste the entire link
into your browser."
link_text: Confirm email address
email_change_notice:
subject: Change your email address
email_reuse_notice:
Expand Down
12 changes: 0 additions & 12 deletions config/locales/mailer/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,6 @@
es:
mailer:
about: Acerca de %{app}
confirmation_instructions:
first_sentence:
confirmed: "¿Desea cambiar su email?"
forgot_password: Intentó restablecer su contraseña de login.gov pero no tenemos
una cuenta vinculada a esta dirección de correo electrónico. Si desea configurar
una nueva cuenta vinculada a este correo electrónico, confirme su dirección
de correo electrónico a continuación.
unconfirmed: Gracias por crear una cuenta.
footer: Este enlace expira en %{confirmation_period}.
header: "%{intro} Haga clic en el enlace de abajo o copie y pegue el enlace
completo en su navegador."
link_text: Confirmar el correo
email_change_notice:
subject: Cambie su email
email_reuse_notice:
Expand Down
12 changes: 0 additions & 12 deletions config/locales/mailer/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,6 @@
fr:
mailer:
about: À propos de %{app}
confirmation_instructions:
first_sentence:
confirmed: Vous tentez de changer votre adresse courriel?
forgot_password: Vous avez essayé de réinitialiser le mot de passe de votre
compte login.gov, mais nous ne possédons pas de compte associé à cette adresse
courriel. Si vous souhaitez créer un nouveau compte associé à cette adresse
courriel, confirmez votre adresse courriel ci-dessous.
unconfirmed: Merci d'avoir créé un compte.
footer: Ce lien expirera dans %{confirmation_period}.
header: "%{intro} Veuillez cliquer sur le lien ci-dessous ou copier et coller
le lien complet dans votre navigateur."
link_text: Confirmez votre adresse email
email_change_notice:
subject: Changez votre adresse courriel
email_reuse_notice:
Expand Down
12 changes: 12 additions & 0 deletions config/locales/user_mailer/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,18 @@ en:
help: If you did not want to change your email address, please visit the %{app}
%{help_link} or %{contact_link}.
intro: The email address for your %{app} account has been changed.
email_confirmation_instructions:
first_sentence:
confirmed: Trying to change your email address?
forgot_password: You tried to reset your login.gov password but we don't have
an account linked to this email address. If you'd like to set up a new
account linked to this email, confirm your email address below.
unconfirmed: Thanks for creating an account.
footer: This link will expire in %{confirmation_period}.
header: "%{intro} Please click the link below or copy and paste the entire link
into your browser."
link_text: Confirm email address
subject: Confirm your email
help_link_text: Help Center
letter_expired:
help: ''
Expand Down
13 changes: 13 additions & 0 deletions config/locales/user_mailer/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,19 @@ es:
email_changed:
help: Si no desea cambiar su email, visite el %{app} %{help_link} o el %{contact_link}.
intro: El email de su cuenta de %{app} ha sido cambiado.
email_confirmation_instructions:
first_sentence:
confirmed: "¿Desea cambiar su email?"
forgot_password: Intentó restablecer su contraseña de login.gov pero no tenemos
una cuenta vinculada a esta dirección de correo electrónico. Si desea configurar
una nueva cuenta vinculada a este correo electrónico, confirme su dirección
de correo electrónico a continuación.
unconfirmed: Gracias por crear una cuenta.
footer: Este enlace expira en %{confirmation_period}.
header: "%{intro} Haga clic en el enlace de abajo o copie y pegue el enlace
completo en su navegador."
link_text: Confirmar el correo
subject: Confirme su email
help_link_text: Centro de Ayuda
letter_expired:
help: ''
Expand Down
13 changes: 13 additions & 0 deletions config/locales/user_mailer/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,19 @@ fr:
help: Si vous préférez ne pas changer votre adresse courriel, veuillez visiter
le %{help_link} de %{app} ou %{contact_link}.
intro: L'adresse courriel de votre compte %{app} a été changée.
email_confirmation_instructions:
first_sentence:
confirmed: Vous tentez de changer votre adresse courriel?
forgot_password: Vous avez essayé de réinitialiser le mot de passe de votre
compte login.gov, mais nous ne possédons pas de compte associé à cette adresse
courriel. Si vous souhaitez créer un nouveau compte associé à cette adresse
courriel, confirmez votre adresse courriel ci-dessous.
unconfirmed: Merci d'avoir créé un compte.
footer: Ce lien expirera dans %{confirmation_period}.
header: "%{intro} Veuillez cliquer sur le lien ci-dessous ou copier et coller
le lien complet dans votre navigateur."
link_text: Confirmez votre adresse email
subject: Confirmez votre adresse courriel
help_link_text: Centre d'aide
letter_expired:
help: ''
Expand Down
2 changes: 1 addition & 1 deletion spec/controllers/users/emails_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@

expect(response).to redirect_to account_url
expect(flash[:notice]).to eq t('devise.registrations.email_update_needs_confirmation')
expect(response).to render_template('devise/mailer/confirmation_instructions')
expect(response).to render_template('user_mailer/email_confirmation_instructions')
expect(user.reload.email).to eq '[email protected]'
expect(@analytics).to have_received(:track_event).
with(Analytics::EMAIL_CHANGE_REQUEST, analytics_hash)
Expand Down
Loading