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.

Sign up for GitHub

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

Jump to bottom

Fix [#50260] Support :on option in #set_callback #50261

Merged
merged 1 commit into from Jan 12, 2024
Merged

Fix [#50260] Support :on option in #set_callback #50261

merged 1 commit into from Jan 12, 2024

Conversation

joshuay03
Copy link
Contributor

@joshuay03 joshuay03 commented Dec 4, 2023 •
edited

Motivation / Background

Attempts to fix #50260

Detail

Introduces ActiveRecord::Transactions::ClassMethods#set_callback which behaves like ActiveSupport::Callbacks::ClassMethods#set_callback with support for the :on option available on #after_commit and #after_rollback callbacks.

Additional information

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@joshuay03 joshuay03 mentioned this pull request Dec 4, 2023
Closed
@joshuay03 joshuay03 force-pushed the activerecord-transactions-set-callback branch from ce8ebe5 to 41da50f Compare December 4, 2023 12:52
joshuay03
joshuay03 commented Dec 4, 2023
View reviewed changes
activerecord/test/cases/transaction_callbacks_test.rb
topic.update!(title: "Updated topic 3")
expected_history << :after_commit_on_update_2
expected_history << :after_commit_on_update_1
assert_equal expected_history, TopicWithCallbacksOnUpdate.history
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before patch:

Screenshot 2023-12-04 at 8 54 21 pm

@joshuay03 joshuay03 force-pushed the activerecord-transactions-set-callback branch 2 times, most recently from 6085f71 to bfccf88 Compare December 20, 2023 07:22
@byroot byroot force-pushed the activerecord-transactions-set-callback branch from bfccf88 to 93f0687 Compare January 12, 2024 11:30
@byroot byroot merged commit a255742 into rails:main Jan 12, 2024
3 of 4 checks passed
@joshuay03 joshuay03 deleted the activerecord-transactions-set-callback branch January 12, 2024 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
activerecord
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

#set_callback does not support :on option
2 participants
@joshuay03 @byroot