rails icon indicating copy to clipboard operation
rails copied to clipboard

Add custom_error to ActiveModel::Error and ActiveModel::Errors

Open pinzonjulian opened this issue 2 years ago • 7 comments

  • Summary
  • Context
    • Listing errors at the top of a long form
    • Error per input field
      • Current behaviour
  • Implemented behaviour: custom_message
    • Usage
  • Notes on implementation
  • Future work

Summary

Note: This PR is a new take on https://github.com/rails/rails/pull/43283 by @ollieh-m based on the comments provided by me and @rafaelfranca. Note that the description of this PR is similar but not the same as the comment I made on that PR so please read again if you already read the other one :)

Context

This PR adds the custom_message functionality to ActiveModel::Error and helper methods in ActiveModel::Errors in an effort to make feedback in form fields nicer.

Listing errors at the top of a long form

When creating a form, specially a long one, it's usually a good idea for all errors to be at the top of the form. This makes it specially important for users of your app that use assistive technologies (more on that here)

Each item in this list describes a) the attribute that has an error and b) the error on that attribute. Each list item can potentially have a link with an anchor to the element that needs to be corrected

image

This message is currently constructed using the full_message method which concatenates the name of the attribute with the error message.

Error per input field

Current behaviour

As UX on the web has evolved, it's become commonplace to have the error message alongside the input field. This makes it a little bit redundant to have the name of the attribute be repeated. The following example showcases what an input field would look like using the full_messages_for(:attribute) method.

image

Implemented behaviour: custom_message

I've added a custom_message method to create an alternative message in forms like these that don't need the attribute's name or might not follow the format attribute + message. In working in this PR I discovered a feature in full_messages that allows its format to be modified from #{name} #{message} to something else and also a PR https://github.com/rails/rails/pull/42708 that allows this configuration to be made per validation.

However, neither of these accomplish the purpose of this feature to have 2 distinct ways to describe error messages: one conventionally formatted with full_message and one friendlier with custom_message.

image

Usage

<%= form_with(model: @person) do |form|%>
<!-- for accessibility -->
<ul>
<% form.object.errors.full_messages.each do |message| %>
  <li><%= message %></li>
</ul>
<%= form.email_field :email %>
<p class="error"><%= form.object.errors.custom_message_for(:email) %></p>
<% end %>

Notes on the implementation

  1. I based the implementation and tests for the ActiveModel::Error class on the message and generate_message already present.
  2. I decided to go with a new structure for the YAML translation file where the type of error (e.g. blank) is not a single key with a string value but a key with child keys: blank: { message: "can't be blank", custom_message: "Whoops! it's blank!" } . This required for the lookup chain for message to support the previous, simpler nesting: blank: "can't be blank". I've commented this in the lookup chain docs in Errors in case we wish to deprecate this in the future.
  3. I enhanced the as_json and to_hash methods to support a more explicit type of argument: message_method. This allows callers to ask for the type of message they want, either message, custom_message and full_message. I've added deprecation warnings for users to upgrade to using explicit keyword arguments for as_json

Future work

  1. The Error class and specially its tests are quite long. I'm personally not used to this and thought it could be good to split the rendering of message and custom_message into different classes like MessageRenderer and CustomMessageRenderer or something similar.
  2. I wonder it's necessary to deprecate the previous lookup chain values
  3. @rafaelfranca made an interesting comment on full_message not being the right name for what it does and I agree. formatted_message as he suggested describes its behaviour and intention much better. I wonder if we should take this opportunity to rename it and create a deprecation warning for full_message

Other Information

TODO: If you are updating any of the CHANGELOG files or are asked to update the CHANGELOG files by reviewers, please add the CHANGELOG entry at the top of the file.

Finally, if your pull request affects documentation or any non-code changes, guidelines for those changes are available here

Thanks for contributing to Rails! -->

pinzonjulian avatar Nov 28 '21 10:11 pinzonjulian

Hi @pinzonjulian, If you used code from @ollieh-m, could you add them as a coauthor in a commit?

  Co-Authored-By: ....

Could you also squash the commits?

p8 avatar Nov 29 '21 15:11 p8

I actually didn't use any code from his implementation but happy to add him for coming up with the initial implementation.

pinzonjulian avatar Nov 29 '21 20:11 pinzonjulian

Squashed and implemented your suggestions @p8.

pinzonjulian avatar Nov 29 '21 23:11 pinzonjulian

Hey! Happy new year :)

Friendly bump.

pinzonjulian avatar Jan 16 '22 22:01 pinzonjulian

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

rails-bot[bot] avatar Apr 16 '22 22:04 rails-bot[bot]

Bump! @p8 @rafaelfranca could you take a look please?

pinzonjulian avatar Apr 16 '22 23:04 pinzonjulian

Thanks for your comments @ollieh-m. Really appreciate them.

Before working more on this though, I'd like for @p8 or @rafaelfranca the to give their opinions regarding the fallback chains for these. I wouldn't want to change it without some more feedback.

pinzonjulian avatar Sep 09 '22 06:09 pinzonjulian