- Issue created by @tonytheferg
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/...
- last update
6 months ago 129 pass, 7 fail 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.- Status changed to Needs review
6 months ago 3:28am 3 March 2024 - last update
6 months ago 136 pass, 3 fail The last submitted patch, 10: 3424688-10.patch, failed testing. View results →
- 🇮🇱Israel jsacksick
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);
- 🇸🇰Slovakia poker10
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. - First commit to issue fork.
- 🇺🇦Ukraine khiminrm
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.
- 🇺🇦Ukraine khiminrm
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... - 🇮🇱Israel jsacksick
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?
- 🇺🇦Ukraine khiminrm
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.
- last update
5 months ago 138 pass, 3 fail - 🇺🇦Ukraine khiminrm
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.
- 🇺🇦Ukraine khiminrm
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 fieldraw_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.
- last update
5 months ago 138 pass, 4 fail 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.
- 🇺🇦Ukraine khiminrm
@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?
- 🇺🇦Ukraine khiminrm
@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.
- 🇺🇦Ukraine khiminrm
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?
- 🇮🇱Israel jsacksick
I don't really get why there is a need for a new field... Thought that is why original amount was added already?
- 🇮🇱Israel jsacksick
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. - 🇮🇱Israel jsacksick
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?
- Merge request !38Fix shipment amount when using custom alterations with or without promotions. → (Merged) created by khiminrm
- last update
5 months ago 138 pass, 4 fail - last update
5 months ago 138 pass, 4 fail I just wasn't to reiterate #20
This is reproducible without promotions
- 🇺🇦Ukraine khiminrm
@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 commentif ($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! - last update
5 months ago 146 pass, 2 fail - last update
5 months ago 139 pass, 3 fail - last update
5 months ago 133 pass, 3 fail - Open on Drupal.org →Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7last update
5 months ago Not currently mergeable. - last update
5 months ago 150 pass - last update
5 months ago 150 pass - last update
5 months ago 140 pass, 2 fail - last update
5 months ago 150 pass - last update
5 months ago 150 pass - last update
5 months ago 150 pass - last update
5 months ago 150 pass - last update
5 months ago 150 pass - Status changed to Needs review
5 months ago 1:41pm 15 April 2024 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.
- Status changed to Needs work
4 months ago 11:26pm 9 May 2024 - last update
4 months ago 160 pass - last update
4 months ago 160 pass -
jsacksick →
committed 2eb582a7 on 8.x-2.x authored by
khiminrm →
Issue #3424688 by tonytheferg: Rates alterations from ShippingEvents::...
-
jsacksick →
committed 2eb582a7 on 8.x-2.x authored by
khiminrm →
- Status changed to Fixed
4 months ago 8:26am 23 May 2024 - 🇮🇱Israel jsacksick
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.
- 🇺🇸United States rhovland
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!