Problem/Motivation

Whenever a shipment is added, edited, or removed the order is refreshed
https://git.drupalcode.org/project/commerce_shipping/-/blob/8.x-2.x/src/...

While this is fine for draft orders and carts it can lead to data loss when the order is refreshed for placed orders. Placed orders should not change unless expressly done by an administrator. The order refresh causes line item prices to be updated, promotions to be reevaluated, taxes recalculated.

When shipments are changed there are three things that do need to happen on placed orders:

  1. The shipping adjustment total on the order needs to be updated to reflect the total cost of the shipments
  2. The shipping promotions need to be recalculated. Other promotions need to remain untouched.
  3. The tax for shipping, if applicable, needs to be recalculated and an event fired for any tax integrations to subscribe to

Steps to reproduce

Add, edit, or delete a shipment using the Shipments tab UI in the order administration area after an order has already been placed.

Proposed resolution

Trigger a shipping specific order refresh on non draft orders.

Possible ways of doing this:
Call the provided functions that commerce_shipping defines for the order refresh to only refresh shipping specific parts of the order. Probably the wrong way.

Provide a shipment refresh service that handles the refresh similar to the core one but skips anything that is not related to shipping.

CommentFileSizeAuthor
#4 3328162-non-draft-orders-are-refreshed-4.patch1.82 KBrhovland
#3 3328162-non-draft-orders-are-refreshed-3.patch1.85 KBrhovland

Issue fork commerce_shipping-3328162

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

rhovland created an issue. See original summary.

rhovland’s picture

rhovland
he/him
Primary language English
Location Oregon
commented
Related issues: +#2875804: Order Refresh should work for non-draft orders, too
rhovland’s picture

rhovland
he/him
Primary language English
Location Oregon
commented
StatusFileSize
new 3328162-non-draft-orders-are-refreshed-3.patch1.85 KB

Here's a patch that disables refresh unless it's a draft order and does the bare minimum: updates the shipping adjustments after shipment save.
As I argued in the related issue, non draft order edits should be in control of the administrative user and doing things that change the order in meaningful ways (taxes, promotions, etc) should be controlled by the admin user not an automated process.

If a store wants to honor the promotions they gave the customer at time of purchase or recalculate them should be up to the individual store.

Taxes are recalculated and records adjusted once the admin user is done making changes to the order. An automated refresh cannot predict when a good time to do that is.

rhovland’s picture

rhovland
he/him
Primary language English
Location Oregon
commented
StatusFileSize
new 3328162-non-draft-orders-are-refreshed-4.patch1.82 KB

Updated patch for latest version of module

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Active » Needs review

Setting needs review for maintainer feedback on the general points.

zaporylie’s picture

zaporylie
he/him
Primary language Polish
commented
Priority: Major » Critical

I decided to bump the severity to Critical as items can also be deleted from the cart if one of the availability checkers returns that the item is not available - see AvailabilityOrderProcessor => AvailabilityManager

jsacksick’s picture

jsacksick commented
Status: Needs review » Needs work

Definitely bad that the order is refreshed regardless of the order state. I don't believe there is a need to actually check the order state. The order is always refreshed on save if it's a draft order, so setting the refresh state shouldn't even be necessary, but I guess this makes it explicit and make the intent clearer.

The new approach should work in most cases, however, not in all cases. The LateOrderProcessor which is normally invoked on order refresh transfers shipment adjustments to the order.

So with this change, the shipment tax isn't properly recalculated / reapplied for example. I'm thinking we might need to invoke this logic ourselves...
What we need to do is remove all prior shipping tax adjustments added to the order by the shipping tax type, and then somehow reapply them.

Any other custom logic applying shipment adjustments is not going to be invoked...But, this would put us in a much better situation than the one we're currently in...

jsacksick’s picture

jsacksick commented

We're likely missing an equilavent of the OrderRefresh for shipments, a ShipmentRefresh service, but it's not that simple as the shipment affects the order.

The problem with the approach described in my previous comment is that it lives in the form and doesn't address adjustments made to the shipments over the API for example (Programatic updates, or updates via JSONAPI endpoints for example).

jsacksick’s picture

jsacksick commented

Not sure what to do regarding taxes. We could check if there are tax types using the shipping tax type plugin then remove any prior shipping tax adjustments added to the order.
But that assumes the shipping tax is applied by the shpping local tax type plugin, if a service is used for tax calculation, it wouldn't be invoked.

And in other cases, a fee could be added to the shipping which potentially needs a recalculation as well?

So I don't really know whether I should add code to perform a tax recalculation for the case where there were prior tax adjustments added by the Shipping tax type plugin or not? It probably is better than not doing any recalculation?

zaporylie’s picture

zaporylie
he/him
Primary language Polish
commented

I think it's really convenient, from the OMS point of view, to be able to create shipments and have the order updated accordingly. Having to manually add adjustments would be very inconvenient. Having price adjustments automatically set, but tax adjustments not, could somehow be worse, as it would give a false belief that everything is happening automatically until you realize taxes are not there.

IMHO, this issue could potentially benefit from a change to the OrderRefresh flow that would allow running a subset of order pre-/processors on placed orders by introducing order refresh scopes. Scopes would be defined as tagged service attributes and if missing, in the current release cycle we can assume they are only meant to run for draft orders. However, we could provide scopes for placed or even completed orders and run a curated subset of processors that we know can be run deterministically, and the one copying shipping adjustment is a prime example of such a processor. Tax processor could potentially also be safe to run if we resolve taxes for a specific date (current vs placed).

jsacksick’s picture

jsacksick commented

IMHO, this issue could potentially benefit from a change to the OrderRefresh flow that would allow running a subset of order pre-/processors on placed orders by introducing order refresh scopes. Scopes would be defined as tagged service attributes and if missing, in the current release cycle we can assume they are only meant to run for draft orders. However, we could provide scopes for placed or even completed orders and run a curated subset of processors that we know can be run deterministically, and the one copying shipping adjustment is a prime example of such a processor. Tax processor could potentially also be safe to run if we resolve taxes for a specific date (current vs placed).

I don't think it's as simple as that. Processors are kinda dependent indirectly on each other... It's expected that taxes are calculated post promotions for example.

Promotions cannot be reapplied on a placed order, but if quantities were adjusted, promotions should in a way, be recalculated as there would be a risk in bringing the total further down than expected. There are more examples, but it's a very tricky subject.

jsacksick’s picture

jsacksick commented

We've decided internally to simply stop refreshing non draft orders for now, without trying to mess with the adjustments in the case the order isn't a draft, just to address this critical bug report until we address this properly.

With that said, it could be that the tests are broken by the change now, let's see.

jsacksick opened merge request !72

jsacksick’s picture

jsacksick commented

Created a change record: https://www.drupal.org/node/3545609

  • jsacksick committed 7178582f on 8.x-2.x 
    Issue #3328162: Stop refreshing non draft orders when saving shipments.
jsacksick’s picture

jsacksick commented
Status: Needs work » Fixed

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.