accepts_nested_attributes_for patch request

Nice work on the accepts nested attributes for, I've been waiting for
this for a while and haven't had the time to write it.

I am curious to know about the feature for deleting attributes. For a
lot of reasons, I prefer to just not send back the items I want
deleted. Do you think it would make sense to add an attribute called
something like deletes_when_not_present that would essentially update
or create the nested objects, and then deletes any that are present
still in the database that weren't in the hash? Here is the basic of
the workaround I'm using...

old_ids = parent.get_children_ids
parent.update(params[:parent])
old_ids.each do |oid|
  oid.member? parent.children_ids || Parent.destroy(oid)
end

Is there a reason it was skipped?

I'd be more than happy to write a patch for it, but I don't want to
waste time if its breaking convention. I work in very large xml data
sets with flex and sending redundant data gets costly and cuts down on
efficiency.

-Josh

Eloy would know for sure, but I believe it was left out because of the
risk in accidentally deleting objects just because you disabled a form
element, or was paginating in a form, or something similar.

It would also cause problems if you have a multi-user environment. User 1 downloads the form, User 2 completes an edit form that adds a new association to the collection, User 1 posts their form without having record of the newly created record from User 2 --> User 1 accidentally deletes User 2’s addition.

I suspect the same scenario is possible for plain old attributes in a form, but seems more harmful in the case of accepts_nested_attributes_for.

-John

Nice work on the accepts nested attributes for, I've been waiting for
this for a while and haven't had the time to write it.

I am curious to know about the feature for deleting attributes. For a
lot of reasons, I prefer to just not send back the items I want
deleted. Do you think it would make sense to add an attribute called
something like deletes_when_not_present that would essentially update
or create the nested objects, and then deletes any that are present
still in the database that weren't in the hash? Here is the basic of
the workaround I'm using...

old_ids = parent.get_children_ids
parent.update(params[:parent])
old_ids.each do |oid|
oid.member? parent.children_ids || Parent.destroy(oid)
end

Is there a reason it was skipped?

Eloy would know for sure, but I believe it was left out because of the
risk in accidentally deleting objects just because you disabled a form
element, or was paginating in a form, or something similar.

Yep those were the reasons. It was removed especially because it was all highly theoretical.

We could of course add an option to accepts_nested_attributes_for which enables this.
But I would suggest you'd first use your code as is for now. Once you feel you've dealt with multiple types of associations and/or edge cases then create a patch which abstracts your code.

That's just a suggestion though, any patches are welcome of course.

Cheers,
Eloy

I can agree that its dangerous to destroy attributes in this means,
but for my case there is no other way, and each record is only
modified by one person at a time anyways. I realized my code above for
a workaround was wrong and without the ability to know what
association ids were updated its easier to skip the 2.3.2
nested_attributes.rb features and go back to the same way I've done it
since pre 2.0.

So now I am using my own version called dangerous_nested_attributes.rb
which includes a
:destroy_missing => true
property.

I put a post about it on my site, here is the link to the article =>
http://jibwa.com/samples/dangerous_nested_attributes.rb

Here is a link to the file =>http://dangerous_nested_attributes.rb

I'm somewhat new to these groups but if there is some other way I
should share my code with the community, or if I should submit it as a
patch let me know.

-Josh

Here is the file
http://jibwa.com/samples/dangerous_nested_attributes.rb

here is the article
http://jibwa.com/code-and-documentation-for-programmers/rails-nested-attributes-custom-modific.html

"I can agree that its dangerous to destroy attributes in this means,
but for my case there is no other way"

That's a pretty strong claim. Could you explain a little?

I am not using rails views for this project and instead I am using
advanced custom flex (and Air) forms that operates with or without
connectivity until the user saves. For those of you familir with it,
there are multiple xml bound lists, and controls emulating
itemrenderers, some with internal lists. The data is being sent back
and fourth as xml, and the rails params hashing/netsing works
wonderfully for this. It is difficult to explain but there are forms
that build forms, and there are administrative interfaces to decide
which forms provision which data for the particular forms.

There are two main reasons I can't send the _delete with id methods.
For one the user is able to create instances of things within the flex
forms, and then append children, which contain large control sets of
buttons, dropdowns, etc. They can also completely remove items, then
build them back.

I suppose in summary the forms themselves generate sample reports and
reflect all their data in some means, and the users approve the
reports and E-Sign before submission. Unfortunately the nature of my
need to have a form save and completely revise its database tables is
crucial to keeping that approval process accurate and keeping old
references is more risky then deleting new ones since the user is then
approving what is in front of them, and I send it all back as on xml
object.

If this were Ajax/JS I would just do background removal of information
as the user goes along, but its just too dynamic to support sending
these requests....

Sorry, the second main reason is that (especially with XML), I feel
like sending back blank tags is redundant, especially with the number
of controls I have, the doctor could create and destroy hundreds of
little xml objects while they are working on or updating their reading/
diagnosis, and again create as many. This potentially tags on lots of
information since at the moment, I only send back the user input from
activated controls.

Each control has a descriptor xml object which tells it how it should
look, and what group, placement on the form. Then there is another
descriptor that comes nested telling it whether or not the user has
selected it, or what the user typed into it, clicked on, where it was
dragged to. There are an average 300 of these controls throughout the
form and its tabs and renderers. I at the moment delete the nested
attributes (from my xml in flex) and add new references back in when
the user selects, or deselects/destroys them, and it means the
smallest amount of data transmission from the user to the sever.

I suppose my situation sits this way. I can either send only the
params/xml representing the controls the user has activated. Or I
could send back a params/xml block representing every control, some
with ids and _deleted, others just blank. Sending back blank data for
all 300+++ (as scope creeps) could would mean 2 to 10 times more data
(depending on the situation) going back up to the server.

-Josh

first off, nested attributes are wickedly cool, so thanks for that.

i just built a form that has a bunch of STI models, each with a
'value' attribute. with STI + nested_attributes not only was i able
to handle all the children in the same fashion, but i was also able to
merge the new/create/edit actions. here's the view

  http://gist.github.com/91531

and the associated controller, which is of course less interesting
except maybe the 'blank!' method which just hangs empty associations
off of the model object

  http://gist.github.com/91532

now re-using the view for all those actions makes this a bit more
complicated (but also nicer to the user), nevertheless three things
stood out as being *too* hard for most people of even modest skill:

  1) building up the right ids and names - the patch i sent it to name
them in a POLS fashion, which you reviewed, fixes this completely
although the names/ids are still verbose there's no way around it

  2) the whole blank association thing isn't quite obvious. i have
":reject_if => {|h| h.values.all?{|v| v.blank?}}" of course, but this
pattern of hanging blank objects off the parent in the right places in
actions (such as when an error occurs on a post and you need the error
page to have the blank objects you just rejected as they were all
blank) could use some thought.

  3) finally, as josh mentions. deleting is a bitch, esp if you are
intermixing valid updates with some deletes. maybe i missed an easier
way but the simplest paradigm would seem to be 'copy and element rails
made and munge the fuck out of it until it makes a valid deletion hash
when unpacked into params'

so, on 3 i am *sort* of with josh. my 2cts, however, is that doing
automatic object deletion is a *huge* deal that could make some users
really *mad* if done right for all the reasons you mention. because
of that i think and new AR method is the ticket: what josh and i
really want is a 'replace_attributes' method. that name is so
explicit, and has no un-intended side effects (like nuking stuff in
the middle of a paged post) that i think it would be the best way.
the impl is obvious, transaction{ nuke; update; } and i'd be happy to
build a patch, but would like some feedback on the idea.

kind regards.

-a

On any update attributes method we don't destroy attributes that
aren't present,
But when an attribute is present we don't combine the old and new
attribute
I am not certain about has_one, but I think for sure we treat it like
a real attribute, so a blank nested attribute should mean unlink, if
not destroy it... (or an option for such!)

In a has many a nested attribute isn't an attribute at all, its a
foriegn key reference to another table. So, maybe instead of making a
total destruction patch, at least have the feature

have a accepts_nested_attributes_for something :unlink_not_present =>
true

This will at least remove the reference to the id in a join table.

Maybe, maybe not?

Josh,

Rails already have keywords for that: :nullify in associations
means :unlink.
So why do not do like this:

accepts_nested_attributes_for :something, :when_missing => :nullify

Those values could
be: :nullify, :destroy, :destroy_all, :delete, :delete_all.
Which are all the options supported by :dependent in associations.

Hi Josh and Ara,

I'm gonna respond inline to all thoughts/questions:

Josh,

Rails already have keywords for that: :nullify in associations
means :unlink.
So why do not do like this:

accepts_nested_attributes_for :something, :when_missing => :nullify

Those values could
be: :nullify, :destroy, :destroy_all, :delete, :delete_all.
Which are all the options supported by :dependent in associations.

I agree that if we are gonna add any of these types of linking/unlinking methods they should reflect those that AR already supports. I myself made the mistake of using _delete instead of _destroy, which is what actually happens, so that would have to be fixed as well then. Sorry :slight_smile:

On any update attributes method we don't destroy attributes that
aren't present,
But when an attribute is present we don't combine the old and new
attribute
I am not certain about has_one, but I think for sure we treat it like
a real attribute, so a blank nested attribute should mean unlink, if
not destroy it... (or an option for such!)

In a has many a nested attribute isn't an attribute at all, its a
foriegn key reference to another table. So, maybe instead of making a
total destruction patch, at least have the feature

have a accepts_nested_attributes_for something :unlink_not_present =>
true

This will at least remove the reference to the id in a join table.

Maybe, maybe not?

You should already be able to at least replace a single associated with a new one by not sending the :id in the attributes.

But I'd love to see improvements in this area which cover collection associations as well and in a way which makes it as transparent as possible, so for instance re-using the keywords from the AR API as suggested by José.

first off, nested attributes are wickedly cool, so thanks for that.

Glad you like it :slight_smile:

now re-using the view for all those actions makes this a bit more
complicated (but also nicer to the user), nevertheless three things
stood out as being *too* hard for most people of even modest skill:

  1) building up the right ids and names - the patch i sent it to name
them in a POLS fashion, which you reviewed, fixes this completely
although the names/ids are still verbose there's no way around it

  2) the whole blank association thing isn't quite obvious. i have
":reject_if => {|h| h.values.all?{|v| v.blank?}}" of course, but this
pattern of hanging blank objects off the parent in the right places in
actions (such as when an error occurs on a post and you need the error
page to have the blank objects you just rejected as they were all
blank) could use some thought.

By “the whole blank association thing” are you referring to instantiating new records before building a form with empty entries so the user can create new ones? If so, that seems imo to something you already have in your new/edit action flow so it shouldn't be a too big hurt, but does allow the developer as much flexibility as possible.

However, there is in fact a patch which could make this a great deal easier by Mike Breen, please verify and/or respond to the ticket if this idea would fit your (anyone's) applications: https://rails.lighthouseapp.com/projects/8994/tickets/2365

I'm not sure if you were suggesting some default option which does: ":reject_if => {|h| h.values.all?{|v| v.blank?}}"? If so, trying to abstract this from the developer went wrong quite fast, because the hash might in fact not be empty at all for various reasons while from the developers pov it was considered "blank". This can happen because you use the checkfields helper which always add a value, or have another blank nested attributes hash inside the "blank" hash, etc. Trying to come up with a way to deal with those issues would require complicated code and was at least at that time a reason for me to leave it as non ambiguous as possible, which was letting the developer handle it exactly as she wanted.

  3) finally, as josh mentions. deleting is a bitch, esp if you are
intermixing valid updates with some deletes. maybe i missed an easier
way but the simplest paradigm would seem to be 'copy and element rails
made and munge the fuck out of it until it makes a valid deletion hash
when unpacked into params'

I don't quite follow. Correct me if I misunderstood;
If you have a form which contains fields for an existing record, then deleting it means setting its :id attribute to a truthy value, that seems fairly easy to me.

so, on 3 i am *sort* of with josh. my 2cts, however, is that doing
automatic object deletion is a *huge* deal that could make some users
really *mad* if done right for all the reasons you mention. because
of that i think and new AR method is the ticket: what josh and i
really want is a 'replace_attributes' method. that name is so
explicit, and has no un-intended side effects (like nuking stuff in
the middle of a paged post) that i think it would be the best way.
the impl is obvious, transaction{ nuke; update; } and i'd be happy to
build a patch, but would like some feedback on the idea.

Yes, we should _never_ turn on any automatic deletion/unlinking method by default. However, I don't think it's necessary to add an extra accessor, we simply need an implementation.

Let me put together what I have said throughout earlier similar discussions. I'm open to any of these sort of changes under the following circumstances:
* It should be an extraction, not a theoretical thing. Since I didn't need that at the time it was safer to leave this kind of behaviour out and see what others who do need it come up with. (Which could have been myself as well, but the situation hasn't occurred yet.)
* As transparent as possible, so the API should be familiar to developers using AR.
* It should be as naive and simple as possible, ie no magic, to not obstruct a developer in creating complex forms or abstract her too far away from the metal. Because these types of forms can become complicated in ways we can never imagine and we should not limit that possibility.

So seeing the two of you actually have applications that need to deal with these situations; Yes please, a patch would be great! What a long answer huh? :wink:

Also see the following tickets which have been discussing this as well:
* https://rails.lighthouseapp.com/projects/8994/tickets/1892 (near the end)
* https://rails.lighthouseapp.com/projects/8994/tickets/2036

Cheers,
Eloy

Eloy, maybe we should think on an API that support all cases without
making things messy.

We have those actions:

  :destroy, :delete, :delete_all, :nullify

And two events:

  :request (when :_delete is sent and is default) and :missing

So my suggestion is:

  accepts_nested_attributes_for :association, :allow => :destroy

This is the same as :allow_destroy => true, except that :_destroy
should be sent. We can also have more than one action:

  accepts_nested_attributes_for :association, :allow =>
[ :destroy, :nullify ]

If we want that to work on missing cases, we should do:

  accepts_nested_attributes_for :association, :allow => :destroy, :on
=> :missing

What do you think? I could help on the implementation as well.

ps: only one thing is not clear on this approach, when :on => :missing
is given, we should also accept :_destroy or :_delete to be sent?