diff --git a/app/mailers/bulk_upload_mailer.rb b/app/mailers/bulk_upload_mailer.rb index fe1e81517..03f30a453 100644 --- a/app/mailers/bulk_upload_mailer.rb +++ b/app/mailers/bulk_upload_mailer.rb @@ -43,8 +43,29 @@ class BulkUploadMailer < NotifyMailer end end - def send_correct_and_upload_again_mail(bulk_upload:) - error_description = "We noticed that you have a lot of similar errors in column #{columns_with_errors(bulk_upload:)}. Please correct your data export and upload again." + def send_correct_and_upload_again_mail(bulk_upload:, errors:) + any_setup_sections_incomplete_error_description = "logs where the setup sections were incomplete" + over_column_error_threshold_error_description = "logs with a lot of similar errors in column #{columns_with_errors(bulk_upload:)}" + any_logs_already_exist_error_description = "logs you are trying to upload have been created previously" + + errors_list = [] + + errors_list << if errors.size.zero? + "Please correct your data export and upload again." + else + "We noticed the following issues with your upload:" + end + + errors + .select { |_k, v| v } + .each_key do |k| + local_var = "#{k}_error_description" + errors_list << "- #{binding.local_variable_get(local_var)}" + end + + errors_list << "" + + error_description = errors_list.join("\n") summary_report_link = if BulkUploadErrorSummaryTableComponent.new(bulk_upload:).errors? summary_bulk_upload_lettings_result_url(bulk_upload) diff --git a/app/services/bulk_upload/lettings/row_parser.rb b/app/services/bulk_upload/lettings/row_parser.rb index b772d0153..46455db04 100644 --- a/app/services/bulk_upload/lettings/row_parser.rb +++ b/app/services/bulk_upload/lettings/row_parser.rb @@ -166,6 +166,7 @@ class BulkUpload::Lettings::RowParser validate :validate_cannot_be_la_referral_if_general_needs_and_la validate :validate_leaving_reason_for_renewal validate :validate_lettings_type_matches_bulk_upload + validate :validate_if_log_already_exists validate :validate_only_one_housing_needs_type validate :validate_no_disabled_needs_conjunction validate :validate_dont_know_disabled_needs_conjunction @@ -226,8 +227,24 @@ class BulkUpload::Lettings::RowParser log.form.setup_sections[0].subsections[0].is_incomplete?(log) end + def log_already_exists? + LettingsLog.exists?(duplicity_check_fields.index_with { |field| log.public_send(field) }) + end + private + def duplicity_check_fields + %w[ + startdate + age1 + sex1 + ecstat1 + owning_organisation + tcharge + propcode + ] + end + def validate_location_related return if scheme.blank? || location.blank? @@ -432,6 +449,25 @@ private log.form.setup_sections[0].subsections[0].questions.include?(question) end + def validate_if_log_already_exists + if log_already_exists? + error_message = "This is a duplicate log" + + errors.add(:field_12, error_message) # age1 + errors.add(:field_20, error_message) # sex1 + errors.add(:field_35, error_message) # ecstat1 + + errors.add(:field_84, error_message) # tcharge + + errors.add(:field_96, error_message) # startdate + errors.add(:field_97, error_message) # startdate + errors.add(:field_98, error_message) # startdate + + errors.add(:field_100, error_message) # propcode + errors.add(:field_111, error_message) # owning_organisation + end + end + def field_mapping_for_errors { lettype: [:field_1], @@ -630,18 +666,10 @@ private Organisation.find_by_id_on_mulitple_fields(field_111) end - def owning_organisation_id - owning_organisation&.id - end - def managing_organisation Organisation.find_by_id_on_mulitple_fields(field_113) end - def managing_organisation_id - managing_organisation&.id - end - def attributes_for_log attributes = {} @@ -650,8 +678,8 @@ private attributes["la"] = field_107 attributes["postcode_known"] = postcode_known attributes["postcode_full"] = postcode_full - attributes["owning_organisation_id"] = owning_organisation_id - attributes["managing_organisation_id"] = managing_organisation_id + attributes["owning_organisation"] = owning_organisation + attributes["managing_organisation"] = managing_organisation attributes["renewal"] = renewal attributes["scheme"] = scheme attributes["location"] = location diff --git a/app/services/bulk_upload/lettings/validator.rb b/app/services/bulk_upload/lettings/validator.rb index 779f0315b..df95e6f4b 100644 --- a/app/services/bulk_upload/lettings/validator.rb +++ b/app/services/bulk_upload/lettings/validator.rb @@ -178,8 +178,10 @@ class BulkUpload::Lettings::Validator return false if any_setup_sections_incomplete? return false if over_column_error_threshold? return false if row_parsers.any?(&:block_log_creation?) + return false if any_logs_already_exist? + return false if any_logs_invalid? - row_parsers.all? { |row_parser| row_parser.log.valid? } + true end def self.question_for_field(field) @@ -190,8 +192,6 @@ class BulkUpload::Lettings::Validator 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 @@ -205,6 +205,16 @@ private end end + def any_logs_already_exist? + row_parsers.any?(&:log_already_exists?) + end + +private + + def any_logs_invalid? + row_parsers.any? { |row_parser| row_parser.log.invalid? } + end + def csv_parser @csv_parser ||= BulkUpload::Lettings::CsvParser.new(path:) end diff --git a/app/services/bulk_upload/processor.rb b/app/services/bulk_upload/processor.rb index dcf68e594..62889d0cb 100644 --- a/app/services/bulk_upload/processor.rb +++ b/app/services/bulk_upload/processor.rb @@ -37,9 +37,14 @@ private 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:, + errors: { + any_setup_sections_incomplete: validator.any_setup_sections_incomplete?, + over_column_error_threshold: validator.over_column_error_threshold?, + any_logs_already_exist: validator.any_logs_already_exist?, + }, + ).deliver_later end def send_fix_errors_mail diff --git a/spec/mailers/bulk_upload_mailer_spec.rb b/spec/mailers/bulk_upload_mailer_spec.rb index 0e38617a3..8c97db31b 100644 --- a/spec/mailers/bulk_upload_mailer_spec.rb +++ b/spec/mailers/bulk_upload_mailer_spec.rb @@ -109,13 +109,8 @@ RSpec.describe BulkUploadMailer do end describe "#send_correct_and_upload_again_mail" do - context "when 2 columns with errors" do - before do - create(:bulk_upload_error, bulk_upload:, col: "A") - create(:bulk_upload_error, bulk_upload:, col: "B") - end - - it "sends correctly formed email with A, B" do + context "when there are no errors" do + it "sends correctly formed email" do expect(notify_client).to receive(:send_email).with( email_address: user.email, template_id: described_class::BULK_UPLOAD_FAILED_CSV_ERRORS_TEMPLATE_ID, @@ -124,16 +119,16 @@ RSpec.describe BulkUploadMailer do upload_timestamp: bulk_upload.created_at.to_fs(:govuk_date_and_time), year_combo: bulk_upload.year_combo, lettings_or_sales: bulk_upload.log_type, - error_description: "We noticed that you have a lot of similar errors in column A, B. Please correct your data export and upload again.", summary_report_link: "http://localhost:3000/lettings-logs/bulk-upload-results/#{bulk_upload.id}", + error_description: "Please correct your data export and upload again.\n", }, ) - mailer.send_correct_and_upload_again_mail(bulk_upload:) + mailer.send_correct_and_upload_again_mail(bulk_upload:, errors: {}) end end - context "when 4 columns with errors" do + context "when are multiple errors" do before do stub_const("BulkUploadErrorSummaryTableComponent::DISPLAY_THRESHOLD", 0) @@ -144,6 +139,8 @@ RSpec.describe BulkUploadMailer do end it "sends correctly formed email with A, B, C and more" do + error_description = "We noticed the following issues with your upload:\n- logs where the setup sections were incomplete\n- logs with a lot of similar errors in column A, B, C and more\n- logs you are trying to upload have been created previously\n" + expect(notify_client).to receive(:send_email).with( email_address: user.email, template_id: described_class::BULK_UPLOAD_FAILED_CSV_ERRORS_TEMPLATE_ID, @@ -152,12 +149,18 @@ RSpec.describe BulkUploadMailer do upload_timestamp: bulk_upload.created_at.to_fs(:govuk_date_and_time), year_combo: bulk_upload.year_combo, lettings_or_sales: bulk_upload.log_type, - error_description: "We noticed that you have a lot of similar errors in column A, B, C and more. Please correct your data export and upload again.", + error_description:, summary_report_link: "http://localhost:3000/lettings-logs/bulk-upload-results/#{bulk_upload.id}/summary", }, ) - mailer.send_correct_and_upload_again_mail(bulk_upload:) + errors = { + any_setup_sections_incomplete: true, + over_column_error_threshold: true, + any_logs_already_exist: true, + } + + mailer.send_correct_and_upload_again_mail(bulk_upload:, errors:) end end end diff --git a/spec/services/bulk_upload/lettings/row_parser_spec.rb b/spec/services/bulk_upload/lettings/row_parser_spec.rb index 73b35d70c..02ee2c5b3 100644 --- a/spec/services/bulk_upload/lettings/row_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/row_parser_spec.rb @@ -209,6 +209,35 @@ RSpec.describe BulkUpload::Lettings::RowParser do expect(questions.map(&:id).size).to eq(0) expect(questions.map(&:id)).to eql([]) end + + context "when the log already exists in the db" do + before do + parser.log.save! + end + + it "is not a valid row" do + expect(parser).not_to be_valid + end + + it "adds an error to all (and only) the fields used to determine duplicity" do + parser.valid? + + error_message = "This is a duplicate log" + + expected_errors = { + field_12: [error_message], # age1 + field_20: [error_message], # sex1 + field_35: [error_message], # ecstat1 + field_84: [error_message], # tcharge + field_96: [error_message], # startdate + field_97: [error_message], # startdate + field_98: [error_message], # startdate + field_100: [error_message], # propcode + field_111: [error_message], # owning_organisation + } + expect(parser.errors.as_json).to eq(expected_errors) + end + end end end diff --git a/spec/services/bulk_upload/processor_spec.rb b/spec/services/bulk_upload/processor_spec.rb index 48b0c0258..8380ea1f1 100644 --- a/spec/services/bulk_upload/processor_spec.rb +++ b/spec/services/bulk_upload/processor_spec.rb @@ -193,8 +193,10 @@ RSpec.describe BulkUpload::Processor do BulkUpload::Lettings::Validator, invalid?: false, call: nil, - any_setup_sections_incomplete?: false, create_logs?: false, + any_setup_sections_incomplete?: false, + over_column_error_threshold?: false, + any_logs_already_exist?: false, ) end