rails icon indicating copy to clipboard operation
rails copied to clipboard

Fix issue with destroy after RestartParentTransaction and SavepointTransaction rollback

Open mguan2020 opened this issue 2 years ago • 13 comments

Motivation / Background

This Pull Request has been created in order to fix issue #50338.

Detail

This Pull Request provides a working solution for the destroy function for the cases of RestartParentTransaction and SavepointTransaction.

Previously, the destroy function was not properly removing entries from the database for the RestartParentTransaction and SavepointTransaction. If @_trigger_destroy_callback is true at the beginning of the destroy function, destroy_row would never be invoked.

This Pull Request will ensure destroy_row is invoked when @_trigger_destroy_callback is true at the beginning of the destroy function. This will remove the corresponding row from the database. Otherwise, if @_trigger_destroy_callback is false, destroy_row is only invoked if persisted? is true.

Additional information

Checklist

Before submitting the PR make sure the following are checked:

  • [x] This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • [x] 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]
  • [x] Tests are added or updated if you fix a bug or add a feature.
  • [x] 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.

mguan2020 avatar Dec 18 '23 00:12 mguan2020

I suggest a fix to your branch, here is the pull request to review: https://github.com/mguan2020/rails/pull/1

rubyrider avatar Dec 19 '23 14:12 rubyrider

@rubyrider Thank you very much for the fix! I have merged your fix into this PR.

mguan2020 avatar Dec 20 '23 19:12 mguan2020

thanks!

rubyrider avatar Dec 20 '23 20:12 rubyrider

@rubyrider I have squashed my changes into a single commit.

mguan2020 avatar Dec 21 '23 05:12 mguan2020

@rubyrider I have squashed my changes into a single commit.

I think keeping that commit would give him co author contributing credit?

auvipy avatar Dec 21 '23 10:12 auvipy

@auvipy thank you for your concern dear, but this is totally fine for me, I don't need a credit 👍 I am here as a volunteer purpose only.

rubyrider avatar Dec 21 '23 11:12 rubyrider

@auvipy Thank you for your idea. I am following the Contributing to Ruby on Rails guide, which states that commits should be squashed for cleaner commit history.

I am really thankful for @rubyrider contributions to this PR and believe he should be listed as a co-author. It is very nice that we worked on this together.

mguan2020 avatar Dec 21 '23 20:12 mguan2020

I notice the build has failed. The reason seems to be from a failed test case in strict_loading_test.rb. I am a bit confused, as the command bundle exec ruby -Itest test/cases/strict_loading_test.rb passes with no errors.

Is there anything I'm missing?

mguan2020 avatar Dec 21 '23 20:12 mguan2020

@mguan2020 i will take a look.

rubyrider avatar Dec 21 '23 22:12 rubyrider

@mguan2020 its passing locally. can you try re-running the test or rebase your branch and update your branch to see if the latest build fixes it?

rubyrider avatar Dec 23 '23 23:12 rubyrider

Thank you @rubyrider for your response. I reran the test and it is also passing locally on my end. I followed your advice of rebasing and updating my branch.

After updating my feature branch and running all the tests in Active Record, I am getting the following output.

Finished in 183.864905s, 47.2031 runs/s, 160.3841 assertions/s. 8679 runs, 29489 assertions, 0 failures, 0 errors, 33 skips

mguan2020 avatar Jan 01 '24 18:01 mguan2020

Could you give me an update on how things are going?

tmhrokmr avatar Feb 15 '24 07:02 tmhrokmr

I'm sorry for asking multiple times. Could you give me an update on how things are going?

tmhrokmr avatar May 10 '24 05:05 tmhrokmr