Bug 37651

Summary: biblio->current_holds and item->current_holds do not respect ConfirmFutureHolds
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Hold requestsAssignee: Marcel de Rooy <m.de.rooy>
Status: Failed QA --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, jonathan.druart, martin.renvoize, nick
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37650
GIT URL: Change sponsored?: ---
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 37651: Add ConfirmFutureHolds to item->current_holds
Bug 37651: Add ConfirmFutureHolds to biblio->current_holds
Bug 37651: Unit tests
Bug 37651: Unit tests
Bug 37651: Add ConfirmFutureHolds to item->current_holds
Bug 37651: Add ConfirmFutureHolds to biblio->current_holds
Bug 37651: Unit tests
Bug 37651: Add ConfirmFutureHolds to item->current_holds
Bug 37651: Add ConfirmFutureHolds to biblio->current_holds
Bug 37651: Unit tests
Bug 37651: (QA follow-up) Add POD for Biblio method
Bug 37651: (QA follow-up) Rewording for pref ConfirmFutureHolds
Bug 37651: Add ConfirmFutureHolds to item->current_holds
Bug 37651: Add ConfirmFutureHolds to biblio->current_holds
Bug 37651: Unit tests
Bug 37651: (QA follow-up) Add POD for Biblio method
Bug 37651: (QA follow-up) Rewording for pref ConfirmFutureHolds

Description Marcel de Rooy 2024-08-15 13:50:28 UTC
See also bug 37650.
When we consider ConfirmFutureHolds to build the holds to pull list, we actually could say that these holds, probably only one or two days ahead, are current holds too. Currently, they are just completely ignored. But e.g. having a library depot further away require collecting those items earlier etc.

At least the majority of calls look like they could use the lookahead days from ConfirmFutureHolds.

Filed as enh, but very close to a bug.
Comment 1 Marcel de Rooy 2024-09-10 12:20:40 UTC
Created attachment 171247 [details] [review]
Bug 37651: Add ConfirmFutureHolds to item->current_holds

Test plan:
Add future item level hold for another branch.
Check in. Confirm and transfer.
Note that without this patch, there is no patron info for the column
On hold for on transferstoreceive.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2024-09-10 12:20:43 UTC
Created attachment 171248 [details] [review]
Bug 37651: Add ConfirmFutureHolds to biblio->current_holds

Test plan:
Easy way is to run the unit tests in the next patch.
One call of biblio->current_holds is hidden in acqui/parcel.pl. The
table column "Item holds / Total holds" should now make a difference
for an order referring to an item having future holds.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2024-09-10 12:20:45 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2024-09-10 12:21:18 UTC
Created attachment 171250 [details] [review]
Bug 37651: Unit tests

Test plan:
Run t/db_dependent/Holds.t (Without previous patches, it would fail.)
Run t/db_dependent/Koha/Biblios.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2025-01-16 13:15:19 UTC
Created attachment 176654 [details] [review]
Bug 37651: Add ConfirmFutureHolds to item->current_holds

Test plan:
Add future item level hold for another branch.
Check in. Confirm and transfer.
Note that without this patch, there is no patron info for the column
On hold for on transferstoreceive.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2025-01-16 13:15:21 UTC
Created attachment 176655 [details] [review]
Bug 37651: Add ConfirmFutureHolds to biblio->current_holds

Test plan:
Easy way is to run the unit tests in the next patch.
One call of biblio->current_holds is hidden in acqui/parcel.pl. The
table column "Item holds / Total holds" should now make a difference
for an order referring to an item having future holds.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2025-01-16 13:15:24 UTC
Created attachment 176656 [details] [review]
Bug 37651: Unit tests

Test plan:
Run t/db_dependent/Holds.t (Without previous patches, it would fail.)
Run t/db_dependent/Koha/Biblios.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2025-01-16 13:15:47 UTC
Rebased. Waiting since September..
Comment 9 Martin Renvoize (ashimema) 2025-01-20 13:44:57 UTC
Created attachment 176808 [details] [review]
Bug 37651: Add ConfirmFutureHolds to item->current_holds

Test plan:
Add future item level hold for another branch.
Check in. Confirm and transfer.
Note that without this patch, there is no patron info for the column
On hold for on transferstoreceive.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize (ashimema) 2025-01-20 13:44:59 UTC
Created attachment 176809 [details] [review]
Bug 37651: Add ConfirmFutureHolds to biblio->current_holds

Test plan:
Easy way is to run the unit tests in the next patch.
One call of biblio->current_holds is hidden in acqui/parcel.pl. The
table column "Item holds / Total holds" should now make a difference
for an order referring to an item having future holds.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize (ashimema) 2025-01-20 13:45:01 UTC
Created attachment 176810 [details] [review]
Bug 37651: Unit tests

Test plan:
Run t/db_dependent/Holds.t (Without previous patches, it would fail.)
Run t/db_dependent/Koha/Biblios.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize (ashimema) 2025-01-20 13:45:04 UTC
Created attachment 176811 [details] [review]
Bug 37651: (QA follow-up) Add POD for Biblio method

We were missing POD here.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize (ashimema) 2025-01-20 13:47:22 UTC
This is a fun one.. current_holds is called in a fair number of places; thus, whilst this is a small and clear patch, it could have some wide-reaching effects.

I'm signing off and will seek some QA opinions.  My gut says it makes sense and is a step toward being able to deprecate C4::Reserves::CheckReserve.
Comment 14 Marcel de Rooy 2025-01-20 13:50:06 UTC
(In reply to Martin Renvoize (ashimema) from comment #12)
> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Thx !
Comment 15 Jonathan Druart 2025-01-28 15:32:32 UTC
This feels wrong to me.

The pref description: "Note that this number of days will be used too in calculating the default end date for the Holds to pull-report. But it does not interfere with issuing, renewing or transferring items."

It seems that we need a filter_by method, and use it only where it needs this pref to be taken into account.
Comment 16 Marcel de Rooy 2025-01-31 08:10:44 UTC
(In reply to Jonathan Druart from comment #15)
> This feels wrong to me.
> 
> The pref description: "Note that this number of days will be used too in
> calculating the default end date for the Holds to pull-report. But it does
> not interfere with issuing, renewing or transferring items."
> 
> It seems that we need a filter_by method, and use it only where it needs
> this pref to be taken into account.

You are right. We should adjust the wording of that pref. Will add a follow-up.

The filter_by comment is valid too. But I dont think that it should be done here. It is an improvement on its own. There are more methods that could be reworked into filter_by constructs. Like for instance:
Koha/Biblio.pm current_checkouts
Koha/Acquisition/Order.pm current_item_level_holds

The point of this report is refine the existing two current_holds methods where the pref should be taken into account.
Comment 17 Marcel de Rooy 2025-01-31 08:19:18 UTC
Created attachment 177372 [details] [review]
Bug 37651: (QA follow-up) Rewording for pref ConfirmFutureHolds

As requested by QA.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2025-02-19 12:52:00 UTC
Created attachment 178325 [details] [review]
Bug 37651: Add ConfirmFutureHolds to item->current_holds

Test plan:
Add future item level hold for another branch.
Check in. Confirm and transfer.
Note that without this patch, there is no patron info for the column
On hold for on transferstoreceive.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Marcel de Rooy 2025-02-19 12:52:03 UTC
Created attachment 178326 [details] [review]
Bug 37651: Add ConfirmFutureHolds to biblio->current_holds

Test plan:
Easy way is to run the unit tests in the next patch.
One call of biblio->current_holds is hidden in acqui/parcel.pl. The
table column "Item holds / Total holds" should now make a difference
for an order referring to an item having future holds.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Marcel de Rooy 2025-02-19 12:52:07 UTC
Created attachment 178327 [details] [review]
Bug 37651: Unit tests

Test plan:
Run t/db_dependent/Holds.t (Without previous patches, it would fail.)
Run t/db_dependent/Koha/Biblios.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Marcel de Rooy 2025-02-19 12:52:10 UTC
Created attachment 178328 [details] [review]
Bug 37651: (QA follow-up) Add POD for Biblio method

We were missing POD here.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Marcel de Rooy 2025-02-19 12:52:13 UTC
Created attachment 178329 [details] [review]
Bug 37651: (QA follow-up) Rewording for pref ConfirmFutureHolds

As requested by QA.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2025-02-19 12:52:34 UTC
Rebased on tidied codebase
Comment 24 Nick Clemens (kidclamp) 2025-04-03 14:08:54 UTC
I am not convinced this is a desired behavior change for all libraries. When they decided to set the pref, it explicitly said "does not affect" several things

Would you be open to a second setting within the pref:

CircConfirmFutureHolds (at checkin, at renewal, at checkout - multi select?) starting no later then XX days
Comment 25 Marcel de Rooy 2025-04-11 10:36:58 UTC
(In reply to Nick Clemens (kidclamp) from comment #24)
> I am not convinced this is a desired behavior change for all libraries. When
> they decided to set the pref, it explicitly said "does not affect" several
> things
> 
> Would you be open to a second setting within the pref:
> 
> CircConfirmFutureHolds (at checkin, at renewal, at checkout - multi select?)
> starting no later then XX days

Hi Nick. Thanks for looking here !

The description "But it does not interfere with issuing, renewing or transferring items" is not the most lucky one. I might have added it myself long ago ;)
Note also that we are talking about a feature that say over 90% of the libraries is not using.  (HEA: 1040 AllowHoldDateinFuture 728 OPACAllowHoldDateInFuture )
Lets look at the processes mentioned. (When writing this patch, I reviewed all occurrences of current_holds btw.)

Checkout
AddIssue already calls MoveReserve and that does check ConfirmFutureHolds. So this is an "existing interference". The description was wrong.

Renew
CanBookBeRenewed calls current_holds.
This is a "new interference". If a library allows future holds and wants to confirm them within a few days specified by ConfirmFutureHolds, currently this is ignored by CanBookBeRenewed. So the book is renewed although there is a hold for it on tomorrow. Not sure if we should write code for that? Probably most do not want to renew here.

Transfer
transferbook calls CheckReserves without lookahead days. So it does not block a transfer for a future hold (when not ignoring them altogether).
Feels like a bug to me that could be solved on another report :)
So there is currently no interference here. Discussion elsewhere?

Checkin
This is not relevant here. ConfirmFutureHolds already affects checkin (especially the popup). And this is already advertized accordingly.

In conclusion, the new extra preference should only pertain to renewals. This would imo be a rather ugly exception. I would advise against it.
Comment 26 Marcel de Rooy 2025-04-17 09:20:34 UTC
Moving back to SO queue.
Any thoughts, Nick?
Comment 27 Nick Clemens (kidclamp) 2025-07-02 14:35:15 UTC
(In reply to Marcel de Rooy from comment #25)
> The description "But it does not interfere with issuing, renewing or
> transferring items" is not the most lucky one. I might have added it myself
> long ago ;)

> Note also that we are talking about a feature that say over 90% of the
> libraries is not using.  (HEA: 1040 AllowHoldDateinFuture 728
> OPACAllowHoldDateInFuture )

10% is not an insignificant amount of libraries imo

> Lets look at the processes mentioned. (When writing this patch, I reviewed
> all occurrences of current_holds btw.)
> 
> Checkout
> AddIssue already calls MoveReserve and that does check ConfirmFutureHolds.
> So this is an "existing interference". The description was wrong.

MoveReserve in this context is checking for future holds, but only to fill the current patrons future holds - or cancel or remove waiting status from an existing hold - it does not block the checkout in any way - and it looks like this won't change after the patch. So the description seems right to me

> Renew
> CanBookBeRenewed calls current_holds.
> This is a "new interference". If a library allows future holds and wants to
> confirm them within a few days specified by ConfirmFutureHolds, currently
> this is ignored by CanBookBeRenewed. So the book is renewed although there
> is a hold for it on tomorrow. Not sure if we should write code for that?
> Probably most do not want to renew here.

I don't know that this is true - a patron who has a back might be allowed to renew as it is in their possession. I think future holds confirmation is meant to capture a book that is returned - forcing others to return it earlier is more akin to what bookings and recalls do. This is a significant change in behavior I think, and should, at least, be added as a test case for CanBookBeRenewed

 
In my review, these are the effects:
C4::Circulation CanBookBeRenewed (will now be no)
C4/ILSDI/Services GetAvailability (will now be on hold)
C4/Reserves AddReserve (will now not be set to waiting if there is a future hold)
Koha/Item first_hold (could now return a future hold over a non-future hold, this seems okay to me, but I am not sure of all contexts of first_hold)


I think below will just add the new holds to display without behaviour change:
Koha/Acquisition/Order current_item_level_holds
C4/SIP/ILS/Item new
acqui/parcel.pl
circ/transferstoreceive.pl  
opac/opac-reserve.pl
reserve/request.pl

I won't block this one, but can leave to the RM to decide - I would like to see tests added to CanBookBeRenewed and GetAvailability for QA