Bug 12310

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.

we have tested your patch with "12,34" and "$12.34" and koha only accept "12.34"
with acquisitions.

sorry

The patch only changes the parsing for manual invoice input, not acquisition.

I'll add the acq when I've got some time.

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

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 

------

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)

Hum, actually you tested on an interface I haven't modified at all, I'll apply my patch on this field to.

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

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.

Created attachment 61357 [details] [review]
BUG 12310: Modified the tooltips

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

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

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

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

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

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

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)

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

*** Bug 17462 has been marked as a duplicate of this bug. ***

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

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

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

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

Rebased

Created attachment 64675 [details]
Screenshot rebasing problem

Screenshot of text that remained from a merge / rebasing.

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

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

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>

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 :)

(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

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

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)?

> 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.

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.

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

It turns out that this is my next task :)

Conflicts T_T

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

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.

Critical: because it must address
- bug 18723
- bug 19521

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)

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"

- 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.

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

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

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) :)

Hi Victor, should this be Needs Signoff?

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

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. ﷐[U+1F428]﷑﷐[U+1F428]﷑﷐[U+1F428]﷑

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.

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. ﷐[U+1F428]﷑﷐[U+1F428]﷑﷐[U+1F428]﷑

(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?

*Has everything been addressed?

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

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. ﷐[U+1F428]﷑﷐[U+1F428]﷑﷐[U+1F428]﷑

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

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 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.

Using DecimalSeparator as ',.', we should be able to enter #,###,###.## and get #######.## back.

(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? ^^"

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)

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. ﷐[U+1F428]﷑﷐[U+1F428]﷑﷐[U+1F428]﷑

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

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>

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>

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. ﷐[U+1F428]﷑﷐[U+1F428]﷑﷐[U+1F428]﷑

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

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>

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>

As it's a sensitive issue, more testing would be better to ensure that there are no regressions.

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

(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.

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. ﷐[U+1F428]﷑﷐[U+1F428]﷑﷐[U+1F428]﷑

Patch tested with a sandbox, by Sonia Bouis <sonia.bouis@univ-lyon3.fr>

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. ﷐[U+1F428]﷑﷐[U+1F428]﷑﷐[U+1F428]﷑

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

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>

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>

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>

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. ﷐[U+1F428]﷑﷐[U+1F428]﷑﷐[U+1F428]﷑

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>

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

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 ?

(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.

(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?

(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?

(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.

(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.

(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)

(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.

(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>

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.

(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

(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.

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. ﷐[U+1F428]﷑﷐[U+1F428]﷑﷐[U+1F428]﷑

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

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>

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>

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>

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. ﷐[U+1F428]﷑﷐[U+1F428]﷑﷐[U+1F428]﷑

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>

Created attachment 75958 [details] [review]
Bug 12310: (QA follow-up) Fix typo: unformating → unformatting

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!

-        - "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.)

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(?).

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?

(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

Created attachment 76896 [details] [review]
Bug 12310: (QA follow-up) Simplify DecimalSeparators syspref description

Thanks Marcel for the new version :)

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

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.

Resuming here now

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 ??

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 );

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.

(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?

Sorry for the inconvenience. Your patch has been attached. With you as git author if that's ok.

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

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. ﷐[U+1F428]﷑﷐[U+1F428]﷑﷐[U+1F428]﷑

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

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>

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>

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>

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. ﷐[U+1F428]﷑﷐[U+1F428]﷑﷐[U+1F428]﷑

Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>

Created attachment 78635 [details] [review]
Bug 12310: (QA follow-up) Fix typo: unformating => unformatting

Created attachment 78636 [details] [review]
Bug 12310: (QA follow-up) Simplify DecimalSeparators syspref description

Thanks Marcel for the new version :)

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

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>

Rebased the patchset (and fixed a sha1 missing error), now moving onto continuing QA.

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.

(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?

(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?

> 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)

bug 21507 is a reboot of this one with simpler specs

Hello 

I wake up this BZ a little :) I'm wonder if this behavior would not be fixed by bug 33028 (and bug 21507 too) ?

bug 33028 looks like to be mostly about cron and circulation rules, right?

So it's not helping for this on and it's successor, bug 21507 :(