Rails Ruby on Rails → 2.x

Comments and changes to this ticket

• You flagged this item as spam.
  

  cainlevy February 6th, 2009 @ 02:00 AM

  • Title changed from “nested forms should not have a :reject_if option” to “accepts_nested_attributes_for should not have a :reject_if option”

• You flagged this item as spam.
  

  cainlevy February 6th, 2009 @ 02:09 AM

  • Tag changed from “attributes, nested” to “2.3, attributes, nested, patch”

  this patch drops the :reject_if option, and fixes a lingering issue from ticket #1861 whereby _delete would still not cause a new record to be ignored if allow_destroy was false.

  • drop_reject_if_option.diff 11.4 KB

• You flagged this item as spam.
  

  cainlevy February 6th, 2009 @ 05:27 AM

  This patch is rolled into the patch for ticket #1892.

• You flagged this item as spam.
  

  Pascal Ehlert February 6th, 2009 @ 08:08 AM

  • Tag cleared.

  Sorry, but we should leave it up to the developers what they consider right configuration here.

  In my case, I use nested attribute forms for e.g. a selection of multiple agencies per project.

  If the user adds a new drop down but does not select an agency (leaves it blank), I will simply reject it to not cause any more inconvenience to the user by giving them a validation error.

  I don't understand your arguments anyway, how could anyone get this wrong? That's why we have documentation.

  -1 on this.

• You flagged this item as spam.
  

  cainlevy February 6th, 2009 @ 08:29 AM

  Why did you clear all of the tags?

  First of all, we shouldn't rely on documentation if there's an option to simply design the system better. Whether this improves the system is open for debate.

  Your use case is easily solved with most any approach, because you know when the submitted record is empty. For any non-trivial case where fields may have default values (such as with booleans, dates, and checkboxes), the delete key is necessary to be absolutely clear. And if the delete key is necessary in some situations, and is sufficient in other situations, then why not just make it the right way?

  Perhaps you could explain how the _delete key could detract from the solution in your case?

• You flagged this item as spam.
  

  Pascal Ehlert February 6th, 2009 @ 09:48 AM

  • Tag set to “2.3, attributes, nested, patch”

  So yes, I know when the record is empty. But the only way to reject it will be to have a failing validation and give the user an error message or manually in the controller, which means more code noise for something we can easily and cleanly solve in the accessor.

  The first option doesn't work because I want to ignore those empty records silently without bothering the user again. The second option is reasonable, but not better than having it directly in the AR part.

  Sorry about the tags, I must have somehow cleared out the box.

• 

  Eloy Duran February 6th, 2009 @ 10:12 AM

  I understand the reasoning, however, I do not want to force the user to have a checkbox in their form to solve the reject/create logic. I understand that the checkbox can be hidden with JS, but things should really Just Work without JS so this is not really an option to me.

  I think that if we support both :reject_if and the '_delete' key as an alternative to reject new records from being build, that we have the tools to support both strategies.

  Could you please create a separate patch for the '_delete' issue, that you mentioned is part of this patch, and add it to #1861 ?

• You flagged this item as spam.
  

  cainlevy February 6th, 2009 @ 09:51 PM

  • Tag cleared.

  will do

• You flagged this item as spam.
  

  cainlevy February 6th, 2009 @ 09:52 PM

  • Tag set to “2.3, attributes, nested, patch”

  huh, now i've cleared the tags. lighthouse bug?

• You flagged this item as spam.
  

  cainlevy February 6th, 2009 @ 10:50 PM

  • Tag changed from “2.3, attributes, nested, patch” to “attributes, nested, patch”

  Ahh, nevermind on the #1861 bug. I wrote the test coverage and undid my changes to make sure the tests failed, only to discover that I had missed Pascal's solution to the problem.

  I still feel that the delete behavior makes reject_if unnecessary, and I don't know how a delete checkbox would make things not Just Work without JS, and I think the burden of proof should be for use cases that require :reject_if. But I'll be content to drop the issue and alter #1892 to keep the :reject_if option.

  Note to self: if this got picked up again, I would need to redo the patch w/o the unnecessary fixes for the bug.

• 

  Eloy Duran May 18th, 2009 @ 07:37 PM

  • State changed from “new” to “wontfix”

  As discussed on the ticket; won't fix, at least for now. Thanks!