Bug 21507 - Decimal separators issues in patrons payments/fines, simplified
Summary: Decimal separators issues in patrons payments/fines, simplified
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Thibaud Guillot
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 12310 17098 19521
  Show dependency treegraph
 
Reported: 2018-10-06 10:04 UTC by Victor Grousset/tuxayo
Modified: 2023-07-07 16:37 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
WIP Bug 21507 - Fix decimal separators issues in patrons payments/fines (7.05 KB, patch)
2019-11-22 18:02 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
WIP not validated at all (2.05 KB, patch)
2019-11-22 18:02 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 21507 : WIP - rebase actual proposal on master (9.55 KB, patch)
2022-12-08 15:48 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 21507 : Set decimal separator according to currency format syspref (8.06 KB, patch)
2022-12-12 15:54 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 21507 : Set decimal separator according to currency format syspref (16.51 KB, patch)
2022-12-12 15:56 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 21507 : Set decimal separator according to currency format syspref (16.51 KB, patch)
2022-12-12 15:57 UTC, Thibaud Guillot
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Grousset/tuxayo 2018-10-06 10:04:56 UTC
This is a reboot of bug 12310 with simpler specs.
kidclamp and paul_p discussed about it at the KohaCon and they would push for simpler requirements than the ones that I got at the 2018 march hackfest.
And after some discussions on #koha, it seems that it should be acceptable :)

== Specs ==
- allow only one separator, the one from CurrencyFormat
  - if some libraries really need supporting multiple ones, it should be another patch, don't hesitate to open a bug to talk about this
- do not handle thousands separator
- block form submission if the format is wrong


== resources: some of the latest IRC discussions ==
http://irc.koha-community.org/koha/2018-10-05#i_2078324
http://irc.koha-community.org/koha/2018-10-05#i_2078377
Comment 1 Victor Grousset/tuxayo 2018-11-26 16:31:16 UTC
Quite complete discussion on the issue in the IRC channel: http://irc.koha-community.org/koha/2018-11-26#i_2100176

The current plan is still valid from what I understand. And various followups have been discussed.
Comment 2 Arthur Suzuki 2019-11-13 14:22:29 UTC Comment hidden (obsolete)
Comment 3 Victor Grousset/tuxayo 2019-11-15 20:06:16 UTC Comment hidden (obsolete)
Comment 4 Victor Grousset/tuxayo 2019-11-22 18:02:43 UTC
Created attachment 95734 [details] [review]
WIP Bug 21507 - Fix decimal separators issues in patrons payments/fines
Comment 5 Victor Grousset/tuxayo 2019-11-22 18:02:52 UTC
Created attachment 95735 [details] [review]
WIP not validated at all
Comment 6 Victor Grousset/tuxayo 2019-11-22 18:03:39 UTC
TODO:
- rebase on master (the last 13 months...)
- write test plan
Comment 7 Jonathan Druart 2019-12-10 14:07:45 UTC
Lowering severity, this does not sound like a critical bug to me. Isn't it a new enhancement finally?
Comment 8 Victor Grousset/tuxayo 2020-01-15 11:51:13 UTC
The criticality came from bug 12310 .

I guess having fines truncated or multiplied by x100 was the cause of that level ^^"
Comment 9 Martin Renvoize 2020-01-22 11:23:35 UTC
I still think inputmasks are the way to go personally.. https://github.com/RobinHerbots/Inputmask
Comment 10 Martin Renvoize 2021-04-22 15:07:06 UTC
I'm coming back around to thinking about this as a coercion/validation function.

We need to allow for pasted input.. so that could include any of the following forms.


$ 1,000
$1000
$1000.00
$1,000.00
1,000.00$
1000.00$
1000$
USD 1000

etc, etc.. the list goes on with various symbols and strings denoting currency and various delimiters and lack of delimiters.

I think perhaps we need to parse all input number during the validation step and coerce them into a standard form prior to form submissions.

Generally speaking I think the following could/shoould catch all cases.

1. Drop non-digit character (excluding delimiters [comma and dot])
2. Work backwards to find if we have decimal digits (i.e. 2 digits after either a comma or dot)
3. If we find a decimal delimiter, drop the opposit delimiter from the string (i.e. if we find a dot then drop any comma's.. if we find a comma, drop any dots)
4. If we found not decimal delimiter.. check for thousands separator then add the opposite decimal delimiter and two 0 digits.
5. Repeat step 3.
6. If the decimal separator at this point is a comma, change it to dot upon submission.

I think that flow would ensure we always end up with a standardised decimal number with a dot as the decimal separator.. which is what the serverside controllers all expect I believe.
Comment 11 Victor Grousset/tuxayo 2021-06-01 16:04:48 UTC
> 2. Work backwards to find if we have decimal digits (i.e. 2 digits after either a comma or dot)
> 3. If we find a decimal delimiter, drop the opposit delimiter from the string (i.e. if we find a dot then drop any comma's.. if we find a comma, drop any dots)

This works thanks to relying on having two decimal digits right? No less, no more. Which is fine, it's already soooo hard as it is.

This looks great, it seems to cover all of the cases and the logic is quite simple, amazing algorithm that your have found there!!!!
Comment 12 Henri-Damien LAURENT 2021-06-30 10:33:28 UTC
Dears,

Your solution seems to fit your environment. But let me state that some countries, like France have different signs for thousand delimiter and decimal delimiter.

And I can see no real support for those "oddities" in the proposed js code. Worse, there seems to be no way to parametrize those 'locales' for currency.

For instance, if someone copy pastes 123,13 €, it would be converted into 12,313.00 which is counter intuitive for usual french folks.  

Moreover, it seems that there is no way to add more decimals... (which should be enable for some rates, if there is a fine per page printed for instance, it could be 0.013 cents per page)

To that aim, decimal_fill (hardcoded at the moment in Koha::Number::Price could be parametrized).
And the way the proposed patch fixed that would take a system variable for number format into account.
So it has my favour.
  
Can it be discussed some time ?
Comment 13 Martin Renvoize 2021-07-01 08:19:53 UTC
(In reply to Henri-Damien LAURENT from comment #12)
> Your solution seems to fit your environment. But let me state that some
> countries, like France have different signs for thousand delimiter and
> decimal delimiter.
> 
> And I can see no real support for those "oddities" in the proposed js code.
> Worse, there seems to be no way to parametrize those 'locales' for currency.
> 
> For instance, if someone copy pastes 123,13 €, it would be converted into
> 12,313.00 which is counter intuitive for usual french folks.  
> 
> To that aim, decimal_fill (hardcoded at the moment in Koha::Number::Price
> could be parametrized).
> And the way the proposed patch fixed that would take a system variable for
> number format into account.
> So it has my favour.
>   
> Can it be discussed some time ?

More than happy to discuss.. I'm a little confused by your comment though.. are you replying to the code attached here or the algorithm I propose in comment 10.  Unfortunately, I've not had a moment to actually try to realise the code to match that algorithm yet.

> Moreover, it seems that there is no way to add more decimals... (which
> should be enable for some rates, if there is a fine per page printed for
> instance, it could be 0.013 cents per page)

I can see this being an issue with my algorithm, though I don't expect the algorithm to be applied to all inputs.. only those that may be pasted into.. for this case I would expect a typed amount and we should allow parts of a cent. (admittedly, I've never actually come across a scheme like this.. koha doesn't support a 'fine per page' rule anywhere currently unless I'm mistaken?
Comment 14 Victor Grousset/tuxayo 2021-07-14 06:26:35 UTC
Hi Henri-Damien, thanks for coming on this topic which desperately needs more attention.

Actually, unless a change of plans, the code is obsoleted by the proposal of comment 10

(In reply to Henri-Damien LAURENT from comment #12)
> Moreover, it seems that there is no way to add more decimals... (which
> should be enable for some rates, if there is a fine per page printed for
> instance, it could be 0.013 cents per page)

The proposed algorithm indeed can't handle that. Due to counting the digits from the right before the separator. If there are 3 then the separator will be a thousands separator, if there are two, it will be a decimal one.

About the case of 3-digits-per-page-fee which even when multiplied by many pages will still often need 3 digits: is the idea just that Koha would keep track of the third digit? Instead of manually rounding it before inputing it in Koha?
Or is there something more? How would that work for payments?

It's to have an idea about what should be done in the next step on decimal numbers (this ticket) and what to plan for follow up tickets in order to have an incremental approach.

It seems from your example that it's not about supporting currencies that have 3 digits. So it doesn't imply as much functional changes in various places.
Comment 15 Thibaud Guillot 2022-12-08 15:48:52 UTC Comment hidden (obsolete)
Comment 16 Thibaud Guillot 2022-12-12 15:54:23 UTC Comment hidden (obsolete)
Comment 17 Thibaud Guillot 2022-12-12 15:56:49 UTC Comment hidden (obsolete)
Comment 18 Thibaud Guillot 2022-12-12 15:57:48 UTC
Created attachment 144537 [details] [review]
Bug 21507 : Set decimal separator according to currency format syspref

This is always a WIP, but I push some code to let the subject open to
any advice

Test plan :

1) Go on 'cgi-bin/koha/members/mancredit.pl?borrowernumber={patron_id}'
2) Create manual invoice or credit and see differents input pattern and example
3) You can also test payment feature with 'total due' transformation
Comment 19 Thibaud Guillot 2022-12-23 09:50:05 UTC
(In reply to Henri-Damien LAURENT from comment #12)
> Dears,
> 
> Your solution seems to fit your environment. But let me state that some
> countries, like France have different signs for thousand delimiter and
> decimal delimiter.
> 
> And I can see no real support for those "oddities" in the proposed js code.
> Worse, there seems to be no way to parametrize those 'locales' for currency.
> 
> For instance, if someone copy pastes 123,13 €, it would be converted into
> 12,313.00 which is counter intuitive for usual french folks.  
> 
> Moreover, it seems that there is no way to add more decimals... (which
> should be enable for some rates, if there is a fine per page printed for
> instance, it could be 0.013 cents per page)
> 
> To that aim, decimal_fill (hardcoded at the moment in Koha::Number::Price
> could be parametrized).
> And the way the proposed patch fixed that would take a system variable for
> number format into account.
> So it has my favour.
>   
> Can it be discussed some time ?

Hi everyone,

I try to make this patch goes forward a bit :) 

So I agree with Henri-Damien, maybe it must be take in case with a new syspref to force 3 digits after the decimal separator, but I ask you if you agree with this particular setting : "If user active this syspref to force usage of 3 digits after the decimal separator, he must always enter 3 digits after separator" because it can be hard to define if it's a decimal or thousand delimiter.

If you agree with this maybe addition of 'comment 10' algoritm and new syspref can be the solution ?
Comment 20 Katrin Fischer 2023-06-30 09:47:06 UTC
Could it be an option to use a library here? Someone has mentioned ICU::Number to me, but I am not sure of the functionality.

And what needs to be done to move this into testing?