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 foreign key constraint between order_promotions and promotions #5469

Merged

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Oct 31, 2023

Summary

When we delete a promotion, the associated order_promotions should also disappear.

Checklist

@mamhoff mamhoff requested a review from a team as a code owner October 31, 2023 17:55
@github-actions github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Oct 31, 2023
Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

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

Couldn't this cause issues with historical orders?

@mamhoff
Copy link
Contributor Author

mamhoff commented Nov 2, 2023

Couldn't this cause issues with historical orders?

Solidus' promotion system won't recalculate promotion adjustments after orders are completed. So adjustments for historical orders would not change. One would not be able to trace why a certain promotion applied to an order, but once you delete the promotion, chances are you don't really care?

Currently the state of deletion in the promotion world is generally weird: PromotionAction records are soft-deleted, but promotions and promotion rules are not.

This is just to stop garbage data from being around - because what good is it to know I have an order_promotion record where the promotion in question has been hard deleted. If the order_promotion record contained a promotion code id, that's also gone.

Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

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

Right, that makes sense.

When we delete a promotion, the associated `order_promotions` should
also disappear.
@mamhoff mamhoff force-pushed the add-order-promotions-dependent-destroy branch from 37ee2d0 to f2649f7 Compare November 3, 2023 11:13
Copy link

codecov bot commented Nov 3, 2023

Codecov Report

Merging #5469 (f2649f7) into main (00031a2) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5469   +/-   ##
=======================================
  Coverage   88.93%   88.93%           
=======================================
  Files         622      622           
  Lines       14909    14909           
=======================================
  Hits        13259    13259           
  Misses       1650     1650           
Files Coverage Δ
core/app/models/spree/promotion.rb 97.58% <100.00%> (ø)

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@tvdeyen tvdeyen merged commit a8c1d8a into solidusio:main Nov 3, 2023
4 checks passed
@elia elia changed the title Add foreign key constraint between order_promotions and promotions Add foreign key constraint between order_promotions and 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.

4 participants