Bugzilla – Attachment 188298 Details for
Bug 28527
Transfers that are cancelled whilst in transit should still appear as 'in transit' until they are received
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 28527: Handle cancelled in-transit transfers
Bug-28527-Handle-cancelled-in-transit-transfers.patch (text/plain), 18.47 KB, created by
Martin Renvoize (ashimema)
on 2025-10-22 14:01:33 UTC
(
hide
)
Description:
Bug 28527: Handle cancelled in-transit transfers
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2025-10-22 14:01:33 UTC
Size:
18.47 KB
patch
obsolete
>From e73cbc6af94c69d97a1dd7abbe67c73bb45894a6 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >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=<item> 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=<item> > 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 @@ > > <!-- case of a mistake in transfer loop --> > [% 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 %] > > <div class="modal-header"> >- <h1 class="modal-title"> Wrong transfer detected, please return item to: [% Branches.GetName( TransferWaitingAt ) | html %] </h1> >+ [% IF WrongTransfer %] >+ <h1 class="modal-title"> Wrong transfer detected, please return item to: [% Branches.GetName( TransferWaitingAt ) | html %] </h1> >+ [% ELSE %] >+ <h1 class="modal-title"> Transfer was cancelled whilst in transit, please return item to: [% Branches.GetName( TransferWaitingAt ) | html %] </h1> >+ [% END %] > </div> > > <div class="modal-body"> >diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t >index 19cdb04deac..52989a48de3 100755 >--- a/t/db_dependent/Koha/Item.t >+++ b/t/db_dependent/Koha/Item.t >@@ -1220,7 +1220,7 @@ subtest 'request_transfer() tests' => sub { > ok( $original_transfer->datecancelled, "Original transfer cancelled" ); > is( $original_transfer->cancellation_reason, "WrongTransfer", "Original cancellation_reason set correctly" ); > $transfers = $item->get_transfers; >- is( $transfers->count, 1, "There is only 1 live transfer in the queue" ); >+ is( $transfers->count, 2, "There are 2 transfers in the queue (1 cancelled, 1 active) (Bug 28527)" ); > $replaced_transfer->datearrived(dt_from_string)->store(); > > # BranchTransferLimits >@@ -1827,7 +1827,7 @@ subtest 'get_transfer' => sub { > }; > > subtest 'get_transfers' => sub { >- plan tests => 17; >+ plan tests => 18; > $schema->storage->txn_begin; > > my $item = $builder->build_sample_item(); >@@ -1954,15 +1954,23 @@ subtest 'get_transfers' => sub { > > $transfer_1->datecancelled( \'NOW()' )->store; > $transfers = $item->get_transfers(); >- is( $transfers->count, 1, 'Once a transfer is cancelled, it no longer appears in the list from ->get_transfers()' ); >+ is( >+ $transfers->count, 2, >+ 'Once a transfer is cancelled, it still appears in the list from ->get_transfers() (Bug 28527)' >+ ); > $result_1 = $transfers->next; >+ $result_2 = $transfers->next; > is( >- $result_1->branchtransfer_id, $transfer_3->branchtransfer_id, >- 'Koha::Item->get_transfers returns the only transfer that remains' >+ $result_1->branchtransfer_id, $transfer_1->branchtransfer_id, >+ 'Koha::Item->get_transfers returns cancelled transfers' >+ ); >+ is( >+ $result_2->branchtransfer_id, $transfer_3->branchtransfer_id, >+ 'Koha::Item->get_transfers returns the active transfer' > ); > > subtest "Ensure prefetches don't affect the return order" => sub { >- plan tests => 13; >+ plan tests => 14; > > # Reset test data > $transfer_2->datesent(undef)->store; >@@ -2049,13 +2057,18 @@ subtest 'get_transfers' => sub { > )->next; > $transfers = $prefetched_item->get_transfers(); > is( >- $transfers->count, 1, >- 'Once a transfer is cancelled, it no longer appears in the list from ->get_transfers()' >+ $transfers->count, 2, >+ 'Once a transfer is cancelled, it still appears in the list from ->get_transfers() (Bug 28527)' > ); > $result_1 = $transfers->next; >+ $result_2 = $transfers->next; > is( >- $result_1->branchtransfer_id, $transfer_3->branchtransfer_id, >- 'Koha::Item->get_transfers returns the only transfer that remains' >+ $result_1->branchtransfer_id, $transfer_1->branchtransfer_id, >+ 'Koha::Item->get_transfers returns cancelled transfers' >+ ); >+ is( >+ $result_2->branchtransfer_id, $transfer_3->branchtransfer_id, >+ 'Koha::Item->get_transfers returns the active transfer' > ); > }; > >diff --git a/t/db_dependent/Koha/Item/Transfer.t b/t/db_dependent/Koha/Item/Transfer.t >index 5a3ecbcdf04..9758a4b2969 100755 >--- a/t/db_dependent/Koha/Item/Transfer.t >+++ b/t/db_dependent/Koha/Item/Transfer.t >@@ -27,7 +27,7 @@ use t::lib::Mocks; > use t::lib::TestBuilder; > > use Test::NoWarnings; >-use Test::More tests => 9; >+use Test::More tests => 10; > use Test::Exception; > > my $schema = Koha::Database->new->schema; >@@ -253,7 +253,42 @@ subtest 'in_transit tests' => sub { > ok( !$transfer->in_transit, 'in_transit returns false when datearrived is defined' ); > > $transfer->set( { datearrived => undef, datecancelled => dt_from_string } )->store; >- ok( !$transfer->in_transit, 'in_transit returns false when datecancelled is defined' ); >+ ok( $transfer->in_transit, 'in_transit returns true even when datecancelled is defined (Bug 28527)' ); >+ >+ $schema->storage->txn_rollback; >+}; >+ >+subtest 'is_cancelled tests' => sub { >+ >+ plan tests => 3; >+ >+ $schema->storage->txn_begin; >+ >+ my $library_from = $builder->build_object( { class => 'Koha::Libraries' } ); >+ my $library_to = $builder->build_object( { class => 'Koha::Libraries' } ); >+ my $item = $builder->build_sample_item( >+ { >+ homebranch => $library_to->branchcode, >+ holdingbranch => $library_from->branchcode, >+ } >+ ); >+ >+ my $transfer = Koha::Item::Transfer->new( >+ { >+ itemnumber => $item->itemnumber, >+ frombranch => $library_from->branchcode, >+ tobranch => $library_to->branchcode, >+ daterequested => dt_from_string, >+ } >+ )->store; >+ >+ ok( !$transfer->is_cancelled, 'is_cancelled returns false when datecancelled is not defined' ); >+ >+ $transfer->datesent(dt_from_string)->store; >+ ok( !$transfer->is_cancelled, 'is_cancelled returns false when transfer is in transit' ); >+ >+ $transfer->datecancelled(dt_from_string)->store; >+ ok( $transfer->is_cancelled, 'is_cancelled returns true when datecancelled is defined' ); > > $schema->storage->txn_rollback; > }; >-- >2.51.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 28527
:
121739
|
121740
|
121741
|
121742
| 188298