From 8e1a4e08b2fe682461b56ec8409d5554ff1c204b Mon Sep 17 00:00:00 2001 From: SamSeed-Softwire <63662292+SamSeed-Softwire@users.noreply.github.com> Date: Tue, 25 Apr 2023 10:23:20 +0100 Subject: [PATCH] CLDC-2226 Update UPRN validations in bulk upload (#1524) * feat: add validation to check UPRN exists if address doesn't * test: add tests for UPRN/address fields * test: make UPRN<=12 chars test more specific * feat: show qu header on BU error template if check_answer_label missing * test: that validate_nulls gets header if check_answer_label is missing * fix: drop "known" from "You must answer UPRN known" * fix: update uprn validation logic * test: make UPRN being missing explicit * test: improve test descriptions * test: that errors added to address fields too when uprn & address fields all null * refactor: standardise check for presence of needs type * test: attempt to stub setup question to have nil check_answer_label * Revert "test: attempt to stub setup question to have nil check_answer_label" This reverts commit f5b3f6179b50f2adf53fcb3790083b4655cdf178. * test: put test within context block for clarity * test: fix typo in test description * test: ensure one other non-blank field exists in row * feat: output "this question" if qu has no header or check_answer_label * chore: lint * fix: add .presence to question.header and remove & before .downcase * fix: add :after_log context to uprn validation --- .../lettings/year2023/row_parser.rb | 16 +++-- .../lettings/year2023/row_parser_spec.rb | 58 ++++++++++++++++++- 2 files changed, 66 insertions(+), 8 deletions(-) diff --git a/app/services/bulk_upload/lettings/year2023/row_parser.rb b/app/services/bulk_upload/lettings/year2023/row_parser.rb index 84f30cc7b..c36994022 100644 --- a/app/services/bulk_upload/lettings/year2023/row_parser.rb +++ b/app/services/bulk_upload/lettings/year2023/row_parser.rb @@ -292,6 +292,7 @@ class BulkUpload::Lettings::Year2023::RowParser validates :field_72, format: { with: /\A\d{1,3}\z|\AR\z/, message: "Age of person 7 must be a number or the letter R" }, allow_blank: true, on: :after_log validates :field_76, format: { with: /\A\d{1,3}\z|\AR\z/, message: "Age of person 8 must be a number or the letter R" }, allow_blank: true, on: :after_log + validates :field_4, presence: { message: I18n.t("validations.not_answered", question: "needs type") }, on: :after_log validates :field_6, presence: { message: I18n.t("validations.not_answered", question: "property renewal") }, on: :after_log validates :field_7, presence: { message: I18n.t("validations.not_answered", question: "tenancy start date (day)") }, on: :after_log validates :field_8, presence: { message: I18n.t("validations.not_answered", question: "tenancy start date (month)") }, on: :after_log @@ -299,7 +300,6 @@ class BulkUpload::Lettings::Year2023::RowParser validates :field_9, format: { with: /\A\d{2}\z/, message: I18n.t("validations.setup.startdate.year_not_two_digits") }, on: :after_log - validate :validate_needs_type_present, on: :after_log validate :validate_data_types, on: :after_log validate :validate_nulls, on: :after_log validate :validate_relevant_collection_window, on: :after_log @@ -334,6 +334,8 @@ class BulkUpload::Lettings::Year2023::RowParser validate :validate_valid_radio_option, on: :before_log + validate :validate_uprn_exists_if_any_key_adddress_fields_are_blank, on: :after_log + def self.question_for_field(field) QUESTIONS[field] end @@ -431,9 +433,9 @@ private @created_by ||= User.find_by(email: field_3) end - def validate_needs_type_present - if field_4.blank? - errors.add(:field_4, I18n.t("validations.not_answered", question: "needs type")) + def validate_uprn_exists_if_any_key_adddress_fields_are_blank + if field_18.blank? && (field_19.blank? || field_21.blank?) + errors.add(:field_18, I18n.t("validations.not_answered", question: "UPRN")) end end @@ -540,13 +542,15 @@ private if setup_question?(question) fields.each do |field| if errors[field].present? - errors.add(field, I18n.t("validations.not_answered", question: question.check_answer_label&.downcase), category: :setup) + question_text = question.check_answer_label.presence || question.header.presence || "this question" + errors.add(field, I18n.t("validations.not_answered", question: question_text.downcase), category: :setup) end end else fields.each do |field| unless errors.any? { |e| fields.include?(e.attribute) } - errors.add(field, I18n.t("validations.not_answered", question: question.check_answer_label&.downcase)) + question_text = question.check_answer_label.presence || question.header.presence || "this question" + errors.add(field, I18n.t("validations.not_answered", question: question_text.downcase)) 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 a94eaac64..d6a1d6e80 100644 --- a/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb @@ -236,6 +236,18 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do expect(questions.map(&:id)).to eql([]) end end + + describe "#validate_nulls" do + context "when non-setup questions are null" do + let(:attributes) { { bulk_upload:, field_1: "a", 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 + parser.valid? + expect(parser.errors[:field_19]).to eql(["You must answer q12 - address"]) + expect(parser.errors[:field_21]).to eql(["You must answer town or city"]) + end + end + end end context "when setup section not complete" do @@ -806,8 +818,50 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do context "when over 12 characters" do let(:attributes) { { bulk_upload:, field_18: "1234567890123" } } - it "has errors on the field" do - expect(parser.errors[:field_18]).to be_present + it "adds an appropriate error" do + expect(parser.errors[:field_18]).to eql(["UPRN must be 12 digits or less"]) + end + end + + context "when neither UPRN nor address fields are given" do + let(:attributes) do + { + bulk_upload:, + field_1: "1", + } + end + + it "adds appropriate errors" do + expect(parser.errors[:field_18]).to eql(["You must answer UPRN"]) + expect(parser.errors[:field_19]).to eql(["You must answer q12 - address"]) + expect(parser.errors[:field_21]).to eql(["You must answer town or city"]) + end + end + + context "when UPRN is given but address fields are not" do + let(:attributes) do + { + bulk_upload:, + field_18: "123456789012", + } + end + + it "doesn't add an error" do + expect(parser.errors[:field_18]).to be_empty + end + end + + context "when address is given but UPRN is not" do + let(:attributes) do + { + bulk_upload:, + field_19: "1 Example Rd", + field_21: "Example Town/City", + } + end + + it "doesn't add an error" do + expect(parser.errors[:field_18]).to be_empty end end end