#2210 Validation on associations can't process I18n aware error message - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#2210 ✓duplicate

Validation on associations can't process I18n aware error message

Reported by Akira Matsuda | March 11th, 2009 @ 07:15 AM | in 2.x

AR#full_messages basically generates I18n translated error_messages, but it fails to translate the model name when validating on associations (e.g. when submitted by nested_form).

For instance, while Pirate has one Ship and validation failed on a new Pirate, errors on the Pirate's attributes are properly internationalized, but the Ship's are not.

The underlying problem is that errors on associated attributes are held in the parent model by


"#{reflection.name}_#{attribute}"

key.
This makes dynamic error messaging impossible, because you can not separete model name and attribute name.

Attatched a patch to fix the whole problem.

Comments and changes to this ticket

Eloy Duran March 11th, 2009 @ 03:02 PM
- Assigned user set to “Eloy Duran”
Could you please add some test cases which actually confirm that the correct localized message is being applied?

Thanks
You flagged this item as spam.
Akira Matsuda March 16th, 2009 @ 06:36 AM
Sorry for my late response.
Just attached another patch with has_on/has_many/belongs_to tests.

Hope this patch makes it for 2.3 stable release...
You flagged this item as spam.
Maxim Chernyak May 15th, 2009 @ 12:53 AM
+1 Running into this atm - and it's a blocker.
Eloy Duran May 16th, 2009 @ 09:40 AM
@Akira I'm not quite sure why I missed your comment, sorry. We have the rubyonosx conference this weekend, so I don't have much time right now. But before the end of the week it should be verified and probably applied.
José Valim May 20th, 2009 @ 10:24 AM
Akira,

Can the tests not use mocks? I had to actually apply the patch to see what the error messages are going to look like.

And the messages were actually hiding the child name. With the patch the message is:
```
parent = Parent.new
parent.child.build # this child has an invalid name
parent.valid?
parent.errors.full_messages #=> Name can't be blank
```
While in Rails 2.3.2, it shows something like:
```
parent = Parent.new
parent.child.build # this child has an invalid name
parent.valid?
parent.errors.full_messages #=> Child name can't be blank
```
Another problem is that is still looking for the key inside the parent. Ie, the current message lookup is:
```
  activerecord.errors.model.parent.attributes.child.name
```
While IMHO it should search for it on the child object as a fallback:
```
  activerecord.errors.model.parent.attributes.child.name
  activerecord.errors.model.child.attributes.name
```
What do you think? I can help to put up a patch as well.
You flagged this item as spam.
alexandre freire June 30th, 2009 @ 12:22 AM
+1 this is really anoying
You flagged this item as spam.
Akira Matsuda July 1st, 2009 @ 01:37 PM
- Tag changed from “2.3-rc2, active_record, association, edge, errors, error_messages_for, nested, validates_associated, validation, validations” to “2-3-stable, active_record, association, edge, errors, error_messages_for, i18n, nested, validates_associated, validation, validations”
@José Thank you very much for your kind reply, but, I'm sorry, I missed your comment...
Can the tests not use mocks? Sure. Rewrote the tests.

Another problem is that is still looking for the key inside the parent. Was it? I think it has to directly lookup:

activerecord.errors.model.child.attributes.name
And it seems like working that way now, maybe because I made another tiny fix this time.
And the messages were actually hiding the child name. Well, in fact, this is totally intentional. I'm sorry that I didn't explain that point in detail.
Actually, I'd like to suggest why don't we stop hardcoding

"model_name" + " " + "column_name"
kind of too simple way in the framework?
One reason for this is that it's kind of ungrammatical and doesn't make sence in our language, Japanese.

You should be aware that not all languages separate their words by a single space character, and some languages tend to require not just a space but another WORD to connect two words in posessive relation.

Actually, it's useless for us if the framework returns a "model_name" + " " + "column_name" like string even if each of them are translated word-by-word.

As another reason, you can not add an error message on the Child model's :base attribute in the 2.3.2 way.

Please take a look at the test I added for this case (the last one in the file).
If you run this case on AR 2.3.2, it returns a message like "Child base" + " " + :message, which is not the expected value for the :base error message. It means that we can't reuse the validation logic on the Child model through autosave.

So, since it does not fit all the languages and :base attribute, my opinion is that the whole I18n thing has to be written in the I18n YAML files.

For example, if you'd like to see a message like "Child name can't be blank" in your app, just put something like
```
en:
  activerecord:
    attributes:
      child:
        name: "Child name"
```
in your YAML file.

Does this make sence for you?
Attached a new patch anyways, so could you check that again please?
You flagged this item as spam.
Akira Matsuda July 1st, 2009 @ 01:47 PM
Terrible formatting... Please ignore my previous post.

@José Thank you very much for your kind reply, but, I'm sorry, I missed your comment...

Can the tests not use mocks?

Sure. Rewrote the tests.

Another problem is that is still looking for the key inside the parent.

Was it? I think it has to directly lookup:
```
activerecord.errors.model.child.attributes.name
```
And it seems like working that way now, maybe because I made another tiny fix this time.

And the messages were actually hiding the child name.

Well, in fact, this is totally intentional. I'm sorry that I didn't explain that point in detail.
Actually, I'd like to suggest why don't we stop hardcoding
```
"model_name" + " " + "column_name"
```
kind of too simple way in the framework?
One reason for this is that it's kind of ungrammatical and doesn't make sence in our language, Japanese.

You should be aware that not all languages separate their words by a single space character, and some languages tend to require not just a space but another WORD to connect two words in posessive relation.

Actually, it's useless for us if the framework returns a "model_name" + " " + "column_name" like string even if each of them are translated word-by-word.

As another reason, you can not add an error message on the Child model's :base attribute in the 2.3.2 way.

Please take a look at the test I added for this case (the last one in the file).
If you run this case on AR 2.3.2, it returns a message like "Child base" + " " + :message, which is not the expected value for the :base error message. It means that we can't reuse the validation logic on the Child model through autosave.

So, since it does not fit all the languages and :base attribute, my opinion is that the whole I18n thing has to be written in the I18n YAML files.

For example, if you'd like to see a message like "Child name can't be blank" in your app, just put something like
```
en:
  activerecord:
    attributes:
      child:
        name: "Child name"
```
in your YAML file.

Does this make sence for you?
Attached a new patch anyways, so could you check that again please?
- associated_error_message_2.diff 9.5 KB
José Valim July 2nd, 2009 @ 08:19 AM
@akira, agreed. :)

Just one small question, why are you using: "reflection.class_name" instead of "reflaction.name"? The class name is not in YAML "default" format, it will return "ProductLine" instead of "product_line". And consider this case:
```
class Member
  has_many :locations
  has_one :current_location, :class_name => "Location", :conditions => { :current => true }
end

class Location
end
```
In this case, I have different associations to the same class, I might want to show different messages for each one of them. So I think that "reflection.name" would reflect better.

What do you think?
You flagged this item as spam.
Akira Matsuda July 8th, 2009 @ 12:36 PM
@José OK. I see what you meant by "fallback" now, and it sounds better. Thanks a lot for your opinion. :)

Attached another patch that performs "fallback".

In your example case, the I18n translator looks up 'activerecord.attributes.current_location.name' first, and then falls back to 'activerecord.attributes.location.name' next.

Please check the attached test case for an actual example.
- associated_error_message_3.diff 11 KB
José Valim July 8th, 2009 @ 01:32 PM
@Akira, this is not the fallback I meant, but it's definitely a good catch. :)

Just now I realized that you were worried with activerecord.attributes generation while I'm worried with activerecord.errors messages generation. In this case, using the example from above, it will search for messages at:
```
activerecord.errors.member.current_location.name
```
But it does not fallback to neither:
```
activerecord.errors.current_location.name
activerecord.errors.location.name
```
What do you think?
You flagged this item as spam.
Akira Matsuda July 8th, 2009 @ 02:50 PM
@José I think it should never fallback to those keys.

There's a namespace like rule in Rails I18n. AR model & attribute names lookup should be limited within
```
activerecord.models
activerecord.attributes
```
namespaces.
Otherwise, if it tries to fallback to upper namespace like:
```
activerecord.errors.current_location.name
activerecord.errors.location.name
```
I'm sure that a model named "format" or "template" will cause a weird error, since there are already existing namespaces with certain names like them.
If you want to change the whole error message body, just put the message under
```
activerecord.errors.messages
```
namespace.
Of course, nothing's gonna fallback to this key, so it would never be called automatically until you specify that key explicitly in your validation code.

That's the way it is, and, I think, that's the way it should be.
Am I answering your question?

José Valim July 8th, 2009 @ 02:59 PM

@Akira, yep, I'm aware of the behavior, just wanted to be sure if you agree with the fallback or not. :)

Do you think that you could use :default in your current patch instead of raising and searching again? For example:

  defaults = []
  defaults << :"#{arr[0]}.#{arr[1]}"
  reflection = @base.class.reflections[arr[0].to_sym]
  defaults << :"#{refletion.class_name.underscore}.#{arr[1]}" if reflection
  defaults << arr[1].humanize
  I18n.translate defaults.shift, :scope => "activerecord.attributes", :default => defaults

Thanks for your work!

You flagged this item as spam.
Akira Matsuda July 9th, 2009 @ 08:14 AM
@José Thanks for the advice again!

Rewrote the patch to use :default instead of raising.
José Valim July 9th, 2009 @ 08:38 AM
@Akira, it appears that Lighthouse had a problem with S3, I cannot access the file. Do you mind submitting it again? Thanks!

Bringing back the error message discussion, just now I realized that I suggested the wrong namespace. Right now, the messages lookup is like:
```
  activerecord.errors.models.user.attributes.current_location.name.blank
  activerecord.errors.models.user.current_location.name.blank
  activerecord.errors.messages.blank
```
(Above I forgot to add the "models"). It does not search neither in the current_location and/or location, while it should add them:
```
  activerecord.errors.models.user.attributes.current_location.name.blank
  activerecord.errors.models.user.current_location.name.blank
  activerecord.errors.models.current_location.attributes.name.blank
  activerecord.errors.models.current_location.blank
  activerecord.errors.models.location.attributes.name.blank
  activerecord.errors.models.location.blank
  activerecord.errors.messages.blank
```
The way it's today, we have to set the message in both models.user and models.location, even if they are equal. I guess that the first two lookup are not even needed.

Sounds better now? :)
You flagged this item as spam.
Akira Matsuda July 9th, 2009 @ 09:37 AM
@José

I guess that the first two lookup are not even needed.

Agreed. I'm not thinking of nesting I18n translation resource, simply because I don't want to change the YAML format in this case.

So, keys like these won't be resolved:
```
activerecord.errors.models.user.attributes.current_location.name.blank
activerecord.errors.models.user.current_location.name.blank
```
bacause the YAML should be written like this in order to I18n.t them.
```
en: 
  activerecord:
```
```
attributes:
  user:
    current_location:
      name: &quot;Name&quot;</code>
```
I'm sure this will cause other problems like
"How can we provide a translation for the association name such as :'activerecord.attributes.user.current_location'?", and I don't want to discuss about them here.

Anyways, just attached the same file again. Could you try downloading again? Thank you!
You flagged this item as spam.
Akira Matsuda July 9th, 2009 @ 09:45 AM
Hmm, still can't download the attachment file. Trying again...
You flagged this item as spam.
Akira Matsuda July 10th, 2009 @ 12:56 AM
Just renamed the file and try uploading again...
- associated_error_message_5.diff 11 KB
You flagged this item as spam.
Akira Matsuda July 10th, 2009 @ 01:22 AM
I reported the attachment file problem to LH, and it seems they are working on the issue now.

So I just put it on gist http://gist.github.com/144116

Could you please get the patch via this link and give it a try?

Thank you!
Eloy Duran July 11th, 2009 @ 12:32 PM
I think it's best you guys finish this and I go verify patches that are done, as it seems you guys are on the right track. Especially as you guys use it in non-English applications (and I haven't got much experience with the I18n internals yet).

Let me know once you both feel it's done so I can get it applied.

Thanks!
José Valim July 11th, 2009 @ 07:15 PM
@Akira, after a chat on IRC with @eloy we discussed a new solution, please check the mailing list for more information:

http://groups.google.com/group/rubyonrails-core/browse_thread/threa...

To have the same effect as your ticket, you would simply have to change this:
```
activerecord:
  errors:
    format:
      association: "{{association}} {{attribute}} {{message}}"
```
To this:
```
activerecord:
  errors:
    format:
      association: "{{attribute}} {{message}}"
```
I still want to confirm the I18n format with Sven Fuchs, I will put up a patch during this week.
José Valim August 8th, 2009 @ 12:15 PM
- State changed from “new” to “duplicate”
@Akira, @Eloy,

I've proposed a more complete solution in #2904, after discussing the problem with Josh Peek and Sven Fuchs. Marking this as duplicated.
Ryan Bigg October 9th, 2010 @ 10:01 PM
- Tag cleared.
Automatic cleanup of spam.
Jeff Kreeftmeijer November 7th, 2010 @ 04:52 PM
Automatic cleanup of spam.