From 83ba83aac25b84416c4efb6414534ea6026129f2 Mon Sep 17 00:00:00 2001 From: jmax-fearless <66315527+jmax-fearless@users.noreply.github.com> Date: Tue, 3 Aug 2021 14:39:41 -0400 Subject: [PATCH] SRCH-2243 change bulk upload email error message order (#758) SRCH-2243 - Change bulk upload email error message order. Made 'Url already taken' messages show last in the email --- app/services/bulk_url_uploader.rb | 9 +++- .../bulk_url_upload_results_mailer_spec.rb | 42 ++++++++----------- .../bulk_url_upload_results_mailer_preview.rb | 1 + .../bulk_url_uploader/results_spec.rb | 23 ++++++++++ 4 files changed, 49 insertions(+), 26 deletions(-) create mode 100644 spec/services/bulk_url_uploader/results_spec.rb diff --git a/app/services/bulk_url_uploader.rb b/app/services/bulk_url_uploader.rb index 6ca043c228..f20f84540d 100644 --- a/app/services/bulk_url_uploader.rb +++ b/app/services/bulk_url_uploader.rb @@ -3,6 +3,7 @@ class BulkUrlUploader MAXIMUM_FILE_SIZE = 4.megabytes VALID_CONTENT_TYPES = %w[text/plain].freeze + URL_ALREADY_TAKEN_MESSAGE = 'Validation failed: Url has already been taken' attr_reader :results @@ -35,7 +36,13 @@ def total_count end def error_messages - @errors.keys + # Make sure the 'URL already taken' errors show up last, so as + # not to obscure more actionable errors + already_taken = ->(error) { error == URL_ALREADY_TAKEN_MESSAGE } + errors = @errors.keys + errors.reject(&already_taken) + errors.select(&already_taken) + + # @errors.keys end def urls_with(error_message) diff --git a/spec/mailers/bulk_url_upload_results_mailer_spec.rb b/spec/mailers/bulk_url_upload_results_mailer_spec.rb index 02fa4821d3..443c36dc75 100644 --- a/spec/mailers/bulk_url_upload_results_mailer_spec.rb +++ b/spec/mailers/bulk_url_upload_results_mailer_spec.rb @@ -16,6 +16,10 @@ it 'has the correct reply-to' do expect(mail.reply_to).to eq([ApplicationMailer::REPLY_TO_EMAIL_ADDRESS]) end + + it 'has the correct total number of URLs' do + expect(mail_body).to match(/There were #{results.total_count} URLs/) + end end RSpec.describe BulkUrlUploadResultsMailer, type: :mailer do @@ -23,7 +27,7 @@ let(:user) { users(:affiliate_admin) } let(:filename) { 'test-file.txt' } let(:results) do - results= BulkUrlUploader::Results.new(filename) + results = BulkUrlUploader::Results.new(filename) results.add_ok(SearchgovUrl.create(url: 'https://agency.gov/ok-url-1')) results.add_ok(SearchgovUrl.create(url: 'https://agency.gov/ok-url-2')) results @@ -36,48 +40,27 @@ describe 'with no errors' do it_behaves_like 'a bulk upload notification email' - it 'has the correct total number of URLs' do - expect(mail_body).to match(/There were #{results.total_count} URLs/) - end - it 'reports that there were no URLs with problems' do expect(mail_body).to match(/There were no errors/) end end describe 'with errors' do + let(:already_taken_error_message) { 'Validation failed: Url has already been taken' } + let(:duplicate_url) { 'https://duplicate.agency.gov' } let(:first_error_message) { 'First validation failure' } let(:first_bad_url) { 'https://agency.gov/first-bad-url' } let(:second_error_message) { 'Second validation failure' } let(:second_bad_url) { 'https://agency.gov/second-bad-url' } before do + results.add_error(already_taken_error_message, duplicate_url) results.add_error(first_error_message, first_bad_url) results.add_error(second_error_message, second_bad_url) end it_behaves_like 'a bulk upload notification email' - it 'has the correct subject' do - expect(mail.subject).to eq("Bulk URL upload results for #{filename}") - end - - it 'has the correct recepient' do - expect(mail.to).to eq([user.email]) - end - - it 'has the correct from header' do - expect(mail.from).to eq([ApplicationMailer::DELIVER_FROM_EMAIL_ADDRESS]) - end - - it 'has the correct reply-to' do - expect(mail.reply_to).to eq([ApplicationMailer::REPLY_TO_EMAIL_ADDRESS]) - end - - it 'has the correct total number of URLs' do - expect(mail_body).to match(/There were #{results.total_count} URLs/) - end - it 'reports the correct number of OK URLs' do expect(mail_body).to match(/#{results.ok_count} URLs were created successfully/) end @@ -93,6 +76,15 @@ it 'shows the second URL validation failure' do expect(mail_body).to match(/#{second_error_message}\s+#{second_bad_url}/) end + + it 'shows the urls with error "url already taken" after all the other errors' do + first_error_message_position = mail_body.index(first_error_message) + second_error_message_position = mail_body.index(second_error_message) + already_taken_error_message_position = mail_body.index(already_taken_error_message) + + expect(already_taken_error_message_position).to be > first_error_message_position + expect(already_taken_error_message_position).to be > second_error_message_position + end end end end diff --git a/spec/mailers/previews/bulk_url_upload_results_mailer_preview.rb b/spec/mailers/previews/bulk_url_upload_results_mailer_preview.rb index 05ce9ddb28..8aacbf3f25 100644 --- a/spec/mailers/previews/bulk_url_upload_results_mailer_preview.rb +++ b/spec/mailers/previews/bulk_url_upload_results_mailer_preview.rb @@ -15,6 +15,7 @@ def results_email_with_errors user = User.first results = BulkUrlUploader::Results.new('the-file.txt') results.add_ok(SearchgovUrl.create(url: 'https://ok-url.test')) + results.add_error('Url has already been taken', 'https://taken.test') results.add_error('one error', 'https://bogus.test') results.add_error('one error', 'https://realy-bogus.test') results.add_error('another error', 'https://left-field.test') diff --git a/spec/services/bulk_url_uploader/results_spec.rb b/spec/services/bulk_url_uploader/results_spec.rb new file mode 100644 index 0000000000..2e2e023f88 --- /dev/null +++ b/spec/services/bulk_url_uploader/results_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +describe BulkUrlUploader::Results do + subject(:results) { described_class.new('test bulk uploader') } + + describe '#errors' do + context 'when there are "url already taken" errors' do + before do + results.add_error('Validation failed: Url has already been taken', 'https://irrelevant-to-the-spec.gov') + results.add_error('Validation failed: SearchgovDomain is not a valid SearchgovDomain', 'https://irrelevant-to-the-spec.gov') + end + + it 'puts them at the end of the list' do + expect(results.error_messages).to eq( + [ + 'Validation failed: SearchgovDomain is not a valid SearchgovDomain', + 'Validation failed: Url has already been taken' + ] + ) + end + end + end +end