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 <katrina@kosiak.co.uk>

* 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 cdecdfebfe.

---------

Co-authored-by: Robert Sullivan <Robert.Sullivan@Softwire.com>
Co-authored-by: Kat <katrina@kosiak.co.uk>

---------

Co-authored-by: Robert Sullivan <Robert.Sullivan@Softwire.com>
Co-authored-by: Kat <katrina@kosiak.co.uk>

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

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

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

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_17]).to eql(["You must answer address line 1"])
-          expect(parser.errors[:field_19]).to be_empty
+          expect(parser.errors[:field_19]).to eql(["You must answer town or city"])
         end
       end
 

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