CLDC-2073 app crashing if needs type is changed (#1368)

* add some methods on lettings log and use them and already existing methods to make depends on human readbale for pages in property information subsection

* reformat answer options for neatness

* ensure that property information subsection is conditionally not shown under correct conditions

* amend method to find the path for the next question to answer so that if no questions in the next section are currently routed to, it goes to the section after that

* make amendments for failing test and linting

* remove unnecessary method

* remove unnecessary conditions from depends on, the displayed in tasklist method override is sufficient

* test the change to the method creating a link to the next subsection

* minor refactor to displayed_is_tasklist condition and changes in testing

* implement the fix on 22/23 form also

* write a test covering the fix

* removal not_renewal? method and tweak depends on for all pages using that method

* linty fluff begone

* correct some errors in tests after changes to factories

* minor testing amendments after rebase

app/models/form.rb

@@ -111,8 +111,8 @@ class Form
     when :in_progress
       "#{next_subsection.id}/check_answers".dasherize
     when :not_started
-      first_question_in_subsection = next_subsection.pages.find { |page| page.routed_to?(log, nil) }.id
-      first_question_in_subsection.to_s.dasherize
+      first_question_in_subsection = next_subsection.pages.find { |page| page.routed_to?(log, nil) }
+      first_question_in_subsection ? first_question_in_subsection.id.to_s.dasherize : next_incomplete_section_redirect_path(next_subsection, log)
     else
       "error"
     end

app/models/form/lettings/pages/first_time_property_let_as_social_housing.rb

@@ -2,7 +2,7 @@ class Form::Lettings::Pages::FirstTimePropertyLetAsSocialHousing < ::Form::Page
   def initialize(id, hsh, subsection)
     super
     @id = "first_time_property_let_as_social_housing"
-    @depends_on = [{ "renewal" => 0 }]
+    @depends_on = [{ "is_renewal?" => false }]
   end
 
   def questions

app/models/form/lettings/pages/new_build_handover_date.rb

@@ -2,11 +2,7 @@ class Form::Lettings::Pages::NewBuildHandoverDate < ::Form::Page
   def initialize(id, hsh, subsection)
     super
     @id = "new_build_handover_date"
-    @depends_on = [
-      { "renewal" => 0, "rsnvac" => 15 },
-      { "renewal" => 0, "rsnvac" => 16 },
-      { "renewal" => 0, "rsnvac" => 17 },
-    ]
+    @depends_on = [{ "is_renewal?" => false, "has_first_let_vacancy_reason?" => true }]
   end
 
   def questions

app/models/form/lettings/pages/property_building_type.rb

@@ -2,7 +2,7 @@ class Form::Lettings::Pages::PropertyBuildingType < ::Form::Page
   def initialize(id, hsh, subsection)
     super
     @id = "property_building_type"
-    @depends_on = [{ "needstype" => 1 }]
+    @depends_on = [{ "is_general_needs?" => true }]
   end
 
   def questions

app/models/form/lettings/pages/property_let_type.rb

@@ -2,7 +2,7 @@ class Form::Lettings::Pages::PropertyLetType < ::Form::Page
   def initialize(id, hsh, subsection)
     super
     @id = "property_let_type"
-    @depends_on = [{ "first_time_property_let_as_social_housing" => 0, "renewal" => 0 }]
+    @depends_on = [{ "first_time_property_let_as_social_housing" => 0, "is_renewal?" => false }]
   end
 
   def questions

app/models/form/lettings/pages/property_local_authority.rb

@@ -2,7 +2,7 @@ class Form::Lettings::Pages::PropertyLocalAuthority < ::Form::Page
   def initialize(id, hsh, subsection)
     super
     @id = "property_local_authority"
-    @depends_on = [{ "is_la_inferred" => false, "needstype" => 1 }]
+    @depends_on = [{ "is_la_inferred" => false, "is_general_needs?" => true }]
   end
 
   def questions

app/models/form/lettings/pages/property_major_repairs.rb

@@ -2,18 +2,7 @@ class Form::Lettings::Pages::PropertyMajorRepairs < ::Form::Page
   def initialize(id, hsh, subsection)
     super
     @id = "property_major_repairs"
-    @depends_on = [
-      { "renewal" => 0, "rsnvac" => 5 },
-      { "renewal" => 0, "rsnvac" => 6 },
-      { "renewal" => 0, "rsnvac" => 8 },
-      { "renewal" => 0, "rsnvac" => 9 },
-      { "renewal" => 0, "rsnvac" => 10 },
-      { "renewal" => 0, "rsnvac" => 11 },
-      { "renewal" => 0, "rsnvac" => 12 },
-      { "renewal" => 0, "rsnvac" => 13 },
-      { "renewal" => 0, "rsnvac" => 18 },
-      { "renewal" => 0, "rsnvac" => 19 },
-    ]
+    @depends_on = [{ "is_renewal?" => false, "vacancy_reason_not_renewal_or_first_let?" => true }]
   end
 
   def questions

app/models/form/lettings/pages/property_number_of_bedrooms.rb

@@ -2,7 +2,7 @@ class Form::Lettings::Pages::PropertyNumberOfBedrooms < ::Form::Page
   def initialize(id, hsh, subsection)
     super
     @id = "property_number_of_bedrooms"
-    @depends_on = [{ "needstype" => 1 }]
+    @depends_on = [{ "is_general_needs?" => true }]
   end
 
   def questions

app/models/form/lettings/pages/property_number_of_times_relet_not_social_let.rb

@@ -2,7 +2,7 @@ class Form::Lettings::Pages::PropertyNumberOfTimesReletNotSocialLet < ::Form::Pa
   def initialize(id, hsh, subsection)
     super
     @id = "property_number_of_times_relet_not_social_let"
-    @depends_on = [{ "first_time_property_let_as_social_housing" => 0, "renewal" => 0 }]
+    @depends_on = [{ "first_time_property_let_as_social_housing" => 0, "is_renewal?" => false }]
   end
 
   def questions

app/models/form/lettings/pages/property_number_of_times_relet_social_let.rb

@@ -2,7 +2,7 @@ class Form::Lettings::Pages::PropertyNumberOfTimesReletSocialLet < ::Form::Page
   def initialize(id, hsh, subsection)
     super
     @id = "property_number_of_times_relet_social_let"
-    @depends_on = [{ "first_time_property_let_as_social_housing" => 1, "renewal" => 0 }]
+    @depends_on = [{ "first_time_property_let_as_social_housing" => 1, "is_renewal?" => false }]
   end
 
   def questions

app/models/form/lettings/pages/property_postcode.rb

@@ -2,7 +2,7 @@ class Form::Lettings::Pages::PropertyPostcode < ::Form::Page
   def initialize(id, hsh, subsection)
     super
     @id = "property_postcode"
-    @depends_on = [{ "needstype" => 1 }]
+    @depends_on = [{ "is_general_needs?" => true }]
   end
 
   def questions

app/models/form/lettings/pages/property_unit_type.rb

@@ -2,7 +2,7 @@ class Form::Lettings::Pages::PropertyUnitType < ::Form::Page
   def initialize(id, hsh, subsection)
     super
     @id = "property_unit_type"
-    @depends_on = [{ "needstype" => 1 }]
+    @depends_on = [{ "is_general_needs?" => true }]
   end
 
   def questions

app/models/form/lettings/pages/property_vacancy_reason_first_let.rb

@@ -2,7 +2,7 @@ class Form::Lettings::Pages::PropertyVacancyReasonFirstLet < ::Form::Page
   def initialize(id, hsh, subsection)
     super
     @id = "property_vacancy_reason_first_let"
-    @depends_on = [{ "first_time_property_let_as_social_housing" => 1, "renewal" => 0 }]
+    @depends_on = [{ "first_time_property_let_as_social_housing" => 1, "is_renewal?" => false }]
   end
 
   def questions

app/models/form/lettings/pages/property_vacancy_reason_not_first_let.rb

@@ -2,7 +2,7 @@ class Form::Lettings::Pages::PropertyVacancyReasonNotFirstLet < ::Form::Page
   def initialize(id, hsh, subsection)
     super
     @id = "property_vacancy_reason_not_first_let"
-    @depends_on = [{ "first_time_property_let_as_social_housing" => 0, "renewal" => 0 }]
+    @depends_on = [{ "first_time_property_let_as_social_housing" => 0, "is_renewal?" => false }]
   end
 
   def questions

app/models/form/lettings/pages/property_wheelchair_accessible.rb

@@ -2,7 +2,7 @@ class Form::Lettings::Pages::PropertyWheelchairAccessible < ::Form::Page
   def initialize(id, hsh, subsection)
     super
     @id = "property_wheelchair_accessible"
-    @depends_on = [{ "is_supported_housing?" => false }]
+    @depends_on = [{ "is_general_needs?" => true }]
   end
 
   def questions

app/models/form/lettings/pages/void_or_renewal_date.rb

@@ -2,18 +2,7 @@ class Form::Lettings::Pages::VoidOrRenewalDate < ::Form::Page
   def initialize(id, hsh, subsection)
     super
     @id = "void_or_renewal_date"
-    @depends_on = [
-      { "renewal" => 0, "rsnvac" => 5 },
-      { "renewal" => 0, "rsnvac" => 6 },
-      { "renewal" => 0, "rsnvac" => 8 },
-      { "renewal" => 0, "rsnvac" => 9 },
-      { "renewal" => 0, "rsnvac" => 10 },
-      { "renewal" => 0, "rsnvac" => 11 },
-      { "renewal" => 0, "rsnvac" => 12 },
-      { "renewal" => 0, "rsnvac" => 13 },
-      { "renewal" => 0, "rsnvac" => 18 },
-      { "renewal" => 0, "rsnvac" => 19 },
-    ]
+    @depends_on = [{ "is_renewal?" => false, "vacancy_reason_not_renewal_or_first_let?" => true }]
   end
 
   def questions

app/models/form/lettings/questions/rsnvac_first_let.rb

@@ -12,11 +12,8 @@ class Form::Lettings::Questions::RsnvacFirstLet < ::Form::Question
   end
 
   ANSWER_OPTIONS = {
-    "16" =>
-        { "value" => "First let of conversion, rehabilitation or acquired property" },
-    "17" =>
-  { "value" => "First let of leased property" },
-    "15" =>
-  { "value" => "First let of new-build property" },
+    "16" => { "value" => "First let of conversion, rehabilitation or acquired property" },
+    "17" => { "value" => "First let of leased property" },
+    "15" => { "value" => "First let of new-build property" },
   }.freeze
 end

app/models/form/lettings/subsections/property_information.rb

@@ -27,4 +27,8 @@ class Form::Lettings::Subsections::PropertyInformation < ::Form::Subsection
       Form::Lettings::Pages::PropertyMajorRepairsValueCheck.new(nil, nil, self),
     ].compact
   end
+
+  def displayed_in_tasklist?(log)
+    !(log.is_supported_housing? && log.is_renewal?)
+  end
 end

app/models/form/subsection.rb

@@ -7,6 +7,7 @@ class Form::Subsection
     if hsh
       @label = hsh["label"]
       @depends_on = hsh["depends_on"]
+      @displayed_in_tasklist_from_json = hsh["displayed_in_tasklist"]
       @pages = hsh["pages"].map { |s_id, p| Form::Page.new(s_id, p, self) }
     end
   end
@@ -48,7 +49,13 @@ class Form::Subsection
     end
   end
 
-  def displayed_in_tasklist?(_log)
-    true
+  def displayed_in_tasklist?(log)
+    return true unless @displayed_in_tasklist_from_json
+
+    @displayed_in_tasklist_from_json.any? do |conditions|
+      conditions.all? do |method, expected_return_value|
+        log.send(method) == expected_return_value
+      end
+    end
   end
 end

app/models/lettings_log.rb

@@ -287,6 +287,10 @@ class LettingsLog < Log
     [15, 16, 17].include?(rsnvac)
   end
 
+  def vacancy_reason_not_renewal_or_first_let?
+    [5, 6, 8, 9, 10, 11, 12, 13, 18, 19].include? rsnvac
+  end
+
   def previous_tenancy_was_temporary?
     # 4: Tied housing or renting with job
     # 6: Supported housing

config/forms/2022_2023.json

@@ -939,7 +939,15 @@
                 }
               }
             }
-          }
+          },
+          "displayed_in_tasklist": [
+            {
+              "is_supported_housing?": false
+            },
+            {
+              "is_renewal?": false
+            }
+          ]
         },
         "tenancy_information": {
           "label": "Tenancy information",

spec/factories/lettings_log.rb

@@ -30,7 +30,7 @@ FactoryBot.define do
       age1 { 34 }
       sex1 { "M" }
       ethnic { 2 }
-      national { 4 }
+      national { 18 }
       ecstat1 { 2 }
       hhmemb { 1 }
     end
@@ -43,7 +43,7 @@ FactoryBot.define do
       sex1 { "F" }
       ethnic_group { 0 }
       ethnic { 2 }
-      national { 4 }
+      national { 13 }
       prevten { 6 }
       ecstat1 { 0 }
       hhmemb { 2 }
@@ -79,7 +79,7 @@ FactoryBot.define do
       supcharg { 35 }
       tcharge { 325 }
       layear { 2 }
-      waityear { 1 }
+      waityear { 7 }
       postcode_known { 1 }
       postcode_full { Faker::Address.postcode }
       reasonpref { 1 }

spec/fixtures/exports/general_needs_log.csv

@@ -1,2 +1,2 @@
 status,tenancycode,age1,sex1,ethnic,national,prevten,ecstat1,hhmemb,age2,sex2,ecstat2,age3,sex3,ecstat3,age4,sex4,ecstat4,age5,sex5,ecstat5,age6,sex6,ecstat6,age7,sex7,ecstat7,age8,sex8,ecstat8,homeless,underoccupation_benefitcap,leftreg,reservist,illness,preg_occ,startertenancy,tenancylength,tenancy,ppostcode_full,rsnvac,unittype_gn,beds,offered,wchair,earnings,incfreq,benefits,period,layear,waityear,postcode_full,reasonpref,cbl,chr,cap,reasonother,housingneeds_a,housingneeds_b,housingneeds_c,housingneeds_f,housingneeds_g,housingneeds_h,illness_type_1,illness_type_2,illness_type_3,illness_type_4,illness_type_8,illness_type_5,illness_type_6,illness_type_7,illness_type_9,illness_type_10,rp_homeless,rp_insan_unsat,rp_medwel,rp_hardship,rp_dontknow,tenancyother,irproduct_other,reason,propcode,la,prevloc,hb,hbrentshortfall,mrcdate,incref,startdate,armedforces,unitletas,builtype,voiddate,renttype,needstype,lettype,totchild,totelder,totadult,nocharge,referral,brent,scharge,pscharge,supcharg,tcharge,tshortfall,chcharge,ppcodenk,has_benefits,renewal,wrent,wscharge,wpschrge,wsupchrg,wtcharge,wtshortfall,refused,housingneeds,wchchrg,newprop,relat2,relat3,relat4,relat5,relat6,relat7,relat8,lar,irproduct,joint,sheltered,hhtype,new_old,vacdays,formid,owningorgid,owningorgname,hcnum,maningorgid,maningorgname,manhcnum,createddate,uploaddate
-2,BZ737,35,F,2,4,6,0,2,32,M,6,,,,,,,,,,,,,,,,,,,1,4,1,1,1,2,1,5,1,SE2 6RT,6,7,3,2,1,68,1,1,2,2,1,NW1 5TY,1,2,1,2,,1,0,0,0,0,0,0,1,0,0,0,0,0,0,0,0,0,1,0,0,0,,,4,123,E09000003,E07000105,6,1,2020-05-05T10:36:49+01:00,0,2022-02-02T10:36:49+00:00,1,2,1,2019-11-03T00:00:00+00:00,2,1,7,0,0,2,0,2,200.0,50.0,40.0,35.0,325.0,12.0,,1,1,0,100.0,25.0,20.0,17.5,162.5,6.0,0,1,,2,P,,,,,,,,,,,4,2,638,{id},{owning_org_id},DLUHC,1234,{managing_org_id},DLUHC,1234,2022-05-01T00:00:00+01:00,2022-05-01T00:00:00+01:00
+2,BZ737,35,F,2,13,6,0,2,32,M,6,,,,,,,,,,,,,,,,,,,1,4,1,1,1,2,1,5,1,SE2 6RT,6,7,3,2,1,68,1,1,2,2,7,NW1 5TY,1,2,1,2,,1,0,0,0,0,0,0,1,0,0,0,0,0,0,0,0,0,1,0,0,0,,,4,123,E09000003,E07000105,6,1,2020-05-05T10:36:49+01:00,0,2022-02-02T10:36:49+00:00,1,2,1,2019-11-03T00:00:00+00:00,2,1,7,0,0,2,0,2,200.0,50.0,40.0,35.0,325.0,12.0,,1,1,0,100.0,25.0,20.0,17.5,162.5,6.0,0,1,,2,P,,,,,,,,,,,4,2,638,{id},{owning_org_id},DLUHC,1234,{managing_org_id},DLUHC,1234,2022-05-01T00:00:00+01:00,2022-05-01T00:00:00+01:00

spec/fixtures/exports/general_needs_log.xml

@@ -6,7 +6,7 @@
     <age1>35</age1>
     <sex1>F</sex1>
     <ethnic>2</ethnic>
-    <national>4</national>
+    <national>13</national>
     <prevten>6</prevten>
     <ecstat1>0</ecstat1>
     <hhmemb>2</hhmemb>
@@ -51,7 +51,7 @@
     <benefits>1</benefits>
     <period>2</period>
     <layear>2</layear>
-    <waityear>1</waityear>
+    <waityear>7</waityear>
     <postcode_full>NW1 5TY</postcode_full>
     <reasonpref>1</reasonpref>
     <cbl>2</cbl>

spec/fixtures/exports/supported_housing_logs.xml

@@ -6,7 +6,7 @@
     <age1>35</age1>
     <sex1>F</sex1>
     <ethnic>2</ethnic>
-    <national>4</national>
+    <national>13</national>
     <prevten>6</prevten>
     <ecstat1>0</ecstat1>
     <hhmemb>2</hhmemb>
@@ -50,7 +50,7 @@
     <benefits>1</benefits>
     <period>2</period>
     <layear>2</layear>
-    <waityear>1</waityear>
+    <waityear>7</waityear>
     <postcode_full>SW1A 2AA</postcode_full>
     <reasonpref>1</reasonpref>
     <cbl>2</cbl>

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

@@ -6,7 +6,7 @@ RSpec.describe Form::Lettings::Pages::PropertyWheelchairAccessible, type: :model
   let(:subsection) { instance_double(Form::Subsection) }
 
   it "has correct subsection" do
-    expect(page.subsection).to eq(subsection)
+    expect(page.subsection).to be(subsection)
   end
 
   it "has correct questions" do
@@ -26,6 +26,6 @@ RSpec.describe Form::Lettings::Pages::PropertyWheelchairAccessible, type: :model
   end
 
   it "has the correct depends_on" do
-    expect(page.depends_on).to eq([{ "is_supported_housing?" => false }])
+    expect(page.depends_on).to eq([{ "is_general_needs?" => true }])
   end
 end

spec/models/form/lettings/subsections/property_information_spec.rb

@@ -1,10 +1,8 @@
 require "rails_helper"
 
 RSpec.describe Form::Lettings::Subsections::PropertyInformation, type: :model do
-  subject(:property_information) { described_class.new(subsection_id, subsection_definition, section) }
+  subject(:property_information) { described_class.new(nil, nil, section) }
 
-  let(:subsection_id) { nil }
-  let(:subsection_definition) { nil }
   let(:section) { instance_double(Form::Lettings::Sections::TenancyAndProperty) }
 
   it "has correct section" do
@@ -44,10 +42,6 @@ RSpec.describe Form::Lettings::Subsections::PropertyInformation, type: :model do
   end
 
   it "has the correct depends_on" do
-    expect(property_information.depends_on).to eq([
-      {
-        "non_location_setup_questions_completed?" => true,
-      },
-    ])
+    expect(property_information.depends_on).to eq([{ "non_location_setup_questions_completed?" => true }])
   end
 end

spec/models/form_spec.rb

@@ -183,6 +183,26 @@ RSpec.describe Form, type: :model do
         expect(form.next_incomplete_section_redirect_path(subsection, lettings_log)).to eq("declaration")
       end
     end
+
+    context "when no pages or questions in the next subsection are routed to" do
+      let(:subsection) { form.get_subsection("setup") }
+
+      around do |example|
+        FormHandler.instance.use_real_forms!
+
+        example.run
+
+        FormHandler.instance.use_fake_forms!
+      end
+
+      it "finds the path to the section after" do
+        lettings_log.startdate = Time.zone.local(2022, 9, 1)
+        lettings_log.renewal = 1
+        lettings_log.needstype = 2
+        lettings_log.postcode_known = 0
+        expect(form.next_incomplete_section_redirect_path(subsection, lettings_log)).to eq("joint")
+      end
+    end
   end
 
   describe "invalidated_page_questions" do

spec/requests/lettings_logs_controller_spec.rb

@@ -891,6 +891,38 @@ RSpec.describe LettingsLogsController, type: :request do
             end
           end
 
+          context "when a lettings log is for a renewal of supported housing, so property information does not need to show" do
+            let(:lettings_log) do
+              FactoryBot.create(
+                :lettings_log,
+                owning_organisation: user.organisation,
+                managing_organisation: user.organisation,
+                created_by: user,
+                startdate: Time.zone.now,
+                renewal: 1,
+                needstype: 2,
+                rent_type: 3,
+                postcode_known: 0,
+              )
+            end
+
+            before do
+              sign_in user
+            end
+
+            around do |example|
+              FormHandler.instance.use_real_forms!
+              example.run
+              FormHandler.instance.use_fake_forms!
+            end
+
+            it "does not crash the app if postcode_known is not nil" do
+              expect {
+                get "/lettings-logs/#{lettings_log.id}", headers:, params: {}
+              }.not_to raise_error(ActionView::Template::Error)
+            end
+          end
+
           context "with a lettings log with a single section complete" do
             let(:section_completed_lettings_log) do
               FactoryBot.create(