magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.5k stars 9.3k forks source link

Instant purchase "cheapest shipping" broken for configurable products #38811

Closed Tomasito665 closed 2 months ago

Tomasito665 commented 4 months ago

Preconditions and environment

(1) Install magento

(2) Set up in-store delivery

(3) Enable products for in-store pickup

(4) Configure flat rate as the cheapest option

(5) Create a customer account

(6) Verify that in-store delivery is available

Try purchasing a Joust Duffle Bag and a Chaz Kangeroo Hoodie through the checkout flow using in-store delivery. We assigned stock for both products to our in-store delivery source in step 3. Thus, in-store delivery should be available for both. Furthermore, we configured in-store delivery to cost 6$, which is more expensive than the 5$ flat-rate shipment method. These prices should show on the checkout page as seen in the screenshots below. Simple product checkout Configurable product checkout
Step6*CheckoutSimpleProduct Step6*CheckoutConfigurableProduct

(7) Verify that instant purchase is available

Open any product detail page and ensure that the Instant Purchase button renders. If it does not, make sure that the Magento_InstantPurchaseMock module is correctly installed and enabled as described in step 1. Also, the Instant Purchase button only renders if you are logged in as a customer and if that customer has both a default billing and a default shipping address. Make sure these addresses are properly configured as described in step 5. Instant Purchase button
Step7_InstantPurchaseButton

Steps to reproduce

Expected result

The order is placed using the cheapest shipment method, which, as we saw in step 6, is the $5.00 Flat Rate shipment method.

<img width="1124" alt="Expected behavior" src="https://github.com/magento/magento2/assets/12563382/541e573b-0e3a-4174-b066-1f1c4f99160f"]

Actual result

The order is placed using the $6.00 In-Store Delivery shipment method, which is NOT the cheapest shipment method. {quote}[!NOTE] {quote} {quote}As seen in the screenshot, with my setup, the order placement does not finish successfully and ends with a "Quote does not have Pickup Location assigned" error. The cause of this error is irrelevant to the bug described in this issue. However, the fact that it gives this error in particular shows that the instant purchase flow attempts to place the order using the In-Store Delivery shipment method, which is sufficient to demonstrate the bug. {quote} [img width="1136" alt="ActualBehavior_2of2" src="https://github.com/magento/magento2/assets/12563382/bce0144a-5faf-4f2e-9487-4da90f8315df">

Additional information

It works for simple products

While the "cheapest price" shipment method does not work for configurable products, it does work for simple products—see screenshots below. For simple products ... ... it works
ExpectedBehavior*1of2 ExpectedBehavior*2of2

Flat rate counted double for configurable products 🐛

The "cheapest price" shipment method is calculated in the CheapestMethodDeferredChooser class in the following method. The $5.00 Flat Rate shipping is not correctly identified as the cheapest option because, for reasons unclear to me, its price is doubled when performing the comparison to the $6.00 In-Store Delivery. <https://github.com/magento/magento2/blob/001e5188cd59309f5de9205946ede5ddccdadaff/app/code/Magento/InstantPurchase/Model/ShippingMethodChoose/CheapestMethodDeferredChooser.php#L35-L50]

When debugging the instant purchase of the Joust Duffle Bag simple product, we see that the $shippingRates array contains the following rates.

``{{javascript [ {_data:

Unable to find source-code formatter for language: "flatrate_flatrate", carrier: "flatrate", price: 5.0, .... Available languages are: actionscript, ada, applescript, bash, c, c#, c<ins></ins>, cpp, css, erlang, go, groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl, php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml, yaml{code}
, ...},
{_data:
{code}
Unable to find source-code formatter for language: "instore_pickup", carrier: "instore", price: 6.0, .... Available languages are: actionscript, ada, applescript, bash, c, c#, c<ins></ins>, cpp, css, erlang, go, groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl, php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml, yaml{code}
, ...},
{_data:
{code}
Unable to find source-code formatter for language: "tablerate_bestway", carrier: "tablerate", price: 15.0, .... Available languages are: actionscript, ada, applescript, bash, c, c#, c<ins></ins>, cpp, css, erlang, go, groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl, php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml, yaml{code}
, ...},
]
}}``

However, when debugging the instant purchase of some permutation of the Chaz Kangeroo Hoodie configurable product, we see that the `$shippingRates` array contains different rates—double flat rate price.

``{{javascript
[
{_data:
{code}
Unable to find source-code formatter for language: "instore_pickup", carrier: "instore", price: 6.0, .... Available languages are: actionscript, ada, applescript, bash, c, c#, c<ins></ins>, cpp, css, erlang, go, groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl, php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml, yaml{code}
, ...},
// twice the 5.00 flat rate shipment method item price for only 1 item 💥👇
{_data:
{code}
Unable to find source-code formatter for language: "flatrate_flatrate", carrier: "flatrate", price: 10.0, .... Available languages are: actionscript, ada, applescript, bash, c, c#, c<ins></ins>, cpp, css, erlang, go, groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl, php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml, yaml{code}
, ...},
{_data:
{code}
Unable to find source-code formatter for language: "tablerate_bestway", carrier: "tablerate", price: 15.0, .... Available languages are: actionscript, ada, applescript, bash, c, c#, c<ins></ins>, cpp, css, erlang, go, groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl, php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml, yaml```
, ...},
]
}}``

In this case, Flat Rate costs not the expected $5.00 but $10.00, which is indeed not the cheapest option anymore. In that case, the $6.00 In-Store Delivery shipment is the cheapest method, which explains why the instant purchase flow attempts to place the order using in-store delivery. I believe that the crux of the issue boils down to why the price is doubled for configurable products. It clearly shouldn't. Even though for configurable products there are two items (parent and child), there is only one item shipped (= child). So, the Flat Rate item price should be $5.00, not $10.00.
### Release note

"magento/product-community-edition": "2.4.7"


### Triage and priority
 - [ ] Severity: **S0** *- Affects critical data or functionality and leaves users without workaround.*
 - [X] Severity: **S1** *- Affects critical data or functionality and forces users to employ a workaround.*
 - [ ] Severity: **S2** *- Affects non-critical data or functionality and forces users to employ a workaround.*
 - [ ] Severity: **S3** *- Affects non-critical data or functionality and does not force users to employ a workaround.*
 - [ > Severity: **S4** *- Affects aesthetics, professional look and feel, “quality” or “usability”.*
m2-assistant[bot] commented 4 months ago

Hi @Tomasito665. Thank you for your report. To speed up processing of this issue, make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, Add a comment to the issue:

m2-assistant[bot] commented 4 months ago

Hi @engcom-Dash. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:


Tomasito665 commented 3 months ago

I found the issue. The instant purchase PlaceOrder::placeOrder() method:

  1. Uses the instant purchase QuoteCreation::createQuote method to create an "empty" quote, i.e., with store id, customer, shipping address, and billing address, but without products.

https://github.com/magento/magento2/blob/001e5188cd59309f5de9205946ede5ddccdadaff/app/code/Magento/InstantPurchase/Model/PlaceOrder.php#L103-L108

  1. Uses the instant purchase QuoteFilling::fillQuote method to add the product to the quote.

https://github.com/magento/magento2/blob/001e5188cd59309f5de9205946ede5ddccdadaff/app/code/Magento/InstantPurchase/Model/PlaceOrder.php#L109-L113

  1. Calls the Quote::collectTotals() to compute the quote price.

https://github.com/magento/magento2/blob/001e5188cd59309f5de9205946ede5ddccdadaff/app/code/Magento/InstantPurchase/Model/PlaceOrder.php#L115

So far, so good. However, if we take a closer look at QuoteCreation::createQuote, we see that it assigns the shipping and billing address to the quote, but does not flag them for rate recalculation before totals collection, i.e., does not call setCollectShippingRates(true) before Quote::collectTotals(). It does so, but further down the pipeline when choosing the cheapest method—after having collected the totals already.

https://github.com/magento/magento2/blob/e81f28e4e6739924594c266b011c68b08a227c2d/app/code/Magento/InstantPurchase/Model/ShippingMethodChoose/CheapestMethodDeferredChooser.php#L21-L25

If we set setCollectShippingRates(true) before Quote::collectTotals(), then shipping rates are collected correctly as part of the "collect totals" operation. Diving into the code that calculates quote address totals, we see that it sets some "magic" fields immediately before collecting shipping rates.

https://github.com/magento/magento2/blob/e81f28e4e6739924594c266b011c68b08a227c2d/app/code/Magento/Quote/Model/Quote/Address/Total/Shipping.php#L87-L92

Take the "item qty" field. The abovementioned code snippet computes the number of "sendable" items and assigns that value to the address "item qty" field. I call this a "magic" field because it is never persisted in the database. Its only purpose, as far as I can tell, is to provide a way to override the return value of Address::getItemQty, which would normally "simply" return the sum of the item quantities, regardless of whether those are sendable.

https://github.com/magento/magento2/blob/e81f28e4e6739924594c266b011c68b08a227c2d/app/code/Magento/Quote/Model/Quote/Address.php#L725-L735

This is what makes shipping rate calculation work when called within the context of collecting totals. It overrides the address item quantity with the "sendable" quantity, which is the correct quantity in the context of computing shipping rates. With instant purchase, however, the sendable quantity is not set, and rates are computed incorrectly when choosing "cheapest shipping." In summary:

  1. The correct shipping rates are correctly computed as part of the call to Quote::collectTotals() but not retained as the "collect shipping rates" flags are not set at that point.

  2. The CheapestMethodDeferredChooser sets the "collect shipping rates" flags and calls collectShippingRates() to compute the shipping rates. However, it does not specify the "sendable" items when doing so, and rates are computed incorrectly.

As a straightforward solution, I propose to:

engcom-Bravo commented 3 months ago

Hi,

Internal team has started to work on it

Thanks

engcom-Hotel commented 3 months ago

As the internal team has started working on it, we are moving this issue to on hold until we receive further updates. We will provide updates on this issue as they become available.

engcom-Bravo commented 2 months ago

Hello,

As I can see this issue got fixed in the scope of the internal Jira ticket AC-12119 by the internal team Related commits: https://github.com/search?q=repo%3Amagento%2Fmagento2+AC-12119-v1&type=commits

Thanks.