Tax calculation process does not follow "Apply Tax On" setting

[vbuck]

Tax is not correctly calculated when "Apply Tax On" option is set to "Original price only." Tax is always based on item calculation price (custom price).

Preconditions

• Magento Open Source 2.2.3
• PHP 7.1.14
• Apache 2.2.15
• Magento sample data installed
• Shipping methods Flat Rate, Table Rates, and UPS enabled
• Payment methods Check/Money Order, Credit Card (Braintree) enabled
• Setting "Base Currency" set to US Dollar
• Setting "Default Display Currency" set to US Dollar
• Setting "Tax Calculation Method Based On" set to Total
• Setting "Tax Calculation Based On" set to Shipping Address
• Setting "Shipping Tax Class" set to Taxable Goods
• Setting "Apply Tax On" set to Original price only
• New tax rate configured as: any zip code match (*), region of PA/US, rate of 6.0000
• Existing tax rule "Rule1" expanded to accept new tax rate

Steps to reproduce

1. From the admin panel, go to Sales > Orders
2. Select Create New Order
3. Choose existing customer Veronica Costello
4. Click Add Products
5. Select simple product Overnight Duffle ($45.00)
6. Click Add Selected Product(s) to Order
7. In the Shipping Address section, uncheck option Same As Billing Address
8. Change the Street Address to 123 Main Street
9. Change the City to Mechanicsburg
10. Change the State to Pennsylvania
11. Change the Zip Code to 17055
12. Observe Tax line-item reads $2.70 (0.06 * 45 = 2.6999)
13. For quote item Overnight Duffle, select option Custom Price
14. Enter value 10.00
15. Click Update Items and Quantities
16. Observe Tax line-item reads $2.70 (0.06 * 45 = 2.6999)

Expected result

• Final tax line-item reads $2.70

Actual result

• Final tax line-item reads $0.60

Additional Information

Class Magento\Tax\Model\Sales\Total\Quote\CommonTaxCollector is the underlying processor for the total and subtotal tax collectors. This class declares method mapItem, and this method is responsible for converting quote items into a more specific instance of Magento\Tax\Api\Data\QuoteDetailsItemInterface. Lines 216-228 determine the unit price of the given item:

    if ($useBaseCurrency) {
        if (!$item->getBaseTaxCalculationPrice()) {
            $item->setBaseTaxCalculationPrice($item->getBaseCalculationPriceOriginal());
        }
        $itemDataObject->setUnitPrice($item->getBaseTaxCalculationPrice())
            ->setDiscountAmount($item->getBaseDiscountAmount());
    } else {
        if (!$item->getTaxCalculationPrice()) {
            $item->setTaxCalculationPrice($item->getCalculationPriceOriginal());
        }
        $itemDataObject->setUnitPrice($item->getTaxCalculationPrice())
            ->setDiscountAmount($item->getDiscountAmount());
    }

My understanding is as follows:

• Look for presence of tax_calculation_price via magic getter (note: no implementation found)
• If not set, use calculation_price via method AbstractItem::getCalculationPriceOriginal

Given the name "tax calculation price," it can be assumed that any modifications to the calculation price should be made here for the sake of tax-specific constraints. Yet, there are none made. Instead the original calculation price is used as-is.

This becomes problematic under the following conditions:

• Method AbstractItem::setCustomPrice is used; and,
• Tax configuration is set to "Apply Tax On" the original price

These conditions cause the mapItem method to incorrectly refer to a price that is different from the originating product's base price. That is, a custom price. Furthermore, the "Apply Tax On" setting itself appears to be unused in the core altogether. There are even helper methods in Magento\Tax\Helper\Data, applyTaxOnCustomPrice and applyTaxOnOriginalPrice, which appear to be useful to this mapping process.

I believe the solution is to conditionally fall back to AbstractItem::getCalculationPriceOriginal only when the setting "Apply Tax On" is set to "Custom price if available." Otherwise, if set to "Original price only," then the tax calculation price must be derived from AbstractItem::getBaseOriginalPrice.

[iindranil]

Yes, That's correct. Apply Tax On setting not working. Any solution for this issue ?

Thanks in advance,

[vbuck]

@iindranil I haven't submitted a PR for this issue, but I believe one solution is given in my closing statement of the issue description:

conditionally fall back to AbstractItem::getCalculationPriceOriginal only when the setting "Apply Tax On" is set to "Custom price if available." Otherwise, if set to "Original price only," then the tax calculation price must be derived from AbstractItem::getBaseOriginalPrice

I happen to be working on this very issue today in my day job, so I may follow up with a diff or PR to reference when I come out of it.

[iindranil]

For example, I have added one product in my cart where product original price is $20 and I have set custom price $15.
then I want to add tax on original price $20 which is not working as I reported.

If I do the changes as per your solution on CommonTaxCollector.php then cart item row update with $20 which is incorrect.

I want to sale item at $15 but want to add tax on $20 which is not working with your solution.

Any idea ?

Thank you,

[vbuck]

@iindranil I am continuing to research this issue in pursuit of a fix.

As an update to the issue, I have one additional architectural comment. By design this method, being called mapItem and based on its DocBlock description, should not be modifying any properties on the given AbstractItem. It should be treating that item as read-only as it transfers properties to the resulting QuoteDetailsItemInterface object. Therefore it seems this bit of code was an afterthought, or maybe a "quick fix" to a problem encountered during development.

I am not sure it's worth fixing the method to adhere to the principle I outlined, but it's worth noting in case "the dog has to return to its vomit."

[ghost]

Hi @vbuck , thank you for your report.
We've acknowledged the issue and added to our backlog.

[vbuck]

@engcom-backlog-nazar I am not able to get to a PR at the moment, but in response to your update I wanted to share a diff I've had sitting around until I could come back to this issue:

• https://gist.github.com/vbuck/fdc552f8bc6fa3434aa521684c242b0c

I noticed that you made a comment about not being able to reproduce, which I received via email. But by the time I came back to this issue the comment had changed. I'm guessing you were finally able to reproduce.

Let me know if that diff above will solve your problem. If so, I'd be glad to open a PR over the weekend so that you can approve it.

[sdzhepa]

Hello @vbuck

Thank you for contribution and collaboration!

The corresponding internal ticket MC-4316 (MAGETWO-93593), MAGETWO-93592 were fixed and closed by Magento team

Delivered to 2.3-develop branch and should be available with 2.3.1 release
Please see details in the next commits:

• 2bb407e
• a05fdf6
• 4ec8dfb

Delivered to 2.2-develop branch and should be available with 2.2.9 release
Please see details in the next commits:

• 5698d56