Bug 23129

Summary: Items holdingbranch should be set to the originating library when generating a transfer
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: CirculationAssignee: Nick Clemens (kidclamp) <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: alexbuckley, bouzid.fergani, cbrannon, gmcharlt, kyle.m.hall, martin.renvoize, mfuerst, olli-antti.kivilahti, sally.healey, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12362
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00, 19.05.16
Circulation function:
Bug Depends on:    
Bug Blocks: 27692, 23695, 25883, 26078    
Attachments: Bug 23129: Unit test
Bug 23129: Set holdingbranch to library that initiates transfer
[SIGNED-OFF] Bug 23129: Unit test
[SIGNED-OFF] Bug 23129: Set holdingbranch to library that initiates transfer
Bug 23129: Unit test
Bug 23129: Set holdingbranch to library that initiates transfer
Bug 23129: Unit test
Bug 23129: Set holdingbranch to library that initiates transfer
Bug 23129: (RM follow-up) Fix failing test

Description Nick Clemens (kidclamp) 2019-06-14 18:11:59 UTC
In C4::Items::ModItemTransfer we call ModItem to update the holdingbranch of the item.

Currently we set this to the transfer destination. By definition we are generating a transfer because the item has not yet arrived. We should set the holdingbranch to the current location of the item
Comment 1 Nick Clemens (kidclamp) 2019-06-14 18:20:24 UTC
Created attachment 90619 [details] [review]
Bug 23129: Unit test
Comment 2 Nick Clemens (kidclamp) 2019-06-14 18:20:27 UTC
Created attachment 90620 [details] [review]
Bug 23129: Set holdingbranch to library that initiates transfer

To test:
1 - Find an item
2 - Check it in at anothe rbranch and initiate a transfer
3 - View the item details or check db - note that holdingbranch is set as the destination library
4 - Complete or cancel the transfer
5 - Apply patch
6 - Initiate a new transfer
7 - Note the holdingbranch is set to the initiating library
Comment 3 Claire Gravely 2019-07-12 13:41:28 UTC
Created attachment 91488 [details] [review]
[SIGNED-OFF] Bug 23129: Unit test

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 4 Claire Gravely 2019-07-12 13:41:32 UTC
Created attachment 91489 [details] [review]
[SIGNED-OFF] Bug 23129: Set holdingbranch to library that initiates transfer

To test:
1 - Find an item
2 - Check it in at anothe rbranch and initiate a transfer
3 - View the item details or check db - note that holdingbranch is set as the destination library
4 - Complete or cancel the transfer
5 - Apply patch
6 - Initiate a new transfer
7 - Note the holdingbranch is set to the initiating library

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 5 Bouzid Fergani 2019-09-20 19:31:21 UTC
Created attachment 93043 [details] [review]
Bug 23129: Unit test

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 6 Bouzid Fergani 2019-09-20 19:31:36 UTC
Created attachment 93045 [details] [review]
Bug 23129: Set holdingbranch to library that initiates transfer

To test:
1 - Find an item
2 - Check it in at anothe rbranch and initiate a transfer
3 - View the item details or check db - note that holdingbranch is set as the destination library
4 - Complete or cancel the transfer
5 - Apply patch
6 - Initiate a new transfer
7 - Note the holdingbranch is set to the initiating library

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 7 Katrin Fischer 2019-09-30 09:49:49 UTC
This works for transfers initiated by holds and on returning the item at 'not the home library'. What's missing is the same behavior manual transfers initiated from the circulation home page. I have talked to Nick and will file a new bug for that. I suggest that both should be fixed/pushed pre-release so we have the same beahviour for all kinds of transfers:

Bug 23695 - Items holdingbranch should be set to the originating library when generating a manual transfer
Comment 8 Katrin Fischer 2019-09-30 09:50:29 UTC
Created attachment 93227 [details] [review]
Bug 23129: Unit test

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Katrin Fischer 2019-09-30 09:50:33 UTC
Created attachment 93228 [details] [review]
Bug 23129: Set holdingbranch to library that initiates transfer

To test:
1 - Find an item
2 - Check it in at anothe rbranch and initiate a transfer
3 - View the item details or check db - note that holdingbranch is set as the destination library
4 - Complete or cancel the transfer
5 - Apply patch
6 - Initiate a new transfer
7 - Note the holdingbranch is set to the initiating library

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2019-10-14 06:00:43 UTC
Apparently I forgot to switch the status...
Comment 11 Martin Renvoize (ashimema) 2019-10-15 13:16:35 UTC
Nice work!

Pushed to master for 19.11.00
Comment 12 Martin Renvoize (ashimema) 2019-10-15 15:14:54 UTC
Hmm, so this breaks a test which expects the opposite behaviour when dealing with items getting marked as lost from the transfers window.

Question is, is the test incorrect or the functionality?
Comment 13 Martin Renvoize (ashimema) 2019-10-15 15:18:59 UTC
CC'd Alex Buckley as he wrote the tests that's currently failing.. I'd be interested in his thoughts regarding what the holdingbranch of the item should end up as during lost in transfer processes.
Comment 14 Martin Renvoize (ashimema) 2019-10-15 15:51:11 UTC
Created attachment 94219 [details] [review]
Bug 23129: (RM follow-up) Fix failing test

This patch corrects a test that was testing for the oposite behaviour to
that introduced in this patchset.  We have agreed on the bug that the
new behaviour is desired and so I have updated the test to follow.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize (ashimema) 2019-10-15 15:52:01 UTC
Discussed with Nick, the tests was wrong and so I have updated it in a followup.
Comment 16 Katrin Fischer 2020-01-08 22:33:54 UTC
*** Bug 15439 has been marked as a duplicate of this bug. ***
Comment 17 Martha Fuerst 2020-01-08 23:08:49 UTC
Any thoughts on backporting to 19.05?

-Marti
Huntsville-Madison County Public Library
Comment 18 Christopher Brannon 2020-01-08 23:17:30 UTC
(In reply to Martha Fuerst from comment #17)
> Any thoughts on backporting to 19.05?
> 
> -Marti
> Huntsville-Madison County Public Library

+1
Comment 19 Katrin Fischer 2020-01-08 23:44:53 UTC
*** Bug 15892 has been marked as a duplicate of this bug. ***
Comment 20 Victor Grousset/tuxayo 2020-09-20 17:18:19 UTC
(In reply to Martha Fuerst from comment #17)
> Any thoughts on backporting to 19.05?

Hi next (i.e. current) 19.05.x maintainer here. Is there there still interest in seeing this backported?
I could try to backport this and bug 26078 if you, Christopher or anyone else would be available to test that it doesn't cause regressions.
Comment 21 Martha Fuerst 2020-09-21 15:27:49 UTC
Yes, please. As many people won't see 19.11 until next spring and this fix greatly affects systems that do lots of transfers, I think it would be a very good idea to backport.

Thanks!
Comment 22 Victor Grousset/tuxayo 2020-10-11 11:11:03 UTC
Backported to 19.05.x branch for 19.05.16