CLDC-3483: Prevent/fix missing letting allocation values (#2453)

* CLDC-3483: Enforce that all lettings allocation method fields are present for bulk uploads

* CLDC-3483: Add task to fix existing nil letting allocation values

* Better capitalisation

* Keep previous punctuation

* Use find_each for task

app/services/bulk_upload/lettings/year2023/row_parser.rb

@@ -338,6 +338,10 @@ class BulkUpload::Lettings::Year2023::RowParser
             on: :after_log
 
   validates :field_116,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "was the letting made under the Choice-Based Lettings (CBL)?"),
+              category: :not_answered,
+            },
             inclusion: {
               in: [1, 2],
               message: I18n.t("validations.invalid_option", question: "was the letting made under the Choice-Based Lettings (CBL)"),
@@ -346,6 +350,10 @@ class BulkUpload::Lettings::Year2023::RowParser
             on: :after_log
 
   validates :field_117,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "was the letting made under the Common Allocation Policy (CAP)?"),
+              category: :not_answered,
+            },
             inclusion: {
               in: [1, 2],
               message: I18n.t("validations.invalid_option", question: "was the letting made under the Common Allocation Policy (CAP)"),
@@ -354,6 +362,10 @@ class BulkUpload::Lettings::Year2023::RowParser
             on: :after_log
 
   validates :field_118,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "was the letting made under the Common Housing Register (CHR)?"),
+              category: :not_answered,
+            },
             inclusion: {
               in: [1, 2],
               message: I18n.t("validations.invalid_option", question: "was the letting made under the Common Housing Register (CHR)"),
@@ -384,7 +396,6 @@ class BulkUpload::Lettings::Year2023::RowParser
   validate :validate_no_housing_needs_questions_answered, on: :after_log
   validate :validate_reasonable_preference_homeless, on: :after_log
   validate :validate_condition_effects, on: :after_log
-  validate :validate_lettings_allocation, on: :after_log
   validate :validate_if_log_already_exists, on: :after_log, if: -> { FeatureToggle.bulk_upload_duplicate_log_check_enabled? }
 
   validate :validate_owning_org_data_given, on: :after_log
@@ -679,14 +690,6 @@ private
     end
   end
 
-  def validate_lettings_allocation
-    if cbl.blank? && cap.blank? && chr.blank?
-      errors.add(:field_116, I18n.t("validations.not_answered", question: "was the letting made under the Choice-Based Lettings (CBL)?"), category: :not_answered)
-      errors.add(:field_117, I18n.t("validations.not_answered", question: "was the letting made under the Common Allocation Policy (CAP)?"), category: :not_answered)
-      errors.add(:field_118, I18n.t("validations.not_answered", question: "was the letting made under the Common Housing Register (CHR)?"), category: :not_answered)
-    end
-  end
-
   def household_no_illness?
     field_89 != 1
   end

app/services/bulk_upload/lettings/year2024/row_parser.rb

@@ -338,6 +338,10 @@ class BulkUpload::Lettings::Year2024::RowParser
             on: :after_log
 
   validates :field_112,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "was the letting made under the Choice-Based Lettings (CBL)?"),
+              category: :not_answered,
+            },
             inclusion: {
               in: [1, 2],
               message: I18n.t("validations.invalid_option", question: "was the letting made under the Choice-Based Lettings (CBL)"),
@@ -346,6 +350,10 @@ class BulkUpload::Lettings::Year2024::RowParser
             on: :after_log
 
   validates :field_113,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "was the letting made under the Common Allocation Policy (CAP)?"),
+              category: :not_answered,
+            },
             inclusion: {
               in: [1, 2],
               message: I18n.t("validations.invalid_option", question: "was the letting made under the Common Allocation Policy (CAP)"),
@@ -354,6 +362,10 @@ class BulkUpload::Lettings::Year2024::RowParser
             on: :after_log
 
   validates :field_114,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "was the letting made under the Common Housing Register (CHR)?"),
+              category: :not_answered,
+            },
             inclusion: {
               in: [1, 2],
               message: I18n.t("validations.invalid_option", question: "was the letting made under the Common Housing Register (CHR)"),
@@ -362,6 +374,10 @@ class BulkUpload::Lettings::Year2024::RowParser
             on: :after_log
 
   validates :field_115,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "was the letting made under the Accessible Register?"),
+              category: :not_answered,
+            },
             inclusion: {
               in: [1, 2],
               message: I18n.t("validations.invalid_option", question: "was the letting made under the Accessible Register"),
@@ -391,7 +407,6 @@ class BulkUpload::Lettings::Year2024::RowParser
   validate :validate_no_housing_needs_questions_answered, on: :after_log
   validate :validate_reasonable_preference_homeless, on: :after_log
   validate :validate_condition_effects, on: :after_log
-  validate :validate_lettings_allocation, on: :after_log
   validate :validate_if_log_already_exists, on: :after_log, if: -> { FeatureToggle.bulk_upload_duplicate_log_check_enabled? }
 
   validate :validate_owning_org_data_given, on: :after_log
@@ -712,15 +727,6 @@ private
     end
   end
 
-  def validate_lettings_allocation
-    if cbl.blank? && cap.blank? && chr.blank? && accessible_register.blank?
-      errors.add(:field_112, I18n.t("validations.not_answered", question: "was the letting made under the Choice-Based Lettings (CBL)?"))
-      errors.add(:field_113, I18n.t("validations.not_answered", question: "was the letting made under the Common Allocation Policy (CAP)?"))
-      errors.add(:field_114, I18n.t("validations.not_answered", question: "was the letting made under the Common Housing Register (CHR)?"))
-      errors.add(:field_115, I18n.t("validations.not_answered", question: "was the letting made under the Accessible Register?"))
-    end
-  end
-
   def household_no_illness?
     field_85 != 1
   end

lib/tasks/fix_nil_letting_allocation_values.rake

@@ -0,0 +1,25 @@
+desc "Infer nil letting allocation values as no"
+task fix_nil_letting_allocation_values: :environment do
+  LettingsLog.where(cbl: nil)
+     .or(LettingsLog.where(chr: nil))
+     .or(LettingsLog.where(cap: nil))
+     .or(LettingsLog.filter_by_year(2024).where(accessible_register: nil))
+     .find_each do |log|
+    next unless log.cbl.present? || log.chr.present? || log.cap.present? || log.accessible_register.present? || log.letting_allocation_unknown.present?
+
+    log.cbl = 0 if log.cbl.blank?
+    log.chr = 0 if log.chr.blank?
+    log.cap = 0 if log.cap.blank?
+    log.accessible_register = 0 if log.form.start_year_after_2024? && log.accessible_register.blank?
+
+    log.letting_allocation_unknown = if log.cbl == 1 || log.chr == 1 || log.cap == 1 || log.accessible_register == 1
+                                       0
+                                     else
+                                       1
+                                     end
+
+    next if log.save
+
+    Rails.logger.log("NilLettingsAllocationValues: Unable to save changes to log #{log.id}")
+  end
+end

spec/lib/tasks/fix_nil_letting_allocation_values_spec.rb

@@ -0,0 +1,73 @@
+require "rails_helper"
+require "rake"
+
+RSpec.describe "fix_nil_letting_allocation_values" do
+  describe ":fix_nil_letting_allocation_values", type: :task do
+    subject(:task) { Rake::Task["fix_nil_letting_allocation_values"] }
+
+    before do
+      Rake.application.rake_require("tasks/fix_nil_letting_allocation_values")
+      Rake::Task.define_task(:environment)
+      task.reenable
+    end
+
+    it "sets nil values to 0 when one allocation type value is non-nil" do
+      log = create(:lettings_log, :setup_completed, :startdate_today, cbl: nil, chr: nil, cap: 1, accessible_register: nil, letting_allocation_unknown: nil)
+
+      task.invoke
+
+      log.reload
+      expect(log.cbl).to be 0
+      expect(log.chr).to be 0
+      expect(log.cap).to be 1
+      expect(log.accessible_register).to be 0
+      expect(log.letting_allocation_unknown).to be 0
+    end
+
+    it "sets nil values to 0 and letting_allocation_unknown to 1 when non-nil allocation type values are 0" do
+      log = create(:lettings_log, :setup_completed, :startdate_today, cbl: 0, chr: 0, cap: nil, accessible_register: nil, letting_allocation_unknown: nil)
+
+      task.invoke
+
+      log.reload
+      expect(log.cbl).to be 0
+      expect(log.chr).to be 0
+      expect(log.cap).to be 0
+      expect(log.accessible_register).to be 0
+      expect(log.letting_allocation_unknown).to be 1
+    end
+
+    it "does not set anything when question has not been answered at all" do
+      log = create(:lettings_log, :setup_completed, :startdate_today, cbl: nil, chr: nil, cap: nil, accessible_register: nil, letting_allocation_unknown: nil)
+
+      task.invoke
+
+      log.reload
+      expect(log.cbl).to be_nil
+      expect(log.chr).to be_nil
+      expect(log.cap).to be_nil
+      expect(log.accessible_register).to be_nil
+      expect(log.letting_allocation_unknown).to be_nil
+    end
+
+    it "does not set accessible_register for logs before 2024" do
+      log = create(:lettings_log, :setup_completed, startdate: Time.zone.local(2023, 5, 1), cbl: 1, chr: nil, cap: nil, accessible_register: nil, letting_allocation_unknown: nil)
+
+      task.invoke
+
+      log.reload
+      expect(log.cbl).to be 1
+      expect(log.chr).to be 0
+      expect(log.cap).to be 0
+      expect(log.accessible_register).to be_nil
+      expect(log.letting_allocation_unknown).to be 0
+    end
+
+    it "logs the log id if the change cannot be saved" do
+      log = create(:lettings_log, :ignore_validation_errors, :setup_completed, startdate: Time.zone.local(2022, 4, 1), cbl: 1, chr: nil, cap: nil, letting_allocation_unknown: nil)
+
+      expect(Rails.logger).to receive(:log).with(match(/log #{log.id}/))
+      task.invoke
+    end
+  end
+end

spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb

@@ -1306,14 +1306,18 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
       end
     end
 
-    describe "#field_116, 117, 118" do
-      context "when none of field_116, 117, 118 are given" do
-        let(:attributes) { { bulk_upload:, field_116: "", field_117: "", field_118: "", field_89: "1" } }
-
-        it "sets correct errors" do
-          expect(parser.errors[:field_116]).to include("You must answer was the letting made under the Choice-Based Lettings (CBL)?")
-          expect(parser.errors[:field_117]).to include("You must answer was the letting made under the Common Allocation Policy (CAP)?")
-          expect(parser.errors[:field_118]).to include("You must answer was the letting made under the Common Housing Register (CHR)?")
+    describe "#field_116, 117, 118 (lettings allocation methods)" do
+      %i[field_116 field_117 field_118].each do |field|
+        context "when only #{field} is not given" do
+          let(:attributes) do
+            override = {}
+            override[field] = ""
+            { bulk_upload:, field_116: "2", field_117: "1", field_118: "2" }.merge(override)
+          end
+
+          it "adds an error to #{field}" do
+            expect(parser.errors[field]).to be_present
+          end
         end
       end
     end

spec/services/bulk_upload/lettings/year2024/row_parser_spec.rb

@@ -220,6 +220,7 @@ RSpec.describe BulkUpload::Lettings::Year2024::RowParser do
             field_112: "1",
             field_113: "2",
             field_114: "2",
+            field_115: "2",
 
             field_116: "2",
 
@@ -1188,15 +1189,18 @@ RSpec.describe BulkUpload::Lettings::Year2024::RowParser do
       end
     end
 
-    describe "#field_112, 117, 119" do
-      context "when none of field_112, 117, 119 are given" do
-        let(:attributes) { { bulk_upload:, field_112: "", field_113: "", field_114: "", field_85: "1" } }
+    describe "#field_112 - 115 (lettings allocation methods)" do
+      %i[field_112 field_113 field_114 field_115].each do |field|
+        context "when only #{field} is not given" do
+          let(:attributes) do
+            override = {}
+            override[field] = ""
+            { bulk_upload:, field_112: "2", field_113: "1", field_114: "2", field_115: "1" }.merge(override)
+          end
 
-        it "sets correct errors" do
-          expect(parser.errors[:field_112]).to include("You must answer was the letting made under the Choice-Based Lettings (CBL)?")
-          expect(parser.errors[:field_113]).to include("You must answer was the letting made under the Common Allocation Policy (CAP)?")
-          expect(parser.errors[:field_114]).to include("You must answer was the letting made under the Common Housing Register (CHR)?")
-          expect(parser.errors[:field_115]).to include("You must answer was the letting made under the Accessible Register?")
+          it "adds an error to #{field}" do
+            expect(parser.errors[field]).to be_present
+          end
         end
       end
     end