Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  Daniel Schierbeck October 11th, 2008 @ 08:15 PM

  In the following example:

  # Creating associated models. Note that no records will be instantiated for any empty hashes: params[:member] # => { 'name' => 'joe', 'posts_attributes' => {

  'new_12345' => { 'title' => 'first psot' },
  'new_54321' => { 'title' => 'other post' },
  'new_67890' => { 'title' => '' } # This one is empty and will not be instantiated.
  
  

  }} , what significance does new_12345 and the other keys have?

  Would it make more sense to have the posts_attributes be an array of hashes instead?

• You flagged this item as spam.
  

  Daniel Schierbeck October 11th, 2008 @ 08:16 PM

  The last example should of course have been:

  # Creating associated models. Note that no records will be instantiated for any empty hashes:
  params[:member] # => { 'name' => 'joe', 'posts_attributes' => {
    'new_12345' => { 'title' => 'first psot' },
    'new_54321' => { 'title' => 'other post' },
    'new_67890' => { 'title' => '' } # This one is empty and will not be instantiated.
  }}
  
  

• 

  Eloy Duran October 12th, 2008 @ 12:43 PM

  An array of hashes for new records is also supported.

  The “new_123” keys however, are so that you can mix existing records with new ones. These keys are generated by the modified FormBuilder#fields_for for new records, or you can create them yourself if your using an API for instance.

• 

  Eloy Duran October 12th, 2008 @ 12:51 PM

  I should have added that a hash is what gets created by the parameter parser if you have records with an id and an array is what gets created for a collection of records without an id. So for this to work, in a scenario where you have mixed new and existing records, it should support a hash and not an array and thus should have at least some sort of index key for each record.

• 

  Eloy Duran October 15th, 2008 @ 11:14 AM

  Added a new patch with: - better NestedParams docs by Manfred Stienstra. - #acts_like_hash? for Hash and ActiveSupport::OrderedHash - updated to work with current edge Rails (09c1718).

• You flagged this item as spam.
  

  Pascal Ehlert October 16th, 2008 @ 04:25 PM

  This is the best and most complete approach to that issue that I've seen to far.

  +1 from my side, but I don't think it should make it into 2.2 as you can never be sure it doesn't break something.

  That said, do you think you could make this a heavily monkey-patching plugin or gem for the time being? I would love to use it for my next project, together with considerably stable 2.2.

• 

  Eloy Duran October 16th, 2008 @ 05:24 PM

  @Pascal: If I find some time I'll see about back porting into the plugin.

  I myself think it would need more testing, so 2.2 is probably too close already.

• You flagged this item as spam.
  

  linoj October 21st, 2008 @ 05:40 AM

  looks like a winner to me +1

• You flagged this item as spam.
  

  James November 22nd, 2008 @ 04:49 PM

  I like this patch a lot, but have a few comments.

  The whole autosave thing seems flawed, as it's currently implemented. Why save all associations all the time? We have changed? and friends to help us decide when to save. Saving associations all the time will emit a lot of queries. But, it's possible that I missed something, since I didn't read every line.

  I hate the name. params is an action_controller term. They should have some other name in active_record, since they're not tied to external params. Perhaps :attributes => true ?

  Lastly, I'm sorry to say that I don't feel that the code quality is really there. I find the methods very long, winding and difficult to read. My big problem with this patch is that I've been staring at it for quite a while, and I still don't feel like I really understand how it works.

  I really think that it would be nice to refactor this code in to a class that handles all the logic around saving and validating collections of active_record objects. I have been working on something similar for my active_presenter plugin, but haven't got anything quite finished yet. It would be a lot more flexible that way, because you could use it for more than just associations (i.e. collections of top level objects in one form). It would also provide better encapsulation and testability.

• 

  Eloy Duran November 22nd, 2008 @ 06:03 PM

  “The whole autosave thing seems flawed, as it's currently implemented. Why save all associations all the time? We have changed? and friends to help us decide when to save. Saving associations all the time will emit a lot of queries. But, it's possible that I missed something, since I didn't read every line.”

  ActiveRecord::Base#save shouldn't actually perform any queries if the record is not changed? So there's no need to duplicate that code. I'll double check though if it really doesn't, when I update the patch one of these days.

  “I hate the name. params is an action_controller term. They should have some other name in active_record, since they're not tied to external params. Perhaps :attributes => true ?”

  I agree. However just :attributes doesn't tell me enough. How about :nested_attributes ?

  “Lastly, I'm sorry to say that I don't feel that the code quality is really there. I find the methods very long, winding and difficult to read. My big problem with this patch is that I've been staring at it for quite a while, and I still don't feel like I really understand how it works.”

  The long methods were meant as being descriptive. Please tell me which methods and why you find they are hard to grok.

  “I really think that it would be nice to refactor this code in to a class that handles all the logic around saving and validating collections of active_record objects. I have been working on something similar for my active_presenter plugin, but haven't got anything quite finished yet. It would be a lot more flexible that way, because you could use it for more than just associations (i.e. collections of top level objects in one form). It would also provide better encapsulation and testability.”

  I haven't had a need for that in the application that we used it in.

  Thanks, Eloy

• You flagged this item as spam.
  

  James November 22nd, 2008 @ 06:20 PM

  They're not nested in the context of the declaration though. You don't say has_many :nested_class_name =>... You say :class_name. The fact that it's has_many implies that it's nested.

  One example of code that I find difficult to grok is define_nested_params_writer_for_collection_association. It goes up to about 12 levels of indentation with many nested conditionals and other statements. The method is about 30 lines long, which makes it easy to lose context as you read through it. Nested conditionals like that are ugly, and there's really no reason to use them in ruby. There are other examples in this set of patches.

  I don't understand your point about not needing that in your application. You don't need well factored, properly encapsulated code? You may not need the feature of editing many top-level models at once, but adding all this code to one place blurs responsibilities, and means that extracting it to do anything else later on is going to be impossible.

  I'm currently working on a refactoring of action_pack, and let me tell you, the lack of encapsulation is making it VERY hard. ActionController::Base has about a million responsibilities that are all tangled up in eachother. That makes it really bloody hard to refactor. Really, really bloody hard.

• 

  Eloy Duran November 23rd, 2008 @ 12:26 PM

  “They're not nested in the context of the declaration though. You don't say has_many :nested_class_name =>... You say :class_name. The fact that it's has_many implies that it's nested.”

  :attributes is still too abstract for me.

  “I don't understand your point about not needing that in your application. You don't need well factored, properly encapsulated code? You may not need the feature of editing many top-level models at once, but adding all this code to one place blurs responsibilities, and means that extracting it to do anything else later on is going to be impossible.”

  Obviously the latter.

• 

  Eloy Duran December 3rd, 2008 @ 08:19 PM

  Uploaded an updated patch for current HEAD.

  I changed the option from :nested_params to :accept_nested_attributes, which should more clearly express it's intent. Also the big method in ActiveRecord::NestedAttributes has been split up into smaller methods.

• 

  Pratik December 4th, 2008 @ 11:04 AM

  • Milestone cleared.
  • Assigned user set to “Michael Koziarski”
• 

  Michael Koziarski December 10th, 2008 @ 07:18 PM

  Could you rebase the patch down to one changeset, it's pretty hard to follow when spread over several changes.

  Also, the use of alias_method_chain isn't needed here. You should be able to make those changes inline.

• 

  Eloy Duran December 11th, 2008 @ 01:48 PM

  I created a flattened patch, which is updated for edge again. The changes to OrderedHash caused some breakage, so this patch depends on the patch in ticket #1559.

• You flagged this item as spam.
  

  cainlevy December 14th, 2008 @ 10:22 PM

  Sorry that I missed this before rolling yet another solution!

  Some thoughts on the patch:

  • Form builders are nice! There's something I'm missing in my plugin.
  • I'm not a fan of the new_1234 hash keys. Are those actually meaningful? I didn't see if they're parsed in the code, but they're still used in the examples and tests and that's a bit confusing.
  • Your patch ignores new records that are "empty". That's how I did this back in ActiveScaffold, and it doesn't work in practice. You will run into problems with form elements that always have values like date selectors, radio buttons, or dropdowns.
  • I think you're supporting too many different ways of doing things. We should be opinionated, right? Let's not support deleting records with missing rows. That may work sometimes, but not always, whereas the :_destroy key does work always (and can solve the problem with empty new records, too).
  • I don't like :autosave as an option. It's too much configuration for me. I would rather have an intelligent autosave that simply worked as expected.
  • The :allow_destroy, :destroy_missing, and :accept_nested_attributes options don't feel like association configuration. That is, they don't relate to how the association is set up. Instead they relate to how the association responds to mass assignment from a form. It just feels ... slightly off. I'd rather keep the association setup all database-related with foreign keys, class names, conditions, etc.

  Please feel free to steal any ideas from my new plugin at http://github.com/cainlevy/neste.... An example implementation is available at http://github.com/cainlevy/compl...>

  -Lance

• You flagged this item as spam.
  

  Paweł Kondzior December 17th, 2008 @ 11:33 AM

  When this will land in 2.3 ?

• 

  Eloy Duran December 17th, 2008 @ 12:24 PM

  I will first update the patch once more with some of the feedback from Lance, on this ticket but also by email. So at least it won't be until I've had time for that, which will probably be in this/next week.

• 

  Michael Koziarski December 17th, 2008 @ 12:29 PM

  I'm also travelling for a week or two but intend to take a closer look when I get back on deck.

  Things will slip a bit over the holidays, but we'll get there :)

• You flagged this item as spam.
  

  Paweł Kondzior December 17th, 2008 @ 04:05 PM

  This is Model#nested_changes proposal of implementation.

  changes format for one_to_one: http://pastie.org/341324

  for collections there is only one difference, they are all in [] instead of directly hash: { "reflaction" => { :created => [{...}, {...}, ...] }

  There is no tests in this patch, and no nested_changes.clear after save.

  What do you thnik about this ? If there is place for such implementation here i can do the rest of issues that need to be solved here.

  BTW. there is a bug in assign_to_or_destroy_nested_attributes_records, it should throw NoRecord or similar exception for id that doesn't exsists. Now it throws nil.attributes= exception.

  • nested_changes.diff 3.3 KB

• You flagged this item as spam.
  

  Pascal Ehlert January 9th, 2009 @ 02:23 PM

  bump Any progress on this? I'm really curious to see this functionality in the core as none of the available plugins can keep up with the proposed patches.

• 

  Eloy Duran January 9th, 2009 @ 03:14 PM

  It's still progressing, I just started discussing some stuff with Michael. So expect a new and hopefully final patch.

• You flagged this item as spam.
  

  Pascal Ehlert January 9th, 2009 @ 03:26 PM

  Excellent, I'm glad to hear that. I just thought it might not get the attention it deserves ;-)

  Thanks a lot for your hard work guys.

• You flagged this item as spam.
  

  Ryan Bates January 9th, 2009 @ 04:40 PM

  I know I'm late to the party, but I'd like to test and help contribute to the next patch if it's okay. Eloy, do you think we can communicate over email? ryan AT railscasts DOT com.

• You flagged this item as spam.
  

  Jérôme January 11th, 2009 @ 11:36 AM

  +1

• You flagged this item as spam.
  

  Nicolas Blanco January 12th, 2009 @ 09:05 AM

  +1

• 

  Eloy Duran January 21st, 2009 @ 03:57 PM

  New AutosaveAssociation patch, updated for edge and normalized into 1 diff.

  • 3.autosave_association.diff 32 KB
• 

  Eloy Duran January 21st, 2009 @ 03:57 PM

  New NestedAttributes patch, updated for edge and normalized into 1 diff.

• You flagged this item as spam.
  

  Pascal Ehlert January 21st, 2009 @ 04:42 PM

  How will this handle validation for multiple records, e.g. when you have a project which has_many comments and in your form template you're using fields_for to render these, will validation errors for individual projects be added to the correct fields? (so that field_error_proc works out of the box).

  Also I saw that for has_one associations, you're adding an error for author.name to the parent model as author_name.. What if I'm having an actual author_name field on the parent?

  I think validation is the hardest part here and we should try to do it properly right away.. Why do we even need to add it to the parent model? Wouldn't it be sufficient and cleaner to just refactor the form helpers that actually add the errors so that they will look for errors in the associated objects?

  Last but not least there is a typo/broken sentence in line 205 of the nested_attributes patch. ;)

  Thanks a lot for your effort to finally get this right and if you need any help, let me know. I'd like to contribute as well, maybe you can get me into the boat? pascal DOT ehlert AT odadata DOT eu

• 

  Eloy Duran January 21st, 2009 @ 05:01 PM

  Yes the validations will be added to the correct fields, just as it is right now.

  If you have an author_name field on the parent and an author which has a name it would seem there is unnecessary overlap, so it seems as a non-issue to me at least for now. Unless someone can give me a real life example of breakage of course :)

  The validation messages are added to the parent because in our case I want only 1 error messages list at the top of the form instead of multiple ones breaking the complete layout (and it's ugly anyways). But note that of course the individual fields are still marked as a fieldWithErrors.

  Ah yes that sentence is wrong, thanks for reading it!

  If you would like to help out then please checkout the complex-form-examples and play with it and the patches. Thanks in advance :)

• You flagged this item as spam.
  

  Pascal Ehlert January 21st, 2009 @ 05:09 PM

  Sounds good to me.

  Only the author_name issue kinda bugs me, it may be an unnecessary overlap in most cases, but can we justify to break peoples applications by saying "the field names they're using are wrong"?

  And your argument with the multiple lists is one more reason for me to patch error_messages_for instead of adding them to the parent model. It's not only the naming issue, it's also that it doesn't feel too clean for me to ignore the separation their.

  Just my 2 cents, I'd like to see some further discussion. (Although I'm strongly for getting this into core asap).

• 

  Eloy Duran January 21st, 2009 @ 05:10 PM

  Fixed error in sentence. Thanks Pascal.

• 

  Eloy Duran January 21st, 2009 @ 07:57 PM

  Well, willingly breaking someone's code would indeed be bad :) What I meant was that it's justified for us to assume it won't break any code. And like I said, if someone would popup with a valid reason for why it breaks, I will definitely fix it.

  About the other issue, I personally think it's fine that the parent knows of the errors of its children. Because ultimately you are only dealing with that parent object and saving its children should be its responsibility so you should be able to query the possible errors. Maybe moving it into a separate Errors object would help? (#nested_errors)

• You flagged this item as spam.
  

  Pascal Ehlert January 21st, 2009 @ 11:47 PM

  Yes, that was my thought as well.. Having a nested_errors accessor that directly queries the child error objects.

  That way we could keep it separated but it would still be accessible from the parent object (which indeed is a good idea).

• 

  Eloy Duran January 22nd, 2009 @ 10:44 AM

  Updated the patch to remove the optional second argument for the fields_for block as discussed with Michael. The collection member is simply available through foo_form#object.

• 

  Eloy Duran January 22nd, 2009 @ 10:49 AM

  @Pascal After giving it some more thought, and discussing with Michael, I have decided to mark this as a "could do later" TODO. (I have a few others as well.) I don't think it's an issue to deal with right away for this specific patch.

  Once the patch is applied I will create tickets for these TODO's. This should also, for instance, make it easier for you to work on if you wish.

• You flagged this item as spam.
  

  Pascal Ehlert January 22nd, 2009 @ 11:32 AM

  That's reasonable and speeding the process up a little is a good thing. :) Thanks again and I hope this will make it into trunk quickly now.

  I will then comment on the other tickets (with patches that time ;)).

• 

  Eloy Duran January 22nd, 2009 @ 12:17 PM

  @Pascal Awesome, thanks again for your feedback.

• 

  Eloy Duran January 27th, 2009 @ 10:35 AM

  New patch: Renamed accept_nested_attributes_for method to accepts_nested_attributes_for.

• 

  Michael Koziarski January 29th, 2009 @ 04:20 AM

  That patch doesn't apply to master any more. If you can rebase the latest version, I think we're good to go!

• 

  Eloy Duran January 29th, 2009 @ 08:31 AM

  Updated nested_attributes.diff for edge.

  • 4.nested_attributes.diff 63.7 KB
• 

  Jon Leighton January 29th, 2009 @ 11:06 PM

  I came across this via the Rails blog post. I'm interested because I have been working on solving a similar problem through a plugin I have called Conductor.

  My initial reaction is that this seems very good. I'm pleased to see the problem being addressed in core Rails; whilst I am happy with the API I've come up with for my plugin, this definitely requires less cognitive overhead, which is obviously a good thing.

  I'm not going to comment on the implementation (don't have time to study it properly), but with regard to the API I worry that putting this directly into ActiveRecord is a step too far. There's a lot of "hacking" in the syntax of the parameters which exist purely to overcome the limitations of HTTP parameters and HTML forms. (For instance "_delete", and "new_x".)

  That was the idea behind having a "conductor" as a separate object - it acts as a buffer between the controller in the model. However I think using a completely separate object is probably the opposite extreme and I wouldn't advocate putting that into Rails either.

  With regard to syntax for the parameters, I vastly prefer Approach 1 here, as it contains nothing which is specific to HTTP/HTML. The issue with implementing it is communicating the array over HTTP and HTML validation of the form.

  In my conductor plugin I use an indexed hash like in approach 4. It's a bit more ugly but solves the problem. An ideal solution might be to have ActionController automatically convert "indexed" arrays from the request into "real" arrays in the params hash. I don't know though; perhaps that would be too magic.

  Anyway, in general I think this looks very promising, though I don't particularly like HTTP workarounds creeping into ActiveRecord.

• 

  Eloy Duran January 30th, 2009 @ 09:50 AM

  Thanks for your feedback Jon.

  Rest assured that the '_delete' and 'new_x' keys have nothing to do with any HTTP workarounds :)

  However, if you care to investigate an alternative API feel free to submit a patch.

• You flagged this item as spam.
  

  Kip Cole January 31st, 2009 @ 04:51 AM

  Eloy, tried to apply your Jan 29th patch to the current master branch (commit ed0e5640879fd42c00fc5900e0355a0ea1dcf2ad) but it won't apply. Any chance of an updated patch - unless the commit to master is coming in the next day or so?

• 

  Eloy Duran January 31st, 2009 @ 06:03 PM

  Created a new patch because apparently for some reason my format-patch skills fail… This is just 1 patch with all patches combined in one. Here's the commit message I compiled:

  Added the association accessor methods association_instance_set' andassociation_instance_get'. Replaced all code with this which was accessing the association ivars directly. See #1728.

  Fixed assert_queries to not count BEGIN and COMMIT queries. See #1732.

  Added AutosaveAssociation, which is a module that takes care of automatically saving your associations when the parent is saved. In addition to saving, it also destroys any associations that were marked for destruction.

  Added NestedAttributes module which adds the accepts_nested_attributes_for class method. With this method an attributes writer is defined for an association. This allows you to set and update attributes of associated models through the parent model as well as destroying them. Also included are adjustments to FormBuilder#fields_for to support such nested model attributes.

• 

  Eloy Duran January 31st, 2009 @ 06:03 PM

  And the new patch.

  • association_instance_accessors_and_autosave_association_and_nested_attributes.diff 83.9 KB
• 

  Repository February 1st, 2009 @ 02:02 AM

  • State changed from “new” to “committed”

  (from [ec8f04584479aff895b0b511a7ba1e9d33f84067]) Add support for nested object forms to ActiveRecord and the helpers in ActionPack

  Signed-Off-By: Michael Koziarski michael@koziarski.com

  [#1202 state:committed] http://github.com/rails/rails/co...

• You flagged this item as spam.
  

  orangechicken March 20th, 2009 @ 07:50 PM

  This doesn't work well when validating the relationship. For example, http://gist.github.com/82527. At least I've seen no docs on it yet (and probably haven't looked hard enough...)

  This is a problem when creating new objects.

• 

  Eloy Duran March 21st, 2009 @ 12:11 AM

  @orangechicken See this ticket #1943.

• You flagged this item as spam.
  

  Hans August 2nd, 2009 @ 08:42 AM

  • Assigned user changed from “Michael Koziarski” to “Eloy Duran”

  I am using nested forms on a person model that has many pictures.
  I would like to allow the user to remove on or more of the associated
  pictures from the person using nested forms as suggested by Ryan Bates.

  However delete will delete the picture not remove the associatioon
  I can do it in the controller, but it should be done in the same way as
  delete is performed using attribute delete
  Is there a remove attribute or any other way to solve the problem
  (e.g. that a command like allow_remove=true)

  Another problem is that it seems as nested forms do not validate the
  attributes of the associated object in this case picture (the name should be unique).
  I have solved the problem by a fix in the controller, but shouldn't that be a
  feature of nested forms

  Any comment would be much appreciated

• You flagged this item as spam.
  

  Hans August 3rd, 2009 @ 09:27 AM

  • Assigned user changed from “Eloy Duran” to “Michael Koziarski”

  Sorry about the change of assigned user - my mistake
  Have changed the code and then the validation seems to work
  However, the problem of removing / deleting associations is still a problem

• You flagged this item as spam.
  

  Dusan Maliarik December 3rd, 2009 @ 02:45 PM

  Sorry for the term, but {"id" => 22, "delete" => 1} is retarded. How about checking that attributes hash, and if it contains child key_ and is set to nil, then destroy that child?

  That means, for has_one associations, it should be enough to pass a hash like:

  { :author => nil }
  

  ...and for deleting in has_many:

  { :comments => {
    22 => nil,
    23 => nil
  }}
  

  ...and for deleting, updating and creating in one go:

  { :comments => {
    22 => nil,
    23 => { :title => 'twenytri' },
    0  => { :title => 'titty', :body => 'ritty' }
  }}
  

  However, that 0 key is obvious hack, any ideas on that?

• 

  Ryan Bigg October 9th, 2010 @ 10:10 PM

  • Tag cleared.
  • Importance changed from “” to “Low”

  Automatic cleanup of spam.

• 

  Ryan Bigg October 14th, 2010 @ 08:22 AM

  Automatic cleanup of spam.

• 

  Jeff Kreeftmeijer October 24th, 2010 @ 01:10 PM

  Automatic cleanup of spam.

• 

  Jeff Kreeftmeijer October 24th, 2010 @ 01:12 PM

  Automatic cleanup of spam.

• You flagged this item as spam.
  

  sexshop March 23rd, 2011 @ 01:29 AM

  Excelente blogue, parabéns!

• You flagged this item as spam.
  

  bingbing March 26th, 2011 @ 01:16 AM

  luxury watch

• You flagged this item as spam.
  

  links london April 1st, 2011 @ 09:28 AM

  links of london

• You flagged this item as spam.
  

  fakeraybans May 22nd, 2011 @ 12:26 PM

  i love fake ray bans