From e8c41df269a63424251f4293f030e6d88436d0d4 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Tue, 12 Aug 2025 11:13:54 -0300 Subject: [PATCH] Bug 40636: Regression tests Content-Type: text/plain; charset=utf-8 This patch adds regression tests that demonstrate the bug in CancelExpiredReserves where the holiday logic checks if today is a holiday instead of checking if the hold's expiration date was a holiday. The bug causes: 1. Holds that expired on non-holidays to remain active if the cleanup script runs on a holiday 2. Holds that expired on holidays to be cancelled if the cleanup script runs on a non-holiday Test scenarios added: - Hold expired on Monday (non-holiday), script runs on Tuesday (holiday) - Hold expired on Wednesday (holiday), script runs on Thursday (non-holiday) - Hold expired on Friday (non-holiday), script runs on Sunday (holiday) - Hold expired on Saturday (holiday), script runs on Sunday (holiday) These tests currently fail, demonstrating the bug exists. When the bug is fixed, all tests should pass. Signed-off-by: Paul Derscheid Signed-off-by: Marcel de Rooy --- .../Reserves/CancelExpiredReserves.t | 164 +++++++++++++++++- 1 file changed, 155 insertions(+), 9 deletions(-) diff --git a/t/db_dependent/Reserves/CancelExpiredReserves.t b/t/db_dependent/Reserves/CancelExpiredReserves.t index 2cc84788ed..00664d1854 100755 --- a/t/db_dependent/Reserves/CancelExpiredReserves.t +++ b/t/db_dependent/Reserves/CancelExpiredReserves.t @@ -1,9 +1,10 @@ #!/usr/bin/perl use Modern::Perl; -use Test::More tests => 5; +use Test::More tests => 6; use Test::NoWarnings; use Test::MockModule; +use Time::Fake; use t::lib::Mocks; use t::lib::TestBuilder; @@ -15,14 +16,14 @@ use Koha::DateUtils qw( dt_from_string ); use Koha::Holds; use Koha::Old::Holds; -my $schema = Koha::Database->new->schema; +my $builder = t::lib::TestBuilder->new(); +my $schema = Koha::Database->new->schema; + $schema->storage->txn_begin; subtest 'CancelExpiredReserves tests incl. holidays' => sub { plan tests => 4; - my $builder = t::lib::TestBuilder->new(); - t::lib::Mocks::mock_preference( 'ExpireReservesOnHolidays', 0 ); # Waiting holds could be cancelled only if ExpireReservesMaxPickUpDelay is set to "allow", see bug 19260 @@ -119,7 +120,6 @@ subtest 'Test handling of waiting reserves by CancelExpiredReserves' => sub { Koha::Holds->delete; - my $builder = t::lib::TestBuilder->new(); my $category = $builder->build( { source => 'Category' } ); my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; my $item = $builder->build_sample_item; @@ -184,8 +184,6 @@ subtest 'Test handling of waiting reserves by CancelExpiredReserves' => sub { subtest 'Test handling of in transit reserves by CancelExpiredReserves' => sub { plan tests => 2; - my $builder = t::lib::TestBuilder->new(); - t::lib::Mocks::mock_preference( 'ExpireReservesMaxPickUpDelay', 1 ); my $expdate = dt_from_string->add( days => -2 ); my $reserve = $builder->build( @@ -225,8 +223,6 @@ subtest 'Test handling of in transit reserves by CancelExpiredReserves' => sub { subtest 'Test handling of cancellation reason if passed' => sub { plan tests => 2; - my $builder = t::lib::TestBuilder->new(); - my $expdate = dt_from_string->add( days => -2 ); my $reserve = $builder->build( { @@ -254,4 +250,154 @@ subtest 'Test handling of cancellation reason if passed' => sub { is( $old_reserve->cancellation_reason, 'EXPIRED', "Hold cancellation_reason was set correctly" ); }; +subtest 'Holiday logic edge cases' => sub { + + plan tests => 2; + + subtest 'Hold expired on business day should be cancelled retrospectively when script runs on holiday' => sub { + plan tests => 1; + + $schema->storage->txn_begin; + + t::lib::Mocks::mock_preference( 'ExpireReservesOnHolidays', 0 ); + t::lib::Mocks::mock_preference( 'ExpireReservesMaxPickUpDelay', 1 ); + + # Clear any existing holidays from previous tests + Koha::Caches->get_instance()->flush_all(); + + # Create a library for our tests + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); + + # The bug scenario: Hold expired on Monday (business day), script runs on Wednesday (holiday) + # The hold SHOULD be cancelled because it expired on a business day and Tuesday was a business day + # that was missed, so we need retrospective cancellation even though today is a holiday + + my $monday = dt_from_string('2024-01-15'); # Monday (business day, hold expires) + my $wednesday = dt_from_string('2024-01-17'); # Wednesday (holiday, script runs now) + + # Create a hold that expired on Monday (business day) + my $hold_expired_monday = $builder->build_object( + { + class => 'Koha::Holds', + value => { + branchcode => $library->branchcode, + patron_expiration_date => $monday->ymd, + expirationdate => $monday->ymd, + cancellationdate => undef, + priority => 0, + found => 'W', + }, + } + ); + + # Make Wednesday a holiday (but Monday and Tuesday are business days) + my $wednesday_holiday = $builder->build( + { + source => 'SpecialHoliday', + value => { + branchcode => $library->branchcode, + day => $wednesday->day, + month => $wednesday->month, + year => $wednesday->year, + title => 'Wednesday Holiday', + isexception => 0 + }, + } + ); + + # Clear cache so holiday is recognized + Koha::Caches->get_instance()->flush_all(); + + # Set fake time to Wednesday (holiday) + Time::Fake->offset( $wednesday->epoch ); + + # The hold SHOULD be cancelled because: + # 1. It expired on Monday (business day) + # 2. Tuesday was a business day that was missed + # 3. Even though today (Wednesday) is a holiday, we need retrospective cancellation + CancelExpiredReserves(); + + my $hold_check = Koha::Holds->find( $hold_expired_monday->id ); + is( + $hold_check, undef, + 'Hold that expired on business day should be cancelled retrospectively even when script runs on holiday' + ); + + # Reset time + Time::Fake->reset; + + $schema->storage->txn_rollback; + }; + + subtest 'Hold expired on holiday should not be cancelled when script runs on same holiday' => sub { + plan tests => 1; + + $schema->storage->txn_begin; + + t::lib::Mocks::mock_preference( 'ExpireReservesOnHolidays', 0 ); + t::lib::Mocks::mock_preference( 'ExpireReservesMaxPickUpDelay', 1 ); + + # Clear any existing holidays from previous tests + Koha::Caches->get_instance()->flush_all(); + + # Create a library for our tests + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); + + # Control scenario: Hold expired on Monday (holiday), script runs on Monday (same holiday) + # The hold should NOT be cancelled because it expired today and today is a holiday + + my $monday = dt_from_string('2024-01-15'); # Monday (holiday, hold expires and script runs) + + # Create a hold that expired on Monday (holiday) + my $hold_expired_monday = $builder->build_object( + { + class => 'Koha::Holds', + value => { + branchcode => $library->branchcode, + patron_expiration_date => $monday->ymd, + expirationdate => $monday->ymd, + cancellationdate => undef, + priority => 0, + found => 'W', + }, + } + ); + + # Make Monday a holiday + my $monday_holiday = $builder->build( + { + source => 'SpecialHoliday', + value => { + branchcode => $library->branchcode, + day => $monday->day, + month => $monday->month, + year => $monday->year, + title => 'Monday Holiday', + isexception => 0 + }, + } + ); + + # Clear cache so holiday is recognized + Koha::Caches->get_instance()->flush_all(); + + # Set fake time to Monday (holiday) + Time::Fake->offset( $monday->epoch ); + + # The hold should NOT be cancelled because it expired today and today is a holiday + CancelExpiredReserves(); + + my $hold_check = Koha::Holds->find( $hold_expired_monday->id ); + ok( + $hold_check, + 'Hold that expired on holiday should not be cancelled when script runs on same holiday' + ); + + # Reset time + Time::Fake->reset; + + $schema->storage->txn_rollback; + }; +}; + $schema->storage->txn_rollback; -- 2.39.5