Bug 40636

Summary: C4::Reserves::CancelExpiredReserves behavior depends on date it is run
Product: Koha Reporter: Tomás Cohen Arazi (tcohen) <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi (tcohen) <tomascohen>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: emily.lamancusa, katrin.fischer, lisette, nick, paul.derscheid, tomascohen
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 40636: Add `Koha::Calendar->has_business_days_between()` method
Bug 40636: Regression tests
Bug 40636: Fix holiday logic in CancelExpiredReserves with comprehensive edge case handling
Bug 40636: (follow-up) Cache the calendar object
Bug 40636: Add `Koha::Calendar->has_business_days_between()` method
Bug 40636: Regression tests
Bug 40636: Fix holiday logic in CancelExpiredReserves with comprehensive edge case handling
Bug 40636: (follow-up) Cache the calendar object
Bug 40636: Add `Koha::Calendar->has_business_days_between()` method
Bug 40636: Regression tests
Bug 40636: Fix holiday logic in CancelExpiredReserves with comprehensive edge case handling
Bug 40636: (follow-up) Cache the calendar object

Description Tomás Cohen Arazi (tcohen) 2025-08-11 20:08:14 UTC
Looking at this method, I noticed (a few things, of which the more relevant is) calculation depends on the date the method is called. For example:

If a hold expired on Monday but the script runs on Tuesday (which happens to be a holiday), the hold that should have been canceled on Monday will be skipped entirely, even though Monday wasn't a holiday.

The logic should be checking if the expiration date itself was a holiday, not whether today is a holiday
Comment 1 Tomás Cohen Arazi (tcohen) 2025-08-12 17:38:31 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi (tcohen) 2025-08-12 17:38:34 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi (tcohen) 2025-08-12 17:38:37 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi (tcohen) 2025-08-12 17:48:06 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi (tcohen) 2025-08-12 18:08:34 UTC
Created attachment 185350 [details] [review]
Bug 40636: Add `Koha::Calendar->has_business_days_between()` method

This patch adds a new method to Koha::Calendar that returns a boolean indicating
whether there are any business days between two given dates (exclusive).

Implementation details:
* Returns 1 if any business days exist between start and end dates
* Returns 0 for consecutive days, same dates, or only holidays between
* Uses exclusive range (does not include start or end dates)
* Handles date comparison and timezone normalization
* Efficient early return when first business day is found

This is designed specifically for use cases like detecting missed
business days due to server downtime or scheduling issues.

To test:
1. Apply patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Calendar.t
=> SUCCESS: All tests pass!
3. Sign off :-D
Comment 6 Tomás Cohen Arazi (tcohen) 2025-08-12 18:08:37 UTC
Created attachment 185351 [details] [review]
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.
Comment 7 Tomás Cohen Arazi (tcohen) 2025-08-12 18:08:40 UTC
Created attachment 185352 [details] [review]
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

To test:
1. Apply the tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Reserves/CancelExpiredReserves.t
=> FAIL: Tests fail! The logic in the method is wrong!
3. Apply this patch
4. Repeat 2
=> SUCCESS: All tests pass, demonstrating correct behavior in all scenarios
5. Sign off :-D
Comment 8 Tomás Cohen Arazi (tcohen) 2025-08-12 18:08:43 UTC
Created attachment 185353 [details] [review]
Bug 40636: (follow-up) Cache the calendar object
Comment 9 Paul Derscheid 2025-08-14 17:39:29 UTC
Created attachment 185411 [details] [review]
Bug 40636: Add `Koha::Calendar->has_business_days_between()` method

This patch adds a new method to Koha::Calendar that returns a boolean indicating
whether there are any business days between two given dates (exclusive).

Implementation details:
* Returns 1 if any business days exist between start and end dates
* Returns 0 for consecutive days, same dates, or only holidays between
* Uses exclusive range (does not include start or end dates)
* Handles date comparison and timezone normalization
* Efficient early return when first business day is found

This is designed specifically for use cases like detecting missed
business days due to server downtime or scheduling issues.

To test:
1. Apply patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Calendar.t
=> SUCCESS: All tests pass!
3. Sign off :-D

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 10 Paul Derscheid 2025-08-14 17:39:31 UTC
Created attachment 185412 [details] [review]
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>
Comment 11 Paul Derscheid 2025-08-14 17:39:34 UTC
Created attachment 185413 [details] [review]
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

To test:
1. Apply the tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Reserves/CancelExpiredReserves.t
=> FAIL: Tests fail! The logic in the method is wrong!
3. Apply this patch
4. Repeat 2
=> SUCCESS: All tests pass, demonstrating correct behavior in all scenarios
5. Sign off :-D

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 12 Paul Derscheid 2025-08-14 17:39:36 UTC
Created attachment 185414 [details] [review]
Bug 40636: (follow-up) Cache the calendar object

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 13 Paul Derscheid 2025-08-14 17:41:01 UTC
The good news is: all tests pass.

But: I couldn't get the tests to fail at all. What's the correct order to apply and run here, Tomás?
Comment 14 Tomás Cohen Arazi (tcohen) 2025-08-14 18:35:05 UTC
(In reply to Paul Derscheid from comment #13)
> The good news is: all tests pass.
> 
> But: I couldn't get the tests to fail at all. What's the correct order to
> apply and run here, Tomás?

Let me check. They failed consistently to me with the first two commits.