Rates alterations from ShippingEvents::SHIPPING_RATES event are not reflected on the order

Finding in my testing that the event rate alterations only apply if I comment out this section in the EarlyOrderProcessor.php:

      if (!$should_refresh) {
        continue;
      }

https://git.drupalcode.org/project/commerce_shipping/-/blob/8.x-2.x/src/...

Curious how much stuff blows up by not checking $should_refresh.

It may be good to invoke the event in the test and make selections here between the two rates, and assert that the order summary reflects those changes:
https://git.drupalcode.org/project/commerce_shipping/-/blob/8.x-2.x/test...

The more I think about this, (and I don't know the codebase that well) I think this needs to be fixed in the LateOrderProcessor. So that the $rate is passed to the $shipment_amount.

https://git.drupalcode.org/project/commerce_shipping/-/blob/8.x-2.x/src/...

Maybe we could set the altered rates in data, so that we could use them, and then unset the data on refresh.

This works... Not sure what might break tho.

The last submitted patch, 10: 3424688-10.patch, failed testing. View results →

Definitely not the right fix... Having the original amount allows us to show a strike-through price in case the amount is lower (that is used by the promotion module for example).
I'd be curious to know if your behavior persists after downgrading to a lower commerce_shipping version. (I'm suspecting this regression was introduced not so long ago).

Yeah.
Well, it's present in 8.x-2.5 which was released in Jan 2023. I didn't chase it any farther back than that due to composer dependancies on my environment.

I think in my use case I can just set the Original amounts in my ES, and it works pretty well.

        $rate_number = $rate->getAmount()->getNumber();
        $new_number = $rate_number + $markup;
        $new_price = new Price($new_number, 'USD');
        $rate->setOriginalAmount($new_price);
        $rate->setAmount($new_price);

We are experiencing this issue as well. We are using the ShippingEvents::SHIPPING_RATES event subscriber to alter one of the shipping rates - to calculate it dynamically via 3rd party API. Just setting $rate->setAmount($new_price); does not work as supposed (the rate is displayed correctly in shipping pane, but not in the order). Workaround in #13 works. I did not have time to track the exact problem down yet.

I've tested custom event subscriber and can confirm, it updates only shipping rates amount, not order total.

I've also tested shipment promotions.
Created 'Fixed amount off the shipment amount' with 2$ amount.
With these settings:

I had such results:

And with these settings:

Such results:

I've checked also that PromotionSubscriber.php, which uses the same 'hippingEvents::SHIPPING_RATES' event. It updates only the amount of the rates and not order total. And it updates only when 'Include the discount in the displayed amount' option is selected in the promotion.

I think there is conflict between shipment promotions and custom event subscriber to the event.
I've found that when commenting these lines https://git.drupalcode.org/project/commerce_shipping/-/commit/90b5400440... the custom rates from the event subscriber works, but when there is shipment promotion and custom event subscriber for changing rates, there is a mess. So maybe we need more conditions in the EarlyOrderProcessor. But if there is custom event subscriber and it alters a rate, maybe it should be in higher priority than promoted rate? Trying to find solution...

Can you check by any chance if the patch from 🐛 Duplicate promotion adjustments added to order items because of the promotion subscriber Needs review has an impact?

@jsacksick I've tested the patch. didn't notice changes.

This is reproducible without promotions.

I've found that we need to set shipment amount in EarlyOrderProcessor only if the shipment has adjustments and we should calculate this as rate amount + any discounts. I've tested only with fixed amount off promotion, without any promotion and it looks like it works. When there is some discount it's calculated from the altered rate amount. So in custom event subscriber can contain only $rate->setAmount() without $rate->setOriginalAmount().

Attaching a quick patch I've created and tested along with the patch from https://www.drupal.org/project/commerce_shipping/issues/3413473 🐛 Duplicate promotion adjustments added to order items because of the promotion subscriber Needs review . Haven't tested without it yet.

Looks like we can't use adjustments in EarlyOrderProcessor to calculate the shipment amount with discounts as on this step we can have wrong adjustments.

Another solution is to use new additional field in shipment and property in rate to save the shipment amount just before applying promotions in PromotionSubscriber and re-use this value in EarlyOrderProcessor.
I've called this field raw_amount. Not best name, I think, but just for testing, reviewing should work.
So it should be amount value after all rate alterations and just before PromotionSubscriber.

Attaching a patch, but it includes also fixes from https://www.drupal.org/project/commerce_shipping/issues/3413473 🐛 Duplicate promotion adjustments added to order items because of the promotion subscriber Needs review . Can try to remove those fixes and re-test and attach patch without the fixes.

Well if another amount field is considered, we should include that field in the form display so that users can manually specify an amount through the UI.

@tonytheferg not sure about the case when we should use it on the form. This is just to save final altered value just before applying the promotions and make this all work with custom event subscriber where we alter the rate amount by using $rate->setAmount() or/and when the promotions are used. Can you share your ideas why we would we need those field on the form?

@tonytheferg thanks! I will check those issue, but maybe tomorrow.

No problem. See Ryan's quote in the IS linked.

I think we should think about that and account for those thoughts.

A third amount field seems like a bit much IMO.

I agree regarding third field. I don't like this idea too, but I don't see other solution how to save the latest altered amount without promotions applied. I couldn't find a way how to calculate this amount in EarlyOrderProcessor.

@jsacksick what do you think? What else we can try to do there?

I don't really get why there is a need for a new field... Thought that is why original amount was added already?

So the patch from #23 has changes from the other issue I linked in comment #18.
I don't think introducing another persistent field with a confusing name is going to help. If all we need to do is store the amount before alterations (which was originally the intent of "original_amount", then we can/should use the shipment data for that.
We would set the value from the early order processor and unset it from the late order processor or something.
This would simplify the code and wouldn't require adding additional getters/setters etc.

The reason why the amount is "reset" from the early order processor is that it represents the unaltered shipping rate amount, prior to invoking event subscribers and prior to applying promotions.

In case we don't reset it, then the subscribers are going to alter an already altered amount. So in case there is a price markup applied from a subscriber, we'll keep applying the markup on each refresh indefinitely?

I just wasn't to reiterate #20

This is reproducible without promotions

@tonytheferg yes, I know about it. But as I wrote in https://www.drupal.org/project/commerce_shipping/issues/3424688#comment-... 🐛 Rates alterations from ShippingEvents::SHIPPING_RATES event are not reflected on the order Needs work those change which was done for promotion fixes, breaks calculations when custom subscriber is used to alter the rates.
You can try just to comment

      if ($original_amount = $shipment->getOriginalAmount()) {
        $shipment->setAmount($original_amount);
      }

in EarlyOrderProcessor and re-test.
And if you can, please, test also my solution from the MR. Thanks!

khiminrm → closed merge request !39

@tonytheferg: Did you get a chance to test the MR?

I can try to have a look today.

With brief manual testing, the patch seems to fix the behavior reported in the issue. I don't understand how promotions were breaking this, but If you guys are confident this is the fix, then I say +1

I think we definitely need to make sure that the test is checking for an altered rate from an event subscriber, and accounting for a couple of selections on the checkout pane.

This bug was not caught before because there wasn't sufficient testing in to test that the event subscriber rate was applied both to the checkout Pane and also to the order summary

So I would highly recommend adding that test to make sure that this doesn't get broken later down the road.

Setting to needs work for more explicit functional tests.

Since you've confirmed the fix and I'm about to tag a new release, went ahead and merged the MR. We can expand the tests in a followup issue, in the meantime, closing this one.

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

Interesting. I encountered this during development of the best rate shipping module and got around it by setting both the amount and the original amount. Didn't know it was actually a bug!