Bug 18736 - Problems in order calculations (rounding errors)
Summary: Problems in order calculations (rounding errors)
Status: CLOSED FIXED
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: Marcel de Rooy
URL:
Keywords:
Depends on: 13321
Blocks: 22618 22791
  Show dependency treegraph
 
Reported: 2017-06-06 14:36 UTC by Nick Clemens
Modified: 2020-11-17 20:06 UTC (History)
22 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patch introduces to new system preference to govern how rounding is applied to values in acquisitions. It defaults to the american practice of rounding to the nearest 'cent' but future options should become available as we start to understand how other nations round for accounting purposes.
Version(s) released in:
19.05.00, 18.11.04


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, Mark 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, Mark Tompsett
Details | Diff | Splinter Review
Bug 18736 - Unit tests for rounding (5.64 KB, patch)
2018-02-14 18:14 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18736: Followup - Fix SQL and uncomment db update (2.66 KB, patch)
2018-02-14 18:14 UTC, Mark 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, Mark 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, Mark Tompsett
Details | Diff | Splinter Review
Bug 18736: Add OrderPriceRounding syspref (3.37 KB, patch)
2018-04-17 14:42 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Use rounding syspref to determine correct prices in calculations (20.91 KB, patch)
2018-04-17 14:42 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Unit tests for rounding (5.64 KB, patch)
2018-04-17 14:42 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) - Fix SQL and uncomment db update (2.66 KB, patch)
2018-04-17 14:42 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) fix wrong call to _get_rounding_sql (1.67 KB, patch)
2018-04-17 14:42 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Use CAST instead of CEIL for appropriate rounding (2.24 KB, patch)
2018-04-17 14:42 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Correctly assign values in subroutines (1.28 KB, patch)
2018-04-17 14:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell (5.00 KB, patch)
2018-04-17 14:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Follow to prevent duplicate run noise (1.48 KB, patch)
2018-04-17 17:15 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18736: Follow to clean up admin policy preference (1.97 KB, patch)
2018-04-17 17:39 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18736: [DO NOT PUSH] C4/Acquisition print scaffolding. (4.91 KB, patch)
2018-04-17 18:09 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18736: [DO NOT PUSH] Print scaffolding for C4/Budgets.pm (3.68 KB, patch)
2018-04-17 23:39 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18736: Add missing test cases [BUGGY] (5.93 KB, patch)
2018-04-17 23:42 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add missing test cases (8.83 KB, patch)
2018-04-18 11:13 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Undo changes to GetLateOrders (1.56 KB, patch)
2018-04-18 11:39 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Follow up tests (4.47 KB, patch)
2018-04-18 14:41 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18736: Add OrderPriceRounding syspref (3.37 KB, patch)
2018-07-16 11:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Use rounding syspref to determine correct prices in calculations (20.88 KB, patch)
2018-07-16 11:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Unit tests for rounding (5.64 KB, patch)
2018-07-16 11:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) - Fix SQL and uncomment db update (2.66 KB, patch)
2018-07-16 11:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) fix wrong call to _get_rounding_sql (1.67 KB, patch)
2018-07-16 11:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Use CAST instead of CEIL for appropriate rounding (2.24 KB, patch)
2018-07-16 11:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Correctly assign values in subroutines (1.28 KB, patch)
2018-07-16 11:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell (5.00 KB, patch)
2018-07-16 11:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Follow to prevent duplicate run noise (1.48 KB, patch)
2018-07-16 11:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Follow to clean up admin policy preference (1.97 KB, patch)
2018-07-16 11:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add missing test cases (8.83 KB, patch)
2018-07-16 11:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Undo changes to GetLateOrders (1.53 KB, patch)
2018-07-16 11:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Unit tests (3.51 KB, patch)
2018-07-16 11:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Add OrderPriceRounding syspref (3.39 KB, patch)
2018-08-09 10:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Use rounding syspref to determine correct prices in calculations (21.01 KB, patch)
2018-08-09 10:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Unit tests for rounding (9.02 KB, patch)
2018-08-09 10:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell (5.11 KB, patch)
2018-08-09 10:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add missing test cases (8.81 KB, patch)
2018-08-09 10:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Fix missing rounding and bad formatting (3.69 KB, patch)
2018-08-24 01:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Unit tests doe GetBudgetHierarchy (4.03 KB, patch)
2018-08-24 11:12 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Add OrderPriceRounding syspref (3.39 KB, patch)
2018-09-16 18:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Use rounding syspref to determine correct prices in calculations (21.01 KB, patch)
2018-09-16 18:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Unit tests for rounding (8.89 KB, patch)
2018-09-16 18:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell (5.07 KB, patch)
2018-09-16 18:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add missing test cases (8.81 KB, patch)
2018-09-16 18:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Fix missing rounding and bad formatting (3.69 KB, patch)
2018-09-16 18:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Unit tests for GetBudgetHierarchy (4.03 KB, patch)
2018-09-16 18:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER (1.52 KB, patch)
2018-09-16 18:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Add OrderPriceRounding syspref (3.39 KB, patch)
2018-09-19 12:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Use rounding syspref to determine correct prices in calculations (21.01 KB, patch)
2018-09-19 12:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Unit tests for rounding (8.89 KB, patch)
2018-09-19 12:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell (5.07 KB, patch)
2018-09-19 12:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add missing test cases (8.81 KB, patch)
2018-09-19 12:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Fix missing rounding and bad formatting (3.66 KB, patch)
2018-09-19 12:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Unit tests for GetBudgetHierarchy (4.03 KB, patch)
2018-09-19 12:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER (1.65 KB, patch)
2018-09-19 12:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Add OrderPriceRounding syspref (3.45 KB, patch)
2018-09-19 12:51 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18736: Use rounding syspref to determine correct prices in calculations (21.07 KB, patch)
2018-09-19 12:52 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18736: Unit tests for rounding (8.95 KB, patch)
2018-09-19 12:52 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell (5.14 KB, patch)
2018-09-19 12:52 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add missing test cases (8.87 KB, patch)
2018-09-19 12:52 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18736: (follow-up) Fix missing rounding and bad formatting (3.75 KB, patch)
2018-09-19 12:52 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18736: Unit tests for GetBudgetHierarchy (4.09 KB, patch)
2018-09-19 12:52 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER (1.72 KB, patch)
2018-09-19 12:52 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18736: (follow-up) Remove duplicate code (6.14 KB, patch)
2018-09-20 19:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Remove duplicate code and adjust tests (7.10 KB, patch)
2018-09-21 11:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (QA follow-up) Cosmetic changes (2.31 KB, patch)
2018-09-28 10:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18736: (QA follow-up) Change to signed, add large int test (2.40 KB, patch)
2018-09-28 10:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18736: Add OrderPriceRounding syspref (3.55 KB, patch)
2018-12-07 13:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Use rounding syspref to determine correct prices in calculations (21.21 KB, patch)
2018-12-07 13:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Unit tests for rounding (8.97 KB, patch)
2018-12-07 13:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell (5.14 KB, patch)
2018-12-07 13:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add missing test cases (8.88 KB, patch)
2018-12-07 13:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Fix missing rounding and bad formatting (3.78 KB, patch)
2018-12-07 13:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Unit tests for GetBudgetHierarchy (4.09 KB, patch)
2018-12-07 13:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER (1.72 KB, patch)
2018-12-07 13:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Remove duplicate code and adjust tests (7.16 KB, patch)
2018-12-07 13:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (QA follow-up) Cosmetic changes (2.28 KB, patch)
2018-12-07 13:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (QA follow-up) Change to signed, add large int test (2.37 KB, patch)
2018-12-07 13:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (QA follow-up) Resolve uninitialized warn from Biblio.pm L3334 (1.25 KB, patch)
2018-12-14 10:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Test basket (40.61 KB, image/png)
2018-12-14 10:59 UTC, Marcel de Rooy
Details
Test budget (13.26 KB, image/png)
2018-12-14 11:00 UTC, Marcel de Rooy
Details
Bug 18736: Add OrderPriceRounding syspref (3.55 KB, patch)
2019-01-15 15:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Use rounding syspref to determine correct prices in calculations (21.21 KB, patch)
2019-01-15 15:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Unit tests for rounding (9.00 KB, patch)
2019-01-15 15:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell (5.14 KB, patch)
2019-01-15 15:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add missing test cases (8.88 KB, patch)
2019-01-15 15:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Fix missing rounding and bad formatting (3.78 KB, patch)
2019-01-15 15:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Unit tests for GetBudgetHierarchy (4.09 KB, patch)
2019-01-15 15:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER (1.72 KB, patch)
2019-01-15 15:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (follow-up) Remove duplicate code and adjust tests (7.16 KB, patch)
2019-01-15 15:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (QA follow-up) Cosmetic changes (2.28 KB, patch)
2019-01-15 15:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (QA follow-up) Change to signed, add large int test (2.37 KB, patch)
2019-01-15 15:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: (QA follow-up) Resolve uninitialized warn from Biblio.pm L3334 (1.22 KB, patch)
2019-01-15 15:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Calculate tax depending on rounding (14.52 KB, patch)
2019-01-15 15:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18736: Add OrderPriceRounding syspref (3.65 KB, patch)
2019-03-15 10:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18736: Use rounding syspref to determine correct prices in calculations (21.30 KB, patch)
2019-03-15 10:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18736: Unit tests for rounding (9.10 KB, patch)
2019-03-15 10:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell (5.24 KB, patch)
2019-03-15 10:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18736: (follow-up) Add missing test cases (8.98 KB, patch)
2019-03-15 10:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18736: (follow-up) Fix missing rounding and bad formatting (3.88 KB, patch)
2019-03-15 10:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18736: Unit tests for GetBudgetHierarchy (4.19 KB, patch)
2019-03-15 10:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER (1.82 KB, patch)
2019-03-15 10:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18736: (follow-up) Remove duplicate code and adjust tests (7.25 KB, patch)
2019-03-15 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18736: (QA follow-up) Cosmetic changes (2.32 KB, patch)
2019-03-15 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18736: (QA follow-up) Change to signed, add large int test (2.41 KB, patch)
2019-03-15 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18736: (QA follow-up) Resolve uninitialized warn from Biblio.pm L3334 (1.26 KB, patch)
2019-03-15 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18736: Calculate tax depending on rounding (14.62 KB, patch)
2019-03-15 10:06 UTC, Marcel de Rooy
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 University 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 University 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 Mark 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 Mark 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 Mark 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 Mark Tompsett 2018-02-14 18:11:32 UTC
Rebasing.
Comment 25 Mark Tompsett 2018-02-14 18:14:31 UTC
Created attachment 71627 [details] [review]
Bug 18736 - Add OrerPriceRounding syspref
Comment 26 Mark 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 Mark 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 Mark Tompsett 2018-02-14 18:14:44 UTC
Created attachment 71630 [details] [review]
Bug 18736: Followup - Fix SQL and uncomment db update
Comment 29 Mark 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 Mark 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 Mark 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 Mark 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 University 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 Mark 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 Mark 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 Mark 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 Mark 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 Mark 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 Mark 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 Mark 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 Mark 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 Mark 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
Comment 128 Brian 2018-11-01 20:02:07 UTC
Whats the status of bug 18736?

Carnegie Stout Team
Comment 129 Nick Clemens 2018-12-07 13:22:25 UTC
Created attachment 82967 [details] [review]
Bug 18736: Add OrderPriceRounding syspref

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 130 Nick Clemens 2018-12-07 13:22:32 UTC
Created attachment 82968 [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 131 Nick Clemens 2018-12-07 13:22:39 UTC
Created attachment 82969 [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 132 Nick Clemens 2018-12-07 13:22:44 UTC
Created attachment 82970 [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 133 Nick Clemens 2018-12-07 13:22:51 UTC
Created attachment 82971 [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 134 Nick Clemens 2018-12-07 13:22:57 UTC
Created attachment 82972 [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 135 Nick Clemens 2018-12-07 13:23:03 UTC
Created attachment 82973 [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 136 Nick Clemens 2018-12-07 13:23:10 UTC
Created attachment 82974 [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 137 Nick Clemens 2018-12-07 13:23:18 UTC
Created attachment 82975 [details] [review]
Bug 18736: (follow-up) Remove duplicate code and adjust tests
Comment 138 Nick Clemens 2018-12-07 13:23:26 UTC
Created attachment 82976 [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 139 Nick Clemens 2018-12-07 13:23:33 UTC
Created attachment 82977 [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 140 Nick Clemens 2018-12-07 13:26:16 UTC
Rebased on master

(In reply to Marcel de Rooy from comment #127)
> Look in C4::Budgets not Acquisition
> 
> GetBudgetsPlanCell line 238, 251 ?
> GetBudgetSpent line 335
> GetBudgetOrdered line 366 ?
> GetBudgetHierarchy line 560, line 567

Sorry Marcel, all of the occurrences I see use get_rounding_sql()

>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 let me know where you are seeing this? Apologies if I am missing something obvious
Comment 141 Marcel de Rooy 2018-12-14 10:20:51 UTC
Created attachment 83212 [details] [review]
Bug 18736: (QA follow-up) Resolve uninitialized warn from Biblio.pm L3334

Running t/db/Budgets.t:
Use of uninitialized value in join or string at C4/Biblio.pm line 3334.

Test plan:
Run again. Verify no warnings left.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 142 Marcel de Rooy 2018-12-14 10:59:45 UTC
Created attachment 83213 [details]
Test basket
Comment 143 Marcel de Rooy 2018-12-14 11:00:07 UTC
Created attachment 83214 [details]
Test budget
Comment 144 Marcel de Rooy 2018-12-14 11:12:25 UTC
(In reply to Nick Clemens from comment #140)
> >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 let me know where you are seeing this? Apologies if I am missing
> something obvious

Still seeing this. Best to illustrate by two images (see atts).
The basket shows 78,80 and 15,33 tax. (94,13)
The budget shows 92,16 (note that includes a -2, so 94,16)
Three cents difference. This comes from rounding at different moments.

The db shows:
+-----------------------+
| tax_value_on_ordering |
+-----------------------+
|             15.326600 |
+-----------------------+
+--------------------+
| ecost_tax_excluded |
+--------------------+
|           9.854200 |
+--------------------+
+--------------------+
| ecost_tax_included |
+--------------------+
|          11.770842 |
+--------------------+

8 * 11,77 = 94,16
8 * 9,85[rounded] * 1,1945 (tax percentage ) = 94,13

Difference: For 94,16 tax is calculated on not rounded price (9,8542). But for 94,13 tax is based on rounded price (9,85).
How much tax do you want to pay ? ;)

Not a real blocker for me btw. But this shows that we are not making consistent calculations in Acqui. If we still want to push, we should be aware of it at least.

Will ask Joubu to comment on this finding too. Changing status for now.
Comment 145 Nick Clemens 2019-01-15 15:17:02 UTC
Created attachment 83973 [details] [review]
Bug 18736: Add OrderPriceRounding syspref

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 146 Nick Clemens 2019-01-15 15:17:08 UTC
Created attachment 83974 [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 147 Nick Clemens 2019-01-15 15:17:13 UTC
Created attachment 83975 [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 148 Nick Clemens 2019-01-15 15:17:18 UTC
Created attachment 83976 [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 149 Nick Clemens 2019-01-15 15:17:23 UTC
Created attachment 83977 [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 150 Nick Clemens 2019-01-15 15:17:30 UTC
Created attachment 83978 [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 151 Nick Clemens 2019-01-15 15:17:35 UTC
Created attachment 83979 [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 152 Nick Clemens 2019-01-15 15:17:40 UTC
Created attachment 83980 [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 153 Nick Clemens 2019-01-15 15:17:47 UTC
Created attachment 83981 [details] [review]
Bug 18736: (follow-up) Remove duplicate code and adjust tests
Comment 154 Nick Clemens 2019-01-15 15:17:53 UTC
Created attachment 83982 [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 155 Nick Clemens 2019-01-15 15:17:59 UTC
Created attachment 83983 [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 156 Nick Clemens 2019-01-15 15:18:06 UTC
Created attachment 83984 [details] [review]
Bug 18736: (QA follow-up) Resolve uninitialized warn from Biblio.pm L3334

Running t/db/Budgets.t:
Use of uninitialized value in join or string at C4/Biblio.pm line 3334.

Test plan:
Run again. Verify no warnings left.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 157 Nick Clemens 2019-01-15 15:18:12 UTC
Created attachment 83985 [details] [review]
Bug 18736: Calculate tax depending on rounding

Marcel's comments pointed out that while I tried to avoid storing
rounded values it is required for tax generation.

This patch makes that change and adds test coverage and POD for
populate_order_with_prices

To test:
Follow plan on other patches, ensure that orders and totals match on the
basket, invoice, and budget pages

prove -v t/db_dependent/Acquisition/populate_order_with_prices.t
Comment 158 Nick Clemens 2019-01-15 15:22:12 UTC
(In reply to Marcel de Rooy from comment #144)
> (In reply to Nick Clemens from comment #140)

Ah, okay, I understand now - it seems we cannot leave the tax values untouched when enabling rounding, I added a followup for this. Let me know if the new calculations and tests match your expectations.
Comment 159 Marcel de Rooy 2019-02-15 10:21:54 UTC
Spending a bit of time here now
Comment 160 Marcel de Rooy 2019-02-15 10:39:19 UTC
Small textual problems:
+    receiving    => 1 # boolean representing order stage, should pass only this or ordering
+    ordering     => 1 # boolean representing order stage
except fot
Comment 161 Marcel de Rooy 2019-02-15 10:54:04 UTC
Test pass. QA pass.
Hmm. Not wanting to be too precise, but we still have some subtle differences in some lines of the last patch. The first line rounds at the unit; with a whole quantity, it will be rounded. The second line is not rounded since you multiply with tax rate. The algorithm of tax = incl - excl is only used in the first line.
The third line shows rounding at final stage.
+            $order->{tax_value_on_ordering} = ( get_rounded_price($order->{ecost_tax_included}) - get_rounded_price($order->{ecost_tax_excluded}) ) * $order->{quantity};
+            $order->{tax_value_on_ordering} = $order->{quantity} * get_rounded_price($order->{ecost_tax_excluded}) * $order->{tax_rate_on_ordering};

+        my $subtotal = get_rounded_price( $left * $data->{'ecost_tax_included'} );

Please explain why you do what you do.
Comment 162 Nick Clemens 2019-03-06 16:52:23 UTC
(In reply to Marcel de Rooy from comment #161)
> Test pass. QA pass.
> Hmm. Not wanting to be too precise, but we still have some subtle
> differences in some lines of the last patch. The first line rounds at the
> unit; with a whole quantity, it will be rounded. The second line is not
> rounded since you multiply with tax rate. The algorithm of tax = incl - excl
> is only used in the first line.
> The third line shows rounding at final stage.
> +            $order->{tax_value_on_ordering} = (
> get_rounded_price($order->{ecost_tax_included}) -
> get_rounded_price($order->{ecost_tax_excluded}) ) * $order->{quantity};
> +            $order->{tax_value_on_ordering} = $order->{quantity} *
> get_rounded_price($order->{ecost_tax_excluded}) *
> $order->{tax_rate_on_ordering};
> 
> +        my $subtotal = get_rounded_price( $left *
> $data->{'ecost_tax_included'} );
> 
> Please explain why you do what you do.

The first line is when the user entered prices including tax - so we have to calculate excluding tax(In reply to Marcel de Rooy from comment #161)
> Test pass. QA pass.
> Hmm. Not wanting to be too precise, but we still have some subtle
> differences in some lines of the last patch. The first line rounds at the
> unit; with a whole quantity, it will be rounded. The second line is not
> rounded since you multiply with tax rate. The algorithm of tax = incl - excl
> is only used in the first line.
> The third line shows rounding at final stage.
> +            $order->{tax_value_on_ordering} = (
> get_rounded_price($order->{ecost_tax_included}) -
> get_rounded_price($order->{ecost_tax_excluded}) ) * $order->{quantity};
> +            $order->{tax_value_on_ordering} = $order->{quantity} *
> get_rounded_price($order->{ecost_tax_excluded}) *
> $order->{tax_rate_on_ordering};
> 
> +        my $subtotal = get_rounded_price( $left *
> $data->{'ecost_tax_included'} );
> 
> Please explain why you do what you do.

In the first line we are looking at prices supplied including tax - I assumed we get a rounded number from the vendor - I also assume they charge the tax to a single item then round - so we need to back calculate the tax, not knowing the unrounded end product - then charge this same amount per item

In the second case (lines 2 and 3) we are given prices without tax, so we can calculate it on our own - and I assume they would perform the same caulcation - round discounted price, apply quantity and tax

The issue is really that when getting a price including tax I assume we don't have full info, so do my best to calculate what I expect.

Can you explain how you would expect to calculate that and what numbers you would expect to see?

The tests illustrate that I do expect different numbers in the different scenarios, I honestly don't know what the vendors or librarians expect and would accept any quidance
Comment 163 Martin Renvoize 2019-03-08 13:25:28 UTC
I think I'm happy with Nicks logic here.. 

Unfortunately, as we store calculated values we're open to having issues like this.  EDI for example can often give you both tax inclusive and tax exclusive values and pretty often the rounding math at their end will be different from at our end and so yield different results.

I think this patch improves the situation a fair amount and is a good compromise given our existing codebase.. perhaps it's the foundations for more work in a followup bug but I don't think that should hold up this work any longer really.. 

Did you have any more specific issues Marcel, that Nick could go on for keeping this moving?
Comment 164 Marcel de Rooy 2019-03-11 13:21:58 UTC
(In reply to Martin Renvoize from comment #163)
> I think I'm happy with Nicks logic here.. 
> 
> Unfortunately, as we store calculated values we're open to having issues
> like this.  EDI for example can often give you both tax inclusive and tax
> exclusive values and pretty often the rounding math at their end will be
> different from at our end and so yield different results.
> 
> I think this patch improves the situation a fair amount and is a good
> compromise given our existing codebase.. perhaps it's the foundations for
> more work in a followup bug but I don't think that should hold up this work
> any longer really.. 
> 
> Did you have any more specific issues Marcel, that Nick could go on for
> keeping this moving?

Will have a look this week
Comment 165 Marcel de Rooy 2019-03-15 08:47:39 UTC
Back here again ;)
Comment 166 Marcel de Rooy 2019-03-15 10:05:09 UTC
Created attachment 86646 [details] [review]
Bug 18736: Add OrderPriceRounding syspref

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 167 Marcel de Rooy 2019-03-15 10:05:17 UTC
Created attachment 86647 [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>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 168 Marcel de Rooy 2019-03-15 10:05:24 UTC
Created attachment 86648 [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>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 169 Marcel de Rooy 2019-03-15 10:05:31 UTC
Created attachment 86649 [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>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 170 Marcel de Rooy 2019-03-15 10:05:38 UTC
Created attachment 86650 [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>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 171 Marcel de Rooy 2019-03-15 10:05:44 UTC
Created attachment 86651 [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>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 172 Marcel de Rooy 2019-03-15 10:05:51 UTC
Created attachment 86652 [details] [review]
Bug 18736: Unit tests for GetBudgetHierarchy

prove -v t/db_dependent/Budgets.t

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 173 Marcel de Rooy 2019-03-15 10:05:58 UTC
Created attachment 86653 [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>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 174 Marcel de Rooy 2019-03-15 10:06:04 UTC
Created attachment 86654 [details] [review]
Bug 18736: (follow-up) Remove duplicate code and adjust tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 175 Marcel de Rooy 2019-03-15 10:06:11 UTC
Created attachment 86655 [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 176 Marcel de Rooy 2019-03-15 10:06:17 UTC
Created attachment 86656 [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 177 Marcel de Rooy 2019-03-15 10:06:24 UTC
Created attachment 86657 [details] [review]
Bug 18736: (QA follow-up) Resolve uninitialized warn from Biblio.pm L3334

Running t/db/Budgets.t:
Use of uninitialized value in join or string at C4/Biblio.pm line 3334.

Test plan:
Run again. Verify no warnings left.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 178 Marcel de Rooy 2019-03-15 10:06:31 UTC
Created attachment 86658 [details] [review]
Bug 18736: Calculate tax depending on rounding

Marcel's comments pointed out that while I tried to avoid storing
rounded values it is required for tax generation.

This patch makes that change and adds test coverage and POD for
populate_order_with_prices

To test:
Follow plan on other patches, ensure that orders and totals match on the
basket, invoice, and budget pages

prove -v t/db_dependent/Acquisition/populate_order_with_prices.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 179 Marcel de Rooy 2019-03-15 10:24:03 UTC
QA Comment:
I re-entered the example with 8 * 16,99 and 42% discount, 19.45% tax.

With the following results when patches applied:
OrderPriceRounding=Dont
Basket page 78,83 + 15,33 tax. == 94,16
Fund page 94,17
=> No match between basket and fund !

OrderPriceRounding=Nearest cent
Basket page: 78,80 + 15,33 tax == 94,13
Fund page: 94,16
=> No match either between basket and fund !

And without these patches:
Basket page 78,83 + 15,33 tax. == 94,16
Fund page: 94,17
=> No match either.

Conclusion: Please see also my previous comments. I am not repeating them here. With the default setting of Don't round we make no difference in this example; so no behavior change.. The Nearest cent choice does not resolve the discrepancy between basket and fund, as noted before.

As already mentioned, I do not object to push this patch set. But this simple exercise already shows that we did not solve the rounding issues.

Passed QA
Comment 180 Nick Clemens 2019-03-21 18:34:29 UTC
Awesome work all!

Pushed to master for 19.05
Comment 181 Nick Clemens 2019-03-21 18:35:52 UTC
(In reply to Marcel de Rooy from comment #179)
> QA Comment:
> I re-entered the example with 8 * 16,99 and 42% discount, 19.45% tax.
> 
> With the following results when patches applied:
> OrderPriceRounding=Dont
> Basket page 78,83 + 15,33 tax. == 94,16
> Fund page 94,17
> => No match between basket and fund !
> 
> OrderPriceRounding=Nearest cent
> Basket page: 78,80 + 15,33 tax == 94,13
> Fund page: 94,16
> => No match either between basket and fund !
> 
> And without these patches:
> Basket page 78,83 + 15,33 tax. == 94,16
> Fund page: 94,17
> => No match either.
> 
> Conclusion: Please see also my previous comments. I am not repeating them
> here. With the default setting of Don't round we make no difference in this
> example; so no behavior change.. The Nearest cent choice does not resolve
> the discrepancy between basket and fund, as noted before.
> 
> As already mentioned, I do not object to push this patch set. But this
> simple exercise already shows that we did not solve the rounding issues.
> 
> Passed QA

You are right :-) I talked to others though and general consensus was this is a big change and should tackle issues raised on follow-ups. I think this does some good things and paves the way for more work forward so pushed.  Thanks Marcel
Comment 182 Martin Renvoize 2019-03-25 14:34:56 UTC
Pushed to 18.11.x for 18.11.04!
Comment 183 Lucas Gass 2019-04-01 15:18:41 UTC
not backporting enhancement to 18.05.x