Bugzilla – Attachment 182052 Details for
Bug 20747
Allow LocalHoldsPriority to fill by hierarchical groups system rather than individual library
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 20747: (follow-up) Fix hold queue to work with LocalHoldPriority Group options
Bug-20747-follow-up-Fix-hold-queue-to-work-with-Lo.patch (text/plain), 22.64 KB, created by
Brendan Lawlor
on 2025-05-07 20:29:44 UTC
(
hide
)
Description:
Bug 20747: (follow-up) Fix hold queue to work with LocalHoldPriority Group options
Filename:
MIME Type:
Creator:
Brendan Lawlor
Created:
2025-05-07 20:29:44 UTC
Size:
22.64 KB
patch
obsolete
>From b0fa96e2545a19a150b3d0d2ef212a768d51008b Mon Sep 17 00:00:00 2001 >From: Lisette Scheer <lisette@bywatersolutions.com> >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) > >Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> >--- > 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 383800bd1d..c0ffd43ac2 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 7a90abe9a3..8526a67381 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 e347f5410e..6f8a97541e 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 ea0fff354e..0d516da340 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
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 20747
:
181220
|
181221
|
181311
|
181313
|
181314
|
181319
|
181320
|
181321
|
182008
|
182048
|
182049
|
182050
|
182051
|
182052
|
182191
|
182192
|
182193
|
182194
|
182195