Nested form objects during validation have the wrong ActiveModel::Name

[mcmire]

Say you have two forms, and one is nested in relation to the other, and you have validations on both forms:

class Order < ApplicationRecord
  has_many :items, class_name: OrderItem
end

class OrderItem < ApplicationRecord
end

class OrderForm < Reform::Form
  property :recipient_name
  collection :items, form: OrderItemForm

  validates :recipient_name, presence: true
end

class OrderItemForm < Reform::Form
  validates :product_id, presence: true    # links to some fictitious Product, whatever
end

Also (for the sake of completion) say you have a generic setup like this in your controller:

class OrdersController < ApplicationController
  def create
    order_form = OrderForm.new(Order.new)

    if order_form.validate(params[:order])
      # ...
    else
      # ...
    end
  end
end

Now let's say that you've want to provide custom translations for these validations. You can do this easily for the parent form:

en:
  activemodel:
    errors:
      models:
        order:
          attributes:
            recipient_name:
              blank: "Please provide a name for the recipient."

But if you want to provide translations for the child form...

en:
  activemodel:
    errors:
      models:
        order_item:
          attributes:
            product_id:
              blank: "Please choose a product."

...this won't work. It seems that if you simulate the code in Rails that translates validation messages, you get something interesting:

> order.items[0].class.lookup_ancestors[0].model_name
=> #<ActiveModel::Name:0x007fe1c671d888
 @collection="items",
 @element="item",
 @human="Item",
 @i18n_key=:item,
 @klass=OrderItemForm,
 @name="Item",
 @param_key="item",
 @plural="items",
 @route_key="items",
 @singular="item",
 @singular_route_key="item">

What this means is, I actually need to say this in my locale file:

en:
  activemodel:
    errors:
      models:
        item:
          attributes:
            product_id:
              blank: "Please choose a product."

Now I can fix this by adding model :order_item to OrderItemForm, but 1) I just found out about that and 2) I don't think I should have to do that.

I've been hunting around in Reform and I stumbled upon these lines: https://github.com/trailblazer/reform-rails/blob/master/lib/reform/form/active_model.rb#L34

Could this have anything to do with it? If not, do you know what's happening here?

[fran-worley]

@mcmire this isn't isolated to nested forms, as per the docs this is exactly how reform is designed to work.

If you don't think you should have to do that, what do you propose instead? We don't get involved in trying to infer classes from names like rails does as it gets messy and brittle pretty quickly.

[mcmire]

@fran-worley I believe the issue I am describing is isolated to nested forms: it seems like there is custom behavior around nested forms such that they get a specific name that is different from the model name that a non-nested form would usually get.

What I'm saying is that if you have a nested form such as a collection and it is called items, it makes sense to me that names for fields generated from this form would contain items, e.g. order[items][0][product_id]. However, it does not make sense to me that the I18n key would be items. It would be unintuitive, and could potentially trip up other developers, to look in the locale file and see a plain item key sitting within models if that is not the actual name of the "model". It also means that if you wanted to use the nested form class on its own -- again, in my example, OrderItemForm -- you would have to have duplicate keys in your locale file under two keys, item and order_item.

But say the default model name were order_items. Then field names would contain order_items and not items, and the i18n key would be order_item and not item. I would not expect that either of those changes would trip anyone up, and it means you could keep one set of keys in your locale file to represent that form class.

So, what I'm proposing is to change the default model name for a nested form so that the name of the form class is used to generate that model name instead of the name of the collection/property. What do you think?

[apotonick]

@mcmire In the nested form, you need to use model to configure it.

collection :items do
  model LineItem

[mcmire]

@apotonick Right. I am using a separate form class, though -- I am saying collection :items, form: OrderItemForm, and then I have model :order_item in my OrderItemForm class. So when you have it that way, it's a little bit repetitive, I would think, to see

class OrderItemForm < Reform::Form
  model :order_item
end

First, the form class is called OrderItemForm. Second (because inferring names from class names is nasty, I do agree), I am passing an OrderItem instance to the OrderItemForm instance. So it seems like between these two options, the form should already know what the model class is, without my having to specify it.

[apotonick]

Why is the naming so important in the first place? For nested forms, it doesn't call URL helpers or anything, and Reform's deserializer will map it back properly.

[fran-worley]

@apotonick, it's important in rails because it influences the input names which will mess up your params hash.

…

Sent from my iPhone On 24 Feb 2017, at 09:12, Nick Sutterer <[email protected]<mailto:[email protected]>> wrote: Why is the naming so important in the first place? For nested forms, it doesn't call URL helpers or anything, and Reform's deserializer will map it back properly. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#36 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ACNaJTsbYKFvVjRLfbEH62-hp_qryL-Eks5rfp8MgaJpZM4Kb3_4>.

[apotonick]

The solution is to release Formular and leave this crap behind us, ahead into a brighter future.

[mcmire]

Formular looks like a good idea, but does it have anything to do with model validation error messages? Because that's what this issue is really about.

Let me see if I can explain this again :) Reform::Form is essentially an ActiveModel::Model (or brings in ActiveModel functionality at least). When ActiveModel produces validation error messages, it will pull them from select keys in i18n. The namespace of these i18n keys is based off the ActiveModel::Name of the class. Usually the Name corresponds to the name of the class. In the case of nested forms, reform-rails actually changes it to match the name of the property or collection that was used to add that form to the parent form. Hence, that namespace could potentially be unintuitive, as I described above in my examples. (Yes, the Name is also used to generate input names, but that's not what I'm talking about here.)

Does this make sense?

[RKushnir]

I just stumbled into this as well. And I'm pretty sure the problem is in reform-rails, specifically here. The ultimate model name depends on the load order of the form files, where the last one wins.

Why the name is important? In my case it's used in the error messages, leading to different behavior when I run a single test file for the nested form vs. the whole test suite(when the parent form messes the name).

[pdfrod]

I just got bitten by this as well. In my case, the model name is used to generate the form ID that is picked up by JavaScript code. Everything worked fine in my computer, but in production the JavaScript didn't run at all because the model name was different, thus the ID was different. It took me a while to figure out why this happened.

Granted, there other ways to solve my issue, but I steel feel that the piece of code that @RKushnir mentioned looks dangerous and could use some rethinking.