Validate :on option on after_commit and after_rollback callbacks

[paukul]

the :on option on after_commit and after_rollback is just a shortcut for :if => "transaction_include_action?...", because of that the valid actions are very well known. unfortunately it doesn't do any validation on the :on condition. If, for example, somebody uses :on => :save, active record will happily setup a transaction_include_action?(:save) (or :whatever) which will never be true.

This patch adds a check if the on condition is one of Transactions::ACTIONS and raises a meaningful ArgumentError if the check fails.

In case you wonder, I've moved the check to a private module method to avoid pollution of the including classes namespaces.

[paukul]

here's the same patch for 3-0-stable: paukul@efba6a8

[jeremyf]

The tests all pass, however, the :private declaration is not privatizing the methods (otherwise calling Transactions.set_options_for_callbacks! would raise an exception).

[steveklabnik]

@paukul ping! any interest in keeping this updated?

[paukul]

Ah sure, forgot about it. I'll take care of the changes

Am Samstag, 15. Dezember 2012 um 22:04 schrieb Steve Klabnik:

@paukul (https://github.com/paukul) ping! any interest in keeping this updated?

—
Reply to this email directly or view it on GitHub (#5100 (comment)).

[schneems]

We can use args.extract_options! here to get rid of the Hash check (http://apidock.com/rails/ActiveSupport/CoreExtensions/Array/ExtractOptions/extract_options!)

options = args.extract_options!
if options[:on]
  #...

[paukul]

the first alternative would be to extract_options! and add them back to the args array again afterwards. I think a simple check for a hash doesn't hurt whereas the whole removing and adding things is simply unnecessary.

the second alternative would be to extract the options in after_rollback and after_commit and add them back to set_callbacks manually. This isn't really dry and frankly speaking, I don't think this is a big deal at all

[paukul]

I moved the helper methods out of the ClassMethods module up to the Transactions module and got rid of the useless private keyword after merging the current master.

@steveklabnik the reason why I have an extra method "for just two lines" is simple.
a) the error message is already long enough and adding "unless ACTIONS.include?..." doesn't really help with that
b) nesting another unless condition just hurts my eyes.

I consider assert_ methods as something like gatekeeper methods. there exist, they don't return anything useful, they just make sure that the current state is still sane or blow up otherwise.

Having said that, If bringing the assert... method back inline is a precondition to getting this change into master I'll do so (with a very grumpy face) :)

[steveklabnik]

No, I was just curious. I don't feel strongly enough about it to make a huge fuss, I was just wondering.

You have a merge commit, it'll need to be removed, and in general, to have them squashed. And a CHANGELOG entry. But the code looks fine to me, but I'd want someone more familliar with AR like @rafaelfranca to merge.

[rafaelfranca]

I would leave these two methods in the ClassMethods module as private

[rafaelfranca]

>> module Foo
>>   extend ActiveSupport::Concern  
>>   module ClassMethods
>>     def foo
>>       bar
>>     end
>>
>>     private
>>
>>     def bar
>>       "bar"
>>     end
>>   end
>> end
=> nil
>> class Baz
>>   include Foo
>> end
=> Baz
>> Baz.foo
=> "bar"
>> Baz.bar
NoMethodError: private method `bar' called for Baz:Class
    from (irb):19
    from /Users/rafaelmfranca/Projects/github/rails/railties/lib/rails/commands/console.rb:78:in `start'
    from /Users/rafaelmfranca/Projects/github/rails/railties/lib/rails/commands/console.rb:9:in `start'
    from /Users/rafaelmfranca/Projects/github/rails/railties/lib/rails/commands.rb:71:in `<top (required)>'
    from script/rails:6:in `require'
    from script/rails:6:in `<main>'

[paukul]

here we go. a squashed commit, @rafaelfranca changes incorporated, passing tests. cheers :)

[steveklabnik]

Looks good to me.

[rafaelfranca]

Very good. Now we need the CHANGELOG entry

[paukul]

CHANGELOG, check!

[guilleiguaran]

@paukul sorry but GitHub reports "This pull request cannot be automatically merged", can you rebase this please? 😄

[paukul]

let's see if I find yet another way to fail.. anyways, here's the commit against the current master. cheers!

[rafaelfranca]

Thank you