Bug 37966

Summary: When overriding a hold to renew a book the due date becomes "now" if not specified
Product: Koha Reporter: Michael Hafen <michael.hafen>
Component: CirculationAssignee: Michael Hafen <michael.hafen>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: andrew, baptiste.wojtkowski, gmcharlt, jonathan.druart, kebliss, kyle, laurence.rault, lucas, marion.durand, martin.renvoize, nick, shi-yao.wang
Version: Main   
Hardware: All   
OS: All   
GIT URL: Initiative type: ---
Sponsorship status: --- Comma delimited list of Sponsors:
Crowdfunding goal: 0 Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 37966: When Renewing a book on hold, don't require the due date
When Renewing a book on hold, don't require the due date
Bug 37966: When Renewing a book on hold, don't require the due date
Bug 37966: Add test coverage for AddRenewal with empty due date
Bug 37966: When Renewing a book on hold, don't require the due date
Bug 37966: Add test coverage for AddRenewal with empty due date
Bug 37966: When Renewing a book on hold, don't require the due date
Bug 37966: Add test coverage for AddRenewal with empty due date
Bug 37966: (follow-up) Make empty string handling more explicit in Circulation.pm
Bug 37966: Fix renewal on hold override in circulation.pl

Description Michael Hafen 2024-09-19 18:05:27 UTC
In circ/renew.pl, when trying to renew a book, if the book is on hold the override dialog is presented.  If the 'renewonholdduedate' field is left empty, then the due date ends up being "now".
Comment 1 Michael Hafen 2024-09-19 18:20:51 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.
Comment 2 Michael Hafen 2024-09-19 18:33:10 UTC
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.
Comment 3 Shi Yao Wang 2024-09-26 18:22:40 UTC
It seems to work fine as step 7 indicates without the patch?
Comment 4 Michael Hafen 2024-10-02 21:11:22 UTC
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.
Comment 5 Laurence Rault 2025-03-19 13:58:56 UTC
Patch does not apply on sandbox : 
CONFLICT (content): Merge conflict in C4/Circulation.pm
Comment 6 Michael Hafen 2025-03-19 17:09:54 UTC
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.
Comment 7 Baptiste Wojtkowski (bwoj) 2025-03-20 14:59:33 UTC
Could not reproduce the issue here. The date correctly updates for me on step 4
Comment 8 Michael Hafen 2025-03-20 17:53:36 UTC
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'.
Comment 9 Marion Durand 2025-04-03 09:36:52 UTC
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>
Comment 10 Lucas Gass (lukeg) 2025-10-24 14:23:22 UTC
Any change to AddRenewal should be covered with a test.
Comment 11 Michael Hafen 2025-10-28 18:21:57 UTC
Created attachment 188524 [details] [review]
Bug 37966: Add test coverage for AddRenewal with empty due date
Comment 12 Martin Renvoize (ashimema) 2025-12-17 15:56:33 UTC
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>
Comment 13 Martin Renvoize (ashimema) 2025-12-17 15:56:34 UTC
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>
Comment 14 Martin Renvoize (ashimema) 2026-01-12 13:56:09 UTC
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
Comment 15 Martin Renvoize (ashimema) 2026-01-12 14:07:22 UTC
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>
Comment 16 Martin Renvoize (ashimema) 2026-01-12 14:07:23 UTC
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>
Comment 17 Martin Renvoize (ashimema) 2026-01-12 14:07:25 UTC
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.
Comment 18 Martin Renvoize (ashimema) 2026-01-12 14:07:27 UTC
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.