From 23bfb5cfb2d7192c8c12a540d938116a13c469ef Mon Sep 17 00:00:00 2001 From: Jeff Bigler Date: Sun, 18 Apr 2010 14:04:12 -0700 Subject: [PATCH] Fix bug preventing nested children from saving Nested children were not being saved unless the parent was also saved. Fix recursively checks child objects for nested children which should be auto-saved for children with changes. [#4242 state:resolved] --- .../lib/active_record/autosave_association.rb | 33 ++++++++++++++- activerecord/test/cases/nested_attributes_test.rb | 43 ++++++++++++++++---- activerecord/test/models/parrot.rb | 3 + activerecord/test/models/phrase.rb | 3 + activerecord/test/schema/schema.rb | 5 ++ 5 files changed, 77 insertions(+), 10 deletions(-) create mode 100644 activerecord/test/models/phrase.rb diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index 325a8aa..4ed801e 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -225,12 +225,41 @@ module ActiveRecord if new_record association elsif autosave - association.target.find_all { |record| record.new_record? || record.changed? || record.marked_for_destruction? } + association.target.find_all { |record| record.new_record? || record.changed? || record.marked_for_destruction? || nested_records_have_changes?(record) } else association.target.find_all { |record| record.new_record? } end end + # Returns true if any nested records in an association have changes to be saved. + # Will recursively examine autosave associations to find deeply nested children. + def nested_records_have_changes?(record) + record.class.reflect_on_all_autosave_associations.each do |reflection| + ivar = "@#{reflection.name}" + if record.instance_variable_defined?(ivar) + association = record.instance_variable_get(ivar) + if association.respond_to?(:loaded?) + if association.respond_to?(:each) + association.each do |nested_record| + if nested_record.changed? then + return true + elsif nested_record.class.reflect_on_all_autosave_associations + nested_records_have_changes?(nested_record) + end + end + else + if association.changed? then + return true + elsif association.class.reflect_on_all_autosave_associations + nested_records_have_changes?(association) + end + end + end + end + end + return false + end + # Validate the association if :validate or :autosave is # turned on for the association specified by +reflection+. def validate_single_association(reflection) @@ -371,4 +400,4 @@ module ActiveRecord end end end -end \ No newline at end of file +end diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb index eae8ae7..eaf8cac 100644 --- a/activerecord/test/cases/nested_attributes_test.rb +++ b/activerecord/test/cases/nested_attributes_test.rb @@ -3,6 +3,7 @@ require "models/pirate" require "models/ship" require "models/bird" require "models/parrot" +require "models/phrase" require "models/treasure" require "models/man" require "models/interest" @@ -432,13 +433,21 @@ module NestedAttributesOnACollectionAssociationTests def test_should_take_a_hash_with_string_keys_and_assign_the_attributes_to_the_associated_models @alternate_params[association_getter].stringify_keys! @pirate.update_attributes @alternate_params - assert_equal ['Grace OMalley', 'Privateers Greed'], [@child_1.reload.name, @child_2.reload.name] + assert_equal ['Grace OMalley', 'Privateers Greed', 'Davy Jones'], [@child_1.reload.name, @child_2.reload.name, @child_3.reload.name] + if @association_name == :parrots then + assert_equal ['Walk the plank.', 'A pirates life for me.'], [@grandchild_1.reload.phrase, @grandchild_2.reload.phrase] + assert_equal ['Walk the plank.', 'A pirates life for me.'], [@grandchild_3.reload.phrase, @grandchild_4.reload.phrase] + end 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] + assert_equal ['Grace OMalley', 'Privateers Greed', 'Davy Jones'], [@child_1.reload.name, @child_2.reload.name, @child_3.reload.name] + if @association_name == :parrots then + assert_equal ['Walk the plank.', 'A pirates life for me.'], [@grandchild_1.reload.phrase, @grandchild_2.reload.phrase] + assert_equal ['Walk the plank.', 'A pirates life for me.'], [@grandchild_3.reload.phrase, @grandchild_4.reload.phrase] + end end def test_should_also_work_with_a_HashWithIndifferentAccess @@ -450,7 +459,10 @@ module NestedAttributesOnACollectionAssociationTests 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 + assert_equal 'Davy Jones', @pirate.send(@association_name).last.name + if @association_name == :parrots then + assert_equal 'Walk the plank.', @pirate.send(@association_name).last.phrases.first.phrase + end end def test_should_not_load_association_when_updating_existing_records @@ -528,7 +540,7 @@ module NestedAttributesOnACollectionAssociationTests 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 + assert_equal ['Posideons Killer', 'Killer bandita Dionne', 'Davy Jones', 'Privateers Greed', 'Grace OMalley'].to_set, @pirate.send(@association_name).map(&:name).to_set end def test_should_raise_an_argument_error_if_something_else_than_a_hash_is_passed @@ -552,7 +564,7 @@ module NestedAttributesOnACollectionAssociationTests assert_difference('@pirate.send(@association_name).count', +1) do @pirate.update_attributes @alternate_params end - assert_equal ['Grace OMalley', 'Privateers Greed', 'Buccaneers Servant'].to_set, @pirate.reload.send(@association_name).map(&:name).to_set + assert_equal ['Grace OMalley', 'Privateers Greed', 'Davy Jones', 'Buccaneers Servant'].to_set, @pirate.reload.send(@association_name).map(&:name).to_set end def test_should_be_possible_to_destroy_a_record @@ -642,8 +654,9 @@ class TestNestedAttributesOnAHasManyAssociation < ActiveRecord::TestCase @pirate = Pirate.create!(:catchphrase => "Don' botharrr talkin' like one, savvy?") @pirate.birds.create!(:name => 'Posideons Killer') @pirate.birds.create!(:name => 'Killer bandita Dionne') + @pirate.birds.create!(:name => 'Davy Jones') - @child_1, @child_2 = @pirate.birds + @child_1, @child_2, @child_3 = @pirate.birds @alternate_params = { :birds_attributes => { @@ -664,13 +677,27 @@ class TestNestedAttributesOnAHasAndBelongsToManyAssociation < ActiveRecord::Test @pirate = Pirate.create!(:catchphrase => "Don' botharrr talkin' like one, savvy?") @pirate.parrots.create!(:name => 'Posideons Killer') @pirate.parrots.create!(:name => 'Killer bandita Dionne') + @pirate.parrots.create!(:name => 'Davy Jones') - @child_1, @child_2 = @pirate.parrots + @child_1, @child_2, @child_3 = @pirate.parrots + @child_3.phrases.create!(:phrase => 'Polly wants a cracker.') + @child_3.phrases.create!(:phrase => 'Get off my ship.') + @child_2.phrases.create!(:phrase => 'Polly wants a cracker.') + @child_2.phrases.create!(:phrase => 'Get off my ship.') + @grandchild_1, @grandchild_2 = @child_3.phrases + @grandchild_3, @grandchild_4 = @child_2.phrases @alternate_params = { :parrots_attributes => { 'foo' => { :id => @child_1.id, :name => 'Grace OMalley' }, - 'bar' => { :id => @child_2.id, :name => 'Privateers Greed' } + 'bar' => { :id => @child_2.id, :name => 'Privateers Greed', + :phrases_attributes => { + 'phrase1' => { :id => @grandchild_3.id, :phrase => 'Walk the plank.' }, + 'phrase2' => { :id => @grandchild_4.id, :phrase => 'A pirates life for me.' } } }, + 'bit' => { :id => @child_3.id, + :phrases_attributes => { + 'phrase1' => { :id => @grandchild_1.id, :phrase => 'Walk the plank.' }, + 'phrase2' => { :id => @grandchild_2.id, :phrase => 'A pirates life for me.' } } } } } end diff --git a/activerecord/test/models/parrot.rb b/activerecord/test/models/parrot.rb index 737ef91..02d8677 100644 --- a/activerecord/test/models/parrot.rb +++ b/activerecord/test/models/parrot.rb @@ -3,10 +3,13 @@ class Parrot < ActiveRecord::Base has_and_belongs_to_many :pirates has_and_belongs_to_many :treasures has_many :loots, :as => :looter + has_many :phrases alias_attribute :title, :name validates_presence_of :name + accepts_nested_attributes_for :phrases + attr_accessor :cancel_save_from_callback before_save :cancel_save_callback_method, :if => :cancel_save_from_callback def cancel_save_callback_method diff --git a/activerecord/test/models/phrase.rb b/activerecord/test/models/phrase.rb new file mode 100644 index 0000000..445b6d4 --- /dev/null +++ b/activerecord/test/models/phrase.rb @@ -0,0 +1,3 @@ +class Phrase < ActiveRecord::Base + belongs_to :parrot +end diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 7a0cf55..ddbcf32 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -370,6 +370,11 @@ ActiveRecord::Schema.define do t.integer :owner_id, :integer end + create_table :phrases, :force => true do |t| + t.string :phrase + t.integer :parrot_id + end + create_table :pirates, :force => true do |t| t.column :catchphrase, :string t.column :parrot_id, :integer -- 1.7.0.4