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?
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
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>
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>
Logic is sound, tests are added, works as expected. Passing QA
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.
Created attachment 108259 [details] [review] Bug 26078: (follow-up) Use a boolean rather than changing returnbranch
yes, that's the idea. However should not we unset validTransfer if WrongTransfer (and remove the test)?
Created attachment 108278 [details] [review] Bug 26078: (follow-up) used validTransfer instead of checking WrongTransfer message
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
Pushed to master for 20.11, thanks to everybody involved!
backported to 20.05.x for 20.05.03
backported to 19.11.x for 19.11.10
Missing dependencies for 19.05.x, can't backport.
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.....