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

@@ -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)

spec/mailers/bulk_url_upload_results_mailer_spec.rb

@@ -16,14 +16,18 @@
   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
   describe '#results_email' do
     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

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')

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