Bug 28078 - Add option to ignore hold counts when checking CanItemBeReserved
Summary: Add option to ignore hold counts when checking CanItemBeReserved
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 28013
  Show dependency treegraph
 
Reported: 2021-04-02 14:28 UTC by Nick Clemens
Modified: 2021-05-24 14:09 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.06,20.05.12


Attachments
Bug 28078 - Add 'ignore_hold_counts' param to CanItemBeReserved (4.58 KB, patch)
2021-04-02 14:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28078 - Add 'ignore_hold_counts' param to CanItemBeReserved (4.63 KB, patch)
2021-04-02 18:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 28078: (follow-up) Add param check to 'itemAlreadyOnHold' test (2.26 KB, patch)
2021-04-06 13:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28078: Add 'ignore_hold_counts' param to CanItemBeReserved (4.83 KB, patch)
2021-04-07 08:54 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28078: (follow-up) Add param check to 'itemAlreadyOnHold' test (2.41 KB, patch)
2021-04-07 08:54 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-04-02 14:28:46 UTC
In 'CanBookBeRenewed' we call 'CanItemBeReserved' to see if an item is available to fill a hold - this doesn't work because:
1 - We call it wrong (bug 28013)
2 - We count the current hold against the limit

This bug adds an optional param 'ignore_hold_counts' to the routine, while still forbidding holds when 0 are allowed
Comment 1 Nick Clemens 2021-04-02 14:31:45 UTC
Created attachment 119124 [details] [review]
Bug 28078 - Add 'ignore_hold_counts' param to CanItemBeReserved

This patch adds an optional param 'ignore_hold_counts' to the routine, while still forbidding holds when 0 are allowed

To test:
1 - prove -v t/db_dependent/Holds.t
Comment 2 David Nind 2021-04-02 18:42:13 UTC
Created attachment 119143 [details] [review]
Bug 28078 - Add 'ignore_hold_counts' param to CanItemBeReserved

This patch adds an optional param 'ignore_hold_counts' to the routine, while still forbidding holds when 0 are allowed

To test:
1 - prove -v t/db_dependent/Holds.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Joonas Kylmälä 2021-04-06 07:15:22 UTC
Let me come back to this later but I'm pretty sure we cannot do this because (at least how the parameter is used in the bug 28013) it wouldn't follow circulation rules. We have libraries intentionally defining the amounts of holds a patron in a specific patron category and in a specific library can do to a specific item type. See also Bug 26659.
Comment 4 Nick Clemens 2021-04-06 11:45:48 UTC
(In reply to Joonas Kylmälä from comment #3)
> Let me come back to this later but I'm pretty sure we cannot do this because
> (at least how the parameter is used in the bug 28013) it wouldn't follow
> circulation rules. We have libraries intentionally defining the amounts of
> holds a patron in a specific patron category and in a specific library can
> do to a specific item type. See also Bug 26659.

I see what you are saying, the way I understand it is this:
In CanBookBeRenewed we call CanItemBeReserved to see if the item could fill the reserve

Those are two different questions:

CanItemBeReserved answers the question: "Can the patron place a hold on this item?"

What we are asking in CanBookBeRenewed is: "Can this item fill any hold for this patron?"

In the first question the number of holds the patron currently has should be taken into account as the end result would be adding one more

In the second question the number of holds don't matter, because we will not add any holds, we just want to know if we can fill the hold


The other solution is two subroutines:
CanItemBeReserved
CanItemFillHold

Note: We do have Koha::Holds->get_items_that_can_fill, however, that leaves out checks on transfer limits and counts of holds
Comment 5 Joonas Kylmälä 2021-04-06 12:41:13 UTC
(In reply to Nick Clemens from comment #4)
> (In reply to Joonas Kylmälä from comment #3)
> > Let me come back to this later but I'm pretty sure we cannot do this because
> > (at least how the parameter is used in the bug 28013) it wouldn't follow
> > circulation rules. We have libraries intentionally defining the amounts of
> > holds a patron in a specific patron category and in a specific library can
> > do to a specific item type. See also Bug 26659.
> 
> I see what you are saying, the way I understand it is this:
> In CanBookBeRenewed we call CanItemBeReserved to see if the item could fill
> the reserve
> 
> Those are two different questions:
> 
> CanItemBeReserved answers the question: "Can the patron place a hold on this
> item?"
> 
> What we are asking in CanBookBeRenewed is: "Can this item fill any hold for
> this patron?"
> 
> In the first question the number of holds the patron currently has should be
> taken into account as the end result would be adding one more
> 
> In the second question the number of holds don't matter, because we will not
> add any holds, we just want to know if we can fill the hold
> 
> 
> The other solution is two subroutines:
> CanItemBeReserved
> CanItemFillHold
> 
> Note: We do have Koha::Holds->get_items_that_can_fill, however, that leaves
> out checks on transfer limits and counts of holds

Interesting. I see now you added the condition

> && (!$params->{ignore_hold_counts} || $allowedreserves == 0 )

so it still keeps follows the circulation rules also when allowedreserves rules is 0. Maybe doing the check on also how many hold requests the patron has to the biblio or item is not relevant here as you said.

However, I think this patch should also skip the itemAlreadyOnHold return value for this to work on item-level holds as well, could you please re-do the patch?
Comment 6 Nick Clemens 2021-04-06 13:22:12 UTC
Created attachment 119220 [details] [review]
Bug 28078: (follow-up) Add param check to 'itemAlreadyOnHold' test
Comment 7 Joonas Kylmälä 2021-04-07 08:54:50 UTC
Created attachment 119263 [details] [review]
Bug 28078: Add 'ignore_hold_counts' param to CanItemBeReserved

This patch adds an optional param 'ignore_hold_counts' to the routine, while still forbidding holds when 0 are allowed

To test:
1 - prove -v t/db_dependent/Holds.t

Signed-off-by: David Nind <david@davidnind.com>
JK: Commit message amended: Fixed title formatting
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 8 Joonas Kylmälä 2021-04-07 08:54:56 UTC
Created attachment 119264 [details] [review]
Bug 28078: (follow-up) Add param check to 'itemAlreadyOnHold' test

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 9 Joonas Kylmälä 2021-04-07 08:55:23 UTC
Passing QA.
Comment 10 Jonathan Druart 2021-04-07 14:38:46 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 11 Fridolin Somers 2021-04-19 10:08:48 UTC
+  should not check if there are too many holds as we only csre about reservability

Is it a typo for "care" ?

This is more enhancement than bug no ?
Comment 12 Joonas Kylmälä 2021-04-19 10:21:58 UTC
(In reply to Fridolin Somers from comment #11)
> +  should not check if there are too many holds as we only csre about
> reservability
> 
> Is it a typo for "care" ?

yup.

> This is more enhancement than bug no ?

This is a dependency to fix a bug. The bug fix is in bug 28013. The issues caused by this bug were reported also in other bugs, like bug 26659.
Comment 13 Fridolin Somers 2021-04-29 09:00:29 UTC
Pushed to 20.11.x for 20.11.06
Comment 14 Andrew Fuerste-Henry 2021-05-24 13:24:33 UTC
Pushed to 20.05.x for 20.05.12
Comment 15 Victor Grousset/tuxayo 2021-05-24 14:09:41 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.