From d1258faa6cd658dc58f566aa47c6b8345868b717 Mon Sep 17 00:00:00 2001 From: Manny Dinssa <44172848+Dinssa@users.noreply.github.com> Date: Thu, 12 Sep 2024 09:10:28 +0100 Subject: [PATCH 1/6] Update guidance text (#2631) --- app/views/bulk_upload_shared/guidance.html.erb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/bulk_upload_shared/guidance.html.erb b/app/views/bulk_upload_shared/guidance.html.erb index c32295b13..e530aa5b5 100644 --- a/app/views/bulk_upload_shared/guidance.html.erb +++ b/app/views/bulk_upload_shared/guidance.html.erb @@ -81,8 +81,8 @@ <%= accordion.with_section(heading_text: "Next steps") do %>

Once you've saved your CSV file, you can upload it via a button at the top of the lettings and sales logs pages.

When your file is done processing, you will receive an email explaining your next steps. If all your data is valid, your logs will be created. If some data is invalid, you’ll receive an email with instructions about how to resolve the errors.

-

If your file has errors on fields 1 through 17, you must fix these in the CSV. This is because we need to know these answers to validate the rest of the data. Any errors in these fields will be featured in the error report’s summary tab.

-

If none of your errors are in fields 1 through 17, you can choose how to fix the errors. You can either fix them in the CSV and reupload, or create partially complete logs and answer the remaining questions on the CORE site. Any errors that affect a significant number of logs will be featured in the error report’s summary tab to help you decide.

+

If your file has errors on fields 1 through 15 for lettings, or 1 through 18 for sales, you must fix these in the CSV. This is because we need to know these answers to validate the rest of the data. Any errors in these fields will be featured in the error report’s summary tab.

+

If none of your errors are in fields 1 through 15 for lettings, or 1 through 18 for sales, you can choose how to fix the errors. You can either fix them in the CSV and reupload, or create partially complete logs and answer the remaining questions on the CORE site. Any errors that affect a significant number of logs will be featured in the error report’s summary tab to help you decide.

<% end %> From aeb998c537dc3000f9761ebe6ed11cc6a7910b66 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Fri, 13 Sep 2024 12:21:44 +0100 Subject: [PATCH 2/6] turn on duplicate log check on staging (#2635) --- app/services/feature_toggle.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/feature_toggle.rb b/app/services/feature_toggle.rb index 91989ff86..c51b34d8b 100644 --- a/app/services/feature_toggle.rb +++ b/app/services/feature_toggle.rb @@ -4,7 +4,7 @@ class FeatureToggle end def self.bulk_upload_duplicate_log_check_enabled? - !Rails.env.staging? + true end def self.upload_enabled? From c45cb160d2f5fc036c2001250c8de600f39dca0c Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Fri, 13 Sep 2024 13:53:19 +0100 Subject: [PATCH 3/6] Do not check duplicates on empty rows (#2636) --- .../bulk_upload/lettings/year2024/row_parser.rb | 2 ++ .../bulk_upload/sales/year2024/row_parser.rb | 2 ++ .../bulk_upload/lettings/year2024/row_parser_spec.rb | 12 ++++++++++++ .../bulk_upload/sales/year2024/row_parser_spec.rb | 12 ++++++++++++ 4 files changed, 28 insertions(+) diff --git a/app/services/bulk_upload/lettings/year2024/row_parser.rb b/app/services/bulk_upload/lettings/year2024/row_parser.rb index 913e5d9e5..8fc913055 100644 --- a/app/services/bulk_upload/lettings/year2024/row_parser.rb +++ b/app/services/bulk_upload/lettings/year2024/row_parser.rb @@ -513,6 +513,8 @@ class BulkUpload::Lettings::Year2024::RowParser end def log_already_exists? + return false if blank_row? + @log_already_exists ||= LettingsLog .where(status: %w[not_started in_progress completed]) .exists?(duplicate_check_fields.index_with { |field| log.public_send(field) }) diff --git a/app/services/bulk_upload/sales/year2024/row_parser.rb b/app/services/bulk_upload/sales/year2024/row_parser.rb index d39715f30..8be08d62f 100644 --- a/app/services/bulk_upload/sales/year2024/row_parser.rb +++ b/app/services/bulk_upload/sales/year2024/row_parser.rb @@ -539,6 +539,8 @@ class BulkUpload::Sales::Year2024::RowParser end def log_already_exists? + return false if blank_row? + @log_already_exists ||= SalesLog .where(status: %w[not_started in_progress completed]) .exists?(duplicate_check_fields.index_with { |field| log.public_send(field) }) diff --git a/spec/services/bulk_upload/lettings/year2024/row_parser_spec.rb b/spec/services/bulk_upload/lettings/year2024/row_parser_spec.rb index 42a05e33c..3faa0a699 100644 --- a/spec/services/bulk_upload/lettings/year2024/row_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/year2024/row_parser_spec.rb @@ -1843,6 +1843,18 @@ RSpec.describe BulkUpload::Lettings::Year2024::RowParser do end end end + + describe "log_already_exists?" do + let(:attributes) { { bulk_upload: } } + + before do + build(:lettings_log, owning_organisation: nil, startdate: nil, tenancycode: nil, location: nil, age1: nil, sex1: nil, ecstat1: nil, brent: nil, scharge: nil, pscharge: nil, supcharg: nil).save(validate: false) + end + + it "does not add duplicate logs validation to the blank row" do + expect(parser.log_already_exists?).to eq(false) + end + end end describe "#log" do diff --git a/spec/services/bulk_upload/sales/year2024/row_parser_spec.rb b/spec/services/bulk_upload/sales/year2024/row_parser_spec.rb index f19a61d78..e428f7792 100644 --- a/spec/services/bulk_upload/sales/year2024/row_parser_spec.rb +++ b/spec/services/bulk_upload/sales/year2024/row_parser_spec.rb @@ -1417,6 +1417,18 @@ RSpec.describe BulkUpload::Sales::Year2024::RowParser do end end end + + describe "log_already_exists?" do + let(:attributes) { { bulk_upload: } } + + before do + build(:sales_log, owning_organisation: nil, saledate: nil, purchid: nil, age1: nil, sex1: nil, ecstat1: nil).save(validate: false) + end + + it "does not add duplicate logs validation to the blank row" do + expect(parser.log_already_exists?).to eq(false) + end + end end describe "#log" do From c3bfadfe7f0a945ebf4fab30e0e44544b25acb01 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Mon, 16 Sep 2024 08:09:11 +0100 Subject: [PATCH 4/6] Empty bulk upload row parsing (#2638) * empty * Add stripped_row back * Skip empty rows --- app/services/bulk_upload/lettings/year2024/csv_parser.rb | 7 +++++-- app/services/bulk_upload/sales/year2024/csv_parser.rb | 6 ++++-- .../bulk_upload/lettings/year2024/csv_parser_spec.rb | 5 +++++ .../services/bulk_upload/sales/year2024/csv_parser_spec.rb | 5 +++++ 4 files changed, 19 insertions(+), 4 deletions(-) diff --git a/app/services/bulk_upload/lettings/year2024/csv_parser.rb b/app/services/bulk_upload/lettings/year2024/csv_parser.rb index 061f3bbbd..d8d430755 100644 --- a/app/services/bulk_upload/lettings/year2024/csv_parser.rb +++ b/app/services/bulk_upload/lettings/year2024/csv_parser.rb @@ -30,12 +30,15 @@ class BulkUpload::Lettings::Year2024::CsvParser end def row_parsers - @row_parsers ||= body_rows.map do |row| + @row_parsers ||= body_rows.map { |row| + next if row.empty? + stripped_row = row[col_offset..] + hash = Hash[field_numbers.zip(stripped_row)] BulkUpload::Lettings::Year2024::RowParser.new(hash) - end + }.compact end def body_rows diff --git a/app/services/bulk_upload/sales/year2024/csv_parser.rb b/app/services/bulk_upload/sales/year2024/csv_parser.rb index 2dc9d38a1..9ba99c19b 100644 --- a/app/services/bulk_upload/sales/year2024/csv_parser.rb +++ b/app/services/bulk_upload/sales/year2024/csv_parser.rb @@ -30,12 +30,14 @@ class BulkUpload::Sales::Year2024::CsvParser end def row_parsers - @row_parsers ||= body_rows.map do |row| + @row_parsers ||= body_rows.map { |row| + next if row.empty? + stripped_row = row[col_offset..] hash = Hash[field_numbers.zip(stripped_row)] BulkUpload::Sales::Year2024::RowParser.new(hash) - end + }.compact end def body_rows diff --git a/spec/services/bulk_upload/lettings/year2024/csv_parser_spec.rb b/spec/services/bulk_upload/lettings/year2024/csv_parser_spec.rb index 6db8e1806..d0e5b3692 100644 --- a/spec/services/bulk_upload/lettings/year2024/csv_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/year2024/csv_parser_spec.rb @@ -41,6 +41,7 @@ RSpec.describe BulkUpload::Lettings::Year2024::CsvParser do file.write("Duplicate check field?\n") file.write(BulkUpload::LettingsLogToCsv.new(log:).default_2024_field_numbers_row) file.write(BulkUpload::LettingsLogToCsv.new(log:).to_2024_csv_row) + file.write("\n") file.rewind end @@ -52,6 +53,10 @@ RSpec.describe BulkUpload::Lettings::Year2024::CsvParser do it "parses csv correctly" do expect(service.row_parsers[0].field_13).to eql(log.tenancycode) end + + it "does not parse the last empty row" do + expect(service.row_parsers.count).to eq(1) + end end context "when parsing csv with headers in arbitrary order" do diff --git a/spec/services/bulk_upload/sales/year2024/csv_parser_spec.rb b/spec/services/bulk_upload/sales/year2024/csv_parser_spec.rb index ef90bd834..9440b7e8c 100644 --- a/spec/services/bulk_upload/sales/year2024/csv_parser_spec.rb +++ b/spec/services/bulk_upload/sales/year2024/csv_parser_spec.rb @@ -17,6 +17,7 @@ RSpec.describe BulkUpload::Sales::Year2024::CsvParser do file.write("Duplicate check field?\n") file.write(BulkUpload::SalesLogToCsv.new(log:).default_2024_field_numbers_row) file.write(BulkUpload::SalesLogToCsv.new(log:).to_2024_csv_row) + file.write("\n") file.rewind end @@ -32,6 +33,10 @@ RSpec.describe BulkUpload::Sales::Year2024::CsvParser do it "counts the number of valid field numbers correctly" do expect(service).to be_correct_field_count end + + it "does not parse the last empty row" do + expect(service.row_parsers.count).to eq(1) + end end context "when parsing csv with headers in arbitrary order" do From 0f4a8403ff09f6ce28c56abc8c3b5e45a083d043 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Mon, 16 Sep 2024 09:12:42 +0100 Subject: [PATCH 5/6] Revert "turn on duplicate log check on staging (#2635)" (#2639) * Revert "turn on duplicate log check on staging (#2635)" This reverts commit aeb998c537dc3000f9761ebe6ed11cc6a7910b66. * Update fixed git version --- Dockerfile | 2 +- app/services/feature_toggle.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Dockerfile b/Dockerfile index b26aad246..a26c80ee9 100644 --- a/Dockerfile +++ b/Dockerfile @@ -10,7 +10,7 @@ RUN apk add --update --no-cache tzdata && \ # build-base: compilation tools for bundle # yarn: node package manager # postgresql-dev: postgres driver and libraries -RUN apk add --no-cache build-base=0.5-r3 busybox=1.36.1-r7 nodejs-current=20.8.1-r0 yarn=1.22.19-r0 postgresql13-dev=13.15-r0 git=2.40.1-r0 bash=5.2.15-r5 +RUN apk add --no-cache build-base=0.5-r3 busybox=1.36.1-r7 nodejs-current=20.8.1-r0 yarn=1.22.19-r0 postgresql13-dev=13.15-r0 git=2.40.3-r0 bash=5.2.15-r5 # Bundler version should be the same version as what the Gemfile.lock was bundled with RUN gem install bundler:2.3.14 --no-document diff --git a/app/services/feature_toggle.rb b/app/services/feature_toggle.rb index c51b34d8b..91989ff86 100644 --- a/app/services/feature_toggle.rb +++ b/app/services/feature_toggle.rb @@ -4,7 +4,7 @@ class FeatureToggle end def self.bulk_upload_duplicate_log_check_enabled? - true + !Rails.env.staging? end def self.upload_enabled? From 15e4f79b838bc2d0e58b5a775ceb5977361ea9c4 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Tue, 17 Sep 2024 10:47:50 +0100 Subject: [PATCH 6/6] CLDC-3404 Allow support to update user organisation (#2596) * Allow support to change user organisation * Redirect to log-reassignment page when updating organisation * Add log reassignment page * Allow setting log reassignment choice * Add organisation change confirmation page * Update logs based on log reassignment selection * Skip log reassignment question if user has no logs * Send organisation update email * Update routes in accessibility tests * Allo moving deactivated users * Move org field above role * Only display banner if email has changed * rescue StandardError * Only reassign visible logs * Clean user error messages * Update pluralization in email * Mark related bulk uploads as cancelled by user * Add moved user banner * Update routing for fix-choice page * Update pluralization again --- app/controllers/users_controller.rb | 120 ++++- app/helpers/user_helper.rb | 42 ++ app/models/bulk_upload.rb | 4 + .../bulk_upload_lettings_resume/fix_choice.rb | 4 +- .../bulk_upload_sales_resume/fix_choice.rb | 4 +- app/models/user.rb | 118 ++++ app/policies/user_policy.rb | 24 +- .../show.html.erb | 2 + .../summary.html.erb | 2 + .../bulk_upload_sales_results/show.html.erb | 2 + .../summary.html.erb | 2 + .../_moved_user_banner.html.erb | 12 + app/views/users/edit.html.erb | 19 + app/views/users/log_reassignment.html.erb | 36 ++ .../organisation_change_confirmation.html.erb | 24 + app/views/users/show.html.erb | 10 +- config/locales/en.yml | 4 + config/routes.rb | 4 + .../20240911152702_add_moved_user_to_bu.rb | 5 + db/schema.rb | 3 +- spec/features/accessibility_spec.rb | 6 + spec/helpers/user_helper_spec.rb | 86 +++ spec/models/user_spec.rb | 300 +++++++++++ spec/policies/user_policy_spec.rb | 14 + ...upload_lettings_results_controller_spec.rb | 45 ++ ..._upload_lettings_resume_controller_spec.rb | 39 ++ ...lk_upload_sales_results_controller_spec.rb | 59 ++ ...ulk_upload_sales_resume_controller_spec.rb | 39 ++ spec/requests/users_controller_spec.rb | 506 +++++++++++++++--- 29 files changed, 1452 insertions(+), 83 deletions(-) create mode 100644 app/views/bulk_upload_shared/_moved_user_banner.html.erb create mode 100644 app/views/users/log_reassignment.html.erb create mode 100644 app/views/users/organisation_change_confirmation.html.erb create mode 100644 db/migrate/20240911152702_add_moved_user_to_bu.rb diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index c5c5241f9..91b0ccd40 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -56,20 +56,24 @@ class UsersController < ApplicationController def key_contact; end def edit - redirect_to user_path(@user) unless @user.active? + redirect_to user_path(@user) unless @user.active? || current_user.support? end def update validate_attributes - if @user.errors.empty? && @user.update(user_params) + if @user.errors.empty? && @user.update(user_params_without_org) 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") + if user_params.key?("email") && user_params[:email] != @user.email flash[:notice] = I18n.t("devise.email.updated", email: @user.unconfirmed_email) end - redirect_to account_path + if updating_organisation? + redirect_to user_log_reassignment_path(@user, organisation_id: user_params[:organisation_id]) + else + redirect_to account_path + end else user_name = @user.name&.possessive || @user.email.possessive if user_params[:active] == "false" @@ -79,10 +83,15 @@ class UsersController < ApplicationController @user.reactivate! @user.send_confirmation_instructions flash[:notice] = I18n.t("devise.activation.reactivated", user_name:) - elsif user_params.key?("email") + elsif user_params.key?("email") && user_params[:email] != @user.email flash[:notice] = I18n.t("devise.email.updated", email: @user.unconfirmed_email) end - redirect_to user_path(@user) + + if updating_organisation? + redirect_to user_log_reassignment_path(@user, organisation_id: user_params[:organisation_id]) + else + redirect_to user_path(@user) + end end elsif user_params.key?("password") format_error_messages @@ -144,6 +153,58 @@ class UsersController < ApplicationController redirect_to users_organisation_path(@user.organisation), notice: I18n.t("notification.user_deleted", name: @user.name) end + def log_reassignment + authorize @user + assigned_to_logs_count = @user.assigned_to_lettings_logs.visible.count + @user.assigned_to_sales_logs.visible.count + return redirect_to user_organisation_change_confirmation_path(@user, organisation_id: params[:organisation_id]) if assigned_to_logs_count.zero? + + if params[:organisation_id].present? && Organisation.where(id: params[:organisation_id]).exists? + @new_organisation = Organisation.find(params[:organisation_id]) + else + redirect_to user_path(@user) + end + end + + def update_log_reassignment + authorize @user + 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_organisation_change_confirmation_path(@user, log_reassignment_params) + else + render :log_reassignment, status: :unprocessable_entity + end + end + + def organisation_change_confirmation + authorize @user + assigned_to_logs_count = @user.assigned_to_lettings_logs.visible.count + @user.assigned_to_sales_logs.visible.count + + return redirect_to user_path(@user) if params[:organisation_id].blank? || !Organisation.where(id: params[:organisation_id]).exists? + return redirect_to user_path(@user) if params[:log_reassignment].blank? && assigned_to_logs_count.positive? + + @new_organisation = Organisation.find(params[:organisation_id]) + @log_reassignment = params[:log_reassignment] + end + + def confirm_organisation_change + authorize @user + assigned_to_logs_count = @user.assigned_to_lettings_logs.visible.count + @user.assigned_to_sales_logs.visible.count + + return redirect_to user_path(@user) if log_reassignment_params[:organisation_id].blank? || !Organisation.where(id: log_reassignment_params[:organisation_id]).exists? + return redirect_to user_path(@user) if log_reassignment_params[:log_reassignment].blank? && assigned_to_logs_count.positive? + + @new_organisation = Organisation.find(log_reassignment_params[:organisation_id]) + @log_reassignment = log_reassignment_params[:log_reassignment] + @user.reassign_logs_and_update_organisation(@new_organisation, @log_reassignment) + + redirect_to user_path(@user) + end + private def validate_attributes @@ -157,6 +218,10 @@ private elsif !user_params[:phone].nil? && !valid_phone_number?(user_params[:phone]) @user.errors.add :phone end + + if user_params.key?(:organisation_id) && user_params[:organisation_id].blank? + @user.errors.add :organisation_id, :blank + end end def valid_phone_number?(number) @@ -191,8 +256,10 @@ private def user_params if @user == current_user - if current_user.data_coordinator? || current_user.support? + if current_user.data_coordinator? params.require(:user).permit(:email, :phone, :phone_extension, :name, :password, :password_confirmation, :role, :is_dpo, :is_key_contact, :initial_confirmation_sent) + elsif current_user.support? + params.require(:user).permit(:email, :phone, :phone_extension, :name, :password, :password_confirmation, :role, :is_dpo, :is_key_contact, :initial_confirmation_sent, :organisation_id) else params.require(:user).permit(:email, :phone, :phone_extension, :name, :password, :password_confirmation, :initial_confirmation_sent) end @@ -203,6 +270,14 @@ private end end + def user_params_without_org + 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 @@ -234,4 +309,35 @@ private def session_filters filter_manager.session_filters end + + 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.visible.map(&:managing_organisation) + @user.assigned_to_sales_logs.visible.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.visible.map(&:owning_organisation) + @user.assigned_to_sales_logs.visible.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/helpers/user_helper.rb b/app/helpers/user_helper.rb index 6fcf6ca4c..06fe2bc7d 100644 --- a/app/helpers/user_helper.rb +++ b/app/helpers/user_helper.rb @@ -14,4 +14,46 @@ module UserHelper def delete_user_link(user) govuk_button_link_to "Delete this user", delete_confirmation_user_path(user), warning: true end + + def organisation_change_warning(user, new_organisation) + logs_count = user.assigned_to_lettings_logs.count + user.assigned_to_sales_logs.count + logs_count_text = logs_count == 1 ? "is #{logs_count} log" : "are #{logs_count} logs" + + "You’re moving #{user.name} from #{user.organisation.name} to #{new_organisation.name}. There #{logs_count_text} assigned to them." + end + + def organisation_change_confirmation_warning(user, new_organisation, log_reassignment) + log_reassignment_text = "There are no logs assigned to them." + + logs_count = user.assigned_to_lettings_logs.count + user.assigned_to_sales_logs.count + if logs_count.positive? + case log_reassignment + when "reassign_all" + log_reassignment_text = "The stock owner and managing agent on their logs will change to #{new_organisation.name}." + when "reassign_stock_owner" + log_reassignment_text = "The stock owner on their logs will change to #{new_organisation.name}." + when "reassign_managing_agent" + log_reassignment_text = "The managing agent on their logs will change to #{new_organisation.name}." + when "unassign" + log_reassignment_text = "Their logs will be unassigned." + end + end + + "You’re moving #{user.name} from #{user.organisation.name} to #{new_organisation.name}. #{log_reassignment_text}" + end + + def remove_attributes_from_error_messages(user) + modified_errors = [] + + user.errors.each do |error| + cleaned_message = error.type.gsub(error.attribute.to_s.humanize, "").strip + modified_errors << [error.attribute, cleaned_message] + end + + user.errors.clear + + modified_errors.each do |attribute, message| + user.errors.add(attribute, message) + end + end end diff --git a/app/models/bulk_upload.rb b/app/models/bulk_upload.rb index 7af43ee28..69ab42871 100644 --- a/app/models/bulk_upload.rb +++ b/app/models/bulk_upload.rb @@ -101,6 +101,10 @@ class BulkUpload < ApplicationRecord logs.filter_by_status("in_progress").map(&:missing_answers_count).sum(0) end + def moved_user_name + User.find_by(id: moved_user_id)&.name + end + private def generate_identifier diff --git a/app/models/forms/bulk_upload_lettings_resume/fix_choice.rb b/app/models/forms/bulk_upload_lettings_resume/fix_choice.rb index a97d6e3b9..5ee4d37fd 100644 --- a/app/models/forms/bulk_upload_lettings_resume/fix_choice.rb +++ b/app/models/forms/bulk_upload_lettings_resume/fix_choice.rb @@ -56,7 +56,7 @@ module Forms end def preflight_valid? - bulk_upload.choice != "create-fix-inline" && bulk_upload.choice != "bulk-confirm-soft-validations" + bulk_upload.choice.blank? end def preflight_redirect @@ -65,6 +65,8 @@ module Forms page_bulk_upload_lettings_resume_path(bulk_upload, :chosen) when "bulk-confirm-soft-validations" page_bulk_upload_lettings_soft_validations_check_path(bulk_upload, :chosen) + else + bulk_upload_lettings_result_path(bulk_upload) end end end diff --git a/app/models/forms/bulk_upload_sales_resume/fix_choice.rb b/app/models/forms/bulk_upload_sales_resume/fix_choice.rb index 0cd2ae0f5..b34f50d3a 100644 --- a/app/models/forms/bulk_upload_sales_resume/fix_choice.rb +++ b/app/models/forms/bulk_upload_sales_resume/fix_choice.rb @@ -56,7 +56,7 @@ module Forms end def preflight_valid? - bulk_upload.choice != "create-fix-inline" && bulk_upload.choice != "bulk-confirm-soft-validations" + bulk_upload.choice.blank? end def preflight_redirect @@ -65,6 +65,8 @@ module Forms page_bulk_upload_sales_resume_path(bulk_upload, :chosen) when "bulk-confirm-soft-validations" page_bulk_upload_sales_soft_validations_check_path(bulk_upload, :chosen) + else + bulk_upload_sales_result_path(bulk_upload) end end end diff --git a/app/models/user.rb b/app/models/user.rb index 31956e54d..0a26a254b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -49,6 +49,13 @@ class User < ApplicationRecord support: 99, }.freeze + LOG_REASSIGNMENT = { + reassign_all: "Yes, change the stock owner and the managing agent", + reassign_stock_owner: "Yes, change the stock owner but keep the managing agent the same", + reassign_managing_agent: "Yes, change the managing agent but keep the stock owner the same", + unassign: "No, unassign the logs", + }.freeze + enum role: ROLES scope :search_by_name, ->(name) { where("users.name ILIKE ?", "%#{name}%") } @@ -80,6 +87,8 @@ class User < ApplicationRecord scope :visible, -> { where(discarded_at: nil) } scope :own_and_managing_org_users, ->(organisation) { where(organisation: organisation.child_organisations + [organisation]) } + attr_accessor :log_reassignment + def lettings_logs if support? LettingsLog.all @@ -161,6 +170,7 @@ class User < ApplicationRecord 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 + ORGANISATION_UPDATE_TEMPLATE_ID = "4b7716c0-cc5c-41dd-92e4-a0dff03bdf5e".freeze def reset_password_notify_template RESET_PASSWORD_TEMPLATE_ID @@ -270,6 +280,48 @@ class User < ApplicationRecord "#{phone}, Ext. #{phone_extension}" end + def assigned_to_lettings_logs + lettings_logs.where(assigned_to: self) + end + + def assigned_to_sales_logs + sales_logs.where(assigned_to: self) + end + + def reassign_logs_and_update_organisation(new_organisation, log_reassignment) + return unless new_organisation + + ActiveRecord::Base.transaction do + lettings_logs_to_reassign = assigned_to_lettings_logs.visible + sales_logs_to_reassign = assigned_to_sales_logs.visible + current_organisation = organisation + + logs_count = lettings_logs_to_reassign.count + sales_logs_to_reassign.count + return if logs_count.positive? && (log_reassignment.blank? || !LOG_REASSIGNMENT.key?(log_reassignment.to_sym)) + + update!(organisation: new_organisation) + + case log_reassignment + when "reassign_all" + reassign_all_orgs(new_organisation, lettings_logs_to_reassign, sales_logs_to_reassign) + when "reassign_stock_owner" + reassign_stock_owners(new_organisation, lettings_logs_to_reassign, sales_logs_to_reassign) + when "reassign_managing_agent" + reassign_managing_agents(new_organisation, lettings_logs_to_reassign, sales_logs_to_reassign) + when "unassign" + unassign_organisations(lettings_logs_to_reassign, sales_logs_to_reassign, current_organisation) + end + + cancel_related_bulk_uploads + send_organisation_change_email(current_organisation, new_organisation, log_reassignment, logs_count) + rescue StandardError => e + Rails.logger.error("User update failed with: #{e.message}") + Sentry.capture_exception(e) + + raise ActiveRecord::Rollback + end + end + protected # Checks whether a password is needed or not. For validations only. @@ -294,4 +346,70 @@ private DataProtectionConfirmationMailer.send_confirmation_email(self).deliver_later end + + def reassign_all_orgs(new_organisation, lettings_logs_to_reassign, sales_logs_to_reassign) + lettings_logs_to_reassign.update_all(owning_organisation_id: new_organisation.id, managing_organisation_id: new_organisation.id, values_updated_at: Time.zone.now) + sales_logs_to_reassign.update_all(owning_organisation_id: new_organisation.id, managing_organisation_id: new_organisation.id, values_updated_at: Time.zone.now) + end + + def reassign_stock_owners(new_organisation, lettings_logs_to_reassign, sales_logs_to_reassign) + lettings_logs_to_reassign.update_all(owning_organisation_id: new_organisation.id, values_updated_at: Time.zone.now) + sales_logs_to_reassign.update_all(owning_organisation_id: new_organisation.id, values_updated_at: Time.zone.now) + end + + def reassign_managing_agents(new_organisation, lettings_logs_to_reassign, sales_logs_to_reassign) + lettings_logs_to_reassign.update_all(managing_organisation_id: new_organisation.id, values_updated_at: Time.zone.now) + sales_logs_to_reassign.update_all(managing_organisation_id: new_organisation.id, values_updated_at: Time.zone.now) + end + + def unassign_organisations(lettings_logs_to_reassign, sales_logs_to_reassign, current_organisation) + if User.find_by(name: "Unassigned", organisation: current_organisation) + unassigned_user = User.find_by(name: "Unassigned", organisation: current_organisation) + else + unassigned_user = User.new( + name: "Unassigned", + organisation_id:, + is_dpo: false, + encrypted_password: SecureRandom.hex(10), + email: SecureRandom.uuid, + confirmed_at: Time.zone.now, + active: false, + ) + unassigned_user.save!(validate: false) + end + lettings_logs_to_reassign.update_all(assigned_to_id: unassigned_user.id, values_updated_at: Time.zone.now) + sales_logs_to_reassign.update_all(assigned_to_id: unassigned_user.id, values_updated_at: Time.zone.now) + end + + def send_organisation_change_email(current_organisation, new_organisation, log_reassignment, logs_count) + reassigned_logs_text = "" + assigned_logs_count = logs_count == 1 ? "is 1 log" : "are #{logs_count} logs" + + case log_reassignment + when "reassign_all" + reassigned_logs_text = "There #{assigned_logs_count} assigned to you. The stock owner and managing agent on #{logs_count == 1 ? 'this log' : 'these logs'} has been changed from #{current_organisation.name} to #{new_organisation.name}." + when "reassign_stock_owner" + reassigned_logs_text = "There #{assigned_logs_count} assigned to you. The stock owner on #{logs_count == 1 ? 'this log' : 'these logs'} has been changed from #{current_organisation.name} to #{new_organisation.name}." + when "reassign_managing_agent" + reassigned_logs_text = "There #{assigned_logs_count} assigned to you. The managing agent on #{logs_count == 1 ? 'this log' : 'these logs'} has been changed from #{current_organisation.name} to #{new_organisation.name}." + when "unassign" + reassigned_logs_text = "There #{assigned_logs_count} assigned to you. #{logs_count == 1 ? 'This' : 'These'} have now been unassigned." + end + + template_id = ORGANISATION_UPDATE_TEMPLATE_ID + personalisation = { + from_organisation: "#{current_organisation.name} (Organisation ID: #{current_organisation.id})", + to_organisation: "#{new_organisation.name} (Organisation ID: #{new_organisation.id})", + reassigned_logs_text:, + } + DeviseNotifyMailer.new.send_email(email, template_id, personalisation) + end + + def cancel_related_bulk_uploads + lettings_bu_ids = LettingsLog.where(assigned_to: self, status: "pending").map(&:bulk_upload_id).compact.uniq + BulkUpload.where(id: lettings_bu_ids).update!(choice: "cancelled-by-moved-user", moved_user_id: id) + + sales_bu_ids = SalesLog.where(assigned_to: self, status: "pending").map(&:bulk_upload_id).compact.uniq + BulkUpload.where(id: sales_bu_ids).update!(choice: "cancelled-by-moved-user", moved_user_id: id) + end end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index ff7d871f2..e45614df7 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -10,17 +10,15 @@ class UserPolicy @current_user == @user end - def edit_roles? - (@current_user.data_coordinator? || @current_user.support?) && @user.active? - end - %w[ edit_roles? edit_dpo? edit_key_contact? ].each do |method_name| define_method method_name do - (@current_user.data_coordinator? || @current_user.support?) && @user.active? + return true if @current_user.support? + + @current_user.data_coordinator? && @user.active? end end @@ -30,7 +28,9 @@ class UserPolicy edit_names? ].each do |method_name| define_method method_name do - (@current_user == @user || @current_user.data_coordinator? || @current_user.support?) && @user.active? + return true if @current_user.support? + + (@current_user == @user || @current_user.data_coordinator?) && @user.active? end end @@ -45,6 +45,18 @@ class UserPolicy !has_any_logs_in_editable_collection_period && !has_signed_data_protection_agreement? end + %w[ + edit_organisation? + log_reassignment? + update_log_reassignment? + organisation_change_confirmation? + confirm_organisation_change? + ].each do |method_name| + define_method method_name do + @current_user.support? + end + end + private def has_any_logs_in_editable_collection_period diff --git a/app/views/bulk_upload_lettings_results/show.html.erb b/app/views/bulk_upload_lettings_results/show.html.erb index 56448f24e..30a6fd585 100644 --- a/app/views/bulk_upload_lettings_results/show.html.erb +++ b/app/views/bulk_upload_lettings_results/show.html.erb @@ -2,6 +2,8 @@ <%= govuk_back_link(href: :back) %> <% end %> +<%= render partial: "bulk_upload_shared/moved_user_banner" %> +
Bulk upload for lettings (<%= @bulk_upload.year_combo %>) diff --git a/app/views/bulk_upload_lettings_results/summary.html.erb b/app/views/bulk_upload_lettings_results/summary.html.erb index 8c36af632..b144793bf 100644 --- a/app/views/bulk_upload_lettings_results/summary.html.erb +++ b/app/views/bulk_upload_lettings_results/summary.html.erb @@ -1,3 +1,5 @@ +<%= render partial: "bulk_upload_shared/moved_user_banner" %> +
Bulk upload for lettings (<%= @bulk_upload.year_combo %>) diff --git a/app/views/bulk_upload_sales_results/show.html.erb b/app/views/bulk_upload_sales_results/show.html.erb index b7c838567..0d645db33 100644 --- a/app/views/bulk_upload_sales_results/show.html.erb +++ b/app/views/bulk_upload_sales_results/show.html.erb @@ -2,6 +2,8 @@ <%= govuk_back_link(href: :back) %> <% end %> +<%= render partial: "bulk_upload_shared/moved_user_banner" %> +
Bulk Upload for sales (<%= @bulk_upload.year_combo %>) diff --git a/app/views/bulk_upload_sales_results/summary.html.erb b/app/views/bulk_upload_sales_results/summary.html.erb index af504acbd..0fa51b9dc 100644 --- a/app/views/bulk_upload_sales_results/summary.html.erb +++ b/app/views/bulk_upload_sales_results/summary.html.erb @@ -1,3 +1,5 @@ +<%= render partial: "bulk_upload_shared/moved_user_banner" %> +
Bulk upload for sales (<%= @bulk_upload.year_combo %>) diff --git a/app/views/bulk_upload_shared/_moved_user_banner.html.erb b/app/views/bulk_upload_shared/_moved_user_banner.html.erb new file mode 100644 index 000000000..9ab97022e --- /dev/null +++ b/app/views/bulk_upload_shared/_moved_user_banner.html.erb @@ -0,0 +1,12 @@ +<% if @bulk_upload.choice == "cancelled-by-moved-user" %> + <%= govuk_notification_banner(title_text: "Important") do %> +

+ This error report is out of date. +

+ <% if current_user.id == @bulk_upload.moved_user_id %> + You moved to a different organisation since this file was uploaded. Reupload the file to get an accurate error report. + <% else %> + Some logs in this upload are assigned to <%= @bulk_upload.moved_user_name %>, who has moved to a different organisation since this file was uploaded. Reupload the file to get an accurate error report. + <% end %> + <% end %> +<% end %> diff --git a/app/views/users/edit.html.erb b/app/views/users/edit.html.erb index 6fdfdb5aa..4c610c22a 100644 --- a/app/views/users/edit.html.erb +++ b/app/views/users/edit.html.erb @@ -7,6 +7,7 @@ <%= form_for(@user, as: :user, html: { method: :patch }) do |f| %>

+ <% remove_attributes_from_error_messages(@user) %> <%= f.govuk_error_summary %>

@@ -32,6 +33,24 @@ autocomplete: "tel-extension", spellcheck: "false" %> + <% if UserPolicy.new(current_user, @user).edit_organisation? %> + <% null_option = [OpenStruct.new(id: "", name: "Select an option")] %> + <% organisations = Organisation.filter_by_active.map { |org| OpenStruct.new(id: org.id, name: org.name) } %> + <% answer_options = null_option + organisations %> + + <%= f.govuk_select(:organisation_id, + label: { text: "Organisation", size: "m" }, + "data-controller": "accessible-autocomplete") do %> + <% answer_options.each do |answer| %> + + <% end %> + <% end %> + <% end %> + <% if current_user.data_coordinator? || current_user.support? %> <% roles = current_user.assignable_roles.map { |key, _| OpenStruct.new(id: key, name: key.to_s.humanize) } %> diff --git a/app/views/users/log_reassignment.html.erb b/app/views/users/log_reassignment.html.erb new file mode 100644 index 000000000..aef6d3f22 --- /dev/null +++ b/app/views/users/log_reassignment.html.erb @@ -0,0 +1,36 @@ +<% content_for :title, "Should this user’s logs move to their new organisation?" %> + +<% content_for :before_content do %> + <%= govuk_back_link(href: aliased_user_edit(@user, current_user)) %> +<% end %> + +<%= form_with model: @user, method: :patch, url: user_log_reassignment_path(@user) do |f| %> +
+
+ <%= f.govuk_error_summary %> + +

+ <%= content_for(:title) %> +

+ + <%= govuk_warning_text do %> + <%= organisation_change_warning(@user, @new_organisation) %> + <% end %> + + <% log_reassignment = User::LOG_REASSIGNMENT.map { |key, value| OpenStruct.new(id: key, name: value) } %> + + <%= f.govuk_collection_radio_buttons :log_reassignment, + log_reassignment, + :id, + :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 %> +
+
+
+<% end %> diff --git a/app/views/users/organisation_change_confirmation.html.erb b/app/views/users/organisation_change_confirmation.html.erb new file mode 100644 index 000000000..8dc56e17c --- /dev/null +++ b/app/views/users/organisation_change_confirmation.html.erb @@ -0,0 +1,24 @@ +<% content_for :before_content do %> + <% content_for :title, "Are you sure you want to move this user?" %> + <%= govuk_back_link(href: :back) %> +<% end %> + +
+
+

+ <%= content_for(:title) %> +

+ + <%= govuk_warning_text(text: organisation_change_confirmation_warning(@user, @new_organisation, @log_reassignment)) %> + + <%= form_with model: @user, url: user_organisation_change_confirmation_path(@user), method: :patch do |f| %> + <%= f.hidden_field :organisation_id, value: @new_organisation.id %> + <%= f.hidden_field :log_reassignment, value: @log_reassignment %> + +
+ <%= f.govuk_submit "Move this user" %> + <%= govuk_button_link_to "Cancel", user_log_reassignment_path(@user, organisation_id: @new_organisation.id, log_reassignment: @log_reassignment), secondary: true %> +
+ <% end %> +
+
diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index df8c0e915..7f42107f8 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -68,7 +68,15 @@ <%= summary_list.with_row do |row| row.with_key { "Organisation" } row.with_value { current_user.support? ? govuk_link_to(@user.organisation.name, lettings_logs_organisation_path(@user.organisation)) : @user.organisation.name } - row.with_action + if UserPolicy.new(current_user, @user).edit_organisation? + row.with_action( + visually_hidden_text: "organisation", + href: aliased_user_edit(@user, current_user), + html_attributes: { "data-qa": "change-organisation" }, + ) + else + row.with_action + end end %> <%= summary_list.with_row do |row| 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 4817bebb9..f2c86e37c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -124,6 +124,10 @@ Rails.application.routes.draw do resources :users 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" + get "organisation-change-confirmation", to: "users#organisation_change_confirmation" + patch "organisation-change-confirmation", to: "users#confirm_organisation_change" collection do get :search diff --git a/db/migrate/20240911152702_add_moved_user_to_bu.rb b/db/migrate/20240911152702_add_moved_user_to_bu.rb new file mode 100644 index 000000000..9a6079858 --- /dev/null +++ b/db/migrate/20240911152702_add_moved_user_to_bu.rb @@ -0,0 +1,5 @@ +class AddMovedUserToBu < ActiveRecord::Migration[7.0] + def change + add_column :bulk_uploads, :moved_user_id, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index 05f9bfde2..2d6808e6f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2024_08_22_080228) do +ActiveRecord::Schema[7.0].define(version: 2024_09_11_152702) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -42,6 +42,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_08_22_080228) do t.text "choice" t.integer "total_logs_count" t.string "rent_type_fix_status", default: "not_applied" + t.integer "moved_user_id" t.index ["identifier"], name: "index_bulk_uploads_on_identifier", unique: true t.index ["user_id"], name: "index_bulk_uploads_on_user_id" end diff --git a/spec/features/accessibility_spec.rb b/spec/features/accessibility_spec.rb index d0c218445..97f632d92 100644 --- a/spec/features/accessibility_spec.rb +++ b/spec/features/accessibility_spec.rb @@ -29,9 +29,15 @@ RSpec.describe "Accessibility", js: true do Rails.application.routes.routes.select { |route| route.verb == "GET" && route.path.spec.to_s.start_with?("/user") }.map { |route| route_path = route.path.spec.to_s route_path.gsub(":id", other_user.id.to_s).gsub(":user_id", other_user.id.to_s).gsub("(.:format)", "") + .gsub("log-reassignment", "log-reassignment?&organisation_id=#{other_user.organisation_id}") + .gsub("organisation-change-confirmation", "organisation-change-confirmation?&organisation_id=#{other_user.organisation_id}&log_reassignment=reassign_all") }.uniq end + before do + create(:lettings_log, assigned_to: other_user) + end + it "is has accessible pages" do user_paths.each do |path| visit(path) diff --git a/spec/helpers/user_helper_spec.rb b/spec/helpers/user_helper_spec.rb index 829195f6c..c88790aa0 100644 --- a/spec/helpers/user_helper_spec.rb +++ b/spec/helpers/user_helper_spec.rb @@ -61,4 +61,90 @@ RSpec.describe UserHelper do end end end + + describe "organisation_change_warning" do + context "when user doesn't own any logs" do + it "returns a message with the number of logs" do + expected_text = "You’re moving #{user.name} from #{user.organisation.name} to #{current_user.organisation.name}. There are 0 logs assigned to them." + expect(organisation_change_warning(user, current_user.organisation)).to eq(expected_text) + end + end + + context "when user owns 1 lettings log" do + before do + create(:lettings_log, assigned_to: user) + end + + it "returns a message with the number of logs" do + expected_text = "You’re moving #{user.name} from #{user.organisation.name} to #{current_user.organisation.name}. There is 1 log assigned to them." + expect(organisation_change_warning(user, current_user.organisation)).to eq(expected_text) + end + end + + context "when user owns 1 sales log" do + before do + create(:sales_log, assigned_to: user) + end + + it "returns a message with the number of logs" do + expected_text = "You’re moving #{user.name} from #{user.organisation.name} to #{current_user.organisation.name}. There is 1 log assigned to them." + expect(organisation_change_warning(user, current_user.organisation)).to eq(expected_text) + end + end + + context "when user owns multiple logs" do + before do + create(:lettings_log, assigned_to: user) + create(:sales_log, assigned_to: user) + end + + it "returns a message with the number of logs" do + expected_text = "You’re moving #{user.name} from #{user.organisation.name} to #{current_user.organisation.name}. There are 2 logs assigned to them." + expect(organisation_change_warning(user, current_user.organisation)).to eq(expected_text) + end + end + end + + describe "organisation_change_confirmation_warning" do + context "when user owns logs" do + before do + create(:lettings_log, assigned_to: user) + end + + context "with reassign all choice" do + it "returns the correct message" do + expected_text = "You’re moving #{user.name} from #{user.organisation.name} to #{current_user.organisation.name}. The stock owner and managing agent on their logs will change to #{current_user.organisation.name}." + expect(organisation_change_confirmation_warning(user, current_user.organisation, "reassign_all")).to eq(expected_text) + end + end + + context "with reassign stock owners choice" do + it "returns the correct message" do + expected_text = "You’re moving #{user.name} from #{user.organisation.name} to #{current_user.organisation.name}. The stock owner on their logs will change to #{current_user.organisation.name}." + expect(organisation_change_confirmation_warning(user, current_user.organisation, "reassign_stock_owner")).to eq(expected_text) + end + end + + context "with reassign managing agent choice" do + it "returns the correct message" do + expected_text = "You’re moving #{user.name} from #{user.organisation.name} to #{current_user.organisation.name}. The managing agent on their logs will change to #{current_user.organisation.name}." + expect(organisation_change_confirmation_warning(user, current_user.organisation, "reassign_managing_agent")).to eq(expected_text) + end + end + + context "with unassign choice" do + it "returns the correct message" do + expected_text = "You’re moving #{user.name} from #{user.organisation.name} to #{current_user.organisation.name}. Their logs will be unassigned." + expect(organisation_change_confirmation_warning(user, current_user.organisation, "unassign")).to eq(expected_text) + end + end + end + + context "when user doesn't own logs" do + it "returns the correct message" do + expected_text = "You’re moving #{user.name} from #{user.organisation.name} to #{current_user.organisation.name}. There are no logs assigned to them." + expect(organisation_change_confirmation_warning(user, current_user.organisation, "reassign_all")).to eq(expected_text) + end + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 6a04e9a0b..5cb6cb580 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -528,4 +528,304 @@ RSpec.describe User, type: :model do end end end + + describe "#reassign_logs_and_update_organisation" do + let(:user) { create(:user) } + let(:new_organisation) { create(:organisation) } + let!(:lettings_log) { create(:lettings_log, assigned_to: user) } + let!(:sales_log) { create(:sales_log, assigned_to: user) } + let(:notify_client) { instance_double(Notifications::Client) } + let(:devise_notify_mailer) { DeviseNotifyMailer.new } + + before do + allow(DeviseNotifyMailer).to receive(:new).and_return(devise_notify_mailer) + allow(devise_notify_mailer).to receive(:notify_client).and_return(notify_client) + allow(notify_client).to receive(:send_email).and_return(true) + end + + context "when reassigning all orgs for logs" do + it "reassigns all logs to the new organisation" do + user.reassign_logs_and_update_organisation(new_organisation, "reassign_all") + expect(lettings_log.reload.owning_organisation).to eq(new_organisation) + expect(lettings_log.managing_organisation).to eq(new_organisation) + expect(lettings_log.values_updated_at).not_to be_nil + expect(sales_log.reload.owning_organisation).to eq(new_organisation) + expect(sales_log.managing_organisation).to eq(new_organisation) + expect(sales_log.values_updated_at).not_to be_nil + end + + it "moves the user to the new organisation" do + user.reassign_logs_and_update_organisation(new_organisation, "reassign_all") + + expect(user.organisation).to eq(new_organisation) + end + + it "sends organisation update emails" do + expected_personalisation = { + from_organisation: "#{user.organisation.name} (Organisation ID: #{user.organisation_id})", + to_organisation: "#{new_organisation.name} (Organisation ID: #{new_organisation.id})", + reassigned_logs_text: "There are 2 logs assigned to you. The stock owner and managing agent on these logs has been changed from #{user.organisation.name} to #{new_organisation.name}.", + } + user.reassign_logs_and_update_organisation(new_organisation, "reassign_all") + + expect(notify_client).to have_received(:send_email).with(email_address: user.email, template_id: User::ORGANISATION_UPDATE_TEMPLATE_ID, personalisation: expected_personalisation).once + end + + context "and there is an error" do + before do + allow(user).to receive(:update!).and_raise(ActiveRecord::RecordInvalid) + end + + it "rolls back the changes" do + user.reassign_logs_and_update_organisation(new_organisation, "reassign_all") + expect(lettings_log.reload.owning_organisation).not_to eq(new_organisation) + expect(lettings_log.managing_organisation).not_to eq(new_organisation) + expect(lettings_log.values_updated_at).to be_nil + expect(sales_log.reload.owning_organisation).not_to eq(new_organisation) + expect(sales_log.managing_organisation).not_to eq(new_organisation) + expect(sales_log.values_updated_at).to be_nil + expect(user.organisation).not_to eq(new_organisation) + end + end + + context "and the user has pending logs assigned to them" do + let(:lettings_bu) { create(:bulk_upload, :lettings) } + let(:sales_bu) { create(:bulk_upload, :sales) } + let!(:pending_lettings_log) { build(:lettings_log, status: "pending", assigned_to: user, bulk_upload: lettings_bu) } + let!(:pending_sales_log) { build(:sales_log, status: "pending", assigned_to: user, bulk_upload: sales_bu) } + + before do + pending_lettings_log.skip_update_status = true + pending_lettings_log.save! + pending_sales_log.skip_update_status = true + pending_sales_log.save! + end + + it "sets choice for fixing the logs to cancelled-by-moved-user" do + user.reassign_logs_and_update_organisation(new_organisation, "reassign_all") + + expect(lettings_bu.reload.choice).to eq("cancelled-by-moved-user") + expect(sales_bu.reload.choice).to eq("cancelled-by-moved-user") + expect(lettings_bu.moved_user_id).to eq(user.id) + expect(sales_bu.moved_user_id).to eq(user.id) + + expect(pending_lettings_log.reload.status).to eq("pending") + expect(pending_sales_log.reload.status).to eq("pending") + end + end + end + + context "when reassigning stock owners for logs" do + it "reassigns stock owners for logs to the new organisation" do + user.reassign_logs_and_update_organisation(new_organisation, "reassign_stock_owner") + expect(lettings_log.reload.owning_organisation).to eq(new_organisation) + expect(lettings_log.managing_organisation).not_to eq(new_organisation) + expect(lettings_log.values_updated_at).not_to be_nil + expect(sales_log.reload.owning_organisation).to eq(new_organisation) + expect(sales_log.managing_organisation).not_to eq(new_organisation) + expect(sales_log.values_updated_at).not_to be_nil + end + + it "moves the user to the new organisation" do + user.reassign_logs_and_update_organisation(new_organisation, "reassign_stock_owner") + + expect(user.organisation).to eq(new_organisation) + end + + it "sends organisation update emails" do + expected_personalisation = { + from_organisation: "#{user.organisation.name} (Organisation ID: #{user.organisation_id})", + to_organisation: "#{new_organisation.name} (Organisation ID: #{new_organisation.id})", + reassigned_logs_text: "There are 2 logs assigned to you. The stock owner on these logs has been changed from #{user.organisation.name} to #{new_organisation.name}.", + } + user.reassign_logs_and_update_organisation(new_organisation, "reassign_stock_owner") + + expect(notify_client).to have_received(:send_email).with(email_address: user.email, template_id: User::ORGANISATION_UPDATE_TEMPLATE_ID, personalisation: expected_personalisation).once + end + + context "and there is an error" do + before do + allow(user).to receive(:update!).and_raise(ActiveRecord::RecordInvalid) + end + + it "rolls back the changes" do + user.reassign_logs_and_update_organisation(new_organisation, "reassign_all") + expect(lettings_log.reload.owning_organisation).not_to eq(new_organisation) + expect(lettings_log.managing_organisation).not_to eq(new_organisation) + expect(lettings_log.values_updated_at).to be_nil + expect(sales_log.reload.owning_organisation).not_to eq(new_organisation) + expect(sales_log.managing_organisation).not_to eq(new_organisation) + expect(sales_log.values_updated_at).to be_nil + expect(user.organisation).not_to eq(new_organisation) + end + end + end + + context "when reassigning managing agents for logs" do + it "reassigns managing agents for logs to the new organisation" do + user.reassign_logs_and_update_organisation(new_organisation, "reassign_managing_agent") + expect(lettings_log.reload.owning_organisation).not_to eq(new_organisation) + expect(lettings_log.managing_organisation).to eq(new_organisation) + expect(lettings_log.values_updated_at).not_to be_nil + expect(sales_log.reload.owning_organisation).not_to eq(new_organisation) + expect(sales_log.managing_organisation).to eq(new_organisation) + expect(sales_log.values_updated_at).not_to be_nil + end + + it "moves the user to the new organisation" do + user.reassign_logs_and_update_organisation(new_organisation, "reassign_managing_agent") + + expect(user.organisation).to eq(new_organisation) + end + + it "sends organisation update emails" do + expected_personalisation = { + from_organisation: "#{user.organisation.name} (Organisation ID: #{user.organisation_id})", + to_organisation: "#{new_organisation.name} (Organisation ID: #{new_organisation.id})", + reassigned_logs_text: "There are 2 logs assigned to you. The managing agent on these logs has been changed from #{user.organisation.name} to #{new_organisation.name}.", + } + user.reassign_logs_and_update_organisation(new_organisation, "reassign_managing_agent") + + expect(notify_client).to have_received(:send_email).with(email_address: user.email, template_id: User::ORGANISATION_UPDATE_TEMPLATE_ID, personalisation: expected_personalisation).once + end + + context "and there is an error" do + before do + allow(user).to receive(:update!).and_raise(ActiveRecord::RecordInvalid) + end + + it "rolls back the changes" do + user.reassign_logs_and_update_organisation(new_organisation, "reassign_all") + expect(lettings_log.reload.owning_organisation).not_to eq(new_organisation) + expect(lettings_log.managing_organisation).not_to eq(new_organisation) + expect(lettings_log.values_updated_at).to be_nil + expect(sales_log.reload.owning_organisation).not_to eq(new_organisation) + expect(sales_log.managing_organisation).not_to eq(new_organisation) + expect(sales_log.values_updated_at).to be_nil + expect(user.organisation).not_to eq(new_organisation) + end + end + end + + context "when unassigning the logs" do + context "and unassigned user exists" do + let!(:unassigned_user) { create(:user, name: "Unassigned", organisation: user.organisation) } + + it "reassigns all the logs to the unassigned user" do + user.reassign_logs_and_update_organisation(new_organisation, "unassign") + + expect(lettings_log.reload.assigned_to).to eq(unassigned_user) + expect(lettings_log.values_updated_at).not_to be_nil + expect(sales_log.reload.assigned_to).to eq(unassigned_user) + expect(sales_log.values_updated_at).not_to be_nil + end + + it "moves the user to the new organisation" do + user.reassign_logs_and_update_organisation(new_organisation, "unassign") + + expect(user.organisation).to eq(new_organisation) + end + + it "sends organisation update emails" do + expected_personalisation = { + from_organisation: "#{user.organisation.name} (Organisation ID: #{user.organisation_id})", + to_organisation: "#{new_organisation.name} (Organisation ID: #{new_organisation.id})", + reassigned_logs_text: "There are 2 logs assigned to you. These have now been unassigned.", + } + user.reassign_logs_and_update_organisation(new_organisation, "unassign") + + expect(notify_client).to have_received(:send_email).with(email_address: user.email, template_id: User::ORGANISATION_UPDATE_TEMPLATE_ID, personalisation: expected_personalisation).once + end + + context "and there is an error" do + before do + allow(user).to receive(:update!).and_raise(ActiveRecord::RecordInvalid) + end + + it "rolls back the changes" do + user.reassign_logs_and_update_organisation(new_organisation, "unassign") + expect(lettings_log.reload.assigned_to).to eq(user) + expect(lettings_log.values_updated_at).to be_nil + expect(sales_log.reload.assigned_to).to eq(user) + expect(sales_log.values_updated_at).to be_nil + expect(user.organisation).not_to eq(new_organisation) + end + end + end + + context "and unassigned user doesn't exist" do + it "reassigns all the logs to the unassigned user" do + user.reassign_logs_and_update_organisation(new_organisation, "unassign") + + expect(lettings_log.reload.assigned_to.name).to eq("Unassigned") + expect(lettings_log.values_updated_at).not_to be_nil + expect(sales_log.reload.assigned_to.name).to eq("Unassigned") + expect(sales_log.values_updated_at).not_to be_nil + end + + it "moves the user to the new organisation" do + user.reassign_logs_and_update_organisation(new_organisation, "unassign") + + expect(user.organisation).to eq(new_organisation) + end + + context "and there is an error" do + before do + allow(user).to receive(:update!).and_raise(ActiveRecord::RecordInvalid) + end + + it "rolls back the changes" do + user.reassign_logs_and_update_organisation(new_organisation, "unassign") + expect(lettings_log.reload.assigned_to).to eq(user) + expect(lettings_log.values_updated_at).to be_nil + expect(sales_log.reload.assigned_to).to eq(user) + expect(sales_log.values_updated_at).to be_nil + expect(user.organisation).not_to eq(new_organisation) + end + end + end + end + + context "when log_reassignent is not given" do + context "and user has no logs" do + let(:user_without_logs) { create(:user) } + + it "moves the user to the new organisation" do + user_without_logs.reassign_logs_and_update_organisation(new_organisation, nil) + + expect(user_without_logs.organisation).to eq(new_organisation) + end + + context "and there is an error" do + before do + allow(user_without_logs).to receive(:update!).and_raise(ActiveRecord::RecordInvalid) + end + + it "rolls back the changes" do + user_without_logs.reassign_logs_and_update_organisation(new_organisation, nil) + expect(user_without_logs.organisation).not_to eq(new_organisation) + end + end + + it "sends organisation update emails" do + expected_personalisation = { + from_organisation: "#{user_without_logs.organisation.name} (Organisation ID: #{user_without_logs.organisation_id})", + to_organisation: "#{new_organisation.name} (Organisation ID: #{new_organisation.id})", + reassigned_logs_text: "", + } + user_without_logs.reassign_logs_and_update_organisation(new_organisation, nil) + + expect(notify_client).to have_received(:send_email).with(email_address: user_without_logs.email, template_id: User::ORGANISATION_UPDATE_TEMPLATE_ID, personalisation: expected_personalisation).once + end + end + + context "and user has logs" do + it "does not move the user to the new organisation" do + user.reassign_logs_and_update_organisation(new_organisation, nil) + + expect(user.organisation).not_to eq(new_organisation) + end + end + end + end end diff --git a/spec/policies/user_policy_spec.rb b/spec/policies/user_policy_spec.rb index e2266cb48..63f3317d8 100644 --- a/spec/policies/user_policy_spec.rb +++ b/spec/policies/user_policy_spec.rb @@ -100,6 +100,20 @@ RSpec.describe UserPolicy do end end + permissions :edit_organisation? do + it "as a provider it does not allow changing organisation" do + expect(policy).not_to permit(data_provider, data_provider) + end + + it "as a coordinator it does not allow changing organisatio" do + expect(policy).not_to permit(data_coordinator, data_provider) + end + + it "as a support user allows changing other user's organisation" do + expect(policy).to permit(support, data_provider) + end + end + permissions :delete? do context "with active user" do let(:user) { create(:user, last_sign_in_at: Time.zone.yesterday) } diff --git a/spec/requests/bulk_upload_lettings_results_controller_spec.rb b/spec/requests/bulk_upload_lettings_results_controller_spec.rb index 50f5ecd31..dc78c9d78 100644 --- a/spec/requests/bulk_upload_lettings_results_controller_spec.rb +++ b/spec/requests/bulk_upload_lettings_results_controller_spec.rb @@ -60,6 +60,28 @@ RSpec.describe BulkUploadLettingsResultsController, type: :request do expect(response).to be_successful expect(response.body).to include(bulk_upload.filename) end + + context "and bulk upload has been cancelled by not the current moved user" do + let(:bulk_upload) { create(:bulk_upload, :lettings, user:, bulk_upload_errors:, choice: "cancelled-by-moved-user", moved_user_id: user.id) } + + it "is displays a correct banner" do + get "/lettings-logs/bulk-upload-results/#{bulk_upload.id}/summary" + + expect(response.body).to include("This error report is out of date.") + expect(response.body).to include("Some logs in this upload are assigned to #{user.name}, who has moved to a different organisation since this file was uploaded. Reupload the file to get an accurate error report.") + end + end + + context "and bulk upload has been cancelled by the current moved user" do + let(:bulk_upload) { create(:bulk_upload, :lettings, user:, bulk_upload_errors:, choice: "cancelled-by-moved-user", moved_user_id: other_user.id) } + + it "is displays a correct banner" do + get "/lettings-logs/bulk-upload-results/#{bulk_upload.id}/summary" + + expect(response.body).to include("This error report is out of date.") + expect(response.body).to include("You moved to a different organisation since this file was uploaded. Reupload the file to get an accurate error report.") + end + end end end @@ -107,5 +129,28 @@ RSpec.describe BulkUploadLettingsResultsController, type: :request do expect(response).to be_not_found end end + + context "and bulk upload has been cancelled by not the current moved user" do + let(:other_user) { create(:user, organisation: user.organisation) } + let(:bulk_upload) { create(:bulk_upload, :lettings, user:, bulk_upload_errors:, choice: "cancelled-by-moved-user", moved_user_id: other_user.id) } + + it "is displays a correct banner" do + get "/lettings-logs/bulk-upload-results/#{bulk_upload.id}/summary" + + expect(response.body).to include("This error report is out of date.") + expect(response.body).to include("Some logs in this upload are assigned to #{other_user.name}, who has moved to a different organisation since this file was uploaded. Reupload the file to get an accurate error report.") + end + end + + context "and bulk upload has been cancelled by the current moved user" do + let(:bulk_upload) { create(:bulk_upload, :lettings, user:, bulk_upload_errors:, choice: "cancelled-by-moved-user", moved_user_id: user.id) } + + it "is displays a correct banner" do + get "/lettings-logs/bulk-upload-results/#{bulk_upload.id}/summary" + + expect(response.body).to include("This error report is out of date.") + expect(response.body).to include("You moved to a different organisation since this file was uploaded. Reupload the file to get an accurate error report.") + end + end end end diff --git a/spec/requests/bulk_upload_lettings_resume_controller_spec.rb b/spec/requests/bulk_upload_lettings_resume_controller_spec.rb index d110768f0..f6ac9bb4a 100644 --- a/spec/requests/bulk_upload_lettings_resume_controller_spec.rb +++ b/spec/requests/bulk_upload_lettings_resume_controller_spec.rb @@ -32,6 +32,26 @@ RSpec.describe BulkUploadLettingsResumeController, type: :request do expect(response.body).to include("You have created logs from your bulk upload, and the logs are complete. Return to lettings logs to view them.") end end + + context "when a choice to reupload has been made" do + it "redirects to the error report" do + bulk_upload.update!(choice: "upload-again") + + get "/lettings-logs/bulk-upload-resume/#{bulk_upload.id}/start" + follow_redirect! + expect(response).to redirect_to("/lettings-logs/bulk-upload-results/#{bulk_upload.id}") + end + end + + context "when bulk upload was cancelled by moved user" do + it "redirects to the error report" do + bulk_upload.update!(choice: "cancelled-by-moved-user") + + get "/lettings-logs/bulk-upload-resume/#{bulk_upload.id}/start" + follow_redirect! + expect(response).to redirect_to("/lettings-logs/bulk-upload-results/#{bulk_upload.id}") + end + end end describe "GET /lettings-logs/bulk-upload-resume/:ID/fix-choice" do @@ -73,6 +93,25 @@ RSpec.describe BulkUploadLettingsResumeController, type: :request do expect(response).to redirect_to("/lettings-logs/bulk-upload-soft-validations-check/#{bulk_upload.id}/chosen") end end + + context "when a choice to reupload has been made" do + let(:bulk_upload) { create(:bulk_upload, :lettings, user:, bulk_upload_errors:, choice: "upload-again") } + + it "redirects to the error report" do + get "/lettings-logs/bulk-upload-resume/#{bulk_upload.id}/fix-choice" + expect(response).to redirect_to("/lettings-logs/bulk-upload-results/#{bulk_upload.id}") + end + end + + context "when bulk upload was cancelled by moved user" do + let(:bulk_upload) { create(:bulk_upload, :lettings, user:, bulk_upload_errors:, choice: "cancelled-by-moved-user") } + + it "redirects to the error report" do + get "/lettings-logs/bulk-upload-resume/#{bulk_upload.id}/start" + follow_redirect! + expect(response).to redirect_to("/lettings-logs/bulk-upload-results/#{bulk_upload.id}") + end + end end describe "GET /lettings-logs/bulk-upload-resume/:ID/fix-choice?soft_errors_only=true" do diff --git a/spec/requests/bulk_upload_sales_results_controller_spec.rb b/spec/requests/bulk_upload_sales_results_controller_spec.rb index ac759529a..1bd171dec 100644 --- a/spec/requests/bulk_upload_sales_results_controller_spec.rb +++ b/spec/requests/bulk_upload_sales_results_controller_spec.rb @@ -11,6 +11,42 @@ RSpec.describe BulkUploadSalesResultsController, type: :request do sign_in viewing_user end + describe "GET /sales-logs/bulk-upload-results/:ID/summary" do + context "when viewed by another user in the same org" do + let(:other_user) { create(:user, organisation: user.organisation) } + let(:viewing_user) { other_user } + + it "is accessible" do + get "/sales-logs/bulk-upload-results/#{bulk_upload.id}/summary" + + expect(response).to be_successful + expect(response.body).to include(bulk_upload.filename) + end + + context "and bulk upload has been cancelled by not the current moved user" do + let(:bulk_upload) { create(:bulk_upload, :sales, user:, bulk_upload_errors:, choice: "cancelled-by-moved-user", moved_user_id: user.id) } + + it "is displays a correct banner" do + get "/sales-logs/bulk-upload-results/#{bulk_upload.id}/summary" + + expect(response.body).to include("This error report is out of date.") + expect(response.body).to include("Some logs in this upload are assigned to #{user.name}, who has moved to a different organisation since this file was uploaded. Reupload the file to get an accurate error report.") + end + end + + context "and bulk upload has been cancelled by the current moved user" do + let(:bulk_upload) { create(:bulk_upload, :sales, user:, bulk_upload_errors:, choice: "cancelled-by-moved-user", moved_user_id: other_user.id) } + + it "is displays a correct banner" do + get "/sales-logs/bulk-upload-results/#{bulk_upload.id}/summary" + + expect(response.body).to include("This error report is out of date.") + expect(response.body).to include("You moved to a different organisation since this file was uploaded. Reupload the file to get an accurate error report.") + end + end + end + end + describe "GET /sales-logs/bulk-upload-results/:ID" do it "renders correct year" do get "/sales-logs/bulk-upload-results/#{bulk_upload.id}" @@ -68,5 +104,28 @@ RSpec.describe BulkUploadSalesResultsController, type: :request do expect(response).to be_not_found end end + + context "and bulk upload has been cancelled by not the current moved user" do + let(:other_user) { create(:user, organisation: user.organisation) } + let(:bulk_upload) { create(:bulk_upload, :sales, user:, bulk_upload_errors:, choice: "cancelled-by-moved-user", moved_user_id: other_user.id) } + + it "is displays a correct banner" do + get "/sales-logs/bulk-upload-results/#{bulk_upload.id}/summary" + + expect(response.body).to include("This error report is out of date.") + expect(response.body).to include("Some logs in this upload are assigned to #{other_user.name}, who has moved to a different organisation since this file was uploaded. Reupload the file to get an accurate error report.") + end + end + + context "and bulk upload has been cancelled by the current moved user" do + let(:bulk_upload) { create(:bulk_upload, :sales, user:, bulk_upload_errors:, choice: "cancelled-by-moved-user", moved_user_id: user.id) } + + it "is displays a correct banner" do + get "/sales-logs/bulk-upload-results/#{bulk_upload.id}/summary" + + expect(response.body).to include("This error report is out of date.") + expect(response.body).to include("You moved to a different organisation since this file was uploaded. Reupload the file to get an accurate error report.") + end + end end end diff --git a/spec/requests/bulk_upload_sales_resume_controller_spec.rb b/spec/requests/bulk_upload_sales_resume_controller_spec.rb index 36db459d3..9c0efded8 100644 --- a/spec/requests/bulk_upload_sales_resume_controller_spec.rb +++ b/spec/requests/bulk_upload_sales_resume_controller_spec.rb @@ -32,6 +32,26 @@ RSpec.describe BulkUploadSalesResumeController, type: :request do expect(response.body).to include("You have created logs from your bulk upload, and the logs are complete. Return to sales logs to view them.") end end + + context "when a choice to reupload has been made" do + it "redirects to the error report" do + bulk_upload.update!(choice: "upload-again") + + get "/sales-logs/bulk-upload-resume/#{bulk_upload.id}/start" + follow_redirect! + expect(response).to redirect_to("/sales-logs/bulk-upload-results/#{bulk_upload.id}") + end + end + + context "when bulk upload was cancelled by moved user" do + it "redirects to the error report" do + bulk_upload.update!(choice: "cancelled-by-moved-user") + + get "/sales-logs/bulk-upload-resume/#{bulk_upload.id}/start" + follow_redirect! + expect(response).to redirect_to("/sales-logs/bulk-upload-results/#{bulk_upload.id}") + end + end end describe "GET /sales-logs/bulk-upload-resume/:ID/fix-choice" do @@ -73,6 +93,25 @@ RSpec.describe BulkUploadSalesResumeController, type: :request do expect(response).to redirect_to("/sales-logs/bulk-upload-soft-validations-check/#{bulk_upload.id}/chosen") end end + + context "when a choice to reupload has been made" do + let(:bulk_upload) { create(:bulk_upload, :sales, user:, bulk_upload_errors:, choice: "upload-again") } + + it "redirects to the error report" do + get "/sales-logs/bulk-upload-resume/#{bulk_upload.id}/fix-choice" + expect(response).to redirect_to("/sales-logs/bulk-upload-results/#{bulk_upload.id}") + end + end + + context "when bulk upload was cancelled by moved user" do + let(:bulk_upload) { create(:bulk_upload, :sales, user:, bulk_upload_errors:, choice: "cancelled-by-moved-user") } + + it "redirects to the error report" do + get "/sales-logs/bulk-upload-resume/#{bulk_upload.id}/start" + follow_redirect! + expect(response).to redirect_to("/sales-logs/bulk-upload-results/#{bulk_upload.id}") + end + end end describe "GET /sales-logs/bulk-upload-resume/:ID/fix-choice?soft_errors_only=true" do diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index dac2806a5..1ab5aa9d4 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -124,6 +124,13 @@ RSpec.describe UsersController, type: :request do expect(response).to redirect_to("/account/sign-in") end end + + describe "#log_reassignment" do + it "redirects to the sign in page" do + get "/users/#{user.id}/log-reassignment" + expect(response).to redirect_to("/account/sign-in") + end + end end context "when user is signed in as a data provider" do @@ -149,6 +156,7 @@ RSpec.describe UsersController, type: :request do expect(page).not_to have_link("Change", text: "role") expect(page).not_to have_link("Change", text: "if data protection officer") expect(page).not_to have_link("Change", text: "if a key contact") + expect(page).not_to have_link("Change", text: "organisation") end it "does not allow deactivating the user" do @@ -208,6 +216,7 @@ RSpec.describe UsersController, type: :request do expect(page).not_to have_link("Change", text: "role") expect(page).not_to have_link("Change", text: "if data protection officer") expect(page).not_to have_link("Change", text: "if a key contact") + expect(page).not_to have_link("Change", text: "organisation") end it "does not allow deactivating the user" do @@ -258,6 +267,7 @@ RSpec.describe UsersController, type: :request do expect(page).not_to have_field("user[role]") expect(page).not_to have_field("user[is_dpo]") expect(page).not_to have_field("user[is_key_contact]") + expect(page).not_to have_field("user[organisation_id]") end end @@ -430,6 +440,13 @@ RSpec.describe UsersController, type: :request do expect(result.keys).to match_array([org_user.id.to_s, managing_user.id.to_s]) end end + + describe "#log_reassignment" do + it "returns unauthorized status" do + get "/users/#{user.id}/log-reassignment" + expect(response).to have_http_status(:unauthorized) + end + end end context "when user is signed in as a data coordinator" do @@ -607,6 +624,7 @@ RSpec.describe UsersController, type: :request do expect(page).to have_link("Change", text: "role") expect(page).to have_link("Change", text: "if data protection officer") expect(page).to have_link("Change", text: "if a key contact") + expect(page).not_to have_link("Change", text: "organisation") end it "does not allow deactivating the user" do @@ -655,6 +673,7 @@ RSpec.describe UsersController, type: :request do expect(page).to have_link("Change", text: "role") expect(page).to have_link("Change", text: "if data protection officer") expect(page).to have_link("Change", text: "if a key contact") + expect(page).not_to have_link("Change", text: "organisation") end it "allows deactivating the user" do @@ -713,6 +732,7 @@ RSpec.describe UsersController, type: :request do expect(page).to have_field("user[name]") expect(page).to have_field("user[email]") expect(page).to have_field("user[role]") + expect(page).not_to have_field("user[organisation_id]") end it "does not allow setting the role to `support`" do @@ -738,6 +758,7 @@ RSpec.describe UsersController, type: :request do expect(page).to have_field("user[name]") expect(page).to have_field("user[email]") expect(page).to have_field("user[role]") + expect(page).not_to have_field("user[organisation_id]") end end @@ -1227,6 +1248,13 @@ RSpec.describe UsersController, type: :request do expect(result.keys).to match_array([org_user.id.to_s, managing_user.id.to_s]) end end + + describe "#log_reassignment" do + it "returns unauthorised status" do + get "/users/#{user.id}/log-reassignment" + expect(response).to have_http_status(:unauthorized) + end + end end context "when user is signed in as a support user" do @@ -1459,6 +1487,7 @@ RSpec.describe UsersController, type: :request do expect(page).to have_link("Change", text: "role") expect(page).to have_link("Change", text: "if data protection officer") expect(page).to have_link("Change", text: "if a key contact") + expect(page).to have_link("Change", text: "organisation") end it "does not allow deactivating the user" do @@ -1488,6 +1517,7 @@ RSpec.describe UsersController, type: :request do expect(page).to have_link("Change", text: "role") expect(page).to have_link("Change", text: "if data protection officer") expect(page).to have_link("Change", text: "if a key contact") + expect(page).to have_link("Change", text: "organisation") end it "links to user organisation" do @@ -1626,6 +1656,7 @@ RSpec.describe UsersController, type: :request do expect(page).to have_field("user[role]") expect(page).to have_field("user[phone]") expect(page).to have_field("user[phone_extension]") + expect(page).to have_field("user[organisation_id]") end it "allows setting the role to `support`" do @@ -1653,6 +1684,7 @@ RSpec.describe UsersController, type: :request do expect(page).to have_field("user[role]") expect(page).to have_field("user[phone]") expect(page).to have_field("user[phone_extension]") + expect(page).to have_field("user[organisation_id]") end end @@ -1673,6 +1705,7 @@ RSpec.describe UsersController, type: :request do expect(page).to have_field("user[role]") expect(page).to have_field("user[phone]") expect(page).to have_field("user[phone_extension]") + expect(page).to have_field("user[organisation_id]") end end @@ -1682,10 +1715,11 @@ RSpec.describe UsersController, type: :request do get "/users/#{other_user.id}/edit", headers:, params: {} end - it "redirects to user details page" do - expect(response).to redirect_to("/users/#{other_user.id}") - follow_redirect! - expect(page).not_to have_link("Change") + it "allows editing the user" do + expect(page).to have_field("user[name]") + expect(page).to have_field("user[email]") + expect(page).to have_field("user[role]") + expect(page).to have_field("user[organisation_id]") end end end @@ -1820,6 +1854,20 @@ RSpec.describe UsersController, type: :request do patch "/users/#{other_user.id}", headers:, params: end end + + context "and email address hasn't changed" do + let(:params) { { id: user.id, user: { name: new_name, email: other_user.email, is_dpo: "true", is_key_contact: "true" } } } + + before do + user.legacy_users.destroy_all + end + + it "shows flash notice" do + patch("/users/#{other_user.id}", headers:, params:) + + expect(flash[:notice]).to be_nil + end + end end context "when we update the user password" do @@ -1838,6 +1886,39 @@ RSpec.describe UsersController, type: :request do expect(page).to have_selector(".govuk-error-summary__title") end end + + context "when updating organisation" do + let(:new_organisation) { create(:organisation) } + + before do + patch "/users/#{user.id}", headers:, params: + end + + context "and organisation id is nil" do + let(:params) { { id: 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: 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/#{user.id}/log-reassignment?organisation_id=#{new_organisation.id}") + end + + it "updated other fields" do + expect(user.reload.name).to eq("new_name") + end + end + end end context "when the current user does not match the user ID" do @@ -1890,89 +1971,237 @@ 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 + + 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 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 "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) - 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 + 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 - 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) + 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 "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 } - } + context "and log reassignment choice is not present" do + let(:params) { { user: { organisation_id: new_organisation.id, log_reassignment: nil } } } + + before do + patch "/users/#{other_user.id}/log-reassignment", headers:, params: end - let(:personalisation) do - { - name: params[:user][:name], - email: new_email, - organisation: other_user.organisation.name, - link: include("/account/confirmation?confirmation_token="), - } + it "does not update the user's organisation" do + expect(other_user.reload.organisation).not_to eq(new_organisation) + end + + 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 + + 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 "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}/organisation-change-confirmation?log_reassignment=reassign_all&organisation_id=#{new_organisation.id}") + end + end + + context "and log reassignment choice is to unassign logs" do + let(:params) { { user: { organisation_id: new_organisation.id, log_reassignment: "unassign" } } } + before do - other_user.legacy_users.destroy_all + patch "/users/#{other_user.id}/log-reassignment", headers:, params: end - it "shows flash notice" do - 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 - expect(flash[:notice]).to eq("An email has been sent to #{new_email} to confirm this change.") + it "redirects to confirmation page" do + expect(response).to redirect_to("/users/#{other_user.id}/organisation-change-confirmation?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" } } } + + 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 "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 "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_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 log reassignment choice is to change managing agent" do + let(:params) { { user: { organisation_id: new_organisation.id, log_reassignment: "reassign_managing_agent" } } } + + 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 "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 + + 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 end @@ -2194,6 +2423,151 @@ RSpec.describe UsersController, type: :request do expect(result.keys).to match_array([org_user.id.to_s, managing_user.id.to_s, owner_user.id.to_s, other_user.id.to_s]) end end + + describe "#log_reassignment" do + context "when organisation id is not given" do + before do + create(:lettings_log, assigned_to: other_user) + end + + it "redirects to the user page" do + get "/users/#{other_user.id}/log-reassignment" + expect(response).to redirect_to("/users/#{other_user.id}") + end + end + + context "when organisation id does not exist" do + before do + create(:lettings_log, assigned_to: other_user) + end + + it "redirects to the user page" do + get "/users/#{other_user.id}/log-reassignment?organisation_id=123123" + expect(response).to redirect_to("/users/#{other_user.id}") + end + end + + context "with valid organisation id" do + let(:new_organisation) { create(:organisation, name: "new org") } + + context "and user has assigned logs" do + before do + create(:lettings_log, assigned_to: other_user) + end + + it "allows reassigning logs" do + get "/users/#{other_user.id}/log-reassignment?organisation_id=#{new_organisation.id}" + expect(page).to have_content("Should this user’s logs move to their new organisation?") + expect(page).to have_content("You’re moving #{other_user.name} from #{other_user.organisation_name} to new org. There is 1 log assigned to them.") + expect(page).to have_button("Continue") + expect(page).to have_link("Back", href: "/users/#{other_user.id}/edit") + expect(page).to have_link("Cancel", href: "/users/#{other_user.id}/edit") + end + end + + context "and user has no assigned logs" do + it "redirects to confirm organisation change page" do + get "/users/#{other_user.id}/log-reassignment?organisation_id=#{new_organisation.id}" + expect(response).to redirect_to("/users/#{other_user.id}/organisation-change-confirmation?organisation_id=#{new_organisation.id}") + end + end + end + end + + describe "#confirm_organisation_change" do + context "when organisation id is not given" do + it "redirects to the user page" do + get "/users/#{other_user.id}/organisation-change-confirmation?log_reassignment=reassign_all" + expect(response).to redirect_to("/users/#{other_user.id}") + end + end + + context "when reassignment option is not given" do + let(:new_organisation) { create(:organisation, name: "new org") } + + before do + create(:lettings_log, assigned_to: other_user) + end + + it "redirects to the user page" do + get "/users/#{other_user.id}/organisation-change-confirmation?organisation_id=#{new_organisation.id}" + expect(response).to redirect_to("/users/#{other_user.id}") + end + end + + context "when organisation id does not exist" do + it "redirects to the user page" do + get "/users/#{other_user.id}/organisation-change-confirmation?organisation_id=123123&log_reassignment=reassign_all" + expect(response).to redirect_to("/users/#{other_user.id}") + end + end + + context "with valid organisation id" do + let(:new_organisation) { create(:organisation, name: "new org") } + + before do + create(:lettings_log, assigned_to: other_user) + end + + it "displays confirm organisation change page" do + get "/users/#{other_user.id}/organisation-change-confirmation?organisation_id=#{new_organisation.id}&log_reassignment=reassign_all" + expect(page).to have_content("Are you sure you want to move this user?") + expect(page).to have_content("You’re moving #{other_user.name} from #{other_user.organisation_name} to #{new_organisation.name}. The stock owner and managing agent on their logs will change to #{new_organisation.name}.") + end + end + end + + describe "#confirm_organisation_change patch" do + context "when organisation id is not given" do + let(:params) { { user: { organisation_id: nil, log_reassignment: "reassign_all" } } } + + it "redirects to the user page" do + patch "/users/#{other_user.id}/organisation-change-confirmation", headers:, params: params + expect(response).to redirect_to("/users/#{other_user.id}") + end + end + + context "when reassignment option is not given" do + let(:new_organisation) { create(:organisation, name: "new org") } + let(:params) { { user: { organisation_id: new_organisation.id, log_reassignment: "reassign_all" } } } + + it "redirects to the user page" do + patch "/users/#{other_user.id}/organisation-change-confirmation", headers:, params: params + expect(response).to redirect_to("/users/#{other_user.id}") + end + end + + context "when organisation id does not exist" do + let(:params) { { user: { organisation_id: 123_123, log_reassignment: "reassign_all" } } } + + it "redirects to the user page" do + patch "/users/#{other_user.id}/organisation-change-confirmation", headers:, params: params + expect(response).to redirect_to("/users/#{other_user.id}") + end + end + + context "with valid organisation id" do + let(:new_organisation) { create(:organisation, name: "new org") } + let(:params) { { user: { organisation_id: new_organisation.id, log_reassignment: "reassign_all" } } } + let!(:lettings_log) { create(:lettings_log, assigned_to: other_user) } + let!(:sales_log) { create(:sales_log, assigned_to: other_user) } + + context "and reassign all option" do + it "updates logs and moves the user" do + patch "/users/#{other_user.id}/organisation-change-confirmation", headers:, params: params + expect(response).to redirect_to("/users/#{other_user.id}") + + expect(other_user.reload.organisation).to eq(new_organisation) + expect(other_user.lettings_logs.count).to eq(1) + expect(other_user.sales_logs.count).to eq(1) + expect(lettings_log.reload.managing_organisation).to eq(new_organisation) + expect(lettings_log.owning_organisation).to eq(new_organisation) + expect(sales_log.reload.managing_organisation).to eq(new_organisation) + expect(sales_log.owning_organisation).to eq(new_organisation) + end + end + end + end end describe "title link" do