https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 Raphael Straub <raphael.straub@kit.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Change sponsored?|--- |Sponsored Patch complexity|--- |Medium patch -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 --- Comment #3 from David Nind <david@davidnind.com> --- (In reply to David Nind from comment #2)

Hi Raphael.

I had a go at testing, however the price doesn't change for me - it stays as 17.95.

David

Replying to myself - it worked when I set OrderPriceRounding to round. Also wondering whether this is the same/similar to bug 35114 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #161918|0 |1 is obsolete| | --- Comment #4 from David Nind <david@davidnind.com> --- Created attachment 161951 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161951&action=edit Bug 36049: Fix price rounding and add a unit test This patch only fixes price rounding by using Math::BigFloat. To test: 1) Create an order line in acquisitions with vendor price 18.90 and a discount of 5 %. 2) Verify that the total (tax excl.) in the order table on the basket page is 17.95. 3) Apply the patch. 4) Reload the basket page. 5) Verify that the total (tax excl.) in the order table on the basket page now is 17.96, which is correct. 6) Run: prove t/Number/Price.t Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 --- Comment #6 from Raphael Straub <raphael.straub@kit.edu> --- (In reply to David Nind from comment #3)

Also wondering whether this is the same/similar to bug 35114 The similarity to bug 35114 is that both bugs deal with rounding issues. Thats why I added this bug to "See Also".

I think that the patch in bug 35114 only solves part of the problem by rearranging rounding operations and using a nearest rounding mode instead of truncation in JavaScript. Yes, it improves the situation as there are fewer cases with incorrect results, but cases with incorrect results still remain as the underlying problem is not using decimal arithmetic. For example "Math.round(18.90 * (100 - 5)) / 100" in JavaScript evaluates to "17.95" instead of "17.96". This can only be fixed by using a decimal arithmetic package like bignumber.js or big.js. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 --- Comment #8 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 162699 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162699&action=edit Bug 36049: Add unit test Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net Attachment #161951|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=36049 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |victor@tuxayo.net |y.org | Status|Signed Off |Failed QA --- Comment #11 from Victor Grousset/tuxayo <victor@tuxayo.net> --- In basket view, shouldn't "Budgeted cost tax exc." also honor OrderPriceRounding ? Same about "Budgeted cost tax inc" and "GST". In the new order form, "Total" (and "Budgeted cost:" depending on previous question) now aren't consistent with the basket view. It's the previous step and the same data should have the same value. So we would have basket's "total tax exc" honoring well OrderPriceRounding but there is still buggy rounding around. Either for the same data on the previous page or on the same page close to "total tax exc". --- Commit was split to confirm that the test fails without implementation. That shows the test does it's work :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 --- Comment #12 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Victor Grousset/tuxayo from comment #11)

In basket view, shouldn't "Budgeted cost tax exc." also honor OrderPriceRounding ? Same about "Budgeted cost tax inc" and "GST".

If it doesn't honor it right now, then this seems a bit out of scope here and would be better suited for a separate bug for discussion/decision.

In the new order form, "Total" (and "Budgeted cost:" depending on previous question) now aren't consistent with the basket view. It's the previous step and the same data should have the same value.

I believe that this calculation is done with JavaScript only and might require a different kind of fix. It might be also not following the system preference?

So we would have basket's "total tax exc" honoring well OrderPriceRounding but there is still buggy rounding around. Either for the same data on the previous page or on the same page close to "total tax exc".

Can you explain this one a bit differently? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 --- Comment #13 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to Katrin Fischer from comment #12)

(In reply to Victor Grousset/tuxayo from comment #11)

...

In basket view, shouldn't "Budgeted cost tax exc." also honor OrderPriceRounding ? Same about "Budgeted cost tax inc" and "GST".

If it doesn't honor it right now, then this seems a bit out of scope here and would be better suited for a separate bug for discussion/decision.

Ok. It introduces an inconsistency (17.95 & 17.96 in the same page) but now that it's known and deemed fine for this bug. Not blocking then.

...

In the new order form, "Total" (and "Budgeted cost:" depending on previous question) now aren't consistent with the basket view. It's the previous step and the same data should have the same value.

I believe that this calculation is done with JavaScript only and might require a different kind of fix. It might be also not following the system preference?

Ok, same as above, not a issue for here then :)

...

So we would have basket's "total tax exc" honoring well OrderPriceRounding but there is still buggy rounding around. Either for the same data on the previous page or on the same page close to "total tax exc".

Can you explain this one a bit differently?

That was a sum up of the two previous points. But failed since it actually doesn't help and adds confusion ^^" -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #162701|0 |1 is obsolete| | --- Comment #15 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 171599 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171599&action=edit Bug 36049: Fix price rounding This patch only fixes price rounding by using Math::BigFloat. To test: 0) Enable syspref OrderPriceRounding 1) Create an order line in acquisitions with vendor price 18.90 and a discount of 5 %. 2) Verify that the total (tax excl.) in the order table on the basket page is 17.95. 3) Apply the patch. 4) Reload the basket page. 5) Verify that the total (tax excl.) in the order table on the basket page now is 17.96, which is correct. 6) Run: prove t/Number/Price.t Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=37937 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |37937 See Also|https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=37937 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37937 [Bug 37937] Budgeted cost tax exc. in basket page isn't honoring OrderPriceRounding -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 --- Comment #18 from Raphael Straub <raphael.straub@kit.edu> --- (In reply to Katrin Fischer from comment #17)

I'll have another look before pushing this as you are in principle right about adding inconsistencies. Maybe Raphael can weigh in here as he might have a better idea having looked at the code more closely?

To fix the whole problem, we would have to change every financial calculation in Koha (in Perl and JavaScript, the database does it right) to use a decimal datatype. I think it is not feasible to do it all at once. So, we should identify isolated parts of Koha that could be fixed independently without introducing new inconsistencies and fix those parts separately. The patch in this bug fixes a rounding function that is used in several parts of Koha and would introduce at least one inconsistency in the acquisition module (see bug 37937). I would not push this patch until we think that no new inconsistencies are introduced by it. I wrote this bug report mainly to point out that there is a problem with financial calculations in Koha and to provide an example of how this problem can be solved. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 --- Comment #19 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Thanks for all the hard work! Pushed to main for the next 24.11.00 release as RM Assistant -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |This fixes the values and release notes| |totals shown for orders | |when rounding prices using | |the OrderPriceRounding | |system preference. Example: | |vendor price for an item is | |18.90 and the discount is | |5%, the total would show as | |17.95 instead of 17.96. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtj@kohaaloha.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to oldstable |Pushed to oldoldstable Version(s)|24.11.00,24.05.06 |24.11.00,24.05.06,23.11.11 released in| | --- Comment #24 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 23.11.x for 23.11.11 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 Jesse Maseto <jesse@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jesse@bywatersolutions.com Status|Pushed to oldoldstable |Pushed to oldoldoldstable --- Comment #26 from Jesse Maseto <jesse@bywatersolutions.com> --- Pushed to 22.11.x for 22.11.26 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36049 Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED --- Comment #27 from Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> --- This seems to fix a bug, so there is nothing to change in the manual. -- You are receiving this mail because: You are watching all bug changes.