Bug 26078

Summary: "Item returns to issuing library" creates infinite transfer loop
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: CirculationAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: aleisha, gmcharlt, jean-manuel.broust, jonathan.druart, kyle.m.hall, lucas, martin.renvoize, Niamh.WalkerHeadon, nick, sally.healey, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28520
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.03, 19.11.10
Bug Depends on: 23129    
Bug Blocks:    
Attachments: Bug 26078: Prevent generating a new transfer from return policy when resolving a transfer
Bug 26078: Prevent generating a new transfer from return policy when resolving a transfer
Bug 26078: Prevent generating a new transfer from return policy when resolving a transfer
Bug 26078: (follow-up) Use a boolean rather than changing returnbranch
Bug 26078: (follow-up) used validTransfer instead of checking WrongTransfer message
Bug 26078: (follow-up) Use a boolean rather than changing returnbranch

Description Andrew Fuerste-Henry 2020-07-28 20:43:12 UTC
Bug 23129 changed ModItemTransfer to set the holding branch of an item to current location when generating a transfer. This doesn't play well with the return policy "Item returns to issuing library," which sends an item to its holding branch at checkin.

To recreate:
- Have 2+ branches
- Set your default return policy to Item returns to issuing library
- Find an item with holding branch of Branch A
- Set your library to Branch B
- Check item in
- Koha generates a transfer to Branch A. In doing so, it sets the item holding branch to Branch B
- Set your library to Branch A
- Check item in
- Koha generate a transfer to Branch B. In doing so, it sets the item holding branch to Branch A
- Set you library to Branch A
- Check the item in 
 . . . Repeat indefinitely.

It's easy enough to cancel a transfer to get out of this loop, but better not to get into it in the first place. That said, I'm not entirely sure how to avoid it. Can we somehow tell Koha that a checkin that ends a transfer should never start a new transfer? Unless there's a hold involved?
Comment 1 Nick Clemens 2020-07-30 17:00:56 UTC
Created attachment 107597 [details] [review]
Bug 26078: Prevent generating a new transfer from return policy when resolving a transfer

To test:
- Have 2+ branches
- Set your default return policy to Item returns to issuing library
- Find an item with holding branch of Branch A
- Set your library to Branch B
- Check item in
- Koha generates a transfer to Branch A. In doing so, it sets the item holding branch to Branch B
- Set your library to Branch A
- Check item in
- Koha generate a transfer to Branch B. In doing so, it sets the item holding branch to Branch A
- Set you library to Branch A
- Check the item in
 . . . Repeat indefinitely.
 Apply patch
 Repeat
 No more loop
Comment 2 ByWater Sandboxes 2020-07-31 09:26:59 UTC
Created attachment 107625 [details] [review]
Bug 26078: Prevent generating a new transfer from return policy when resolving a transfer

To test:
- Have 2+ branches
- Set your default return policy to Item returns to issuing library
- Find an item with holding branch of Branch A
- Set your library to Branch B
- Check item in
- Koha generates a transfer to Branch A. In doing so, it sets the item holding branch to Branch B
- Set your library to Branch A
- Check item in
- Koha generate a transfer to Branch B. In doing so, it sets the item holding branch to Branch A
- Set you library to Branch A
- Check the item in
 . . . Repeat indefinitely.
 Apply patch
 Repeat
 No more loop

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 3 Martin Renvoize 2020-08-13 14:01:40 UTC
Created attachment 108188 [details] [review]
Bug 26078: Prevent generating a new transfer from return policy when resolving a transfer

To test:
- Have 2+ branches
- Set your default return policy to Item returns to issuing library
- Find an item with holding branch of Branch A
- Set your library to Branch B
- Check item in
- Koha generates a transfer to Branch A. In doing so, it sets the item holding branch to Branch B
- Set your library to Branch A
- Check item in
- Koha generate a transfer to Branch B. In doing so, it sets the item holding branch to Branch A
- Set you library to Branch A
- Check the item in
 . . . Repeat indefinitely.
 Apply patch
 Repeat
 No more loop

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2020-08-13 14:02:55 UTC
Logic is sound, tests are added, works as expected.

Passing QA
Comment 5 Jonathan Druart 2020-08-14 10:46:23 UTC
If I understand correctly, by modifying the return branch we actually want to not transfer. I'd suggest to use a meaningful separated bool variable.

I noticed there is a $validTransfer variable that is not used, and its last used come from 0a64cfba5e10be80a220f6c8164905788ba5a78b where we have:

-    if ($doreturn and ($branch ne $hbr) and not $messages->{'WrongTransfer'} and ($validTransfert ne 1) ){                                                                                                       
+    if (($doreturn or $messages->{'NotIssued'}) and !$resfound and ($branch ne $hbr) and not $messages->{'WrongTransfer'}){

It seems that is was actually used for that.
Comment 6 Nick Clemens 2020-08-14 11:30:40 UTC
Created attachment 108259 [details] [review]
Bug 26078: (follow-up) Use a boolean rather than changing returnbranch
Comment 7 Jonathan Druart 2020-08-14 12:14:31 UTC
yes, that's the idea. However should not we unset validTransfer if WrongTransfer (and remove the test)?
Comment 8 Nick Clemens 2020-08-14 13:35:11 UTC
Created attachment 108278 [details] [review]
Bug 26078: (follow-up) used validTransfer instead of checking WrongTransfer message
Comment 9 Jonathan Druart 2020-08-18 12:52:36 UTC
Created attachment 108496 [details] [review]
Bug 26078: (follow-up) Use a boolean rather than changing returnbranch

Bug 26078: (follow-up) used validTransfer instead of checking WrongTransfer message
Comment 10 Jonathan Druart 2020-08-18 15:47:16 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 11 Lucas Gass 2020-09-03 14:42:48 UTC
backported to 20.05.x for 20.05.03
Comment 12 Aleisha Amohia 2020-09-09 04:03:24 UTC
backported to 19.11.x for 19.11.10
Comment 13 Victor Grousset/tuxayo 2020-09-20 17:07:37 UTC
Missing dependencies for 19.05.x, can't backport.
Comment 14 Victor Grousset/tuxayo 2020-10-11 11:22:50 UTC
Dependencies ok now.
Can't backport to 19.05.x: can't solve a conflict.
If there is an interest in having this backported, feel free to submit a patch for 19.05.

Conflict:


    # if we have a transfer to do, we update the line of transfers with the datearrived
    my $is_in_rotating_collection = C4::RotatingCollections::isItemInAnyCollection( $item->itemnumber );
    if ($datesent) {
        if ( $tobranch eq $branch ) {
            my $sth = C4::Context->dbh->prepare(
                "UPDATE branchtransfers SET datearrived = now() WHERE itemnumber= ? AND datearrived IS NULL"
            );
            $sth->execute( $item->itemnumber );
<<<<<<< HEAD
            # if we have a reservation with valid transfer, we can set it's status to 'W'
            C4::Reserves::ModReserveStatus($item->itemnumber, 'W');
||||||| parent of 1b2eedbea9... Bug 26078: Prevent generating a new transfer from return policy when resolving a transfer
=======

            # If we are completing a transfer we should not generate a new transfer from return policy
            $returnbranch = $branch;
>>>>>>> 1b2eedbea9... Bug 26078: Prevent generating a new transfer from return policy when resolving a transfer
        } else {
            $messages->{'WrongTransfer'}     = $tobranch;
            $messages->{'WrongTransferItem'} = $item->itemnumber;
        }
        $validTransfert = 1;
    }

    # fix up the accounts.....