Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide better error message on :required association #18696

Closed
noinkling opened this issue Jan 27, 2015 · 5 comments · Fixed by #18700
Closed

Provide better error message on :required association #18696

noinkling opened this issue Jan 27, 2015 · 5 comments · Fixed by #18700

Comments

@noinkling
Copy link
Contributor

The :required option introduced for belongs_to/has_one associations (#16056) is very nice, however it still uses the default error message from the presence validator - "... can't be blank" - which doesn't make much sense for associations. It would be much nicer to have a default message like "... doesn't exist" or something along those lines.

Changing the locale string works, but of course it still doesn't let you differentiate between presence validations on associations and presence validations on normal model attributes. Additionally, there's no support for setting the message manually on a case-by-case basis like with the normal validation syntax.

I understand that if someone wants the other options they can just go back to the old validation syntax, but I think the separation provides a good opportunity to use a different default message.

required: true calls validates_presence_of behind the scenes, so I think you could just add the :message option to that call, but I don't know how to integrate that with the I18n stuff, hence no pull request (and there's probably a better way anyway). I guess a simpler way to get some customization would be to allow :required to take a hash of the same options as the validator and simply pass those through, but I don't know if this would be desirable or not.

/cc @sgrif

@sgrif
Copy link
Contributor

sgrif commented Jan 27, 2015

I think #18700 is a good solution. I don't think we need to have required take a hash of options, at that point you should just define the validation separately.

@carlosantoniodasilva
Copy link
Member

I just want to add that you currently can use different messages on a case by case basis, by only configuring the error message based on the model/attribute (or association in this case) name.

@sgrif
Copy link
Contributor

sgrif commented Jan 27, 2015

After thinking about it a bit, I'm not sure we should be taking any action here. In the original commit message (00f5551) it's stated:

This helps to draw a distinction between types of validations, since validations on associations are generally for data integrity purposes, and aren't usually set through form inputs.

If the validation is there to provide feedback for user input, I think it makes the most sense to leave it with the rest of the validations, so future readers will have one clear source of all of the places that create user-facing errors.

@noinkling
Copy link
Contributor Author

@carlosantoniodasilva: is there an example floating around showing how you would achieve this?

@sgrif: I disagree that they aren't likely to be set through form inputs, that seems like an unfair generalization, but the message is probably unlikely to come up if your application uses any sort of sane interface (e.g. a select dropdown rather than a text/number field).

Nevertheless, I just think the current message is a poor description of what went wrong if it does come up (whether in fringe use cases or during development), and at the end of the day I think it's simply nicer and more informative to look at something like this in my models:

belongs_to :whatever, required: true
belongs_to :something, required: true

validates :whatever_id, :something_id,
  presence: true

Rather than this:

belongs_to :whatever
belongs_to :something

validates :whatever_id, :something_id,
  presence: true
validates :whatever, :something,
  presence: { message: "must exist" }

Given how the simple the implementation in #18700 turned out to be, I can't help but feel like the message is low-hanging fruit.

@carlosantoniodasilva
Copy link
Member

Something like this:

en:
  activerecord:
    errors:
      models:
        user:
          attributes:
            nickname:
              blank: "must exist"
            password:
              too_short: "try again with more than %{count} chars"

The attributes nickname and password are examples, they could be association attributes for instance. The default lookup chain can be seen in the errors docs.

nygrenh added a commit to nygrenh/rails that referenced this issue Jan 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants