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

Add dependent: :destroy to Spree::Order#order_promotions #5411

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Oct 3, 2023

Summary

We found a lot of orphaned records in our database due to this missing dependent: :destroy.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

@mamhoff mamhoff requested a review from a team as a code owner October 3, 2023 15:26
@github-actions github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Oct 3, 2023
@mamhoff mamhoff force-pushed the destroy-dependent-order-promotions branch 2 times, most recently from 869ecd2 to 885a62d Compare October 3, 2023 16:39
@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #5411 (9e4a2c9) into main (c0d7d95) will not change coverage.
Report is 3 commits behind head on main.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5411   +/-   ##
=======================================
  Coverage   88.96%   88.96%           
=======================================
  Files         617      617           
  Lines       14876    14876           
=======================================
  Hits        13235    13235           
  Misses       1641     1641           
Files Coverage Δ
core/app/models/spree/order.rb 94.65% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

👍

mamhoff added a commit to friendlycart/solidus_friendly_promotions that referenced this pull request Oct 4, 2023
@mamhoff
Copy link
Contributor Author

mamhoff commented Oct 4, 2023

Should I provide a migration to clear out orphaned records and add a foreign key constraint as well?

We found a lot of orphaned records in our database due to this missing
dependent: :destroy.
@mamhoff mamhoff force-pushed the destroy-dependent-order-promotions branch from 885a62d to b202ce7 Compare October 27, 2023 08:51
@mamhoff
Copy link
Contributor Author

mamhoff commented Oct 27, 2023

Rebased, added foreign key constraint.

@tvdeyen
Copy link
Member

tvdeyen commented Oct 27, 2023

@mamhoff can we ask for a

bundle exec rubocop --fix

?

thanks

This adds a foreign key constraint that makes sure at the database level
that we don't have non-existent order IDs in the spree_orders_promotions
table.
@mamhoff mamhoff force-pushed the destroy-dependent-order-promotions branch from b202ce7 to 9e4a2c9 Compare October 27, 2023 09:36
@mamhoff
Copy link
Contributor Author

mamhoff commented Oct 27, 2023

Done

@tvdeyen tvdeyen merged commit 39b1c92 into solidusio:main Nov 1, 2023
10 checks passed
@mamhoff mamhoff deleted the destroy-dependent-order-promotions branch November 1, 2023 08:58
@elia elia changed the title Add dependent: :destroy to Spree::Order#order_promotions Add dependent: :destroy to Spree::Order#order_promotions Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_core Changes to the solidus_core gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants