From 1eced514d65c46c3d75378700f818598c9c94f64 Mon Sep 17 00:00:00 2001 From: "Dr.(USA) Joerg Schray" Date: Fri, 16 Mar 2012 12:26:24 +0100 Subject: [PATCH] Issue #1: :before_add callback is called when nested attributes assignment assigns to existing record if the association is not yet loaded Issue #2: Nested Attributes assignment does not affect the record in the association target when callback triggers loading of the association Complete rewrite of tests --- .../lib/active_record/nested_attributes.rb | 19 ++- .../nested_attributes_with_callbacks_test.rb | 145 ++++++++++++++++++ 2 files changed, 154 insertions(+), 10 deletions(-) create mode 100644 activerecord/test/cases/nested_attributes_with_callbacks_test.rb diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index 602ab9e2f4a49..19ab1b23016ee 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -412,16 +412,15 @@ def assign_nested_attributes_for_collection_association(association_name, attrib association.build(attributes.except(*UNASSIGNABLE_KEYS)) end elsif existing_record = existing_records.detect { |record| record.id.to_s == attributes['id'].to_s } - unless association.loaded? || call_reject_if(association_name, attributes) - # Make sure we are operating on the actual object which is in the association's - # proxy_target array (either by finding it, or adding it if not found) - target_record = association.target.detect { |record| record == existing_record } - - if target_record - existing_record = target_record - else - association.add_to_target(existing_record) - end + # Make sure we are operating on the actual object which is in the association's + # proxy_target array (either by finding it, or adding it if not found) + # Take into account that the proxy_target may have changed due to callbacks + target_record = association.target.detect { |record| record.id.to_s == attributes['id'].to_s } + if target_record + existing_record = target_record + else + #FIXME: there is no good way of adding the record without callback + association.target << existing_record end if !call_reject_if(association_name, attributes) diff --git a/activerecord/test/cases/nested_attributes_with_callbacks_test.rb b/activerecord/test/cases/nested_attributes_with_callbacks_test.rb new file mode 100644 index 0000000000000..7da98bf675c24 --- /dev/null +++ b/activerecord/test/cases/nested_attributes_with_callbacks_test.rb @@ -0,0 +1,145 @@ +require "cases/helper" +require "models/pirate" +require "models/bird" + +class TestNestedAttributesWithCallbacksInterferingWithAssignment < ActiveRecord::TestCase + Pirate.has_many(:birds_with_interfering_callback, + :class_name => "Bird", + :before_add => proc { |p,b| + @@add_callback_called << b + p.birds_with_interfering_callback.to_a + }) + Pirate.has_many(:birds_with_callback, + :class_name => "Bird", + :before_add => proc { |p,b| @@add_callback_called << b }) + + Pirate.accepts_nested_attributes_for(:birds_with_interfering_callback, + :birds_with_callback, + :allow_destroy => true) + + def setup + @@add_callback_called = [] + @expect_callbacks_for = [] + @pirate_with_two_birds = Pirate.new.tap do |pirate| + pirate.catchphrase = "Don't call me!" + pirate.birds_attributes = [{:name => 'Bird1'},{:name => 'Bird2'}] + pirate.save! + end + @birds = @pirate_with_two_birds.birds.to_a + @birds_attributes = @birds.map do |bird| + bird.attributes.slice("id","name") + end + end + + def new_birds + @pirate_with_two_birds.birds_with_callback.to_a - @birds + end + + def new_bird_attributes + [{'name' => "New Bird"}] + end + + def bird2_deletion_attributes + [{'id' => @birds[1].id.to_s, "_destroy" => true}] + end + + def b1_update_new_b_b2_destroy_attributes + [{'id' => @birds[0].id.to_s, 'name' => 'New Name'}, + {'name' => "New Bird"}, + {'id' => @birds[1].id.to_s, "_destroy" => true}] + end + + # Characterizing when :before_add callback is called + test ":before_add called for new bird when not loaded" do + assert_birds_with_callback_not_loaded + assert_callback_called_for_new_bird_assignment + end + + test ":before_add called for new bird when loaded" do + @pirate_with_two_birds.birds_with_callback.load_target + assert_callback_called_for_new_bird_assignment + end + + def assert_callback_called_for_new_bird_assignment + @pirate_with_two_birds.birds_with_callback_attributes = new_bird_attributes + assert_equal(1,new_birds.size) + assert_callback_called_for_new_birds + end + + test ":before_add not called for identical assignment when not loaded" do + assert_birds_with_callback_not_loaded + assert_callback_not_called_for_identical_assignment + end + + test ":before_add not called for identical assignment when loaded" do + @pirate_with_two_birds.birds_with_callback.load_target + assert_callback_not_called_for_identical_assignment + end + + def assert_callback_not_called_for_identical_assignment + @pirate_with_two_birds.birds_with_callback_attributes = @birds_attributes + assert_equal([],new_birds) + assert_callback_called_for_new_birds + end + + test ":before_add not called for deletion assignment when not loaded" do + assert_birds_with_callback_not_loaded + assert_callback_not_called_for_deletion_assignment + end + + test ":before_add not called for deletion assignment when loaded" do + @pirate_with_two_birds.birds_with_callback.load_target + assert_callback_not_called_for_deletion_assignment + end + + def assert_callback_not_called_for_deletion_assignment + @pirate_with_two_birds.birds_with_callback_attributes = + bird2_deletion_attributes + assert_callback_called_for_new_birds + end + + def assert_birds_with_callback_not_loaded + assert_equal(false,@pirate_with_two_birds.birds_with_callback.loaded?) + end + + def assert_callback_called_for_new_birds + assert_equal(new_birds,@@add_callback_called) + end + + # Ensuring that the records in the association target are updated, + # whether the association is loaded before or not + test "Assignment updates records in target when not loaded" do + assert_equal(false,@pirate_with_two_birds.birds_with_callback.loaded?) + assert_assignment_affects_records_in_target(:birds_with_callback) + end + + test "Assignment updates records in target when loaded" do + @pirate_with_two_birds.birds_with_callback.load_target + assert_assignment_affects_records_in_target(:birds_with_callback) + end + + test("Assignment updates records in target when not loaded" + + " and callback loads target") do + assert_equal(false, + @pirate_with_two_birds.birds_with_interfering_callback.loaded?) + assert_assignment_affects_records_in_target( + :birds_with_interfering_callback) + end + + test("Assignment updates records in target when loaded" + + " and callback loads target") do + @pirate_with_two_birds.birds_with_interfering_callback.load_target + assert_assignment_affects_records_in_target( + :birds_with_interfering_callback) + end + + def assert_assignment_affects_records_in_target(association_name) + @pirate_with_two_birds.send("#{association_name}_attributes=", + b1_update_new_b_b2_destroy_attributes) + association = @pirate_with_two_birds.send(association_name) + birds_in_target = @birds.map { |b| association.detect { |b_in_t| b_in_t == b }} + assert_equal(true,birds_in_target[0].name_changed?,'First record not changed') + assert_equal(true,birds_in_target[1].marked_for_destruction?, + 'Second record not marked for destruction') + end +end