[Bug 40636] New: C4::Reserves::CancelExpiredReserves behavior depends on date it is run
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Bug ID: 40636 Summary: C4::Reserves::CancelExpiredReserves behavior depends on date it is run Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Status|NEW |ASSIGNED CC| |katrin.fischer@bsz-bw.de, | |lisette@bywatersolutions.co | |m, | |nick@bywatersolutions.com, | |tomascohen@gmail.com -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #1 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185346 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185346&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #2 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185347 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185347&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #3 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185348 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185348&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #4 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185349 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185349&action=edit Bug 40636: (follow-up) Cache the calendar object -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185346|0 |1 is obsolete| | Attachment #185347|0 |1 is obsolete| | Attachment #185348|0 |1 is obsolete| | Attachment #185349|0 |1 is obsolete| | --- Comment #5 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185350 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185350&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #6 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185351 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185351&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #7 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185352 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185352&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #8 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185353 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185353&action=edit Bug 40636: (follow-up) Cache the calendar object -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |emily.lamancusa@montgomeryc | |ountymd.gov -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185350|0 |1 is obsolete| | --- Comment #9 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 185411 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185411&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185351|0 |1 is obsolete| | --- Comment #10 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 185412 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185412&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185352|0 |1 is obsolete| | --- Comment #11 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 185413 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185413&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185353|0 |1 is obsolete| | --- Comment #12 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 185414 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185414&action=edit Bug 40636: (follow-up) Cache the calendar object Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |paul.derscheid@lmscloud.de --- Comment #13 from 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #14 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #16 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Could you please clarify why the regression tests without last two patches fail with you and not with us ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #17 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #18 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185411|0 |1 is obsolete| | Attachment #185412|0 |1 is obsolete| | Attachment #185413|0 |1 is obsolete| | Attachment #185414|0 |1 is obsolete| | --- Comment #19 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185907 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185907&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #20 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185908 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185908&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #21 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185909 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185909&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #22 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 185910 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185910&action=edit Bug 40636: (follow-up) Cache the calendar object Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #23 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC|katrin.fischer@bsz-bw.de | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #24 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- I re-did the tests. Any comments about the implemented code Marcel? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #25 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185907|0 |1 is obsolete| | --- Comment #26 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 186133 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186133&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185908|0 |1 is obsolete| | --- Comment #27 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 186134 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186134&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185909|0 |1 is obsolete| | --- Comment #28 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 186135 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186135&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185910|0 |1 is obsolete| | --- Comment #29 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 186136 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186136&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |25.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #30 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to main for 25.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|25.11.00 |25.11.00,25.05.04 released in| | Status|Pushed to main |Pushed to stable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 --- Comment #31 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Nice work everyone! Pushed to 25.05.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Status|Pushed to stable |Needs documenting --- Comment #32 from Fridolin Somers <fridolin.somers@biblibre.com> --- This is a complex behavior change, I prefer not impact 24.11.x LTS for stability -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40636 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Status|Needs documenting |RESOLVED Resolution|--- |FIXED --- Comment #33 from David Nind <david@davidnind.com> --- Bug fix, no changes to the manual required. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org