diff --git a/app/components/bulk_upload_error_summary_table_component.rb b/app/components/bulk_upload_error_summary_table_component.rb index 909fb5f0d..8dddb61d2 100644 --- a/app/components/bulk_upload_error_summary_table_component.rb +++ b/app/components/bulk_upload_error_summary_table_component.rb @@ -12,7 +12,7 @@ class BulkUploadErrorSummaryTableComponent < ViewComponent::Base end def sorted_errors - @sorted_errors ||= bulk_upload + @sorted_errors ||= setup_errors.presence || bulk_upload .bulk_upload_errors .group(:col, :field, :error) .having("count(*) > ?", display_threshold) @@ -26,6 +26,16 @@ class BulkUploadErrorSummaryTableComponent < ViewComponent::Base private + def setup_errors + @setup_errors ||= bulk_upload + .bulk_upload_errors + .where(category: "setup") + .group(:col, :field, :error) + .having("count(*) > ?", display_threshold) + .count + .sort_by { |el| el[0][0].rjust(3, "0") } + end + def display_threshold DISPLAY_THRESHOLD end diff --git a/app/mailers/bulk_upload_mailer.rb b/app/mailers/bulk_upload_mailer.rb index 144acb3e1..d91842bce 100644 --- a/app/mailers/bulk_upload_mailer.rb +++ b/app/mailers/bulk_upload_mailer.rb @@ -33,19 +33,7 @@ class BulkUploadMailer < NotifyMailer ) end - def columns_with_errors(bulk_upload:) - array = bulk_upload.columns_with_errors - - if array.size > 3 - "#{array.take(3).join(', ')} and more" - else - array.join(", ") - 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." - summary_report_link = if BulkUploadErrorSummaryTableComponent.new(bulk_upload:).errors? summary_bulk_upload_lettings_result_url(bulk_upload) else @@ -60,7 +48,6 @@ class BulkUploadMailer < NotifyMailer 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:, summary_report_link:, }, ) diff --git a/app/models/bulk_upload.rb b/app/models/bulk_upload.rb index ff334b968..0952b60af 100644 --- a/app/models/bulk_upload.rb +++ b/app/models/bulk_upload.rb @@ -30,14 +30,6 @@ class BulkUpload < ApplicationRecord end end - def columns_with_errors - bulk_upload_errors - .select(:col) - .distinct(:col) - .pluck(:col) - .sort_by { |col| col.rjust(2, "0") } - end - def general_needs? needstype == 1 end diff --git a/spec/components/bulk_upload_error_summary_table_component_spec.rb b/spec/components/bulk_upload_error_summary_table_component_spec.rb index 4c307515c..32da38119 100644 --- a/spec/components/bulk_upload_error_summary_table_component_spec.rb +++ b/spec/components/bulk_upload_error_summary_table_component_spec.rb @@ -9,87 +9,114 @@ RSpec.describe BulkUploadErrorSummaryTableComponent, type: :component do stub_const("BulkUploadErrorSummaryTableComponent::DISPLAY_THRESHOLD", 0) end - context "when no errors" do - it "does not renders any tables" do - result = render_inline(component) - expect(result).not_to have_selector("table") + describe "#sorted_errors" do + context "when no errors" do + it "does not renders any tables" do + result = render_inline(component) + expect(result).not_to have_selector("table") + end end - end - context "when below threshold" do - before do - stub_const("BulkUploadErrorSummaryTableComponent::DISPLAY_THRESHOLD", 16) + context "when below threshold" do + before do + stub_const("BulkUploadErrorSummaryTableComponent::DISPLAY_THRESHOLD", 16) - create(:bulk_upload_error, bulk_upload:, col: "A", row: 1) - end + create(:bulk_upload_error, bulk_upload:, col: "A", row: 1) + end - it "does not render tables" do - result = render_inline(component) - expect(result).to have_selector("table", count: 0) + it "does not render tables" do + result = render_inline(component) + expect(result).to have_selector("table", count: 0) + end end - end - context "when there are 2 independent errors" do - let!(:error_2) { create(:bulk_upload_error, bulk_upload:, col: "B", row: 2) } - let!(:error_1) { create(:bulk_upload_error, bulk_upload:, col: "A", row: 1) } + context "when there are 2 independent errors" do + let!(:error_2) { create(:bulk_upload_error, bulk_upload:, col: "B", row: 2) } + let!(:error_1) { create(:bulk_upload_error, bulk_upload:, col: "A", row: 1) } - it "renders table for each error" do - result = render_inline(component) - expect(result).to have_selector("table", count: 2) - end + it "renders table for each error" do + result = render_inline(component) + expect(result).to have_selector("table", count: 2) + end - it "renders by col order" do - result = render_inline(component) - order = result.css("table thead th:nth-of-type(2)").map(&:content) - expect(order).to eql(["Column A", "Column B"]) - end + it "renders by col order" do + result = render_inline(component) + order = result.css("table thead th:nth-of-type(2)").map(&:content) + expect(order).to eql(["Column A", "Column B"]) + end - it "render correct data" do - result = render_inline(component) + it "render correct data" do + result = render_inline(component) - table_1 = result.css("table").first.css("th, td").map(&:content) + table_1 = result.css("table").first.css("th, td").map(&:content) - expect(table_1).to eql([ - bulk_upload.prefix_namespace::RowParser.question_for_field(error_1.field.to_sym).to_s, - "Column A", - error_1.error, - "1 error", - ]) + expect(table_1).to eql([ + bulk_upload.prefix_namespace::RowParser.question_for_field(error_1.field.to_sym).to_s, + "Column A", + error_1.error, + "1 error", + ]) - table_2 = result.css("table")[1].css("th, td").map(&:content) + table_2 = result.css("table")[1].css("th, td").map(&:content) - expect(table_2).to eql([ - bulk_upload.prefix_namespace::RowParser.question_for_field(error_2.field.to_sym).to_s, - "Column B", - error_2.error, - "1 error", - ]) + expect(table_2).to eql([ + bulk_upload.prefix_namespace::RowParser.question_for_field(error_2.field.to_sym).to_s, + "Column B", + error_2.error, + "1 error", + ]) + end end - end - context "when there are 2 grouped errors" do - let!(:error_1) { create(:bulk_upload_error, bulk_upload:, col: "A", row: 1, field: "field_1") } + context "when there are 2 grouped errors" do + let!(:error_1) { create(:bulk_upload_error, bulk_upload:, col: "A", row: 1, field: "field_1") } - before do - create(:bulk_upload_error, bulk_upload:, col: "A", row: 2, field: "field_1") - end + before do + create(:bulk_upload_error, bulk_upload:, col: "A", row: 2, field: "field_1") + end + + it "renders 1 table combining the errors" do + result = render_inline(component) + expect(result).to have_selector("table", count: 1) + end - it "renders 1 table combining the errors" do - result = render_inline(component) - expect(result).to have_selector("table", count: 1) + it "render correct data" do + result = render_inline(component) + + table_1 = result.css("table").css("th, td").map(&:content) + + expect(table_1).to eql([ + bulk_upload.prefix_namespace::RowParser.question_for_field(error_1.field.to_sym).to_s, + "Column A", + error_1.error, + "2 errors", + ]) + end end - it "render correct data" do - result = render_inline(component) + context "when mix of setup and other errors" do + let!(:error_1) { create(:bulk_upload_error, bulk_upload:, col: "A", row: 1, category: "setup") } - table_1 = result.css("table").css("th, td").map(&:content) + before do + create(:bulk_upload_error, bulk_upload:, col: "B", row: 2, category: nil) + end + + it "only returns the setup errors" do + result = render_inline(component) + + tables = result.css("table") - expect(table_1).to eql([ - bulk_upload.prefix_namespace::RowParser.question_for_field(error_1.field.to_sym).to_s, - "Column A", - error_1.error, - "2 errors", - ]) + expect(tables.size).to be(1) + + table = result.css("table").css("th, td").map(&:content) + + expect(table).to eql([ + bulk_upload.prefix_namespace::RowParser.question_for_field(error_1.field.to_sym).to_s, + "Column A", + error_1.error, + "1 error", + ]) + end end end diff --git a/spec/mailers/bulk_upload_mailer_spec.rb b/spec/mailers/bulk_upload_mailer_spec.rb index 9f59de365..2e4337328 100644 --- a/spec/mailers/bulk_upload_mailer_spec.rb +++ b/spec/mailers/bulk_upload_mailer_spec.rb @@ -116,7 +116,7 @@ RSpec.describe BulkUploadMailer do create(:bulk_upload_error, bulk_upload:, col: "B") end - it "sends correctly formed email with A, B" 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, @@ -125,7 +125,6 @@ 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}", }, ) @@ -133,33 +132,5 @@ RSpec.describe BulkUploadMailer do mailer.send_correct_and_upload_again_mail(bulk_upload:) end end - - context "when 4 columns with errors" do - before do - stub_const("BulkUploadErrorSummaryTableComponent::DISPLAY_THRESHOLD", 0) - - create(:bulk_upload_error, bulk_upload:, col: "A") - create(:bulk_upload_error, bulk_upload:, col: "B") - create(:bulk_upload_error, bulk_upload:, col: "C") - create(:bulk_upload_error, bulk_upload:, col: "D") - end - - it "sends correctly formed email with A, B, C and more" do - expect(notify_client).to receive(:send_email).with( - email_address: user.email, - template_id: described_class::BULK_UPLOAD_FAILED_CSV_ERRORS_TEMPLATE_ID, - personalisation: { - filename: bulk_upload.filename, - 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.", - summary_report_link: "http://localhost:3000/lettings-logs/bulk-upload-results/#{bulk_upload.id}/summary", - }, - ) - - mailer.send_correct_and_upload_again_mail(bulk_upload:) - end - end end end