Description
Victor Grousset/tuxayo
2018-10-06 10:04:56 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. Here is a SQL query which helps detecting incoherences in the account lines. SELECT invoicenumber, link.ordernumber, items.itemnumber, unitprice, unitprice_tax_excluded, unitprice_tax_included, items.price, ecost, unitprice_tax_excluded/ecost as ratio FROM aqorders LEFT JOIN aqinvoices ON aqinvoices.invoiceid = aqorders.invoiceid LEFT JOIN aqorders_items as link ON link.ordernumber=aqorders.ordernumber LEFT JOIN items ON link.itemnumber=items.itemnumber WHERE unitprice/ecost > 5 OR unitprice_tax_excluded/ecost > 5 OR items.price/ecost > 5 ORDER BY ratio DESC; > Here is a SQL query which helps detecting incoherences in the account lines. We mistakenly sent the message here instead of bug 18723 about acquisitions. See there for the queries. Created attachment 95734 [details] [review] WIP Bug 21507 - Fix decimal separators issues in patrons payments/fines Created attachment 95735 [details] [review] WIP not validated at all TODO: - rebase on master (the last 13 months...) - write test plan Lowering severity, this does not sound like a critical bug to me. Isn't it a new enhancement finally? The criticality came from bug 12310 . I guess having fines truncated or multiplied by x100 was the cause of that level ^^" I still think inputmasks are the way to go personally.. https://github.com/RobinHerbots/Inputmask 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. > 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!!!!
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 ? (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? 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. Created attachment 144502 [details] [review] Bug 21507 : WIP - rebase actual proposal on master 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 Let's discuss about it :) Created attachment 144535 [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 Created attachment 144536 [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 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 (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 ? 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? (In reply to Thibaud Guillot from comment #19) > 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. Sure, okay. > If you agree with this maybe addition of 'comment 10' > algoritm and new syspref > can be the solution ? Looks good. You just need to arbitrarily decide what would "1,234" (with syspref 3 decimal digits on) mean. It's unsolvable with certainty because IRL it's unsolvable. A currency with 3 decimal digits wouldn't use arbitrarily dot or comma as a thousands separator. I think you can arbitrarily solve and pick the easiest way to implement that might still make sense functionally. And that would solve all the simple and medium difficulty cases. And would be a good step to supporting the 3 decimal digits case. And for better supports let's wait for real world feedback. As of now 3 decimal digits isn't supported at all so it's definitely step in the right direction. And with real world feedback the next increment for uncovered cases could be plane. Trying now would be hazardous. And comment 10 algo is already a big step. (In reply to Katrin Fischer from comment #20) > 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. That would be great, ICU is the kind of project that should already provide an implementation for must stuff regarding this. Instead of trying in invent something to cover all cases and have it ruin by a new case. >_< (that's a lot of the story of this and bug 12310) > ICU::Number Where is it? The closest thing I found is https://metacpan.org/dist/Unicode-ICU and there is no sign of number formatting. (In reply to Victor Grousset/tuxayo from comment #21) > (In reply to Katrin Fischer from comment #20) > > 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. > > That would be great, ICU is the kind of project that should already provide > an implementation for must stuff regarding this. Instead of trying in invent > something to cover all cases and have it ruin by a new case. >_< (that's a > lot of the story of this and bug 12310) > > > ICU::Number > > Where is it? The closest thing I found is > https://metacpan.org/dist/Unicode-ICU > and there is no sign of number formatting. Unfortunately, the NumberFormatter is not part of this package [1], but perhaps we could use MessageFormat [2] for number formatting, e.g. my $formatter = Unicode::ICU::MessageFormat->new('en'); my $chars = $formatter->format('{0, number, currency}', [123.4]); my $chars = $formatter->format('{0, number, :: .00}', [123.4]); See [3] and [4] for an explanation of the format string. [1] https://metacpan.org/release/CPANEL/Unicode-ICU-0.06/source/README.md [2] https://metacpan.org/pod/Unicode%3A%3AICU%3A%3AMessageFormat [3] https://unicode-org.github.io/icu-docs/apidoc/dev/icu4c/classicu_1_1MessageFormat.html#patterns [4] https://unicode-org.github.io/icu/userguide/format_parse/numbers/skeletons.html (In reply to Raphael Straub from comment #22) > Unfortunately, the NumberFormatter is not part of this package [1], but > perhaps we could use MessageFormat [2] for number formatting, e.g. Does anyone know a way to make it accept at the same time 123.45 and 123,45 ? The tricky thing here is that we need to find a tool that can handle multiple formats and when it's not ambiguous, sort it out. I looked at Math::Currency and it seems it only works with a locale switch so it can't handle at the same time german, french and us formats. For witch all cases seem not ambiguous. |