From feaefcdd5dad8647fe9aa1bb85a7cb722b5e35b1 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Fri, 21 Apr 2023 15:52:21 +0100 Subject: [PATCH] fix setup errors for bulk upload validation --- .../lettings/year2022/row_parser.rb | 73 +++++++++++++------ .../bulk_upload/lettings/validator_spec.rb | 2 +- .../lettings/year2022/row_parser_spec.rb | 58 +++++++-------- 3 files changed, 79 insertions(+), 54 deletions(-) diff --git a/app/services/bulk_upload/lettings/year2022/row_parser.rb b/app/services/bulk_upload/lettings/year2022/row_parser.rb index 80b65c133..13b24334b 100644 --- a/app/services/bulk_upload/lettings/year2022/row_parser.rb +++ b/app/services/bulk_upload/lettings/year2022/row_parser.rb @@ -280,9 +280,20 @@ class BulkUpload::Lettings::Year2022::RowParser attribute :field_133, :integer attribute :field_134, :integer - validates :field_1, presence: { message: I18n.t("validations.not_answered", question: "letting type") }, - inclusion: { in: (1..12).to_a, message: I18n.t("validations.invalid_option", question: "letting type") }, on: :after_log - validates :field_4, presence: { if: proc { [2, 4, 6, 8, 10, 12].include?(field_1) } }, on: :after_log + validate :validate_valid_radio_option, on: :before_log + + validates :field_1, + presence: { + message: I18n.t("validations.not_answered", question: "letting type"), + category: :setup, + }, + inclusion: { + in: (1..12).to_a, + message: I18n.t("validations.invalid_option", question: "letting type"), + category: :setup, + unless: -> { field_1.blank? }, + }, + on: :after_log validates :field_12, format: { with: /\A\d{1,3}\z|\AR\z/, message: "Age of person 1 must be a number or the letter R" }, on: :after_log validates :field_13, format: { with: /\A\d{1,3}\z|\AR\z/, message: "Age of person 2 must be a number or the letter R" }, allow_blank: true, on: :after_log @@ -293,14 +304,33 @@ class BulkUpload::Lettings::Year2022::RowParser validates :field_18, 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_19, 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_96, presence: { message: I18n.t("validations.not_answered", question: "tenancy start date (day)") }, on: :after_log - validates :field_97, presence: { message: I18n.t("validations.not_answered", question: "tenancy start date (month)") }, on: :after_log - validates :field_98, presence: { message: I18n.t("validations.not_answered", question: "tenancy start date (year)") }, on: :after_log - - validates :field_98, format: { with: /\A\d{2}\z/, message: I18n.t("validations.setup.startdate.year_not_two_digits") }, on: :after_log + validates :field_96, + presence: { + message: I18n.t("validations.not_answered", question: "tenancy start date (day)"), + category: :setup, + }, on: :after_log + + validates :field_97, + presence: { + message: I18n.t("validations.not_answered", question: "tenancy start date (month)"), + category: :setup, + }, + on: :after_log + + validates :field_98, + presence: { + message: I18n.t("validations.not_answered", question: "tenancy start date (year)"), + category: :setup, + }, + format: { + with: /\A\d{2}\z/, + message: I18n.t("validations.setup.startdate.year_not_two_digits"), + unless: -> { field_98.blank? }, + category: :setup, + }, + on: :after_log validate :validate_data_types, on: :after_log - validate :validate_nulls, on: :after_log validate :validate_relevant_collection_window, on: :after_log validate :validate_la_with_local_housing_referral, on: :after_log validate :validate_cannot_be_la_referral_if_general_needs_and_la, on: :after_log @@ -335,9 +365,10 @@ class BulkUpload::Lettings::Year2022::RowParser validate :validate_declaration_acceptance, on: :after_log - validate :validate_valid_radio_option, on: :before_log validate :validate_incomplete_soft_validations, on: :after_log + validate :validate_nulls, on: :after_log + def self.question_for_field(field) QUESTIONS[field] end @@ -481,13 +512,13 @@ private def validate_location_exists if scheme && field_5.present? && location.nil? - errors.add(:field_5, "Location could be found with provided scheme code") + errors.add(:field_5, "Location could be found with provided scheme code", category: :setup) end end def validate_location_data_given if bulk_upload.supported_housing? && field_5.blank? - errors.add(:field_5, "The scheme code must be present", category: "setup") + errors.add(:field_5, I18n.t("validations.not_answered", question: "scheme code"), category: :setup) end end @@ -499,19 +530,19 @@ private unless owned_by_owning_org || owned_by_managing_org block_log_creation! - errors.add(:field_4, "This management group code does not belong to your organisation, or any of your stock owners / managing agents") + errors.add(:field_4, "This management group code does not belong to your organisation, or any of your stock owners / managing agents", category: :setup) end end def validate_scheme_exists if field_4.present? && scheme.nil? - errors.add(:field_4, "The management group code is not correct") + errors.add(:field_4, "The management group code is not correct", category: :setup) end end def validate_scheme_data_given if bulk_upload.supported_housing? && field_4.blank? - errors.add(:field_4, "The management group code is not correct", category: "setup") + errors.add(:field_4, I18n.t("validations.not_answered", question: "management group code"), category: :setup) end end @@ -538,7 +569,7 @@ private def validate_managing_org_data_given if field_113.blank? block_log_creation! - errors.add(:field_113, "The managing organisation code is incorrect", category: :setup) + errors.add(:field_113, I18n.t("validations.not_answered", question: "managing organisation"), category: :setup) end end @@ -567,7 +598,7 @@ private block_log_creation! if errors[:field_111].blank? - errors.add(:field_111, "The owning organisation code is incorrect", category: :setup) + errors.add(:field_111, I18n.t("validations.not_answered", question: "owning organisation"), category: :setup) end end end @@ -645,9 +676,9 @@ private return if start_date.blank? || bulk_upload.form.blank? unless bulk_upload.form.valid_start_date_for_form?(start_date) - errors.add(:field_96, I18n.t("validations.date.outside_collection_window")) - errors.add(:field_97, I18n.t("validations.date.outside_collection_window")) - errors.add(:field_98, I18n.t("validations.date.outside_collection_window")) + errors.add(:field_96, I18n.t("validations.date.outside_collection_window"), category: :setup) + errors.add(:field_97, I18n.t("validations.date.outside_collection_window"), category: :setup) + errors.add(:field_98, I18n.t("validations.date.outside_collection_window"), category: :setup) end end @@ -704,7 +735,7 @@ private if setup_question?(question) fields.each do |field| - if errors[field].present? + if errors.select { |e| fields.include?(e.attribute) }.none? errors.add(field, I18n.t("validations.not_answered", question: question.check_answer_label&.downcase), category: :setup) end end diff --git a/spec/services/bulk_upload/lettings/validator_spec.rb b/spec/services/bulk_upload/lettings/validator_spec.rb index 10adcb86a..3ba36dfc0 100644 --- a/spec/services/bulk_upload/lettings/validator_spec.rb +++ b/spec/services/bulk_upload/lettings/validator_spec.rb @@ -208,7 +208,7 @@ RSpec.describe BulkUpload::Lettings::Validator do error = BulkUploadError.find_by(row: "7", field: "field_96", category: "setup") expect(error.field).to eql("field_96") - expect(error.error).to eql("You must answer tenancy start date") + expect(error.error).to eql("You must answer tenancy start date (day)") expect(error.tenant_code).to eql("123") expect(error.property_ref).to be_nil expect(error.row).to eql("7") diff --git a/spec/services/bulk_upload/lettings/year2022/row_parser_spec.rb b/spec/services/bulk_upload/lettings/year2022/row_parser_spec.rb index 25fc64cf9..e7bf44968 100644 --- a/spec/services/bulk_upload/lettings/year2022/row_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/year2022/row_parser_spec.rb @@ -303,9 +303,9 @@ RSpec.describe BulkUpload::Lettings::Year2022::RowParser do let(:attributes) { { bulk_upload:, field_7: "123" } } it "has errors on setup fields" do - errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute) + errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute).sort - expect(errors).to eql(%i[field_1 field_98 field_97 field_96 field_111 field_113 field_132]) + expect(errors).to eql(%i[field_1 field_111 field_113 field_96 field_97 field_98]) end end @@ -403,15 +403,13 @@ RSpec.describe BulkUpload::Lettings::Year2022::RowParser do end end - describe "#field_4" do + describe "#field_4" do # mangement group code context "when nullable not permitted" do let(:bulk_upload) { create(:bulk_upload, :lettings, user:, needstype: 2) } let(:attributes) { { bulk_upload:, field_1: "2", field_4: nil } } it "cannot be nulled" do - setup_errors = parser.errors.select { |e| e.options[:category] == "setup" } - - expect(setup_errors.find { |e| e.attribute == :field_4 }).to be_present + expect(parser.errors.where(:field_4, category: :setup).map(&:message)).to eql(["You must answer management group code"]) end end @@ -427,7 +425,7 @@ RSpec.describe BulkUpload::Lettings::Year2022::RowParser do let(:attributes) { { bulk_upload:, field_1: "1", field_4: "123" } } it "returns an error" do - expect(parser.errors[:field_4]).to be_present + expect(parser.errors.where(:field_4, category: :setup)).to be_present end end @@ -436,7 +434,7 @@ RSpec.describe BulkUpload::Lettings::Year2022::RowParser do let(:attributes) { { bulk_upload:, field_1: "1", field_4: other_scheme.old_visible_id, field_111: owning_org.old_visible_id } } it "returns an error" do - expect(parser.errors[:field_4]).to include("This management group code does not belong to your organisation, or any of your stock owners / managing agents") + expect(parser.errors.where(:field_4, category: :setup).map(&:message)).to include("This management group code does not belong to your organisation, or any of your stock owners / managing agents") end end @@ -459,15 +457,13 @@ RSpec.describe BulkUpload::Lettings::Year2022::RowParser do end end - describe "#field_5" do + describe "#field_5" do # scheme code context "when not nullable" do let(:bulk_upload) { create(:bulk_upload, :lettings, user:, needstype: 2) } let(:attributes) { { bulk_upload:, field_1: "2", field_5: nil } } it "cannot be nulled" do - setup_errors = parser.errors.select { |e| e.options[:category] == "setup" } - - expect(setup_errors.find { |e| e.attribute == :field_5 }).to be_present + expect(parser.errors.where(:field_5, category: :setup).map(&:message)).to eql(["You must answer scheme code"]) end end @@ -517,8 +513,8 @@ RSpec.describe BulkUpload::Lettings::Year2022::RowParser do } end - it "returns an error" do - expect(parser.errors[:field_5]).to be_present + it "returns as setup error" do + expect(parser.errors.where(:field_5, category: :setup).map(&:message)).to eql(["Location could be found with provided scheme code"]) end end end @@ -715,21 +711,21 @@ RSpec.describe BulkUpload::Lettings::Year2022::RowParser do let(:attributes) { { bulk_upload:, field_1: "1", field_96: nil, field_97: nil, field_98: nil } } it "returns them as setup errors" do - setup_errors = parser.errors.select { |e| e.options[:category] == :setup } - - expect(setup_errors.find { |e| e.attribute == :field_96 }).to be_present - expect(setup_errors.find { |e| e.attribute == :field_97 }).to be_present - expect(setup_errors.find { |e| e.attribute == :field_98 }).to be_present + expect(parser.errors.where(:field_96, category: :setup)).to be_present + expect(parser.errors.where(:field_97, category: :setup)).to be_present + expect(parser.errors.where(:field_98, category: :setup)).to be_present end end - context "when one of these fields is blank" do + context "when one of these fields is blank", aggregate_failures: true do let(:attributes) { { bulk_upload:, field_1: "1", field_96: "1", field_97: "1", field_98: nil } } it "returns an error only on blank field" do - expect(parser.errors[:field_96]).to be_blank - expect(parser.errors[:field_97]).to be_blank - expect(parser.errors[:field_98]).to be_present + expect(parser.errors.where(:field_96, category: :setup)).to be_blank + expect(parser.errors.where(:field_97, category: :setup)).to be_blank + + expect(parser.errors.where(:field_98).map(&:message)).to eql(["You must answer tenancy start date (year)"]) + expect(parser.errors.where(:field_98, category: :setup).map(&:message)).to eql(["You must answer tenancy start date (year)"]) end end @@ -737,7 +733,7 @@ RSpec.describe BulkUpload::Lettings::Year2022::RowParser do let(:attributes) { { bulk_upload:, field_98: "2022" } } it "returns an error" do - expect(parser.errors[:field_98]).to include("Tenancy start year must be 2 digits") + expect(parser.errors.where(:field_98, category: :setup).map(&:message)).to include("Tenancy start year must be 2 digits") end end @@ -774,9 +770,9 @@ RSpec.describe BulkUpload::Lettings::Year2022::RowParser do let(:bulk_upload) { create(:bulk_upload, :lettings, user:, year: 2022) } it "returns errors" do - expect(parser.errors[:field_96]).to be_present - expect(parser.errors[:field_97]).to be_present - expect(parser.errors[:field_98]).to be_present + expect(parser.errors.where(:field_96, category: :setup)).to be_present + expect(parser.errors.where(:field_97, category: :setup)).to be_present + expect(parser.errors.where(:field_98, category: :setup)).to be_present end end end @@ -788,7 +784,7 @@ RSpec.describe BulkUpload::Lettings::Year2022::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_111 }.message).to eql("The owning organisation code is incorrect") + expect(setup_errors.find { |e| e.attribute == :field_111 }.message).to eql("You must answer owning organisation") end it "blocks log creation" do @@ -897,12 +893,10 @@ RSpec.describe BulkUpload::Lettings::Year2022::RowParser do describe "#field_113" do # managing org context "when blank" do - let(:attributes) { { bulk_upload:, field_113: "", field_4: 1 } } + let(:attributes) { setup_section_params.merge(field_113: nil) } 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_113 }.message).to eql("The managing organisation code is incorrect") + expect(parser.errors.where(:field_113, category: :setup).map(&:message)).to eql(["You must answer managing organisation"]) end it "blocks log creation" do