Bug 37651 - biblio->current_holds and item->current_holds do not respect ConfirmFutureHolds
Summary: biblio->current_holds and item->current_holds do not respect ConfirmFutureHolds
Status: Signed Off
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: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-08-15 13:50 UTC by Marcel de Rooy
Modified: 2025-02-19 12:52 UTC (History)
3 users (show)

See Also:
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 (1.48 KB, patch)
2024-09-10 12:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37651: Add ConfirmFutureHolds to biblio->current_holds (1.61 KB, patch)
2024-09-10 12:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37651: Unit tests (3.08 KB, patch)
2024-09-10 12:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37651: Unit tests (3.13 KB, patch)
2024-09-10 12:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37651: Add ConfirmFutureHolds to item->current_holds (1.48 KB, patch)
2025-01-16 13:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37651: Add ConfirmFutureHolds to biblio->current_holds (1.61 KB, patch)
2025-01-16 13:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37651: Unit tests (3.13 KB, patch)
2025-01-16 13:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37651: Add ConfirmFutureHolds to item->current_holds (1.51 KB, patch)
2025-01-20 13:44 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37651: Add ConfirmFutureHolds to biblio->current_holds (1.64 KB, patch)
2025-01-20 13:44 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37651: Unit tests (3.16 KB, patch)
2025-01-20 13:45 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37651: (QA follow-up) Add POD for Biblio method (1.22 KB, patch)
2025-01-20 13:45 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37651: (QA follow-up) Rewording for pref ConfirmFutureHolds (1.53 KB, patch)
2025-01-31 08:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37651: Add ConfirmFutureHolds to item->current_holds (1.38 KB, patch)
2025-02-19 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37651: Add ConfirmFutureHolds to biblio->current_holds (1.68 KB, patch)
2025-02-19 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37651: Unit tests (3.20 KB, patch)
2025-02-19 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37651: (QA follow-up) Add POD for Biblio method (1.26 KB, patch)
2025-02-19 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37651: (QA follow-up) Rewording for pref ConfirmFutureHolds (1.53 KB, patch)
2025-02-19 12:52 UTC, Marcel de Rooy
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 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