Deactivating a shipping method with fees leads to wrong delivery method and payment status in previous orders

[RachL]

Description

This is something that I have found while following hub 922 in French prod.

This hub has 2 delivery method: pickup and delivery by bike. Delivery by bike has a fixed fee of 6 euros.

A typical week for them goes like this:

• On Tuesday they open their OC with meat products. Everyone has a choice between home delivery and pick up because they have only a few customers buying meat

• On Friday they open their OC with vegetables. A lot more customers are involved, so they open in 2 stages :

1. From 7 am to 1 PM their coop members who can choose between home delivery and pick-up get access thanks to a special OC tag
2. At 1PM they remove the tag on the OC: everyone gets to order. However only the first 40 people will get to be able to select home delivery. So they check delivery report. Once they hit 40 home deliveries they de-activate the home delivery shipping method.

When the week start again, they start this cycle again.

However, when they deactivate the home delivery shipping method, all the previous orders display then the only shipping method available : pick up, with no fees. So all these orders are now showing a credit due to the customer of 6 euros, which is wrong: these customer where delivered by bike, so should NOT be refunded the bike fees.

All reports are wrong too.

Steps to Reproduce

Orders on French prod that are incorrect: # R088866556, # R008087351, # R266862838 ... but maybe many that I have troubles to find 😭

To reproduce on staging:

1. Create 2 shipping method, one with fees, the other without
2. Order something with the shipping method with fees
3. De-activate that shipping method
4. Check the order, see that it is now showing the shipping method with no fees and a credit owned to the customer

Animated Gif/Screenshot

Workaround

I can't find a workaround. For this week, I thought on changing manually all previous order to refund the correct amount. However this is currently a nightmare because :

1. The only report giving me shipping fees and shipping method is OC customer totals, and the line with the fees are only the total line, so I cannot filter on both the fees and the shipping method to get an accurate list of all orders in that case
2. When trying to change the shipping method as a manager, the amount of the fee is not correct: it is showing a 6,33 euros amount, which is the fee + a 5,5 % VAT. But shipping method fees are entered VAT incl. I've opened Adding a shipping method to an order as an admin should be VAT excl.  #5368 for that
3. Even if I fix this, I don't have a workaround for deactivating the delivery on Friday. Tags are not manageable for this. They have 200 customers a week. Or would it work to just had a tag that said this shipping method is invisible for everyone ? And then remove it when needed?
   Even if it would fix it, I cannot chase all my hubs de-activating shipping methods. So I think we need a proper solution here?

Severity

bug-s2: a non-critical feature is broken, no workaround

Your Environment

• Version used: v2.9.7
• Browser name and version: Firefox 76
• Operating System and version (desktop or mobile): Desktop

Possible Fix

De-activated shipping method should stay available for the previous orders or for the manager when editing an order?

[RachL]

Ok it is worth than I thought.... Those orders:

• R600433468
• R214658240
• R145130144

Had a shipping fee with the shipping method that has no fee, in the order customer report yesterday

Today when downloading the report the shipping fee is at 0.

I have yesterday's report thanks to the user. When checking those orders we had a very strange effect, the first time one of us would open the order, the shipping fee was displayed. She zoomed too much out sorry:

When I came to see that order, or even he she went to see the payment menu and then back to the details, she would see the fee is not there anymore... So I'm guessing cache problems?

What's super freaky is that it would mean that there might be quite a lot more orders with the wrong shipping method, and that we have now no ways to find except if we have logs?...

[luisramos0]

The problem is this magical code (inherited from spree) that runs when you load the edit order page.
If you dont load the admin order edit page, the ship method is not updated.

For example:
1- create order with ship method 1
2- deactivate ship method 1
3- activate ship method 1
4 -edit order
The problem "only" occurs if someone loads the order edit page between point 2 and 3 (when ship method is not linked to the distributor).

I tested now, it doesnt need to be a new order, you can select a ship method in any order, if you unlink the ship method from the distributor of the order and load the edit order page, the order ship method will be updated to an active ship method.

BUT, here's the main point, this will only happen if the order was not shipped yet, shipment state 'pending'. If the order is shipped, shipment state "shipped", the inactive ship method will remain assigned to the order.
The logic behind is that the system is "fixing" the situation automatically, the order is not shipped and marked with a ship method that is not available, so, it changes to a ship method that is available.
The conceptual problem is how we read the link between distributor and ship method:

• ship method not available on checkout but still available in the real world to ship orders not shipped yet
  OR
• ship method not available in the real world to ship things, the way the code understands it

Anyway, I think it's obvious we want the code to not touch ship method of an order.
The challenge is that if we do this, we will have unshipped orders with unlinked ship methods. When the user clicks ship on these orders, it will probably blow up because the ship method is not valid... we may need to adapt the code for that case.
I'll have a look at that now.

[luisramos0]

The process is called refresh_shipping_rates and is executed when the order is edited in the backoffice and also when the order customer details are updated.

For now I just removed these 2 calls and I'll see what does the build say: these calls may be supporting other processes/details: #5373

[luisramos0]

In this new PR without this refresh rates process on the edit order page, the order keeps assigned to a ship method not linked to the order distributor. I tested shipping the order in the order list page and it just works 👍

Also, the result of the build is pretty straight forward, no major problems detected.

Only one spec is broken and makes me realize one important point: with this fix, in the same way the unlinked ship method will not be removed from the order, new linked ship methods will not be available to be selected on the edit orders page.
For example, you have one ship method available and the order is placed by the customer, if you link a new ship method to the distributor and want to assign it to the order in the admin edit page, the ship method will not be available there.

To improve this we could keep running the refresh ship rates process but customize it so that it only adds newly available ship methods to the order and keep all the existing ones (the ones available on checkout) irregardless of them being available now or not. This will be a bit complicated to achieve but can be done.
Thoughts?

[RachL]

@luisramos0 I think it would depend how we define what is a new shipping method.

If it is a totally fresh shipping method, it is maybe an edge case to want them on past orders. But not so much to get them when doing a new order in the admin. How does that work? We are talking only about editing here right?

The problem I see is if we consider new shipping method, shipping method that have been re-activated. And I wonder if this is what you meant by "linked" shipping method.
There I think we do want them to be available on any order. But I wonder if fixing this: #4837 would solve this case.

Indeed, I think we have an issue around what de-activating means.For our users it means "not available in the shopfront". But it's not the code definition right? For the code it's a shipping method that is not used anymore?

[luisramos0]

yes, linking and activate is the same: connect a shop with the ship method.

now I realize we have another dimension: "Display"

I need to investigate this. this is probably meant for these cases, remove the ship method from checkout but keep it in the backoffice.

[luisramos0]

Marking a ship method "backoffice only" was not working but this PR makes it work: #5392
I think users can use that field (display on) instead of unlinking/deactivating their ship methods.
Does that make sense as a good workaround @RachL ?

The root cause of this issue is not fixed. I am still wondering if this issue could be considered a feature, not a bug if we understand it as "If a manager edits an order that has not been shipped yet, the shipping methods available for that order will be automatically updated to the the shipping methods currently linked to the order distributor, and if the selected shipping method is not currently linked to the order distributor, it will be changed to a shipping method that is currently linked to the order distributor".

Anyway, I think PR #5392 moves this to S3 at least.

[RachL]

Does that make sense as a good workaround @RachL ?

yes awesome :)

I wonder if we cannot display something like this "do not edit your shipping method with big changes (completely change the name, de-activate or remove fees) if you have orders with this shipping method that has not been shipped yet". On the shipping method list or edit page (or both). Maybe a bit long :D but at least people see it, it's not just an email we send them and then they forget?

[luisramos0]

hmmm, instead of that message I think we should fix the root cause.
I think this issue is still an S3 after this last PR. So I think the best approach is to prioritize this S3 at some point and get the first PR forward: make the order edit page less smart and not remove deactivated ship methods if they are assigned to an order (even if the order has not been shipped yet).

I said it was complicated but it's just one method we need to re-write: Shipment#refresh_rates

[RachL]

yes @luisramos0 but there are a lot of s3 to prioritize... including the one where de-activating is making the method disappear for the user... So at least during that time the user has a warning?

[luisramos0]

ok, I agree to add the message.
I created a new issue for that: #5399
I can create a new PR very easily to address it. Let's agree on its details first.

[luisramos0]

Quick summary:

• PR Make "backoffice only" ship methods work and remove option "frontoffice only" #5392 makes shipping method "Display" option "backoffice only" work again and removes "front end only" (too difficult to fix)
• PR Add warning to enterprise Shipping Method list and to Shipping method edit page #5424 adds a warning message to the Shipping Methods list, and the edit page, recommending the "backoffice only" option instead of de-activating shipping methods (which can have consequences)
• PR Add warning to payment method edit page #5425 adds the same warning for Payment Methods, advising against de-activation and recommending the option "Back office only". It also makes the values of "Display" match the new ones in Shipping methods "Checkout only" instead of "front end". I have tested the "Display" field of Payment methods and they both work: you can hide payment methods from checkout or from orders edit page!

So, with these 3 PRs, I think we can now downgrade this to S3 and move it to All the things, ok @RachL?

[RachL]

@luisramos0 yes perfect! Thank you :)