Rename nested attributes _delete to _destroy to reflect its actual behavior and DSL (:allow_destroy). Deprecation warning added. [#2889 state:resolved]

Signed-off-by: Eloy Duran <eloy.de.enige@gmail.com>

activerecord/lib/active_record/nested_attributes.rb

@@ -66,10 +66,10 @@ module ActiveRecord
     #     accepts_nested_attributes_for :avatar, :allow_destroy => true
     #   end
     #
-    # Now, when you add the <tt>_delete</tt> key to the attributes hash, with a
+    # Now, when you add the <tt>_destroy</tt> key to the attributes hash, with a
     # value that evaluates to +true+, you will destroy the associated model:
     #
-    #   member.avatar_attributes = { :id => '2', :_delete => '1' }
+    #   member.avatar_attributes = { :id => '2', :_destroy => '1' }
     #   member.avatar.marked_for_destruction? # => true
     #   member.save
     #   member.avatar #=> nil
@@ -89,14 +89,14 @@ module ActiveRecord
     # the attribute hash.
     #
     # For each hash that does _not_ have an <tt>id</tt> key a new record will
-    # be instantiated, unless the hash also contains a <tt>_delete</tt> key
+    # be instantiated, unless the hash also contains a <tt>_destroy</tt> key
     # that evaluates to +true+.
     #
     #   params = { :member => {
     #     :name => 'joe', :posts_attributes => [
     #       { :title => 'Kari, the awesome Ruby documentation browser!' },
     #       { :title => 'The egalitarian assumption of the modern citizen' },
-    #       { :title => '', :_delete => '1' } # this will be ignored
+    #       { :title => '', :_destroy => '1' } # this will be ignored
     #     ]
     #   }}
     #
@@ -144,7 +144,7 @@ module ActiveRecord
     # By default the associated records are protected from being destroyed. If
     # you want to destroy any of the associated records through the attributes
     # hash, you have to enable it first using the <tt>:allow_destroy</tt>
-    # option. This will allow you to also use the <tt>_delete</tt> key to
+    # option. This will allow you to also use the <tt>_destroy</tt> key to
     # destroy existing records:
     #
     #   class Member < ActiveRecord::Base
@@ -153,7 +153,7 @@ module ActiveRecord
     #   end
     #
     #   params = { :member => {
-    #     :posts_attributes => [{ :id => '2', :_delete => '1' }]
+    #     :posts_attributes => [{ :id => '2', :_destroy => '1' }]
     #   }}
     #
     #   member.attributes = params['member']
@@ -176,14 +176,14 @@ module ActiveRecord
       # Supported options:
       # [:allow_destroy]
       #   If true, destroys any members from the attributes hash with a
-      #   <tt>_delete</tt> key and a value that evaluates to +true+
+      #   <tt>_destroy</tt> key and a value that evaluates to +true+
       #   (eg. 1, '1', true, or 'true'). This option is off by default.
       # [:reject_if]
       #   Allows you to specify a Proc that checks whether a record should be
       #   built for a certain attribute hash. The hash is passed to the Proc
       #   and the Proc should return either +true+ or +false+. When no Proc
       #   is specified a record will be built for all attribute hashes that
-      #   do not have a <tt>_delete</tt> that evaluates to true.
+      #   do not have a <tt>_destroy</tt> value that evaluates to true.
       #   Passing <tt>:all_blank</tt> instead of a Proc will create a proc
       #   that will reject a record where all the attributes are blank.
       #
@@ -236,15 +236,25 @@ module ActiveRecord
     # destruction of this association.
     #
     # See ActionView::Helpers::FormHelper::fields_for for more info.
-    def _delete
+    def _destroy
       marked_for_destruction?
     end
 
+    # Deal with deprecated _delete.
+    #
+    def _delete #:nodoc:
+      ActiveSupport::Deprecation.warn "_delete is deprecated in nested attributes. Use _destroy instead."
+      _destroy
+    end
+
     private
 
     # Attribute hash keys that should not be assigned as normal attributes.
     # These hash keys are nested attributes implementation details.
-    UNASSIGNABLE_KEYS = %w{ id _delete }
+    #
+    # TODO Remove _delete from UNASSIGNABLE_KEYS when deprecation warning are
+    # removed.
+    UNASSIGNABLE_KEYS = %w( id _destroy _delete )
 
     # Assigns the given attributes to the association.
     #
@@ -253,7 +263,7 @@ module ActiveRecord
     # record will be built.
     #
     # If the given attributes include a matching <tt>:id</tt> attribute _and_ a
-    # <tt>:_delete</tt> key set to a truthy value, then the existing record
+    # <tt>:_destroy</tt> key set to a truthy value, then the existing record
     # will be marked for destruction.
     def assign_nested_attributes_for_one_to_one_association(association_name, attributes, allow_destroy)
       attributes = attributes.stringify_keys
@@ -277,7 +287,7 @@ module ActiveRecord
     # Hashes with an <tt>:id</tt> value matching an existing associated record
     # will update that record. Hashes without an <tt>:id</tt> value will build
     # a new record for the association. Hashes with a matching <tt>:id</tt>
-    # value and a <tt>:_delete</tt> key set to a truthy value will mark the
+    # value and a <tt>:_destroy</tt> key set to a truthy value will mark the
     # matched record for destruction.
     #
     # For example:
@@ -285,7 +295,7 @@ module ActiveRecord
     #   assign_nested_attributes_for_collection_association(:people, {
     #     '1' => { :id => '1', :name => 'Peter' },
     #     '2' => { :name => 'John' },
-    #     '3' => { :id => '2', :_delete => true }
+    #     '3' => { :id => '2', :_destroy => true }
     #   })
     #
     # Will update the name of the Person with ID 1, build a new associated
@@ -297,7 +307,7 @@ module ActiveRecord
     #   assign_nested_attributes_for_collection_association(:people, [
     #     { :id => '1', :name => 'Peter' },
     #     { :name => 'John' },
-    #     { :id => '2', :_delete => true }
+    #     { :id => '2', :_destroy => true }
     #   ])
     def assign_nested_attributes_for_collection_association(association_name, attributes_collection, allow_destroy)
       unless attributes_collection.is_a?(Hash) || attributes_collection.is_a?(Array)
@@ -322,25 +332,26 @@ module ActiveRecord
     end
 
     # Updates a record with the +attributes+ or marks it for destruction if
-    # +allow_destroy+ is +true+ and has_delete_flag? returns +true+.
+    # +allow_destroy+ is +true+ and has_destroy_flag? returns +true+.
     def assign_to_or_mark_for_destruction(record, attributes, allow_destroy)
-      if has_delete_flag?(attributes) && allow_destroy
+      if has_destroy_flag?(attributes) && allow_destroy
         record.mark_for_destruction
       else
         record.attributes = attributes.except(*UNASSIGNABLE_KEYS)
       end
     end
 
-    # Determines if a hash contains a truthy _delete key.
+    # Determines if a hash contains a truthy _destroy key.
-    def has_delete_flag?(hash)
+    def has_destroy_flag?(hash)
-      ConnectionAdapters::Column.value_to_boolean hash['_delete']
+      ConnectionAdapters::Column.value_to_boolean(hash['_destroy']) ||
+      ConnectionAdapters::Column.value_to_boolean(hash['_delete']) # TODO Remove after deprecation.
     end
 
     # Determines if a new record should be build by checking for
-    # has_delete_flag? or if a <tt>:reject_if</tt> proc exists for this
+    # has_destroy_flag? or if a <tt>:reject_if</tt> proc exists for this
     # association and evaluates to +true+.
     def reject_new_record?(association_name, attributes)
-      has_delete_flag?(attributes) ||
+      has_destroy_flag?(attributes) ||
         self.class.reject_new_nested_attributes_procs[association_name].try(:call, attributes)
     end
   end

activerecord/test/cases/nested_attributes_test.rb

@@ -68,15 +68,21 @@ class TestNestedAttributesInGeneral < ActiveRecord::TestCase
     ship = pirate.create_ship(:name => 'Nights Dirty Lightning')
 
     assert_no_difference('Ship.count') do
-      pirate.update_attributes(:ship_attributes => { '_delete' => true })
+      pirate.update_attributes(:ship_attributes => { '_destroy' => true })
     end
   end
 
-  def test_a_model_should_respond_to_underscore_delete_and_return_if_it_is_marked_for_destruction
+  def test_a_model_should_respond_to_underscore_destroy_and_return_if_it_is_marked_for_destruction
     ship = Ship.create!(:name => 'Nights Dirty Lightning')
-    assert !ship._delete
+    assert !ship._destroy
     ship.mark_for_destruction
-    assert ship._delete
+    assert ship._destroy
+  end
+
+  def test_underscore_delete_is_deprecated
+    ActiveSupport::Deprecation.expects(:warn)
+    ship = Ship.create!(:name => 'Nights Dirty Lightning')
+    ship._delete
   end
 end
 
@@ -106,9 +112,9 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase
     assert_equal 'Davy Jones Gold Dagger', @pirate.ship.name
   end
 
-  def test_should_not_build_a_new_record_if_there_is_no_id_and_delete_is_truthy
+  def test_should_not_build_a_new_record_if_there_is_no_id_and_destroy_is_truthy
     @ship.destroy
-    @pirate.reload.ship_attributes = { :name => 'Davy Jones Gold Dagger', :_delete => '1' }
+    @pirate.reload.ship_attributes = { :name => 'Davy Jones Gold Dagger', :_destroy => '1' }
 
     assert_nil @pirate.ship
   end
@@ -128,8 +134,8 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase
     assert_equal 'Nights Dirty Lightning', @ship.name
   end
 
-  def test_should_not_replace_an_existing_record_if_there_is_no_id_and_delete_is_truthy
+  def test_should_not_replace_an_existing_record_if_there_is_no_id_and_destroy_is_truthy
-    @pirate.reload.ship_attributes = { :name => 'Davy Jones Gold Dagger', :_delete => '1' }
+    @pirate.reload.ship_attributes = { :name => 'Davy Jones Gold Dagger', :_destroy => '1' }
 
     assert_equal @ship, @pirate.ship
     assert_equal 'Nights Dirty Lightning', @pirate.ship.name
@@ -156,29 +162,29 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase
     assert_equal 'Davy Jones Gold Dagger', @pirate.ship.name
   end
 
-  def test_should_delete_an_existing_record_if_there_is_a_matching_id_and_delete_is_truthy
+  def test_should_destroy_an_existing_record_if_there_is_a_matching_id_and_destroy_is_truthy
     @pirate.ship.destroy
     [1, '1', true, 'true'].each do |truth|
       @pirate.reload.create_ship(:name => 'Mister Pablo')
       assert_difference('Ship.count', -1) do
-        @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_delete => truth })
+        @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_destroy => truth })
       end
     end
   end
 
-  def test_should_not_delete_an_existing_record_if_delete_is_not_truthy
+  def test_should_not_destroy_an_existing_record_if_destroy_is_not_truthy
     [nil, '0', 0, 'false', false].each do |not_truth|
       assert_no_difference('Ship.count') do
-        @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_delete => not_truth })
+        @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_destroy => not_truth })
       end
     end
   end
 
-  def test_should_not_delete_an_existing_record_if_allow_destroy_is_false
+  def test_should_not_destroy_an_existing_record_if_allow_destroy_is_false
     Pirate.accepts_nested_attributes_for :ship, :allow_destroy => false, :reject_if => proc { |attributes| attributes.empty? }
 
     assert_no_difference('Ship.count') do
-      @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_delete => '1' })
+      @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_destroy => '1' })
     end
 
     Pirate.accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? }
@@ -201,7 +207,7 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase
 
   def test_should_not_destroy_the_associated_model_until_the_parent_is_saved
     assert_no_difference('Ship.count') do
-      @pirate.attributes = { :ship_attributes => { :id => @ship.id, :_delete => '1' } }
+      @pirate.attributes = { :ship_attributes => { :id => @ship.id, :_destroy => '1' } }
     end
     assert_difference('Ship.count', -1) do
       @pirate.save
@@ -232,9 +238,9 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase
     assert_equal 'Arr', @ship.pirate.catchphrase
   end
 
-  def test_should_not_build_a_new_record_if_there_is_no_id_and_delete_is_truthy
+  def test_should_not_build_a_new_record_if_there_is_no_id_and_destroy_is_truthy
     @pirate.destroy
-    @ship.reload.pirate_attributes = { :catchphrase => 'Arr', :_delete => '1' }
+    @ship.reload.pirate_attributes = { :catchphrase => 'Arr', :_destroy => '1' }
 
     assert_nil @ship.pirate
   end
@@ -254,8 +260,8 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase
     assert_equal 'Aye', @pirate.catchphrase
   end
 
-  def test_should_not_replace_an_existing_record_if_there_is_no_id_and_delete_is_truthy
+  def test_should_not_replace_an_existing_record_if_there_is_no_id_and_destroy_is_truthy
-    @ship.reload.pirate_attributes = { :catchphrase => 'Arr', :_delete => '1' }
+    @ship.reload.pirate_attributes = { :catchphrase => 'Arr', :_destroy => '1' }
 
     assert_equal @pirate, @ship.pirate
     assert_equal 'Aye', @ship.pirate.catchphrase
@@ -282,29 +288,29 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase
     assert_equal 'Arr', @ship.pirate.catchphrase
   end
 
-  def test_should_delete_an_existing_record_if_there_is_a_matching_id_and_delete_is_truthy
+  def test_should_destroy_an_existing_record_if_there_is_a_matching_id_and_destroy_is_truthy
     @ship.pirate.destroy
     [1, '1', true, 'true'].each do |truth|
       @ship.reload.create_pirate(:catchphrase => 'Arr')
       assert_difference('Pirate.count', -1) do
-        @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_delete => truth })
+        @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_destroy => truth })
       end
     end
   end
 
-  def test_should_not_delete_an_existing_record_if_delete_is_not_truthy
+  def test_should_not_destroy_an_existing_record_if_destroy_is_not_truthy
     [nil, '0', 0, 'false', false].each do |not_truth|
       assert_no_difference('Pirate.count') do
-        @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_delete => not_truth })
+        @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_destroy => not_truth })
       end
     end
   end
 
-  def test_should_not_delete_an_existing_record_if_allow_destroy_is_false
+  def test_should_not_destroy_an_existing_record_if_allow_destroy_is_false
     Ship.accepts_nested_attributes_for :pirate, :allow_destroy => false, :reject_if => proc { |attributes| attributes.empty? }
 
     assert_no_difference('Pirate.count') do
-      @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_delete => '1' })
+      @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_destroy => '1' })
     end
 
     Ship.accepts_nested_attributes_for :pirate, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? }
@@ -320,7 +326,7 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase
 
   def test_should_not_destroy_the_associated_model_until_the_parent_is_saved
     assert_no_difference('Pirate.count') do
-      @ship.attributes = { :pirate_attributes => { :id => @ship.pirate.id, '_delete' => true } }
+      @ship.attributes = { :pirate_attributes => { :id => @ship.pirate.id, '_destroy' => true } }
     end
     assert_difference('Pirate.count', -1) { @ship.save }
   end
@@ -388,18 +394,18 @@ module NestedAttributesOnACollectionAssociationTests
     assert_equal 'Privateers Greed', @pirate.send(@association_name).last.name
   end
 
-  def test_should_not_assign_delete_key_to_a_record
+  def test_should_not_assign_destroy_key_to_a_record
     assert_nothing_raised ActiveRecord::UnknownAttributeError do
-      @pirate.send(association_setter, { 'foo' => { '_delete' => '0' }})
+      @pirate.send(association_setter, { 'foo' => { '_destroy' => '0' }})
     end
   end
 
-  def test_should_ignore_new_associated_records_with_truthy_delete_attribute
+  def test_should_ignore_new_associated_records_with_truthy_destroy_attribute
     @pirate.send(@association_name).destroy_all
     @pirate.reload.attributes = {
       association_getter => {
         'foo' => { :name => 'Grace OMalley' },
-        'bar' => { :name => 'Privateers Greed', '_delete' => '1' }
+        'bar' => { :name => 'Privateers Greed', '_destroy' => '1' }
       }
     }
 
@@ -451,7 +457,7 @@ module NestedAttributesOnACollectionAssociationTests
     ['1', 1, 'true', true].each do |true_variable|
       record = @pirate.reload.send(@association_name).create!(:name => 'Grace OMalley')
       @pirate.send(association_setter,
-        @alternate_params[association_getter].merge('baz' => { :id => record.id, '_delete' => true_variable })
+        @alternate_params[association_getter].merge('baz' => { :id => record.id, '_destroy' => true_variable })
       )
 
       assert_difference('@pirate.send(@association_name).count', -1) do
@@ -462,7 +468,7 @@ module NestedAttributesOnACollectionAssociationTests
 
   def test_should_not_destroy_the_associated_model_with_a_non_truthy_argument
     [nil, '', '0', 0, 'false', false].each do |false_variable|
-      @alternate_params[association_getter]['foo']['_delete'] = false_variable
+      @alternate_params[association_getter]['foo']['_destroy'] = false_variable
       assert_no_difference('@pirate.send(@association_name).count') do
         @pirate.update_attributes(@alternate_params)
       end
@@ -471,7 +477,7 @@ module NestedAttributesOnACollectionAssociationTests
 
   def test_should_not_destroy_the_associated_model_until_the_parent_is_saved
     assert_no_difference('@pirate.send(@association_name).count') do
-      @pirate.send(association_setter, @alternate_params[association_getter].merge('baz' => { :id => @child_1.id, '_delete' => true }))
+      @pirate.send(association_setter, @alternate_params[association_getter].merge('baz' => { :id => @child_1.id, '_destroy' => true }))
     end
     assert_difference('@pirate.send(@association_name).count', -1) { @pirate.save }
   end