Non draft orders are refreshed when editing shipments

Updated patch for latest version of module

Setting needs review for maintainer feedback on the general points.

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

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...

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).

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?

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).

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.

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.

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

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.