CLDC-2001 Bulk upload stacked errors (#1377)

* limit setup errors if errors already present

* bulk upload beter handles age validations

- limit the number of errors on a field. if the field already has an
  existing error it does not add further errors to the field
- shim in extra validation for ages which take precedent over existing
  log level validations which are lacking

* setup errors always added

- no longer observes if the grouping already has errors
- as we have fine grain control in this class of how errors should be

* fix missing pass through for renewal

app/services/bulk_upload/lettings/row_parser.rb

@@ -143,6 +143,20 @@ class BulkUpload::Lettings::RowParser
   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") }
   validates :field_4, presence: { if: proc { [2, 4, 6, 8, 10, 12].include?(field_1) } }
+
+  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" }
+  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" }
+  validates :field_14, format: { with: /\A\d{1,3}\z|\AR\z/, message: "Age of person 3 must be a number or the letter R" }
+  validates :field_15, format: { with: /\A\d{1,3}\z|\AR\z/, message: "Age of person 4 must be a number or the letter R" }
+  validates :field_16, format: { with: /\A\d{1,3}\z|\AR\z/, message: "Age of person 5 must be a number or the letter R" }
+  validates :field_17, format: { with: /\A\d{1,3}\z|\AR\z/, message: "Age of person 6 must be a number or the letter R" }
+  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" }
+  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" }
+
+  validates :field_96, presence: { message: I18n.t("validations.not_answered", question: "tenancy start date (day)") }
+  validates :field_97, presence: { message: I18n.t("validations.not_answered", question: "tenancy start date (month)") }
+  validates :field_98, presence: { message: I18n.t("validations.not_answered", question: "tenancy start date (year)") }
+
   validates :field_98, format: { with: /\A\d{2}\z/, message: I18n.t("validations.setup.startdate.year_not_two_digits") }
 
   validate :validate_data_types
@@ -181,7 +195,12 @@ class BulkUpload::Lettings::RowParser
 
     log.errors.each do |error|
       fields = field_mapping_for_errors[error.attribute] || []
-      fields.each { |field| errors.add(field, error.type) }
+
+      fields.each do |field|
+        unless errors.include?(field)
+          errors.add(field, error.type)
+        end
+      end
     end
 
     errors.blank?
@@ -341,7 +360,7 @@ private
   end
 
   def validate_relevant_collection_window
-    return unless start_date && bulk_upload.form
+    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"))
@@ -351,6 +370,8 @@ private
   end
 
   def start_date
+    return if field_98.blank? || field_97.blank? || field_96.blank?
+
     Date.parse("20#{field_98.to_s.rjust(2, '0')}-#{field_97}-#{field_96}")
   rescue StandardError
     nil
@@ -392,9 +413,17 @@ private
       next if question.completed?(log)
 
       if setup_question?(question)
-        fields.each { |field| errors.add(field, I18n.t("validations.not_answered", question: question.check_answer_label&.downcase), category: :setup) }
+        fields.each do |field|
+          if errors[field].present?
+            errors.add(field, I18n.t("validations.not_answered", question: question.check_answer_label&.downcase), category: :setup)
+          end
+        end
       else
-        fields.each { |field| errors.add(field, I18n.t("validations.not_answered", question: question.check_answer_label&.downcase)) }
+        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))
+          end
+        end
       end
     end
   end
@@ -642,28 +671,28 @@ private
     attributes["declaration"] = field_132
 
     attributes["age1_known"] = field_12 == "R" ? 1 : 0
-    attributes["age1"] = field_12 if attributes["age1_known"].zero?
+    attributes["age1"] = field_12 if attributes["age1_known"].zero? && field_12&.match(/\A\d{1,3}\z|\AR\z/)
 
     attributes["age2_known"] = field_13 == "R" ? 1 : 0
-    attributes["age2"] = field_13 if attributes["age2_known"].zero?
+    attributes["age2"] = field_13 if attributes["age2_known"].zero? && field_13&.match(/\A\d{1,3}\z|\AR\z/)
 
     attributes["age3_known"] = field_14 == "R" ? 1 : 0
-    attributes["age3"] = field_14 if attributes["age3_known"].zero?
+    attributes["age3"] = field_14 if attributes["age3_known"].zero? && field_14&.match(/\A\d{1,3}\z|\AR\z/)
 
     attributes["age4_known"] = field_15 == "R" ? 1 : 0
-    attributes["age4"] = field_15 if attributes["age4_known"].zero?
+    attributes["age4"] = field_15 if attributes["age4_known"].zero? && field_15&.match(/\A\d{1,3}\z|\AR\z/)
 
     attributes["age5_known"] = field_16 == "R" ? 1 : 0
-    attributes["age5"] = field_16 if attributes["age5_known"].zero?
+    attributes["age5"] = field_16 if attributes["age5_known"].zero? && field_16&.match(/\A\d{1,3}\z|\AR\z/)
 
     attributes["age6_known"] = field_17 == "R" ? 1 : 0
-    attributes["age6"] = field_17 if attributes["age6_known"].zero?
+    attributes["age6"] = field_17 if attributes["age6_known"].zero? && field_17&.match(/\A\d{1,3}\z|\AR\z/)
 
     attributes["age7_known"] = field_18 == "R" ? 1 : 0
-    attributes["age7"] = field_18 if attributes["age7_known"].zero?
+    attributes["age7"] = field_18 if attributes["age7_known"].zero? && field_18&.match(/\A\d{1,3}\z|\AR\z/)
 
     attributes["age8_known"] = field_19 == "R" ? 1 : 0
-    attributes["age8"] = field_19 if attributes["age8_known"].zero?
+    attributes["age8"] = field_19 if attributes["age8_known"].zero? && field_19&.match(/\A\d{1,3}\z|\AR\z/)
 
     attributes["sex1"] = field_20
     attributes["sex2"] = field_21
@@ -882,6 +911,8 @@ private
       0
     when nil
       rsnvac == 14 ? 1 : 0
+    else
+      field_134
     end
   end
 

spec/services/bulk_upload/lettings/row_parser_spec.rb

@@ -27,6 +27,118 @@ RSpec.describe BulkUpload::Lettings::RowParser do
     }
   end
 
+  let(:valid_attributes) do
+    {
+      bulk_upload:,
+      field_1: "1",
+      field_4: scheme.old_visible_id,
+      field_7: "123",
+      field_96: now.day.to_s,
+      field_97: now.month.to_s,
+      field_98: now.strftime("%g"),
+      field_108: "EC1N",
+      field_109: "2TD",
+      field_111: owning_org.old_visible_id,
+      field_113: managing_org.old_visible_id,
+      field_130: "1",
+      field_134: "2",
+      field_102: "2",
+      field_103: "1",
+      field_104: "1",
+      field_101: "1",
+      field_133: "2",
+      field_8: "1",
+      field_9: "2",
+      field_132: "1",
+
+      field_12: "42",
+      field_13: "41",
+      field_14: "20",
+      field_15: "18",
+      field_16: "16",
+      field_17: "14",
+      field_18: "12",
+      field_19: "20",
+
+      field_20: "F",
+      field_21: "M",
+      field_22: "F",
+      field_23: "M",
+      field_24: "F",
+      field_25: "M",
+      field_26: "F",
+      field_27: "M",
+
+      field_43: "17",
+      field_44: "18",
+
+      field_28: "P",
+      field_29: "C",
+      field_30: "X",
+      field_31: "R",
+      field_32: "C",
+      field_33: "C",
+      field_34: "X",
+
+      field_35: "1",
+      field_36: "2",
+      field_37: "6",
+      field_38: "7",
+      field_39: "8",
+      field_40: "9",
+      field_41: "0",
+      field_42: "10",
+
+      field_45: "1",
+      field_114: "4",
+      field_46: "1",
+
+      field_47: "1",
+
+      field_118: "2",
+
+      field_66: "5",
+      field_67: "2",
+      field_52: "31",
+      field_61: "3",
+      field_68: "12",
+
+      field_65: "1",
+      field_63: "EC1N",
+      field_64: "2TD",
+
+      field_69: "1",
+      field_70: "1",
+      field_71: "",
+      field_72: "1",
+      field_73: "",
+      field_74: "",
+
+      field_75: "1",
+      field_76: "2",
+      field_77: "2",
+
+      field_78: "2",
+
+      field_51: "1",
+      field_50: "2000",
+      field_116: "2",
+      field_48: "1",
+      field_49: "1",
+
+      field_79: "4",
+      field_80: "1234.56",
+      field_87: "1",
+      field_88: "234.56",
+
+      field_106: "15",
+      field_99: "0",
+      field_89: now.day.to_s,
+      field_90: now.month.to_s,
+      field_91: now.strftime("%g"),
+    }
+  end
+
   before do
     create(:organisation_relationship, parent_organisation: owning_org, child_organisation: managing_org)
   end
@@ -83,117 +195,7 @@ RSpec.describe BulkUpload::Lettings::RowParser do
       end
 
       context "when valid row" do
-        let(:attributes) do
+        let(:attributes) { valid_attributes }
-          {
-            bulk_upload:,
-            field_1: "1",
-            field_4: scheme.old_visible_id,
-            field_7: "123",
-            field_96: now.day.to_s,
-            field_97: now.month.to_s,
-            field_98: now.strftime("%g"),
-            field_108: "EC1N",
-            field_109: "2TD",
-            field_111: owning_org.old_visible_id,
-            field_113: managing_org.old_visible_id,
-            field_130: "1",
-            field_134: "2",
-            field_102: "2",
-            field_103: "1",
-            field_104: "1",
-            field_101: "1",
-            field_133: "2",
-            field_8: "1",
-            field_9: "2",
-            field_132: "1",
-
-            field_12: "42",
-            field_13: "41",
-            field_14: "20",
-            field_15: "18",
-            field_16: "16",
-            field_17: "14",
-            field_18: "12",
-            field_19: "20",
-
-            field_20: "F",
-            field_21: "M",
-            field_22: "F",
-            field_23: "M",
-            field_24: "F",
-            field_25: "M",
-            field_26: "F",
-            field_27: "M",
-
-            field_43: "17",
-            field_44: "18",
-
-            field_28: "P",
-            field_29: "C",
-            field_30: "X",
-            field_31: "R",
-            field_32: "C",
-            field_33: "C",
-            field_34: "X",
-
-            field_35: "1",
-            field_36: "2",
-            field_37: "6",
-            field_38: "7",
-            field_39: "8",
-            field_40: "9",
-            field_41: "0",
-            field_42: "10",
-
-            field_45: "1",
-            field_114: "4",
-            field_46: "1",
-
-            field_47: "1",
-
-            field_118: "2",
-
-            field_66: "5",
-            field_67: "2",
-            field_52: "31",
-            field_61: "3",
-            field_68: "12",
-
-            field_65: "1",
-            field_63: "EC1N",
-            field_64: "2TD",
-
-            field_69: "1",
-            field_70: "1",
-            field_71: "",
-            field_72: "1",
-            field_73: "",
-            field_74: "",
-
-            field_75: "1",
-            field_76: "2",
-            field_77: "2",
-
-            field_78: "2",
-
-            field_51: "1",
-            field_50: "2000",
-            field_116: "2",
-            field_48: "1",
-            field_49: "1",
-
-            field_79: "4",
-            field_80: "1234.56",
-            field_87: "1",
-            field_88: "234.56",
-
-            field_106: "15",
-            field_99: "0",
-            field_89: now.day.to_s,
-            field_90: now.month.to_s,
-            field_91: now.strftime("%g"),
-          }
-        end
 
         it "returns true" do
           expect(parser).to be_valid
@@ -216,7 +218,7 @@ RSpec.describe BulkUpload::Lettings::RowParser do
       it "has errors on setup fields" do
         errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute)
 
-        expect(errors).to eql(%i[field_1 field_129 field_130 field_98 field_97 field_96 field_111 field_113])
+        expect(errors).to eql(%i[field_1 field_98 field_97 field_96 field_111 field_113])
       end
     end
 
@@ -420,6 +422,16 @@ RSpec.describe BulkUpload::Lettings::RowParser do
       end
     end
 
+    describe "#field_12" do
+      context "when set to a non-sensical value" do
+        let(:attributes) { valid_attributes.merge(field_12: "A", field_35: "1") }
+
+        it "returns only one error" do
+          expect(parser.errors[:field_12].size).to be(1)
+        end
+      end
+    end
+
     describe "#field_52" do # leaving reason
       context "when field_134 is 1 meaning it is a renewal" do
         context "when field_52 is 40" do
@@ -529,7 +541,7 @@ RSpec.describe BulkUpload::Lettings::RowParser do
     end
 
     describe "fields 96, 97, 98 => startdate" do
-      context "when any one of these fields is blank" do
+      context "when all of these fields are blank" do
         let(:attributes) { { bulk_upload:, field_1: "1", field_96: nil, field_97: nil, field_98: nil } }
 
         it "returns an error" do
@@ -541,6 +553,18 @@ RSpec.describe BulkUpload::Lettings::RowParser do
         end
       end
 
+      context "when one of these fields is blank" 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
+          parser.valid?
+
+          expect(parser.errors[:field_96]).to be_blank
+          expect(parser.errors[:field_97]).to be_blank
+          expect(parser.errors[:field_98]).to be_present
+        end
+      end
+
       context "when field 98 is 4 digits instead of 2" do
         let(:attributes) { { bulk_upload:, field_98: "2022" } }
 
@@ -658,7 +682,7 @@ RSpec.describe BulkUpload::Lettings::RowParser do
 
     describe "#field_134" do
       context "when an unpermitted value" do
-        let(:attributes) { { bulk_upload:, field_134: 3 } }
+        let(:attributes) { { bulk_upload:, field_134: "3" } }
 
         it "has errors on the field" do
           expect(parser.errors[:field_134]).to be_present
@@ -724,6 +748,18 @@ RSpec.describe BulkUpload::Lettings::RowParser do
             expect(parser.log.public_send(age)).to be(50)
           end
         end
+
+        context "when #{field} is a non-sensical value" do
+          let(:attributes) { { bulk_upload:, field.to_s => "A" } }
+
+          it "sets ##{known} to 0" do
+            expect(parser.log.public_send(known)).to be(0)
+          end
+
+          it "sets ##{age} to nil" do
+            expect(parser.log.public_send(age)).to be_nil
+          end
+        end
       end
     end
 

spec/services/bulk_upload/lettings/validator_spec.rb

@@ -42,7 +42,7 @@ RSpec.describe BulkUpload::Lettings::Validator do
       it "create validation error with correct values" do
         validator.call
 
-        error = BulkUploadError.order(:row, :field).first
+        error = BulkUploadError.find_by(field: "field_11")
 
         expect(error.field).to eql("field_11")
         expect(error.error).to eql("You must only answer the length of the tenancy if it's fixed-term")
@@ -260,11 +260,14 @@ RSpec.describe BulkUpload::Lettings::Validator do
         let(:log_5) { build(:lettings_log, renttype: 2, created_by: user, builtype: nil, startdate: Time.zone.local(2022, 5, 1)) }
 
         before do
-          file.write(BulkUpload::LogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_csv_row)
+          overrides = { age1: 50, age2: "R", age3: "R", age4: "4", age5: "R", age6: "R", age7: "R", age8: "R" }
-          file.write(BulkUpload::LogToCsv.new(log: log_2, line_ending: "\r\n", col_offset: 0).to_csv_row)
+
-          file.write(BulkUpload::LogToCsv.new(log: log_3, line_ending: "\r\n", col_offset: 0).to_csv_row)
+          file.write(BulkUpload::LogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0, overrides:).to_csv_row)
-          file.write(BulkUpload::LogToCsv.new(log: log_4, line_ending: "\r\n", col_offset: 0).to_csv_row)
+          file.write(BulkUpload::LogToCsv.new(log: log_2, line_ending: "\r\n", col_offset: 0, overrides:).to_csv_row)
-          file.write(BulkUpload::LogToCsv.new(log: log_5, line_ending: "\r\n", col_offset: 0).to_csv_row)
+          file.write(BulkUpload::LogToCsv.new(log: log_3, line_ending: "\r\n", col_offset: 0, overrides:).to_csv_row)
+          file.write(BulkUpload::LogToCsv.new(log: log_4, line_ending: "\r\n", col_offset: 0, overrides:).to_csv_row)
+          file.write(BulkUpload::LogToCsv.new(log: log_5, line_ending: "\r\n", col_offset: 0, overrides:).to_csv_row)
+
           file.close
         end
 

spec/support/bulk_upload/log_to_csv.rb

@@ -22,14 +22,14 @@ class BulkUpload::LogToCsv
       log.tenancy,
       log.tenancyother, # 10
       log.tenancylength,
-      log.age1,
+      log.age1 || overrides[:age1],
-      log.age2,
+      log.age2 || overrides[:age2],
-      log.age3,
+      log.age3 || overrides[:age3],
-      log.age4,
+      log.age4 || overrides[:age4],
-      log.age5,
+      log.age5 || overrides[:age5],
-      log.age6,
+      log.age6 || overrides[:age6],
-      log.age7,
+      log.age7 || overrides[:age7],
-      log.age8,
+      log.age8 || overrides[:age8],
 
       log.sex1, # 20
       log.sex2,