From 2cdeca88e5699cf7a12af7231533ab10bdd779b8 Mon Sep 17 00:00:00 2001 From: Pedro Amorim Date: Tue, 22 Jul 2025 12:27:17 +0000 Subject: [PATCH] Bug 40529: Add unit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test plan, k-t-d, patches applied: 1) Apply patches, enable DisplayAddHoldGroups system preference 2) Do a search for 'test': /cgi-bin/koha/catalogue/search.pl?q=test 3) Click the 'Select all' link at the top of the search results, click the 'Place hold' button. 4) Enter the 'koha' user 5) Pick a pickup location for each of the holds and click 'Place holds'. 6) Visit the 'koha' patron: /cgi-bin/koha/members/moremember.pl?borrowernumber=51 7) Notice there are 3 holds all part of hold group '1' 8) Check-in one of the items in this hold group: 39999000005578. Confirm hold. 9) Repeat 6). Notice 'Continuous delivery' is now the hold group 'target'. 10) Place a new hold for one of the other 2 'non-target' holds for a different patron. Visit: /cgi-bin/koha/reserve/request.pl?biblionumber=29 11) Enter 'henry' on the patron input. Pick 'Henry Acevedo (23529000035676)'. Click 'Place hold'. Notice Henry's hold is priority 2. Let's check-in the item for this record: 39999000001310 12) Notice the eligible hold is Henry's, not the koha user's hold. This is because the hold that relates to koha's user is part of a hold group that already has a target so it was skipped. You may confirm the hold. 13) Now let's cancel 'target' hold, visit: /cgi-bin/koha/members/moremember.pl?borrowernumber=51 14) Click the 'Holds' tab and the red trash icon next to 'Continuous delivery'. Confirm cancellation. 15) Check the holds again, now there's only 2 holds in the existing group. 16) Let's repeat the exercise as before and place a hold for 'Henry' for 'Clean code', visit: /cgi-bin/koha/reserve/request.pl?biblionumber=114 17) Enter 'henry' and place a hold as before. 18) The 'Clean code' record now has 2 holds: first for 'koha' user and the second for 'Henry'. 19) Now check-in the item for that record. Notice it's the 'koha' user's hold that comes up. This is because it's first in priority and the hold is part of a hold group that does not yet have a target. Confirm hold. 20) Go back to 'Henry' and cancel his holds. Visit: /cgi-bin/koha/members/moremember.pl?borrowernumber=19 21) Select 'both', click 'Cancel selected'. Confirm cancellation. 22) Visit 'holds to pull': /cgi-bin/koha/circ/pendingreserves.pl 23) Notice no holds show, even though 'koha' user has a hold that does not yet have an item assigned to it. This hold was skipped because it's part of a hold group that already has a target. To confirm this, disable DisplayAddHoldGroups and refresh 'Holds to pull'. Confirm it shows. 24) The same happens for the 'holds queue' i.e. 'non-target' holds are skipped. To test, with the sys pref still disabled, run the build_holds_queue.pl script: $ perl misc/cronjobs/holds/build_holds_queue.pl 25) Notice the 'non-target' hold shows, and that is expected if DisplayAddHoldGroups is disabled. Enable DisplayAddHoldGroups and repeat 24. Confirm the hold is gone. The behavior above works as described for holds that become 'In transit' or 'Waiting'. Run the updated test files: prove ./t/db_dependent/Koha/Hold.t prove ./t/db_dependent/Reserves/HoldGroup.t Relevant tests worth running to ensure no regressions are added to existing covered functionality: prove ./t/db_dependent/Circulation* prove ./t/db_dependent/Hold* prove ./t/db_dependent/Reserves* Signed-off-by: Anneli Österman --- t/db_dependent/Koha/Hold.t | 273 +++++++++++++++++++++++++++- t/db_dependent/Reserves/HoldGroup.t | 85 ++++++++- 2 files changed, 356 insertions(+), 2 deletions(-) diff --git a/t/db_dependent/Koha/Hold.t b/t/db_dependent/Koha/Hold.t index c34441971c..646fbcef55 100755 --- a/t/db_dependent/Koha/Hold.t +++ b/t/db_dependent/Koha/Hold.t @@ -20,7 +20,7 @@ use Modern::Perl; use Test::NoWarnings; -use Test::More tests => 16; +use Test::More tests => 18; use Test::Exception; use Test::MockModule; @@ -34,6 +34,8 @@ use Koha::DateUtils qw(dt_from_string); use Koha::Holds; use Koha::Libraries; use Koha::Old::Holds; +use C4::Reserves qw( AddReserve ModReserveAffect ); +use C4::Circulation qw( AddReturn ); my $schema = Koha::Database->new->schema; my $builder = t::lib::TestBuilder->new; @@ -1259,3 +1261,272 @@ subtest 'strings_map() tests' => sub { $schema->txn_rollback; }; +subtest 'is_hold_group_target, cleanup_hold_group and set_as_hold_group_target tests' => sub { + + plan tests => 16; + + $schema->storage->txn_begin; + + t::lib::Mocks::mock_preference( 'DisplayAddHoldGroups', 1 ); + + my $patron_category = $builder->build( + { + source => 'Category', + value => { + category_type => 'P', + enrolmentfee => 0, + BlockExpiredPatronOpacActions => 'follow_syspref_BlockExpiredPatronOpacActions', + } + } + ); + + my $library_1 = $builder->build( { source => 'Branch' } ); + my $patron_1 = $builder->build( + { + source => 'Borrower', + value => { branchcode => $library_1->{branchcode}, categorycode => $patron_category->{categorycode} } + } + ); + my $library_2 = $builder->build( { source => 'Branch' } ); + my $patron_2 = $builder->build_object( + { + class => 'Koha::Patrons', + value => { branchcode => $library_2->{branchcode}, categorycode => $patron_category->{categorycode} } + } + ); + + my $item = $builder->build_sample_item( + { + library => $library_1->{branchcode}, + } + ); + + my $item_2 = $builder->build_sample_item( + { + library => $library_1->{branchcode}, + } + ); + + set_userenv($library_2); + my $reserve_id = AddReserve( + { + branchcode => $library_2->{branchcode}, + borrowernumber => $patron_2->borrowernumber, + biblionumber => $item->biblionumber, + priority => 1, + } + ); + + my $reserve_2_id = AddReserve( + { + branchcode => $library_2->{branchcode}, + borrowernumber => $patron_2->borrowernumber, + biblionumber => $item_2->biblionumber, + priority => 1, + } + ); + + my $reserve_3_id = AddReserve( + { + branchcode => $library_2->{branchcode}, + borrowernumber => $patron_2->borrowernumber, + biblionumber => $item_2->biblionumber, + priority => 1, + } + ); + + my $new_hold_group = $patron_2->create_hold_group( [ $reserve_id, $reserve_2_id, $reserve_3_id ] ); + + set_userenv($library_1); + my $do_transfer = 1; + my ( $returned, $messages, $issue, $borrower ) = AddReturn( $item->barcode, $library_1->{branchcode} ); + + ok( exists $messages->{ResFound}, 'ResFound key exists' ); + + ModReserveAffect( $item->itemnumber, undef, $do_transfer, $reserve_id ); + + my $hold = Koha::Holds->find($reserve_id); + is( $hold->hold_group->hold_group_id, $new_hold_group->hold_group_id, 'First hold belongs to hold group' ); + is( $hold->found, 'T', 'Hold is in transit' ); + + is( $hold->is_hold_group_target, 1, 'First hold is the hold group target' ); + my $hold_2 = Koha::Holds->find($reserve_2_id); + is( $hold_2->hold_group->hold_group_id, $new_hold_group->hold_group_id, 'Second hold belongs to hold group' ); + is( $hold_2->is_hold_group_target, 0, 'Second hold is not the hold group target' ); + + set_userenv($library_2); + $do_transfer = 0; + AddReturn( $item->barcode, $library_2->{branchcode} ); + ModReserveAffect( $item->itemnumber, undef, $do_transfer, $reserve_id ); + $hold = Koha::Holds->find($reserve_id); + is( $hold->found, 'W', 'Hold is waiting' ); + + is( $hold->is_hold_group_target, 1, 'First hold is still the hold group target' ); + is( $hold_2->is_hold_group_target, 0, 'Second hold is still not the hold group target' ); + + $hold->cancel(); + is( $hold->is_hold_group_target, 0, 'First hold is no longer the hold group target' ); + is( $hold_2->is_hold_group_target, 0, 'Second hold is still not the hold group target' ); + + # Return item for second hold + AddReturn( $item_2->barcode, $library_2->{branchcode} ); + ModReserveAffect( $item_2->itemnumber, undef, $do_transfer, $reserve_2_id ); + $new_hold_group->discard_changes; + is( $hold_2->get_from_storage->found, 'W', 'Hold is waiting' ); + is( $hold_2->is_hold_group_target, 1, 'Second hold is now the hold group target' ); + + my $hold_3 = Koha::Holds->find($reserve_3_id); + is( $hold_3->hold_group->hold_group_id, $new_hold_group->hold_group_id, 'Third hold belongs to hold group' ); + is( $hold_3->is_hold_group_target, 0, 'Third hold is not the hold group target' ); + + $hold_2->cancel(); + is( + $hold_3->hold_group, undef, + 'Third hold was left as the only member of its group. Group was deleted and the hold is no longer part of a hold group' + ); + + $schema->storage->txn_rollback; +}; + +subtest '_Findgroupreserve in the context of hold groups' => sub { + plan tests => 13; + + $schema->storage->txn_begin; + + t::lib::Mocks::mock_preference( 'DisplayAddHoldGroups', 1 ); + + my $patron_category = $builder->build( + { + source => 'Category', + value => { + category_type => 'P', + enrolmentfee => 0, + BlockExpiredPatronOpacActions => 'follow_syspref_BlockExpiredPatronOpacActions', + } + } + ); + + my $library_1 = $builder->build( { source => 'Branch' } ); + my $patron_1 = $builder->build_object( + { + class => 'Koha::Patrons', + value => { branchcode => $library_1->{branchcode}, categorycode => $patron_category->{categorycode} } + } + ); + my $library_2 = $builder->build( { source => 'Branch' } ); + my $patron_2 = $builder->build_object( + { + class => 'Koha::Patrons', + value => { branchcode => $library_2->{branchcode}, categorycode => $patron_category->{categorycode} } + } + ); + + my $item = $builder->build_sample_item( + { + library => $library_1->{branchcode}, + } + ); + + my $item_2 = $builder->build_sample_item( + { + library => $library_1->{branchcode}, + } + ); + + set_userenv($library_2); + my $reserve_id = AddReserve( + { + branchcode => $library_2->{branchcode}, + borrowernumber => $patron_2->borrowernumber, + biblionumber => $item->biblionumber, + priority => 1, + } + ); + + my $reserve_2_id = AddReserve( + { + branchcode => $library_2->{branchcode}, + borrowernumber => $patron_2->borrowernumber, + biblionumber => $item_2->biblionumber, + priority => 1, + } + ); + + my $reserve_3_id = AddReserve( + { + branchcode => $library_2->{branchcode}, + borrowernumber => $patron_1->borrowernumber, + biblionumber => $item_2->biblionumber, + priority => 2, + } + ); + + my @reserves = C4::Reserves::_Findgroupreserve( $item_2->biblionumber, $item_2->id, 0, [] ); + is( scalar @reserves, 2, "No hold group created yet. We should get both holds" ); + is( $reserves[0]->{reserve_id}, $reserve_2_id, "We got the expected reserve" ); + + my $new_hold_group = $patron_2->create_hold_group( [ $reserve_id, $reserve_2_id ] ); + + set_userenv($library_1); + my $do_transfer = 1; + my ( $returned, $messages, $issue, $borrower ) = AddReturn( $item->barcode, $library_1->{branchcode} ); + + ok( exists $messages->{ResFound}, 'ResFound key exists' ); + + ModReserveAffect( $item->itemnumber, undef, $do_transfer, $reserve_id ); + + my $hold = Koha::Holds->find($reserve_id); + is( $hold->hold_group->hold_group_id, $new_hold_group->hold_group_id, 'First hold belongs to hold group' ); + is( $hold->found, 'T', 'Hold is in transit' ); + + is( $hold->is_hold_group_target, 1, 'First hold is the hold group target' ); + + my @new_reserves = C4::Reserves::_Findgroupreserve( $item_2->biblionumber, $item_2->id, 0, [] ); + is( scalar @new_reserves, 1, "reserve_2_id is now part of a group that has a target. Should not be here." ); + is( + $new_reserves[0]->{reserve_id}, $reserve_3_id, + "reserve_2_id is now part of a group that has a target. Should not be here." + ); + + $hold->cancel(); + + my @reserves_after_cancel = C4::Reserves::_Findgroupreserve( $item_2->biblionumber, $item_2->id, 0, [] ); + is( + scalar @reserves_after_cancel, 2, + "reserve_2_id should be back in the pool as it's no longer part of a group." + ); + is( $reserves_after_cancel[0]->{reserve_id}, $reserve_2_id, "We got the expected reserve" ); + is( $reserves_after_cancel[1]->{reserve_id}, $reserve_3_id, "We got the expected reserve" ); + + my $first_reserve_id_again = AddReserve( + { + branchcode => $library_2->{branchcode}, + borrowernumber => $patron_2->borrowernumber, + biblionumber => $item->biblionumber, + priority => 1, + } + ); + + # Fake the holds queue + my $dbh = C4::Context->dbh; + $dbh->do( + q{INSERT INTO hold_fill_targets VALUES (?, ?, ?, ?, ?,?)}, undef, + ( + $patron_1->borrowernumber, $item->biblionumber, $item->itemnumber, $item->homebranch, 0, + $first_reserve_id_again + ) + ); + + my @reserves_after_holds_queue = C4::Reserves::_Findgroupreserve( $item_2->biblionumber, $item_2->id, 0, [] ); + is( scalar @new_reserves, 1, "Only reserve_3_id should exist." ); + is( + $new_reserves[0]->{reserve_id}, $reserve_3_id, + "reserve_3_id should not be here." + ); +}; + +sub set_userenv { + my ($library) = @_; + my $staff = $builder->build_object( { class => "Koha::Patrons" } ); + t::lib::Mocks::mock_userenv( { patron => $staff, branchcode => $library->{branchcode} } ); +} diff --git a/t/db_dependent/Reserves/HoldGroup.t b/t/db_dependent/Reserves/HoldGroup.t index 5f92edaa44..890d4bc1aa 100755 --- a/t/db_dependent/Reserves/HoldGroup.t +++ b/t/db_dependent/Reserves/HoldGroup.t @@ -19,9 +19,13 @@ use Modern::Perl; -use Test::More tests => 1; +use Test::More tests => 2; use t::lib::TestBuilder; +use t::lib::Mocks; + +use C4::Reserves qw( AddReserve ModReserveAffect ); +use C4::Circulation qw( AddReturn ); my $schema = Koha::Database->new->schema; my $builder = t::lib::TestBuilder->new; @@ -51,3 +55,82 @@ subtest 'holds tests' => sub { $schema->storage->txn_rollback; }; + +subtest "target_hold_id tests" => sub { + + plan tests => 2; + + $schema->storage->txn_begin; + + t::lib::Mocks::mock_preference( 'DisplayAddHoldGroups', 1 ); + + my $patron_category = $builder->build( + { + source => 'Category', + value => { + category_type => 'P', + enrolmentfee => 0, + BlockExpiredPatronOpacActions => 'follow_syspref_BlockExpiredPatronOpacActions', + } + } + ); + + my $library_1 = $builder->build( { source => 'Branch' } ); + my $library_2 = $builder->build( { source => 'Branch' } ); + my $patron_2 = $builder->build_object( + { + class => 'Koha::Patrons', + value => { branchcode => $library_2->{branchcode}, categorycode => $patron_category->{categorycode} } + } + ); + + my $item = $builder->build_sample_item( + { + library => $library_1->{branchcode}, + } + ); + + my $item_2 = $builder->build_sample_item( + { + library => $library_1->{branchcode}, + } + ); + + set_userenv($library_2); + my $reserve_id = AddReserve( + { + branchcode => $library_2->{branchcode}, + borrowernumber => $patron_2->borrowernumber, + biblionumber => $item->biblionumber, + priority => 1, + } + ); + + my $reserve_2_id = AddReserve( + { + branchcode => $library_2->{branchcode}, + borrowernumber => $patron_2->borrowernumber, + biblionumber => $item_2->biblionumber, + priority => 1, + } + ); + + my $hold_group = $patron_2->create_hold_group( [ $reserve_id, $reserve_2_id ] ); + + set_userenv($library_1); + my $do_transfer = 1; + AddReturn( $item->barcode, $library_1->{branchcode} ); + ModReserveAffect( $item->itemnumber, undef, $do_transfer, $reserve_id ); + my $hold = Koha::Holds->find($reserve_id); + is( $hold_group->get_from_storage->target_hold_id, $hold->reserve_id, 'target_hold_id is correct' ); + $hold->fill(); + is( $hold_group->get_from_storage, undef, 'hold group no longer exists' ); + + $schema->storage->txn_rollback; +}; + +sub set_userenv { + my ($library) = @_; + my $staff = $builder->build_object( { class => "Koha::Patrons" } ); + t::lib::Mocks::mock_userenv( { patron => $staff, branchcode => $library->{branchcode} } ); +} -- 2.39.5