|
Description
Michael Hafen
2024-09-19 18:05:27 UTC
I just read the description for the RenewalOnHoldOverride sys pref ("Allow staff to renew items that are on hold by manually specifying a due date."), and it seems this is more expected behavior than bug.
Still I think an improvement is needed. First off, the date due field in that dialog didn't seem required to me.
I see three options: Change the sys pref description and the code, calculate the new due date and use that as a default, or just change the style of the due date field to indicate it is required.
I'm leaning toward changing the sys pref and the code, but I'm open to other opinions.
Created attachment 171790 [details] [review] Bug 37966: When Renewing a book on hold, don't require the due date When renewing a book on hold with the RenewalOnHoldOverride sys pref set to "Allow", if the due date is not specified then it becomes "now". By checking the input is set rather than just defined the due date is set as expected based on circulation rules. Also change the description of the RenewalOnHoldOverride sys pref as the due date would no longer be required to be entered with this change. Test plan: 1. make sure RenewalOnHoldOverride is set to 'Allow' 2. check out a book, then place a title-level hold on it 3. renew the book, see the hold override dialog 4. leave the due date empty and hit the override button 5. observe the new due date is "now" 6. apply patch and restart plack 7. renew the book again, overriding the hold, and see the expected due date as per circulation rules. It seems to work fine as step 7 indicates without the patch? On my dev install of the main branch on my workstation, which I pulled Sept. 30th, it behaves as the plan outlines, with the new due date on renewal, after overriding the hold, being the date and time that the book was renewed rather than the expected 3 weeks later (in my case) when the new due date field in the hold override dialog is left empty. I don't know why it didn't behave as expected for you. Patch does not apply on sandbox : CONFLICT (content): Merge conflict in C4/Circulation.pm Created attachment 179490 [details] [review] When Renewing a book on hold, don't require the due date [Rebased 2025-03-19] When renewing a book on hold with the RenewalOnHoldOverride sys pref set to "Allow", if the due date is not specified then it becomes "now". By checking the input is set rather than just defined the due date is set as expected based on circulation rules. Also change the description of the RenewalOnHoldOverride sys pref as the due date would no longer be required to be entered with this change. Test plan: 1. make sure RenewalOnHoldOverride is set to 'Allow' 2. check out a book, then place a title-level hold on it 3. renew the book, see the hold override dialog 4. leave the due date empty and hit the override button 5. observe the new due date is "now" 6. apply patch and restart plack 7. renew the book again, overriding the hold, and see the expected due date as per circulation rules. Could not reproduce the issue here. The date correctly updates for me on step 4 Maybe I didn't describe step 4 well enough, or maybe there's something in my preferences, but I just tried this on master and the due date went from 4/3/2025 to 3/20/2025 (today) when using the Renew page after overriding the hold when the 'Override due date' field was left empty. Looking through the preferences, the only one I see, besides the AllowRenewal*Override ones that might affect this is 'RenewalPeriodBase' which I have set to 'the current date'. Created attachment 180478 [details] [review] Bug 37966: When Renewing a book on hold, don't require the due date When renewing a book on hold with the RenewalOnHoldOverride sys pref set to "Allow", if the due date is not specified then it becomes "now". By checking the input is set rather than just defined the due date is set as expected based on circulation rules. Also change the description of the RenewalOnHoldOverride sys pref as the due date would no longer be required to be entered with this change. Test plan: 1. make sure RenewalOnHoldOverride is set to 'Allow' 2. check out a book, then place a title-level hold on it 3. renew the book, see the hold override dialog 4. leave the due date empty and hit the override button 5. observe the new due date is "now" 6. apply patch and restart plack 7. renew the book again, overriding the hold, and see the expected due date as per circulation rules. Signed-off-by: Marion Durand <marion.durand@biblibre.com> Any change to AddRenewal should be covered with a test. Created attachment 188524 [details] [review] Bug 37966: Add test coverage for AddRenewal with empty due date Created attachment 190590 [details] [review] Bug 37966: When Renewing a book on hold, don't require the due date When renewing a book on hold with the RenewalOnHoldOverride sys pref set to "Allow", if the due date is not specified then it becomes "now". By checking the input is set rather than just defined the due date is set as expected based on circulation rules. Also change the description of the RenewalOnHoldOverride sys pref as the due date would no longer be required to be entered with this change. Test plan: 1. make sure RenewalOnHoldOverride is set to 'Allow' 2. check out a book, then place a title-level hold on it 3. renew the book, see the hold override dialog 4. leave the due date empty and hit the override button 5. observe the new due date is "now" 6. apply patch and restart plack 7. renew the book again, overriding the hold, and see the expected due date as per circulation rules. Signed-off-by: Marion Durand <marion.durand@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 190591 [details] [review] Bug 37966: Add test coverage for AddRenewal with empty due date Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> I've decided I'd like to make the fix here more explicit and also make it comprehensively handle renewing via circ/circulation.pl as well as circ/renew.pl Follow-up inbound Created attachment 191256 [details] [review] Bug 37966: When Renewing a book on hold, don't require the due date When renewing a book on hold with the RenewalOnHoldOverride sys pref set to "Allow", if the due date is not specified then it becomes "now". By checking the input is set rather than just defined the due date is set as expected based on circulation rules. Also change the description of the RenewalOnHoldOverride sys pref as the due date would no longer be required to be entered with this change. Test plan: 1. make sure RenewalOnHoldOverride is set to 'Allow' 2. check out a book, then place a title-level hold on it 3. renew the book, see the hold override dialog 4. leave the due date empty and hit the override button 5. observe the new due date is "now" 6. apply patch and restart plack 7. renew the book again, overriding the hold, and see the expected due date as per circulation rules. Signed-off-by: Marion Durand <marion.durand@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 191257 [details] [review] Bug 37966: Add test coverage for AddRenewal with empty due date Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 191258 [details] [review] Bug 37966: (follow-up) Make empty string handling more explicit in Circulation.pm This commit improves upon the original fix by making the intent more explicit when handling empty string date parameters in AddRenewal and AddIssue functions. WHY THIS IS BETTER: The original fix (changing `defined $datedue` to just `$datedue`) works correctly but relies on Perl's implicit truthy/falsy semantics: if ( $datedue && ref $datedue ne 'DateTime' ) This is subtle because: - Empty string "" is falsy, so parsing is skipped ✓ - But it's not immediately obvious that empty strings are being handled specially - The code doesn't explicitly show its intent THIS APPROACH: if ( defined $datedue && $datedue ne '' && ref $datedue ne 'DateTime' ) Benefits: 1. EXPLICIT: Clearly states "only parse if defined AND not empty AND not already a DateTime" 2. SELF-DOCUMENTING: Intent is obvious to future developers 3. NO RELIANCE on truthy/falsy semantics 4. CONSISTENT: Same pattern applied across all date parameter handling COMPREHENSIVE CHANGES: 1. AddRenewal (C4/Circulation.pm:3449) - Made datedue empty string check explicit 2. AddIssue (C4/Circulation.pm:1611) - Made datedue empty string check explicit - Also updated issuedate handling (line 1616) to explicitly handle empty strings 3. Test Coverage (t/db_dependent/Circulation/issue.t) - Added test for AddIssue with empty string datedue - Added test for AddIssue with empty string issuedate - Ensures both functions properly calculate dates from circulation rules instead of using "now" The existing test for AddRenewal already covered empty string datedue (added in the original Bug 37966 commit). These new tests ensure the same correct behavior for AddIssue. Created attachment 191259 [details] [review] Bug 37966: Fix renewal on hold override in circulation.pl This extends the fix from Bug 37966 to circ/circulation.pl. The original bug fix addressed circ/renew.pl, where renewing a book on hold with an empty due date would result in the due date being set to "now" (today). The same issue occurs in circ/circulation.pl when: 1. Attempting to check out an item that is already checked out to the same patron (which triggers a renewal) 2. The item has a hold on it, triggering the hold override dialog 3. The librarian overrides the hold without specifying a due date In this scenario, the renewal would incorrectly set the due date to today instead of calculating it based on circulation rules. CHANGES: 1. circ/circulation.pl (lines 550-558) - Added logic to handle the `renewonholdduedate` parameter when both RENEW_ISSUE and RESERVED conditions are present - Follows the same pattern as circ/renew.pl - If `renewonholdduedate` is empty, passes empty string to AddIssue/ AddRenewal, allowing proper date calculation 2. koha-tmpl/.../circulation.tt (lines 351-356) - Added "Renewal due date" input field in the hold override dialog - Only appears when both RESERVED and RENEW_ISSUE are true - Optional field - if left empty, due date is calculated per circulation rules TEST PLAN: 1. Enable the RenewalOnHoldOverride system preference (set to "Allow") 2. Set up a patron, item, and circulation rules: - Create a patron (or use existing) - Create an item (or use existing) - Ensure circulation rules have a renewal period (e.g., 14 days) 3. Create a hold scenario: - Check out the item to the patron - Place a title-level hold on the same item (you may need a second patron or use the same patron depending on your holds rules) 4. Test renewal via circulation.pl: - Go to Circulation (Home > Circulation) - Scan the patron's barcode - Scan the item's barcode (the one that's already checked out with a hold) - Observe the hold override dialog appears 5. Test WITHOUT this patch: - Leave the "Renewal due date" field empty - Click "Yes, renew" - BUG: The due date is set to today (same as check-in date) 6. Test WITH this patch: - Leave the "Renewal due date" field empty - Click "Yes, renew" - SUCCESS: The due date is calculated based on circulation rules (e.g., 14 days from now if that's your renewal period) 7. Test optional due date override: - Repeat the renewal process - This time, enter a specific date in "Renewal due date" field - Click "Yes, renew" - SUCCESS: The due date is set to your specified date 8. Test SpecifyDueDate preference interaction: - If SpecifyDueDate is enabled and a due date is set via that interface, it should take precedence over renewonholdduedate TECHNICAL NOTES: The fix ensures that when `renewonholdduedate` is empty (empty string), it's passed through to AddIssue which then calls AddRenewal. The improved empty string handling in C4::Circulation (from the follow-up commit) ensures empty strings are not parsed as "now" but instead trigger proper date calculation based on circulation rules. This matches the behavior fixed in circ/renew.pl and provides a consistent user experience across both circulation interfaces. |