Bug 32595 - Price format cost in ILL requests table
Summary: Price format cost in ILL requests table
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Pedro Amorim
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-01-09 13:29 UTC by Katrin Fischer
Modified: 2023-12-12 08:44 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 32595: Add price formatting to cost and price paid fields in ILL list table (1.76 KB, patch)
2023-08-23 16:53 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32595: Add price formatting to cost and price paid fields in ILL list table (1.81 KB, patch)
2023-08-24 12:31 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 32595: Add price formatting to cost and price paid fields in ILL list table (1.81 KB, patch)
2023-08-25 08:42 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32595: Add price formatting to cost and price paid fields in ILL list table (1.96 KB, patch)
2023-08-25 09:22 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32595: Update price handling in UI edit form (2.13 KB, patch)
2023-08-25 09:22 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32595: Add price formatting to cost and price paid fields in ILL list table (2.01 KB, patch)
2023-10-22 18:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 32595: Update price handling in UI edit form (2.18 KB, patch)
2023-10-22 18:53 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2023-01-09 13:29:41 UTC
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.
Comment 1 Martin Renvoize 2023-06-22 16:12:40 UTC
Is this still the case?
Comment 2 Pedro Amorim 2023-08-23 16:53:46 UTC
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'
Comment 3 Owen Leonard 2023-08-24 12:31:40 UTC
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>
Comment 4 Pedro Amorim 2023-08-25 08:42:40 UTC
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>
Comment 5 Pedro Amorim 2023-08-25 08:45:03 UTC Comment hidden (obsolete)
Comment 6 Pedro Amorim 2023-08-25 08:45:37 UTC
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.
Comment 7 Pedro Amorim 2023-08-25 09:22:14 UTC
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>
Comment 8 Pedro Amorim 2023-08-25 09:22:17 UTC
Created attachment 154788 [details] [review]
Bug 32595: Update price handling in UI edit form
Comment 9 Pedro Amorim 2023-08-25 09:23:53 UTC
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.
Comment 10 Pedro Amorim 2023-08-25 09:25:02 UTC
(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.
Comment 11 Katrin Fischer 2023-08-25 09:50:28 UTC
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 :)
Comment 12 Pedro Amorim 2023-08-25 10:10:44 UTC
(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?
Comment 13 David Nind 2023-10-22 18:53:46 UTC
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>
Comment 14 David Nind 2023-10-22 18:53:49 UTC
Created attachment 157623 [details] [review]
Bug 32595: Update price handling in UI edit form

Signed-off-by: David Nind <david@davidnind.com>
Comment 15 David Nind 2023-10-22 19:00:56 UTC
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
Comment 16 Katrin Fischer 2023-11-01 11:56:36 UTC
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.