DRYed up Associations#clear. Closes #1906 [Caleb]

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@2580 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

6 files changed, 37 insertions, 36 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index dc9bb28862..1f39b823c8 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,4 +1,6 @@
-*SVN*
+*SVN*                                                                                                                 
+
+* Simplified .clear on active record associations by using the existing delete_records method. #1906 [Caleb <me@cpb.ca>]
 
 * Delegate access to a customized primary key to the conventional id method. #2444. [Blair Zajac <blair@orcaware.com>]
 
diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb
index 932e8e2aae..ced734cfcd 100644
--- a/activerecord/lib/active_record/associations/association_collection.rb
+++ b/activerecord/lib/active_record/associations/association_collection.rb
@@ -33,6 +33,12 @@ module ActiveRecord
 
       alias_method :push, :<<
       alias_method :concat, :<<
+                      
+      # Remove all records from this association
+      def delete_all
+        delete(@target)
+        @target = []
+      end
 
       # Remove +records+ from this association.  Does not destroy +records+.
       def delete(*records)
@@ -50,6 +56,17 @@ module ActiveRecord
           end
         end
       end
+
+      # Removes all records from this association.  Returns +self+ so method calls may be chained.
+      def clear
+        return self if empty? # forces load_target if hasn't happened already
+        if @options[:exclusively_dependent]
+          destroy_all
+        else          
+          delete_all
+        end
+        self
+      end
       
       def destroy_all
         @owner.transaction do
diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
index d386a5a906..04774fa28a 100644
--- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
@@ -19,26 +19,6 @@ module ActiveRecord
         record
       end
 
-      # Removes all records from this association.  Returns +self+ so method calls may be chained.
-      def clear
-        return self if size == 0 # forces load_target if hasn't happened already
-
-        if sql = @options[:delete_sql]
-          each { |record| @owner.connection.execute(sql) }
-        elsif @options[:conditions] 
-          sql = 
-            "DELETE FROM #{@join_table} WHERE #{@association_class_primary_key_name} = #{@owner.quoted_id} " +
-            "AND #{@association_foreign_key} IN (#{collect { |record| record.id }.join(", ")})"
-          @owner.connection.execute(sql)
-        else
-          sql = "DELETE FROM #{@join_table} WHERE #{@association_class_primary_key_name} = #{@owner.quoted_id}"
-          @owner.connection.execute(sql)
-        end
-
-        @target = []
-        self
-      end
-
       def find_first
         load_target.first
       end
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 2c881290aa..114f84ed5d 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -83,20 +83,6 @@ module ActiveRecord
           @association_class.find(*args)
         end
       end
-
-      # Removes all records from this association.  Returns +self+ so
-      # method calls may be chained.
-      def clear
-        if @options[:dependent]
-          each { |associate| associate.destroy }
-        elsif @options[:exclusively_dependent]
-          @association_class.delete_all("#{@association_class_primary_key_name} = #{@owner.quoted_id}")
-        else
-          @association_class.update_all("#{@association_class_primary_key_name} = NULL", "#{@association_class_primary_key_name} = #{@owner.quoted_id}")
-        end
-        @target = []
-        self
-      end
       
       protected
         def find_target
diff --git a/activerecord/test/association_callbacks_test.rb b/activerecord/test/association_callbacks_test.rb
index 5ea1f541ed..1426fb71a9 100644
--- a/activerecord/test/association_callbacks_test.rb
+++ b/activerecord/test/association_callbacks_test.rb
@@ -83,6 +83,20 @@ class AssociationCallbacksTest < Test::Unit::TestCase
     assert_equal ["before_removing#{david.id}", "after_removing#{david.id}", "before_removing#{jamis.id}", 
                   "after_removing#{jamis.id}"], activerecord.developers_log
   end
+
+  def test_has_and_belongs_to_many_remove_callback_on_clear
+    activerecord = projects(:active_record)
+    assert activerecord.developers_log.empty?
+    if activerecord.developers_with_callbacks.size == 0
+      activerecord.developers << developers(:david)
+      activerecord.developers << developers(:jamis)
+      activerecord.reload
+      assert activerecord.developers_with_callbacks.size == 2
+    end
+    log_array = activerecord.developers_with_callbacks.collect {|d| ["before_removing#{d.id}","after_removing#{d.id}"]}.flatten.sort
+    assert activerecord.developers_with_callbacks.clear
+    assert_equal log_array, activerecord.developers_log.sort
+  end
   
   def test_dont_add_if_before_callback_raises_exception
     assert !@david.unchangable_posts.include?(@authorless)
diff --git a/activerecord/test/associations_test.rb b/activerecord/test/associations_test.rb
index fc8d59e862..5cafa010f9 100755
--- a/activerecord/test/associations_test.rb
+++ b/activerecord/test/associations_test.rb
@@ -551,13 +551,15 @@ class HasManyAssociationsTest < Test::Unit::TestCase
     client_id = firm.exclusively_dependent_clients_of_firm.first.id
     assert_equal 1, firm.exclusively_dependent_clients_of_firm.size
 
+    assert_equal [], Client.destroyed_client_ids[firm.id]
+
     # :exclusively_dependent means each client is deleted directly from
     # the database without looping through them calling destroy.
     firm.exclusively_dependent_clients_of_firm.clear
 
     assert_equal 0, firm.exclusively_dependent_clients_of_firm.size
     assert_equal 0, firm.exclusively_dependent_clients_of_firm(true).size
-    assert_equal [], Client.destroyed_client_ids[firm.id]
+    assert_equal [3], Client.destroyed_client_ids[firm.id]
 
     # Should be destroyed since the association is exclusively dependent.
     assert Client.find_by_id(client_id).nil?