diff --git a/C4/HoldsQueue.pm b/C4/HoldsQueue.pm index 8a23493142..7cf6575af8 100644 --- a/C4/HoldsQueue.pm +++ b/C4/HoldsQueue.pm @@ -911,6 +911,7 @@ sub MapItemsToHoldRequests { $item_map{ $allocation->[0] } = $allocation->[1]; $num_items_remaining--; } + _flag_local_holdgroup_matches( \%item_map, \%items_by_itemnumber ); return \%item_map; } @@ -922,7 +923,10 @@ sub MapItemsToHoldRequests { push @{ $items_by_branch{ $item->{holdingbranch} } }, $item unless exists $allocated_items{ $item->{itemnumber} }; } - return \%item_map unless keys %items_by_branch; + unless ( keys %items_by_branch ) { + _flag_local_holdgroup_matches( \%item_map, \%items_by_itemnumber ); + return \%item_map; + } # now handle the title-level requests $num_items_remaining = scalar(@$available_items) - scalar( keys %allocated_items ); @@ -1034,9 +1038,63 @@ sub MapItemsToHoldRequests { $num_items_remaining--; } } + _flag_local_holdgroup_matches( \%item_map, \%items_by_itemnumber ); return \%item_map; } +=head2 _flag_local_holdgroup_matches + + _flag_local_holdgroup_matches( $item_map, $items_by_itemnumber ); + +For each mapped item that does not already have local_holdgroup_match set, +check whether the item and the patron belong to the same library hold group +using the LocalHoldsExclusivity* system preferences. This runs independently +of LocalHoldsPriority so that the exclusivity feature can operate on its own. + +=cut + +sub _flag_local_holdgroup_matches { + my ( $item_map, $items_by_itemnumber ) = @_; + + my $excl_period = C4::Context->preference('LocalHoldsExclusivityPeriod'); + return unless $excl_period; + + my $patron_control = C4::Context->preference('LocalHoldsExclusivityPatronControl') || 'PickupLibrary'; + my $item_control = C4::Context->preference('LocalHoldsExclusivityItemControl') || 'holdingbranch'; + + foreach my $itemnumber ( keys %$item_map ) { + my $mapped = $item_map->{$itemnumber}; + next if $mapped->{local_holdgroup_match}; + + my $item_branchcode = + $items_by_itemnumber->{$itemnumber} + ? $items_by_itemnumber->{$itemnumber}->{$item_control} + : undef; + + unless ($item_branchcode) { + my $item_obj = Koha::Items->find($itemnumber); + $item_branchcode = $item_obj->$item_control if $item_obj; + } + next unless $item_branchcode; + + my $patron_branchcode = + ( $patron_control eq 'PickupLibrary' ) ? $mapped->{pickup_branch} + : ( $patron_control eq 'HomeLibrary' ) ? Koha::Patrons->find( $mapped->{borrowernumber} )->branchcode + : undef; + next unless $patron_branchcode; + + if ( $item_branchcode eq $patron_branchcode ) { + $mapped->{local_holdgroup_match} = 1; + next; + } + + my $item_library = Koha::Libraries->find( { branchcode => $item_branchcode } ); + if ( $item_library && $item_library->validate_hold_sibling( { branchcode => $patron_branchcode } ) ) { + $mapped->{local_holdgroup_match} = 1; + } + } +} + =head2 _can_item_fill_request my $bool = _can_item_fill_request( $item, $request, $libraries ); diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 1abef7ac71..4f4fdb9275 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -895,27 +895,50 @@ sub CheckReserves { } } - # Local holds priority exclusivity period: when the queue has + # Local holds exclusivity period: when the queue has # targeted a local-group item for this hold and we're still # within the exclusivity window, skip non-local items so the # targeted local item can fulfil it. If the queue has not yet # run for this reserve (no hold_fill_targets row), we fall # through and let the hold be filled normally. - my $excl_period = C4::Context->preference('LocalHoldsPriorityExclusivityPeriod'); - if ( $excl_period - && !$local_hold_match - && !$local_hold_group_match - && $res->{reserve_id} ) - { - my $reservedate_dt = eval { dt_from_string( $res->{reservedate} ) }; - if ($reservedate_dt) { - my $age_days = dt_from_string->delta_days($reservedate_dt)->in_units('days'); - if ( $age_days < $excl_period ) { - my $target = - Koha::Database->new->schema->resultset('HoldFillTarget') - ->search( { reserve_id => $res->{reserve_id} }, { rows => 1 } ) - ->next; - next if $target && $target->local_holdgroup_match; + # This is independent of LocalHoldsPriority. + my $excl_period = C4::Context->preference('LocalHoldsExclusivityPeriod'); + if ( $excl_period && $res->{reserve_id} ) { + my $excl_item_control = + C4::Context->preference('LocalHoldsExclusivityItemControl') || 'holdingbranch'; + my $excl_patron_control = + C4::Context->preference('LocalHoldsExclusivityPatronControl') || 'PickupLibrary'; + + my $excl_item_branchcode = $item->$excl_item_control; + $patron //= Koha::Patrons->find( $res->{borrowernumber} ); + my $excl_patron_branchcode = + ( $excl_patron_control eq 'PickupLibrary' ) ? $res->{branchcode} + : ( $excl_patron_control eq 'HomeLibrary' ) ? $patron->branchcode + : undef; + + my $is_local_for_exclusivity = 0; + if ( $excl_item_branchcode && $excl_patron_branchcode ) { + if ( $excl_item_branchcode eq $excl_patron_branchcode ) { + $is_local_for_exclusivity = 1; + } else { + my $excl_library = Koha::Libraries->find( { branchcode => $excl_item_branchcode } ); + $is_local_for_exclusivity = 1 + if $excl_library + && $excl_library->validate_hold_sibling( { branchcode => $excl_patron_branchcode } ); + } + } + + if ( !$is_local_for_exclusivity ) { + my $reservedate_dt = eval { dt_from_string( $res->{reservedate} ) }; + if ($reservedate_dt) { + my $age_days = dt_from_string->delta_days($reservedate_dt)->in_units('days'); + if ( $age_days < $excl_period ) { + my $target = + Koha::Database->new->schema->resultset('HoldFillTarget') + ->search( { reserve_id => $res->{reserve_id} }, { rows => 1 } ) + ->next; + next if $target && $target->local_holdgroup_match; + } } } } diff --git a/installer/data/mysql/atomicupdate/bug_41410.pl b/installer/data/mysql/atomicupdate/bug_41410.pl index adf4893f20..195f60cfba 100755 --- a/installer/data/mysql/atomicupdate/bug_41410.pl +++ b/installer/data/mysql/atomicupdate/bug_41410.pl @@ -3,7 +3,7 @@ use Koha::Installer::Output qw(say_success say_info say_warning); return { bug_number => "41410", - description => "Add local holds priority exclusivity period", + description => "Add local holds exclusivity period", up => sub { my ($args) = @_; my ( $dbh, $out ) = @$args{qw(dbh out)}; @@ -36,23 +36,61 @@ return { say_success( $out, "Added column 'tmp_holdsqueue.local_holdgroup_match'" ); } + # Rename old pref if it exists (upgrade path) + my ($old_exists) = $dbh->selectrow_array( + q{SELECT COUNT(*) FROM systempreferences WHERE variable = 'LocalHoldsPriorityExclusivityPeriod'}); + if ($old_exists) { + $dbh->do( + q{UPDATE systempreferences SET variable = 'LocalHoldsExclusivityPeriod', + explanation = 'For this many days after a hold is placed, only items flagged as a local hold group match by the holds queue may fill the hold. Set to 0 to disable.' + WHERE variable = 'LocalHoldsPriorityExclusivityPeriod'} + ); + say_success( + $out, + "Renamed system preference 'LocalHoldsPriorityExclusivityPeriod' to 'LocalHoldsExclusivityPeriod'" + ); + } else { + $dbh->do( + q{ + INSERT IGNORE INTO systempreferences (variable, value, options, explanation, type) + VALUES ( + 'LocalHoldsExclusivityPeriod', + '0', + NULL, + 'For this many days after a hold is placed, only items flagged as a local hold group match by the holds queue may fill the hold. Set to 0 to disable.', + 'Integer' + ) + } + ); + say_success( $out, "Added new system preference 'LocalHoldsExclusivityPeriod'" ); + } + $dbh->do( q{ INSERT IGNORE INTO systempreferences (variable, value, options, explanation, type) VALUES ( - 'LocalHoldsPriorityExclusivityPeriod', - '0', - NULL, - 'When LocalHoldsPriority is set, for this many days after a hold is placed, only items flagged as a local hold group match by the holds queue may fill the hold. Set to 0 to disable.', - 'Integer' + 'LocalHoldsExclusivityPatronControl', + 'PickupLibrary', + 'PickupLibrary|HomeLibrary', + 'Determine local hold group match for exclusivity by comparing the hold pickup library or the patron home library.', + 'Choice' ) } ); - say_success( $out, "Added new system preference 'LocalHoldsPriorityExclusivityPeriod'" ); + say_success( $out, "Added new system preference 'LocalHoldsExclusivityPatronControl'" ); - say_info( - $out, - "The LocalHoldsPriorityExclusivityPeriod feature only takes effect when LocalHoldsPriority is set to something other than 'None'." + $dbh->do( + q{ + INSERT IGNORE INTO systempreferences (variable, value, options, explanation, type) + VALUES ( + 'LocalHoldsExclusivityItemControl', + 'holdingbranch', + 'homebranch|holdingbranch', + 'Determine local hold group match for exclusivity by comparing the item home library or holding library.', + 'Choice' + ) + } ); + say_success( $out, "Added new system preference 'LocalHoldsExclusivityItemControl'" ); }, }; diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql index 0e546d9434..00e5754310 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -420,8 +420,10 @@ INSERT INTO systempreferences ( `variable`, `value` ) VALUES ('LoadCheckoutsTableDelay','0'), ('LoadSearchHistoryToTheFirstLoggedUser', '1'), ('LocalCoverImages','0'), +('LocalHoldsExclusivityItemControl', 'holdingbranch'), +('LocalHoldsExclusivityPatronControl', 'PickupLibrary'), +('LocalHoldsExclusivityPeriod', '0'), ('LocalHoldsPriority', 'None'), -('LocalHoldsPriorityExclusivityPeriod', '0'), ('LocalHoldsPriorityItemControl', 'holdingbranch'), ('LocalHoldsPriorityPatronControl', 'PickupLibrary'), ('LockExpiredDelay',''), 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 a27cb4b6b8..c0484d7b55 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 @@ -1105,10 +1105,20 @@ Circulation: -
Note: If a group option is selected, LocalHoldsPriorityPatronControl and LocalHoldsPriorityItemControl will use the library hold group of the selected libraries. - - For - - pref: LocalHoldsPriorityExclusivityPeriod + - pref: LocalHoldsExclusivityPeriod class: integer - - days after a hold is placed, only items flagged by the holds queue as a local hold group match may be used to fill the hold. Set to 0 to disable. - -
Note: Only takes effect when LocalHoldsPriority is set to something other than 'Don't give'. + - days after a hold is placed, only items whose + - pref: LocalHoldsExclusivityItemControl + choices: + homebranch: "home library" + holdingbranch: "holding library" + - is in the same library hold group as the patron's + - pref: LocalHoldsExclusivityPatronControl + choices: + PickupLibrary: "pickup library" + HomeLibrary: "home library" + - may be used to fill the hold. Set to 0 to disable. + -
Note: This feature is independent of LocalHoldsPriority. It restricts which items can fill a hold rather than altering the priority order. - - pref: OPACHoldsIfAvailableAtPickup choices: diff --git a/t/db_dependent/Holds/LocalHoldsPriority.t b/t/db_dependent/Holds/LocalHoldsPriority.t index c3e19485e4..2de5a9d9d4 100755 --- a/t/db_dependent/Holds/LocalHoldsPriority.t +++ b/t/db_dependent/Holds/LocalHoldsPriority.t @@ -392,16 +392,19 @@ subtest "exclude from local holds" => sub { $schema->storage->txn_rollback; }; -subtest 'LocalHoldsPriorityExclusivityPeriod' => sub { - plan tests => 4; +subtest 'LocalHoldsExclusivityPeriod' => sub { + plan tests => 5; $schema->storage->txn_begin; my $dbh = C4::Context->dbh; - t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'GiveLibraryGroup' ); - t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'HomeLibrary' ); - t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); + # Exclusivity is independent of LocalHoldsPriority — disable it to prove that. + t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'None' ); + + # Configure exclusivity control prefs + t::lib::Mocks::mock_preference( 'LocalHoldsExclusivityPatronControl', 'HomeLibrary' ); + t::lib::Mocks::mock_preference( 'LocalHoldsExclusivityItemControl', 'homebranch' ); my $group = Koha::Library::Group->new( { title => "Excl period group", ft_local_hold_group => 1 } )->store(); @@ -419,18 +422,16 @@ subtest 'LocalHoldsPriorityExclusivityPeriod' => sub { my $biblio = $builder->build_sample_biblio(); my $local_item = $builder->build_sample_item( { - biblionumber => $biblio->biblionumber, - homebranch => $other_lib_in->branchcode, - holdingbranch => $other_lib_in->branchcode, - exclude_from_local_holds_priority => 0, + biblionumber => $biblio->biblionumber, + homebranch => $other_lib_in->branchcode, + holdingbranch => $other_lib_in->branchcode, } ); my $non_local_item = $builder->build_sample_item( { - biblionumber => $biblio->biblionumber, - homebranch => $non_local_lib->branchcode, - holdingbranch => $non_local_lib->branchcode, - exclude_from_local_holds_priority => 0, + biblionumber => $biblio->biblionumber, + homebranch => $non_local_lib->branchcode, + holdingbranch => $non_local_lib->branchcode, } ); @@ -469,7 +470,7 @@ subtest 'LocalHoldsPriorityExclusivityPeriod' => sub { ); # 1. Pref disabled (0): the non-local item may still fill the hold. - t::lib::Mocks::mock_preference( 'LocalHoldsPriorityExclusivityPeriod', 0 ); + t::lib::Mocks::mock_preference( 'LocalHoldsExclusivityPeriod', 0 ); my ( undef, $reserve ) = CheckReserves($non_local_item); is( $reserve->{reserve_id}, $reserve_id, @@ -477,14 +478,21 @@ subtest 'LocalHoldsPriorityExclusivityPeriod' => sub { ); # 2. Pref active, hold is fresh: non-local item must be skipped. - t::lib::Mocks::mock_preference( 'LocalHoldsPriorityExclusivityPeriod', 7 ); + t::lib::Mocks::mock_preference( 'LocalHoldsExclusivityPeriod', 7 ); ( undef, $reserve ) = CheckReserves($non_local_item); ok( !$reserve, "Non-local item is blocked by exclusivity period when queue targeted a local match" ); - # 3. Push reservedate back past the exclusivity window: should fill again. + # 3. The local-group item itself should still fill the hold during the window. + ( undef, $reserve ) = CheckReserves($local_item); + is( + $reserve->{reserve_id}, $reserve_id, + "Local-group item fills the hold even during the exclusivity window" + ); + + # 4. Push reservedate back past the exclusivity window: should fill again. $dbh->do( q{UPDATE reserves SET reservedate = DATE_SUB(CURRENT_DATE, INTERVAL 10 DAY) WHERE reserve_id = ?}, undef, $reserve_id @@ -495,7 +503,7 @@ subtest 'LocalHoldsPriorityExclusivityPeriod' => sub { "Non-local item fills the hold once the exclusivity period has expired" ); - # 4. Remove the hold_fill_targets row: with no queue target we must not block. + # 5. Remove the hold_fill_targets row: with no queue target we must not block. $dbh->do( q{UPDATE reserves SET reservedate = CURRENT_DATE WHERE reserve_id = ?}, undef, $reserve_id ); $dbh->do( q{DELETE FROM hold_fill_targets WHERE reserve_id = ?}, undef, $reserve_id ); ( undef, $reserve ) = CheckReserves($non_local_item); diff --git a/t/db_dependent/HoldsQueue.t b/t/db_dependent/HoldsQueue.t index 316bedfcf0..c651cba3cf 100755 --- a/t/db_dependent/HoldsQueue.t +++ b/t/db_dependent/HoldsQueue.t @@ -9,7 +9,7 @@ use Modern::Perl; use Test::NoWarnings; -use Test::More tests => 67; +use Test::More tests => 68; use Data::Dumper; use C4::Calendar; @@ -471,7 +471,7 @@ is( ); is( $holds_queue->[0]->{local_holdgroup_match}, 1, - "tmp_holdsqueue.local_holdgroup_match is set when LocalHoldsPriority finds a local match" + "tmp_holdsqueue.local_holdgroup_match is set for a local match" ); my $hft_row = $dbh->selectrow_hashref( "SELECT local_holdgroup_match FROM hold_fill_targets WHERE borrowernumber = ?", @@ -479,7 +479,7 @@ my $hft_row = $dbh->selectrow_hashref( ); is( $hft_row->{local_holdgroup_match}, 1, - "hold_fill_targets.local_holdgroup_match is set when LocalHoldsPriority finds a local match" + "hold_fill_targets.local_holdgroup_match is set for a local match" ); ### Test branch transfer limits ### @@ -2546,3 +2546,104 @@ subtest "Test unallocated option" => sub { "Previously allocated hold not updated when unallocated passed and others are allocated" ); }; + +subtest "local_holdgroup_match is set independently of LocalHoldsPriority" => sub { + plan tests => 4; + + $schema->storage->txn_begin; + + # Disable LocalHoldsPriority to prove independence + t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 'None' ); + + # Enable exclusivity so _flag_local_holdgroup_matches runs + t::lib::Mocks::mock_preference( 'LocalHoldsExclusivityPeriod', 7 ); + t::lib::Mocks::mock_preference( 'LocalHoldsExclusivityPatronControl', 'HomeLibrary' ); + t::lib::Mocks::mock_preference( 'LocalHoldsExclusivityItemControl', 'homebranch' ); + + my $group = $builder->build_object( { class => 'Koha::Library::Groups', value => { ft_local_hold_group => 1 } } ); + + my $lib_a = $builder->build_object( { class => 'Koha::Libraries' } ); + my $lib_b = $builder->build_object( { class => 'Koha::Libraries' } ); + my $lib_c = $builder->build_object( { class => 'Koha::Libraries' } ); # not in group + + $builder->build_object( + { class => 'Koha::Library::Groups', value => { parent_id => $group->id, branchcode => $lib_a->branchcode } } ); + $builder->build_object( + { class => 'Koha::Library::Groups', value => { parent_id => $group->id, branchcode => $lib_b->branchcode } } ); + + Koha::CirculationRules->set_rule( + { + rule_name => 'holdallowed', + rule_value => 'from_any_library', + branchcode => undef, + itemtype => undef, + } + ); + + # Use a single item at lib_b so the allocation is deterministic. + # lib_b is in the hold group with lib_a but not with lib_c. + my $biblio = $builder->build_sample_biblio(); + my $item = $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + homebranch => $lib_b->branchcode, + holdingbranch => $lib_b->branchcode, + } + ); + + my $patron_local = + $builder->build_object( { class => 'Koha::Patrons', value => { branchcode => $lib_a->branchcode } } ); + my $patron_non_local = + $builder->build_object( { class => 'Koha::Patrons', value => { branchcode => $lib_c->branchcode } } ); + + # Hold by the local patron (lib_a, in same group as item at lib_b) + my $reserve_id_local = AddReserve( + { + branchcode => $lib_a->branchcode, + borrowernumber => $patron_local->borrowernumber, + biblionumber => $biblio->biblionumber, + priority => 1, + } + ); + + C4::HoldsQueue::CreateQueue(); + + my $hft_local = + $schema->resultset('HoldFillTarget')->search( { reserve_id => $reserve_id_local }, { rows => 1 } )->next; + ok( $hft_local, "Hold fill target created for the local patron's hold" ); + +SKIP: { + skip "No hold_fill_target row to inspect", 1 unless $hft_local; + is( + $hft_local->local_holdgroup_match, 1, + "local_holdgroup_match is set even though LocalHoldsPriority is 'None'" + ); + } + + # Now test a non-local patron (lib_c, NOT in hold group with item at lib_b) + $schema->resultset('Reserve')->search( {} )->delete; + my $reserve_id_non_local = AddReserve( + { + branchcode => $lib_c->branchcode, + borrowernumber => $patron_non_local->borrowernumber, + biblionumber => $biblio->biblionumber, + priority => 1, + } + ); + + C4::HoldsQueue::CreateQueue(); + + my $hft_non_local = + $schema->resultset('HoldFillTarget')->search( { reserve_id => $reserve_id_non_local }, { rows => 1 } )->next; + ok( $hft_non_local, "Hold fill target created for the non-local patron's hold" ); + +SKIP: { + skip "No hold_fill_target row to inspect", 1 unless $hft_non_local; + is( + $hft_non_local->local_holdgroup_match, 0, + "local_holdgroup_match is NOT set for a non-local match" + ); + } + + $schema->storage->txn_rollback; +};