Rails Ruby on Rails → 2.3.4

Comments and changes to this ticket

• 

  Eloy Duran July 13th, 2009 @ 10:05 AM

  • Milestone changed from “2.x” to “2.3.4”

  I'll check this patch out during my flight probably, the next few days are a bit too hectic :)

  One quick note though; I see you added a method to return the associated objects which are invalid, which is great. But I wonder if we could make it easier for people, who don't use error_messages_for but (for instance) #to_json (API access), if that method would return a collection of the aggregated errors instead of the full objects. Thoughts?

• 

  José Valim July 13th, 2009 @ 10:35 AM

  @eloy, to solve this problem I would suggest to move the association logic down to ActiveRecord::Errors, so we avoid having the same logic on error_messages_for and in serializers (to_xml, to_json). I'm just not sure about the API, since we can follow several paths.

  1) Extend full_messages to return a nested structure and not just an array of strings.

    project.errors.full_messages(:associations => :tasks)
    # Returns:
    # [
    #   "Title can't be blank",
    #   { "Tasks" => [
    #     "Name can't be blank"
    #   ] }
    # ]
  

  I don't like this solution because it would change the semantic behavior of full_messages.

  2) Add a to_html method to errors that will be used by error_messages_for, behave exactly like to_xml and to_json, and will return a nested structure with

  • and
  • . I don't like it since it will be hard to customize the messages in this case.

    3) Add a method called to_ruby. It returns the structure above and can be used by any serializer and error_messages_for, keeping the code simple. But I don't know how people feel about adding a new method to the API which 90% of the cases returns the same as thing as full_messages.

• 

  José Valim July 13th, 2009 @ 10:46 AM

  (Formatting issues, continuing...)

  2) Add a to_html method to errors that will be used by error_messages_for, behave exactly like to_xml and to_json, and will return a nested structure with ul and li. I don't like it since it will be hard to customize the messages in this case.

  3) Add a method called to_ruby. It returns the structure above and can be used by any serializer and error_messages_for, keeping the code simple. But I don't know how people feel about adding a new method to the API which 90% of the cases returns the same as thing as full_messages.

• 

  Eloy Duran July 17th, 2009 @ 08:08 AM

  About the way to get the messages; I'd like the functionality of the last option, however, I'm on the fence on if it should be a separate method or not and what the name should be. I'll have another thought about it and let you know, unless you figure something out in the meantime.

  Also I have a few comments about the patch:
  * The simplest one is simply that there's a test name which is incomplete: “test_should_merge_errors_on_the_associated_models_onto_the_parent_even_if” * The second one is on how to display error messages for children from the 2nd level (grandchildren). It seems to me that there's currently no way to specify these. I could be wrong of course :). I haven't got a clear idea yet, but some ideas are:

  error_messages_for :parent, :associations => { :child => [:grand_child1, :grand_child2] }
  

  But this might become very ugly… Another one is something like:

  error_messages_for :parent, :associations => :child do |association_name, association|
    # obviously this is too long, but I'm sure it could be shortened in some way
    error_messages_for association_name, :object => association, :associations => [:grand_child1, :grand_child2]
  end
  

• 

  José Valim July 17th, 2009 @ 08:40 AM

  If we want to include grandchild, we can use :include to reflect find and to_xml API:

  error_messages_for :parent, :include => { :child => [ :grand_child1, :grand_child2 ] }
  

  But we still need a solution for the first problem. I'll talk with Koz and Pratik and see if they can jump in the discussion.

• 

  José Valim July 18th, 2009 @ 12:20 AM

  Just talked with Josh. A few suggestion he made:

  1) One way to deal with the problem is to link the child hash in the parent:

    parent.errors[:child] = child.errors
  

  This would make a couple things look better in the current code. In this case, full_messages would just collect parent and child messages and flatten everything.

  2) As Josh current pointed, current errors.to_xml cannot support a nested errors structure:

  <errors>
    <error>Content Empty</error>
    <error>Title is Wrong Create</error>
  </errors>
  

  So the flattened full_messages above would be enough. A suggestion was made to change how seralizer works (but that should be a Rails 3.0 thing):

  <errors>
    <project>
      <title>can't be blank</title>
      <tasks>
        <name>is invalid</name>
      </tasks>
    </project>
  </errors>
  

  3) Even if the two points discussed above, we still need a method that will returned a nested structure to be used in error_messages_for. My suggestion still sticks with to_ruby or to_a:

    project.errors.to_a(:include => :tasks)
    # Returns:
    # [
    #   "Title can't be blank",
    #   { "Tasks" => [
    #     "Name can't be blank"
    #   ] }
    # ]
  

• 

  José Valim July 19th, 2009 @ 01:05 PM

  Ok, just created a new patch taking some of Josh and Eloy advices.

  1) Errors are nested, that means that nested attributes is just doing:

    parent.errors.add(child_name, child[:errors])
  

  2) full_messages behaves the same way as previously. Associated object errors are flattened.

  3) A method called to_a was added (that is already added in ActiveRecord::Errors in Rails 3.0 branch). At first it behaves just as full_messages, but it allows you to give associations to return a nested structure:

    class Project < ActiveRecord::Base
      validates_presence_of :title
      accepts_nested_attributes_for :tasks
    end
  
    class Task < ActiveRecord::Base
      validates_presence_of :name
    end
  
    # Start a new project
    project = Project.new
  
    # Build two tasks
    project.tasks.build
    project.tasks.build
  
    # Project is invalid since both itself and tasks are invalid
    project.valid?
  
    project.errors.full_messages
    # => ["Title can't be blank", "Name can't be blank"]
  
    project.errors.to_a
    # => ["Title can't be blank", "Name can't be blank"]
  
    project.errors.to_a(:associations => :tasks)
    # => [
    #    "Title can't be blank",
    #    :tasks => [
    #      "Name can't be blank"
    #    ]
    # ]
  

  Since the association errors are always included, I've decided to use :associations as key instead of :include.

  4) As Eloy suggested, to_xml now show associated object errors but the messages are flattened, as Josh pointed out.

  The patch added (0002) only includes ActiveRecord changes. I will provide another patch with changes to make error_messages_for work as expected (patch 0001 is deprecated).

  • 0002_add_nested_errors_support.diff 12.7 KB
• 

  José Valim July 19th, 2009 @ 04:17 PM

  Here is the patch for error_messages_for.

  • 0003_update_error_messages_for_to_use_to_a.diff 8.3 KB
• 

  Eloy Duran July 21st, 2009 @ 07:54 AM

  Looks great! Once I'm back I will verify and apply. Thanks!

• You flagged this item as spam.
  

  jonnii August 7th, 2009 @ 06:51 PM

  I raised a duplicate of this issue here:

  https://rails.lighthouseapp.com/projects/8994/tickets/3002-error_me...

  The only thing I have to add is that I think it would be nice if you could do

  <%= f.error_messages :associations => :flatten %>

• 

  José Valim August 8th, 2009 @ 12:20 PM

  Jonnii,

  Agreed. I will make a patch so it flatten the associations by default (so it will be backcompat), unless :associations is specified. However I will make it after those ones are applied.

• You flagged this item as spam.
  

  Josh Sharpe September 10th, 2009 @ 07:11 AM

  I don't mean to pile on... but this is turning into a error_messages_for refactor...

  Would anyone be opposed to allowing :style to be passed to error_messages_for for the wrapping div?

  I could immediately use this to dynamically set the width of the div which makes for very nice looking error blocks. I know that's trivial/edge case, but it's pretty sweet.

• You flagged this item as spam.
  

  Josh Sharpe September 10th, 2009 @ 07:18 AM

  Also, what is the status of the above 3 patches? Is anyone currently working on them?
  

  I can consolidate them and attempt Jose's last addition.

• You flagged this item as spam.
  

  Josh Sharpe September 10th, 2009 @ 07:21 AM

  Also, after looking at these patches, it doesn't look like :excepts works the same way that :associations does.

  Say I want to show errors for an association

  :associations => {:child => [:grand_child]}

  Well, I should be able to use :excepts in a similar nested format, to remove an error from the child or grand child object.

  I think we should make it just as easy to pick and choose which errors we want to remove. I've been looking for this functionality for a while now...

• You flagged this item as spam.
  

  Josh Sharpe September 10th, 2009 @ 07:33 AM

  Hmm, I can't get 0001 to apply to 2.3. In the original post Jose mentioned it was to be applied on top of Eloy's branch, did those parts make it into 2.3?

  Also, I'm done commenting for now :)

• 

  José Valim September 10th, 2009 @ 07:56 AM

  Hey @Josh. Yes, it needs to be applied against Eloy fork. Anyway, it will need to be rebased.

  Another thing is: we are still not sure this patch is going to be applied. There is definitely a interest in fixing nested attributes I18n messages, but as you can see in the patch, it requires a lot of work. We are searching for a simpler solution.

• You flagged this item as spam.
  

  Josh Sharpe September 10th, 2009 @ 08:20 AM

  IMO I think you guys are on the right track.

  Maybe picking and choosing from the above patch is in order...

  a) Errors should be on the object that they belong to
  -- That is, do away with adding the child errors in the form of "#{reflection.name} #{attribute}" onto the parent.

  This keeps all the errors exactly where they belong, and no extra/duplicated errors are thrown in... It also allows for:

  b) All the new fanciness added to error_messages_for , :associations

  "The displaying of error messages on associations was then moved to error_messages_for, since it's basically a view concern"

  ^^ I think that's a very key point here -- it's the view that needs to dig out exactly which errors are displayed.

  c) I don't think that :except is necessary. Instead, error_messages_for should show every error except "foo_association is invalid", which are meaningless in the context of a view. -- I'm not sure how to pick out just these messages though.

• 

  Eloy Duran September 10th, 2009 @ 03:28 PM

  • State changed from “new” to “hold”

  After a talk with José, we are now certain that this patch will be rewritten with a slightly different approach. Although the gist of it is still that AutosaveAssociation won't touch your error messages, as it should be. So my apologies that I said on the mailing list that I would merge it this week, give it a bit more time. Thanks!

• You flagged this item as spam.
  

  Josh Sharpe September 10th, 2009 @ 08:50 PM

  Are you still planning to release a 2.3 patch for this? If you guys are busy on other issues at the moment I'd be more than willing to take your ideas and get something started.

  I have the will and the need :)

• 

  José Valim September 10th, 2009 @ 10:47 PM

  @Josh, what is your use case? Just for me to take into account in the next implementation. :)

• You flagged this item as spam.
  

  Josh Sharpe September 11th, 2009 @ 02:54 AM

  I want whatever solution will allow me to show my users the errors that pertain to them. So if:

  Profile validates_associated :address and
  I have a profile/address with the profile.address.city.blank? == true

  I do not want error_messages_for to show me:

  "Address is invalid" or "Address City cannot be blank"

  I want it to output:

  "City cannot be blank"

  and that's it.

  It's okay that "Address is invalid" is on my profile object -- but I want error_messages_for to ignore it.
  It's not okay if "Address City cannot be blank" is on the profile object, since that is just redundant.

  I really don't like that children errors get added to the parent in various situations (but it looks like that's being done away with)

  As a side note... It'd be really nice if error_messages_for took :style to add styles to the wrapping div, but that could be another patch/conversation if you want.

  If I can be any help please let me know, my email is at github.com/crankharder

  Thanks for working this out with me!! :)

• 

  José Valim September 11th, 2009 @ 08:51 AM

  @Josh,

  This was hard to accomplish in Rails 2.3.3, because error messages were not self contained. I think we can easily achieve that now in Rails 2.3.4. What I'm planning is:

  1) Stop treating nested attributes as special case. Give the user the option whether he wants simply to validate the parent or copy the error from child to parent. Suggestion for API (needs to be refined):

    has_many :tasks, :validate => :associated # same as true, probably used just internally
    has_many :tasks, :validate => :copy_errors
    has_many :tasks, :validate => false
  

  Nested attributes just needs to enforce that :validate is not false.

  2) When copying errors, maintain errors state and use appropriate namespace. In Rails 2.3.3, when we copy errors, we copy to a weird attribute on the parent called :child_attribute. If the messages had to be customized through I18n files, it had to be done in both: errors.messages.child.attribute and errors.messages.parent.child_attribute.

  In Rails 2.3.4, it's broken. My solution is simply copy the messages from child to parent, but keep the lookup the same, i.e. errors.messages.child.attribute.

  I guess this will work exactly as you want.

• You flagged this item as spam.
  

  Josh Sharpe September 11th, 2009 @ 03:57 PM

  I don't like putting :validate on has_many. If we do that, then what's the relevance of validates_associated? Also, what happens with nested attributes? Also :associated/:copy_errors feels hackish.

  Edits to above:

  1) Stop treated nested attributes as a special case. That's it, keep it dead simple. This keeps everything extremely consistent.

  2) Don't bother copying errors. Ever. Keep them on the object that they belong to. This way makes a ton more sense to me.

  Are we copying errors as some kind of backwards compatibility concern? Like you said way above, showing the right errors is primarily a view concern and therefore should be left to the view helpers. If you copy errors to the parent you're just going to confuse the view helpers down the road.

• 

  José Valim September 11th, 2009 @ 04:05 PM

  I prefer has_many :validate in comparison with validates_associated, because I let all associations declaration (and their validation in the same place) but I see that you can give me the exactly opposite argument. ;)

  2) Don't bother copying errors. Ever. Keep them on the object that they belong to. This way makes a ton more sense to me.

  What if I have a project and creating 10 comments with nested attributes? I would need to retrieve them and send them all to error_messages_for? For me it's ok, but I need to raise this issue into the discussion. In this while, let's wait for more feedback. ;)

• You flagged this item as spam.
  

  Josh Sharpe September 11th, 2009 @ 04:11 PM

  Let error_messages_for compile the errors into the nested structure that you described above. (With a :flatten option) I think that'd work great.

• 

  José Valim September 11th, 2009 @ 04:18 PM

  @Josh,

  It was my first attempt. Check the first patch attached. We should do something like this:

    error_messages_for :project => :comments
    # Or:
    error_messages_for :project, :associations => [:comments]
  

  I prefer the first because it supports multiple objects. One concern raised was: and what about to_xml and to_json? We would have do duplicate the code for deal nested scenarios in both error_messages_for and serializers.

• You flagged this item as spam.
  

  Josh Sharpe September 11th, 2009 @ 04:27 PM

  Ideally error_messages_for would support an unlimited about nesting, the same way find(:include/:join) does.

  error_messages_for :project, :associations => [{:activities => :comments}, :other_assoc]

  I actually can't imagine a form that complex, but I don't think it'd be that hard to make the processive recurssive so that option is there. Maybe I'm reaching here and we should only allow, say, 2 levels of associations. Please discuss.

  to_xml and to_json should be simple to solve.

  The first thing error_messages_for does is build the nested hash of errors as described above. Then just let Hash#to_json/xml do the heavy lifting. Maybe error_messages_for can accept a :format option.

• 

  José Valim September 11th, 2009 @ 04:36 PM

  Yes, error_messages_for should handle that recursively.

  A :format option does not make sense for error_messages_for, to_xml and to_json are usually called inside models and controllers, where error_messages_for is not available. For now, I'm will just avoid copying errors from child to parent.

  If you wish, you can make a patch for error_messages_for to accept a :include (or :associations) option and another for error serializers to accept :include as well.

• 

  José Valim September 11th, 2009 @ 04:56 PM

  @Josh, another issue from this approach you propose is that is not easy to retrieve records that are not valid in an collection association. That is why I had to add a invalid_records_by_association_name(name) in the first patch.

• 

  José Valim September 22nd, 2009 @ 01:52 PM

  • State changed from “hold” to “invalid”

  I'm marking this ticket as invalid since we could not agree in a solution. Luckily, the initial problem is partially solved on Rails 2.3.4, since Error messages can be self-contained, solving the duplication of message on I18n file.

  If someone wants to work on this, fell free to re-raise the discussion in the mailing list and/or rebase the patches.