Multiple taxes support in ot modules

[piloujp]

Rewrite of Order Total modules, mainly discount modules, to support different modules options and multiple tax classes and rates.
Replace PR #6392
See issue #6425 for more details.
This PR needs PR #6411 and is complementary to PRs #6391 and #6393 and optionally #6394 .
All options concerning calculation have been tested thoroughly. Options for validation are partially tested.

It can't pass actual tests because there is a new array in 'order->info' and some options have been suppressed too.

[drbyte]

Looking at the test failure for Low Order Fee, it appears as though this new code is simply not charging any tax. The price mismatch is the amount of the tax: $43.49 instead of $46.01 ... the difference is $2.52

zencart/not_for_release/testFramework/FeatureStore/LowOrderFees/LowOrderFeeTest.php

Lines 249 to 254 in dac1dbc

	$this->assertStringContainsString('39.99', (string)$response->getContent() );
	$this->assertStringContainsString('2.50', (string)$response->getContent() );
	$this->assertStringContainsString('-$4.00', (string)$response->getContent() );
	$this->assertStringContainsString('2.52', (string)$response->getContent() );
	$this->assertStringContainsString('5.00', (string)$response->getContent() );
	$this->assertStringContainsString('-$46.01', (string)$response->getContent() );

[drbyte]

@piloujp Thanks for all your work on this. .... lots to review!

[piloujp]

Looking at the test failure for Low Order Fee, it appears as though this new code is simply not charging any tax. The price mismatch is the amount of the tax: $43.49 instead of $46.01 ... the difference is $2.52

zencart/not_for_release/testFramework/FeatureStore/LowOrderFees/LowOrderFeeTest.php

Lines 249 to 254 in dac1dbc

	$this->assertStringContainsString('39.99', (string)$response->getContent() );
	$this->assertStringContainsString('2.50', (string)$response->getContent() );
	$this->assertStringContainsString('-$4.00', (string)$response->getContent() );
	$this->assertStringContainsString('2.52', (string)$response->getContent() );
	$this->assertStringContainsString('5.00', (string)$response->getContent() );
	$this->assertStringContainsString('-$46.01', (string)$response->getContent() );

I really don't know, I get 46.01 in my test cart, which is the actual GitHub ZC with only this PR applied.
Settings are as follows:
Shipping(2.5) and low order fee(5) have no tax class.
Group discount of 10% does not include shipping ->-4$
tax is 7%(2.52). Florida zone with store and customer in Florida and tax basis set to 'shipping'.
If I apply a GV of -46.01, result is 0$.

[piloujp]

I found the problem. It is because of the reversed calculation of taxes 'Credit Note' <=> 'Standard' between original code and new one. The problem is probably the same with other test failing. When test is using 'Standard' recalculation, I get the same results if I use 'Credit Note'.

[drbyte]

I found the problem. It is because of the reversed calculation of taxes 'Credit Note' <=> 'Standard' between original code and new one. The problem is probably the same with other test failing. When test is using 'Standard' recalculation, I get the same results if I use 'Credit Note'.

Checking: do you have a potential fix in mind?

[drbyte]

Several configuration settings have been removed which previously allowed specific tax handling for GV/Coupon/GroupPricing, including the ability to turn tax off altogether for them.
(Albeit, I think they were imperfectly implemented, and most stores rarely touched those settings.)

Is it accurate to say that now whatever tax rates apply to a customer's order, are also being applied to their GV's and coupons?
And that tax rules are forced by ot discount modules, without option to bypass.

The description now reads Discount amount is tax included -> Standard, if tax excluded -> Credit Note. But I'm not sure I know how to explain that to a storeowner.
1 - does "amount is tax included" mean "if my store uses tax-included pricing I must choose Standard"? Or does it mean "the discount that will be shown to customer already includes taxes"?
2 - similarly, "if tax excluded->Credit Note" ... does that mean "if I want to exclude tax handling on the discount, choose Credit Note"? or does it mean "the discount shown will not yet have taxes applied yet, but they will be assessed in subsequent calculations"?

[drbyte]

I've updated the Unit Tests to make them show the reason why the assertions are failing:

[drbyte]

I've also shortened the scanned HTML for (several of) the Feature Tests to make it easier to find the relevant part of the HTML that's being asserted upon.

[piloujp]

Is it accurate to say that now whatever tax rates apply to a customer's order, are also being applied to their GV's and coupons? And that tax rules are forced by ot discount modules, without option to bypass.

Roughly yes, but your wording is not accurate, see below.

The description now reads Discount amount is tax included -> Standard, if tax excluded -> Credit Note. But I'm not sure I know how to explain that to a storeowner. 1 - does "amount is tax included" mean "if my store uses tax-included pricing I must choose Standard"? Or does it mean "the discount that will be shown to customer already includes taxes"?

Both.

2 - similarly, "if tax excluded->Credit Note" ... does that mean "if I want to exclude tax handling on the discount, choose Credit Note"? or does it mean "the discount shown will not yet have taxes applied yet, but they will be assessed in subsequent calculations"?

The second one.

Well, I think the problem is that discounts are being seen as another item like those in cart or like charges added after, but in my opinion it is wrong.
For a programmer, it would be like mixing property and method in an object.
Let say the invoice is the parent object. Inside you have children objects like items in cart, shipping module, c.o.d. module and low order fee module. They all have their properties like amounts (totals, subtotals), tax class, tax rates, values and descriptions, and methods to calculate these values (shipping cost, tax value...).
Invoice object has its methods too, like discount modules, tax module and total module. These methods just modify different objects properties. Discounts modules reduce values in a predefined way, tax module retrieves taxes descriptions and values to display them and total just displays the final total.

It is why I removed options like tax class, include tax and re-calculate 'none'.
Plus, applying a tax to a discount would mean having negative tax! Never heard of it... Tax authority given away money... it would be nice!
Taxes don't apply to discounts, they apply to items and charges which are discounted (or not). It is why taxes should always be re-calculated when a discount is applied.

Actually, in ZC, tax class in discount modules were used to recalculate taxes (in Credit Note option) in a way they replace taxes already applied. Big problem here, if everything was at 7% tax rate and then a 20% rate is used, you get a discount very different from what was announced.

Finally, option 'include tax' is still partially a mystery. It was only possible to activate it when re-calculate was set to 'none'.

For options 'Credit Note' and 'Standard', wording could be better. To be precise, 'Credit Note' is when discount is applied before tax and 'Standard' is when discount is applied after tax of each item they apply to.

I hope this clarifies things a bit.

[piloujp]

I've updated the Unit Tests to make them show the reason why the assertions are failing:

Very good idea. Looking at test results, I could not see what was the actual error.

[piloujp]

I've updated the Unit Tests to make them show the reason why the assertions are failing:

Test 1 and 2 easily fixed with a rounding: round($result[0]['value'], 2)

Test 3, I think, is wrong. Shipping cost has been discounted 100% and should be 0. Before I modified the code, shipping cost stayed untouched until the end, but what do you do when multiple discounts including shipping are applied? The second discount should be applied on shipping cost discounted from the first, if not you might end up with a discount on shipping higher than original shipping cost.

Test 4 has the same problem as 3. The no tax calculation makes it looks like when set to Credit Note, but there might be some differences.

Test 5 has a reversed option problem. I got 501 for discount if set to Credit Note, but 468.3767.... when set to Standard.

Test 6 -> reversed option problem too.

Test 7 free shipping calculation is a little different. Free shipping discount is added at the end to discount without shipping. Before, they were mixed together. I don't know where the test is taking discount amount, but value correspond at discount before adding shipping discount.

Tests with fixed discount are mixed up by reversed options, but tests with no tax calculation option that does not exist are ok because treated as Credit Note option.

[drbyte]

Well, I think the problem is that discounts are being seen as another item like those in cart or like charges added after, but in my opinion it is wrong.

I see what you're saying, but I don't think it's wrong.

In accounting, it's important to (at least be able to) treat discounts as separate values, because they "could" (if the storeowner chose) be treated as marketing/advertising/promotion expenses.

---

Aside:
The current ot infrastructure is quite rigid because it's still mostly based on the osC roots from which it was first derived.

We've rewritten (offline) it in a way that makes orders super flexible and manageable, but haven't put it into core yet because every plugin that deals with orders and totals, possibly even payment, will be affected.
It treats orders as a bunch of line-items, consisting of products, surcharges, discounts, shipping, taxes, etc, and then payments against it, and even returns/exchanges/refunds and additional payments, etc, are all easily incorporated. Accounting departments like it because every number affecting the total has an explanation.
And never any penny rounding issues, because there's only one source of truth and one set of calculations.
It pretty much does away with all the pains we're discussing in this PR.
But there's a big cost to such a change. Including incompatibility of old order data.

[drbyte]

For options 'Credit Note' and 'Standard', wording could be better. To be precise, 'Credit Note' is when discount is applied before tax and 'Standard' is when discount is applied after tax of each item they apply to.

So, you're saying with this PR "Credit Note" and "Standard" have new meanings: now they just mean "(apply discount) before tax" and "(apply discount) after tax"?

[piloujp]

Well, I think the problem is that discounts are being seen as another item like those in cart or like charges added after, but in my opinion it is wrong.

I see what you're saying, but I don't think it's wrong.

More than the way discount are seen, I meant the consequences of it are wrong, looking at all these options actually available but not usable by store owners.

In accounting, it's important to (at least be able to) treat discounts as separate values, because they "could" (if the storeowner chose) be treated as marketing/advertising/promotion expenses.

I completely agree with that and don't see any incompatibility here... ZC lacks a little bit of traceability for accounting, but I think my PRs go in the right direction.

Aside: The current ot infrastructure is quite rigid because it's still mostly based on the osC roots from which it was first derived.

We've rewritten (offline) it in a way that makes orders super flexible and manageable, but haven't put it into core yet because every plugin that deals with orders and totals, possibly even payment, will be affected. It treats orders as a bunch of line-items, consisting of products, surcharges, discounts, shipping, taxes, etc, and then payments against it, and even returns/exchanges/refunds and additional payments, etc, are all easily incorporated. Accounting departments like it because every number affecting the total has an explanation. And never any penny rounding issues, because there's only one source of truth and one set of calculations. It pretty much does away with all the pains we're discussing in this PR. But there's a big cost to such a change. Including incompatibility of old order data.

It sounds too good to be true.

[piloujp]

For options 'Credit Note' and 'Standard', wording could be better. To be precise, 'Credit Note' is when discount is applied before tax and 'Standard' is when discount is applied after tax of each item they apply to.

So, you're saying with this PR "Credit Note" and "Standard" have new meanings: now they just mean "(apply discount) before tax" and "(apply discount) after tax"?

Yes, and it is a meaning easy to understand for any store owner. After using ZC for years as a store owner and more recently digging deeply into its code, I still can't see any meaning in the following discount modules options:
Include tax
Tax class
Re-calculate set to 'None'
Re-calculate set to 'Credit Note'

And I am pretty sure that I am not alone.
It seems to me that some of these options are there as a quick fix to deal with complicated situations that normally ZC can not deal with, but I won't be surprise to be wrong on this.

[torvista]

We've rewritten (offline) it in a way that makes orders super flexible and manageable...It pretty much does away with all the pains we're discussing in this PR....but there's a big cost to such a change

Better that time is invested in progress than all the time I see invested/wasted in trying to polish an oSC t*rd that seems to be unfixable with all the price/discount/tax options available.

Break plugins....listen to people complaining about what functionality they have lost....put it on a roadmap....ask people to take on those tasks. Organised progress.