Bug 35114 - Calculation around basket details and basketgroup not correct
Summary: Calculation around basket details and basketgroup not correct
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Thibaud Guillot
QA Contact: Testopia
URL:
Keywords: no-sandbox
Depends on:
Blocks:
 
Reported: 2023-10-20 08:04 UTC by Thibaud Guillot
Modified: 2024-04-15 07:48 UTC (History)
5 users (show)

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


Attachments
Bug 35114: Adjust logic around basket total calculation (1.96 KB, patch)
2023-10-20 08:16 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 35114: Fix calculation around basketgroup and basket details (4.21 KB, patch)
2024-01-16 15:17 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 35114: Fix calculation around basketgroup and basket details (4.43 KB, patch)
2024-01-31 14:54 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 35114: Fix calculation around basketgroup and basket details (6.04 KB, patch)
2024-02-07 13:03 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 35114: Fix calculation around basketgroup and basket details (6.24 KB, patch)
2024-02-07 13:20 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 35114: Fix calculation around basketgroup and basket details (4.91 KB, patch)
2024-02-12 16:04 UTC, Thibaud Guillot
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Thibaud Guillot 2023-10-20 08:04:32 UTC
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).
Comment 1 Thibaud Guillot 2023-10-20 08:16:39 UTC Comment hidden (obsolete)
Comment 2 Philip Orr 2023-12-04 14:26:44 UTC
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!
Comment 3 Thibaud Guillot 2023-12-04 14:37:58 UTC
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 ;)
Comment 4 Philip Orr 2023-12-08 11:52:16 UTC
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.
Comment 5 Sabrina Kiehl 2023-12-14 11:18:09 UTC
Hi,
I wasn't able to reproduce the problem, too. So please add more testing instructions.
Thanks
Comment 6 Thibaud Guillot 2024-01-16 15:17:45 UTC
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)
Comment 7 Thibaud Guillot 2024-01-16 15:19:48 UTC
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.
Comment 8 Thibaud Guillot 2024-01-31 14:54:28 UTC
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
Comment 9 Thibaud Guillot 2024-01-31 14:55:51 UTC
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
Comment 10 Thibaud Guillot 2024-01-31 14:57:00 UTC
I forgot to tell which VAT was applied: 5.5%
Comment 11 Thibaud Guillot 2024-01-31 15:20:19 UTC
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.
Comment 12 Thibaud Guillot 2024-02-07 13:03:11 UTC Comment hidden (obsolete)
Comment 13 Thibaud Guillot 2024-02-07 13:11:19 UTC Comment hidden (obsolete)
Comment 14 Thibaud Guillot 2024-02-07 13:20:53 UTC Comment hidden (obsolete)
Comment 15 Thibaud Guillot 2024-02-12 16:04:39 UTC
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
Comment 16 Myka Kennedy Stephens 2024-03-05 00:57:43 UTC
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!
Comment 17 Koha Team University Lyon 3 2024-04-12 08:21:33 UTC
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
Comment 18 Koha Team University Lyon 3 2024-04-12 09:38:09 UTC
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
Comment 19 Thibaud Guillot 2024-04-15 07:48:07 UTC
(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.