From 16027298909ab2f5b557aa7efe77431107b673d2 Mon Sep 17 00:00:00 2001 From: Jack <113976590+bibblobcode@users.noreply.github.com> Date: Wed, 16 Aug 2023 07:29:41 +0100 Subject: [PATCH] New email journey for unconfirmed users (#1835) * New emal journey for unconfirmed users * Remove feature flag for new email journey --- app/controllers/users_controller.rb | 4 +- app/mailers/devise_notify_mailer.rb | 21 ++-- app/services/feature_toggle.rb | 4 - spec/requests/users_controller_spec.rb | 140 +++++++++++-------------- 4 files changed, 73 insertions(+), 96 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index ac9a767f2..f9a86055d 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -53,7 +53,7 @@ class UsersController < ApplicationController if @user == current_user bypass_sign_in @user flash[:notice] = I18n.t("devise.passwords.updated") if user_params.key?("password") - if user_params.key?("email") && FeatureToggle.new_email_journey? + if user_params.key?("email") flash[:notice] = I18n.t("devise.email.updated", email: @user.unconfirmed_email) end @@ -66,7 +66,7 @@ class UsersController < ApplicationController elsif user_params[:active] == "true" @user.send_confirmation_instructions flash[:notice] = I18n.t("devise.activation.reactivated", user_name:) - elsif user_params.key?("email") && FeatureToggle.new_email_journey? + elsif user_params.key?("email") flash[:notice] = I18n.t("devise.email.updated", email: @user.unconfirmed_email) end redirect_to user_path(@user) diff --git a/app/mailers/devise_notify_mailer.rb b/app/mailers/devise_notify_mailer.rb index f4bde0fd9..5ee3fd898 100644 --- a/app/mailers/devise_notify_mailer.rb +++ b/app/mailers/devise_notify_mailer.rb @@ -40,13 +40,11 @@ class DeviseNotifyMailer < Devise::Mailer def confirmation_instructions(record, token, _opts = {}) if email_changed?(record) - if new_email_journey? - send_email_changed_to_old_email(record) - send_email_changed_to_new_email(record, token) - else - send_confirmation_email(record.unconfirmed_email, record, token, record.unconfirmed_email) - send_confirmation_email(record.email, record, token, record.unconfirmed_email) - end + send_email_changed_to_old_email(record) + send_email_changed_to_new_email(record, token) + elsif !record.confirmed? && record.unconfirmed_email + send_confirmation_email(record.unconfirmed_email, record, token, record.unconfirmed_email) + send_email_changed_to_old_email(record) else send_confirmation_email(record.email, record, token, record.email) end @@ -97,16 +95,11 @@ class DeviseNotifyMailer < Devise::Mailer record.confirmable_template == User::CONFIRMABLE_TEMPLATE_ID && ( record.unconfirmed_email.present? && record.unconfirmed_email != record.email) ) || ( - new_email_journey? && - record.versions.last.changeset.key?("unconfirmed_email") && - record.confirmed? + record.versions.last.changeset.key?("unconfirmed_email") && + record.confirmed? ) end - def new_email_journey? - FeatureToggle.new_email_journey? - end - def send_confirmation_email(email, record, token, username) url = "#{user_confirmation_url}?confirmation_token=" diff --git a/app/services/feature_toggle.rb b/app/services/feature_toggle.rb index aed14d3e6..d7df829ad 100644 --- a/app/services/feature_toggle.rb +++ b/app/services/feature_toggle.rb @@ -33,8 +33,4 @@ class FeatureToggle def self.deduplication_flow_enabled? !Rails.env.production? && !Rails.env.staging? end - - def self.new_email_journey? - !Rails.env.production? - end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 1b62900fb..f35be6b0e 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -1584,27 +1584,46 @@ RSpec.describe UsersController, type: :request do before do user.legacy_users.destroy_all - - allow(FeatureToggle).to receive(:new_email_journey?).and_return(false) end - it "allows changing email and dpo" do - request - user.reload - expect(user.unconfirmed_email).to eq(new_email) - expect(user.is_data_protection_officer?).to be true - expect(user.is_key_contact?).to be true + it "shows flash notice" do + patch("/users/#{other_user.id}", headers:, params:) + + expect(flash[:notice]).to eq("An email has been sent to #{new_email} to confirm this change.") end - it "sends a confirmation email to both emails" do - expect(notify_client).to receive(:send_email).with(email_address: new_email, template_id: User::CONFIRMABLE_TEMPLATE_ID, personalisation:).once - expect(notify_client).to receive(:send_email).with(email_address: user.email, template_id: User::CONFIRMABLE_TEMPLATE_ID, personalisation:).once - request + it "sends new flow emails" do + expect(notify_client).to receive(:send_email).with( + email_address: other_user.email, + template_id: User::FOR_OLD_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, + personalisation: { + new_email:, + old_email: other_user.email, + }, + ).once + + expect(notify_client).to receive(:send_email).with( + email_address: new_email, + template_id: User::FOR_NEW_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, + personalisation: { + new_email:, + old_email: other_user.email, + link: include("/account/confirmation?confirmation_token="), + }, + ).once + + expect(notify_client).not_to receive(:send_email) + + patch "/users/#{other_user.id}", headers:, params: end - context "with new email journy enabled" do + context "when user has never confirmed email address" do + let(:old_email) { "old@test.com" } + let(:new_email) { "new@test.com" } + let(:other_user) { create(:user, organisation: user.organisation, email: old_email, confirmed_at: nil) } + before do - allow(FeatureToggle).to receive(:new_email_journey?).and_return(true) + other_user.legacy_users.destroy_all end it "shows flash notice" do @@ -1615,21 +1634,22 @@ RSpec.describe UsersController, type: :request do it "sends new flow emails" do expect(notify_client).to receive(:send_email).with( - email_address: other_user.email, - template_id: User::FOR_OLD_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, + email_address: new_email, + template_id: User::RECONFIRMABLE_TEMPLATE_ID, personalisation: { - new_email:, - old_email: other_user.email, + name: new_name, + email: new_email, + organisation: other_user.organisation.name, + link: include("/account/confirmation?confirmation_token="), }, ).once expect(notify_client).to receive(:send_email).with( - email_address: new_email, - template_id: User::FOR_NEW_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, + email_address: old_email, + template_id: User::FOR_OLD_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, personalisation: { new_email:, - old_email: other_user.email, - link: include("/account/confirmation?confirmation_token="), + old_email:, }, ).once @@ -1769,70 +1789,38 @@ RSpec.describe UsersController, type: :request do before do other_user.legacy_users.destroy_all - - allow(FeatureToggle).to receive(:new_email_journey?).and_return(false) end - it "does not update the password" do - expect { patch "/users/#{other_user.id}", headers:, params: } - .not_to change(other_user, :encrypted_password) - end + it "shows flash notice" do + patch("/users/#{other_user.id}", headers:, params:) - it "does update other values" do - expect { patch "/users/#{other_user.id}", headers:, params: } - .to change { other_user.reload.name }.from("Danny Rojas").to("new name") + expect(flash[:notice]).to eq("An email has been sent to #{new_email} to confirm this change.") end - it "allows changing email" do - expect { patch "/users/#{other_user.id}", headers:, params: } - .to change { other_user.reload.unconfirmed_email }.from(nil).to(new_email) - end - - it "sends a confirmation email to both emails" do - expect(notify_client).to receive(:send_email).with(email_address: other_user.email, template_id: User::CONFIRMABLE_TEMPLATE_ID, personalisation:).once - expect(notify_client).to receive(:send_email).with(email_address: new_email, template_id: User::CONFIRMABLE_TEMPLATE_ID, personalisation:).once + it "sends new flow emails" do + expect(notify_client).to receive(:send_email).with( + email_address: other_user.email, + template_id: User::FOR_OLD_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, + personalisation: { + new_email:, + old_email: other_user.email, + }, + ).once + + expect(notify_client).to receive(:send_email).with( + email_address: new_email, + template_id: User::FOR_NEW_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, + personalisation: { + new_email:, + old_email: other_user.email, + link: include("/account/confirmation?confirmation_token="), + }, + ).once expect(notify_client).not_to receive(:send_email) patch "/users/#{other_user.id}", headers:, params: end - - context "with new user email flow enabled" do - before do - allow(FeatureToggle).to receive(:new_email_journey?).and_return(true) - end - - it "shows flash notice" do - patch("/users/#{other_user.id}", headers:, params:) - - expect(flash[:notice]).to eq("An email has been sent to #{new_email} to confirm this change.") - end - - it "sends new flow emails" do - expect(notify_client).to receive(:send_email).with( - email_address: other_user.email, - template_id: User::FOR_OLD_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, - personalisation: { - new_email:, - old_email: other_user.email, - }, - ).once - - expect(notify_client).to receive(:send_email).with( - email_address: new_email, - template_id: User::FOR_NEW_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, - personalisation: { - new_email:, - old_email: other_user.email, - link: include("/account/confirmation?confirmation_token="), - }, - ).once - - expect(notify_client).not_to receive(:send_email) - - patch "/users/#{other_user.id}", headers:, params: - end - end end end end