Codecov Report

Merging #668 into master will decrease coverage by 0.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #668      +/-   ##
==========================================
- Coverage   95.14%   95.01%   -0.14%     
==========================================
  Files          35       35              
  Lines        1236     1243       +7     
==========================================
+ Hits         1176     1181       +5     
- Misses         60       62       +2     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39e47ea...84171e2. Read the comment docs.

Codecov Report

Merging #668 into master will decrease coverage by 0.13%.
The diff coverage is 95%.

@@            Coverage Diff             @@
##           master     #668      +/-   ##
==========================================
- Coverage   95.14%   95.01%   -0.14%     
==========================================
  Files          35       35              
  Lines        1236     1243       +7     
==========================================
+ Hits         1176     1181       +5     
- Misses         60       62       +2

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39e47ea...84171e2. Read the comment docs.

Codecov Report

Merging #668 into master will decrease coverage by 0.05%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##           master     #668      +/-   ##
==========================================
- Coverage   95.14%   95.09%   -0.06%     
==========================================
  Files          35       35              
  Lines        1236     1243       +7     
==========================================
+ Hits         1176     1182       +6     
- Misses         60       61       +1     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39e47ea...55a01ec. Read the comment docs.

Apologies for intruding, but can I suggest using after_commit_everywhere from the good folks at Evil Martian instead?
I saw this PR in the changelog because of the revert of #660. A few advantages I see from using after_commit_everywhere:

• Actively updated/maintained
• Only has a dependency on ActiveRecord, as expected from a gem that interacts with transactions
• Technical implementation is more sound, including checking if the transaction is joinable (so specs / database cleaner will work)
• Test coverage is better

Both provides the same functionality. @stokarenko what do you think?

@louim

after_commit_everywhere looks really a way stronger than after_commit_action ) In additional, it is able to deal with after_rollback AASM callbacks on the way..

Historically I came to after_commit_action just because of counter_culture which using it under the hood.

Will play with after_commit_everywhere asap )

@louim tried after_commit_everywhere , it works in general but requires activerecord (>= 5.0), that is much larger than rails (~> 3.2.22) which AASM supports still..

@anilmaurya could you review this PR please? )

@stokarenko I see. If I understand the code correctly, ActiveRecord > 5 should only be required for the before_commit hook , the gemspec seems to require 4.2+. That still wouldn't work for rails 3.2.22 however 😢, but there is a long standing PR pending to add support. Maybe after_commit_action could be used only if rails version < 4.2 until a future update of AASM drop old and unsupported version of Rails.

But I'll leave that to your discretion and / or the maintainer call. Thank you for trying after_commit_everywhere

Codecov Report

Merging #668 into master will decrease coverage by 0.05%.
The diff coverage is 95%.

@@            Coverage Diff             @@
##           master     #668      +/-   ##
==========================================
- Coverage   95.14%   95.09%   -0.06%     
==========================================
  Files          35       35              
  Lines        1236     1243       +7     
==========================================
+ Hits         1176     1182       +6     
- Misses         60       61       +1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39e47ea...55a01ec. Read the comment docs.

Codecov Report

Merging #668 into master will decrease coverage by 0.05%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##           master     #668      +/-   ##
==========================================
- Coverage   95.14%   95.09%   -0.06%     
==========================================
  Files          35       35              
  Lines        1236     1243       +7     
==========================================
+ Hits         1176     1182       +6     
- Misses         60       61       +1     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39e47ea...55a01ec. Read the comment docs.

Codecov Report

Merging #668 into master will decrease coverage by 0.05%.
The diff coverage is 95%.

@@            Coverage Diff             @@
##           master     #668      +/-   ##
==========================================
- Coverage   95.14%   95.09%   -0.06%     
==========================================
  Files          35       35              
  Lines        1236     1243       +7     
==========================================
+ Hits         1176     1182       +6     
- Misses         60       61       +1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39e47ea...55a01ec. Read the comment docs.

Codecov Report

Merging #668 into master will decrease coverage by 0.05%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##           master     #668      +/-   ##
==========================================
- Coverage   95.14%   95.09%   -0.06%     
==========================================
  Files          35       35              
  Lines        1236     1243       +7     
==========================================
+ Hits         1176     1182       +6     
- Misses         60       61       +1     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39e47ea...55a01ec. Read the comment docs.

Hi @stokarenko @louim
Thank you for your contribution and sorry for late reply.

I think it's time to remove support for Rails 3.2 .
after_commit_everywhere looks good, I think we should go ahead with this gem.

I will release a major version AASM 6.0.0 to remove support for Rails 3.2

@anilmaurya @louim

Roger! )

I'll prepare the changes using after_commit_everywhere. Waiting for feedback regarding Envek/after_commit_everywhere#8 on the way )

@anilmaurya @louim

Pushed one more commit using after_commit_everywhere instead of after_commit_action.
That will fail some CI builds for sure..

Just thinking - perhaps we need to check the version of after_commit_everywhere and warn when it appears to be lower than 0.1.5 - otherwise the after_commit callback will be silently skipped..

@anilmaurya could you revisit please? )

Thanks for reminder @stokarenko , I will revisit on coming weekend.

Hi @stokarenko
Can you help in resolving conflict and rebase to master ?

Code Climate has analyzed commit 32dc48a and detected 0 issues on this pull request.

View more on Code Climate.

@anilmaurya done! )

also added the next constraint, just in case:

raise LoadError unless Gem::Version.new(::AfterCommitEverywhere::VERSION) >= Gem::Version.new('0.1.5')

Merged.

Thank you @stokarenko for your contribution. I will do a major release 6.0.0 to support this change.

@anilmaurya Hey again )

thinking about the bang method requirement for after_commit:

If you want to make sure a depending action happens only after the transaction is committed, use the after_commit callback along with the auto-save (bang) methods..

This behavior is frustrating a lot I believe, again the callback is silently skipped sometimes. Perhaps that bang method was required due to synthetic nature of transactions support. But now we support them in right way, and can discard bang-method limitation.

For sure that will be a breaking change in fact, but fortunately we are going to a new major release, so - what do you think? )

Hi @stokarenko
Thanks for your suggestion but I think this behaviour is fine.

Most of AASM users comes from Rails background. In Rails after_commit is invoked when record is updated. AASM follows same behaviour to avoid confusion.

@anilmaurya

In Rails after_commit is invoked when record is updated

right, but the sample from readme shows yet another behavior:

Note that the following will not run the after_commit callbacks because the auto-save method is not used:

job = Job.where(state: 'sleeping').first!
job.run
job.save! #notify_about_running_job is not run

AASM follows same behaviour to avoid confusion.

The record has been updated, but AASM's after_commit callback was silently skipped..

Please note that with a help of after_commit_everywhere we are able to fire after_commit exactly as it expected to work - on job.save! evaluation but not on job.run.

Hi @stokarenko

Please note that with a help of after_commit_everywhere we are able to fire after_commit exactly as it expected to work - on job.save! evaluation but not on job.run.

Ideally after_commit should be fired in following cases:

1. job.run!
2. job.run; job.save

and it won't fire on job.run

If this is possible using after_commit_everywhere then I am willing to make this change in 6.0.0

@anilmaurya Roger, will try to prepare PR )

Hi @stokarenko any news about this ? We are stuck with a current scenario:

ActiveRecord::Base.transaction do
   objects.each { |object| object.change_state! }
end

class Object
  aasm requires_new_transaction: false do
    event :change_state do
       after_commit: :send_email
    end 
  end
end

The issue still remain, even specifying the option, we do have child transaction created so the email is sent before the entire collection got changed.

@aovertus This PR should fix your problem. It is in master already but not released as a new AASM version though. Try to point your gemfile to the edge master upstream for the while )

@aovertus on the way try to remove requires_new_transaction: false, just in case.

and for sure you need to add after_commit_everywhere gem manually - https://github.com/aasm/aasm#activerecord

working, thanks a lot @stokarenko