Bug 14541 - Tax rate should not be forced to an arbitrary precision
Summary: Tax rate should not be forced to an arbitrary precision
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 10253 (view as bug list)
Depends on:
Blocks: 14543
  Show dependency treegraph
 
Reported: 2015-07-16 15:15 UTC by Kyle M Hall
Modified: 2017-01-02 10:45 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14541 - Tax rate should not be forced to an arbitrary precision (2.08 KB, patch)
2015-07-16 15:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14541 - Tax rate should not be forced to an arbitrary precision (2.16 KB, patch)
2015-07-18 04:21 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 14541: Do not truncate tax rate values (9.14 KB, patch)
2016-11-09 09:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14541: Do not truncate tax rate values (9.19 KB, patch)
2016-12-02 01:45 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 14541: Do not truncate tax rate values (9.25 KB, patch)
2016-12-08 17:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14541 (QA followup) Add warning to gist system preference (1.20 KB, patch)
2016-12-08 17:20 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2015-07-16 15:15:21 UTC
The following is an example gstrate for a real library
0|.08625|.0863|.085|.09|.0825|.0875|.075|.07625

Note the .08625 and .0863

Both of those rates show up as 8.6% for the order line editor ( acqui/neworderempty.pl ).

There is no reason to force these numbers to be rounded and in fact makes the label not match the value!
Comment 1 Kyle M Hall 2015-07-16 15:17:48 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2015-07-16 15:19:03 UTC
Upping severity as this deals with money. It's possible for a librarian to accidentally select the wrong tax rate due to this bug!
Comment 3 Aleisha Amohia 2015-07-17 02:40:03 UTC
Patch applied cleanly and works as expected, signing off
Comment 4 Amit Gupta 2015-07-18 04:21:08 UTC
Created attachment 41075 [details] [review]
Bug 14541 - Tax rate should not be forced to an arbitrary  precision

Work without error.
Comment 5 Jonathan Druart 2015-08-05 10:24:04 UTC
Kyle,
The tax rate is displayed at several places, at least acqui/supplier.tt.
The change should affect all the places where it is displayed.
You should also have a look at how I display them on bug 13323 using a template plugin: [% order.gstrate * 100 | $Price %]

I can provide a patch on top of bug 13323 if you can wait for it to be pushed.
Comment 6 Katrin Fischer 2015-08-09 22:29:50 UTC
Hm, is the Price plugin the right one there? Because it's not an amount but a percentage value?
Comment 7 Jonathan Druart 2015-08-10 08:28:07 UTC
(In reply to Katrin Fischer from comment #6)
> Hm, is the Price plugin the right one there? Because it's not an amount but
> a percentage value?

I'd say yes, we could add an option to the plugin later (something like "display as %").
Comment 8 Katrin Fischer 2015-08-10 08:39:19 UTC
For me Price feels not right - and only 2 decimals I think? not enough for the percentage?
Comment 9 Jonathan Druart 2015-08-10 09:25:21 UTC
(In reply to Katrin Fischer from comment #8)
> For me Price feels not right - and only 2 decimals I think? not enough for
> the percentage?

It's better than 1, and the patch suggested to set it to 2, what the plugin does.
Comment 10 Katrin Fischer 2015-08-10 09:34:36 UTC
Hm, but is 2 enough? Int wouldn't be for Kyle's example. Maybe we should just display 'as it is configured' and not cut it off. .07625 would be 7.625 I think?
Comment 11 Kyle M Hall 2015-08-28 14:08:15 UTC
(In reply to Jonathan Druart from comment #5)
> Kyle,
> The tax rate is displayed at several places, at least acqui/supplier.tt.
> The change should affect all the places where it is displayed.
> You should also have a look at how I display them on bug 13323 using a
> template plugin: [% order.gstrate * 100 | $Price %]
> 
> I can provide a patch on top of bug 13323 if you can wait for it to be
> pushed.

That works for me Jonathan!
Comment 12 Kyle M Hall 2016-11-07 17:14:38 UTC
Jonathan, now that bug 13323 has been taken care of, can you follow up on this? Thanks!

(In reply to Kyle M Hall from comment #11)
> (In reply to Jonathan Druart from comment #5)
> > Kyle,
> > The tax rate is displayed at several places, at least acqui/supplier.tt.
> > The change should affect all the places where it is displayed.
> > You should also have a look at how I display them on bug 13323 using a
> > template plugin: [% order.gstrate * 100 | $Price %]
> > 
> > I can provide a patch on top of bug 13323 if you can wait for it to be
> > pushed.
> 
> That works for me Jonathan!
Comment 13 Jonathan Druart 2016-11-09 09:17:19 UTC
Created attachment 57363 [details] [review]
Bug 14541: Do not truncate tax rate values

Tax rates are stored in decimal(6,4) fields which means that 4 decimals
are allowed.
If a tax rate is 8.42%, it is stored as 0.0842
If a tax rate has more precision than that, Koha won't deal correctly
with it. We will need to update the DB structure.

With this patch, the tax rate will be displayed with the same precision
as in the DB. So if you enter 8.42, you will see 8.42% instead of 8.4%
without this patch.

Test plan:
Do a full acquisition workflow with a tax rate like 8.42% and confirm
that it is correctly displayed.
Comment 14 Jonathan Druart 2016-11-09 09:18:29 UTC
*** Bug 10253 has been marked as a duplicate of this bug. ***
Comment 15 Alex Buckley 2016-12-02 01:45:57 UTC
Created attachment 57872 [details] [review]
Bug 14541: Do not truncate tax rate values

Tax rates are stored in decimal(6,4) fields which means that 4 decimals
are allowed.
If a tax rate is 8.42%, it is stored as 0.0842
If a tax rate has more precision than that, Koha won't deal correctly
with it. We will need to update the DB structure.

With this patch, the tax rate will be displayed with the same precision
as in the DB. So if you enter 8.42, you will see 8.42% instead of 8.4%
without this patch.

Test plan:
Do a full acquisition workflow with a tax rate like 8.42% and confirm
that it is correctly displayed.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 16 Nick Clemens 2016-12-07 12:01:17 UTC
This works, one issue is that the pref allows you to enter any number of decimals. These are then displayed in full on the dropdowns, however, they are saved into the db truncated.

Example:

In gist preference: .12345

Shows in dropdowns as: 12.345%

Saves to db as: .1235

Shows in order/basket tables as: 12.35%


We should either truncate the dropdowns or add a warning in the system preference
Comment 17 Kyle M Hall 2016-12-07 16:17:46 UTC
(In reply to Nick Clemens from comment #16)
> This works, one issue is that the pref allows you to enter any number of
> decimals. These are then displayed in full on the dropdowns, however, they
> are saved into the db truncated.
> 
> Example:
> 
> In gist preference: .12345
> 
> Shows in dropdowns as: 12.345%
> 
> Saves to db as: .1235
> 
> Shows in order/basket tables as: 12.35%
> 
> 
> We should either truncate the dropdowns or add a warning in the system
> preference

The database is limited to 4 digits of precision. I think for the time being a warning on the system preference should be sufficient. I *do* think the order/basket tables should show 12.345%. We should not be rounding where we don't need to.
Comment 18 Kyle M Hall 2016-12-07 16:27:34 UTC
> The database is limited to 4 digits of precision. I think for the time being
> a warning on the system preference should be sufficient. I *do* think the
> order/basket tables should show 12.345%. We should not be rounding where we
> don't need to.

Had a brain fart there. 12.245% would be truncated because of the db limit. As such, only the warning is necessary.
Comment 19 Nick Clemens 2016-12-08 17:20:11 UTC
Created attachment 58050 [details] [review]
Bug 14541: Do not truncate tax rate values

Tax rates are stored in decimal(6,4) fields which means that 4 decimals
are allowed.
If a tax rate is 8.42%, it is stored as 0.0842
If a tax rate has more precision than that, Koha won't deal correctly
with it. We will need to update the DB structure.

With this patch, the tax rate will be displayed with the same precision
as in the DB. So if you enter 8.42, you will see 8.42% instead of 8.4%
without this patch.

Test plan:
Do a full acquisition workflow with a tax rate like 8.42% and confirm
that it is correctly displayed.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 20 Nick Clemens 2016-12-08 17:20:18 UTC
Created attachment 58051 [details] [review]
Bug 14541 (QA followup) Add warning to gist system preference

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 21 Kyle M Hall 2016-12-09 16:29:47 UTC
Pushed to master for 17.05, thanks Jonathan, Nick!
Comment 22 Katrin Fischer 2016-12-12 22:20:21 UTC
These patches have been pushed to 16.11.x, will be in 16.11.01.
Comment 23 Julian Maurice 2017-01-02 10:45:00 UTC
Pushed to 3.22.x for 3.22.14