From ce42ff4f2eb6a46bf8cac7cd5a6819eb910e4750 Mon Sep 17 00:00:00 2001 From: Lisette Scheer Date: Tue, 6 May 2025 21:38:41 +0000 Subject: [PATCH] Bug 20747: (follow-up) Fix hold queue to work with LocalHoldPriority Group options This fixes the holds queue and associated tests. I also updated the sql file to fix an issue and the LocalHoldsPriority tests to fix a typo. I'm working on final unit tests for the holds queue for all the options for QA but I wanted to get the queue changes ready for sign-off. UPDATED TEST PLAN This patch adds new options to the LocalHoldsPriority system preference to allow for the following options: 'None' the equivalent to 'Don't' pre-patch 'GiveLibrary' the equivalent to 'Do' pre-patch 'GiveLibraryAndGroup' Allows for checking the library first, then hold groups 'GiveGroup' Allows to prioritize within a group without prioritizing any library first. The included patches will update the preference in existing installations to maintain current behavior. To Test: BEFORE APPLYING THE PATCH /Tip/ I kept the following tabs open: -The holds screen (for suspending, checking order, etc) -Check in screen (can ignore holds to keep them all in place) -Holds to Pull with "All" for Libraries -System Preferences /Setup Groups and confirm current behavior/ 1. Set the following system preferences: -" LocalHoldsPriority " system preference to "Give" -"LocalHoldsPriorityPatronControl" to "home library" -"LocalHoldsPriorityItemControl" to "home library" -"RealTimeHoldsQueue" to "Enable" 2. Create 2 Library Groups - Set them to be hold groups - I called mine group 1 & group 2 3. Add 2 libraries each to each group. - Group 1 - Centerville - Fairfield - Group 2 - Fairview - Franklin 4. Find a patron for each branch in the groups - Centerville - Changed 23529000035676 (Henry) - Fairfield - Changed 23529000050113 (Jordan) - Fairview - Changed 23529001000463 (Edna) - Franklin - 23529000121682 (Nicole) 5. Find a record with at least 2 items 6. If needed change the home and holding libraries so you have at least one item for each hold group - Centerville - 39999000000252 - Fairview - Changed 39999000000238 7. Place 2 record level holds for the record. - One for 23529000035676 (Henry) for pickup at Centerville (Priority 1) - One for 23529001000463 (Edna) for pickup at Fairview (Priority 2) 8. Check in 39999000000238. The hold popup should be for the second hold (Henry), not the first (Edna.) Ignore the hold 9. Edit the record then save to trigger real time queue update 10. On the Holds Queue (submit after each set of changes): - Centerville should be for Henry - Fairview should be for Edna /apply patch and confirm behavior still works/ 10. Apply patch 11. Rebuild the database then restart_all (in sandboxes it's an action, in ktd " dbic && updatedatabase && restart_all ") 12. Confirm that " LocalHoldsPriority " is set to "Give Library" 13. Repeat steps 8-10, behavior should be the same. / check library then library group setting/ 14. Change " LocalHoldsPriority " to "Give library then library group" 15. Set the following record level holds for the record in the following order: *You can add Jordan and Nicole's, then rearrange the order. - Pickup at Fairfield - 23529000050113 (Jordan) - Pickup at Franklin - 23529000121682 (Nicole) - Pickup at Centerville - 23529000035676 (Henry) - Pickup at Fairview - 23529001000463 (Edna) 17. Check in 39999000000252 - should try and fill to Centerville - 23529000035676 (Henry) 17. Check in 39999000000238 - should try and fill to Fairview - 23529001000463 (Edna) 18. On the Hold Queue: - Centerville should be for Henry - Fairview should be for Edna 19. Suspend the hold for 23529001000463 (Edna) 20. Check in 39999000000238 should try and fill to 23529000121682 (Nicole) (ignore the hold) 21. On the Holds Queue: - Centerville should be for Henry - Fairview should be for Nicole / check library group setting / 22. Reset the holds to match step 15. 23. Change " LocalHoldsPriority " to "Give group" 24. Check in 39999000000252 - should try and fill to Fairfield (Jordan) (ignore the hold) 25. Check in 39999000000238 - should try and fill to Franklin (Nicole) (ignore the hold) 26. On the Holds Queue: - Centerville should be for Jordan - Fairview should be for Nicole /check no priority / 27. Change " LocalHoldsPriority " to "Don't give" 28. Reset the holds to match step 15. 29. Check in 39999000000238 - Should fill to Fairfield (Jordan) (ignore the hold) 30. Check in 39999000000252 - Should fill to Fairfield hold (Jordan) (ignore the hold) 31. On the Holds Queue: - Centerville should be for Jordan - Fairview should be for Nicole /Also check/ Item level holds behave as expected Different combinations of LocalHoldsPriorityItemControl and LocalHoldsPriorityPatronControl Sponsored by: Cape Libraries Automated Materials Sharing Sponsored by: Northeast Kansas Library System (NEKLS) --- C4/HoldsQueue.pm | 129 ++++++++++++++------ installer/data/mysql/mandatory/sysprefs.sql | 2 +- t/db_dependent/Holds/LocalHoldsPriority.t | 10 +- t/db_dependent/HoldsQueue.t | 24 ++-- 4 files changed, 109 insertions(+), 56 deletions(-) diff --git a/C4/HoldsQueue.pm b/C4/HoldsQueue.pm index 383800bd1d9..c0ffd43ac2f 100644 --- a/C4/HoldsQueue.pm +++ b/C4/HoldsQueue.pm @@ -732,51 +732,104 @@ sub MapItemsToHoldRequests { my $num_items_remaining = scalar(@$available_items); # Look for Local Holds Priority matches first - if ( C4::Context->preference( 'LocalHoldsPriority' ne "None" ) ) { + + my $LocalHoldsPriority = C4::Context->preference('LocalHoldsPriority'); + if ( $LocalHoldsPriority ne 'None' ) { my $LocalHoldsPriorityPatronControl = C4::Context->preference('LocalHoldsPriorityPatronControl'); my $LocalHoldsPriorityItemControl = C4::Context->preference('LocalHoldsPriorityItemControl'); - foreach my $request (@$hold_requests) { last if $num_items_remaining == 0; my $patron = Koha::Patrons->find( $request->{borrowernumber} ); next if $patron->category->exclude_from_local_holds_priority; + if ( $LocalHoldsPriority eq 'GiveLibrary' || $LocalHoldsPriority eq 'GiveLibraryAndGroup' ) { + my $local_hold_match; + foreach my $item (@$available_items) { + next if $item->{_object}->exclude_from_local_holds_priority; + + next unless _can_item_fill_request( $item, $request, $libraries ); + + next if $request->{itemnumber} && $request->{itemnumber} != $item->{itemnumber}; + + my $local_holds_priority_item_branchcode = $item->{$LocalHoldsPriorityItemControl}; + + my $local_holds_priority_patron_branchcode = + ( $LocalHoldsPriorityPatronControl eq 'PickupLibrary' ) ? $request->{branchcode} + : ( $LocalHoldsPriorityPatronControl eq 'HomeLibrary' ) ? $request->{borrowerbranch} + : undef; + + $local_hold_match = + $local_holds_priority_item_branchcode eq $local_holds_priority_patron_branchcode; + + if ($local_hold_match) { + if ( exists $items_by_itemnumber{ $item->{itemnumber} } + and not exists $allocated_items{ $item->{itemnumber} } + and not $request->{allocated} ) + { + $item_map{ $item->{itemnumber} } = { + borrowernumber => $request->{borrowernumber}, + biblionumber => $request->{biblionumber}, + holdingbranch => $item->{holdingbranch}, + pickup_branch => $request->{branchcode} + || $request->{borrowerbranch}, + reserve_id => $request->{reserve_id}, + item_level => $request->{item_level_hold}, + reservedate => $request->{reservedate}, + reservenotes => $request->{reservenotes}, + }; + $allocated_items{ $item->{itemnumber} }++; + $request->{allocated} = 1; + $num_items_remaining--; + } + } + } + } + } - my $local_hold_match; - foreach my $item (@$available_items) { - next if $item->{_object}->exclude_from_local_holds_priority; - - next unless _can_item_fill_request( $item, $request, $libraries ); - - next if $request->{itemnumber} && $request->{itemnumber} != $item->{itemnumber}; - - my $local_holds_priority_item_branchcode = $item->{$LocalHoldsPriorityItemControl}; - - my $local_holds_priority_patron_branchcode = - ( $LocalHoldsPriorityPatronControl eq 'PickupLibrary' ) ? $request->{branchcode} - : ( $LocalHoldsPriorityPatronControl eq 'HomeLibrary' ) ? $request->{borrowerbranch} - : undef; - - $local_hold_match = $local_holds_priority_item_branchcode eq $local_holds_priority_patron_branchcode; - - if ($local_hold_match) { - if ( exists $items_by_itemnumber{ $item->{itemnumber} } - and not exists $allocated_items{ $item->{itemnumber} } - and not $request->{allocated} ) - { - $item_map{ $item->{itemnumber} } = { - borrowernumber => $request->{borrowernumber}, - biblionumber => $request->{biblionumber}, - holdingbranch => $item->{holdingbranch}, - pickup_branch => $request->{branchcode} - || $request->{borrowerbranch}, - reserve_id => $request->{reserve_id}, - item_level => $request->{item_level_hold}, - reservedate => $request->{reservedate}, - reservenotes => $request->{reservenotes}, - }; - $allocated_items{ $item->{itemnumber} }++; - $request->{allocated} = 1; - $num_items_remaining--; + # Look for local group matches if no library matches were found + if ( $LocalHoldsPriority eq 'GiveLibraryGroup' || $LocalHoldsPriority eq 'GiveLibraryAndGroup' ) { + my $local_hold_group_match; + foreach my $request (@$hold_requests) { + last if $num_items_remaining == 0; + my $patron = Koha::Patrons->find( $request->{borrowernumber} ); + next if $patron->category->exclude_from_local_holds_priority; + foreach my $item (@$available_items) { + next if $item->{_object}->exclude_from_local_holds_priority; + + next unless _can_item_fill_request( $item, $request, $libraries ); + + next if $request->{itemnumber} && $request->{itemnumber} != $item->{itemnumber}; + + my $local_holds_priority_item_branchcode = $item->{$LocalHoldsPriorityItemControl}; + + my $local_holds_priority_patron_branchcode = + ( $LocalHoldsPriorityPatronControl eq 'PickupLibrary' ) ? $request->{branchcode} + : ( $LocalHoldsPriorityPatronControl eq 'HomeLibrary' ) ? $request->{borrowerbranch} + : undef; + + $local_hold_group_match = + Koha::Libraries->find( { branchcode => $local_holds_priority_item_branchcode } ) + ->validate_hold_sibling( { branchcode => $local_holds_priority_patron_branchcode } ); + + if ($local_hold_group_match) { + if ( exists $items_by_itemnumber{ $item->{itemnumber} } + and not exists $allocated_items{ $item->{itemnumber} } + and not $request->{allocated} ) + { + $item_map{ $item->{itemnumber} } = { + borrowernumber => $request->{borrowernumber}, + biblionumber => $request->{biblionumber}, + holdingbranch => $item->{holdingbranch}, + pickup_branch => $request->{branchcode} + || $request->{borrowerbranch}, + reserve_id => $request->{reserve_id}, + item_level => $request->{item_level_hold}, + reservedate => $request->{reservedate}, + reservenotes => $request->{reservenotes}, + }; + $allocated_items{ $item->{itemnumber} }++; + $request->{allocated} = 1; + $num_items_remaining--; + } } } } diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql index 7a90abe9a38..8526a673813 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -393,7 +393,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('LoadCheckoutsTableDelay','0','','Delay before auto-loading checkouts table on checkouts screen','Integer'), ('LoadSearchHistoryToTheFirstLoggedUser', '1', NULL, 'If ON, the next user will automatically get the last searches in their history', 'YesNo'), ('LocalCoverImages','0','1','Display local cover images on intranet details pages.','YesNo'), -('LocalHoldsPriority', 'None', 'GiveLibrary|None|GiveLibraryGroup|GiveLibraryAndGroup' 'Enables the LocalHoldsPriority feature', 'None' 'Choice'), +('LocalHoldsPriority', 'None', 'GiveLibrary|None|GiveLibraryGroup|GiveLibraryAndGroup' 'Enables the LocalHoldsPriority feature', 'Choice'), ('LocalHoldsPriorityItemControl', 'holdingbranch', 'holdingbranch|homebranch', 'decides if the feature operates using the item''s home or holding library.', 'Choice'), ('LocalHoldsPriorityPatronControl', 'PickupLibrary', 'HomeLibrary|PickupLibrary', 'decides if the feature operates using the library set as the patron''s home library, or the library set as the pickup library for the given hold.', 'Choice'), ('LockExpiredDelay','','','Delay for locking expired patrons (empty means no locking)','Integer'), diff --git a/t/db_dependent/Holds/LocalHoldsPriority.t b/t/db_dependent/Holds/LocalHoldsPriority.t index e347f5410e0..6f8a97541ed 100755 --- a/t/db_dependent/Holds/LocalHoldsPriority.t +++ b/t/db_dependent/Holds/LocalHoldsPriority.t @@ -112,7 +112,7 @@ foreach ( 0 .. $borrowers_count - 1 ) { # Create five record level holds AddReserve( { - branchcode => $library1, + branchcode => $library1->{branchcode}, borrowernumber => $borrowernumbers[0], biblionumber => $item->biblionumber, priority => 1, @@ -120,7 +120,7 @@ AddReserve( ); AddReserve( { - branchcode => $library2, + branchcode => $library2->{branchcode}, borrowernumber => $borrowernumbers[1], biblionumber => $item->biblionumber, priority => 1, @@ -128,7 +128,7 @@ AddReserve( ); AddReserve( { - branchcode => $library3, + branchcode => $library3->{branchcode}, borrowernumber => $borrowernumbers[2], biblionumber => $item->biblionumber, priority => 1, @@ -136,7 +136,7 @@ AddReserve( ); AddReserve( { - branchcode => $library4, + branchcode => $library4->{branchcode}, borrowernumber => $borrowernumbers[3], biblionumber => $item->biblionumber, priority => 1, @@ -257,7 +257,7 @@ subtest 'LocalHoldsPriority, GiveLibraryGroup' => sub { #Test hold group only ( $status, $reserve, $all_reserves ) = CheckReserves($item); ok( $reserve->{borrowernumber} eq $borrowernumbers[3], - "Local patron in group is given priority when patron home library and item home branch matchh" + "Local patron in group is given priority when patron home library and item home branch match" ); }; diff --git a/t/db_dependent/HoldsQueue.t b/t/db_dependent/HoldsQueue.t index ea0fff354eb..0d516da3407 100755 --- a/t/db_dependent/HoldsQueue.t +++ b/t/db_dependent/HoldsQueue.t @@ -84,7 +84,7 @@ my $itemtype = $builder->build( { source => 'Itemtype', value => { #Set up the stage # Sysprefs and cost matrix t::lib::Mocks::mock_preference( 'HoldsQueueSkipClosed', 0 ); -t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 0 ); +t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'None' ); $dbh->do( "UPDATE systempreferences SET value = ? WHERE variable = 'StaticHoldsQueueWeight'", undef, join( ',', @other_branches, $borrower_branchcode, $least_cost_branch_code ) @@ -436,7 +436,7 @@ is( scalar(@$holds_queue), 2, "Holds not filled with items from closed libraries t::lib::Mocks::mock_preference( 'HoldsQueueSkipClosed', 0 ); ## Test LocalHoldsPriority -t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 1 ); +t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'GiveLibrary' ); $dbh->do("DELETE FROM circulation_rules"); Koha::CirculationRules->set_rule( @@ -583,7 +583,7 @@ is( "Holds queue giving priority to patron who's home library matches item's holding library" ); -t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 0 ); +t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'None' ); ## End testing of LocalHoldsPriority # Bug 14297 @@ -1018,7 +1018,7 @@ subtest "Test Local Holds Priority - Bib level" => sub { plan tests => 3; Koha::Biblios->delete(); - t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 1 ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'GiveLibrary' ); t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'PickupLibrary' ); t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); my $branch = $builder->build_object( { class => 'Koha::Libraries' } ); @@ -1089,7 +1089,7 @@ subtest "Test Local Holds Priority - Item level" => sub { plan tests => 2; Koha::Biblios->delete(); - t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 1 ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'GiveLibrary' ); t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'PickupLibrary' ); t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); my $branch = $builder->build_object( { class => 'Koha::Libraries' } ); @@ -1161,7 +1161,7 @@ subtest "Test Local Holds Priority - Item level hold over Record level hold (Bug plan tests => 2; Koha::Biblios->delete(); - t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 1 ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'GiveLibrary' ); t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'PickupLibrary' ); t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); my $branch = $builder->build_object( { class => 'Koha::Libraries' } ); @@ -1232,7 +1232,7 @@ subtest "Test Local Holds Priority - Get correct item for item level hold" => su plan tests => 3; Koha::Biblios->delete(); - t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 1 ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'GiveLibrary' ); t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'PickupLibrary' ); t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); my $branch = $builder->build_object( { class => 'Koha::Libraries' } ); @@ -1316,7 +1316,7 @@ subtest "Test Local Holds Priority - Ensure no duplicate requests in holds queue $dbh->do("DELETE FROM circulation_rules"); Koha::Biblios->delete(); - t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 1 ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'GiveLibrary' ); t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'PickupLibrary' ); t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); my $branch = $builder->build_object( { class => 'Koha::Libraries' } ); @@ -1481,7 +1481,7 @@ subtest 'Excludes from local holds priority' => sub { Koha::Holds->delete; - t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 1 ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'GiveLibrary' ); t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'PickupLibrary' ); t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); @@ -1612,7 +1612,7 @@ subtest "Test item group holds" => sub { $dbh->do("DELETE FROM circulation_rules"); t::lib::Mocks::mock_preference( 'HoldsQueueSkipClosed', 0 ); - t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 0 ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'None' ); my $library = $builder->build_object( { class => 'Koha::Libraries' } ); my $category = $builder->build_object( @@ -2200,7 +2200,7 @@ subtest "Test HoldsQueuePrioritizeBranch" => sub { $schema->storage->txn_begin; - t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 0 ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'None' ); t::lib::Mocks::mock_preference( 'UseTransportCostMatrix', 0 ); my $branch1 = $builder->build_object( { class => 'Koha::Libraries' } ); @@ -2409,7 +2409,7 @@ subtest "Canceled holds should be removed from the holds queue" => sub { $schema->storage->txn_begin; - t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 0 ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'None' ); t::lib::Mocks::mock_preference( 'UseTransportCostMatrix', 0 ); my $branch1 = $builder->build_object( { class => 'Koha::Libraries' } ); -- 2.39.5