From 2aee5a80c5980758cc5d4c505f67b34f48cf6cf0 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Mon, 6 Mar 2023 10:00:44 +0000 Subject: [PATCH] CLDC-1768 Bulk upload file setup email (#1358) * add placeholder tests for bulk upload mailer * bulk upload fix setup errors email - this plumbs in the condition so if any setup sections are incomplete we send that partcular email and prevent the remaining flow * tag bulk upload setup errors for downsteam use * add category to bulk upload errors * persist bulk upload error category * populate bulk upload mailer with errors --- app/mailers/bulk_upload_mailer.rb | 39 +++++++++++++++---- .../bulk_upload/lettings/row_parser.rb | 24 +++++++++--- .../bulk_upload/lettings/validator.rb | 7 ++-- app/services/bulk_upload/processor.rb | 36 ++++++++++++----- ...0116_add_category_to_bulk_upload_errors.rb | 5 +++ db/schema.rb | 1 + spec/mailers/bulk_upload_mailer_spec.rb | 32 +++++++++++++++ .../bulk_upload/lettings/row_parser_spec.rb | 10 +++++ .../bulk_upload/lettings/validator_spec.rb | 5 +++ spec/services/bulk_upload/processor_spec.rb | 39 +++++++++++++++++++ 10 files changed, 173 insertions(+), 25 deletions(-) create mode 100644 db/migrate/20230301120116_add_category_to_bulk_upload_errors.rb diff --git a/app/mailers/bulk_upload_mailer.rb b/app/mailers/bulk_upload_mailer.rb index 94079828f..fe1e81517 100644 --- a/app/mailers/bulk_upload_mailer.rb +++ b/app/mailers/bulk_upload_mailer.rb @@ -66,17 +66,40 @@ class BulkUploadMailer < NotifyMailer ) end - def send_bulk_upload_failed_file_setup_error_mail(user, bulk_upload) + def send_bulk_upload_failed_file_setup_error_mail(bulk_upload:) + bulk_upload_link = if bulk_upload.lettings? + start_bulk_upload_lettings_logs_url + else + start_bulk_upload_sales_logs_url + end + + validator_class = if bulk_upload.lettings? + BulkUpload::Lettings::Validator + else + BulkUpload::Sales::Validator + end + + errors = bulk_upload + .bulk_upload_errors + .where(category: "setup") + .group(:col, :field) + .count + .keys + .sort_by { |_col, field| field } + .map do |col, field| + "- Column #{col} (#{validator_class.question_for_field(field.to_sym)})" + end + send_email( - user.email, + bulk_upload.user.email, BULK_UPLOAD_FAILED_FILE_SETUP_ERROR_TEMPLATE_ID, { - filename: "[#{bulk_upload} filename]", - upload_timestamp: "[#{bulk_upload} upload_timestamp]", - lettings_or_sales: "[#{bulk_upload} lettings_or_sales]", - year_combo: "[#{bulk_upload} year_combo]", - errors_list: "[#{bulk_upload} errors_list]", - bulk_upload_link: "[#{bulk_upload} bulk_upload_link]", + filename: bulk_upload.filename, + upload_timestamp: bulk_upload.created_at.to_fs(:govuk_date_and_time), + lettings_or_sales: bulk_upload.log_type, + year_combo: bulk_upload.year_combo, + errors_list: errors.join("\n"), + bulk_upload_link:, }, ) end diff --git a/app/services/bulk_upload/lettings/row_parser.rb b/app/services/bulk_upload/lettings/row_parser.rb index 426f122a1..289566508 100644 --- a/app/services/bulk_upload/lettings/row_parser.rb +++ b/app/services/bulk_upload/lettings/row_parser.rb @@ -203,6 +203,10 @@ class BulkUpload::Lettings::RowParser block_log_creation end + def setup_section_incomplete? + log.form.setup_sections[0].subsections[0].is_incomplete?(log) + end + private def validate_location_related @@ -222,7 +226,7 @@ private def validate_location_exists if scheme && field_5.present? && location.nil? - errors.add(:field_5, "Location could be found with provided scheme code") + errors.add(:field_5, "Location could be found with provided scheme code", category: :setup) end end @@ -240,7 +244,7 @@ private def validate_scheme_exists if field_4.present? && scheme.nil? - errors.add(:field_4, "The management group code is not correct") + errors.add(:field_4, "The management group code is not correct", category: :setup) end end @@ -254,7 +258,7 @@ private def validate_managing_org_exists if managing_organisation.nil? errors.delete(:field_113) - errors.add(:field_113, "The managing organisation code is incorrect") + errors.add(:field_113, "The managing organisation code is incorrect", category: :setup) end end @@ -269,7 +273,7 @@ private def validate_owning_org_exists if owning_organisation.nil? errors.delete(:field_111) - errors.add(:field_111, "The owning organisation code is incorrect") + errors.add(:field_111, "The owning organisation code is incorrect", category: :setup) end end @@ -387,10 +391,18 @@ private next if log.optional_fields.include?(question.id) next if question.completed?(log) - fields.each { |field| errors.add(field, I18n.t("validations.not_answered", question: question.check_answer_label&.downcase)) } + if setup_question?(question) + fields.each { |field| errors.add(field, I18n.t("validations.not_answered", question: question.check_answer_label&.downcase), category: :setup) } + else + fields.each { |field| errors.add(field, I18n.t("validations.not_answered", question: question.check_answer_label&.downcase)) } + end end end + def setup_question?(question) + log.form.setup_sections[0].subsections[0].questions.include?(question) + end + def field_mapping_for_errors { lettype: [:field_1], @@ -398,6 +410,8 @@ private postcode_known: %i[field_107 field_108 field_109], postcode_full: %i[field_107 field_108 field_109], la: %i[field_107], + owning_organisation: [:field_111], + managing_organisation: [:field_113], owning_organisation_id: [:field_111], managing_organisation_id: [:field_113], renewal: [:field_134], diff --git a/app/services/bulk_upload/lettings/validator.rb b/app/services/bulk_upload/lettings/validator.rb index 992f06196..779f0315b 100644 --- a/app/services/bulk_upload/lettings/validator.rb +++ b/app/services/bulk_upload/lettings/validator.rb @@ -168,6 +168,7 @@ class BulkUpload::Lettings::Validator row:, cell: "#{cols[field_number_for_attribute(error.attribute) - col_offset + 1]}#{row}", col: cols[field_number_for_attribute(error.attribute) - col_offset + 1], + category: error.options[:category], ) end end @@ -185,12 +186,12 @@ class BulkUpload::Lettings::Validator QUESTIONS[field] end -private - def any_setup_sections_incomplete? - row_parsers.any? { |row_parser| row_parser.log.form.setup_sections[0].subsections[0].is_incomplete?(row_parser.log) } + row_parsers.any?(&:setup_section_incomplete?) end +private + def over_column_error_threshold? fields = ("field_1".."field_134").to_a percentage_threshold = (row_parsers.size * COLUMN_PERCENTAGE_ERROR_THRESHOLD).ceil diff --git a/app/services/bulk_upload/processor.rb b/app/services/bulk_upload/processor.rb index bbf7e6cd1..dcf68e594 100644 --- a/app/services/bulk_upload/processor.rb +++ b/app/services/bulk_upload/processor.rb @@ -12,11 +12,15 @@ class BulkUpload::Processor validator.call - create_logs if validator.create_logs? - send_correct_and_upload_again_mail unless validator.create_logs? - - send_fix_errors_mail if created_logs_but_incompleted? - send_success_mail if created_logs_and_all_completed? + if validator.any_setup_sections_incomplete? + send_setup_errors_mail + elsif validator.create_logs? + create_logs + send_fix_errors_mail if created_logs_but_incompleted? + send_success_mail if created_logs_and_all_completed? + else + send_correct_and_upload_again_mail + end rescue StandardError => e Sentry.capture_exception(e) send_failure_mail @@ -26,16 +30,28 @@ class BulkUpload::Processor private + def send_setup_errors_mail + BulkUploadMailer + .send_bulk_upload_failed_file_setup_error_mail(bulk_upload:) + .deliver_later + end + def send_correct_and_upload_again_mail - BulkUploadMailer.send_correct_and_upload_again_mail(bulk_upload:).deliver_later + BulkUploadMailer + .send_correct_and_upload_again_mail(bulk_upload:) + .deliver_later end def send_fix_errors_mail - BulkUploadMailer.send_bulk_upload_with_errors_mail(bulk_upload:).deliver_later + BulkUploadMailer + .send_bulk_upload_with_errors_mail(bulk_upload:) + .deliver_later end def send_success_mail - BulkUploadMailer.send_bulk_upload_complete_mail(user:, bulk_upload:).deliver_later + BulkUploadMailer + .send_bulk_upload_complete_mail(user:, bulk_upload:) + .deliver_later end def created_logs_but_incompleted? @@ -47,7 +63,9 @@ private end def send_failure_mail - BulkUploadMailer.send_bulk_upload_failed_service_error_mail(bulk_upload:).deliver_later + BulkUploadMailer + .send_bulk_upload_failed_service_error_mail(bulk_upload:) + .deliver_later end def user diff --git a/db/migrate/20230301120116_add_category_to_bulk_upload_errors.rb b/db/migrate/20230301120116_add_category_to_bulk_upload_errors.rb new file mode 100644 index 000000000..000cbbd6d --- /dev/null +++ b/db/migrate/20230301120116_add_category_to_bulk_upload_errors.rb @@ -0,0 +1,5 @@ +class AddCategoryToBulkUploadErrors < ActiveRecord::Migration[7.0] + def change + add_column :bulk_upload_errors, :category, :text, null: true + end +end diff --git a/db/schema.rb b/db/schema.rb index afa0b6b74..c2f8f8600 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -26,6 +26,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_03_01_144555) do t.datetime "created_at", null: false t.datetime "updated_at", null: false t.text "col" + t.text "category" t.index ["bulk_upload_id"], name: "index_bulk_upload_errors_on_bulk_upload_id" end diff --git a/spec/mailers/bulk_upload_mailer_spec.rb b/spec/mailers/bulk_upload_mailer_spec.rb index a0042cf69..0e38617a3 100644 --- a/spec/mailers/bulk_upload_mailer_spec.rb +++ b/spec/mailers/bulk_upload_mailer_spec.rb @@ -12,6 +12,38 @@ RSpec.describe BulkUploadMailer do allow(notify_client).to receive(:send_email).and_return(true) end + describe "#send_bulk_upload_failed_file_setup_error_mail" do + before do + create(:bulk_upload_error, bulk_upload:, col: "A", field: "field_1", category: "setup") + create(:bulk_upload_error, bulk_upload:, col: "E", field: "field_4", category: "setup") + create(:bulk_upload_error, bulk_upload:, col: "F", field: "field_5") + end + + let(:expected_errors) do + [ + "- Column A (What is the letting type?)", + "- Column E (Management group code)", + ] + end + + it "sends correctly formed email" do + expect(notify_client).to receive(:send_email).with( + email_address: bulk_upload.user.email, + template_id: described_class::BULK_UPLOAD_FAILED_FILE_SETUP_ERROR_TEMPLATE_ID, + personalisation: { + filename: bulk_upload.filename, + upload_timestamp: bulk_upload.created_at.to_fs(:govuk_date_and_time), + lettings_or_sales: bulk_upload.log_type, + year_combo: bulk_upload.year_combo, + errors_list: expected_errors.join("\n"), + bulk_upload_link: start_bulk_upload_lettings_logs_url, + }, + ) + + mailer.send_bulk_upload_failed_file_setup_error_mail(bulk_upload:) + end + end + describe "#send_bulk_upload_complete_mail" do it "sends correctly formed email" do expect(notify_client).to receive(:send_email).with( diff --git a/spec/services/bulk_upload/lettings/row_parser_spec.rb b/spec/services/bulk_upload/lettings/row_parser_spec.rb index cd16a88a1..867e5028b 100644 --- a/spec/services/bulk_upload/lettings/row_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/row_parser_spec.rb @@ -210,6 +210,16 @@ RSpec.describe BulkUpload::Lettings::RowParser do end end + context "when setup section not complete" do + let(:attributes) { { bulk_upload:, field_7: "123" } } + + 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]) + end + end + describe "#field_1" do context "when null" do let(:attributes) { { bulk_upload:, field_1: nil, field_4: "1" } } diff --git a/spec/services/bulk_upload/lettings/validator_spec.rb b/spec/services/bulk_upload/lettings/validator_spec.rb index 0aaeaac78..ca52a19fe 100644 --- a/spec/services/bulk_upload/lettings/validator_spec.rb +++ b/spec/services/bulk_upload/lettings/validator_spec.rb @@ -51,6 +51,11 @@ RSpec.describe BulkUpload::Lettings::Validator do expect(error.row).to eql("7") expect(error.cell).to eql("L7") expect(error.col).to eql("L") + expect(error.category).to be_nil + + error = BulkUploadError.order(:row, :field).find_by(field: "field_111") + + expect(error.category).to eql("setup") end end diff --git a/spec/services/bulk_upload/processor_spec.rb b/spec/services/bulk_upload/processor_spec.rb index d56b2ff4e..48b0c0258 100644 --- a/spec/services/bulk_upload/processor_spec.rb +++ b/spec/services/bulk_upload/processor_spec.rb @@ -91,6 +91,42 @@ RSpec.describe BulkUpload::Processor do end end + context "when a log has an incomplete setup section" do + let(:mock_downloader) do + instance_double( + BulkUpload::Downloader, + call: nil, + path: file_fixture("2022_23_lettings_bulk_upload.csv"), + delete_local_file!: nil, + ) + end + + let(:mock_validator) do + instance_double( + BulkUpload::Lettings::Validator, + invalid?: false, + call: nil, + any_setup_sections_incomplete?: true, + ) + end + + before do + allow(BulkUpload::Downloader).to receive(:new).with(bulk_upload:).and_return(mock_downloader) + allow(BulkUpload::Lettings::Validator).to receive(:new).and_return(mock_validator) + end + + it "sends setup failure email" do + mail_double = instance_double("ActionMailer::MessageDelivery", deliver_later: nil) + + allow(BulkUploadMailer).to receive(:send_bulk_upload_failed_file_setup_error_mail).and_return(mail_double) + + processor.call + + expect(BulkUploadMailer).to have_received(:send_bulk_upload_failed_file_setup_error_mail) + expect(mail_double).to have_received(:deliver_later) + end + end + context "when processing a bulk upload with errors but below threshold (therefore creates logs)" do let(:mock_downloader) do instance_double( @@ -106,6 +142,7 @@ RSpec.describe BulkUpload::Processor do BulkUpload::Lettings::Validator, invalid?: false, call: nil, + any_setup_sections_incomplete?: false, create_logs?: true, ) end @@ -156,6 +193,7 @@ RSpec.describe BulkUpload::Processor do BulkUpload::Lettings::Validator, invalid?: false, call: nil, + any_setup_sections_incomplete?: false, create_logs?: false, ) end @@ -216,6 +254,7 @@ RSpec.describe BulkUpload::Processor do BulkUpload::Lettings::Validator, call: nil, create_logs?: true, + any_setup_sections_incomplete?: false, invalid?: false, ) end