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
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.
(In reply to Tomás Cohen Arazi (tcohen) from comment #14) > (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. They also pass with me with the first two commits.
Could you please clarify why the regression tests without last two patches fail with you and not with us ?
(In reply to Marcel de Rooy from comment #16) > Could you please clarify why the regression tests without last two patches > fail with you and not with us ? Maybe I squashed too much before submitting. Let me go back and see where I messed it up.
(In reply to Marcel de Rooy from comment #16) > Could you please clarify why the regression tests without last two patches > fail with you and not with us ? Yeah, the behavior is sensible to the `ExpireReservesOnHolidays` syspref value and I missed mocking it correctly in the tests. That's why I was able to reproduce the issue locally and have the tests fail. I'll submit fixed tests now.
Created attachment 185907 [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 185908 [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 185909 [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 185910 [details] [review] Bug 40636: (follow-up) Cache the calendar object Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
(In reply to Tomás Cohen Arazi (tcohen) from comment #17) > (In reply to Marcel de Rooy from comment #16) > > Could you please clarify why the regression tests without last two patches > > fail with you and not with us ? > > Maybe I squashed too much before submitting. Let me go back and see where I > messed it up. I shouldn't squash tiny cosmetic changes at the end of the day right before submitting :-D
I re-did the tests. Any comments about the implemented code Marcel?
(In reply to Tomás Cohen Arazi (tcohen) from comment #24) > I re-did the tests. Any comments about the implemented code Marcel? This looks OK now (strange enough :) t/db_dependent/Reserves/CancelExpiredReserves.t (Wstat: 256 (exited 1) Tests: 6 Failed: 1) Failed test: 5 Non-zero exit status: 1
Created attachment 186133 [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> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 186134 [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> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 186135 [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> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 186136 [details] [review] Bug 40636: (follow-up) Cache the calendar object Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Nice work everyone! Pushed to main for 25.11
Nice work everyone! Pushed to 25.05.x