From 4a5a912816327c830523e42b0ce75a7b65bf3503 Mon Sep 17 00:00:00 2001 From: Rachael Booth Date: Tue, 11 Jun 2024 17:01:34 +0100 Subject: [PATCH] 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 --- .../lettings/year2023/row_parser.rb | 21 +++--- .../lettings/year2024/row_parser.rb | 26 ++++--- .../fix_nil_letting_allocation_values.rake | 25 +++++++ .../fix_nil_letting_allocation_values_spec.rb | 73 +++++++++++++++++++ .../lettings/year2023/row_parser_spec.rb | 20 +++-- .../lettings/year2024/row_parser_spec.rb | 20 +++-- 6 files changed, 150 insertions(+), 35 deletions(-) create mode 100644 lib/tasks/fix_nil_letting_allocation_values.rake create mode 100644 spec/lib/tasks/fix_nil_letting_allocation_values_spec.rb diff --git a/app/services/bulk_upload/lettings/year2023/row_parser.rb b/app/services/bulk_upload/lettings/year2023/row_parser.rb index 89469b040..51f772bcc 100644 --- a/app/services/bulk_upload/lettings/year2023/row_parser.rb +++ b/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 diff --git a/app/services/bulk_upload/lettings/year2024/row_parser.rb b/app/services/bulk_upload/lettings/year2024/row_parser.rb index 47ad36186..688c58bdf 100644 --- a/app/services/bulk_upload/lettings/year2024/row_parser.rb +++ b/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 diff --git a/lib/tasks/fix_nil_letting_allocation_values.rake b/lib/tasks/fix_nil_letting_allocation_values.rake new file mode 100644 index 000000000..770fc63d7 --- /dev/null +++ b/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 diff --git a/spec/lib/tasks/fix_nil_letting_allocation_values_spec.rb b/spec/lib/tasks/fix_nil_letting_allocation_values_spec.rb new file mode 100644 index 000000000..c8992a6e6 --- /dev/null +++ b/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 diff --git a/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb b/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb index 01de24879..37291cf08 100644 --- a/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb +++ b/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 diff --git a/spec/services/bulk_upload/lettings/year2024/row_parser_spec.rb b/spec/services/bulk_upload/lettings/year2024/row_parser_spec.rb index 2c4d475ee..db3bbe716 100644 --- a/spec/services/bulk_upload/lettings/year2024/row_parser_spec.rb +++ b/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