This project is archived and is in readonly mode.
Nested attribute accessors should ignore new records with truthy _delete key
Reported by Pascal Ehlert | February 3rd, 2009 @ 04:49 PM
Currently when you supply a _delete attribute on new nested records, it will be passed through to the actual model's build method and raise an UnknownArgumentError there.
In my opinion those records should simply be ignored, so that you can delete newly added records the same way that you delete existing ones without much JS or controller magic.
Comments and changes to this ticket
-
Eloy Duran February 3rd, 2009 @ 05:55 PM
I would suggest to change the test test_should_remove_delete_key_from_arguments_hash_of_new_records to use '0' for the 'delete' key. Right now the record is rejected because delete has a truthy value, if you use '0' it will build and actually test what you intended to.
Other then that I agree with the patch.
-
DHH February 3rd, 2009 @ 06:34 PM
- Milestone cleared.
-
Pascal Ehlert February 3rd, 2009 @ 06:47 PM
Haha yes, I've been pretty tired when I did the patch. Thanks for the review and please find attached the updated version.
-
Eloy Duran February 4th, 2009 @ 08:27 AM
I have added some documentation and fixed a few whitespace errors. I'm a whitespace fanatic :)
+1 The patch indeed tests the expected behaviour.
-
Pascal Ehlert February 4th, 2009 @ 10:27 AM
Excellent, thanks a lot. I thought it would be obvious that a _delete key on new records rejects them, but it's certainly good to clearly state it in the documentation.
Where have you found a whitespace error? I'm using TextMate with two spaces indentation, so there shouldn't be any. I suppose we just have a different idea of how it should be done here, because I'm usually careful with whitespaces as well. ;-)
Thanks.
-
Eloy Duran February 4th, 2009 @ 10:42 AM
The Rails source in general uses no spaces at all for empty lines. Textmate, however, adds spaces up-to the current indentation level. I myself prefer Textmate's style, but we should follow the general Rails convention.
The “whitespace errors” I mentioned were around your test method :-)
-
Pascal Ehlert February 4th, 2009 @ 10:51 AM
Totally agree and having no meaningless characters in the code does actually make more sense to me.
-
Repository February 6th, 2009 @ 12:48 AM
- State changed from new to committed
(from [455a7633dbdb295de828eb2657433d47d85eb0bc]) Nested attribute accessors should ignore new records with truthy _delete key.
Signed-off-by: Michael Koziarski michael@koziarski.com [#1861 state:committed] http://github.com/rails/rails/co...
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
People watching this ticket
Attachments
Tags
Referenced by
- 1870 nested forms should not have a :reject_if option This could be seen as an extension of ticket #1861.
- 1861 Nested attribute accessors should ignore new records with truthy _delete key Signed-off-by: Michael Koziarski michael@koziarski.com [#...
- 1870 accepts_nested_attributes_for should not have a :reject_if option this patch drops the :reject_if option, and fixes a linge...
- 1870 accepts_nested_attributes_for should not have a :reject_if option Could you please create a separate patch for the '_delete...
- 1870 accepts_nested_attributes_for should not have a :reject_if option Ahh, nevermind on the #1861 bug. I wrote the test coverag...