From a8bb37aae594c3a2429cfdb70c9c70f0a4e35aa4 Mon Sep 17 00:00:00 2001 From: Lisette Scheer Date: Mon, 21 Apr 2025 15:43:36 +0000 Subject: [PATCH] Bug 20747: Allow LocalHoldsPriority to fill by hierarchical groups system rather than individual library 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 /Setup Groups and confirm current behavior/ 1. Set the " LocalHoldsPriority " system preference to "Give", "LocalHoldsPriorityPatronControl" to "home library" and "LocalHoldsPriorityItemControl" to "home library" 2. Create 2 Library 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 - 23529000035676 (Henry) - Fairfield - Changed 23529000050113 - Fairview - Changed 23529001000463 (Edna) - Franklin - 23529000121682 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 for pickup at Centerville (Priority 1) - One for 23529001000463 for pickup at fairview (Priority 2) 8. Check in 39999000000238. The hold popup should be for the second hold, not the first. /apply patch and confirm behavior still works/ 9. Apply patch 10. Rebuild the database then restart_all (in sandboxes it's an action, in ktd " dbic && updatedatabase && restart_all ") 11. Confirm that " LocalHoldsPriority " is set to "Give Library" 12. Repeat steps 7-8, behavior should be the same. / check library then library group setting/ 13. Change " LocalHoldsPriority " to "Give library then library group" 14. Set the following record level holds for the record: - Pickup at Fairfield - 23529000050113 - Pickup at Franklin - 23529000121682 - Pickup at Centerville - 23529000035676 (Henry) - Pickup at Fairview - 23529001000463 (Edna) 15. Check in 39999000000252 - should try and fill to Centerville - 23529000035676 (Henry) 16. Check in 39999000000238 - should try and fill to Fairview - 23529001000463 (Edna) 17. Cancel the hold for 23529001000463 18. Check in 39999000000238 should try and fill to 23529000121682 / check library group setting / 19. Reset the holds to match step 14. 20. Check in 39999000000252 - should try and fill to Fairfield 21. Check in 39999000000238 - should try and fill to Franklin /check no priority / 22. Reset the holds to match step 14. 23. Check in 39999000000238 - Should fill the Fairfield hold 24. Check in 39999000000252 - Should fill the Franklin hold /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: Jason Robb Signed-off-by: Brendan Lawlor Signed-off-by: Matt Blenkinsop --- C4/HoldsQueue.pm | 2 +- C4/Reserves.pm | 34 ++- installer/data/mysql/mandatory/sysprefs.sql | 2 +- .../admin/preferences/circulation.pref | 7 +- t/db_dependent/Holds/LocalHoldsPriority.t | 218 +++++++++++++++--- 5 files changed, 217 insertions(+), 46 deletions(-) diff --git a/C4/HoldsQueue.pm b/C4/HoldsQueue.pm index e3a8bc9e8e8..383800bd1d9 100644 --- a/C4/HoldsQueue.pm +++ b/C4/HoldsQueue.pm @@ -732,7 +732,7 @@ sub MapItemsToHoldRequests { my $num_items_remaining = scalar(@$available_items); # Look for Local Holds Priority matches first - if ( C4::Context->preference('LocalHoldsPriority') ) { + if ( C4::Context->preference( 'LocalHoldsPriority' ne "None" ) ) { my $LocalHoldsPriorityPatronControl = C4::Context->preference('LocalHoldsPriorityPatronControl'); my $LocalHoldsPriorityItemControl = C4::Context->preference('LocalHoldsPriorityItemControl'); diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 9630b3feac7..cadd4d413e0 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -43,6 +43,7 @@ use Koha::Holds; use Koha::ItemTypes; use Koha::Items; use Koha::Libraries; +use Koha::Library; use Koha::Patrons; use Koha::Plugins; use Koha::Policy::Holds; @@ -897,8 +898,8 @@ sub CheckReserves { } else { my $patron; my $local_hold_match; - - if ($LocalHoldsPriority) { + my $local_hold_group_match; + if ( $LocalHoldsPriority ne 'None' ) { $patron = Koha::Patrons->find( $res->{borrowernumber} ); unless ( $item->exclude_from_local_holds_priority @@ -909,13 +910,31 @@ sub CheckReserves { ( $LocalHoldsPriorityPatronControl eq 'PickupLibrary' ) ? $res->{branchcode} : ( $LocalHoldsPriorityPatronControl eq 'HomeLibrary' ) ? $patron->branchcode : undef; - $local_hold_match = - $local_holds_priority_item_branchcode eq $local_holds_priority_patron_branchcode; + if ( $LocalHoldsPriority eq 'GiveLibrary' || $LocalHoldsPriority eq 'GiveLibraryAndGroup' ) + { #Check the library first + $local_hold_match = + $local_holds_priority_item_branchcode eq $local_holds_priority_patron_branchcode; + if ( !$local_hold_match && $LocalHoldsPriority eq ('GiveLibraryAndGroup') ) + { # If there's no match at the library level, check hold groups + $local_hold_group_match = + Koha::Libraries->find( { branchcode => $local_holds_priority_item_branchcode } ) + ->validate_hold_sibling( + { branchcode => $local_holds_priority_patron_branchcode } ); + } + } + if ( $LocalHoldsPriority eq 'GiveLibraryGroup' ) { #Check only the group + $local_hold_group_match = + Koha::Libraries->find( { branchcode => $local_holds_priority_item_branchcode } ) + ->validate_hold_sibling( { branchcode => $local_holds_priority_patron_branchcode } ); + } } } # See if this item is more important than what we've got so far - if ( ( $res->{'priority'} && $res->{'priority'} < $priority ) || $local_hold_match ) { + if ( ( $res->{'priority'} && $res->{'priority'} < $priority ) + || $local_hold_match + || $local_hold_group_match ) + { next if $res->{item_group_id} && ( !$item->item_group || $item->item_group->id != $res->{item_group_id} ); @@ -944,7 +963,10 @@ sub CheckReserves { next unless $item->can_be_transferred( { to => Koha::Libraries->find( $res->{branchcode} ) } ); $priority = $res->{'priority'}; $highest = $res; - last if $local_hold_match; + last + if $local_hold_match + || ( ( $LocalHoldsPriority eq 'GiveLibraryGroup' ) && $local_hold_group_match ); + next if $local_hold_group_match; } } } diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql index 9fea00c13b7..ca5500c79ec 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -395,7 +395,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', '0', NULL, 'Enables the LocalHoldsPriority feature', 'YesNo'), +('LocalHoldsPriority', 'None', 'GiveLibrary|None|GiveLibraryGroup|GiveLibraryAndGroup' 'Enables the LocalHoldsPriority feature', 'None' '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/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref index 291c6ccec6b..565be9f41d1 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref @@ -1024,8 +1024,10 @@ Circulation: - - pref: LocalHoldsPriority choices: - 1: Give - 0: "Don't give" + GiveLibrary: Give library + None: "Don't give" + GiveLibraryGroup: Give library group + GiveLibraryAndGroup: Give library then library group - priority for filling holds to patrons whose - pref: LocalHoldsPriorityPatronControl choices: @@ -1036,6 +1038,7 @@ Circulation: choices: homebranch: "home library" holdingbranch: "holding library" + -
Note: If a group option is selected, LocalHoldsPriorityPatronControl and LocalHoldsPriorityItemControl will use the library hold group of the selected libraries. - - pref: OPACHoldsIfAvailableAtPickup choices: diff --git a/t/db_dependent/Holds/LocalHoldsPriority.t b/t/db_dependent/Holds/LocalHoldsPriority.t index c78a06e11ed..e347f5410e0 100755 --- a/t/db_dependent/Holds/LocalHoldsPriority.t +++ b/t/db_dependent/Holds/LocalHoldsPriority.t @@ -6,7 +6,7 @@ use t::lib::Mocks; use C4::Context; use Test::NoWarnings; -use Test::More tests => 8; +use Test::More tests => 7; use MARC::Record; use Koha::Patrons; @@ -14,6 +14,7 @@ use Koha::Holds; use C4::Biblio; use C4::Items; use Koha::Database; +use Koha::Library::Groups; use t::lib::TestBuilder; @@ -32,6 +33,7 @@ my $library1 = $builder->build( { source => 'Branch', } ); my $library2 = $builder->build( { source => 'Branch', } ); my $library3 = $builder->build( { source => 'Branch', } ); my $library4 = $builder->build( { source => 'Branch', } ); +my $library5 = $builder->build( { source => 'Branch', } ); my $itemtype = $builder->build( { source => 'Itemtype', @@ -41,6 +43,29 @@ my $itemtype = $builder->build( my $borrowers_count = 5; +# Create some groups +my $group1 = + Koha::Library::Group->new( { title => "Test hold group 1", ft_local_hold_group => '1' } )->store(); +my $group2 = + Koha::Library::Group->new( { title => "Test hold group 2", ft_local_hold_group => '1' } )->store(); + +#Add branches to the groups +my $group1_library1 = + Koha::Library::Group->new( { parent_id => $group1->id, branchcode => $library1->{branchcode} } )->store(); +my $group1_library2 = + Koha::Library::Group->new( { parent_id => $group1->id, branchcode => $library2->{branchcode} } )->store(); +my $group2_library1 = + Koha::Library::Group->new( { parent_id => $group2->id, branchcode => $library3->{branchcode} } )->store(); +my $group2_library2 = + Koha::Library::Group->new( { parent_id => $group2->id, branchcode => $library4->{branchcode} } )->store(); + +# group1 +# group1_library1 +# group1_library2 +# group2 +# group2_library1 +# group2_library2 + my $biblio = $builder->build_sample_biblio(); my $item = Koha::Item->new( { @@ -52,6 +77,16 @@ my $item = Koha::Item->new( }, )->store->get_from_storage; +my $item2 = Koha::Item->new( + { + biblionumber => $biblio->biblionumber, + homebranch => $library1->{branchcode}, + holdingbranch => $library1->{branchcode}, + itype => $itemtype, + exclude_from_local_holds_priority => 0, + }, +)->store->get_from_storage; + my @branchcodes = ( $library1->{branchcode}, $library2->{branchcode}, $library3->{branchcode}, $library4->{branchcode}, $library3->{branchcode}, $library4->{branchcode} @@ -60,60 +95,171 @@ my $patron_category = $builder->build( { source => 'Category', value => { exclud # Create some borrowers my @borrowernumbers; +my $r = 0; foreach ( 0 .. $borrowers_count - 1 ) { my $borrowernumber = Koha::Patron->new( { firstname => 'my firstname', surname => 'my surname ' . $_, categorycode => $patron_category->{categorycode}, - branchcode => $branchcodes[$_], + branchcode => $branchcodes[$r], } )->store->borrowernumber; push @borrowernumbers, $borrowernumber; + $r++; } -# Create five item level holds -my $i = 1; -foreach my $borrowernumber (@borrowernumbers) { - AddReserve( - { - branchcode => $branchcodes[$i], - borrowernumber => $borrowernumber, - biblionumber => $biblio->biblionumber, - priority => $i, - } +# Create five record level holds +AddReserve( + { + branchcode => $library1, + borrowernumber => $borrowernumbers[0], + biblionumber => $item->biblionumber, + priority => 1, + } +); +AddReserve( + { + branchcode => $library2, + borrowernumber => $borrowernumbers[1], + biblionumber => $item->biblionumber, + priority => 1, + } +); +AddReserve( + { + branchcode => $library3, + borrowernumber => $borrowernumbers[2], + biblionumber => $item->biblionumber, + priority => 1, + } +); +AddReserve( + { + branchcode => $library4, + borrowernumber => $borrowernumbers[3], + biblionumber => $item->biblionumber, + priority => 1, + } +); +my ( $status, $reserve, $all_reserves ); +t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'None' ); +( $status, $reserve, $all_reserves ) = CheckReserves($item); +ok( $reserve->{borrowernumber} eq $borrowernumbers[0], "Received expected results with LocalHoldsPriority disabled" ); + +subtest 'LocalHoldsPriority, GiveLibrary' => sub { #Test library only priority + plan tests => 4; + + t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'GiveLibrary' ); + + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'PickupLibrary' ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); + ( $status, $reserve, $all_reserves ) = CheckReserves($item); + ok( + $reserve->{borrowernumber} eq $borrowernumbers[3], + "Local patron is given priority when patron pickup and item home branch match" ); - $i++; -} + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'PickupLibrary' ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'holdingbranch' ); + ( $status, $reserve, $all_reserves ) = CheckReserves($item); + ok( + $reserve->{borrowernumber} eq $borrowernumbers[2], + "Local patron is given priority when patron pickup location and item home branch match" + ); -my ( $status, $reserve, $all_reserves ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'HomeLibrary' ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'holdingbranch' ); + ( $status, $reserve, $all_reserves ) = CheckReserves($item); + ok( + $reserve->{borrowernumber} eq $borrowernumbers[2], + "Local patron is given priority when patron home library and item holding branch match" + ); -t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 0 ); -( $status, $reserve, $all_reserves ) = CheckReserves($item); -ok( $reserve->{borrowernumber} eq $borrowernumbers[0], "Received expected results with LocalHoldsPriority disabled" ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'HomeLibrary' ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); + ( $status, $reserve, $all_reserves ) = CheckReserves($item); + ok( + $reserve->{borrowernumber} eq $borrowernumbers[3], + "Local patron is given priority when patron home location and item home branch match" + ); -t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 1 ); +}; -t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'PickupLibrary' ); -t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); -( $status, $reserve, $all_reserves ) = CheckReserves($item); -ok( $reserve->{borrowernumber} eq $borrowernumbers[2], "Received expected results with PickupLibrary/homebranch" ); +subtest 'LocalHoldsPriority, GiveLibraryAndGroup' => sub { #Test library then hold group priority + plan tests => 4; + t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'GiveLibraryAndGroup' ); -t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'PickupLibrary' ); -t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'holdingbranch' ); -( $status, $reserve, $all_reserves ) = CheckReserves($item); -ok( $reserve->{borrowernumber} eq $borrowernumbers[1], "Received expected results with PickupLibrary/holdingbranch" ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'PickupLibrary' ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); + ( $status, $reserve, $all_reserves ) = CheckReserves($item); + ok( + $reserve->{borrowernumber} eq $borrowernumbers[3], + "Local patron is given priority when patron pickup and item home branch match" + ); -t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'HomeLibrary' ); -t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'holdingbranch' ); -( $status, $reserve, $all_reserves ) = CheckReserves($item); -ok( $reserve->{borrowernumber} eq $borrowernumbers[2], "Received expected results with HomeLibrary/holdingbranch" ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'PickupLibrary' ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'holdingbranch' ); + ( $status, $reserve, $all_reserves ) = CheckReserves($item); + ok( + $reserve->{borrowernumber} eq $borrowernumbers[2], + "Local patron in group is given priority when patron pickup location and item home branch match" + ); -t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'HomeLibrary' ); -t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); -( $status, $reserve, $all_reserves ) = CheckReserves($item); -ok( $reserve->{borrowernumber} eq $borrowernumbers[3], "Received expected results with HomeLibrary/homebranch" ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'HomeLibrary' ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'holdingbranch' ); + ( $status, $reserve, $all_reserves ) = CheckReserves($item2); + ok( + $reserve->{borrowernumber} eq $borrowernumbers[0], + "Local patron in group is given priority when patron home library and item holding branch match" + ); + + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'HomeLibrary' ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); + ( $status, $reserve, $all_reserves ) = CheckReserves($item); + ok( + $reserve->{borrowernumber} eq $borrowernumbers[3], + "Local patron is given priority when patron home location and item home branch match" + ); + +}; + +subtest 'LocalHoldsPriority, GiveLibraryGroup' => sub { #Test hold group only priority + plan tests => 4; + t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'GiveLibraryGroup' ); + + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'PickupLibrary' ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); + ( $status, $reserve, $all_reserves ) = CheckReserves($item); + ok( + $reserve->{borrowernumber} eq $borrowernumbers[3], + "Local patron in group is given priority when patron pickup location and item home branch match" + ); + + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'PickupLibrary' ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'holdingbranch' ); + ( $status, $reserve, $all_reserves ) = CheckReserves($item); + ok( + $reserve->{borrowernumber} eq $borrowernumbers[3], + "Local patron in group is given priority when patron pickup location and item holding branch match" + ); + + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'HomeLibrary' ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'holdingbranch' ); + ( $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 holding branch match" + ); + + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'HomeLibrary' ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); + ( $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" + ); +}; $schema->storage->txn_rollback; @@ -122,7 +268,7 @@ subtest "exclude from local holds" => sub { $schema->storage->txn_begin; - t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 1 ); + t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'GiveLibrary' ); t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'HomeLibrary' ); t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); -- 2.48.1