Bugzilla – Attachment 185348 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: Fix holiday logic in CancelExpiredReserves with comprehensive edge case handling
Bug-40636-Fix-holiday-logic-in-CancelExpiredReserv.patch (text/plain), 6.43 KB, created by
Tomás Cohen Arazi (tcohen)
on 2025-08-12 17:38:37 UTC
(
hide
)
Description:
Bug 40636: Fix holiday logic in CancelExpiredReserves with comprehensive edge case handling
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2025-08-12 17:38:37 UTC
Size:
6.43 KB
patch
obsolete
>From 88309cc8e7d0ff3e2e451808cbdbd9906c80307b Mon Sep 17 00:00:00 2001 >From: Tomas Cohen Arazi <tomascohen@theke.io> >Date: Tue, 12 Aug 2025 11:22:26 -0300 >Subject: [PATCH] Bug 40636: Fix holiday logic in CancelExpiredReserves with > comprehensive edge case handling > >CancelExpiredReserves had incorrect holiday logic that checked if **today** is a holiday instead of checking if the **hold's expiration date** was a holiday. > >This caused: >* Holds that expired on non-holidays to remain active if cleanup script ran on holidays >* Holds that expired on holidays to be cancelled if cleanup script ran on non-holidays > >The simple fix exposed additional edge cases around server downtime that could leave holds active indefinitely. > >Changes: >* Implements comprehensive holiday logic when `ExpireReservesOnHolidays = 0` >* Same-day protection: Don't cancel holds that expire TODAY if today is a holiday >* Historical cleanup: Cancel holds that expired in the past when running on business days >* Retrospective cancellation: Handle server downtime by checking for missed business days >* Comprehensive test suite covering all scenarios including long-term expired holds >* Detailed POD documentation with use cases and examples > >To test: >1. Apply patch >2. Run: > $ ktd --shell > k$ cd /kohadevbox/koha > k$ prove t/db_dependent/Reserves/CancelExpiredReserves.t >=> SUCCESS: All tests pass, demonstrating correct behavior in all scenarios >3. Sign off :-D >--- > C4/Reserves.pm | 108 ++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 106 insertions(+), 2 deletions(-) > >diff --git a/C4/Reserves.pm b/C4/Reserves.pm >index 1ffddd5b9f3..8d467d7d4a6 100644 >--- a/C4/Reserves.pm >+++ b/C4/Reserves.pm >@@ -985,10 +985,70 @@ sub CheckReserves { > > =head2 CancelExpiredReserves > >- CancelExpiredReserves(); >+ CancelExpiredReserves(); >+ CancelExpiredReserves($cancellation_reason); > > Cancels all reserves with an expiration date from before today. > >+When the ExpireReservesOnHolidays system preference is disabled (0), implements >+sophisticated holiday logic to handle various edge cases: >+ >+=head3 Use Cases and Logic >+ >+=over 4 >+ >+=item B<Case 1: Hold expires today on a holiday> >+ >+If a hold expires today and today is a holiday, the hold is NOT cancelled. >+This respects the library's policy of not processing cancellations on holidays. >+ >+Example: Hold expires on Christmas Day, script runs on Christmas Day => Hold preserved >+ >+=item B<Case 2: Hold expired in the past, script runs on business day> >+ >+If a hold expired in the past (regardless of whether the expiration date was a holiday), >+and today is a business day, the hold IS cancelled. >+ >+Example: Hold expired on Christmas 2023, script runs in August 2024 => Hold cancelled >+ >+=item B<Case 3: Hold expired in the past, script runs on holiday, no business days missed> >+ >+If a hold expired recently and today is a holiday, but no business days have passed >+since expiration, the hold is NOT cancelled. >+ >+Example: Hold expires Friday, script runs Saturday (holiday) => Hold preserved >+ >+=item B<Case 4: Hold expired in the past, script runs on holiday, business days were missed> >+ >+If a hold expired in the past and today is a holiday, but business days have passed >+since expiration (due to server downtime, cron failures, etc.), the hold IS cancelled >+retrospectively to prevent holds from remaining active indefinitely. >+ >+Example: Hold expires Monday, server down Tue-Wed (business days), script runs Thursday (holiday) => Hold cancelled >+ >+=item B<Case 5: ExpireReservesOnHolidays enabled> >+ >+When ExpireReservesOnHolidays is enabled (1), all expired holds are cancelled >+regardless of holidays, maintaining the original simple behavior. >+ >+=back >+ >+=head3 Parameters >+ >+=over 4 >+ >+=item $cancellation_reason (optional) >+ >+Optional cancellation reason to be recorded in the hold's cancellation_reason field. >+ >+=back >+ >+This logic ensures that: >+- Library holiday policies are respected >+- Holds don't remain active indefinitely due to technical issues >+- The system is robust against server downtime and cron failures >+- Behavior is predictable and well-defined for all scenarios >+ > =cut > > sub CancelExpiredReserves { >@@ -1013,7 +1073,51 @@ sub CancelExpiredReserves { > while ( my $hold = $holds->next ) { > my $calendar = Koha::Calendar->new( branchcode => $hold->branchcode ); > >- next if !$cancel_on_holidays && $calendar->is_holiday($today); >+ # Get the actual expiration date for this hold >+ my $expiration_date = $hold->expirationdate || $hold->patron_expiration_date; >+ >+ # When ExpireReservesOnHolidays is disabled, implement proper holiday logic: >+ if ( !$cancel_on_holidays && $expiration_date ) { >+ my $expiration_dt = dt_from_string($expiration_date); >+ >+ # Check if hold expired today >+ my $expired_today = $expiration_dt->ymd eq $today->ymd; >+ >+ if ($expired_today) { >+ >+ # Rule 1: Don't cancel holds that expired TODAY if today is a holiday >+ next if $calendar->is_holiday($today); >+ } else { >+ >+ # Rule 2: For holds that expired in the past, check for missed business days >+ if ( $calendar->is_holiday($today) ) { >+ >+ # If today is a holiday, check if business days were missed since expiration >+ my $days_since_expiration = $today->delta_days($expiration_dt)->in_units('days'); >+ >+ if ( $days_since_expiration > 0 ) { >+ >+ # Use Calendar's has_business_days_between for cleaner logic >+ my $business_days_missed = $calendar->has_business_days_between( $expiration_dt, $today ); >+ >+ # Rule 3: If business days were missed, cancel retrospectively even on holidays >+ if ( !$business_days_missed ) { >+ >+ # No business days were missed, skip cancellation >+ next; >+ } >+ >+ # If business days were missed, continue with cancellation >+ } else { >+ >+ # This shouldn't happen (expired_today would be true), but skip to be safe >+ next; >+ } >+ } >+ >+ # If today is a business day, cancel expired holds from the past >+ } >+ } > > my $cancel_params = {}; > $cancel_params->{cancellation_reason} = $cancellation_reason if defined($cancellation_reason); >-- >2.50.1
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