Fix shipment state when all units cancelled#6314
Conversation
We don't need to be modifying the shipment state in our
OrderCancellations object. There are only two current situations where
this code will run:
1. All inventory units are shipped and one or more are cancelled.
-> In this situation, the shipment will already be in a "shipped"
state and there's no reason to change the `shipped_at` column.
2. All inventory units are cancelled.
-> We don't really want to mark shipments as "shipped" in this
situation anyway. So it's better to not do anything here.
Co-authored-by: Alistair Norman <alistair@super.gd>
Co-authored-by: Noah Silvera <noah@super.gd>
If we cancel all inventory units in a shipment, we should mark it as cancelled itself because there's nothing to ship. Co-authored-by: Alistair Norman <alistair@super.gd> Co-authored-by: Noah Silvera <noah@super.gd>
github-actions
bot
added
the
changelog:solidus_core
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6314 +/- ##
=======================================
Coverage ? 89.31%
=======================================
Files ? 959
Lines ? 20110
Branches ? 0
=======================================
Hits ? 17961
Misses ? 2149
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I would love to see a spec that covers that we are calling determine_state on the shipment when the order is cancelled, or even more high-level, that we are setting the state to cancelled when all the items are cancelled. Maybe there's already one?
By the way this is not blocking, approving, thanks!
tvdeyen
left a comment
tvdeyen
left a comment
There was a problem hiding this comment.
I agree with Alberto. If there is a spec verifying the behavior, then fine. Otherwise we should add a spec that ensures what is stated in the first commit message.
|
@adammathys You gonna add a spec here? |
4 participants
Summary
Includes two key changes:
Removes logic in
OrderCancellationsto update shipment state. I tracked this logic back to a cherry-pick from some Bonobos code about a decade ago (c98d247), so the original reasoning for it is unfortunately lost to time. Our commit includes some reasoning around the removal, but essentially I don't think there's any scenario right now where this code makes sense. If we're triggering it because there's a mix of shipped and cancelled items, the shipment should already be "shipped" due to the shipped inventory units. If we're triggering it because all of the items are cancelled, we don't want to mark the shipment as "shipped" anyway.Updates
Shipment#determine_stateto mark shipments as cancelled if all inventory units are cancelled. As mentioned above, marking a shipment as shipped if all of the units are cancelled doesn't make a lot of sense. Since cancelling a unit will result in an order recalculation, we can rely on this method to update the shipment state appropriately.Alternative for: #5220 & #4446
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: