From e73cbc6af94c69d97a1dd7abbe67c73bb45894a6 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 22 Oct 2025 14:56:43 +0100 Subject: [PATCH] Bug 28527: Handle cancelled in-transit transfers This patch addresses the issue where items with cancelled in-transit transfers become stuck in an inconsistent state, preventing new transfers from being created and causing confusion at checkin. Problem: -------- When a hold is cancelled while an item is in transit, the transfer is cancelled but remains in an incomplete state (datesent populated, datecancelled populated, datearrived not set). This causes: 1. Items cannot have new transfers created (blocked by cancelled transfer) 2. Checking in items at wrong locations doesn't provide clear messaging 3. No automatic workflow to return items to their original location 4. Cancelled transfers disappear from the system visibility Solution: --------- This patch implements a complete workflow for handling cancelled in-transit transfers: 1. Koha::Item::Transfer->is_cancelled() - New method to check if a transfer has been cancelled 2. Koha::Item::Transfer->in_transit() - Modified to return true even for cancelled transfers (now only checks datesent and datearrived) 3. Koha::Item->get_transfers() - Modified to include cancelled transfers in the result set, making them visible in the system 4. Koha::Item->get_transfer() - Modified to skip over cancelled transfers and return the first active transfer 5. Koha::Hold->cancel() - When cancelling a hold, now automatically cancels the associated in-transit transfer with reason 'CancelReserve' 6. C4::Circulation::AddReturn() - Modified to loop through and receive all cancelled transfers at checkin, track the original holding branch, and set new 'TransferCancelled' message 7. circ/returns.pl - Added handler for TransferCancelled message that automatically creates a reverse transfer to return the item to its original holding branch using reason 'TransferCancellation' 8. returns.tt - Updated checkin modal to display appropriate messages for both wrong transfers and cancelled transfers Test plan: ---------- 1. Setup - Create three library branches: BranchA, BranchB, BranchC 2. Test hold cancellation cancels transfer: a) Create an item at BranchA b) Create a patron at BranchB c) Place a hold on the item for the patron d) At BranchA, check out the item (triggers transfer to BranchB) e) Verify transfer is created: SELECT * FROM branchtransfers WHERE itemnumber= ORDER BY datesent DESC LIMIT 1 f) Cancel the hold g) Verify transfer is cancelled: Check datecancelled is populated h) Verify cancellation_reason is 'CancelReserve' 3. Test cancelled transfers remain visible: a) Using item from test 2 with cancelled transfer b) Search for the item in staff interface c) View item details d) Verify cancelled transfer information is still visible 4. Test new transfers can be created after cancellation: a) With the same item from test 2 b) Go to Circulation > Transfer c) Scan the item barcode d) Select BranchC as destination e) Verify transfer is created successfully (not blocked) f) Check branchtransfers table shows both cancelled and new transfer: SELECT * FROM branchtransfers WHERE itemnumber= ORDER BY datesent DESC 5. Test checkin with cancelled transfer (wrong branch): a) Create fresh scenario: Item at BranchA, patron at BranchB b) Place hold on item for patron (triggers transfer A->B) c) Cancel the hold (cancels the transfer) d) Item is still physically in transit e) At BranchC (wrong branch), check in the item f) Verify modal appears with message: "Transfer was cancelled whilst in transit, please return item to: BranchA" g) Click OK h) Verify new transfer created from BranchC to BranchA i) Check branchtransfers table: - Old transfer has datearrived populated - New transfer exists with reason 'TransferCancellation' 6. Test checkin with cancelled transfer (correct branch): a) Create scenario: Item at BranchA with cancelled transfer b) Check in item at BranchA (original holding branch) c) Verify cancelled transfer is received (datearrived set) d) Verify item returns to available status at BranchA e) Verify appropriate messaging displayed to staff 7. Test get_transfer() skips cancelled transfers: a) Create item with one cancelled transfer and one active transfer b) Call $item->get_transfer() via prove or koha-shell c) Verify it returns the active transfer, not the cancelled one 8. Test multiple cancelled transfers: a) Create item at BranchA b) Create and cancel transfer to BranchB c) Create and cancel transfer to BranchC d) Create active transfer to BranchB (don't cancel) e) Verify get_transfer() returns the active transfer f) Verify get_transfers() returns all three transfers 9. Run unit tests: prove t/db_dependent/Koha/Item/Transfer.t prove t/db_dependent/Koha/Item.t prove t/db_dependent/Circulation/Returns.t All tests should pass. --- C4/Circulation.pm | 20 ++++++++-- Koha/Hold.pm | 11 ++++++ Koha/Item.pm | 20 ++++++---- Koha/Item/Transfer.pm | 14 ++++++- circ/returns.pl | 24 ++++++++++++ .../prog/en/modules/circ/returns.tt | 8 +++- t/db_dependent/Koha/Item.t | 33 +++++++++++----- t/db_dependent/Koha/Item/Transfer.t | 39 ++++++++++++++++++- 8 files changed, 143 insertions(+), 26 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 0a8df342638..652b1585121 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2496,12 +2496,24 @@ sub AddReturn { } # check if we have a transfer for this document - my $transfer = $item->get_transfer; + my $transfers = $item->get_transfers; + my $transfer = $transfers->next; # if we have a transfer to complete, we update the line of transfers with the datearrived if ($transfer) { $validTransfer = 0; - if ( $transfer->in_transit ) { + + # Catch cancelled transfers, we've found the item now + my $reverse; + while ( $transfer && $transfer->is_cancelled ) { + $transfer->receive; + $messages->{'TransferTrigger'} = $transfer->reason; + $messages->{'WrongTransferItem'} = $item->itemnumber; + $reverse = $transfer->frombranch; + $transfer = $transfers->next; + } + + if ( $transfer && $transfer->in_transit ) { if ( $transfer->tobranch eq $branch ) { $transfer->receive; $messages->{'TransferArrived'} = $transfer->frombranch; @@ -2514,7 +2526,7 @@ sub AddReturn { $messages->{'WrongTransferItem'} = $item->itemnumber; $messages->{'TransferTrigger'} = $transfer->reason; } - } else { + } elsif ($transfer) { if ( $transfer->tobranch eq $branch ) { $transfer->receive; $messages->{'TransferArrived'} = $transfer->frombranch; @@ -2531,6 +2543,8 @@ sub AddReturn { $messages->{'WrongTransferItem'} = $item->itemnumber; } } + } else { + $messages->{'TransferCancelled'} = $reverse; } } diff --git a/Koha/Hold.pm b/Koha/Hold.pm index 3c4fcb8362f..098ec5547a5 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -768,6 +768,17 @@ sub cancel { $self->_result->result_source->schema->txn_do( sub { + # Cancel the transfer if this hold is in transit + if ( $self->is_in_transit ) { + my $transfer = $self->item->get_transfer; + + # Check that there is actually a transfer enqueued and in transit + # before trying to cancel it + if ( $transfer && $transfer->in_transit ) { + $transfer->cancel( { reason => 'CancelReserve', force => 1 } ); + } + } + my $patron = $self->patron; $self->cancellationdate( dt_from_string->strftime('%Y-%m-%d %H:%M:%S') ); diff --git a/Koha/Item.pm b/Koha/Item.pm index 05d2e97570e..2dac41df793 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -869,13 +869,17 @@ we still expect the item to end up at a final location eventually. sub get_transfer { my ($self) = @_; - my $transfers = $self->_result->current_branchtransfers->search( - undef, - { 'order_by' => [ { '-desc' => 'datesent' }, { '-asc' => 'daterequested' } ] } - ); + # Get all outstanding transfers, including cancelled ones + my $transfers = $self->get_transfers; + + # Skip over cancelled transfers and return the first active one my $transfer = $transfers->next; - return Koha::Item::Transfer->_new_from_dbic($transfer) if $transfer; + while ( $transfer && $transfer->is_cancelled ) { + $transfer = $transfers->next; + } + + return $transfer; } =head3 transfer @@ -919,9 +923,9 @@ we still expect the item to end up at a final location eventually. sub get_transfers { my ($self) = @_; - my $transfers_rs = $self->_result->current_branchtransfers->search( - undef, - { 'order_by' => [ { '-desc' => 'datesent' }, { '-asc' => 'daterequested' } ] } + my $transfers_rs = $self->_result->branchtransfers->search( + { datearrived => undef }, + { 'order_by' => [ { '-desc' => 'datesent' }, { '-asc' => 'daterequested' } ] } ); return Koha::Item::Transfers->_new_from_dbic($transfers_rs); diff --git a/Koha/Item/Transfer.pm b/Koha/Item/Transfer.pm index 1abc0767c25..d2f4597b729 100644 --- a/Koha/Item/Transfer.pm +++ b/Koha/Item/Transfer.pm @@ -157,7 +157,19 @@ Boolean returning whether the transfer is in transit or waiting sub in_transit { my ($self) = @_; - return ( defined( $self->datesent ) && !defined( $self->datearrived ) && !defined( $self->datecancelled ) ); + return ( defined( $self->datesent ) && !defined( $self->datearrived ) ); +} + +=head3 is_cancelled + +Boolean returning whether the transfer was cancelled or not + +=cut + +sub is_cancelled { + my ($self) = @_; + + return defined( $self->datecancelled ); } =head3 receive diff --git a/circ/returns.pl b/circ/returns.pl index 35c3367ea4b..45d45a1d92a 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -549,6 +549,28 @@ if ( $messages->{'Wrongbranch'} ) { ); } +# +# case of transfer cancelled whilst in transit +# +if ( $messages->{'TransferCancelled'} ) { + $template->param( + CancelledTransfer => 1, + TransferWaitingAt => $messages->{'TransferCancelled'}, + trigger => $messages->{'TransferTrigger'}, + ); + + # Add reverse transfer to return the item to its original holdingbranch + my $item = Koha::Items->find( $messages->{'WrongTransferItem'} ); + my $new_transfer = $item->request_transfer( + { + to => Koha::Libraries->find( $messages->{'TransferCancelled'} ), + reason => 'TransferCancellation', + ignore_limits => 1 + } + ); + $template->param( NewTransfer => $new_transfer->id ); +} + # case of wrong transfer, if the document wasn't transferred to the right library (according to branchtransfer (tobranch) BDD) if ( $messages->{'WrongTransfer'} and not $messages->{'WasTransfered'} ) { @@ -725,6 +747,8 @@ foreach my $code ( keys %$messages ) { ; # FIXME... anything to do here? } elsif ( $code eq 'WrongTransferItem' ) { ; # FIXME... anything to do here? + } elsif ( $code eq 'TransferCancelled' ) { + ; # Handled above } elsif ( $code eq 'NeedsTransfer' ) { } elsif ( $code eq 'TransferTrigger' ) { ; # Handled alongside NeedsTransfer diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt index 9e8d328d450..72c53dc461f 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt @@ -598,7 +598,7 @@ [% UNLESS ( hold_auto_filled && diffbranch ) %] - [% IF WrongTransfer && !transfertodo %] + [% IF ( WrongTransfer || CancelledTransfer ) && !transfertodo %] [% IF Koha.Preference('TransfersBlockCirc') %] [% SET div_class='block' %] [% ELSE %] @@ -625,7 +625,11 @@ [% END %]