Bug 40636 - C4::Reserves::CancelExpiredReserves behavior depends on date it is run
Summary: C4::Reserves::CancelExpiredReserves behavior depends on date it is run
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-08-11 20:08 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-08-14 18:35 UTC (History)
6 users (show)

See Also:
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 (7.62 KB, patch)
2025-08-12 17:38 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40636: Regression tests (13.93 KB, patch)
2025-08-12 17:38 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40636: Fix holiday logic in CancelExpiredReserves with comprehensive edge case handling (6.43 KB, patch)
2025-08-12 17:38 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40636: (follow-up) Cache the calendar object (1.23 KB, patch)
2025-08-12 17:48 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40636: Add `Koha::Calendar->has_business_days_between()` method (7.57 KB, patch)
2025-08-12 18:08 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40636: Regression tests (13.93 KB, patch)
2025-08-12 18:08 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40636: Fix holiday logic in CancelExpiredReserves with comprehensive edge case handling (6.45 KB, patch)
2025-08-12 18:08 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40636: (follow-up) Cache the calendar object (1.23 KB, patch)
2025-08-12 18:08 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40636: Add `Koha::Calendar->has_business_days_between()` method (7.62 KB, patch)
2025-08-14 17:39 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 40636: Regression tests (13.98 KB, patch)
2025-08-14 17:39 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 40636: Fix holiday logic in CancelExpiredReserves with comprehensive edge case handling (6.50 KB, patch)
2025-08-14 17:39 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 40636: (follow-up) Cache the calendar object (1.28 KB, patch)
2025-08-14 17:39 UTC, Paul Derscheid
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.