From af1fe815de2d0ce36c818d39a9d3864d664d8c7f Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Wed, 1 Mar 2023 16:28:44 +0000 Subject: [PATCH] CLDC-2065 Bulk upload error summary minimum limit (#1347) * bulk upload error summary min limit - the errors summary table only show errors for column where there at least 16 errors for that column - if there are fewer than 16 errors that can see detailed report to pin point and fix those specific issues * add #errors? to summary component * show full error report if not enough errors - if errors summary table is empty as below threshold - send users to full report - remove the back button so they cannot access summary report * add upload again cta otherwise a dead end --- ...lk_upload_error_summary_table_component.rb | 13 +++++++ app/mailers/bulk_upload_mailer.rb | 8 ++++- .../show.html.erb | 8 +++-- ...load_error_summary_table_component_spec.rb | 35 +++++++++++++++++++ spec/mailers/bulk_upload_mailer_spec.rb | 4 ++- 5 files changed, 64 insertions(+), 4 deletions(-) diff --git a/app/components/bulk_upload_error_summary_table_component.rb b/app/components/bulk_upload_error_summary_table_component.rb index ac236af4d..a3d295110 100644 --- a/app/components/bulk_upload_error_summary_table_component.rb +++ b/app/components/bulk_upload_error_summary_table_component.rb @@ -1,4 +1,6 @@ class BulkUploadErrorSummaryTableComponent < ViewComponent::Base + DISPLAY_THRESHOLD = 16 + attr_reader :bulk_upload def initialize(bulk_upload:) @@ -11,7 +13,18 @@ class BulkUploadErrorSummaryTableComponent < ViewComponent::Base @sorted_errors ||= bulk_upload .bulk_upload_errors .group(:col, :field, :error) + .having("count(*) > ?", display_threshold) .count .sort_by { |el| el[0][0].rjust(3, "0") } end + + def errors? + sorted_errors.present? + end + +private + + def display_threshold + DISPLAY_THRESHOLD + end end diff --git a/app/mailers/bulk_upload_mailer.rb b/app/mailers/bulk_upload_mailer.rb index be0a71e05..94079828f 100644 --- a/app/mailers/bulk_upload_mailer.rb +++ b/app/mailers/bulk_upload_mailer.rb @@ -46,6 +46,12 @@ class BulkUploadMailer < NotifyMailer 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 + bulk_upload_lettings_result_url(bulk_upload) + end + send_email( bulk_upload.user.email, BULK_UPLOAD_FAILED_CSV_ERRORS_TEMPLATE_ID, @@ -55,7 +61,7 @@ class BulkUploadMailer < NotifyMailer year_combo: bulk_upload.year_combo, lettings_or_sales: bulk_upload.log_type, error_description:, - summary_report_link: summary_bulk_upload_lettings_result_url(bulk_upload), + summary_report_link:, }, ) end diff --git a/app/views/bulk_upload_lettings_results/show.html.erb b/app/views/bulk_upload_lettings_results/show.html.erb index 84374e26a..9a8ccddec 100644 --- a/app/views/bulk_upload_lettings_results/show.html.erb +++ b/app/views/bulk_upload_lettings_results/show.html.erb @@ -1,5 +1,7 @@ -<% content_for :before_content do %> - <%= govuk_back_link(text: "Back", href: summary_bulk_upload_lettings_result_path(@bulk_upload)) %> +<% if BulkUploadErrorSummaryTableComponent.new(bulk_upload: @bulk_upload).errors? %> + <% content_for :before_content do %> + <%= govuk_back_link(text: "Back", href: summary_bulk_upload_lettings_result_path(@bulk_upload)) %> + <% end %> <% end %>
@@ -22,3 +24,5 @@ <% end %>
+ +<%= govuk_button_link_to "Upload your file again", start_bulk_upload_lettings_logs_path %> 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 cdb0b58bf..a7468d70a 100644 --- a/spec/components/bulk_upload_error_summary_table_component_spec.rb +++ b/spec/components/bulk_upload_error_summary_table_component_spec.rb @@ -5,6 +5,10 @@ RSpec.describe BulkUploadErrorSummaryTableComponent, type: :component do let(:bulk_upload) { create(:bulk_upload) } + before do + stub_const("BulkUploadErrorSummaryTableComponent::DISPLAY_THRESHOLD", 0) + end + context "when no errors" do it "does not renders any rows" do result = render_inline(component) @@ -12,6 +16,19 @@ RSpec.describe BulkUploadErrorSummaryTableComponent, type: :component do end end + context "when below threshold" do + before do + stub_const("BulkUploadErrorSummaryTableComponent::DISPLAY_THRESHOLD", 16) + + create(:bulk_upload_error, bulk_upload:, col: "A", row: 1) + end + + it "does not render rows" do + result = render_inline(component) + expect(result).to have_selector("tbody tr", count: 0) + 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) } @@ -78,4 +95,22 @@ RSpec.describe BulkUploadErrorSummaryTableComponent, type: :component do ]) end end + + describe "#errors?" do + context "when there are no errors" do + it "returns false" do + expect(component).not_to be_errors + end + end + + context "when there are errors" do + before do + create(:bulk_upload_error, bulk_upload:, col: "A", row: 2, field: "field_1") + end + + it "returns true" do + expect(component).to be_errors + end + end + end end diff --git a/spec/mailers/bulk_upload_mailer_spec.rb b/spec/mailers/bulk_upload_mailer_spec.rb index d0217bfaf..a0042cf69 100644 --- a/spec/mailers/bulk_upload_mailer_spec.rb +++ b/spec/mailers/bulk_upload_mailer_spec.rb @@ -93,7 +93,7 @@ RSpec.describe BulkUploadMailer do 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}/summary", + summary_report_link: "http://localhost:3000/lettings-logs/bulk-upload-results/#{bulk_upload.id}", }, ) @@ -103,6 +103,8 @@ RSpec.describe BulkUploadMailer do 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")