From 695f542fdcd1f6c0b593cf399e1bff34f603b3ec Mon Sep 17 00:00:00 2001 From: natdeanlewissoftwire <94526761+natdeanlewissoftwire@users.noreply.github.com> Date: Mon, 25 Mar 2024 09:11:28 +0000 Subject: [PATCH] CLDC-3327 Deduplicate null errors in bulk upload (#2338) * feat: validate nulls last in bu * feat: add tests * feat: update org errors and tests * feat: update test copy * feat: update tests * CLDC-3328 Make bulk upload errors consistent between lettings and sales (#2341) * feat: add validate_address_fields to lettings for 2024 * CLDC-3338: Add tolerance to discounted sale calculations (#2333) * Rename method * Update staircase/non staircase validations * Add errors to type * Remove validate_shared_ownership_deposit * Don't add setup BU errors, deduplicate different sale type errors * Add tolerance * Reuse method * Rename methods * Skip type error completely in BU * Update validation messages * Update over tolerance method * C:DC-3338: Add tolerance to grant calculations --------- Co-authored-by: Kat * feat: add validate_address_fields to lettings for 2024 * feat: update tests * Revert "CLDC-3338: Add tolerance to discounted sale calculations (#2333)" This reverts commit cdecdfebfefa1e3d0f61810e408bafc1df2a7401. --------- Co-authored-by: Robert Sullivan Co-authored-by: Kat --------- Co-authored-by: Robert Sullivan Co-authored-by: Kat --- .../lettings/year2024/row_parser.rb | 23 +++++++++++++++---- .../bulk_upload/sales/year2024/row_parser.rb | 7 +++--- .../lettings/year2023/row_parser_spec.rb | 2 +- .../lettings/year2024/row_parser_spec.rb | 19 +++++++++++---- .../sales/year2024/row_parser_spec.rb | 22 +++++++++++++++++- 5 files changed, 58 insertions(+), 15 deletions(-) diff --git a/app/services/bulk_upload/lettings/year2024/row_parser.rb b/app/services/bulk_upload/lettings/year2024/row_parser.rb index 0ad096c2a..2994408c8 100644 --- a/app/services/bulk_upload/lettings/year2024/row_parser.rb +++ b/app/services/bulk_upload/lettings/year2024/row_parser.rb @@ -411,13 +411,14 @@ class BulkUpload::Lettings::Year2024::RowParser validate :validate_all_charges_given, on: :after_log, if: proc { is_carehome.zero? } validate :validate_address_option_found, on: :after_log - validate :validate_nulls, on: :after_log - validate :validate_uprn_exists_if_any_key_address_fields_are_blank, on: :after_log, unless: -> { supported_housing? } + validate :validate_address_fields, on: :after_log validate :validate_incomplete_soft_validations, on: :after_log validate :validate_nationality, on: :after_log + validate :validate_nulls, on: :after_log + def self.question_for_field(field) QUESTIONS[field] end @@ -581,6 +582,18 @@ private end end + def validate_address_fields + if field_16.blank? || log.errors.attribute_names.include?(:uprn) + if field_17.blank? + errors.add(:field_17, I18n.t("validations.not_answered", question: "address line 1")) + end + + if field_19.blank? + errors.add(:field_19, I18n.t("validations.not_answered", question: "town or city")) + end + end + end + def validate_incomplete_soft_validations routed_to_soft_validation_questions = log.form.questions.filter { |q| q.type == "interruption_screen" && q.page.routed_to?(log, nil) }.compact routed_to_soft_validation_questions.each do |question| @@ -809,7 +822,7 @@ private if managing_organisation.nil? block_log_creation! - if errors[:field_2].blank? + if field_2.present? && errors[:field_2].blank? errors.add(:field_2, "The managing organisation code is incorrect", category: :setup) end end @@ -818,7 +831,7 @@ private def validate_managing_org_data_given if field_2.blank? block_log_creation! - errors.add(:field_2, "The managing organisation code is incorrect", category: :setup) + errors.add(:field_2, I18n.t("validations.not_answered", question: "managing organisation"), category: :setup) end end @@ -836,7 +849,7 @@ private if owning_organisation.nil? block_log_creation! - if errors[:field_1].blank? + if field_1.present? && errors[:field_1].blank? errors.add(:field_1, "The owning organisation code is incorrect", category: :setup) end end diff --git a/app/services/bulk_upload/sales/year2024/row_parser.rb b/app/services/bulk_upload/sales/year2024/row_parser.rb index e5f4f1d61..9b401e119 100644 --- a/app/services/bulk_upload/sales/year2024/row_parser.rb +++ b/app/services/bulk_upload/sales/year2024/row_parser.rb @@ -453,7 +453,6 @@ class BulkUpload::Sales::Year2024::RowParser validate :validate_buyer1_economic_status, on: :before_log validate :validate_address_option_found, on: :after_log validate :validate_buyer2_economic_status, on: :before_log - validate :validate_nulls, on: :after_log validate :validate_valid_radio_option, on: :before_log validate :validate_owning_org_data_given, on: :after_log @@ -474,6 +473,8 @@ class BulkUpload::Sales::Year2024::RowParser validate :validate_nationality, on: :after_log validate :validate_buyer_2_nationality, on: :after_log + validate :validate_nulls, on: :after_log + def self.question_for_field(field) QUESTIONS[field] end @@ -1259,7 +1260,7 @@ private block_log_creation! if errors[:field_1].blank? - errors.add(:field_1, "The owning organisation code is incorrect", category: :setup) + errors.add(:field_1, I18n.t("validations.not_answered", question: "owning organisation"), category: :setup) end end end @@ -1268,7 +1269,7 @@ private if owning_organisation.nil? block_log_creation! - if errors[:field_1].blank? + if field_1.present? && errors[:field_1].blank? errors.add(:field_1, "The owning organisation code is incorrect", category: :setup) 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 aba3b0670..6203b9c59 100644 --- a/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb @@ -698,7 +698,7 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do context "when non-setup questions are null" do let(:attributes) { setup_section_params.merge({ field_18: "", field_19: "", field_21: "" }) } - it "fetches the question's check_answer_label if it exists, otherwise it gets the question's header" do + it "fetches the question's check_answer_label if it exists" do parser.valid? expect(parser.errors[:field_19]).to eql(["You must answer address line 1"]) expect(parser.errors[:field_21]).to eql(["You must answer town or city"]) 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 c19dbcbc0..9549cfe12 100644 --- a/spec/services/bulk_upload/lettings/year2024/row_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/year2024/row_parser_spec.rb @@ -793,11 +793,20 @@ RSpec.describe BulkUpload::Lettings::Year2024::RowParser do context "when non-setup questions are null" do let(:attributes) { setup_section_params.merge({ field_43: "" }) } - it "fetches the question's check_answer_label if it exists, otherwise it gets the question's header" do + it "fetches the question's check_answer_label if it exists" do parser.valid? expect(parser.errors[:field_43]).to eql(["You must answer lead tenant’s gender identity"]) end end + + context "when other null error is added" do + let(:attributes) { setup_section_params.merge({ field_112: nil }) } + + it "only has one error added to the field" do + parser.valid? + expect(parser.errors[:field_112]).to eql(["You must answer was the letting made under the Choice-Based Lettings (CBL)?"]) + end + end end end @@ -1389,7 +1398,7 @@ RSpec.describe BulkUpload::Lettings::Year2024::RowParser do it "is not permitted as setup error" do setup_errors = parser.errors.select { |e| e.options[:category] == :setup } - expect(setup_errors.find { |e| e.attribute == :field_2 }.message).to eql("The managing organisation code is incorrect") + expect(setup_errors.find { |e| e.attribute == :field_2 }.message).to eql("You must answer managing organisation") end it "blocks log creation" do @@ -1486,10 +1495,10 @@ RSpec.describe BulkUpload::Lettings::Year2024::RowParser do field_1: "1" } end - it "does not add UPRN errors" do + it "does not add UPRN errors (but still adds missing address errors)" do expect(parser.errors[:field_16]).to be_empty - expect(parser.errors[:field_17]).to be_empty - expect(parser.errors[:field_19]).to be_empty + expect(parser.errors[:field_17]).to eql(["You must answer address line 1"]) + expect(parser.errors[:field_19]).to eql(["You must answer town or city"]) end end diff --git a/spec/services/bulk_upload/sales/year2024/row_parser_spec.rb b/spec/services/bulk_upload/sales/year2024/row_parser_spec.rb index d80aca533..1f3c4800c 100644 --- a/spec/services/bulk_upload/sales/year2024/row_parser_spec.rb +++ b/spec/services/bulk_upload/sales/year2024/row_parser_spec.rb @@ -282,6 +282,26 @@ RSpec.describe BulkUpload::Sales::Year2024::RowParser do expect(questions.map(&:id)).to eql([]) end end + + describe "#validate_nulls" do + context "when non-setup questions are null" do + let(:attributes) { setup_section_params.merge({ field_32: "" }) } + + it "fetches the question's check_answer_label if it exists" do + parser.valid? + expect(parser.errors[:field_32]).to eql(["You must answer buyer 1’s gender identity"]) + end + end + + context "when other null error is added" do + let(:attributes) { setup_section_params.merge({ field_23: nil }) } + + it "only has one error added to the field" do + parser.valid? + expect(parser.errors[:field_23]).to eql(["You must answer address line 1"]) + end + end + end end context "when setup section not complete and type is not given" do @@ -418,7 +438,7 @@ RSpec.describe BulkUpload::Sales::Year2024::RowParser do let(:attributes) { { bulk_upload:, field_1: "donotexist" } } it "is not permitted as a setup error" do - expect(parser.errors.where(:field_1, category: :setup).map(&:message)).to eql(["You must answer owning organisation"]) + expect(parser.errors.where(:field_1, category: :setup).map(&:message)).to eql(["The owning organisation code is incorrect"]) end it "blocks log creation" do