gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Bug 41410: (follow-up) Decouple exclusivity from LocalHoldsPriority 

LocalHoldsExclusivity and LocalHoldsPriority are adjacent but distinct
features: priority alters *which patron's hold* gets filled first, while
exclusivity restricts *which items* may fill a hold during a time window.
Each should be usable independently of the other.

Changes:
- Rename LocalHoldsPriorityExclusivityPeriod to LocalHoldsExclusivityPeriod
- Add LocalHoldsExclusivityPatronControl (PickupLibrary/HomeLibrary)
- Add LocalHoldsExclusivityItemControl (homebranch/holdingbranch)
- HoldsQueue: add _flag_local_holdgroup_matches() that flags
  local_holdgroup_match on allocated items using hold-group matching
  and the new exclusivity control prefs, regardless of LocalHoldsPriority
- Reserves: CheckReserves exclusivity check now computes its own
  "is this item local" using the new prefs and hold-group siblings,
  independent of the LocalHoldsPriority matching variables
- Atomicupdate handles both fresh install and rename-from-old-pref paths
- Pref UI note clarifies independence from LocalHoldsPriority

Test plan:
1. Apply patch, run updatedatabase.pl
2. Verify three new sysprefs exist in Administration > System preferences
   > Circulation: LocalHoldsExclusivityPeriod, LocalHoldsExclusivityPatronControl,
   LocalHoldsExclusivityItemControl
3. Set LocalHoldsPriority to "Don't give" (None)
4. Create a library hold group with at least two libraries (e.g. Lib-A, Lib-B)
5. Create a third library (Lib-C) NOT in the hold group
6. Create an item at Lib-B
7. Place a hold for a patron at Lib-A (pickup at Lib-A)
8. Set LocalHoldsExclusivityPeriod to 7, PatronControl to HomeLibrary,
   ItemControl to homebranch
9. Run the holds queue builder (misc/cronjobs/holds/build_holds_queue.pl)
10. Verify hold_fill_targets.local_holdgroup_match = 1 for that hold
    (item at Lib-B is in the same hold group as patron at Lib-A)
11. Check in the item at Lib-C — it should NOT fill the hold (exclusivity
    blocks the non-local item within the 7-day window)
12. Check in the item at Lib-B — it SHOULD fill the hold (local match)
13. Reset: delete the hold, create a new one with a patron at Lib-C
    (pickup at Lib-C). Run the queue again.
14. Verify hold_fill_targets.local_holdgroup_match = 0 for this hold
15. The item at Lib-B should now fill this hold at Lib-C checkin
    (non-local patron, no exclusivity protection)
16. Set LocalHoldsExclusivityPeriod to 0 — verify non-local items can
    fill holds normally regardless of hold_fill_targets flag
17. prove t/db_dependent/Holds/LocalHoldsPriority.t — passes
18. prove t/db_dependent/HoldsQueue.t — passes
19. prove t/db_dependent/Reserves.t — passes

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This commit is contained in:
Martin Renvoize 2026-04-16 09:54:18 +01:00 committed by Pedro Amorim
parent bec5c357b4
commit 4b23e5c5d1
No known key found for this signature in database
GPG Key ID: 06B75BA7F3894D55
7 changed files with 291 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

60
C4/HoldsQueue.pm
View File

@ -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 );

55
C4/Reserves.pm
View File

@ -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;
}
}
}
}

58
installer/data/mysql/atomicupdate/bug_41410.pl
View File

@ -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'" );
},
};

4
installer/data/mysql/mandatory/sysprefs.sql
View File

@ -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',''),

16
koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
View File

@ -1105,10 +1105,20 @@ Circulation:
- <br/><strong>Note:</strong> If a group option is selected, LocalHoldsPriorityPatronControl and LocalHoldsPriorityItemControl will use the <a href="/cgi-bin/koha/admin/library_groups.pl">library hold group</a> 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.
- <br/><strong>Note:</strong> 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 <a href="/cgi-bin/koha/admin/library_groups.pl">library hold group</a> 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.
- <br/><strong>Note:</strong> This feature is independent of LocalHoldsPriority. It restricts which items can fill a hold rather than altering the priority order.
-
- pref: OPACHoldsIfAvailableAtPickup
choices:

42
t/db_dependent/Holds/LocalHoldsPriority.t
View File

@ -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);

107
t/db_dependent/HoldsQueue.t
View File

@ -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;
};