Fix after commit within nested transaction vol2 #668

stokarenko · 2020-03-05T05:08:32Z

It brings #666 back, but without explicit after_commit_action dependency.

kitop · 2020-03-05T05:47:39Z

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

Impacted Files	Coverage Δ
lib/aasm/core/state.rb	`89.79% <0.00%> (-2.05%)`	⬇️

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-io · 2020-03-05T05:47:43Z

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

Impacted Files	Coverage Δ
lib/aasm/persistence/orm.rb	`91.04% <100%> (+0.27%)`	⬆️
lib/aasm/persistence/active_record_persistence.rb	`93.15% <80%> (-0.97%)`	⬇️
lib/aasm/core/state.rb	`89.79% <0%> (-2.05%)`	⬇️

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.

kitop · 2020-03-05T05:48:03Z

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

Impacted Files	Coverage Δ
lib/aasm/persistence/active_record_persistence.rb	`93.15% <80.00%> (-0.97%)`	⬇️
lib/aasm/persistence/orm.rb	`91.04% <100.00%> (+0.27%)`	⬆️

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.

louim · 2020-03-09T14:15:09Z

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?

stokarenko · 2020-03-09T14:31:45Z

@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 )

stokarenko · 2020-03-09T14:59:41Z

@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..

stokarenko · 2020-03-09T15:00:59Z

@anilmaurya could you review this PR please? )

louim · 2020-03-09T15:25:25Z

@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-io · 2020-03-09T15:32:52Z

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

Impacted Files	Coverage Δ
lib/aasm/persistence/orm.rb	`91.04% <100%> (+0.27%)`	⬆️
lib/aasm/persistence/active_record_persistence.rb	`93.15% <80%> (-0.97%)`	⬇️

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.

kitop · 2020-03-09T15:33:00Z

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

Impacted Files	Coverage Δ
lib/aasm/persistence/active_record_persistence.rb	`93.15% <80.00%> (-0.97%)`	⬇️
lib/aasm/persistence/orm.rb	`91.04% <100.00%> (+0.27%)`	⬆️

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.

kitop · 2020-03-09T15:33:03Z

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

Impacted Files	Coverage Δ
lib/aasm/persistence/active_record_persistence.rb	`93.15% <80.00%> (-0.97%)`	⬇️
lib/aasm/persistence/orm.rb	`91.04% <100.00%> (+0.27%)`	⬆️

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-io · 2020-03-09T15:33:05Z

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

Impacted Files	Coverage Δ
lib/aasm/persistence/orm.rb	`91.04% <100%> (+0.27%)`	⬆️
lib/aasm/persistence/active_record_persistence.rb	`93.15% <80%> (-0.97%)`	⬇️

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.

kitop · 2020-03-09T15:33:25Z

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

Impacted Files	Coverage Δ
lib/aasm/persistence/active_record_persistence.rb	`93.15% <80.00%> (-0.97%)`	⬇️
lib/aasm/persistence/orm.rb	`91.04% <100.00%> (+0.27%)`	⬆️

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.

anilmaurya · 2020-03-22T03:19:20Z

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

stokarenko · 2020-03-22T07:25:53Z

@anilmaurya @louim

Roger! )

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

stokarenko · 2020-03-22T16:33:02Z

@anilmaurya @louim

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

stokarenko · 2020-03-23T15:00:34Z

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

stokarenko · 2020-05-06T10:26:25Z

@anilmaurya could you revisit please? )

anilmaurya · 2020-05-06T10:28:40Z

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

anilmaurya · 2020-05-10T06:17:30Z

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

This reverts commit 59ebeca.

codeclimate · 2020-05-12T11:22:32Z

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

View more on Code Climate.

stokarenko · 2020-05-12T11:33:05Z

@anilmaurya done! )

also added the next constraint, just in case:

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

anilmaurya · 2020-05-12T17:43:11Z

Merged.

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

stokarenko · 2020-05-13T11:35:30Z

@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? )

anilmaurya · 2020-05-14T02:45:29Z

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.

stokarenko · 2020-05-14T03:59:46Z

@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.

anilmaurya · 2020-05-14T06:15:14Z

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:

job.run!
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

stokarenko · 2020-05-14T10:06:54Z

@anilmaurya Roger, will try to prepare PR )

aovertus · 2020-06-19T14:15:31Z

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.

stokarenko · 2020-06-19T16:06:50Z

@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 )

stokarenko · 2020-06-19T16:10:54Z

@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

aovertus · 2020-06-19T20:15:29Z

working, thanks a lot @stokarenko

stokarenko force-pushed the fix-after-commit-within-nested-transaction-vol2 branch from bab3796 to 84171e2 Compare March 5, 2020 05:36

stokarenko mentioned this pull request Mar 5, 2020

Fix :after_commit within nested transaction #666

Merged

stokarenko force-pushed the fix-after-commit-within-nested-transaction-vol2 branch from 84171e2 to 55a01ec Compare March 9, 2020 15:21

stokarenko mentioned this pull request Mar 13, 2020

after_commit does not work as expected in nested transactions #536

Closed

stokarenko mentioned this pull request Mar 22, 2020

Fix callback names for aliased DSL methods Envek/after_commit_everywhere#8

Merged

stokarenko added 4 commits May 12, 2020 14:06

Revert "Revert "Fix :after_commit within nested transaction""

838428c

This reverts commit 59ebeca.

Remove explicit after_commit_action dependency

19406b2

Use after_commit_everywhere instead of after_commit_action

86920c6

Check the version of after-commit-everywhere gem

32dc48a

stokarenko force-pushed the fix-after-commit-within-nested-transaction-vol2 branch from 5a33ba7 to 32dc48a Compare May 12, 2020 11:22

anilmaurya merged commit 32dc48a into aasm:master May 12, 2020

Envek mentioned this pull request Dec 27, 2021

after_commit deprecation - what is actually deprecated? #761

Closed

Envek mentioned this pull request Mar 10, 2022

Delay job push within DB transaction sidekiq/sidekiq#5239

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix after commit within nested transaction vol2 #668

Fix after commit within nested transaction vol2 #668

stokarenko commented Mar 5, 2020

kitop commented Mar 5, 2020

codecov-io commented Mar 5, 2020 •

edited

kitop commented Mar 5, 2020 •

edited

louim commented Mar 9, 2020

stokarenko commented Mar 9, 2020

stokarenko commented Mar 9, 2020 •

edited

stokarenko commented Mar 9, 2020

louim commented Mar 9, 2020

codecov-io commented Mar 9, 2020 •

edited

kitop commented Mar 9, 2020

kitop commented Mar 9, 2020

codecov-io commented Mar 9, 2020 •

edited

kitop commented Mar 9, 2020 •

edited

anilmaurya commented Mar 22, 2020

stokarenko commented Mar 22, 2020

stokarenko commented Mar 22, 2020

stokarenko commented Mar 23, 2020

stokarenko commented May 6, 2020

anilmaurya commented May 6, 2020

anilmaurya commented May 10, 2020

codeclimate bot commented May 12, 2020

stokarenko commented May 12, 2020

anilmaurya commented May 12, 2020

stokarenko commented May 13, 2020 •

edited

anilmaurya commented May 14, 2020

stokarenko commented May 14, 2020 •

edited

anilmaurya commented May 14, 2020

stokarenko commented May 14, 2020

aovertus commented Jun 19, 2020

stokarenko commented Jun 19, 2020

stokarenko commented Jun 19, 2020

aovertus commented Jun 19, 2020

Fix after commit within nested transaction vol2 #668

Fix after commit within nested transaction vol2 #668

Conversation

stokarenko commented Mar 5, 2020

kitop commented Mar 5, 2020

Codecov Report

codecov-io commented Mar 5, 2020 • edited

Codecov Report

kitop commented Mar 5, 2020 • edited

Codecov Report

louim commented Mar 9, 2020

stokarenko commented Mar 9, 2020

stokarenko commented Mar 9, 2020 • edited

stokarenko commented Mar 9, 2020

louim commented Mar 9, 2020

codecov-io commented Mar 9, 2020 • edited

Codecov Report

kitop commented Mar 9, 2020

Codecov Report

kitop commented Mar 9, 2020

Codecov Report

codecov-io commented Mar 9, 2020 • edited

Codecov Report

kitop commented Mar 9, 2020 • edited

Codecov Report

anilmaurya commented Mar 22, 2020

stokarenko commented Mar 22, 2020

stokarenko commented Mar 22, 2020

stokarenko commented Mar 23, 2020

stokarenko commented May 6, 2020

anilmaurya commented May 6, 2020

anilmaurya commented May 10, 2020

codeclimate bot commented May 12, 2020

stokarenko commented May 12, 2020

anilmaurya commented May 12, 2020

stokarenko commented May 13, 2020 • edited

anilmaurya commented May 14, 2020

stokarenko commented May 14, 2020 • edited

anilmaurya commented May 14, 2020

stokarenko commented May 14, 2020

aovertus commented Jun 19, 2020

stokarenko commented Jun 19, 2020

stokarenko commented Jun 19, 2020

aovertus commented Jun 19, 2020

codecov-io commented Mar 5, 2020 •

edited

kitop commented Mar 5, 2020 •

edited

stokarenko commented Mar 9, 2020 •

edited

codecov-io commented Mar 9, 2020 •

edited

codecov-io commented Mar 9, 2020 •

edited

kitop commented Mar 9, 2020 •

edited

stokarenko commented May 13, 2020 •

edited

stokarenko commented May 14, 2020 •

edited