Bug 18639

Summary: Separate replacement cost and retail price fields in acquisitions
Product: Koha Reporter: Nick Clemens <nick>
Component: AcquisitionsAssignee: Nick Clemens <nick>
Status: RESOLVED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: enhancement    
Priority: P5 - low CC: colin.campbell, jdemuth, jonathan.druart, katrin.fischer, kyle, laurence.rault, m.de.rooy, martin.renvoize
Version: masterKeywords: Manual
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18736
Whiteboard:
Change sponsored?: --- Patch complexity: Small patch
Bot Control: --- When did the bot last check this:
Who signed the patch off: Text to go in the release notes:
This patch attempts to remove some confusion as the 'Replacement price/rrp' field was being used as retail price during ordering process, but for item replacement price when receiving. This patch splits these fields so that each may be set independently. RRP will be used to determine costs while ordering, and replacement price will populate the items replacement price upon receiving.
Bug Depends on: 13321    
Bug Blocks: 21324    
Attachments: Bug 18639 - Add replacementprice field to aqorders
Bug 18639 - Schema updates (DO NOT PUSH)
Bug 18639 - Add replacementprice field to acquisitions workflow
Bug 18639 - Unit tests
patch to rename field in Koha/EDI.pm
Bug 18639 - Add replacementprice field to aqorders
Bug 18639 - Schema updates (DO NOT PUSH)
Bug 18639 - Add replacementprice field to acquisitions workflow
Bug 18639 - Unit tests
Bug 18639 Use replacementprice when creating order from quote
Bug 18639 - Add replacementprice field to aqorders
Bug 18639 - Schema updates (DO NOT PUSH)
Bug 18639 - Add replacementprice field to acquisitions workflow
Bug 18639 - Unit tests
Bug 18639 Use replacementprice when creating order from quote
Bug 18639: (follow-up) Populate replacement price from list price during ordering
Bug 18639: Alter DB update to populate replacementprice from rpp initially
Bug 18639: Add replacementprice field to aqorders
Bug 18639: Schema updates (DO NOT PUSH)
Bug 18639: Add replacementprice field to acquisitions workflow
Bug 18639: Unit tests
Bug 18639: Use replacementprice when creating order from quote
Bug 18639: (follow-up) Populate replacement price from list price during ordering
Bug 18639: Alter DB update to populate replacementprice from rpp initially
Bug 18639: (QA follow-up) Fix formatting of replacement cost when modifying an order
Bug 18639: (follow-up) Remove debugging code

Description Nick Clemens 2017-05-19 12:52:15 UTC
Work to be done:

1 - Add a field 'replacementprice' to the aqorders table
2 - Display this field as 'Replacement price' when ordering
3 - Rename current 'Replacement price' field for orders to 'Retail price'
3a - 'Retail price' field will only be editable during initial ordering
3b - If 'Retail price' field is update, dependent fields will also be updated (currently they are only updated on change of 'lsit price')
4 - Allow for replacementprice to be populated via ItemFieldsToOrder preferences
5 - Adjust EDI scripts to allow for use of 'replacementprice' field
6 - Pass 'replacementprice' in aqorders through as 'items.replacementprice' when creating items
7 - aqorders.replacementprice should be editable throughout entire acquisitions workflow
Comment 1 Laurence Rault 2017-05-23 07:20:37 UTC
The trouble is that now the budgeted cost (ecost) is calculated based on Replacement cost (rrp) and not (not only in fact) from vendor price (listprice) anymore. And I think it may be bug : 

When you change rrp in an order, budgeted cost does not change in the form, but after validation, values in the table for budgeted cost are re-calculated against the modified rrp.


Maybe we should consider alternatives : 

1) make the budgeted cost only calculated from vendor price ; if rrp is modified, no effect on ecost (as it is still the case in Koha 3.22). And then make the rrp editable through the whole workflow. Would it be enough ?

2) I agree with your proposition, with some questions : 
- what would be the difference between Vendor price and Retail price ?
- I am not at all familiar with EDI, but is the replacementprice sent or received to/by the vendor ? I think vendor price and/or actual cost should be (maybe the are already).
Comment 2 Jonathan Druart 2017-06-19 21:01:46 UTC
I am not sure I will have time to work on this one.
Comment 3 Nick Clemens 2017-12-29 16:52:08 UTC
Created attachment 70213 [details] [review]
Bug 18639 - Add replacementprice field to aqorders
Comment 4 Nick Clemens 2017-12-29 16:52:11 UTC
Created attachment 70214 [details] [review]
Bug 18639 - Schema updates (DO NOT PUSH)
Comment 5 Nick Clemens 2017-12-29 16:52:15 UTC
Created attachment 70215 [details] [review]
Bug 18639 - Add replacementprice field to acquisitions workflow

To test:
 0 - Apply patches and updatedatabase
 1 - Add an order to a basket
 2 - You should note new 'Retail price field'
 3 - You should have a separate 'Replacement price' field
 4 - Enter values and ensure they are saved as expected
 5 - In the basket you should see the replacement price
 6 - Modify order and ensure value is loaded and saved correctly
 7 - Add and cancle an order and esure replacement price shows/saves
 8 - Close basket
 9 - Receive an order
10 - You should be able to edit replacement price
11 - 'retail price' field is not editable
12 - Check associated item, replacement price in item should be updated
13 - Cancel receipt, check item. receive again with new replacement
price, check item
14 - Price should be correctly updated
15 - Finish receipt, value should show in table
16 - Test with receive from file
Comment 6 Nick Clemens 2017-12-29 16:52:17 UTC
Created attachment 70216 [details] [review]
Bug 18639 - Unit tests
Comment 7 Nick Clemens 2017-12-29 16:54:17 UTC
Couple things:
3b - Not so much possible - retail price is calculated from listprice and is converted. Changing this field doesn't actually save either. I think we can just make this not editable - but that is for another bug report

5 - I need an EDI person to look at the scripts and update as needed

Otherwise I think this should work as expected
Comment 8 Colin Campbell 2018-06-11 10:52:29 UTC
Created attachment 75946 [details] [review]
patch to rename field in Koha/EDI.pm

Change the column name in order creation to match schema name change
Comment 9 Martin Renvoize 2018-07-11 20:09:06 UTC
Created attachment 76867 [details] [review]
Bug 18639 - Add replacementprice field to aqorders

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2018-07-11 20:09:11 UTC
Created attachment 76868 [details] [review]
Bug 18639 - Schema updates (DO NOT PUSH)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2018-07-11 20:09:15 UTC
Created attachment 76869 [details] [review]
Bug 18639 - Add replacementprice field to acquisitions workflow

To test:
 0 - Apply patches and updatedatabase
 1 - Add an order to a basket
 2 - You should note new 'Retail price field'
 3 - You should have a separate 'Replacement price' field
 4 - Enter values and ensure they are saved as expected
 5 - In the basket you should see the replacement price
 6 - Modify order and ensure value is loaded and saved correctly
 7 - Add and cancle an order and esure replacement price shows/saves
 8 - Close basket
 9 - Receive an order
10 - You should be able to edit replacement price
11 - 'retail price' field is not editable
12 - Check associated item, replacement price in item should be updated
13 - Cancel receipt, check item. receive again with new replacement
price, check item
14 - Price should be correctly updated
15 - Finish receipt, value should show in table
16 - Test with receive from file

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2018-07-11 20:09:19 UTC
Created attachment 76870 [details] [review]
Bug 18639 - Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2018-07-11 20:09:22 UTC
Created attachment 76871 [details] [review]
Bug 18639 Use replacementprice when creating order from quote

Updates column name from rrp -> replacementprice when creating
order from EDI QUOTE message
Also ensure other rrp_tax* fields populated

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2018-07-11 20:12:12 UTC
All seems to work as expected and seems to make sense.

I look the liberty of re-basing it against master whilst signing off.
Comment 15 Marcel de Rooy 2018-07-13 09:31:59 UTC
Moving to enh
Please explain why it should be seen as a bugfix.
Comment 16 Nick Clemens 2018-07-13 10:58:06 UTC
(In reply to Marcel de Rooy from comment #15)
> Moving to enh
> Please explain why it should be seen as a bugfix.

The naming and use of this field has caused a lot of confusion for libraries - They enter a price here, expecting it to be used as the 'replacementprice' in the item record, however, it is used as the cost for the order. They generally expect the cost to be the vendor price or something else.

I consider a bug as it doesn't behave in the manner expected by the end user. Changes on 13321 also changed the way fields behaved (rrp is now basis for order calculation) and this restores some functionality that was expected by users.
Comment 17 Martin Renvoize 2018-07-13 12:19:47 UTC
From a RMaint perspective, I agree that I'd treat it as an enhancement. Although it's somewhat of a bugfix in that it makes functionality clearer, it is a change of said functionality which has been in stable branches for long enough that it would not be an expected change on a stable branch only receiving bugfixes.

I'd look at it as a positive being an enhancement.. we can advertise it as such for the next release and give it the attention it deserves in the release notes ;)
Comment 18 Martin Renvoize 2018-07-13 12:31:14 UTC
If I had my QA hat on, I'd be asking for a corresponding documentation update patch linked to here (as a merge request on the gitlab koha-manual) and some release notes text summarising the change.

I'd also be interested to understand whether we ought to also include a DB update script which moved the rrp value's into replacementprice field as that's what the functional change appears to do.
Comment 19 Katrin Fischer 2018-07-13 12:43:03 UTC
As it includes database changes and such I would not backport it. I think it's somewhat in a grey area between enh and bug. Too much of a change for a backport is my feel, but not a strict enh either.
Comment 20 Katrin Fischer 2018-08-16 20:55:58 UTC
Some notes from reading through the bug and code review:

1) Leftover warn in finishreceive.pl
+$replacementprice = Koha::Number::Price->new( $replacementprice )->unformat();
+warn "Replacement $replacementprice";

2) Database update
I also wonder if we should prefill replacementprice from rrp. We could argue that not all libraries will want it to be set to rrp by default, especially as it would change the behaviour for pending orders. 

3) Notes for for the release notes would be great. Nick?

4) Question: Why not allow editing of retail price later on? In case of a typo you'd have to reopen the basket and that will mess with the order dates and such... wouldn't it be easier to leave it editable?

I can remove the warn in a follow-up, still have to finish testing of the behaviour and calculations.
Comment 21 Nick Clemens 2018-08-17 11:36:17 UTC
(In reply to Katrin Fischer from comment #20)
> Some notes from reading through the bug and code review:
> 1) Leftover warn in finishreceive.pl

Indeed, apologies

> 2) Database update
> I also wonder if we should prefill replacementprice from rrp. We could argue
> that not all libraries will want it to be set to rrp by default, especially
> as it would change the behaviour for pending orders. 

I personally don't like the idea, it would preserve current behaviour for orders,  but I think we are trying to chnage that behaviour :-) I can be swayed here though as I am not sure the bets path - maybe just cover in release notes and allow libraries to decide

> 
> 3) Notes for for the release notes would be great. Nick?

Sure

> 
> 4) Question: Why not allow editing of retail price later on? In case of a
> typo you'd have to reopen the basket and that will mess with the order dates
> and such... wouldn't it be easier to leave it editable?

It is not editable before this patch, this simply preserves that behaviour - it can be modified, but that is for a future bug

> 
> I can remove the warn in a follow-up, still have to finish testing of the
> behaviour and calculations.
Thank you! Let me know what you find :-)
Comment 22 Katrin Fischer 2018-08-19 20:43:55 UTC
I think there is an issue here with the replacement price remaining empty on the form and the fact that before this patch, Koha did use the value in rrp to set the replacement price in the item.

I think we might be safer if we keep this behaviour: Use rrp if replacement price is not set.
Comment 23 Katrin Fischer 2018-08-19 20:59:20 UTC
Thinking about it some more... maybe the better option would be to fill in the field in the order form automatically (rrp = replacement cost), and then allow it to be adjusted. That would probably give the most control over the field and libraries could choose 0/empty.

My worry is adding more work/steps - with the current patch you have added steps to get to the same result as before.
Comment 24 Nick Clemens 2018-08-27 13:34:25 UTC
Created attachment 78184 [details] [review]
Bug 18639 - Add replacementprice field to aqorders

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Nick Clemens 2018-08-27 13:34:29 UTC
Created attachment 78185 [details] [review]
Bug 18639 - Schema updates (DO NOT PUSH)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Nick Clemens 2018-08-27 13:34:33 UTC
Created attachment 78186 [details] [review]
Bug 18639 - Add replacementprice field to acquisitions workflow

To test:
 0 - Apply patches and updatedatabase
 1 - Add an order to a basket
 2 - You should note new 'Retail price field'
 3 - You should have a separate 'Replacement price' field
 4 - Enter values and ensure they are saved as expected
 5 - In the basket you should see the replacement price
 6 - Modify order and ensure value is loaded and saved correctly
 7 - Add and cancle an order and esure replacement price shows/saves
 8 - Close basket
 9 - Receive an order
10 - You should be able to edit replacement price
11 - 'retail price' field is not editable
12 - Check associated item, replacement price in item should be updated
13 - Cancel receipt, check item. receive again with new replacement
price, check item
14 - Price should be correctly updated
15 - Finish receipt, value should show in table
16 - Test with receive from file

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Nick Clemens 2018-08-27 13:34:37 UTC
Created attachment 78187 [details] [review]
Bug 18639 - Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Nick Clemens 2018-08-27 13:34:41 UTC
Created attachment 78188 [details] [review]
Bug 18639 Use replacementprice when creating order from quote

Updates column name from rrp -> replacementprice when creating
order from EDI QUOTE message
Also ensure other rrp_tax* fields populated

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Nick Clemens 2018-08-27 13:34:44 UTC
Created attachment 78189 [details] [review]
Bug 18639: (follow-up) Populate replacement price from list price during ordering
Comment 30 Nick Clemens 2018-08-27 18:22:12 UTC
Created attachment 78191 [details] [review]
Bug 18639: Alter DB update to populate replacementprice from rpp initially
Comment 31 Katrin Fischer 2018-08-27 20:26:50 UTC
(In reply to Nick Clemens from comment #30)
> Created attachment 78191 [details] [review] [review]
> Bug 18639: Alter DB update to populate replacementprice from rpp initially

Hi Nick, hoping to get back to this soon. Why did you change your mind about the db update?
Comment 32 Katrin Fischer 2018-08-29 05:52:20 UTC
When you go back and modify the order, the replacement cost ist not properly formatted. 113.00 shows as 113.000000.
Comment 33 Katrin Fischer 2018-08-29 06:02:53 UTC
Created attachment 78249 [details] [review]
Bug 18639: Add replacementprice field to aqorders

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 34 Katrin Fischer 2018-08-29 06:02:58 UTC
Created attachment 78250 [details] [review]
Bug 18639: Schema updates (DO NOT PUSH)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 35 Katrin Fischer 2018-08-29 06:03:02 UTC
Created attachment 78251 [details] [review]
Bug 18639: Add replacementprice field to acquisitions workflow

To test:
 0 - Apply patches and updatedatabase
 1 - Add an order to a basket
 2 - You should note new 'Retail price field'
 3 - You should have a separate 'Replacement price' field
 4 - Enter values and ensure they are saved as expected
 5 - In the basket you should see the replacement price
 6 - Modify order and ensure value is loaded and saved correctly
 7 - Add and cancle an order and esure replacement price shows/saves
 8 - Close basket
 9 - Receive an order
10 - You should be able to edit replacement price
11 - 'retail price' field is not editable
12 - Check associated item, replacement price in item should be updated
13 - Cancel receipt, check item. receive again with new replacement
price, check item
14 - Price should be correctly updated
15 - Finish receipt, value should show in table
16 - Test with receive from file

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 36 Katrin Fischer 2018-08-29 06:03:07 UTC
Created attachment 78252 [details] [review]
Bug 18639: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 37 Katrin Fischer 2018-08-29 06:03:11 UTC
Created attachment 78253 [details] [review]
Bug 18639: Use replacementprice when creating order from quote

Updates column name from rrp -> replacementprice when creating
order from EDI QUOTE message
Also ensure other rrp_tax* fields populated

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 38 Katrin Fischer 2018-08-29 06:03:15 UTC
Created attachment 78254 [details] [review]
Bug 18639: (follow-up) Populate replacement price from list price during ordering

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 39 Katrin Fischer 2018-08-29 06:03:19 UTC
Created attachment 78255 [details] [review]
Bug 18639: Alter DB update to populate replacementprice from rpp initially

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 40 Katrin Fischer 2018-08-29 06:03:23 UTC
Created attachment 78256 [details] [review]
Bug 18639: (QA follow-up) Fix formatting of replacement cost when modifying an order
Comment 41 Nick Clemens 2018-08-31 12:58:23 UTC
Awesome work all!

Pushed to master for 18.11
Comment 42 Martin Renvoize 2018-09-05 14:39:58 UTC
Enhancement, will not be backported to 18.05.x series.
Comment 43 Jonathan Druart 2018-10-01 22:55:41 UTC
Debugging leftover in finishreceive.pl
+warn "Replacement $replacementprice";
Comment 44 Nick Clemens 2018-10-02 00:09:38 UTC
Created attachment 79789 [details] [review]
Bug 18639: (follow-up) Remove debugging code
Comment 45 Nick Clemens 2018-10-02 00:13:12 UTC
(In reply to Nick Clemens from comment #44)
> Created attachment 79789 [details] [review] [review]
> Bug 18639: (follow-up) Remove debugging code

Followup pushed to master, thanks Jonathan