Bug 27203

Summary: Order unitprice is not set anymore and totals are 0
Product: Koha Reporter: Didier Gautheron <didier.gautheron>
Component: AcquisitionsAssignee: Didier Gautheron <didier.gautheron>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: andrewfh, fridolin.somers, jean-manuel.broust, jonathan.druart, marjorie.barry-vila, martin.renvoize, nick, victor
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.06,20.05.12,19.11.18
Bug Depends on: 23294    
Bug Blocks:    
Attachments: Bug 27203: (Bug 23294 follow up) set unitprice to ecost if it's 0.
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set
Bug 27203: (Bug 23294 follow up) set unitprice to ecost if it's 0.
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set
sandbox_ecost
Bug 27203: (Bug 23294 follow up) set unitprice to ecost if it's 0.
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set
Bug 27203: Adjust unit tests
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set
Bug 27203: Adjust unit tests
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set
Changes after the patch
Bug 27203: Adjust unit tests
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set

Description Didier Gautheron 2020-12-11 11:58:58 UTC
After 23294 commit unitprice is not undefined or equal to '' but is 0. and it's not set to ecost in acqui/addorder.pl.

Either set uniprice to '' or if 0. use ecost.
Comment 1 Didier Gautheron 2020-12-11 13:33:55 UTC
Created attachment 114342 [details] [review]
Bug 27203: (Bug 23294 follow up) set unitprice to ecost if it's 0.

Test plan:
1 - Create a basket
2 - Add an order line, don't set 'Actual cost: ', but use vendor price
and a discount.
3 - Save
4 - Display all columns, 'Actual cost tax inc.' are GST columns aren't set.
5 - Apply batch
6 - Redo 1,2, 3
7 - Display all columns, pre 23294 behaviour is back columns are set.
Comment 2 Nick Clemens 2020-12-17 12:31:55 UTC
Created attachment 114476 [details] [review]
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set

There is code in populate_order_with_prices that is intended to use ecost over unitprice,
it just doens't seem to be working.

Making it more explicit seems to take care of the issue.

To test:
1 - Create a basket
2 - Add an order line, don't set 'Actual cost: ', but use vendor price
and a discount.
3 - Save
4 - Display all columns, 'Actual cost tax inc.' are GST columns aren't set.
5 - Apply batch
6 - Redo 1,2, 3
7 - Display all columns, GST is calculated correctly, unitprice remains 0.
Comment 3 PTFS Europe Sandboxes 2021-04-23 11:09:06 UTC
Created attachment 120051 [details] [review]
Bug 27203: (Bug 23294 follow up) set unitprice to ecost if it's 0.

Test plan:
1 - Create a basket
2 - Add an order line, don't set 'Actual cost: ', but use vendor price
and a discount.
3 - Save
4 - Display all columns, 'Actual cost tax inc.' are GST columns aren't set.
5 - Apply batch
6 - Redo 1,2, 3
7 - Display all columns, pre 23294 behaviour is back columns are set.

Signed-off-by: Marjorie <marjorie.barry-vila@collecto.ca>
Comment 4 PTFS Europe Sandboxes 2021-04-23 11:09:10 UTC
Created attachment 120052 [details] [review]
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set

There is code in populate_order_with_prices that is intended to use ecost over unitprice,
it just doens't seem to be working.

Making it more explicit seems to take care of the issue.

To test:
1 - Create a basket
2 - Add an order line, don't set 'Actual cost: ', but use vendor price
and a discount.
3 - Save
4 - Display all columns, 'Actual cost tax inc.' are GST columns aren't set.
5 - Apply batch
6 - Redo 1,2, 3
7 - Display all columns, GST is calculated correctly, unitprice remains 0.

Signed-off-by: Marjorie <marjorie.barry-vila@collecto.ca>
Comment 5 Marjorie Barry-Vila 2021-04-23 11:12:58 UTC
Created attachment 120053 [details]
sandbox_ecost
Comment 6 Victor Grousset/tuxayo 2021-04-28 00:30:45 UTC
Looks signed-off

Should the choice between the two alternatives be done now? Or is it for QA to make it?
Maybe there was talk about this ticket elsewhere?
Comment 7 Victor Grousset/tuxayo 2021-04-28 01:52:27 UTC
Wait, IIUC sandboxes don't allow to apply only one patch. Retest results coming.
Comment 8 Victor Grousset/tuxayo 2021-04-28 01:56:00 UTC
Created attachment 120249 [details] [review]
Bug 27203: (Bug 23294 follow up) set unitprice to ecost if it's 0.

Test plan:
1 - Create a basket
2 - Add an order line, don't set 'Actual cost: ', but use vendor price
and a discount.
3 - Save
4 - Display all columns, 'Actual cost tax inc.' are GST columns aren't set.
5 - Apply batch
6 - Redo 1,2, 3
7 - Display all columns, pre 23294 behaviour is back columns are set.

Signed-off-by: Marjorie <marjorie.barry-vila@collecto.ca>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 9 Victor Grousset/tuxayo 2021-04-28 01:56:05 UTC
Created attachment 120250 [details] [review]
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set

There is code in populate_order_with_prices that is intended to use ecost over unitprice,
it just doens't seem to be working.

Making it more explicit seems to take care of the issue.

To test:
1 - Create a basket
2 - Add an order line, don't set 'Actual cost: ', but use vendor price
and a discount.
3 - Save
4 - Display all columns, 'Actual cost tax inc.' are GST columns aren't set.
5 - Apply batch
6 - Redo 1,2, 3
7 - Display all columns, GST is calculated correctly, unitprice remains 0.

Signed-off-by: Marjorie <marjorie.barry-vila@collecto.ca>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 10 Victor Grousset/tuxayo 2021-04-28 01:58:37 UTC
Both patches do what they advertise. Including the difference on step 7 (I missed it the first when testing)
Comment 11 Jonathan Druart 2021-05-05 07:37:22 UTC
Nick, can you confirm your patch is independent from the first one?

Tests are missing.
Comment 12 Nick Clemens 2021-05-05 15:37:55 UTC
Created attachment 120551 [details] [review]
Bug 27203: Adjust unit tests

It seems the issue here is that the price passed in is a string, and not a number, so the tax
value is not calculated when no unitprice is provided
Comment 13 Nick Clemens 2021-05-05 15:37:58 UTC
Created attachment 120552 [details] [review]
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set

There is code in populate_order_with_prices that is intended to use ecost over unitprice,
it just doens't seem to be working.

Making it more explicit seems to take care of the issue.

To test:
1 - Create a basket
2 - Add an order line, don't set 'Actual cost: ', but use vendor price
and a discount.
3 - Save
4 - Display all columns, 'Actual cost tax inc.' are GST columns aren't set.
5 - Apply batch
6 - Redo 1,2, 3
7 - Display all columns, GST is calculated correctly, unitprice remains 0.

Signed-off-by: Marjorie <marjorie.barry-vila@collecto.ca>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 14 Nick Clemens 2021-05-05 15:38:25 UTC
(In reply to Jonathan Druart from comment #11)
> Nick, can you confirm your patch is independent from the first one?
> 
> Tests are missing.

They are independent, I obsoleted the first and added tests
Comment 15 Victor Grousset/tuxayo 2021-05-08 19:19:55 UTC
> unitprice remains 0.

More like 'Actual cost tax inc.' right?
Comment 16 Victor Grousset/tuxayo 2021-05-08 19:20:41 UTC
Created attachment 120724 [details] [review]
Bug 27203: Adjust unit tests

It seems the issue here is that the price passed in is a string, and not a number, so the tax
value is not calculated when no unitprice is provided

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 17 Victor Grousset/tuxayo 2021-05-08 19:20:45 UTC
Created attachment 120725 [details] [review]
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set

There is code in populate_order_with_prices that is intended to use ecost over unitprice,
it just doens't seem to be working.

Making it more explicit seems to take care of the issue.

To test:
1 - Create a basket
2 - Add an order line, don't set 'Actual cost: ', but use vendor price
and a discount.
3 - Save
4 - Display all columns, 'Actual cost tax inc.' are GST columns aren't set.
5 - Apply batch
6 - Redo 1,2, 3
7 - Display all columns, GST is calculated correctly, unitprice remains 0.

Signed-off-by: Marjorie <marjorie.barry-vila@collecto.ca>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 18 Victor Grousset/tuxayo 2021-05-08 19:21:20 UTC
Still works. Tests work.
Comment 19 Katrin Fischer 2021-05-09 12:22:16 UTC
I am sorry, but I fail to understand the problem here :(

I assume that we are looking at the basket summary page. Correct?

I created 2 orders, one pre-patch and one post-patch, actual cost was left empty.

The "Actual cost" (incl. or excl.) look exactly the same to me, both showing 0.00.

Can you please help me figure this one out?
Comment 20 Victor Grousset/tuxayo 2021-05-09 20:47:20 UTC
> The "Actual cost" (incl. or excl.) look exactly the same to me, both showing 0.00.

IIUC it's what the patch does. It's different from the 1st patch which not only made the GST to be set but also the Actual cost tax inc.

===

Something I'm not sure is:
> 7 - Display all columns, GST is calculated correctly, unitprice remains 0.

unitprice? I expected "Actual cost tax inc." to be mentioned instead.
Comment 21 Katrin Fischer 2021-05-09 20:48:16 UTC
To me the table with the prices (basket summary) looks exactly the same with and without patch...so that's why I am confused.
Comment 22 Nick Clemens 2021-05-10 11:27:29 UTC
Created attachment 120765 [details]
Changes after the patch

Existing orders are not affected - the 'Actual cost/unitprice' is not affected (0 is correct)

GST is not calculated before patch, but is correct after patch, based on the 'Budgeted cost/ecost' an is correct in totals

Orders before the patch are not affected, but will correctly calculate on receipt or edit
Comment 23 Victor Grousset/tuxayo 2021-05-10 17:07:16 UTC
(In reply to Katrin Fischer from comment #21)
> To me the table with the prices (basket summary) looks exactly the same with
> and without patch...so that's why I am confused.

Confusing indeed. When testing I had the new orders having GST calculated with the patch. That was the difference.

comment 22: yep, that's what I had. (didn't tested receipt or edit)
Comment 24 Katrin Fischer 2021-05-10 21:32:32 UTC
4 - Display all columns, 'Actual cost tax inc.' are GST columns aren't set.

That is the line that realy confused me. I kept looking at the Actual cost columns, but you actually need to look at the GST column!
Comment 25 Katrin Fischer 2021-05-10 21:35:10 UTC
Created attachment 120814 [details] [review]
Bug 27203: Adjust unit tests

It seems the issue here is that the price passed in is a string, and not a number, so the tax
value is not calculated when no unitprice is provided

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Katrin Fischer 2021-05-10 21:35:16 UTC
Created attachment 120815 [details] [review]
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set

There is code in populate_order_with_prices that is intended to use ecost over unitprice,
it just doens't seem to be working.

Making it more explicit seems to take care of the issue.

To test:
1 - Create a basket
2 - Add an order line, don't set 'Actual cost: ', but use vendor price
and a discount.
3 - Save
4 - Display all columns, 'Actual cost tax inc.' are GST columns aren't set.
5 - Apply batch
6 - Redo 1,2, 3
7 - Display all columns, GST is calculated correctly, unitprice remains 0.

Signed-off-by: Marjorie <marjorie.barry-vila@collecto.ca>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Victor Grousset/tuxayo 2021-05-10 22:42:40 UTC
Thanks for the test and QA :D
Comment 28 Jonathan Druart 2021-05-11 13:39:21 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 29 Fridolin Somers 2021-05-12 15:10:41 UTC
Pushed to 20.11.x for 20.11.06
Comment 30 Andrew Fuerste-Henry 2021-05-25 13:16:59 UTC
Pushed to 20.05.x for 20.05.12
Comment 31 Victor Grousset/tuxayo 2021-05-25 14:42:28 UTC
Backported: Pushed to 19.11.x branch for 19.11.18