Bug 40435 - Allow CanBookBeRenewed to consider future holds
Summary: Allow CanBookBeRenewed to consider future holds
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Marcel de Rooy
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 37651
Blocks:
  Show dependency treegraph
 
Reported: 2025-07-17 13:18 UTC by Marcel de Rooy
Modified: 2025-12-22 14:09 UTC (History)
5 users (show)

See Also:
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 40435: Database revision (2.92 KB, patch)
2025-12-12 09:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40435: New preference FutureHoldsBlockRenewals (1.78 KB, patch)
2025-12-12 09:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40435: Add to HEA (1.21 KB, patch)
2025-12-12 09:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40435: Module changes (2.58 KB, patch)
2025-12-12 09:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40435: Add tests (3.31 KB, patch)
2025-12-12 09:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40435: Database revision (2.94 KB, patch)
2025-12-15 21:45 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 40435: New preference FutureHoldsBlockRenewals (1.80 KB, patch)
2025-12-15 21:45 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 40435: Add to HEA (1.22 KB, patch)
2025-12-15 21:45 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 40435: Module changes (2.60 KB, patch)
2025-12-15 21:45 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 40435: Add tests (3.32 KB, patch)
2025-12-15 21:45 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 40435: Database revision (3.00 KB, patch)
2025-12-22 14:07 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40435: New preference FutureHoldsBlockRenewals (1.87 KB, patch)
2025-12-22 14:07 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40435: Add to HEA (1.29 KB, patch)
2025-12-22 14:07 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40435: Module changes (2.67 KB, patch)
2025-12-22 14:07 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40435: Add tests (3.39 KB, patch)
2025-12-22 14:07 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40435: (follow-up) Fix DateTime mutation in test (1.51 KB, patch)
2025-12-22 14:07 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40435: (follow-up) Change preference type to YesNo (3.16 KB, patch)
2025-12-22 14:07 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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?