Bugzilla – Attachment 185412 Details for
Bug 40636
C4::Reserves::CancelExpiredReserves behavior depends on date it is run
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 40636: Regression tests
Bug-40636-Regression-tests.patch (text/plain), 13.98 KB, created by
Paul Derscheid
on 2025-08-14 17:39:31 UTC
(
hide
)
Description:
Bug 40636: Regression tests
Filename:
MIME Type:
Creator:
Paul Derscheid
Created:
2025-08-14 17:39:31 UTC
Size:
13.98 KB
patch
obsolete
>From 0771ebdd5ccd15a8f064d0b86e4715f0ffdc5c82 Mon Sep 17 00:00:00 2001 >From: Tomas Cohen Arazi <tomascohen@theke.io> >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. > >Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> >--- > .../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 2cc84788ed..e9a2de54a9 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.39.5
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 40636
:
185346
|
185347
|
185348
|
185349
|
185350
|
185351
|
185352
|
185353
|
185411
| 185412 |
185413
|
185414