Bug 30139 - Point of sale sets wrong 'Amount being paid' with CurrencyFormat = FR
Summary: Point of sale sets wrong 'Amount being paid' with CurrencyFormat = FR
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Shi Yao Wang
QA Contact: Testopia
URL:
Keywords:
Depends on: 27801
Blocks: 30750
  Show dependency treegraph
 
Reported: 2022-02-19 20:58 UTC by Katrin Fischer
Modified: 2022-05-14 01:42 UTC (History)
6 users (show)

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


Attachments
Bug 30139: Fix javascript moneyFormat to be compatible with FR format (2.43 KB, patch)
2022-05-10 17:41 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30139: Fix javascript moneyFormat to be compatible with FR format (2.47 KB, patch)
2022-05-11 00:12 UTC, David Nind
Details | Diff | Splinter Review
Screenshot - after patch applied (101.41 KB, image/png)
2022-05-11 00:27 UTC, David Nind
Details
Bug 30139: Fix javascript moneyFormat to be compatible with FR format (2.53 KB, patch)
2022-05-13 09:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30139: Fix javascript moneyFormat to be compatible with FR format (4.78 KB, patch)
2022-05-13 12:49 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30139: Same fix for the other javascript moneyFormat function (2.14 KB, patch)
2022-05-13 14:22 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30139: Same fix for the other javascript moneyFormat function (2.23 KB, patch)
2022-05-13 14:35 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30139: Same fix for the other javascript moneyFormat function (2.29 KB, patch)
2022-05-13 14:50 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2022-02-19 20:58:58 UTC
With CurrencyFormat set to 'FR' (prices with decimal comma) the 'Amount being paid' on the Point for sale page will turn a price of 10.00 into 1000.00.

This makes it impossible to use the feature with CurrencyFormat FR.
Comment 1 Katrin Fischer 2022-02-19 21:00:27 UTC
To reproduce:

- Set CurrencyFormat pref to FR
- Activate Poit of sale and cash register features
- Add cash register
- Add debit type that 'can be sold' with a price of 10.00
- Go to point of sale, click on the item to purchase
- Verify the Amount to be paid shows the wrong amount and can't be fixed
Comment 2 Blou 2022-04-06 14:57:32 UTC
It is caused by Bug 27801.

I don't dare to suggest a modification since we had no problem prior to that fix, but obviously it was meant to fix something.
Comment 3 Martin Renvoize 2022-04-26 12:54:06 UTC
It's the interaction between format_price and moneyFormat.. moneyFormat appears to be hard coded to '.' decimal separators.. but then Koha's internals expect point decimal separators in inputs too.
Comment 4 Shi Yao Wang 2022-05-10 17:41:45 UTC
Created attachment 134830 [details] [review]
Bug 30139: Fix javascript moneyFormat to be compatible with FR format

New way of formatting money amount to make it work with FR format

Test plan:
1- Set CurrencyFormat pref to FR
2- Activate Point of sale and cash register features
3- Add cash register
4- Add debit type that 'can be sold' with a price of 10.00
5- Go to point of sale, click on the item to purchase
6- Verify the Amount to be paid shows the wrong amount and can't be fixed on the interface
7- Apply the patch
8- Refresh the page and do step 5 again
9- Notice it now shows the right amount
Comment 5 Shi Yao Wang 2022-05-10 17:44:29 UTC
If this is good. We can copy paste the changes to paycollect.tt moneyFormat function as well.
Comment 6 David Nind 2022-05-11 00:12:18 UTC
Created attachment 134842 [details] [review]
Bug 30139: Fix javascript moneyFormat to be compatible with FR format

New way of formatting money amount to make it work with FR format

Test plan:
1- Set CurrencyFormat pref to FR
2- Activate Point of sale and cash register features
3- Add cash register
4- Add debit type that 'can be sold' with a price of 10.00
5- Go to point of sale, click on the item to purchase
6- Verify the Amount to be paid shows the wrong amount and can't be fixed on the interface
7- Apply the patch
8- Refresh the page and do step 5 again
9- Notice it now shows the right amount

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2022-05-11 00:27:23 UTC
Created attachment 134843 [details]
Screenshot - after patch applied

I've attached a screenshot after the patch was applied.

I'm assuming that for the cost column, amount being paid, and amount tendered will be fixed to show a comma rather than a period once changes to paycollect.tt moneyFormat function are made (comment #5).

Note: it was a little tricky to capture the screen with the amount tendered entered as 10,00 - once you click out it defaults to being 10.00 as the amount tendered.

I'm also not sure whether you would normally enter the amount tendered as 10.00 or 10,00.
Comment 8 Katrin Fischer 2022-05-13 09:30:13 UTC
Starting QA here
Comment 9 Katrin Fischer 2022-05-13 09:42:23 UTC
I've tested POS with the 3 different currency formats (FR, CH and US) and found it all working now.

I also did a quick run through some other monetary actions, like a cash-up etc., no regressions spotted.
Comment 10 Katrin Fischer 2022-05-13 09:43:28 UTC
@David: In input fields you should always use . right now, independent from the CurrencyFormat setting, this only applies for display.
Comment 11 Katrin Fischer 2022-05-13 09:43:58 UTC
Created attachment 134957 [details] [review]
Bug 30139: Fix javascript moneyFormat to be compatible with FR format

New way of formatting money amount to make it work with FR format

Test plan:
1- Set CurrencyFormat pref to FR
2- Activate Point of sale and cash register features
3- Add cash register
4- Add debit type that 'can be sold' with a price of 10.00
5- Go to point of sale, click on the item to purchase
6- Verify the Amount to be paid shows the wrong amount and can't be fixed on the interface
7- Apply the patch
8- Refresh the page and do step 5 again
9- Notice it now shows the right amount

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 12 Shi Yao Wang 2022-05-13 12:49:23 UTC
Created attachment 134969 [details] [review]
Bug 30139: Fix javascript moneyFormat to be compatible with FR format

New way of formatting money amount to make it work with FR format

Test plan:
1- Set CurrencyFormat pref to FR
2- Activate Point of sale and cash register features
3- Add cash register
4- Add debit type that 'can be sold' with a price of 10.00
5- Go to point of sale, click on the item to purchase
6- Verify the Amount to be paid shows the wrong amount and can't be fixed on the interface
7- Apply the patch
8- Refresh the page and do step 5 again
9- Notice it now shows the right amount

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 13 Shi Yao Wang 2022-05-13 12:50:22 UTC
Copy pasted the same function into paycollect.tt
Comment 14 Katrin Fischer 2022-05-13 14:01:00 UTC
(In reply to Shi Yao Wang from comment #13)
> Copy pasted the same function into paycollect.tt

Hi Shi, please don't add changes to your patches after the patches have been signed and QA. This is not what people tested - please always provide the changes in a separate patch.

I'd also be inclined to say it should maybe be a separate bug and also have a new test plan. I had tested paying fines and had no issue - where and why is the new change needed?
Comment 15 Katrin Fischer 2022-05-13 14:05:08 UTC
I've restored the previous patch for now - awaiting follow-up patch here or on a separate bug.
Comment 16 Shi Yao Wang 2022-05-13 14:16:23 UTC
(In reply to Katrin Fischer from comment #14)
> (In reply to Shi Yao Wang from comment #13)
> > Copy pasted the same function into paycollect.tt
> 
> Hi Shi, please don't add changes to your patches after the patches have been
> signed and QA. This is not what people tested - please always provide the
> changes in a separate patch.
> 
> I'd also be inclined to say it should maybe be a separate bug and also have
> a new test plan. I had tested paying fines and had no issue - where and why
> is the new change needed?

Noted, thanks for letting me know. I sent the changes because both files had the exact same function. It seems inconsistent to have 2 functions with the same name, but with different code. I guess it doesn't really matter if the other one doesn't have any issue in the first place.
Comment 17 Shi Yao Wang 2022-05-13 14:22:52 UTC
Created attachment 134973 [details] [review]
Bug 30139: Same fix for the other javascript moneyFormat function
Comment 18 Katrin Fischer 2022-05-13 14:23:53 UTC
Consistency is always an argument - I'll try to do some testing on paycollect for any regressions.
Comment 19 Shi Yao Wang 2022-05-13 14:35:07 UTC
Created attachment 134974 [details] [review]
Bug 30139: Same fix for the other javascript moneyFormat function

This patch makes javascript moneyFormat function in paycollect.tt consistent
with the one in pay.tt
Comment 20 Katrin Fischer 2022-05-13 14:50:15 UTC
Created attachment 134976 [details] [review]
Bug 30139: Same fix for the other javascript moneyFormat function

This patch makes javascript moneyFormat function in paycollect.tt consistent
with the one in pay.tt

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 21 Fridolin Somers 2022-05-14 01:42:40 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄