https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #1 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 64061 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64061&action=edit Bug 18736 - Problems in order calculations (rounding errors) Unit tests to highlight the issue To test: Prove t/db_dependent/Budgets.t -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |veron@veron.ch -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 64065 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64065&action=edit Bug 18736: Correctly handle rounding The values displayed should be the sum of the rounded values, not the reverse. In ModReceiveOrder, i am wondering if: tax_value_on_ordering = quantity * format(ecost_tax_excluded) * format(tax_rate_on_ordering) is correct or if it must be: tax_value_on_ordering = quantity * format(ecost_tax_excluded * tax_rate_on_ordering) Note: the second form is used in 16.06.00.042 (bug 13321) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Séverine Queune <severine.queune@bulac.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |severine.queune@bulac.fr -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Koha Team Lyon 3 <koha@univ-lyon3.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |koha@univ-lyon3.fr --- Comment #7 from Koha Team Lyon 3 <koha@univ-lyon3.fr> --- 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=15741 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #9 from Marc Véron <veron@veron.ch> --- (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) -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #11 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #13 from Marc Véron <veron@veron.ch> --- Independently of the discussion above: Typo in C4/Budgets.pm line 224 SELECT SUM(ROUD(ecost_tax_included, 2)) FROM aqorders -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff Patch complexity|--- |Medium patch -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #15 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 70188 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=70188&action=edit 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #20 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 70768 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=70768&action=edit Bug 18736: Followup - Fix SQL and uncomment db update -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #22 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 71063 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=71063&action=edit Bug 18736 (follow-up) Use CAST instead of CEIL for appropriate rounding -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply --- Comment #24 from M. Tompsett <mtompset@hotmail.com> --- Rebasing. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #70188|0 |1 is obsolete| | --- Comment #26 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 71628 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=71628&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #70768|0 |1 is obsolete| | --- Comment #28 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 71630 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=71630&action=edit Bug 18736: Followup - Fix SQL and uncomment db update -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #71063|0 |1 is obsolete| | --- Comment #30 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 71632 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=71632&action=edit Bug 18736 (follow-up) Use CAST instead of CEIL for appropriate rounding -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #32 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #34 from Koha Team Lyon 3 <koha@univ-lyon3.fr> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #37 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 74336 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=74336&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #39 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 74338 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=74338&action=edit Bug 18736: (follow-up) - Fix SQL and uncomment db update -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #41 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 74340 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=74340&action=edit Bug 18736: (follow-up) Use CAST instead of CEIL for appropriate rounding -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #43 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 74342 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=74342&action=edit Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell prove t/db_dependent/Budgets.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #45 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 74371 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=74371&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #47 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 74374 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=74374&action=edit Bug 18736: [DO NOT PUSH] C4/Acquisition print scaffolding. Need to trigger all the print STDERR "C4/Acquisition.pm... cases. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #49 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 74398 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=74398&action=edit 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! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #52 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 74438 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=74438&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #74374|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #54 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |colin.campbell@ptfs-europe. | |com --- Comment #56 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Need to test with year end process. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Fiona Borthwick <fiona.borthwick@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fiona.borthwick@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #59 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 76980 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76980&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #61 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 76982 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76982&action=edit Bug 18736: (follow-up) - Fix SQL and uncomment db update -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #63 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 76984 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76984&action=edit Bug 18736: (follow-up) Use CAST instead of CEIL for appropriate rounding -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #65 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 76986 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76986&action=edit Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell prove t/db_dependent/Budgets.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #67 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 76988 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76988&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #69 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 76990 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76990&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #71 from Nick Clemens <nick@bywatersolutions.com> --- Applied cleanly on master for me - I moved additional tests to subtest in Acquisition.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |julian.maurice@biblibre.com --- Comment #73 from Julian Maurice <julian.maurice@biblibre.com> --- Patches do not apply. Please rebase -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #76979|0 |1 is obsolete| | Attachment #76980|0 |1 is obsolete| | Attachment #76981|0 |1 is obsolete| | Attachment #76982|0 |1 is obsolete| | Attachment #76983|0 |1 is obsolete| | Attachment #76984|0 |1 is obsolete| | Attachment #76985|0 |1 is obsolete| | Attachment #76986|0 |1 is obsolete| | Attachment #76987|0 |1 is obsolete| | Attachment #76988|0 |1 is obsolete| | Attachment #76989|0 |1 is obsolete| | Attachment #76990|0 |1 is obsolete| | Attachment #76991|0 |1 is obsolete| | --- Comment #74 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 77614 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=77614&action=edit Bug 18736: Add OrderPriceRounding syspref -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #76 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 77616 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=77616&action=edit Bug 18736: Unit tests for rounding Unit tests to highlight the issue To test: Prove t/db_dependent/Budgets.t prove t/Acquisition.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #78 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 77618 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=77618&action=edit Bug 18736: (follow-up) Add missing test cases We now fully cover GetBudgetsPlanCell in these tests -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #81 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply CC| |m.de.rooy@rijksmuseum.nl --- Comment #83 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Applying: Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell fatal: sha1 information is lacking or useless (C4/Budgets.pm). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #77614|0 |1 is obsolete| | Attachment #77615|0 |1 is obsolete| | Attachment #77616|0 |1 is obsolete| | Attachment #77617|0 |1 is obsolete| | Attachment #77618|0 |1 is obsolete| | Attachment #78101|0 |1 is obsolete| | Attachment #78120|0 |1 is obsolete| | --- Comment #84 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 78981 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78981&action=edit Bug 18736: Add OrderPriceRounding syspref -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #86 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 78983 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78983&action=edit Bug 18736: Unit tests for rounding Unit tests to highlight the issue To test: Prove t/db_dependent/Budgets.t prove t/Acquisition.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #88 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 78985 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78985&action=edit Bug 18736: (follow-up) Add missing test cases We now fully cover GetBudgetsPlanCell in these tests -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #90 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 78987 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78987&action=edit Bug 18736: Unit tests for GetBudgetHierarchy prove -v t/db_dependent/Budgets.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #78981|0 |1 is obsolete| | Attachment #78982|0 |1 is obsolete| | Attachment #78983|0 |1 is obsolete| | Attachment #78984|0 |1 is obsolete| | Attachment #78985|0 |1 is obsolete| | Attachment #78986|0 |1 is obsolete| | Attachment #78987|0 |1 is obsolete| | Attachment #78988|0 |1 is obsolete| | --- Comment #94 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 79087 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=79087&action=edit Bug 18736: Add OrderPriceRounding syspref -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #96 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 79089 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=79089&action=edit Bug 18736: Unit tests for rounding Unit tests to highlight the issue To test: Prove t/db_dependent/Budgets.t prove t/Acquisition.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #98 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 79091 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=79091&action=edit Bug 18736: (follow-up) Add missing test cases We now fully cover GetBudgetsPlanCell in these tests -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #100 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 79093 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=79093&action=edit Bug 18736: Unit tests for GetBudgetHierarchy prove -v t/db_dependent/Budgets.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #102 from Julian Maurice <julian.maurice@biblibre.com> --- Thanks Nick! Reviewing this patchset once again, for the last time hopefully :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #79088|0 |1 is obsolete| | --- Comment #104 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 79099 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=79099&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #79090|0 |1 is obsolete| | --- Comment #106 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 79101 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=79101&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #79092|0 |1 is obsolete| | --- Comment #108 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 79103 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=79103&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #79094|0 |1 is obsolete| | --- Comment #110 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 79105 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=79105&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #112 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | --- Comment #114 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QA: Looking here -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=21427 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #118 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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.. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #120 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #122 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 79563 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=79563&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |josef.moravec@gmail.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Failed QA --- Comment #125 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #127 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Look in C4::Budgets not Acquisition GetBudgetsPlanCell line 238, 251 ? GetBudgetSpent line 335 GetBudgetOrdered line 366 ? GetBudgetHierarchy line 560, line 567 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #130 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 82968 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=82968&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #132 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 82970 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=82970&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #134 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 82972 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=82972&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #136 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 82974 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=82974&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #138 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 82976 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=82976&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #140 from Nick Clemens <nick@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #142 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 83213 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83213&action=edit Test basket -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #144 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #82968|0 |1 is obsolete| | --- Comment #146 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 83974 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83974&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #82970|0 |1 is obsolete| | --- Comment #148 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 83976 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83976&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #82972|0 |1 is obsolete| | --- Comment #150 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 83978 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83978&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #82974|0 |1 is obsolete| | --- Comment #152 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 83980 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83980&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #82976|0 |1 is obsolete| | --- Comment #154 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 83982 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83982&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #83212|0 |1 is obsolete| | --- Comment #156 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 83984 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83984&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #158 from Nick Clemens <nick@bywatersolutions.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #160 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Small textual problems: + receiving => 1 # boolean representing order stage, should pass only this or ordering + ordering => 1 # boolean representing order stage except fot -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |In Discussion --- Comment #162 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 --- Comment #164 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #83973|0 |1 is obsolete| | --- Comment #166 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 86646 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86646&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #83975|0 |1 is obsolete| | --- Comment #168 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 86648 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86648&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #83977|0 |1 is obsolete| | --- Comment #170 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 86650 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86650&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #83979|0 |1 is obsolete| | --- Comment #172 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 86652 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86652&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #83981|0 |1 is obsolete| | --- Comment #174 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 86654 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86654&action=edit Bug 18736: (follow-up) Remove duplicate code and adjust tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #83983|0 |1 is obsolete| | --- Comment #176 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 86656 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86656&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #83985|0 |1 is obsolete| | --- Comment #178 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 86658 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86658&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #180 from Nick Clemens <nick@bywatersolutions.com> --- Awesome work all! Pushed to master for 19.05 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable --- Comment #182 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Pushed to 18.11.x for 18.11.04! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |release-notes-needed -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #183 from Lucas Gass <lucas@bywatersolutions.com> --- not backporting enhancement to 18.05.x -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |22791 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22791 [Bug 22791] Calculation differs on aqui-home/spent and ordered.pl -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |19.05.00, 18.11.04 released in| | -- You are receiving this mail because: You are watching all bug changes.