While debugging an issue with the free shipping discount not properly applied when the discount is associated with a coupon, I realized our current implementation works in a really specific scenario.
First of all, these offers don't react to the same event as other regular order discounts ("commerce_discount_order") which is fired inside the hook_commerce_cart_order_refresh().
Shipping discount offers are only evaluated when the shipping rates are collected, as opposed to each cart refresh for "regular" discounts.
Once the discount is applied, if you disable the rule, of if for some reasons the conditions are not met (discount date expired or any other reason), the shipping discount will stay forever, unless you happen to go back to the page that contains the shipping service selection pane.
Additionally, the discount is not tracked by a separate commerce_discount line item, and no discount price component will be added (instead, a negative amount is added to the shipping line item), causing the discount not to be properly detected by the compatibility code...
Furthermore, the rules action for applying the shipping discount relies on an ephemeral property added by Commerce Shipping ($order->shipping_rates), the reason for that is that the shipping line item is only created on actual checkout form submission...
So it isn't available when evaluated by the action.
If we update the action to check the shipping line item instead, then the discount won't be reflected until you actually submit the checkout form (so, it won't be properly reflected in the shipping options)....
So maybe we should first check $order->shipping_rates if available, & if not, check if a shipping line item exists?
I suggest we update the current handling to match the regular discounts, but that means drastic changes, and possibly introducing regressions.
Finally, in a scenario like mine where you associate shipping discounts with a coupon code.
If the coupon pane comes after the shipping service selection then the discount won't be applied unless you go back to the shipping service selection page...
| Comment | File | Size | Author |
|---|---|---|---|
| #22 | commerce_discount-2910160-free_shipping-22.patch | 26.74 KB | czigor |
| #21 | commerce_discount-2910160-free_shipping_FAIL-21.patch | 26.68 KB | czigor |
| #20 | interdiff-2910160-19-20.txt | 579 bytes | jsacksick |
| #20 | commerce_discount-2910160-free_shipping-20.patch | 25.47 KB | jsacksick |
| #19 | interdiff-2910160-16-19.txt | 4.49 KB | jsacksick |
Comments
Comment #2
jsacksick commentedComment #3
joelpittetThis sounds like something that could be shown through a test, tagging. Thanks for digging into this @jsacksick
Comment #4
czigor commentedThese are the tests that should pass for the "Free shipping" offer. (They will not.)
Comment #6
czigor commentedClean up a bit.
Comment #8
czigor commentedTalked this over with @jsacksick in Vienna. Adding a patch with tests and the fix for the free shipping discount. The other two discount types for shipping are also coming.
Comment #9
czigor commentedThe patch is already big as it is. Let's focus on free shipping here and once we nail it, it's easy to add percentage discount in a followup issue.
Comment #11
czigor commentedFix coding standards.
Comment #13
czigor commentedMeh, one more try.
Comment #14
czigor commentedComment #15
czigor commentedRe-adding the percentage off rules event invocation, removing the free shipping rules event and fixing some comments.
Comment #16
czigor commentedBy @jsacksick's suggestion let's call commerce_discount_commerce_cart_order_refresh() directly instead of commerce_cart_order_refresh() to improve performance when displaying shipping options on checkout.
Comment #17
jsacksick commented@czigor: I just spent some time testing it locally.
On the shipping service selection pane, the discount was applied twice leading to a wrong price displayed (two discount price components present).
In my usecase, I have a free shipping discount configured to "discount_all", that is associated to a coupon code.
The shipping service selection happens before entering the coupon code (that happens on the next checkout pag)e. Once I entered the coupon code on the next page, free shipping is correctly applied.
However, If I go back to the page that contains the shipping service selection pane, prices are correct the first time the page is loaded, but when the "calculate shipping" button is clicked (that triggers an AJAX request), discounts are applied twice.
Additionally, I think we shouldn't rely on the boolean $save_line_item, but instead only save the line item if it has an ID (i.e
if (empty($line_item->is_new))or probablyif (!empty($line_item->line_item_id))we should make sure we don't save a line item that has never been saved.This bug is probably related to the issue described in #2699345: Shipping discount applied multiple times to the same shipping service.
Comment #18
jsacksick commentedWe could either make sure the discount price components are removed from the discount unit price (or probably check if the cart refresh hook hasn't been called already by checking the static cache?).
Comment #19
jsacksick commentedI realized the issue was due to the fact that we're implementing the
hook_commerce_shipping_method_collect_rates()meaning the logic that invokes the rules event and calls our cart refresh implementation is called for every single shipping method Shipping is collecting rates for (if you have 10 shipping methods, that logic will run 10 times)...I updated the code to provide a default rule that reacts to the
"commerce_shipping_collect_rates" (Collecting shipping rates for an order) rules event.instead.We can't implement the hook because the hook is invoked before the rule event which means the shipping rates aren't present yet.
I configured the rule with a weight = 10 in order to make sure the rates have been collected for all the shipping methods.
Additionally, I check if the line items we're manipulating are not new before saving them.
Comment #20
jsacksick commentedI removed a line by accident, here's the correct patch.
Comment #21
czigor commentedAdded a test for the bug described in #19 without the fix (that, is, the test should fail).
The test now creates 2 shipping methods because that's what triggers the bug.
Comment #22
czigor commentedThis is the fix and the test. Should not fail.
Comment #25
jsacksick commented@czigor: Good work on this, committed!
Comment #26
jsacksick commentedComment #28
joelpittet@jsacksick and @czigor, this change seems to have made a dependency on
commerce_shippingthat wasn't there before.#2920331: Commerce shipping dependency? Could you take a look at this? Probably just needs to be wrapped in
if (module_exists('commerce_shipping')) {as a quick fix.