3688 error handling #3702
3688 error handling #3702
Conversation
…age location given
…th 'What location are you auditing?'
app/controllers/audits_controller.rb
Outdated
| def handle_audit_errors | ||
| if @audit.errors.present? | ||
| errors = @audit.errors.collect { |error| "#{error.attribute.capitalize} ".tr("_", " ") + error.message } | ||
| flash[:error] = errors[1] |
There was a problem hiding this comment.
This smells like a magic number - why are we getting only the second error?
There was a problem hiding this comment.
I grabbed the second one because that error message was the closest to what @cielf had said in the ticket that it should say. I am happy to display both of them if you would prefer.
There was a problem hiding this comment.
Relying on the order of errors is incredibly fragile. If you are trying to condense two similar errors into one, you might want to actually examine the strings for sentinel values and remove one if you find two of them.
There was a problem hiding this comment.
@dorner Heard! No problem 😉 Is this more like what you would like to see?
There was a problem hiding this comment.
Hmm... not quite. What this is doing is picking out one specific error message (one that has "blank" in it) and only showing it if it exists, and swallowing all other messages.
I was thinking something like:
flash[:error] = @audit.errors.uniq(&:attribute).map(&:message)That should make it so that you only get one error message per attribute.
There was a problem hiding this comment.
Ahhhh! okay! I see! I was able to implement your suggestion while displaying the attribute of the error with the error message.
| it "re-renders the 'new' template with an error message when an invalid storage location is given" do | ||
| post audits_path(default_params.merge(audit: invalid_storage_location_attributes)) | ||
| expect(response).to render_template(:new) | ||
| expect(flash[:error]).to_not eq("Storage location can't be blank") |
There was a problem hiding this comment.
This line isn't necessary - if something is equal to X (next line) by definition it isn't equal to anything but X 😁
|
Nice, thanks! |
Checklist:
[x]I have performed a self-review of my own code,
[x]I have commented my code, particularly in hard-to-understand areas,
[x]I have made corresponding changes to the documentation,
[x]I have added tests that prove my fix is effective or that my feature works,
[x]New and existing unit tests pass locally with my changes ("bundle exec rake"),
Resolves #3688
Description
Re-worked error message related to Storage Location, and refactored error handling into a private method with a custom error message to clearly convey to the user what the error is. Re-labeled the 'From Storage Location' field to 'Storage location', and re-named the error associated with the field.
List any dependencies that are required for this change. (gems, js libraries, etc.)
Include anything else we should know about. -->
Type of change
How Has This Been Tested?
Tested in the audit_request_spec.rb This test ensures that the correct template is rendered if no Storage location is selected, and that the flash message is rendering the custom error message.
Provide instructions so we can reproduce.
Do we need to do anything else to verify your changes?
If so, provide instructions (including any relevant configuration) so that we can reproduce? -->
To reproduce the rendered error messages. Log in to the application as a Bank User - Organization Admin, begin a new audit without selecting a storage location, save the audit, and the custom error messages will be displayed.
Screenshots