Bug 27203 - Order unitprice is not set anymore and totals are 0
Summary: Order unitprice is not set anymore and totals are 0
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Didier Gautheron
QA Contact: Testopia
URL:
Keywords:
Depends on: 23294
Blocks:
  Show dependency treegraph
 
Reported: 2020-12-11 11:58 UTC by Didier Gautheron
Modified: 2022-06-06 20:27 UTC (History)
8 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:
21.05.00,20.11.06,20.05.12,19.11.18


Attachments
Bug 27203: (Bug 23294 follow up) set unitprice to ecost if it's 0. (1.21 KB, patch)
2020-12-11 13:33 UTC, Didier Gautheron
Details | Diff | Splinter Review
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set (2.33 KB, patch)
2020-12-17 12:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27203: (Bug 23294 follow up) set unitprice to ecost if it's 0. (1.27 KB, patch)
2021-04-23 11:09 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set (2.39 KB, patch)
2021-04-23 11:09 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
sandbox_ecost (186.11 KB, image/png)
2021-04-23 11:12 UTC, Marjorie Barry-Vila
Details
Bug 27203: (Bug 23294 follow up) set unitprice to ecost if it's 0. (1.32 KB, patch)
2021-04-28 01:56 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set (2.45 KB, patch)
2021-04-28 01:56 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 27203: Adjust unit tests (953 bytes, patch)
2021-05-05 15:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set (2.45 KB, patch)
2021-05-05 15:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27203: Adjust unit tests (1012 bytes, patch)
2021-05-08 19:20 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set (2.45 KB, patch)
2021-05-08 19:20 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Changes after the patch (118.97 KB, image/png)
2021-05-10 11:27 UTC, Nick Clemens
Details
Bug 27203: Adjust unit tests (1.04 KB, patch)
2021-05-10 21:35 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27203: [alternate] Calculate tax based on ecost if unitprice not set (2.50 KB, patch)
2021-05-10 21:35 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 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