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.
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.
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.
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>
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>
Created attachment 120053 [details] sandbox_ecost
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?
Wait, IIUC sandboxes don't allow to apply only one patch. Retest results coming.
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>
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>
Both patches do what they advertise. Including the difference on step 7 (I missed it the first when testing)
Nick, can you confirm your patch is independent from the first one? Tests are missing.
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
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>
(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
> unitprice remains 0. More like 'Actual cost tax inc.' right?
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>
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>
Still works. Tests work.
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?
> 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.
To me the table with the prices (basket summary) looks exactly the same with and without patch...so that's why I am confused.
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
(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)
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!
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>
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>
Thanks for the test and QA :D
Pushed to master for 21.05, thanks to everybody involved!
Pushed to 20.11.x for 20.11.06
Pushed to 20.05.x for 20.05.12
Backported: Pushed to 19.11.x branch for 19.11.18