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

@@ -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)

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="
 

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

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