Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

When a has_many association is not :uniq, appending the same record m…

…ultiple times should append it to the @target multiple times [#5964 state:resolved]
  • Loading branch information...
commit ff7bde62c857ec94f45a5be3bc76468deb8b0b3a 1 parent 030480a
@jonleighton jonleighton authored tenderlove committed
View
4 activerecord/lib/active_record/associations/association_collection.rb
@@ -462,10 +462,10 @@ def add_record_to_target_with_callbacks(record)
callback(:before_add, record)
yield(record) if block_given?
@target ||= [] unless loaded?
- if index = @target.index(record)
+ if @reflection.options[:uniq] && index = @target.index(record)
@target[index] = record
else
- @target << record
+ @target << record
end
callback(:after_add, record)
set_inverse_instance(record, @owner)
View
13 activerecord/lib/active_record/nested_attributes.rb
@@ -405,7 +405,18 @@ def assign_nested_attributes_for_collection_association(association_name, attrib
end
elsif existing_record = existing_records.detect { |record| record.id.to_s == attributes['id'].to_s }
- association.send(:add_record_to_target_with_callbacks, existing_record) if !association.loaded? && !call_reject_if(association_name, attributes)
+ unless association.loaded? || call_reject_if(association_name, attributes)
+ # Make sure we are operating on the actual object which is in the association's
+ # proxy_target array (either by finding it, or adding it if not found)
+ target_record = association.proxy_target.detect { |record| record == existing_record }
+
+ if target_record
+ existing_record = target_record
+ else
+ association.send(:add_record_to_target_with_callbacks, existing_record)
+ end
+ end
+
assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy])
else
View
10 activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -45,6 +45,16 @@ def test_associate_existing
assert posts(:thinking).reload.people(true).include?(people(:david))
end
+ def test_associate_existing_record_twice_should_add_to_target_twice
+ post = posts(:thinking)
+ person = people(:david)
+
+ assert_difference 'post.people.to_a.count', 2 do
+ post.people << person
+ post.people << person
+ end
+ end
+
def test_associating_new
assert_queries(1) { posts(:thinking) }
new_person = nil # so block binding catches it
Please sign in to comment.
Something went wrong with that request. Please try again.