Bug 40435

Summary: Allow CanBookBeRenewed to consider future holds
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Hold requestsAssignee: Marcel de Rooy <m.de.rooy>
Status: Passed QA --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, lisette, martin.renvoize, nick, tomascohen
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:
Bug Depends on: 37651    
Bug Blocks:    
Attachments: Bug 40435: Database revision
Bug 40435: New preference FutureHoldsBlockRenewals
Bug 40435: Add to HEA
Bug 40435: Module changes
Bug 40435: Add tests
Bug 40435: Database revision
Bug 40435: New preference FutureHoldsBlockRenewals
Bug 40435: Add to HEA
Bug 40435: Module changes
Bug 40435: Add tests
Bug 40435: Database revision
Bug 40435: New preference FutureHoldsBlockRenewals
Bug 40435: Add to HEA
Bug 40435: Module changes
Bug 40435: Add tests
Bug 40435: (follow-up) Fix DateTime mutation in test
Bug 40435: (follow-up) Change preference type to YesNo

Description Marcel de Rooy 2025-07-17 13:18:39 UTC
This is a follow-up of bug 37651.
Comment 1 Marcel de Rooy 2025-12-12 09:10:22 UTC
Created attachment 190449 [details] [review]
Bug 40435: Database revision

Test plan:
[1] Run install or upgrade (updatedatabase.pl). Verify that pref was added to database.
* perl -MC4::Context -e'print C4::Context->preference('FutureHoldsBlockRenewals'),"\n"'
* You should see a zero printed.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2025-12-12 09:10:24 UTC
Created attachment 190450 [details] [review]
Bug 40435: New preference FutureHoldsBlockRenewals

Test plan:
Change this pref value under Administration.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2025-12-12 09:10:26 UTC
Created attachment 190451 [details] [review]
Bug 40435: Add to HEA

Grouping all future hold prefs here too.

Test plan:
prove t/db_dependent/UsageStats.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2025-12-12 09:10:28 UTC
Created attachment 190452 [details] [review]
Bug 40435: Module changes

CanBookBeRenewed will by default pass the skip_future_holds flag
to item->current_holds so that renewals are not blocked by future
holds.
The new pref FutureHoldsBlockRenewals allows you to change that
historical behavior.

Test plan:
[1] Set pref ConfirmFutureHolds=3, FutureHoldsBlockRenewals=Allow,
    OPACAllowHoldDateInFuture=Allow
[2] Checkout some item to patron A.
[3] Place hold on same item for patron B, reserve date tomorrow.
[4] Try to renew item with patron A on account page. Should be
    marked as Not renewable.
[5] Set pref FutureHoldsBlockRenewals=Dont
[6] Repeat step 4. Renew should be allowed.
[7] prove t/db_dependent/Holds.t t/db_dependent/Reserves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2025-12-12 09:10:30 UTC
Created attachment 190453 [details] [review]
Bug 40435: Add tests

Test plan:
Run t/db_dependent/Circulation.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2025-12-12 09:15:28 UTC
At this point, I have chosen to set the period for blocking renewals equal to the period of ConfirmFutureHolds (given that you enable the new pref). This seems to make the most sense now.
Comment 7 Nick Clemens (kidclamp) 2025-12-15 21:45:23 UTC
Created attachment 190520 [details] [review]
Bug 40435: Database revision

Test plan:
[1] Run install or upgrade (updatedatabase.pl). Verify that pref was added to database.
* perl -MC4::Context -e'print C4::Context->preference('FutureHoldsBlockRenewals'),"\n"'
* You should see a zero printed.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Nick Clemens (kidclamp) 2025-12-15 21:45:26 UTC
Created attachment 190521 [details] [review]
Bug 40435: New preference FutureHoldsBlockRenewals

Test plan:
Change this pref value under Administration.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Nick Clemens (kidclamp) 2025-12-15 21:45:29 UTC
Created attachment 190522 [details] [review]
Bug 40435: Add to HEA

Grouping all future hold prefs here too.

Test plan:
prove t/db_dependent/UsageStats.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Nick Clemens (kidclamp) 2025-12-15 21:45:32 UTC
Created attachment 190523 [details] [review]
Bug 40435: Module changes

CanBookBeRenewed will by default pass the skip_future_holds flag
to item->current_holds so that renewals are not blocked by future
holds.
The new pref FutureHoldsBlockRenewals allows you to change that
historical behavior.

Test plan:
[1] Set pref ConfirmFutureHolds=3, FutureHoldsBlockRenewals=Allow,
    OPACAllowHoldDateInFuture=Allow
[2] Checkout some item to patron A.
[3] Place hold on same item for patron B, reserve date tomorrow.
[4] Try to renew item with patron A on account page. Should be
    marked as Not renewable.
[5] Set pref FutureHoldsBlockRenewals=Dont
[6] Repeat step 4. Renew should be allowed.
[7] prove t/db_dependent/Holds.t t/db_dependent/Reserves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Nick Clemens (kidclamp) 2025-12-15 21:45:35 UTC
Created attachment 190524 [details] [review]
Bug 40435: Add tests

Test plan:
Run t/db_dependent/Circulation.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Marcel de Rooy 2025-12-16 16:04:10 UTC
(In reply to Nick Clemens (kidclamp) from comment #11)
> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Great! Thx
Comment 13 Martin Renvoize (ashimema) 2025-12-22 14:07:34 UTC
Created attachment 190671 [details] [review]
Bug 40435: Database revision

Test plan:
[1] Run install or upgrade (updatedatabase.pl). Verify that pref was added to database.
* perl -MC4::Context -e'print C4::Context->preference('FutureHoldsBlockRenewals'),"\n"'
* You should see a zero printed.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 14 Martin Renvoize (ashimema) 2025-12-22 14:07:36 UTC
Created attachment 190672 [details] [review]
Bug 40435: New preference FutureHoldsBlockRenewals

Test plan:
Change this pref value under Administration.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 15 Martin Renvoize (ashimema) 2025-12-22 14:07:37 UTC
Created attachment 190673 [details] [review]
Bug 40435: Add to HEA

Grouping all future hold prefs here too.

Test plan:
prove t/db_dependent/UsageStats.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 16 Martin Renvoize (ashimema) 2025-12-22 14:07:38 UTC
Created attachment 190674 [details] [review]
Bug 40435: Module changes

CanBookBeRenewed will by default pass the skip_future_holds flag
to item->current_holds so that renewals are not blocked by future
holds.
The new pref FutureHoldsBlockRenewals allows you to change that
historical behavior.

Test plan:
[1] Set pref ConfirmFutureHolds=3, FutureHoldsBlockRenewals=Allow,
    OPACAllowHoldDateInFuture=Allow
[2] Checkout some item to patron A.
[3] Place hold on same item for patron B, reserve date tomorrow.
[4] Try to renew item with patron A on account page. Should be
    marked as Not renewable.
[5] Set pref FutureHoldsBlockRenewals=Dont
[6] Repeat step 4. Renew should be allowed.
[7] prove t/db_dependent/Holds.t t/db_dependent/Reserves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 17 Martin Renvoize (ashimema) 2025-12-22 14:07:40 UTC
Created attachment 190675 [details] [review]
Bug 40435: Add tests

Test plan:
Run t/db_dependent/Circulation.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 18 Martin Renvoize (ashimema) 2025-12-22 14:07:41 UTC
Created attachment 190676 [details] [review]
Bug 40435: (follow-up) Fix DateTime mutation in test

The test had a bug where $future_date was being mutated by the
add() method on line 1975, causing line 1982 to use 4 days
instead of 2 days. This meant the test wasn't properly verifying
that FutureHoldsBlockRenewals=0 prevents blocking for holds
within the ConfirmFutureHolds window.

Test plan:
1. Run: prove t/db_dependent/Circulation.t :: "CanBookBeRenewed | future holds"
2. Verify all tests pass
3. Confirm the test now properly validates the intended behavior

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 19 Martin Renvoize (ashimema) 2025-12-22 14:07:43 UTC
Created attachment 190677 [details] [review]
Bug 40435: (follow-up) Change preference type to YesNo

Change FutureHoldsBlockRenewals from type 'Integer' to 'YesNo'
for consistency with other boolean preferences in Koha (e.g.,
AllowHoldDateInFuture, OPACAllowHoldDateInFuture).

While the preference functionally works as Integer due to the
.pref file defining the choices, YesNo is the established
convention for binary system preferences.

Test plan:
1. Apply this patch
2. Run the database update
3. Verify the preference appears correctly in system preferences
4. Verify the preference still functions properly with tests:
   prove t/db_dependent/Circulation.t :: "CanBookBeRenewed | future holds"

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 20 Martin Renvoize (ashimema) 2025-12-22 14:09:07 UTC
Sponsored Marcel?  Even if just by your own organisation?