Bug 12310 - Decimal separators issues in patrons payments/fines
Summary: Decimal separators issues in patrons payments/fines
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: master
Hardware: All All
: P5 - low major with 10 votes (vote)
Assignee: Victor Grousset/tuxayo
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 18723
  Show dependency treegraph
 
Reported: 2014-05-23 07:38 UTC by paxed
Modified: 2018-09-14 14:23 UTC (History)
18 users (show)

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


Attachments
Accept different monetary formats as input (4.44 KB, patch)
2014-05-23 08:39 UTC, paxed
Details | Diff | Splinter Review
BUG 12310 : Modified scripts in order to convert any string to the good format (5.21 KB, patch)
2017-02-21 16:08 UTC, Baptiste
Details | Diff | Splinter Review
BUG 12310 : Modified scripts in order to convert any string to the good format (13.45 KB, patch)
2017-02-22 16:42 UTC, Baptiste
Details | Diff | Splinter Review
BUG 12310: Modified the tooltips (1.73 KB, patch)
2017-03-21 10:18 UTC, Baptiste
Details | Diff | Splinter Review
BUG 12310: Modified the tooltips (2.54 KB, patch)
2017-03-24 10:34 UTC, Baptiste
Details | Diff | Splinter Review
BUG 12310: Modified the tooltips (2.43 KB, patch)
2017-03-24 10:36 UTC, Baptiste
Details | Diff | Splinter Review
Bug 12310: Format prices on other pages (10.69 KB, patch)
2017-04-25 10:24 UTC, Baptiste
Details | Diff | Splinter Review
BUG 12310 : Modified scripts in order to convert any string to the good format (13.57 KB, patch)
2017-06-27 08:44 UTC, Baptiste
Details | Diff | Splinter Review
BUG 12310 : Modified scripts in order to convert any string to the good format (13.57 KB, patch)
2017-06-27 08:45 UTC, Baptiste
Details | Diff | Splinter Review
BUG 12310: Modified the tooltips (2.43 KB, patch)
2017-06-27 08:45 UTC, Baptiste
Details | Diff | Splinter Review
Bug 12310: Format prices on other pages (10.69 KB, patch)
2017-06-27 08:45 UTC, Baptiste
Details | Diff | Splinter Review
Screenshot rebasing problem (11.66 KB, image/png)
2017-06-27 09:51 UTC, Marc Véron
Details
Bug 12310 : Modify scripts in order to convert any string to the good format (23.01 KB, patch)
2017-06-27 14:50 UTC, Baptiste
Details | Diff | Splinter Review
Bug 12310 : Modify scripts in order to convert any string to the good format (23.04 KB, patch)
2017-06-27 14:52 UTC, Baptiste
Details | Diff | Splinter Review
Bug 12310 : Modify scripts in order to convert any string to the good format (19.80 KB, patch)
2017-06-28 08:56 UTC, Baptiste
Details | Diff | Splinter Review
Bug 12310 : Modify scripts in order to convert any string to the good format (35.26 KB, patch)
2018-02-23 10:28 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310 : Modify scripts in order to convert any string to the good format (20.70 KB, patch)
2018-02-26 15:05 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310 : Decimal separators issues: fix maninvoice, mancredit (21.26 KB, patch)
2018-03-15 17:50 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310 : Decimal separators issues: some fixes in fines and acquisitions (19.41 KB, patch)
2018-03-16 18:08 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310 : Decimal separators issues: some fixes in fines and acquisitions (19.51 KB, patch)
2018-03-23 17:46 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310 : Decimal separator issues: some fixes in fines (15.22 KB, patch)
2018-04-03 08:01 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310 : Decimal separator issues: some fixes in fines (15.48 KB, patch)
2018-04-04 18:39 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310 : Decimal separator issues: some fixes in fines (15.44 KB, patch)
2018-04-06 13:40 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: Follow up to keep example text (3.13 KB, patch)
2018-04-06 14:20 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 12310 : Decimal separator issues: fixes in fines (15.44 KB, patch)
2018-04-09 14:34 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310 : Follow up to keep the example text in fines (3.21 KB, patch)
2018-04-09 14:34 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: Follow up to prevent updatedatabase noise (1.16 KB, patch)
2018-04-09 17:23 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 12310 : Decimal separator issues: fixes in fines (15.43 KB, patch)
2018-04-19 09:31 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 12310 : Follow up to keep the example text in fines (3.36 KB, patch)
2018-04-19 09:31 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 12310: Follow up to prevent updatedatabase noise (1.31 KB, patch)
2018-04-19 09:31 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 12310 : Decimal separator issues: fixes in fines (15.43 KB, patch)
2018-04-19 09:32 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 12310 : Follow up to keep the example text in fines (3.36 KB, patch)
2018-04-19 09:32 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 12310: Follow up to prevent updatedatabase noise (1.31 KB, patch)
2018-04-19 09:32 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 12310: Fix decimal separators issues in fines: writeoff amount field (2.81 KB, patch)
2018-05-07 13:22 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310 : Decimal separator issues: fixes in fines (15.62 KB, patch)
2018-05-07 14:52 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 12310 : Follow up to keep the example text in fines (3.41 KB, patch)
2018-05-07 14:52 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 12310: Follow up to prevent updatedatabase noise (1.37 KB, patch)
2018-05-07 14:52 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 12310: Fix decimal separators issues in fines: writeoff amount field (2.87 KB, patch)
2018-05-07 14:52 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 12310: Decimal separator issues: fixes in fines (15.57 KB, patch)
2018-06-11 12:36 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: (follow-up) Keep the example text in fines (3.36 KB, patch)
2018-06-11 12:36 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: (QA follow-up) Prevent updatedatabase noise (1.31 KB, patch)
2018-06-11 12:36 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: (follow-up) Fix forgotten writeoff amount field (2.80 KB, patch)
2018-06-11 12:36 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: (QA follow-up) Fix typo: unformating → unformatting (5.41 KB, patch)
2018-06-11 12:37 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: (follow-up) Complete DecimalSeparators syspref description (2.06 KB, patch)
2018-06-11 12:37 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: Decimal separator issues: fixes in fines (15.69 KB, patch)
2018-06-11 14:49 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: (follow-up) Keep the example text in fines (3.41 KB, patch)
2018-06-11 14:49 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: (QA follow-up) Prevent updatedatabase noise (1.37 KB, patch)
2018-06-11 14:49 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: (follow-up) Fix forgotten writeoff amount field (2.90 KB, patch)
2018-06-11 14:50 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: (QA follow-up) Fix typo: unformating → unformatting (5.46 KB, patch)
2018-06-11 14:50 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: (follow-up) Complete DecimalSeparators syspref description (2.06 KB, patch)
2018-06-11 14:50 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: (QA follow-up) Simplify DecimalSeparators syspref description (1.67 KB, patch)
2018-07-12 17:11 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: (follow-up) Add relevant sysprefs to HEA (758 bytes, patch)
2018-07-13 07:35 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: (QA follow-up) Fix typo: unformating => unformatting (5.34 KB, patch)
2018-08-17 09:20 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: (QA follow-up) Simplify DecimalSeparators syspref description (1.67 KB, patch)
2018-08-17 09:20 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: (follow-up) Add relevant sysprefs to HEA (758 bytes, patch)
2018-08-17 09:20 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: (follow-up) Complete HEA tests (747 bytes, patch)
2018-08-17 17:45 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: (QA follow-up) Add prefs to test too (898 bytes, patch)
2018-08-20 12:53 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 12310: Decimal separator issues: fixes in fines (15.75 KB, patch)
2018-09-14 08:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12310: (follow-up) Keep the example text in fines (3.43 KB, patch)
2018-09-14 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12310: (QA follow-up) Prevent updatedatabase noise (1.37 KB, patch)
2018-09-14 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12310: (follow-up) Fix forgotten writeoff amount field (2.92 KB, patch)
2018-09-14 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12310: (QA follow-up) Fix typo: unformating => unformatting (5.39 KB, patch)
2018-09-14 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12310: (QA follow-up) Simplify DecimalSeparators syspref description (1.67 KB, patch)
2018-09-14 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12310: (follow-up) Add relevant sysprefs to HEA (765 bytes, patch)
2018-09-14 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12310: (QA follow-up) Add prefs to test too (837 bytes, patch)
2018-09-14 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description paxed 2014-05-23 07:38:19 UTC
When a monetary amount is inputted by user, Koha only accepts it in the format "XX.XX", when in some countries the cent separator is a comma, not a full stop.

Eg. when entering a manual invoice amount of "12,34" it is saved as "12.00" in the system.

Should accept formats with full stop or comma as the cents separator, optionally prefixed or suffixed with a currency symbol.

eg. "12.34", "$12.34", "12,34", "12,34 €", etc.
Comment 1 paxed 2014-05-23 08:39:45 UTC
Created attachment 28455 [details] [review]
Accept different monetary formats as input

Adds a new module Koha::MoneyUtils and tests for it.
Adds monetary amount parsing when entering a manual invoice
for a patron.

To test:

1) Test different manual invoice values, note which ones
   do not work correctly. Eg. when entering amount of "12,34"
   it is saved as "12.00" in the system.
2) Apply patch
3) Repeat part 1, all should work correctly.
Comment 2 Lise 2014-06-03 08:14:45 UTC
we have tested your patch with "12,34" and "$12.34" and koha only accept "12.34"
with acquisitions.

sorry
Comment 3 paxed 2014-06-03 08:28:39 UTC
The patch only changes the parsing for manual invoice input, not acquisition.

I'll add the acq when I've got some time.
Comment 4 Baptiste 2017-02-21 16:08:42 UTC
Created attachment 60530 [details] [review]
BUG 12310 : Modified scripts in order to convert any string to the good format

paxed's solution is obsolete since the form asks for a number.

The script includes 2 function which check if it is possible to transform
properly into the format "XX.XX" and which do it.
The money symbols are ignored.
All spaces (before or after)  are ignored.

Test plan :
1) Check you can't enter any string except like "XX.XX" when
   - you create a Budget (Administration->Budget)
   - you write a fine (Circulation->[select a Borrower]->new fine )
2) Apply patch
3) Check you can enter any string with
   - $, "Euro" (before or after), or nothing
   - , or . as separator
   - no more than 2 digits after separator
   - as much digits as you wish before the separator
   - as much spaces as you wish before or after the number
Comment 5 Marc Véron 2017-02-22 10:19:11 UTC
Testing on Home > Patrons > Pay fines (Pay amount)...

CHF 1.45 -> 1.45 OK
12.50$ -> 12.50 OK
199 -> 199.00 OK
1'345.95 -> 1345.95 OK (Switzerland)
12,34,567.89 -> 1234567.89 OK (Pakistan, India)
12,34,567 -> 1234567.00 OK
1 234 567,89 -> 123456789.00  OK (Germany...)

However:
1.234.567,89 -> 0.23 NOT OK (Spain...)
1.234.567 -> 0.23 NOT OK
(Maybe a Spanish speaking person could comment on that one? Mail to dev list?)

See also:
https://en.wikipedia.org/wiki/Decimal_mark

-----
Note for testing:

On Home > Patrons > Pay fines (Pay amount), it immediately triggers on field exit

On Home > Patrons > Manual invoice and Home > Patrons > Manual credit, it triggers while submitting, same with Home > Administration > Budgets > Add budget 

------
Comment 6 Baptiste 2017-02-22 10:33:15 UTC
Hum, I'm stunned that following inputs work on your koha :

12,34,567.89 -> 1234567.89 OK (Pakistan, India)
12,34,567 -> 1234567.00 OK

I'll work on a syspref to choose the monetary format, in order to let the user decide how he wants to input the budgets (it shouldn't change inside one library)
Comment 7 Baptiste 2017-02-22 10:54:08 UTC
Hum, actually you tested on an interface I haven't modified at all, I'll apply my patch on this field to.
Comment 8 Baptiste 2017-02-22 16:42:54 UTC
Created attachment 60563 [details] [review]
BUG 12310 : Modified scripts in order to convert any string to the good format

paxed's solution is obsolete since the form asks for a number.

The script includes 2 function which check if it is possible to transform
properly into the format "XX.XX" and which do it.
The money symbols are ignored.
All other symbols  are ignored.

Test plan :
1) Check you can't enter any string except like "XX.XX" when
   - you create a Budget (Administration->Budget)
   - you write a fine (Circulation->[select a Borrower]->new fine )
   - you pay an existing fine
2) Apply patch
3) Check you can enter any string with
   - anything you want
   - a , or a . as separator (both are default)
   - change the syspref DigitSeparator into the separator you with and check again
          ex : XXX,XXX,XXX.XX => .
               XXX XXX XXX,XX => ,. or ,
               XXX XXX XXX.XX => ,. or .
   The string is immediately changed after you change the field
Please note that any other symbole is ignored
Comment 9 Rozenn Davenel 2017-03-20 14:52:35 UTC
With or without the "", the result is the same.
when entering amount of "12,34" it is saved as "12.40" in the system.

Don't accept formats with full stop or comma as the cents separator, optionally prefixed or suffixed with a currency symbol.
Comment 10 Baptiste 2017-03-21 10:18:29 UTC
Created attachment 61357 [details] [review]
BUG 12310: Modified the tooltips

Modified the tooltips helping the users entering monetary information in the right format.
Comment 11 Baptiste 2017-03-21 10:21:03 UTC
Hi, 
Sorry, the testplan was wrong, you have to update the database before testing, so the new test plan is. I Changed the tooltips in order to let the users know something is wrong if the database is not up to date.

Test plan :
1) Check you can't enter any string except like "XX.XX" when
   - you create a Budget (Administration->Budget)
   - you write a fine (Circulation->[select a Borrower]->new fine )
2) Apply patch
2.1) update database (installer/data/mysql/updatedatabase.pl)
3) Check you can enter any string with
   - $, "Euro" (before or after), or nothing
   - , or . as separator
   - no more than 2 digits after separator
   - as much digits as you wish before the separator
   - as much spaces as you wish before or after the number
Comment 12 Koha Team Lyon 3 2017-03-23 10:12:04 UTC
IF you enter 16,34 it's changed in 16.4
It seems that the number after the comma go away when comma is transformed in dot
Comment 13 Baptiste 2017-03-24 10:34:28 UTC
Created attachment 61603 [details] [review]
BUG 12310: Modified the tooltips

Modified the tooltips helping the users entering monrtary information in the right format.

BUG 12310: Corrected the RegExp
Comment 14 Baptiste 2017-03-24 10:36:01 UTC
Created attachment 61604 [details] [review]
BUG 12310: Modified the tooltips

Modified the tooltips helping the users entering monrtary information in the right format.

BUG 12310: Corrected the RegExp
Comment 15 Koha Team Lyon 3 2017-03-24 11:52:25 UTC
I'm testing now.
Looks goog but I haven't finished yet.
But It should be extended to this page : /cgi-bin/koha/admin/aqbudgets.pl
/cgi-bin/koha/members/mancredit.pl
and /cgi-bin/koha/members/maninvoice.pl? as well
Perhaps in cgi-bin/koha/acqui/neworderempty.pl too
Comment 16 Koha Team Lyon 3 2017-03-24 13:21:03 UTC
I repeat tests proposed by Marc in Comment 5  :

With syspref DigitSeparator : ",."

CHF 1.45 -> 1.45 OK
12.50$ -> 12.50 OK
199 -> 199.00 OK
1'345.95 -> 1345.95 OK (Switzerland)

With syspref DigitSeparator : "."

12,34,567.89 -> 1234567.89 OK (Pakistan, India)
12,34,567 -> 1234567.00 OK
1 234 567,89 -> 123456789.00  OK (Germany...)

With syspref DigitSeparator : ","
1.234.567,89 -> 1234567.89 	
1.234.567 -> 1234567.00 	

Calculations are good but I don't want to sign-off this patch before the other pages are done (see comment 15)
Comment 17 Baptiste 2017-04-25 10:24:33 UTC
Created attachment 62655 [details] [review]
Bug 12310: Format prices on other pages

Fields on thoses page are now automatically formatted.
- cgi-bin/koha/admin/aqbudgetperiods.tt
- cgi-bin/koha/admin/aqbudgets.tt
- cgi-bin/koha/members/mancredit.tt
- cgi-bin/koha/members/maninvoice.tt
Comment 18 Fridolin SOMERS 2017-06-27 08:40:54 UTC
*** Bug 17462 has been marked as a duplicate of this bug. ***
Comment 19 Baptiste 2017-06-27 08:44:18 UTC
Created attachment 64671 [details] [review]
BUG 12310 : Modified scripts in order to convert any string to the good format

paxed's solution is obsolete since the form asks for a number.

The script includes 2 function which check if it is possible to transform
properly into the format "XX.XX" and which do it.
The money symbols are ignored.
All other symbols  are ignored.

Test plan :
1) Check you can't enter any string except like "XX.XX" when
   - you create a Budget (Administration->Budget)
   - you write a fine (Circulation->[select a Borrower]->new fine )
   - you pay an existing fine
2) Apply patch
3) Check you can enter any string with
   - anything you want
   - a , or a . as separator (both are default)
   - change the syspref DigitSeparator into the separator you with and check again
          ex : XXX,XXX,XXX.XX => .
               XXX XXX XXX,XX => ,. or ,
               XXX XXX XXX.XX => ,. or .
   The string is immediately changed after you change the field
Please note that any other symbole is ignored
Comment 20 Baptiste 2017-06-27 08:45:23 UTC
Created attachment 64672 [details] [review]
BUG 12310 : Modified scripts in order to convert any string to the good format

paxed's solution is obsolete since the form asks for a number.

The script includes 2 function which check if it is possible to transform
properly into the format "XX.XX" and which do it.
The money symbols are ignored.
All other symbols  are ignored.

Test plan :
1) Check you can't enter any string except like "XX.XX" when
   - you create a Budget (Administration->Budget)
   - you write a fine (Circulation->[select a Borrower]->new fine )
   - you pay an existing fine
2) Apply patch
3) Check you can enter any string with
   - anything you want
   - a , or a . as separator (both are default)
   - change the syspref DigitSeparator into the separator you with and check again
          ex : XXX,XXX,XXX.XX => .
               XXX XXX XXX,XX => ,. or ,
               XXX XXX XXX.XX => ,. or .
   The string is immediately changed after you change the field
Please note that any other symbole is ignored
Comment 21 Baptiste 2017-06-27 08:45:28 UTC
Created attachment 64673 [details] [review]
BUG 12310: Modified the tooltips

Modified the tooltips helping the users entering monrtary information in the right format.

BUG 12310: Corrected the RegExp
Comment 22 Baptiste 2017-06-27 08:45:33 UTC
Created attachment 64674 [details] [review]
Bug 12310: Format prices on other pages

Fields on thoses page are now automatically formatted.
- cgi-bin/koha/admin/aqbudgetperiods.tt
- cgi-bin/koha/admin/aqbudgets.tt
- cgi-bin/koha/members/mancredit.tt
- cgi-bin/koha/members/maninvoice.tt
Comment 23 Baptiste 2017-06-27 08:45:57 UTC
Rebased
Comment 24 Marc Véron 2017-06-27 09:51:17 UTC
Created attachment 64675 [details]
Screenshot rebasing problem

Screenshot of text that remained from a merge / rebasing.
Comment 25 Marc Véron 2017-06-27 10:15:26 UTC
1)

The syspref is populated with ,.
The text says: 
"The separator used between integer and decimal part"

Maybe it would be better to explain a little bit more:
"Accept any of the following characters as separator between integer and decimal part. Note: To format prices use the system preference 'CurrencyFormat'."


2)

The explanation near the input field says:
Please use the "," as separator : ex "XX,XX", if you want to change it, click here 

It should explain that other chars are accepted, something like:
"Please use any of the following character(s) as decimal separator: ',.'"

3)

The part ",if you want to change it..." should be a separate sentence 
 - for better translatability (keep sentences simple)
 - to hide it if the patron has no permission to edit system preferences
Comment 26 Baptiste 2017-06-27 14:50:32 UTC Comment hidden (obsolete)
Comment 27 Baptiste 2017-06-27 14:52:45 UTC
Created attachment 64679 [details] [review]
Bug 12310 : Modify scripts in order to convert any string to the good format

paxed's solution is obsolete since the form asks for a number.

The script includes 2 function which check if it is possible to transform
properly into the format "XX.XX" and which do it.
The money symbols are ignored.
All other symbols  are ignored.

Modify the tooltips helping the users entering monrtary information in the right format.

Fields on all those pages are now automatically formatted
- cgi-bin/koha/admin/aqbudgetperiods.tt
- cgi-bin/koha/admin/aqbudgets.tt
- cgi-bin/koha/members/mancredit.tt
- cgi-bin/koha/members/maninvoice.tt

Test plan :
1) Check you can't enter any string except like "XX.XX" when
   - you create a Budget (Administration->Budget)
   - you write a fine (Circulation->[select a Borrower]->new fine )
   - you pay an existing fine
2) Apply patch
3) Check you can enter any string with
   - anything you want
   - a , or a . as separator (both are default)
   - change the syspref DigitSeparator into the separator you with and check again
          ex : XXX,XXX,XXX.XX => .
               XXX XXX XXX,XX => ,. or ,
               XXX XXX XXX.XX => ,. or .
   The string is immediately changed after you change the field
Please note that any other symbole is ignored
Comment 28 Marc Véron 2017-06-27 17:47:07 UTC
Sorry, there is still something wrong (example see below).

And I think it would be a good idea to put the whole "hint" part in an include file that can be re-used. At the moment we have code duplication in several tt files.

For exammple in maninvoice.tt line 381 is obsolete
In line 382, instead of a <\span> there should be a  <span class="hint">
Line 383 displays only the first of the separators, should display all of them.
In line 385, the <span> should be a <span class="hint">
Line 389 should be enclosed in <span class="hint"> .... </span>

Then I have one more issue regarding translatability (sorry, I was not aware before):

If you want to change it, click <a href="/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=DigitSeparator">here </a>

This will split the sentence in Pootle, translators will have:
"If you want to change it, click "
and a stray
"here"

It is much better for translatability not to split sentences by HTML tags.
What do you think about the following?

<a href="/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=DigitSeparator">Click here to edit separators</a>
Comment 29 Katrin Fischer 2017-06-27 21:08:47 UTC
What about just "Edit separators"?

It's a pet peeve of mine... not so much a fan of 'click here' - a link should be obvious enough :)
Comment 30 Marc Véron 2017-06-27 22:14:03 UTC
(In reply to Katrin Fischer from comment #29)
> What about just "Edit separators"?
> 
> It's a pet peeve of mine... not so much a fan of 'click here' - a link
> should be obvious enough :)

+1
Comment 31 Baptiste 2017-06-28 08:56:50 UTC
Created attachment 64696 [details] [review]
Bug 12310 : Modify scripts in order to convert any string to the good format

paxed's solution is obsolete since the form asks for a number.

The script includes 2 function which check if it is possible to transform
properly into the format "XX.XX" and which do it.
The money symbols are ignored.
All other symbols  are ignored.

Modify the tooltips helping the users entering monrtary information in the right format.

Fields on all those pages are now automatically formatted
- cgi-bin/koha/admin/aqbudgetperiods.tt
- cgi-bin/koha/admin/aqbudgets.tt
- cgi-bin/koha/members/mancredit.tt
- cgi-bin/koha/members/maninvoice.tt

Test plan :
1) Check you can't enter any string except like "XX.XX" when
   - you create a Budget (Administration->Budget)
   - you write a fine (Circulation->[select a Borrower]->new fine )
   - you pay an existing fine
2) Apply patch
3) Check you can enter any string with
   - anything you want
   - a , or a . as separator (both are default)
   - change the syspref DigitSeparator into the separator you with and check again
          ex : XXX,XXX,XXX.XX => .
               XXX XXX XXX,XX => ,. or ,
               XXX XXX XXX.XX => ,. or .
   The string is immediately changed after you change the field
Please note that any other symbole is ignored
Comment 32 Marc Véron 2017-06-29 06:07:28 UTC
Hi Babtiste

- Functionality works OK

- In syspref DigitSeparator I have . and , defined as separators, however the message says:
Please use any of the following character(s) "," as separator : ex "XX,XX". Edit Separator 
I would expect:
Please use any of the following character(s) ".," as separator : ex "XX,XX". Edit Separator 
(BTW: the space before : (French style) is not necessary in English)

- In 'Create manual credit", the message says:
Example: 5.00 Please use any of the following character(s) "," as separator : ex "XX,XX". Edit Separator 
The first part 'Example: 5.00' is superfluous, or maybe better: Reword your hint message to:
Please use any of the following character(s) "," as separator. Example: 5,00 Edit Separator 

- In aqbudgets.pl (Administration > Budgets > Funds > Modify fund we have 3 fields with decimal separator, but only 'Amount' and 'Warning at (amount)' trigger. 'Warning at (%)' does not trigger. - Additionally I think it would be enough to display the hint message only once (with the first field).

- There are more input fields with decimal separator, e.g. in acquisitions when ordering, receiving... Do you plan to implement the decimal separator here as well (maybe in a bug that depends on this one)?
Comment 33 Baptiste 2017-06-29 07:53:49 UTC
> Hi Babtiste
> 
> - Functionality works OK

Owww yeah ! :p

> 
> - In syspref DigitSeparator I have . and , defined as separators, however
> the message says:
> Please use any of the following character(s) "," as separator : ex "XX,XX".
> Edit Separator 
> I would expect:
> Please use any of the following character(s) ".," as separator : ex "XX,XX".
> Edit Separator 
> (BTW: the space before : (French style) is not necessary in English)

hum ok, I'll do it 

> 
> - In 'Create manual credit", the message says:
> Example: 5.00 Please use any of the following character(s) "," as separator
> : ex "XX,XX". Edit Separator 
> The first part 'Example: 5.00' is superfluous, or maybe better: Reword your
> hint message to:
> Please use any of the following character(s) "," as separator. Example: 5,00
> Edit Separator 

This isn't so easy to do since the general message is now in an include: this is not splittable, I think I can just remove the old message and use only the common message for this one too.

> 
> - In aqbudgets.pl (Administration > Budgets > Funds > Modify fund we have 3
> fields with decimal separator, but only 'Amount' and 'Warning at (amount)'
> trigger. 'Warning at (%)' does not trigger. - Additionally I think it would
> be enough to display the hint message only once (with the first field).
> 
> - There are more input fields with decimal separator, e.g. in acquisitions
> when ordering, receiving... Do you plan to implement the decimal separator
> here as well (maybe in a bug that depends on this one)?

Initially, this was designed to allow cc/paste of monetary sums (with $ or € symbol eventually). At the moment I don't think I will extend this to other fields if they are not monetary: I'm living Biblibre in two weeks and I'm not sure I will have enough time, but if someone wants to open a new ticket (or continue this one) please feel free.
Comment 34 Victor Grousset/tuxayo 2018-02-08 13:31:33 UTC
Baptiste doesn't work anymore at BibLibre.
Unassigning to avoid signaling that this bug is still being supervized. Which could wrongly discourage someone to continue the work.
Comment 35 Koha Team Lyon 3 2018-02-22 15:29:10 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 36 Victor Grousset/tuxayo 2018-02-22 17:20:27 UTC
It turns out that this is my next task :)
Comment 37 Victor Grousset/tuxayo 2018-02-22 17:38:01 UTC
Conflicts T_T
Comment 38 Victor Grousset/tuxayo 2018-02-23 10:28:37 UTC Comment hidden (obsolete)
Comment 39 Victor Grousset/tuxayo 2018-02-26 15:05:15 UTC Comment hidden (obsolete)
Comment 40 Victor Grousset/tuxayo 2018-02-26 15:27:35 UTC Comment hidden (obsolete)
Comment 41 Koha Team Lyon 3 2018-03-13 11:13:12 UTC
First test, first issue...
I had ".," in the digitseparator syspref.
I tried to add a manual invoice in Patron (/cgi-bin/koha/members/maninvoice.pl)
I put "10,34" and it was saved "10.00", decimal values are not saved...

Sonia
Comment 42 Victor Grousset/tuxayo 2018-03-15 10:14:06 UTC
some QA feedback:
- maybe hardcode the two decimal separators (,.) instead of creating a syspref
- don't display a long hint message next to each field. Even display nothing might be okay.
Comment 43 Victor Grousset/tuxayo 2018-03-15 10:32:02 UTC Comment hidden (obsolete)
Comment 44 Victor Grousset/tuxayo 2018-03-15 10:39:21 UTC
Critical: because it must address
- bug 18723
- bug 19521
Comment 45 Victor Grousset/tuxayo 2018-03-15 10:43:04 UTC
After talking to cait: must support thousands separators
After talking to BULAC and Lyon 3 and at least 2 other libraries: must support multiple decimal separators in **the same instance** (12.56 and 12,56 → 12.56)
Comment 46 Victor Grousset/tuxayo 2018-03-15 11:28:57 UTC
some QA feedback:
- enforce JS9 coding guideline
- JS10: use JQuery validation
    https://gitlab.com/koha-community/Koha/blob/master/koha-tmpl/intranet-tmpl/prog/en/includes/password_check.inc
- it might be acceptable to not have JS form validation (as there is none now). Is there is, it could be a follow up bug.
- test if there is server-side validation. Check at least what happens.
  - "T_T"
  - "11.34,45.5"
  - "madness12.34"
  - "12.34nooooo"
  - "lol12.34why"
  - "1.000,000.56"
  - "1,000.000,56"
Comment 47 Victor Grousset/tuxayo 2018-03-15 12:51:38 UTC
- 1.1
- 1.000,6 → 1 with syspref ",."
- 1.000,6 → 1000.6 with syspref ","
- .6,
- 0
- 1,346.55 → 1.346.55 with syspref ",."
- 1,346.55 → 1346.55 with syspref ","

So syspref needed to have support of thousands separators at the expense of supporting both . and , as decimal one.
Comment 48 Victor Grousset/tuxayo 2018-03-15 15:00:17 UTC
After talking to Lyon 3, BULAC, Owen, cait and another library. The conclusions are:
- the current behavior is okay
- we need to keep the syspref to leave the option to handle thousands separator

Finally we will be able to move on! :D
Comment 49 Victor Grousset/tuxayo 2018-03-15 15:48:37 UTC
TODO check that bug 18471 isn't back.

== Plan ==
- Make the patch work again
- Fix all the QA issues, make a clean thing
- sign off (spam Lyon 3, cait, Marc Véron, BULAC, etc to make them do that)
- etc
Comment 50 Victor Grousset/tuxayo 2018-03-15 17:50:26 UTC Comment hidden (obsolete)
Comment 51 Séverine Queune 2018-03-16 09:09:12 UTC
Tested with different separators on the syspref : patch correctly applies on the indicated pages.
The messages "Please use any of the following character(s) "." as separator : ex "XX.XX"." should be removed, or be more responsive and clone the informations on the DigitSeparator syspref.
The link "Edit Separator" should also be removed, or apperas only for users with the "parameters_remaining_permissions" permission.
Would be good to have this enhencement for receipt page (cgi-bin/koha/acqui/orderreceive.pl) :)
Comment 52 Victor Grousset/tuxayo 2018-03-16 18:08:56 UTC Comment hidden (obsolete)
Comment 53 Victor Grousset/tuxayo 2018-03-23 17:46:46 UTC Comment hidden (obsolete)
Comment 54 Katrin Fischer 2018-03-24 10:48:15 UTC
Hi Victor, should this be Needs Signoff?
Comment 55 Victor Grousset/tuxayo 2018-03-26 09:57:20 UTC
not yet, TODO:
- don't bind change event in HTML but with JS
- be consistent with the position of the includes
- ensure var and function names are clear
- check if things can be done to improve backportability
- add to test plan thousands sep and other relevant cases
- complete the test cases in the test plan or at least test once on my machine, see [1] for possible additions
- check that bug 18471 isn't back.
- remove useless or redundant test cases


== [1] possible additions of the test plan ==
Draft, there might be redundant or useless things
  1.000,00
  1,000.00
  1.000.000,00
  1,000,000.00
  1.000
  1,000
  1.000.000
  1,000,000
  1,123 → 1123
  1.123,456 → 1123.456 (sent to the server) → 1123.46
  1.123,45 → 1123.45(sent to the server) → 1123,45
  1,234,567 → 1234567
  1.234.567 → 1234567
  1.234.567,89 → 1234567.89
  1,234,567.89 → 1234567.89
  1'345.95 -> 1345.95 (Switzerland)
  12,34,567.89 -> 1234567.89 (Pakistan, India)
  12,34,567 -> 1234567.00
  1 234 567,89 -> 123456789.00  (Germany...)
  1.234.567,89 (Spain...)
  1.234.567
  ,1234
- change language of the UI
- Chromium
- IE 9
Comment 56 Victor Grousset/tuxayo 2018-04-03 08:01:26 UTC Comment hidden (obsolete)
Comment 57 Victor Grousset/tuxayo 2018-04-04 18:39:45 UTC
Created attachment 73657 [details] [review]
Bug 12310 : Decimal separator issues: some fixes in fines

The main goal is to prevent
- x100 or /100 of the amounts
- decimal part truncation

Fixes Bug 19521 - Partial fine payment is not working with comma decimal separator

Bonuses:
- Gets rid of the spaces, currency symbols, names (EUR)
  There is nothing specific about currency symbols is this patch.
  It just removes everything that is not a digit a minus or a separator.

It handles:
- by default comma and dot as decimal separators on the same instance
- dot or comma as thousands separators if choose to use only one of them
    as decimal separator. See DigitSeparator syspref.
- minuses, doesn't remove them

== Test plan ==
1. Use a translation of the staff interface if possible.
2. When you pay an existing fine (more details in the next steps if needed).
     Check that you can't enter any string except like "XX.XX" (examples in
     Annex 1) without having an issue after validation. (x100,
     truncation, etc)
3. Apply this patch
4. run ~/src/installer/data/mysql/updatedatabase.pl
5. In the manual invoice (cgi-bin/koha/members/maninvoice.pl)
6. Apply the annex 1 to the "Amount" field
7. When paying a fine (cgi-bin/koha/members/paycollect.pl)
8. Click on "Pay amount" to go to the page to pay all fines
9. Apply the annex 1 to the "Collect from patron" field
10. Click on "Pay" to go to the page to pay fine
11. Apply the annex 1 to the "Collect from patron" field
12. Go to "Create manual credit" (staff://cgi-bin/koha/members/mancredit.pl)
13. Apply the annex 1 to the "Amount" field
14. If you have ideas of stuff to try to find *regressions* from the previous
    versions, then try. You might spare librarian tears by finding something
    before it's released. (and prevent me from having bounty on my head)
15. Express your happiness. This is important, otherwise the sign off spell won't
    work! The details of expressing your happiness depend on your culture and
    habits.

=== Annex 1: Subpart for each concerned page ===
1. Set DecimalSeparators syspref to ",."
2. Input "12,34"
3. Press the "Tab/Tabulation" key. It will unfocus the field and trigger the
     conversion. Validating the form does the same but it's a trick to test more
     quickly by not switching to another page.
4. The value should have been converted to 12.34
5. What follows is just a list of examples. They don't have to be tried on each
     field.
6. Do the same with "12,34 EUR" and ensure that it's converted to "12,34"
7. Same with "EUR 12.34" -> "12.34"
8. "EUR 12.34€hi lyonthree" -> "12.34"
9. "123 456,7" -> "123456.7"
10. "2" -> "2"
11. "2,0" -> "2.0"
12. "-1'345.95" -> "-1345.95"
13. ",1234" -> ".1234"
14. Set DecimalSeparators syspref to ","
15. Reload the page with the form
16. "1.000" -> "1000"
17. "1.000.000,10" -> "1000000.10"
18. Set DecimalSeparators syspref to "."
19. Reload the page with the form
20. "1,000" -> "1000"
21. "1,000,000.10" -> "1000000.10"
22. Validate the form and check that the final value with the dot was
      well handled.
23. Think about Koalas and smile. 🐨🐨🐨
Comment 58 M. Tompsett 2018-04-05 15:16:56 UTC
Comment on attachment 73657 [details] [review]
Bug 12310 : Decimal separator issues: some fixes in fines

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

::: koha-tmpl/intranet-tmpl/prog/en/includes/decimal-input-js.inc
@@ +16,5 @@
> +    }
> +
> +    var decimal_from_string = function(formatted_decimal) {
> +        if (!is_decimal(formatted_decimal)) {
> +            return false;

The function name is decimal_from_string. It's not intuitive to return false. Why not just return what was passed when it isn't a decimal? Which leads to the question, why call the parameter formatted_decimal if there is a is_decimal check? It is just a formatted_value. It may or may not be a decimal. Also, why do a is_decimal check in this function and the calling function?

@@ +34,5 @@
> +         * to care about decimal separators (see DecimalSeparators syspref) and
> +         * currency symbols for example.
> +         */
> +        handler_unformat_input: function(field) {
> +            if (is_decimal(field.value)) {

Why do a is_decimal check here and in the decimal_from_string function?
Just do the assignment, and make sure the decimal_from_string returns what was passed, if it isn't a decimal.

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/mancredit.tt
@@ -40,4 @@
>  	<li><label for="barcode">Barcode: </label><input type="text" name="barcode" id="barcode" /></li>
>  	<li><label for="desc">Description: </label><input type="text" name="desc" size="50" id="desc" /></li>
>      <li><label for="note">Note: </label><input type="text" name="note" size="50" id="note" /></li>
> -    <li><label for="amount">Amount: </label><input type="number" name="amount" id="amount" required="required" value="" step="any" min="0" /> Example: 5.00</li>

placeholder="Example 5.00" would be a better solution than cutting this.

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/maninvoice.tt
@@ -53,4 @@
>  	<li><label for="barcode">Barcode: </label><input type="text" name="barcode" id="barcode" /></li>
>  	<li><label for="desc">Description: </label><input type="text" name="desc" id="desc" size="50" /></li>
>      <li><label for="note">Note: </label><input type="text" name="note" size="50" id="note" /></li>
> -    <li><label for="amount">Amount: </label><input type="number" name="amount" id="amount" required="required" value="" step="any" min="0" /> Example: 5.00</li>

placeholder="Example 5.00" is a better solution.
Comment 59 Victor Grousset/tuxayo 2018-04-06 13:40:53 UTC
Created attachment 73784 [details] [review]
Bug 12310 : Decimal separator issues: some fixes in fines

The main goal is to prevent
- x100 or /100 of the amounts
- decimal part truncation

Fixes Bug 19521 - Partial fine payment is not working with comma decimal separator

Bonuses:
- Gets rid of the spaces, currency symbols, names (EUR)
  There is nothing specific about currency symbols is this patch.
  It just removes everything that is not a digit a minus or a separator.

It handles:
- by default comma and dot as decimal separators on the same instance
- dot or comma as thousands separators if choose to use only one of them
    as decimal separator. See DigitSeparator syspref.
- minuses, doesn't remove them
- non-decimal values: it doesn't touch them

== Test plan ==
1. Use a translation of the staff interface if possible.
2. When you pay an existing fine (more details in the next steps if needed).
     Check that you can't enter any string except like "XX.XX" (examples in
     Annex 1) without having an issue after validation. (x100,
     truncation, etc)
3. Apply this patch
4. run ~/src/installer/data/mysql/updatedatabase.pl
5. In the manual invoice (cgi-bin/koha/members/maninvoice.pl)
6. Apply the annex 1 to the "Amount" field
7. When paying a fine (cgi-bin/koha/members/paycollect.pl)
8. Click on "Pay amount" to go to the page to pay all fines
9. Apply the annex 1 to the "Collect from patron" field
10. Click on "Pay" to go to the page to pay fine
11. Apply the annex 1 to the "Collect from patron" field
12. Go to "Create manual credit" (staff://cgi-bin/koha/members/mancredit.pl)
13. Apply the annex 1 to the "Amount" field
14. If you have ideas of stuff to try to find *regressions* from the previous
    versions, then try. You might spare librarian tears by finding something
    before it's released. (and prevent me from having bounty on my head)
15. Express your happiness. This is important, otherwise the sign off spell won't
    work! The details of expressing your happiness depend on your culture and
    habits.

=== Annex 1: Subpart for each concerned page ===
1. Set DecimalSeparators syspref to ",."
2. Input "12,34"
3. Press the "Tab/Tabulation" key. It will unfocus the field and trigger the
     conversion. Validating the form does the same but it's a trick to test more
     quickly by not switching to another page.
4. The value should have been converted to 12.34
5. What follows is just a list of examples. They don't have to be tried on each
     field.
6. Do the same with "12,34 EUR" and ensure that it's converted to "12,34"
7. Same with "EUR 12.34" -> "12.34"
8. "EUR 12.34€hi lyonthree" -> "12.34"
9. "123 456,7" -> "123456.7"
10. "2" -> "2"
11. "2,0" -> "2.0"
12. "-1'345.95" -> "-1345.95"
13. ",1234" -> ".1234"
14. Set DecimalSeparators syspref to ","
15. Reload the page with the form
16. "1.000" -> "1000"
17. "1.000.000,10" -> "1000000.10"
18. Set DecimalSeparators syspref to "."
19. Reload the page with the form
20. "1,000" -> "1000"
21. "1,000,000.10" -> "1000000.10"
22. Validate the form and check that the final value with the dot was
      well handled.
23. Think about Koalas and smile. 🐨🐨🐨
Comment 60 Victor Grousset/tuxayo 2018-04-06 13:43:09 UTC
(In reply to M. Tompsett from comment #58)
> The function name is decimal_from_string. It's not intuitive to return false. 

It doesn't make sense to return false, removing it.

> Also, why do a is_decimal check in this function and the calling function?

Because I missed this when refactoring the legacy patch ^^ Thanks for catching this :)

> Just do the assignment, and make sure the decimal_from_string returns what was passed, if it isn't a decimal.

It doesn't touch the content when the input isn't a number.
I feared that there might cases where this could be confusing or annoying. (to always unformat the field)
I wasn't confident enough that I wouldn't create edge cases issues.

But other opinions are welcome and I will change that if we consider safe to always unformat the field.
After considering that this feature could be used on all decimal fields of Koha.

> placeholder="Example 5.00"

When the syspref equals "," to use the dots as thousand separators.
Then this placeholder or tip wouldn't make sense.


Has everything be addressed?
Comment 61 Victor Grousset/tuxayo 2018-04-06 13:43:29 UTC
*Has everything been addressed?
Comment 62 M. Tompsett 2018-04-06 14:20:33 UTC
Created attachment 73785 [details] [review]
Bug 12310: Follow up to keep example text

This put the example text back in with the correct decimal
separator.

TEST PLAN
---------
1) Test first patch
2) Go to a patron's fines tab.
3) Confirm that Create manual invoice and Create manual credit
   tabs both have blank amounts.
4) apply this patch
5) restart_all
6) reload the page and check both create tabs.
   -- Example appears.
7) set your I18N/L10N DecimalSeparators to something strange like #.
8) go back and check both tabs.
   -- Example appears with the decimal separator that was set.
9) run koha qa test tools
Comment 63 Victor Grousset/tuxayo 2018-04-09 14:34:16 UTC
Created attachment 73873 [details] [review]
Bug 12310 : Decimal separator issues: fixes in fines

The main goal is to prevent
- x100 or /100 of the amounts
- decimal part truncation

Fixes Bug 19521 - Partial fine payment is not working with comma decimal separator

Bonuses:
- Gets rid of the spaces, currency symbols, names (EUR)
  There is nothing specific about currency symbols is this patch.
  It just removes everything that is not a digit a minus or a separator.

It handles:
- by default comma and dot as decimal separators on the same instance
- dot or comma as thousands separators if choose to use only one of them
    as decimal separator. See DecimalSeparators syspref.
- minuses, doesn't remove them
- non-decimal values: it doesn't touch them

== Test plan ==
1. Use a translation of the staff interface if possible.
2. When you pay an existing fine (more details in the next steps if needed).
     Check that you can't enter any string except like "XX.XX" (examples in
     Annex 1) without having an issue after validation. (x100,
     truncation, etc)
3. Apply this patch
4. run ~/src/installer/data/mysql/updatedatabase.pl
5. In the manual invoice (cgi-bin/koha/members/maninvoice.pl)
6. Apply the annex 1 to the "Amount" field
7. When paying a fine (cgi-bin/koha/members/paycollect.pl)
8. Click on "Pay amount" to go to the page to pay all fines
9. Apply the annex 1 to the "Collect from patron" field
10. Click on "Pay" to go to the page to pay fine
11. Apply the annex 1 to the "Collect from patron" field
12. Go to "Create manual credit" (staff://cgi-bin/koha/members/mancredit.pl)
13. Apply the annex 1 to the "Amount" field
14. If you have ideas of stuff to try to find *regressions* from the previous
    versions, then try. You might spare librarian tears by finding something
    before it's released. (and prevent me from having bounty on my head)
15. Express your happiness. This is important, otherwise the sign off spell won't
    work! The details of expressing your happiness depend on your culture and
    habits.

=== Annex 1: Subpart for each concerned page ===
1. Set DecimalSeparators syspref to ",."
2. Input "12,34"
3. Press the "Tab/Tabulation" key. It will unfocus the field and trigger the
     conversion. Validating the form does the same but it's a trick to test more
     quickly by not switching to another page.
4. The value should have been converted to 12.34
5. What follows is just a list of examples. They don't have to be tried on each
     field.
6. Do the same with "12,34 EUR" and ensure that it's converted to "12,34"
7. Same with "EUR 12.34" -> "12.34"
8. "EUR 12.34€hi lyonthree" -> "12.34"
9. "123 456,7" -> "123456.7"
10. "2" -> "2"
11. "2,0" -> "2.0"
12. "-1'345.95" -> "-1345.95"
13. ",1234" -> ".1234"
14. Set DecimalSeparators syspref to ","
15. Reload the page with the form
16. "1.000" -> "1000"
17. "1.000.000,10" -> "1000000.10"
18. Set DecimalSeparators syspref to "."
19. Reload the page with the form
20. "1,000" -> "1000"
21. "1,000,000.10" -> "1000000.10"
22. Validate the form and check that the final value with the dot was
      well handled.
23. Think about Koalas and smile. 🐨🐨🐨
Comment 64 Victor Grousset/tuxayo 2018-04-09 14:34:23 UTC
Created attachment 73874 [details] [review]
Bug 12310 : Follow up to keep the example text in fines

This puts the example text back in with the correct decimal
separator. The example uses the first char of the syspref.

TEST PLAN
---------
1) set the I18N/L10N DecimalSeparators syspref to ","
2) go to a patron's fines tab.
3) open Create manual invoice and Create manual credit tabs
4) check both tabs
   -- Example: 5.00 appears
5) apply this patch
6) reload the page and check both create tabs.
   -- Example: 5,12 appears
7) set the I18N/L10N DecimalSeparators syspref to ".,"
8) go back and check both tabs.
   -- Example: 5.12 appears (only the first separator is picked)
9) optionnal: run koha qa test tools
Comment 65 M. Tompsett 2018-04-09 17:23:28 UTC
Created attachment 73880 [details] [review]
Bug 12310: Follow up to prevent updatedatabase noise

Added IGNORE into the SQL statement.
TEST PLAN
---------
run the atomic update multiple times.
-- noise
apply patch
run the atomic update multiple times.
-- no noise
Comment 66 M. Tompsett 2018-04-09 17:31:03 UTC
Comment on attachment 73873 [details] [review]
Bug 12310 : Decimal separator issues: fixes in fines

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

::: koha-tmpl/intranet-tmpl/prog/en/includes/decimal-input-js.inc
@@ +19,5 @@
> +        // remove everything except digits, minuses, and the chars in DecimalSeparators syspref
> +        var formatted_decimal = formatted_decimal
> +                              .replace(/[^0-9-[% Koha.Preference('DecimalSeparators') %]]/g, '');
> +        var decimal = formatted_decimal
> +                    .replace(/[[% Koha.Preference('DecimalSeparators') %]]/g, '.');

Sorry, but this doesn't quite work.
5,123.12 -> 5.123.12

The decimal point is likely the LAST one that matches the DecimalSeparators. While everything else is either noise or thousands separators.
Comment 67 M. Tompsett 2018-04-09 17:33:18 UTC
Using DecimalSeparator as ',.', we should be able to enter #,###,###.## and get #######.## back.
Comment 68 Victor Grousset/tuxayo 2018-04-12 09:25:32 UTC
(In reply to M. Tompsett from comment #67)
> Using DecimalSeparator as ',.', we should be able to enter #,###,###.## and
> get #######.## back.

There is a contradiction between DecimalSeparator=',.' and wishing to input #,###,###.## and get #######.## back.

The current aimed functional behavior is to only accept thousands separators when the DecimalSeparator syspref does not contain a thousands separator in it.
Because having a character that depending on the context can be either a decimal sep or a thousands sep could easily create regressions in forgotten edge cases.
And it will silently accept user mistakes leading to erroneous values being processed. (e.g. input 5,123.12 instead of 5.123.12 or 5,123,12 when wanting to input 512312)

We might want to try to be even more clever and flexible. Actually, handling your case could be a nice addition. But if we do, can it be in a follow up bug? ^^"
Comment 69 Victor Grousset/tuxayo 2018-04-19 08:19:51 UTC
Switching to "Needs Signoff" as the above topic doesn't prevent the current code and behavior to be tested. (and moving even further unless there is more feedback about the above topic)
Comment 70 Séverine Queune 2018-04-19 09:31:27 UTC
Created attachment 74524 [details] [review]
Bug 12310 : Decimal separator issues: fixes in fines

The main goal is to prevent
- x100 or /100 of the amounts
- decimal part truncation

Fixes Bug 19521 - Partial fine payment is not working with comma decimal separator

Bonuses:
- Gets rid of the spaces, currency symbols, names (EUR)
  There is nothing specific about currency symbols is this patch.
  It just removes everything that is not a digit a minus or a separator.

It handles:
- by default comma and dot as decimal separators on the same instance
- dot or comma as thousands separators if choose to use only one of them
    as decimal separator. See DecimalSeparators syspref.
- minuses, doesn't remove them
- non-decimal values: it doesn't touch them

== Test plan ==
1. Use a translation of the staff interface if possible.
2. When you pay an existing fine (more details in the next steps if needed).
     Check that you can't enter any string except like "XX.XX" (examples in
     Annex 1) without having an issue after validation. (x100,
     truncation, etc)
3. Apply this patch
4. run ~/src/installer/data/mysql/updatedatabase.pl
5. In the manual invoice (cgi-bin/koha/members/maninvoice.pl)
6. Apply the annex 1 to the "Amount" field
7. When paying a fine (cgi-bin/koha/members/paycollect.pl)
8. Click on "Pay amount" to go to the page to pay all fines
9. Apply the annex 1 to the "Collect from patron" field
10. Click on "Pay" to go to the page to pay fine
11. Apply the annex 1 to the "Collect from patron" field
12. Go to "Create manual credit" (staff://cgi-bin/koha/members/mancredit.pl)
13. Apply the annex 1 to the "Amount" field
14. If you have ideas of stuff to try to find *regressions* from the previous
    versions, then try. You might spare librarian tears by finding something
    before it's released. (and prevent me from having bounty on my head)
15. Express your happiness. This is important, otherwise the sign off spell won't
    work! The details of expressing your happiness depend on your culture and
    habits.

=== Annex 1: Subpart for each concerned page ===
1. Set DecimalSeparators syspref to ",."
2. Input "12,34"
3. Press the "Tab/Tabulation" key. It will unfocus the field and trigger the
     conversion. Validating the form does the same but it's a trick to test more
     quickly by not switching to another page.
4. The value should have been converted to 12.34
5. What follows is just a list of examples. They don't have to be tried on each
     field.
6. Do the same with "12,34 EUR" and ensure that it's converted to "12,34"
7. Same with "EUR 12.34" -> "12.34"
8. "EUR 12.34€hi lyonthree" -> "12.34"
9. "123 456,7" -> "123456.7"
10. "2" -> "2"
11. "2,0" -> "2.0"
12. "-1'345.95" -> "-1345.95"
13. ",1234" -> ".1234"
14. Set DecimalSeparators syspref to ","
15. Reload the page with the form
16. "1.000" -> "1000"
17. "1.000.000,10" -> "1000000.10"
18. Set DecimalSeparators syspref to "."
19. Reload the page with the form
20. "1,000" -> "1000"
21. "1,000,000.10" -> "1000000.10"
22. Validate the form and check that the final value with the dot was
      well handled.
23. Think about Koalas and smile. 🐨🐨🐨

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 71 Séverine Queune 2018-04-19 09:31:32 UTC
Created attachment 74525 [details] [review]
Bug 12310 : Follow up to keep the example text in fines

This puts the example text back in with the correct decimal
separator. The example uses the first char of the syspref.

TEST PLAN
---------
1) set the I18N/L10N DecimalSeparators syspref to ","
2) go to a patron's fines tab.
3) open Create manual invoice and Create manual credit tabs
4) check both tabs
   -- Example: 5.00 appears
5) apply this patch
6) reload the page and check both create tabs.
   -- Example: 5,12 appears
7) set the I18N/L10N DecimalSeparators syspref to ".,"
8) go back and check both tabs.
   -- Example: 5.12 appears (only the first separator is picked)
9) optionnal: run koha qa test tools

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 72 Séverine Queune 2018-04-19 09:31:36 UTC
Created attachment 74526 [details] [review]
Bug 12310: Follow up to prevent updatedatabase noise

Added IGNORE into the SQL statement.
TEST PLAN
---------
run the atomic update multiple times.
-- noise
apply patch
run the atomic update multiple times.
-- no noise

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 73 Séverine Queune 2018-04-19 09:32:24 UTC
Created attachment 74527 [details] [review]
Bug 12310 : Decimal separator issues: fixes in fines

The main goal is to prevent
- x100 or /100 of the amounts
- decimal part truncation

Fixes Bug 19521 - Partial fine payment is not working with comma decimal separator

Bonuses:
- Gets rid of the spaces, currency symbols, names (EUR)
  There is nothing specific about currency symbols is this patch.
  It just removes everything that is not a digit a minus or a separator.

It handles:
- by default comma and dot as decimal separators on the same instance
- dot or comma as thousands separators if choose to use only one of them
    as decimal separator. See DecimalSeparators syspref.
- minuses, doesn't remove them
- non-decimal values: it doesn't touch them

== Test plan ==
1. Use a translation of the staff interface if possible.
2. When you pay an existing fine (more details in the next steps if needed).
     Check that you can't enter any string except like "XX.XX" (examples in
     Annex 1) without having an issue after validation. (x100,
     truncation, etc)
3. Apply this patch
4. run ~/src/installer/data/mysql/updatedatabase.pl
5. In the manual invoice (cgi-bin/koha/members/maninvoice.pl)
6. Apply the annex 1 to the "Amount" field
7. When paying a fine (cgi-bin/koha/members/paycollect.pl)
8. Click on "Pay amount" to go to the page to pay all fines
9. Apply the annex 1 to the "Collect from patron" field
10. Click on "Pay" to go to the page to pay fine
11. Apply the annex 1 to the "Collect from patron" field
12. Go to "Create manual credit" (staff://cgi-bin/koha/members/mancredit.pl)
13. Apply the annex 1 to the "Amount" field
14. If you have ideas of stuff to try to find *regressions* from the previous
    versions, then try. You might spare librarian tears by finding something
    before it's released. (and prevent me from having bounty on my head)
15. Express your happiness. This is important, otherwise the sign off spell won't
    work! The details of expressing your happiness depend on your culture and
    habits.

=== Annex 1: Subpart for each concerned page ===
1. Set DecimalSeparators syspref to ",."
2. Input "12,34"
3. Press the "Tab/Tabulation" key. It will unfocus the field and trigger the
     conversion. Validating the form does the same but it's a trick to test more
     quickly by not switching to another page.
4. The value should have been converted to 12.34
5. What follows is just a list of examples. They don't have to be tried on each
     field.
6. Do the same with "12,34 EUR" and ensure that it's converted to "12,34"
7. Same with "EUR 12.34" -> "12.34"
8. "EUR 12.34€hi lyonthree" -> "12.34"
9. "123 456,7" -> "123456.7"
10. "2" -> "2"
11. "2,0" -> "2.0"
12. "-1'345.95" -> "-1345.95"
13. ",1234" -> ".1234"
14. Set DecimalSeparators syspref to ","
15. Reload the page with the form
16. "1.000" -> "1000"
17. "1.000.000,10" -> "1000000.10"
18. Set DecimalSeparators syspref to "."
19. Reload the page with the form
20. "1,000" -> "1000"
21. "1,000,000.10" -> "1000000.10"
22. Validate the form and check that the final value with the dot was
      well handled.
23. Think about Koalas and smile. 🐨🐨🐨

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 74 Séverine Queune 2018-04-19 09:32:29 UTC
Created attachment 74528 [details] [review]
Bug 12310 : Follow up to keep the example text in fines

This puts the example text back in with the correct decimal
separator. The example uses the first char of the syspref.

TEST PLAN
---------
1) set the I18N/L10N DecimalSeparators syspref to ","
2) go to a patron's fines tab.
3) open Create manual invoice and Create manual credit tabs
4) check both tabs
   -- Example: 5.00 appears
5) apply this patch
6) reload the page and check both create tabs.
   -- Example: 5,12 appears
7) set the I18N/L10N DecimalSeparators syspref to ".,"
8) go back and check both tabs.
   -- Example: 5.12 appears (only the first separator is picked)
9) optionnal: run koha qa test tools

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 75 Séverine Queune 2018-04-19 09:32:33 UTC
Created attachment 74529 [details] [review]
Bug 12310: Follow up to prevent updatedatabase noise

Added IGNORE into the SQL statement.
TEST PLAN
---------
run the atomic update multiple times.
-- noise
apply patch
run the atomic update multiple times.
-- no noise

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 76 Victor Grousset/tuxayo 2018-04-20 09:37:30 UTC
As it's a sensitive issue, more testing would be better to ensure that there are no regressions.
Comment 77 Koha Team Lyon 3 2018-05-04 13:05:17 UTC
Hello,
I have followed the test plan (really enthousastic one ;) )and tried different inputs (with dot, comma, symbol, names) in fines patrons tab : maninvoice.pl, mancredit.pl, paycollect.pl)
I test it with different setups in DecimalSeparators and the results were consistent.
I found only one issue, perhaps related to bug19521.
With DecimalSeparators=',.', when you are on paycollect.pl and choose to writeoff a fine, if you enter "1,50", the writeoff amount saved is only 1.
The problem occurs only with the writeoff

I think that we should change the name of this ticket as it handles only the members payments. 


Sonia
Comment 78 Victor Grousset/tuxayo 2018-05-07 12:18:24 UTC
(In reply to Koha Team Lyon 3 from comment #77)
> With DecimalSeparators=',.', when you are on paycollect.pl and choose to
> writeoff a fine, if you enter "1,50", the writeoff amount saved is only 1.
> The problem occurs only with the writeoff

Confirmed. Thanks for catching this. Follow up patch in progress


> I think that we should change the name of this ticket as it handles only the
> members payments.

Indeed.
Comment 79 Victor Grousset/tuxayo 2018-05-07 13:22:21 UTC
Created attachment 75123 [details] [review]
Bug 12310: Fix decimal separators issues in fines: writeoff amount field

This is a follow up for this forgotten field.
See main patch more details.

== Test plan ==
1. Use a translation of the staff interface if possible.
2. Go to a patrons's page → fines → pay fines
3. Click on "Write off"
    This assumes an existing debt. Which should exists if you just
    tested the previous patches.
4. If you enter "1,50", the writeoff amount saved is only 1.
     This is the bug.
5. Apply this patch
6. run ~/src/installer/data/mysql/updatedatabase.pl
7. Refresh the page
8. Retry the write off.
     Apply the annex 1 to the "Writeoff amount" field
9. It works! \o/

=== Annex 1: Subpart for each concerned page ===
1. Set DecimalSeparators syspref to ",."
2. Input "12,34"
3. Press the "Tab/Tabulation" key. It will unfocus the field and trigger the
     conversion. Validating the form does the same but it's a trick to test more
     quickly by not switching to another page.
4. The value should have been converted to 12.34
5. Do the same with "12,34 EUR" and ensure that it's converted to "12,34"
   Same with "EUR 12.34" -> "12.34"
6. "EUR 12.34€thanks lyonthree" -> "12.34"
7. "123 456,7" -> "123456.7"
8.. "2" -> "2"
9.. "2,0" -> "2.0"
10. "-1'345.95" -> "-1345.95"
11. ",1234" -> ".1234"
12. Set DecimalSeparators syspref to ","
13. Reload the page with the form
14. "1.000" -> "1000"
15. "1.000.000,10" -> "1000000.10"
16. Set DecimalSeparators syspref to "."
17. Reload the page with the form
18. "1,000" -> "1000"
19. "1,000,000.10" -> "1000000.10"
20. Validate the form and check that the final value with the dot was
21    well handled.
22. Think about Koalas and smile. 🐨🐨🐨
Comment 80 sandboxes@biblibre.com 2018-05-07 14:51:12 UTC
Patch tested with a sandbox, by Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Comment 81 sandboxes@biblibre.com 2018-05-07 14:52:12 UTC
Created attachment 75127 [details] [review]
Bug 12310 : Decimal separator issues: fixes in fines

The main goal is to prevent
- x100 or /100 of the amounts
- decimal part truncation

Fixes Bug 19521 - Partial fine payment is not working with comma decimal separator

Bonuses:
- Gets rid of the spaces, currency symbols, names (EUR)
  There is nothing specific about currency symbols is this patch.
  It just removes everything that is not a digit a minus or a separator.

It handles:
- by default comma and dot as decimal separators on the same instance
- dot or comma as thousands separators if choose to use only one of them
    as decimal separator. See DecimalSeparators syspref.
- minuses, doesn't remove them
- non-decimal values: it doesn't touch them

== Test plan ==
1. Use a translation of the staff interface if possible.
2. When you pay an existing fine (more details in the next steps if needed).
     Check that you can't enter any string except like "XX.XX" (examples in
     Annex 1) without having an issue after validation. (x100,
     truncation, etc)
3. Apply this patch
4. run ~/src/installer/data/mysql/updatedatabase.pl
5. In the manual invoice (cgi-bin/koha/members/maninvoice.pl)
6. Apply the annex 1 to the "Amount" field
7. When paying a fine (cgi-bin/koha/members/paycollect.pl)
8. Click on "Pay amount" to go to the page to pay all fines
9. Apply the annex 1 to the "Collect from patron" field
10. Click on "Pay" to go to the page to pay fine
11. Apply the annex 1 to the "Collect from patron" field
12. Go to "Create manual credit" (staff://cgi-bin/koha/members/mancredit.pl)
13. Apply the annex 1 to the "Amount" field
14. If you have ideas of stuff to try to find *regressions* from the previous
    versions, then try. You might spare librarian tears by finding something
    before it's released. (and prevent me from having bounty on my head)
15. Express your happiness. This is important, otherwise the sign off spell won't
    work! The details of expressing your happiness depend on your culture and
    habits.

=== Annex 1: Subpart for each concerned page ===
1. Set DecimalSeparators syspref to ",."
2. Input "12,34"
3. Press the "Tab/Tabulation" key. It will unfocus the field and trigger the
     conversion. Validating the form does the same but it's a trick to test more
     quickly by not switching to another page.
4. The value should have been converted to 12.34
5. What follows is just a list of examples. They don't have to be tried on each
     field.
6. Do the same with "12,34 EUR" and ensure that it's converted to "12,34"
7. Same with "EUR 12.34" -> "12.34"
8. "EUR 12.34€hi lyonthree" -> "12.34"
9. "123 456,7" -> "123456.7"
10. "2" -> "2"
11. "2,0" -> "2.0"
12. "-1'345.95" -> "-1345.95"
13. ",1234" -> ".1234"
14. Set DecimalSeparators syspref to ","
15. Reload the page with the form
16. "1.000" -> "1000"
17. "1.000.000,10" -> "1000000.10"
18. Set DecimalSeparators syspref to "."
19. Reload the page with the form
20. "1,000" -> "1000"
21. "1,000,000.10" -> "1000000.10"
22. Validate the form and check that the final value with the dot was
      well handled.
23. Think about Koalas and smile. 🐨🐨🐨

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Comment 82 sandboxes@biblibre.com 2018-05-07 14:52:17 UTC
Created attachment 75128 [details] [review]
Bug 12310 : Follow up to keep the example text in fines

This puts the example text back in with the correct decimal
separator. The example uses the first char of the syspref.

TEST PLAN
---------
1) set the I18N/L10N DecimalSeparators syspref to ","
2) go to a patron's fines tab.
3) open Create manual invoice and Create manual credit tabs
4) check both tabs
   -- Example: 5.00 appears
5) apply this patch
6) reload the page and check both create tabs.
   -- Example: 5,12 appears
7) set the I18N/L10N DecimalSeparators syspref to ".,"
8) go back and check both tabs.
   -- Example: 5.12 appears (only the first separator is picked)
9) optionnal: run koha qa test tools

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Comment 83 sandboxes@biblibre.com 2018-05-07 14:52:22 UTC
Created attachment 75129 [details] [review]
Bug 12310: Follow up to prevent updatedatabase noise

Added IGNORE into the SQL statement.
TEST PLAN
---------
run the atomic update multiple times.
-- noise
apply patch
run the atomic update multiple times.
-- no noise

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Comment 84 sandboxes@biblibre.com 2018-05-07 14:52:27 UTC
Created attachment 75130 [details] [review]
Bug 12310: Fix decimal separators issues in fines: writeoff amount field

This is a follow up for this forgotten field.
See main patch more details.

== Test plan ==
1. Use a translation of the staff interface if possible.
2. Go to a patrons's page → fines → pay fines
3. Click on "Write off"
    This assumes an existing debt. Which should exists if you just
    tested the previous patches.
4. If you enter "1,50", the writeoff amount saved is only 1.
     This is the bug.
5. Apply this patch
6. run ~/src/installer/data/mysql/updatedatabase.pl
7. Refresh the page
8. Retry the write off.
     Apply the annex 1 to the "Writeoff amount" field
9. It works! \o/

=== Annex 1: Subpart for each concerned page ===
1. Set DecimalSeparators syspref to ",."
2. Input "12,34"
3. Press the "Tab/Tabulation" key. It will unfocus the field and trigger the
     conversion. Validating the form does the same but it's a trick to test more
     quickly by not switching to another page.
4. The value should have been converted to 12.34
5. Do the same with "12,34 EUR" and ensure that it's converted to "12,34"
   Same with "EUR 12.34" -> "12.34"
6. "EUR 12.34€thanks lyonthree" -> "12.34"
7. "123 456,7" -> "123456.7"
8.. "2" -> "2"
9.. "2,0" -> "2.0"
10. "-1'345.95" -> "-1345.95"
11. ",1234" -> ".1234"
12. Set DecimalSeparators syspref to ","
13. Reload the page with the form
14. "1.000" -> "1000"
15. "1.000.000,10" -> "1000000.10"
16. Set DecimalSeparators syspref to "."
17. Reload the page with the form
18. "1,000" -> "1000"
19. "1,000,000.10" -> "1000000.10"
20. Validate the form and check that the final value with the dot was
21    well handled.
22. Think about Koalas and smile. 🐨🐨🐨

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Comment 85 Koha Team Lyon 3 2018-05-07 15:23:44 UTC
Hello,
tested it again with different inputs (with dot, comma, symbol, names) in fines patrons tab  (maninvoice.pl, mancredit.pl, paycollect.pl) and it works, even for Write off :)
So for me it's OK.

I have just a little remark for a tricky case :
If you choose DecimalSeparators = , and in addition, you choose CurrencyFormat = 360 000,00 (FR). I know that there's no direct relation with this patch, but it's to explain the user experience : user can see comma on amount on "account" and "pay fines" tabs.
When you choose to pay or to write off a fine, you arrive on paycollect.pl : there, the amounts are displayed with a dot. If you don't change anything, the dot is considered as a decimal separator. If you want to change the amount without changing the separator which is already displayed, it will not be consider. 
for example :
'500.00' is displayed
You change it on '1.00'
It will be transform in '100'

It's consistent with your DecimalSeparators syspref but quite confusing.

At least, it would be great to have on paycollect the same help we can find mancredit ou maninvoice (Example: 5,12).
By the way, this help only figures the first character of the DecimalSeparators syspref which is not completely right :
DecimalSeparators = .,
You will see : "Example: 5.12"

DecimalSeparators = ,.
You will see : "Example: 5,12"

But as it is, the patches handle the main issue, so I hope it could be in the 18.05 release !

Sonia
Comment 86 Marcel de Rooy 2018-05-11 09:13:23 UTC
Reading: "So thousands separators come at the expense of handling both dots and commas."

If I understand correctly, you do not know which separator has which function? I would probably prefer to assign both roles in separate preferences to resolve the ambiguity. Now we still do not know what means 3,512 ? Is it three thousand or three ?
Comment 87 Victor Grousset/tuxayo 2018-05-11 17:37:36 UTC
(In reply to Koha Team Lyon 3 from comment #85)
> When you choose to pay or to write off a fine, you arrive on paycollect.pl :
> there, the amounts are displayed with a dot. 

Indeed that's not very consistent and confusing. We could hide the processed value in a hidden field but that might not be clean and could lead to other issues. (in follow-up BZ I hope)

> If you don't change anything,
> the dot is considered as a decimal separator. If you want to change the
> amount without changing the separator which is already displayed, it will
> not be consider. 
> for example :
> '500.00' is displayed
> You change it on '1.00'
> It will be transform in '100'

That's not great :(

> It's consistent with your DecimalSeparators syspref but quite confusing.
> 
> At least, it would be great to have on paycollect the same help we can find
> mancredit ou maninvoice (Example: 5,12).
> By the way, this help only figures the first character of the
> DecimalSeparators syspref which is not completely right :
> DecimalSeparators = .,
> You will see : "Example: 5.12"
> 
> DecimalSeparators = ,.
> You will see : "Example: 5,12"

That's still a valid example, which is the point of the help.
But I agree get that there is something not optimal there.

> 
> But as it is, the patches handle the main issue, 

+1

> so I hope it could be in
> the 18.05 release !

We're too, late. At least, that leaves time to find remaining bugs or submit follow ups.
Comment 88 Victor Grousset/tuxayo 2018-05-11 18:00:33 UTC
(In reply to Marcel de Rooy from comment #86)
> Reading: "So thousands separators come at the expense of handling both dots
> and commas."
> 
> If I understand correctly, you do not know which separator has which
> function? 

That means that in the case of wanting to use thousands separators;
one has to renounce having , AND . as decimal seps.
The one that is elected as the thousands sep can't be in the DecimalSeparators syspref.

> I would probably prefer to assign both roles in separate
> preferences to resolve the ambiguity. Now we still do not know what means
> 3,512 ? Is it three thousand or three ?

Does the above explanation addresses all of your concerns?

Is there a way to better phrase the syspref text?
Comment 89 Marcel de Rooy 2018-05-14 07:31:37 UTC
(In reply to Victor Grousset/tuxayo from comment #88)
> (In reply to Marcel de Rooy from comment #86)
> > Reading: "So thousands separators come at the expense of handling both dots
> > and commas."
> > 
> > If I understand correctly, you do not know which separator has which
> > function? 
> 
> That means that in the case of wanting to use thousands separators;
> one has to renounce having , AND . as decimal seps.
> The one that is elected as the thousands sep can't be in the
> DecimalSeparators syspref.

Sounds better to me. Do you need more feedback? From the list ?

> > I would probably prefer to assign both roles in separate
> > preferences to resolve the ambiguity. Now we still do not know what means
> > 3,512 ? Is it three thousand or three ?
> 
> Does the above explanation addresses all of your concerns?

Can't say that yet.

> Is there a way to better phrase the syspref text?

Depends on the choices we will make. Do you add another pref for thousand sep?
Comment 90 Victor Grousset/tuxayo 2018-05-14 08:17:46 UTC
(In reply to Marcel de Rooy from comment #89)
> (In reply to Victor Grousset/tuxayo from comment #88)
> > That means that in the case of wanting to use thousands separators;
> > one has to renounce having , AND . as decimal seps.
> > The one that is elected as the thousands sep can't be in the
> > DecimalSeparators syspref.
> 
> Sounds better to me. Do you need more feedback? From the list ?

Hopefully not, because on the IRC and mainly during the hackfest, I have collected a lot of feedback.
Which caused the proposed solution to change more than 5 times.
I already bothered a lot of people (from france, germany and the US) to ensure that there won't be librarian tears if this solution goes into production. ^^
Each simpler solution raised issues about existing use cases that work now or worked before the ~16.11 regression.


> > > I would probably prefer to assign both roles in separate
> > > preferences to resolve the ambiguity. Now we still do not know what means
> > > 3,512 ? Is it three thousand or three ?
> > 
> > Does the above explanation addresses all of your concerns?
> 
> Can't say that yet.
> 
> > Is there a way to better phrase the syspref text?
> 
> Depends on the choices we will make. Do you add another pref for thousand
> sep?

No need for it.

«Gets rid of the spaces, currency symbols, names (EUR)
There is nothing specific about currency symbols is this patch.
It just removes everything that is not a digit a minus or a separator.»

So one can use anything as a thousands sep. As long as it's not a digit a minus or in the DecimalSeparators syspref.
Comment 91 Marcel de Rooy 2018-05-14 08:32:23 UTC
(In reply to Victor Grousset/tuxayo from comment #90)
> Hopefully not, because on the IRC and mainly during the hackfest, I have
> collected a lot of feedback.
> Which caused the proposed solution to change more than 5 times.
> I already bothered a lot of people (from france, germany and the US) to
> ensure that there won't be librarian tears if this solution goes into
> production. ^^
> Each simpler solution raised issues about existing use cases that work now
> or worked before the ~16.11 regression.

No simple solutions ;)

> So one can use anything as a thousands sep. As long as it's not a digit a
> minus or in the DecimalSeparators syspref.

In our country the . is the thousands separator and the , is the decimal sep. (Just opposite to Anglo-Saxon style.)
No problem for my library btw.
Comment 92 Victor Grousset/tuxayo 2018-05-14 09:09:40 UTC
(In reply to Marcel de Rooy from comment #91)
> > So one can use anything as a thousands sep. As long as it's not a digit a
> > minus or in the DecimalSeparators syspref.
> 
> In our country the . is the thousands separator and the , is the decimal
> sep. (Just opposite to Anglo-Saxon style.)

So if you want to use the . as the thousands separator then you will have to put only , in the DecimalSeparators syspref.

> No problem for my library btw.

Great :) Are all your potentially blocking concerns addressed?
(having discussions not closed could cause a QAer to hesitate)
Comment 93 Marcel de Rooy 2018-05-14 09:46:32 UTC
(In reply to Victor Grousset/tuxayo from comment #92)
> (In reply to Marcel de Rooy from comment #91)
> > > So one can use anything as a thousands sep. As long as it's not a digit a
> > > minus or in the DecimalSeparators syspref.
> > 
> > In our country the . is the thousands separator and the , is the decimal
> > sep. (Just opposite to Anglo-Saxon style.)
> 
> So if you want to use the . as the thousands separator then you will have to
> put only , in the DecimalSeparators syspref.
> 
> > No problem for my library btw.
> 
> Great :) Are all your potentially blocking concerns addressed?
> (having discussions not closed could cause a QAer to hesitate)

I could be the next QAer.. (Only glanced thru so far.)
The sentence I referred to earlier "at the expense" is somewhat unclear and could be improved. Not a blocking concern.
The fact that the default value for the pref contains a thousands separator for many people is a bit confusing too. But I understand that it is the end of a long discussion.
Comment 94 Victor Grousset/tuxayo 2018-05-14 13:01:59 UTC
(In reply to Marcel de Rooy from comment #93)
> I could be the next QAer.. (Only glanced thru so far.)

:)

> The sentence I referred to earlier "at the expense" is somewhat unclear and
> could be improved. Not a blocking concern.

If anyone reading this has an idea, feel free to post a comment.

> The fact that the default value for the pref contains a thousands separator
> for many people is a bit confusing too. But I understand that it is the end
> of a long discussion.

It seems that any value will contain a thousands separator for many people.
https://en.wikipedia.org/wiki/Decimal_separator#Digit_grouping

The idea was that the default value would fit the basic use case (no thousands seps) for almost everyone (, and . users)
https://en.wikipedia.org/wiki/Decimal_separator#Hindu%E2%80%93Arabic_numeral_system
without requiring any configuration choice when migrating on the 18.11 for example. So system librarians can miss this change in the release notes without causing the very same chaos that this patch wants to fix. [1]
Which hopefully will remove a obstacle for this path to be backportable for example.


[1] And with drones becoming a commodity in a lot of countries, the cost for angry users to hire a hitman — using "modern" techniques that up to now were reserved to the USA — is constantly lowering. That's why I try to choose defaults that will keep people happy ^^"
</troll>
Comment 95 Marcel de Rooy 2018-06-01 10:19:14 UTC
Preliminary QA:

Very brave to take this upon you !

Tiny suggestion:
.decimal-unformating
Fix spelling please. At least t => tt but what is decimal unformat btw? Better name? Something with normalize or so??

I have the feeling that DecimalSeparators and CurrencyFormat don't really harmonize. And yes, I can understand how problematic making changes in this area must be..

So thousands separators come at the expense of handling both dots and commas.
Still opt for something that is more understandable.

The number of changes to be expected here would be much larger. You actually use the new js formatting in three acq templates.
What about things like:
C4/Biblio.pm:                $subfield_value =~ s/\./,/ if C4::Context->preference("CurrencyFormat") eq "FR";
acqui/addorderiso2709.pl:                        $price =~ s/\./,/ if C4::Context->preference("CurrencyFormat") eq "FR";
acqui/addorderiso2709.pl:                $price =~ s/\./,/ if C4::Context->preference("CurrencyFormat") eq "FR";
Koha/Number/Price.pm
    $format_params = {
        %$format_params,
        int_curr_symbol   => '',
        mon_thousands_sep => '',
        mon_decimal_point => '.',
    };
koha-tmpl/intranet-tmpl/prog/en/includes/format_price.inc
    [% IF Koha.Preference("CurrencyFormat") == 'FR' %]
        var default_value = {
            thousands_sep: ' ',
            decimal_point: ',',
            decimal_digits: 2
        };
And this is just to name a few; the list could probably be larger when searching harder.
It might be impossible to make global changes but the risk of this is adding just another approach in a few places and leaving Koha even more fragmented on the subject?

Changing state for need of feedback or adjustments.
Comment 96 Victor Grousset/tuxayo 2018-06-01 16:22:35 UTC
(In reply to Marcel de Rooy from comment #95)
> Very brave to take this upon you !

Thanks Marcel :)

> Tiny suggestion:
> .decimal-unformating
> Fix spelling please. At least t => tt

Indeed >< thanks for catching this. QA++ :)
TODO implement

> but what is decimal unformat btw?

I followed this example:
https://metacpan.org/pod/Number::Format#unformat_number($formatted)

> Better name? Something with normalize or so??

.number-normalizing 
.number-normalizer
.number-normalize
.decimal-normalizing 
.decimal-normalizer
.decimal-normalize

Which one would be good enough?
TODO wait answer, blocking question

> I have the feeling that DecimalSeparators and CurrencyFormat don't really
> harmonize. And yes, I can understand how problematic making changes in this
> area must be..
> 
> So thousands separators come at the expense of handling both dots and commas.
> Still opt for something that is more understandable.

I'm not sure to understand. Is the the current state the patch on this topic acceptable?

> The number of changes to be expected here would be much larger. You actually
> use the new js formatting in three acq templates.
> What about things like:
> [...]
> And this is just to name a few; the list could probably be larger when
> searching harder.
> It might be impossible to make global changes but the risk of this is adding
> just another approach in a few places and leaving Koha even more fragmented
> on the subject?

I split the patch because the test plan was way too long.
http://irc.koha-community.org/koha/2018-03-30#i_2024887
http://irc.koha-community.org/koha/2018-03-30#i_2024900

When this bug and bug 18723[1] will be in master there won't be more fragmentation than today.
And less actually.

locally: acquisitions and payments/fines will be internally less fragmented.
  as there are currently different behaviors in *successive pages* [2]
globally: the new approach replaces an existing one (I checked that it not used elsewhere)
  see moneyFormat function.
  https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=75127&action=diff

So, are these changes a step in the right direct worth of begin integrated?
TODO wait answer, blocking question
 

[1] which will also apply these changes to aqbudgetperiods.tt  aqbudgets.tt in
addition of the pages concerned by the bug description.

[2] a big reason of why I get the most requests to fix decimal sep issues in these views
Comment 97 Marcel de Rooy 2018-06-04 06:42:58 UTC
(In reply to Victor Grousset/tuxayo from comment #96)
> I followed this example:
> https://metacpan.org/pod/Number::Format#unformat_number($formatted)
OK If you feel the name is good enough, keep it. No big deal. Just do not use formating, single t :)

> > So thousands separators come at the expense of handling both dots and commas.
> > Still opt for something that is more understandable.
> I'm not sure to understand. Is the the current state the patch on this topic
> acceptable?
Would it be clearer if you would say that you c/should not use characters from the DecimalSeparator pref also as thousands separator?


> I split the patch because the test plan was way too long.
> http://irc.koha-community.org/koha/2018-03-30#i_2024887
> http://irc.koha-community.org/koha/2018-03-30#i_2024900
> 
> When this bug and bug 18723[1] will be in master there won't be more
> fragmentation than today.
> And less actually.
> 
> locally: acquisitions and payments/fines will be internally less fragmented.
>   as there are currently different behaviors in *successive pages* [2]
> globally: the new approach replaces an existing one (I checked that it not
> used elsewhere)
>   see moneyFormat function.
>  
> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=75127&action=diff
> 
> So, are these changes a step in the right direct worth of begin integrated?
> TODO wait answer, blocking question
Well, I just hope so. But willing to follow your argument. Feel free to submit the tiny adjustments you intend to, and I will try to finish here.
Comment 98 Victor Grousset/tuxayo 2018-06-11 12:36:43 UTC Comment hidden (obsolete)
Comment 99 Victor Grousset/tuxayo 2018-06-11 12:36:48 UTC Comment hidden (obsolete)
Comment 100 Victor Grousset/tuxayo 2018-06-11 12:36:54 UTC Comment hidden (obsolete)
Comment 101 Victor Grousset/tuxayo 2018-06-11 12:36:59 UTC Comment hidden (obsolete)
Comment 102 Victor Grousset/tuxayo 2018-06-11 12:37:04 UTC Comment hidden (obsolete)
Comment 103 Victor Grousset/tuxayo 2018-06-11 12:37:09 UTC Comment hidden (obsolete)
Comment 104 Victor Grousset/tuxayo 2018-06-11 14:49:45 UTC
Created attachment 75954 [details] [review]
Bug 12310: Decimal separator issues: fixes in fines

The main goal is to prevent
- x100 or /100 of the amounts
- decimal part truncation

Fixes Bug 19521 - Partial fine payment is not working with comma decimal separator

Bonuses:
- Gets rid of the spaces, currency symbols, names (EUR)
  There is nothing specific about currency symbols is this patch.
  It just removes everything that is not a digit a minus or a separator.

It handles:
- by default comma and dot as decimal separators on the same instance
- dot or comma as thousands separators if choose to use only one of them
    as decimal separator. See DecimalSeparators syspref.
- minuses, doesn't remove them
- non-decimal values: it doesn't touch them

== Test plan ==
1. Use a translation of the staff interface if possible.
2. When you pay an existing fine (more details in the next steps if needed).
     Check that you can't enter any string except like "XX.XX" (examples in
     Annex 1) without having an issue after validation. (x100,
     truncation, etc)
3. Apply this patch
4. run ~/src/installer/data/mysql/updatedatabase.pl
5. In the manual invoice (cgi-bin/koha/members/maninvoice.pl)
6. Apply the annex 1 to the "Amount" field
7. When paying a fine (cgi-bin/koha/members/paycollect.pl)
8. Click on "Pay amount" to go to the page to pay all fines
9. Apply the annex 1 to the "Collect from patron" field
10. Click on "Pay" to go to the page to pay fine
11. Apply the annex 1 to the "Collect from patron" field
12. Go to "Create manual credit" (staff://cgi-bin/koha/members/mancredit.pl)
13. Apply the annex 1 to the "Amount" field
14. If you have ideas of stuff to try to find *regressions* from the previous
    versions, then try. You might spare librarian tears by finding something
    before it's released. (and prevent me from having bounty on my head)
15. Express your happiness. This is important, otherwise the sign off spell won't
    work! The details of expressing your happiness depend on your culture and
    habits.

=== Annex 1: Subpart for each concerned page ===
1. Set DecimalSeparators syspref to ",."
2. Input "12,34"
3. Press the "Tab/Tabulation" key. It will unfocus the field and trigger the
     conversion. Validating the form does the same but it's a trick to test more
     quickly by not switching to another page.
4. The value should have been converted to 12.34
5. What follows is just a list of examples. They don't have to be tried on each
     field.
6. Do the same with "12,34 EUR" and ensure that it's converted to "12,34"
7. Same with "EUR 12.34" -> "12.34"
8. "EUR 12.34€hi lyonthree" -> "12.34"
9. "123 456,7" -> "123456.7"
10. "2" -> "2"
11. "2,0" -> "2.0"
12. "-1'345.95" -> "-1345.95"
13. ",1234" -> ".1234"
14. Set DecimalSeparators syspref to ","
15. Reload the page with the form
16. "1.000" -> "1000"
17. "1.000.000,10" -> "1000000.10"
18. Set DecimalSeparators syspref to "."
19. Reload the page with the form
20. "1,000" -> "1000"
21. "1,000,000.10" -> "1000000.10"
22. Validate the form and check that the final value with the dot was
      well handled.
23. Think about Koalas and smile. 🐨🐨🐨

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Comment 105 Victor Grousset/tuxayo 2018-06-11 14:49:50 UTC
Created attachment 75955 [details] [review]
Bug 12310: (follow-up) Keep the example text in fines

This puts the example text back in with the correct decimal
separator. The example uses the first char of the syspref.

TEST PLAN
---------
1) set the I18N/L10N DecimalSeparators syspref to ","
2) go to a patron's fines tab.
3) open Create manual invoice and Create manual credit tabs
4) check both tabs
   -- Example: 5.00 appears
5) apply this patch
6) reload the page and check both create tabs.
   -- Example: 5,12 appears
7) set the I18N/L10N DecimalSeparators syspref to ".,"
8) go back and check both tabs.
   -- Example: 5.12 appears (only the first separator is picked)
9) optionnal: run koha qa test tools

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Comment 106 Victor Grousset/tuxayo 2018-06-11 14:49:56 UTC
Created attachment 75956 [details] [review]
Bug 12310: (QA follow-up) Prevent updatedatabase noise

Added IGNORE into the SQL statement.
TEST PLAN
---------
run the atomic update multiple times.
-- noise
apply patch
run the atomic update multiple times.
-- no noise

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Comment 107 Victor Grousset/tuxayo 2018-06-11 14:50:01 UTC
Created attachment 75957 [details] [review]
Bug 12310: (follow-up) Fix forgotten writeoff amount field

This is a follow up for this forgotten field.
See main patch more details.

== Test plan ==
1. Use a translation of the staff interface if possible.
2. Go to a patrons's page → fines → pay fines
3. Click on "Write off"
    This assumes an existing debt. Which should exists if you just
    tested the previous patches.
4. If you enter "1,50", the writeoff amount saved is only 1.
     This is the bug.
5. Apply this patch
6. run ~/src/installer/data/mysql/updatedatabase.pl
7. Refresh the page
8. Retry the write off.
     Apply the annex 1 to the "Writeoff amount" field
9. It works! \o/

=== Annex 1: Subpart for each concerned page ===
1. Set DecimalSeparators syspref to ",."
2. Input "12,34"
3. Press the "Tab/Tabulation" key. It will unfocus the field and trigger the
     conversion. Validating the form does the same but it's a trick to test more
     quickly by not switching to another page.
4. The value should have been converted to 12.34
5. Do the same with "12,34 EUR" and ensure that it's converted to "12,34"
   Same with "EUR 12.34" -> "12.34"
6. "EUR 12.34€thanks lyonthree" -> "12.34"
7. "123 456,7" -> "123456.7"
8.. "2" -> "2"
9.. "2,0" -> "2.0"
10. "-1'345.95" -> "-1345.95"
11. ",1234" -> ".1234"
12. Set DecimalSeparators syspref to ","
13. Reload the page with the form
14. "1.000" -> "1000"
15. "1.000.000,10" -> "1000000.10"
16. Set DecimalSeparators syspref to "."
17. Reload the page with the form
18. "1,000" -> "1000"
19. "1,000,000.10" -> "1000000.10"
20. Validate the form and check that the final value with the dot was
21    well handled.
22. Think about Koalas and smile. 🐨🐨🐨

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Comment 108 Victor Grousset/tuxayo 2018-06-11 14:50:08 UTC
Created attachment 75958 [details] [review]
Bug 12310: (QA follow-up) Fix typo: unformating → unformatting
Comment 109 Victor Grousset/tuxayo 2018-06-11 14:50:13 UTC Comment hidden (obsolete)
Comment 110 Victor Grousset/tuxayo 2018-06-11 16:27:11 UTC
Rebased and solved simple conflicts.

(In reply to Marcel de Rooy from comment #97)
> OK If you feel the name is good enough

As most active people here have more experience with the code and the English language, I'll happily change to any suggestion. (even if I might ask for confirmation if I feel that it's not that clear or ambiguous ^^)

> Just do not use formating, single t :)

Fixed ^^

> > > So thousands separators come at the expense of handling both dots and commas.
> > > Still opt for something that is more understandable.
> > I'm not sure to understand. Is the the current state the patch on this topic
> > acceptable?
> Would it be clearer if you would say that you c/should not use characters
> from the DecimalSeparator pref also as thousands separator?

completed: What do you think of the current version?
TODO wait answer, blocking question

Does the followups require all the fields to be retested? And put back the patch to "Needs Signoff".
TODO wait answer, blocking question

> > When this bug and bug 18723[1] will be in master there won't be more
> > fragmentation than today.
> > And less actually.
> > 
> > locally: acquisitions and payments/fines will be internally less fragmented.
> >   as there are currently different behaviors in *successive pages* [2]
> > globally: the new approach replaces an existing one (I checked that it not
> > used elsewhere)
> >   see moneyFormat function.
> >  
> > https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=75127&action=diff
> > 
> > So, are these changes a step in the right direct worth of begin integrated?
> Well, I just hope so. But willing to follow your argument. Feel free to
> submit the tiny adjustments you intend to, and I will try to finish here.

Great, I think every feedback has been addressed if I'm not mistaken :)
Thanks again for you time!
Comment 111 Marcel de Rooy 2018-07-02 13:06:21 UTC
-        - "Any of these characters can be used as a separator between integer and decimal parts in the price fields. To work well with thousands separators you must (usually) choose between dot and comma to avoid ambiguity. So thousands separators come at the expense of handling both dots and commas. It you want to format price output, please use the system preference 'CurrencyFormat'"
+        - "Any of these characters can be used as a separator between integer and decimal parts in the decimal number fields that use the associated normalizing/unformatting mechanism. To also use thousands separators you must choose between dot and comma to avoid ambiguity. So using thousands separators comes at the expense of handling both dots and commas as decimal separators. This means reserving one character for decimal separation and another for thousands separation. So you must not use characters from the DecimalSeparators preference also as thousands separator. Also, if you want to format price output, please use the system preference 'CurrencyFormat'"

The second description is imo too long. Let me try to suggest something shorter:

Any of these characters can be used as a separator between integer and decimal part of a price field. To avoid ambiguity, do not use the same characters as thousands separator. Use pref CurrencyFormat to format price ouput.

=> Instead of the cryptic "fields that use the associated normalizing/unformatting mechanism", you might add something like:

(This pref is currently used on the patron fines tab.)
Comment 112 Marcel de Rooy 2018-07-02 13:07:57 UTC
Does the followups require all the fields to be retested? And put back the patch to "Needs Signoff".
TODO wait answer, blocking question

Imo we can keep the signed off state. Please adjust the last follow-up and let me finish QA on friday(?).
Comment 113 Victor Grousset/tuxayo 2018-07-02 14:09:35 UTC
Thanks Marcel for your feedback, I hope to process it in the following days.

I just came to ask this question: Should the DecimalSeparators syspref be shared to HEA?
Comment 114 Marcel de Rooy 2018-07-03 12:04:31 UTC
(In reply to Victor Grousset/tuxayo from comment #113)
> I just came to ask this question: Should the DecimalSeparators syspref be
> shared to HEA?
Good idea
Comment 115 Victor Grousset/tuxayo 2018-07-12 17:11:10 UTC
Created attachment 76896 [details] [review]
Bug 12310: (QA follow-up) Simplify DecimalSeparators syspref description

Thanks Marcel for the new version :)
Comment 116 Victor Grousset/tuxayo 2018-07-13 07:35:07 UTC
Created attachment 76933 [details] [review]
Bug 12310: (follow-up) Add relevant sysprefs to HEA

DecimalSeparators: the new one
CurrencyFormat: related because it's the display format
Comment 117 Victor Grousset/tuxayo 2018-07-13 08:06:58 UTC
In HEA I've added the sysprefs next to other i18n ones. And added CurrencyFormat as it's somewhat linked with the one introduced by this patch.
Comment 118 Marcel de Rooy 2018-07-27 09:45:28 UTC
Resuming here now
Comment 119 Marcel de Rooy 2018-07-27 10:22:48 UTC
There is something strange here with git-bz.
When adding a follow-up to the test bug 6473, no problem.
When attaching to this report:
Traceback (most recent call last):
  File "/usr/local/bin/git-bz", line 2632, in <module>
    do_attach(*args)
  File "/usr/local/bin/git-bz", line 1998, in do_attach
    attach_commits(bug, commits, edit_comments=global_options.edit)
  File "/usr/local/bin/git-bz", line 1913, in attach_commits
    description, body, obsoletes, statuses, patch_complexities, depends = edit_attachment_comment(bug, commit.subject, body)
  File "/usr/local/bin/git-bz", line 1866, in edit_attachment_comment
    lines = edit_template(template.getvalue())
  File "/usr/local/bin/git-bz", line 811, in edit_template
    f.write(template)
UnicodeEncodeError: 'ascii' codec can't encode character u'\u2192' in position 600: ordinal not in range(128)
This is a rightwards arrow; where is it coming from ??
Comment 120 Marcel de Rooy 2018-07-27 10:23:30 UTC
Wanted to add:
diff --git a/t/db_dependent/UsageStats.t b/t/db_dependent/UsageStats.t
index f2770f7..e4704b4 100644
--- a/t/db_dependent/UsageStats.t
+++ b/t/db_dependent/UsageStats.t
@@ -588,6 +588,8 @@ sub mocking_systempreferences_to_a_set_value {
         OAI-PMH
         version
         AudioAlerts
+        CurrencyFormat
+        DecimalSeparators
         /
       ) {
         t::lib::Mocks::mock_preference( $_, $set_value );
Comment 121 Marcel de Rooy 2018-07-27 10:30:34 UTC
I am changing the status (temporarily?) to Failed QA and will ask for additional QA eyes.
The relation between CurrencyFormat and DecimalSeparators still worries me.

On paycollect CurrencyFormat formats the suggested price (using the separators in CurrencyFormat) that I will edit (using the other pref). Suppose that DecimalSeparators is not exactly identical, I will have issues here.

Should decimal separator not just be the one identified as such in CurrencyFormat?

Hypothetically, I could use # as decimal separator (it works), but on the form the prices are shown according to currency format.
Comment 122 Victor Grousset/tuxayo 2018-08-17 09:20:42 UTC Comment hidden (obsolete)
Comment 123 Victor Grousset/tuxayo 2018-08-17 09:20:48 UTC Comment hidden (obsolete)
Comment 124 Victor Grousset/tuxayo 2018-08-17 09:20:54 UTC Comment hidden (obsolete)
Comment 125 Victor Grousset/tuxayo 2018-08-17 09:26:49 UTC
(In reply to Marcel de Rooy from comment #119)
> There is something strange here with git-bz.
> When adding a follow-up to the test bug 6473, no problem.
> When attaching to this report:
> Traceback (most recent call last):
>   File "/usr/local/bin/git-bz", line 2632, in <module>
>     do_attach(*args)
>   File "/usr/local/bin/git-bz", line 1998, in do_attach
>     attach_commits(bug, commits, edit_comments=global_options.edit)
>   File "/usr/local/bin/git-bz", line 1913, in attach_commits
>     description, body, obsoletes, statuses, patch_complexities, depends =
> edit_attachment_comment(bug, commit.subject, body)
>   File "/usr/local/bin/git-bz", line 1866, in edit_attachment_comment
>     lines = edit_template(template.getvalue())
>   File "/usr/local/bin/git-bz", line 811, in edit_template
>     f.write(template)
> UnicodeEncodeError: 'ascii' codec can't encode character u'\u2192' in
> position 600: ordinal not in range(128)
> This is a rightwards arrow; where is it coming from ??

Ow, sorry, there was a rightwards arrow in the first line of the commit message.
I replaced it with "=>"

I had no issues applying the patches, are you on the apply_on_cascade branch for your git-bz installation?
Comment 126 Victor Grousset/tuxayo 2018-08-17 17:45:28 UTC Comment hidden (obsolete)
Comment 127 Victor Grousset/tuxayo 2018-08-17 17:47:16 UTC
Sorry for the inconvenience. Your patch has been attached. With you as git author if that's ok.
Comment 128 Victor Grousset/tuxayo 2018-08-20 12:53:59 UTC Comment hidden (obsolete)
Comment 129 Victor Grousset/tuxayo 2018-08-20 13:17:51 UTC
I realized that your patch was in bug 6473 so I attached it here instead of my version.

- showed the raw patch in bug 6473
- copied the text
- added an attachment here where the text was pasted and the attachment was marked as a patch
Comment 130 Martin Renvoize 2018-09-14 08:18:59 UTC
Created attachment 78631 [details] [review]
Bug 12310: Decimal separator issues: fixes in fines

The main goal is to prevent
- x100 or /100 of the amounts
- decimal part truncation

Fixes Bug 19521 - Partial fine payment is not working with comma decimal separator

Bonuses:
- Gets rid of the spaces, currency symbols, names (EUR)
  There is nothing specific about currency symbols is this patch.
  It just removes everything that is not a digit a minus or a separator.

It handles:
- by default comma and dot as decimal separators on the same instance
- dot or comma as thousands separators if choose to use only one of them
    as decimal separator. See DecimalSeparators syspref.
- minuses, doesn't remove them
- non-decimal values: it doesn't touch them

== Test plan ==
1. Use a translation of the staff interface if possible.
2. When you pay an existing fine (more details in the next steps if needed).
     Check that you can't enter any string except like "XX.XX" (examples in
     Annex 1) without having an issue after validation. (x100,
     truncation, etc)
3. Apply this patch
4. run ~/src/installer/data/mysql/updatedatabase.pl
5. In the manual invoice (cgi-bin/koha/members/maninvoice.pl)
6. Apply the annex 1 to the "Amount" field
7. When paying a fine (cgi-bin/koha/members/paycollect.pl)
8. Click on "Pay amount" to go to the page to pay all fines
9. Apply the annex 1 to the "Collect from patron" field
10. Click on "Pay" to go to the page to pay fine
11. Apply the annex 1 to the "Collect from patron" field
12. Go to "Create manual credit" (staff://cgi-bin/koha/members/mancredit.pl)
13. Apply the annex 1 to the "Amount" field
14. If you have ideas of stuff to try to find *regressions* from the previous
    versions, then try. You might spare librarian tears by finding something
    before it's released. (and prevent me from having bounty on my head)
15. Express your happiness. This is important, otherwise the sign off spell won't
    work! The details of expressing your happiness depend on your culture and
    habits.

=== Annex 1: Subpart for each concerned page ===
1. Set DecimalSeparators syspref to ",."
2. Input "12,34"
3. Press the "Tab/Tabulation" key. It will unfocus the field and trigger the
     conversion. Validating the form does the same but it's a trick to test more
     quickly by not switching to another page.
4. The value should have been converted to 12.34
5. What follows is just a list of examples. They don't have to be tried on each
     field.
6. Do the same with "12,34 EUR" and ensure that it's converted to "12,34"
7. Same with "EUR 12.34" -> "12.34"
8. "EUR 12.34€hi lyonthree" -> "12.34"
9. "123 456,7" -> "123456.7"
10. "2" -> "2"
11. "2,0" -> "2.0"
12. "-1'345.95" -> "-1345.95"
13. ",1234" -> ".1234"
14. Set DecimalSeparators syspref to ","
15. Reload the page with the form
16. "1.000" -> "1000"
17. "1.000.000,10" -> "1000000.10"
18. Set DecimalSeparators syspref to "."
19. Reload the page with the form
20. "1,000" -> "1000"
21. "1,000,000.10" -> "1000000.10"
22. Validate the form and check that the final value with the dot was
      well handled.
23. Think about Koalas and smile. 🐨🐨🐨

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Comment 131 Martin Renvoize 2018-09-14 08:19:06 UTC
Created attachment 78632 [details] [review]
Bug 12310: (follow-up) Keep the example text in fines

This puts the example text back in with the correct decimal
separator. The example uses the first char of the syspref.

TEST PLAN
---------
1) set the I18N/L10N DecimalSeparators syspref to ","
2) go to a patron's fines tab.
3) open Create manual invoice and Create manual credit tabs
4) check both tabs
   -- Example: 5.00 appears
5) apply this patch
6) reload the page and check both create tabs.
   -- Example: 5,12 appears
7) set the I18N/L10N DecimalSeparators syspref to ".,"
8) go back and check both tabs.
   -- Example: 5.12 appears (only the first separator is picked)
9) optionnal: run koha qa test tools

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Comment 132 Martin Renvoize 2018-09-14 08:19:12 UTC
Created attachment 78633 [details] [review]
Bug 12310: (QA follow-up) Prevent updatedatabase noise

Added IGNORE into the SQL statement.
TEST PLAN
---------
run the atomic update multiple times.
-- noise
apply patch
run the atomic update multiple times.
-- no noise

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Comment 133 Martin Renvoize 2018-09-14 08:19:18 UTC
Created attachment 78634 [details] [review]
Bug 12310: (follow-up) Fix forgotten writeoff amount field

This is a follow up for this forgotten field.
See main patch more details.

== Test plan ==
1. Use a translation of the staff interface if possible.
2. Go to a patrons's page → fines → pay fines
3. Click on "Write off"
    This assumes an existing debt. Which should exists if you just
    tested the previous patches.
4. If you enter "1,50", the writeoff amount saved is only 1.
     This is the bug.
5. Apply this patch
6. run ~/src/installer/data/mysql/updatedatabase.pl
7. Refresh the page
8. Retry the write off.
     Apply the annex 1 to the "Writeoff amount" field
9. It works! \o/

=== Annex 1: Subpart for each concerned page ===
1. Set DecimalSeparators syspref to ",."
2. Input "12,34"
3. Press the "Tab/Tabulation" key. It will unfocus the field and trigger the
     conversion. Validating the form does the same but it's a trick to test more
     quickly by not switching to another page.
4. The value should have been converted to 12.34
5. Do the same with "12,34 EUR" and ensure that it's converted to "12,34"
   Same with "EUR 12.34" -> "12.34"
6. "EUR 12.34€thanks lyonthree" -> "12.34"
7. "123 456,7" -> "123456.7"
8.. "2" -> "2"
9.. "2,0" -> "2.0"
10. "-1'345.95" -> "-1345.95"
11. ",1234" -> ".1234"
12. Set DecimalSeparators syspref to ","
13. Reload the page with the form
14. "1.000" -> "1000"
15. "1.000.000,10" -> "1000000.10"
16. Set DecimalSeparators syspref to "."
17. Reload the page with the form
18. "1,000" -> "1000"
19. "1,000,000.10" -> "1000000.10"
20. Validate the form and check that the final value with the dot was
21    well handled.
22. Think about Koalas and smile. 🐨🐨🐨

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Comment 134 Martin Renvoize 2018-09-14 08:19:24 UTC
Created attachment 78635 [details] [review]
Bug 12310: (QA follow-up) Fix typo: unformating => unformatting
Comment 135 Martin Renvoize 2018-09-14 08:19:30 UTC
Created attachment 78636 [details] [review]
Bug 12310: (QA follow-up) Simplify DecimalSeparators syspref description

Thanks Marcel for the new version :)
Comment 136 Martin Renvoize 2018-09-14 08:19:36 UTC
Created attachment 78637 [details] [review]
Bug 12310: (follow-up) Add relevant sysprefs to HEA

DecimalSeparators: the new one
CurrencyFormat: related because it's the display format
Comment 137 Martin Renvoize 2018-09-14 08:19:41 UTC
Created attachment 78638 [details] [review]
Bug 12310: (QA follow-up) Add prefs to test too

Test plan:
Run t/db_dependent/UsageStats.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 138 Martin Renvoize 2018-09-14 08:20:44 UTC
Rebased the patchset (and fixed a sha1 missing error), now moving onto continuing QA.
Comment 139 Martin Renvoize 2018-09-14 10:55:23 UTC
I'm afraid I agree with marcelr, I don't really like that one can enter one thing and when it's displayed later see a different thing due to the distinct preferences.

I also can't help but feel like there's a better way out there, input masking or something?

Finally, there's a lot of test cases here, if we did push this I'd really like to see some tests added to ensure we don't end up with a regression.
Comment 140 Victor Grousset/tuxayo 2018-09-14 13:51:17 UTC
(In reply to Marcel de Rooy from comment #121)
> The relation between CurrencyFormat and DecimalSeparators still worries me.

The only simple solution might be to keep them separated (but one should configure them coherently!) because it's a requirement to be able to use multiple separators in the same instance.

During the last hackfest (march in Marseille) after talking to enough devs and librarians, I couldn't not put that in the requirements :(

> On paycollect CurrencyFormat formats the suggested price (using the
> separators in CurrencyFormat) 

No, the example takes the first char of DecimalSeparators.

> that I will edit (using the other pref).
> Suppose that DecimalSeparators is not exactly identical, I will have issues
> here.

So fortunately this is not the case :) (above answer)

> Should decimal separator not just be the one identified as such in
> CurrencyFormat?

Unfortunately no :( (first answer)

> Hypothetically, I could use # as decimal separator (it works), but on the
> form the prices are shown according to currency format.

Which forms?
Comment 141 Victor Grousset/tuxayo 2018-09-14 14:11:11 UTC
(In reply to Martin Renvoize from comment #139)
> I'm afraid I agree with marcelr, I don't really like that one can enter one
> thing and when it's displayed later see a different thing due to the
> distinct preferences.

Unfortunately the only simple solution might be to keep them separated (but one should configure them coherently!) because it's a requirement to be able to use multiple separators in the same instance.

During the last hackfest (march in Marseille) after talking to enough devs and librarians, I couldn't not put that in the requirements :(


> I also can't help but feel like there's a better way out there, input
> masking or something?

That's not compatible with pasting values [1]

And also we might prefer focusing on getting rid of x100 or /100 of the amounts + decimal part truncation.
It might not be possible in a reasonable time put up a great UX + handle most cases at the same time.
That might be better to refine out decimal number handling in various steps.
Considering how complicated the problem is and that a perfect solution will take years to get enough resources and get into master.


[1] The cases could be in acquisition when copying from another software, an invoice email, a PDF.
Or maybe from somewhere else in Koha? I'm not sure about that use case.


> Finally, there's a lot of test cases here, if we did push this I'd really
> like to see some tests added to ensure we don't end up with a regression.

You having a Selenium test that go to one field and try a part of the cases of the test plan?
Comment 142 Victor Grousset/tuxayo 2018-09-14 14:23:01 UTC
> And also we might prefer focusing on getting rid of x100 or /100 of the amounts + decimal part truncation.

Additional note: these are regressions from the 16.11 or the 16.05
The current state is less functional than few versions ago.

(there were still cases before where this issue happened, that why the aim isn't just to get back to the previous behavior)