Model name in ActiveModel validations for custom error message translations is always "reform/form"

[tilod]

Unfortunatly I found another bug. When using the standard error translations everything works (because they are not in a model namespace), but when using custom translations, I18n uses a wrong model name for the lookup, namely "reform/form".

I put another PR together with tests for this. In short:

en:
  # custom validation error messages
  activemodel:
    errors:
      models:
        song:
          attributes:
            title:
              custom_error: Custom Error Message

class SongForm < Reform::Form
  model :song
  property :title

  validate do
    errors.add :title, :custom_error_message
  end
end

Triggering the validation will add "translation missing: en.activemodel.errors.models.reform/form.attributes.title.custom_error_message" to the errors hash. Notice the model name it uses for the translastion lookup: Although I used model :song to set it to "song", it's "reform/form".

The weird thing is, form.class.model_name.human (which should be called according to the Rails internationalization guide) actually returns "Song". I also tried overriding ::model_name in the form which lead to the same result: calling form.class.model_name returned the correct result, but it's not used in the translation.

[apotonick]

Ah yeah I remember that from years ago.

Another reason I will push things towards a clean implementation using Lotus::Validations. ActiveModel is all but a standard and makes so many assumptions about your "model" that it's impossible to keep track if you're "compliant" to the "standard", even though there's wannabe lint tests available (outdated, of course).

Man, thanks so much for the test, they really really help!

[tilod]

You're welcome.

I played a little bit and found out, the reason for the bug is the implementation of ::model_name in Reform::Form::ActiveModel::Validations::Validator. It is hardcoded to return Reform::Form. It should actually return what the model in the form returns (and maybe Reform::Form as fallback). Given it's a class method which should delegate to another class method, which itself may later (after the included hook is run) be overridden (by calling ::model in the form)… oh well, you will have fun with this one, I guess.

[spectator]

I run into similar issue but with attribute translations.

en:
  activerecord:
    attributes:
      albums/album:
        song_ids: "Songs"

this used to work, but with Reform 2.0.1 I get "Song ids" which is fallback behavior for human_attribute_name

[apotonick]

Hi @tilod, I experimented with your test and simply hardcoded "song" where it should be the dynamic model_name: https://github.com/apotonick/reform/compare/i18n#diff-7039e69b74a923442bc1ec818227ae27R60

Now, the test fails with this: Expected ["can't be blank", "translation missing: en.activemodel.errors.models.song.attributes.title.custom_error_message"] to include "Custom Error Message". As you can see, the path is now correct but it still doesn't find the translation. Can you make this test pass? In the meantime I will try to fix ::model_name.

[tilod]

I tried just now, but found out that I18n is not properly configured in your dummy app.

The YAML files with the translations are not loaded at all. My first tests worked because the standard translations (which I copied over from Sven Fuchs' repository) are actually included in rails. I didn't knew that. I removed the copied section completely and my first tests still work. But any modifications or additions to the YAML file are ignored. Also changing the locale settings (like setting config.i18n.default_locale = :de in application.rb) does not have any effect, I18n.locale still returns :en. I can look at this, but this will propably take some more time.

[apotonick]

That'll be great, @tilod ! :heart: It would greatly help me to implement this form-class-naming feature you need.

[tilod]

Well… when putting raise :hell into application.rb and the tests still run fine, it may indicate that the dummy app is not loaded in the tests at all. Is that intentional?

I now let I18n load the translation file in the test_helper. When putting ActiveModel::Name.new(Song) into validations.rb, tests are green. I will send a PR in a minute.

[tilod]

Aha, github automatically updates the PR when I push my changes to the forked repository -- I didn't knew that either.

[apotonick]

Yeah because you work and push everything to master, you should work on a separate fix branch for that (which will still update the PR when you push to that branch, though). Let me merge. :heart:

[tilod]

Ah, okay. Thanks for the hint. These are actually my first PRs ever, I'm glad I made it this far. :wink:

[apotonick]

Great job, bro! :beers:

[apotonick]

Merged, please test master!

[tilod]

I tested with our production app. Model name is back in the translation lookup string. :+1: The top namespace for the message lookup changed from "activerecord" to "activemodel", but that was expected and I'm fine with that.

[apotonick]

I saw that a few weeks ago (top namespace being "activemodel"). Where is that relevant? Should we change it back?

Thanks so much for your help, @tilod your tests were inevitable to fix this!

[tilod]

The top namespace is relevant for the lookup of model and attribute names and error messages, as described here: http://guides.rubyonrails.org/i18n.html#translations-for-active-record-models. The default form builder in Rails (and some other form builders) make use of this for naming the labels automatically and inserting the model and attribute name in the error messages. This is also done in ActiveModel::Errors#full_messages. I don't think the change will break anything aside from forcing all Reform users who also use I18n to change the namespace in the YAML files -- which is not too much of a hassle.

I read your comment in "validations.rb" that you want to abandon support for ActiveModel validations in Reform 2.1. So, I guess, the error message lookup will change in the future anyway. As long as model and attribute name are part of the lookup string, I don't mind the structure of the YAML file. But you should ask some more people about this of course. :smile:

Oh and given the exquisite name for the class instance variable in the Validation class, I assume I was right, when I said you would have fun fixing this bug. :beers:

[apotonick]

Hahaha, I knew this problem from before, e.g. AM relying on class methods of the "model" and so on, so I was prepared, and as you can see, I kinda hacked it, since I'm not interested in supporting AM long-term.

Plan is to leverage Lotus' simple validations and build cascaded validations and predicate validations on top of that. This will significantly change the way people see validations as they become way more rules-like and high-level in Trailblazer environments and Reform.

As this is impossible to achieve with the mess found in AM I have to drop support for it. However, this will pretty surely end up with happy users that do not even remember how they did validations with AM in a half year or so, just like people would never go back to accepts_nested_attributes_for once they use Reform. :city_sunset: :sunglasses: :beers:

[tilod]

Sounds like a plan. :smile: I actually discovered Reform as I became desperate with accepts_nested_attributes_for two years ago. And I spread the word since then.