From 4d46e9bf7c62b49d89110df48a098ea819d24979 Mon Sep 17 00:00:00 2001 From: Jack S <113976590+bibblobcode@users.noreply.github.com> Date: Mon, 21 Nov 2022 11:38:08 +0000 Subject: [PATCH] [CLDC-20] Address po review - fix all broken flows (#1012) * Show also users from owning_organisation * Correctly handle housing provider question * Managing Organisation page should behave the same if user is support * Fix feature test --- .../form/lettings/pages/housing_provider.rb | 14 +++- .../lettings/pages/managing_organisation.rb | 1 - .../form/lettings/questions/created_by_id.rb | 7 +- spec/features/lettings_log_spec.rb | 3 - .../lettings/pages/housing_provider_spec.rb | 59 ++++++++++++++-- .../pages/managing_organisation_spec.rb | 67 +++++++++++++++++-- .../lettings/questions/created_by_id_spec.rb | 18 +++-- 7 files changed, 145 insertions(+), 24 deletions(-) diff --git a/app/models/form/lettings/pages/housing_provider.rb b/app/models/form/lettings/pages/housing_provider.rb index b63f71a7e..ea801b49c 100644 --- a/app/models/form/lettings/pages/housing_provider.rb +++ b/app/models/form/lettings/pages/housing_provider.rb @@ -16,11 +16,19 @@ class Form::Lettings::Pages::HousingProvider < ::Form::Page def routed_to?(log, current_user) return false unless current_user return true if current_user.support? - return true unless current_user.organisation.holds_own_stock? - return true if current_user.organisation.housing_providers.count.positive? + housing_providers = current_user.organisation.housing_providers - log.update!(owning_organisation: current_user.organisation) + if current_user.organisation.holds_own_stock? + return true if housing_providers.count >= 1 + + log.update!(owning_organisation: current_user.organisation) + else + return false if housing_providers.count.zero? + return true if housing_providers.count > 1 + + log.update!(owning_organisation: housing_providers.first) + end false end diff --git a/app/models/form/lettings/pages/managing_organisation.rb b/app/models/form/lettings/pages/managing_organisation.rb index 67e8ae586..f1d1197f7 100644 --- a/app/models/form/lettings/pages/managing_organisation.rb +++ b/app/models/form/lettings/pages/managing_organisation.rb @@ -15,7 +15,6 @@ class Form::Lettings::Pages::ManagingOrganisation < ::Form::Page def routed_to?(log, current_user) return false unless current_user - return true if current_user.support? return true unless current_user.organisation.holds_own_stock? managing_agents = current_user.organisation.managing_agents diff --git a/app/models/form/lettings/questions/created_by_id.rb b/app/models/form/lettings/questions/created_by_id.rb index cf7323940..a120e72a8 100644 --- a/app/models/form/lettings/questions/created_by_id.rb +++ b/app/models/form/lettings/questions/created_by_id.rb @@ -20,9 +20,12 @@ class Form::Lettings::Questions::CreatedById < ::Form::Question end def displayed_answer_options(log, _user = nil) - return answer_options unless log.owning_organisation + return answer_options unless log.owning_organisation && log.managing_organisation + + user_ids = [""] + user_ids += log.owning_organisation.users.pluck(:id) + user_ids += log.managing_organisation.users.pluck(:id) - user_ids = log.owning_organisation.users.pluck(:id) + [""] answer_options.select { |k, _v| user_ids.include?(k) } end diff --git a/spec/features/lettings_log_spec.rb b/spec/features/lettings_log_spec.rb index 91901659a..7836713e0 100644 --- a/spec/features/lettings_log_spec.rb +++ b/spec/features/lettings_log_spec.rb @@ -84,14 +84,11 @@ RSpec.describe "Lettings Log Features" do click_link("Set up this lettings log") select(support_user.organisation.name, from: "lettings-log-owning-organisation-id-field") click_button("Save and continue") - select("#{support_user.organisation.name} (Owning organisation)", from: "lettings-log-managing-organisation-id-field") - click_button("Save and continue") select(support_user.name, from: "lettings-log-created-by-id-field") click_button("Save and continue") log_id = page.current_path.scan(/\d/).join visit("lettings-logs/#{log_id}/setup/check-answers") expect(page).to have_content("Housing provider #{support_user.organisation.name}") - expect(page).to have_content("Managing agent #{support_user.organisation.name}") expect(page).to have_content("Log owner #{support_user.name}") expect(page).to have_content("You have answered 3 of 9 questions") end diff --git a/spec/models/form/lettings/pages/housing_provider_spec.rb b/spec/models/form/lettings/pages/housing_provider_spec.rb index 9cf3bce6a..014ffbd66 100644 --- a/spec/models/form/lettings/pages/housing_provider_spec.rb +++ b/spec/models/form/lettings/pages/housing_provider_spec.rb @@ -63,12 +63,63 @@ RSpec.describe Form::Lettings::Pages::HousingProvider, type: :model do create(:user, :data_coordinator, organisation: create(:organisation, holds_own_stock: false)) end - it "is shown" do - expect(page.routed_to?(log, user)).to eq(true) + context "with 0 housing_providers" do + it "is not shown" do + expect(page.routed_to?(log, user)).to eq(false) + end + + it "does not update owning_organisation_id" do + expect { page.routed_to?(log, user) }.not_to change(log.reload, :owning_organisation) + 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 1 housing_providers" do + let(:housing_provider) { create(:organisation) } + + before do + create( + :organisation_relationship, + :owning, + child_organisation: user.organisation, + parent_organisation: housing_provider, + ) + end + + it "is not shown" 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(housing_provider) + end + end + + context "with >1 housing_providers" do + let(:housing_provider1) { create(:organisation) } + let(:housing_provider2) { create(:organisation) } + + before do + create( + :organisation_relationship, + :owning, + child_organisation: user.organisation, + parent_organisation: housing_provider1, + ) + create( + :organisation_relationship, + :owning, + child_organisation: user.organisation, + parent_organisation: housing_provider2, + ) + 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) + end end end diff --git a/spec/models/form/lettings/pages/managing_organisation_spec.rb b/spec/models/form/lettings/pages/managing_organisation_spec.rb index fcb93d565..e4f27516f 100644 --- a/spec/models/form/lettings/pages/managing_organisation_spec.rb +++ b/spec/models/form/lettings/pages/managing_organisation_spec.rb @@ -47,15 +47,70 @@ RSpec.describe Form::Lettings::Pages::ManagingOrganisation, type: :model do end context "when support" do - let(:organisation) { create(:organisation) } - let(:user) { create(:user, :support, organisation:) } + context "when does not hold own stock" do + let(:user) do + create(:user, :support, organisation: create(:organisation, holds_own_stock: false)) + end + + it "is shown" do + expect(page.routed_to?(log, user)).to eq(true) + end - it "is shown" do - expect(page.routed_to?(log, user)).to eq(true) + it "does not update managing_organisation_id" do + expect { page.routed_to?(log, user) }.not_to change(log.reload, :managing_organisation) + end end - it "does not update managing_organisation_id" do - expect { page.routed_to?(log, user) }.not_to change(log.reload, :managing_organisation) + context "when holds own stock" do + let(:user) do + create(:user, :support, organisation: create(:organisation, holds_own_stock: true)) + end + + context "with 0 managing_agents" do + it "is not shown" do + expect(page.routed_to?(log, user)).to eq(false) + end + + it "does not update managing_organisation_id" do + expect { page.routed_to?(log, user) }.not_to change(log.reload, :managing_organisation) + end + end + + context "with >1 managing_agents" do + before do + create(:organisation_relationship, :managing, parent_organisation: user.organisation) + create(:organisation_relationship, :managing, parent_organisation: user.organisation) + end + + it "is shown" do + expect(page.routed_to?(log, user)).to eq(true) + end + + it "does not update managing_organisation_id" do + expect { page.routed_to?(log, user) }.not_to change(log.reload, :managing_organisation) + end + end + + context "with 1 managing_agents" do + let(:managing_agent) { create(:organisation) } + + before do + create( + :organisation_relationship, + :managing, + child_organisation: managing_agent, + parent_organisation: user.organisation, + ) + end + + it "is not shown" do + expect(page.routed_to?(log, user)).to eq(false) + end + + it "updates managing_organisation_id" do + expect { page.routed_to?(log, user) }.to change(log.reload, :managing_organisation).to(managing_agent) + end + end end end diff --git a/spec/models/form/lettings/questions/created_by_id_spec.rb b/spec/models/form/lettings/questions/created_by_id_spec.rb index 152a24265..04fac20fb 100644 --- a/spec/models/form/lettings/questions/created_by_id_spec.rb +++ b/spec/models/form/lettings/questions/created_by_id_spec.rb @@ -8,8 +8,9 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do let(:page) { instance_double(Form::Page) } let(:subsection) { instance_double(Form::Subsection) } let(:form) { instance_double(Form) } - let(:user_1) { FactoryBot.create(:user, name: "first user") } - let(:user_2) { FactoryBot.create(:user, name: "second user") } + let(:user_1) { create(:user, name: "first user") } + let(:user_2) { create(:user, name: "second user") } + let(:user_3) { create(:user, name: "third user") } let!(:expected_answer_options) do { "" => "Select an option", @@ -51,7 +52,7 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do end context "when the current user is support" do - let(:support_user) { FactoryBot.build(:user, :support) } + let(:support_user) { build(:user, :support) } it "is shown in check answers" do expect(question.hidden_in_check_answers?(nil, support_user)).to be false @@ -59,7 +60,7 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do end context "when the current user is not support" do - let(:user) { FactoryBot.build(:user) } + let(:user) { build(:user) } it "is not shown in check answers" do expect(question.hidden_in_check_answers?(nil, user)).to be true @@ -67,11 +68,18 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do end context "when the owning organisation is already set" do - let(:lettings_log) { FactoryBot.create(:lettings_log, owning_organisation: user_2.organisation) } + let(:lettings_log) do + create( + :lettings_log, + owning_organisation: user_2.organisation, + managing_organisation: user_3.organisation, + ) + end let(:expected_answer_options) do { "" => "Select an option", user_2.id => "#{user_2.name} (#{user_2.email})", + user_3.id => "#{user_3.name} (#{user_3.email})", } end