Rails Ruby on Rails → 2.x

Comments and changes to this ticket

• 

  Eloy Duran February 22nd, 2009 @ 04:34 PM

  • State changed from “new” to “wontfix”

  Using the nested_attributes example from github, and going into script/console mode, how can you modify the name of the project AND add an /existing/ tag to the project and save that atomically ?

  There's no specific support for habtm with regards to adding children to other parents. What you could do is assign the ids of the children:

  
  project.tag_ids = [1]
  project.name = 'new name'
  project.save
  
  project.reload.name # => "new name"
  project.tags # => [#<Tag id:1>]
  

  About your second question; No I don't expect that to happen atomically as it already automatically saves everything by using #<<. I think this is only an ‘issue’ in test cases and script/console as you mentioned, so it's not a real problem. If you want this to happen atomically you'll just have to wrap it in a transaction yourself.

• You flagged this item as spam.
  

  Lawrence Pit February 22nd, 2009 @ 09:26 PM

  I think the example has_and_belongs_to_many :tags used to show off the accepts_nested_attributes_for feature is misguided then.

  It isn't really an example of what makes a habtm a habtm. To me habtm is all about linking existing records, not about actually CRUD'ing records on the other end.

  I think a requirement for accepts_nested_attributes_for should be that it supports adding existing records to has_many and habtm.

  In your example, when you do project.tag_ids = [1] it will immediately save those ids to the database. It's not an atomic transaction.

  It's not only an issue in test cases and script/console. Consider this, as an example, in one of my forms I save a Group which has and belongs to many People. When an existing group is edited, currently the controller receives this:

  
  Processing GroupsController#update (for 127.0.0.1 at 2009-02-23 07:52:04) [PUT]
    Parameters: {"action"=>"update", "_method"=>"put", "id"=>"2", "controller"=>"groups", "group"=>{"name"=>"foo", "person_ids"=>["1", "3", "2", ""]}}
  

  In the controller I do:

  
  def update
    if @group.update_attributes(params[:group])
      flash[:notice] = 'Group was successfully updated.'
      redirect_to(groups_path)
    else
      render :action => "edit"
    end
  end
  

  Currently this is doing 2 transactions though.

  Wrapping it in a transaction yourself? I'm surprised you're saying that. I could have said that before you committed all that stuff regarding autosave and accepts_nested_attributes_for. Wasn't the whole point here that you want all your changes POSTed to first validate as one, and then commit automatically / atomically as one?

• 

  Eloy Duran February 22nd, 2009 @ 09:52 PM

  I think the example has_and_belongs_to_many :tags used to show off the accepts_nested_attributes_for feature is misguided then. It isn't really an example of what makes a habtm a habtm. To me habtm is all about linking existing records, not about actually CRUD'ing records on the other end.

  I understand. The example was simply meant to test the CRUD part. Sorry if this misguided you. I agree with your point though that it would be nice to have, but it should be an abstraction of an actual app/use case imo so we shouldn't make it up here by committee :) Do you have an idea or rough code on how to solve this?

  Oh and there has already been some discussion already about such functionality on #1892.

  In your example, when you do project.tag_ids = [1] it will immediately save those ids to the database. It's not an atomic transaction.

  You're right, my bad I tested this the wrong way.

  Wrapping it in a transaction yourself? I'm surprised you're saying that. I could have said that before you committed all that stuff regarding autosave and accepts_nested_attributes_for. Wasn't the whole point here that you want all your changes POSTed to first validate as one, and then commit automatically / atomically as one?

  My remark about adding a transaction yourself was directed at your second question about using #<< to add a record but only adding it in the #save call to the parent. Nested attributes and autosave are meant for actions which you want to do in 1 action. Using methods like #<< in this way was not its target.

  But if you have a case which proves otherwise I'm happy to be proven wrong.

• You flagged this item as spam.
  

  Lawrence Pit February 23rd, 2009 @ 01:43 AM

  I forked your complex-form-examples sample app, and created a new branch 'linking', and added some code with a suggestion how to manage habtm relationships:

  http://github.com/lawrencepit/co...

  You get something like:

  http://img.skitch.com/20090223-b...

  Attached is a diff that needs to be applied to the rails edge to get it to actually work.

  Kind of though. The issue of course is that though it does seem to work, two transactions are used. I.e., when the validation of a project fails, the people that were added to and/or removed from the project in the form have already been committed to the database.

  If project.people_ids = [1,2,3] would work without automatically committing then the view code can be even simpler of course, but doing it this way (assigning a hash to project.people_attributes) you can also partially add and/or remove records from a collection.

  I thought I first throw this at you, see what you think of this direction, and possibly you have some suggestions on how to go forward with this.

  • unlink.diff 2.9 KB

• You flagged this item as spam.
  

  Lawrence Pit February 23rd, 2009 @ 01:49 AM

  • Title changed from “autosave unclear for habtm” to “Linking and unlinking via accepts_nested_attributes_for”
• 

  Eloy Duran February 23rd, 2009 @ 08:34 AM

  • State changed from “wontfix” to “hold”

  Kind of though. The issue of course is that though it does seem to work, two transactions are used. I.e., when the validation of a project fails, the people that were added to and/or removed from the project in the form have already been committed to the database.

  I see. The reason is that you are using #delete on the association collection: “send(association_name).delete(existing_record)”. I'm not sure if you expected that using #delete would only remove it from the collection as it does on Array, but on AssociationCollection it actuallt deletes the record from the db. All the methods that mutate the collection seem to do this, but not 100% sure.

  So what you should do is move the actual removing of the record from the collection into autosave_association instead of nested_attributes. Take a look at mark_for_destruction as an example. Records that should be destroyed are marked for destruction in nested_attributes, but they are not removed until the parent is saved and autosave association goes through all records that should be destroyed.

  If you feel that using the foo_ids= method is the best way from experience then you could add a method like mark_for_destruction but assigns the ids.

• You flagged this item as spam.
  

  Lawrence Pit February 24th, 2009 @ 12:33 AM

  I know that normally delete and push on an association collection execute directly on the db. I had expected though that the autosave feature was already implemented in such a way that it wouldn't, i.e. hold off until save was called on the parent object.

  I've just implementing this behavior. Attached a patch for AR that works. Linking and unlinking of association records now is committed atomically when the parent object is saved. If validation of the parent fails, nothing has been done in the database.

  Next step is to modify fields_for_with_nested_attributes in actionpack a bit such that it calls block for unlinked records as well (which can exist if validation failed). Should have that tomorrow.

  • unlink.diff 12.5 KB
• 

  Eloy Duran February 24th, 2009 @ 08:48 AM

  Some thoughts:

  • Please write your patch with the last patch from #1930 applied.
  • Keep any code related to autosaving in the AutosaveModule. (So override methods and use super, or alias_method_chain in cases where super won't work.)
  • Not sure if you were gonna keep it that way, but please refactor/break apart your test case, because it's very hard to follow its intentions.
  • I'd rather not see any changes to fields_for regarding implementation details of the autosaving code, ie it should just work.

• You flagged this item as spam.
  

  Lawrence Pit February 24th, 2009 @ 01:25 PM

  Attached a patch that first requires patch from #1930.

  Hopefully the extra autosave tests are more clear now.

  Can you explain how I could get the code (that I currently added to AssociationCollection) into the AutosaveAssociation module? The AssociationCollection is at a different level, so at least it requires another module if you wanted to keep it really separate I think. But then, I don't quite see how to refactor this nicely. For example, how would you extract the autosave bits from the +delete+ method of AssociationCollection as I patched it?

  • unlink.diff 10.3 KB

• You flagged this item as spam.
  

  Di Marcello March 9th, 2009 @ 06:17 AM

  I wrote my own commit, since i don't know how to use .diff files.

  But i think this is the solution: http://github.com/DiMarcello/rai...

• 

  Eloy Duran March 9th, 2009 @ 08:49 AM

  @Lawrence: If you really need to change the behaviour of other methods you can override them in a Module and then call super. This is done whit the has_many, has_one etc macros ijn AutosaveAssociation.

  However, this is only needed if you want to change the normal #destroy/#push etc to work with AutosaveAssociation. The title of your ticket however suggests you wanted to be able to do this through nested attributes, in which case it isn't necessary to override these methods. You can keep it all in one place, like Di Marcello did.

• 

  Eloy Duran March 9th, 2009 @ 08:49 AM

  @ Di Marcello: I quickly skimmed it and I understand the intent. But there are no tests and I want extensive test coverage.

  Maybe, now that I understand how you guys would like to see this implemented, I can write one. But I'm not promising anything.

• You flagged this item as spam.
  

  Lawrence Pit March 10th, 2009 @ 10:26 PM

  @DiMarcello Thanks mate, that was exactly the implementation I had in mind.

  @Eloy I think it is necessary to the change the normal #destroy/#push methods to work with AutosaveAssociation otherwise the code from DiMarcello doesn't execute as an atomical transaction.

  I'm just back from a holiday, need to catch up on a lot of other stuff, when I have, I'll add some tests for what DiMarcello added.

• You flagged this item as spam.
  

  Di Marcello March 11th, 2009 @ 09:26 PM

  yeah sorry about the tests m8's...

  i don't really test.. later i tested it on my own app (without a real test) found some typo's and sort.

  committed those to. but i really need to start writing tests, also for my apps. havent really gotten in to those

  I also thought the original delete mark should be destroy. so unlink(pretty weird) can get delete as they are called in AR

• You flagged this item as spam.
  

  Lawrence Pit March 12th, 2009 @ 12:58 AM

  delete mark should be destroy, so unlink can be delete

  I agree with that.

• You flagged this item as spam.
  

  PeterW November 24th, 2009 @ 08:15 PM

  I have been stuck with the same problem discussed here (and also in #1892): how to get habtm relationships to work well with accepts_nested_attributes_for. These discussions are a couple months old: do you know whether there has been any changes since then?

  Notably, in the two cases below:

  1) Deleting the association instead of destroying a record.

  In the alloy-complex-form-example, we have:
  class Project < ActiveRecord::Base
  has_and_belongs_to_many :tags end
  If you edit a "Project" and "remove" a specific "Tag", it destroys the
  Tag record itself instead of only the link in the join table.

  2) Creating an association instead of a record.

  With the same example, there doesn't seem to be a way to associate an
  existing Tag to a Project. If you pass a Tag ID (from the view), it does get saved (unless I am doing something wrong?).

• 

  Eloy Duran November 27th, 2009 @ 01:22 PM

  I too would like to know if there has been any progress?

  Just to be clear, I would love the features to be expanded, but this should be written by somebody that actually needs it. Which, at this moment, certainly isn't me…

  Otherwise I'll close this ticket somewhere next week, at least for the time being.

• You flagged this item as spam.
  

  Andrea Campi January 5th, 2010 @ 10:50 PM

  I do need this functionality, and I had already started monkey-patching it as a proof of concept.
  I will look at cleaning up the original patch (Di Marcello's patch seems to be gone, I'm pinging him too), with tests.
  Can you hold on cloing this for a bit?

• You flagged this item as spam.
  

  Lawrence Pit January 5th, 2010 @ 11:19 PM

  Hi, a few months ago I made a plugin for this, I've been using it since then to satisfaction in a real live app.

  I've just uploaded this plugin, see:

  http://github.com/lawrencepit/autosave_habtm

  There a few rspecs in the spec/lib directory. Always intended to integrate this in rails, just never seem to get around it ;[

• You flagged this item as spam.
  

  Lawrence Pit May 2nd, 2010 @ 10:22 AM

  fyi: I'm going to pursue this via various new tickets. First step is done, which allows linking/unlinking of records from a habtm assocation using :autosave => true. See:

  https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets...