From 36f5370b0139f5d89cdb4e4b907808de51050871 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Tue, 19 Aug 2025 13:58:38 +0000 Subject: [PATCH] Bug 40671: Refactor Koha::Hold->revert_waiting into revert_found This patch refactors the revert_waiting method to handle all found statuses instead of just waiting holds, making it more versatile and consistent. Changes: * Renamed method from revert_waiting() to revert_found() * Expanded functionality to handle all found statuses (W, T, P) * Updated exception logic to check is_found instead of is_waiting * Changed exception message from hold_not_waiting to hold_not_found * Added comprehensive tests for all found statuses * Updated POD documentation to reflect new behavior The method now reverts any hold with a found status back to a regular hold with priority 1, regardless of whether it was waiting, in transit, or in processing. To test: 1. Apply the patches 2. Run tests: $ ktd --shell k$ cd /kohadevbox/koha k$ prove t/db_dependent/Koha/Hold.t => SUCCESS: All tests pass including new subtests for T and P statuses 3. Verify all hold test suites pass: k$ prove t/db_dependent/Koha/Hold*.t => SUCCESS: All hold-related tests pass 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi Signed-off-by: Emily Lamancusa --- Koha/Hold.pm | 15 +++-- t/db_dependent/Koha/Hold.t | 128 ++++++++++++++++++++++++++++++++----- 2 files changed, 121 insertions(+), 22 deletions(-) diff --git a/Koha/Hold.pm b/Koha/Hold.pm index c44d46a56a..8371550649 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -889,19 +889,20 @@ sub fill { return $self; } -=head3 revert_waiting +=head3 revert_found - $hold->revert_waiting(); + $hold->revert_found(); -Reverts a 'waiting' hold back to a regular hold with a priority of 1. +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. =cut -sub revert_waiting { +sub revert_found { my ( $self, $params ) = @_; - Koha::Exceptions::InvalidStatus->throw( invalid_status => 'hold_not_waiting' ) - unless $self->is_waiting; + Koha::Exceptions::InvalidStatus->throw( invalid_status => 'hold_not_found' ) + unless $self->is_found; $self->_result->result_source->schema->txn_do( sub { @@ -919,7 +920,7 @@ sub revert_waiting { { no_triggers => 1 } ); - ## Fix up the currently waiting reserve + ## Fix up the currently found reserve $self->set( { priority => 1, diff --git a/t/db_dependent/Koha/Hold.t b/t/db_dependent/Koha/Hold.t index 979192f877..d0ff9a8817 100755 --- a/t/db_dependent/Koha/Hold.t +++ b/t/db_dependent/Koha/Hold.t @@ -1262,9 +1262,9 @@ subtest 'strings_map() tests' => sub { $schema->txn_rollback; }; -subtest 'revert_waiting() tests' => sub { +subtest 'revert_found() tests' => sub { - plan tests => 3; + plan tests => 5; subtest 'item-level holds tests' => sub { @@ -1318,8 +1318,8 @@ subtest 'revert_waiting() tests' => sub { is( $hold->priority, 0, "'priority' set to 0" ); ok( $hold->is_waiting, 'Hold set to waiting' ); - # Revert the waiting status - $hold->revert_waiting(); + # Revert the found status + $hold->revert_found(); is( $hold->waitingdate, undef, "'waitingdate' reset" ); ok( !$hold->is_waiting, 'Hold no longer set to waiting' ); @@ -1342,20 +1342,20 @@ subtest 'revert_waiting() tests' => sub { $hold->set_waiting(); - # Revert the waiting status, RealTimeHoldsQueue => shouldn't add a test - $hold->revert_waiting(); + # Revert the found status, RealTimeHoldsQueue => shouldn't add a test + $hold->revert_found(); my $log_count_after = Koha::ActionLogs->search( { module => 'HOLDS', action => 'MODIFY', object => $hold->reserve_id } )->count; - is( $log_count, $log_count_after, "No logging is added for ->revert_waiting() when HoldsLog is disabled" ); + is( $log_count, $log_count_after, "No logging is added for ->revert_found() when HoldsLog is disabled" ); - # Set as 'processing' to test the exception behavior - $hold->found('P'); - throws_ok { $hold->revert_waiting() } + # Set as regular hold (not found) to test the exception behavior + $hold->found(undef); + throws_ok { $hold->revert_found() } 'Koha::Exceptions::InvalidStatus', - "Hold is not in 'waiting' status, exception thrown"; + "Hold is not in 'found' status, exception thrown"; - is( $@->invalid_status, 'hold_not_waiting', "'invalid_status' set the right value" ); + is( $@->invalid_status, 'hold_not_found', "'invalid_status' set the right value" ); $schema->storage->txn_rollback; }; @@ -1399,8 +1399,8 @@ subtest 'revert_waiting() tests' => sub { is( $hold->priority, 0, "'priority' set to 0" ); ok( $hold->is_waiting, 'Hold set to waiting' ); - # Revert the waiting status - $hold->revert_waiting(); + # Revert the found status + $hold->revert_found(); is( $hold->waitingdate, undef, "'waitingdate' reset" ); ok( !$hold->is_waiting, 'Hold no longer set to waiting' ); @@ -1478,7 +1478,7 @@ subtest 'revert_waiting() tests' => sub { $hold->set_waiting()->discard_changes(); is( $hold->priority, 0, "'priority' set to 0" ); - $hold->revert_waiting()->discard_changes(); + $hold->revert_found()->discard_changes(); is( $hold->priority, 1, "'priority' set to 1" ); my $holds = $item->biblio->holds; @@ -1493,4 +1493,102 @@ subtest 'revert_waiting() tests' => sub { $schema->storage->txn_rollback; }; + + subtest 'in transit holds tests' => sub { + + plan tests => 8; + + $schema->storage->txn_begin; + + my $item = $builder->build_sample_item; + my $patron = $builder->build_object( + { + class => 'Koha::Patrons', + value => { branchcode => $item->homebranch } + } + ); + + # Create item-level hold + my $hold = Koha::Holds->find( + AddReserve( + { + branchcode => $item->homebranch, + borrowernumber => $patron->borrowernumber, + biblionumber => $item->biblionumber, + priority => 1, + itemnumber => $item->itemnumber, + } + ) + ); + + # Mark it in transit + $hold->set_transfer(); + + is( $hold->priority, 0, "'priority' set to 0" ); + ok( $hold->is_in_transit, 'Hold set to in transit' ); + is( $hold->found, 'T', "'found' set to 'T'" ); + + # Revert the found status + $hold->revert_found(); + + ok( !$hold->is_in_transit, 'Hold no longer set to in transit' ); + ok( !$hold->is_found, 'Hold no longer has found status' ); + is( $hold->found, undef, "'found' reset to undef" ); + is( $hold->priority, 1, "'priority' set to 1" ); + is( + $hold->itemnumber, $item->itemnumber, + 'Itemnumber should not be removed when the found status is reverted' + ); + + $schema->storage->txn_rollback; + }; + + subtest 'in processing holds tests' => sub { + + plan tests => 8; + + $schema->storage->txn_begin; + + my $item = $builder->build_sample_item; + my $patron = $builder->build_object( + { + class => 'Koha::Patrons', + value => { branchcode => $item->homebranch } + } + ); + + # Create item-level hold + my $hold = Koha::Holds->find( + AddReserve( + { + branchcode => $item->homebranch, + borrowernumber => $patron->borrowernumber, + biblionumber => $item->biblionumber, + priority => 1, + itemnumber => $item->itemnumber, + } + ) + ); + + # Mark it in processing + $hold->set_processing(); + + is( $hold->priority, 0, "'priority' set to 0" ); + ok( $hold->is_in_processing, 'Hold set to in processing' ); + is( $hold->found, 'P', "'found' set to 'P'" ); + + # Revert the found status + $hold->revert_found(); + + ok( !$hold->is_in_processing, 'Hold no longer set to in processing' ); + ok( !$hold->is_found, 'Hold no longer has found status' ); + is( $hold->found, undef, "'found' reset to undef" ); + is( $hold->priority, 1, "'priority' set to 1" ); + is( + $hold->itemnumber, $item->itemnumber, + 'Itemnumber should not be removed when the found status is reverted' + ); + + $schema->storage->txn_rollback; + }; }; -- 2.34.1