Doesn't that work already with the TT plugin now?
Created attachment 96410 [details] [review] Bug 20352: DB changes
Created attachment 96411 [details] [review] Bug 20352: Schema changes
Created attachment 96412 [details] [review] Bug 20352: Add the ability to chose the position of the currency This patch mimicks what has been done for p_sep_by_space to add a space separation between the symbol and the value. Here we are adding p_cs_precedes to display the symbol before or after the value. Note that so far the symbol is only displayed in one place at the OPAC (opac-main) Test plan: - Create a manual invoice for a patron - Edit the active currency, select 'Before' (should be the default here) - Use the charged patron to login at the OPAC => On the mainpage you should see the charges displayed, with the symbol before the value - Edit the active currency, select 'After' => The symbol is now positionned after the value QA note: From https://www.gnu.org/software/libc/manual/html_node/Currency-Symbol.html p_cs_precedes is only used for positive amount, n_cs_precedes should be used for negative. Here we are assuming that positive and negative amounts are displayed the same way.
Just showing how easy it can be to make things configurable at currency level.
Created attachment 98234 [details] [review] Bug 20352: DB changes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 98235 [details] [review] Bug 20352: Schema changes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 98236 [details] [review] Bug 20352: Add the ability to chose the position of the currency This patch mimicks what has been done for p_sep_by_space to add a space separation between the symbol and the value. Here we are adding p_cs_precedes to display the symbol before or after the value. Note that so far the symbol is only displayed in one place at the OPAC (opac-main) Test plan: - Create a manual invoice for a patron - Edit the active currency, select 'Before' (should be the default here) - Use the charged patron to login at the OPAC => On the mainpage you should see the charges displayed, with the symbol before the value - Edit the active currency, select 'After' => The symbol is now positionned after the value QA note: From https://www.gnu.org/software/libc/manual/html_node/Currency-Symbol.html p_cs_precedes is only used for positive amount, n_cs_precedes should be used for negative. Here we are assuming that positive and negative amounts are displayed the same way. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Works nicely, Signing off
Worth noting: having the formatting at currency level is not ideal actually. For instance we could have: * 1 234,56 € * €1.234,56 * 1.234,56 € * €1,234.56 * ... Letting QA decide if this can be considered as an improvement anyway.
Created attachment 103924 [details] [review] Bug 20352: DB changes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 103925 [details] [review] Bug 20352: Schema changes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 103926 [details] [review] Bug 20352: Add the ability to chose the position of the currency This patch mimicks what has been done for p_sep_by_space to add a space separation between the symbol and the value. Here we are adding p_cs_precedes to display the symbol before or after the value. Note that so far the symbol is only displayed in one place at the OPAC (opac-main) Test plan: - Create a manual invoice for a patron - Edit the active currency, select 'Before' (should be the default here) - Use the charged patron to login at the OPAC => On the mainpage you should see the charges displayed, with the symbol before the value - Edit the active currency, select 'After' => The symbol is now positionned after the value QA note: From https://www.gnu.org/software/libc/manual/html_node/Currency-Symbol.html p_cs_precedes is only used for positive amount, n_cs_precedes should be used for negative. Here we are assuming that positive and negative amounts are displayed the same way. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 103927 [details] [review] Bug 20352: Identify boolean columns Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Jonathan Druart from comment #10) > Worth noting: having the formatting at currency level is not ideal actually. > For instance we could have: > * 1 234,56 € > * €1.234,56 > * 1.234,56 € > * €1,234.56 > * ... > > Letting QA decide if this can be considered as an improvement anyway. Works as advertised. I don't see why we wouldn't allow placing the currenc symbol after the amount. QA script happy as well.
As I tried to explain in the example, for a given currency different displays are possible. The formatting should be done depending on client-side configuration instead (locales).
I've been mulling this one over and I'm inclined to agree, that although this does move us towards the goal, it's at the wrong level. I believe we should be formatting based on locale rather than by currency.
I think for client-side formatting we should use the native JS function (Intl.NumberFormat): https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat For template side, we should try to replicate what the above does via a TT plugin.
https://www.thefinancials.com/Default.aspx?SubSectionID=curformat for reference... there's ALLOT of different formats out there it would seem.
Final comment.. why do we use 3 hard coded values for our money formatting at the moment rather than rely on something like https://metacpan.org/pod/Locale::Currency::Format which claims to try to support the full ISO 4217 set?
(In reply to Martin Renvoize from comment #17) > I've been mulling this one over and I'm inclined to agree, that although > this does move us towards the goal, it's at the wrong level. > > I believe we should be formatting based on locale rather than by currency. On the other hand it's either having it now (but not perfect), or not having it until... who knows? I am not planning to spend more time on this. (In reply to Martin Renvoize from comment #20) > Final comment.. why do we use 3 hard coded values for our money formatting > at the moment rather than rely on something like > https://metacpan.org/pod/Locale::Currency::Format which claims to try to > support the full ISO 4217 set? For backward compatibility when Koha::Number::Price has been implemented.
(In reply to Martin Renvoize from comment #20) > Final comment.. why do we use 3 hard coded values for our money formatting > at the moment rather than rely on something like > https://metacpan.org/pod/Locale::Currency::Format which claims to try to > support the full ISO 4217 set? Because it's buggy and the author is unresponsive. Bug 24278 - Replace Locale::Currency::Format because it's buggy As we support the Goethe libraries, we have run into quite some issues with different currencies. - Koha currently uses it to format fines in overdue notices.
(In reply to Katrin Fischer from comment #22) > (In reply to Martin Renvoize from comment #20) > > Final comment.. why do we use 3 hard coded values for our money formatting > > at the moment rather than rely on something like > > https://metacpan.org/pod/Locale::Currency::Format which claims to try to > > support the full ISO 4217 set? > > Because it's buggy and the author is unresponsive. > Bug 24278 - Replace Locale::Currency::Format because it's buggy > > As we support the Goethe libraries, we have run into quite some issues with > different currencies. - Koha currently uses it to format fines in overdue > notices. Hmm.. I wasn't aware it had issues. What about https://metacpan.org/pod/CLDR::Number::Format::Currency.?
(In reply to Martin Renvoize from comment #23) > > What about https://metacpan.org/pod/CLDR::Number::Format::Currency.? Looks good at a glance although it has 21 current issues and it looks like it can be a bit opinionated. That it might do more than just format (e.g. rounding)? Might not be an issue though if we make sure it's already a valid money number...
(In reply to Jonathan Druart from comment #10) > Worth noting: having the formatting at currency level is not ideal actually. > For instance we could have: > * 1 234,56 € > * €1.234,56 > * 1.234,56 € > * €1,234.56 > * ... > Agreed. In many cases, I think it varies from country to country, but for Canada it actually varies by region, if I recall correctly. The synopsis for https://metacpan.org/pod/CLDR::Number::Format::Currency#SYNOPSIS actually illustrates this well for the Canadian example.
So, Locale::Currency::Format contains an internal hard coded, but easy to read hasref of arrays for a small subset of currencies (without taking regions into account). CLDR::Number::Format::Currency utilises the CLDR international Unicode consortium data which take into account language, region and currency.. but it's no longer maintained and is based on an older version of the CLDR data.. there's another option I wasn't aware of until yesterday, Locale::CLDR which appears to be sort of maintained in so much as it's using a more up to date export of the CLDR data.. but it's also a significantly larger module with all sorts of data that's not directly relevant to us. I'm tempted to come full circle and go back to using our DB to store just a relevant set of combinations.. perhaps populating it initially with a subset of the CLDR data which takes into account region + currency. Right now, I'm working on the maths side over at https://gitlab.com/mrenvoize/Koha/-/merge_requests/2, though I'm about to rip out Data::Money as a dependency.. in fixing that module I decided it would be easier to just replicate the bits we need within our codebase rather than pulling it in as a dependency and trying to disable the bit we don't want (the formatting)
(In reply to Martin Renvoize from comment #26) > I'm tempted to come full circle and go back to using our DB to store just a > relevant set of combinations.. perhaps populating it initially with a subset > of the CLDR data which takes into account region + currency. > Might be preferable to adding an unsupported dependency or an overkill dependency. > Right now, I'm working on the maths side over at > https://gitlab.com/mrenvoize/Koha/-/merge_requests/2, though I'm about to > rip out Data::Money as a dependency.. in fixing that module I decided it > would be easier to just replicate the bits we need within our codebase > rather than pulling it in as a dependency and trying to disable the bit we > don't want (the formatting) Makes sense to me. While I like offloading maintenance onto third-party dependencies, sometimes it's easier to just implement the code ourselves instead of dealing with problematic dependencies.
(In reply to Jonathan Druart from comment #21) > (In reply to Martin Renvoize from comment #17) > > I've been mulling this one over and I'm inclined to agree, that although > > this does move us towards the goal, it's at the wrong level. > > > > I believe we should be formatting based on locale rather than by currency. > > On the other hand it's either having it now (but not perfect), or not having > it until... who knows? > I am not planning to spend more time on this. After more than four years... wouldn't it be just to get the patch anyway? After a little rebasing it seems to still work as intended and would be quite useful in the European context.
I believe this turned into a duplicate of bug 37511 now. Duplicate effort :( *** This bug has been marked as a duplicate of bug 35711 ***
Corrected bug number. *** This bug has been marked as a duplicate of bug 37511 ***