Description
Séverine Queune
2017-06-03 19:30:48 UTC
Linked but not a duplicate. Needs an independent patch due to the specificities of the two different pages. The current plan is to make the order page take into account the CurrencyFormat syspref. Server side that means using Koha::Number::Price->unformat() But it's likely to be done client-side. Because currently, on the client-side there are calculations to convert the amount from one currency to another. With that the behavior would be consistent between the order and reception. The current plan is to make bug 12310 go in master and then in this bug do a follow up in cgi-bin/koha/acqui/orderreceive.pl If we alreay know, that this was caused by 18471 - could we not bring the old behaviour back somehow to make things consistent again? A solution that could be backported. And then bug 12310 as a new feature/enh for the next release? If Bug 18471 causes more harm than good, then that's a nice idea. If so, then no one should feel blocked to submit such a patch because of the ongoing work on bug 12310 and this bug. Unfortunately (or fortunately ;) ) all the issues that are reported to me are about fines, so I'll focus on fixing 12310 to not let it sink. Along with this bug, because it's a nice enhancement and it's won't be to much time. @Séverine Queune, maybe you could rollback bug 18471 on your instance while bug 12310 and this bug are fixed? To test: - Set CurrencyFormat to FR - Order something, verify you can use . on input there - Receive it, verify that if you use . there, it turns into thousands While testing bug 9775, I came upon a decimal problem in orderreceive.pl. I don't know if it's the same bug. This happens in the "Actual cost" box. To replicate: 1- Go to Acquisitions 2- Search for a Vendor 3- Click on "New basket" 4- Give basket a name and click "Save" 5- Click on "Add to basket" 6- Add an order through preferred method 7- In Accounting details, enter a vendor price with dot decimal (i.e. 19.44) 8- Save your order line 9- Click on "Close this basket" 10- Confirm closing of basket by clicking on "Yes, close" 11- Click on "Receive shipment" 12- Enter an invoice number and click "Next" 13- Click on "Receive" to the right of your order 14- In Accounting details, notice the Actual cost is written with a decimal dot. 15- Change the Actual cost, using a dot decimal (i.e. 20.99) 16- Receive the order 17- Click on "Save" 18- In "Already received" notice the price is multiplied by 100 (i.e. 2099,00) 19- Click on "Cancel receipt" 20- Click on "Receive" to the right of your order 21- In Accounting details, change the Actual cost, using a comma decimal (i.e. 20,99) 22- Receive the order 23- Click on "Save" 24- In "Already received", notice the price is correct. Created attachment 79892 [details] [review] Bug 18723: Do not unformat price Test plan: a) Replicate the issue: 0- Set CurrencyFormat to FR 1- Go to Acquisitions 2- Search for a Vendor 3- Click on "New basket" 4- Give basket a name and click "Save" 5- Click on "Add to basket" 6- Add an order through preferred method 7- In Accounting details, enter a vendor price with dot decimal (i.e. 19.44) 8- Save your order line 9- Click on "Close this basket" 10- Confirm closing of basket by clicking on "Yes, close" 11- Click on "Receive shipment" 12- Enter an invoice number and click "Next" 13- Click on "Receive" to the right of your order 14- In Accounting details, notice the Actual cost is written with a decimal dot. 15- Change the Actual cost, using a dot decimal (i.e. 20.99) 16- Receive the order 17- Click on "Save" 18- In "Already received" notice the price is multiplied by 100 (i.e. 2099,00) 19- Click on "Cancel receipt" 20- Click on "Receive" to the right of your order 21- In Accounting details, change the Actual cost, using a comma decimal (i.e. 20,99) 22- Receive the order 23- Click on "Save" 24- In "Already received", notice the price is correct. b) apply the patch c) try again, should be fixe now Hi Josef, Thanks for the patch! I don't know if I didn't notice it before or if it's new, but when I change the actual cost to 20,99 (with a comma), the price once it's received is 20,00. The cents are removed completely. (In reply to Caroline Cyr La Rose from comment #11) > Hi Josef, > > Thanks for the patch! > > I don't know if I didn't notice it before or if it's new, but when I change > the actual cost to 20,99 (with a comma), the price once it's received is > 20,00. The cents are removed completely. That's steps 20-24 While bug 21507 is implemented, here was the work in progress on this bug: https://gitlab.com/tuxayo/Koha/commit/3562c8708c820d46595640459915e0a97a6a9fad If it takes too much time. I hope that the patch of Josef Moravec will be functional enough to be integrated and improve the current situation. Quite complete discussion on the issue in the IRC channel: http://irc.koha-community.org/koha/2018-11-26#i_2100176 The current plan (bug 21507) is still valid from what I understand. And various followups have been discussed. Created attachment 83971 [details] [review] Bug 18723: (Alternate Solution) Block form submission When entering an amount in "Actual Cost" on the orderreceive page, it is multiplied by 100 if the entered number's format does not match the "CurrencyFormat" syspref. This patch adds a value check before submitting the form. It uses regexps based on the "CurrencyFormat" syspref to validate that the number will be processed correctly. Test Plan: a)Replicate the issue: 0- Set CurrencyFormat to FR 1- Go to Acquisitions 2- Search for a Vendor 3- Click on "New basket" 4- Give basket a name and click "Save" 5- Click on "Add to basket" 6- Add an order through preferred method 7- In Accounting details, enter a vendor price with dot decimal (i.e. 19.44) 8- Save your order line 9- Click on "Close this basket" 10- Confirm closing of basket by clicking on "Yes, close" 11- Click on "Receive shipment" 12- Enter an invoice number and click "Next" 13- Click on "Receive" to the right of your order 14- In Accounting details, notice the Actual cost is written with a decimal dot. 15- Change the Actual cost, using a dot decimal (i.e 20.99) 16- Receive the order 17- Click on "Save" 18- In "Already received" notice the price is multiplied by 100 (i.e. 2099,00) 19- Click on "Cancel receipt" 20- Click on "Receive" to the right of your order 21- In Accounting details, change the Actual cost, using a comma decimal (i.e. 20,99) 22- Receive the order 23- Click on "Save" 24- In "Already received", notice the price is correct. b)Apply the patch c)Test the patch: 1- Click on "Cancel receipt" 2- Click on "Receive to the right of your order 3- In Accounting Details, notice the Actual cost is formatted correctly (comma decimal) 4- Change the Actual cost, using a dot decimal (21.99) 5- Receive the order 6- Click on "Save" 7- Notice an error message explaining the proper format for Actual cost (click "Ok") 8- Change the Actual cost, using a comma decimal (21,99) 9- Click on "Save" 10- In "Already received", notice the price is correct. 11- Test the other CurrencyFormat settings Hi, thanks for your help :) Something is a bit weird: > 14- In Accounting details, notice the Actual cost is written with a decimal dot. «Alright» > 15- Change the Actual cost, using a dot decimal (i.e 20.99) «Wait that was already the case» Anyway, I kept going. Created attachment 84150 [details] [review] Bug 18723: (Alternate Solution) Block form submission When entering an amount in "Actual Cost" on the orderreceive page, it is multiplied by 100 if the entered number's format does not match the "CurrencyFormat" syspref. This patch adds a value check before submitting the form. It uses regexps based on the "CurrencyFormat" syspref to validate that the number will be processed correctly. Test Plan: a)Replicate the issue: 0- Set CurrencyFormat to FR 1- Go to Acquisitions 2- Search for a Vendor 3- Click on "New basket" 4- Give basket a name and click "Save" 5- Click on "Add to basket" 6- Add an order through preferred method 7- In Accounting details, enter a vendor price with dot decimal (i.e. 19.44) 8- Save your order line 9- Click on "Close this basket" 10- Confirm closing of basket by clicking on "Yes, close" 11- Click on "Receive shipment" 12- Enter an invoice number and click "Next" 13- Click on "Receive" to the right of your order 14- In Accounting details, notice the Actual cost is written with a decimal dot. 15- Change the Actual cost, using a dot decimal (i.e 20.99) 16- Receive the order 17- Click on "Save" 18- In "Already received" notice the price is multiplied by 100 (i.e. 2099,00) 19- Click on "Cancel receipt" 20- Click on "Receive" to the right of your order 21- In Accounting details, change the Actual cost, using a comma decimal (i.e. 20,99) 22- Receive the order 23- Click on "Save" 24- In "Already received", notice the price is correct. b)Apply the patch c)Test the patch: 1- Click on "Cancel receipt" 2- Click on "Receive to the right of your order 3- In Accounting Details, notice the Actual cost is formatted correctly (comma decimal) 4- Change the Actual cost, using a dot decimal (21.99) 5- Receive the order 6- Click on "Save" 7- Notice an error message explaining the proper format for Actual cost (click "Ok") 8- Change the Actual cost, using a comma decimal (21,99) 9- Click on "Save" 10- In "Already received", notice the price is correct. 11- Test the other CurrencyFormat settings Signed-off-by: Victor Grousset <victor.grousset@biblibre.com> (In reply to Victor Grousset/tuxayo from comment #16) > Hi, thanks for your help :) > > Something is a bit weird: > > > 14- In Accounting details, notice the Actual cost is written with a decimal dot. > > «Alright» > > > 15- Change the Actual cost, using a dot decimal (i.e 20.99) > > «Wait that was already the case» > > Anyway, I kept going. I see how it can be a little confusing. What line 14 shows is that even though the CurrencyFormat uses a comma, the form uses a period when it is being edited. When that price (or a different price) is entered at step 15, the period "breaks" the formatting and leads to the unwanted multiplication. According to the QA and functionnal feedback that I got on bug 21507 and bug 12310: - here we must also use jQuery.validator - https://gitlab.com/tuxayo/Koha/compare/master...bug%2F21507%2Fdecimal-separators-fines#0d520d86f436b1c7c08ed14dc1ac60e58fa114a4_0_19 - can we copy the code from the above patch? So when both will be merged, there will be a consistence between payments/fines and acquisitions - thousands separators shouldn't be allowed, which means the code that prefills the field shouldn't add it. (I'll reread again to see if there are other things to mention) Can we just get a patch for now that makes this form behave like the rest of Koha again and keep elaborate better fixes for the next step? We have still not been able to fix the regression! I think what we want for the stable releases is consistent behaviour. If I have to add my data everywhere with a decimal dot (independent of the CurrencyFormat setting!) then I want that on the receive form as well. Currently only this one input behaves badly which causes a lot of confusion and frustration. And if we are going to make a change here - we need to do it for all input fields in ACQ (or in Koha) at once and release that with a major version. I think while the new patch might be a way forward, it might not work for fixing the regression. Created attachment 84166 [details] [review] Bug 18723: Change dot into comma This fixes the regression that multiplies the amount entered by 100 when CurrencyFormat is set to FR. It replaces the last dot with a comma before dealing with the value of ActualCost and ReplacementCost. Test Plan: a)Replicate the issue: 0- Set CurrencyFormat to FR 1- Go to Acquisitions 2- Search for a Vendor 3- Click on "New basket" 4- Give basket a name and click "Save" 5- Click on "Add to basket" 6- Add an order through preferred method 7- In Accounting details, enter a vendor price with dot decimal (i.e. 19.44) 8- Save your order line 9- Click on "Close this basket" 10- Confirm closing of basket by clicking on "Yes, close" 11- Click on "Receive shipment" 12- Enter an invoice number and click "Next" 13- Click on "Receive" to the right of your order 14- In Accounting details, notice the Actual cost is written with a decimal dot. 15- Change the Actual cost, using a dot decimal (i.e 20.99) 16- Receive the order 17- Click on "Save" 18- In "Already received" notice the price is multiplied by 100 (i.e. 2099,00) 19- Click on "Cancel receipt" 20- Click on "Receive" to the right of your order 21- In Accounting details, change the Actual cost, using a comma decimal (i.e. 20,99) 22- Receive the order 23- Click on "Save" 24- In "Already received", notice the price is correct. b)Apply the patch c)Test the patch: 1- Click on "Cancel receipt" 2- Click on "Receive to the right of your order 3- Change the Actual cost/Replacement cost, using a dot decimal (21.99) 4- Receive the order 5- Click on "Save" 6- Notice that the Actual cost and the Replacement cost use commas 7- Change the Actual cost, using a comma decimal (21,99) 8- Click on "Save" 9- In "Already received", notice the price is still correct. Created attachment 84280 [details] [review] Bug 18723: Change dot into comma This fixes the regression that multiplies the amount entered by 100 when CurrencyFormat is set to FR. It replaces the last dot with a comma before dealing with the value of ActualCost and ReplacementCost. Test Plan: a)Replicate the issue: 0- Set CurrencyFormat to FR 1- Go to Acquisitions 2- Search for a Vendor 3- Click on "New basket" 4- Give basket a name and click "Save" 5- Click on "Add to basket" 6- Add an order through preferred method 7- In Accounting details, enter a vendor price with dot decimal (i.e. 19.44) 8- Save your order line 9- Click on "Close this basket" 10- Confirm closing of basket by clicking on "Yes, close" 11- Click on "Receive shipment" 12- Enter an invoice number and click "Next" 13- Click on "Receive" to the right of your order 14- In Accounting details, notice the Actual cost is written with a decimal dot. 15- Change the Actual cost, using a dot decimal (i.e 20.99) 16- Receive the order 17- Click on "Save" 18- In "Already received" notice the price is multiplied by 100 (i.e. 2099,00) 19- Click on "Cancel receipt" 20- Click on "Receive" to the right of your order 21- In Accounting details, change the Actual cost, using a comma decimal (i.e. 20,99) 22- Receive the order 23- Click on "Save" 24- In "Already received", notice the price is correct. b)Apply the patch c)Test the patch: 1- Click on "Cancel receipt" 2- Click on "Receive to the right of your order 3- Change the Actual cost/Replacement cost, using a dot decimal (21.99) 4- Receive the order 5- Click on "Save" 6- Notice that the Actual cost and the Replacement cost use commas 7- Change the Actual cost, using a comma decimal (21,99) 8- Click on "Save" 9- In "Already received", notice the price is still correct. Signed-off-by: Victor Grousset <victor.grousset@biblibre.com> Here are the result with the 3 formats, is that the expected behavior? CurrencyFormat: - FR: - dot: ok (act as decimal separator) - comma: ok (act as decimal separator) - US - dot: ok - comma: acts as a thousand separator (so multiply by 100 if one expected it to act as a decimal separator) - CH - dot: ok - comma: acts as a thousand separator (so multiply by 100 if one expected it to act as a decimal separator) (In reply to Victor Grousset/tuxayo from comment #24) > Here are the result with the 3 formats, is that the expected behavior? > > CurrencyFormat: > - FR: > - dot: ok (act as decimal separator) > - comma: ok (act as decimal separator) > - US > - dot: ok > - comma: acts as a thousand separator (so multiply by 100 if one expected > it to act as a decimal separator) > - CH > - dot: ok > - comma: acts as a thousand separator (so multiply by 100 if one expected > it to act as a decimal separator) The other 2 formats use commas (or apostrophes for CH) as thousands separators, so that behavior is kind of normal. That's what I understood, good :) @José-Mario The patch "(Alternate Solution) Block form submission" is obsolete right? The previous functional discussion points toward using "Change dot into comma" if I understand correctly. Sorry, quite confused here by now :( I'll try to test myself, but not sure when time will allow. If someone knows right away, what would be the expected behaviour for entering the actual price on receive and CurrencyFormat CH with the patch? Will it expect 10.00 or 10,00? > I'll try to test myself, but not sure when time will allow. Don't worry, that's quite quick. It took me less than 10 min to test with the 3 syspref values. Including some time to get lost due to fall into some traps due to not knowing well this part of acq. > what would be the expected behaviour for entering the actual price on receive and CurrencyFormat CH with the patch? Will it expect 10.00 or 10,00? Both! It depend on what the user expects :) input 10.00 => 10.00 input 10,00 => 1000 (treated a thousands sep and discarded) @Victor the patch is indeed obsolete, I simply forgot to mark it as such. @Katrin As Victor said, in CH format, 10,00 would be treated as 1000 since commas are thousand separators. I do not think anybody using CH would expect 10,00 to be treated as 10.00 due to commas having a different meaning in that format, but I could be wrong. (In reply to José-Mario Monteiro-Santos from comment #30) > @Victor the patch is indeed obsolete, I simply forgot to mark it as such. > > @Katrin As Victor said, in CH format, 10,00 would be treated as 1000 since > commas are thousand separators. I do not think anybody using CH would expect > 10,00 to be treated as 10.00 due to commas having a different meaning in > that format, but I could be wrong. Still confused, but the test plan reads well (too tired from travelling) I will try to have a look during the weekend! This patch works fine for master, but I wonder about the lower versions. Maybe the addition of replacement cost for 18.11 changed something? It's not there in 17.11, so the same fix will not apply I think. On 18.11: 1) Set CurrencyFormat to FR 2) Receive item in acq: Quantity received: 1 Actual cost and replacement cost: 11.11 3) Verify _both_ turned into: 111.10 On Master: 0) - 2) as above 3) Same with the patch: 111.10 4) Repeat with patch: all good. Created attachment 84843 [details] [review] Bug 18723: Change dot into comma This fixes the regression that multiplies the amount entered by 100 when CurrencyFormat is set to FR. It replaces the last dot with a comma before dealing with the value of ActualCost and ReplacementCost. Test Plan: a)Replicate the issue: 0- Set CurrencyFormat to FR 1- Go to Acquisitions 2- Search for a Vendor 3- Click on "New basket" 4- Give basket a name and click "Save" 5- Click on "Add to basket" 6- Add an order through preferred method 7- In Accounting details, enter a vendor price with dot decimal (i.e. 19.44) 8- Save your order line 9- Click on "Close this basket" 10- Confirm closing of basket by clicking on "Yes, close" 11- Click on "Receive shipment" 12- Enter an invoice number and click "Next" 13- Click on "Receive" to the right of your order 14- In Accounting details, notice the Actual cost is written with a decimal dot. 15- Change the Actual cost, using a dot decimal (i.e 20.99) 16- Receive the order 17- Click on "Save" 18- In "Already received" notice the price is multiplied by 100 (i.e. 2099,00) 19- Click on "Cancel receipt" 20- Click on "Receive" to the right of your order 21- In Accounting details, change the Actual cost, using a comma decimal (i.e. 20,99) 22- Receive the order 23- Click on "Save" 24- In "Already received", notice the price is correct. b)Apply the patch c)Test the patch: 1- Click on "Cancel receipt" 2- Click on "Receive to the right of your order 3- Change the Actual cost/Replacement cost, using a dot decimal (21.99) 4- Receive the order 5- Click on "Save" 6- Notice that the Actual cost and the Replacement cost use commas 7- Change the Actual cost, using a comma decimal (21,99) 8- Click on "Save" 9- In "Already received", notice the price is still correct. Signed-off-by: Victor Grousset <victor.grousset@biblibre.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Sorry, I am probably confusing (too early still). This should work for master and 18.11. Will just need to have the one line for replacement cost removed for lower versions. I know this is not the perfect fix - it's a regression fix that will need a better global solution at some point. But please don't hold it up any longer. I just received another ticket about this issue yesterday from a library and I feel it needs solving more than perfect right now. Thx Jose-Mario and Victor! Awesome work all! Pushed to master for 19.05 Pushed to 18.11.x for 18.11.03 can't cleanly apply to 18.05.x. please rebase if needed in 18.05.x The issue goes back as far as 17.11. Maybe a separate patch without the lines for replacement cost would work best (18.11 feature). I think you're right there cait.. should be reasonably trivial to rebase along those lines.. did you want me to have a punt at it for you Lucas? Created attachment 86082 [details] [review] Backport of the patch to 18.05.x Cherry-picked from 18.11.x branch on top of current 18.05.x (0dd262a02e) Removed replacement cost stuff which didn't exist in 18.05 Test plan "ran" successfully. Applying the previous patch (Backport for 18.05) on top of current 17.11.x (tag v17.11.16) works. Test plan "ran" successfully. Lucas, Frido: is there anything else doable to help landing in 18.05.x and 17.11.x ? Thanks Victor, backported to 18.05.x for 18.05.11 Pushed to 17.11.x for 17.11.17 Thanks Lucas and Fridolin! :D Hi :) Here is an SQL query to detect orders and items (items.price) that were affected by this bug. And 2 others to fix these cases. And finally a large price check query that should point any case of price difference > 5 (this is arbitrary) between prices that should be close to each other (unitprice, unitprice_tax_excluded, ecost, items.price, etc) The check queries were applied to a dozen of instances that were reporting price issue with acquisition receiving and thus the budget and funds total And the fix queries in there final version were applied to a bit more than half of them. === BZ 18723 specific price check query === Should only detect cases related to this bug. All values except ecost are x100 time bigger - unitprice - items.price - unitprice_tax_excluded - unitprice_tax_included - tax_value_on_receiving ``` SELECT invoicenumber, link.ordernumber, items.itemnumber, ecost, items.price, unitprice, unitprice_tax_excluded, unitprice_tax_included, tax_value_on_receiving, unitprice_tax_excluded/ecost as ratio FROM aqorders LEFT JOIN aqinvoices ON aqinvoices.invoiceid = aqorders.invoiceid LEFT JOIN aqorders_items as link ON link.ordernumber=aqorders.ordernumber LEFT JOIN items ON link.itemnumber=items.itemnumber WHERE unitprice_tax_excluded / ecost BETWEEN 50 AND 150 AND unitprice_tax_included / ecost BETWEEN 50 AND 150 AND unitprice / ecost BETWEEN 50 AND 150 ORDER BY ratio DESC; -- BETWEEN 50 AND 150 to get the differences between order cost and final cost. -- This is arbitrary. ``` === BZ 18723 specific data fix queries === Caveat, this might be not exhaustive, there might be remaining deriving data that isn't updated. divide by 100 - unitprice - items.price - unitprice_tax_excluded - unitprice_tax_included - tax_value_on_receiving (don't forget this one) ``` UPDATE aqorders SET unitprice=unitprice / 100, unitprice_tax_excluded=unitprice_tax_excluded / 100, unitprice_tax_included=unitprice_tax_included / 100, tax_value_on_receiving=tax_value_on_receiving / 100 WHERE -- we get only the x100 cases unitprice_tax_excluded / ecost BETWEEN 50 AND 150 AND unitprice_tax_included / ecost BETWEEN 50 AND 150 AND unitprice / ecost BETWEEN 50 AND 150; ``` ``` UPDATE items set price=price/100 -- wider criteria than the issue detection request -- because a JOIN in the update would have made it less readable and error prone. -- so it might fix more cases than the only those related to bug 18723 WHERE price/replacementprice between 50 and 150; -- we get only the x100 cases -- caveat; we have seen replacementprice being NULL and thus the fix doesn't happen -- we consider this to not be related to bug 18723 -- and it will be caught be the general analysis query. ``` Hopefully we can finally have a relief about this issue. === general price check query === Compare all the values in a systematic way. It can be summarized by A not more than 5x B B not more than 5x C C not more than 5x A 5 is arbitrary, any feedback welcome. ``` SELECT invoicenumber, link.ordernumber, items.itemnumber, ecost, items.replacementprice AS items_replacementprice, aqorders.replacementprice AS aqorders_replacementprice, items.price, unitprice, unitprice_tax_excluded, unitprice_tax_included, tax_value_on_receiving, unitprice_tax_excluded/ecost as ratio FROM aqorders LEFT JOIN aqinvoices ON aqinvoices.invoiceid = aqorders.invoiceid LEFT JOIN aqorders_items as link ON link.ordernumber=aqorders.ordernumber LEFT JOIN items ON link.itemnumber=items.itemnumber WHERE unitprice_tax_excluded / unitprice_tax_included > 5 OR unitprice_tax_included / unitprice > 5 OR unitprice / aqorders.replacementprice > 5 OR aqorders.replacementprice / ecost > 5 OR ecost / items.price > 5 OR items.price / unitprice_tax_excluded > 5 ORDER BY ratio DESC; ``` Thanks Arthur for you help in finally getting rid of this! |