From 17964be66984c14ba98c546d9354446a2ec77d3d Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Fri, 3 Mar 2023 12:19:01 +0000 Subject: [PATCH] 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 --- .../bulk_upload/lettings/row_parser.rb | 32 ++- .../bulk_upload/lettings/row_parser_spec.rb | 246 ++++++++++-------- .../bulk_upload/lettings/validator_spec.rb | 15 +- spec/support/bulk_upload/log_to_csv.rb | 16 +- 4 files changed, 175 insertions(+), 134 deletions(-) diff --git a/app/services/bulk_upload/lettings/row_parser.rb b/app/services/bulk_upload/lettings/row_parser.rb index 0cd25b40c..2c2dbce56 100644 --- a/app/services/bulk_upload/lettings/row_parser.rb +++ b/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 diff --git a/spec/services/bulk_upload/lettings/row_parser_spec.rb b/spec/services/bulk_upload/lettings/row_parser_spec.rb index 14c4ae3f3..b61d388f8 100644 --- a/spec/services/bulk_upload/lettings/row_parser_spec.rb +++ b/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 - { - 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 + let(:attributes) { valid_attributes } it "returns true" do expect(parser).to be_valid @@ -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 diff --git a/spec/services/bulk_upload/lettings/validator_spec.rb b/spec/services/bulk_upload/lettings/validator_spec.rb index ca52a19fe..b9d351281 100644 --- a/spec/services/bulk_upload/lettings/validator_spec.rb +++ b/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) - 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_4, line_ending: "\r\n", col_offset: 0).to_csv_row) - file.write(BulkUpload::LogToCsv.new(log: log_5, 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_1, line_ending: "\r\n", col_offset: 0, overrides:).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_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 diff --git a/spec/support/bulk_upload/log_to_csv.rb b/spec/support/bulk_upload/log_to_csv.rb index 628ee14b8..e15c9173b 100644 --- a/spec/support/bulk_upload/log_to_csv.rb +++ b/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.age2, - log.age3, - log.age4, - log.age5, - log.age6, - log.age7, - log.age8, + log.age1 || overrides[:age1], + log.age2 || overrides[:age2], + log.age3 || overrides[:age3], + log.age4 || overrides[:age4], + log.age5 || overrides[:age5], + log.age6 || overrides[:age6], + log.age7 || overrides[:age7], + log.age8 || overrides[:age8], log.sex1, # 20 log.sex2,