Bug 12844

Summary: Introduce a centralized way to display prices
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: francois.charbonnier, katrin.fischer, mathsabypro, paola.rossi, tomascohen, veron
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10929
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9410
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 12826, 12830, 12979, 12987, 13684, 18774    
Attachments: Bug 12844: New module to manage prices into Koha
Bug 12844: Use Koha::Number::Price where it can be useful
Bug 12844: Remove the C4::Output::FormatNumber subroutine
Bug 12844: Use Koha::Number::Price where it can be useful
Bug 12844: FIX import package missing
Bug 12844: New module to manage prices into Koha
Bug 12844: Remove the C4::Output::FormatNumber subroutine
Bug 12844: Use Koha::Number::Price where it can be useful
Bug 12844: FIX import package missing
Bug 12844: FIX import package missing
Bug 12844: FIX import package missing
Bug 12844: Force the symbol place
Bug 12844: Force the symbol place in the tests
[PASSED QA] Bug 12844: New module to manage prices into Koha
[PASSED QA] Bug 12844: Remove the C4::Output::FormatNumber subroutine
[PASSED QA] Bug 12844: Use Koha::Number::Price where it can be useful
[PASSED QA] Bug 12844: FIX import package missing
[PASSED QA] Bug 12844: Force the symbol place in the tests

Description Jonathan Druart 2014-08-28 14:24:28 UTC
To display prices depending on the currency, Koha (and especially the acquisition module) needs a module to manage prices.
Comment 1 Jonathan Druart 2014-08-28 14:55:57 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-08-28 14:56:00 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2014-08-28 14:56:06 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2014-08-28 17:41:28 UTC
Could there be an option to the T:T plugin to allow for display of the symbol? Kind of like "|$KohaDates with_hours => 1" ?
Comment 5 Jonathan Druart 2014-08-29 07:34:17 UTC
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).
Comment 6 Marc Véron 2014-08-30 09:12:23 UTC
Small typo in:
koha-tmpl/intranet-tmpl/prog/en/modules/acqui/acqui-home.tt 
Line 81/82: </fonrm> instead of </form>
Comment 7 Katrin Fischer 2014-08-31 08:07:34 UTC
(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.
Comment 8 Jonathan Druart 2014-09-01 07:40:44 UTC
(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.
Comment 9 Jonathan Druart 2014-09-01 07:41:29 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2014-09-01 07:54:32 UTC
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!
Comment 11 Jonathan Druart 2014-09-01 08:13:26 UTC
(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.
Comment 12 Mathieu Saby 2014-09-01 22:32:49 UTC
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
Comment 13 Katrin Fischer 2014-09-01 22:39:04 UTC
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.
Comment 14 Marc Véron 2014-09-02 06:27:01 UTC
I agree with Katrin: separate formatting in preference.

In Switzerland it would be 100'000.00 (Euro or $ or CHF or whatsoever)
Comment 15 Paola Rossi 2014-09-15 15:17:56 UTC
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.
Comment 16 Jonathan Druart 2014-09-16 08:01:01 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2014-09-16 08:03:27 UTC
(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.
Comment 18 Paola Rossi 2014-09-16 10:17:31 UTC
(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.
Comment 19 Jonathan Druart 2014-09-16 10:56:10 UTC
(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?
Comment 20 Paola Rossi 2014-09-16 12:47:22 UTC
(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.
Comment 21 Paola Rossi 2014-09-16 12:49:59 UTC Comment hidden (obsolete)
Comment 22 Paola Rossi 2014-09-16 12:50:42 UTC Comment hidden (obsolete)
Comment 23 Paola Rossi 2014-09-16 12:51:17 UTC Comment hidden (obsolete)
Comment 24 Paola Rossi 2014-09-16 12:51:58 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2014-09-22 15:31:47 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2014-09-23 09:44:25 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2014-10-08 20:53:25 UTC
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.
Comment 28 Katrin Fischer 2014-10-10 20:19:26 UTC
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
Comment 29 Jonathan Druart 2014-10-10 20:51:08 UTC Comment hidden (obsolete)
Comment 30 Katrin Fischer 2014-10-10 21:16:17 UTC
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,

        );
    }
Comment 31 Jonathan Druart 2014-10-11 13:50:06 UTC Comment hidden (obsolete)
Comment 32 Katrin Fischer 2014-10-14 14:18:16 UTC
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>
Comment 33 Katrin Fischer 2014-10-14 14:18:21 UTC
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>
Comment 34 Katrin Fischer 2014-10-14 14:18:25 UTC
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>
Comment 35 Katrin Fischer 2014-10-14 14:18:29 UTC
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>
Comment 36 Katrin Fischer 2014-10-14 14:18:33 UTC
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>
Comment 37 Katrin Fischer 2014-10-14 14:20:05 UTC
I like about this, that it centralized the code and removes a lot of formatting code from the perl files. Thx Jonathan!
Comment 38 Tomás Cohen Arazi 2014-10-27 17:19:06 UTC
Patch pushed to master.
Comment 39 Tomás Cohen Arazi 2014-10-27 17:19:21 UTC
Thanks Jonathan!