Description
Tomás Cohen Arazi (tcohen)
2025-08-11 20:08:14 UTC
Created attachment 185346 [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$ cd /kohadevbox/koha k$ prove t/db_dependent/Koha/Calendar.t => SUCCESS: All tests pass demonstrating correct behavior 3. Sign off :-D Created attachment 185347 [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. Created attachment 185348 [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 * 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 Created attachment 185349 [details] [review] Bug 40636: (follow-up) Cache the calendar object 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 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. 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 Created attachment 185353 [details] [review] Bug 40636: (follow-up) Cache the calendar object 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> 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> 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> Created attachment 185414 [details] [review] Bug 40636: (follow-up) Cache the calendar object Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> 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? (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. |