Add ActiveRecord::Relation#destroy_all! method #13169

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

Contributor

The destroy_all! method is useful for when you want to destroy a collection of objects but want the behavior of destroy!. If the before_destroy callback returns false for an object being destroyed an ActiveRecord::RecordNotDestroyed error will be raised.

I couldn't think of a way to keep this DRY with respect to the destroy_all method without getting too fancy, suggestions welcome of course!

@rafaelfranca rafaelfranca commented on the diff Dec 4, 2013
activerecord/test/cases/relations_test.rb
@@ -738,6 +738,19 @@ def test_destroy_all
assert davids.loaded?
end
+ def test_destroy_all!
+ davids = Author.where(:name => 'David')
+
+ # Force load
+ assert_equal [authors(:david)], davids.to_a
+ assert davids.loaded?
+
+ assert_difference('Author.count', -1) { davids.destroy_all! }
rafaelfranca
rafaelfranca Dec 4, 2013 Owner

It is missing tests for the exception case.

f1sherman
f1sherman Dec 4, 2013 Contributor

Thanks, I added a test for this. I wasn't sure of the best approach to making the before_destroy callback for the model return false, so I used an approach that I saw in the tests for destroy!. Alternatively I could alter the test Author model directly to conditionally return false from the before_destroy. Let me know what you think.

Owner

What happens when I have 10 items and in the ninth the exception is raised? The 8 firsts are reverted?

Contributor

The first 8 would not be reverted automatically, though this behavior could be achieved by wrapping the destroy_all! in a transaction. It seems more inline with the normal destroy_all to not have any special rollback/revert behavior, since that does not revert in the case of an error being raised.

Owner

Yes, agree. Thank you for explaining.

Contributor

@rafaelfranca @f1sherman is there anything I can do to help this patch get merged in?

Just stumbled upon a use case for this method.

@f1sherman f1sherman Add ```ActiveRecord::Relation#destroy_all!``` method
The ```destroy_all!``` method is useful for when you want to destroy a
collection of objects but want the behavior of ```destroy!```.  If the
```before_destroy``` callback returns ```false``` for an object being
destroyed an ```ActiveRecord::RecordNotDestroyed``` error will be
raised.
d2cd822
Contributor

@sidonath thanks for the interest. I think all feedback has been addressed, but I'm guessing we're going to have to wait until 4.1 is released before any new features will be merged in. Of course I defer to @rafaelfranca and the other core rails developers since it's their decision :)

Member

I am not sure if this is still necessary, as after 5aab0c0 , destroy_all method will raise.

Owner

Right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment