Bug 28627 - Revert the order receive page to display 'Actual cost' as ecost_tax_included/ecost_tax_excluded if unitprice not set
Summary: Revert the order receive page to display 'Actual cost' as ecost_tax_included/...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Alex Buckley
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on: 23376
Blocks:
  Show dependency treegraph
 
Reported: 2021-06-24 10:12 UTC by Alex Buckley
Modified: 2021-11-22 01:40 UTC (History)
14 users (show)

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


Attachments
Bug 28627: Fallback to displaying ecost_tax_included, ecost_tax_excluded in orderreceive.pl (2.79 KB, patch)
2021-06-24 10:34 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 28627: [alternate] Calculate unitprice if not set (2.89 KB, patch)
2021-11-12 17:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28627: [alternate] Calculate unitprice if not set (2.96 KB, patch)
2021-11-12 21:22 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28627: [alternate] Calculate unitprice if not set (3.10 KB, patch)
2021-11-13 12:37 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2021-06-24 10:12:54 UTC
Currently, the order receive page (orderreceive.tt) displays unitprice_tax_included/unitprice_tax_excluded in the 'Actual cost' field. However, if the user has not set a unitprice previously in the acquisitions workflow then the 'Actual cost' is displayed with a value of 0.000

In Koha 20.05.x the 'Actual cost' was displayed as ecost_tax_included/ecost_tax_excluded if unitprice was not defined.

Like in Bug 25750 , Koha should fallback to using ecost_tax_included and ecost_tax_excluded if unitprice is not defined.
Comment 1 Alex Buckley 2021-06-24 10:34:08 UTC
Created attachment 122380 [details] [review]
Bug 28627: Fallback to displaying ecost_tax_included, ecost_tax_excluded in orderreceive.pl

Test plan:
1. Create a basket and adding items to it. Set the 'Vendor price' to
'20', do not add an 'Actual cost'
2. Close the basket
3. Select 'Receive shipment'
4. Set a value for vendor invoice
5. Receive the order you created
6. Observe 'Actual cost' is = 0.00
7. Apply patch
8. Refresh the order receival page, and confirm the 'Actual cost' =
20.00

Sponsored-By: South Taranaki District Council, NZ
Comment 2 Julien Sicot (Univ. Rennes 2) 2021-10-08 06:36:18 UTC
Hi Alex,

Thank you this patch, I just tested it (master and 20.11), everything applies well and fixes the issue.

Thank you

Julien Sicot
Systems Librarian
Université Rennes 2
Comment 3 Donna 2021-10-08 17:42:20 UTC
This is not working for me.

1. Create a basket
2. Add item.  Set Vendor price to 20, retail price to 30, and replacement price to 40
3. Close basket
4.  View the Ordered items in /cgi-bin/koha/acqui/ordered.pl for the Fund you assigned the item to.  Note the Estimated cost is 30, which is the retail price, not the vendor price
4.  Receive the item.  The Retail price is correct (30), the replacement price is correct (40), but the Budgeted cost is wrong (30, should be 20), and the actual cost is wrong (30, should be 20).
Comment 4 Nick Clemens 2021-11-12 17:21:35 UTC
Created attachment 127592 [details] [review]
Bug 28627: [alternate] Calculate unitprice if not set

On bug 23376 the developer was too clever by half. Instead of passing
values to the template we moved to simply passing the order object.

The calculations that populated the unitprice, however, were simply dropped.

This patch restores the behaviour of setting unitprice to the estimated cost
by default. We do this conditionally in the template

To test:
 1 - Create a basket and adding items to it. Set the 'Vendor price' to
    '20', do not add an 'Actual cost'
 2 - Close the basket
 3 - Select 'Receive shipment'
 4 - Set a value for vendor invoice
 5 - Receive the order you created
 6 - Observe 'Actual cost' is = 0.00
 7 - Apply patch
 8 - Refresh the order receival page, and confirm the 'Actual cost' =
    20.00
 9 - Cancel receipt - reopen basket - edit order - add an actual cost - close
10 - Receive the order again
11 - Confirm the actual price is used
12 - Repeat this whole plan, but ensure the vendor has a different value for
     Invoice prices: Include tax
Comment 5 Nick Clemens 2021-11-12 17:29:53 UTC
(In reply to Alex Buckley from comment #1)
> Created attachment 122380 [details] [review] [review]
> Bug 28627: Fallback to displaying ecost_tax_included, ecost_tax_excluded in
> orderreceive.pl

I think the logic here is was backward
IF (order.unitprice_tax_included == 0.00000) THEN order.unitprice_tax_included  ELSE order.ecost_tax_included

I submitted an alternate
Comment 6 Andrew Fuerste-Henry 2021-11-12 21:22:10 UTC
Created attachment 127597 [details] [review]
Bug 28627: [alternate] Calculate unitprice if not set

On bug 23376 the developer was too clever by half. Instead of passing
values to the template we moved to simply passing the order object.

The calculations that populated the unitprice, however, were simply dropped.

This patch restores the behaviour of setting unitprice to the estimated cost
by default. We do this conditionally in the template

To test:
 1 - Create a basket and adding items to it. Set the 'Vendor price' to
    '20', do not add an 'Actual cost'
 2 - Close the basket
 3 - Select 'Receive shipment'
 4 - Set a value for vendor invoice
 5 - Receive the order you created
 6 - Observe 'Actual cost' is = 0.00
 7 - Apply patch
 8 - Refresh the order receival page, and confirm the 'Actual cost' =
    20.00
 9 - Cancel receipt - reopen basket - edit order - add an actual cost - close
10 - Receive the order again
11 - Confirm the actual price is used
12 - Repeat this whole plan, but ensure the vendor has a different value for
     Invoice prices: Include tax

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 7 Joonas Kylmälä 2021-11-13 12:37:33 UTC
Created attachment 127604 [details] [review]
Bug 28627: [alternate] Calculate unitprice if not set

On bug 23376 the developer was too clever by half. Instead of passing
values to the template we moved to simply passing the order object.

The calculations that populated the unitprice, however, were simply dropped.

This patch restores the behaviour of setting unitprice to the estimated cost
by default. We do this conditionally in the template

To test:
 1 - Create a basket and adding items to it. Set the 'Vendor price' to
    '20', do not add an 'Actual cost'
 2 - Close the basket
 3 - Select 'Receive shipment'
 4 - Set a value for vendor invoice
 5 - Receive the order you created
 6 - Observe 'Actual cost' is = 0.00
 7 - Apply patch
 8 - Refresh the order receival page, and confirm the 'Actual cost' =
    20.00
 9 - Cancel receipt - reopen basket - edit order - add an actual cost - close
10 - Receive the order again
11 - Confirm the actual price is used
12 - Repeat this whole plan, but ensure the vendor has a different value for
     Invoice prices: Include tax

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 8 Jonathan Druart 2021-11-15 11:39:26 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 9 Sonia Bouis 2021-11-15 15:40:26 UTC
Sorry, but I found a problem with this patch.

I have just tried and compared with a master from this morning.
The preference system AcqCreateItem is on "placing an order"

If I tried to save an order with this patch with an item where all mandatory item are filled, I have the message : "Form not submitted because of the following problem(s): 
- 4 item mandatory fields empty".

If I tried the same on a master branch, my order is saved normally.

Can you reproduce this issue ?
Sonia
Comment 10 Nick Clemens 2021-11-16 12:24:11 UTC
(In reply to Sonia Bouis from comment #9)
> Sorry, but I found a problem with this patch.
> 
> I have just tried and compared with a master from this morning.
> The preference system AcqCreateItem is on "placing an order"
> 
> If I tried to save an order with this patch with an item where all mandatory
> item are filled, I have the message : "Form not submitted because of the
> following problem(s): 
> - 4 item mandatory fields empty".
> 
> If I tried the same on a master branch, my order is saved normally.
> 
> Can you reproduce this issue ?
> Sonia

I don't recreate Sonia. This patch only touches the template on the receiving page. Can you provide a few more steps, or a screenshot?
Comment 11 Koha Team University Lyon 3 2021-11-16 17:24:30 UTC
Hi Nick,
The issue exists but I can reproduce it on the master git branch dating from yesterday (I don't understand why it has worked yesterday.. damn cache..) and on a sandbox.
So, it's not linked to this patch, sorry for the wrong alert.

I'm going to open a new BZ.

(By the way, I haven't managed to have the sandbox provided on the bywatersolutions sandbox system)
Comment 12 Jonathan Druart 2021-11-18 08:08:10 UTC
For the record, Sonia reported on bug 29496.
Comment 13 Kyle M Hall 2021-11-19 16:28:21 UTC
Pushed to 21.05.x for 21.05.05
Comment 14 Fridolin Somers 2021-11-20 00:02:59 UTC
Pushed to 20.11.x for 20.11.12
Comment 15 Victor Grousset/tuxayo 2021-11-22 01:40:55 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.