Bugzilla – Attachment 185305 Details for
Bug 40529
Update how hold groups work
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 40529: Add unit tests
Bug-40529-Add-unit-tests.patch (text/plain), 17.51 KB, created by
OpenFifth Sandboxes
on 2025-08-11 09:24:53 UTC
(
hide
)
Description:
Bug 40529: Add unit tests
Filename:
MIME Type:
Creator:
OpenFifth Sandboxes
Created:
2025-08-11 09:24:53 UTC
Size:
17.51 KB
patch
obsolete
>From 2cdeca88e5699cf7a12af7231533ab10bdd779b8 Mon Sep 17 00:00:00 2001 >From: Pedro Amorim <pedro.amorim@openfifth.co.uk> >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': ><staff_url>/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: ><staff_url>/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: ><staff_url>/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: ><staff_url>/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: ><staff_url>/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: ><staff_url>/cgi-bin/koha/members/moremember.pl?borrowernumber=19 >21) Select 'both', click 'Cancel selected'. Confirm cancellation. >22) Visit 'holds to pull': ><staff_url>/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 <anneli.osterman@koha-suomi.fi> >--- > 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
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 40529
:
184868
|
184869
|
184870
|
184871
|
184872
|
185137
|
185138
|
185139
|
185140
|
185141
|
185210
|
185211
|
185212
|
185213
|
185214
|
185215
|
185303
|
185304
| 185305 |
185306
|
185307