Permalink
Browse files

Ensure AutosaveAssociation runs remove callbacks [#2146 state:resolved]

Signed-off-by: Eloy Duran <eloy.de.enige@gmail.com>
Signed-off-by: Pratik Naik <pratiknaik@gmail.com>
  • Loading branch information...
1 parent 91b98cf commit 47bdf3bf40ec17e1f8ca1c0e3d7f697d0c4cd1bf Luca Guidi committed with lifo Mar 12, 2009
@@ -186,7 +186,6 @@ def count(*args)
end
end
-
# Removes +records+ from this association calling +before_remove+ and
# +after_remove+ callbacks.
#
@@ -195,22 +194,25 @@ def count(*args)
# are actually removed from the database, that depends precisely on
# +delete_records+. They are in any case removed from the collection.
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? }
+ remove_records(records) do |records, old_records|
delete_records(old_records) if old_records.any?
-
- records.each do |record|
- @target.delete(record)
- callback(:after_remove, record)
- end
+ records.each { |record| @target.delete(record) }
end
end
+ # Destroy +records+ and remove from this association calling +before_remove+
+ # and +after_remove+ callbacks.
+ #
+ # Note this method will always remove records from database ignoring the
+ # +:dependent+ option.
+ def destroy(*records)
+ remove_records(records) do |records, old_records|
+ old_records.each { |record| record.destroy }
+ end
+
+ load_target
+ end
+
# Removes all records from this association. Returns +self+ so method calls may be chained.
def clear
return self if length.zero? # forces load_target if it hasn't happened already
@@ -223,15 +225,14 @@ def clear
self
end
-
- def destroy_all
- transaction do
- each { |record| record.destroy }
- end
+ # Destory all the records from this association
+ def destroy_all
+ load_target
+ destroy(@target)
reset_target!
end
-
+
def create(attrs = {})
if attrs.is_a?(Array)
attrs.collect { |attr| create(attr) }
@@ -431,6 +432,18 @@ def add_record_to_target_with_callbacks(record)
record
end
+ def remove_records(*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? }
+ yield(records, old_records)
+ records.each { |record| callback(:after_remove, record) }
+ end
+ end
+
def callback(method, record)
callbacks_for(method).each do |callback|
ActiveSupport::Callbacks::Callback.new(method, callback, record).call(@owner, record)
@@ -283,7 +283,7 @@ def save_collection_association(reflection)
if records = associated_records_to_validate_or_save(association, @new_record_before_save, autosave)
records.each do |record|
if autosave && record.marked_for_destruction?
- record.destroy
+ association.destroy(record)
elsif @new_record_before_save || record.new_record?
if autosave
association.send(:insert_record, record, false, false)
@@ -381,6 +381,33 @@ def test_additional_columns_from_join_table
assert_date_from_db Date.new(2004, 10, 10), Developer.find(1).projects.first.joined_on.to_date
end
+ def test_destroying
+ david = Developer.find(1)
+ active_record = Project.find(1)
+ david.projects.reload
+ assert_equal 2, david.projects.size
+ assert_equal 3, active_record.developers.size
+
+ assert_difference "Project.count", -1 do
+ david.projects.destroy(active_record)
+ end
+
+ assert_equal 1, david.reload.projects.size
+ assert_equal 1, david.projects(true).size
+ end
+
+ def test_destroying_array
+ david = Developer.find(1)
+ david.projects.reload
+
+ assert_difference "Project.count", -Project.count do
+ david.projects.destroy(Project.find(:all))
+ end
+
+ assert_equal 0, david.reload.projects.size
+ assert_equal 0, david.projects(true).size
+ end
+
def test_destroy_all
david = Developer.find(1)
david.projects.reload
@@ -680,6 +680,30 @@ def test_deleting_self_type_mismatch
assert_raise(ActiveRecord::AssociationTypeMismatch) { david.projects.delete(Project.find(1).developers) }
end
+ def test_destroying
+ force_signal37_to_load_all_clients_of_firm
+
+ assert_difference "Client.count", -1 do
+ companies(:first_firm).clients_of_firm.destroy(companies(:first_firm).clients_of_firm.first)
+ end
+
+ assert_equal 0, companies(:first_firm).reload.clients_of_firm.size
+ assert_equal 0, companies(:first_firm).clients_of_firm(true).size
+ end
+
+ def test_destroying_a_collection
+ force_signal37_to_load_all_clients_of_firm
+ companies(:first_firm).clients_of_firm.create("name" => "Another Client")
+ assert_equal 2, companies(:first_firm).clients_of_firm.size
+
+ assert_difference "Client.count", -2 do
+ companies(:first_firm).clients_of_firm.destroy([companies(:first_firm).clients_of_firm[0], companies(:first_firm).clients_of_firm[1]])
+ end
+
+ assert_equal 0, companies(:first_firm).reload.clients_of_firm.size
+ assert_equal 0, companies(:first_firm).clients_of_firm(true).size
+ end
+
def test_destroy_all
force_signal37_to_load_all_clients_of_firm
assert !companies(:first_firm).clients_of_firm.empty?, "37signals has clients after load"
@@ -92,6 +92,24 @@ def test_delete_association
assert posts(:welcome).reload.people(true).empty?
end
+ def test_destroy_association
+ assert_difference "Person.count", -1 do
+ posts(:welcome).people.destroy(people(:michael))
+ end
+
+ assert posts(:welcome).reload.people.empty?
+ assert posts(:welcome).people(true).empty?
+ end
+
+ def test_destroy_all
+ assert_difference "Person.count", -1 do
+ posts(:welcome).people.destroy_all
+ end
+
+ assert posts(:welcome).reload.people.empty?
+ assert posts(:welcome).people(true).empty?
+ end
+
def test_replace_association
assert_queries(4){posts(:welcome);people(:david);people(:michael); posts(:welcome).people(true)}
@@ -556,6 +556,41 @@ def save(*args)
assert_raise(RuntimeError) { assert !@pirate.save }
assert_equal before, @pirate.reload.send(association_name)
end
+
+ # Add and remove callbacks tests for association collections.
+ %w{ method proc }.each do |callback_type|
+ define_method("test_should_run_add_callback_#{callback_type}s_for_#{association_name}") do
+ association_name_with_callbacks = "#{association_name}_with_#{callback_type}_callbacks"
+
+ pirate = Pirate.new(:catchphrase => "Arr")
+ pirate.send(association_name_with_callbacks).build(:name => "Crowe the One-Eyed")
+
+ expected = [
+ "before_adding_#{callback_type}_#{association_name.singularize}_<new>",
+ "after_adding_#{callback_type}_#{association_name.singularize}_<new>"
+ ]
+
+ assert_equal expected, pirate.ship_log
+ end
+
+ define_method("test_should_run_remove_callback_#{callback_type}s_for_#{association_name}") do
+ association_name_with_callbacks = "#{association_name}_with_#{callback_type}_callbacks"
+
+ @pirate.send(association_name_with_callbacks).create!(:name => "Crowe the One-Eyed")
+ @pirate.send(association_name_with_callbacks).each { |c| c.mark_for_destruction }
+ child_id = @pirate.send(association_name_with_callbacks).first.id
+
+ @pirate.ship_log.clear
+ @pirate.save
+
+ expected = [
+ "before_removing_#{callback_type}_#{association_name.singularize}_#{child_id}",
+ "after_removing_#{callback_type}_#{association_name.singularize}_#{child_id}"
+ ]
+
+ assert_equal expected, @pirate.ship_log
+ end
+ end
end
end
@@ -1,16 +1,63 @@
class Pirate < ActiveRecord::Base
belongs_to :parrot
has_and_belongs_to_many :parrots
- has_many :treasures, :as => :looter
+ has_and_belongs_to_many :parrots_with_method_callbacks, :class_name => "Parrot",
+ :before_add => :log_before_add,
+ :after_add => :log_after_add,
+ :before_remove => :log_before_remove,
+ :after_remove => :log_after_remove
+ has_and_belongs_to_many :parrots_with_proc_callbacks, :class_name => "Parrot",
+ :before_add => proc {|p,pa| p.ship_log << "before_adding_proc_parrot_#{pa.id || '<new>'}"},
+ :after_add => proc {|p,pa| p.ship_log << "after_adding_proc_parrot_#{pa.id || '<new>'}"},
+ :before_remove => proc {|p,pa| p.ship_log << "before_removing_proc_parrot_#{pa.id}"},
+ :after_remove => proc {|p,pa| p.ship_log << "after_removing_proc_parrot_#{pa.id}"}
+ has_many :treasures, :as => :looter
has_many :treasure_estimates, :through => :treasures, :source => :price_estimates
# These both have :autosave enabled because accepts_nested_attributes_for is used on them.
has_one :ship
has_many :birds
+ has_many :birds_with_method_callbacks, :class_name => "Bird",
+ :before_add => :log_before_add,
+ :after_add => :log_after_add,
+ :before_remove => :log_before_remove,
+ :after_remove => :log_after_remove
+ has_many :birds_with_proc_callbacks, :class_name => "Bird",
+ :before_add => proc {|p,b| p.ship_log << "before_adding_proc_bird_#{b.id || '<new>'}"},
+ :after_add => proc {|p,b| p.ship_log << "after_adding_proc_bird_#{b.id || '<new>'}"},
+ :before_remove => proc {|p,b| p.ship_log << "before_removing_proc_bird_#{b.id}"},
+ :after_remove => proc {|p,b| p.ship_log << "after_removing_proc_bird_#{b.id}"}
accepts_nested_attributes_for :parrots, :birds, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? }
accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? }
+ accepts_nested_attributes_for :parrots_with_method_callbacks, :parrots_with_proc_callbacks,
+ :birds_with_method_callbacks, :birds_with_proc_callbacks, :allow_destroy => true
validates_presence_of :catchphrase
+
+ def ship_log
+ @ship_log ||= []
+ end
+
+ private
+ def log_before_add(record)
+ log(record, "before_adding_method")
+ end
+
+ def log_after_add(record)
+ log(record, "after_adding_method")
+ end
+
+ def log_before_remove(record)
+ log(record, "before_removing_method")
+ end
+
+ def log_after_remove(record)
+ log(record, "after_removing_method")
+ end
+
+ def log(record, callback)
+ ship_log << "#{callback}_#{record.class.name.downcase}_#{record.id || '<new>'}"
+ end
end

0 comments on commit 47bdf3b

Please sign in to comment.