From 15394b04ac19196795271b6742998326bb111e8c Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Tue, 19 Aug 2025 14:34:31 +0000 Subject: [PATCH] Bug 40672: Reset desk_id when reverting waiting holds This patch adds desk_id reset functionality to the revert_found method, but only for waiting holds where desk_id is actually relevant. Changes: * Modified revert_found() to conditionally reset desk_id * Only resets desk_id for waiting holds (found=W) * Leaves desk_id unchanged for in transit (T) and in processing (P) holds * Added comprehensive test coverage for all scenarios * Updated POD documentation to explain desk_id behavior The logic is conservative and only clears desk_id when it makes sense: waiting holds have their desk assignment cleared since they are no longer waiting at a specific desk, while transit and processing holds remain unchanged as they do not use desk_id. To test: 1. Apply patch 2. Run tests: $ ktd --shell k$ cd /kohadevbox/koha k$ prove t/db_dependent/Koha/Hold.t => SUCCESS: All tests pass including new desk_id handling tests 3. Verify all hold test suites still pass: k$ prove t/db_dependent/Koha/Hold*.t => SUCCESS: All hold-related tests pass 4. Sign off :-D --- Koha/Hold.pm | 5 +++ t/db_dependent/Koha/Hold.t | 83 +++++++++++++++++++++++++++++++++++++- 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/Koha/Hold.pm b/Koha/Hold.pm index 83715506490..994cc89ab0f 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -896,6 +896,10 @@ sub fill { Reverts any 'found' hold back to a regular hold with a priority of 1. This method can revert holds in 'waiting' (W), 'in transit' (T), or 'in processing' (P) status. +For waiting holds, the desk_id is also cleared since the hold is no longer waiting +at a specific desk. For in transit and in processing holds, desk_id remains unchanged +(typically NULL as these statuses don't set desk_id). + =cut sub revert_found { @@ -928,6 +932,7 @@ sub revert_found { waitingdate => undef, expirationdate => $self->patron_expiration_date, itemnumber => $self->item_level_hold ? $self->itemnumber : undef, + ( $self->is_waiting ? ( desk_id => undef ) : () ), } )->store( { hold_reverted => 1 } ); diff --git a/t/db_dependent/Koha/Hold.t b/t/db_dependent/Koha/Hold.t index d0ff9a8817f..f7258f1f41b 100755 --- a/t/db_dependent/Koha/Hold.t +++ b/t/db_dependent/Koha/Hold.t @@ -1264,7 +1264,7 @@ subtest 'strings_map() tests' => sub { subtest 'revert_found() tests' => sub { - plan tests => 5; + plan tests => 6; subtest 'item-level holds tests' => sub { @@ -1591,4 +1591,85 @@ subtest 'revert_found() tests' => sub { $schema->storage->txn_rollback; }; + + subtest 'desk_id handling tests' => sub { + + plan tests => 12; + + $schema->storage->txn_begin; + + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); + my $desk = $builder->build_object( + { + class => 'Koha::Desks', + value => { branchcode => $library->branchcode } + } + ); + my $patron = $builder->build_object( + { + class => 'Koha::Patrons', + value => { branchcode => $library->branchcode } + } + ); + my $item = $builder->build_sample_item( { library => $library->branchcode } ); + + my $hold = $builder->build_object( + { + class => 'Koha::Holds', + value => { + borrowernumber => $patron->borrowernumber, + biblionumber => $item->biblionumber, + itemnumber => $item->itemnumber, + branchcode => $library->branchcode, + priority => 1, + found => undef, + } + } + ); + + # Test 1: Waiting hold - desk_id should be cleared + $hold->set_waiting( $desk->desk_id ); + $hold->discard_changes; + is( $hold->desk_id, $desk->desk_id, 'desk_id set for waiting hold' ); + ok( $hold->is_waiting, 'Hold is in waiting status' ); + + $hold->revert_found(); + $hold->discard_changes; + is( $hold->desk_id, undef, 'desk_id cleared when reverting waiting hold' ); + ok( !$hold->is_found, 'Hold is no longer in found status' ); + + # Test 2: In transit hold with desk_id - desk_id should be preserved + $hold->set_transfer(); + $hold->desk_id( $desk->desk_id )->store(); # Manually set desk_id + $hold->discard_changes; + is( $hold->desk_id, $desk->desk_id, 'desk_id manually set for transit hold' ); + ok( $hold->is_in_transit, 'Hold is in transit status' ); + + $hold->revert_found(); + $hold->discard_changes; + is( $hold->desk_id, $desk->desk_id, 'desk_id preserved when reverting transit hold' ); + + # Test 3: In processing hold with desk_id - desk_id should be preserved + $hold->set_processing(); + $hold->desk_id( $desk->desk_id )->store(); # Manually set desk_id + $hold->discard_changes; + is( $hold->desk_id, $desk->desk_id, 'desk_id manually set for processing hold' ); + ok( $hold->is_in_processing, 'Hold is in processing status' ); + + $hold->revert_found(); + $hold->discard_changes; + is( $hold->desk_id, $desk->desk_id, 'desk_id preserved when reverting processing hold' ); + + # Test 4: In transit hold without desk_id - desk_id should remain NULL + $hold->set_transfer(); + $hold->desk_id(undef)->store(); # Ensure desk_id is NULL + $hold->discard_changes; + is( $hold->desk_id, undef, 'desk_id is NULL for transit hold without desk_id' ); + + $hold->revert_found(); + $hold->discard_changes; + is( $hold->desk_id, undef, 'desk_id remains NULL after reverting transit hold without desk_id' ); + + $schema->storage->txn_rollback; + }; }; -- 2.51.0