Changed API of NestedAttributes to take an array, or hash with index keys, of hashes that have the id on the inside of the attributes hash and updated the FormBuilder to produce such hashes. Also fixed NestedAttributes with composite ids.

Signed-off-by: Michael Koziarski <michael@koziarski.com>
Signed-off-by: Eloy Duran <eloy.de.enige@gmail.com>
[#1892 state:committed]

1 files changed, 212 insertions, 76 deletions

diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb
index 2e531a284e..cd6277c24b 100644
--- a/activerecord/test/cases/nested_attributes_test.rb
+++ b/activerecord/test/cases/nested_attributes_test.rb
@@ -23,7 +23,7 @@ class TestNestedAttributesInGeneral < ActiveRecord::TestCase
   include AssertRaiseWithMessage
 
   def teardown
-    Pirate.accepts_nested_attributes_for :ship, :allow_destroy => true
+    Pirate.accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? }
   end
 
   def test_base_should_have_an_empty_reject_new_nested_attributes_procs
@@ -71,7 +71,7 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase
     assert_respond_to @pirate, :ship_attributes=
   end
 
-  def test_should_automatically_instantiate_an_associated_model_if_there_is_none
+  def test_should_build_a_new_record_if_there_is_no_id
     @ship.destroy
     @pirate.reload.ship_attributes = { :name => 'Davy Jones Gold Dagger' }
 
@@ -79,49 +79,106 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase
     assert_equal 'Davy Jones Gold Dagger', @pirate.ship.name
   end
 
-  def test_should_take_a_hash_and_assign_the_attributes_to_the_existing_associated_model
-    @pirate.ship_attributes = { :name => 'Davy Jones Gold Dagger' }
-    assert !@pirate.ship.new_record?
+  def test_should_not_build_a_new_record_if_there_is_no_id_and_delete_is_truthy
+    @ship.destroy
+    @pirate.reload.ship_attributes = { :name => 'Davy Jones Gold Dagger', :_delete => '1' }
+
+    assert_nil @pirate.ship
+  end
+
+  def test_should_not_build_a_new_record_if_a_reject_if_proc_returns_false
+    @ship.destroy
+    @pirate.reload.ship_attributes = {}
+
+    assert_nil @pirate.ship
+  end
+
+  def test_should_replace_an_existing_record_if_there_is_no_id
+    @pirate.reload.ship_attributes = { :name => 'Davy Jones Gold Dagger' }
+
+    assert @pirate.ship.new_record?
     assert_equal 'Davy Jones Gold Dagger', @pirate.ship.name
+    assert_equal 'Nights Dirty Lightning', @ship.name
   end
 
-  def test_should_also_work_with_a_HashWithIndifferentAccess
-    @pirate.ship_attributes = HashWithIndifferentAccess.new(:name => 'Davy Jones Gold Dagger')
-    assert !@pirate.ship.new_record?
+  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' }
+
+    assert_equal @ship, @pirate.ship
+    assert_equal 'Nights Dirty Lightning', @pirate.ship.name
+  end
+
+  def test_should_modify_an_existing_record_if_there_is_a_matching_id
+    @pirate.reload.ship_attributes = { :id => @ship.id, :name => 'Davy Jones Gold Dagger' }
+
+    assert_equal @ship, @pirate.ship
     assert_equal 'Davy Jones Gold Dagger', @pirate.ship.name
   end
 
-  def test_should_work_with_update_attributes_as_well
-    @pirate.update_attributes({ :catchphrase => 'Arr', :ship_attributes => { :name => 'Mister Pablo' } })
-    @pirate.reload
+  def test_should_take_a_hash_with_string_keys_and_update_the_associated_model
+    @pirate.reload.ship_attributes = { 'id' => @ship.id, 'name' => 'Davy Jones Gold Dagger' }
 
-    assert_equal 'Arr', @pirate.catchphrase
-    assert_equal 'Mister Pablo', @pirate.ship.name
+    assert_equal @ship, @pirate.ship
+    assert_equal 'Davy Jones Gold Dagger', @pirate.ship.name
   end
 
-  def test_should_be_possible_to_destroy_the_associated_model
+  def test_should_modify_an_existing_record_if_there_is_a_matching_composite_id
+    @ship.stubs(:id).returns('ABC1X')
+    @pirate.ship_attributes = { :id => @ship.id, :name => 'Davy Jones Gold Dagger' }
+
+    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
     @pirate.ship.destroy
-    ['1', 1, 'true', true].each do |true_variable|
+    [1, '1', true, 'true'].each do |truth|
       @pirate.reload.create_ship(:name => 'Mister Pablo')
       assert_difference('Ship.count', -1) do
-        @pirate.update_attributes(:ship_attributes => { '_delete' => true_variable })
+        @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_delete => truth })
       end
     end
   end
 
-  def test_should_not_destroy_the_associated_model_with_a_non_truthy_argument
-    [nil, '0', 0, 'false', false].each do |false_variable|
+  def test_should_not_delete_an_existing_record_if_delete_is_not_truthy
+    [nil, '0', 0, 'false', false].each do |not_truth|
       assert_no_difference('Ship.count') do
-        @pirate.update_attributes(:ship_attributes => { '_delete' => false_variable })
+        @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_delete => not_truth })
       end
     end
   end
 
+  def test_should_not_delete_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' })
+    end
+
+    Pirate.accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? }
+  end
+
+  def test_should_also_work_with_a_HashWithIndifferentAccess
+    @pirate.ship_attributes = HashWithIndifferentAccess.new(:id => @ship.id, :name => 'Davy Jones Gold Dagger')
+
+    assert !@pirate.ship.new_record?
+    assert_equal 'Davy Jones Gold Dagger', @pirate.ship.name
+  end
+
+  def test_should_work_with_update_attributes_as_well
+    @pirate.update_attributes({ :catchphrase => 'Arr', :ship_attributes => { :id => @ship.id, :name => 'Mister Pablo' } })
+    @pirate.reload
+
+    assert_equal 'Arr', @pirate.catchphrase
+    assert_equal 'Mister Pablo', @pirate.ship.name
+  end
+
   def test_should_not_destroy_the_associated_model_until_the_parent_is_saved
     assert_no_difference('Ship.count') do
-      @pirate.attributes = { :ship_attributes => { '_delete' => true } }
+      @pirate.attributes = { :ship_attributes => { :id => @ship.id, :_delete => '1' } }
+    end
+    assert_difference('Ship.count', -1) do
+      @pirate.save
     end
-    assert_difference('Ship.count', -1) { @pirate.save }
   end
 
   def test_should_automatically_enable_autosave_on_the_association
@@ -131,15 +188,16 @@ end
 
 class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase
   def setup
-    @ship = Ship.create!(:name => 'Nights Dirty Lightning')
-    @pirate = @ship.create_pirate(:catchphrase => "Don' botharrr talkin' like one, savvy?")
+    @ship = Ship.new(:name => 'Nights Dirty Lightning')
+    @pirate = @ship.build_pirate(:catchphrase => 'Aye')
+    @ship.save!
   end
 
   def test_should_define_an_attribute_writer_method_for_the_association
     assert_respond_to @ship, :pirate_attributes=
   end
 
-  def test_should_automatically_instantiate_an_associated_model_if_there_is_none
+  def test_should_build_a_new_record_if_there_is_no_id
     @pirate.destroy
     @ship.reload.pirate_attributes = { :catchphrase => 'Arr' }
 
@@ -147,47 +205,95 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase
     assert_equal 'Arr', @ship.pirate.catchphrase
   end
 
-  def test_should_take_a_hash_and_assign_the_attributes_to_the_existing_associated_model
-    @ship.pirate_attributes = { :catchphrase => 'Arr' }
-    assert !@ship.pirate.new_record?
+  def test_should_not_build_a_new_record_if_there_is_no_id_and_delete_is_truthy
+    @pirate.destroy
+    @ship.reload.pirate_attributes = { :catchphrase => 'Arr', :_delete => '1' }
+
+    assert_nil @ship.pirate
+  end
+
+  def test_should_not_build_a_new_record_if_a_reject_if_proc_returns_false
+    @pirate.destroy
+    @ship.reload.pirate_attributes = {}
+
+    assert_nil @ship.pirate
+  end
+
+  def test_should_replace_an_existing_record_if_there_is_no_id
+    @ship.reload.pirate_attributes = { :catchphrase => 'Arr' }
+
+    assert @ship.pirate.new_record?
     assert_equal 'Arr', @ship.pirate.catchphrase
+    assert_equal 'Aye', @pirate.catchphrase
   end
 
-  def test_should_also_work_with_a_HashWithIndifferentAccess
-    @ship.pirate_attributes = HashWithIndifferentAccess.new(:catchphrase => 'Arr')
-    assert !@ship.pirate.new_record?
+  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' }
+
+    assert_equal @pirate, @ship.pirate
+    assert_equal 'Aye', @ship.pirate.catchphrase
+  end
+
+  def test_should_modify_an_existing_record_if_there_is_a_matching_id
+    @ship.reload.pirate_attributes = { :id => @pirate.id, :catchphrase => 'Arr' }
+
+    assert_equal @pirate, @ship.pirate
     assert_equal 'Arr', @ship.pirate.catchphrase
   end
 
-  def test_should_work_with_update_attributes_as_well
-    @ship.update_attributes({ :name => 'Mister Pablo', :pirate_attributes => { :catchphrase => 'Arr' } })
-    @ship.reload
+  def test_should_take_a_hash_with_string_keys_and_update_the_associated_model
+    @ship.reload.pirate_attributes = { 'id' => @pirate.id, 'catchphrase' => 'Arr' }
 
-    assert_equal 'Mister Pablo', @ship.name
+    assert_equal @pirate, @ship.pirate
     assert_equal 'Arr', @ship.pirate.catchphrase
   end
 
-  def test_should_be_possible_to_destroy_the_associated_model
+  def test_should_modify_an_existing_record_if_there_is_a_matching_composite_id
+    @pirate.stubs(:id).returns('ABC1X')
+    @ship.pirate_attributes = { :id => @pirate.id, :catchphrase => 'Arr' }
+
+    assert_equal 'Arr', @ship.pirate.catchphrase
+  end
+
+  def test_should_delete_an_existing_record_if_there_is_a_matching_id_and_delete_is_truthy
     @ship.pirate.destroy
-    ['1', 1, 'true', true].each do |true_variable|
+    [1, '1', true, 'true'].each do |truth|
       @ship.reload.create_pirate(:catchphrase => 'Arr')
       assert_difference('Pirate.count', -1) do
-        @ship.update_attributes(:pirate_attributes => { '_delete' => true_variable })
+        @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_delete => truth })
       end
     end
   end
 
-  def test_should_not_destroy_the_associated_model_with_a_non_truthy_argument
-    [nil, '', '0', 0, 'false', false].each do |false_variable|
+  def test_should_not_delete_an_existing_record_if_delete_is_not_truthy
+    [nil, '0', 0, 'false', false].each do |not_truth|
       assert_no_difference('Pirate.count') do
-        @ship.update_attributes(:pirate_attributes => { '_delete' => false_variable })
+        @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_delete => not_truth })
       end
     end
   end
 
+  def test_should_not_delete_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' })
+    end
+
+    Ship.accepts_nested_attributes_for :pirate, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? }
+  end
+
+  def test_should_work_with_update_attributes_as_well
+    @ship.update_attributes({ :name => 'Mister Pablo', :pirate_attributes => { :catchphrase => 'Arr' } })
+    @ship.reload
+
+    assert_equal 'Mister Pablo', @ship.name
+    assert_equal 'Arr', @ship.pirate.catchphrase
+  end
+
   def test_should_not_destroy_the_associated_model_until_the_parent_is_saved
     assert_no_difference('Pirate.count') do
-      @ship.attributes = { :pirate_attributes => { '_delete' => true } }
+      @ship.attributes = { :pirate_attributes => { :id => @ship.pirate.id, '_delete' => true } }
     end
     assert_difference('Pirate.count', -1) { @ship.save }
   end
@@ -210,21 +316,43 @@ module NestedAttributesOnACollectionAssociationTests
     assert_equal ['Grace OMalley', 'Privateers Greed'], [@child_1.reload.name, @child_2.reload.name]
   end
 
+  def test_should_take_an_array_and_assign_the_attributes_to_the_associated_models
+    @pirate.send(association_setter, @alternate_params[association_getter].values)
+    @pirate.save
+    assert_equal ['Grace OMalley', 'Privateers Greed'], [@child_1.reload.name, @child_2.reload.name]
+  end
+
   def test_should_also_work_with_a_HashWithIndifferentAccess
-    @pirate.send(association_setter, HashWithIndifferentAccess.new(@child_1.id => HashWithIndifferentAccess.new(:name => 'Grace OMalley')))
+    @pirate.send(association_setter, HashWithIndifferentAccess.new('foo' => HashWithIndifferentAccess.new(:id => @child_1.id, :name => 'Grace OMalley')))
     @pirate.save
     assert_equal 'Grace OMalley', @child_1.reload.name
   end
 
-  def test_should_take_a_hash_with_integer_keys_and_assign_the_attributes_to_the_associated_models
+  def test_should_take_a_hash_and_assign_the_attributes_to_the_associated_models
     @pirate.attributes = @alternate_params
     assert_equal 'Grace OMalley', @pirate.send(@association_name).first.name
     assert_equal 'Privateers Greed', @pirate.send(@association_name).last.name
   end
 
-  def test_should_automatically_build_new_associated_models_for_each_entry_in_a_hash_where_the_id_starts_with_the_string_new_
+  def test_should_take_a_hash_with_composite_id_keys_and_assign_the_attributes_to_the_associated_models
+    @child_1.stubs(:id).returns('ABC1X')
+    @child_2.stubs(:id).returns('ABC2X')
+
+    @pirate.attributes = {
+      association_getter => [
+        { :id => @child_1.id, :name => 'Grace OMalley' },
+        { :id => @child_2.id, :name => 'Privateers Greed' }
+      ]
+    }
+
+    assert_equal ['Grace OMalley', 'Privateers Greed'], [@child_1.name, @child_2.name]
+  end
+
+  def test_should_automatically_build_new_associated_models_for_each_entry_in_a_hash_where_the_id_is_missing
     @pirate.send(@association_name).destroy_all
-    @pirate.reload.attributes = { association_getter => { 'new_1' => { :name => 'Grace OMalley' }, 'new_2' => { :name => 'Privateers Greed' }}}
+    @pirate.reload.attributes = {
+      association_getter => { 'foo' => { :name => 'Grace OMalley' }, 'bar' => { :name => 'Privateers Greed' }}
+    }
 
     assert @pirate.send(@association_name).first.new_record?
     assert_equal 'Grace OMalley', @pirate.send(@association_name).first.name
@@ -233,24 +361,36 @@ module NestedAttributesOnACollectionAssociationTests
     assert_equal 'Privateers Greed', @pirate.send(@association_name).last.name
   end
 
-  def test_should_remove_delete_key_from_arguments_hash_of_new_records
+  def test_should_not_assign_delete_key_to_a_record
     assert_nothing_raised ActiveRecord::UnknownAttributeError do
-      @pirate.send(association_setter, { 'new_1' => { '_delete' => '0' }})
+      @pirate.send(association_setter, { 'foo' => { '_delete' => '0' }})
     end
   end
 
   def test_should_ignore_new_associated_records_with_truthy_delete_attribute
     @pirate.send(@association_name).destroy_all
-    @pirate.reload.attributes = { association_getter => { 'new_1' => { :name => 'Grace OMalley' }, 'new_2' => { :name => 'Privateers Greed', '_delete' => '1' }}}
+    @pirate.reload.attributes = {
+      association_getter => {
+        'foo' => { :name => 'Grace OMalley' },
+        'bar' => { :name => 'Privateers Greed', '_delete' => '1' }
+      }
+    }
 
     assert_equal 1, @pirate.send(@association_name).length
     assert_equal 'Grace OMalley', @pirate.send(@association_name).first.name
   end
 
+  def test_should_ignore_new_associated_records_if_a_reject_if_proc_returns_false
+    @alternate_params[association_getter]['baz'] = {}
+    assert_no_difference("@pirate.send(@association_name).length") do
+      @pirate.attributes = @alternate_params
+    end
+  end
+
   def test_should_sort_the_hash_by_the_keys_before_building_new_associated_models
     attributes = ActiveSupport::OrderedHash.new
-    attributes['new_123726353'] = { :name => 'Grace OMalley' }
-    attributes['new_2'] = { :name => 'Privateers Greed' } # 2 is lower then 123726353
+    attributes['123726353'] = { :name => 'Grace OMalley' }
+    attributes['2'] = { :name => 'Privateers Greed' } # 2 is lower then 123726353
     @pirate.send(association_setter, attributes)
 
     assert_equal ['Posideons Killer', 'Killer bandita Dionne', 'Privateers Greed', 'Grace OMalley'].to_set, @pirate.send(@association_name).map(&:name).to_set
@@ -260,28 +400,20 @@ module NestedAttributesOnACollectionAssociationTests
     assert_nothing_raised(ArgumentError) { @pirate.send(association_setter, {}) }
     assert_nothing_raised(ArgumentError) { @pirate.send(association_setter, ActiveSupport::OrderedHash.new) }
 
-    assert_raise_with_message ArgumentError, 'Hash expected, got String ("foo")' do
+    assert_raise_with_message ArgumentError, 'Hash or Array expected, got String ("foo")' do
       @pirate.send(association_setter, "foo")
     end
-    assert_raise_with_message ArgumentError, 'Hash expected, got Array ([:foo, :bar])' do
-      @pirate.send(association_setter, [:foo, :bar])
-    end
   end
 
   def test_should_work_with_update_attributes_as_well
-    @pirate.update_attributes({ :catchphrase => 'Arr', association_getter => { @child_1.id => { :name => 'Grace OMalley' }}})
-    assert_equal 'Grace OMalley', @child_1.reload.name
-  end
+    @pirate.update_attributes(:catchphrase => 'Arr',
+      association_getter => { 'foo' => { :id => @child_1.id, :name => 'Grace OMalley' }})
 
-  def test_should_automatically_reject_any_new_record_if_a_reject_if_proc_exists_and_returns_false
-    @alternate_params[association_getter]["new_12345"] = {}
-    assert_no_difference("@pirate.send(@association_name).length") do
-      @pirate.attributes = @alternate_params
-    end
+    assert_equal 'Grace OMalley', @child_1.reload.name
   end
 
-  def test_should_update_existing_records_and_add_new_ones_that_have_an_id_that_start_with_the_string_new_
-    @alternate_params[association_getter]['new_12345'] = { :name => 'Buccaneers Servant' }
+  def test_should_update_existing_records_and_add_new_ones_that_have_no_id
+    @alternate_params[association_getter]['baz'] = { :name => 'Buccaneers Servant' }
     assert_difference('@pirate.send(@association_name).count', +1) do
       @pirate.update_attributes @alternate_params
     end
@@ -292,7 +424,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(record.id => { '_delete' => true_variable })
+        @alternate_params[association_getter].merge('baz' => { :id => record.id, '_delete' => true_variable })
       )
 
       assert_difference('@pirate.send(@association_name).count', -1) do
@@ -303,7 +435,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][@child_1.id]['_delete'] = false_variable
+      @alternate_params[association_getter]['foo']['_delete'] = false_variable
       assert_no_difference('@pirate.send(@association_name).count') do
         @pirate.update_attributes(@alternate_params)
       end
@@ -312,7 +444,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(@child_1.id => { '_delete' => true }))
+      @pirate.send(association_setter, @alternate_params[association_getter].merge('baz' => { :id => @child_1.id, '_delete' => true }))
     end
     assert_difference('@pirate.send(@association_name).count', -1) { @pirate.save }
   end
@@ -338,13 +470,15 @@ class TestNestedAttributesOnAHasManyAssociation < ActiveRecord::TestCase
     @association_name = :birds
 
     @pirate = Pirate.create!(:catchphrase => "Don' botharrr talkin' like one, savvy?")
-    @child_1 = @pirate.birds.create!(:name => 'Posideons Killer')
-    @child_2 = @pirate.birds.create!(:name => 'Killer bandita Dionne')
+    @pirate.birds.create!(:name => 'Posideons Killer')
+    @pirate.birds.create!(:name => 'Killer bandita Dionne')
+
+    @child_1, @child_2 = @pirate.birds
 
     @alternate_params = {
       :birds_attributes => {
-        @child_1.id => { :name => 'Grace OMalley' },
-        @child_2.id => { :name => 'Privateers Greed' }
+        'foo' => { :id => @child_1.id, :name => 'Grace OMalley' },
+        'bar' => { :id => @child_2.id, :name => 'Privateers Greed' }
       }
     }
   end
@@ -358,16 +492,18 @@ class TestNestedAttributesOnAHasAndBelongsToManyAssociation < ActiveRecord::Test
     @association_name = :parrots
 
     @pirate = Pirate.create!(:catchphrase => "Don' botharrr talkin' like one, savvy?")
-    @child_1 = @pirate.parrots.create!(:name => 'Posideons Killer')
-    @child_2 = @pirate.parrots.create!(:name => 'Killer bandita Dionne')
+    @pirate.parrots.create!(:name => 'Posideons Killer')
+    @pirate.parrots.create!(:name => 'Killer bandita Dionne')
+
+    @child_1, @child_2 = @pirate.parrots
 
     @alternate_params = {
       :parrots_attributes => {
-        @child_1.id => { :name => 'Grace OMalley' },
-        @child_2.id => { :name => 'Privateers Greed' }
+        'foo' => { :id => @child_1.id, :name => 'Grace OMalley' },
+        'bar' => { :id => @child_2.id, :name => 'Privateers Greed' }
       }
     }
   end
 
   include NestedAttributesOnACollectionAssociationTests
-end
\ No newline at end of file
+end