From d16f71981ab7e30d9f1a744de1de9746b60cf34b Mon Sep 17 00:00:00 2001
From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com>
Date: Tue, 8 Oct 2024 14:48:00 +0100
Subject: [PATCH] CLDC-3619 Add duplicate schemes after merge view (#2655)
* Add duplicate sets scope to schemes
* Add rake task to write duplicate scheme sets
* Add duplicate sets scope to locations
* Add rake task to write duplicate locations
* lint
* Update location duplicate count
* Add scheme_id back to DUPLICATE_LOCATION_ATTRIBUTES
* Add display duplicate schemes banner method
* Add banner to the schemes page
* Add page content
* Allow confirming duplicate schemes
* Update display_duplicate_schemes_banner? method
* Make title dynamic
---
app/controllers/organisations_controller.rb | 40 +++++
app/helpers/schemes_helper.rb | 8 +
app/policies/organisation_policy.rb | 8 +
.../organisations/duplicate_schemes.html.erb | 138 ++++++++++++++++
app/views/organisations/schemes.html.erb | 10 ++
config/locales/en.yml | 2 +
config/routes.rb | 2 +
...40920144611_add_schemes_deduplicated_at.rb | 5 +
db/schema.rb | 1 +
spec/helpers/schemes_helper_spec.rb | 117 ++++++++++++++
.../requests/organisations_controller_spec.rb | 150 ++++++++++++++++++
11 files changed, 481 insertions(+)
create mode 100644 app/views/organisations/duplicate_schemes.html.erb
create mode 100644 db/migrate/20240920144611_add_schemes_deduplicated_at.rb
diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb
index 044bab74d..61cd43674 100644
--- a/app/controllers/organisations_controller.rb
+++ b/app/controllers/organisations_controller.rb
@@ -44,6 +44,12 @@ class OrganisationsController < ApplicationController
redirect_to schemes_csv_confirmation_organisation_path
end
+ def duplicate_schemes
+ authorize @organisation
+
+ get_duplicate_schemes_and_locations
+ end
+
def show
redirect_to details_organisation_path(@organisation)
end
@@ -295,6 +301,22 @@ class OrganisationsController < ApplicationController
render json: org_data.to_json
end
+ def confirm_duplicate_schemes
+ authorize @organisation
+
+ if scheme_duplicates_checked_params[:scheme_duplicates_checked] == "true"
+ @organisation.schemes_deduplicated_at = Time.zone.now
+ if @organisation.save
+ flash[:notice] = I18n.t("organisation.duplicate_schemes_confirmed")
+ redirect_to schemes_organisation_path(@organisation)
+ end
+ else
+ @organisation.errors.add(:scheme_duplicates_checked, I18n.t("validations.organisation.scheme_duplicates_not_resolved"))
+ get_duplicate_schemes_and_locations
+ render :duplicate_schemes, status: :unprocessable_entity
+ end
+ end
+
private
def filter_type
@@ -325,6 +347,10 @@ private
params.require(:organisation).permit(rent_periods: [], all_rent_periods: [])
end
+ def scheme_duplicates_checked_params
+ params.require(:organisation).permit(:scheme_duplicates_checked)
+ end
+
def codes_only_export?
params.require(:codes_only) == "true"
end
@@ -344,4 +370,18 @@ private
def find_resource
@organisation = Organisation.find(params[:id])
end
+
+ def get_duplicate_schemes_and_locations
+ duplicate_scheme_sets = @organisation.owned_schemes.duplicate_sets
+ @duplicate_schemes = duplicate_scheme_sets.map { |set| set.map { |id| @organisation.owned_schemes.find(id) } }
+ @duplicate_locations = []
+ @organisation.owned_schemes.each do |scheme|
+ duplicate_location_sets = scheme.locations.duplicate_sets
+ next unless duplicate_location_sets.any?
+
+ duplicate_location_sets.each do |duplicate_set|
+ @duplicate_locations << { scheme: scheme, locations: duplicate_set.map { |id| scheme.locations.find(id) } }
+ end
+ end
+ end
end
diff --git a/app/helpers/schemes_helper.rb b/app/helpers/schemes_helper.rb
index 0e318d283..7efb9fffd 100644
--- a/app/helpers/schemes_helper.rb
+++ b/app/helpers/schemes_helper.rb
@@ -85,6 +85,14 @@ module SchemesHelper
end
end
+ def display_duplicate_schemes_banner?(organisation, current_user)
+ return unless organisation.absorbed_organisations.merged_during_open_collection_period.any?
+ return unless current_user.data_coordinator? || current_user.support?
+ return if organisation.schemes_deduplicated_at.present? && organisation.schemes_deduplicated_at > organisation.absorbed_organisations.map(&:merge_date).max
+
+ organisation.owned_schemes.duplicate_sets.any? || organisation.owned_schemes.any? { |scheme| scheme.locations.duplicate_sets.any? }
+ end
+
private
ActivePeriod = Struct.new(:from, :to)
diff --git a/app/policies/organisation_policy.rb b/app/policies/organisation_policy.rb
index 9c27d6e91..9c5fc4449 100644
--- a/app/policies/organisation_policy.rb
+++ b/app/policies/organisation_policy.rb
@@ -34,4 +34,12 @@ class OrganisationPolicy
editable_sales_logs = organisation.sales_logs.visible.after_date(editable_from_date)
organisation.sales_logs.visible.where(saledate: nil).any? || editable_sales_logs.any?
end
+
+ def duplicate_schemes?
+ user.support? || (user.data_coordinator? && user.organisation == organisation)
+ end
+
+ def confirm_duplicate_schemes?
+ duplicate_schemes?
+ end
end
diff --git a/app/views/organisations/duplicate_schemes.html.erb b/app/views/organisations/duplicate_schemes.html.erb
new file mode 100644
index 000000000..427cf427c
--- /dev/null
+++ b/app/views/organisations/duplicate_schemes.html.erb
@@ -0,0 +1,138 @@
+<% content_for :before_content do %>
+ <%= govuk_back_link href: schemes_organisation_path(@organisation) %>
+<% end %>
+<%= form_with model: @organisation, url: schemes_duplicates_organisation_path(@organisation), method: "post" do |f| %>
+ <%= f.govuk_error_summary %>
+
+ <% if @duplicate_schemes.any? %>
+ <% if @duplicate_locations.any? %>
+ <% title = "Review these sets of schemes and locations" %>
+ <% else %>
+ <% title = "Review these sets of schemes" %>
+ <% end %>
+ <% else %>
+ <% title = "Review these sets of locations" %>
+ <% end %>
+
+ <% content_for :title, title %>
+
+ <% if current_user.support? %>
+ <%= render SubNavigationComponent.new(
+ items: secondary_items(request.path, @organisation.id),
+ ) %>
+ <% end %>
+
+
+
+
<%= title %>
+
+
Since your organisation recently merged, we’ve reviewed your schemes for possible duplicates.
+
These sets of schemes and locations might be duplicates because they have the same answers for certain fields.
+
What you need to do
+
+ - Review each set of schemes or locations and decide if they are duplicates.
+ - If they are, choose one to keep and deactivate the others on the date your organisation merged.
+ - When you have resolved all duplicates, confirm below.
+
+
If you need help with this, <%= govuk_link_to "contact the helpdesk (opens in a new tab)", GlobalConstants::HELPDESK_URL, target: "#" %>.
+
+ <% if @duplicate_schemes.any? %>
+
<%= @duplicate_schemes.count == 1 ? "This set" : "These #{@duplicate_schemes.count} sets" %> of schemes might have duplicates
+
+ <%= govuk_details(summary_text: "Why are these schemes identified as duplicates?") do %>
+
+ These schemes have the same answers for the following fields:
+
+
+ - Type of scheme
+ - Registered under Care Standards Act 2000
+ - Housing stock owned by
+ - Support services provided by
+ - Primary client group
+ - Has another client group
+ - Secondary client group
+ - Level of support given
+ - Intended length of stay
+
+ <% end %>
+
+ <%= govuk_table do |table| %>
+ <%= table.with_head do |head| %>
+ <% head.with_row do |row| %>
+ <% row.with_cell(header: true, text: "Schemes") %>
+ <% end %>
+
+ <%= table.with_body do |body| %>
+ <% @duplicate_schemes.each do |duplicate_set| %>
+ <% body.with_row do |row| %>
+ <% row.with_cell do %>
+
+ <% duplicate_set.each do |scheme| %>
+ -
+ <%= govuk_link_to scheme.service_name, scheme %>
+
+ <% end %>
+
+ <% end %>
+ <% end %>
+ <% end %>
+ <% end %>
+ <% end %>
+ <% end %>
+ <% end %>
+
+ <% if @duplicate_locations.any? %>
+
<%= @duplicate_locations.count == 1 ? "This set" : "These #{@duplicate_locations.count} sets" %> of locations might have duplicates
+ <%= govuk_details(summary_text: "Why are these locations identified as duplicates?") do %>
+
+ These locations belong to the same scheme and have the same answers for the following fields:
+
+
+ - Postcode
+ - Mobility standards
+
+ <% end %>
+
+ <%= govuk_table do |table| %>
+ <%= table.with_head do |head| %>
+ <% head.with_row do |row| %>
+ <% row.with_cell(header: true, text: "Locations") %>
+ <% row.with_cell(header: true, text: "Scheme") %>
+ <% end %>
+
+ <%= table.with_body do |body| %>
+ <% @duplicate_locations.each do |duplicate_set| %>
+ <% body.with_row do |row| %>
+ <% row.with_cell do %>
+
+ <% duplicate_set[:locations].each do |location| %>
+ -
+ <%= govuk_link_to location.name, scheme_location_path(location) %>
+
+ <% end %>
+
+ <% end %>
+ <% row.with_cell do %>
+ <%= govuk_link_to duplicate_set[:scheme].service_name, duplicate_set[:scheme] %>
+ <% end %>
+ <% end %>
+ <% end %>
+ <% end %>
+ <% end %>
+ <% end %>
+ <% end %>
+
+ <%= f.govuk_check_boxes_fieldset :scheme_duplicates_checked,
+ legend: { text: "Have you resolved all duplicates?" } do %>
+ <%= f.govuk_check_box :scheme_duplicates_checked,
+ true,
+ false,
+ multiple: false,
+ checked: false,
+ label: { text: "Yes, none of the schemes and locations above are duplicates" } %>
+ <% end %>
+
+ <%= f.govuk_submit "Confirm" %>
+
+
Supported housing schemes
<% end %>
+
+<% if display_duplicate_schemes_banner?(@organisation, current_user) %>
+ <%= govuk_notification_banner(title_text: "Important") do %>
+
+ Some schemes and locations might be duplicates.
+
+ <%= govuk_link_to "Review possible duplicates", href: schemes_duplicates_organisation_path(@organisation) %>
+ <% end %>
+<% end %>
+
<% if SchemePolicy.new(current_user, nil).create? %>
<%= govuk_button_link_to "Create a new supported housing scheme", new_scheme_path, html: { method: :post } %>
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 50a821ce0..e3ef39517 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -37,6 +37,7 @@ en:
updated: "Organisation details updated."
reactivated: "%{organisation} has been reactivated."
deactivated: "%{organisation} has been deactivated."
+ duplicate_schemes_confirmed: "You’ve confirmed the remaining schemes and locations are not duplicates."
user:
create_password: "Create a password to finish setting up your account."
reset_password: "Reset your password."
@@ -229,6 +230,7 @@ en:
blank: "You must choose a managing agent."
already_added: "You have already added this managing agent."
merged: "That organisation has already been merged. Select a different organisation."
+ scheme_duplicates_not_resolved: "You must resolve all duplicates or indicate that there are no duplicates"
not_answered: "You must answer %{question}"
invalid_option: "Enter a valid value for %{question}"
invalid_number: "Enter a number for %{question}"
diff --git a/config/routes.rb b/config/routes.rb
index 77b862e5a..ed6f47bbd 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -178,6 +178,8 @@ Rails.application.routes.draw do
get "schemes/csv-download", to: "organisations#download_schemes_csv"
post "schemes/email-csv", to: "organisations#email_schemes_csv"
get "schemes/csv-confirmation", to: "schemes#csv_confirmation"
+ get "schemes/duplicates", to: "organisations#duplicate_schemes"
+ post "schemes/duplicates", to: "organisations#confirm_duplicate_schemes"
get "stock-owners", to: "organisation_relationships#stock_owners"
get "stock-owners/add", to: "organisation_relationships#add_stock_owner"
get "stock-owners/remove", to: "organisation_relationships#remove_stock_owner"
diff --git a/db/migrate/20240920144611_add_schemes_deduplicated_at.rb b/db/migrate/20240920144611_add_schemes_deduplicated_at.rb
new file mode 100644
index 000000000..02c1a6e05
--- /dev/null
+++ b/db/migrate/20240920144611_add_schemes_deduplicated_at.rb
@@ -0,0 +1,5 @@
+class AddSchemesDeduplicatedAt < ActiveRecord::Migration[7.0]
+ def change
+ add_column :organisations, :schemes_deduplicated_at, :datetime
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index ff1f913df..174ae0199 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -514,6 +514,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_09_23_145326) do
t.bigint "absorbing_organisation_id"
t.datetime "available_from"
t.datetime "discarded_at"
+ t.datetime "schemes_deduplicated_at"
t.index ["absorbing_organisation_id"], name: "index_organisations_on_absorbing_organisation_id"
t.index ["old_visible_id"], name: "index_organisations_on_old_visible_id", unique: true
end
diff --git a/spec/helpers/schemes_helper_spec.rb b/spec/helpers/schemes_helper_spec.rb
index 0df032c3f..8ffde636a 100644
--- a/spec/helpers/schemes_helper_spec.rb
+++ b/spec/helpers/schemes_helper_spec.rb
@@ -118,4 +118,121 @@ RSpec.describe SchemesHelper do
end
end
end
+
+ describe "display_duplicate_schemes_banner?" do
+ let(:organisation) { create(:organisation) }
+ let(:current_user) { create(:user, :support) }
+
+ context "when organisation has not absorbed other organisations" do
+ context "and it has duplicate schemes" do
+ before do
+ create_list(:scheme, 2, :duplicate, owning_organisation: organisation)
+ end
+
+ it "does not display the banner" do
+ expect(display_duplicate_schemes_banner?(organisation, current_user)).to be_falsey
+ end
+ end
+ end
+
+ context "when organisation has absorbed other organisations in open collection year" do
+ before do
+ build(:organisation, merge_date: Time.zone.yesterday, absorbing_organisation_id: organisation.id).save(validate: false)
+ end
+
+ context "and it has duplicate schemes" do
+ before do
+ create_list(:scheme, 2, :duplicate, owning_organisation: organisation)
+ end
+
+ it "displays the banner" do
+ expect(display_duplicate_schemes_banner?(organisation, current_user)).to be_truthy
+ end
+
+ context "and organisation has confirmed duplicate schemes after the most recent merge" do
+ before do
+ organisation.update!(schemes_deduplicated_at: Time.zone.today)
+ end
+
+ it "does not display the banner" do
+ expect(display_duplicate_schemes_banner?(organisation, current_user)).to be_falsey
+ end
+ end
+
+ context "and organisation has confirmed duplicate schemes before the most recent merge" do
+ before do
+ organisation.update!(schemes_deduplicated_at: Time.zone.today - 2.days)
+ end
+
+ it "displays the banner" do
+ expect(display_duplicate_schemes_banner?(organisation, current_user)).to be_truthy
+ end
+ end
+ end
+
+ context "and it has duplicate locations" do
+ let(:scheme) { create(:scheme, owning_organisation: organisation) }
+
+ before do
+ create_list(:location, 2, postcode: "AB1 2CD", mobility_type: "A", scheme:)
+ end
+
+ it "displays the banner" do
+ expect(display_duplicate_schemes_banner?(organisation, current_user)).to be_truthy
+ end
+ end
+
+ context "and it has no duplicate schemes or locations" do
+ it "does not display the banner" do
+ expect(display_duplicate_schemes_banner?(organisation, current_user)).to be_falsey
+ end
+ end
+
+ context "and it is viewed by data provider" do
+ let(:current_user) { create(:user, :data_provider) }
+
+ before do
+ create_list(:scheme, 2, :duplicate, owning_organisation: organisation)
+ end
+
+ it "does not display the banner" do
+ expect(display_duplicate_schemes_banner?(organisation, current_user)).to be_falsey
+ end
+ end
+ end
+
+ context "when organisation has absorbed other organisations in closed collection year" do
+ before do
+ build(:organisation, merge_date: Time.zone.today - 2.years, absorbing_organisation_id: organisation.id).save(validate: false)
+ end
+
+ context "and it has duplicate schemes" do
+ before do
+ create_list(:scheme, 2, :duplicate, owning_organisation: organisation)
+ end
+
+ it "does not display the banner" do
+ expect(display_duplicate_schemes_banner?(organisation, current_user)).to be_falsey
+ end
+ end
+
+ context "and it has duplicate locations" do
+ let(:scheme) { create(:scheme, owning_organisation: organisation) }
+
+ before do
+ create(:location, postcode: "AB1 2CD", mobility_type: "A", scheme:)
+ end
+
+ it "does not display the banner" do
+ expect(display_duplicate_schemes_banner?(organisation, current_user)).to be_falsey
+ end
+ end
+
+ context "and it has no duplicate schemes or locations" do
+ it "does not display the banner" do
+ expect(display_duplicate_schemes_banner?(organisation, current_user)).to be_falsey
+ end
+ end
+ end
+ end
end
diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb
index 91dc07094..223cc9e00 100644
--- a/spec/requests/organisations_controller_spec.rb
+++ b/spec/requests/organisations_controller_spec.rb
@@ -207,6 +207,24 @@ RSpec.describe OrganisationsController, type: :request do
expect(page).to have_title("#{user.organisation.name} (1 scheme matching ‘#{search_param}’) - Submit social housing lettings and sales data (CORE) - GOV.UK")
end
end
+
+ context "when organisation has absorbed other organisations" do
+ before do
+ create(:organisation, merge_date: Time.zone.today, absorbing_organisation: organisation)
+ end
+
+ context "and it has duplicate schemes or locations" do
+ before do
+ create_list(:scheme, 2, :duplicate, owning_organisation: organisation)
+ get "/organisations/#{organisation.id}/schemes", headers:, params: {}
+ end
+
+ it "displays a banner with correct content" do
+ expect(page).to have_content("Some schemes and locations might be duplicates.")
+ expect(page).to have_link("Review possible duplicates", href: "/organisations/#{organisation.id}/schemes/duplicates")
+ end
+ end
+ end
end
context "when data coordinator user" do
@@ -348,6 +366,77 @@ RSpec.describe OrganisationsController, type: :request do
end
end
+ describe "#duplicate_schemes" do
+ context "with support user" do
+ let(:user) { create(:user, :support) }
+
+ before do
+ allow(user).to receive(:need_two_factor_authentication?).and_return(false)
+ sign_in user
+ get "/organisations/#{organisation.id}/schemes/duplicates", headers:
+ end
+
+ context "with duplicate schemes and locations" do
+ let(:schemes) { create_list(:scheme, 5, :duplicate, owning_organisation: organisation) }
+
+ before do
+ create_list(:location, 2, scheme: schemes.first, postcode: "M1 1AA", mobility_type: "M")
+ create_list(:location, 2, scheme: schemes.first, postcode: "M1 1AA", mobility_type: "A")
+ get "/organisations/#{organisation.id}/schemes/duplicates", headers:
+ end
+
+ it "displays the duplicate schemes" do
+ expect(page).to have_content("This set of schemes might have duplicates")
+ end
+
+ it "displays the duplicate locations" do
+ expect(page).to have_content("These 2 sets of locations might have duplicates")
+ end
+
+ it "has page heading" do
+ expect(page).to have_content("Review these sets of schemes and locations")
+ end
+ end
+
+ context "without duplicate schemes and locations" do
+ it "does not display the schemes" do
+ expect(page).not_to have_content("schemes might have duplicates")
+ end
+
+ it "does not display the locations" do
+ expect(page).not_to have_content("locations might have duplicates")
+ end
+ end
+ end
+
+ context "with data coordinator user" do
+ let(:user) { create(:user, :data_coordinator) }
+
+ before do
+ sign_in user
+ create_list(:scheme, 5, :duplicate, owning_organisation: organisation)
+ get "/organisations/#{organisation.id}/schemes/duplicates", headers:
+ end
+
+ it "has page heading" do
+ expect(page).to have_content("Review these sets of schemes")
+ end
+ end
+
+ context "with data provider user" do
+ let(:user) { create(:user, :data_provider) }
+
+ before do
+ sign_in user
+ get "/organisations/#{organisation.id}/schemes/duplicates", headers:
+ end
+
+ it "be unauthorised" do
+ expect(response).to have_http_status(:unauthorized)
+ end
+ end
+ end
+
describe "#show" do
context "with an organisation that the user belongs to" do
let(:set_time) {}
@@ -2263,4 +2352,65 @@ RSpec.describe OrganisationsController, type: :request do
end
end
end
+
+ describe "POST #confirm_duplicate_schemes" do
+ let(:organisation) { create(:organisation) }
+
+ context "when not signed in" do
+ it "redirects to sign in" do
+ post "/organisations/#{organisation.id}/schemes/duplicates", headers: headers
+ expect(response).to redirect_to("/account/sign-in")
+ end
+ end
+
+ context "when signed in" do
+ before do
+ allow(user).to receive(:need_two_factor_authentication?).and_return(false)
+ sign_in user
+ end
+
+ context "when user is data provider" do
+ let(:user) { create(:user, role: "data_provider", organisation:) }
+
+ it "returns not found" do
+ post "/organisations/#{organisation.id}/schemes/duplicates", headers: headers
+ expect(response).to have_http_status(:unauthorized)
+ end
+ end
+
+ context "when user is coordinator" do
+ let(:user) { create(:user, role: "data_coordinator", organisation:) }
+
+ context "and the duplicate schemes have been confirmed" do
+ let(:params) { { "organisation": { scheme_duplicates_checked: "true" } } }
+
+ it "redirects to schemes page" do
+ post "/organisations/#{organisation.id}/schemes/duplicates", headers: headers, params: params
+
+ expect(response).to redirect_to("/organisations/#{organisation.id}/schemes")
+ expect(flash[:notice]).to eq("You’ve confirmed the remaining schemes and locations are not duplicates.")
+ end
+
+ it "updates schemes_deduplicated_at" do
+ expect(organisation.reload.schemes_deduplicated_at).to be_nil
+
+ post "/organisations/#{organisation.id}/schemes/duplicates", headers: headers, params: params
+
+ expect(organisation.reload.schemes_deduplicated_at).not_to be_nil
+ end
+ end
+
+ context "and the duplicate schemes have not been confirmed" do
+ let(:params) { { "organisation": { scheme_duplicates_checked: "" } } }
+
+ it "displays an error" do
+ post "/organisations/#{organisation.id}/schemes/duplicates", headers: headers, params: params
+
+ expect(response).to have_http_status(:unprocessable_entity)
+ expect(page).to have_content("You must resolve all duplicates or indicate that there are no duplicates")
+ end
+ end
+ end
+ end
+ end
end