From a12339f58d5ca31f4bb1b038356def652c29e349 Mon Sep 17 00:00:00 2001 From: Kat Date: Wed, 4 Oct 2023 14:33:24 +0100 Subject: [PATCH] Add issue explanation to the email --- app/jobs/email_missing_addresses_csv_job.rb | 4 +- app/mailers/csv_download_mailer.rb | 25 ++++++++-- lib/tasks/send_missing_addresses_csv.rake | 12 ++++- .../email_missing_addresses_csv_job_spec.rb | 20 ++++---- .../tasks/send_missing_addresses_csv_spec.rb | 46 ++++++++++++------- 5 files changed, 73 insertions(+), 34 deletions(-) diff --git a/app/jobs/email_missing_addresses_csv_job.rb b/app/jobs/email_missing_addresses_csv_job.rb index c39e79bd2..5cc014627 100644 --- a/app/jobs/email_missing_addresses_csv_job.rb +++ b/app/jobs/email_missing_addresses_csv_job.rb @@ -5,7 +5,7 @@ class EmailMissingAddressesCsvJob < ApplicationJob EXPIRATION_TIME = 1.week.to_i MISSING_ADDRESSES_THRESHOLD = 5 - def perform(user_ids, organisation, log_type, skip_uprn_issue_organisations) + def perform(user_ids, organisation, log_type, issue_types, skip_uprn_issue_organisations) csv_service = Csv::MissingAddressesCsvService.new(organisation, skip_uprn_issue_organisations) case log_type when "lettings" @@ -27,7 +27,7 @@ class EmailMissingAddressesCsvJob < ApplicationJob user = User.find(id) next if user.blank? - CsvDownloadMailer.new.send(email_method, user, url, EXPIRATION_TIME) + CsvDownloadMailer.new.send(email_method, user, url, EXPIRATION_TIME, issue_types) end end end diff --git a/app/mailers/csv_download_mailer.rb b/app/mailers/csv_download_mailer.rb index f8dcc5bc6..f35d1f271 100644 --- a/app/mailers/csv_download_mailer.rb +++ b/app/mailers/csv_download_mailer.rb @@ -11,19 +11,36 @@ class CsvDownloadMailer < NotifyMailer ) end - def send_missing_lettings_addresses_csv_download_mail(user, link, duration) + def send_missing_lettings_addresses_csv_download_mail(user, link, duration, issue_types) send_email( user.email, CSV_MISSING_LETTINGS_ADDRESSES_DOWNLOAD_TEMPLATE_ID, - { name: user.name, link:, duration: ActiveSupport::Duration.build(duration).inspect }, + { name: user.name, link:, issue_explanation: issue_explanation(issue_types), duration: ActiveSupport::Duration.build(duration).inspect }, ) end - def send_missing_sales_addresses_csv_download_mail(user, link, duration) + def send_missing_sales_addresses_csv_download_mail(user, link, duration, issue_types) send_email( user.email, CSV_MISSING_SALES_ADDRESSES_DOWNLOAD_TEMPLATE_ID, - { name: user.name, link:, duration: ActiveSupport::Duration.build(duration).inspect }, + { name: user.name, link:, issue_explanation: issue_explanation(issue_types), duration: ActiveSupport::Duration.build(duration).inspect }, ) end + +private + + def issue_explanation(issue_types) + issue_type_explanations = { + "missing_address" => "- Full address required: The UPRN in some logs is incorrect, so address data was not imported.", + "missing_town" => "- Missing town or city: The town or city in some logs is missing. This data is required in the new version of CORE.", + "wrong_uprn" => "- UPRN may be incorrect: The UPRN in some logs may be incorrect, so + wrong address data was imported. We think this is an issue because in + some logs the UPRN is the same as the tenant code or property reference, + and because your organisation has submitted logs for properties in Bristol + for the first time.", + } + + "Some address data is missing or incorrect. We've detected the following issues in + your logs imported to the new version of CORE:\n\n" + issue_types.map { |issue_type| issue_type_explanations[issue_type] }.join("\n") + end end diff --git a/lib/tasks/send_missing_addresses_csv.rake b/lib/tasks/send_missing_addresses_csv.rake index 773a247e8..1abb0382e 100644 --- a/lib/tasks/send_missing_addresses_csv.rake +++ b/lib/tasks/send_missing_addresses_csv.rake @@ -25,9 +25,13 @@ namespace :correct_addresses do missing_addresses_threshold = EmailMissingAddressesCsvJob::MISSING_ADDRESSES_THRESHOLD if logs_impacted_by_missing_address >= missing_addresses_threshold || logs_impacted_by_missing_town_or_city >= missing_addresses_threshold || logs_impacted_by_uprn_issue.any? + issue_types = [] + issue_types << "missing_address" if logs_impacted_by_missing_address.positive? + issue_types << "missing_town" if logs_impacted_by_missing_town_or_city.positive? + issue_types << "wrong_uprn" if logs_impacted_by_uprn_issue.any? data_coordinators = organisation.users.where(role: 2).filter_by_active users_to_contact = data_coordinators.any? ? data_coordinators : organisation.users.filter_by_active - EmailMissingAddressesCsvJob.perform_later(users_to_contact.map(&:id), organisation, "lettings", skip_uprn_issue_organisations) + EmailMissingAddressesCsvJob.perform_later(users_to_contact.map(&:id), organisation, "lettings", issue_types, skip_uprn_issue_organisations) Rails.logger.info("Sending missing lettings addresses CSV for #{organisation.name} to #{users_to_contact.map(&:email).join(', ')}") else Rails.logger.info("Missing addresses below threshold for #{organisation.name}") @@ -59,9 +63,13 @@ namespace :correct_addresses do end missing_addresses_threshold = EmailMissingAddressesCsvJob::MISSING_ADDRESSES_THRESHOLD if logs_impacted_by_missing_address >= missing_addresses_threshold || logs_impacted_by_missing_town_or_city >= missing_addresses_threshold || logs_impacted_by_uprn_issue.any? + issue_types = [] + issue_types << "missing_address" if logs_impacted_by_missing_address.positive? + issue_types << "missing_town" if logs_impacted_by_missing_town_or_city.positive? + issue_types << "wrong_uprn" if logs_impacted_by_uprn_issue.any? data_coordinators = organisation.users.where(role: 2).filter_by_active users_to_contact = data_coordinators.any? ? data_coordinators : organisation.users.filter_by_active - EmailMissingAddressesCsvJob.perform_later(users_to_contact.map(&:id), organisation, "sales", skip_uprn_issue_organisations) + EmailMissingAddressesCsvJob.perform_later(users_to_contact.map(&:id), organisation, "sales", issue_types, skip_uprn_issue_organisations) Rails.logger.info("Sending missing sales addresses CSV for #{organisation.name} to #{users_to_contact.map(&:email).join(', ')}") else Rails.logger.info("Missing addresses below threshold for #{organisation.name}") diff --git a/spec/jobs/email_missing_addresses_csv_job_spec.rb b/spec/jobs/email_missing_addresses_csv_job_spec.rb index 2b194af74..69d90cf51 100644 --- a/spec/jobs/email_missing_addresses_csv_job_spec.rb +++ b/spec/jobs/email_missing_addresses_csv_job_spec.rb @@ -29,38 +29,38 @@ describe EmailMissingAddressesCsvJob do context "when sending missing lettings logs csv" do it "uses an appropriate filename in S3" do expect(storage_service).to receive(:write_file).with(/missing-lettings-logs-addresses-#{organisation.name}-.*\.csv/, anything) - job.perform(users.map(&:id), organisation, "lettings", [1, 2]) + job.perform(users.map(&:id), organisation, "lettings", %w[missing_address wrong_uprn], [1, 2]) end it "creates a MissingAddressesCsvService with the correct organisation and calls create missing lettings logs adresses csv" do expect(Csv::MissingAddressesCsvService).to receive(:new).with(organisation, [1, 2]) expect(missing_addresses_csv_service).to receive(:create_missing_lettings_addresses_csv) - job.perform(users.map(&:id), organisation, "lettings", [1, 2]) + job.perform(users.map(&:id), organisation, "lettings", %w[missing_address wrong_uprn], [1, 2]) end it "sends emails to all the provided users" do - expect(mailer).to receive(:send_missing_lettings_addresses_csv_download_mail).with(users[0], test_url, instance_of(Integer)) - expect(mailer).to receive(:send_missing_lettings_addresses_csv_download_mail).with(users[1], test_url, instance_of(Integer)) - job.perform(users.map(&:id), organisation, "lettings", [1, 2]) + expect(mailer).to receive(:send_missing_lettings_addresses_csv_download_mail).with(users[0], test_url, instance_of(Integer), %w[missing_address wrong_uprn]) + expect(mailer).to receive(:send_missing_lettings_addresses_csv_download_mail).with(users[1], test_url, instance_of(Integer), %w[missing_address wrong_uprn]) + job.perform(users.map(&:id), organisation, "lettings", %w[missing_address wrong_uprn], [1, 2]) end end context "when sending missing sales logs csv" do it "uses an appropriate filename in S3" do expect(storage_service).to receive(:write_file).with(/missing-sales-logs-addresses-#{organisation.name}-.*\.csv/, anything) - job.perform(users.map(&:id), organisation, "sales", [2, 3]) + job.perform(users.map(&:id), organisation, "sales", %w[missing_town], [2, 3]) end it "creates a MissingAddressesCsvService with the correct organisation and calls create missing sales logs adresses csv" do expect(Csv::MissingAddressesCsvService).to receive(:new).with(organisation, [2, 3]) expect(missing_addresses_csv_service).to receive(:create_missing_sales_addresses_csv) - job.perform(users.map(&:id), organisation, "sales", [2, 3]) + job.perform(users.map(&:id), organisation, "sales", %w[missing_town], [2, 3]) end it "sends emails to all the provided users" do - expect(mailer).to receive(:send_missing_sales_addresses_csv_download_mail).with(users[0], test_url, instance_of(Integer)) - expect(mailer).to receive(:send_missing_sales_addresses_csv_download_mail).with(users[1], test_url, instance_of(Integer)) - job.perform(users.map(&:id), organisation, "sales", [2, 3]) + expect(mailer).to receive(:send_missing_sales_addresses_csv_download_mail).with(users[0], test_url, instance_of(Integer), %w[missing_town]) + expect(mailer).to receive(:send_missing_sales_addresses_csv_download_mail).with(users[1], test_url, instance_of(Integer), %w[missing_town]) + job.perform(users.map(&:id), organisation, "sales", %w[missing_town], [2, 3]) end end end diff --git a/spec/lib/tasks/send_missing_addresses_csv_spec.rb b/spec/lib/tasks/send_missing_addresses_csv_spec.rb index a9ea54896..aae5edba6 100644 --- a/spec/lib/tasks/send_missing_addresses_csv_spec.rb +++ b/spec/lib/tasks/send_missing_addresses_csv_spec.rb @@ -59,7 +59,7 @@ RSpec.describe "correct_addresses" do end it "enqueues the job with correct organisations" do - expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "lettings", []) + expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "lettings", %w[missing_address], []) end it "prints out the jobs enqueued" do @@ -78,7 +78,7 @@ RSpec.describe "correct_addresses" do end it "enqueues the job with correct organisations" do - expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings", []) + expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings", %w[missing_address], []) end it "prints out the jobs enqueued" do @@ -109,7 +109,7 @@ RSpec.describe "correct_addresses" do end it "enqueues the job with correct organisations" do - expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "lettings", []) + expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "lettings", %w[missing_town], []) end it "prints out the jobs enqueued" do @@ -128,7 +128,7 @@ RSpec.describe "correct_addresses" do end it "enqueues the job with correct organisations" do - expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings", []) + expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings", %w[missing_town], []) end it "prints out the jobs enqueued" do @@ -159,7 +159,7 @@ RSpec.describe "correct_addresses" do end it "enqueues the job with correct organisations" do - expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "lettings", []) + expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "lettings", %w[wrong_uprn], []) end it "prints out the jobs enqueued" do @@ -178,7 +178,7 @@ RSpec.describe "correct_addresses" do end it "enqueues the job with correct organisations" do - expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings", []) + expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings", %w[wrong_uprn], []) end it "prints out the jobs enqueued" do @@ -198,7 +198,7 @@ RSpec.describe "correct_addresses" do end it "enqueues the job with correct organisations" do - expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings", []) + expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings", %w[wrong_uprn], []) end it "prints out the jobs enqueued" do @@ -218,6 +218,20 @@ RSpec.describe "correct_addresses" do end end + context "when org is included in skip_uprn_issue_organisations list but faces a different issue" do + let!(:data_provider) { create(:user, :data_provider, organisation:, email: "data_provider3@example.com") } + let!(:data_provider2) { create(:user, :data_provider, organisation:, email: "data_provider4@example.com") } + + before do + create_list(:lettings_log, 5, :imported, startdate: Time.zone.local(2023, 9, 9), address_line1: nil, town_or_city: nil, needstype: 1, old_form_id: "form_2", owning_organisation: organisation, managing_organisation: organisation, created_by: organisation.users.first) + create_list(:lettings_log, 5, :imported, startdate: Time.zone.local(2023, 9, 9), uprn: "12", propcode: "12", needstype: 1, owning_organisation: organisation, managing_organisation: organisation, created_by: organisation.users.first) + end + + it "does not enqueue the job" do + expect { task.invoke(organisation.id.to_s) }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings", %w[missing_address], [organisation.id]) + end + end + context "when skip_uprn_issue_organisations list is provided" do let!(:data_provider) { create(:user, :data_provider, organisation:, email: "data_provider3@example.com") } let!(:data_provider2) { create(:user, :data_provider, organisation:, email: "data_provider4@example.com") } @@ -227,7 +241,7 @@ RSpec.describe "correct_addresses" do end it "does enqueues the job with correct skip_uprn_issue_organisations" do - expect { task.invoke("100 400") }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings", [100, 400]) + expect { task.invoke("100 400") }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings", %w[wrong_uprn], [100, 400]) end end end @@ -290,7 +304,7 @@ RSpec.describe "correct_addresses" do end it "enqueues the job with correct organisations" do - expect { task.invoke("70 90") }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "sales", [70, 90]) + expect { task.invoke("70 90") }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "sales", %w[missing_address], [70, 90]) end it "prints out the jobs enqueued" do @@ -309,7 +323,7 @@ RSpec.describe "correct_addresses" do end it "enqueues the job with correct organisations" do - expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales", []) + expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales", %w[missing_address], []) end it "prints out the jobs enqueued" do @@ -340,7 +354,7 @@ RSpec.describe "correct_addresses" do end it "enqueues the job with correct organisations" do - expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "sales", []) + expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "sales", %w[missing_town], []) end it "prints out the jobs enqueued" do @@ -359,7 +373,7 @@ RSpec.describe "correct_addresses" do end it "enqueues the job with correct organisations" do - expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales", []) + expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales", %w[missing_town], []) end it "prints out the jobs enqueued" do @@ -390,7 +404,7 @@ RSpec.describe "correct_addresses" do end it "enqueues the job with correct organisations" do - expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "sales", []) + expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "sales", %w[wrong_uprn], []) end it "prints out the jobs enqueued" do @@ -409,7 +423,7 @@ RSpec.describe "correct_addresses" do end it "enqueues the job with correct organisations" do - expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales", []) + expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales", %w[wrong_uprn], []) end it "prints out the jobs enqueued" do @@ -429,7 +443,7 @@ RSpec.describe "correct_addresses" do end it "enqueues the job with correct organisations" do - expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales", []) + expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales", %w[wrong_uprn], []) end it "prints out the jobs enqueued" do @@ -458,7 +472,7 @@ RSpec.describe "correct_addresses" do end it "does enqueues the job with correct skip_uprn_issue_organisations" do - expect { task.invoke("100 400") }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales", [100, 400]) + expect { task.invoke("100 400") }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales", %w[wrong_uprn], [100, 400]) end end end