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

Shipping tax rounding error - off by 1 cent #26599

Closed willtran closed 3 years ago

willtran commented 4 years ago

Preconditions (*)

  1. Magento 2.4-develop
  2. PHP 7.3

Steps to reproduce (*)

  1. Create a fixed price shipping - cost is unimportant, but the problem is found when shipping cost is $5 fixed
  2. Create product that can be added to cart. Again price is not important but problem can be replicated if product price can be divided by $5
  3. Create tax for product and shipping. When problem is found tax rate is set at 10% applied to both product and shipping

Expected result (*)

  1. Shipping is calculated correctly. Total shipping = shipping price + shipping tax
  2. For example: $5 shipping with 10% tax = 4.54 shipping price, 0.46 shipping tax
  3. Refund shipping should go through successfully

Actual result (*)

  1. Some orders will have wrong shipping tax
  2. Refund shipping will not be possible because the shipping price is recalculated. Magento will throw error, example: "Maximum shipping amount allowed to refund is: $4.99" (this is the result of shipping price $4.54 + 0.45)
  3. This issue only affect some orders, not all. Investigation show that the number of items / price of items in cart affect the shipping tax calculation

Notes

Issue is with how Delta is calculated. Relevant discussion: https://magento.stackexchange.com/questions/15846/why-does-magento-store-a-rounding-delta-when-calculating-taxes The problem is that the delta for products price is being used to calculate shipping tax. See module-tax/Model/Calculation/AbstractAggregateCalculator.php

magento-deployment-service[bot] commented 4 years ago

Thanks for opening this issue!

m2-assistant[bot] commented 4 years ago

Hi @willtran. Thank you for your report. To help us process this issue please make sure that you provided the following information:

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento give me 2.4-develop instance - upcoming 2.4.x release

For more details, please, review the Magento Contributor Assistant documentation.

@willtran do you confirm that you were able to reproduce the issue on vanilla Magento instance following steps to reproduce?


magento-deployment-service[bot] commented 4 years ago

Thanks for opening this issue!

magento-deployment-service[bot] commented 4 years ago

Thanks for opening this issue!

LiamKarlMitchell commented 4 years ago

Also experiencing this in 2.3.0

Flat Rate shipping set to 8.695652173913043 with a GST Tax of 1.15 expecting to see a final shipping cost of $10.00 but I see $10.1

8.695652173913043 * 1.15 should be $10.0.

On the Shipping Methods screen before the Review & Payments step of checkout the cost shows as 8.7 image

What it looks like on checkout.

image

This should also show including GST as well.

engcom-Alfa commented 4 years ago

Hi @willtran .

Unfortunately, we are not able to reproduce this issue on fresh 2.4-develop.

Preconditions:

  1. Simple product with the price -> 5$ for ex.;
  2. Fixed shipping price -> 5 $ for ex.;

Manual testing scenario:

  1. Go to Admin -> Stores -> Configuration -> Sales -> Tax and set like this:

Screenshot from 2020-05-21 15-31-18 Screenshot from 2020-05-21 15-31-42

  1. Create tax for product and shipping ( Go to Admin -> Stores -> Tax Rules ) In my case I set 10% tax

Screenshot from 2020-05-21 15-34-46 Screenshot from 2020-05-21 15-34-55

  1. Go to Admin -> Sales -> Orders -> Create New Order
  2. Add created before the product to Shopping Cart Screenshot from 2020-05-21 15-39-14
  3. Select shipping method ( 5$); Screenshot from 2020-05-21 15-40-03

Screenshot from 2020-05-21 15-40-35

:heavy_check_mark: For 5% shipping calculation looks like: $5 shipping with 10% tax = 4.54 shipping price, 0.46 shipping tax Screenshot from 2020-05-21 15-47-34

  1. Create Order;
  2. Create Invoice;
  3. Create Credit Memo (Refund) Screenshot from 2020-05-21 15-52-25
  4. Click on Refund Offline

Actual Result: :heavy_check_mark: Credit memo was successfully created

Screenshot from 2020-05-21 15-54-00

@willtran Could you take a look?

Thanks!

m2-assistant[bot] commented 4 years ago

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

willtran commented 4 years ago

@engcom-Alfa thanks for checking. I haven't test on 2.4. Have you play around with the number of qty and product prices? This issue only happens to some small percentages of order because the delta calculated for product rounding were used for rounding shipping and only sometimes it is not correct (the delta becomes too small). I'm not that good at maths to reverse engineer the combination of qty / price and tax rate that would 100% result in the error. However on principle does it make more senses for product tax calculation to have its own delta, and shipping to have its own? similar to the pull request https://github.com/magento/magento2/pull/26600 I will try to replicate on 2.4 and let you know.

LiamKarlMitchell commented 4 years ago

Ah actually, my issue stems from, round which is used in the setPrice method and other places throughout the collection of rates which removes extra precision intended to be used.

Could setPrice not round the price? Should Only round on displaying the final prices.

Spoiler warning The item->unit_price is 8.7 when I entered a decimal number with longer precision 8.695652173913043, I need at least 3 decimal places, even calling a setPrice(8.695) then getPrice() shows the wrong value of 8.7. Magento\Quote\Model\Quote\Address\RateResult::setPrice calls a priceCurrency->round which is rounding to 2dp reducing the precision. ```php $rate->setData('price', $this->priceCurrency->round($price)); ```
LiamKarlMitchell commented 3 years ago

I noticed the label was added recently Reported on 2.4.0, is there any update on a fix for this when the cause is loss of precision in the setPrice method due to it rounding the value.

I don't think setting the value should round it, only round when displaying the values.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 14 days if no further activity occurs. Is this issue still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? Thank you for your contributions!

LiamKarlMitchell commented 3 years ago

Bump.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 14 days if no further activity occurs. Is this issue still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? Thank you for your contributions!

marvinhinz commented 3 years ago

Happens on 2.4.2

m2-assistant[bot] commented 3 years ago

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

engcom-Oscar commented 3 years ago

Hi @willtran ! I can confirm that tax calculation is not correct. Here my example with order with some different products with the same price, but taxes are different there. But I was not able to reproduce error for credit memo creation. Had you chance to check the issue if it reproducing for you on the Magento 2.4.2? Screenshot from 2021-04-21 12-28-49

amcguireweb commented 3 years ago

We are also experiencing this problem.

m2-assistant[bot] commented 3 years ago

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

willtran commented 3 years ago

For anyone getting this issue the fix in this PR has been working for us https://github.com/magento/magento2/pull/26600/files so it worth creating a patch file manually.

engcom-Echo commented 3 years ago

Hi willtran ,

We are not able to re-produce the issue on 2.4-develop branch. Requesting you, kindly elaborate the steps to reproduce with screen shots.

Thanks

engcom-Lima commented 3 years ago

Hi @willtran,

Hoping, your issue is resolved and also we did not get any response from you more than 14 days. Hence closing this ticket.

Please feel free to raise the new ticket or reopen the existing ticket. if you have any issue.

homecoded commented 2 years ago

Hi there I'm using Magento ver. 2.4.2-p1 and I believe there is something fundamentally wrong with the way tax is rounded in Magento\Tax\Model\Calculation\AbstractCalculator. The deltas used for the tax calculation change each time the method is called making it essentially a statefull method. The deltas may become so big that is creates serious rounding errors.

I made some sample code demonstrating the problem using the AbstractCalculator class:

class TestCalculator extends \Magento\Tax\Model\Calculation\AbstractCalculator
{
    public function __construct(
        \Magento\Tax\Api\TaxClassManagementInterface         $taxClassService,
        \Magento\Tax\Api\Data\TaxDetailsItemInterfaceFactory $taxDetailsItemDataObjectFactory,
        \Magento\Tax\Api\Data\AppliedTaxInterfaceFactory     $appliedTaxDataObjectFactory,
        \Magento\Tax\Api\Data\AppliedTaxRateInterfaceFactory $appliedTaxRateDataObjectFactory,
        \Magento\Tax\Model\Calculation                       $calculationTool,
        \Magento\Tax\Model\Config                            $config,
                                                             $storeId = 1,
        \Magento\Framework\DataObject                        $addressRateRequest = null
    ) {
        parent::__construct(
            $taxClassService,
            $taxDetailsItemDataObjectFactory,
            $appliedTaxDataObjectFactory,
            $appliedTaxRateDataObjectFactory,
            $calculationTool,
            $config,
            $storeId,
            $addressRateRequest
        );
    }

    public function calculateWithTaxNotInPrice(\Magento\Tax\Api\Data\QuoteDetailsItemInterface $item, $quantity, $round = true) {}
    public function calculateWithTaxInPrice(\Magento\Tax\Api\Data\QuoteDetailsItemInterface $item, $quantity, $round = true) {}

    public function deltaRoundProxy($price, $rate, $direction, $type = self::KEY_REGULAR_DELTA_ROUNDING, $round = true)
    {
        return $this->deltaRound($price, $rate, $direction, $type, $round);
    }
}

$objectManager = \Magento\Framework\App\ObjectManager::getInstance();
$rowBaseCalculator = $objectManager->get(TestCalculator::class);
$priceWithoutTax = 2.95 - (2.95 / 1.19); // 0.47100840336134
echo "<pre>";
for ($i = 0; $i < 15; $i++) {
    echo $i . ':' . $priceWithoutTax . " => " . $rowBaseCalculator->deltaRoundProxy($priceWithoutTax, "19", true) . PHP_EOL;
}
echo "</pre>";

(!) Please note that you may have to change the $storeId in the constructor. (!) You can copy&paste this code at the end of your pub/index.php and check the output in the browser.

The output will be:

0:0.47100840336134 => 0.47
1:0.47100840336134 => 0.47
2:0.47100840336134 => 0.47
3:0.47100840336134 => 0.47
4:0.47100840336134 => 0.48
5:0.47100840336134 => 0.47
6:0.47100840336134 => 0.47
7:0.47100840336134 => 0.47
8:0.47100840336134 => 0.47
9:0.47100840336134 => 0.47
10:0.47100840336134 => 0.47
11:0.47100840336134 => 0.47
12:0.47100840336134 => 0.47
13:0.47100840336134 => 0.47
14:0.47100840336134 => 0.48

As you can see, the calculated tax amount alternates between 0.47 and 0.48. This means the method deltaRound returns different results depending on how often you call it. This cannot be right!

I am fairly sure that is is the root for most of the off-by-one errors we're seeing.

Edit: I think I understand why this is done. The idea is that you split rounding errors across the quote items so you don't loose 1 cent in taxes. If your tax over the whole card is $10 and and you have three items, the tax should be $3.33, $3.33 and $3.34. But this only works if the calculation is done once. In our setup the calculation is done more than 5 times (on one page checkout page call) for the same item (shipping) and the deltas are not reset on each new calculation. So, the deltas keep accumulating and eventually flip the tax rounding.

apedicdev commented 1 day ago

@magento-engcom-team give me 2.4-develop instance