Description
Jonathan Druart
2014-08-28 14:24:28 UTC
Created attachment 31258 [details] [review] Bug 12844: New module to manage prices into Koha This patch introduces a new module to manage prices into Koha and especially the acquisition module. How to use is: 1/ You can use it in a perl script/module: my $price = Koha::Number::Price->new(3); $price->format; # Will display 3.00 (or 3,00 depending on the CurrencyFormat syspref). $price->format({with_symbol => 1}); # Will display €3.00 (or [$]3,00 depending on the CurrencyFormat syspref). 2/ But this module is usefull to display the price from a template file. [% my_price | Price %] Created attachment 31259 [details] [review] Bug 12844: Use Koha::Number::Price where it can be useful This patch use the new module into pl and tt script. Note that we could use it in the acqui/pdfformat/layout*.pm files. Test plan: 1/ Verify that the acquisition home page displayes the prices as before. 2/ Verify that the budgets page displayes the prices as before. 3/ Verify that the funds page displayes the prices as before. 4/ Verify that the planning page displayes the prices as before. (Note that 1 price is now formatted: 'Fund remaining'). 5/ Create an order from a staged file. This stage file should contain a formatted price. Created attachment 31260 [details] [review] Bug 12844: Remove the C4::Output::FormatNumber subroutine This subroutine is now useless. Test plan: git grep FormatNumber should not return any result in pl and pm files. Could there be an option to the T:T plugin to allow for display of the symbol? Kind of like "|$KohaDates with_hours => 1" ? Yes of course, it's planned :) But I didn't understand some points (I highlighted them in the patch, search for FIXME): 1/ Why we don't use the symbol when the CurrencyFormat=='US'? 2/ If CurrencyFormat==FR, we use the symbol (actually, only in admin/aqbudgets.pl) but it's bad placed. I didn't find a way to put the symbol at the end of the string (€3.00 vs 3.00€, in France we use the second form). Small typo in: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/acqui-home.tt Line 81/82: </fonrm> instead of </form> (In reply to Jonathan Druart from comment #5) > Yes of course, it's planned :) > But I didn't understand some points (I highlighted them in the patch, search > for FIXME): > 1/ Why we don't use the symbol when the CurrencyFormat=='US'? I think maybe we should have a 3rd option to display without a currency symbol. Then people can keep the current display or switch to one of the nicer formats. Should the codes be language codes or would it make more sense to use some kind of description of the formatting? I guess formatting could be different by country, even if some countries use the same currency or different currencies but the same formatting. > 2/ If CurrencyFormat==FR, we use the symbol (actually, only in > admin/aqbudgets.pl) but it's bad placed. I didn't find a way to put the > symbol at the end of the string (€3.00 vs 3.00€, in France we use the second > form I think the same is true here - I'd normally write the symbol at the end. In overdue_notices.pl we use Locale::Currency::Format to format the fine in the notices correctly. I am not sure if we should use this, but wanted to bring it up. Switching status for the tiny problem MARC found. (In reply to Katrin Fischer from comment #7) > (In reply to Jonathan Druart from comment #5) > > Yes of course, it's planned :) > > But I didn't understand some points (I highlighted them in the patch, search > > for FIXME): > > 1/ Why we don't use the symbol when the CurrencyFormat=='US'? > > I think maybe we should have a 3rd option to display without a currency > symbol. Then people can keep the current display or switch to one of the > nicer formats. I think the good way to do it is to remove the CurrencyFormat pref and manage the formatting options at the currency level. > Should the codes be language codes or would it make more sense to use some > kind of description of the formatting? I guess formatting could be different > by country, even if some countries use the same currency or different > currencies but the same formatting. Sorry, I don't understand, maybe you are suggesting the same thing as me. > Switching status for the tiny problem MARC found. New patch coming. Created attachment 31304 [details] [review] Bug 12844: Use Koha::Number::Price where it can be useful This patch use the new module into pl and tt script. Note that we could use it in the acqui/pdfformat/layout*.pm files. Test plan: 1/ Verify that the acquisition home page displayes the prices as before. 2/ Verify that the budgets page displayes the prices as before. 3/ Verify that the funds page displayes the prices as before. 4/ Verify that the planning page displayes the prices as before. (Note that 1 price is now formatted: 'Fund remaining'). 5/ Create an order from a staged file. This stage file should contain a formatted price. Hi Jonathan, I think we are talking about the same, only I am not sure if we can tie it to a currency or to the country Koha is used in. I wonder how we (in Europe) would write a Dollar price for example - would we use a decimal point or a comma? Do you stick to what is custom in the country or do you use the format you are used to? But: if it's a plugin... we can improve on it in the future much easier and add new formats :) So - yay! (In reply to Katrin Fischer from comment #10) > Hi Jonathan, > > I think we are talking about the same, only I am not sure if we can tie it > to a currency or to the country Koha is used in. I wonder how we (in Europe) > would write a Dollar price for example - would we use a decimal point or a > comma? > Do you stick to what is custom in the country or do you use the format you > are used to? Hum... I did not really considered the question :) I thought we use the format used in the country (so $3.00). But actually I did not know! > But: if it's a plugin... we can improve on it in the future much easier and > add new formats :) So - yay! Yes, at the moment these patches should not introduce any changes (except 1 small improvement, see commit message) compared to master. Back to needs signoff. Hello I'll say that the choice of the decimal mark has nothing to do with a specific currency. In France, you should write 1 000,45 euros, like 1 000,45 dollars or 1 000,45 kilometers. Interesting page: http://en.wikipedia.org/wiki/Decimal_mark#Examples_of_use Mathieu Saby Hi Mathieu - thx for the link! In Germany I think most commonly we'd use . for better readability - 100.000,00. So having the formatting separate from currencies and in a preference is probably the easiest way to achieve some flexibility here. I agree with Katrin: separate formatting in preference. In Switzerland it would be 100'000.00 (Euro or $ or CHF or whatsoever) I've applied the patches against master 3.17.00.023 1) On the the planning page, the 'Fund remaining' is not formatted anymore after having auto-filled the row. In effect no value but Total is formatted in the row. 2) Ordering from a staged file, after having "selected from import" a record-biblio, and filled the "Price" field by value 1000, and selected "Save", this software error occurred: Can't locate object method "new" via package "Koha::Number::Price" (perhaps you forgot to load "Koha::Number::Price"?) at /var/root-koha/bug-7162/acqui/addorderiso2709.pl line 229. On the contrary, ordering from a staged file, after having "selected from import" a record-biblio, and used "Add order" option, no error occurred. In the "Accounting details" of the order, the price is set well (get from 020$c in the case I tested) and well formatted. [Setting CurrencyFormat pref to "360 000,00 (FR)" the results were the same.] So I pass the patch to "Failed QA" status. NB. Ordering from a staged file, various "Item information" fields were badly filled (like HASH(0x63f4340)). I think this error could be out of this bug. Created attachment 31618 [details] [review] Bug 12844: FIX import package missing (In reply to Paola Rossi from comment #15) > I've applied the patches against master 3.17.00.023 > > 1) On the the planning page, the 'Fund remaining' is not formatted anymore > after having auto-filled the row. > In effect no value but Total is formatted in the row. I don't understand. It seems that the value is formatted *with* the patch (and not without) > 2) Ordering from a staged file, after having "selected from import" a > record-biblio, and filled the "Price" field by value 1000, and selected > "Save", this software error occurred: Fixed with the last patch. > NB. Ordering from a staged file, various "Item information" fields were > badly filled (like HASH(0x63f4340)). I think this error could be out of this > bug. Yes, it comes from bug 12673. I already let a note. (In reply to Jonathan Druart from comment #17) > > > > 1) On the the planning page, the 'Fund remaining' is not formatted anymore > > after having auto-filled the row. > > In effect no value but Total is formatted in the row. > > I don't understand. It seems that the value is formatted *with* the patch > (and not without) > I beg your pardon, I was not clear. Be CurrencyFormat set to "360,000.00 (US)". The patch were already been applied. Before using "Auto-fill row" option, I saw f.e.: [OK] Fund total 100,000.00 [OK] Fund remaining 100,000.00 After having selected "Auto-fill row" option, and before saving, p.e. on planning by months I saw (on a 4-months budget) : [OK] Fund total 100,000.00 2014-09 25000.00 [instead of 25,000.00, which I supposed be right] ... [OK] Fund remaining 00.00 Now, if I set September to 0, the "Fund remaining" was : 25000.00 instead of 25,000.00, and I thought it was not the required "Fund remaining". Afterwards I've saved the planning, and then I saw that the "Fund remaining" was displayed well, as required. (In reply to Paola Rossi from comment #18) I am not sure but I think the behavior is better than before. Before this patch the 'fund remaining' was never well formatted. Now it is, but only on loading the page (after some JS processing, the formatting introduced by the new module cannot be called). This is the only change this patch set introduces (see commit message "Note that 1 price is now formatted: 'Fund remaining'"). Do you think this is blocker? Should I reintroduce the same behavior as before this change? (In reply to Jonathan Druart from comment #19) > (In reply to Paola Rossi from comment #18) > > I am not sure but I think the behavior is better than before. surely, it is > Before this patch the 'fund remaining' was never well formatted. Now it is, > but only on loading the page yes, imo it's exactly so > > This is the only change this patch set introduces (see commit message "Note > that 1 price is now formatted: 'Fund remaining'"). yes, ok > Do you think this is blocker? No, I think this is not blocker at all! Thank you, Jonathan. Everything has been done for the best. Created attachment 31624 [details] [review] Bug 12844: New module to manage prices into Koha I've applied the patches against master 3.17.00.023 I pass the patch to "Signed Off" status. Created attachment 31625 [details] [review] Bug 12844: Remove the C4::Output::FormatNumber subroutine Created attachment 31626 [details] [review] Bug 12844: Use Koha::Number::Price where it can be useful Created attachment 31627 [details] [review] Bug 12844: FIX import package missing Created attachment 31813 [details] [review] Bug 12844: FIX import package missing Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Created attachment 31826 [details] [review] Bug 12844: FIX import package missing Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Note for QA: Please don't consider that removing the currency symbol on the aqbudgets page if CurrencyFormat == 'FR' could be blocker. All patches above this patch won't apply and will break the display if added. I will fix this minor issue and homogenize prices formatting later. The tests are failing on my computer, copying the output here: bumblebee:~/kohaclone (15-12844-priceformat) $ prove t/Number/Price.t t/Number/Price.t .. 1/19 # Failed test 'FR: format 0 with symbol' # at t/Number/Price.t line 60. # got: '0,00 €' # expected: '€0,00' # Failed test 'FR: format 3 with symbol' # at t/Number/Price.t line 62. # got: '3,00 €' # expected: '€3,00' # Failed test at t/Number/Price.t line 64. # got: '1 234 567 890,00 €' # expected: '€1 234 567 890,00' # Looks like you failed 3 tests of 19. t/Number/Price.t .. Dubious, test returned 3 (wstat 768, 0x300) Failed 3/19 subtests Test Summary Report ------------------- t/Number/Price.t (Wstat: 768 Tests: 19 Failed: 3) Failed tests: 14-16 Non-zero exit status: 3 Created attachment 32211 [details] [review] Bug 12844: Force the symbol place Looking with Katrin at the default configuration of Number::Format, she has the p_cs_precedes value set to 0 (put the symbol at the end). The tests should mock this value in order to pass on all configuration. This default value for this variable certainly depends on the locales. This seems to make the tests pass on my system: sub _format_params { my ( $self, $params ) = @_; my $with_symbol = $params->{with_symbol} || 0; my $currency = GetCurrency(); my $currency_format = C4::Context->preference("CurrencyFormat"); my $int_curr_symbol = q||; my %format_params = ( int_curr_symbol => $int_curr_symbol, mon_thousands_sep => ',', mon_decimal_point => '.', + p_cs_precedes => 1, + p_sep_by_space => 0, ); if ( $currency_format eq 'FR' ) { # FIXME This test should be done for all currencies $int_curr_symbol = $currency->{symbol} if $with_symbol; %format_params = ( decimal_fill => '2', decimal_point => ',', int_curr_symbol => $int_curr_symbol, mon_thousands_sep => ' ', thousands_sep => ' ', + mon_decimal_point => ',', + p_cs_precedes => 1, p_sep_by_space => 0, ); } Created attachment 32229 [details] [review] Bug 12844: Force the symbol place in the tests Looking with Katrin at the default configuration of Number::Format, she has the p_cs_precedes value set to 0 (put the symbol at the end) and p_sep_by_sep set to 1. Now it is possible to sent these values to the format subroutine. On this way, the tests can force them in order to pass on all configuration. This default value for this variable certainly depends on the locales. Created attachment 32314 [details] [review] [PASSED QA] Bug 12844: New module to manage prices into Koha This patch introduces a new module to manage prices into Koha and especially the acquisition module. How to use is: 1/ You can use it in a perl script/module: my $price = Koha::Number::Price->new(3); $price->format; # Will display 3.00 (or 3,00 depending on the CurrencyFormat syspref). $price->format({with_symbol => 1}); # Will display €3.00 (or [$]3,00 depending on the CurrencyFormat syspref). 2/ But this module is usefull to display the price from a template file. [% my_price | Price %] Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 32315 [details] [review] [PASSED QA] Bug 12844: Remove the C4::Output::FormatNumber subroutine This subroutine is now useless. Test plan: git grep FormatNumber should not return any result in pl and pm files. Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 32316 [details] [review] [PASSED QA] Bug 12844: Use Koha::Number::Price where it can be useful This patch use the new module into pl and tt script. Note that we could use it in the acqui/pdfformat/layout*.pm files. Test plan: 1/ Verify that the acquisition home page displayes the prices as before. 2/ Verify that the budgets page displayes the prices as before. 3/ Verify that the funds page displayes the prices as before. 4/ Verify that the planning page displayes the prices as before. (Note that 1 price is now formatted: 'Fund remaining'). 5/ Create an order from a staged file. This stage file should contain a formatted price. Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 32317 [details] [review] [PASSED QA] Bug 12844: FIX import package missing Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 32318 [details] [review] [PASSED QA] Bug 12844: Force the symbol place in the tests Looking with Katrin at the default configuration of Number::Format, she has the p_cs_precedes value set to 0 (put the symbol at the end) and p_sep_by_sep set to 1. Now it is possible to sent these values to the format subroutine. On this way, the tests can force them in order to pass on all configuration. This default value for this variable certainly depends on the locales. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> I like about this, that it centralized the code and removes a lot of formatting code from the perl files. Thx Jonathan! Patch pushed to master. Thanks Jonathan! |