Opensourcepos: Incorrect calculation of multiple sales tax

[meadlin]

IMPORTANT IMPORTANT IMPORTANT

Chose what you want to report: New Feature or Bug.
If you remove the template when submitting a Bug your issue will be closed as we cannot help without basic information.

New Feature / Enhacement

For New Features or Enhacements please remove all the template text and clearly write your proposal.
It's important to state whether you expect the community to implement it or you will contribute the work.
Please bear in mind that we will implement new features only on the current code, there is no support for old versions.

Issue / Question / Bug

Before submitting an issue please make sure you remove the first section of the template and you tick (add a x between the square brakets) and agree with all the following check boxes:

• [X] Checked the current issues database and no similar issue was already discussed
• [X] Read the README, WHATS_NEW and UPGRADE
• [X] Read the FAQ (https://github.com/jekkos/opensourcepos#faq) for any known install and/or upgrade gotchas (in specific PHP has php5-gd, php-intl and sockets installed)
• [X] Reporting an issue of an unmodified OSPOS installation
• [X] PHP version is at least 5.5 and not 7.x
• [X] MySQL version is 5.5 or 5.6 and not 5.7
• [X] Ran any database upgrade scripts (e.g. database/2.4_to_3.0.sql)
• [X] Know the version of OSPOS and git commit hash (check the footer of your OSPOS) and will add to my issue report
• [X] Know the name and version of OS, Web server and MySQL and will add to my issue report

Installation information

OSPOS: 3.1.0 - 4f5ad57
OS: Arch linux
PHP: 7.0.14-1
MySQL: (Mariadb: 10.1.21-1)

Expected behaviour

When multiple tax districts are present, calculate and round the sales tax of each instead of as a sum of the taxes.

Actual behaviour

Multiple sales tax percentages are summed, then the tax is calculated/rounded from the total. The individual sales taxes are displayed correctly on the receipt, but the total is generated as a result of the total tax instead of the individual taxes being summed.

Steps to reproduce the issue

Sales tax 1: 4.75%
Sales tax 2: 3.75%

subtotal: $450
tax 1: $21.38
tax 2: $16.88
Total: $488.25

Which causes an accounting deficit of $0.01 as the total should be $488.26.

[daN4cat]

Many thanks for the explanation, this should be the recommended fix for #1094.

At the moment the steps are to calculate the sum of the two taxes % and then apply it to the subtotal to obtain the total, rounding the total.

So each tax % needs to be applied to the subtotal, get the rounded tax and then sum the two taxes on top of subtotal to obtain the total.

I believe there will be some work to change this.

[SteveIreland]

Thanks for addressing this @daN4cat .

I've been working on the sales tax module for awhile now. It's expanding as I learn the nuances of the various tax legislation of each state (at least the ones I have to immediately account for).

But I want to keep an open mind toward VAT tax because there is discussion going around suggesting that the big Donald might trump us with a VAT tax. So in the example above is this a pure Sales tax or a VAT tax or a combination of both? I always assumed that everyone else except for the U.S. used VAT taxes but I'm thinking that I might be totally wrong on this point.

[meadlin]

This example is for pure sales tax. 4.75% for the state and 3.75% for the city, both require their sales taxes be rounded up.

On February 25, 2017 11:52:27 AM MST, Steve Ireland [e-mail] wrote:

Thanks for addressing this @daN4cat .

I've been working on the sales tax module for awhile now. It's
expanding as I learn the nuances of the various tax legislation of each
state (at least the ones I have to immediately account for).

But I want to keep an open mind toward VAT tax because there is
discussion going around suggesting that the big Donald might trump us
with a VAT tax. So in the example above is this a pure Sales tax or a
VAT tax or a combination of both? I always assumed that everyone else
except for the U.S. used VAT taxes but I'm thinking that I might be
totally wrong on this point.

--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
https://github.com/jekkos/opensourcepos/issues/1163#issuecomment-282504146

[daN4cat]

@SteveIreland in EU/UK prices are with VAT included. That's why @jekkos and I tend to use and test with that.
This two level of taxation is mainly a US thing, and it's tricky to deal with when calculating totals and etc.

I've just checked Sale model code and the searches need to change completely to move from sum of tax % to the correct way.
And to be honest I'm not particularly inspired because of that work I will use 0. Oh joy...

[meadlin]

I'll take a look at it and the rest of the code to see if I can come up to speed and help take a stab at it. It might be beneficial to add the capability to add more than two tax districts, as in my example at least, the state is really 4 different sales tax districts that are being combined to conform to the SW model.

[daN4cat]

@meadlin it's all in models/sale.php
I'm currently checking what can be done...

[daN4cat]

I'm currently working on this....

[daN4cat]

... and I fixed the summary report as first step.

[daN4cat]

@meadlin @odiea see my last commit to master. Please test as I reworked the tax calculation.
Hopefully I managed to fix all was required, but you never know.... it took half day of work!

[meadlin]

@daN4cat Just pulled, and it looks like the total still isn't updating correctly.

[daN4cat]

@meadlin that is with the register only or even the receipt?

[meadlin]

@daN4cat both

[daN4cat]

Ok

[odiea]

Now it appears that reports are showing that lost penny Ok But Register and Receipts are incorrect.

[meadlin]

@odiea On a side note, how did you git the third sales tax? I can't seem to get more than two.

[odiea]

I had 1 item at 7.4 and the second item at 3.25 and 7.35 so it was 2 items sold.

[meadlin]

@odiea Ok, I was thinking it was 3 different tax rates on the same item(s).

[odiea]

No here in Ft fun we just use the combine sales tax .

[daN4cat]

@meadlin , @SteveIreland is working on multiple taxes see #1133

[meadlin]

@daN4cat Thanks, nice to know. It'll definitely help with accounting/reporting.

[daN4cat]

@meadlin @odiea if you reprint a receipt do you get the same issue or no?

[daN4cat]

because when in sale register mode the tax calculation is done in Sale_lib.php which is different from the reports or re-print of a receipt.... weird isn't it?

[meadlin]

@daN4cat Reprint has the same error.

[daN4cat]

ok, thanks

[odiea]

Yes the receipt I showed was from a reprint.

[daN4cat]

So the problem is the total, taxes are correct now.

[meadlin]

@daN4cat Yea, from what I've seen, the taxes have been correct, just the total was the issue.

[odiea]

Yes. With a calculator they were showing the correct value. It depends on the rounding I guess.

[daN4cat]

Yes, trying to figure out were it goes wrong with the total...

[daN4cat]

And when in report detailed sale, having just one line is the issue the same or no? I guess it's the same.

[meadlin]

The issue, for me at least, is that the taxes to be collected are the sum of the rounded district taxes, while the total appears to have the sum of the district tax rates applied to it.

I.e.:
subtotal + ROUND(subtotaltax1) + ROUND(subtotaltax2)
vs
subtotal + ROUND(subtotal* (tax1+tax2))

[daN4cat]

could be the rounding to 3 decimals instead of 2... because both values are something like 23.375 + 12.325

if they are rounded at display time they look ok, but in the total they'll make -0.01

[meadlin]

Possibly

[daN4cat]

That part of the code related to receipt and invoice is convoluted. It uses a different code flow and redoes the tax calculations in a different way from what is fetched from the database in the reports/tabular views.

I fixed the latter not the receipt/invoice. Yes the code is really crap in this part...

[odiea]

Quite a mess you inherited.

[meadlin]

As is the way of any software project :smile:

[jekkos]

I told you that this is one of the hardest parts in this application.. A domain driven design might simplify things here..
It's just that once features are added and nothing refactored technical debt will keep increasing. Think we need to think about this one moment or another.

[daN4cat]

It would probably make sense to fetch the cart line from the same temp table style not straight from the db.

This requires some serious refactoring as now the core part is working fine, it's just those silly methods get_sale_items_ordered and get_sale_items that fetch the data from the DB and then rely on the Sale_lib to redo exactly the same it's done in the other methods leveraging SQL functions....

so it's a duplication, PHP on one side, MySQL functions on the other.

grrrrrrrrrrrr

[jekkos]

Hm ic, I think it has always been that way, I spent quite some time in the past in that part of the receipt code. But it's annoying that there are two ways.

On the other hand I think it's a pity that CI has such poor testing support. If we'd want to refactor this part, I'd prefer to write some tests first so one can go ahead without breaking half of the application

[daN4cat]

@meadlin @odiea please check the latest master and see if this fix solves the problem.

[daN4cat]

I rounded the tax in Sale_lib get_taxes function as that was giving the output with no rounding so 3 or 4 decimals.
The mistake in your example was with 3 decimals and the last one being 5 on both, so adding up to the 0.01 difference.

[odiea]

Looks good to me now. Both register and Receipt is adding up correctly.

[meadlin]

When I test the change with a new sale, the taxes/total come out correct.

When I pulled up a historical sale, the taxes are now rounded down, but the total is correct for the false rounding.

The takings for the new sale also still show the rounded down value

[daN4cat]

Ok, actual is now fine and historical wrong. Do you see the same with sale reports? I mean detailed and summary?

[meadlin]

Detailed

and summary

are both incorrect now, oh the joys of multi-sales tax...

[daN4cat]

Yep, big issue....

[daN4cat]

I wonder what it is now that makes the stored data go wrong.
I guess in fixing the rounding on the cart line fetch calculation I interfered with a flow in the code related to saving the data.
That can be the only explanation as the totals seens now consistent just the taxation is stored not right.

[meadlin]

It may be stored correctly, those reports were for the original historic sale. I'm trying to generate a new sale now to test. Anyone know why on occasion after adding a payment, the complete button doesn't appear?

[meadlin]

Got a new sale to work, takings, summary and detailed are all still incorrect.

[RamkrishnaMondal]

@meadlin Can you set tax decimal to 3 and check ?

[meadlin]

@RamkrishnaMondal I cannot test anything using new data at the moment. Sadly I cannot complete a sale, or suspend one for that matter....I don't know how I got it to work last night, but I'm dead in the water so far. Hopefully I can get it working without too much trouble.

[meadlin]

ok, had to update the DB, to get going again... Weee...anyways, with tax decimal set to 3, detailed/summary reports and takings are still incorrect.

[daN4cat]

The issue is the 0.01 issue that is outside the rounding interval that would enable the complete button.

[daN4cat]

Add a payment of 0.005 and you'll see it enabled.

[araalrai]

Another issue is with when the "Tax Included" is enabled. It does not calculating taxes properly when you have two taxes. For example:

Tax 1 @ 5%
Tax 2 @ 7%

Lets say the total price including taxes is $112
The break down should be as follows:
Sub Total: $99.34
Tax1: $5.33
Tax2: $7.33

It should be:
Sub total: $100
Tax1 $5
Tax2 $7

What the program is doing is basically calculating each tax separately. So it is taking the total $112 x Tax 1 divided by 100 + Tax 1 = $5.33

Any fix or suggestions so i may fix it would be appreciated.

Thanks

[araalrai]

Correction to my previous comments:

The breakdown shows as follows:
Sub Total: $99.34
Tax1: $5.33
Tax2: $7.33

It should be:
Sub total: $100
Tax1 $5
Tax2 $7

[daN4cat]

@araalrai that problem is part-fixed in current master, however there is an additional issue when it comes to lookup a past sale.
The code is tricky and have multiple flows doing the same.

[araalrai]

Could you please let me know which php files are used to calculate sales
taxes. I may have something which i may implement which may fix the tax
related issues.

On Mar 1, 2017 8:59 AM, "FrancescoUK" [e-mail] wrote:

@araalrai https://github.com/araalrai that problem is part-fixed in
current master, however there is an additional issue when it comes to
lookup an past sale.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/jekkos/opensourcepos/issues/1163#issuecomment-283400824,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AY3gGTDVQi7JMr3Y_0nsteCT1CO1ESRDks5rhaPZgaJpZM4ML4GI
.

[daN4cat]

It's convoluted, you really need to know well the code to understand where.

Most likely your issue is fixed already and touches 3 files. Just check the commit history to understand which ones. I'm on my mobile phone at the moment and far from the code.

[SteveIreland]

FYI - I'm addressing these calculation issues in the "soon to be available" Customer Sales Tax module.

[daN4cat]

@SteveIreland if you are reworking this part I don't spend anymore time to investigate and fix bugs.
I'll focus on integrating table pr next days.

[jekkos]

@SteveIreland Might be cool if you could try to add some unit tests for this crucial part of the application. Php unit support should be there using composer . Tax calculations feels like something that should be well coherent and modular to write some tests for.

[jekkos]

Having unit tests will help us refactor in the future without making regressions.. you add support for one scenario and break a whole couple of others. This is painful and kind of demotivates people to dive into the code

[SteveIreland]

I will look into this over the weekend.

[daN4cat]

@jekkos it's all the fun with dealing with OSPOS code, it's a free of charge endurance adventure 😊

[daN4cat]

1133 supersedes this issue.

[daN4cat]

@odiea is this now fixed in master?

[odiea]

It appears that all reports and receipts are showing the same now. Looks good to me.

[meadlin]

What's the easiest way to apply the DB changes without losing data? I can try it with my data set here too.

[daN4cat]

Make sure you make a copy of your database.
Use the script in database folder, the one to upgrade from 3.0.2 to 3.1.0. Not sure what is the starting version but you may just need to use of that script the last part after the rewards upgrade to get the changes Steve introduced.

[meadlin]

K, thanks

[meadlin]

Register/Receipt look good:

Detailed, summary and takings are still off:

Unless I messed something up in the database update.

[SteveIreland]

I don't think you messed anything up

The focus of my corrections was on the application of payment to the amount due in the register panel and getting that number to calculate correctly every time (i.e. no rounding errors regardless of the affect of tax rounding ... regardless of the number of tax entities involved.

Reports are a different matter. A brief exchange suggested that I shouldn't try to provide a data migration script for existing data so I didn't. And because I didn't provide a data migration script I could not change the SQL for the reports - so the reports are still impacted by potential rounding errors.

I should be capturing the correct tax amount on the sales_taxes table, but reports are still being driven by recalculating the sales tax via the sales_item_taxes table and that still has the possibility of introducing rounding errors. After about a year I was going to come back and ask to see if there was any interest in migrating the reports over to use the sales_tax tables instead of the sales_item_tax table (since everyone would by then have a good year of sales history).

On a related note, the sales tax is still a single tax rate regardless of the number of tax jurisdictions in play. I haven't seen anything to suggest that this isn't acceptable and I've been working on tax systems for a long time.

All indications are that no one wants to burden OSPOS with back office tax, payments, expense tracking and profit reporting. So I backed off of breaking out the tax rates by tax jurisdiction and I'm deferring tax reporting to an external module (which I will start development on this month). This will break out the sales taxes to collected so that appropriate tax reporting and payment can take place. Of course, I'll be primarily focused on making it work for my client, but I will also include support for any other state specific requirements for anyone who wants to try it out when it's finished.

[daN4cat]

@SteveIreland I was about to write this morning too because I'm really confused.
We agreed that your tax rework will replace all the current tax calculations and have one approach to all.
I'm double confused by your statement about the migration, I said at that time that yes you need to provide a migration step in the database script upgrade.

I'm afraid that if we don't rework totally this tax story throughout the current code, it will be just an half baked solution and I'm not sure it's what we want to release in 3.1.0.

Could you please check the rest of the code and migrate all tax codes and taxes to the new system and have reports working properly?
Reports use one base class and function, so it should be easy to do one fix for all.

I'm fine with the tax calculation story as office backend story.

[jekkos]

I also think that we should do it all or nothing. I'd rather stick with a more crippled but consistent functionality instead of having something that's half done. I do need to have a look at the code as I'm a bit left behind there atm

[SteveIreland]

Okay. By the end of this week I'll have an option for migrating prior data and modify the tax reporting to use the new sales_tax table.

[daN4cat]

@SteveIreland sounds good. Thanks.

[SteveIreland]

I'm going to go ahead a split this into two pull requests. I finally have the sales tax history migration working in a somewhat reasonable way. I spent far too much time trying to make it elegant and never achieved nirvana. So I reverted back to something a little more simple, but I think might be acceptable.

I'll submit the pull request for the migration today with an explanation as to how to run it. It's simple but may not be intuitive.

If the migration meets with majority approval (does it need to be a super majority?) then I'll proceed with the changes to the reporting that will use the new tax data. I just don't want to break anything before it's ready.

[daN4cat]

@SteveIreland I'll test tomorrow morning the migration story.

[daN4cat]

I believe #1133 is covering any remaining issue with the tax rework and calculation