Skip to content
This repository
Browse code

Remove all revelant through records.

If a record is removed from a has_many :through, all of the join records
relating to that record should also be removed from the through
association's target.

(Previously the records were removed in the database, but only one was
removed from the in-memory target array.)
  • Loading branch information...
commit 19b2a5f2bdd5bf6404bfc3e574b7477038e9b2bf 1 parent 71bc921
Jon Leighton authored November 03, 2011
5  activerecord/CHANGELOG
@@ -39,6 +39,11 @@
39 39
 
40 40
 *Rails 3.1.2 (unreleased)*
41 41
 
  42
+* If a record is removed from a has_many :through, all of the join records relating to that
  43
+  record should also be removed from the through association's target.
  44
+
  45
+  [Jon Leighton]
  46
+
42 47
 * Fix adding multiple instances of the same record to a has_many :through. [GH #3425]
43 48
 
44 49
   [Jon Leighton]
18  activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -79,12 +79,6 @@ def save_through_record(record)
79 79
           @through_records.delete(record.object_id)
80 80
         end
81 81
 
82  
-        def through_record(record)
83  
-          attributes = construct_join_attributes(record)
84  
-          candidates = Array.wrap(through_association.target)
85  
-          candidates.find { |c| c.attributes.slice(*attributes.keys) == attributes }
86  
-        end
87  
-
88 82
         def build_record(attributes, options = {})
89 83
           ensure_not_nested
90 84
 
@@ -145,14 +139,20 @@ def delete_records(records, method)
145 139
           update_counter(-count)
146 140
         end
147 141
 
  142
+        def through_records_for(record)
  143
+          attributes = construct_join_attributes(record)
  144
+          candidates = Array.wrap(through_association.target)
  145
+          candidates.find_all { |c| c.attributes.slice(*attributes.keys) == attributes }
  146
+        end
  147
+
148 148
         def delete_through_records(through, records)
149 149
           records.each do |record|
150  
-            through_record = through_record(record)
  150
+            through_records = through_records_for(record)
151 151
 
152 152
             if through_reflection.macro == :has_many
153  
-              through.target.delete(through_record)
  153
+              through_records.each { |r| through.target.delete(r) }
154 154
             else
155  
-              through.target = nil if through.target == through_record
  155
+              through.target = nil if through_records.include?(through.target)
156 156
             end
157 157
 
158 158
             @through_records.delete(record.object_id)
15  activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -77,6 +77,21 @@ def test_associate_existing_record_twice_should_add_records_twice
77 77
     end
78 78
   end
79 79
 
  80
+  def test_add_two_instance_and_then_deleting
  81
+    post   = posts(:thinking)
  82
+    person = people(:david)
  83
+
  84
+    post.people << person
  85
+    post.people << person
  86
+
  87
+    counts = ['post.people.count', 'post.people.to_a.count', 'post.readers.count', 'post.readers.to_a.count']
  88
+    assert_difference counts, -2 do
  89
+      post.people.delete(person)
  90
+    end
  91
+
  92
+    assert !post.people.reload.include?(person)
  93
+  end
  94
+
80 95
   def test_associating_new
81 96
     assert_queries(1) { posts(:thinking) }
82 97
     new_person = nil # so block binding catches it

0 notes on commit 19b2a5f

Please sign in to comment.
Something went wrong with that request. Please try again.