Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Priority of order processors conflicts with shipping method resolving #12554

Closed
aleho opened this issue Apr 21, 2021 · 3 comments · Fixed by #13769
Closed

Priority of order processors conflicts with shipping method resolving #12554

aleho opened this issue Apr 21, 2021 · 3 comments · Fixed by #13769
Labels
Bug Confirmed bugs or bugfixes.

Comments

@aleho
Copy link
Contributor

aleho commented Apr 21, 2021 •
edited
Loading

Sylius version affected: 1.9

Description
Looking at order_processing.xml the current priority of processors is

  50. sylius.order_processing.order_shipment_processor:   OrderShipmentProcessor
  40. sylius.order_processing.order_prices_recalculator:  OrderPricesRecalculator
  30. sylius.order_processing.shipping_charges_processor: ShippingChargesProcessor
  ...

If shipping method rules are based on totals (e.g. shipment unit totals, order sum, etc.) shipping method resolving will not work in 50: OrderShipmentProcessor as expected.

The reason for this is that updated sums will not yet be available as all items' values are calculated in OrderPricesRecalculator one step afterwards.

Steps to reproduce

  1. Configure two shipping methods. One with rule "order total sum <= 29.99", one with "order total sum >= 30.00"
  2. Empty cart.
  3. Add one item to cart that makes rule checker condition true (e.g. "order total sum >= 30.00").
  4. Check shipping method being set. It's not the one that matches the condition.

Depending on the template this might not be observed immediately, as cart summary / overview might not include shipping costs (in our templates they do).

Possible Solution
Switch priorities of OrderPricesRecalculator and OrderShipmentProcessor so that all fields are calculated before shipping method is determined.
@Zales0123 Zales0123 added the Bug Confirmed bugs or bugfixes. label Apr 27, 2021
@mdevlamynck
Copy link
Contributor

mdevlamynck commented Sep 16, 2021 •
edited
Loading

There is a similar issue with taxes where the incorrect shipping method is selected:

Steps to reproduce

  1. Create a shipping method A with "order total sum <= 29.99" and another B with "order total sum >= 30.00".
  2. Create a cart where the total without taxes is bellow 29.99 and the total with taxes is above 30.00.
  3. On the shipping method form the shipping method B is proposed and not A (since the total is above 30.00).
  4. On submission when OrderShipmentProcessor runs the total is without taxes so method B is unavailable.

Possible solution

To fix the first I moved OrderPricesRecalculator before OrderShipmentProcessor.
To fix the second issue a added a second OrderTaxesProcessor berofe OrderShipmentProcessor. Since it resets taxes when it runs it shouldn't be an issue to run it twice.

The steps looks like this:

sylius.order_processing.order_adjustments_clearer                    60         Sylius\Component\Core\OrderProcessing\OrderAdjustmentsClearer
sylius.order_processing.order_prices_recalculator                    55         Sylius\Component\Core\OrderProcessing\OrderPricesRecalculator
sylius.order_processing.order_taxes_processor_for_correct_shipping   54         Sylius\Component\Core\OrderProcessing\OrderTaxesProcessor
sylius.order_processing.order_shipment_processor                     50         Sylius\Component\Core\OrderProcessing\OrderShipmentProcessor
sylius.order_processing.shipping_charges_processor                   30         Sylius\Component\Core\OrderProcessing\ShippingChargesProcessor
sylius.order_processing.order_promotion_processor                    20         Sylius\Component\Core\OrderProcessing\OrderPromotionProcessor
sylius.order_processing.order_taxes_processor                        10         Sylius\Component\Core\OrderProcessing\OrderTaxesProcessor
sylius.order_processing.order_payment_processor.checkout             0          Sylius\Component\Core\OrderProcessing\OrderPaymentProcessor

@mdevlamynck mdevlamynck mentioned this issue Sep 20, 2021
Closed
@Leobaillard
Copy link

Hi! Is there any current plans to address this?

This seems to me like something that would be used commonly in e-commerce applications. Maybe the people actually doing this are using third-party plugins?

@coldic3 coldic3 mentioned this issue Mar 15, 2022
Merged
@CoderMaggie
Copy link
Member

@Leobaillard @aleho @mdevlamynck we're on it, see #13769 🚀

@CoderMaggie CoderMaggie linked a pull request Mar 16, 2022 that will close this issue
[Core][Shipping] Fix estimated shipping costs #13769
Merged
@GSadee GSadee closed this as completed in 6c674b2 Mar 18, 2022
Zales0123 pushed a commit to Sylius/SyliusCoreBundle that referenced this issue Jun 2, 2022
@GSadee
bug #13769 [Core][Shipping] Fix estimated shipping costs (coldic3, lc…
72549ce 
…hrusciel)

This PR was merged into the 1.10 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | 1.10
| Bug fix?        | yes
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | fixes Sylius/Sylius#12554, Sylius/Sylius#13123, related to Sylius/Sylius#13126
| License         | MIT



Commits
-------

2863527a9dc012b92dceb8fe6f067db59c5f4879 [Core][Shipping][Behat] Add estimated shipping cost scenarios
caa04093d1ec25bce846395f0472dc5cbd1dcfab [Core][Shipping] Change order processing priorities
d6dbbcdd785236c85a00416ecca5f19b9b68066a [Core][Shipping] Update UPGRADE-1.10.md
9d9f8d9b18573d361672b77dc2a93b08728be85b [Core][Shipping][Behat] Update estimated shipping cost scenarios
04ab77c3ad75d6f2879e0486d167f91e02b2a52b [Core][Configuration] Add possibilty to change priorities based on configuration
225557e9c7f2eb01934c88503ce997049a34859b [Core][Configuration] Rename flag that changes priorities in order processing
12ba23573f24c125209a69885953a207398ff4c1 [Core][Configuration] Fix SyliusCoreConfigurationTest
3333d077aca47ce0f31333c01ce6fdd9b3caa833 [Core][Configuration] Refactor test cases
9fcae9e31fa9804576aa98bc6e2de2d2497ed5e1 [Core][Configuration] Rename flag that changes priorities in order processing
aac69ca011fd0dd92397f460fecdc5995634afdd [Core][Shipping] Update UPGRADE-1.10.md
d790944fe36a02d356a001c9bef4e16ae6da372a [Core][Configuration] Rename flag that changes priorities in order processing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Confirmed bugs or bugfixes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Core][Shipping] Fix estimated shipping costs coldic3/Sylius
5 participants
@aleho @Leobaillard @mdevlamynck @Zales0123 @CoderMaggie