From 893a69c64294347c443676c8fb091b565fea819c 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 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. --- .../Reserves/CancelExpiredReserves.t | 333 +++++++++++++++++- 1 file changed, 332 insertions(+), 1 deletion(-) diff --git a/t/db_dependent/Reserves/CancelExpiredReserves.t b/t/db_dependent/Reserves/CancelExpiredReserves.t index 2cc84788eda..e9a2de54a91 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; @@ -254,4 +255,334 @@ 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 => 6; + + # This test demonstrates the bug where CancelExpiredReserves checks if TODAY is a holiday + # instead of implementing proper grace period logic. + # + # The correct behavior should be: + # - Don't run cleanup when script runs on holidays (regardless of when holds expired) + # - Cancel expired holds (including those that expired on holidays) when script runs on business days + # - This provides a grace period for holds that expired on holidays while ensuring cleanup happens + + # Clear any existing holidays from previous tests + Koha::Caches->get_instance()->flush_all(); + + my $builder = t::lib::TestBuilder->new(); + + t::lib::Mocks::mock_preference( 'ExpireReservesOnHolidays', 0 ); + t::lib::Mocks::mock_preference( 'ExpireReservesMaxPickUpDelay', 1 ); + + # Create a library for our tests + my $library = $builder->build( { source => 'Branch' } ); + my $branchcode = $library->{branchcode}; + + # Scenario 1: Hold expired on Monday (non-holiday), script runs on Tuesday (holiday) + # The hold should NOT be cancelled because we don't run cleanup on holidays + + my $monday = dt_from_string('2024-01-15'); # Monday + my $tuesday = dt_from_string('2024-01-16'); # Tuesday + + # Create a hold that expired on Monday + my $hold_expired_on_monday = $builder->build( + { + source => 'Reserve', + value => { + branchcode => $branchcode, + patron_expiration_date => $monday->ymd, + expirationdate => $monday->ymd, + cancellationdate => undef, + priority => 0, + found => 'W', + }, + } + ); + + # Make Tuesday a holiday + my $tuesday_holiday = $builder->build( + { + source => 'SpecialHoliday', + value => { + branchcode => $branchcode, + day => $tuesday->day, + month => $tuesday->month, + year => $tuesday->year, + title => 'Tuesday Holiday', + isexception => 0 + }, + } + ); + + # Clear cache so holiday is recognized + Koha::Caches->get_instance()->flush_all(); + + # Set fake time to Tuesday (the holiday) + Time::Fake->offset( $tuesday->epoch ); + + # The hold should NOT be cancelled because we don't run cleanup on holidays + # (even though it expired on a non-holiday) + CancelExpiredReserves(); + my $hold_check = Koha::Holds->find( $hold_expired_on_monday->{reserve_id} ); + ok( $hold_check, 'Hold should not be cancelled when script runs on holiday (regardless of when it expired)' ); + + # Reset time + Time::Fake->reset; + + # Scenario 2: Hold expired on Wednesday (holiday), script runs on Thursday (non-holiday) + # The hold SHOULD be cancelled because grace period has ended (running on business day) + + my $wednesday = dt_from_string('2024-01-17'); # Wednesday + my $thursday = dt_from_string('2024-01-18'); # Thursday + + # Create a hold that expired on Wednesday + my $hold_expired_on_wednesday = $builder->build( + { + source => 'Reserve', + value => { + branchcode => $branchcode, + patron_expiration_date => $wednesday->ymd, + expirationdate => $wednesday->ymd, + cancellationdate => undef, + priority => 0, + found => 'W', + }, + } + ); + + # Make Wednesday a holiday + my $wednesday_holiday = $builder->build( + { + source => 'SpecialHoliday', + value => { + branchcode => $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 Thursday (non-holiday) + Time::Fake->offset( $thursday->epoch ); + + # The hold SHOULD be cancelled because we're running on a business day + # The hold SHOULD be cancelled because it expired in the past (not today) + # Even though it expired on a holiday, we're running on a business day + CancelExpiredReserves(); + $hold_check = Koha::Holds->find( $hold_expired_on_wednesday->{reserve_id} ); + is( + $hold_check, undef, + 'Hold that expired on holiday in the past should be cancelled when script runs on business day' + ); + + # Reset time + Time::Fake->reset; + + # Scenario 3: Multiple holds expired on different days, script runs on holiday + # Only holds that expired on non-holidays should be cancelled + + my $friday = dt_from_string('2024-01-19'); # Friday (non-holiday) + my $saturday = dt_from_string('2024-01-20'); # Saturday (holiday) + my $sunday = dt_from_string('2024-01-21'); # Sunday (script runs today, also holiday) + + # Hold that expired on Friday (non-holiday) + my $hold_expired_friday = $builder->build( + { + source => 'Reserve', + value => { + branchcode => $branchcode, + patron_expiration_date => $friday->ymd, + expirationdate => $friday->ymd, + cancellationdate => undef, + priority => 0, + found => 'W', + }, + } + ); + + # Hold that expired on Saturday (holiday) + my $hold_expired_saturday = $builder->build( + { + source => 'Reserve', + value => { + branchcode => $branchcode, + patron_expiration_date => $saturday->ymd, + expirationdate => $saturday->ymd, + cancellationdate => undef, + priority => 0, + found => 'W', + }, + } + ); + + # Make Saturday and Sunday holidays + my $saturday_holiday = $builder->build( + { + source => 'SpecialHoliday', + value => { + branchcode => $branchcode, + day => $saturday->day, + month => $saturday->month, + year => $saturday->year, + title => 'Saturday Holiday', + isexception => 0 + }, + } + ); + + my $sunday_holiday = $builder->build( + { + source => 'SpecialHoliday', + value => { + branchcode => $branchcode, + day => $sunday->day, + month => $sunday->month, + year => $sunday->year, + title => 'Sunday Holiday', + isexception => 0 + }, + } + ); + + # Clear cache so holidays are recognized + Koha::Caches->get_instance()->flush_all(); + + # Set fake time to Sunday (holiday) + Time::Fake->offset( $sunday->epoch ); + + CancelExpiredReserves(); + + # Hold that expired on Friday (non-holiday) should NOT be cancelled because script runs on holiday + my $friday_hold_check = Koha::Holds->find( $hold_expired_friday->{reserve_id} ); + ok( + $friday_hold_check, + 'Hold should not be cancelled when script runs on Sunday (holiday), regardless of when it expired' + ); + + # Hold that expired on Saturday (holiday) should NOT be cancelled + my $saturday_hold_check = Koha::Holds->find( $hold_expired_saturday->{reserve_id} ); + ok( + $saturday_hold_check, + 'Hold that expired on Saturday (holiday) should not be cancelled when script runs on Sunday (holiday)' + ); + + # Reset time + Time::Fake->reset; + + # Scenario 4: Demonstrate grace period - hold expired on holiday, script runs on business day + # This shows that holds that expired on holidays DO get cancelled when script runs on business days + + my $monday_holiday = dt_from_string('2024-01-22'); # Monday (holiday) + my $tuesday_business = dt_from_string('2024-01-23'); # Tuesday (business day) + + # Create a hold that expired on Monday (holiday) + my $hold_expired_monday_holiday = $builder->build( + { + source => 'Reserve', + value => { + branchcode => $branchcode, + patron_expiration_date => $monday_holiday->ymd, + expirationdate => $monday_holiday->ymd, + cancellationdate => undef, + priority => 0, + found => 'W', + }, + } + ); + + # Make Monday a holiday + my $monday_holiday_record = $builder->build( + { + source => 'SpecialHoliday', + value => { + branchcode => $branchcode, + day => $monday_holiday->day, + month => $monday_holiday->month, + year => $monday_holiday->year, + title => 'Monday Holiday', + isexception => 0 + }, + } + ); + + # Clear cache so holiday is recognized + Koha::Caches->get_instance()->flush_all(); + + # Set fake time to Tuesday (business day) + Time::Fake->offset( $tuesday_business->epoch ); + + # The hold SHOULD be cancelled because it expired in the past (not today) + # Even though it expired on a holiday, we're running on a business day + CancelExpiredReserves(); + my $monday_hold_check = Koha::Holds->find( $hold_expired_monday_holiday->{reserve_id} ); + is( + $monday_hold_check, undef, + 'Hold that expired on Monday (holiday) should be cancelled when script runs on Tuesday (business day)' + ); + + # Reset time + Time::Fake->reset; + + # Scenario 5: CRITICAL BUG - Hold expired on holiday LONG AGO should be cancelled + # This exposes the bug where holds that expired on holidays are never cancelled + + my $christmas_2023 = dt_from_string('2023-12-25'); # Christmas 2023 (holiday) + my $august_2024 = dt_from_string('2024-08-15'); # August 2024 (business day) + + # Create a hold that expired on Christmas 2023 (8 months ago!) + my $hold_expired_long_ago = $builder->build( + { + source => 'Reserve', + value => { + branchcode => $branchcode, + patron_expiration_date => $christmas_2023->ymd, + expirationdate => $christmas_2023->ymd, + cancellationdate => undef, + priority => 0, + found => 'W', + }, + } + ); + + # Make Christmas 2023 a holiday + my $christmas_holiday = $builder->build( + { + source => 'SpecialHoliday', + value => { + branchcode => $branchcode, + day => $christmas_2023->day, + month => $christmas_2023->month, + year => $christmas_2023->year, + title => 'Christmas', + isexception => 0 + }, + } + ); + + # Clear cache so holiday is recognized + Koha::Caches->get_instance()->flush_all(); + + # Set fake time to August 2024 (business day, 8 months later) + Time::Fake->offset( $august_2024->epoch ); + + # This hold SHOULD be cancelled because it expired 8 months ago + # Even though it expired on a holiday, it should not remain active forever + CancelExpiredReserves(); + my $old_hold_check = Koha::Holds->find( $hold_expired_long_ago->{reserve_id} ); + is( + $old_hold_check, undef, + 'Hold that expired on holiday 8 months ago should be cancelled (not remain active forever)' + ); + + # Reset time + Time::Fake->reset; +}; + $schema->storage->txn_rollback; -- 2.50.1