Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  Yuval Kordov August 24th, 2009 @ 11:19 PM

  Looks like my formatting got wrecked, and an entire code block has vanished. I apologize. The rendered field looks like:
  
input id="user_address_attributes_city" name="user[address_attributes][city]" size="30" type="text"


• You flagged this item as spam.
  

  Pascal Ehlert August 25th, 2009 @ 11:41 AM

  Hi Yuval,

  thanks for your first report.
  This is a clear problem that I experienced myself.

  The question I was asking myself when reading this was whether it makes more sense to alter the errors attribute or rather change the code that does the highlighting of fields with errors.

  What do you guys think?

• You flagged this item as spam.
  

  Yuval Kordov August 25th, 2009 @ 03:15 PM

  Thanks Pascal. IMO, the error collection should absolutely return a correct representation of the attributes that failed. That's how it works in non-nested cases. Otherwise we're left doing brute force subs on our error handlers to look for specific objects that have been nested. It gets even worse if they are nested in some forms and not on others.

• You flagged this item as spam.
  

  Yuval Kordov November 30th, 2009 @ 05:12 PM

  I am updating this ticket because the problem has become even worse in Rails 2.3.5. Nested attributes are now coming back with a period instead of an underscore. For instance, going back to the original post where user accepts nested attributes for address, I now get address.city as the failed attribute, rather than address_city. This is not only just weird, it breaks simple unit tests such as assert user.errors.invalid?(:billing_address_city)

• You flagged this item as spam.
  

  Yuval Kordov November 30th, 2009 @ 05:13 PM

  • Tag changed from “accepts_nested_attributes_for fields_for, nested attributes, nested objects, validation” to “accepts_nested_attributes_for fields_for, nested attributes, nested objects, 2.3.5, rails, validation”
• 

  Eloy Duran December 30th, 2009 @ 07:48 PM

  • Assigned user set to “José Valim”
• 

  José Valim December 30th, 2009 @ 07:53 PM

  Which Rails version this happened please?

• You flagged this item as spam.
  

  Yuval Kordov December 30th, 2009 @ 09:05 PM

  I didn't run Rails 2.3.4 because of other bugs in that release, so I can't say if the erroneous period issue happened then or in 2.3.5. It definitely did not happen in 2.3.3.

  As it stands, in 2.3.5 you can't even do basic unit tests on nested objects. In 2.3.3, with the code in the original post I could do a unit test such as this:

  @@@ test "create with invalid nested billing address" do

  user = User.new(:email_address => 'foo@bar.com', :first_name => 'Foo', :last_name => 'Bar', :password => 'foo',
    :billing_address_attributes => {})
  assert !user.valid?
  assert user.errors.invalid?(:address_line_1)
  assert user.errors.invalid?(:address_city)
  assert user.errors.invalid?(:address_postal_code)
  assert user.errors.invalid?(:address_country_code)
  assert !user.billing_address.valid?
  assert !user.save
  

  end

  
  But this is no longer possible, as the attributes are being handled internally as address.line_1 etc.
  

• You flagged this item as spam.
  

  Yuval Kordov December 30th, 2009 @ 09:07 PM

  Sorry for the formatting issues. And "billing_address" above should be "address".

• 

  José Valim December 30th, 2009 @ 09:36 PM

  As far as I know, this is feature. You should use :"address.line_1" instead of :address_line_1. And I will try to explain why.

  On Rails < 2.3.5, when working with I18n, if you had a customized messages for Address, and then it becomes a nested association, you needed to customized them in two places:

  en:
    activerecord:
      errors:
        models:
          address:
            attributes:
              line_1:
                invalid: some special error
              city:
                invalid: another special error
          user:
            attributes:
              address_line_1:
                invalid: some special error
              address_city:
                invalid: another special error
  

  Now, if we use "." instead of "_", it becomes like this:
  

  en:
    activerecord:
      errors:
        models:
          address:
            attributes:
              line_1:
                invalid: some special error
              city:
                invalid: another special error
          user:
            attributes:
              address:
                line_1:
                  invalid: some special error
                city:
                  invalid: another special error
  

  It does not sounds like a big difference, unless we take into account the features available in the last I18n gem version, which actually allow us to do this:

  en:
    activerecord:
      errors:
        models:
          address:
            attributes:
              line_1:
                invalid: some special error
              city:
                invalid: another special error
          user:
            attributes:
              address: :"activerecord.errors.models.address.attributes"
  

  Your yml is DRYer and you likely won't make mistakes. Everything should work as expected. You should be able to install i18n >= 0.3.3 in a project and make use of such features. If you don't, then we will really have a bug.

• 

  José Valim December 30th, 2009 @ 09:46 PM

  About the f.fields_for, it was supposed to work because it will look for the error in the @user.address.first.errors, instead of @user.errors. I will try to reproduce and create a patch.

• 

  Eloy Duran December 30th, 2009 @ 09:48 PM

  Thanks for the update José, that's what I thought too. But looking at the way one has to use a symbol, do you think it would be feasible to allow a string as well?

  Ie: "address.line_1" instead of :"address.line_1"

• 

  José Valim December 30th, 2009 @ 09:49 PM

  I don't know if we can use strings. This is completely up to ActiveRecord#Errors implementation.

• You flagged this item as spam.
  

  Yuval Kordov December 30th, 2009 @ 10:06 PM

  To me, :"address.line_1" is extremely bizarre notation. And by the time your error makes it to the front end, you have to do onerous transformations just to figure out what's in error.

  For instance (and I'm going to try the formatting again, but I'll be damned if I can ever get it to work as intended):

  Here are the models.

  user.rb
  class User < ActiveRecord::Base
    has_one :address
    accepts_nested_attributes_for :address
  end
  
  address.rb
  class Address < ActiveRecord::Base
    validates_presence_of :line_1
  end
  

  So we have a user object that accepts nested attributes for an address object, and that address object has validations.

  Here's the controller that creates a new user with a nested address:

  def create
    user = User.new(params[:user])
    render :update do |page|
    if user.save
      page.redirect_to user
    else
      show_errors(page, user)
    end
  end
  

  So let's say line_1 was left blank. In my error handler, we look at the user model and output all the errors in whatever fashion we want. In my case, the meaty part looks something like this. Note the substitutions that are required just to obtain the correct fields from the nested address object.

  def show_errors(page, obj)
    obj.errors.each do |attr, msg|
    model_prefix = obj.class.name.underscore.downcase + "_"
    # at this point, the attribute is being incorrectly returned as *address.line_1* instead of matching the rendered field id of *user_address_attributes_line_1*
    # note that if first_name was required on the user object and left blank, we would correctly receive *first_name* as the attribute
    # So now we are required to substitute an underscore for the period and also to do a very specific sub to find an attribute beginning with *address* so that *_attributes* can be added onto it.
    attr = model_prefix + s.sub('.', '_').sub(/(address)/,'\1_attributes')
    # Then we flag the field and show a message
  end
  

  Now obviously, this is a custom error handler. But you get my point. What's happening on the model side is not at all being represented on the controller/form side. Hope that clarifies.

• You flagged this item as spam.
  

  Yuval Kordov December 30th, 2009 @ 10:13 PM

  Really wish I could edit ticket entries. :) s above is supposed to be attr.

• 

  José Valim December 30th, 2009 @ 10:28 PM

  Yes, I see your point. I was not the one who planned to use "user_address_attributes_line_1" instead of "user_address_line_1", so I cannot tell what motivated that difference in the first place. But regarding to "address.line_1" in error messages, I think this is an improvement compared with what we had before. I discussed several options extensively with different people and we settled on it. So I'm -1 for changing it unless you come up with a good solution. ;)

• 

  José Valim December 31st, 2009 @ 11:26 AM

  Ok, the reason for having params[:user][:address_attributes] instead of params[:user][:address] is because the second would do "user.address = params[:user][:address]", which wouldn't work. That's why we need to do: "user.address_attributes = params[:user][:address_attributes]".

  So we could change errors messages to be: user.errors[:"address.attributes.line_1"]. But this is not getting anywhere prettier. Again, we need the dots so we can DRY up I18n files.

  By the way, the dot actually helps you, because all you need to do (in Rails 2.3.5) is replace the '.' by '_attributes_'. Consider a relationship with underscore:

  class Post
    has_one :super_user
  end
  

  And the error message like:

  post.errors["super_user.special_comment"] #=> can't be blank
  

  In your helper, to find the proper field, you can simply replace the dot. However, considering Rails 2.3.3 approach, this would be impossible, because there is nothing telling where the relationship starts and where the attribute start:

  post.errors["super_user_special_comment"] #=> can't be blank
  

  You would need to track the relationships, which is just more complicated.

  This pretty much won't fix unless a good solution is proposed and we already tried several options, you can check tickets #2210 and #2904 for some background. Does it make sense?

• 

  Eloy Duran December 31st, 2009 @ 11:29 AM

  It does to me, imo the ticket can be closed.

• 

  José Valim December 31st, 2009 @ 12:12 PM

  • State changed from “new” to “wontfix”

• You flagged this item as spam.
  

  Yuval Kordov December 31st, 2009 @ 04:42 PM

  I still feel like there's a disconnect here, especially with basic unit testing. Thanks anyway for your time. Happy new year!