From 960b633ac57ca2afabf88a28675ab2e713fb1f85 Mon Sep 17 00:00:00 2001 From: natdeanlewissoftwire <94526761+natdeanlewissoftwire@users.noreply.github.com> Date: Mon, 10 Jul 2023 15:32:10 +0100 Subject: [PATCH] CLDC-2474 Allow numeric fields to accept decimal representations in bulk upload (#1753) * feat: allow decimals for lettings type when they only have trailing 0s and add tests * refactor: linting * refactor: DRY tests * refactor: linting --- .../lettings/year2022/row_parser.rb | 2 +- .../lettings/year2023/row_parser.rb | 2 +- .../lettings/year2022/row_parser_spec.rb | 24 ++ .../lettings/year2023/row_parser_spec.rb | 226 ++++++++++-------- 4 files changed, 153 insertions(+), 101 deletions(-) diff --git a/app/services/bulk_upload/lettings/year2022/row_parser.rb b/app/services/bulk_upload/lettings/year2022/row_parser.rb index fc105f0cc..a1e7dbb06 100644 --- a/app/services/bulk_upload/lettings/year2022/row_parser.rb +++ b/app/services/bulk_upload/lettings/year2022/row_parser.rb @@ -773,7 +773,7 @@ private end def validate_data_types - unless attribute_set["field_1"].value_before_type_cast&.match?(/\A\d+\z/) + unless attribute_set["field_1"].value_before_type_cast&.match?(/^\d+\.?0*$/) errors.add(:field_1, I18n.t("validations.invalid_number", question: "letting type")) 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 0f81ff543..11e9a1e5b 100644 --- a/app/services/bulk_upload/lettings/year2023/row_parser.rb +++ b/app/services/bulk_upload/lettings/year2023/row_parser.rb @@ -702,7 +702,7 @@ private end def validate_data_types - unless attribute_set["field_5"].value_before_type_cast&.match?(/\A\d+\z/) + unless attribute_set["field_5"].value_before_type_cast&.match?(/^\d+\.?0*$/) errors.add(:field_5, I18n.t("validations.invalid_number", question: "letting type")) end end 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 1a1a0caf9..4d2c28ee8 100644 --- a/spec/services/bulk_upload/lettings/year2022/row_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/year2022/row_parser_spec.rb @@ -339,6 +339,30 @@ RSpec.describe BulkUpload::Lettings::Year2022::RowParser do end end end + + context "when valid row with valid decimal (integer) field_1" do + before do + allow(FeatureToggle).to receive(:bulk_upload_duplicate_log_check_enabled?).and_return(true) + end + + let(:attributes) { valid_attributes.merge(field_1: "1.00") } + + it "returns true" do + expect(parser).to be_valid + end + end + + context "when valid row with invalid decimal (non-integer) field_1" do + before do + allow(FeatureToggle).to receive(:bulk_upload_duplicate_log_check_enabled?).and_return(true) + end + + let(:attributes) { valid_attributes.merge(field_1: "1.56") } + + it "returns false" do + expect(parser).not_to be_valid + end + end end context "when setup section not complete" do 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 ef54d5e56..e177eb859 100644 --- a/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb @@ -120,12 +120,8 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do end end - context "when valid row" do - before do - allow(FeatureToggle).to receive(:bulk_upload_duplicate_log_check_enabled?).and_return(true) - end - - let(:attributes) do + context "when testing valid/invalid attributes" do + let(:valid_attributes) do { bulk_upload:, field_5: "1", @@ -250,128 +246,160 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do } end - it "returns true" do - expect(parser).to be_valid - end - - xit "instantiates a log with everything completed", aggregate_failures: true do - questions = parser.send(:questions).reject do |q| - parser.send(:log).optional_fields.include?(q.id) || q.completed?(parser.send(:log)) + context "when valid row" do + before do + allow(FeatureToggle).to receive(:bulk_upload_duplicate_log_check_enabled?).and_return(true) end - expect(questions.map(&:id).size).to eq(0) - expect(questions.map(&:id)).to eql([]) - end - - context "when a general needs log already exists in the db" do - let(:attributes) { { bulk_upload:, field_4: "1" } } + let(:attributes) { valid_attributes } - before do - parser.log.save! - parser.instance_variable_set(:@valid, nil) + it "returns true" do + expect(parser).to be_valid end - it "is not a valid row" do - expect(parser).not_to be_valid + xit "instantiates a log with everything completed", aggregate_failures: true do + questions = parser.send(:questions).reject do |q| + parser.send(:log).optional_fields.include?(q.id) || q.completed?(parser.send(:log)) + end + + expect(questions.map(&:id).size).to eq(0) + expect(questions.map(&:id)).to eql([]) end - it "adds an error to all (and only) the fields used to determine duplicates" do - parser.valid? + context "when a general needs log already exists in the db" do + let(:attributes) { { bulk_upload:, field_4: "1" } } + + before do + parser.log.save! + parser.instance_variable_set(:@valid, nil) + end - error_message = "This is a duplicate log" - - [ - :field_1, # owning_organisation - :field_7, # startdate - :field_8, # startdate - :field_9, # startdate - :field_13, # tenancycode - :field_14, # propcode - :field_23, # postcode_full - :field_24, # postcode_full - :field_25, # postcode_full - :field_46, # age1 - :field_47, # sex1 - :field_50, # ecstat1 - :field_132, # tcharge - ].each do |field| - expect(parser.errors[field]).to include(error_message) + it "is not a valid row" do + expect(parser).not_to be_valid end - expect(parser.errors[:field_17]).not_to include(error_message) + it "adds an error to all (and only) the fields used to determine duplicates" do + parser.valid? + + error_message = "This is a duplicate log" + + [ + :field_1, # owning_organisation + :field_7, # startdate + :field_8, # startdate + :field_9, # startdate + :field_13, # tenancycode + :field_14, # propcode + :field_23, # postcode_full + :field_24, # postcode_full + :field_25, # postcode_full + :field_46, # age1 + :field_47, # sex1 + :field_50, # ecstat1 + :field_132, # tcharge + ].each do |field| + expect(parser.errors[field]).to include(error_message) + end + + expect(parser.errors[:field_17]).not_to include(error_message) + end end - end - context "when a supported housing log already exists in the db" do - let(:attributes) { { bulk_upload:, field_4: "2" } } + context "when a supported housing log already exists in the db" do + let(:attributes) { { bulk_upload:, field_4: "2" } } - before do - parser.log.save! - parser.instance_variable_set(:@valid, nil) - end + before do + parser.log.save! + parser.instance_variable_set(:@valid, nil) + end - it "is not a valid row" do - expect(parser).not_to be_valid + it "is not a valid row" do + expect(parser).not_to be_valid + end + + it "adds an error to all the fields used to determine duplicates" do + parser.valid? + + error_message = "This is a duplicate log" + + [ + :field_1, # owning_organisation + :field_7, # startdate + :field_8, # startdate + :field_9, # startdate + :field_13, # tenancycode + :field_14, # propcode + :field_17, # location + :field_46, # age1 + :field_47, # sex1 + :field_50, # ecstat1 + :field_132, # tcharge + ].each do |field| + expect(parser.errors[field]).to include(error_message) + end + + expect(parser.errors[:field_23]).not_to include(error_message) + expect(parser.errors[:field_24]).not_to include(error_message) + expect(parser.errors[:field_25]).not_to include(error_message) + end end - it "adds an error to all the fields used to determine duplicates" do - parser.valid? + context "when a hidden log already exists in db" do + before do + parser.log.status = "pending" + parser.log.skip_update_status = true + parser.log.save! + end - error_message = "This is a duplicate log" - - [ - :field_1, # owning_organisation - :field_7, # startdate - :field_8, # startdate - :field_9, # startdate - :field_13, # tenancycode - :field_14, # propcode - :field_17, # location - :field_46, # age1 - :field_47, # sex1 - :field_50, # ecstat1 - :field_132, # tcharge - ].each do |field| - expect(parser.errors[field]).to include(error_message) + it "is a valid row" do + expect(parser).to be_valid end - expect(parser.errors[:field_23]).not_to include(error_message) - expect(parser.errors[:field_24]).not_to include(error_message) - expect(parser.errors[:field_25]).not_to include(error_message) + it "does not add duplicate errors" do + parser.valid? + + [ + :field_1, # owning_organisation + :field_7, # startdate + :field_8, # startdate + :field_9, # startdate + :field_14, # propcode + :field_17, # location + :field_23, # postcode_full + :field_24, # postcode_full + :field_25, # postcode_full + :field_46, # age1 + :field_47, # sex1 + :field_50, # ecstat1 + :field_132, # tcharge + ].each do |field| + expect(parser.errors[field]).to be_blank + end + end end end - context "when a hidden log already exists in db" do + context "when valid row with valid decimal (integer) field_5" do before do - parser.log.status = "pending" - parser.log.skip_update_status = true - parser.log.save! + allow(FeatureToggle).to receive(:bulk_upload_duplicate_log_check_enabled?).and_return(true) end - it "is a valid row" do + let(:attributes) { valid_attributes.merge(field_5: "1.00") } + + it "returns true" do expect(parser).to be_valid end + end - it "does not add duplicate errors" do - parser.valid? + context "when valid row with invalid decimal (non-integer) field_5" do + before do + allow(FeatureToggle).to receive(:bulk_upload_duplicate_log_check_enabled?).and_return(true) + end - [ - :field_1, # owning_organisation - :field_7, # startdate - :field_8, # startdate - :field_9, # startdate - :field_14, # propcode - :field_17, # location - :field_23, # postcode_full - :field_24, # postcode_full - :field_25, # postcode_full - :field_46, # age1 - :field_47, # sex1 - :field_50, # ecstat1 - :field_132, # tcharge - ].each do |field| - expect(parser.errors[field]).to be_blank - end + let(:attributes) { valid_attributes.merge(field_5: "1.56") } + + it "returns false" do + expect(parser).not_to be_valid end end end