Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

accepts_nested_attributes_for should not have a :reject_if option

#1870

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 Nested attribute accessors should ignore new records with truthy _delete key.

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?

Reported by cainlevy · February 4th, 2009 @ 09:05 AM

State: wontfix
Milestone: 2.x
Assigned to: nobody
Importance: none

Activity

  1. cainlevy
    cainlevy
    • Title changed from nested forms should not have a :reject_if option to accepts_nested_attributes_for should not have a :reject_if option

    February 6th, 2009 @ 02:00 AM

  2. cainlevy
    cainlevy
    • 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 Nested attribute accessors should ignore new records with truthy _delete key whereby _delete would still not cause a new record to be ignored if allow_destroy was false.

    February 6th, 2009 @ 02:09 AM

  3. cainlevy
    cainlevy

    This patch is rolled into the patch for ticket #1892 nested attributes should not have meaningful hash keys.

    February 6th, 2009 @ 05:27 AM

  4. Pascal Ehlert
    Pascal Ehlert
    • 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.

    February 6th, 2009 @ 08:08 AM

  5. cainlevy
    cainlevy

    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?

    February 6th, 2009 @ 08:29 AM

  6. Pascal Ehlert
    Pascal Ehlert
    • 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.

    February 6th, 2009 @ 09:48 AM

  7. Eloy Duran
    Eloy Duran

    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 Nested attribute accessors should ignore new records with truthy _delete key ?

    February 6th, 2009 @ 10:12 AM

  8. cainlevy
    cainlevy
    • Tag cleared.

    will do

    February 6th, 2009 @ 09:51 PM

  9. cainlevy
    cainlevy
    • Tag set to 2.3, attributes, nested, patch

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

    February 6th, 2009 @ 09:52 PM

  10. cainlevy
    cainlevy
    • Tag changed from 2.3, attributes, nested, patch to attributes, nested, patch

    Ahh, nevermind on the #1861 Nested attribute accessors should ignore new records with truthy _delete key 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 nested attributes should not have meaningful hash keys 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.

    February 6th, 2009 @ 10:50 PM

  11. Eloy Duran
    Eloy Duran
    • State changed from new to wontfix

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

    May 18th, 2009 @ 07:37 PM