Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Backport #3329 to 3-2-stable #6935

Merged
merged 1 commit into from

4 participants

@frodsan

Fix bug with autosave collection association on new record with a marked
for destroy record in autosave collection.

Fixes #6918.

@frodsan frodsan Backport #3329 to 3-2-stable
Fix bug with autosave collection association on new record with a marked
for destroy record in autosave collection.

Fixes #6918.
b1e509a
@rafaelfranca
Owner

OMG! Too much change. I'm afraid that this can introduce more bugs. Can we do a simpler change?

@frodsan

@rafaelfranca No indentation? :(

@rafaelfranca
Owner

Ahhh, now I got. Merged. Thanks.

@rafaelfranca rafaelfranca merged commit dacc947 into from
@adacosta

It'd be nice if the collection to be operated on had its logic outside of the operations, so we don't have to follow logic like this "next" method. It seems like there are lots of different things going on here in general, also record skipping and setting a "saved" variable and then potentially acting on that variable state.

edit: this method is too large. Is it too late to refactor?

never is too late, send a pull request :)

@fingermark

The code has if autosave && ... on line 342 and elsif autosave != false && ... on line 344. Why not be consistent and change line 344 to elsif autosave && ...?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 2, 2012
  1. @frodsan

    Backport #3329 to 3-2-stable

    frodsan authored
    Fix bug with autosave collection association on new record with a marked
    for destroy record in autosave collection.
    
    Fixes #6918.
This page is out of date. Refresh to see the latest.
View
36 activerecord/lib/active_record/autosave_association.rb
@@ -332,25 +332,31 @@ def save_collection_association(reflection)
if records = associated_records_to_validate_or_save(association, @new_record_before_save, autosave)
begin
- records.each do |record|
- next if record.destroyed?
+ records_to_destroy = []
+
+ records.each do |record|
+ next if record.destroyed?
+
+ saved = true
+
+ if autosave && record.marked_for_destruction?
+ records_to_destroy << record
+ elsif autosave != false && (@new_record_before_save || record.new_record?)
+ if autosave
+ saved = association.insert_record(record, false)
+ else
+ association.insert_record(record) unless reflection.nested?
+ end
+ elsif autosave
+ saved = record.save(:validate => false)
+ end
- saved = true
+ raise ActiveRecord::Rollback unless saved
+ end
- if autosave && record.marked_for_destruction?
+ records_to_destroy.each do |record|
association.proxy.destroy(record)
- elsif autosave != false && (@new_record_before_save || record.new_record?)
- if autosave
- saved = association.insert_record(record, false)
- else
- association.insert_record(record) unless reflection.nested?
- end
- elsif autosave
- saved = record.save(:validate => false)
end
-
- raise ActiveRecord::Rollback unless saved
- end
rescue
records.each {|x| IdentityMap.remove(x) } if IdentityMap.enabled?
raise
View
10 activerecord/test/cases/autosave_association_test.rb
@@ -770,6 +770,16 @@ def destroy(*args)
assert_equal before, @pirate.reload.birds
end
+ def test_when_new_record_a_child_marked_for_destruction_should_not_affect_other_records_from_saving
+ @pirate = @ship.build_pirate(:catchphrase => "Arr' now I shall keep me eye on you matey!") # new record
+
+ 3.times { |i| @pirate.birds.build(:name => "birds_#{i}") }
+ @pirate.birds[1].mark_for_destruction
+ @pirate.save!
+
+ assert_equal 2, @pirate.birds.reload.length
+ 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_has_many") do
Something went wrong with that request. Please try again.