Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failing test case for "after_commit" on destroy #228

Open
wants to merge 1 commit into
base: rails4
Choose a base branch
from

Conversation

skorfmann
Copy link

We're just upgrading an application which uses the paranoia gem (rails4 branch) to Rails 4.2.1 - We've several models which are using conditional after_commit hooks, e.g.:

after_commit   -> { do_stuff }, on: :destroy

This breaks for destroy, since it's depending on destroy? which changed its behaviour with #177.

Our workaround will probably look somehow like this:

class ActiveRecord::Base
  private

  # Determine if a transaction included an action for :create, :update, or :destroy. Used in filtering callbacks.
  def transaction_include_any_action?(actions) #:nodoc:
    actions.any? do |action|
      case action
      when :create
        transaction_record_state(:new_record)
      when :destroy
        destroyed? || (respond_to?(:paranoia_destroyed?) && paranoia_destroyed?)
      when :update
        !(transaction_record_state(:new_record) || destroyed? || (respond_to?(:paranoia_destroyed?) && paranoia_destroyed?))
      end
    end
  end
end

However, we're wondering if anyone has (or could think of) a nicer solution for this?

@Martin288
Copy link

@skorfmann I am facing the same problem because our application is using paranoia and sunspot together. The latter is dependent on the after_commit :hook, on: :create callback but paranoia has broken it. I wonder if I can take your monkey patching as a working solution.

@skorfmann
Copy link
Author

@Martin288 I think we went this solution and it's working fine for us

@Martin288
Copy link

@skorfmann cool! But I have fallback to use :after_destory instead_of :after_commint for sunspot's auto_remove_callback configuration now. And it also works for our case. The README has some related tips: https://github.com/rubysherpas/paranoia#callbacks. Hope paranoia will fix this ASAP.

@MatayoshiMariano
Copy link

👍 to this PR please!

@AlexRiedler
Copy link

AlexRiedler commented Dec 14, 2016

I am not exactly clear why but the workaround @skorfmann didn't work for me until I changed transaction in #destroy to be with_transaction_returning_status instead; otherwise the attributes are not set on the model when doing the transaction_include_any_action check; and as a result would not run the necessary callbacks.

^ not sure if the above is necessary as well for this change; but might give insight for anyone working on it.

Also this test given might only works in Rails 5 since after_commit are not called in a lot of rails test environments (rails 4 and less need something like test_after_commit gem).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants