The table with all ILL requests on the start page of the ILL module shows the Cost as 4.00 indepent of the PriceFormat system preference. It needs to have price formatting applied.
Is this still the case?
Created attachment 154717 [details] [review] Bug 32595: Add price formatting to cost and price paid fields in ILL list table Test plan: 1) Enable ILLmodule and install FreeForm, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) 2) Create a new FreeForm request and edit its price on the manage page to something like '123' 3) Check back the ILL list table and confirm its shows as '123' 4) Apply patch 5) Verify the table again, see that it now shows as '123.00'
Created attachment 154731 [details] [review] Bug 32595: Add price formatting to cost and price paid fields in ILL list table Test plan: 1) Enable ILLmodule and install FreeForm, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) 2) Create a new FreeForm request and edit its price on the manage page to something like '123' 3) Check back the ILL list table and confirm its shows as '123' 4) Apply patch 5) Verify the table again, see that it now shows as '123.00' Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 154780 [details] [review] Bug 32595: Add price formatting to cost and price paid fields in ILL list table Test plan: 1) Enable ILLmodule and install FreeForm, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) 2) Create a new FreeForm request and edit its price on the manage page to something like '123' 3) Check back the ILL list table and confirm its shows as '123' 4) Apply patch 5) Verify the table again, see that it now shows as '123.00' Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Changed parseInt to Number, parseInt was ignoring float values (i.e. if price was 123,5 it would show as 123,00 on the table). However there is still a deeper issue here I think in the way the price is saved. If we add "123,5" (with a comma instead of a dot) the table shows 0. I'm revisiting this.
Changed parseInt to Number, parseInt was ignoring float values (i.e. if price was 123.5 it would show as 123,00 on the table). However there is still a deeper issue here I think in the way the price is saved. If we add "123,5" (with a comma instead of a dot) the table shows 0. I'm revisiting this.
Created attachment 154787 [details] [review] Bug 32595: Add price formatting to cost and price paid fields in ILL list table Test plan: 1) Enable ILLmodule and install FreeForm, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) 2) Create a new FreeForm request and edit its price on the manage page to something like '123' 3) Check back the ILL list table and confirm its shows as '123' 4) Apply patch 5) Verify the table again, see that it now shows as '123.00' Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 154788 [details] [review] Bug 32595: Update price handling in UI edit form
Updated original patch to consider possibility of price data having commas in it and replacing that with a dot to convert to the data format expected by format_price.
(In reply to Owen Leonard from comment #3) > Created attachment 154731 [details] [review] [review] > Bug 32595: Add price formatting to cost and price paid fields in ILL list > table > > Test plan: > 1) Enable ILLmodule and install FreeForm, run: > bash <(curl -s > https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev. > sh) > 2) Create a new FreeForm request and edit its price on the manage page > to something like '123' > 3) Check back the ILL list table and confirm its shows as '123' > 4) Apply patch > 5) Verify the table again, see that it now shows as '123.00' > > Signed-off-by: Owen Leonard <oleonard@myacpl.org> Owen sorry would you please take a second look at this? I've updated the status to "Needs Signoff" because I've changed the code enough to justify it.
The changes on the edit form are great, but I wonder if we should do the replace or not put this into the backends 'hands'. It seems that there is a mistake/flaw in the architecture being that price_paid and cost are both varchar(20) instead of a decimal? The changes to the edit form I fully agree with :)
(In reply to Katrin Fischer from comment #11) Correct, I noticed that too. The replacing of , -> . is visual only and to ensure compatibility with existing data, because there's no telling what may exist currently on live databases. We could come up with an atomic update like: 1) UPDATE illrequests SET price_paid = CAST(REPLACE(price_paid, ',', ',') as DECIMAL(28,6)) 2) ALTER TABLE illrequests MODIFY price_paid DECIMAL(28,6) But this may cause issues as there may be non-numeric data in current databases?
Created attachment 157622 [details] [review] Bug 32595: Add price formatting to cost and price paid fields in ILL list table Test plan: 1) Enable ILLmodule and install FreeForm, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) 2) Create a new FreeForm request and edit its price on the manage page to something like '123' 3) Check back the ILL list table and confirm its shows as '123' 4) Apply patch 5) Verify the table again, see that it now shows as '123.00' Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 157623 [details] [review] Bug 32595: Update price handling in UI edit form Signed-off-by: David Nind <david@davidnind.com>
Have added my sign off, as things work as per the test plan. Feel free to change the status if anything more is required for comments #11 and #12
I still think we should not do this part here: let cost = row.cost && row.cost.replaceAll(',','.'); If the information wasn't correctly stored by the backend, we should not try to fix it like hat. It could possibly hide other issues, like calculations with those values would not work correctly.
How can we progress this one?
Can we perhaps fix incoming data to be a consistent form rather than fixing it as it comes out?
I think I was opposed to use this: let cost = row.cost && row.cost.replaceAll(',','.'); Don't we have a standard price formatting in JS now as well? I'd use that and assume the data is in the format Koha expects (decimal with dot) and that would also work for calculating in Perl.