[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

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
+
+    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

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

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
 

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

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
+        context "with 0 housing_providers" 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
 
           it "does not update owning_organisation_id" do
-          expect { page.routed_to?(log, user) }.not_to change(log.reload, :owning_organisation).from(nil)
+            expect { page.routed_to?(log, user) }.not_to change(log.reload, :owning_organisation)
+          end
+        end
+
+        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
 

spec/models/form/lettings/pages/managing_organisation_spec.rb

@@ -47,8 +47,40 @@ RSpec.describe Form::Lettings::Pages::ManagingOrganisation, type: :model do
     end
 
     context "when support" do
-      let(:organisation) { create(:organisation) }
+      context "when does not hold own stock" do
-      let(:user) { create(:user, :support, organisation:) }
+        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 "does not update managing_organisation_id" do
+          expect { page.routed_to?(log, user) }.not_to change(log.reload, :managing_organisation)
+        end
+      end
+
+      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)
@@ -59,6 +91,29 @@ RSpec.describe Form::Lettings::Pages::ManagingOrganisation, type: :model do
           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
+
     context "when not support" do
       context "when does not hold own stock" do
         let(:user) do

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_1) { create(:user, name: "first user") }
-  let(:user_2) { FactoryBot.create(:user, name: "second 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