From 1053607ed84b5ea79f7358bba40f2adf62be3885 Mon Sep 17 00:00:00 2001
From: Phil Lee <asmega@users.noreply.github.com>
Date: Wed, 10 May 2023 10:00:39 +0100
Subject: [PATCH] CLDC-2287 Bulk upload setup errors (#1585)

# Context

- https://digital.dclg.gov.uk/jira/browse/CLDC-2287
- bulk upload errors on setup fields are not being categorised as setup errors

# Changes

- ensure errors on setup field are categorised as `setup` errors
- ordering of validations tweaked, so `validate_nulls` is further down the execution order. this is so any existing validation run first and `validate_nulls` only adds errors if there aren't any existing errors on a field
- removed old 16/60% tests are no longer required due to introduction of `how to fix` journey
---
 .../lettings/year2022/row_parser.rb           |  79 ++++++++++----
 .../lettings/year2023/row_parser.rb           | 100 +++++++++++++-----
 .../bulk_upload/sales/year2022/row_parser.rb  |   6 +-
 .../bulk_upload/lettings/validator_spec.rb    |   2 +-
 .../lettings/year2022/row_parser_spec.rb      |  68 ++++++------
 .../lettings/year2023/row_parser_spec.rb      |  57 +++++-----
 .../sales/year2022/row_parser_spec.rb         |   9 +-
 7 files changed, 200 insertions(+), 121 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..62cf9e872 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
@@ -424,7 +455,11 @@ private
       fields = field_mapping_for_errors[question_id.to_sym] || []
 
       fields.each do |field|
-        errors.add(field, I18n.t("validations.invalid_option", question: QUESTIONS[field]))
+        if setup_question?(question)
+          errors.add(field, I18n.t("validations.invalid_option", question: QUESTIONS[field]), category: :setup)
+        else
+          errors.add(field, I18n.t("validations.invalid_option", question: QUESTIONS[field]))
+        end
       end
     end
   end
@@ -481,13 +516,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 +534,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 +573,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 +602,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 +680,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 +739,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/app/services/bulk_upload/lettings/year2023/row_parser.rb b/app/services/bulk_upload/lettings/year2023/row_parser.rb
index 46ff0ff12..9dc5b60de 100644
--- a/app/services/bulk_upload/lettings/year2023/row_parser.rb
+++ b/app/services/bulk_upload/lettings/year2023/row_parser.rb
@@ -280,9 +280,61 @@ class BulkUpload::Lettings::Year2023::RowParser
   attribute :field_133, :integer
   attribute :field_134, :decimal
 
-  validates :field_5, 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_16, presence: { if: proc { [2, 4, 6, 8, 10, 12].include?(field_5) } }, on: :after_log
+  validate :validate_valid_radio_option, on: :before_log
+
+  validates :field_5,
+            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"),
+              unless: -> { field_5.blank? },
+              category: :setup,
+            },
+            on: :after_log
+
+  validates :field_6,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "property renewal"),
+              category: :setup,
+            },
+            on: :after_log
+
+  validates :field_7,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "tenancy start date (day)"),
+              category: :setup,
+            },
+            on: :after_log
+
+  validates :field_8,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "tenancy start date (month)"),
+              category: :setup,
+            },
+            on: :after_log
+
+  validates :field_9,
+            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"),
+              category: :setup,
+              unless: -> { field_9.blank? },
+            },
+            on: :after_log
+
+  validates :field_16,
+            presence: {
+              if: proc { [2, 4, 6, 8, 10, 12].include?(field_5) },
+              category: :setup,
+            },
+            on: :after_log
 
   validates :field_46, 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_52, 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,16 +345,8 @@ 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
-  validates :field_9, presence: { message: I18n.t("validations.not_answered", question: "tenancy start date (year)") }, on: :after_log
-
-  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
   validate :validate_la_with_local_housing_referral, on: :after_log
   validate :validate_cannot_be_la_referral_if_general_needs_and_la, on: :after_log
@@ -336,7 +380,7 @@ class BulkUpload::Lettings::Year2023::RowParser
 
   validate :validate_declaration_acceptance, on: :after_log
 
-  validate :validate_valid_radio_option, on: :before_log
+  validate :validate_nulls, on: :after_log
 
   validate :validate_uprn_exists_if_any_key_adddress_fields_are_blank, on: :after_log
 
@@ -428,7 +472,11 @@ private
       fields = field_mapping_for_errors[question_id.to_sym] || []
 
       fields.each do |field|
-        errors.add(field, I18n.t("validations.invalid_option", question: QUESTIONS[field]))
+        if setup_question?(question)
+          errors.add(field, I18n.t("validations.invalid_option", question: QUESTIONS[field]), category: :setup)
+        else
+          errors.add(field, I18n.t("validations.invalid_option", question: QUESTIONS[field]))
+        end
       end
     end
   end
@@ -493,7 +541,7 @@ private
 
   def validate_needs_type_present
     if field_4.blank?
-      errors.add(:field_4, I18n.t("validations.not_answered", question: "needs type"))
+      errors.add(:field_4, I18n.t("validations.not_answered", question: "needs type"), category: :setup)
     end
   end
 
@@ -576,9 +624,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_7, I18n.t("validations.date.outside_collection_window"))
-      errors.add(:field_8, I18n.t("validations.date.outside_collection_window"))
-      errors.add(:field_9, I18n.t("validations.date.outside_collection_window"))
+      errors.add(:field_7, I18n.t("validations.date.outside_collection_window"), category: :setup)
+      errors.add(:field_8, I18n.t("validations.date.outside_collection_window"), category: :setup)
+      errors.add(:field_9, I18n.t("validations.date.outside_collection_window"), category: :setup)
     end
   end
 
@@ -599,7 +647,7 @@ private
 
       if setup_question?(question)
         fields.each do |field|
-          if errors[field].present?
+          if errors.select { |e| fields.include?(e.attribute) }.none?
             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
@@ -620,19 +668,19 @@ private
 
     unless location.scheme == scheme
       block_log_creation!
-      errors.add(:field_17, "Scheme code must relate to a location that is owned by owning organisation or managing organisation")
+      errors.add(:field_17, "Scheme code must relate to a location that is owned by owning organisation or managing organisation", category: :setup)
     end
   end
 
   def validate_location_exists
     if scheme && field_17.present? && location.nil?
-      errors.add(:field_17, "Location could be found with provided scheme code")
+      errors.add(:field_17, "Location could be found with provided scheme code", category: :setup)
     end
   end
 
   def validate_location_data_given
     if supported_housing? && field_17.blank?
-      errors.add(:field_17, "The scheme code must be present", category: "setup")
+      errors.add(:field_17, I18n.t("validations.not_answered", question: "scheme code"), category: "setup")
     end
   end
 
@@ -644,19 +692,19 @@ private
 
     unless owned_by_owning_org || owned_by_managing_org
       block_log_creation!
-      errors.add(:field_16, "This management group code does not belong to your organisation, or any of your stock owners / managing agents")
+      errors.add(:field_16, "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_16.present? && scheme.nil?
-      errors.add(:field_16, "The management group code is not correct")
+      errors.add(:field_16, "The management group code is not correct", category: :setup)
     end
   end
 
   def validate_scheme_data_given
     if supported_housing? && field_16.blank?
-      errors.add(:field_16, "The management group code is not correct", category: "setup")
+      errors.add(:field_16, I18n.t("validations.not_answered", question: "management group code"), category: "setup")
     end
   end
 
@@ -710,7 +758,7 @@ private
   def validate_owning_org_data_given
     if field_1.blank?
       block_log_creation!
-      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
 
diff --git a/app/services/bulk_upload/sales/year2022/row_parser.rb b/app/services/bulk_upload/sales/year2022/row_parser.rb
index e53b42ecb..4383898fb 100644
--- a/app/services/bulk_upload/sales/year2022/row_parser.rb
+++ b/app/services/bulk_upload/sales/year2022/row_parser.rb
@@ -941,9 +941,9 @@ private
     return if saledate.blank? || bulk_upload.form.blank?
 
     unless bulk_upload.form.valid_start_date_for_form?(saledate)
-      errors.add(:field_2, I18n.t("validations.date.outside_collection_window"))
-      errors.add(:field_3, I18n.t("validations.date.outside_collection_window"))
-      errors.add(:field_4, I18n.t("validations.date.outside_collection_window"))
+      errors.add(:field_2, I18n.t("validations.date.outside_collection_window"), category: :setup)
+      errors.add(:field_3, I18n.t("validations.date.outside_collection_window"), category: :setup)
+      errors.add(:field_4, I18n.t("validations.date.outside_collection_window"), 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..99adb049a 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_132 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
@@ -955,6 +949,16 @@ RSpec.describe BulkUpload::Lettings::Year2022::RowParser do
       end
     end
 
+    describe "#field_134" do # renewal
+      context "when none possible option selected" do
+        let(:attributes) { setup_section_params.merge({ field_134: "101" }) }
+
+        it "adds a setup error" do
+          expect(parser.errors.where(:field_134, category: :setup).map(&:message)).to include("Enter a valid value for Is this letting a renewal?")
+        end
+      end
+    end
+
     describe "soft validations" do
       context "when soft validation is triggered" do
         let(:attributes) { setup_section_params.merge({ field_12: 22, field_35: 5 }) }
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 f98eed6d0..ef9935b44 100644
--- a/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb
+++ b/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb
@@ -501,8 +501,8 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
       context "when matching scheme cannot be found" do
         let(:attributes) { { bulk_upload:, field_5: "1", field_16: "123" } }
 
-        it "returns an error" do
-          expect(parser.errors[:field_16]).to be_present
+        it "returns a setup error" do
+          expect(parser.errors.where(:field_16, category: :setup)).to be_present
         end
       end
 
@@ -510,8 +510,8 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
         let(:other_scheme) { create(:scheme, :with_old_visible_id) }
         let(:attributes) { { bulk_upload:, field_5: "1", field_16: other_scheme.old_visible_id, field_1: owning_org.old_visible_id } }
 
-        it "returns an error" do
-          expect(parser.errors[:field_16]).to include("This management group code does not belong to your organisation, or any of your stock owners / managing agents")
+        it "returns a setup error" do
+          expect(parser.errors.where(:field_16, 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
 
@@ -547,8 +547,8 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
           }
         end
 
-        it "returns an error" do
-          expect(parser.errors[:field_17]).to be_present
+        it "returns a setup error" do
+          expect(parser.errors.where(:field_17, category: :setup)).to be_present
         end
       end
 
@@ -581,8 +581,8 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
           }
         end
 
-        it "returns an error" do
-          expect(parser.errors[:field_17]).to be_present
+        it "returns a setup error" do
+          expect(parser.errors.where(:field_17, category: :setup)).to be_present
         end
       end
     end
@@ -717,8 +717,6 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
         let(:attributes) { { bulk_upload:, field_5: "1", field_7: nil, field_8: nil, field_9: nil } }
 
         it "returns an error" do
-          parser.valid?
-
           expect(parser.errors[:field_7]).to be_present
           expect(parser.errors[:field_8]).to be_present
           expect(parser.errors[:field_9]).to be_present
@@ -729,8 +727,6 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
         let(:attributes) { { bulk_upload:, field_9: "2022" } }
 
         it "returns an error" do
-          parser.valid?
-
           expect(parser.errors[:field_9]).to include("Tenancy start year must be 2 digits")
         end
       end
@@ -749,8 +745,6 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
         let(:bulk_upload) { create(:bulk_upload, :lettings, user:, year: 2022) }
 
         it "does not return errors" do
-          parser.valid?
-
           expect(parser.errors[:field_7]).not_to be_present
           expect(parser.errors[:field_8]).not_to be_present
           expect(parser.errors[:field_9]).not_to be_present
@@ -762,19 +756,16 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
           Timecop.freeze(Date.new(2022, 4, 2)) do
             example.run
           end
-          Timecop.return
         end
 
         let(:attributes) { { bulk_upload:, field_7: "1", field_8: "1", field_9: "22" } }
 
         let(:bulk_upload) { create(:bulk_upload, :lettings, user:, year: 2022) }
 
-        it "returns errors" do
-          parser.valid?
-
-          expect(parser.errors[:field_7]).to be_present
-          expect(parser.errors[:field_8]).to be_present
-          expect(parser.errors[:field_9]).to be_present
+        it "returns setup errors" do
+          expect(parser.errors.where(:field_7, category: :setup)).to be_present
+          expect(parser.errors.where(:field_8, category: :setup)).to be_present
+          expect(parser.errors.where(:field_9, category: :setup)).to be_present
         end
       end
     end
@@ -784,9 +775,7 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
         let(:attributes) { { bulk_upload:, field_1: "", field_4: 1 } }
 
         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_1 }.message).to eql("The owning organisation code is incorrect")
+          expect(parser.errors.where(:field_1, category: :setup).map(&:message)).to eql(["You must answer owning organisation"])
         end
 
         it "blocks log creation" do
@@ -887,16 +876,12 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
       end
     end
 
-    describe "#field_4" do
+    describe "#field_4" do # needs type
       context "when blank" do
         let(:attributes) { { bulk_upload:, field_4: nil, field_13: "123" } }
 
         it "is reported as a setup error" do
-          errors = parser.errors.select { |e| e.options[:category] == :setup }
-          error = errors.find { |e| e.attribute == :field_4 }
-
-          expect(error).to be_present
-          expect(error.type).to eql("You must answer needs type")
+          expect(parser.errors.where(:field_4, category: :setup).map(&:message)).to eql(["You must answer needs type"])
         end
       end
     end
@@ -905,8 +890,16 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
       context "when blank" do
         let(:attributes) { { bulk_upload:, field_1: owning_org.old_visible_id, field_6: "" } }
 
-        it "has errors on the field" do
-          expect(parser.errors[:field_6]).to include("You must answer property renewal")
+        it "has setup errors on the field" do
+          expect(parser.errors.where(:field_6, category: :setup).map(&:message)).to eql(["You must answer property renewal"])
+        end
+      end
+
+      context "when none possible option selected" do
+        let(:attributes) { setup_section_params.merge({ field_6: "101" }) }
+
+        it "adds a setup error" do
+          expect(parser.errors.where(:field_6, category: :setup).map(&:message)).to include("Enter a valid value for Is this letting a renewal?")
         end
       end
     end
diff --git a/spec/services/bulk_upload/sales/year2022/row_parser_spec.rb b/spec/services/bulk_upload/sales/year2022/row_parser_spec.rb
index 8c0a4cd45..7fdf8da4d 100644
--- a/spec/services/bulk_upload/sales/year2022/row_parser_spec.rb
+++ b/spec/services/bulk_upload/sales/year2022/row_parser_spec.rb
@@ -506,17 +506,16 @@ RSpec.describe BulkUpload::Sales::Year2022::RowParser do
           Timecop.freeze(Date.new(2022, 4, 2)) do
             example.run
           end
-          Timecop.return
         end
 
         let(:attributes) { setup_section_params.merge({ field_2: "1", field_3: "1", field_4: "22" }) }
 
         let(:bulk_upload) { create(:bulk_upload, :sales, user:, year: 2022) }
 
-        it "returns errors" do
-          expect(parser.errors[:field_2]).to be_present
-          expect(parser.errors[:field_3]).to be_present
-          expect(parser.errors[:field_4]).to be_present
+        it "returns setup errors" do
+          expect(parser.errors.where(:field_2, category: :setup)).to be_present
+          expect(parser.errors.where(:field_3, category: :setup)).to be_present
+          expect(parser.errors.where(:field_4, category: :setup)).to be_present
         end
       end
     end