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'.
Created attachment 68276 [details] [review] Patch to format totals according to the CurrencyFormat syspref.
Wow.. that was very quick ! Tested with the 3 valeus in Currencyformat, it always worked. Thanks a lot !
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."
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.
Patch commit message updated to match formatting requirements in https://wiki.koha-community.org/wiki/Commit_messages
Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>
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>
The price formatting must be done template side, search for '$Price' or '[% USE Price %]' in **/*.tt
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.
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.
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.
(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.
You should base your patch on top of bug 4078 instead.
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!
Created attachment 73290 [details] [review] Attempt to display price formatted according to CurrencyFormat syspref using templates, with the English template as an example.
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.
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. :(
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).
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.
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).
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.
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.
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>
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 !
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.
(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.
(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 :)
> 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 ;)
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.
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?
(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).
Ok, if you prefer another set of eyes, I am not against that at all :)
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>
This price display seems clearer to me even though we don't have currency symbols. As user, this patch is good for me :)
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
Thx for your patience and perseverance, Jon! I've fixed a few tabs in basketgroup.pl, be careful: Koha uses 4 spaces for indenting.
So we should remove the 2x2 lines you add in the .pl to retrieve the active currency, right?
Ah that's a good point. Do you want me to generate a new patch without them?
(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.
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.
Pushed to master for 18.05, thanks to everybody involved!