Permalink
Browse files

Refactor the implementations of AssociatioCollection#delete and #dest…

…roy to be more consistent with each other, and to stop passing blocks around, thus making the execution easier to follow.
  • Loading branch information...
1 parent d9870d9 commit e62b57647258fad34129975c5a264d19af2dbbe8 @jonleighton jonleighton committed Feb 6, 2011
@@ -178,10 +178,7 @@ def count(column_name = nil, options = {})
# are actually removed from the database, that depends precisely on
# +delete_records+. They are in any case removed from the collection.
def delete(*records)
- remove_records(records) do |_records, old_records|
- delete_records(old_records) if old_records.any?
- _records.each { |record| @target.delete(record) }
- end
+ delete_or_destroy(records, @reflection.options[:dependent])
end
# Destroy +records+ and remove them from this association calling
@@ -190,12 +187,8 @@ def delete(*records)
# Note that this method will _always_ remove records from the database
# ignoring the +:dependent+ option.
def destroy(*records)
- records = find(records) if records.any? {|record| record.kind_of?(Fixnum) || record.kind_of?(String)}
- remove_records(records) do |_records, old_records|
- delete_records(old_records, :destroy) if old_records.any?
- end
-
- load_target
+ records = find(records) if records.any? { |record| record.kind_of?(Fixnum) || record.kind_of?(String) }
+ delete_or_destroy(records, :destroy)
end
def create(attrs = {})
@@ -450,21 +443,24 @@ def build_record(attributes, &block)
add_record_to_target_with_callbacks(record, &block)
end
- def remove_records(*records)
+ def delete_or_destroy(records, method)
records = records.flatten
records.each { |record| raise_on_type_mismatch(record) }
+ existing_records = records.reject { |r| r.new_record? }
transaction do
records.each { |record| callback(:before_remove, record) }
- old_records = records.reject { |r| r.new_record? }
- yield(records, old_records)
+
+ delete_records(existing_records, method) if existing_records.any?
+ records.each { |record| @target.delete(record) }
+
records.each { |record| callback(:after_remove, record) }
end
end
# Delete the given records from the association, using one of the methods :destroy,
- # :delete_all or :nullify. The default method used is given by the :dependent option.
- def delete_records(records, method = @reflection.options[:dependent])
+ # :delete_all or :nullify (or nil, in which case a default is used).
+ def delete_records(records, method)
raise NotImplementedError
end
@@ -40,7 +40,7 @@ def count_records
load_target.size
end
- def delete_records(records, method = nil)
+ def delete_records(records, method)
if sql = @reflection.options[:delete_sql]
records.each { |record| @owner.connection.delete(interpolate_sql(sql, record)) }
else
@@ -80,7 +80,7 @@ def inverse_updates_counter_cache?(reflection = @reflection)
end
# Deletes the records according to the <tt>:dependent</tt> option.
- def delete_records(records, method = @reflection.options[:dependent])
+ def delete_records(records, method)
if method == :destroy
records.each { |r| r.destroy }
update_counter(-records.length) unless inverse_updates_counter_cache?
@@ -57,7 +57,7 @@ def update_through_counter?(method)
end
end
- def delete_records(records, method = @reflection.options[:dependent])
+ def delete_records(records, method)
through = @owner.send(:association_proxy, @reflection.through_reflection.name)
scope = through.scoped.where(construct_join_attributes(*records))
@@ -837,7 +837,7 @@ def test_a_child_marked_for_destruction_should_not_be_destroyed_twice_while_savi
@pirate.parrots.each { |parrot| parrot.mark_for_destruction }
assert @pirate.save
- assert_queries(1) do
+ assert_no_queries do
assert @pirate.save
end
end

0 comments on commit e62b576

Please sign in to comment.