SRCH-2243 change bulk upload email error message order #758
Conversation
|
@jmax-fearless , please squash the commits into a single commit that includes the Jira ticket # in the commit message. |
Made 'Url already taken' messages show last in the email
| 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 |
There was a problem hiding this comment.
@jmax-fearless , this is not a valid spec, because it passes without any of the changes in spec/services/bulk_url_uploader_spec.rb. The before block sets the errors in the correct order, so the spec is testing errors that are already in the expected order:
before do
results.add_error(first_error_message, first_bad_url)
results.add_error(second_error_message, second_bad_url)
results.add_error(already_taken_error_message, duplicate_url)
endPlease add a valid spec using TDD. As the results are being ordered in BulkUrlUploader::Results, you could add a unit test for that class, rather than testing this via the mailer.
There was a problem hiding this comment.
I'm not comfortable removing the mailer spec, but I agree that a unit test for this would be a fine thing. So I added one, and also re-arranged the order of add_error calls per your comment. Both tests fail if you remove the changes to #error_messages
Moved the new spec for BulkUrlUploader::Results out to its own file, per CodeClimate's wishes.
| errors = @errors.keys | ||
| errors.reject(&already_taken) + errors.select(&already_taken) | ||
|
|
||
| # @errors.keys |
| subject(:results) { described_class.new('test bulk uploader') } | ||
|
|
||
| describe '#errors' do | ||
| context 'when there are "url not found" errors' do |
There was a problem hiding this comment.
This context should be when there are "Url has already been taken" errors
| results.add_error('Validation failed: Url has already been taken', 'junk') | ||
| results.add_error('Validation failed: SearchgovDomain is not a valid SearchgovDomain', 'junk') |
There was a problem hiding this comment.
Please change junk to a URL, i.e. https://alreadytaken.gov/https://invaliddomain.gov. That will help document the behavior of the code, whereas a future developer looking at this spec would not know what 'junk' might be.
Changed name of context and made url names more verbose.
SRCH-2243 change bulk upload email error message order.
Make 'Url already taken' errors appear after all other errors.