Exclude _destroy parameter in :all_blank check (issue #2937)

[surfacedamage]

When using accepts_nested_attributes_for and a :reject_if => all_blank, the _destroy parameter is also taken into consideration for blank attributes. This can present a situation where a user leaves a form blank for a nested model, submits it, and the nested model record is created anyway.

A Contrived Example

A sample model:

class Pirate < ActiveRecord::Base
  has_many :ships
  accepts_nested_attributes_for :ships, :reject_if => :all_blank,  :allow_destroy => true
end

partial rendered html form for pirate:

<div>
  <input type="text" value="" name="pirate[ships_attributes][0][name]" />
  <input type="hidden" value="0" name="pirate[ships_attributes][0][_destroy]">
  <input type="checkbox" value="1" id="pirate_ships_attributes_0__destroy" name="pirate[ships_attributes][0][_destroy]">
  <label for="pirate_ships_attributes_0__destroy"> destroy</label>
</div>

submitted parameters will contain:

 "ships_attributes"=>{"0"=>{"name"=>"", "_destroy"=>"0"}}

Problem

Although the intention is to reject the ship, the _destroy parameter technically has a non-blank value, therefore the ship record would still be created anyway.

Strategy

My recommendation for this pull request is to modify the ALL_BLANK proc to ignore the _destroy meta attribute. This would preserve the ability to destroy records by setting it truthy, but reject it if all core model attributes are blank.

Issue

https://gist.github.com/rails/rails/issues/2937

[josevalim]

I guess this issue is indicating a larger problem exists. The real issue here is not _destroy, but it is the fact that any boolean value send by a checkbox will make the :all_blank case fail. So another possible solution is to provide something called :all_falsy that would work similar to :all_blank but instead it would test for:

REJECT_ALL_FALSY_PROC = proc { |attributes| attributes.all? { |key, value| value.blank? || FALSE_VALUES.include?(value) } }

You can get all the values considered false by active record adapters here:

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/column.rb#L9

[surfacedamage]

José, you are absolutely correct: this is a much larger problem with boolean fields. I hadn't considered that until I read your response.

However, I am concerned that checking for all false values in this case might be overreaching. What if I have a model called "Amounts" that does nothing but store dollar amounts where zero is a valid value? Indiscriminately checking for false values would incorrectly reject the creation of this record.

As you mentioned, this is a problem with checkboxes, so I think what we really want to check is: 1) is the value blank 2) is the value false AND the attribute is a boolean field

However, since _destroy does not actually exist on the model, it would also need to ignore this meta field separately. This really starts to make implementation messy:

# horrible, horrible pseudo-code -- only being shown as illustration
proc { |attributes| attributes.all? { |key, value| value.blank? || key=='_destroy' || (FALSE_VALUES.include?(value) && boolean_column?(key))} }

What are your thoughts on perusing a simpler implementation that handles 90%+ cases versus just allowing developers to implement their own rejection method?

I'm willing to submit a patch either way.

[josevalim]

key == '_destroy' would be more inline with Rails coding guidelines.

[josevalim]

Ok, good points. I have added some comments, could you please address them before I merge this commit? Also, please add a change to ActiveRecord CHANGELOG as I think this change is worth it. Thanks a lot!

[surfacedamage]

José, made those changes. Let me know if you want me to squash the commits.

[josevalim]

Please do squash as they will be a better unit when checking out the logs. Thanks a lot!

[surfacedamage]

Squashed and force pushed.