diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 05ce8ff..b6d2e3a 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -65,7 +65,7 @@ module ActiveRecord # See ActiveRecord::Associations::ClassMethods for documentation. module Associations # :nodoc: - # These classes will be loaded when associatoins are created. + # These classes will be loaded when associations are created. # So there is no need to eager load them. autoload :AssociationCollection, 'active_record/associations/association_collection' autoload :AssociationProxy, 'active_record/associations/association_proxy' @@ -791,7 +791,7 @@ module ActiveRecord configure_dependency_for_has_many(reflection) add_multiple_associated_validation_callbacks(reflection.name) unless options[:validate] == false - add_multiple_associated_save_callbacks(reflection.name) + add_multiple_associated_save_callbacks(reflection) add_association_callbacks(reflection.name, reflection.options) if options[:through] @@ -1243,7 +1243,7 @@ module ActiveRecord reflection = create_has_and_belongs_to_many_reflection(association_id, options, &extension) add_multiple_associated_validation_callbacks(reflection.name) unless options[:validate] == false - add_multiple_associated_save_callbacks(reflection.name) + add_multiple_associated_save_callbacks(reflection) collection_accessor_methods(reflection, HasAndBelongsToManyAssociation) # Don't use a before_destroy callback since users' before_destroy @@ -1397,7 +1397,8 @@ module ActiveRecord validate method_name end - def add_multiple_associated_save_callbacks(association_name) + def add_multiple_associated_save_callbacks(reflection) + association_name = reflection.name method_name = "before_save_associated_records_for_#{association_name}".to_sym define_method(method_name) do @new_record_before_save = new_record? @@ -1408,20 +1409,16 @@ module ActiveRecord method_name = "after_create_or_update_associated_records_for_#{association_name}".to_sym define_method(method_name) do association = association_instance_get(association_name) + + if association + records_to_delete = association.records_to_delete + association.send(:delete_records, records_to_delete) unless records_to_delete.empty? - records_to_save = if @new_record_before_save - association - elsif association && association.loaded? - association.select { |record| record.new_record? } - elsif association && !association.loaded? - association.target.select { |record| record.new_record? } - else - [] + records_to_insert = association.records_to_insert(@new_record_before_save) + records_to_insert.each { |record| association.send(:insert_record, record) } + # reconstruct the SQL queries now that we know the owner's id + association.send(:construct_sql) if association.respond_to?(:construct_sql) end - records_to_save.each { |record| association.send(:insert_record, record) } unless records_to_save.blank? - - # reconstruct the SQL queries now that we know the owner's id - association.send(:construct_sql) if association.respond_to?(:construct_sql) end # Doesn't use after_save as that would save associations added in after_create/after_update twice diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index 0fefec1..6566906 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -116,7 +116,7 @@ module ActiveRecord flatten_deeper(records).each do |record| raise_on_type_mismatch(record) add_record_to_target_with_callbacks(record) do |r| - result &&= insert_record(record) unless @owner.new_record? + result &&= insert_record(record) unless @owner.new_record? || @reflection.options[:autosave] end end end @@ -197,15 +197,17 @@ module ActiveRecord def delete(*records) records = flatten_deeper(records) records.each { |record| raise_on_type_mismatch(record) } - + transaction do records.each { |record| callback(:before_remove, record) } - - old_records = records.reject {|r| r.new_record? } - delete_records(old_records) if old_records.any? - + + unless @reflection.options[:autosave] + old_records = records.reject {|r| r.new_record? } + delete_records(old_records) if old_records.any? + end + records.each do |record| - @target.delete(record) + delete_from_target(record) callback(:after_remove, record) end end @@ -334,6 +336,24 @@ module ActiveRecord super || @reflection.klass.respond_to?(method, include_private) end + # @private + def records_to_insert(new_record_before_save) + if new_record_before_save + self + elsif @reflection.options[:autosave] + @records_to_insert || [] + elsif self.loaded? + self.select { |record| record.new_record? } + else + self.target.select { |record| record.new_record? } + end + end + + # @private + def records_to_delete + @records_to_delete || [] + end + protected def construct_find_options!(options) end @@ -384,6 +404,8 @@ module ActiveRecord def reset_target! @target = Array.new + @records_to_insert = Array.new + @records_to_delete = Array.new end def find_target @@ -425,12 +447,23 @@ module ActiveRecord def add_record_to_target_with_callbacks(record) callback(:before_add, record) yield(record) if block_given? - @target ||= [] unless loaded? + @target ||= [] @target << record unless @reflection.options[:uniq] && @target.include?(record) + if loaded? && @reflection.options[:autosave] + (@records_to_delete ||= []).delete(record) + (@records_to_insert ||= []) << record + end callback(:after_add, record) record end + def delete_from_target(record) + @target.delete(record) + if loaded? && @reflection.options[:autosave] + (@records_to_delete ||= []) << record unless (@records_to_insert ||= []).delete(record) + end + end + def callback(method, record) callbacks_for(method).each do |callback| ActiveSupport::Callbacks::Callback.new(method, callback, record).call(@owner, record) diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index e3122d1..fc26fde 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -227,11 +227,18 @@ module ActiveRecord marked_for_destruction? end + # See ActionView::Helpers::FormHelper::fields_for for more info. + def _unlink + # TODO: should return true when validation of a form fails and + # a record has been marked for unlinking. + false + 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 } + UNASSIGNABLE_KEYS = %w{ id _delete _unlink } # Assigns the given attributes to the association. # @@ -297,8 +304,18 @@ module ActiveRecord unless reject_new_record?(association_name, attributes) send(association_name).build(attributes.except(*UNASSIGNABLE_KEYS)) end - elsif existing_record = send(association_name).detect { |record| record.id.to_s == attributes['id'].to_s } - assign_to_or_mark_for_destruction(existing_record, attributes, allow_destroy) + else + existing_record = send(association_name).detect { |record| record.id.to_s == attributes['id'].to_s } + if attributes.has_key?("_unlink") + if has_unlink_flag?(attributes) + send(association_name).delete(existing_record) + elsif existing_record.nil? + existing_record = self.class.reflections[association_name].klass.find(attributes['id'].to_s.to_i) + send(association_name).push(existing_record) if existing_record + end + elsif existing_record + assign_to_or_mark_for_destruction(existing_record, attributes, allow_destroy) + end end end end @@ -318,11 +335,17 @@ module ActiveRecord ConnectionAdapters::Column.value_to_boolean hash['_delete'] end + # Determines if a hash contains a truthy _unlink key. + def has_unlink_flag?(hash) + ConnectionAdapters::Column.value_to_boolean hash['_unlink'] + end + # Determines if a new record should be build by checking for - # has_delete_flag? or if a :reject_if proc exists for this - # association and evaluates to +true+. + # has_delete_flag? or has_unlink_flag? or if a :reject_if proc + # exists for this association and evaluates to +true+. def reject_new_record?(association_name, attributes) has_delete_flag?(attributes) || + has_unlink_flag?(attributes) || self.class.reject_new_nested_attributes_procs[association_name].try(:call, attributes) end end diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 381249c..f467ea4 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -376,6 +376,27 @@ module AutosaveAssociationOnACollectionAssociationTests @pirate.save! end end + + def test_should_link_unlink_associated_models + targets = @association_name.to_s.singularize.classify.constantize.all + new_child_1, new_child_2, new_child_3 = targets[2], targets[5], @child_2 + @pirate.send("#{@association_name.to_s.singularize}_ids=", [new_child_1.id, new_child_2.id, new_child_3.id]) + assocs = @pirate.send(@association_name) + assert_equal 3, assocs.size + assert assocs.include?(new_child_1) + assert assocs.include?(new_child_2) + assert assocs.include?(new_child_3) + pirate_stuff_in_db = Pirate.find(@pirate.id).send(@association_name) + assert_equal 2, pirate_stuff_in_db.size + assert pirate_stuff_in_db.include?(@child_1) + assert pirate_stuff_in_db.include?(@child_2) + @pirate.save! + pirate_stuff_in_db = Pirate.find(@pirate.id).send(@association_name) + assert_equal 3, pirate_stuff_in_db.size + assert pirate_stuff_in_db.include?(new_child_1) + assert pirate_stuff_in_db.include?(new_child_2) + assert pirate_stuff_in_db.include?(new_child_3) + end end class TestAutosaveAssociationOnAHasManyAssociation < ActiveRecord::TestCase @@ -385,8 +406,8 @@ class TestAutosaveAssociationOnAHasManyAssociation < 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') + @child_1 = @pirate.birds.create(:name => 'Posideons Killer2') + @child_2 = @pirate.birds.create(:name => 'Killer bandita Dionne2') end include AutosaveAssociationOnACollectionAssociationTests @@ -400,8 +421,8 @@ class TestAutosaveAssociationOnAHasAndBelongsToManyAssociation < ActiveRecord::T @habtm = true @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') + @child_1 = @pirate.parrots.create(:name => 'Posideons Killer3') + @child_2 = @pirate.parrots.create(:name => 'Killer bandita Dionne3') end include AutosaveAssociationOnACollectionAssociationTests