This project is archived and is in readonly mode.
Unsaved changes to nested attribute collections are lost when using collection
Commit 5efb1503dd88b59fe491dade92790c3f06293445 causes unsaved changes to records in nested attribute collections to be lost when the collection is used afterwards. This is because the collection is no longer loaded when the update occurs and any subsequent loading afterwards erases the changes.
I tried to find some reasonable way to work around this but even using the new association callbacks doesn't work. If I load the collection with the before_add callback, the record then appears in the collection twice, once from the load and once from the update. Having to load the collection explicitly before any update is just a pain.
I think this feature is fundamentally broken unless some extra code is written to append the records that haven't been updated instead. I therefore would like to see this commit reverted. Attached is a failing test case.
Assigning this to Pratik as it was his commit.
-
James Le Cuirot- Tag set to nested attributes, active_record, testcase
-
Rizwan Reza- No changes were found…
-
Raphael EmourgeonSame problem here and no good workaround.
-
Pete DeffendolI"m not sure if my problem is exactly the same as reported here, but I've narrowed the cause down to the same commit. My issue is that the _destroy attributes in my nested collection hashes aren't causing ActiveRecord to remove the referenced members from the collection. It doesn't happen for all of my models using nested attribute, though.
I'll try to squash it down to a simple example soon.
-
James Le Cuirot
I think that would happen if you're somehow loading (i.e. accessing) the collection before the parent record is finally saved.
-
Pete Deffendol
I stripped down my model and controller to pinpoint the problem, and it turns out that I have a custom validation method that is accessing the collection before it is being saved. So, yes, I have the same problem as the other commenters to this ticket.
-
James Le Cuirot
Here's a possible fix that doesn't involve removing the feature. Records that are already present in the collection are removed from the records that are loaded. I tried to make it not load these existings records in the first place but I couldn't get it to work.
-
James Le Cuirot
Oh and I wasn't sure if it was worth preserving the order of the loaded records. With the above patch, updated records would appear on the end. Does this matter? Maybe it does for things like acts_as_list. If so, I'll adjust it.
-
Jarl Friis
Is #4766 duplicate of this one?
-
James Le Cuirot
Yes!
-
PratikHey James,
Yeah, I think it'll be good to preserve the order. Could you also use git-format-patch for generating the patch so that you're credited when it gets committed. Might be a good idea to combine test and fix too.
Thanks !
-
James Le Cuirot
- Tag changed from nested attributes, active_record, testcase to nested attributes, active_record, patch, testcase
Here you go. I thought it might be as simple as
find_target.map { |r| @target.delete(r) || r } + @targetbut to my surprise, Array#delete returns the object you give it, not the object that was actually deleted.I haven't tried this with _destroy but it should work just the same.
-
Repository
- State changed from new to resolved
(from [0265c708b9696c3943518ad5f3dabdc22c5eba11]) Don't overwrite unsaved updates when loading an association but preserve the order of the loaded records. [#4642 state:resolved]
Signed-off-by: Pratik Naik pratiknaik@gmail.com
http://github.com/rails/rails/commit/0265c708b9696c3943518ad5f3dabd... -
Repository
(from [b41c3ba154c2038ecc7b230693662257833869b8]) Don't overwrite unsaved updates when loading an association but preserve the order of the loaded records. [#4642 state:resolved]
Signed-off-by: Pratik Naik pratiknaik@gmail.com
http://github.com/rails/rails/commit/b41c3ba154c2038ecc7b2306936622... -
Repository
- State changed from resolved to open
(from [85cc1fa657f441417f36998a32a6a158c2697aad]) Revert "Don't overwrite unsaved updates when loading an association but preserve the order of the loaded records. [#4642 state:open]"
This commit introduced a regression described in ticket [#4830].
This reverts commit 0265c708b9696c3943518ad5f3dabdc22c5eba11.
http://github.com/rails/rails/commit/85cc1fa657f441417f36998a32a6a1... -
José Valim- Milestone cleared.
Guys,
This commit was reverted because it make Sam's Ruby test suite (that runs the Depot web application for the Agile Web Development with Rails) fail.
Sam attached a failing scenario at ticket #4830. Could someone please work on it?
Thanks!
-
Jeff BiglerOn a side note, rolling back this commit prevents nested attributes that are not mapped to database fields from being updated.
-
James Le Cuirot
Don't worry, I have fixed the above failure in a way that satisfies both issues. See #4830.
-
Jeremy Kemper- State changed from open to duplicate
-
helg
- Importance changed from to High
at :allow_destroy -> true, I'm still loosing _destroy attribute
-
James Le Cuirot
Damn. Sorry. I should have tested that explicitly. Now I have and here's a patch with a fix. Note that this will still load any outside updates to the record if there haven't been any locally. The key difference is that the destruction property is carried across.
Devs, please apply this to master and 2-3-stable ASAP.
-
Repository
- State changed from duplicate to resolved
(from [f3fedd7f84c25d1d99a70af1e21e20abb48f100f]) Don't remove scheduled destroys when loading an association. [#4642 state:resolved]
Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/f3fedd7f84c25d1d99a70af1e21e20... -
Jarl Friis
Why is Milestone not set to 2.3.9 on this ticket (as it is on ticket #4766) when it apparently is high-priority? Version 2.3.8 is seriously broken with this bug.
Further when milestone is set to 2.3.9 (please...), then the patch should also be applied to 2-3-stable. I am still stuck with 2.3.5 due to this bug... Hoping to see the fix in 2.3.9 before being forced to upgrade to 3.x one day.
Jarl
-
José Valim
This was also applied on 2-3-stable. Please check the commits on Github.
-
Jarl Friis
Thanks for the info, I really tried to find the info myself (I really did), but lack of experience with github and git made me run a
git branch --contains f3fedd7f84c25d1d99a70af1e21e20abb48f100fbut that only revealed the master branch (because the commit on the 2-3-stable was a different commit). I would have expcted a commit on one branch and the a cherry-pick merge from one to the other...
Thanks and sorry for the inconvenience.
-
José Valim
No problem! I usually cherry pick commits from one branch to another but that is not always possible. :(
-