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

app/services/bulk_upload/lettings/row_parser.rb

@@ -144,6 +144,15 @@ class BulkUpload::Lettings::RowParser
                       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)") }
@@ -186,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?
@@ -657,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

spec/services/bulk_upload/lettings/row_parser_spec.rb

@@ -27,63 +27,7 @@ RSpec.describe BulkUpload::Lettings::RowParser do
     }
   end
 
-  before do
+  let(:valid_attributes) do
-    create(:organisation_relationship, parent_organisation: owning_org, child_organisation: managing_org)
-  end
-
-  around do |example|
-    FormHandler.instance.use_real_forms!
-
-    example.run
-
-    FormHandler.instance.use_fake_forms!
-  end
-
-  describe "#blank_row?" do
-    context "when a new object" do
-      it "returns true" do
-        expect(parser).to be_blank_row
-      end
-    end
-
-    context "when any field is populated" do
-      before do
-        parser.field_1 = "1"
-      end
-
-      it "returns false" do
-        expect(parser).not_to be_blank_row
-      end
-    end
-  end
-
-  describe "validations" do
-    before do
-      stub_request(:get, /api.postcodes.io/)
-      .to_return(status: 200, body: "{\"status\":200,\"result\":{\"admin_district\":\"Manchester\", \"codes\":{\"admin_district\": \"E08000003\"}}}", headers: {})
-
-      parser.valid?
-    end
-
-    describe "#valid?" do
-      context "when the row is blank" do
-        let(:attributes) { { bulk_upload: } }
-
-        it "returns true" do
-          expect(parser).to be_valid
-        end
-      end
-
-      context "when calling the method multiple times" do
-        let(:attributes) { { bulk_upload:, field_134: 2 } }
-
-        it "does not add keep adding errors to the pile" do
-          expect { parser.valid? }.not_to change(parser.errors, :count)
-        end
-      end
-
-      context "when valid row" do
-        let(:attributes) do
     {
       bulk_upload:,
       field_1: "1",
@@ -195,6 +139,64 @@ RSpec.describe BulkUpload::Lettings::RowParser do
     }
   end
 
+  before do
+    create(:organisation_relationship, parent_organisation: owning_org, child_organisation: managing_org)
+  end
+
+  around do |example|
+    FormHandler.instance.use_real_forms!
+
+    example.run
+
+    FormHandler.instance.use_fake_forms!
+  end
+
+  describe "#blank_row?" do
+    context "when a new object" do
+      it "returns true" do
+        expect(parser).to be_blank_row
+      end
+    end
+
+    context "when any field is populated" do
+      before do
+        parser.field_1 = "1"
+      end
+
+      it "returns false" do
+        expect(parser).not_to be_blank_row
+      end
+    end
+  end
+
+  describe "validations" do
+    before do
+      stub_request(:get, /api.postcodes.io/)
+      .to_return(status: 200, body: "{\"status\":200,\"result\":{\"admin_district\":\"Manchester\", \"codes\":{\"admin_district\": \"E08000003\"}}}", headers: {})
+
+      parser.valid?
+    end
+
+    describe "#valid?" do
+      context "when the row is blank" do
+        let(:attributes) { { bulk_upload: } }
+
+        it "returns true" do
+          expect(parser).to be_valid
+        end
+      end
+
+      context "when calling the method multiple times" do
+        let(:attributes) { { bulk_upload:, field_134: 2 } }
+
+        it "does not add keep adding errors to the pile" do
+          expect { parser.valid? }.not_to change(parser.errors, :count)
+        end
+      end
+
+      context "when valid row" do
+        let(:attributes) { valid_attributes }
+
         it "returns true" do
           expect(parser).to be_valid
         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
@@ -736,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,