Bug 18736 - Problems in order calculations (rounding errors)
Summary: Problems in order calculations (rounding errors)
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 13321
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-06 14:36 UTC by Nick Clemens
Modified: 2018-02-22 15:27 UTC (History)
12 users (show)

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


Attachments
Bug 18736 - Problems in order calculations (rounding errors) (4.36 KB, patch)
2017-06-06 14:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Correctly handle rounding (17.75 KB, patch)
2017-06-06 19:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18736 - Add OrerPriceRounding syspref (3.14 KB, patch)
2017-12-28 16:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736 - Use rounding syspref to determine correct prices in calculations (20.85 KB, patch)
2017-12-28 16:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736 - Unit tests for rounding (5.64 KB, patch)
2017-12-28 16:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Followup - Fix SQL and uncomment db update (2.66 KB, patch)
2018-01-19 17:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) fix wrong call to _get_rounding_sql (1.65 KB, patch)
2018-01-30 15:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736 (follow-up) Use CAST instead of CEIL for appropriate rounding (2.24 KB, patch)
2018-01-30 15:42 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736 - Add OrerPriceRounding syspref (3.14 KB, patch)
2018-02-14 18:14 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18736 - Use rounding syspref to determine correct prices in calculations (20.91 KB, patch)
2018-02-14 18:14 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18736 - Unit tests for rounding (5.64 KB, patch)
2018-02-14 18:14 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18736: Followup - Fix SQL and uncomment db update (2.66 KB, patch)
2018-02-14 18:14 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18736: (follow-up) fix wrong call to _get_rounding_sql (1.67 KB, patch)
2018-02-14 18:14 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18736 (follow-up) Use CAST instead of CEIL for appropriate rounding (2.24 KB, patch)
2018-02-14 18:14 UTC, M. Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2017-06-06 14:36:49 UTC
The way we are calculating and/or storing our prices in acquisitions is causing some odd issues.

For example:

Place an order (no tax, let's not even deal with that right now please)
 listprice/rrp = 16.99
 discount = 42%
 quantity = 8

estimated calculated at 9.85

but order total is 78.83, but 8 times 9.85 = 78.80
Comment 1 Nick Clemens 2017-06-06 14:38:32 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2017-06-06 14:46:37 UTC
The issue we need to decide here is whether we store the extra digits and correct calculations based on these, or store the price with 2 digits precision
Comment 3 Marc Véron 2017-06-06 14:53:26 UTC
We have a similar issue with fines:
Bug 15741 - Incorrect rounding in total fines calculations 
(see comment #12)
Bug 17140 - Incorrect rounding in total fines calculations, part 2
Comment 4 Jonathan Druart 2017-06-06 19:11:33 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2017-06-06 19:14:35 UTC
Here is my try, for discussion.
We must understand and fix everything in one go, we need people that have been involved in bug 13321 and related to confirm the correct behaviours.
See also bug 18639.
Comment 6 Nick Clemens 2017-06-07 12:53:56 UTC Comment hidden (obsolete)
Comment 7 Koha Team Lyon 3 2017-06-20 12:03:04 UTC
Hi,
what we need in our library is to have the more precise price for order and the exact invoice price. So we sould surely store precise result rather than rounded price.
But, people will not understand if we display rounded prices and if the total is calculated on prices with 4 decimals...
Perhaps, we souhld display more precise price (4 decimals ?)...

I'm not really sure to help in this discussion because I feel that I miss some technical points.

Sonia
Comment 8 Nick Clemens 2017-06-26 14:43:33 UTC
Hi Sonia,

So do your invoices from vendors include portions of a cent? 

It seems most libraries/vendors round to nearest penny in the final calculations so not rounding causes errors. If you have the opposite situation perhaps we need a system preference to control exact vs. rounded prices?

-Nick

(In reply to Koha Team Lyon 3 from comment #7)
> Hi,
> what we need in our library is to have the more precise price for order and
> the exact invoice price. So we sould surely store precise result rather than
> rounded price.
> But, people will not understand if we display rounded prices and if the
> total is calculated on prices with 4 decimals...
> Perhaps, we souhld display more precise price (4 decimals ?)...
> 
> I'm not really sure to help in this discussion because I feel that I miss
> some technical points.
> 
> Sonia
Comment 9 Marc Véron 2017-06-26 16:15:52 UTC
(In reply to Nick Clemens from comment #8)
> Hi Sonia,
> 
> So do your invoices from vendors include portions of a cent? 
> 
> It seems most libraries/vendors round to nearest penny in the final
> calculations so not rounding causes errors. If you have the opposite
> situation perhaps we need a system preference to control exact vs. rounded
> prices?
> 
> -Nick
> 
> (In reply to Koha Team Lyon 3 from comment #7)
> > Hi,
> > what we need in our library is to have the more precise price for order and
> > the exact invoice price. So we sould surely store precise result rather than
> > rounded price.
> > But, people will not understand if we display rounded prices and if the
> > total is calculated on prices with 4 decimals...
> > Perhaps, we souhld display more precise price (4 decimals ?)...
> > 
> > I'm not really sure to help in this discussion because I feel that I miss
> > some technical points.
> > 
> > Sonia

To make things a little bit more complicated: 
Switzerland and Finland round to 5 cents. 
    1,000–1,024 → 1,00
    1,025–1,074 → 1,05
    1,075–1,099 → 1,10

See: https://de.wikipedia.org/wiki/Rundung 
("Rappenrundung": 5 Rappen (cents) is the smallest coin in Switzerland)
Comment 10 Tomás Cohen Arazi 2017-06-26 17:51:07 UTC
I think prices should be stored in 4 decimal digits precision, and so calculations made. Then for display purposes we could provide a way to specify how rounding should be done (number of digits).
Comment 11 Katrin Fischer 2017-06-27 20:57:42 UTC
I am not sure if this is only a display issue. Thinking of the calculations we do like adding up the values of all orders to determine the available money. If you store exactly, display rounded, that will never 'add up correctly' and will make people wonder. I wonder how other ILS handle this?
Comment 12 Koha Team Lyon 3 2017-07-12 09:31:09 UTC
(In reply to Nick Clemens from comment #8)
> Hi Sonia,
> 
> So do your invoices from vendors include portions of a cent? 
> 
Hi Nick,
Our invoice amount looks like 1453.78 for example.

But our colleagues often say us that there's few penny difference between Koha and the invoice amount.

It seems really tricky to have the exact calculation so I suppose that rounding only at the last step of calculation (addition of prices of an invoice) could reduce rounding error ?


> It seems most libraries/vendors round to nearest penny in the final
> calculations so not rounding causes errors. If you have the opposite
> situation perhaps we need a system preference to control exact vs. rounded
> prices?

Yes, as Finland and Switserland are other rounding pratices, a syspref could be a solution...
Sonia
Comment 13 Marc Véron 2017-08-06 09:11:57 UTC Comment hidden (obsolete)
Comment 14 Nick Clemens 2017-12-28 16:40:35 UTC
Created attachment 70187 [details] [review]
Bug 18736 - Add OrerPriceRounding syspref
Comment 15 Nick Clemens 2017-12-28 16:40:39 UTC
Created attachment 70188 [details] [review]
Bug 18736 - Use rounding syspref to determine correct prices in calculations

To test:
Place an order (no tax just for simplicity)
 listprice/rrp = 16.99
 discount = 42%
 quantity = 8
 estimated calculated at 9.85
 but order total is 78.83, but 8 times 9.85 = 78.80
Apply patches, set OrderPriceRounding syspref to 'Nearest cent'
Not order total is now as expected
View ordered.pl and confirm values are correct
Complete order, view invoice and confirm values
View spent.pl and confirm values
Go through acquisitions module and confirm prices throughout are
correct.
Comment 16 Nick Clemens 2017-12-28 16:40:42 UTC
Created attachment 70189 [details] [review]
Bug 18736 - Unit tests for rounding

Unit tests to highlight the issue

To test:
Prove t/db_dependent/Budgets.t
Comment 17 Nick Clemens 2017-12-28 16:43:24 UTC
This is an attempt to repair the calculations here by providing a system preference. I only add a 'nearest_cent' option, but I tried to leave things open for further options to be added later.

Please test thouroughly and let me know any feedback.
Comment 18 M. Tompsett 2018-01-17 18:22:22 UTC
Comment on attachment 70187 [details] [review]
Bug 18736 - Add OrerPriceRounding syspref

Review of attachment 70187 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/atomicupdate/bug18736_add_rounding_syspref.perl
@@ +1,3 @@
> +$DBversion = 'XXX';  # will be replaced by the RM
> +if( CheckVersion( $DBversion ) ) {
> +    # $dbh->do( "INSERT INTO systempreferences (variable,value,explanation,options,type) VALUES ('OrderPriceRounding',NULL,'Local preference for rounding orders before calculations to ensure correct calculations','|nearest_cent','Choice')" );

This line is commented out. Why?
Comment 19 M. Tompsett 2018-01-17 18:33:45 UTC
Comment on attachment 70188 [details] [review]
Bug 18736 - Use rounding syspref to determine correct prices in calculations

Review of attachment 70188 [details] [review]:
-----------------------------------------------------------------

::: C4/Acquisition.pm
@@ +2010,5 @@
> +
> +sub _get_rounding_sql {
> +    my $round_string = @_;
> +    my $rounding_pref = C4::Context->preference('OrderPriceRounding');
> +    if ( $rounding_pref eq "nearest_cent"  ) { return ("ROUND($round_string,2)"); }

ROUND() is not ANSI SQL.
CEIL($round_string*100)/100 would be ANSI SQL 2003, I believe.

::: C4/Budgets.pm
@@ +1291,5 @@
> +
> +sub _get_rounding_sql {
> +    my $to_round = shift;
> +    my $rounding_pref = C4::Context->preference('OrderPriceRounding');
> +    if   ($rounding_pref eq 'nearest_cent') { return "ROUND($to_round,2)"; }

Same problem here. Not ANSI SQL.
Comment 20 Nick Clemens 2018-01-19 17:57:01 UTC
Created attachment 70768 [details] [review]
Bug 18736: Followup - Fix SQL and uncomment db update
Comment 21 Nick Clemens 2018-01-30 15:07:45 UTC
Created attachment 71062 [details] [review]
Bug 18736: (follow-up) fix wrong call to _get_rounding_sql
Comment 22 Nick Clemens 2018-01-30 15:42:05 UTC
Created attachment 71063 [details] [review]
Bug 18736 (follow-up) Use CAST instead of CEIL for appropriate rounding
Comment 23 M. Tompsett 2018-01-30 15:57:49 UTC
The only thing lacking is test coverage of C4::Budgets::GetBudgetsPlanCell, where some changes were made. This would be a Failed QA, but I know Nick is creating an improve test coverage bug elsewhere to link here, right? :)
Comment 24 M. Tompsett 2018-02-14 18:11:32 UTC
Rebasing.
Comment 25 M. Tompsett 2018-02-14 18:14:31 UTC
Created attachment 71627 [details] [review]
Bug 18736 - Add OrerPriceRounding syspref
Comment 26 M. Tompsett 2018-02-14 18:14:36 UTC
Created attachment 71628 [details] [review]
Bug 18736 - Use rounding syspref to determine correct prices in calculations

To test:
Place an order (no tax just for simplicity)
 listprice/rrp = 16.99
 discount = 42%
 quantity = 8
 estimated calculated at 9.85
 but order total is 78.83, but 8 times 9.85 = 78.80
Apply patches, set OrderPriceRounding syspref to 'Nearest cent'
Not order total is now as expected
View ordered.pl and confirm values are correct
Complete order, view invoice and confirm values
View spent.pl and confirm values
Go through acquisitions module and confirm prices throughout are
correct.
Comment 27 M. Tompsett 2018-02-14 18:14:40 UTC
Created attachment 71629 [details] [review]
Bug 18736 - Unit tests for rounding

Unit tests to highlight the issue

To test:
Prove t/db_dependent/Budgets.t
Comment 28 M. Tompsett 2018-02-14 18:14:44 UTC
Created attachment 71630 [details] [review]
Bug 18736: Followup - Fix SQL and uncomment db update
Comment 29 M. Tompsett 2018-02-14 18:14:49 UTC
Created attachment 71631 [details] [review]
Bug 18736: (follow-up) fix wrong call to _get_rounding_sql
Comment 30 M. Tompsett 2018-02-14 18:14:53 UTC
Created attachment 71632 [details] [review]
Bug 18736 (follow-up) Use CAST instead of CEIL for appropriate rounding
Comment 31 M. Tompsett 2018-02-14 18:50:27 UTC
The only thing lacking is test coverage of C4::Budgets::GetBudgetsPlanCell, where some changes were made. As a result, this is Failed QA.
Comment 32 Jonathan Druart 2018-02-14 21:24:36 UTC
(In reply to M. Tompsett from comment #31)
> The only thing lacking is test coverage of C4::Budgets::GetBudgetsPlanCell,
> where some changes were made. As a result, this is Failed QA.

Does it mean you tested everything and it works as you expect?
Comment 33 M. Tompsett 2018-02-15 00:21:51 UTC
(In reply to Jonathan Druart from comment #32)
> Does it mean you tested everything and it works as you expect?

No, that was from an eyeball stance only.
I just tested what was there, and my Total tax exc. (USD) column is 8?!
Regardless of system preference setting.
There is a mistake somewhere.
Comment 34 Koha Team Lyon 3 2018-02-22 15:27:58 UTC
Hello,
if someone is OK to work on this before the next hackfest in March, I'm OK to test it during the hackfest.
Sonia