Bug 18736

Summary: Problems in order calculations (rounding errors)
Product: Koha Reporter: Nick Clemens <nick>
Component: AcquisitionsAssignee: Nick Clemens <nick>
Status: Failed QA --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: critical    
Priority: P5 - low CC: colin.campbell, david.roberts, fiona.borthwick, francois.charbonnier, jonathan.druart, josef.moravec, julian.maurice, katrin.fischer, koha, kyle, laurence.rault, m.de.rooy, martin.renvoize, mtompset, nicolas.legrand, severine.queune, sonia.bouis, tomascohen, veron
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18639
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17140
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15741
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21427
Whiteboard:
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:
Bug Depends on: 13321    
Bug Blocks:    
Attachments: Bug 18736 - Problems in order calculations (rounding errors)
Bug 18736: Correctly handle rounding
Bug 18736 - Add OrerPriceRounding syspref
Bug 18736 - Use rounding syspref to determine correct prices in calculations
Bug 18736 - Unit tests for rounding
Bug 18736: Followup - Fix SQL and uncomment db update
Bug 18736: (follow-up) fix wrong call to _get_rounding_sql
Bug 18736 (follow-up) Use CAST instead of CEIL for appropriate rounding
Bug 18736 - Add OrerPriceRounding syspref
Bug 18736 - Use rounding syspref to determine correct prices in calculations
Bug 18736 - Unit tests for rounding
Bug 18736: Followup - Fix SQL and uncomment db update
Bug 18736: (follow-up) fix wrong call to _get_rounding_sql
Bug 18736 (follow-up) Use CAST instead of CEIL for appropriate rounding
Bug 18736: Add OrderPriceRounding syspref
Bug 18736: Use rounding syspref to determine correct prices in calculations
Bug 18736: Unit tests for rounding
Bug 18736: (follow-up) - Fix SQL and uncomment db update
Bug 18736: (follow-up) fix wrong call to _get_rounding_sql
Bug 18736: (follow-up) Use CAST instead of CEIL for appropriate rounding
Bug 18736: (follow-up) Correctly assign values in subroutines
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell
Bug 18736: Follow to prevent duplicate run noise
Bug 18736: Follow to clean up admin policy preference
Bug 18736: [DO NOT PUSH] C4/Acquisition print scaffolding.
Bug 18736: [DO NOT PUSH] Print scaffolding for C4/Budgets.pm
Bug 18736: Add missing test cases [BUGGY]
Bug 18736: (follow-up) Add missing test cases
Bug 18736: (follow-up) Undo changes to GetLateOrders
Bug 18736: Follow up tests
Bug 18736: Add OrderPriceRounding syspref
Bug 18736: Use rounding syspref to determine correct prices in calculations
Bug 18736: Unit tests for rounding
Bug 18736: (follow-up) - Fix SQL and uncomment db update
Bug 18736: (follow-up) fix wrong call to _get_rounding_sql
Bug 18736: (follow-up) Use CAST instead of CEIL for appropriate rounding
Bug 18736: (follow-up) Correctly assign values in subroutines
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell
Bug 18736: Follow to prevent duplicate run noise
Bug 18736: Follow to clean up admin policy preference
Bug 18736: (follow-up) Add missing test cases
Bug 18736: (follow-up) Undo changes to GetLateOrders
Bug 18736: (follow-up) Unit tests
Bug 18736: Add OrderPriceRounding syspref
Bug 18736: Use rounding syspref to determine correct prices in calculations
Bug 18736: Unit tests for rounding
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell
Bug 18736: (follow-up) Add missing test cases
Bug 18736: (follow-up) Fix missing rounding and bad formatting
Bug 18736: Unit tests doe GetBudgetHierarchy
Bug 18736: Add OrderPriceRounding syspref
Bug 18736: Use rounding syspref to determine correct prices in calculations
Bug 18736: Unit tests for rounding
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell
Bug 18736: (follow-up) Add missing test cases
Bug 18736: (follow-up) Fix missing rounding and bad formatting
Bug 18736: Unit tests for GetBudgetHierarchy
Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER
Bug 18736: Add OrderPriceRounding syspref
Bug 18736: Use rounding syspref to determine correct prices in calculations
Bug 18736: Unit tests for rounding
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell
Bug 18736: (follow-up) Add missing test cases
Bug 18736: (follow-up) Fix missing rounding and bad formatting
Bug 18736: Unit tests for GetBudgetHierarchy
Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER
Bug 18736: Add OrderPriceRounding syspref
Bug 18736: Use rounding syspref to determine correct prices in calculations
Bug 18736: Unit tests for rounding
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell
Bug 18736: (follow-up) Add missing test cases
Bug 18736: (follow-up) Fix missing rounding and bad formatting
Bug 18736: Unit tests for GetBudgetHierarchy
Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER
Bug 18736: (follow-up) Remove duplicate code
Bug 18736: (follow-up) Remove duplicate code and adjust tests
Bug 18736: (QA follow-up) Cosmetic changes
Bug 18736: (QA follow-up) Change to signed, add large int test

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
Comment 35 Katrin Fischer 2018-03-31 07:29:43 UTC
Should we move this to Needs Sign-off for another round of testing or tests first?
Comment 36 Nick Clemens 2018-04-17 14:42:39 UTC
Created attachment 74335 [details] [review]
Bug 18736: Add OrderPriceRounding syspref
Comment 37 Nick Clemens 2018-04-17 14:42:43 UTC
Created attachment 74336 [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 38 Nick Clemens 2018-04-17 14:42:47 UTC
Created attachment 74337 [details] [review]
Bug 18736: Unit tests for rounding

Unit tests to highlight the issue

To test:
Prove t/db_dependent/Budgets.t
Comment 39 Nick Clemens 2018-04-17 14:42:50 UTC
Created attachment 74338 [details] [review]
Bug 18736: (follow-up) - Fix SQL and uncomment db update
Comment 40 Nick Clemens 2018-04-17 14:42:54 UTC
Created attachment 74339 [details] [review]
Bug 18736: (follow-up) fix wrong call to _get_rounding_sql
Comment 41 Nick Clemens 2018-04-17 14:42:58 UTC
Created attachment 74340 [details] [review]
Bug 18736: (follow-up) Use CAST instead of CEIL for appropriate rounding
Comment 42 Nick Clemens 2018-04-17 14:43:01 UTC
Created attachment 74341 [details] [review]
Bug 18736: (follow-up) Correctly assign values in subroutines
Comment 43 Nick Clemens 2018-04-17 14:43:05 UTC
Created attachment 74342 [details] [review]
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell

prove t/db_dependent/Budgets.t
Comment 44 M. Tompsett 2018-04-17 17:15:05 UTC
Created attachment 74370 [details] [review]
Bug 18736: Follow to prevent duplicate run noise

TEST PLAN
---------
update the database twice
-- noise
apply patch
update the database
-- no noise
Comment 45 M. Tompsett 2018-04-17 17:39:19 UTC
Created attachment 74371 [details] [review]
Bug 18736: Follow to clean up admin policy preference

Two preferences were merged into the same box. This likely
was unintended. This reformats them.

TEST PLAN
---------
Go to Acquisitions tab of the system preference
-- in the policy section PurgeSuggestionsOlderThan and
   OrderPriceRounding are in the same box
Apply this patch
Refresh the page
-- OrderPriceRounding is in its own box, and better formatted.
Comment 46 M. Tompsett 2018-04-17 18:07:30 UTC
Need test for GetLateOrders with both settings of OrderPriceRounding.
Other feedback still may follow. I'll summarize in one final comment afterwards.

It would be nice to also trigger the else case for the dbdriver, but that might take some expert mocking, which is low priority as mysql/mariadb are the preferred db drivers.
Comment 47 M. Tompsett 2018-04-17 18:09:21 UTC
Created attachment 74374 [details] [review]
Bug 18736: [DO NOT PUSH] C4/Acquisition print scaffolding.

Need to trigger all the print STDERR "C4/Acquisition.pm...
cases.
Comment 48 M. Tompsett 2018-04-17 23:39:40 UTC
Created attachment 74397 [details] [review]
Bug 18736: [DO NOT PUSH] Print scaffolding for C4/Budgets.pm

The ELSE case is a pain. Please get it to return something
rather than undef.
Comment 49 M. Tompsett 2018-04-17 23:42:58 UTC
Created attachment 74398 [details] [review]
Bug 18736: Add missing test cases [BUGGY]

The ELSE case is flawed. This needs fixing.
The third parameter is budgets, and needs to
be valid. It currently is passing, because
undef+0= 0, and the expected values are set
to zero. They should be non-zero!
Comment 50 Nick Clemens 2018-04-18 11:13:57 UTC
Created attachment 74422 [details] [review]
Bug 18736: (follow-up) Add missing test cases

We now fully cover GetBudgetsPlanCell in these tests
Comment 51 Nick Clemens 2018-04-18 11:39:53 UTC
Created attachment 74427 [details] [review]
Bug 18736: (follow-up) Undo changes to GetLateOrders

GetLateOrders calculates subtotal based on rrp which is a 2 decimal
precision field. If we need more precision here it should be fixed on a
separate bug
Comment 52 M. Tompsett 2018-04-18 14:41:07 UTC
Created attachment 74438 [details] [review]
Bug 18736: Follow up tests

Test coverage for new C4::Acquisition functions.

prove t/Acquisition/_get_rounding_sql.t
prove t/Acquisition/get_rounded_price.t
Comment 53 M. Tompsett 2018-04-18 14:42:37 UTC
All Acquisition.pm and Budgets.pm changes are fully covered. Obsoleting the scaffolding.
Comment 54 Jonathan Druart 2018-04-18 14:47:32 UTC
(In reply to M. Tompsett from comment #52)
> prove t/Acquisition/_get_rounding_sql.t
> prove t/Acquisition/get_rounded_price.t

Create subtests in t/Acquisition.t instead.
Comment 55 M. Tompsett 2018-04-18 14:49:40 UTC
(In reply to Marc Véron from comment #9)
> 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)

Canada has something similar, but only when paying with cash.
1.00-1.02 -> 1.00
1.03-1.07 -> 1.05
1.08-1.12 -> 1.10

But I think improving the rounding for 5 cent issues should be another followup bug on this.
Comment 56 Colin Campbell 2018-05-02 09:51:40 UTC
Need to test with year end process.
Comment 57 Martin Renvoize 2018-07-11 19:31:15 UTC
Sorry guys, can't get this set to apply right now: 

Applying: Bug 18736: (follow-up) Undo changes to GetLateOrders
error: sha1 information is lacking or useless (C4/Acquisition.pm).
error: could not build fake ancestor
Comment 58 Nick Clemens 2018-07-16 11:43:34 UTC
Created attachment 76979 [details] [review]
Bug 18736: Add OrderPriceRounding syspref
Comment 59 Nick Clemens 2018-07-16 11:43:39 UTC
Created attachment 76980 [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 60 Nick Clemens 2018-07-16 11:44:12 UTC
Created attachment 76981 [details] [review]
Bug 18736: Unit tests for rounding

Unit tests to highlight the issue

To test:
Prove t/db_dependent/Budgets.t
Comment 61 Nick Clemens 2018-07-16 11:44:17 UTC
Created attachment 76982 [details] [review]
Bug 18736: (follow-up) - Fix SQL and uncomment db update
Comment 62 Nick Clemens 2018-07-16 11:44:22 UTC
Created attachment 76983 [details] [review]
Bug 18736: (follow-up) fix wrong call to _get_rounding_sql
Comment 63 Nick Clemens 2018-07-16 11:44:28 UTC
Created attachment 76984 [details] [review]
Bug 18736: (follow-up) Use CAST instead of CEIL for appropriate rounding
Comment 64 Nick Clemens 2018-07-16 11:44:33 UTC
Created attachment 76985 [details] [review]
Bug 18736: (follow-up) Correctly assign values in subroutines
Comment 65 Nick Clemens 2018-07-16 11:44:38 UTC
Created attachment 76986 [details] [review]
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell

prove t/db_dependent/Budgets.t
Comment 66 Nick Clemens 2018-07-16 11:44:43 UTC
Created attachment 76987 [details] [review]
Bug 18736: Follow to prevent duplicate run noise

TEST PLAN
---------
update the database twice
-- noise
apply patch
update the database
-- no noise
Comment 67 Nick Clemens 2018-07-16 11:44:48 UTC
Created attachment 76988 [details] [review]
Bug 18736: Follow to clean up admin policy preference

Two preferences were merged into the same box. This likely
was unintended. This reformats them.

TEST PLAN
---------
Go to Acquisitions tab of the system preference
-- in the policy section PurgeSuggestionsOlderThan and
   OrderPriceRounding are in the same box
Apply this patch
Refresh the page
-- OrderPriceRounding is in its own box, and better formatted.
Comment 68 Nick Clemens 2018-07-16 11:44:53 UTC
Created attachment 76989 [details] [review]
Bug 18736: (follow-up) Add missing test cases

We now fully cover GetBudgetsPlanCell in these tests
Comment 69 Nick Clemens 2018-07-16 11:44:59 UTC
Created attachment 76990 [details] [review]
Bug 18736: (follow-up) Undo changes to GetLateOrders

GetLateOrders calculates subtotal based on rrp which is a 2 decimal
precision field. If we need more precision here it should be fixed on a
separate bug
Comment 70 Nick Clemens 2018-07-16 11:45:04 UTC
Created attachment 76991 [details] [review]
Bug 18736: (follow-up) Unit tests

Test coverage for new C4::Acquisition functions.

prove t/Acquisition.t
Comment 71 Nick Clemens 2018-07-16 11:47:49 UTC
Applied cleanly on master for me - I moved additional tests to subtest in Acquisition.t
Comment 72 Jonathan Druart 2018-07-31 12:08:45 UTC
There is no signoff! Please test!
Comment 73 Julian Maurice 2018-08-09 09:03:05 UTC
Patches do not apply. Please rebase
Comment 74 Nick Clemens 2018-08-09 10:30:15 UTC
Created attachment 77614 [details] [review]
Bug 18736: Add OrderPriceRounding syspref
Comment 75 Nick Clemens 2018-08-09 10:30:22 UTC
Created attachment 77615 [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 76 Nick Clemens 2018-08-09 10:30:27 UTC
Created attachment 77616 [details] [review]
Bug 18736: Unit tests for rounding

Unit tests to highlight the issue

To test:
Prove t/db_dependent/Budgets.t
prove t/Acquisition.t
Comment 77 Nick Clemens 2018-08-09 10:30:32 UTC
Created attachment 77617 [details] [review]
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell

prove t/db_dependent/Budgets.t
Comment 78 Nick Clemens 2018-08-09 10:30:38 UTC
Created attachment 77618 [details] [review]
Bug 18736: (follow-up) Add missing test cases

We now fully cover GetBudgetsPlanCell in these tests
Comment 79 Julian Maurice 2018-08-09 11:51:05 UTC
There are a few problems:
- 'use C4::Acquisition' is missing in acqui/spent.pl
- acqui/order.pl uses aqorders.ecost, which for me is already rounded in the database ('9.85'), so the amount doesn't change when I change the syspref value (it's like syspref is always "Round")
- same for reports/orders_by_fund.pl
- In admin/aqbudgets.pl, the amount doesn't change when I change the syspref value (it's like syspref is always "Don't round")
- The result of get_rounded_price is often used in multiplication, but this is wrong because get_rounded_price use Koha::Number::Format and this can return strings that are not valid numbers (ex: "1,234.000")
Comment 80 Nick Clemens 2018-08-24 01:27:12 UTC
Created attachment 78101 [details] [review]
Bug 18736: (follow-up) Fix missing rounding and bad formatting

This patch:
Adds a missing use
Uses 'Koha::Number::Price->round()' instead of 'format()' to ensure numeric
returns
Ensures too big numbers don't crash round()
Uses syspref in 'GetBudgetHierarchy'

To test:
Follow previous test plan
Check values on admin/aqbudgets.pl are affected by syspref
Ensure values throughout acquisitions are correctly calculated/displayed
(even when greater than 1,000)
Comment 81 Nick Clemens 2018-08-24 01:28:27 UTC
(In reply to Julian Maurice from comment #79)
> There are a few problems:
> - 'use C4::Acquisition' is missing in acqui/spent.pl

Fixed!

> - acqui/order.pl uses aqorders.ecost, which for me is already rounded in the
> database ('9.85'), so the amount doesn't change when I change the syspref
> value (it's like syspref is always "Round")
> - same for reports/orders_by_fund.pl

ecost is a 2 decimal precision number - beyond the scope here

> - In admin/aqbudgets.pl, the amount doesn't change when I change the syspref
> value (it's like syspref is always "Don't round")

Fixed

> - The result of get_rounded_price is often used in multiplication, but this
> is wrong because get_rounded_price use Koha::Number::Format and this can
> return strings that are not valid numbers (ex: "1,234.000")

I switched to round(), this should return numeric values
Comment 82 Nick Clemens 2018-08-24 11:12:28 UTC
Created attachment 78120 [details] [review]
Bug 18736: Unit tests doe GetBudgetHierarchy

prove -v t/db_dependent/Budgets.t
Comment 83 Marcel de Rooy 2018-09-07 06:27:15 UTC
Applying: Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell
fatal: sha1 information is lacking or useless (C4/Budgets.pm).
Comment 84 Nick Clemens 2018-09-16 18:49:19 UTC
Created attachment 78981 [details] [review]
Bug 18736: Add OrderPriceRounding syspref
Comment 85 Nick Clemens 2018-09-16 18:49:25 UTC
Created attachment 78982 [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 86 Nick Clemens 2018-09-16 18:49:31 UTC
Created attachment 78983 [details] [review]
Bug 18736: Unit tests for rounding

Unit tests to highlight the issue

To test:
Prove t/db_dependent/Budgets.t
prove t/Acquisition.t
Comment 87 Nick Clemens 2018-09-16 18:49:37 UTC
Created attachment 78984 [details] [review]
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell

prove t/db_dependent/Budgets.t
Comment 88 Nick Clemens 2018-09-16 18:49:44 UTC
Created attachment 78985 [details] [review]
Bug 18736: (follow-up) Add missing test cases

We now fully cover GetBudgetsPlanCell in these tests
Comment 89 Nick Clemens 2018-09-16 18:49:50 UTC
Created attachment 78986 [details] [review]
Bug 18736: (follow-up) Fix missing rounding and bad formatting

This patch:
Adds a missing use
Uses 'Koha::Number::Price->round()' instead of 'format()' to ensure numeric
returns
Ensures too big numbers don't crash round()
Uses syspref in 'GetBudgetHierarchy'

To test:
Follow previous test plan
Check values on admin/aqbudgets.pl are affected by syspref
Ensure values throughout acquisitions are correctly calculated/displayed
(even when greater than 1,000)
Comment 90 Nick Clemens 2018-09-16 18:49:56 UTC
Created attachment 78987 [details] [review]
Bug 18736: Unit tests for GetBudgetHierarchy

prove -v t/db_dependent/Budgets.t
Comment 91 Nick Clemens 2018-09-16 18:50:02 UTC
Created attachment 78988 [details] [review]
Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER
Comment 92 Julian Maurice 2018-09-19 09:52:04 UTC
Comment on attachment 78986 [details] [review]
Bug 18736: (follow-up) Fix missing rounding and bad formatting

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

::: Koha/Number/Price.pm
@@ +19,4 @@
>  
>  use Modern::Perl;
>  
> +use Number::Format qw( format_price round );

As these subs are used as Number::Format object's methods, I don't think importing them is necessary.
Comment 93 Julian Maurice 2018-09-19 09:54:57 UTC
(In reply to Nick Clemens from comment #91)
> Created attachment 78988 [details] [review] [review]
> Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER

Can you tell in the commit message why you are doing that ?
Comment 94 Nick Clemens 2018-09-19 12:01:19 UTC
Created attachment 79087 [details] [review]
Bug 18736: Add OrderPriceRounding syspref
Comment 95 Nick Clemens 2018-09-19 12:01:25 UTC
Created attachment 79088 [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 96 Nick Clemens 2018-09-19 12:01:30 UTC
Created attachment 79089 [details] [review]
Bug 18736: Unit tests for rounding

Unit tests to highlight the issue

To test:
Prove t/db_dependent/Budgets.t
prove t/Acquisition.t
Comment 97 Nick Clemens 2018-09-19 12:01:35 UTC
Created attachment 79090 [details] [review]
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell

prove t/db_dependent/Budgets.t
Comment 98 Nick Clemens 2018-09-19 12:01:40 UTC
Created attachment 79091 [details] [review]
Bug 18736: (follow-up) Add missing test cases

We now fully cover GetBudgetsPlanCell in these tests
Comment 99 Nick Clemens 2018-09-19 12:01:46 UTC
Created attachment 79092 [details] [review]
Bug 18736: (follow-up) Fix missing rounding and bad formatting

This patch:
Adds a missing use
Uses 'Koha::Number::Price->round()' instead of 'format()' to ensure numeric
returns
Ensures too big numbers don't crash round()
Uses syspref in 'GetBudgetHierarchy'

To test:
Follow previous test plan
Check values on admin/aqbudgets.pl are affected by syspref
Ensure values throughout acquisitions are correctly calculated/displayed
(even when greater than 1,000)
Comment 100 Nick Clemens 2018-09-19 12:01:51 UTC
Created attachment 79093 [details] [review]
Bug 18736: Unit tests for GetBudgetHierarchy

prove -v t/db_dependent/Budgets.t
Comment 101 Nick Clemens 2018-09-19 12:01:57 UTC
Created attachment 79094 [details] [review]
Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER

More recent MySql versions require the use of SIGNED or UNSIGNED rather
than INTEGER. UNSIGNED will still work in older installs
Comment 102 Julian Maurice 2018-09-19 12:37:51 UTC
Thanks Nick!

Reviewing this patchset once again, for the last time hopefully :)
Comment 103 Julian Maurice 2018-09-19 12:51:58 UTC
Created attachment 79098 [details] [review]
Bug 18736: Add OrderPriceRounding syspref

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 104 Julian Maurice 2018-09-19 12:52:05 UTC
Created attachment 79099 [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.

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 105 Julian Maurice 2018-09-19 12:52:13 UTC
Created attachment 79100 [details] [review]
Bug 18736: Unit tests for rounding

Unit tests to highlight the issue

To test:
Prove t/db_dependent/Budgets.t
prove t/Acquisition.t

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 106 Julian Maurice 2018-09-19 12:52:19 UTC
Created attachment 79101 [details] [review]
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell

prove t/db_dependent/Budgets.t

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 107 Julian Maurice 2018-09-19 12:52:26 UTC
Created attachment 79102 [details] [review]
Bug 18736: (follow-up) Add missing test cases

We now fully cover GetBudgetsPlanCell in these tests

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 108 Julian Maurice 2018-09-19 12:52:32 UTC
Created attachment 79103 [details] [review]
Bug 18736: (follow-up) Fix missing rounding and bad formatting

This patch:
Adds a missing use
Uses 'Koha::Number::Price->round()' instead of 'format()' to ensure numeric
returns
Ensures too big numbers don't crash round()
Uses syspref in 'GetBudgetHierarchy'

To test:
Follow previous test plan
Check values on admin/aqbudgets.pl are affected by syspref
Ensure values throughout acquisitions are correctly calculated/displayed
(even when greater than 1,000)

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 109 Julian Maurice 2018-09-19 12:52:39 UTC
Created attachment 79104 [details] [review]
Bug 18736: Unit tests for GetBudgetHierarchy

prove -v t/db_dependent/Budgets.t

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 110 Julian Maurice 2018-09-19 12:52:45 UTC
Created attachment 79105 [details] [review]
Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER

More recent MySql versions require the use of SIGNED or UNSIGNED rather
than INTEGER. UNSIGNED will still work in older installs

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 111 Julian Maurice 2018-09-19 12:55:29 UTC
The bug status was 'Signed off' but it looks like it was never signed off...
Well, it is now :-)
Comment 112 Martin Renvoize 2018-09-20 16:01:40 UTC
QA Comment: _get_rounding_sql is present in both C4::Budgets and C4::Acquisitions, should this not be factored out somewhere to prevent it falling out of sync across the two locations?
Comment 113 Nick Clemens 2018-09-20 19:34:50 UTC
Created attachment 79187 [details] [review]
Bug 18736: (follow-up) Remove duplicate code
Comment 114 Marcel de Rooy 2018-09-21 09:14:17 UTC
QA: Looking here
Comment 115 Marcel de Rooy 2018-09-21 09:15:33 UTC
ok 1 - use C4::Acquisition;
# Subtest: Tests for _get_rounding_sql
    1..2
    # No tests run!
not ok 2 - No tests run for subtest "Tests for _get_rounding_sql"
#   Failed test 'No tests run for subtest "Tests for _get_rounding_sql"'
#   at t/Acquisition.t line 40.
Undefined subroutine &C4::Acquisition::_get_rounding_sql called at t/Acquisition.t line 33.
Comment 116 Nick Clemens 2018-09-21 11:31:59 UTC
Created attachment 79223 [details] [review]
Bug 18736: (follow-up) Remove duplicate code and adjust tests
Comment 117 Marcel de Rooy 2018-09-28 07:24:05 UTC
QA: Giving this another try
Comment 118 Marcel de Rooy 2018-09-28 08:13:34 UTC
No blocker:
Koha/Number/Price.pm
return $self->value if $self->value > Number::Format::MAX_INT/100;

What about self->value < -2**53 ?
Big credit note..
Comment 119 Marcel de Rooy 2018-09-28 08:15:29 UTC
Would have expected a small change in t/Prices.t too btw
Comment 120 Marcel de Rooy 2018-09-28 10:25:00 UTC
Got across this internal server error while testing:
Undefined subroutine &C4::Acquisition::IsMarcStructureInternal called at /usr/share/koha/devclone/C4/Acquisition.pm line 3209.

The famous bug to resolve with C4::Biblio::
Seems strictly unrelated however
Comment 121 Marcel de Rooy 2018-09-28 10:26:10 UTC
Created attachment 79562 [details] [review]
Bug 18736: (QA follow-up) Cosmetic changes

[1] Resolve warnings like:
Use of uninitialized value $rounding_pref in string eq at /usr/share/koha/devclone/C4/Acquisition.pm line 2040.

[2] Fixing unusual use of whitespace too.

[3] Remove list operator from get_rounding_sql return. Only used in scalar
context.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 122 Marcel de Rooy 2018-09-28 10:26:17 UTC
Created attachment 79563 [details] [review]
Bug 18736: (QA follow-up) Change to signed, add large int test

[1] Add trivial subtest in t/Number/Price.t
Checking a negative number and a large number for the MAX_INT change.
Note: Confusing to have t/Prices.t too.

[2] Change UNSIGNED to SIGNED in get_rounding_sql. Although I did not spot
problems with negative prices, we theoretically could while casting.
cast(-2 as unsigned) == 18446744073709551614

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 123 Marcel de Rooy 2018-09-28 10:27:56 UTC
Unable to finish this one right now, still in interface testing. Leaving it in blocked, hope to return soon..
Comment 124 Marcel de Rooy 2018-10-05 05:29:16 UTC
(In reply to Marcel de Rooy from comment #123)
> Unable to finish this one right now, still in interface testing. Leaving it
> in blocked, hope to return soon..

Resuming now
Comment 125 Marcel de Rooy 2018-10-05 07:03:09 UTC
QA Comment:
Kind of walking in a minefield here. So very brave try! Think we are doing fine and we have the pref to control it.
One area did not have attention yet:

C4/Budgets.pm needs similar adjustments:
Look at e.g. SUM( COALESCE(unitprice_tax_included, ecost_tax_included) * quantity ) AS budget_spent
or SUM(ecost_tax_included *  quantity) AS budget_ordered

With the pref to nearest cent, the total ordered drops one cent, while it should drop three cent (using your example where 78,83 becomes 78,80).

Another note:
I still have the Internal Server Error:
Undefined subroutine &C4::Acquisition::IsMarcStructureInternal called at /usr/share/koha/devclone/C4/Acquisition.pm line 3211.
I added the C4::Biblio prefix here to resolve it. Please check if you need it too.

Changing status.
Comment 126 Nick Clemens 2018-10-10 18:45:50 UTC
(In reply to Marcel de Rooy from comment #125)
> One area did not have attention yet:
> 
> C4/Budgets.pm needs similar adjustments:
> Look at e.g. SUM( COALESCE(unitprice_tax_included, ecost_tax_included) *
> quantity ) AS budget_spent
> or SUM(ecost_tax_included *  quantity) AS budget_ordered
> 
> With the pref to nearest cent, the total ordered drops one cent, while it
> should drop three cent (using your example where 78,83 becomes 78,80).
> 
Can you point out some line numbers? I don't think I see where you are takling about?
Comment 127 Marcel de Rooy 2018-10-11 08:31:24 UTC
Look in C4::Budgets not Acquisition

GetBudgetsPlanCell line 238, 251 ?
GetBudgetSpent line 335
GetBudgetOrdered line 366 ?
GetBudgetHierarchy line 560, line 567