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 1/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 2/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 3/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 4/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 5/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 From 72a52a58c957641db6b2fa40c8f7dc4170125d78 Mon Sep 17 00:00:00 2001 From: Rachael Booth Date: Wed, 18 Sep 2024 15:47:21 +0100 Subject: [PATCH 6/6] CLDC-3387: Allow support users to create notifications (#2613) * CLDC-3387: Allow support users to create notifications * Refactor rendering * Adjust notifications helper * Add rubocop ignore * Fix path checks around notification viewing * Notification path regex should work on review apps * Tidy * Remove unused function * Reference external url in link to markdown syntax guide * CLDC-3614: Allow markdown page content when creating notifications (#2622) * CLDC-3614: Allow markdown page content when creating notifications * Try using beginning of day for time in failing test * Remove ignored breaks * Add paper trail to notifications * CLDC-3607: Allow support users to delete notifications (by setting their end date) (#2630) * CLDC-3607: Allow support users to delete notifications (by setting their end date) * Refactor render_for_page helper * Move db queries to homepage presenter * Fix lint * Use a before_action to find notification in controller * Make delete notification links red * Tweak link to markdown guide * Open markdown guide in new tab --- Gemfile | 1 + Gemfile.lock | 1 + app/controllers/notifications_controller.rb | 90 +++++++++- app/helpers/application_helper.rb | 4 +- app/helpers/navigation_items_helper.rb | 2 +- app/helpers/notifications_helper.rb | 82 +++++++++ app/models/notification.rb | 21 ++- app/presenters/homepage_presenter.rb | 3 +- app/views/home/index.html.erb | 4 + app/views/notifications/_form.html.erb | 33 ++++ .../_notification_banner.html.erb | 8 +- .../_notification_home_section.html.erb | 17 ++ .../notifications/check_answers.html.erb | 61 +++++++ .../delete_confirmation.html.erb | 27 +++ app/views/notifications/edit.html.erb | 3 + app/views/notifications/new.html.erb | 3 + app/views/notifications/show.html.erb | 5 +- config/locales/en.yml | 15 +- config/routes.rb | 5 +- ...additional_page_column_to_notifications.rb | 5 + db/schema.rb | 15 +- spec/factories/notification.rb | 1 + ...ons_page_spec.rb => notifications_spec.rb} | 22 ++- spec/features/start_page_spec.rb | 7 +- .../lettings_log_derived_fields_spec.rb | 4 +- spec/models/notification_spec.rb | 37 ++++ .../requests/notifications_controller_spec.rb | 163 ++++++++++++++++++ 27 files changed, 592 insertions(+), 47 deletions(-) create mode 100644 app/views/notifications/_form.html.erb create mode 100644 app/views/notifications/_notification_home_section.html.erb create mode 100644 app/views/notifications/check_answers.html.erb create mode 100644 app/views/notifications/delete_confirmation.html.erb create mode 100644 app/views/notifications/edit.html.erb create mode 100644 app/views/notifications/new.html.erb create mode 100644 db/migrate/20240904135306_add_show_additional_page_column_to_notifications.rb rename spec/features/{notifications_page_spec.rb => notifications_spec.rb} (51%) create mode 100644 spec/models/notification_spec.rb create mode 100644 spec/requests/notifications_controller_spec.rb diff --git a/Gemfile b/Gemfile index 332359f98..4dc93aaf7 100644 --- a/Gemfile +++ b/Gemfile @@ -23,6 +23,7 @@ gem "govuk-components", "~> 5.1" gem "govuk_design_system_formbuilder", "~> 5.0" # Convert Markdown into GOV.UK frontend-styled HTML gem "govuk_markdown" +gem "redcarpet", "~> 3.6" # GOV UK Notify gem "notifications-ruby-client" # A modest javascript framework for the html you already have diff --git a/Gemfile.lock b/Gemfile.lock index 086491321..7cc2addda 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -559,6 +559,7 @@ DEPENDENCIES rack-mini-profiler (~> 2.0) rails (~> 7.0.8.3) rails_admin (~> 3.1) + redcarpet (~> 3.6) redis (~> 4.8) roo rspec-rails diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index e3aff82e6..d0181ead6 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -1,19 +1,93 @@ class NotificationsController < ApplicationController + before_action :authenticate_user!, except: %i[show] + before_action :authenticate_scope!, except: %i[show dismiss] + before_action :find_notification, except: %i[new create] + + rescue_from ActiveRecord::RecordNotFound, with: :render_not_found + def dismiss - if current_user.blank? - redirect_to root_path - else - current_user.newest_active_unread_notification.mark_as_read! for: current_user if current_user.newest_active_unread_notification.present? - redirect_back(fallback_location: root_path) - end + @notification.mark_as_read! for: current_user + redirect_back(fallback_location: root_path) end def show - @notification = current_user&.newest_active_unread_notification || Notification.newest_active_unauthenticated_notification - if @notification&.page_content + if !@notification.show_on_unauthenticated_pages && !current_user + render_not_found and return + end + + if @notification.show_additional_page render "show" else redirect_back(fallback_location: root_path) end end + + def new + @notification = Notification.new + end + + def create + @notification = Notification.new(notification_model_params) + + if @notification.errors.empty? && @notification.save + redirect_to notification_check_answers_path(@notification) + else + render :new, status: :unprocessable_entity + end + end + + def update + start_now = params[:notification][:start_now] + + if @notification.errors.empty? && @notification.update(notification_model_params) + if start_now + flash[:notice] = "The notification has been created" + redirect_to root_path + else + redirect_to notification_check_answers_path(@notification) + end + elsif start_now + render :check_answers, status: :unprocessable_entity + else + render :edit, status: :unprocessable_entity + end + end + + def delete + @notification.update!(end_date: Time.zone.now) + flash[:notice] = "The notification has been deleted" + redirect_to root_path + end + +private + + def notification_params + params.require(:notification).permit(:title, :show_on_unauthenticated_pages, :show_additional_page, :link_text, :page_content, :start_now) + end + + def authenticate_scope! + render_not_found unless current_user.support? + end + + def notification_model_params + model_params = notification_params.except(:start_now) + + if notification_params[:show_additional_page] == "0" + model_params[:link_text] = nil + model_params[:page_content] = nil + end + + model_params[:start_date] = Time.zone.now if notification_params[:start_now] + + model_params + end + + def find_notification + id = params[:id] || params[:notification_id] + @notification = current_user&.support? ? Notification.find_by(id:) : Notification.active.find_by(id:) + + raise ActiveRecord::RecordNotFound unless @notification + + @notification + end end diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 01f7734c2..bb119b29e 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -12,7 +12,7 @@ module ApplicationHelper def govuk_header_classes(current_user) if current_user&.support? "app-header app-header--orange" - elsif ((current_user.blank? && Notification.active_unauthenticated_notifications.present?) || current_user&.active_unread_notifications.present?) && !current_page?(notifications_path) + elsif notifications_to_display? "app-header app-header__no-border-bottom" else "app-header" @@ -28,7 +28,7 @@ module ApplicationHelper end def notifications_to_display? - !current_page?(notifications_path) && (authenticated_user_has_notifications? || unauthenticated_user_has_notifications?) + !request.path.match?(/\/notifications\/\d+$/) && (authenticated_user_has_notifications? || unauthenticated_user_has_notifications?) end private diff --git a/app/helpers/navigation_items_helper.rb b/app/helpers/navigation_items_helper.rb index 1cecb97ec..b1a0c6a4c 100644 --- a/app/helpers/navigation_items_helper.rb +++ b/app/helpers/navigation_items_helper.rb @@ -47,7 +47,7 @@ module NavigationItemsHelper private def home_current?(path) - path == root_path || path == notifications_path + path == root_path || path.match?(/\/notifications\/\d+$/) end def lettings_logs_current?(path) diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index ab4c8ec21..318918134 100644 --- a/app/helpers/notifications_helper.rb +++ b/app/helpers/notifications_helper.rb @@ -14,4 +14,86 @@ module NotificationsHelper Notification.newest_active_unauthenticated_notification end end + + def render_for_banner(title) + # rubocop:disable Rails/HelperInstanceVariable + @banner_renderer ||= NotificationRenderer.new({ invert_link_colour: true, bold_all_text: true }) + @banner_markdown ||= Redcarpet::Markdown.new(@banner_renderer, no_intra_emphasis: true) + @banner_markdown.render(title) + # rubocop:enable Rails/HelperInstanceVariable + end + + def render_for_summary(title) + render_normal_markdown(title) + end + + def render_for_page(notification) + content_includes_own_title = /\A\s*#[^#]/.match?(notification.page_content) + return render_normal_markdown(notification.page_content) if content_includes_own_title + + content = "# #{notification.title}\n#{notification.page_content}" + render_normal_markdown(content) + end + + def render_for_home(notification) + return render_normal_markdown(notification.title) unless notification.show_additional_page + + content = "#{notification.title} \n[#{notification.link_text}](#{notification_path(notification)})" + render_normal_markdown(content) + end + +private + + def render_normal_markdown(content) + # rubocop:disable Rails/HelperInstanceVariable + @on_page_renderer ||= NotificationRenderer.new({ invert_link_colour: false, bold_all_text: false }) + @on_page_markdown ||= Redcarpet::Markdown.new(@on_page_renderer, no_intra_emphasis: true) + @on_page_markdown.render(content) + # rubocop:enable Rails/HelperInstanceVariable + end +end + +class NotificationRenderer < Redcarpet::Render::HTML + def initialize(options = {}) + link_class = "govuk-link" + link_class += " govuk-link--inverse" if options[:invert_link_colour] + @bold = options[:bold_all_text] # rubocop:disable Rails/HelperInstanceVariable + base_options = { escape_html: true, safe_links_only: true, link_attributes: { class: link_class } } + super base_options + end + + def header(text, header_level) + header_size = case header_level + when 1 + "xl" + when 2 + "l" + when 3 + "m" + else + "s" + end + + %(#{text}) + end + + def paragraph(text) + return %(

#{text}

) if @bold # rubocop:disable Rails/HelperInstanceVariable + + %(

#{text}

) + end + + def list(contents, list_type) + return %(
    #{contents}
) if list_type == :ordered + + %(
    #{contents}
) + end + + def hrule + %(
) + end + + def block_quote(quote) + %(
#{quote}
) + end end diff --git a/app/models/notification.rb b/app/models/notification.rb index 86978ebea..cd7c9972a 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -1,7 +1,12 @@ class Notification < ApplicationRecord acts_as_readable - scope :active, -> { where("start_date <= ? AND end_date >= ?", Time.zone.now, Time.zone.now) } + has_paper_trail + + validates :title, presence: { message: I18n.t("activerecord.errors.models.notification.attributes.title.blank") } + validate :validate_additional_page_information + + scope :active, -> { where("start_date <= ? AND (end_date >= ? OR end_date is null)", Time.zone.now, Time.zone.now) } scope :unauthenticated, -> { where(show_on_unauthenticated_pages: true) } def self.active_unauthenticated_notifications @@ -11,4 +16,18 @@ class Notification < ApplicationRecord def self.newest_active_unauthenticated_notification active_unauthenticated_notifications.last end + +private + + def validate_additional_page_information + return unless show_additional_page + + if link_text.blank? + errors.add :link_text, I18n.t("activerecord.errors.models.notification.attributes.link_text.blank_when_additional_page_set") + end + + if page_content.blank? + errors.add :page_content, I18n.t("activerecord.errors.models.notification.attributes.page_content.blank_when_additional_page_set") + end + end end diff --git a/app/presenters/homepage_presenter.rb b/app/presenters/homepage_presenter.rb index 1cb784e3a..3b3314667 100644 --- a/app/presenters/homepage_presenter.rb +++ b/app/presenters/homepage_presenter.rb @@ -2,7 +2,7 @@ class HomepagePresenter include Rails.application.routes.url_helpers include CollectionTimeHelper - attr_reader :current_year_in_progress_lettings_data, :current_year_completed_lettings_data, :current_year_in_progress_sales_data, :current_year_completed_sales_data, :last_year_in_progress_lettings_data, :last_year_completed_lettings_data, :last_year_in_progress_sales_data, :last_year_completed_sales_data, :incomplete_schemes_data + attr_reader :current_year_in_progress_lettings_data, :current_year_completed_lettings_data, :current_year_in_progress_sales_data, :current_year_completed_sales_data, :last_year_in_progress_lettings_data, :last_year_completed_lettings_data, :last_year_in_progress_sales_data, :last_year_completed_sales_data, :incomplete_schemes_data, :active_notifications def initialize(user) @user = user @@ -25,6 +25,7 @@ class HomepagePresenter path: schemes_path(status: [:incomplete], owning_organisation_select: "all"), } end + @active_notifications = Notification.active if @user.support? end def title_text_for_user diff --git a/app/views/home/index.html.erb b/app/views/home/index.html.erb index 6995c28e5..e55dc6c99 100644 --- a/app/views/home/index.html.erb +++ b/app/views/home/index.html.erb @@ -6,6 +6,10 @@

<%= @homepage_presenter.title_text_for_user %>

+ <% if @current_user.support? %> + <%= render partial: "notifications/notification_home_section", locals: { active_notifications: @homepage_presenter.active_notifications } %> + <% end %> +
<% if @homepage_presenter.in_crossover_period? %> diff --git a/app/views/notifications/_form.html.erb b/app/views/notifications/_form.html.erb new file mode 100644 index 000000000..32af1adc0 --- /dev/null +++ b/app/views/notifications/_form.html.erb @@ -0,0 +1,33 @@ +
+
+ <%= f.govuk_error_summary %> + + <% content_for :page_title, "Create a new notification" %> + + <% content_for :before_content do %> + <%= govuk_back_link(href: :back) %> + <% end %> + +

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

+ + This notification will be visible to all users until you delete it + + <%= f.govuk_text_field :title, + label: { text: "Title", size: "m" }, + hint: { text: "Use markdown for links to existing pages" } %> + + <%= f.govuk_check_boxes_fieldset :display_options, multiple: false, legend: { text: "Display Options" } do %> + <%= f.govuk_check_box :show_on_unauthenticated_pages, 1, 0, multiple: false, label: { text: "Show this notification on unauthenticated pages, for example the start page" } %> + <%= f.govuk_check_box :show_additional_page, 1, 0, multiple: false, label: { text: "Include a link to a separate page with additional information" } do %> + <%= f.govuk_text_field :link_text, label: { text: "Link text" }, hint: { text: "Use descriptive language and relevant terms. The link text should make sense out of context." } %> + <%= f.govuk_text_area :page_content, label: { text: "Page content" }, hint: { text: "Use markdown to format the page content. The page title will be the notification title by default. Use a heading level one if you want to override it." } %> + <% end %> + <% end %> + + <%= govuk_link_to "Find out more about using Markdown at Markdown Guide", "https://www.markdownguide.org/basic-syntax/", new_tab: true %> + + <%= f.govuk_submit "Continue" %> +
+
diff --git a/app/views/notifications/_notification_banner.html.erb b/app/views/notifications/_notification_banner.html.erb index cd7dfffac..a9cc8bdff 100644 --- a/app/views/notifications/_notification_banner.html.erb +++ b/app/views/notifications/_notification_banner.html.erb @@ -6,16 +6,16 @@ <% if notification_count > 1 && current_user.present? %>

Notification 1 of <%= notification_count %>

<% end %> -

<%= notification.title.html_safe %>

- <% if notification.page_content.present? %> + <%== render_for_banner(notification.title) %> + <% if notification.show_additional_page %>
- <%= govuk_link_to notification.link_text, notifications_path, class: "govuk-link--inverse govuk-!-font-weight-bold" %> + <%= govuk_link_to notification.link_text, notification_path(notification), class: "govuk-link--inverse govuk-!-font-weight-bold" %>
<% end %>
<% if current_user.present? %>

- <%= govuk_link_to "Dismiss", dismiss_notifications_path, class: "govuk-link--inverse" %> + <%= govuk_link_to "Dismiss", notification_dismiss_path(notification), class: "govuk-link--inverse" %>

<% end %>
diff --git a/app/views/notifications/_notification_home_section.html.erb b/app/views/notifications/_notification_home_section.html.erb new file mode 100644 index 000000000..522bc2840 --- /dev/null +++ b/app/views/notifications/_notification_home_section.html.erb @@ -0,0 +1,17 @@ +
+

<%== I18n.t("active_notifications", count: active_notifications.count) %>

+ <% active_notifications.each do |notification| %> +
+
+ <%== render_for_home(notification) %> +
+
+ <%= govuk_link_to("Delete notification", notification_delete_confirmation_path(notification), class: "app-!-colour-red") %> +
+
+ <% end %> +
+ +
+ <%= govuk_button_link_to "Create a new notification", new_notification_path %> +
diff --git a/app/views/notifications/check_answers.html.erb b/app/views/notifications/check_answers.html.erb new file mode 100644 index 000000000..8ddbc0abe --- /dev/null +++ b/app/views/notifications/check_answers.html.erb @@ -0,0 +1,61 @@ +<% content_for :title, "Create a notification" %> + +
+
+ <% content_for :before_content do %> + <%= govuk_back_link(href: :back) %> + <% end %> + +

+ Create a notification + Check your answers +

+ + This notification will be visible to all users until you delete it + + <%= form_for(@notification, method: :patch) do |f| %> + <%= f.govuk_error_summary %> + +
+
+ <%= govuk_summary_list do |summary_list| %> + <% summary_list.with_row do |row| %> + <% row.with_key { "Title" } %> + <% row.with_value do %> + <%== render_for_summary(@notification.title) %> + <% end %> + <% row.with_action(text: "Change", href: edit_notification_path(@notification)) %> + <% end %> + <% summary_list.with_row do |row| %> + <% row.with_key { "Show on unauthenticated pages?" } %> + <% row.with_value { @notification.show_on_unauthenticated_pages ? "Yes" : "No" } %> + <% row.with_action(text: "Change", href: edit_notification_path(@notification)) %> + <% end %> + <% summary_list.with_row do |row| %> + <% row.with_key { "Link to additional information?" } %> + <% row.with_value { @notification.show_additional_page ? "Yes" : "No" } %> + <% row.with_action(text: "Change", href: edit_notification_path(@notification)) %> + <% end %> + <% if @notification.show_additional_page %> + <% summary_list.with_row do |row| %> + <% row.with_key { "Link text" } %> + <% row.with_value { @notification.link_text } %> + <% row.with_action(text: "Change", href: edit_notification_path(@notification)) %> + <% end %> + <% summary_list.with_row do |row| %> + <% row.with_key { "Page content" } %> + <% row.with_value do %> + <%= govuk_link_to "View page preview", notification_path(@notification), new_tab: true %> + <% end %> + <% row.with_action(text: "Change", href: edit_notification_path(@notification)) %> + <% end %> + <% end %> + <% end %> +
+
+ + <%= f.hidden_field :start_now, value: true %> + <%= f.govuk_submit "Create notification" %> + <% end %> +
+
diff --git a/app/views/notifications/delete_confirmation.html.erb b/app/views/notifications/delete_confirmation.html.erb new file mode 100644 index 000000000..4107c3050 --- /dev/null +++ b/app/views/notifications/delete_confirmation.html.erb @@ -0,0 +1,27 @@ +<% content_for :before_content do %> + <% content_for :title, "Are you sure you want to delete this notification?" %> + <%= govuk_back_link(href: :back) %> +<% end %> + +
+
+

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

+ +

<%== render_for_summary("**Notification:** #{@notification.title}") %>

+ +

Users will no longer see this notification.

+ + <%= govuk_warning_text(text: "You will not be able to undo this action.") %> + +
+ <%= govuk_button_to( + "Delete notification", + notification_delete_path(@notification), + method: :delete, + ) %> + <%= govuk_button_link_to "Cancel", root_path, html: { method: :get }, secondary: true %> +
+
+
diff --git a/app/views/notifications/edit.html.erb b/app/views/notifications/edit.html.erb new file mode 100644 index 000000000..373cff20d --- /dev/null +++ b/app/views/notifications/edit.html.erb @@ -0,0 +1,3 @@ +<%= form_for(@notification, as: :notification, method: :patch) do |f| %> + <% render partial: "form", locals: { notification: @notification, f: } %> +<% end %> diff --git a/app/views/notifications/new.html.erb b/app/views/notifications/new.html.erb new file mode 100644 index 000000000..d996b9624 --- /dev/null +++ b/app/views/notifications/new.html.erb @@ -0,0 +1,3 @@ +<%= form_for(@notification, as: :notification, method: :post) do |f| %> + <% render partial: "form", locals: { notification: @notification, f: } %> +<% end %> diff --git a/app/views/notifications/show.html.erb b/app/views/notifications/show.html.erb index abdc77044..a3933db55 100644 --- a/app/views/notifications/show.html.erb +++ b/app/views/notifications/show.html.erb @@ -5,10 +5,7 @@
-

<%= @notification.title %>

-

- <%= sanitize @notification.page_content %> -

+ <%== render_for_page(@notification) %>

diff --git a/config/locales/en.yml b/config/locales/en.yml index 42ef43be1..c8c5cffc4 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -41,6 +41,11 @@ en: create_password: "Create a password to finish setting up your account" reset_password: "Reset your password" + active_notifications: + zero: "There are no active notifications" + one: "There is one active notification:" + other: "There are %{count} active notifications:" + activemodel: errors: models: @@ -180,11 +185,19 @@ en: attributes: absorbing_organisation_id: blank: "Select the absorbing organisation" - merge_date: + merge_date: blank: "Enter a merge date" invalid: "Enter a valid merge date" existing_absorbing_organisation: blank: "You must answer absorbing organisation already active?" + notification: + attributes: + title: + blank: "Enter a title" + link_text: + blank_when_additional_page_set: "Enter the link text" + page_content: + blank_when_additional_page_set: "Enter the page content" notification: logs_deleted: diff --git a/config/routes.rb b/config/routes.rb index f2c86e37c..77b862e5a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -142,8 +142,11 @@ Rails.application.routes.draw do end end - resource :notifications do + resources :notifications do get "dismiss", to: "notifications#dismiss" + get "check-answers", to: "notifications#check_answers" + get "delete-confirmation", to: "notifications#delete_confirmation" + delete "delete", to: "notifications#delete" end resources :organisations do diff --git a/db/migrate/20240904135306_add_show_additional_page_column_to_notifications.rb b/db/migrate/20240904135306_add_show_additional_page_column_to_notifications.rb new file mode 100644 index 000000000..c0992bb62 --- /dev/null +++ b/db/migrate/20240904135306_add_show_additional_page_column_to_notifications.rb @@ -0,0 +1,5 @@ +class AddShowAdditionalPageColumnToNotifications < ActiveRecord::Migration[7.0] + def change + add_column :notifications, :show_additional_page, :boolean + end +end diff --git a/db/schema.rb b/db/schema.rb index 2d6808e6f..80463eaad 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_09_11_152702) do +ActiveRecord::Schema[7.0].define(version: 2024_09_18_112702) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -55,7 +55,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_09_11_152702) do t.datetime "last_accessed" t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.check_constraint "log_type::text = ANY (ARRAY['lettings'::character varying::text, 'sales'::character varying::text])", name: "log_type_check" + t.check_constraint "log_type::text = ANY (ARRAY['lettings'::character varying, 'sales'::character varying]::text[])", name: "log_type_check" t.check_constraint "year >= 2000 AND year <= 2099", name: "year_check" end @@ -206,14 +206,14 @@ ActiveRecord::Schema[7.0].define(version: 2024_09_11_152702) do t.integer "hb" t.integer "hbrentshortfall" t.integer "property_relet" - t.datetime "mrcdate", precision: nil + t.datetime "mrcdate" t.integer "incref" - t.datetime "startdate", precision: nil + t.datetime "startdate" t.integer "armedforces" t.integer "first_time_property_let_as_social_housing" t.integer "unitletas" t.integer "builtype" - t.datetime "voiddate", precision: nil + t.datetime "voiddate" t.bigint "owning_organisation_id" t.bigint "managing_organisation_id" t.integer "renttype" @@ -459,6 +459,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_09_11_152702) do t.boolean "show_on_unauthenticated_pages" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.boolean "show_additional_page" end create_table "organisation_relationships", force: :cascade do |t| @@ -779,8 +780,8 @@ ActiveRecord::Schema[7.0].define(version: 2024_09_11_152702) do t.string "name" t.bigint "organisation_id" t.integer "sign_in_count", default: 0, null: false - t.datetime "current_sign_in_at", precision: nil - t.datetime "last_sign_in_at", precision: nil + t.datetime "current_sign_in_at" + t.datetime "last_sign_in_at" t.string "current_sign_in_ip" t.string "last_sign_in_ip" t.integer "role" diff --git a/spec/factories/notification.rb b/spec/factories/notification.rb index 05d4faa3f..92d4bf4c9 100644 --- a/spec/factories/notification.rb +++ b/spec/factories/notification.rb @@ -6,5 +6,6 @@ FactoryBot.define do start_date { Time.zone.yesterday } end_date { Time.zone.tomorrow } show_on_unauthenticated_pages { false } + show_additional_page { true } end end diff --git a/spec/features/notifications_page_spec.rb b/spec/features/notifications_spec.rb similarity index 51% rename from spec/features/notifications_page_spec.rb rename to spec/features/notifications_spec.rb index 97bbeb7eb..e2bd4b151 100644 --- a/spec/features/notifications_page_spec.rb +++ b/spec/features/notifications_spec.rb @@ -1,7 +1,7 @@ require "rails_helper" require_relative "form/helpers" -RSpec.describe "Notifications Page Features" do +RSpec.describe "Notifications Features" do include Helpers context "when there are notifications" do @@ -9,32 +9,30 @@ RSpec.describe "Notifications Page Features" do context "when the notifications are currently active" do before do - create(:notification, title: "Notification title 1") - create(:notification, title: "Notification title 2") - create(:notification, title: "Notification title 3") + create(:notification, start_date: Time.zone.yesterday, title: "Notification title 1") + create(:notification, start_date: Time.zone.yesterday, end_date: Time.zone.tomorrow, title: "Notification title 2") sign_in user - visit(notifications_path) + visit(root_path) end - it "does not show the notification banner" do - expect(page).not_to have_content("Notification 1 of") - expect(page).not_to have_link("Dismiss") - expect(page).not_to have_link("Link text") + it "shows the notification banner" do + expect(page).to have_content("Notification 1 of") + expect(page).to have_link("Dismiss") end end context "when the notifications are not currently active" do before do - create(:notification, end_date: Time.zone.yesterday, title: "Notification title 1") + create(:notification, start_date: Time.zone.yesterday, end_date: Time.zone.yesterday, title: "Notification title 1") create(:notification, start_date: Time.zone.tomorrow, title: "Notification title 2") + create(:notification, start_date: nil, title: "Notification title 3") sign_in user - visit(notifications_path) + visit(root_path) end it "does not show the notifications banner" do expect(page).not_to have_content("Notification 1 of") expect(page).not_to have_link("Dismiss") - expect(page).not_to have_link("Link text") end end end diff --git a/spec/features/start_page_spec.rb b/spec/features/start_page_spec.rb index d90a0c1f0..ab09fd446 100644 --- a/spec/features/start_page_spec.rb +++ b/spec/features/start_page_spec.rb @@ -30,14 +30,15 @@ RSpec.describe "Start Page Features" do end context "when the unauthenticated user clicks a notification link" do + let!(:notification) { create(:notification, title: "Notification title", link_text: "link", page_content: "Some html content", show_on_unauthenticated_pages: true) } + before do - create(:notification, show_on_unauthenticated_pages: true) visit(root_path) - click_link("Link text") + click_link("link") end it "takes them to the notification details page" do - expect(page).to have_current_path(notifications_path) + expect(page).to have_current_path(notification_path(notification)) expect(page).to have_content("Notification title") expect(page).to have_content("Some html content") expect(page).to have_link("Back to Start") diff --git a/spec/models/lettings_log_derived_fields_spec.rb b/spec/models/lettings_log_derived_fields_spec.rb index 54dc438bc..aae1396d7 100644 --- a/spec/models/lettings_log_derived_fields_spec.rb +++ b/spec/models/lettings_log_derived_fields_spec.rb @@ -981,9 +981,9 @@ RSpec.describe LettingsLog, type: :model do end describe "deriving voiddate from startdate" do - let(:startdate) { Time.zone.now } + let(:startdate) { Time.zone.now.beginning_of_day } - it "correctly derives voiddate if the letting is a renewal and clears it if it is not" do + it "correctly derives voiddate if the letting is a renewal" do log.assign_attributes(renewal: 1, startdate:) expect { log.set_derived_fields! }.to change(log, :voiddate).to startdate diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb new file mode 100644 index 000000000..86c02476f --- /dev/null +++ b/spec/models/notification_spec.rb @@ -0,0 +1,37 @@ +require "rails_helper" + +RSpec.describe Notification, type: :model do + describe "#valid?" do + context "when show additional page is true" do + context "and page_content is blank" do + let(:notification) { build(:notification, show_additional_page: true, page_content: "") } + + it "adds an error to page_content" do + notification.valid? + + expect(notification.errors[:page_content]).to include("Enter the page content") + end + end + + context "and link_text is blank" do + let(:notification) { build(:notification, show_additional_page: true, link_text: nil) } + + it "adds an error to link_text" do + notification.valid? + + expect(notification.errors[:link_text]).to include("Enter the link text") + end + end + end + + context "when show additional page is false" do + context "and page_content and link_text are blank" do + let(:notification) { build(:notification, show_additional_page: false, link_text: nil, page_content: nil) } + + it "is valid" do + expect(notification).to be_valid + end + end + end + end +end diff --git a/spec/requests/notifications_controller_spec.rb b/spec/requests/notifications_controller_spec.rb new file mode 100644 index 000000000..0982057ab --- /dev/null +++ b/spec/requests/notifications_controller_spec.rb @@ -0,0 +1,163 @@ +require "rails_helper" + +RSpec.describe NotificationsController, type: :request do + context "when user is signed in as a support user" do + let(:support_user) { create(:user, :support) } + + before do + allow(support_user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in support_user + end + + describe "#create" do + let(:request) { post notifications_path, params: params } + + context "with valid parameters" do + let(:params) { { "notification": { title: "Test Create", show_on_unauthenticated_pages: "1", show_additional_page: "1", link_text: "link", page_content: "page" } } } + + it "creates a new notification with no start date set" do + request + notification = Notification.find_by(title: "Test Create") + expect(notification.show_on_unauthenticated_pages).to be(true) + expect(notification.show_additional_page).to be(true) + expect(notification.link_text).to eq("link") + expect(notification.page_content).to eq("page") + expect(notification.start_date).to be_nil + end + + it "redirects to check answers page" do + request + notification = Notification.find_by(title: "Test Create") + expect(response).to redirect_to(notification_check_answers_path(notification)) + end + end + + context "with invalid parameters" do + let(:params) { { "notification": { title: "", show_on_unauthenticated_pages: "1" } } } + + it "gives an error response" do + request + expect(response).to have_http_status(:unprocessable_entity) + end + end + + context "when show additional page is false" do + let(:params) { { "notification": { title: "No Additional Page", show_on_unauthenticated_pages: "1", show_additional_page: "0", link_text: "text", page_content: "content" } } } + + it "ignores values for link_text and page_content" do + request + notification = Notification.find_by(title: "No Additional Page") + expect(notification.link_text).to be_nil + expect(notification.page_content).to be_nil + end + end + end + + describe "#update" do + let(:notification) { create(:notification, title: "Initial Title", start_date: nil, end_date: nil) } + let(:request) { patch notification_path(notification), params: params } + + context "when start_now is set to true" do + let(:params) { { "notification": { start_now: true } } } + + it "sets the start date on the notification" do + request + notification.reload + expect(notification.start_date).not_to be_nil + expect(notification.start_date).to be < Time.zone.now + end + + it "redirects to the home page" do + request + expect(response).to redirect_to(root_path) + end + end + + context "when start_now is not set" do + let(:params) { { "notification": { title: "Updated Title", show_on_unauthenticated_pages: "1" } } } + + it "sets the relevant values on the notification" do + request + notification.reload + expect(notification.title).to eql("Updated Title") + expect(notification.start_date).to be_nil + end + + it "redirects to check answers" do + request + expect(response).to redirect_to(notification_check_answers_path(notification)) + end + end + + context "when show additional page is false" do + let(:notification) { create(:notification, show_additional_page: "0", link_text: "link", page_content: "page") } + let(:params) { { "notification": { show_additional_page: "0", link_text: "text", page_content: "content" } } } + + it "removes values for link_text and page_content" do + request + notification.reload + expect(notification.link_text).to be_nil + expect(notification.page_content).to be_nil + end + end + end + + describe "#delete" do + let(:notification) { create(:notification, end_date: nil) } + let(:request) { delete notification_delete_path(notification) } + + it "sets end_date on the notification" do + request + notification.reload + expect(notification.end_date).to be < Time.zone.now + end + end + end + + context "when user is signed in as a non-support user" do + let(:user) { create(:user, :data_coordinator) } + + before do + sign_in user + end + + describe "#create" do + let(:request) { post notifications_path, params: { "notification": { title: "Test Create" } } } + + it "returns not found" do + request + expect(response).to have_http_status(:not_found) + end + + it "does not create a notification" do + expect { request }.not_to change(Notification, :count) + end + end + + describe "#update" do + let(:notification) { create(:notification) } + let(:request) { patch notification_path(notification), params: { "notification": { title: "Test Update" } } } + + it "returns not found" do + request + expect(response).to have_http_status(:not_found) + end + end + + describe "#delete" do + let(:notification) { create(:notification, end_date: nil) } + let(:request) { delete notification_delete_path(notification) } + + it "returns not found" do + request + expect(response).to have_http_status(:not_found) + end + + it "does not set end_date on the notification" do + request + notification.reload + expect(notification.end_date).to be_nil + end + end + end +end