From d3e16481d83540a5d32cdf1988bd2c5ef432e707 Mon Sep 17 00:00:00 2001 From: Ian White Date: Tue, 18 May 2010 10:30:11 +0100 Subject: [PATCH] Nested records (re: autosave) are now updated even when the intermediate parent record is unchanged [#4242] --- .../lib/active_record/autosave_association.rb | 25 ++++++- activerecord/test/cases/nested_attributes_test.rb | 80 ++++++++++++++++++++ activerecord/test/models/ship.rb | 3 +- activerecord/test/models/ship_part.rb | 4 +- 4 files changed, 108 insertions(+), 4 deletions(-) diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index 8d40eb5..853336c 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -217,6 +217,12 @@ module ActiveRecord @marked_for_destruction end + # Returns whether or not this record has been changed in any way (including whether + # any of its nested autosave associations are likewise changed) + def changed_for_autosave? + new_record? || changed? || marked_for_destruction? || nested_records_changed_for_autosave? + end + private # Returns the record for an association collection that should be validated @@ -226,12 +232,27 @@ module ActiveRecord if new_record association elsif autosave - association.target.select { |record| record.new_record? || record.changed? || record.marked_for_destruction? } + association.target.select { |record| record.changed_for_autosave? } else association.target.select { |record| record.new_record? } end end - + + # go through nested autosave associations that are loaded in memory (without loading + # any new ones), and return true if is changed for autosave + def nested_records_changed_for_autosave? + self.class.reflect_on_all_autosave_associations.each do |reflection| + if association = association_instance_get(reflection.name) + if [:belongs_to, :has_one].include?(reflection.macro) + return true if association.target && association.target.changed_for_autosave? + else + association.target.each {|record| return true if record.changed_for_autosave? } + end + end + end + false + end + # Validate the association if :validate or :autosave is # turned on for the association specified by +reflection+. def validate_single_association(reflection) diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb index c001db6..2202714 100644 --- a/activerecord/test/cases/nested_attributes_test.rb +++ b/activerecord/test/cases/nested_attributes_test.rb @@ -1,6 +1,7 @@ require "cases/helper" require "models/pirate" require "models/ship" +require "models/ship_part" require "models/bird" require "models/parrot" require "models/treasure" @@ -731,3 +732,82 @@ class TestNestedAttributesWithNonStandardPrimaryKeys < ActiveRecord::TestCase assert_equal ['Foo', 'Bar'], @owner.pets.map(&:name) end end + +class TestHasOneAutosaveAssoictaionWhichItselfHasAutosaveAssociations < ActiveRecord::TestCase + self.use_transactional_fixtures = false + + def setup + @pirate = Pirate.create!(:catchphrase => "My baby takes tha mornin' train!") + @ship = @pirate.create_ship(:name => "The good ship Dollypop") + @part = @ship.parts.create!(:name => "Mast") + @trinket = @part.trinkets.create!(:name => "Necklace") + end + + test "when great-grandchild changed in memory, saving parent should save great-grandchild" do + @trinket.name = "changed" + @pirate.save + assert_equal "changed", @trinket.reload.name + end + + test "when great-grandchild changed via attributes, saving parent should save great-grandchild" do + @pirate.attributes = {:ship_attributes => {:id => @ship.id, :parts_attributes => [{:id => @part.id, :trinkets_attributes => [{:id => @trinket.id, :name => "changed"}]}]}} + @pirate.save + assert_equal "changed", @trinket.reload.name + end + + test "when great-grandchild marked_for_destruction via attributes, saving parent should destroy great-grandchild" do + @pirate.attributes = {:ship_attributes => {:id => @ship.id, :parts_attributes => [{:id => @part.id, :trinkets_attributes => [{:id => @trinket.id, :_destroy => true}]}]}} + assert_difference('@part.trinkets.count', -1) { @pirate.save } + end + + test "when great-grandchild added via attributes, saving parent should create great-grandchild" do + @pirate.attributes = {:ship_attributes => {:id => @ship.id, :parts_attributes => [{:id => @part.id, :trinkets_attributes => [{:name => "created"}]}]}} + assert_difference('@part.trinkets.count', 1) { @pirate.save } + end + + test "when extra records exist for associations, validate (which calls nested_records_changed_for_autosave?) should not load them up" do + @trinket.name = "changed" + Ship.create!(:pirate => @pirate, :name => "The Black Rock") + ShipPart.create!(:ship => @ship, :name => "Stern") + assert_no_queries { @pirate.valid? } + end +end + +class TestHasManyAutosaveAssoictaionWhichItselfHasAutosaveAssociations < ActiveRecord::TestCase + self.use_transactional_fixtures = false + + def setup + @ship = Ship.create!(:name => "The good ship Dollypop") + @part = @ship.parts.create!(:name => "Mast") + @trinket = @part.trinkets.create!(:name => "Necklace") + end + + test "when grandchild changed in memory, saving parent should save grandchild" do + @trinket.name = "changed" + @ship.save + assert_equal "changed", @trinket.reload.name + end + + test "when grandchild changed via attributes, saving parent should save grandchild" do + @ship.attributes = {:parts_attributes => [{:id => @part.id, :trinkets_attributes => [{:id => @trinket.id, :name => "changed"}]}]} + @ship.save + assert_equal "changed", @trinket.reload.name + end + + test "when grandchild marked_for_destruction via attributes, saving parent should destroy grandchild" do + @ship.attributes = {:parts_attributes => [{:id => @part.id, :trinkets_attributes => [{:id => @trinket.id, :_destroy => true}]}]} + assert_difference('@part.trinkets.count', -1) { @ship.save } + end + + test "when grandchild added via attributes, saving parent should create grandchild" do + @ship.attributes = {:parts_attributes => [{:id => @part.id, :trinkets_attributes => [{:name => "created"}]}]} + assert_difference('@part.trinkets.count', 1) { @ship.save } + end + + test "when extra records exist for associations, validate (which calls nested_records_changed_for_autosave?) should not load them up" do + @trinket.name = "changed" + Ship.create!(:name => "The Black Rock") + ShipPart.create!(:ship => @ship, :name => "Stern") + assert_no_queries { @ship.valid? } + end +end \ No newline at end of file diff --git a/activerecord/test/models/ship.rb b/activerecord/test/models/ship.rb index 75c792d..3da0319 100644 --- a/activerecord/test/models/ship.rb +++ b/activerecord/test/models/ship.rb @@ -3,8 +3,9 @@ class Ship < ActiveRecord::Base belongs_to :pirate belongs_to :update_only_pirate, :class_name => 'Pirate' - has_many :parts, :class_name => 'ShipPart', :autosave => true + has_many :parts, :class_name => 'ShipPart' + accepts_nested_attributes_for :parts, :allow_destroy => true accepts_nested_attributes_for :pirate, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? } accepts_nested_attributes_for :update_only_pirate, :update_only => true diff --git a/activerecord/test/models/ship_part.rb b/activerecord/test/models/ship_part.rb index 0a606db..8e61d70 100644 --- a/activerecord/test/models/ship_part.rb +++ b/activerecord/test/models/ship_part.rb @@ -1,5 +1,7 @@ class ShipPart < ActiveRecord::Base belongs_to :ship - + has_many :trinkets, :class_name => "Treasure", :as => :looter + accepts_nested_attributes_for :trinkets, :allow_destroy => true + validates_presence_of :name end \ No newline at end of file -- 1.7.0.5