This project is archived and is in readonly mode.

#1870 ✓wontfix
cainlevy

accepts_nested_attributes_for should not have a :reject_if option

Reported by cainlevy | February 4th, 2009 @ 09:05 AM | in 2.x

So I was thinking about the :reject_if option in Eloy's nested forms patch, and I've come to the conclusion that there is only one right configuration, and as such, the option should disappear. I don't have a patch or tests or anything yet, but Eloy asked me to write up a ticket with my argument.

This could be seen as an extension of ticket #1861.

From my comments on http://ryandaigle.com/articles/2...>

I believe that the only right :reject_if option is:

:reject_if => proc{|attrs| attrs[:_delete] == "1"}

Any other configuration could possibly reject intentional user input. Even checkboxes, date fields, and booleans could be submitted with default values.

We have the _delete flag so we can capture explicit user intention and not concern ourselves with logic that checks for missing rows, etc. That’s the only way to always get it right. So why not capture explicit user intention for the creation of new rows as well?

And if that is the only right :reject_if configuration, should the :reject_if option disappear altogether so people don’t have the opportunity to get it wrong?

Comments and changes to this ticket

  • cainlevy

    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”
  • cainlevy

    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.

  • cainlevy

    cainlevy February 6th, 2009 @ 05:27 AM

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

  • Pascal Ehlert

    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.

  • cainlevy

    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?

  • Pascal Ehlert

    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

    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 ?

  • cainlevy
  • cainlevy

    cainlevy February 6th, 2009 @ 09:52 PM

    • Tag set to 2.3, attributes, nested, patch

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

  • cainlevy

    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

    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!

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>

Attachments

Referenced by

Pages