Bug 20352 - Add ability of currency/price to have a symbol after amount
Summary: Add ability of currency/price to have a symbol after amount
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 24278 4078
Blocks: 14825
  Show dependency treegraph
 
Reported: 2018-03-07 16:37 UTC by Josef Moravec
Modified: 2021-01-07 22:54 UTC (History)
5 users (show)

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


Attachments
Bug 20352: DB changes (1.63 KB, patch)
2019-12-18 11:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20352: Schema changes (1.84 KB, patch)
2019-12-18 11:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20352: Add the ability to chose the position of the currency (4.75 KB, patch)
2019-12-18 11:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20352: DB changes (1.69 KB, patch)
2020-01-31 14:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20352: Schema changes (1.91 KB, patch)
2020-01-31 14:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20352: Add the ability to chose the position of the currency (4.80 KB, patch)
2020-01-31 14:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20352: DB changes (1.75 KB, patch)
2020-04-29 12:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20352: Schema changes (1.96 KB, patch)
2020-04-29 12:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20352: Add the ability to chose the position of the currency (4.85 KB, patch)
2020-04-29 12:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20352: Identify boolean columns (1013 bytes, patch)
2020-04-29 12:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Josef Moravec 2018-03-07 16:37:23 UTC

    
Comment 1 Katrin Fischer 2018-03-31 21:15:29 UTC
Doesn't that work already with the TT plugin now?
Comment 2 Jonathan Druart 2019-12-18 11:05:43 UTC
Created attachment 96410 [details] [review]
Bug 20352: DB changes
Comment 3 Jonathan Druart 2019-12-18 11:05:47 UTC
Created attachment 96411 [details] [review]
Bug 20352: Schema changes
Comment 4 Jonathan Druart 2019-12-18 11:05:51 UTC
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.
Comment 5 Jonathan Druart 2019-12-18 11:06:16 UTC
Just showing how easy it can be to make things configurable at currency level.
Comment 6 Martin Renvoize 2020-01-31 14:09:07 UTC
Created attachment 98234 [details] [review]
Bug 20352: DB changes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2020-01-31 14:09:10 UTC
Created attachment 98235 [details] [review]
Bug 20352: Schema changes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2020-01-31 14:09:14 UTC
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>
Comment 9 Martin Renvoize 2020-01-31 14:09:44 UTC
Works nicely, Signing off
Comment 10 Jonathan Druart 2020-02-18 11:03:27 UTC
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.
Comment 11 Tomás Cohen Arazi 2020-04-29 12:07:31 UTC
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>
Comment 12 Tomás Cohen Arazi 2020-04-29 12:07:36 UTC
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>
Comment 13 Tomás Cohen Arazi 2020-04-29 12:07:40 UTC
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>
Comment 14 Tomás Cohen Arazi 2020-04-29 12:07:44 UTC
Created attachment 103927 [details] [review]
Bug 20352: Identify boolean columns

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2020-04-29 12:09:22 UTC
(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.
Comment 16 Jonathan Druart 2020-04-29 13:51:43 UTC
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).
Comment 17 Martin Renvoize 2020-05-01 07:22:37 UTC
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.
Comment 18 Martin Renvoize 2020-05-01 07:49:53 UTC
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.
Comment 19 Martin Renvoize 2020-05-01 07:50:37 UTC
https://www.thefinancials.com/Default.aspx?SubSectionID=curformat for reference... there's ALLOT of different formats out there it would seem.
Comment 20 Martin Renvoize 2020-05-01 08:24:59 UTC
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?
Comment 21 Jonathan Druart 2020-05-01 10:30:52 UTC
(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.
Comment 22 Katrin Fischer 2020-05-01 10:33:19 UTC
(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.
Comment 23 Martin Renvoize 2020-05-01 10:50:56 UTC
(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.?
Comment 24 David Cook 2021-01-07 00:39:09 UTC
(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...
Comment 25 David Cook 2021-01-07 00:40:51 UTC
(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.
Comment 26 Martin Renvoize 2021-01-07 08:04:25 UTC
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)
Comment 27 David Cook 2021-01-07 22:54:57 UTC
(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.