From f5461f8d790055e391d30e1bb15b2d7c521d8ef2 Mon Sep 17 00:00:00 2001 From: Kat Date: Mon, 10 Jun 2024 19:55:52 +0100 Subject: [PATCH] Do not display deleted organisations as an option --- .../questions/managing_organisation.rb | 8 +++---- .../form/lettings/questions/stock_owner.rb | 8 +++---- .../sales/questions/managing_organisation.rb | 8 +++---- .../sales/questions/owning_organisation_id.rb | 22 +++++++---------- .../questions/managing_organisation_spec.rb | 9 ++++++- .../lettings/questions/stock_owner_spec.rb | 24 ++++++++++++++++++- .../questions/managing_organisation_spec.rb | 4 ++++ .../questions/owning_organisation_id_spec.rb | 9 ++++++- 8 files changed, 64 insertions(+), 28 deletions(-) diff --git a/app/models/form/lettings/questions/managing_organisation.rb b/app/models/form/lettings/questions/managing_organisation.rb index ea85ff207..090eee7df 100644 --- a/app/models/form/lettings/questions/managing_organisation.rb +++ b/app/models/form/lettings/questions/managing_organisation.rb @@ -31,14 +31,14 @@ class Form::Lettings::Questions::ManagingOrganisation < ::Form::Question end orgs = if user.support? - log.owning_organisation.managing_agents.filter_by_active + log.owning_organisation.managing_agents.visible.filter_by_active elsif user.organisation.absorbed_organisations.include?(log.owning_organisation) - user.organisation.managing_agents.filter_by_active + log.owning_organisation.managing_agents.filter_by_active + user.organisation.managing_agents.visible.filter_by_active + log.owning_organisation.managing_agents.visible.filter_by_active # here else - user.organisation.managing_agents.filter_by_active + user.organisation.managing_agents.visible.filter_by_active end - user.organisation.absorbed_organisations.each do |absorbed_org| + user.organisation.absorbed_organisations.visible.each do |absorbed_org| opts[absorbed_org.id] = "#{absorbed_org.name} (inactive as of #{absorbed_org.merge_date.to_fs(:govuk_date)})" end diff --git a/app/models/form/lettings/questions/stock_owner.rb b/app/models/form/lettings/questions/stock_owner.rb index 6ce0e5496..abdc91d56 100644 --- a/app/models/form/lettings/questions/stock_owner.rb +++ b/app/models/form/lettings/questions/stock_owner.rb @@ -30,7 +30,7 @@ class Form::Lettings::Questions::StockOwner < ::Form::Question end if user.support? - Organisation.filter_by_active.where(holds_own_stock: true).find_each do |org| + Organisation.visible.filter_by_active.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? && org.available_from.present? @@ -40,10 +40,10 @@ class Form::Lettings::Questions::StockOwner < ::Form::Question end end else - user.organisation.stock_owners.filter_by_active.each do |stock_owner| + user.organisation.stock_owners.visible.filter_by_active.each do |stock_owner| answer_opts[stock_owner.id] = stock_owner.name end - recently_absorbed_organisations.each do |absorbed_org| + recently_absorbed_organisations.visible.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 @@ -64,7 +64,7 @@ class Form::Lettings::Questions::StockOwner < ::Form::Question def hidden_in_check_answers?(_log, user = nil) return false if user.support? - stock_owners = user.organisation.stock_owners + user.organisation.absorbed_organisations.where(holds_own_stock: true) + stock_owners = user.organisation.stock_owners.visible + user.organisation.absorbed_organisations.visible.where(holds_own_stock: true) if user.organisation.holds_own_stock? stock_owners.count.zero? diff --git a/app/models/form/sales/questions/managing_organisation.rb b/app/models/form/sales/questions/managing_organisation.rb index 54d7ab6e0..9a86868cf 100644 --- a/app/models/form/sales/questions/managing_organisation.rb +++ b/app/models/form/sales/questions/managing_organisation.rb @@ -31,14 +31,14 @@ class Form::Sales::Questions::ManagingOrganisation < ::Form::Question end orgs = if user.support? - log.owning_organisation.managing_agents.filter_by_active + log.owning_organisation.managing_agents.visible.filter_by_active elsif user.organisation.absorbed_organisations.include?(log.owning_organisation) - user.organisation.managing_agents.filter_by_active + log.owning_organisation.managing_agents.filter_by_active + user.organisation.managing_agents.visible.filter_by_active + log.owning_organisation.managing_agents.visible.filter_by_active else - user.organisation.managing_agents.filter_by_active + user.organisation.managing_agents.visible.filter_by_active end.pluck(:id, :name).to_h - user.organisation.absorbed_organisations.each do |absorbed_org| + user.organisation.absorbed_organisations.visible.each do |absorbed_org| opts[absorbed_org.id] = "#{absorbed_org.name} (inactive as of #{absorbed_org.merge_date.to_fs(:govuk_date)})" end diff --git a/app/models/form/sales/questions/owning_organisation_id.rb b/app/models/form/sales/questions/owning_organisation_id.rb index dc3913882..cf417cb4a 100644 --- a/app/models/form/sales/questions/owning_organisation_id.rb +++ b/app/models/form/sales/questions/owning_organisation_id.rb @@ -16,25 +16,21 @@ class Form::Sales::Questions::OwningOrganisationId < ::Form::Question return answer_opts unless user return answer_opts unless log - unless user.support? - if log.owning_organisation_id.present? - answer_opts[log.owning_organisation.id] = log.owning_organisation.name - end + if log.owning_organisation_id.present? + answer_opts[log.owning_organisation.id] = log.owning_organisation.name + end + unless user.support? if user.organisation.holds_own_stock? answer_opts[user.organisation.id] = "#{user.organisation.name} (Your organisation)" end - user.organisation.stock_owners.filter_by_active.where(holds_own_stock: true).find_each do |org| + user.organisation.stock_owners.visible.filter_by_active.where(holds_own_stock: true).find_each do |org| answer_opts[org.id] = org.name end end - if log.owning_organisation_id.present? - answer_opts[log.owning_organisation.id] = log.owning_organisation.name - end - - recently_absorbed_organisations = user.organisation.absorbed_organisations.merged_during_open_collection_period + recently_absorbed_organisations = user.organisation.absorbed_organisations.visible.merged_during_open_collection_period if !user.support? && user.organisation.holds_own_stock? answer_opts[user.organisation.id] = if recently_absorbed_organisations.exists? && user.organisation.available_from.present? "#{user.organisation.name} (Your organisation, active as of #{user.organisation.available_from.to_fs(:govuk_date)})" @@ -44,7 +40,7 @@ class Form::Sales::Questions::OwningOrganisationId < ::Form::Question end if user.support? - Organisation.filter_by_active.where(holds_own_stock: true).find_each do |org| + Organisation.visible.filter_by_active.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? && org.available_from.present? @@ -54,7 +50,7 @@ class Form::Sales::Questions::OwningOrganisationId < ::Form::Question end end else - recently_absorbed_organisations.each do |absorbed_org| + recently_absorbed_organisations.visible.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 @@ -75,7 +71,7 @@ class Form::Sales::Questions::OwningOrganisationId < ::Form::Question def hidden_in_check_answers?(_log, user = nil) return false if user.support? - stock_owners = user.organisation.stock_owners + user.organisation.absorbed_organisations.where(holds_own_stock: true) + stock_owners = user.organisation.stock_owners.visible + user.organisation.absorbed_organisations.visible.where(holds_own_stock: true) if user.organisation.holds_own_stock? stock_owners.count.zero? diff --git a/spec/models/form/lettings/questions/managing_organisation_spec.rb b/spec/models/form/lettings/questions/managing_organisation_spec.rb index 70814b9d5..99319fc71 100644 --- a/spec/models/form/lettings/questions/managing_organisation_spec.rb +++ b/spec/models/form/lettings/questions/managing_organisation_spec.rb @@ -58,6 +58,7 @@ RSpec.describe Form::Lettings::Questions::ManagingOrganisation, type: :model do let(:managing_org2) { create(:organisation, name: "Managing org 2") } let(:managing_org3) { create(:organisation, name: "Managing org 3") } let(:inactive_managing_org) { create(:organisation, name: "Inactive managing org", active: false) } + let(:deleted_managing_org) { create(:organisation, name: "Deleted managing org", discarded_at: Time.zone.yesterday) } let(:log) { create(:lettings_log, managing_organisation: managing_org1) } let!(:org_rel1) do @@ -79,6 +80,7 @@ RSpec.describe Form::Lettings::Questions::ManagingOrganisation, type: :model do it "shows current managing agent at top, followed by user's org (with hint), followed by the active managing agents of the user's org" do create(:organisation_relationship, parent_organisation: user.organisation, child_organisation: inactive_managing_org) + create(:organisation_relationship, parent_organisation: user.organisation, child_organisation: deleted_managing_org) expect(question.displayed_answer_options(log, user)).to eq(options) end @@ -104,7 +106,10 @@ RSpec.describe Form::Lettings::Questions::ManagingOrganisation, type: :model do end before do - create(:organisation, name: "Inactive managing org", active: false) + inactive_managing_org = create(:organisation, name: "Inactive managing org", active: false) + create(:organisation_relationship, parent_organisation: log_owning_org, child_organisation: inactive_managing_org) + deleted_managing_org = create(:organisation, name: "Deleted managing org", discarded_at: Time.zone.yesterday) + create(:organisation_relationship, parent_organisation: log_owning_org, child_organisation: deleted_managing_org) end context "when org owns stock" do @@ -192,10 +197,12 @@ RSpec.describe Form::Lettings::Questions::ManagingOrganisation, type: :model do context "when organisation has merged" do let(:absorbing_org) { create(:organisation, name: "Absorbing org", holds_own_stock: true) } let!(:merged_org) { create(:organisation, name: "Merged org", holds_own_stock: false) } + let!(:merged_deleted_org) { create(:organisation, name: "Merged org", holds_own_stock: false, discarded_at: Time.zone.yesterday) } let(:user) { create(:user, :data_coordinator, organisation: absorbing_org) } let(:log) do merged_org.update!(merge_date: Time.zone.local(2023, 8, 2), absorbing_organisation_id: absorbing_org.id) + merged_deleted_org.update!(merge_date: Time.zone.local(2023, 8, 2), absorbing_organisation_id: absorbing_org.id) create(:lettings_log, owning_organisation: absorbing_org, managing_organisation: nil) end diff --git a/spec/models/form/lettings/questions/stock_owner_spec.rb b/spec/models/form/lettings/questions/stock_owner_spec.rb index a4c7d8fd6..8443f3680 100644 --- a/spec/models/form/lettings/questions/stock_owner_spec.rb +++ b/spec/models/form/lettings/questions/stock_owner_spec.rb @@ -47,6 +47,7 @@ RSpec.describe Form::Lettings::Questions::StockOwner, type: :model do let(:owning_org_1) { create(:organisation, name: "Owning org 1") } let(:owning_org_2) { create(:organisation, name: "Owning org 2") } let(:inactive_owning_org) { create(:organisation, name: "Inactive owning org", active: false) } + let(:deleted_owning_org) { create(:organisation, name: "Deleted owning org", discarded_at: Time.zone.yesterday) } let!(:org_rel) do create(:organisation_relationship, child_organisation: user.organisation, parent_organisation: owning_org_2) end @@ -64,6 +65,7 @@ RSpec.describe Form::Lettings::Questions::StockOwner, type: :model do it "shows current stock owner at top, followed by user's org (with hint), followed by the active stock owners of the user's org" do create(:organisation_relationship, child_organisation: user.organisation, parent_organisation: inactive_owning_org) + create(:organisation_relationship, child_organisation: user.organisation, parent_organisation: deleted_owning_org) user.organisation.update!(holds_own_stock: true) expect(question.displayed_answer_options(log, user)).to eq(options) end @@ -127,6 +129,7 @@ RSpec.describe Form::Lettings::Questions::StockOwner, type: :model do context "when user's org has recently absorbed other orgs and does not have available from date" do let(:merged_organisation) { create(:organisation, name: "Merged org") } + let(:merged_deleted_organisation) { create(:organisation, name: "Merged deleted org", discarded_at: Time.zone.yesterday) } let(:options) do { "" => "Select an option", @@ -139,6 +142,7 @@ RSpec.describe Form::Lettings::Questions::StockOwner, type: :model do before do merged_organisation.update!(merge_date: Time.zone.local(2023, 2, 2), absorbing_organisation: user.organisation) + merged_deleted_organisation.update!(merge_date: Time.zone.local(2023, 2, 2), absorbing_organisation: user.organisation) allow(Time).to receive(:now).and_return(Time.zone.local(2023, 11, 10)) end @@ -172,6 +176,7 @@ RSpec.describe Form::Lettings::Questions::StockOwner, type: :model do 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(:merged_deleted_organisation) { create(:organisation, name: "Merged deleted org", discarded_at: Time.zone.yesterday) } let(:options) do { "" => "Select an option", @@ -184,6 +189,7 @@ RSpec.describe Form::Lettings::Questions::StockOwner, type: :model do allow(Time).to receive(:now).and_return(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) + merged_deleted_organisation.update!(merge_date: Time.zone.local(2021, 6, 2), absorbing_organisation: user.organisation) user.organisation.update!(available_from: Time.zone.local(2021, 2, 2)) end @@ -200,8 +206,9 @@ RSpec.describe Form::Lettings::Questions::StockOwner, type: :model do it "shows active orgs where organisation holds own stock" do non_stock_organisation = create(:organisation, name: "Non-stockholding org", holds_own_stock: false) inactive_organisation = create(:organisation, name: "Inactive org", active: false) + deleted_organisation = create(:organisation, name: "Deleted org", discarded_at: Time.zone.yesterday) - expected_opts = Organisation.filter_by_active.where(holds_own_stock: true).each_with_object(options) do |organisation, hsh| + expected_opts = Organisation.visible.filter_by_active.where(holds_own_stock: true).each_with_object(options) do |organisation, hsh| hsh[organisation.id] = organisation.name hsh end @@ -209,10 +216,12 @@ RSpec.describe Form::Lettings::Questions::StockOwner, 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) expect(question.displayed_answer_options(log, user)).not_to include(inactive_organisation.id) + expect(question.displayed_answer_options(log, user)).not_to include(deleted_organisation.id) end context "and org has recently absorbed other orgs and does not have available from date" do let(:merged_organisation) { create(:organisation, name: "Merged org") } + let(:merged_deleted_organisation) { create(:organisation, name: "Merged deleted org", discarded_at: Time.zone.yesterday) } let(:org) { create(:organisation, name: "User org") } let(:options) do { @@ -226,6 +235,7 @@ RSpec.describe Form::Lettings::Questions::StockOwner, type: :model do before do merged_organisation.update!(merge_date: Time.zone.local(2023, 2, 2), absorbing_organisation: org) + merged_deleted_organisation.update!(merge_date: Time.zone.local(2023, 2, 2), absorbing_organisation: org) allow(Time).to receive(:now).and_return(Time.zone.local(2023, 11, 10)) end @@ -273,6 +283,18 @@ RSpec.describe Form::Lettings::Questions::StockOwner, type: :model do expect(question.hidden_in_check_answers?(nil, user)).to be false end end + + context "when stock owner is deleted" do + before do + organisation_relationship = create(:organisation_relationship, child_organisation: user.organisation) + organisation_relationship.parent_organisation.update!(discarded_at: Time.zone.yesterday) + end + + it "is hidden in check answers" do + expect(user.organisation.stock_owners.count).to eq(1) + expect(question.hidden_in_check_answers?(nil, user)).to be true + end + end end context "when org does not hold own stock", :aggregate_failures do diff --git a/spec/models/form/sales/questions/managing_organisation_spec.rb b/spec/models/form/sales/questions/managing_organisation_spec.rb index c9db71423..2ebbf5e01 100644 --- a/spec/models/form/sales/questions/managing_organisation_spec.rb +++ b/spec/models/form/sales/questions/managing_organisation_spec.rb @@ -58,6 +58,7 @@ RSpec.describe Form::Sales::Questions::ManagingOrganisation, type: :model do let(:managing_org2) { create(:organisation, name: "Managing org 2") } let(:managing_org3) { create(:organisation, name: "Managing org 3") } let(:inactive_org) { create(:organisation, name: "Inactive org", active: false) } + let(:deleted_org) { create(:organisation, name: "Deleted org", discarded_at: Time.zone.yesterday) } let(:log) do create(:lettings_log, owning_organisation: log_owning_org, managing_organisation: managing_org1, @@ -83,6 +84,7 @@ RSpec.describe Form::Sales::Questions::ManagingOrganisation, type: :model do it "shows current managing agent at top, followed by the current owning organisation (with hint), followed by the active managing agents of the current owning organisation" do create(:organisation_relationship, parent_organisation: log_owning_org, child_organisation: inactive_org) + create(:organisation_relationship, parent_organisation: log_owning_org, child_organisation: deleted_org) log_owning_org.update!(holds_own_stock: true) expect(question.displayed_answer_options(log, user)).to eq(options) end @@ -135,10 +137,12 @@ RSpec.describe Form::Sales::Questions::ManagingOrganisation, type: :model do context "when organisation has merged" do let(:absorbing_org) { create(:organisation, name: "Absorbing org", holds_own_stock: true) } let!(:merged_org) { create(:organisation, name: "Merged org", holds_own_stock: false) } + let!(:merged_deleted_org) { create(:organisation, name: "Merged deleted org", holds_own_stock: false, discarded_at: Time.zone.yesterday) } let(:user) { create(:user, :support, organisation: absorbing_org) } let(:log) do merged_org.update!(merge_date: Time.zone.local(2023, 8, 2), absorbing_organisation_id: absorbing_org.id) + merged_deleted_org.update!(merge_date: Time.zone.local(2023, 8, 2), absorbing_organisation_id: absorbing_org.id) create(:lettings_log, owning_organisation: absorbing_org, managing_organisation: nil) 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 ea904b400..bc6fad6e5 100644 --- a/spec/models/form/sales/questions/owning_organisation_id_spec.rb +++ b/spec/models/form/sales/questions/owning_organisation_id_spec.rb @@ -123,6 +123,11 @@ RSpec.describe Form::Sales::Questions::OwningOrganisationId, type: :model do it "shows merged organisation as an option" do expect(question.displayed_answer_options(log, user)).to eq(options) end + + it "does not show absorbed organisation if it has been deleted" do + merged_organisation.update!(discarded_at: Time.zone.yesterday) + expect(question.displayed_answer_options(log, user)).to eq(options.except(merged_organisation.id)) + end end context "when user's org has recently absorbed other orgs and it has available from date" do @@ -200,8 +205,9 @@ RSpec.describe Form::Sales::Questions::OwningOrganisationId, type: :model do it "shows active orgs where organisation holds own stock" do non_stock_organisation = create(:organisation, holds_own_stock: false) inactive_org = create(:organisation, active: false) + deleted_organisation = create(:organisation, discarded_at: Time.zone.yesterday) - expected_opts = Organisation.filter_by_active.where(holds_own_stock: true).each_with_object(options) do |organisation, hsh| + expected_opts = Organisation.visible.filter_by_active.where(holds_own_stock: true).each_with_object(options) do |organisation, hsh| hsh[organisation.id] = organisation.name hsh end @@ -210,6 +216,7 @@ RSpec.describe Form::Sales::Questions::OwningOrganisationId, type: :model do expect(question.displayed_answer_options(log, user)).not_to include(non_stock_organisation.id) expect(question.displayed_answer_options(log, user)).not_to include(inactive_org.id) expect(question.displayed_answer_options(log, user)).to include(organisation_1.id) + expect(question.displayed_answer_options(log, user)).not_to include(deleted_organisation.id) end context "when an org has recently absorbed other orgs" do