This is a follow-up of bug 37651.
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>
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>
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>
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>
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>
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.
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>
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>
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>
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>
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>
(In reply to Nick Clemens (kidclamp) from comment #11) > Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Great! Thx
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>
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>
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>
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>
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>
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>
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>
Sponsored Marcel? Even if just by your own organisation?