Bug 19479 - Price display on a basketgroup
Summary: Price display on a basketgroup
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jon Knight
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 4078
Blocks:
  Show dependency treegraph
 
Reported: 2017-10-18 09:05 UTC by Séverine Queune
Modified: 2018-07-18 11:09 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Patch to format totals according to the CurrencyFormat syspref. (1021 bytes, patch)
2017-10-18 17:03 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 19479: Patch to format totals according to the CurrencyFormat syspref. (1.41 KB, patch)
2017-10-30 15:58 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 19479: Patch to format totals according to the CurrencyFormat syspref. (1.61 KB, patch)
2017-11-06 13:17 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Attempt to display price formatted according to CurrencyFormat syspref using templates, with the English template as an example. (3.82 KB, patch)
2018-02-07 19:16 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 19479 - Display prices in basket group with correct currency formatting (3.82 KB, patch)
2018-02-07 19:17 UTC, Jon Knight
Details | Diff | Splinter Review
Attempt to display price formatted according to CurrencyFormat syspref using templates, with the English template as an example. (4.11 KB, patch)
2018-03-26 18:09 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 19479: Display price in basked group formatted according to CurrencyFormat syspref using templates. (4.49 KB, patch)
2018-03-26 18:20 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 19479: Display price in basket group formatted according to CurrencyFormat syspref using templates and currency symbols. (4.57 KB, patch)
2018-03-28 09:43 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 19479: Display price in basket group formatted according to CurrencyFormat syspref using templates and currency symbols. (4.58 KB, patch)
2018-03-28 09:54 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 19479: Display price in basket group formatted according to CurrencyFormat syspref using templates and currency symbols. (4.56 KB, patch)
2018-03-28 11:53 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 19479: Display price in basket group formatted according to CurrencyFormat syspref using templates and currency symbols. (4.70 KB, patch)
2018-03-30 14:53 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 19479: Display price in basket group formatted according to CurrencyFormat syspref using templates. (4.41 KB, patch)
2018-04-05 08:51 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 19479: Display price in basket group formatted according to CurrencyFormat syspref using templates. (4.50 KB, patch)
2018-04-16 20:34 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 19479: Display price in basket group formatted according to CurrencyFormat syspref using templates. (4.64 KB, patch)
2018-04-16 22:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19479: Display price in basket group formatted according to CurrencyFormat syspref using templates. (3.58 KB, patch)
2018-04-23 15:42 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Séverine Queune 2017-10-18 09:05:27 UTC
On the page acqui/basketgroup.pl, the prices of the baskets are always displayed with a point as separator and 4 decimals.

It would be clearer for users to  display only 2 decimals and use the separator chosen on the syspref 'CurrencyFormat'.
Comment 1 Jon Knight 2017-10-18 17:03:40 UTC
Created attachment 68276 [details] [review]
Patch to format totals according to the CurrencyFormat syspref.
Comment 2 Séverine Queune 2017-10-18 18:11:53 UTC
Wow.. that was very quick !
Tested with the 3 valeus in Currencyformat, it always worked.
Thanks a lot !
Comment 3 Séverine Queune 2017-10-19 08:29:47 UTC
I tried to sign the patch using the Biblibre's sandbox #3 yesterday and this morning but always get this message :
"It seems you don't have applied a patch, so you cannot sign it off. If you applied patches from the right report, check the commit message of the last patch. It should start with "Bug XXXXX", if not, please inform the author of the patch."
Comment 4 Jon Knight 2017-10-30 15:58:48 UTC
Created attachment 68849 [details] [review]
Bug 19479: Patch to format totals according to the CurrencyFormat syspref.

On the page acqui/basketgroup.pl the prices of baskets should be shown
according to the selected CurrencyFormat syspref.

Test plan:

1) Create some baskets with items in them.
2) Go to acqui/basketgroup.pl page and check that the price format matches
   the current CurrencyFormat syspref.
3) Go to Administration and change CurrencyFormat syspref to one of the other
   available options and recheck step 2.
Comment 5 Jon Knight 2017-10-30 16:00:02 UTC
Patch commit message updated to match formatting requirements in https://wiki.koha-community.org/wiki/Commit_messages
Comment 6 sandboxes@biblibre.com 2017-11-06 13:17:11 UTC
Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>
Comment 7 sandboxes@biblibre.com 2017-11-06 13:17:31 UTC
Created attachment 68972 [details] [review]
Bug 19479: Patch to format totals according to the CurrencyFormat syspref.

On the page acqui/basketgroup.pl the prices of baskets should be shown
according to the selected CurrencyFormat syspref.

Test plan:

1) Create some baskets with items in them.
2) Go to acqui/basketgroup.pl page and check that the price format matches
   the current CurrencyFormat syspref.
3) Go to Administration and change CurrencyFormat syspref to one of the other
   available options and recheck step 2.

https://bugs.koha-community.org/show_bug.cgi?id=19479

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 8 Jonathan Druart 2018-01-10 13:35:29 UTC
The price formatting must be done template side, search for '$Price' or '[% USE Price %]' in **/*.tt
Comment 9 Jon Knight 2018-02-07 19:16:16 UTC
Created attachment 71312 [details] [review]
Attempt to display price formatted according to CurrencyFormat syspref using templates, with the English template as an example.

1) Create some baskets with items in them.
2) Go to acqui/basketgroup.pl page and check that the price format matches
   the current CurrencyFormat syspref.
3) Go to Administration and change CurrencyFormat syspref to one of the other
   available options and recheck step 2.
Comment 10 Jon Knight 2018-02-07 19:17:31 UTC
Created attachment 71313 [details] [review]
Bug 19479 - Display prices in basket group with correct currency formatting

Attempt to display price formatted according to CurrencyFormat syspref using templates, with the English template as an example.

1) Create some baskets with items in them.
2) Go to acqui/basketgroup.pl page and check that the price format matches
   the current CurrencyFormat syspref.
3) Go to Administration and change CurrencyFormat syspref to one of the other
   available options and recheck step 2.
Comment 11 Jon Knight 2018-02-07 19:19:47 UTC
One thing to note is that if I use the templates rather than my original patch that just altered basketgroup.pl to do the formatting, I'm guessing that each language that Koha is in will also need its basketgroup.tt template updated? Is that right?  I've only done the en (default English) one in this current patch.
Comment 12 Katrin Fischer 2018-02-07 19:27:36 UTC
(In reply to Jon Knight from comment #11)
> One thing to note is that if I use the templates rather than my original
> patch that just altered basketgroup.pl to do the formatting, I'm guessing
> that each language that Koha is in will also need its basketgroup.tt
> template updated? Is that right?  I've only done the en (default English)
> one in this current patch.

Thx for thinking about translations :) But you are fine - the translated templates are generated automatically from the English templates with scripts. Changing the English 'master' template is enough.
Comment 13 Jonathan Druart 2018-02-13 20:10:28 UTC
You should base your patch on top of bug 4078 instead.
Comment 14 Jon Knight 2018-03-14 15:19:08 UTC
I'll wait until 4078 is in master (its Passed QA at the moment) before changing this patch again just so that I'm aiming for a stable target!
Comment 15 Jon Knight 2018-03-26 18:09:39 UTC
Created attachment 73290 [details] [review]
Attempt to display price formatted according to CurrencyFormat syspref using templates, with the English template as an example.
Comment 16 Jon Knight 2018-03-26 18:20:02 UTC
Created attachment 73291 [details] [review]
Bug 19479: Display price in basked group formatted according to CurrencyFormat syspref using templates.

On the page acqui/basketgroup.pl the prices of baskets should be shown
formatted according to the selected CurrencyFormat syspref.

Test plan:

1) Create a basket with items in it worth more than 1000 currency units.
2) Close the basket.
3) Go to acqui/basketgroup.pl page and check that the price format matches
   the current CurrencyFormat syspref.
4) Go to Administration and change CurrencyFormat syspref to one of the
   other available options and recheck step 3.
Comment 17 M. Tompsett 2018-03-26 21:57:19 UTC
It seems to me the more I read code, the more I wish to cry over the state of Currency code. While it may be the current methodology to grab the active currency, it seems to me that baskets should have their own currency, and the currency grabbed should be from that. However, I don't have time to refactor all currency code. :(
Comment 18 Jonathan Druart 2018-03-27 18:07:44 UTC
Jon,

  Total: [% selectedbasket.total | $Price %] ([% selectedbasket.currency %])
Will display: 42 (EUR) or 42 (USD)

With bug 4078 you can use the with_symbol flag to display the symbol.
  Total: [% selectedbasket.total | $Price with_symbol => 1
Will display: €42 or $42

I think it is what we want here.

Actually what we really want is: "42 €" and "$42", but it's not configurable (yet).
Comment 19 Jon Knight 2018-03-28 09:43:12 UTC
Created attachment 73376 [details] [review]
Bug 19479: Display price in basket group formatted according to CurrencyFormat syspref using templates and currency symbols.

On the page acqui/basketgroup.pl the prices of baskets should be shown
formatted according to the selected CurrencyFormat syspref, using the
currency symbol rather than three letter currency code.

Test plan:

1) Create a basket with items in it worth more than 1000 currency units.
2) Close the basket.
3) Go to acqui/basketgroup.pl page and check that the price format matches
   the current CurrencyFormat syspref.
4) Go to Administration and change CurrencyFormat syspref to one of the
   other available options and recheck step 3.
Comment 20 Jon Knight 2018-03-28 09:46:52 UTC
The most recently uploaded version of this patch uses the currency symbols instead of the currency code.  I've not done anything about per basket currency defaults as I think that is a separate, larger change to this (which was supposed to be a simple formatting change).  That really needs a new bug report (and probably someone other than me who understands the nuances of how libraries want to handle multiple currency international orders, as the items, baskets and basketgroups would all be affected).
Comment 21 Jon Knight 2018-03-28 09:54:01 UTC
Created attachment 73377 [details] [review]
Bug 19479: Display price in basket group formatted according to CurrencyFormat syspref using templates and currency symbols.

On the page acqui/basketgroup.pl the prices of baskets should be shown
formatted according to the selected CurrencyFormat syspref, using the
currency symbol rather than three letter currency code.

Test plan:

1) Create a basket with items in it worth more than 1000 currency units.
2) Close the basket.
3) Go to acqui/basketgroup.pl page and check that the price format matches
   the current CurrencyFormat syspref.
   4) Go to Administration and change CurrencyFormat syspref to one of the
      other available options and recheck step 3.
Comment 22 Jon Knight 2018-03-28 11:53:45 UTC
Created attachment 73379 [details] [review]
Bug 19479: Display price in basket group formatted according to CurrencyFormat syspref using templates and currency symbols.

On the page acqui/basketgroup.pl the prices of baskets should be shown
formatted according to the selected CurrencyFormat syspref, using the
currency symbol rather than three letter currency code.

Test plan:

1) Create a basket with items in it worth more than 1000 currency units.
2) Close the basket.
3) Go to acqui/basketgroup.pl page and check that the price format matches
   the current CurrencyFormat syspref.
4) Go to Administration and change CurrencyFormat syspref to one of the
   other available options and recheck step 3.
Comment 23 Séverine Queune 2018-03-30 14:53:08 UTC
Created attachment 73491 [details] [review]
Bug 19479: Display price in basket group formatted according to CurrencyFormat syspref using templates and currency symbols.

On the page acqui/basketgroup.pl the prices of baskets should be shown
formatted according to the selected CurrencyFormat syspref, using the
currency symbol rather than three letter currency code.

Test plan:

1) Create a basket with items in it worth more than 1000 currency units.
2) Close the basket.
3) Go to acqui/basketgroup.pl page and check that the price format matches
   the current CurrencyFormat syspref.
4) Go to Administration and change CurrencyFormat syspref to one of the
   other available options and recheck step 3.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 24 Séverine Queune 2018-03-30 14:57:21 UTC
The 3 formats defined on 'CurrencyFormat' correctly apply with this patch.
Maybe for a better visibility (especially for 'FR format') we could have a space between the currency and the prices, but excepted that detail, everything looked good for me.
Thank you Jon for rebase !
Comment 25 Katrin Fischer 2018-03-31 11:01:26 UTC
Hi Séverine, there is no a setting in administration > currency for each currency, where you can define if there should be a space or not.

Hi Jon, thank you for your perseverance! 2 small things:

If this line is no longer needed, please remove it, instead of commenting out:
+#    $total .= " " . ($bookseller->invoiceprice // 0);

I am torn about seeing the currency. I think we agreed a long while ago that we should not show the currency symbols in acq as long as it's the active currency as it would clutter a lot of the displays, where you deal with a lot of numbers and prices. Would you agree to remove the currency symbol display?
 $Price with_symbol => 1 

Please switch back to signed off if you add a follow up patch.
Comment 26 Jon Knight 2018-03-31 11:43:11 UTC
(In reply to Katrin Fischer from comment #25)
> Hi Séverine, there is no a setting in administration > currency for each
> currency, where you can define if there should be a space or not.
> 
> I am torn about seeing the currency. I think we agreed a long while ago that
> we should not show the currency symbols in acq as long as it's the active
> currency as it would clutter a lot of the displays, where you deal with a
> lot of numbers and prices. Would you agree to remove the currency symbol
> display?
>  $Price with_symbol => 1 

That's how I had it before Jonathan Druart asked me to put the symbols in place in comment #18 above. I don't mind either way as long as the QA team and RM can agree (or do I have to add in a syspref for this? :-) ).

I'll await a decision before changing this simple patch again.
Comment 27 Jonathan Druart 2018-04-02 14:55:54 UTC
(In reply to Jon Knight from comment #26)
> (In reply to Katrin Fischer from comment #25)
> > Hi Séverine, there is no a setting in administration > currency for each
> > currency, where you can define if there should be a space or not.
> > 
> > I am torn about seeing the currency. I think we agreed a long while ago that
> > we should not show the currency symbols in acq as long as it's the active
> > currency as it would clutter a lot of the displays, where you deal with a
> > lot of numbers and prices. Would you agree to remove the currency symbol
> > display?
> >  $Price with_symbol => 1 
> 
> That's how I had it before Jonathan Druart asked me to put the symbols in
> place in comment #18 above. I don't mind either way as long as the QA team
> and RM can agree (or do I have to add in a syspref for this? :-) ).
> 
> I'll await a decision before changing this simple patch again.

IIRC you displayed the currency code :)
Comment 28 Katrin Fischer 2018-04-02 20:08:52 UTC
> That's how I had it before Jonathan Druart asked me to put the symbols in
> place in comment #18 above. I don't mind either way as long as the QA team
> and RM can agree (or do I have to add in a syspref for this? :-) ).
> 
> I'll await a decision before changing this simple patch again.

I talked to Joubu and removing the currency symbols will be safe ;)
Comment 29 Jon Knight 2018-04-05 08:51:20 UTC
Created attachment 73677 [details] [review]
Bug 19479: Display price in basket group formatted according to CurrencyFormat syspref using templates.

On the page acqui/basketgroup.pl the prices of baskets should be shown
formatted according to the selected CurrencyFormat syspref, with no
currency symbol or code.

Test plan:

1) Create a basket with items in it worth more than 1000 currency units.
2) Close the basket.
3) Go to acqui/basketgroup.pl page and check that the price format matches
   the current CurrencyFormat syspref.
4) Go to Administration and change CurrencyFormat syspref to one of the
   other available options and recheck step 3.
Comment 30 Katrin Fischer 2018-04-09 06:07:53 UTC
Hi Jon, if QA demands a follow-up what you can usually do is make a small extra patch with the additional changes and attach it. In this case it would have just been removing the attribute. This way you can keep the sign-off on your first patch and the signed-off status and it goes directly back to QA. Do you want to change it and restore the signed off patch with a little addition?
Comment 31 Jon Knight 2018-04-09 06:38:49 UTC
(In reply to Katrin Fischer from comment #30)
> Hi Jon, if QA demands a follow-up what you can usually do is make a small
> extra patch with the additional changes and attach it. In this case it would
> have just been removing the attribute. This way you can keep the sign-off on
> your first patch and the signed-off status and it goes directly back to QA.
> Do you want to change it and restore the signed off patch with a little
> addition?

No, because the last patch changed the behaviour (again), rather than just being a small change to get rid of the commented out line.  That really needs someone to check that it is what people actually want and sign that off (again).
Comment 32 Katrin Fischer 2018-04-09 08:07:26 UTC
Ok, if you prefer another set of eyes, I am not against that at all :)
Comment 33 Séverine Queune 2018-04-16 20:34:33 UTC
Created attachment 74274 [details] [review]
Bug 19479: Display price in basket group formatted according to CurrencyFormat syspref using templates.

On the page acqui/basketgroup.pl the prices of baskets should be shown
formatted according to the selected CurrencyFormat syspref, with no
currency symbol or code.

Test plan:

1) Create a basket with items in it worth more than 1000 currency units.
2) Close the basket.
3) Go to acqui/basketgroup.pl page and check that the price format matches
   the current CurrencyFormat syspref.
4) Go to Administration and change CurrencyFormat syspref to one of the
   other available options and recheck step 3.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 34 Séverine Queune 2018-04-16 20:41:10 UTC
This price display seems clearer to me even though we don't have currency symbols.
As user, this patch is good for me :)
Comment 35 Katrin Fischer 2018-04-16 22:21:44 UTC
Created attachment 74284 [details] [review]
Bug 19479: Display price in basket group formatted according to CurrencyFormat syspref using templates.

On the page acqui/basketgroup.pl the prices of baskets should be shown
formatted according to the selected CurrencyFormat syspref, with no
currency symbol or code.

Test plan:

1) Create a basket with items in it worth more than 1000 currency units.
2) Close the basket.
3) Go to acqui/basketgroup.pl page and check that the price format matches
   the current CurrencyFormat syspref.
4) Go to Administration and change CurrencyFormat syspref to one of the
   other available options and recheck step 3.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Fixed some tabs.

https://bugs.koha-community.org/show_bug.cgi?id=19497
Comment 36 Katrin Fischer 2018-04-16 22:23:50 UTC
Thx for your patience and perseverance, Jon!

I've fixed a few tabs in basketgroup.pl, be careful: Koha uses 4 spaces for indenting.
Comment 37 Jonathan Druart 2018-04-19 19:37:07 UTC
So we should remove the 2x2 lines you add in the .pl to retrieve the active currency, right?
Comment 38 Jon Knight 2018-04-23 15:40:26 UTC
Ah that's a good point. Do you want me to generate a new patch without them?
Comment 39 Jonathan Druart 2018-04-23 15:41:12 UTC
(In reply to Jon Knight from comment #38)
> Ah that's a good point. Do you want me to generate a new patch without them?

I will do it.
Comment 40 Jonathan Druart 2018-04-23 15:42:20 UTC
Created attachment 74739 [details] [review]
Bug 19479: Display price in basket group formatted according to CurrencyFormat syspref using templates.

On the page acqui/basketgroup.pl the prices of baskets should be shown
formatted according to the selected CurrencyFormat syspref, with no
currency symbol or code.

Test plan:

1) Create a basket with items in it worth more than 1000 currency units.
2) Close the basket.
3) Go to acqui/basketgroup.pl page and check that the price format matches
   the current CurrencyFormat syspref.
4) Go to Administration and change CurrencyFormat syspref to one of the
   other available options and recheck step 3.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Fixed some tabs.
Comment 41 Jonathan Druart 2018-04-23 17:29:10 UTC
Pushed to master for 18.05, thanks to everybody involved!