From c0541b054b442ef02b323b7285b52f866d082333 Mon Sep 17 00:00:00 2001
From: Jack S <113976590+bibblobcode@users.noreply.github.com>
Date: Wed, 23 Nov 2022 13:13:40 +0000
Subject: [PATCH] [CLDC-20] Address more ACs (#1020)

* Bugfix provider_type on nil

* Do not infer managing org

* Always show 'your org' to non support users

* Never show created_by_id in CYA

* Show HousingProvider in CYA when not inferred

* Always show ManagingOrganisation

* Spec fix

* Show ManagingOrganisation if managing agents >=1

* typo
---
 .../lettings_log_variables.rb                 |  4 +-
 .../lettings/pages/managing_organisation.rb   |  9 +--
 .../form/lettings/questions/created_by_id.rb  |  4 +-
 .../lettings/questions/housing_provider.rb    | 11 +++-
 .../questions/managing_organisation.rb        |  8 +--
 spec/features/lettings_log_spec.rb            |  6 +-
 .../pages/managing_organisation_spec.rb       | 48 ++--------------
 .../lettings/questions/created_by_id_spec.rb  | 16 +-----
 .../questions/housing_provider_spec.rb        | 56 ++++++++++++++-----
 .../questions/managing_organisation_spec.rb   | 50 +++++++++--------
 10 files changed, 94 insertions(+), 118 deletions(-)

diff --git a/app/models/derived_variables/lettings_log_variables.rb b/app/models/derived_variables/lettings_log_variables.rb
index c76d5c2d9..1b1817ec3 100644
--- a/app/models/derived_variables/lettings_log_variables.rb
+++ b/app/models/derived_variables/lettings_log_variables.rb
@@ -49,8 +49,8 @@ module DerivedVariables::LettingsLogVariables
       self.waityear = 2
       if is_general_needs?
         # fixed term
-        self.prevten = 32 if managing_organisation.provider_type == "PRP"
-        self.prevten = 30 if managing_organisation.provider_type == "LA"
+        self.prevten = 32 if managing_organisation&.provider_type == "PRP"
+        self.prevten = 30 if managing_organisation&.provider_type == "LA"
       end
     end
 
diff --git a/app/models/form/lettings/pages/managing_organisation.rb b/app/models/form/lettings/pages/managing_organisation.rb
index fc2ef5e36..bc8534999 100644
--- a/app/models/form/lettings/pages/managing_organisation.rb
+++ b/app/models/form/lettings/pages/managing_organisation.rb
@@ -25,13 +25,6 @@ class Form::Lettings::Pages::ManagingOrganisation < ::Form::Page
     return false unless organisation
     return true unless organisation.holds_own_stock?
 
-    managing_agents = organisation.managing_agents
-
-    return false if managing_agents.count.zero?
-    return true if managing_agents.count > 1
-
-    log.update!(managing_organisation: managing_agents.first)
-
-    false
+    organisation.managing_agents.count >= 1
   end
 end
diff --git a/app/models/form/lettings/questions/created_by_id.rb b/app/models/form/lettings/questions/created_by_id.rb
index a120e72a8..d5921e935 100644
--- a/app/models/form/lettings/questions/created_by_id.rb
+++ b/app/models/form/lettings/questions/created_by_id.rb
@@ -35,8 +35,8 @@ class Form::Lettings::Questions::CreatedById < ::Form::Question
     answer_options[value]
   end
 
-  def hidden_in_check_answers?(_log, current_user)
-    !current_user.support?
+  def hidden_in_check_answers?(_log, _current_user)
+    true
   end
 
   def derived?
diff --git a/app/models/form/lettings/questions/housing_provider.rb b/app/models/form/lettings/questions/housing_provider.rb
index 8d8db24b6..f46ea1632 100644
--- a/app/models/form/lettings/questions/housing_provider.rb
+++ b/app/models/form/lettings/questions/housing_provider.rb
@@ -42,10 +42,15 @@ class Form::Lettings::Questions::HousingProvider < ::Form::Question
   def hidden_in_check_answers?(_log, user = nil)
     @current_user = user
 
-    return false unless @current_user
-    return false if @current_user.support?
+    return false if current_user.support?
 
-    housing_providers_answer_options.count < 2
+    housing_providers = current_user.organisation.housing_providers
+
+    if current_user.organisation.holds_own_stock?
+      housing_providers.count.zero?
+    else
+      housing_providers.count <= 1
+    end
   end
 
   def enabled
diff --git a/app/models/form/lettings/questions/managing_organisation.rb b/app/models/form/lettings/questions/managing_organisation.rb
index 42662202c..82ae2aabd 100644
--- a/app/models/form/lettings/questions/managing_organisation.rb
+++ b/app/models/form/lettings/questions/managing_organisation.rb
@@ -21,7 +21,7 @@ class Form::Lettings::Questions::ManagingOrganisation < ::Form::Question
       if log.owning_organisation.holds_own_stock?
         opts[log.owning_organisation.id] = "#{log.owning_organisation.name} (Owning organisation)"
       end
-    elsif current_user.organisation.holds_own_stock?
+    else
       opts[current_user.organisation.id] = "#{current_user.organisation.name} (Your organisation)"
     end
 
@@ -47,11 +47,7 @@ class Form::Lettings::Questions::ManagingOrganisation < ::Form::Question
 
   def hidden_in_check_answers?(_log, user = nil)
     @current_user = user
-
-    return false unless @current_user
-    return false if @current_user.support?
-
-    managing_organisations_answer_options.count < 2
+    @current_user.nil?
   end
 
   def enabled
diff --git a/spec/features/lettings_log_spec.rb b/spec/features/lettings_log_spec.rb
index 7836713e0..6d09b205a 100644
--- a/spec/features/lettings_log_spec.rb
+++ b/spec/features/lettings_log_spec.rb
@@ -89,8 +89,7 @@ RSpec.describe "Lettings Log Features" do
         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("Log owner #{support_user.name}")
-        expect(page).to have_content("You have answered 3 of 9 questions")
+        expect(page).to have_content("You have answered 2 of 8 questions")
       end
     end
   end
@@ -119,8 +118,7 @@ RSpec.describe "Lettings Log Features" do
         expect(page).to have_current_path("/lettings-logs/#{log_id}/needs-type")
         visit("lettings-logs/#{log_id}/setup/check-answers")
         expect(page).not_to have_content("Owning organisation #{user.organisation.name}")
-        expect(page).not_to have_content("User #{user.name}")
-        expect(page).to have_content("You have answered 0 of 6 questions")
+        expect(page).not_to have_content("Log owner #{user.name}")
       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 4a44aa25d..65c2589c6 100644
--- a/spec/models/form/lettings/pages/managing_organisation_spec.rb
+++ b/spec/models/form/lettings/pages/managing_organisation_spec.rb
@@ -40,10 +40,6 @@ RSpec.describe Form::Lettings::Pages::ManagingOrganisation, type: :model do
       it "is not shown" do
         expect(page.routed_to?(log, nil)).to eq(false)
       end
-
-      it "does not update managing_organisation_id" do
-        expect { page.routed_to?(log, nil) }.not_to change(log.reload, :managing_organisation)
-      end
     end
 
     context "when support" do
@@ -56,10 +52,6 @@ RSpec.describe Form::Lettings::Pages::ManagingOrganisation, type: :model do
         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 owning_organisation not set" do
@@ -69,10 +61,6 @@ RSpec.describe Form::Lettings::Pages::ManagingOrganisation, type: :model 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 "when holds own stock" do
@@ -84,10 +72,6 @@ RSpec.describe Form::Lettings::Pages::ManagingOrganisation, type: :model 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
@@ -99,10 +83,6 @@ RSpec.describe Form::Lettings::Pages::ManagingOrganisation, type: :model do
           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
@@ -117,12 +97,8 @@ RSpec.describe Form::Lettings::Pages::ManagingOrganisation, type: :model do
             )
           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)
+          it "is shown" do
+            expect(page.routed_to?(log, user)).to eq(true)
           end
         end
       end
@@ -137,10 +113,6 @@ RSpec.describe Form::Lettings::Pages::ManagingOrganisation, type: :model do
         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
@@ -152,10 +124,6 @@ RSpec.describe Form::Lettings::Pages::ManagingOrganisation, type: :model 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
@@ -167,10 +135,6 @@ RSpec.describe Form::Lettings::Pages::ManagingOrganisation, type: :model do
           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
@@ -185,12 +149,8 @@ RSpec.describe Form::Lettings::Pages::ManagingOrganisation, type: :model do
             )
           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)
+          it "is shown" do
+            expect(page.routed_to?(log, user)).to eq(true)
           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 04fac20fb..a5e60ad2b 100644
--- a/spec/models/form/lettings/questions/created_by_id_spec.rb
+++ b/spec/models/form/lettings/questions/created_by_id_spec.rb
@@ -51,20 +51,8 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do
     expect(question.derived?).to be true
   end
 
-  context "when the current user is support" do
-    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
-    end
-  end
-
-  context "when the current user is not support" do
-    let(:user) { build(:user) }
-
-    it "is not shown in check answers" do
-      expect(question.hidden_in_check_answers?(nil, user)).to be true
-    end
+  it "is not shown in check answers" do
+    expect(question.hidden_in_check_answers?(nil, user_1)).to be true
   end
 
   context "when the owning organisation is already set" do
diff --git a/spec/models/form/lettings/questions/housing_provider_spec.rb b/spec/models/form/lettings/questions/housing_provider_spec.rb
index 073c59e37..5363a0227 100644
--- a/spec/models/form/lettings/questions/housing_provider_spec.rb
+++ b/spec/models/form/lettings/questions/housing_provider_spec.rb
@@ -83,34 +83,64 @@ RSpec.describe Form::Lettings::Questions::HousingProvider, type: :model do
   end
 
   describe "#hidden_in_check_answers?" do
-    context "when housing providers < 2" do
-      context "when not support user" do
-        let(:user) { create(:user) }
+    context "when support" do
+      let(:user) { create(:user, :support) }
+
+      it "is not hidden in check answers" do
+        expect(question.hidden_in_check_answers?(nil, user)).to be false
+      end
+    end
+
+    context "when org holds own stock", :aggregate_failures do
+      let(:user) { create(:user, :data_coordinator, organisation: create(:organisation, holds_own_stock: true)) }
+
+      context "when housing providers == 0" do
+        before do
+          user.organisation.housing_providers.delete_all
+        end
 
         it "is hidden in check answers" do
+          expect(user.organisation.housing_providers.count).to eq(0)
           expect(question.hidden_in_check_answers?(nil, user)).to be true
         end
       end
 
-      context "when support" do
-        let(:user) { create(:user, :support) }
+      context "when housing providers != 0" do
+        before do
+          create(:organisation_relationship, :owning, child_organisation: user.organisation)
+        end
 
-        it "is not hiddes in check answers" do
+        it "is visible in check answers" do
+          expect(user.organisation.housing_providers.count).to eq(1)
           expect(question.hidden_in_check_answers?(nil, user)).to be false
         end
       end
     end
 
-    context "when housing providers >= 2" do
-      let(:user) { create(:user) }
+    context "when org does not hold own stock", :aggregate_failures do
+      let(:user) { create(:user, :data_coordinator, organisation: create(:organisation, holds_own_stock: false)) }
 
-      before do
-        create(:organisation_relationship, :owning, child_organisation: user.organisation)
-        create(:organisation_relationship, :owning, child_organisation: user.organisation)
+      context "when housing providers <= 1" do
+        before do
+          create(:organisation_relationship, :owning, child_organisation: user.organisation)
+        end
+
+        it "is hidden in check answers" do
+          expect(user.organisation.housing_providers.count).to eq(1)
+          expect(question.hidden_in_check_answers?(nil, user)).to be true
+        end
       end
 
-      it "is not hidden in check answers" do
-        expect(question.hidden_in_check_answers?(nil, user)).to be false
+      context "when housing providers >= 2" do
+        before do
+          create(:organisation_relationship, :owning, child_organisation: user.organisation)
+          create(:organisation_relationship, :owning, child_organisation: user.organisation)
+        end
+
+        it "is visible in check answers" do
+          expect(user.organisation.housing_providers.count).to eq(2)
+          expect(question.hidden_in_check_answers?(nil, user)).to be false
+        end
       end
     end
   end
diff --git a/spec/models/form/lettings/questions/managing_organisation_spec.rb b/spec/models/form/lettings/questions/managing_organisation_spec.rb
index f80099d67..e045601bf 100644
--- a/spec/models/form/lettings/questions/managing_organisation_spec.rb
+++ b/spec/models/form/lettings/questions/managing_organisation_spec.rb
@@ -73,6 +73,27 @@ RSpec.describe Form::Lettings::Questions::ManagingOrganisation, type: :model do
       end
     end
 
+    context "when user not support and does not own stock" do
+      let(:user) { create(:user, :data_coordinator, organisation: create(:organisation, holds_own_stock: false)) }
+
+      let(:log) { create(:lettings_log) }
+      let!(:org_rel1) { create(:organisation_relationship, :managing, parent_organisation: user.organisation) }
+      let!(:org_rel2) { create(:organisation_relationship, :managing, parent_organisation: user.organisation) }
+
+      let(:options) do
+        {
+          "" => "Select an option",
+          user.organisation.id => "#{user.organisation.name} (Your organisation)",
+          org_rel1.child_organisation.id => org_rel1.child_organisation.name,
+          org_rel2.child_organisation.id => org_rel2.child_organisation.name,
+        }
+      end
+
+      it "shows managing agents with own org at the top" do
+        expect(question.displayed_answer_options(log, user)).to eq(options)
+      end
+    end
+
     context "when support user and org does not own own stock" do
       let(:user) { create(:user, :support) }
       let(:log_owning_org) { create(:organisation, holds_own_stock: false) }
@@ -121,34 +142,19 @@ RSpec.describe Form::Lettings::Questions::ManagingOrganisation, type: :model do
   end
 
   describe "#hidden_in_check_answers?" do
-    context "when housing providers < 2" do
-      context "when not support user" do
-        let(:user) { create(:user) }
-
-        it "is hidden in check answers" do
-          expect(question.hidden_in_check_answers?(nil, user)).to be true
-        end
-      end
-
-      context "when support" do
-        let(:user) { create(:user, :support) }
+    context "when user present" do
+      let(:user) { create(:user) }
 
-        it "is not hiddes in check answers" do
-          expect(question.hidden_in_check_answers?(nil, user)).to be false
-        end
+      it "is hidden in check answers" do
+        expect(question.hidden_in_check_answers?(nil, user)).to be false
       end
     end
 
-    context "when managing agents >= 2" do
-      let(:user) { create(:user) }
-
-      before do
-        create(:organisation_relationship, :managing, parent_organisation: user.organisation)
-        create(:organisation_relationship, :managing, parent_organisation: user.organisation)
-      end
+    context "when user not provided" do
+      let(:user) { create(:user, :support) }
 
       it "is not hidden in check answers" do
-        expect(question.hidden_in_check_answers?(nil, user)).to be false
+        expect(question.hidden_in_check_answers?(nil)).to be true
       end
     end
   end