From 98cc162dda4e85ff4046eab160c4c739b9ccddeb Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Thu, 16 Mar 2023 14:06:01 +0000 Subject: [PATCH] bulk upload checks column count of CSV (#1423) --- app/mailers/bulk_upload_mailer.rb | 3 ++- app/services/bulk_upload/lettings/validator.rb | 11 ++++++++++- .../bulk_upload/lettings/year2022/csv_parser.rb | 1 + .../bulk_upload/lettings/year2023/csv_parser.rb | 1 + app/services/bulk_upload/processor.rb | 6 +++--- config/locales/en.yml | 5 +++++ spec/mailers/bulk_upload_mailer_spec.rb | 3 ++- spec/services/bulk_upload/lettings/validator_spec.rb | 12 ++++++++++++ spec/services/bulk_upload/processor_spec.rb | 1 + 9 files changed, 37 insertions(+), 6 deletions(-) diff --git a/app/mailers/bulk_upload_mailer.rb b/app/mailers/bulk_upload_mailer.rb index 3d5fca769..ebc9df7a6 100644 --- a/app/mailers/bulk_upload_mailer.rb +++ b/app/mailers/bulk_upload_mailer.rb @@ -100,7 +100,7 @@ class BulkUploadMailer < NotifyMailer ) end - def send_bulk_upload_failed_service_error_mail(bulk_upload:) + def send_bulk_upload_failed_service_error_mail(bulk_upload:, errors: []) bulk_upload_link = if bulk_upload.lettings? start_bulk_upload_lettings_logs_url else @@ -115,6 +115,7 @@ class BulkUploadMailer < NotifyMailer upload_timestamp: bulk_upload.created_at, lettings_or_sales: bulk_upload.log_type, year_combo: bulk_upload.year_combo, + errors: errors.map { |e| "- #{e}" }.join("\n"), bulk_upload_link:, }, ) diff --git a/app/services/bulk_upload/lettings/validator.rb b/app/services/bulk_upload/lettings/validator.rb index ea5015e56..fe2768427 100644 --- a/app/services/bulk_upload/lettings/validator.rb +++ b/app/services/bulk_upload/lettings/validator.rb @@ -9,6 +9,7 @@ class BulkUpload::Lettings::Validator attr_reader :bulk_upload, :path validate :validate_file_not_empty + validate :validate_min_columns validate :validate_max_columns def initialize(bulk_upload:, path:) @@ -127,12 +128,20 @@ private end end + def validate_min_columns + return if halt_validations? + + column_count = rows.map(&:size).min + + errors.add(:base, :under_min_column_count) if column_count < csv_parser.class::MIN_COLUMNS + end + def validate_max_columns return if halt_validations? column_count = rows.map(&:size).max - errors.add(:file, :column_count) if column_count > csv_parser.class::MAX_COLUMNS + errors.add(:base, :over_max_column_count) if column_count > csv_parser.class::MAX_COLUMNS end def halt_validations! diff --git a/app/services/bulk_upload/lettings/year2022/csv_parser.rb b/app/services/bulk_upload/lettings/year2022/csv_parser.rb index 81977c3cb..c0d8d8900 100644 --- a/app/services/bulk_upload/lettings/year2022/csv_parser.rb +++ b/app/services/bulk_upload/lettings/year2022/csv_parser.rb @@ -1,6 +1,7 @@ require "csv" class BulkUpload::Lettings::Year2022::CsvParser + MIN_COLUMNS = 134 MAX_COLUMNS = 136 attr_reader :path diff --git a/app/services/bulk_upload/lettings/year2023/csv_parser.rb b/app/services/bulk_upload/lettings/year2023/csv_parser.rb index 550ef1825..6f661922c 100644 --- a/app/services/bulk_upload/lettings/year2023/csv_parser.rb +++ b/app/services/bulk_upload/lettings/year2023/csv_parser.rb @@ -1,6 +1,7 @@ require "csv" class BulkUpload::Lettings::Year2023::CsvParser + MIN_COLUMNS = 141 MAX_COLUMNS = 143 attr_reader :path diff --git a/app/services/bulk_upload/processor.rb b/app/services/bulk_upload/processor.rb index ab4fe50a7..2d27464f0 100644 --- a/app/services/bulk_upload/processor.rb +++ b/app/services/bulk_upload/processor.rb @@ -8,7 +8,7 @@ class BulkUpload::Processor def call download - return send_failure_mail if validator.invalid? + return send_failure_mail(errors: validator.errors.full_messages) if validator.invalid? validator.call @@ -62,9 +62,9 @@ private validator.create_logs? && bulk_upload.logs.group(:status).count.keys == %w[completed] end - def send_failure_mail + def send_failure_mail(errors: []) BulkUploadMailer - .send_bulk_upload_failed_service_error_mail(bulk_upload:) + .send_bulk_upload_failed_service_error_mail(bulk_upload:, errors:) .deliver_later end diff --git a/config/locales/en.yml b/config/locales/en.yml index 9c8324d34..d21bb5b5c 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -41,6 +41,11 @@ en: activemodel: errors: models: + bulk_upload/lettings/validator: + attributes: + base: + over_max_column_count: "too many columns, please ensure you have used the correct template" + under_min_column_count: "too few columns, please ensure you have used the correct template" forms/bulk_upload_lettings/year: attributes: year: diff --git a/spec/mailers/bulk_upload_mailer_spec.rb b/spec/mailers/bulk_upload_mailer_spec.rb index 0e38617a3..323b49b8f 100644 --- a/spec/mailers/bulk_upload_mailer_spec.rb +++ b/spec/mailers/bulk_upload_mailer_spec.rb @@ -72,11 +72,12 @@ RSpec.describe BulkUploadMailer do upload_timestamp: bulk_upload.created_at, lettings_or_sales: bulk_upload.log_type, year_combo: bulk_upload.year_combo, + errors: "- foo\n- bar", bulk_upload_link: start_bulk_upload_lettings_logs_url, }, ) - mailer.send_bulk_upload_failed_service_error_mail(bulk_upload:) + mailer.send_bulk_upload_failed_service_error_mail(bulk_upload:, errors: %w[foo bar]) end end diff --git a/spec/services/bulk_upload/lettings/validator_spec.rb b/spec/services/bulk_upload/lettings/validator_spec.rb index dda7722c6..59c64a796 100644 --- a/spec/services/bulk_upload/lettings/validator_spec.rb +++ b/spec/services/bulk_upload/lettings/validator_spec.rb @@ -28,6 +28,18 @@ RSpec.describe BulkUpload::Lettings::Validator do end end + context "when file has too few columns" do + before do + file.write("a," * 132) + file.write("\n") + file.rewind + end + + it "is not valid" do + expect(validator).not_to be_valid + end + end + context "when incorrect headers" end diff --git a/spec/services/bulk_upload/processor_spec.rb b/spec/services/bulk_upload/processor_spec.rb index 8aaef8c36..2254e1b75 100644 --- a/spec/services/bulk_upload/processor_spec.rb +++ b/spec/services/bulk_upload/processor_spec.rb @@ -21,6 +21,7 @@ RSpec.describe BulkUpload::Processor do BulkUpload::Lettings::Validator, invalid?: true, call: nil, + errors: [], ) end