From b6f13be12fae2789e78909721ce7555d21f5b838 Mon Sep 17 00:00:00 2001 From: Jack S Date: Tue, 15 Aug 2023 09:44:12 +0100 Subject: [PATCH] Remove feature flag for new email journey --- app/controllers/users_controller.rb | 4 +- app/mailers/devise_notify_mailer.rb | 20 +-- app/services/feature_toggle.rb | 4 - spec/requests/users_controller_spec.rb | 180 +++++++++---------------- 4 files changed, 70 insertions(+), 138 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 37506ef7f..b235b527b 100644 --- a/app/mailers/devise_notify_mailer.rb +++ b/app/mailers/devise_notify_mailer.rb @@ -36,14 +36,9 @@ 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 - elsif !record.confirmed? && record.unconfirmed_email && new_email_journey? + 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 @@ -96,16 +91,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 82d01852c..9db4d03e4 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -1551,27 +1551,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 @@ -1582,21 +1601,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 @@ -1604,48 +1624,6 @@ RSpec.describe UsersController, type: :request do patch "/users/#{other_user.id}", headers:, params: end - - 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 - other_user.legacy_users.destroy_all - 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: new_email, - template_id: User::RECONFIRMABLE_TEMPLATE_ID, - personalisation: { - 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: old_email, - template_id: User::FOR_OLD_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, - personalisation: { - new_email:, - old_email:, - }, - ).once - - expect(notify_client).not_to receive(:send_email) - - patch "/users/#{other_user.id}", headers:, params: - end - end end end @@ -1778,70 +1756,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 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 - 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 + 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