diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index c60c3cb39..7610e7efb 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -163,6 +163,20 @@ class UsersController < ApplicationController end end + def update_log_reassignment + return redirect_to user_path(@user) unless log_reassignment_params[:organisation_id].present? && Organisation.where(id: log_reassignment_params[:organisation_id]).exists? + + @new_organisation = Organisation.find(log_reassignment_params[:organisation_id]) + + validate_log_reassignment + + if @user.errors.empty? + redirect_to user_confirm_organisation_change_path(@user, log_reassignment_params) + else + render :log_reassignment, status: :unprocessable_entity + end + end + private def validate_attributes @@ -232,6 +246,10 @@ private user_params.except(:organisation_id) end + def log_reassignment_params + params.require(:user).permit(:log_reassignment, :organisation_id) + end + def created_user_redirect_path if current_user.support? users_path @@ -267,4 +285,31 @@ private def updating_organisation? user_params["organisation_id"].present? && @user.organisation_id != user_params["organisation_id"].to_i end + + def validate_log_reassignment + return @user.errors.add :log_reassignment, :blank if log_reassignment_params[:log_reassignment].blank? + + case log_reassignment_params[:log_reassignment] + when "reassign_stock_owner" + required_managing_agents = (@user.assigned_to_lettings_logs.map(&:managing_organisation) + @user.assigned_to_sales_logs.map(&:managing_organisation)).uniq + current_managing_agents = @new_organisation.managing_agents + missing_managing_agents = required_managing_agents - current_managing_agents + + if missing_managing_agents.any? + new_organisation = @new_organisation.name + missing_managing_agents = missing_managing_agents.map(&:name).sort.to_sentence + @user.errors.add :log_reassignment, I18n.t("activerecord.errors.models.user.attributes.log_reassignment.missing_managing_agents", new_organisation:, missing_managing_agents:) + end + when "reassign_managing_agent" + required_stock_owners = (@user.assigned_to_lettings_logs.map(&:owning_organisation) + @user.assigned_to_sales_logs.map(&:owning_organisation)).uniq + current_stock_owners = @new_organisation.stock_owners + missing_stock_owners = required_stock_owners - current_stock_owners + + if missing_stock_owners.any? + new_organisation = @new_organisation.name + missing_stock_owners = missing_stock_owners.map(&:name).sort.to_sentence + @user.errors.add :log_reassignment, I18n.t("activerecord.errors.models.user.attributes.log_reassignment.missing_stock_owners", new_organisation:, missing_stock_owners:) + end + end + end end diff --git a/app/views/users/log_reassignment.html.erb b/app/views/users/log_reassignment.html.erb index 356368022..aef6d3f22 100644 --- a/app/views/users/log_reassignment.html.erb +++ b/app/views/users/log_reassignment.html.erb @@ -4,7 +4,7 @@ <%= govuk_back_link(href: aliased_user_edit(@user, current_user)) %> <% end %> -<%= form_for(@user, html: { method: :patch }) do |f| %> +<%= form_with model: @user, method: :patch, url: user_log_reassignment_path(@user) do |f| %>
<%= f.govuk_error_summary %> @@ -25,6 +25,8 @@ :name, legend: { text: "Log reassignment", hidden: true } %> + <%= f.hidden_field :organisation_id, value: @new_organisation.id %> +
<%= f.govuk_submit "Continue" %> <%= govuk_button_link_to "Cancel", aliased_user_edit(@user, current_user), secondary: true %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 7d52a92e0..42ef43be1 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -172,6 +172,10 @@ en: too_short: The password you entered is too short. Enter a password that is %{count} characters or longer. reset_password_token: invalid: "That link is invalid. Check you are using the correct link." + log_reassignment: + blank: "Select if you want to reassign logs" + missing_managing_agents: "%{new_organisation} must be a stock owner of %{missing_managing_agents} to make this change." + missing_stock_owners: "%{new_organisation} must be a managing agent of %{missing_stock_owners} to make this change." merge_request: attributes: absorbing_organisation_id: diff --git a/config/routes.rb b/config/routes.rb index 483303476..d30d1aa88 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -125,6 +125,8 @@ Rails.application.routes.draw do get "edit-dpo", to: "users#dpo" get "edit-key-contact", to: "users#key_contact" get "log-reassignment", to: "users#log_reassignment" + patch "log-reassignment", to: "users#update_log_reassignment" + patch "confirm-organisation-change", to: "users#confirm-organisation-change" collection do get :search diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 324349919..57796f9b0 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -1956,121 +1956,236 @@ RSpec.describe UsersController, type: :request do end end - context "when the current user does not match the user ID" do - context "when the user is not part of the same organisation as the current user" do - let(:other_user) { create(:user) } - let(:params) { { id: other_user.id, user: { name: new_name } } } + context "when the user is not part of the same organisation as the current user" do + let(:other_user) { create(:user) } + let(:params) { { id: other_user.id, user: { name: new_name } } } - it "updates the user" do - expect { patch "/users/#{other_user.id}", headers:, params: } - .to change { other_user.reload.name }.from(other_user.name).to(new_name) + it "updates the user" do + expect { patch "/users/#{other_user.id}", headers:, params: } + .to change { other_user.reload.name }.from(other_user.name).to(new_name) + end + + it "tracks who updated the record" do + expect { patch "/users/#{other_user.id}", headers:, params: } + .to change { other_user.reload.versions.last.actor&.id }.from(nil).to(user.id) + end + + context "when user changes email, dpo, key_contact" do + let(:params) { { id: other_user.id, user: { name: new_name, email: new_email, is_dpo: "true", is_key_contact: "true" } } } + + it "allows changing email, dpo, key_contact" do + patch "/users/#{other_user.id}", headers: headers, params: params + other_user.reload + expect(other_user.unconfirmed_email).to eq(new_email) + expect(other_user.is_data_protection_officer?).to be true + expect(other_user.is_key_contact?).to be true end + end - it "tracks who updated the record" do - expect { patch "/users/#{other_user.id}", headers:, params: } - .to change { other_user.reload.versions.last.actor&.id }.from(nil).to(user.id) + it "does not bypass sign in for the support user" do + patch "/users/#{other_user.id}", headers: headers, params: params + follow_redirect! + expect(page).to have_content("#{other_user.reload.name}’s account") + expect(page).to have_content(other_user.reload.email.to_s) + end + + 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", email: new_email } + } end - context "when user changes email, dpo, key_contact" do - let(:params) { { id: other_user.id, user: { name: new_name, email: new_email, is_dpo: "true", is_key_contact: "true" } } } + let(:personalisation) do + { + name: params[:user][:name], + email: new_email, + organisation: other_user.organisation.name, + link: include("/account/confirmation?confirmation_token="), + } + end - it "allows changing email, dpo, key_contact" do - patch "/users/#{other_user.id}", headers: headers, params: params - other_user.reload - expect(other_user.unconfirmed_email).to eq(new_email) - expect(other_user.is_data_protection_officer?).to be true - expect(other_user.is_key_contact?).to be true - end + before do + other_user.legacy_users.destroy_all end - it "does not bypass sign in for the support user" do - patch "/users/#{other_user.id}", headers: headers, params: params - follow_redirect! - expect(page).to have_content("#{other_user.reload.name}’s account") - expect(page).to have_content(other_user.reload.email.to_s) + 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 - 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", email: 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 - let(:personalisation) do - { - name: params[:user][:name], - email: new_email, - organisation: other_user.organisation.name, + 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 + + context "when updating organisation" do + let(:new_organisation) { create(:organisation) } + + before do + patch "/users/#{other_user.id}", headers:, params: + end + + context "and organisation id is nil" do + let(:params) { { id: other_user.id, user: { organisation_id: "" } } } + + it "does not update the organisation" do + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_selector(".govuk-error-summary__title") end + end + + context "and organisation id is not nil" do + let(:params) { { id: other_user.id, user: { organisation_id: new_organisation.id, name: "new_name" } } } + + it "does not update the organisation" do + expect(user.reload.organisation).not_to eq(new_organisation) + end + + it "redirects to log reassignment page" do + expect(response).to redirect_to("/users/#{other_user.id}/log-reassignment?organisation_id=#{new_organisation.id}") + end + + it "updated other fields" do + expect(other_user.reload.name).to eq("new_name") + end + end + end + + context "when updating log reassignment" do + let(:new_organisation) { create(:organisation, name: "New org") } + let(:new_organisation_2) { create(:organisation, name: "New org 2") } + let(:new_organisation_3) { create(:organisation, name: "New org 3") } + + context "and log reassignment choice is not present" do + let(:params) { { user: { organisation_id: new_organisation.id, log_reassignment: nil } } } before do - other_user.legacy_users.destroy_all + patch "/users/#{other_user.id}/log-reassignment", headers:, params: + end + + it "does not update the user's organisation" do + expect(other_user.reload.organisation).not_to eq(new_organisation) end - it "shows flash notice" do - patch("/users/#{other_user.id}", headers:, params:) + it "displays the error message" do + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content("Select if you want to reassign logs") + end + end - expect(flash[:notice]).to eq("An email has been sent to #{new_email} to confirm this change.") + context "and log reassignment choice is to change the stock owner and managing agent" do + let(:params) { { user: { organisation_id: new_organisation.id, log_reassignment: "reassign_all" } } } + + before do + patch "/users/#{other_user.id}/log-reassignment", headers:, params: 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: + it "does not update the user's organisation" do + expect(other_user.reload.organisation).not_to eq(new_organisation) + end + + it "redirects to confirmation page" do + expect(response).to redirect_to("/users/#{other_user.id}/confirm-organisation-change?log_reassignment=reassign_all&organisation_id=#{new_organisation.id}") end end - context "when updating organisation" do - let(:new_organisation) { create(:organisation) } + context "and log reassignment choice is to unassign logs" do + let(:params) { { user: { organisation_id: new_organisation.id, log_reassignment: "unassign" } } } before do - patch "/users/#{other_user.id}", headers:, params: + patch "/users/#{other_user.id}/log-reassignment", headers:, params: + end + + it "does not update the user's organisation" do + expect(other_user.reload.organisation).not_to eq(new_organisation) end - context "and organisation id is nil" do - let(:params) { { id: other_user.id, user: { organisation_id: "" } } } + it "redirects to confirmation page" do + expect(response).to redirect_to("/users/#{other_user.id}/confirm-organisation-change?log_reassignment=unassign&organisation_id=#{new_organisation.id}") + end + end + + context "and log reassignment choice is to change stock owner" do + let(:params) { { user: { organisation_id: new_organisation.id, log_reassignment: "reassign_stock_owner" } } } - it "does not update the organisation" do + context "when users organisation manages the logs" do + before do + create(:lettings_log, managing_organisation: other_user.organisation, assigned_to: other_user) + create(:sales_log, managing_organisation: other_user.organisation, assigned_to: other_user) + patch "/users/#{other_user.id}/log-reassignment", headers:, params: + end + + it "required the new org to have stock owner relationship with the current user org" do + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content("New org must be a stock owner of #{other_user.organisation_name} to make this change.") + end + end + + context "when different organisations manage the logs" do + before do + create(:lettings_log, managing_organisation: other_user.organisation, assigned_to: other_user) + create(:lettings_log, managing_organisation: new_organisation_2, assigned_to: other_user) + create(:sales_log, managing_organisation: new_organisation_3, assigned_to: other_user) + patch "/users/#{other_user.id}/log-reassignment", headers:, params: + end + + it "required the new org to have stock owner relationship with the managing organisations" do expect(response).to have_http_status(:unprocessable_entity) - expect(page).to have_selector(".govuk-error-summary__title") + expect(page).to have_content("New org must be a stock owner of #{other_user.organisation_name}, #{new_organisation_2.name}, and #{new_organisation_3.name} to make this change.") end end + end - context "and organisation id is not nil" do - let(:params) { { id: other_user.id, user: { organisation_id: new_organisation.id, name: "new_name" } } } + context "and log reassignment choice is to change managing agent" do + let(:params) { { user: { organisation_id: new_organisation.id, log_reassignment: "reassign_managing_agent" } } } - it "does not update the organisation" do - expect(user.reload.organisation).not_to eq(new_organisation) + context "when users organisation manages the logs" do + before do + create(:lettings_log, owning_organisation: other_user.organisation, assigned_to: other_user) + create(:sales_log, owning_organisation: other_user.organisation, assigned_to: other_user) + patch "/users/#{other_user.id}/log-reassignment", headers:, params: end - it "redirects to log reassignment page" do - expect(response).to redirect_to("/users/#{other_user.id}/log-reassignment?organisation_id=#{new_organisation.id}") + it "required the new org to have managing agent relationship with the current user org" do + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content("New org must be a managing agent of #{other_user.organisation_name} to make this change.") end + end - it "updated other fields" do - expect(other_user.reload.name).to eq("new_name") + context "when different organisations manage the logs" do + before do + create(:lettings_log, owning_organisation: other_user.organisation, assigned_to: other_user) + create(:lettings_log, owning_organisation: new_organisation_2, assigned_to: other_user) + create(:sales_log, owning_organisation: new_organisation_3, managing_organisation: other_user.organisation, assigned_to: other_user) + patch "/users/#{other_user.id}/log-reassignment", headers:, params: + end + + it "required the new org to have managing agent relationship with owning organisations" do + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content("New org must be a managing agent of #{other_user.organisation_name}, #{new_organisation_2.name}, and #{new_organisation_3.name} to make this change.") end end end