+ This scheme has <%= pluralize(@affected_locations.count, "location") %> active on <%= @deactivation_date.to_formatted_s(:govuk_date) %>. <%= @affected_locations.count == 1 ? "This location" : "These locations" %> will deactivate on that date. If the scheme is ever reactivated, <%= @affected_locations.count == 1 ? "this location" : "these locations" %> will reactivate as well.
+
<%= f.govuk_radio_buttons_fieldset date_type_question(action),
- legend: { text: I18n.t("questions.scheme.toggle_active.apply_from") },
- caption: { text: title },
- hint: { text: I18n.t("hints.scheme.toggle_active", date: start_date.to_formatted_s(:govuk_date)) } do %>
- <%= govuk_warning_text text: I18n.t("warnings.scheme.#{action}.existing_logs") %>
+ legend: { text: I18n.t("questions.scheme.toggle_active.apply_from"), hidden: true } do %>
<%= f.govuk_radio_button date_type_question(action),
"default",
label: { text: "From the start of the open collection period (#{start_date.to_formatted_s(:govuk_date)})" } %>
@@ -26,7 +27,7 @@
**basic_conditional_html_attributes({ "deactivation_date" => %w[other] }, "scheme") do %>
<%= f.govuk_date_field date_question(action),
legend: { text: "Date", size: "m" },
- hint: { text: "For example, 27 3 2022" },
+ hint: { text: "For example, 27 3 2025" },
width: 20 %>
<% end %>
<% end %>
diff --git a/spec/fixtures/files/locations_csv_export.csv b/spec/fixtures/files/locations_csv_export.csv
index 51844b6c5..5b80ae024 100644
--- a/spec/fixtures/files/locations_csv_export.csv
+++ b/spec/fixtures/files/locations_csv_export.csv
@@ -1,2 +1,2 @@
scheme_code,location_code,location_postcode,location_name,location_status,location_local_authority,location_units,location_type_of_unit,location_mobility_type,location_active_dates
-,,SW1A 2AA,Downing Street,deactivating_soon,Westminster,20,Self-contained house,Fitted with equipment and adaptations,"Active from 1 April 2022 to 25 December 2023, Deactivated on 26 December 2023"
+,,SW1A 2AA,Downing Street,deactivating_soon,Westminster,20,Self-contained house,Fitted with equipment and adaptations,"Active from 1 April 2023 to 25 December 2023, Deactivated on 26 December 2023"
diff --git a/spec/fixtures/files/schemes_and_locations_csv_export.csv b/spec/fixtures/files/schemes_and_locations_csv_export.csv
index 79c9132f6..d662dceb3 100644
--- a/spec/fixtures/files/schemes_and_locations_csv_export.csv
+++ b/spec/fixtures/files/schemes_and_locations_csv_export.csv
@@ -1,2 +1,2 @@
scheme_code,scheme_service_name,scheme_status,scheme_confidential,scheme_type,scheme_registered_under_care_act,scheme_owning_organisation_name,scheme_support_services_provided_by,scheme_primary_client_group,scheme_has_other_client_group,scheme_secondary_client_group,scheme_support_type,scheme_intended_stay,scheme_created_at,scheme_active_dates,location_code,location_postcode,location_name,location_status,location_local_authority,location_units,location_type_of_unit,location_mobility_type,location_active_dates
-,Test name,active,Yes,Housing for older people,No,MHCLG,The same organisation that owns the housing stock,People with alcohol problems,Yes,Older people with support needs,High level,Medium stay,2021-04-01T00:00:00+01:00,"Active from 1 April 2020 to 31 March 2022, Deactivated on 1 April 2022, Active from 1 April 2023",,SW1A 2AA,Downing Street,deactivating_soon,Westminster,20,Self-contained house,Fitted with equipment and adaptations,"Active from 1 April 2022 to 25 December 2023, Deactivated on 26 December 2023"
+,Test name,active,Yes,Housing for older people,No,MHCLG,The same organisation that owns the housing stock,People with alcohol problems,Yes,Older people with support needs,High level,Medium stay,2021-04-01T00:00:00+01:00,"Active from 1 April 2020 to 31 March 2022, Deactivated on 1 April 2022, Active from 1 April 2023",,SW1A 2AA,Downing Street,deactivating_soon,Westminster,20,Self-contained house,Fitted with equipment and adaptations,"Active from 1 April 2023 to 25 December 2023, Deactivated on 26 December 2023"
diff --git a/spec/models/location_spec.rb b/spec/models/location_spec.rb
index 2f991ab31..4856c5662 100644
--- a/spec/models/location_spec.rb
+++ b/spec/models/location_spec.rb
@@ -930,6 +930,39 @@ RSpec.describe Location, type: :model do
expect(location.status).to eq(:activating_soon)
end
end
+
+ context "when the scheme the location belongs to is deactivated" do
+ before do
+ FactoryBot.create(:scheme_deactivation_period, deactivation_date: Time.zone.yesterday, scheme: location.scheme)
+ location.save!
+ end
+
+ it "returns deactivated" do
+ expect(location.status).to eq(:deactivated)
+ end
+ end
+
+ context "when the scheme the location belongs to is deactivating soon" do
+ before do
+ FactoryBot.create(:scheme_deactivation_period, deactivation_date: Time.zone.today + 1.month, scheme: location.scheme)
+ location.save!
+ end
+
+ it "returns deactivating soon" do
+ expect(location.status).to eq(:deactivating_soon)
+ end
+ end
+
+ context "when the scheme the location belongs to is reactivating soon" do
+ before do
+ FactoryBot.create(:scheme_deactivation_period, deactivation_date: Time.zone.yesterday, reactivation_date: Time.zone.tomorrow, scheme: location.scheme)
+ location.save!
+ end
+
+ it "returns reactivating soon" do
+ expect(location.status).to eq(:reactivating_soon)
+ end
+ end
end
describe "status_at" do
diff --git a/spec/requests/schemes_controller_spec.rb b/spec/requests/schemes_controller_spec.rb
index 579cfd1a9..d93bc873d 100644
--- a/spec/requests/schemes_controller_spec.rb
+++ b/spec/requests/schemes_controller_spec.rb
@@ -612,9 +612,9 @@ RSpec.describe SchemesController, type: :request do
context "with scheme that's deactivating soon" do
let(:scheme_deactivation_period) { create(:scheme_deactivation_period, deactivation_date: Time.zone.local(2022, 10, 12), scheme:) }
- it "does not render toggle scheme link" do
+ it "does render the reactivate this scheme button" do
expect(response).to have_http_status(:ok)
- expect(page).not_to have_link("Reactivate this scheme")
+ expect(page).to have_link("Reactivate this scheme")
expect(page).not_to have_link("Deactivate this scheme")
end
end
@@ -680,9 +680,9 @@ RSpec.describe SchemesController, type: :request do
context "with scheme that's deactivating soon" do
let(:scheme_deactivation_period) { create(:scheme_deactivation_period, deactivation_date: Time.zone.local(2022, 10, 12), scheme: specific_scheme) }
- it "does not render toggle scheme link" do
+ it "does render the reactivate this scheme button" do
expect(response).to have_http_status(:ok)
- expect(page).not_to have_link("Reactivate this scheme")
+ expect(page).to have_link("Reactivate this scheme")
expect(page).not_to have_link("Deactivate this scheme")
end
end
@@ -784,6 +784,7 @@ RSpec.describe SchemesController, type: :request do
context "and associated logs in editable collection period" do
before do
+ create(:location, scheme:)
create(:lettings_log, :sh, scheme:, startdate: Time.zone.local(2022, 9, 9), owning_organisation: user.organisation)
get "/schemes/#{scheme.id}"
end
@@ -2533,14 +2534,18 @@ RSpec.describe SchemesController, type: :request do
it "redirects to the confirmation page" do
follow_redirect!
expect(response).to have_http_status(:ok)
- expect(page).to have_content("This change will affect #{scheme.lettings_logs.count} logs")
+ expect(page).to have_content("This change will affect #{scheme.lettings_logs.count} log and 1 location.")
end
end
context "and no affected logs" do
let(:setup_schemes) { scheme.lettings_logs.update(scheme: nil) }
- it "redirects to the location page and updates the deactivation period" do
+ before do
+ create(:location_deactivation_period, deactivation_date: Time.zone.local(2022, 4, 1), reactivation_date: nil, location:)
+ end
+
+ it "redirects to the scheme page and updates the deactivation period" do
follow_redirect!
follow_redirect!
follow_redirect!
@@ -2560,14 +2565,18 @@ RSpec.describe SchemesController, type: :request do
it "redirects to the confirmation page" do
follow_redirect!
expect(response).to have_http_status(:ok)
- expect(page).to have_content("This change will affect #{scheme.lettings_logs.count} logs")
+ expect(page).to have_content("This change will affect #{scheme.lettings_logs.count} log and 1 location.")
end
end
context "and no affected logs" do
let(:setup_schemes) { scheme.lettings_logs.update(scheme: nil) }
- it "redirects to the location page and updates the deactivation period" do
+ before do
+ create(:location_deactivation_period, deactivation_date: Time.zone.local(2022, 4, 5), reactivation_date: nil, location:)
+ end
+
+ it "redirects to the scheme page and updates the deactivation period" do
follow_redirect!
follow_redirect!
follow_redirect!
@@ -2751,6 +2760,10 @@ RSpec.describe SchemesController, type: :request do
let(:params) { { scheme_deactivation_period: { deactivation_date_type: "other", "deactivation_date(3i)": "8", "deactivation_date(2i)": "9", "deactivation_date(1i)": "2023" } } }
let(:add_deactivations) { create(:scheme_deactivation_period, deactivation_date: Time.zone.local(2023, 6, 5), reactivation_date: nil, scheme:) }
+ before do
+ create(:location_deactivation_period, deactivation_date: Time.zone.local(2022, 4, 5), reactivation_date: nil, location:)
+ end
+
it "redirects to the scheme page and updates the existing deactivation period" do
follow_redirect!
follow_redirect!
@@ -2771,7 +2784,7 @@ RSpec.describe SchemesController, type: :request do
it "redirects to the confirmation page" do
follow_redirect!
expect(response).to have_http_status(:ok)
- expect(page).to have_content("This change will affect 1 logs")
+ expect(page).to have_content("This change will affect 1 log and 1 location.")
end
end
end
diff --git a/spec/views/schemes/deactivate_confirm.html.erb_spec.rb b/spec/views/schemes/deactivate_confirm.html.erb_spec.rb
new file mode 100644
index 000000000..fd1f2a2ac
--- /dev/null
+++ b/spec/views/schemes/deactivate_confirm.html.erb_spec.rb
@@ -0,0 +1,46 @@
+require "rails_helper"
+
+RSpec.describe "schemes/deactivate_confirm.html.erb", type: :view do
+ let(:scheme) { create(:scheme, service_name: "ABCScheme") }
+ let(:deactivation_date) { Time.zone.today + 1.month }
+ let(:affected_logs) { create_list(:lettings_log, 2, scheme:, status: 1) }
+ let(:affected_locations) { create_list(:location, 3, scheme:) }
+ let(:scheme_deactivation_period) { SchemeDeactivationPeriod.new }
+
+ before do
+ assign(:scheme, scheme)
+ assign(:deactivation_date, deactivation_date)
+ assign(:affected_logs, affected_logs)
+ assign(:affected_locations, affected_locations)
+ assign(:scheme_deactivation_period, scheme_deactivation_period)
+ render
+ end
+
+ it "displays the service name in the caption" do
+ expect(rendered).to have_css("span.govuk-caption-l", text: scheme.service_name)
+ end
+
+ it "displays the correct heading" do
+ expect(rendered).to have_css("h1.govuk-heading-l", text: "This change will affect 2 logs and 3 locations.")
+ end
+
+ it "displays the affected logs count" do
+ expect(rendered).to have_text("2 existing logs using this scheme have a tenancy start date after #{deactivation_date.to_formatted_s(:govuk_date)}.")
+ end
+
+ it "displays the warning text" do
+ expect(rendered).to have_css(".govuk-warning-text", text: I18n.t("warnings.scheme.deactivate.review_logs"))
+ end
+
+ it "displays the affected locations count" do
+ expect(rendered).to have_text("This scheme has 3 locations active on #{deactivation_date.to_formatted_s(:govuk_date)}.")
+ end
+
+ it "renders the submit button" do
+ expect(rendered).to have_button("Deactivate this scheme")
+ end
+
+ it "renders the cancel button" do
+ expect(rendered).to have_link("Cancel", href: scheme_details_path(scheme))
+ end
+end
From d1258faa6cd658dc58f566aa47c6b8345868b717 Mon Sep 17 00:00:00 2001
From: Manny Dinssa <44172848+Dinssa@users.noreply.github.com>
Date: Thu, 12 Sep 2024 09:10:28 +0100
Subject: [PATCH 3/8] Update guidance text (#2631)
---
app/views/bulk_upload_shared/guidance.html.erb | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/app/views/bulk_upload_shared/guidance.html.erb b/app/views/bulk_upload_shared/guidance.html.erb
index c32295b13..e530aa5b5 100644
--- a/app/views/bulk_upload_shared/guidance.html.erb
+++ b/app/views/bulk_upload_shared/guidance.html.erb
@@ -81,8 +81,8 @@
<%= accordion.with_section(heading_text: "Next steps") do %>
Once you've saved your CSV file, you can upload it via a button at the top of the lettings and sales logs pages.
When your file is done processing, you will receive an email explaining your next steps. If all your data is valid, your logs will be created. If some data is invalid, you’ll receive an email with instructions about how to resolve the errors.
-
If your file has errors on fields 1 through 17, you must fix these in the CSV. This is because we need to know these answers to validate the rest of the data. Any errors in these fields will be featured in the error report’s summary tab.
-
If none of your errors are in fields 1 through 17, you can choose how to fix the errors. You can either fix them in the CSV and reupload, or create partially complete logs and answer the remaining questions on the CORE site. Any errors that affect a significant number of logs will be featured in the error report’s summary tab to help you decide.
+
If your file has errors on fields 1 through 15 for lettings, or 1 through 18 for sales, you must fix these in the CSV. This is because we need to know these answers to validate the rest of the data. Any errors in these fields will be featured in the error report’s summary tab.
+
If none of your errors are in fields 1 through 15 for lettings, or 1 through 18 for sales, you can choose how to fix the errors. You can either fix them in the CSV and reupload, or create partially complete logs and answer the remaining questions on the CORE site. Any errors that affect a significant number of logs will be featured in the error report’s summary tab to help you decide.
<% end %>
From aeb998c537dc3000f9761ebe6ed11cc6a7910b66 Mon Sep 17 00:00:00 2001
From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com>
Date: Fri, 13 Sep 2024 12:21:44 +0100
Subject: [PATCH 4/8] 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 5/8] 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 6/8] 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 7/8] 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 8/8] 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 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| %>
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