fix setup errors for bulk upload validation

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

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")

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