Bug 42608: Stop hold pickup expiration from week-jumping over closures
Koha::Hold::set_waiting calls Koha::Calendar::days_forward to derive the
holds-shelf expiration date when ExcludeHolidaysFromMaxPickUpDelay is on.
days_forward iterated next_open_days($dt, 1) ReservesMaxPickUpDelay times,
and next_open_days consults get_push_amt for every closed day it
encounters. Under useDaysMode=Dayweek, get_push_amt returns 7 for any
closure that is not a permanently closed weekday -- so an intermediate
Bank Holiday Monday inside the pickup window pushed the cursor a whole
week forward instead of one day, inflating the expiration by seven days.
Dayweek is a loan-due-date concept: it exists to keep a Friday-due loan
due on a Friday across closures. A patron does not pick a weekday for a
hold-shelf collection; staff set the expiration when they trap the item,
and the only thing that should drive it is how many open days the patron
should have to come and collect. days_forward is now decoupled from
useDaysMode and always counts forward one open day at a time, ignoring
closures without consuming them from the count.
next_open_days, prev_open_days, get_push_amt and addDays are unchanged --
loan due-date math keeps the existing Dayweek behaviour.
Test plan:
1. Inside KTD, run the updated unit tests:
prove t/db_dependent/Calendar.t
prove t/db_dependent/Hold.t
Confirm both pass, including the new Bug 42608 sub-tests. These cover
the full matrix of starting weekdays and useDaysMode values; the
manual steps below are a single confirmatory pass through the UI.
2. Manual reproduction (UI only -- no database or CLI access needed):
- Set sysprefs: useDaysMode=Dayweek, ReservesMaxPickUpDelay=6,
ExcludeHolidaysFromMaxPickUpDelay=ON, ExpireReservesOnHolidays=ON.
- Administration > Calendar: on a test branch, add one single
(one-time) holiday -- not weekly/repeating -- on any weekday within
the next 6 days that isn't already closed (e.g. the day after
tomorrow, as long as it isn't a Sunday or other closed day). This
one-off closure inside the pickup window is what triggers the bug.
- Place a hold on an available item, then check the item in at that
branch and confirm/trap it. This stamps waitingdate with today's
date automatically -- no manual date entry required.
- Check the hold's expiration date (Holds awaiting pickup list, or
the patron's holds tab):
Without the fix: expiration lands a full 7 days later than it
should (today + 6 + 7), from the week-jump.
With the fix: expiration is today + 6 open days, only skipping
the one closed day.
3. Verify the existing Calendar mode behaviour is unchanged:
- Switch useDaysMode to Calendar and repeat step 2 -- the same
expected expiration date should apply.
4. Verify loan due-date behaviour is unchanged:
- With useDaysMode=Dayweek, check out an item whose loan period would
place its due date on the same closed day used in step 2. Confirm
the due date still rolls forward by a whole week (the original
Dayweek intent), proving this patch did not regress
addDays/addDuration.
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Jackie Usher <jackie.usher@westsussex.gov.uk>
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
Pedro Amorim
parent
9c3d3306e3
commit
58d41bfedf
|
|
@ -330,8 +330,22 @@ sub days_forward {
|
|||
|
||||
my $base_dt = $start_dt->clone();
|
||||
|
||||
# Advance one open day at a time, skipping any closed days encountered
|
||||
# along the way. days_mode is deliberately not consulted here: this
|
||||
# method's purpose is to count open days forward (e.g. for a hold
|
||||
# pickup window), not to align a recurring due-date to the same
|
||||
# weekday, which is what next_open_days does under 'Dayweek'.
|
||||
while ( $num_days-- ) {
|
||||
$base_dt = $self->next_open_days( $base_dt, 1 );
|
||||
$base_dt->add( days => 1 );
|
||||
my $i = 0;
|
||||
while ( $self->is_holiday($base_dt) && $i < OPEN_DAYS_SEARCH_MAX_ITERATIONS ) {
|
||||
$base_dt->add( days => 1 );
|
||||
++$i;
|
||||
}
|
||||
if ( $self->is_holiday($base_dt) ) {
|
||||
Koha::Exceptions::Calendar::NoOpenDays->throw(
|
||||
sprintf( 'Unable to find an open day for library %s', $self->{branchcode} ) );
|
||||
}
|
||||
}
|
||||
|
||||
return $base_dt;
|
||||
|
|
@ -761,10 +775,16 @@ syspref is set to either 'Datedue', 'Calendar' or 'Dayweek'.
|
|||
|
||||
$datetime = $calendar->days_forward($start_dt, $to_add)
|
||||
|
||||
Passed a Datetime and number of days, returns another Datetime representing
|
||||
the next open day after adding the passed number of days. It is intended for
|
||||
use to calculate the due date when useDaysMode syspref is set to either
|
||||
'Datedue', 'Calendar' or 'Dayweek'.
|
||||
Passed a DateTime and a number of days, returns a DateTime representing that
|
||||
many open days after the starting date. Closed days (weekly closures and
|
||||
holidays) are skipped without being counted toward $to_add, so the result is
|
||||
always $to_add open days on from $start_dt.
|
||||
|
||||
Unlike L</next_open_days>, this method does not consult C<useDaysMode>: it
|
||||
always advances one calendar day at a time. It is intended for windows that
|
||||
should give the patron a fixed number of open days, such as a hold's pickup
|
||||
shelf expiration -- not for aligning a recurring due-date to a particular
|
||||
weekday.
|
||||
|
||||
=head2 set_daysmode
|
||||
|
||||
|
|
|
|||
|
|
@ -430,7 +430,7 @@ $builder->build_object(
|
|||
);
|
||||
|
||||
subtest 'days_forward' => sub {
|
||||
plan tests => 4;
|
||||
plan tests => 9;
|
||||
|
||||
my $forwarded_dt = $calendar->days_forward( $today, 2 );
|
||||
my $expected = $today->clone->add( days => 2 );
|
||||
|
|
@ -448,6 +448,67 @@ subtest 'days_forward' => sub {
|
|||
|
||||
$forwarded_dt = $calendar->days_forward( $today, -2 );
|
||||
is( $forwarded_dt->ymd, $today->ymd, 'negative day should return start dt' );
|
||||
|
||||
# Bug 42608: days_forward must count open days regardless of useDaysMode.
|
||||
# Previously Dayweek mode caused a 7-day jump when an intermediate
|
||||
# closure landed on a normally-open weekday, over-extending hold
|
||||
# pickup windows by a week.
|
||||
my $dayweek_library = $builder->build_object( { class => 'Koha::Libraries' } );
|
||||
my $dayweek_branch = $dayweek_library->branchcode;
|
||||
|
||||
# Sundays are the only weekly closure.
|
||||
$schema->resultset('RepeatableHoliday')->create(
|
||||
{
|
||||
branchcode => $dayweek_branch,
|
||||
weekday => 0,
|
||||
description => q{Sunday},
|
||||
title => q{Sunday},
|
||||
}
|
||||
);
|
||||
|
||||
# Single (one-off) holiday on a Monday 2 weeks out from a known Tuesday.
|
||||
my $tuesday = dt_from_string('2026-04-28'); # Tuesday
|
||||
my $bh = dt_from_string('2026-05-04'); # Monday Bank Holiday
|
||||
$schema->resultset('SpecialHoliday')->create(
|
||||
{
|
||||
branchcode => $dayweek_branch,
|
||||
day => $bh->day,
|
||||
month => $bh->month,
|
||||
year => $bh->year,
|
||||
isexception => 0,
|
||||
title => q{May Bank Holiday},
|
||||
description => q{May Bank Holiday},
|
||||
}
|
||||
);
|
||||
|
||||
my $cache = Koha::Caches->get_instance();
|
||||
$cache->clear_from_cache( $dayweek_branch . '_holidays' );
|
||||
|
||||
for my $mode (qw(Calendar Datedue Dayweek Days)) {
|
||||
my $dw_cal = Koha::Calendar->new(
|
||||
branchcode => $dayweek_branch,
|
||||
days_mode => $mode,
|
||||
);
|
||||
|
||||
# Tue 2026-04-28 + 6 open days, skipping Sun 3 May and Mon 4 May,
|
||||
# should land on Wed 2026-05-06 -- not Tue 2026-05-12.
|
||||
my $got = $dw_cal->days_forward( $tuesday, 6 );
|
||||
is(
|
||||
$got->ymd, '2026-05-06',
|
||||
"days_forward(+6) over Sun + Mon BH from Tue lands on Wed (days_mode=$mode)"
|
||||
);
|
||||
}
|
||||
|
||||
# Confirm same behaviour over the BH from Monday (final day lands past
|
||||
# the closures): Mon 27 Apr + 6 open days = Tue 5 May.
|
||||
my $monday = dt_from_string('2026-04-27');
|
||||
my $dw_cal = Koha::Calendar->new( branchcode => $dayweek_branch, days_mode => 'Dayweek' );
|
||||
is(
|
||||
$dw_cal->days_forward( $monday, 6 )->ymd, '2026-05-05',
|
||||
'days_forward(+6) from Mon over Sun + Mon BH lands on Tue (no week-jump in Dayweek)'
|
||||
);
|
||||
|
||||
$cache->clear_from_cache( $dayweek_branch . '_holidays' );
|
||||
};
|
||||
|
||||
subtest 'crossing_DST' => sub {
|
||||
|
|
|
|||
|
|
@ -20,6 +20,7 @@ use Modern::Perl;
|
|||
use t::lib::Mocks;
|
||||
use C4::Context;
|
||||
use C4::Biblio qw( AddBiblio );
|
||||
use Koha::Caches;
|
||||
use Koha::Database;
|
||||
use Koha::Libraries;
|
||||
use C4::Calendar;
|
||||
|
|
@ -317,7 +318,7 @@ subtest "store() tests" => sub {
|
|||
|
||||
subtest "set_waiting() tests" => sub {
|
||||
|
||||
plan tests => 2;
|
||||
plan tests => 3;
|
||||
|
||||
$schema->storage->txn_begin();
|
||||
|
||||
|
|
@ -331,6 +332,76 @@ subtest "set_waiting() tests" => sub {
|
|||
$hold->set_waiting();
|
||||
isnt( $hold->waitingdate, undef, "Setting waiting when not waiting already should update waitingdate" );
|
||||
|
||||
subtest 'expirationdate over a Bank Holiday with useDaysMode=Dayweek' => sub {
|
||||
|
||||
# Bug 42608: when ExcludeHolidaysFromMaxPickUpDelay is on and
|
||||
# useDaysMode=Dayweek, intermediate Bank Holiday Mondays should
|
||||
# not cause the pickup window to jump a whole week forward. The
|
||||
# patron should always get ReservesMaxPickUpDelay open days.
|
||||
plan tests => 5;
|
||||
|
||||
t::lib::Mocks::mock_preference( 'ReservesMaxPickUpDelay', 6 );
|
||||
t::lib::Mocks::mock_preference( 'ExcludeHolidaysFromMaxPickUpDelay', 1 );
|
||||
t::lib::Mocks::mock_preference( 'ExpireReservesOnHolidays', 1 );
|
||||
t::lib::Mocks::mock_preference( 'useDaysMode', 'Dayweek' );
|
||||
|
||||
my $library = $builder->build_object( { class => 'Koha::Libraries' } );
|
||||
my $branch = $library->branchcode;
|
||||
|
||||
# Sundays weekly closed, single holiday on Mon 4 May 2026.
|
||||
$schema->resultset('RepeatableHoliday')->create(
|
||||
{
|
||||
branchcode => $branch,
|
||||
weekday => 0,
|
||||
description => q{Sunday},
|
||||
title => q{Sunday},
|
||||
}
|
||||
);
|
||||
$schema->resultset('SpecialHoliday')->create(
|
||||
{
|
||||
branchcode => $branch,
|
||||
day => 4,
|
||||
month => 5,
|
||||
year => 2026,
|
||||
isexception => 0,
|
||||
title => q{May Bank Holiday},
|
||||
description => q{May Bank Holiday},
|
||||
}
|
||||
);
|
||||
Koha::Caches->get_instance->clear_from_cache( $branch . '_holidays' );
|
||||
|
||||
my %expected = (
|
||||
'2026-04-27' => '2026-05-05', # Mon trapped, skip Sun + BH -> Tue
|
||||
'2026-04-28' => '2026-05-06', # Tue trapped, skip Sun + BH -> Wed
|
||||
'2026-04-29' => '2026-05-07', # Wed trapped -> Thu
|
||||
'2026-04-30' => '2026-05-08', # Thu trapped -> Fri
|
||||
'2026-05-01' => '2026-05-09', # Fri trapped -> Sat
|
||||
);
|
||||
|
||||
for my $waitingdate ( sort keys %expected ) {
|
||||
my $h = $builder->build_object(
|
||||
{
|
||||
class => 'Koha::Holds',
|
||||
value => {
|
||||
branchcode => $branch,
|
||||
found => undef,
|
||||
waitingdate => undef,
|
||||
expirationdate => undef,
|
||||
patron_expiration_date => undef,
|
||||
}
|
||||
}
|
||||
);
|
||||
$h->waitingdate($waitingdate)->store;
|
||||
$h->set_waiting;
|
||||
is(
|
||||
$h->expirationdate, $expected{$waitingdate},
|
||||
"waitingdate=$waitingdate => expirationdate=$expected{$waitingdate} (no week-jump)"
|
||||
);
|
||||
}
|
||||
|
||||
Koha::Caches->get_instance->clear_from_cache( $branch . '_holidays' );
|
||||
};
|
||||
|
||||
$schema->storage->txn_rollback();
|
||||
};
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue