Description
Chris Cormack
2010-05-21 01:22:54 UTC
Created attachment 7308 [details] [review] Added a sub that places currency symbol. Added to most prices. I've called this sub on as many prices as I could find. However there still might be a few that I've missed. This can be seen on /cgi-bin/koha/members/boraccount.pl It seems to me that currency formatting is much the same as date formatting, and as such should be handled by a T:T plugin. Koha passes the raw numeric values to the template, and the template formats the currency as appropriate. Hi Aleksa, The more I look at your patch, the more I think it's the *perfect* candidate for Template::Toolkit plugins. Something similar to what has been made for displaying dates: see : http://wiki.koha-community.org/wiki/Coding_Guidelines#Displaying_dates Look at Koha/Template/Plugin/KohaDates/KohaDates.pm, create a KohaCurrencies.pm that could be (untested) use Template::Plugin::Filter; use base qw( Template::Plugin::Filter ); use C4::Dates; sub filter { my ($self,$amount) = @_; my $curr_symbol = GetCurrency()->{symbol}; if ($curr_symbol && $amount) { if ($amount =~ /^\-/) { $amount =~ s/^\-/\-$curr_symbol/; } else { $amount = $curr_symbol.$amount; } } return $amount; } and just update all template to display currencies: [% USE KohaCurrencies %] and [% totspent | $KohaCurrencies %] ... thanks for your feedback on this 1; I propose to expand this functionality to the formatting of currency values as well. Up to now, formatting is handled via the preference "CurrencyFormat". User can choose between 360,000.00 (US) and 360 000,00 (FR). The preference is evaluated in a hard coded way in several .pl and .pm files as acqui-home.pl aqbudgetperiods.pl layout2pages.pm aqplan.pl aqbudgetperiods.pl layout3pages.pm layout2pages.pm (maybe more) The code reads: ---Snip--- my $cur_format = C4::Context->preference("CurrencyFormat"); my $num; if ( $cur_format eq 'US' ) { $num = new Number::Format( 'int_curr_symbol' => '', 'mon_thousands_sep' => ',', 'mon_decimal_point' => '.' ); } elsif ( $cur_format eq 'FR' ) { $num = new Number::Format( 'decimal_fill' => '2', 'decimal_point' => ',', 'int_curr_symbol' => '', 'mon_thousands_sep' => ' ', 'thousands_sep' => ' ', 'mon_decimal_point' => ',' ); } ---Snip--- To make things more configurable I propose to do the following: 1) Expand table currency (http://schema.koha-community.org/tables/currency.html) with an additional column "format" This column could be pre-filled by system_preferences.sql, and it should be editable (/admin/currency.pl). Maybe some input validation is necessary to get well formed strings. 2) Remove preference "CurrencyFormat" (not longer used) 3) The sub GetCurrency()(located in Budgets.pm) will return the currency format string along with the currency symbol. No chnges needed (Select *....) Use GetCurrency()->{format} in sub filter and format the number as appropriate. Hi Marc, I agree with 1) and 2) - not sure I get 3). I would really like to see a new TT plugin doing the formatting on template level. Then we could start stepping through the code and making it show correctly everywhere. Do we need to distinguish between active currency and other currency display? I think later is only needed in acquisitions. Hi Katrin, I expect that GetCurrency() would be used in a context like proposed by Paul Poluain in Comment 3, i.e. it would be the template that does the formatting. GetCurency() returns an aray resulting form a select * statement, you simply could use $active_currency->{format} along with the existing $active_currency->{currency} The same with the result of GetCurrencies() (but it seems that it is currently only used to get the rates). (In reply to comment #4) > I propose to expand this functionality to the formatting of currency values > as well. Marc, when you say that you "propose" does it mean that you'll submit a patch ? Or you propose to go this way (and I agree), and hope someone else will write a patch. Aleksa = we have no feedback from you ! Aleksa was a high school student doing some work on the school holidays. He may time and/or the desire to work on this again. But to expect him to leap in and make a template::toolkit plugin is a pretty big ask for a high school kid. Paul, If everybody agrees with my proposition I can try to submit a patch within the next weeks. However I would like to try first with some smaller issue, just to find out how to patch. Marc (In reply to comment #9) > Paul, > > If everybody agrees with my proposition I can try to submit a patch within > the next weeks. I don't understand why we need a column in currencies table. There is only one currency displayed, it's the one that has "active=1". When you select a different currency when you enter an amount, it's transformed, with the rate, to become a "currency that has active=1". So I think the CurrencyFormat should be switched to a formatting display, but we shouldn't need a column. > However I would like to try first with some smaller issue, just to find out > how to patch. Just in case: Marc, I see you're from switzerland. Do you know that next week, BibLibre organize a hackfest in Marseille (France), there will be mbalmer, from Switzerland too, you're welcomed if you want & can join us ( http://drupal.biblibre.com/en/blog/entry/2012-hackfest-in-europe) The hackfest would be the perfect place to write your first patches ! just drop me a mail or catch me on IRC for more information if you want Should be done on top of bug 12979, see Koha::Number::Price. Created attachment 43320 [details] [review] Bug 4078 - Display active currency symbol on currency output and input Display active currency using international formatting patterns and active currency symbol withplugin price. The patch inlcudes an example on Opac. To test: Without patch: - Log in to the Opac with an user who has credits or fines. - Go to 'Your summary'. The amount appears in the tab 'Credits'rsp. 'Fines' - Apply patch - Make sure that you have defined an active currency in Home > Administration > Currency & Exchange rates - Search for Syspref 'Currency Format' - Select a formatting pattern from the drop down list - Go back to the Opac and verify that the amount is properly formatted, including the position of the currency symbol. Note: the symbol should not be displayed by default. (I have not tested it, but it's a personal note for later). Still applies. Removed 12979 as blocking (12979 is pushed to Master). (In reply to Marc Véron from comment #14) > Still applies. > Removed 12979 as blocking (12979 is pushed to Master). I think it's a good thing to keep the dependencies, even when the patches are pushed. Still applies. Created attachment 45877 [details] [review] Bug 4078 - Display active currency symbol on currency output and input Display active currency using international formatting patterns and active currency symbol withplugin price. To test: Without patch: - Go to 'Your summary'. The amount appears in the tab 'Credits'rsp. 'Fines' - Apply patch - Make sure that you have defined an active currency in Home > Administration > Currency & Exchange rates - Search for Syspref 'Currency Format' - Select a formatting pattern from the drop down list - Go to a page that uses Price formatting and verify that currency symbol displays wheere price formatting is applied (git grep '| $Price') (Amended patch to remove example opac-user.tt. To test with OPAC see bug 15374) Created attachment 45878 [details] [review] Bug 4078 - Display active currency symbol on currency output and input Display active currency using international formatting patterns and active currency symbol withplugin price. To test: - Apply patch - Make sure that you have defined an active currency in Home > Administration > Currency & Exchange rates - Search for Syspref 'Currency Format' - Select a formatting pattern from the drop down list - Go to a page that uses Price formatting and verify that currency symbol displays wheere price formatting is applied (git grep '| $Price') (Amended patch to remove example opac-user.tt. To test with OPAC see bug 15374) Created attachment 45888 [details] [review] Bug 4078 - Display active currency symbol on currency output and input Display active currency using international formatting patterns and active currency symbol withplugin price. To test: - Apply patch - Make sure that you have defined an active currency in Home > Administration > Currency & Exchange rates - Search for Syspref 'Currency Format' - Select a formatting pattern from the drop down list - Go to a page that uses Price formatting and verify that currency symbol displays wheere price formatting is applied (git grep '| $Price') (Amended patch to remove example opac-user.tt. To test with OPAC see bug 15374) Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Comment on attachment 45888 [details] [review] Bug 4078 - Display active currency symbol on currency output and input Review of attachment 45888 [details] [review]: ----------------------------------------------------------------- You will have to write tests. I don't think it's the good approach. What about the other cases? For instance : "-£127.54" vs "-127,54 €", you will have to add 3 values. I think that what you are trying to add with this patch should be done at the currency level (currency table), not the syspref. Actually the syspref should be removed. ::: Koha/Number/Price.pm @@ +79,4 @@ > > sub _format_params { > my ( $self, $params ) = @_; > + my $with_symbol = $params->{with_symbol}; You do you remove this default value, it does not look mandatory. @@ +96,5 @@ > + $p_cs_precedes = 1 unless ( defined $p_cs_precedes ); > + } > + > + if ( $currency_format =~ m/(_TRAIL)/ ) { > + $p_cs_precedes = 0 unless ( defined $p_cs_precedes ); You are mixing ternary operator in the conditions, it does not make the code easy to read. (In reply to Jonathan Druart from comment #20) > You do you remove this default value, it does not look mandatory. s/You/Why (In reply to Jonathan Druart from comment #20) > Comment on attachment 45888 [details] [review] [review] > (...) should be done at the currency level (currency table), not the syspref. > Actually the syspref should be removed. > (...) Hi Jonathan, your approach will be better, for sure. I remove assignement from this long outstanding bug and will be happy to sign-off your solution. I simply hope that we soon have the possibility to display currency symbols and more currency formatting than the US and French ones only, e.g. 360'000.00 for Switzerland. I am stealing this bug to add a new column to the currency table. It will allow to display or not a space between the symbol and the value. The problem we also have is that the with_symbol flag is not correctly handled in template (not passed from Koha::Template::Plugin::Price to Koha::Format::Price) Created attachment 71118 [details] [review] Bug 4078: DBIC Schema changes Created attachment 71119 [details] [review] Bug 4078: Update DB entry - add new column currency.p_sep_by_space Created attachment 71120 [details] [review] Bug 4078: Add the checkbox to the UI Created attachment 71121 [details] [review] Bug 4078: Pass with_symbol to module The template plugin did not pass the with_symbol flag to the module and so was not taken into account Created attachment 71122 [details] [review] Bug 4078: Add the ability to display and configure the symbol for prices The symbol of currencies are not displayed (or not correctly) so far. This patch set adds the ability to configure the display of the symbol (with or without a whitespace between the symbol and the price). Test plan: 1. Execute the update DB entry, go to the currency admin page and tick the new "Space separation between symbol and value" checkbox 2. Add a fine to a patron and use their credentials to login at the OPAC 3. You should see the "$ 42 due fines and charges" info in the dashboard 4. Untick the new checkbox to remove the space and reload the OPAC main page. => The space should not be longer displayed ($42) Created attachment 71123 [details] [review] Bug 4078: Add a new test Hi, I applied the patch, did an updatedatabase, restart_all and emptied cache. I am not seeing the checkbox on the currency admin page or any currency symbols in the OPAC. Am I missing a step still? (In reply to Claire Gravely from comment #30) > Hi, > > I applied the patch, did an updatedatabase, restart_all and emptied cache. I > am not seeing the checkbox on the currency admin page or any currency > symbols in the OPAC. Am I missing a step still? sorry, I do see the currency symbol in the OPAC, just not the checkbox to add a space. It's named "Space separation between symbol and value:" on cgi-bin/koha/admin/currency.pl?op=add_form¤cy_code=USD (when you edit a currency). Tested right now and I confirm it is displayed. Created attachment 72378 [details] [review] Bug 4078: DBIC Schema changes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 72379 [details] [review] Bug 4078: Update DB entry - add new column currency.p_sep_by_space Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 72380 [details] [review] Bug 4078: Add the checkbox to the UI Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 72381 [details] [review] Bug 4078: Pass with_symbol to module The template plugin did not pass the with_symbol flag to the module and so was not taken into account Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 72382 [details] [review] Bug 4078: Add the ability to display and configure the symbol for prices The symbol of currencies are not displayed (or not correctly) so far. This patch set adds the ability to configure the display of the symbol (with or without a whitespace between the symbol and the price). Test plan: 1. Execute the update DB entry, go to the currency admin page and tick the new "Space separation between symbol and value" checkbox 2. Add a fine to a patron and use their credentials to login at the OPAC 3. You should see the "$ 42 due fines and charges" info in the dashboard 4. Untick the new checkbox to remove the space and reload the OPAC main page. => The space should not be longer displayed ($42) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 72383 [details] [review] Bug 4078: Add a new test Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 72511 [details] [review] Bug 4078: DBIC Schema changes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 72512 [details] [review] Bug 4078: Update DB entry - add new column currency.p_sep_by_space Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 72513 [details] [review] Bug 4078: Add the checkbox to the UI Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 72514 [details] [review] Bug 4078: Pass with_symbol to module The template plugin did not pass the with_symbol flag to the module and so was not taken into account Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 72515 [details] [review] Bug 4078: Add the ability to display and configure the symbol for prices The symbol of currencies are not displayed (or not correctly) so far. This patch set adds the ability to configure the display of the symbol (with or without a whitespace between the symbol and the price). Test plan: 1. Execute the update DB entry, go to the currency admin page and tick the new "Space separation between symbol and value" checkbox 2. Add a fine to a patron and use their credentials to login at the OPAC 3. You should see the "$ 42 due fines and charges" info in the dashboard 4. Untick the new checkbox to remove the space and reload the OPAC main page. => The space should not be longer displayed ($42) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 72516 [details] [review] Bug 4078: Add a new test Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Passed QA. This area really need more work, but it is out of the scope of this bug report I think. Pushed to master for 18.05, thanks to everybody involved! Created attachment 73196 [details] [review] Bug 4078: Fix test when fr_FR.UTF-8 is installed (In reply to Jonathan Druart from comment #47) > Created attachment 73196 [details] [review] [review] > Bug 4078: Fix test when fr_FR.UTF-8 is installed Pushed to master. Enhancement, not backported for 17.11 |