From 325540f70ecb5b27747aa23845ebf9b315d7b7be Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Thu, 10 Aug 2023 14:11:48 +0100 Subject: [PATCH 1/3] CLDC 2107 Add merged org/startdate validations (#1824) * Add start date validations for merging orgs * Add errors for merged orgs * tests * don't run validations unless the date exists * Adjust the dates in validations --- app/models/validations/setup_validations.rb | 128 +++++++++ config/locales/en.yml | 15 + .../validations/setup_validations_spec.rb | 256 ++++++++++++++++++ 3 files changed, 399 insertions(+) diff --git a/app/models/validations/setup_validations.rb b/app/models/validations/setup_validations.rb index 055923536..d3f72a365 100644 --- a/app/models/validations/setup_validations.rb +++ b/app/models/validations/setup_validations.rb @@ -14,6 +14,8 @@ module Validations::SetupValidations unless record.startdate.between?(first_collection_start_date, current_collection_end_date) record.errors.add :startdate, startdate_validation_error_message end + + validate_merged_organisations_start_date(record) end def validate_irproduct_other(record) @@ -46,6 +48,33 @@ module Validations::SetupValidations record.errors.add :owning_organisation_id, I18n.t("validations.setup.owning_organisation.invalid") record.errors.add :managing_organisation_id, I18n.t("validations.setup.managing_organisation.invalid") end + return unless record.startdate + + if owning_organisation.present? + if owning_organisation&.merge_date.present? && owning_organisation.merge_date <= record.startdate + record.errors.add :owning_organisation_id, I18n.t("validations.setup.owning_organisation.inactive_merged_organisation", + owning_organisation: record.owning_organisation.name, + owning_organisation_merge_date: record.owning_organisation.merge_date.to_formatted_s(:govuk_date), + owning_absorbing_organisation: record.owning_organisation.absorbing_organisation.name) + elsif owning_organisation&.absorbed_organisations.present? && owning_organisation.created_at.to_date > record.startdate.to_date + record.errors.add :owning_organisation_id, I18n.t("validations.setup.owning_organisation.inactive_absorbing_organisation", + owning_organisation: record.owning_organisation.name, + owning_organisation_available_from: record.owning_organisation.created_at.to_formatted_s(:govuk_date)) + end + end + + if managing_organisation.present? + if managing_organisation&.merge_date.present? && managing_organisation.merge_date <= record.startdate + record.errors.add :managing_organisation_id, I18n.t("validations.setup.managing_organisation.inactive_merged_organisation", + managing_organisation: record.managing_organisation.name, + managing_organisation_merge_date: record.managing_organisation.merge_date.to_formatted_s(:govuk_date), + managing_absorbing_organisation: record.managing_organisation.absorbing_organisation.name) + elsif managing_organisation&.absorbed_organisations.present? && managing_organisation.created_at.to_date > record.startdate.to_date + record.errors.add :managing_organisation_id, I18n.t("validations.setup.managing_organisation.inactive_absorbing_organisation", + managing_organisation: record.managing_organisation.name, + managing_organisation_available_from: record.managing_organisation.created_at.to_formatted_s(:govuk_date)) + end + end end def validate_managing_organisation_data_sharing_agremeent_signed(record) @@ -98,4 +127,103 @@ private def intermediate_product_rent_type?(record) record.rent_type == 5 end + + def validate_merged_organisations_start_date(record) + return add_same_merge_organisation_error(record) if record.owning_organisation == record.managing_organisation + return add_same_merge_error(record) if organisations_belong_to_same_merge?(record.owning_organisation, record.managing_organisation) + + add_merged_organisations_errors(record) + add_absorbing_organisations_errors(record) + end + + def add_same_merge_organisation_error(record) + if merged_owning_organisation_inactive?(record) + record.errors.add :startdate, I18n.t("validations.setup.startdate.invalid_merged_organisations_start_date.same_organisation", + owning_organisation: record.owning_organisation.name, + owning_organisation_merge_date: record.owning_organisation.merge_date.to_formatted_s(:govuk_date), + owning_absorbing_organisation: record.owning_organisation.absorbing_organisation.name) + elsif absorbing_owning_organisation_inactive?(record) + record.errors.add :startdate, I18n.t("validations.setup.startdate.invalid_absorbing_organisations_start_date.same_organisation", + owning_organisation: record.owning_organisation.name, + owning_organisation_available_from: record.owning_organisation.created_at.to_formatted_s(:govuk_date)) + end + end + + def add_same_merge_error(record) + if merged_owning_organisation_inactive?(record) + record.errors.add :startdate, I18n.t("validations.setup.startdate.invalid_merged_organisations_start_date.same_merge", + owning_organisation: record.owning_organisation.name, + managing_organisation: record.managing_organisation.name, + owning_organisation_merge_date: record.owning_organisation.merge_date.to_formatted_s(:govuk_date), + owning_absorbing_organisation: record.owning_organisation.absorbing_organisation.name) + end + end + + def add_merged_organisations_errors(record) + if merged_owning_organisation_inactive?(record) && merged_managing_organisation_inactive?(record) + record.errors.add :startdate, I18n.t("validations.setup.startdate.invalid_merged_organisations_start_date.different_merge", + owning_organisation: record.owning_organisation.name, + owning_organisation_merge_date: record.owning_organisation.merge_date.to_formatted_s(:govuk_date), + owning_absorbing_organisation: record.owning_organisation.absorbing_organisation.name, + managing_organisation: record.managing_organisation.name, + managing_organisation_merge_date: record.managing_organisation.merge_date.to_formatted_s(:govuk_date), + managing_absorbing_organisation: record.managing_organisation.absorbing_organisation.name) + else + if merged_owning_organisation_inactive?(record) + record.errors.add :startdate, I18n.t("validations.setup.startdate.invalid_merged_organisations_start_date.owning_organisation", + owning_organisation: record.owning_organisation.name, + owning_organisation_merge_date: record.owning_organisation.merge_date.to_formatted_s(:govuk_date), + owning_absorbing_organisation: record.owning_organisation.absorbing_organisation.name) + end + + if merged_managing_organisation_inactive?(record) + record.errors.add :startdate, I18n.t("validations.setup.startdate.invalid_merged_organisations_start_date.managing_organisation", + managing_organisation: record.managing_organisation.name, + managing_organisation_merge_date: record.managing_organisation.merge_date.to_formatted_s(:govuk_date), + managing_absorbing_organisation: record.managing_organisation.absorbing_organisation.name) + end + end + end + + def add_absorbing_organisations_errors(record) + if absorbing_owning_organisation_inactive?(record) && absorbing_managing_organisation_inactive?(record) + record.errors.add :startdate, I18n.t("validations.setup.startdate.invalid_absorbing_organisations_start_date.different_organisations", + owning_organisation: record.owning_organisation.name, + owning_organisation_active_from: record.owning_organisation.created_at.to_formatted_s(:govuk_date), + managing_organisation: record.managing_organisation.name, + managing_organisation_active_from: record.managing_organisation.created_at.to_formatted_s(:govuk_date)) + else + if absorbing_owning_organisation_inactive?(record) + record.errors.add :startdate, I18n.t("validations.setup.startdate.invalid_absorbing_organisations_start_date.owning_organisation", + owning_organisation: record.owning_organisation.name, + owning_organisation_available_from: record.owning_organisation.created_at.to_formatted_s(:govuk_date)) + end + + if absorbing_managing_organisation_inactive?(record) + record.errors.add :startdate, I18n.t("validations.setup.startdate.invalid_absorbing_organisations_start_date.managing_organisation", + managing_organisation: record.managing_organisation.name, + managing_organisation_available_from: record.managing_organisation.created_at.to_formatted_s(:govuk_date)) + end + end + end + + def merged_owning_organisation_inactive?(record) + record.owning_organisation&.merge_date.present? && record.owning_organisation.merge_date <= record.startdate + end + + def merged_managing_organisation_inactive?(record) + record.managing_organisation&.merge_date.present? && record.managing_organisation.merge_date <= record.startdate + end + + def absorbing_owning_organisation_inactive?(record) + record.owning_organisation&.absorbed_organisations.present? && record.owning_organisation.created_at.to_date > record.startdate.to_date + end + + def absorbing_managing_organisation_inactive?(record) + record.managing_organisation&.absorbed_organisations.present? && record.managing_organisation.created_at.to_date > record.startdate.to_date + end + + def organisations_belong_to_same_merge?(organisation_a, organisation_b) + organisation_a&.merge_date.present? && organisation_b&.merge_date.present? && organisation_a.merge_date == organisation_b.merge_date && organisation_a.absorbing_organisation == organisation_b.absorbing_organisation + end end diff --git a/config/locales/en.yml b/config/locales/en.yml index 5b7654c12..182a91b26 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -252,6 +252,17 @@ en: year_not_two_digits: Tenancy start year must be 2 digits ten_years_after_void_date: "Enter a tenancy start date that is no more than 10 years after the void date" ten_years_after_mrc_date: "Enter a tenancy start date that is no more than 10 years after the major repairs completion date" + invalid_merged_organisations_start_date: + same_organisation: "Enter a date when the owning and managing organisation was active. %{owning_organisation} became inactive on %{owning_organisation_merge_date} and was replaced by %{owning_absorbing_organisation}." + same_merge: "Enter a date when the owning and managing organisations were active. %{owning_organisation} and %{managing_organisation} became inactive on %{owning_organisation_merge_date} and were replaced by %{owning_absorbing_organisation}." + owning_organisation: "Enter a date when the owning organisation was active. %{owning_organisation} became inactive on %{owning_organisation_merge_date} and was replaced by %{owning_absorbing_organisation}." + managing_organisation: "Enter a date when the managing organisation was active. %{managing_organisation} became inactive on %{managing_organisation_merge_date} and was replaced by %{managing_absorbing_organisation}." + different_merge: "Enter a date when the owning and managing organisations were active. %{owning_organisation} became inactive on %{owning_organisation_merge_date} and was replaced by %{owning_absorbing_organisation}. %{managing_organisation} became inactive on %{managing_organisation_merge_date} and was replaced by %{managing_absorbing_organisation}." + invalid_absorbing_organisations_start_date: + same_organisation: "Enter a date when the owning and managing organisation was active. %{owning_organisation} became active on %{owning_organisation_available_from}." + owning_organisation: "Enter a date when the owning organisation was active. %{owning_organisation} became active on %{owning_organisation_available_from}." + managing_organisation: "Enter a date when the managing organisation was active. %{managing_organisation} became active on %{managing_organisation_available_from}." + different_organisations: "Enter a date when the owning and managing organisations were active. %{owning_organisation} became active on %{owning_organisation_active_from}, and %{managing_organisation} became active on %{managing_organisation_active_from}." location: deactivated: @@ -273,9 +284,13 @@ en: owning_organisation: invalid: "Please select the owning organisation or managing organisation that you belong to" data_sharing_agreement_not_signed: "The organisation must accept the Data Sharing Agreement before it can be selected as the owning organisation." + inactive_merged_organisation: "The owning organisation must be active on the tenancy start date. %{owning_organisation} became inactive on %{owning_organisation_merge_date} and was replaced by %{owning_absorbing_organisation}." + inactive_absorbing_organisation: "The owning organisation must be active on the tenancy start date. %{owning_organisation} became active on %{owning_organisation_available_from}." managing_organisation: invalid: "Please select the owning organisation or managing organisation that you belong to" data_sharing_agreement_not_signed: "The organisation must accept the Data Sharing Agreement before it can be selected as the managing organisation." + inactive_merged_organisation: "The managing organisation must be active on the tenancy start date. %{managing_organisation} became inactive on %{managing_organisation_merge_date} and was replaced by %{managing_absorbing_organisation}." + inactive_absorbing_organisation: "The managing organisation must be active on the tenancy start date. %{managing_organisation} became active on %{managing_organisation_available_from}." created_by: invalid: "Please select the owning organisation or managing organisation that you belong to" lettype: diff --git a/spec/models/validations/setup_validations_spec.rb b/spec/models/validations/setup_validations_spec.rb index bcd7924c5..3def42da4 100644 --- a/spec/models/validations/setup_validations_spec.rb +++ b/spec/models/validations/setup_validations_spec.rb @@ -128,6 +128,186 @@ RSpec.describe Validations::SetupValidations do end end end + + context "when organisations were merged" do + around do |example| + Timecop.freeze(Time.zone.local(2023, 5, 1)) + example.run + Timecop.return + end + + let(:absorbing_organisation) { create(:organisation, created_at: Time.zone.local(2023, 2, 1, 4, 5, 6), name: "Absorbing org") } + let(:absorbing_organisation_2) { create(:organisation, created_at: Time.zone.local(2023, 2, 1), name: "Absorbing org 2") } + let(:merged_organisation) { create(:organisation, name: "Merged org") } + let(:merged_organisation_2) { create(:organisation, name: "Merged org 2") } + + before do + merged_organisation.update!(absorbing_organisation:, merge_date: Time.zone.local(2023, 2, 2)) + merged_organisation_2.update!(absorbing_organisation:, merge_date: Time.zone.local(2023, 2, 2)) + end + + context "and owning organisation is no longer active" do + it "does not allow startate after organisation has been merged" do + record.startdate = Time.zone.local(2023, 3, 1) + record.owning_organisation_id = merged_organisation.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to include(match "Enter a date when the owning organisation was active. Merged org became inactive on 2 February 2023 and was replaced by Absorbing org.") + end + + it "allows startate before organisation has been merged" do + record.startdate = Time.zone.local(2023, 1, 1) + record.owning_organisation_id = merged_organisation.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to be_empty + end + end + + context "and owning organisation is not yet active during the startdate" do + it "does not allow startate before absorbing organisation has been created" do + record.startdate = Time.zone.local(2023, 1, 1) + record.owning_organisation_id = absorbing_organisation.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to include(match "Enter a date when the owning organisation was active. Absorbing org became active on 1 February 2023.") + end + + it "allows startate after absorbing organisation has been created" do + record.startdate = Time.zone.local(2023, 2, 2) + record.owning_organisation_id = absorbing_organisation.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to be_empty + end + end + + context "and managing organisation is no longer active during the startdate" do + it "does not allow startate after organisation has been merged" do + record.startdate = Time.zone.local(2023, 3, 1) + record.managing_organisation_id = merged_organisation.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to include(match "Enter a date when the managing organisation was active. Merged org became inactive on 2 February 2023 and was replaced by Absorbing org.") + end + + it "allows startate before organisation has been merged" do + record.startdate = Time.zone.local(2023, 1, 1) + record.managing_organisation_id = merged_organisation.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to be_empty + end + end + + context "and managing organisation is not yet active during the startdate" do + it "does not allow startate before absorbing organisation has been created" do + record.startdate = Time.zone.local(2023, 1, 1) + record.managing_organisation_id = absorbing_organisation.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to include(match "Enter a date when the managing organisation was active. Absorbing org became active on 1 February 2023.") + end + + it "allows startate after absorbing organisation has been created" do + record.startdate = Time.zone.local(2023, 2, 2) + record.managing_organisation_id = absorbing_organisation.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to be_empty + end + end + + context "and owning and managing organisation is no longer active during the startdate" do + it "does not allow startate after organisation has been merged" do + record.startdate = Time.zone.local(2023, 3, 1) + record.managing_organisation_id = merged_organisation.id + record.owning_organisation_id = merged_organisation.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to include(match "Enter a date when the owning and managing organisation was active. Merged org became inactive on 2 February 2023 and was replaced by Absorbing org.") + end + + it "allows startate before organisation has been merged" do + record.startdate = Time.zone.local(2023, 1, 1) + record.managing_organisation_id = merged_organisation.id + record.owning_organisation_id = merged_organisation.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to be_empty + end + end + + context "and owning and managing organisation is not yet active during the startdate" do + it "does not allow startate before absorbing organisation has been created" do + record.startdate = Time.zone.local(2023, 1, 1) + record.managing_organisation_id = absorbing_organisation.id + record.owning_organisation_id = absorbing_organisation.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to include(match "Enter a date when the owning and managing organisation was active. Absorbing org became active on 1 February 2023.") + end + + it "allows startate after absorbing organisation has been created" do + record.startdate = Time.zone.local(2023, 2, 1) + record.managing_organisation_id = absorbing_organisation.id + record.owning_organisation_id = absorbing_organisation.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to be_empty + end + end + + context "and owning and managing organisations are no longer active during the startdate" do + it "does not allow startate after organisation have been merged" do + record.startdate = Time.zone.local(2023, 2, 2) + record.managing_organisation_id = merged_organisation.id + record.owning_organisation_id = merged_organisation_2.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to include(match "Enter a date when the owning and managing organisations were active. Merged org 2 and Merged org became inactive on 2 February 2023 and were replaced by Absorbing org.") + end + + it "allows startate before organisations have been merged" do + record.startdate = Time.zone.local(2023, 1, 1) + record.managing_organisation_id = merged_organisation.id + record.owning_organisation_id = merged_organisation_2.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to be_empty + end + end + + context "and owning and managing organisations are from different merges and no longer active during the startdate" do + before do + merged_organisation_2.update!(absorbing_organisation: absorbing_organisation_2, merge_date: Time.zone.local(2023, 2, 2)) + end + + it "does not allow startate after organisations have been merged" do + record.startdate = Time.zone.local(2023, 3, 1) + record.managing_organisation_id = merged_organisation.id + record.owning_organisation_id = merged_organisation_2.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to include(match "Enter a date when the owning and managing organisations were active. Merged org 2 became inactive on 2 February 2023 and was replaced by Absorbing org 2. Merged org became inactive on 2 February 2023 and was replaced by Absorbing org.") + end + + it "allows startate before organisations have been merged" do + record.startdate = Time.zone.local(2023, 1, 1) + record.managing_organisation_id = merged_organisation.id + record.owning_organisation_id = merged_organisation_2.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to be_empty + end + end + + context "and owning and managing organisation have different merges and are not yet active during the startdate" do + before do + merged_organisation_2.update!(absorbing_organisation: absorbing_organisation_2, merge_date: Time.zone.local(2023, 2, 2)) + end + + it "does not allow startate before absorbing organisation has been created" do + record.startdate = Time.zone.local(2023, 1, 1) + record.managing_organisation_id = absorbing_organisation.id + record.owning_organisation_id = absorbing_organisation_2.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to include(match "Enter a date when the owning and managing organisations were active. Absorbing org 2 became active on 1 February 2023, and Absorbing org became active on 1 February 2023.") + end + + it "allows startate after absorbing organisation has been created" do + record.startdate = Time.zone.local(2023, 2, 2) + record.managing_organisation_id = absorbing_organisation.id + record.owning_organisation_id = absorbing_organisation.id + setup_validator.validate_startdate_setup(record) + expect(record.errors["startdate"]).to be_empty + end + end + end end describe "#validate_irproduct" do @@ -521,6 +701,82 @@ RSpec.describe Validations::SetupValidations do expect(record.errors["owning_organisation_id"]).to be_empty expect(record.errors["managing_organisation_id"]).to be_empty end + + context "when organisations are merged" do + let(:absorbing_organisation) { create(:organisation, created_at: Time.zone.local(2023, 2, 1, 4, 5, 6), name: "Absorbing org") } + let(:merged_organisation) { create(:organisation, name: "Merged org") } + + around do |example| + Timecop.freeze(Time.zone.local(2023, 5, 1)) + merged_organisation.update!(merge_date: Time.zone.local(2023, 2, 2), absorbing_organisation:) + example.run + Timecop.return + end + + context "and owning organisation is no longer active" do + it "does not allow organisation that has been merged" do + record.startdate = Time.zone.local(2023, 3, 1) + record.owning_organisation_id = merged_organisation.id + setup_validator.validate_organisation(record) + expect(record.errors["owning_organisation_id"]).to include(match "The owning organisation must be active on the tenancy start date. Merged org became inactive on 2 February 2023 and was replaced by Absorbing org.") + end + + it "allows organisation before it has been merged" do + record.startdate = Time.zone.local(2023, 1, 1) + record.owning_organisation_id = merged_organisation.id + setup_validator.validate_organisation(record) + expect(record.errors["owning_organisation_id"]).to be_empty + end + end + + context "and owning organisation is not yet active during the startdate" do + it "does not allow absorbing organisation before it had been created" do + record.startdate = Time.zone.local(2023, 1, 1) + record.owning_organisation_id = absorbing_organisation.id + setup_validator.validate_organisation(record) + expect(record.errors["owning_organisation_id"]).to include(match "The owning organisation must be active on the tenancy start date. Absorbing org became active on 1 February 2023.") + end + + it "allows absorbing organisation after it has been created" do + record.startdate = Time.zone.local(2023, 2, 2) + record.owning_organisation_id = absorbing_organisation.id + setup_validator.validate_organisation(record) + expect(record.errors["owning_organisation_id"]).to be_empty + end + end + + context "when managing organisation is no longer active" do + it "does not allow organisation that has been merged" do + record.startdate = Time.zone.local(2023, 3, 1) + record.managing_organisation_id = merged_organisation.id + setup_validator.validate_organisation(record) + expect(record.errors["managing_organisation_id"]).to include(match "The managing organisation must be active on the tenancy start date. Merged org became inactive on 2 February 2023 and was replaced by Absorbing org.") + end + + it "allows organisation before it has been merged" do + record.startdate = Time.zone.local(2023, 1, 1) + record.managing_organisation_id = merged_organisation.id + setup_validator.validate_organisation(record) + expect(record.errors["managing_organisation_id"]).to be_empty + end + end + + context "when managing organisation is not yet active during the startdate" do + it "does not allow absorbing organisation before it had been created" do + record.startdate = Time.zone.local(2023, 1, 1) + record.managing_organisation_id = absorbing_organisation.id + setup_validator.validate_organisation(record) + expect(record.errors["managing_organisation_id"]).to include(match "The managing organisation must be active on the tenancy start date. Absorbing org became active on 1 February 2023.") + end + + it "allows absorbing organisation after it has been created" do + record.startdate = Time.zone.local(2023, 2, 2) + record.managing_organisation_id = absorbing_organisation.id + setup_validator.validate_organisation(record) + expect(record.errors["managing_organisation_id"]).to be_empty + end + end + end end describe "#validate_scheme_has_confirmed_locations_validation" do From a5f020bfee07295c08ede4c3dcef13087a4a053d Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Thu, 10 Aug 2023 14:12:41 +0100 Subject: [PATCH 2/3] CLDC-2108 Update sales owning organisation dropdown (#1821) * Display absorbed orgs/absorbed org stock owners * Only display orgs merged during open collection period * split guard clauses back * Do not show stock owners in sales logs owning org options * Update routed to method * Display active dates for support --- app/models/form/sales/pages/organisation.rb | 17 +-- .../sales/questions/owning_organisation_id.rb | 35 +++-- app/models/organisation.rb | 1 + .../form/sales/pages/organisation_spec.rb | 32 ++--- .../questions/owning_organisation_id_spec.rb | 132 +++++++++++++++--- 5 files changed, 158 insertions(+), 59 deletions(-) diff --git a/app/models/form/sales/pages/organisation.rb b/app/models/form/sales/pages/organisation.rb index 8f6728821..405cf51c1 100644 --- a/app/models/form/sales/pages/organisation.rb +++ b/app/models/form/sales/pages/organisation.rb @@ -10,24 +10,17 @@ class Form::Sales::Pages::Organisation < ::Form::Page ] end - def routed_to?(log, current_user) + def routed_to?(_log, current_user) return false unless current_user return true if current_user.support? - stock_owners = current_user.organisation.stock_owners + current_user.organisation.absorbed_organisations.where(holds_own_stock: true) + absorbed_stock_owners = current_user.organisation.absorbed_organisations.where(holds_own_stock: true) if current_user.organisation.holds_own_stock? - if current_user.organisation.absorbed_organisations.any?(&:holds_own_stock?) - return true - end - return true if stock_owners.count >= 1 - - log.update!(owning_organisation: current_user.organisation) + return true if absorbed_stock_owners.count >= 1 else - return false if stock_owners.count.zero? - return true if stock_owners.count > 1 - - log.update!(owning_organisation: stock_owners.first) + return false if absorbed_stock_owners.count.zero? + return true if absorbed_stock_owners.count > 1 end false diff --git a/app/models/form/sales/questions/owning_organisation_id.rb b/app/models/form/sales/questions/owning_organisation_id.rb index 0db5ec1a0..a09fe05ed 100644 --- a/app/models/form/sales/questions/owning_organisation_id.rb +++ b/app/models/form/sales/questions/owning_organisation_id.rb @@ -15,20 +15,35 @@ class Form::Sales::Questions::OwningOrganisationId < ::Form::Question return answer_opts unless log if log.owning_organisation_id.present? - answer_opts = answer_opts.merge({ log.owning_organisation.id => log.owning_organisation.name }) + answer_opts[log.owning_organisation.id] = log.owning_organisation.name end + recently_absorbed_organisations = user.organisation.absorbed_organisations.merged_during_open_collection_period if !user.support? && user.organisation.holds_own_stock? - answer_opts[user.organisation.id] = "#{user.organisation.name} (Your organisation)" + answer_opts[user.organisation.id] = if recently_absorbed_organisations.exists? + "#{user.organisation.name} (Your organisation, active as of #{user.organisation.created_at.to_fs(:govuk_date)})" + else + "#{user.organisation.name} (Your organisation)" + end end - user_answer_options = if user.support? - Organisation.where(holds_own_stock: true) - else - user.organisation.stock_owners + user.organisation.absorbed_organisations.where(holds_own_stock: true) - end.pluck(:id, :name).to_h + if user.support? + Organisation.where(holds_own_stock: true).find_each do |org| + if org.merge_date.present? + answer_opts[org.id] = "#{org.name} (inactive as of #{org.merge_date.to_fs(:govuk_date)})" if org.merge_date >= FormHandler.instance.start_date_of_earliest_open_for_editing_collection_period + elsif org.absorbed_organisations.merged_during_open_collection_period.exists? + answer_opts[org.id] = "#{org.name} (active as of #{org.created_at.to_fs(:govuk_date)})" + else + answer_opts[org.id] = org.name + end + end + else + recently_absorbed_organisations.each do |absorbed_org| + answer_opts[absorbed_org.id] = merged_organisation_label(absorbed_org.name, absorbed_org.merge_date) if absorbed_org.holds_own_stock? + end + end - answer_opts.merge(user_answer_options) + answer_opts end def displayed_answer_options(log, user = nil) @@ -66,4 +81,8 @@ private def selected_answer_option_is_derived?(_log) true end + + def merged_organisation_label(name, merge_date) + "#{name} (inactive as of #{merge_date.to_fs(:govuk_date)})" + end end diff --git a/app/models/organisation.rb b/app/models/organisation.rb index 1c048703f..dac2b0ff5 100644 --- a/app/models/organisation.rb +++ b/app/models/organisation.rb @@ -32,6 +32,7 @@ class Organisation < ApplicationRecord scope :search_by_name, ->(name) { where("name ILIKE ?", "%#{name}%") } scope :search_by, ->(param) { search_by_name(param) } + scope :merged_during_open_collection_period, -> { where("merge_date >= ?", FormHandler.instance.start_date_of_earliest_open_for_editing_collection_period) } has_paper_trail diff --git a/spec/models/form/sales/pages/organisation_spec.rb b/spec/models/form/sales/pages/organisation_spec.rb index 710edbecc..8f160ca53 100644 --- a/spec/models/form/sales/pages/organisation_spec.rb +++ b/spec/models/form/sales/pages/organisation_spec.rb @@ -88,8 +88,8 @@ RSpec.describe Form::Sales::Pages::Organisation, type: :model do expect(page.routed_to?(log, user)).to eq(false) end - it "updates owning_organisation_id" do - expect { page.routed_to?(log, user) }.to change(log.reload, :owning_organisation).from(nil).to(stock_owner) + it "does not update owning_organisation_id" do + expect { page.routed_to?(log, user) }.not_to change(log.reload, :owning_organisation) end end @@ -111,11 +111,7 @@ RSpec.describe Form::Sales::Pages::Organisation, type: :model do end it "is not shown" do - expect(page.routed_to?(log, user)).to eq(true) - end - - it "updates owning_organisation_id" do - expect { page.routed_to?(log, user) }.not_to change(log.reload, :owning_organisation) + expect(page.routed_to?(log, user)).to eq(false) end end end @@ -129,12 +125,6 @@ RSpec.describe Form::Sales::Pages::Organisation, type: :model do it "is not shown" do expect(page.routed_to?(log, user)).to eq(false) end - - it "updates owning_organisation_id to user organisation" do - expect { - page.routed_to?(log, user) - }.to change(log.reload, :owning_organisation).from(nil).to(user.organisation) - end end context "with >0 stock_owners" do @@ -143,12 +133,20 @@ RSpec.describe Form::Sales::Pages::Organisation, type: :model do create(:organisation_relationship, child_organisation: user.organisation) end - it "is shown" do - expect(page.routed_to?(log, user)).to eq(true) + it "is not shown" do + expect(page.routed_to?(log, user)).to eq(false) end + end - it "does not update owning_organisation_id" do - expect { page.routed_to?(log, user) }.not_to change(log.reload, :owning_organisation).from(nil) + context "with merged organisations" do + let(:merged_org) { create(:organisation) } + + before do + merged_org.update!(absorbing_organisation: user.organisation, merge_date: Time.zone.now) + end + + it "is shown" do + expect(page.routed_to?(log, user)).to eq(true) end end end diff --git a/spec/models/form/sales/questions/owning_organisation_id_spec.rb b/spec/models/form/sales/questions/owning_organisation_id_spec.rb index 815a42319..9cfefbf8b 100644 --- a/spec/models/form/sales/questions/owning_organisation_id_spec.rb +++ b/spec/models/form/sales/questions/owning_organisation_id_spec.rb @@ -70,53 +70,99 @@ RSpec.describe Form::Sales::Questions::OwningOrganisationId, type: :model do "" => "Select an option", owning_org_1.id => "Owning org 1", user.organisation.id => "User org (Your organisation)", - owning_org_2.id => "Owning org 2", } end - it "shows current stock owner at top, followed by user's org (with hint), followed by the stock owners of the user's org" do + it "does not show stock owner" do user.organisation.update!(holds_own_stock: true) expect(question.displayed_answer_options(log, user)).to eq(options) end + end - context "when the owning-managing organisation relationship is deleted" do - let(:options) do - { - "" => "Select an option", - user.organisation.id => "User org (Your organisation)", - owning_org_2.id => "Owning org 2", - } - end + context "when user's org doesn't own stock" do + let(:options) do + { + "" => "Select an option", + owning_org_1.id => "Owning org 1", + } + end - it "doesn't remove the housing provider from the list of allowed housing providers" do - log.update!(owning_organisation: owning_org_2) - expect(question.displayed_answer_options(log, user)).to eq(options) - org_rel.destroy! - expect(question.displayed_answer_options(log, user)).to eq(options) - end + it "shows current log owner" do + user.organisation.update!(holds_own_stock: false) + expect(question.displayed_answer_options(log, user)).to eq(options) end end - context "when user's org doesn't own stock" do + context "when user's org has recently absorbed other orgs" do + let(:merged_organisation) { create(:organisation, name: "Merged org") } let(:options) do { "" => "Select an option", + user.organisation.id => "User org (Your organisation, active as of 2 February 2021)", owning_org_1.id => "Owning org 1", - owning_org_2.id => "Owning org 2", + merged_organisation.id => "Merged org (inactive as of 2 February 2023)", } end - it "shows current stock owner at top, followed by the stock owners of the user's org" do - user.organisation.update!(holds_own_stock: false) + before do + merged_organisation.update!(merge_date: Time.zone.local(2023, 2, 2), absorbing_organisation: user.organisation) + user.organisation.update!(created_at: Time.zone.local(2021, 2, 2)) + end + + it "shows merged organisation as an option" do + expect(question.displayed_answer_options(log, user)).to eq(options) + end + end + + context "when user's org has recently absorbed other orgs with parent organisations" do + let(:merged_organisation) { create(:organisation, name: "Merged org") } + let(:options) do + { + "" => "Select an option", + user.organisation.id => "User org (Your organisation, active as of 2 February 2021)", + owning_org_1.id => "Owning org 1", + merged_organisation.id => "Merged org (inactive as of 2 February 2023)", + } + end + + before do + org_rel.update!(child_organisation: merged_organisation) + merged_organisation.update!(merge_date: Time.zone.local(2023, 2, 2), absorbing_organisation: user.organisation) + user.organisation.update!(created_at: Time.zone.local(2021, 2, 2)) + end + + it "does not show merged organisations stock owners as options" do + expect(question.displayed_answer_options(log, user)).to eq(options) + end + end + + context "when user's org has absorbed other orgs with parent organisations during closed collection periods" do + let(:merged_organisation) { create(:organisation, name: "Merged org") } + let(:options) do + { + "" => "Select an option", + user.organisation.id => "User org (Your organisation)", + owning_org_1.id => "Owning org 1", + } + end + + before do + Timecop.freeze(Time.zone.local(2023, 4, 2)) + org_rel.update!(child_organisation: merged_organisation) + merged_organisation.update!(merge_date: Time.zone.local(2021, 6, 2), absorbing_organisation: user.organisation) + user.organisation.update!(created_at: Time.zone.local(2021, 2, 2)) + end + + it "shows merged organisation as an option" do expect(question.displayed_answer_options(log, user)).to eq(options) end end end context "when user is support" do - let(:user) { create(:user, :support) } + let(:user) { create(:user, :support, organisation: organisation_1) } - let(:log) { create(:lettings_log) } + let(:log) { create(:lettings_log, created_by: user) } let(:non_stock_organisation) { create(:organisation, holds_own_stock: false) } let(:expected_opts) do @@ -130,6 +176,48 @@ RSpec.describe Form::Sales::Questions::OwningOrganisationId, type: :model do expect(question.displayed_answer_options(log, user)).to eq(expected_opts) expect(question.displayed_answer_options(log, user)).not_to include(non_stock_organisation.id) end + + context "when an org has recently absorbed other orgs" do + let(:merged_organisation) { create(:organisation, name: "Merged org") } + let(:options) do + { + "" => "Select an option", + organisation_1.id => "first test org (active as of 2 February 2021)", + organisation_2.id => "second test org", + merged_organisation.id => "Merged org (inactive as of 2 February 2023)", + } + end + + before do + merged_organisation.update!(merge_date: Time.zone.local(2023, 2, 2), absorbing_organisation: organisation_1) + organisation_1.update!(created_at: Time.zone.local(2021, 2, 2)) + end + + it "shows merged organisation as an option" do + expect(question.displayed_answer_options(log, user)).to eq(options) + end + end + + context "when user's org has absorbed other orgs during closed collection periods" do + let(:merged_organisation) { create(:organisation, name: "Merged org") } + let(:options) do + { + "" => "Select an option", + organisation_1.id => "first test org", + organisation_2.id => "second test org", + } + end + + before do + Timecop.freeze(Time.zone.local(2023, 4, 2)) + merged_organisation.update!(merge_date: Time.zone.local(2021, 6, 2), absorbing_organisation: user.organisation) + user.organisation.update!(created_at: Time.zone.local(2021, 2, 2)) + end + + it "shows merged organisation as an option" do + expect(question.displayed_answer_options(log, user)).to eq(options) + end + end end end From 8d085528c49b369b21cfe02d226a9886a939702b Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Thu, 10 Aug 2023 15:03:08 +0100 Subject: [PATCH 3/3] CLDC-2496 Correct duplicate data (#1793) * Update check answers to use the correct logs * Display different link when correcting duplicates * allow deduplicating logs by changing the answers * Route back to the initial log if it still has duplicates * Update back to log button * Add banner * Update success banner * Fix a flaky test * Rename param and update conditionals * Add owning organisation to deduplication fields, only return back to the original log if it still belongs to the org --- app/controllers/duplicate_logs_controller.rb | 11 +- app/controllers/form_controller.rb | 52 +++++- app/helpers/duplicate_logs_helper.rb | 13 +- app/models/lettings_log.rb | 2 +- app/models/sales_log.rb | 2 +- .../_duplicate_log_check_answers.erb | 27 ++- app/views/duplicate_logs/show.html.erb | 4 +- app/views/logs/delete_duplicates.html.erb | 2 +- config/locales/en.yml | 2 + spec/features/lettings_log_spec.rb | 23 +++ spec/features/sales_log_spec.rb | 23 +++ spec/models/lettings_log_spec.rb | 31 ++-- spec/models/sales_log_spec.rb | 29 +-- .../duplicate_logs_controller_spec.rb | 173 ++++++++++++------ spec/requests/form_controller_spec.rb | 82 +++++++++ spec/requests/sales_logs_controller_spec.rb | 8 +- 16 files changed, 365 insertions(+), 119 deletions(-) diff --git a/app/controllers/duplicate_logs_controller.rb b/app/controllers/duplicate_logs_controller.rb index b084a8124..e5ae95bbf 100644 --- a/app/controllers/duplicate_logs_controller.rb +++ b/app/controllers/duplicate_logs_controller.rb @@ -2,7 +2,7 @@ class DuplicateLogsController < ApplicationController before_action :authenticate_user! before_action :find_resource_by_named_id before_action :find_duplicate_logs - before_action :find_original_log_id + before_action :find_original_log def show if @log @@ -61,8 +61,13 @@ private end end - def find_original_log_id + def find_original_log query_params = URI.parse(request.url).query - @original_log_id = CGI.parse(query_params)["original_log_id"][0]&.to_i if query_params.present? + original_log_id = CGI.parse(query_params)["original_log_id"][0]&.to_i if query_params.present? + @original_log = if params[:sales_log_id].present? + current_user.sales_logs.find_by(id: original_log_id) + else + current_user.lettings_logs.find_by(id: original_log_id) + end end end diff --git a/app/controllers/form_controller.rb b/app/controllers/form_controller.rb index b7ab5de29..3df834add 100644 --- a/app/controllers/form_controller.rb +++ b/app/controllers/form_controller.rb @@ -48,7 +48,7 @@ class FormController < ApplicationController def show_page if request.params["referrer"] == "interruption_screen" && request.headers["HTTP_REFERER"].present? @interruption_page_id = URI.parse(request.headers["HTTP_REFERER"]).path.split("/").last.underscore - @interruption_page_referrer_type = referrer_from_query + @interruption_page_referrer_type = from_referrer_query("referrer") end if @log @@ -130,10 +130,10 @@ private end def is_referrer_type?(referrer_type) - referrer_from_query == referrer_type + from_referrer_query("referrer") == referrer_type end - def referrer_from_query + def from_referrer_query(query_param) referrer = request.headers["HTTP_REFERER"] return unless referrer @@ -141,7 +141,7 @@ private return unless query_params parsed_params = CGI.parse(query_params) - parsed_params["referrer"]&.first + parsed_params[query_param]&.first end def original_duplicate_log_id_from_query @@ -163,11 +163,15 @@ private def successful_redirect_path if FeatureToggle.deduplication_flow_enabled? - if @log.lettings? - if current_user.lettings_logs.duplicate_logs(@log).count.positive? - return send("lettings_log_duplicate_logs_path", @log, original_log_id: @log.id) - end - elsif current_user.sales_logs.duplicate_logs(@log).count.positive? + if is_referrer_type?("duplicate_logs") + return correcting_duplicate_logs_redirect_path + end + + if @log.lettings? && current_user.lettings_logs.duplicate_logs(@log).count.positive? + return send("lettings_log_duplicate_logs_path", @log, original_log_id: @log.id) + end + + if @log.sales? && current_user.sales_logs.duplicate_logs(@log).count.positive? return send("sales_log_duplicate_logs_path", @log, original_log_id: @log.id) end end @@ -237,4 +241,34 @@ private end CONFIRMATION_PAGE_IDS = %w[uprn_confirmation].freeze + + def correcting_duplicate_logs_redirect_path + class_name = @log.class.name.underscore + + original_log = current_user.send(class_name.pluralize).find_by(id: from_referrer_query("original_log_id")) + + if original_log.present? && current_user.send(class_name.pluralize).duplicate_logs(original_log).count.positive? + flash[:notice] = deduplication_success_banner unless current_user.send(class_name.pluralize).duplicate_logs(@log).count.positive? + send("#{class_name}_duplicate_logs_path", original_log, original_log_id: original_log.id) + else + flash[:notice] = deduplication_success_banner + send("#{class_name}_duplicate_logs_path", "#{class_name}_id".to_sym => from_referrer_query("first_remaining_duplicate_id"), original_log_id: from_referrer_query("original_log_id")) + end + end + + def deduplication_success_banner + deduplicated_log_link = "Log #{@log.id}" + changed_labels = { + property_postcode: "postcode", + lead_tenant_age: "lead tenant’s age", + rent_4_weekly: "household rent and charges", + rent_bi_weekly: "household rent and charges", + rent_monthly: "household rent and charges", + rent_or_other_charges: "household rent and charges", + address: "postcode", + } + changed_question_label = changed_labels[@page.id.to_sym] || (@page.questions.first.check_answer_label.to_s.presence || @page.questions.first.header.to_s).downcase + + I18n.t("notification.duplicate_logs.deduplication_success_banner", log_link: deduplicated_log_link, changed_question_label:).html_safe + end end diff --git a/app/helpers/duplicate_logs_helper.rb b/app/helpers/duplicate_logs_helper.rb index b73fc5cf3..f1efec21b 100644 --- a/app/helpers/duplicate_logs_helper.rb +++ b/app/helpers/duplicate_logs_helper.rb @@ -1,18 +1,18 @@ module DuplicateLogsHelper include GovukLinkHelper - def duplicate_logs_continue_button(all_duplicates, duplicate_log, original_log_id) + def duplicate_logs_continue_button(all_duplicates, duplicate_log, original_log) if all_duplicates.count > 1 return govuk_button_link_to "Keep this log and delete duplicates", url_for( controller: "duplicate_logs", action: "delete_duplicates", "#{duplicate_log.class.name.underscore}_id": duplicate_log.id, - original_log_id:, + original_log_id: original_log.id, ) end - if original_log_id == duplicate_log.id - govuk_button_link_to "Back to Log #{duplicate_log.id}", send("#{duplicate_log.class.name.underscore}_path", duplicate_log) + if !original_log.deleted? + govuk_button_link_to "Back to Log #{original_log.id}", send("#{original_log.class.name.underscore}_path", original_log) else type = duplicate_log.lettings? ? "lettings" : "sales" govuk_button_link_to "Back to #{type} logs", url_for(duplicate_log.class) @@ -22,4 +22,9 @@ module DuplicateLogsHelper def duplicate_logs_action_href(log, page_id, original_log_id) send("#{log.model_name.param_key}_#{page_id}_path", log, referrer: "interruption_screen", original_log_id:) end + + def change_duplicate_logs_action_href(log, page_id, all_duplicates, original_log_id) + first_remaining_duplicate_id = all_duplicates.map(&:id).reject { |id| id == log.id }.first + send("#{log.model_name.param_key}_#{page_id}_path", log, referrer: "duplicate_logs", first_remaining_duplicate_id:, original_log_id:) + end end diff --git a/app/models/lettings_log.rb b/app/models/lettings_log.rb index 2f11e31ff..6104ee3d5 100644 --- a/app/models/lettings_log.rb +++ b/app/models/lettings_log.rb @@ -79,7 +79,7 @@ class LettingsLog < Log NUM_OF_WEEKS_FROM_PERIOD = { 2 => 26, 3 => 13, 4 => 12, 5 => 50, 6 => 49, 7 => 48, 8 => 47, 9 => 46, 1 => 52, 10 => 53 }.freeze SUFFIX_FROM_PERIOD = { 2 => "every 2 weeks", 3 => "every 4 weeks", 4 => "every month" }.freeze RETIREMENT_AGES = { "M" => 67, "F" => 60, "X" => 67 }.freeze - DUPLICATE_LOG_ATTRIBUTES = %w[tenancycode startdate age1_known age1 sex1 ecstat1 tcharge household_charge chcharge].freeze + DUPLICATE_LOG_ATTRIBUTES = %w[owning_organisation_id tenancycode startdate age1_known age1 sex1 ecstat1 tcharge household_charge chcharge].freeze def form FormHandler.instance.get_form(form_name) || FormHandler.instance.current_lettings_form diff --git a/app/models/sales_log.rb b/app/models/sales_log.rb index 2a0a6d5c4..4e829d760 100644 --- a/app/models/sales_log.rb +++ b/app/models/sales_log.rb @@ -55,7 +55,7 @@ class SalesLog < Log OPTIONAL_FIELDS = %w[purchid othtype].freeze RETIREMENT_AGES = { "M" => 65, "F" => 60, "X" => 65 }.freeze - DUPLICATE_LOG_ATTRIBUTES = %w[purchid saledate age1_known age1 sex1 ecstat1 postcode_full].freeze + DUPLICATE_LOG_ATTRIBUTES = %w[owning_organisation_id purchid saledate age1_known age1 sex1 ecstat1 postcode_full].freeze def lettings? false diff --git a/app/views/duplicate_logs/_duplicate_log_check_answers.erb b/app/views/duplicate_logs/_duplicate_log_check_answers.erb index a5abaea16..ee350545b 100644 --- a/app/views/duplicate_logs/_duplicate_log_check_answers.erb +++ b/app/views/duplicate_logs/_duplicate_log_check_answers.erb @@ -7,14 +7,14 @@ <% row.value do %> <%= simple_format( - get_answer_label(question, @log), + get_answer_label(question, log), wrapper_tag: "span", class: "govuk-!-margin-right-4", ) %> - <% extra_value = question.get_extra_check_answer_value(@log) %> + <% extra_value = question.get_extra_check_answer_value(log) %> - <% if extra_value && question.answer_label(@log, current_user).present? %> + <% if extra_value && question.answer_label(log, current_user).present? %> <%= simple_format( extra_value, wrapper_tag: "span", @@ -22,16 +22,23 @@ ) %> <% end %> - <% question.get_inferred_answers(@log).each do |inferred_answer| %> + <% question.get_inferred_answers(log).each do |inferred_answer| %> <%= inferred_answer %> <% end %> <% end %> - - <% row.action( - text: question.action_text(@log), - href: duplicate_logs_action_href(@log, question.page.id, @original_log_id), - visually_hidden_text: question.check_answer_label.to_s.downcase, - ) %> + <% if @all_duplicates.many? %> + <% row.action( + text: question.action_text(log), + href: change_duplicate_logs_action_href(log, question.page.id, @all_duplicates, @original_log.id), + visually_hidden_text: question.check_answer_label.to_s.downcase, + ) %> + <% else %> + <% row.action( + text: question.action_text(log), + href: duplicate_logs_action_href(log, question.page.id, @original_log.id), + visually_hidden_text: question.check_answer_label.to_s.downcase, + ) %> + <% end %> <% end %> <% end %> <% end %> diff --git a/app/views/duplicate_logs/show.html.erb b/app/views/duplicate_logs/show.html.erb index 1a7dd8e86..1f225b938 100644 --- a/app/views/duplicate_logs/show.html.erb +++ b/app/views/duplicate_logs/show.html.erb @@ -1,7 +1,7 @@ <% content_for :title, "Check duplicate logs" %>
- <% if @all_duplicates.count > 1 %> + <% if @all_duplicates.many? %> <%= govuk_panel( classes: "app-panel--interruption", ) do %> @@ -18,7 +18,7 @@ <% @all_duplicates.each_with_index do |log, index| %> <%= render partial: "duplicate_log", locals: { log: } %> <%= render partial: "duplicate_log_check_answers", locals: { log: } %> - <%= duplicate_logs_continue_button(@all_duplicates, log, @original_log_id) %> + <%= duplicate_logs_continue_button(@all_duplicates, log, @original_log) %> <% if index < @all_duplicates.count - 1 %>
<% end %> diff --git a/app/views/logs/delete_duplicates.html.erb b/app/views/logs/delete_duplicates.html.erb index 5ba4459e2..18b3d56e4 100644 --- a/app/views/logs/delete_duplicates.html.erb +++ b/app/views/logs/delete_duplicates.html.erb @@ -29,7 +29,7 @@ <%= govuk_button_to @duplicate_logs.count == 1 ? "Delete this log" : "Delete these logs", send("delete_logs_#{@log.class.name.underscore}s_path"), method: "delete", - params: { ids: @duplicate_logs.map(&:id), original_log_id: @original_log_id, remaining_log_id: @log.id } %> + params: { ids: @duplicate_logs.map(&:id), original_log_id: @original_log.id, remaining_log_id: @log.id } %> <%= govuk_button_link_to( "Cancel", send("#{@log.class.name.underscore}_duplicate_logs_path", @log), diff --git a/config/locales/en.yml b/config/locales/en.yml index 182a91b26..f99d08c33 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -187,6 +187,8 @@ en: duplicate_logs_deleted: one: "%{log_ids} has been deleted." other: "%{log_ids} have been deleted." + duplicate_logs: + deduplication_success_banner: "%{log_link} is no longer a duplicate and has been removed from the list.

You changed the %{changed_question_label}.

" validations: organisation: diff --git a/spec/features/lettings_log_spec.rb b/spec/features/lettings_log_spec.rb index f849decc1..9f8cfbb77 100644 --- a/spec/features/lettings_log_spec.rb +++ b/spec/features/lettings_log_spec.rb @@ -505,6 +505,29 @@ RSpec.describe "Lettings Log Features" do expect(page).to have_current_path("/lettings-logs/#{duplicate_log.id}/duplicate-logs?original_log_id=#{lettings_log.id}") expect(page).to have_link("Back to lettings logs", href: "/lettings-logs") end + + it "allows deduplicating logs by changing the answers on the duplicate log" do + expect(page).to have_current_path("/lettings-logs/#{lettings_log.id}/duplicate-logs?original_log_id=#{lettings_log.id}") + click_link("Change", href: "/lettings-logs/#{duplicate_log.id}/tenant-code?first_remaining_duplicate_id=#{lettings_log.id}&original_log_id=#{lettings_log.id}&referrer=duplicate_logs") + fill_in("lettings-log-tenancycode-field", with: "something else") + click_button("Save and continue") + expect(page).to have_current_path("/lettings-logs/#{lettings_log.id}/duplicate-logs?original_log_id=#{lettings_log.id}") + expect(page).to have_link("Back to Log #{lettings_log.id}", href: "/lettings-logs/#{lettings_log.id}") + expect(page).to have_css(".govuk-notification-banner.govuk-notification-banner--success") + expect(page).to have_content("Log #{duplicate_log.id} is no longer a duplicate and has been removed from the list") + expect(page).to have_content("You changed the tenant code.") + end + + it "allows deduplicating logs by changing the answers on the original log" do + click_link("Change", href: "/lettings-logs/#{lettings_log.id}/tenant-code?first_remaining_duplicate_id=#{duplicate_log.id}&original_log_id=#{lettings_log.id}&referrer=duplicate_logs") + fill_in("lettings-log-tenancycode-field", with: "something else") + click_button("Save and continue") + expect(page).to have_current_path("/lettings-logs/#{duplicate_log.id}/duplicate-logs?original_log_id=#{lettings_log.id}") + expect(page).to have_link("Back to Log #{lettings_log.id}", href: "/lettings-logs/#{lettings_log.id}") + expect(page).to have_css(".govuk-notification-banner.govuk-notification-banner--success") + expect(page).to have_content("Log #{lettings_log.id} is no longer a duplicate and has been removed from the list") + expect(page).to have_content("You changed the tenant code.") + end end end end diff --git a/spec/features/sales_log_spec.rb b/spec/features/sales_log_spec.rb index 023353fef..f03287723 100644 --- a/spec/features/sales_log_spec.rb +++ b/spec/features/sales_log_spec.rb @@ -244,5 +244,28 @@ RSpec.describe "Sales Log Features" do expect(page).to have_current_path("/sales-logs/#{duplicate_log.id}/duplicate-logs?original_log_id=#{sales_log.id}") expect(page).to have_link("Back to sales logs", href: "/sales-logs") end + + it "allows deduplicating logs by changing the answers on the duplicate log" do + expect(page).to have_current_path("/sales-logs/#{sales_log.id}/duplicate-logs?original_log_id=#{sales_log.id}") + click_link("Change", href: "/sales-logs/#{duplicate_log.id}/purchaser-code?first_remaining_duplicate_id=#{sales_log.id}&original_log_id=#{sales_log.id}&referrer=duplicate_logs") + fill_in("sales-log-purchid-field", with: "something else") + click_button("Save and continue") + expect(page).to have_current_path("/sales-logs/#{sales_log.id}/duplicate-logs?original_log_id=#{sales_log.id}") + expect(page).to have_link("Back to Log #{sales_log.id}", href: "/sales-logs/#{sales_log.id}") + expect(page).to have_css(".govuk-notification-banner.govuk-notification-banner--success") + expect(page).to have_content("Log #{duplicate_log.id} is no longer a duplicate and has been removed from the list") + expect(page).to have_content("You changed the purchaser code.") + end + + it "allows deduplicating logs by changing the answers on the original log" do + click_link("Change", href: "/sales-logs/#{sales_log.id}/purchaser-code?first_remaining_duplicate_id=#{duplicate_log.id}&original_log_id=#{sales_log.id}&referrer=duplicate_logs") + fill_in("sales-log-purchid-field", with: "something else") + click_button("Save and continue") + expect(page).to have_current_path("/sales-logs/#{duplicate_log.id}/duplicate-logs?original_log_id=#{sales_log.id}") + expect(page).to have_link("Back to Log #{sales_log.id}", href: "/sales-logs/#{sales_log.id}") + expect(page).to have_css(".govuk-notification-banner.govuk-notification-banner--success") + expect(page).to have_content("Log #{sales_log.id} is no longer a duplicate and has been removed from the list") + expect(page).to have_content("You changed the purchaser code.") + end end end diff --git a/spec/models/lettings_log_spec.rb b/spec/models/lettings_log_spec.rb index 1ae394c3d..489a31b9b 100644 --- a/spec/models/lettings_log_spec.rb +++ b/spec/models/lettings_log_spec.rb @@ -2836,8 +2836,9 @@ RSpec.describe LettingsLog do end context "when filtering duplicate logs" do - let(:log) { create(:lettings_log, :duplicate) } - let!(:duplicate_log) { create(:lettings_log, :duplicate) } + let(:organisation) { create(:organisation) } + let(:log) { create(:lettings_log, :duplicate, owning_organisation: organisation) } + let!(:duplicate_log) { create(:lettings_log, :duplicate, owning_organisation: organisation) } it "returns all duplicate logs for given log" do expect(described_class.duplicate_logs(log).count).to eq(1) @@ -2852,7 +2853,7 @@ RSpec.describe LettingsLog do end context "when there is a deleted duplicate log" do - let!(:deleted_duplicate_log) { create(:lettings_log, :duplicate, discarded_at: Time.zone.now) } + let!(:deleted_duplicate_log) { create(:lettings_log, :duplicate, discarded_at: Time.zone.now, owning_organisation: organisation) } it "does not return the deleted log as a duplicate" do expect(described_class.duplicate_logs(log)).not_to include(deleted_duplicate_log) @@ -2860,7 +2861,7 @@ RSpec.describe LettingsLog do end context "when there is a log with a different start date" do - let!(:different_start_date_log) { create(:lettings_log, :duplicate, startdate: Time.zone.tomorrow) } + let!(:different_start_date_log) { create(:lettings_log, :duplicate, startdate: Time.zone.tomorrow, owning_organisation: organisation) } it "does not return a log with a different start date as a duplicate" do expect(described_class.duplicate_logs(log)).not_to include(different_start_date_log) @@ -2868,7 +2869,7 @@ RSpec.describe LettingsLog do end context "when there is a log with a different age1" do - let!(:different_age1) { create(:lettings_log, :duplicate, age1: 50) } + let!(:different_age1) { create(:lettings_log, :duplicate, age1: 50, owning_organisation: organisation) } it "does not return a log with a different age1 as a duplicate" do expect(described_class.duplicate_logs(log)).not_to include(different_age1) @@ -2876,7 +2877,7 @@ RSpec.describe LettingsLog do end context "when there is a log with a different sex1" do - let!(:different_sex1) { create(:lettings_log, :duplicate, sex1: "F") } + let!(:different_sex1) { create(:lettings_log, :duplicate, sex1: "F", owning_organisation: organisation) } it "does not return a log with a different sex1 as a duplicate" do expect(described_class.duplicate_logs(log)).not_to include(different_sex1) @@ -2884,7 +2885,7 @@ RSpec.describe LettingsLog do end context "when there is a log with a different ecstat1" do - let!(:different_ecstat1) { create(:lettings_log, :duplicate, ecstat1: 1) } + let!(:different_ecstat1) { create(:lettings_log, :duplicate, ecstat1: 1, owning_organisation: organisation) } it "does not return a log with a different ecstat1 as a duplicate" do expect(described_class.duplicate_logs(log)).not_to include(different_ecstat1) @@ -2892,7 +2893,7 @@ RSpec.describe LettingsLog do end context "when there is a log with a different tcharge" do - let!(:different_tcharge) { create(:lettings_log, :duplicate, brent: 100) } + let!(:different_tcharge) { create(:lettings_log, :duplicate, brent: 100, owning_organisation: organisation) } it "does not return a log with a different tcharge as a duplicate" do expect(described_class.duplicate_logs(log)).not_to include(different_tcharge) @@ -2900,7 +2901,7 @@ RSpec.describe LettingsLog do end context "when there is a log with a different tenancycode" do - let!(:different_tenancycode) { create(:lettings_log, :duplicate, tenancycode: "different") } + let!(:different_tenancycode) { create(:lettings_log, :duplicate, tenancycode: "different", owning_organisation: organisation) } it "does not return a log with a different tenancycode as a duplicate" do expect(described_class.duplicate_logs(log)).not_to include(different_tenancycode) @@ -2908,7 +2909,7 @@ RSpec.describe LettingsLog do end context "when there is a log with a different postcode_full" do - let!(:different_postcode_full) { create(:lettings_log, :duplicate, postcode_full: "B1 1AA") } + let!(:different_postcode_full) { create(:lettings_log, :duplicate, postcode_full: "B1 1AA", owning_organisation: organisation) } it "does not return a log with a different postcode_full as a duplicate" do expect(described_class.duplicate_logs(log)).not_to include(different_postcode_full) @@ -2916,7 +2917,7 @@ RSpec.describe LettingsLog do end context "when there is a log with nil values for duplicate check fields" do - let!(:duplicate_check_fields_not_given) { create(:lettings_log, :duplicate, age1: nil, sex1: nil, ecstat1: nil, postcode_known: 2, postcode_full: nil) } + let!(:duplicate_check_fields_not_given) { create(:lettings_log, :duplicate, age1: nil, sex1: nil, ecstat1: nil, postcode_known: 2, postcode_full: nil, owning_organisation: organisation) } it "does not return a log with nil values as a duplicate" do log.update!(age1: nil, sex1: nil, ecstat1: nil, postcode_known: 2, postcode_full: nil) @@ -2925,7 +2926,7 @@ RSpec.describe LettingsLog do end context "when there is a log with nil values for tenancycode" do - let!(:tenancycode_not_given) { create(:lettings_log, :duplicate, tenancycode: nil) } + let!(:tenancycode_not_given) { create(:lettings_log, :duplicate, tenancycode: nil, owning_organisation: organisation) } it "returns the log as a duplicate if tenancy code is nil" do log.update!(tenancycode: nil) @@ -2934,7 +2935,7 @@ RSpec.describe LettingsLog do end context "when there is a log with age1 not known" do - let!(:age1_not_known) { create(:lettings_log, :duplicate, age1_known: 1, age1: nil) } + let!(:age1_not_known) { create(:lettings_log, :duplicate, age1_known: 1, age1: nil, owning_organisation: organisation) } it "returns the log as a duplicate if age1 is not known" do log.update!(age1_known: 1, age1: nil) @@ -2946,8 +2947,8 @@ RSpec.describe LettingsLog do let(:scheme) { create(:scheme) } let(:location) { create(:location, scheme:) } let(:location_2) { create(:location, scheme:) } - let(:supported_housing_log) { create(:lettings_log, :duplicate, needstype: 2, location:, scheme:) } - let!(:duplicate_supported_housing_log) { create(:lettings_log, :duplicate, needstype: 2, location:, scheme:) } + let(:supported_housing_log) { create(:lettings_log, :duplicate, needstype: 2, location:, scheme:, owning_organisation: organisation) } + let!(:duplicate_supported_housing_log) { create(:lettings_log, :duplicate, needstype: 2, location:, scheme:, owning_organisation: organisation) } it "returns the log as a duplicate" do expect(described_class.duplicate_logs(supported_housing_log)).to include(duplicate_supported_housing_log) diff --git a/spec/models/sales_log_spec.rb b/spec/models/sales_log_spec.rb index d032293c8..1aa27cbd3 100644 --- a/spec/models/sales_log_spec.rb +++ b/spec/models/sales_log_spec.rb @@ -161,8 +161,9 @@ RSpec.describe SalesLog, type: :model do end context "when filtering duplicate logs" do - let(:log) { create(:sales_log, :duplicate) } - let!(:duplicate_log) { create(:sales_log, :duplicate) } + let(:organisation) { create(:organisation) } + let(:log) { create(:sales_log, :duplicate, owning_organisation: organisation) } + let!(:duplicate_log) { create(:sales_log, :duplicate, owning_organisation: organisation) } it "returns all duplicate logs for given log" do expect(described_class.duplicate_logs(log).count).to eq(1) @@ -177,7 +178,7 @@ RSpec.describe SalesLog, type: :model do end context "when there is a deleted duplicate log" do - let!(:deleted_duplicate_log) { create(:sales_log, :duplicate, discarded_at: Time.zone.now) } + let!(:deleted_duplicate_log) { create(:sales_log, :duplicate, discarded_at: Time.zone.now, owning_organisation: organisation) } it "does not return the deleted log as a duplicate" do expect(described_class.duplicate_logs(log)).not_to include(deleted_duplicate_log) @@ -185,7 +186,7 @@ RSpec.describe SalesLog, type: :model do end context "when there is a log with a different sale date" do - let!(:different_sale_date_log) { create(:sales_log, :duplicate, saledate: Time.zone.tomorrow) } + let!(:different_sale_date_log) { create(:sales_log, :duplicate, saledate: Time.zone.tomorrow, owning_organisation: organisation) } it "does not return a log with a different sale date as a duplicate" do expect(described_class.duplicate_logs(log)).not_to include(different_sale_date_log) @@ -193,7 +194,7 @@ RSpec.describe SalesLog, type: :model do end context "when there is a log with a different age1" do - let!(:different_age1) { create(:sales_log, :duplicate, age1: 50) } + let!(:different_age1) { create(:sales_log, :duplicate, age1: 50, owning_organisation: organisation) } it "does not return a log with a different age1 as a duplicate" do expect(described_class.duplicate_logs(log)).not_to include(different_age1) @@ -201,7 +202,7 @@ RSpec.describe SalesLog, type: :model do end context "when there is a log with a different sex1" do - let!(:different_sex1) { create(:sales_log, :duplicate, sex1: "M") } + let!(:different_sex1) { create(:sales_log, :duplicate, sex1: "M", owning_organisation: organisation) } it "does not return a log with a different sex1 as a duplicate" do expect(described_class.duplicate_logs(log)).not_to include(different_sex1) @@ -209,7 +210,7 @@ RSpec.describe SalesLog, type: :model do end context "when there is a log with a different ecstat1" do - let!(:different_ecstat1) { create(:sales_log, :duplicate, ecstat1: 0) } + let!(:different_ecstat1) { create(:sales_log, :duplicate, ecstat1: 0, owning_organisation: organisation) } it "does not return a log with a different ecstat1 as a duplicate" do expect(described_class.duplicate_logs(log)).not_to include(different_ecstat1) @@ -217,7 +218,7 @@ RSpec.describe SalesLog, type: :model do end context "when there is a log with a different purchid" do - let!(:different_purchid) { create(:sales_log, :duplicate, purchid: "different") } + let!(:different_purchid) { create(:sales_log, :duplicate, purchid: "different", owning_organisation: organisation) } it "does not return a log with a different purchid as a duplicate" do expect(described_class.duplicate_logs(log)).not_to include(different_purchid) @@ -225,7 +226,7 @@ RSpec.describe SalesLog, type: :model do end context "when there is a log with a different postcode_full" do - let!(:different_postcode_full) { create(:sales_log, :duplicate, postcode_full: "B1 1AA") } + let!(:different_postcode_full) { create(:sales_log, :duplicate, postcode_full: "B1 1AA", owning_organisation: organisation) } it "does not return a log with a different postcode_full as a duplicate" do expect(described_class.duplicate_logs(log)).not_to include(different_postcode_full) @@ -233,7 +234,7 @@ RSpec.describe SalesLog, type: :model do end context "when there is a log with nil values for duplicate check fields" do - let!(:duplicate_check_fields_not_given) { create(:sales_log, :duplicate, age1: nil, sex1: nil, ecstat1: nil, pcodenk: 1, postcode_full: nil) } + let!(:duplicate_check_fields_not_given) { create(:sales_log, :duplicate, age1: nil, sex1: nil, ecstat1: nil, pcodenk: 1, postcode_full: nil, owning_organisation: organisation) } it "does not return a log with nil values as a duplicate" do log.update!(age1: nil, sex1: nil, ecstat1: nil, pcodenk: 1, postcode_full: nil) @@ -242,7 +243,7 @@ RSpec.describe SalesLog, type: :model do end context "when there is a log with nil values for purchid" do - let!(:purchid_not_given) { create(:sales_log, :duplicate, purchid: nil) } + let!(:purchid_not_given) { create(:sales_log, :duplicate, purchid: nil, owning_organisation: organisation) } it "returns the log as a duplicate if purchid is nil" do log.update!(purchid: nil) @@ -251,7 +252,7 @@ RSpec.describe SalesLog, type: :model do end context "when there is a log age not known" do - let!(:age1_not_known) { create(:sales_log, :duplicate, age1_known: 1, age1: nil) } + let!(:age1_not_known) { create(:sales_log, :duplicate, age1_known: 1, age1: nil, owning_organisation: organisation) } it "returns the log as a duplicate if age is not known" do log.update!(age1_known: 1, age1: nil) @@ -260,7 +261,7 @@ RSpec.describe SalesLog, type: :model do end context "when there is a log age pefers not to say" do - let!(:age1_prefers_not_to_say) { create(:sales_log, :duplicate, age1_known: 2, age1: nil) } + let!(:age1_prefers_not_to_say) { create(:sales_log, :duplicate, age1_known: 2, age1: nil, owning_organisation: organisation) } it "returns the log as a duplicate if age is prefers not to say" do log.update!(age1_known: 2, age1: nil) @@ -269,7 +270,7 @@ RSpec.describe SalesLog, type: :model do end context "when there is a log age pefers not to say and not known" do - let!(:age1_prefers_not_to_say) { create(:sales_log, :duplicate, age1_known: 2, age1: nil) } + let!(:age1_prefers_not_to_say) { create(:sales_log, :duplicate, age1_known: 2, age1: nil, owning_organisation: organisation) } it "does not return the log as a duplicate if age is prefers not to say" do log.update!(age1_known: 1, age1: nil) diff --git a/spec/requests/duplicate_logs_controller_spec.rb b/spec/requests/duplicate_logs_controller_spec.rb index 1de8af9ae..096b5ed61 100644 --- a/spec/requests/duplicate_logs_controller_spec.rb +++ b/spec/requests/duplicate_logs_controller_spec.rb @@ -34,68 +34,131 @@ RSpec.describe DuplicateLogsController, type: :request do sign_in user end - context "with multiple duplicate lettings logs" do - let(:duplicate_logs) { create_list(:lettings_log, 2, :completed) } - - before do - allow(LettingsLog).to receive(:duplicate_logs).and_return(duplicate_logs) - get "/lettings-logs/#{lettings_log.id}/duplicate-logs?original_log_id=#{lettings_log.id}" - end - - it "displays links to all the duplicate logs" do - expect(page).to have_link("Log #{lettings_log.id}", href: "/lettings-logs/#{lettings_log.id}") - expect(page).to have_link("Log #{duplicate_logs.first.id}", href: "/lettings-logs/#{duplicate_logs.first.id}") - expect(page).to have_link("Log #{duplicate_logs.second.id}", href: "/lettings-logs/#{duplicate_logs.second.id}") - end - - it "displays check your answers for each log with correct questions" do - expect(page).to have_content("Q5 - Tenancy start date", count: 3) - expect(page).to have_content("Q7 - Tenant code", count: 3) - expect(page).to have_content("Q12 - Postcode", count: 3) - expect(page).to have_content("Q32 - Lead tenant’s age", count: 3) - expect(page).to have_content("Q33 - Lead tenant’s gender identity", count: 3) - expect(page).to have_content("Q37 - Lead tenant’s working situation", count: 3) - expect(page).to have_content("Household rent and charges", count: 3) - expect(page).to have_link("Change", count: 21) + context "when viewing lettings logs duplicates" do + context "when there are multiple duplicate logs" do + let(:duplicate_logs) { create_list(:lettings_log, 2, :completed) } + + before do + allow(LettingsLog).to receive(:duplicate_logs).and_return(duplicate_logs) + get "/lettings-logs/#{lettings_log.id}/duplicate-logs?original_log_id=#{lettings_log.id}" + end + + it "displays links to all the duplicate logs" do + expect(page).to have_link("Log #{lettings_log.id}", href: "/lettings-logs/#{lettings_log.id}") + expect(page).to have_link("Log #{duplicate_logs.first.id}", href: "/lettings-logs/#{duplicate_logs.first.id}") + expect(page).to have_link("Log #{duplicate_logs.second.id}", href: "/lettings-logs/#{duplicate_logs.second.id}") + end + + it "displays check your answers for each log with correct questions" do + expect(page).to have_content("Q5 - Tenancy start date", count: 3) + expect(page).to have_content("Q7 - Tenant code", count: 3) + expect(page).to have_content("Q12 - Postcode", count: 3) + expect(page).to have_content("Q32 - Lead tenant’s age", count: 3) + expect(page).to have_content("Q33 - Lead tenant’s gender identity", count: 3) + expect(page).to have_content("Q37 - Lead tenant’s working situation", count: 3) + expect(page).to have_content("Household rent and charges", count: 3) + expect(page).to have_link("Change", count: 21) + expect(page).to have_link("Change", href: "/lettings-logs/#{lettings_log.id}/tenant-code?first_remaining_duplicate_id=#{duplicate_logs[0].id}&original_log_id=#{lettings_log.id}&referrer=duplicate_logs") + expect(page).to have_link("Change", href: "/lettings-logs/#{duplicate_logs[0].id}/tenant-code?first_remaining_duplicate_id=#{lettings_log.id}&original_log_id=#{lettings_log.id}&referrer=duplicate_logs") + expect(page).to have_link("Change", href: "/lettings-logs/#{duplicate_logs[1].id}/tenant-code?first_remaining_duplicate_id=#{lettings_log.id}&original_log_id=#{lettings_log.id}&referrer=duplicate_logs") + end + + it "displays buttons to delete" do + expect(page).to have_link("Keep this log and delete duplicates", count: 3) + expect(page).to have_link("Keep this log and delete duplicates", href: "/lettings-logs/#{lettings_log.id}/delete-duplicates?original_log_id=#{lettings_log.id}") + expect(page).to have_link("Keep this log and delete duplicates", href: "/lettings-logs/#{duplicate_logs.first.id}/delete-duplicates?original_log_id=#{lettings_log.id}") + expect(page).to have_link("Keep this log and delete duplicates", href: "/lettings-logs/#{duplicate_logs.second.id}/delete-duplicates?original_log_id=#{lettings_log.id}") + end end - it "displays buttons to delete" do - expect(page).to have_link("Keep this log and delete duplicates", count: 3) - expect(page).to have_link("Keep this log and delete duplicates", href: "/lettings-logs/#{lettings_log.id}/delete-duplicates?original_log_id=#{lettings_log.id}") - expect(page).to have_link("Keep this log and delete duplicates", href: "/lettings-logs/#{duplicate_logs.first.id}/delete-duplicates?original_log_id=#{lettings_log.id}") - expect(page).to have_link("Keep this log and delete duplicates", href: "/lettings-logs/#{duplicate_logs.second.id}/delete-duplicates?original_log_id=#{lettings_log.id}") + context "when there are no more duplicate logs" do + before do + allow(LettingsLog).to receive(:duplicate_logs).and_return(LettingsLog.none) + get "/lettings-logs/#{lettings_log.id}/duplicate-logs?original_log_id=#{lettings_log.id}" + end + + it "displays check your answers for each log with correct questions" do + expect(page).to have_content("Q5 - Tenancy start date", count: 1) + expect(page).to have_content("Q7 - Tenant code", count: 1) + expect(page).to have_content("Q12 - Postcode", count: 1) + expect(page).to have_content("Q32 - Lead tenant’s age", count: 1) + expect(page).to have_content("Q33 - Lead tenant’s gender identity", count: 1) + expect(page).to have_content("Q37 - Lead tenant’s working situation", count: 1) + expect(page).to have_content("Household rent and charges", count: 1) + expect(page).to have_link("Change", count: 7) + expect(page).to have_link("Change", href: "/lettings-logs/#{lettings_log.id}/tenant-code?original_log_id=#{lettings_log.id}&referrer=interruption_screen") + end + + it "displays buttons to return to log" do + expect(page).to have_link("Back to Log #{lettings_log.id}", href: "/lettings-logs/#{lettings_log.id}") + end + + it "displays no duplicates banner" do + expect(page).to have_content("This log had the same answers but it is no longer a duplicate. Make sure the answers are correct.") + end end end - context "with multiple duplicate sales logs" do - let(:duplicate_logs) { create_list(:sales_log, 2, :completed) } - - before do - allow(SalesLog).to receive(:duplicate_logs).and_return(duplicate_logs) - get "/sales-logs/#{sales_log.id}/duplicate-logs?original_log_id=#{sales_log.id}" - end - - it "displays links to all the duplicate logs" do - expect(page).to have_link("Log #{sales_log.id}", href: "/sales-logs/#{sales_log.id}") - expect(page).to have_link("Log #{duplicate_logs.first.id}", href: "/sales-logs/#{duplicate_logs.first.id}") - expect(page).to have_link("Log #{duplicate_logs.second.id}", href: "/sales-logs/#{duplicate_logs.second.id}") - end - - it "displays check your answers for each log with correct questions" do - expect(page).to have_content("Q1 - Sale completion date", count: 3) - expect(page).to have_content("Q2 - Purchaser code", count: 3) - expect(page).to have_content("Q20 - Lead buyer’s age", count: 3) - expect(page).to have_content("Q21 - Buyer 1’s gender identity", count: 3) - expect(page).to have_content("Q25 - Buyer 1's working situation", count: 3) - expect(page).to have_content("Q15 - Postcode", count: 3) - expect(page).to have_link("Change", count: 18) + context "when viewing sales logs duplicates" do + context "when there are multiple duplicate logs" do + let(:duplicate_logs) { create_list(:sales_log, 2, :completed) } + + before do + allow(SalesLog).to receive(:duplicate_logs).and_return(duplicate_logs) + get "/sales-logs/#{sales_log.id}/duplicate-logs?original_log_id=#{sales_log.id}" + end + + it "displays links to all the duplicate logs" do + expect(page).to have_link("Log #{sales_log.id}", href: "/sales-logs/#{sales_log.id}") + expect(page).to have_link("Log #{duplicate_logs.first.id}", href: "/sales-logs/#{duplicate_logs.first.id}") + expect(page).to have_link("Log #{duplicate_logs.second.id}", href: "/sales-logs/#{duplicate_logs.second.id}") + end + + it "displays check your answers for each log with correct questions" do + expect(page).to have_content("Q1 - Sale completion date", count: 3) + expect(page).to have_content("Q2 - Purchaser code", count: 3) + expect(page).to have_content("Q20 - Lead buyer’s age", count: 3) + expect(page).to have_content("Q21 - Buyer 1’s gender identity", count: 3) + expect(page).to have_content("Q25 - Buyer 1's working situation", count: 3) + expect(page).to have_content("Q15 - Postcode", count: 3) + expect(page).to have_link("Change", count: 18) + expect(page).to have_link("Change", href: "/sales-logs/#{sales_log.id}/purchaser-code?first_remaining_duplicate_id=#{duplicate_logs[0].id}&original_log_id=#{sales_log.id}&referrer=duplicate_logs") + expect(page).to have_link("Change", href: "/sales-logs/#{duplicate_logs[0].id}/purchaser-code?first_remaining_duplicate_id=#{sales_log.id}&original_log_id=#{sales_log.id}&referrer=duplicate_logs") + expect(page).to have_link("Change", href: "/sales-logs/#{duplicate_logs[1].id}/purchaser-code?first_remaining_duplicate_id=#{sales_log.id}&original_log_id=#{sales_log.id}&referrer=duplicate_logs") + end + + it "displays buttons to delete" do + expect(page).to have_link("Keep this log and delete duplicates", count: 3) + expect(page).to have_link("Keep this log and delete duplicates", href: "/sales-logs/#{sales_log.id}/delete-duplicates?original_log_id=#{sales_log.id}") + expect(page).to have_link("Keep this log and delete duplicates", href: "/sales-logs/#{duplicate_logs.first.id}/delete-duplicates?original_log_id=#{sales_log.id}") + expect(page).to have_link("Keep this log and delete duplicates", href: "/sales-logs/#{duplicate_logs.second.id}/delete-duplicates?original_log_id=#{sales_log.id}") + end end - it "displays buttons to delete" do - expect(page).to have_link("Keep this log and delete duplicates", count: 3) - expect(page).to have_link("Keep this log and delete duplicates", href: "/sales-logs/#{sales_log.id}/delete-duplicates?original_log_id=#{sales_log.id}") - expect(page).to have_link("Keep this log and delete duplicates", href: "/sales-logs/#{duplicate_logs.first.id}/delete-duplicates?original_log_id=#{sales_log.id}") - expect(page).to have_link("Keep this log and delete duplicates", href: "/sales-logs/#{duplicate_logs.second.id}/delete-duplicates?original_log_id=#{sales_log.id}") + context "when there are no more duplicate logs" do + before do + allow(SalesLog).to receive(:duplicate_logs).and_return(SalesLog.none) + get "/sales-logs/#{sales_log.id}/duplicate-logs?original_log_id=#{sales_log.id}" + end + + it "displays check your answers for each log with correct questions" do + expect(page).to have_content("Q1 - Sale completion date", count: 1) + expect(page).to have_content("Q2 - Purchaser code", count: 1) + expect(page).to have_content("Q20 - Lead buyer’s age", count: 1) + expect(page).to have_content("Q21 - Buyer 1’s gender identity", count: 1) + expect(page).to have_content("Q25 - Buyer 1's working situation", count: 1) + expect(page).to have_content("Q15 - Postcode", count: 1) + expect(page).to have_link("Change", count: 6) + expect(page).to have_link("Change", href: "/sales-logs/#{sales_log.id}/purchaser-code?original_log_id=#{sales_log.id}&referrer=interruption_screen") + end + + it "displays buttons to return to log" do + expect(page).to have_link("Back to Log #{sales_log.id}", href: "/sales-logs/#{sales_log.id}") + end + + it "displays no duplicates banner" do + expect(page).to have_content("This log had the same answers but it is no longer a duplicate. Make sure the answers are correct.") + end end end end diff --git a/spec/requests/form_controller_spec.rb b/spec/requests/form_controller_spec.rb index 8f16efcca..b0631a4f2 100644 --- a/spec/requests/form_controller_spec.rb +++ b/spec/requests/form_controller_spec.rb @@ -732,6 +732,88 @@ RSpec.describe FormController, type: :request do end end end + + context "when the question was accessed from a duplicate logs screen" do + let(:lettings_log) { create(:lettings_log, :duplicate, created_by: user) } + let(:duplicate_log) { create(:lettings_log, :duplicate, created_by: user) } + let(:referrer) { "/lettings-logs/#{lettings_log.id}/lead-tenant-age?referrer=duplicate_logs&first_remaining_duplicate_id=#{duplicate_log.id}&original_log_id=#{lettings_log.id}" } + let(:params) do + { + id: lettings_log.id, + lettings_log: { + page: "lead_tenant_age", + age1: 20, + age1_known: 1, + }, + } + end + + before do + post "/lettings-logs/#{lettings_log.id}/lead-tenant-age", params:, headers: headers.merge({ "HTTP_REFERER" => referrer }) + end + + it "redirects back to the duplicates page for remaining duplicates" do + expect(response).to redirect_to("/lettings-logs/#{duplicate_log.id}/duplicate-logs?original_log_id=#{lettings_log.id}") + end + + context "and the answer didn't change" do + let(:params) do + { + id: lettings_log.id, + lettings_log: { + page: "lead_tenant_age", + age1: lettings_log.age1, + age1_known: lettings_log.age1_known, + }, + } + end + + it "redirects back to the duplicates page for remaining duplicates" do + expect(response).to have_http_status(:ok) + end + end + end + + context "when the sales question was accessed from a duplicate logs screen" do + let(:sales_log) { create(:sales_log, :duplicate, created_by: user) } + let(:duplicate_log) { create(:sales_log, :duplicate, created_by: user) } + let(:referrer) { "/sales-logs/#{sales_log.id}/buyer-1-age?referrer=duplicate_logs&first_remaining_duplicate_id=#{duplicate_log.id}&original_log_id=#{sales_log.id}" } + let(:params) do + { + id: sales_log.id, + sales_log: { + page: "buyer_1_age", + age1: 20, + age1_known: 1, + }, + } + end + + before do + post "/sales-logs/#{sales_log.id}/buyer-1-age", params:, headers: headers.merge({ "HTTP_REFERER" => referrer }) + end + + it "redirects back to the duplicates page for remaining duplicates" do + expect(response).to redirect_to("/sales-logs/#{duplicate_log.id}/duplicate-logs?original_log_id=#{sales_log.id}") + end + + context "and the answer didn't change" do + let(:params) do + { + id: sales_log.id, + sales_log: { + page: "buyer_1_age", + age1: sales_log.age1, + age1_known: sales_log.age1_known, + }, + } + end + + it "redirects back to the duplicates page for remaining duplicates" do + expect(response).to have_http_status(:ok) + end + end + end end context "with checkbox questions" do diff --git a/spec/requests/sales_logs_controller_spec.rb b/spec/requests/sales_logs_controller_spec.rb index abe10af8e..0305690f8 100644 --- a/spec/requests/sales_logs_controller_spec.rb +++ b/spec/requests/sales_logs_controller_spec.rb @@ -354,8 +354,8 @@ RSpec.describe SalesLogsController, type: :request do let(:user) { create(:user, organisation:) } let(:bulk_upload) { create(:bulk_upload, :sales, user:) } - let!(:included_log) { create(:sales_log, :in_progress, bulk_upload:, owning_organisation: organisation) } - let!(:excluded_log) { create(:sales_log, :in_progress, owning_organisation: organisation) } + let!(:included_log) { create(:sales_log, :in_progress, purchid: "included_id", bulk_upload:, owning_organisation: organisation) } + let!(:excluded_log) { create(:sales_log, :in_progress, purchid: "excluded_id", owning_organisation: organisation) } before do create(:bulk_upload_error, bulk_upload:, col: "A", row: 1) @@ -364,8 +364,8 @@ RSpec.describe SalesLogsController, type: :request do it "returns logs only associated with the bulk upload" do get "/sales-logs?bulk_upload_id[]=#{bulk_upload.id}" - expect(page).to have_content(included_log.id) - expect(page).not_to have_content(excluded_log.id) + expect(page).to have_content(included_log.purchid) + expect(page).not_to have_content(excluded_log.purchid) end it "dislays bulk upload banner" do