Hello :) This patch aims rather to clarify and avoid display errors concerning the total of the basket. When totals are displayed on the basketgroup.pl view the total is calculated relative to the rounded estimated cost (ecost) - tax included or excluded. While on the basket.pl view in detail, the logic is to take the unit price (tax included or excluded) OR then the estimated cost - tax included or excluded. This can cause a slight difference of cents depending on whether the unit price is calculated or not compared to the estimated price calculated. All this depending on the rounding or not of the third decimal for the fields not calculated (OrderPriceRounding activated or not).
Created attachment 157495 [details] [review] Bug 35114: Adjust logic around basket total calculation Like in total basket.pl calculation we took the unitprice first than the ecost to get the same total value between basketgroup and basket details Test plan : 1) Be sure to have basketgroup with any vendor (which must have a discount rate to see ecost calculation) 2) Go to basket details and basketgroup overview to see the difference 3) Apply this basket and refresh pages to see now the same result
Hi Thibaud, I'm trying to test this but running into some questions already before applying the patch. Could you make the test plan a bit more specific? I tried creating a couple of baskets and basket groups and for me the sums in the basket view and in the basket group view are already the same before applying the patch, so I'm not entirely sure what I should do to replicate the display errors. Thanks!
Hi Philip, Thank you for taking the time to test this patch. From what I remember it was a calculation difference related to cents, as one function truncates while another rounds and that's what leads to a discrepancy. I'll take the time soon to write a test plan with exact amounts, but I need to free up some time to do it. Try on your side with several amounts that may cause calculation differences such as X,946 etc... If the result is unclear, I'll draw up a more precise test plan. Thanks again ;)
Hi Thibaud, thanks for the quick response! I tried it again with different amounts including e.g. 10,496 or 10,5996 .. etc. I still didn't see any difference between the two views. They were still both the same (possibly calculated wrongly, but at least both calculated seemingly the same :) ) Also I checked activating OrderPriceRounding, but still then, both sums were the same in basketgroup view and in the basket details itself. So I think you will have to write up a more detailed test plan for this one.
Hi, I wasn't able to reproduce the problem, too. So please add more testing instructions. Thanks
Created attachment 161066 [details] [review] Bug 35114: Fix calculation around basketgroup and basket details Currently calculation on basketgroup is performed on ecost (tax_included and tax_excluded) while in basket detail it's on ecost only if unitprice is not defined. So I keep the same logic in basketgroup and I took the opportunity to remove a call to get_rounded_price on quantity (not logic). Test plan: 1) To test it you must have some items with differents values on ecost_tax_* and unitprice_tax_*. Both have 6 decimal places, but problems arise when the value (calculated for ecost_tax_*) has a 3rd decimal greater than or equal to 5. So I you have already this case in your datas create a basket with this item otherwise you can modify your database to reproduce the bad behavior. e.g set a ecost_tax_included to '18.655000' and '18.650000' for unitprice. 2) After the basket has been created, and in a basketgroup, go to basketgroup and check the amount. Click to see the details of the linked basket, the total will be different because the rounded total displayed previously (basketgroup) is based on the ecost_tax_* and so 18.655000 will become 18.66. In the detail based on the unitprice, which is 18.650000, the difference will be one cent on the total. Not much, but it's only a record... it's not precise (in accounting terms). 3) Apply this patch and repeat step 2)
I've tried to be more complete with this test plan, but it's a bit tricky to test, so I suggest you modify your data to obtain an example of bad behavior.
Created attachment 161691 [details] [review] Bug 35114: Fix calculation around basketgroup and basket details Test plan: 1) I purpose a simple way to test it with new fresh data. First of all go to one of your vendor and set a discount of 9%. Then create a new basket, add to basket a new line from an empty record. Set a vendor price to 15.90 and just add this order 2) Configure the basket.pl table to show all columns, see budgeted cost and actual cost (exc or inc). There is already a difference of one cent. 3) Let's take a look at the basket groups now, for this, close this basket. Go on tab menu and select basket group. Click on new basket group. Normally you will see the new basket created on step 1. The total here is 14.47.. and when you check on basket detail it's 14.46 cause this total (from basket groups) comes from ecost exc. while total line on basket detail comes from actual cost exc. 4) Apply this patch and see changes Sponsored-by: BibLibre
The first fix I proposed was to correct the behavior of basket group, in order to base itself on the calculation of actual cost if it is defined, otherwise it remains on ecost. This is in fact currently the case with basket detail. Except that if I go back to the behavior and error of my second step, I can already see a price difference (of one cent). To understand this, when adding the order (from neworderempty.pl) the shaded lines are calculated lines, with 6 decimal places, whereas the actual cost value, if not defined, will be retrieved by javascript (acq.js updateCosts function), keeping only two decimal places, which distorts the calculations. The ecost is rounded down to the nearest integer (Math.floor), so this logic doesn't fit with the rest of the calculations, which round up to the nearest integer using a get_rounded_price method, plus Math.round to draw a parallel. Except that in our case (test plan example), even this rectification wouldn't change a thing... Let's take a look at the basic data: unitprice : 14.460000 unitprice_tax_excluded: 14.460000 unitprice_tax_included: 15.255300 ecost: 14.46 ecost_tax_excluded: 14.469000 ecost_tax_included: 15.264795 Accounting calculations can't be imprecise, so what I'm proposing is to use only the ecost to obtain the right calculations. The behavior of the unit price (when adding a line from neworderempty.pl for example), on the other hand, can be corrected to match the correct values: Math.round usage
I forgot to tell which VAT was applied: 5.5%
Decidedly... I also forgot (it was in my old commit *obsolete*) to say that I had taken the opportunity to call rounding (get_rounded_price) in a more logical way, after multiplying quantities or on a total.
Created attachment 161799 [details] [review] Bug 35114: Fix calculation around basketgroup and basket details Test plan: 1) I purpose a simple way to test it with new fresh data. First of all go to one of your vendor and set a discount of 9%. Then create a new basket, add to basket a new line from an empty record. Set a vendor price to 15.90 and just add this order 2) Configure the basket.pl table to show all columns, see budgeted cost and actual cost (exc or inc). There is already a difference of one cent. 3) Let's take a look at the basket groups now, for this, close this basket. Go on tab menu and select basket group. Click on new basket group. Normally you will see the new basket created on step 1. The total here is 14.47.. and when you check on basket detail it's 14.46 cause this total (from basket groups) comes from ecost exc. while total line on basket detail comes from actual cost exc. 4) Apply this patch and see changes Sponsored-by: BibLibre
Hello everyone, I've adjusted the rounding functions and their calculation. According to the OrderPriceRounding system preference, when the get_rounded_price function was called, the prices were adjusted at the wrong time. (Note: without this patch, which corrects the use of unit price in calculations, the system preference has no effect, as the unit price is retrieved beforehand and modified in JS to round it off). Lines were rounded one by one and then the total was rounded... This can lead to problems: example: - 13.44567 -> rounded to 13.45 - 2.44567 -> rounded to 2.45 total 13.44567 + 2.44567 = 15.89134 rounded to 15.89 total 13.45 + 2.45 = 15.90 so I've kept this rounding behavior on each of the lines, but I've added a total that I calculate without rounding, and then pass the corresponding roundings to the view.
Created attachment 161800 [details] [review] Bug 35114: Fix calculation around basketgroup and basket details Test plan: 1) I purpose a simple way to test it with new fresh data. First of all go to one of your vendor and set a discount of 9%. Then create a new basket, add to basket a new line from an empty record. Set a vendor price to 15.90 and just add this order 2) Configure the basket.pl table to show all columns, see budgeted cost and actual cost (exc or inc). There is already a difference of one cent. 3) Let's take a look at the basket groups now, for this, close this basket. Go on tab menu and select basket group. Click on new basket group. Normally you will see the new basket created on step 1. The total here is 14.47.. and when you check on basket detail it's 14.46 cause this total (from basket groups) comes from ecost exc. while total line on basket detail comes from actual cost exc. 4) Apply this patch and see changes Sponsored-by: BibLibre
Created attachment 162060 [details] [review] Bug 35114: Fix calculation around basketgroup and basket details Test plan: 1) I purpose a simple way to test it with new fresh data. First of all go to one of your vendor and set a discount of 9%. Then create a new basket, add to basket a new line from an empty record. Set a vendor price to 15.90 and just add this order 2) Configure the basket.pl table to show all columns, see budgeted cost and actual cost (exc or inc). There is already a difference of one cent. 3) Let's take a look at the basket groups now, for this, close this basket. Go on tab menu and select basket group. Click on new basket group. Normally you will see the new basket created on step 1. The total here is 14.47.. and when you check on basket detail it's 14.46 cause this total (from basket groups) comes from ecost exc. while total line on basket detail comes from actual cost exc. 4) Apply this patch and see changes Sponsored-by: BibLibre
Hello. I'm not clearly seeing the issue that this bug attempts to address, so it is difficult to determine if the patch is doing what it is supposed to. To replicate the issue, I did the following (according to the proposed test plan): 1. Created a new vendor with a 9% discount 2. Created a new basket, added an item from a new record - No other details except Koha item type entered for the item - Under accounting details, I entered a vendor price of $15.90 - At this point, the auto calculated budgeted cost and total show $14.46 3. Save item and return to basket table. All columns in table for budgeted cost and total tax show $14.47 - The only way I can see the 1 cent difference is if I attempt to modify the item order details and return to the previous screen (neworderempty.pl) 4. When creating and viewing a basket group, I only see $14.47 as the cost When I applied the patch, the auto calculated cost in the accounting details on neworderempty.pl still showed $14.46 while the basket tables all show $14.47. Where I did find $14.46 listed was when I created an invoice and attempted to receive the shipment. On the pending orders table in parcel.pl, the unit cost and order cost show up as $14.46. When you click the link to receive, the accounting details on the right side of the pop-up show $14.47 as the budgeted cost. Applying the patch did not change this behavior. I hope these notes are helpful!
Hello Thibault, I'm not totally sure, and I don't see it on the test plan: should we activate the system preference called " OrderPriceRounding" ? Sonia
Hello Thibault, I tried several times, activing or not OrderPriceRounding, and I always have the same prices with the patch and on master. I have the same prices in the basket page, the basket group. I use this SQL report to see all the prices : select ordernumber, listprice, unitprice, unitprice_tax_excluded, unitprice_tax_included, ecost, ecost_tax_excluded, ecost_tax_included, rrp, rrp_tax_excluded, rrp_tax_included, replacementprice from aqorders And I have the same results in master and with your patch. Are sure of your examples ? Perhaps this issue is already fix in master ? Sonia
(In reply to Koha Team University Lyon 3 from comment #18) > Hello Thibault, > I tried several times, activing or not OrderPriceRounding, and I always have > the same prices with the patch and on master. > > I have the same prices in the basket page, the basket group. > > I use this SQL report to see all the prices : > select ordernumber, listprice, unitprice, unitprice_tax_excluded, > unitprice_tax_included, ecost, ecost_tax_excluded, ecost_tax_included, rrp, > rrp_tax_excluded, rrp_tax_included, replacementprice from aqorders > > And I have the same results in master and with your patch. > > Are sure of your examples ? Perhaps this issue is already fix in master ? > > Sonia Hi Sonia, Before this patch there was no difference with the system preference to activate the amount taken into account was that of the unit price already rounded beforehand when it was informed about the view preceding the detail of the basket. Now according to its activation yes there is a difference on the basket detail. Do not forget to provide a discount rate on the vendor in order to be able to see the differences on the calculated amounts.
Updating severity. (it's at least normal)
Created attachment 172124 [details] [review] Bug 35114: Fix calculation around basketgroup and basket details Test plan: 1) I purpose a simple way to test it with new fresh data. First of all go to one of your vendor and set a discount of 9%. Then create a new basket, add to basket a new line from an empty record. Set a vendor price to 15.90 and just add this order 2) Configure the basket.pl table to show all columns, see budgeted cost and actual cost (exc or inc). There is already a difference of one cent. 3) Let's take a look at the basket groups now, for this, close this basket. Go on tab menu and select basket group. Click on new basket group. Normally you will see the new basket created on step 1. The total here is 14.47.. and when you check on basket detail it's 14.46 cause this total (from basket groups) comes from ecost exc. while total line on basket detail comes from actual cost exc. 4) Apply this patch and see changes Sponsored-by: BibLibre Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
I've tested without the patch: - created a new vendor with 9% discount - created a basket for this vendor with a vendor price = 15.90, the budgeted cost calculated is false = 14.46 and it should be 14.47 (as the real calculation is 14.69) With the patch, the budgeted cost and actual cost are good and the price are consistent in all the screen (when you enter the order, on the basket page, on the basketgroup) It works as attented. Sonia
WARN acqui/basket.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 184, now: 186) - # FIXME The following is wrong + # FIXME The following is wrong <- still true ? remove if patch 35114 fixed it Lets remove that. A fixme that does not tell what is wrong, is wrong on itself. Lets assume we fixed it. And let the author of this fixme stand up if we did not. - $total_tax_excluded += $$line{total_tax_excluded}; + $total_tax_excluded += get_rounded_price($$line{total_tax_excluded}); $foot{$$line{tax_rate}}{total_tax_included} += $$line{total_tax_included}; - $total_tax_included += $$line{total_tax_included}; + $total_tax_included += get_rounded_price($$line{total_tax_included}); You are rounding here IN the loop. Shouldnt you do that outside ? - my $cost_tax_included = $line{unitprice_tax_included} || $line{ecost_tax_included}; - my $cost_tax_excluded = $line{unitprice_tax_excluded} || $line{ecost_tax_excluded}; - $line{total_tax_included} = get_rounded_price($cost_tax_included) * $line{quantity}; - $line{total_tax_excluded} = get_rounded_price($cost_tax_excluded) * $line{quantity}; + $line{total_tax_included} = get_rounded_price($line{ecost_tax_included} * $line{quantity}); + $line{total_tax_excluded} = get_rounded_price($line{ecost_tax_excluded} * $line{quantity}); This looks like a change that does a bit more than fixing the rounding. Why do you ignore $line{unitprice_tax_included} now ?