Skip to content

Commit

Permalink
Rename nested attributes _delete to _destroy to reflect its actual be…
Browse files Browse the repository at this point in the history 
…havior and DSL (:allow_destroy). Deprecation warning added. [rails#2889 state:resolved]

Signed-off-by: Eloy Duran <eloy.de.enige@gmail.com>
  • Loading branch information
@josevalim @alloy
josevalim authored and alloy committed Jul 11, 2009
1 parent dbc56d6 commit 6d4637b
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 55 deletions.
55 changes: 33 additions & 22 deletions activerecord/lib/active_record/nested_attributes.rb
View file
Expand Up @@ -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
Expand All @@ -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
# ]
# }}
#
Expand Down Expand Up @@ -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
Expand All @@ -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']
Expand All @@ -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=
Expand Down Expand Up @@ -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.
#
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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
Expand Down
72 changes: 39 additions & 33 deletions activerecord/test/cases/nested_attributes_test.rb
View file
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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? }
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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? }
Expand All @@ -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
Expand Down Expand Up @@ -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' }
}
}

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 6d4637b

Please sign in to comment.