Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

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.
  • Loading branch information...
commit b1e509ad7a8c8264544f10f4666705cd806b5408 1 parent 9e0b3fc
Francesco Rodríguez frodsan authored
36 activerecord/lib/active_record/autosave_association.rb
View
@@ -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?

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?

Francesco Rodríguez
frodsan added a note

never is too late, send a pull request :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ saved = true
+
+ if autosave && record.marked_for_destruction?
+ records_to_destroy << record
+ elsif autosave != false && (@new_record_before_save || record.new_record?)

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
+ 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
10 activerecord/test/cases/autosave_association_test.rb
View
@@ -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
Alan Da Costa

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?

Francesco Rodríguez

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 && ...?

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