Mark a shipment as canceled if all the inventory units have been canceled

[BenMorganIO]

Summary

I noticed that if a multi-shipment order goes through and you cancel one of the shipments, an order recalculation will set the shipment as pending/ready. See:

https://github.com/solidusio/solidus/blob/main/core/app/models/spree/shipment.rb#L208

This is because it's only checking if the order is canceled. While this is a correct method to check if a shipment should be marked as canceled, it falls short if it's only 1 shipment in an order that needs to be canceled out of many. If I cancel a shipment, then the shipment will be marked as shipped and that can be quite surprising for customers and admins.

Instead, let's increase the guarantee on canceling a shipment for now by checking if the order is canceled or all of the inventory units for said shipment have been canceled.

I've also updated the logic for checking if the state should be set to shipped. While the check for shipped? remains to ensure immutability, we also mark a shipment as shipped just like in the Spree::OrderCancellations if some of the inventory units are shipped and some are canceled. See:

https://github.com/solidusio/solidus/blob/main/core/app/models/spree/order_cancellations.rb#L121

In the future, I think it would be better that when a shipment is canceled, an inventory unit cancellation should be done on the inventory units it's responsible for. That would help create a guarantee for resolving the bug highlighted above but this is a much larger change. For now, just marking a shipment as canceled when the inventory units have been canceled and matching the existing order cancelation logic is progress in the right direction.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

• I have written a thorough PR description.
• I have kept my commits small and atomic.
• I have used clear, explanatory commit messages.

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.

[codecov]

Codecov Report

Merging #5220 (3bde83e) into main (5f9c960) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5220      +/-   ##
==========================================
- Coverage   88.67%   88.66%   -0.01%     
==========================================
  Files         564      564              
  Lines       13894    13898       +4     
==========================================
+ Hits        12320    12323       +3     
- Misses       1574     1575       +1     

Impacted Files	Coverage Δ
core/app/models/spree/order_cancellations.rb	95.74% <100.00%> (-2.04%)	⬇️
core/app/models/spree/shipment.rb	93.52% <100.00%> (+0.07%)	⬆️

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

[kennyadsl]

Thanks for working on this @BenMorganIO! I tried this locally with @MassimilianoLattanzio, who worked on something similar for one of our stores.

We think this works, but we are not sure about if it's a good thing to consider a shipped shipment as canceled if all of its content has been canceled. E.g. I ship an empty package by mistake, so I will cancel all the items in it. Is it correct to consider that shipment canceled, even if it has been shipped? Yes, there's always the carton that represents the shipped shipment, but maybe in this case it's not clear what actually happened looking at the order's shipment state. What do you think?

Also, I think there was another issue (already there, not related to this PR), because we are updating the shipment date of the shipment after canceling one of the items present in a shipped shipment, so in that case, the shipping date will represent the "all-items cancelation" date.

[BenMorganIO]

If a shipped shipment has all of its items marked as cancelled, it should remain shipped since the shipped state is supposed to be immutable.

[kennyadsl]

But I don't see this happening with your code. Maybe I'm missing something?

[kennyadsl]

@BenMorganIO let me show that with a quick video (I'm using your branch):

Screenshot.2023-07-11.at.10.19.42.mp4

[BenMorganIO]

@kennyadsl I can't understand why someone would cancel all of the items in a shipment after it has been shipped. I think a return would be started if that was the case. However, I want to keep the code so that once a shipment has been shipped, it is effectively immutable and should not transition to canceled.

[BenMorganIO]

@kennyadsl I likely need to add a check here if the shipment is not in a shipped state.

[kennyadsl]

I'm having a look at the state machine of the shipment and it seems like it's not allowed to move a shipment from shipped to canceled already:

solidus/core/lib/spree/core/state_machines/shipment.rb

Line 36 in df68c0c

transition to: :canceled, from: [:pending, :ready]

Do you know if there's a specific reason not to rely on the state machine here? Same for when we set the state to shipped. I think we are also skipping the rest of the callbacks defined in the state machine.

[BenMorganIO]

No, I should be able to use the state machine call. I was mimicking the original code and thought that there must have been a reason to jump the state machine. Although, we should likely use it.

For the code on line 124, should we update that code as well to utilize the state machine?

[kennyadsl]

Yes, I think the best path forward at this point is to make another PR that just changes OrderCancelations to try to use the state machine. If that works and we can't find blockers, changing this PR will be smooth. What do you think?

[kennyadsl]

I can't understand why someone would cancel all of the items in a shipment after it has been shipped. I think a return would be started if that was the case.

You are right; it's hard to find a use case that resonates with this and the most realistic thing I could think of is having two items in the package (e.g., two glasses), both broken. Probably, you don't want to make a return for broken items, and canceling them would work. Do you think it's something worth taking into account?

[BenMorganIO]

@kennyadsl I think if we keep shipped shipments retained as shipped, then it shouldn't be an issue. I can see a customer returning all items from a shipment or an admin canceling the items if the shipment was lost or something. Either way, we do have a comment in the code that the shipped state is supposed to be immutable.