Permalink
Browse files

Rename nested attributes _delete to _destroy to reflect its actual be…

…havior and DSL (:allow_destroy). Deprecation warning added. [#2889 state:resolved]

Signed-off-by: Eloy Duran <eloy.de.enige@gmail.com>
  • Loading branch information...
1 parent c52a50e commit 11c338735c3f6e9562c4c82ccd8913dd8b7a6a80 @josevalim josevalim committed with alloy Jul 11, 2009
Showing with 72 additions and 55 deletions.
  1. +33 −22 activerecord/lib/active_record/nested_attributes.rb
  2. +39 −33 activerecord/test/cases/nested_attributes_test.rb
@@ -62,10 +62,10 @@ def self.included(base)
# 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
@@ -85,14 +85,14 @@ def self.included(base)
# 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
# ]
# }}
#
@@ -140,7 +140,7 @@ def self.included(base)
# 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
@@ -149,7 +149,7 @@ def self.included(base)
# end
#
# params = { :member => {
- # :posts_attributes => [{ :id => '2', :_delete => '1' }]
+ # :posts_attributes => [{ :id => '2', :_destroy => '1' }]
# }}
#
# member.attributes = params['member']
@@ -172,14 +172,14 @@ module ClassMethods
# 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.
+ # is specified, a record will be built for all attribute hashes that
+ # do not have a <tt>_destroy</tt> value that evaluates to true.
#
# Examples:
# # creates avatar_attributes=
@@ -223,15 +223,25 @@ def #{association_name}_attributes=(attributes)
# 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.
#
@@ -240,7 +250,7 @@ def _delete
# 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
@@ -264,15 +274,15 @@ def assign_nested_attributes_for_one_to_one_association(association_name, attrib
# 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:
#
# 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
@@ -284,7 +294,7 @@ def assign_nested_attributes_for_one_to_one_association(association_name, attrib
# 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)
@@ -309,25 +319,26 @@ def assign_nested_attributes_for_collection_association(association_name, attrib
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.
- def has_delete_flag?(hash)
- ConnectionAdapters::Column.value_to_boolean hash['_delete']
+ # Determines if a hash contains a truthy _destroy key.
+ def has_destroy_flag?(hash)
+ 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
@@ -49,15 +49,21 @@ def test_should_disable_allow_destroy_by_default
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
@@ -87,9 +93,9 @@ def test_should_build_a_new_record_if_there_is_no_id
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
@@ -109,8 +115,8 @@ def test_should_replace_an_existing_record_if_there_is_no_id
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
- @pirate.reload.ship_attributes = { :name => 'Davy Jones Gold Dagger', :_delete => '1' }
+ 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', :_destroy => '1' }
assert_equal @ship, @pirate.ship
assert_equal 'Nights Dirty Lightning', @pirate.ship.name
@@ -137,29 +143,29 @@ def test_should_modify_an_existing_record_if_there_is_a_matching_composite_id
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? }
@@ -182,7 +188,7 @@ def test_should_work_with_update_attributes_as_well
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
@@ -213,9 +219,9 @@ def test_should_build_a_new_record_if_there_is_no_id
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
@@ -235,8 +241,8 @@ def test_should_replace_an_existing_record_if_there_is_no_id
assert_equal 'Aye', @pirate.catchphrase
end
- def test_should_not_replace_an_existing_record_if_there_is_no_id_and_delete_is_truthy
- @ship.reload.pirate_attributes = { :catchphrase => 'Arr', :_delete => '1' }
+ def test_should_not_replace_an_existing_record_if_there_is_no_id_and_destroy_is_truthy
+ @ship.reload.pirate_attributes = { :catchphrase => 'Arr', :_destroy => '1' }
assert_equal @pirate, @ship.pirate
assert_equal 'Aye', @ship.pirate.catchphrase
@@ -263,29 +269,29 @@ def test_should_modify_an_existing_record_if_there_is_a_matching_composite_id
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? }
@@ -301,7 +307,7 @@ def test_should_work_with_update_attributes_as_well
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
@@ -369,18 +375,18 @@ def test_should_automatically_build_new_associated_models_for_each_entry_in_a_ha
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' }
}
}
@@ -432,7 +438,7 @@ def test_should_be_possible_to_destroy_a_record
['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
@@ -443,7 +449,7 @@ def test_should_be_possible_to_destroy_a_record
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
@@ -452,7 +458,7 @@ def test_should_not_destroy_the_associated_model_with_a_non_truthy_argument
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

0 comments on commit 11c3387

Please sign in to comment.