Problem/Motivation

When using the ShippingEvents::SHIPPING_RATES event, rates are altered, but those alterations are not reflected in the order.

This means any discounts or markups are shown in the shipping pane, but are not included in the shipping price.
I set this to major, not to be dramatic, but because sites that use the event to mark up rates may have orders being placed with the wrong shipping costs.

Steps to reproduce

  • Add a ShippingEvents::SHIPPING_RATES event subscriber with a custom module.
  • Add two flat rate options through the UI.
  • Alter both rates with your event subscriber.

Result:
When the checkout pane is loaded, the default selected rate is correct, because calculateRates was called, and the rate was applied to the shipment. When you change to the second rate, the rates are correct in the shipping information pane, but the check for $should_refresh prevents the EarlyOrderProcessor from applying the altered rates to the order.

Proposed resolution

Not sure. commenting out:

      if (!$should_refresh) {
        continue;
      }

Allows the altered rates from the event subscriber to be applied, but also causes the rates to be collected, which causes needless requests and responses from carriers.

We should apply the rates upon every selection, but we certainly don't need to recalculate the rates every time a selection is made.

Remaining tasks

Verify the bug through testing, and find a way to reflect the new rates on the shipment.

CommentFileSizeAuthor
#23 3424688-23.patch13.86 KBkhiminrm
#21 3424688-21.patch939 byteskhiminrm
#16 Include the discount in the displayed amount - result.png39.79 KBkhiminrm
#16 Include the discount in the displayed amount.png47.25 KBkhiminrm
#16 Only show the discount on the order total summary - result.png33.61 KBkhiminrm
#16 Only show the discount on the order total summary.png47.16 KBkhiminrm
#10 3424688-10.patch428 bytestonytheferg
#6 3424688-6.patch540 bytestonytheferg

Issue fork commerce_shipping-3424688

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

tonytheferg created an issue. See original summary.

tonytheferg’s picture

tonytheferg commented

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

tonytheferg’s picture

tonytheferg commented
Issue summary: View changes
tonytheferg’s picture

tonytheferg commented
Issue summary: View changes
tonytheferg’s picture

tonytheferg commented
Issue summary: View changes
tonytheferg’s picture

tonytheferg commented
StatusFileSize
new 3424688-6.patch540 bytes

Curious how much stuff blows up by not continuing if !$should_refresh. 🤣

tonytheferg’s picture

tonytheferg commented

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 event is tested here, but there is no verification that the altered rate is applied to the order after selecting between the two altered rates:
https://git.drupalcode.org/project/commerce_shipping/-/blob/8.x-2.x/test...

tonytheferg’s picture

tonytheferg commented

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

tonytheferg’s picture

tonytheferg commented

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

tonytheferg’s picture

tonytheferg commented
Status: Active » Needs review
StatusFileSize
new 3424688-10.patch428 bytes

This works... though I realize it may not be desirable to set the original rate as the altered rate...

I think it shows that the original rate is getting passed to the shipment when there is no refresh while selecting new rates.

One work around might be to just $rate->setOriginalAmount($new_price); in my event subscriber instead of $rate->setAmount($new_price);

Status: Needs review » Needs work

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

jsacksick’s picture

jsacksick commented

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

tonytheferg’s picture

tonytheferg commented

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 addition to setting the Amount in my Event Subscriber.

$rate->setOriginalAmount($new_price);
$rate->setAmount($new_price);

This doesn't trigger any unnecessary recalculations, nor does it hinder shipping promotions from being applied, so it works pretty well.
I think the issue should stay open until someone has time to track it down.

poker10’s picture

poker10 commented

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.

tBKoT made their first commit to this issue’s fork.

khiminrm’s picture

khiminrm commented
StatusFileSize
new Only show the discount on the order total summary.png47.16 KB
new Only show the discount on the order total summary - result.png33.61 KB
new Include the discount in the displayed amount.png47.25 KB
new Include the discount in the displayed amount - result.png39.79 KB

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:

Only show the discount on the order total summary

I had such results:

Only show the discount on the order total summary - result

And with these settings:

Include the discount in the displayed amount

Such results:

Include the discount in the displayed amount - result

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

khiminrm’s picture

khiminrm commented

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 review changes made for https://www.drupal.org/project/commerce_shipping/issues/3107367 and add 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...

jsacksick’s picture

jsacksick commented

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

khiminrm’s picture

khiminrm commented

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

tonytheferg’s picture

tonytheferg commented

This is reproducible without promotions.

khiminrm’s picture

khiminrm commented
StatusFileSize
new 3424688-21.patch939 bytes

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. Haven't tested without it yet.

khiminrm’s picture

khiminrm commented

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.

khiminrm’s picture

khiminrm commented
StatusFileSize
new 3424688-23.patch13.86 KB

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. Can try to remove those fixes and re-test and attach patch without the fixes.

tonytheferg’s picture

tonytheferg commented

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.

See:
https://www.drupal.org/project/commerce_shipping/issues/3420292

And related issues

khiminrm’s picture

khiminrm commented

@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?

khiminrm’s picture

khiminrm commented

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

tonytheferg’s picture

tonytheferg commented

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 seeing as we have original_amount, and amount already.

khiminrm’s picture

khiminrm commented

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?

jsacksick’s picture

jsacksick commented

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

jsacksick’s picture

jsacksick commented

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.

jsacksick’s picture

jsacksick commented

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?

khiminrm opened merge request !38

tonytheferg’s picture

tonytheferg commented

I just wan't to reiterate #20

This is reproducible without promotions

The steps to reproduce in OP have no promotions applied.

khiminrm’s picture

khiminrm commented

@tonytheferg yes, I know about it. But as I wrote in https://www.drupal.org/project/commerce_shipping/issues/3424688#comment-... 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 opened merge request !39

khiminrm closed merge request !39

khiminrm’s picture

khiminrm commented
Status: Needs work » Needs review
jsacksick’s picture

jsacksick commented

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

tonytheferg’s picture

tonytheferg commented

I can try to have a look today.

tonytheferg’s picture

tonytheferg commented

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

tonytheferg’s picture

tonytheferg commented

I think we definitely need to make sure that the test is checking for an altered rate from an event subscriber, and also accounting for a couple of selections being made 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 a Functional Javascript test to make sure that this doesn't get broken later down the road.

tonytheferg’s picture

tonytheferg commented
Status: Needs review » Needs work

Setting to needs work for more explicit functional tests.

  • jsacksick committed 2eb582a7 on 8.x-2.x authored by khiminrm 
    Issue #3424688 by tonytheferg: Rates alterations from ShippingEvents::...
jsacksick’s picture

jsacksick commented
Status: Needs work » Fixed

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.

Status: Fixed » Closed (fixed)

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

rhovland’s picture

rhovland
he/him
Primary language English
Location Oregon
commented

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!