Bug 18900 - wrong number format in receiving order
Summary: wrong number format in receiving order
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Fridolin SOMERS
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 18471
Blocks:
  Show dependency treegraph
 
Reported: 2017-07-06 06:42 UTC by Fridolin SOMERS
Modified: 2017-09-15 23:24 UTC (History)
3 users (show)

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


Attachments
Bug 18900 - add UT to Number/Price.t (2.15 KB, patch)
2017-07-06 08:47 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 18900 - add UT to Number/Price.t (1.94 KB, patch)
2017-07-06 08:49 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 18900 - wrong number format in receiving order (1.50 KB, patch)
2017-07-06 09:01 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
[SIGNED OFF] Bug 18900 - add UT to Number/Price.t (2.01 KB, patch)
2017-08-08 11:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 18900 - wrong number format in receiving order (1.57 KB, patch)
2017-08-08 11:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 18900: add UT to Number/Price.t (2.05 KB, patch)
2017-08-09 15:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18900: (QA followup) Make tests conditional to locale existence (1.99 KB, patch)
2017-08-09 15:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18900: wrong number format in receiving order (1.61 KB, patch)
2017-08-09 15:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin SOMERS 2017-07-06 06:42:33 UTC
Bug 18471 added number formating in order receive.

I found a bug generated by this.
Koha::Number::Price defines methods to (un)format price depending on CurrencyFormat syspref.
The bug is that for CurrencyFormat US, some params are not defined in this module so there are retrieved from system locale. If system locale is not en_US, the params will be wrong. For example fr_FR will define , as decimal and so multiple price value by 1000.
Comment 1 Fridolin SOMERS 2017-07-06 06:42:47 UTC
18471
Comment 2 Fridolin SOMERS 2017-07-06 08:46:22 UTC
I have a UT that reproduces the bug.

For testing in acquisition, one must use Plack mode, I don't understand why.
Comment 3 Fridolin SOMERS 2017-07-06 08:47:10 UTC
Created attachment 64838 [details] [review]
Bug 18900 - add UT to Number/Price.t

This patch adds unit tests to t/Number/Price.t in order to valid the problem.
It sets in the script locale "en_US.UTF-8" by default so that those tests can be run with any system locale. And adds a test dedicated to this bug.

(cherry picked from commit 20b3141bdf750bebb50e837771724324f7a67c1b)
Comment 4 Fridolin SOMERS 2017-07-06 08:49:40 UTC
Created attachment 64839 [details] [review]
Bug 18900 - add UT to Number/Price.t

This patch adds unit tests to t/Number/Price.t in order to valid the problem.
Comment 5 Fridolin SOMERS 2017-07-06 09:01:02 UTC
Created attachment 64840 [details] [review]
Bug 18900 - wrong number format in receiving order

Bug 18471 added number formating in order receive.

I found a bug generated by this.
Koha::Number::Price defines methods to (un)format price depending on CurrencyFormat syspref.
The bug is that for CurrencyFormat US, some params are not defined in this module so there are retrieved from system locale. If system locale is not en_US, the params will be wrong. For example fr_FR will define ',' as decimal and so saved unit price is divided by 1000.

To recreate:
- On a server with locale 'fr_FR'
- With Plack mode
- Set CurrencyFormat on 'US'
- Open a basket
- Place an order for an item
- Close basket
- Receive order with changing 'Actual Cost' to '1,234'
- Note the 'Actual Cost' is now '1.23'
- Run unit test t/Number/Price.t without and with patch
Comment 6 Katrin Fischer 2017-07-06 18:00:46 UTC
Hi Fridolin, we also had some bad luck with Plack and amounts - see bug 18186!
Comment 7 Katrin Fischer 2017-08-08 11:55:44 UTC
Created attachment 65627 [details] [review]
[SIGNED OFF] Bug 18900 - add UT to Number/Price.t

This patch adds unit tests to t/Number/Price.t in order to valid the problem.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Katrin Fischer 2017-08-08 11:56:51 UTC
Created attachment 65628 [details] [review]
[SIGNED OFF] Bug 18900 - wrong number format in receiving order

Bug 18471 added number formating in order receive.

I found a bug generated by this.
Koha::Number::Price defines methods to (un)format price depending on CurrencyFormat syspref.
The bug is that for CurrencyFormat US, some params are not defined in this module so there are retrieved from system locale. If system locale is not en_US, the params will be wrong. For example fr_FR will define ',' as decimal and so saved unit price is divided by 1000.

To recreate:
- On a server with locale 'fr_FR'
- With Plack mode
- Set CurrencyFormat on 'US'
- Open a basket
- Place an order for an item
- Close basket
- Receive order with changing 'Actual Cost' to '1,234'
- Note the 'Actual Cost' is now '1.23'
- Run unit test t/Number/Price.t without and with patch

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Katrin Fischer 2017-08-08 11:57:55 UTC
Tests pass ok, no regressions found. We also have seen some strange phenomenons with prices and Plack, this might help in other cases too.
Comment 10 Tomás Cohen Arazi 2017-08-09 15:08:51 UTC
Created attachment 65759 [details] [review]
Bug 18900: add UT to Number/Price.t

This patch adds unit tests to t/Number/Price.t in order to valid the problem.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2017-08-09 15:08:57 UTC
Created attachment 65760 [details] [review]
Bug 18900: (QA followup) Make tests conditional to locale existence

It is important to highlight that tests have been skipped instead of having them pass
on the absence of the required fr_FR.UTF-8 locale installed on the system.

This patch does that check and effectively skips them.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2017-08-09 15:09:07 UTC
Created attachment 65761 [details] [review]
Bug 18900: wrong number format in receiving order

Bug 18471 added number formating in order receive.

I found a bug generated by this.
Koha::Number::Price defines methods to (un)format price depending on CurrencyFormat syspref.
The bug is that for CurrencyFormat US, some params are not defined in this module so there are retrieved from system locale. If system locale is not en_US, the params will be wrong. For example fr_FR will define ',' as decimal and so saved unit price is divided by 1000.

To recreate:
- On a server with locale 'fr_FR'
- With Plack mode
- Set CurrencyFormat on 'US'
- Open a basket
- Place an order for an item
- Close basket
- Receive order with changing 'Actual Cost' to '1,234'
- Note the 'Actual Cost' is now '1.23'
- Run unit test t/Number/Price.t without and with patch

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Jonathan Druart 2017-08-10 16:53:03 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 14 Fridolin SOMERS 2017-09-12 14:02:57 UTC
Pushed to 17.05.x, will be in 17.05.04.
Comment 15 Katrin Fischer 2017-09-15 23:24:29 UTC
These patches have been pushed to 16.11.x and will be in 16.11.12.