[Issue] Fix shipping total amount calculation for multishipping addresses

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

[Issue] Fix shipping total amount calculation for multishipping addresses #37283

Closed m2-assistant[bot] closed 10 months ago

m2-assistant[bot] commented 1 year ago

This issue is automatically created based on existing pull request: magento/magento2#37272: Fix shipping total amount calculation for multishipping addresses

Description (*)

When Quote contains more than one shipping address, the total collector updates shipping amount for address given in $quote->getShippingAddress() instead of passed via $shippingAssignment

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Create a quote
Add additional shipping address with items
Leave address given via $quote->getShippingAddress() with virtual items only
Collect quote totals
The address given via $quote->getShippingAddress() has fields shipping_amount and base_shipping_amount equal 0.

Questions or comments

Currently that address would contain shipping_amount and base_shipping_amount greater than zero but shipping_incl_tax equals zero that is wrong for multishipping quotes Selection_066

Contribution checklist (*)

[ ] Pull request has a meaningful description of its purpose
[ ] All commits are accompanied by meaningful commit messages
[ ] All new or changed code is covered with unit/integration tests (if applicable)
[ ] README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
[ ] All automated tests passed successfully (all builds are green)

m2-assistant[bot] commented 1 year ago

Hi @engcom-November. 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:

[ ] 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).
[ ] 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue.
[ ] 3. Add Area: XXXXX label to the ticket, indicating the functional areas it may be related to.
[ ] 4. Verify that the issue is reproducible on 2.4-develop branch
Details
- Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
- If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
[ ] 5. Add label Issue: Confirmed once verification is complete.
[ ] 6. Make sure that automatic system confirms that report has been added to the backlog.

m2-assistant[bot] commented 1 year ago

Hi @engcom-Hotel. 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:

[ ] 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).
[ ] 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue.
[ ] 3. Add Area: XXXXX label to the ticket, indicating the functional areas it may be related to.
[ ] 4. Verify that the issue is reproducible on 2.4-develop branch
Details
- Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
- If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
[ ] 5. Add label Issue: Confirmed once verification is complete.
[ ] 6. Make sure that automatic system confirms that report has been added to the backlog.

engcom-Hotel commented 1 year ago

Hello @ilnytskyi,

Thanks for the report and collaboration!

We have tried to reproduce the issue in Magento 2.4-develop branch. We have tried to debug the table quote_address and as per the description shipping_incl_tax column should contain a value greater than zero, Am I right?

We have tried to order 3 different products with 1 qty each, out of 3 one is a virtual product. For the other 2 products, we can see the value for the shipping_incl_tax column contains value but for the virtual product it is showing 0. Please have a look at the screenshot below:

But it seems expected behavior to us because, for virtual products, we are not shipping the same.

Please let us know if we have missed anything.

Thanks

ilnytskyi commented 1 year ago

@engcom-Hotel if your address id = 40 contains only virtual item then all shipping related columns must be = 0 if your address id = 41 contains items to ship then related shipping columns must contain calculated shipping value for its items

Additionally if you take a look at the code the address returned from would take precedence over the address passed to $shippingAssigment that is wrong

$quote->getShippingAddress()

So the code changes data on different address that leads to DB data inconsistency Selection_259

basically in multi-shipping quote, the address

$quote->getShippingAddress() is not same as one passed via $shippingAssigment to that collector

Try to add more addresses and see how data on$quote->getShippingAddress() changes depending on what you get form every other set of items.

The address given by $quote->getShippingAddress() would only be acceptable in that collector in case you have different $quote for every address.

engcom-Hotel commented 1 year ago

Hello @ilnytskyi,

Thanks for the reply!

We have again tried to reproduce the issue in 2.4-develop and with this PR https://github.com/magento/magento2/pull/37272

This time we have followed the below steps in both instances:

Create a Simple as well as virtual product.
Create a user with 2 different addresses.
Create an order with both products and choose the non-default address for the simple product (we are not able to select an address for the virtual product).
Place an order.

Now we have tried to fetch the data for that order from quote_address from both instances (With PR and Without PR). And we found no difference in both of them. Please find attached the CSV for reference:

Data.csv

Thanks

ilnytskyi commented 1 year ago

@engcom-Hotel In your example, i see two scenarios with 2 addresses in each. One shipping and one billing. The issue appears when there is one billing address and two or more shipping addresses. The issue in to related for data example you provided. quote_address table must contain at least 3 rows where data would look like

billing address with no items attached and 0 totals
shipping address - with only virtual items, (might be the address returned from $quote->getShippingAddress())
shipping address with some items and selected shipping method
shipping address with some items and selected shipping method In this scenario the second address (row 2) would contain values in shipping related columns but it should not, due to bug fixed in my PR

btw regarding your first message: does the address with ID 40 contain only virtual items? If isn't it the issue that this address has value in shipping_inl_tax column?

engcom-Hotel commented 1 year ago

@magento give me 2.4-develop instance with edition b2b

magento-deployment-service[bot] commented 1 year ago

Hi @engcom-Hotel. Thank you for your request. I'm working on Magento instance for you.

magento-deployment-service[bot] commented 1 year ago

Hi @engcom-Hotel, unfortunately there is no ability to deploy Magento instance at the moment. Please try again later.

engcom-Hotel commented 1 year ago

@magento give me 2.4-develop instance with edition b2b

magento-deployment-service[bot] commented 1 year ago

Hi @engcom-Hotel. Thank you for your request. I'm working on Magento instance for you.

magento-deployment-service[bot] commented 1 year ago

Hi @engcom-Hotel, unfortunately there is no ability to deploy Magento instance at the moment. Please try again later.

engcom-Hotel commented 1 year ago

Hello @ilnytskyi,

Thanks for the updates!

I think this time we are able to reproduce the issue. This time we have tried to debug the below method:

https://github.com/magento/magento2/blob/98fb0dc90709455737188d1f4256fbbb73c2a6b3/app/code/Magento/Tax/Model/Sales/Total/Quote/Shipping.php#L23-L64

and debug the shipping address return with and without PR.

Please refer to the below screencasts for reference:

Without PR without_pr.webm

With PR with_pr.webm

Let us know if we missed anything.

Thanks

ilnytskyi commented 1 year ago

@engcom-Hotel yes, exactly. As you can see without PR you get same address every time. With PR you get address related to current items.

engcom-Hotel commented 1 year ago

Thanks, @ilnytskyi for the reply!

We are confirming this issue after the analysis mentioned here https://github.com/magento/magento2/issues/37283#issuecomment-1550959448.

Thanks

github-jira-sync-bot commented 1 year ago

:x: Cannot export the issue. This GitHub issue is already linked to Jira issue(s): https://jira.corp.adobe.com/browse/AC-8795