diff --git a/.gitignore b/.gitignore index 93a5a3a05..b2b589114 100644 --- a/.gitignore +++ b/.gitignore @@ -42,6 +42,7 @@ yarn-debug.log* .yarn-integrity .env +.env.development # Code coverage results /coverage diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 5f619ad36..ac9a767f2 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -53,16 +53,21 @@ 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? + flash[:notice] = I18n.t("devise.email.updated", email: @user.unconfirmed_email) + end + redirect_to account_path else user_name = @user.name&.possessive || @user.email.possessive - case user_params[:active] - when "false" + if user_params[:active] == "false" @user.update!(confirmed_at: nil, sign_in_count: 0, initial_confirmation_sent: false) flash[:notice] = I18n.t("devise.activation.deactivated", user_name:) - when "true" + 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? + flash[:notice] = I18n.t("devise.email.updated", email: @user.unconfirmed_email) end redirect_to user_path(@user) end diff --git a/app/mailers/devise_notify_mailer.rb b/app/mailers/devise_notify_mailer.rb index 5560e7925..533bd76b6 100644 --- a/app/mailers/devise_notify_mailer.rb +++ b/app/mailers/devise_notify_mailer.rb @@ -35,12 +35,17 @@ class DeviseNotifyMailer < Devise::Mailer end def confirmation_instructions(record, token, _opts = {}) - username = record.email - if email_changed(record) - username = record.unconfirmed_email - send_confirmation_email(record.unconfirmed_email, record, token, username) + 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 + else + send_confirmation_email(record.email, record, token, record.email) end - send_confirmation_email(record.email, record, token, username) end def intercept_send?(email) @@ -54,10 +59,48 @@ class DeviseNotifyMailer < Devise::Mailer Rails.application.credentials[:email_allowlist] || [] end -private + def send_email_changed_to_old_email(record) + return true if intercept_send?(record.email) - def email_changed(record) - record.confirmable_template == User::CONFIRMABLE_TEMPLATE_ID && (record.unconfirmed_email.present? && record.unconfirmed_email != record.email) + send_email( + record.email, + User::FOR_OLD_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, + { + new_email: record.unconfirmed_email, + old_email: record.email, + }, + ) + end + + def send_email_changed_to_new_email(record, token) + return true if intercept_send?(record.email) + + link = "#{user_confirmation_url}?confirmation_token=#{token}" + + send_email( + record.unconfirmed_email, + User::FOR_NEW_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, + { + new_email: record.unconfirmed_email, + old_email: record.email, + link:, + }, + ) + end + + def email_changed?(record) + ( + 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? + ) + end + + def new_email_journey? + FeatureToggle.new_email_journey? end def send_confirmation_email(email, record, token, username) diff --git a/app/models/user.rb b/app/models/user.rb index 48177d635..eccdaec47 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -121,6 +121,8 @@ class User < ApplicationRecord RECONFIRMABLE_TEMPLATE_ID = "bcdec787-f0a7-46e9-8d63-b3e0a06ee455".freeze BETA_ONBOARDING_TEMPLATE_ID = "b48bc2cd-5887-4611-8296-d0ab3ed0e7fd".freeze USER_REACTIVATED_TEMPLATE_ID = "ac45a899-490e-4f59-ae8d-1256fc0001f9".freeze + FOR_OLD_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID = "3eb80517-1051-4dfc-b4cc-cb18228a3829".freeze + FOR_NEW_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID = "0cdd0be1-7fa5-4808-8225-ae4c5a002352".freeze def reset_password_notify_template RESET_PASSWORD_TEMPLATE_ID @@ -131,7 +133,7 @@ class User < ApplicationRecord USER_REACTIVATED_TEMPLATE_ID elsif was_migrated_from_softwire? && last_sign_in_at.blank? BETA_ONBOARDING_TEMPLATE_ID - elsif initial_confirmation_sent + elsif initial_confirmation_sent && !confirmed? RECONFIRMABLE_TEMPLATE_ID else CONFIRMABLE_TEMPLATE_ID diff --git a/app/services/feature_toggle.rb b/app/services/feature_toggle.rb index d7df829ad..aed14d3e6 100644 --- a/app/services/feature_toggle.rb +++ b/app/services/feature_toggle.rb @@ -33,4 +33,8 @@ 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/app/views/users/show.html.erb b/app/views/users/show.html.erb index 844a0c965..bcdfd02b2 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -111,12 +111,11 @@ <%= govuk_button_to "Resend invite link", resend_invite_user_path(@user), secondary: true %> <% end %> <% else %> - - This user has been deactivated. <%= govuk_button_link_to "Reactivate user", reactivate_user_path(@user) %> - + + This user has been deactivated. <%= govuk_button_link_to "Reactivate user", reactivate_user_path(@user) %> + <% end %> <% end %> - diff --git a/config/locales/en.yml b/config/locales/en.yml index fb29431e1..5b7654c12 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -684,6 +684,8 @@ Make sure these answers are correct." hint_text: "This is more than 5 times the income, which is higher than we would expect." devise: + email: + updated: An email has been sent to %{email} to confirm this change. two_factor_authentication: success: "Two-factor authentication successful" attempt_failed: "Attempt failed" diff --git a/spec/features/user_spec.rb b/spec/features/user_spec.rb index f568ef96b..e1cb65f66 100644 --- a/spec/features/user_spec.rb +++ b/spec/features/user_spec.rb @@ -529,7 +529,7 @@ RSpec.describe "User Features" do end before do - other_user.update!(initial_confirmation_sent: true) + other_user.update!(initial_confirmation_sent: true, confirmed_at: nil) allow(user).to receive(:need_two_factor_authentication?).and_return(false) sign_in(user) visit(user_path(user.id)) diff --git a/spec/mailers/resend_invitation_mailer_spec.rb b/spec/mailers/resend_invitation_mailer_spec.rb index 02a6189d4..a5eadad20 100644 --- a/spec/mailers/resend_invitation_mailer_spec.rb +++ b/spec/mailers/resend_invitation_mailer_spec.rb @@ -57,9 +57,31 @@ RSpec.describe ResendInvitationMailer do end it "sends a reinvitation" do - expect(notify_client).to receive(:send_email).with(email_address: "active_user@example.com", template_id: User::RECONFIRMABLE_TEMPLATE_ID, personalisation:).once + expect(notify_client).to receive(:send_email).with(email_address: "active_user@example.com", template_id: User::CONFIRMABLE_TEMPLATE_ID, personalisation:).once described_class.new.resend_invitation_email(active_user) end end + + context "with unconfirmed user after the initial invitation has been sent" do + let!(:unconfirmed_user) { create(:user, organisation:, confirmation_token: "dluch", initial_confirmation_sent: true, old_user_id: "234", sign_in_count: 0, confirmed_at: nil) } + + let(:personalisation) do + { + name: unconfirmed_user.name, + email: unconfirmed_user.email, + organisation: unconfirmed_user.organisation.name, + link: include("/account/confirmation?confirmation_token=#{unconfirmed_user.confirmation_token}"), + } + end + + before do + LegacyUser.destroy_all + end + + it "sends a reinvitation" do + expect(notify_client).to receive(:send_email).with(email_address: unconfirmed_user.email, template_id: User::RECONFIRMABLE_TEMPLATE_ID, personalisation:).once + described_class.new.resend_invitation_email(unconfirmed_user) + end + end end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 984090863..ec2e0dde8 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -1538,7 +1538,7 @@ RSpec.describe UsersController, type: :request do expect(whodunnit_actor.id).to eq(user.id) end - context "when user changes email, dpo and key contact" do + context "when user changes email, dpo and key contact", :aggregate_failures do let(:params) { { id: user.id, user: { name: new_name, email: new_email, is_dpo: "true", is_key_contact: "true" } } } let(:personalisation) do { @@ -1551,6 +1551,8 @@ 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 @@ -1566,6 +1568,43 @@ RSpec.describe UsersController, type: :request do expect(notify_client).to receive(:send_email).with(email_address: user.email, template_id: User::CONFIRMABLE_TEMPLATE_ID, personalisation:).once request end + + context "with new email journy 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 context "when we update the user password" do @@ -1679,13 +1718,28 @@ RSpec.describe UsersController, type: :request do expect(page).to have_content(other_user.reload.email.to_s) end - context "when the support user tries to update the user’s password" do + context "when the support user tries to update the user’s password", :aggregate_failures do let(:params) do { - id: user.id, user: { password: new_name, password_confirmation: new_name, name: "new name" } + id: user.id, user: { password: new_name, password_confirmation: new_name, name: "new name", email: new_email } } end + let(:personalisation) do + { + name: params[:user][:name], + email: new_email, + organisation: other_user.organisation.name, + link: include("/account/confirmation?confirmation_token="), + } + end + + 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) @@ -1695,6 +1749,57 @@ RSpec.describe UsersController, type: :request do expect { patch "/users/#{other_user.id}", headers:, params: } .to change { other_user.reload.name }.from("Danny Rojas").to("new name") 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 + + 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