Bug 32702 - Item statuses that block holds should be checked in CanItemBeReserved
Summary: Item statuses that block holds should be checked in CanItemBeReserved
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Nick Clemens (kidclamp)
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 30845
  Show dependency treegraph
 
Reported: 2023-01-23 16:48 UTC by Nick Clemens (kidclamp)
Modified: 2024-09-16 02:35 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 32702: Move item status checks to CanItemBeReserved (4.78 KB, patch)
2023-01-23 16:54 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32702: Move item status checks to CanItemBeReserved (5.59 KB, patch)
2023-04-17 17:10 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32702: Unit tests (2.39 KB, patch)
2023-04-17 17:10 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32702: Move item status checks to CanItemBeReserved (6.94 KB, patch)
2023-10-13 19:30 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32702: Unit tests (2.59 KB, patch)
2023-10-13 19:30 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32702: Move item status checks to CanItemBeReserved (7.01 KB, patch)
2024-02-02 20:20 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 32702: Unit tests (2.65 KB, patch)
2024-02-02 20:20 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 32702: Move item status checks to CanItemBeReserved (6.89 KB, patch)
2024-08-08 11:22 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32702: Unit tests (2.64 KB, patch)
2024-08-08 11:22 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32702: (follow-up) Add CanItemBeReserved checks to opac scripts (2.38 KB, patch)
2024-08-08 11:22 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2023-01-23 16:48:45 UTC
Currently true holdability is a combination of CanItemBeReserved/CanBookBeReserved and IsAvailableForItemLevelRequest

We should either combine these two routines, or ensure they are both called whenever needed. That is a long term goal, and this bug aims to simply move the item status checks to 'CanItemBeReserved' as they should reduce processing for any unavailable statuses to boost performance, and ultimately improve current functionality.
Comment 1 Nick Clemens (kidclamp) 2023-01-23 16:54:08 UTC
Created attachment 145590 [details] [review]
Bug 32702: Move item status checks to CanItemBeReserved

This patch moves item status checks from IsAvailableForItemLevelRequest to
CanItemBeReserved.

Changes to existing calls highlighted below.

Existing Calls for CanItemBeReserved/CanBookBeReserved:
Call in Circulation.pm
checkHighHolds - previously would have allowed damaged, notforloan, etc items to fill holds
CanBookBeRenewed - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

Call in ILSDI/Services.pm
HoldItem - would have allowed placing holds on damaged/notforloan/etc items

C4/Reserves:
What is ItemsAnyAvailableAndNotRestricted used for

C4/SIP/ILS/Transaction/Hold.pm:
do_hold - would have allowed hold on lost/damaged/etc.

Koha/Club/Hold/pm:
add - would have allowed etc.

Koha/REST/V1/Holds.pm:
add - would have allowed ...

opac/opac-reserve.pl:
Was already combo of CanItemBeReserved and IsAvailableForItemLevelRequest

reserve/placerequest:
Items would have been filtered on reserve/request.pl by IsAvailableForItemLevelRequest

In this case and opac, I believe before this a hold could have been forced with the right parameters

Existing Calls for IsAvailableForItemLevelRequest
C4/Circulation - see above

Call in ILSDI/Services.pm:
GetServices - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

Call in opac/opac-reserve.pl:
Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

Call in reserve/request.pl:
Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

To test:
1 - Setup a record with items to have:
    1 available item
    1 lost item
    1 item with positive not for loan status
    1 item with negative not for loan status
    1 item withdrawn
    1 item damaged
    1 item not for loan by itemtype
2 - Attempt to place hold on staff and opac and note the statuses
3 - Apply patch
4 - Confirm the statuses have not changed
5 - Attempt to hold lost/damaged/withdrawn/notforloan items via API
6 - You should not be able to place the hold
7 - Attempt to place club holds on these items - it should not be possible
8 - Attempt to place holds via ILSDI - it should not be possible
Comment 2 Nick Clemens (kidclamp) 2023-01-23 16:54:39 UTC
This needs unit tests, and maybe a more thorough plan - posting for opinions
Comment 3 Katrin Fischer 2023-03-07 13:48:19 UTC
In general I think we should stick with the well tested behavior from opac/intranet requests and make sure that the APIs use the same logic. So if we already had the stricter behavior in GUI, I think that's usually the right choice.

In your documentation, there is one inconsistency:

Call in ILSDI/Services.pm
HoldItem - would have allowed placing holds on damaged/notforloan/etc items

Call in ILSDI/Services.pm:
GetServices - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest


I believe you caused an unwanted change in behaviour, negative nfl-values are in fact holdable, but won't be no longer with the change:

-        $item->notforloan > 0  || # item with negative or zero notforloan value is holdable

vs. 

+    return { status => 'notforloan' } if ( $item->notforloan || $notforloan_per_itemtype );


I haven't had time to look at the methods at a whole, but do we really want to remove the checks from IsAvailableForItemLevelRequest?
Comment 4 Nick Clemens (kidclamp) 2023-04-17 17:10:07 UTC
Created attachment 149762 [details] [review]
Bug 32702: Move item status checks to CanItemBeReserved

This patch moves item status checks from IsAvailableForItemLevelRequest to
CanItemBeReserved.

Changes to existing calls highlighted below.

Existing Calls for CanItemBeReserved/CanBookBeReserved:
Call in Circulation.pm
checkHighHolds - previously would have allowed damaged, notforloan, etc items to fill holds
CanBookBeRenewed - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

Call in ILSDI/Services.pm
HoldItem - would have allowed placing holds on damaged/notforloan/etc items
GetServices - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

C4/Reserves:
What is ItemsAnyAvailableAndNotRestricted used for

C4/SIP/ILS/Transaction/Hold.pm:
do_hold - would have allowed hold on lost/damaged/etc.

Koha/Club/Hold/pm:
add - would have allowed etc.

Koha/REST/V1/Holds.pm:
add - would have allowed ...

opac/opac-reserve.pl:
Was already combo of CanItemBeReserved and IsAvailableForItemLevelRequest

reserve/placerequest:
Items would have been filtered on reserve/request.pl by IsAvailableForItemLevelRequest

In this case and opac, I believe before this a hold could have been forced with the right parameters

Existing Calls for IsAvailableForItemLevelRequest
C4/Circulation - see above

Call in opac/opac-reserve.pl:
Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

Call in reserve/request.pl:
Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

To test:
1 - Setup a record with items to have:
    1 available item
    1 lost item
    1 item with positive not for loan status
    1 item with negative not for loan status
    1 item withdrawn
    1 item damaged
    1 item not for loan by itemtype
2 - Attempt to place hold on staff and opac and note the statuses
3 - Apply patch
4 - Confirm the statuses have not changed
5 - Attempt to hold lost/damaged/withdrawn/notforloan items via API
6 - You should not be able to place the hold
7 - Attempt to place club holds on these items - it should not be possible
8 - Attempt to place holds via ILSDI - it should not be possible
Comment 5 Nick Clemens (kidclamp) 2023-04-17 17:10:09 UTC
Created attachment 149763 [details] [review]
Bug 32702: Unit tests
Comment 6 Nick Clemens (kidclamp) 2023-04-18 19:46:55 UTC
(In reply to Katrin Fischer from comment #3)
> I haven't had time to look at the methods at a whole, but do we really want
> to remove the checks from IsAvailableForItemLevelRequest?

Holdability is determined by calling both routines, and we call 'CanItemBeReserved' first - moving these checks to the start of the routine saves time calculating values that will be ignored later
Comment 7 Andrew Fuerste-Henry 2023-05-25 17:48:09 UTC
(In reply to Nick Clemens from comment #4)
> 
> To test:
> 1 - Setup a record with items to have:
>     1 available item
>     1 lost item
>     1 item with positive not for loan status
>     1 item with negative not for loan status
>     1 item withdrawn
>     1 item damaged
>     1 item not for loan by itemtype
> 2 - Attempt to place hold on staff and opac and note the statuses
> 3 - Apply patch
> 4 - Confirm the statuses have not changed
> 5 - Attempt to hold lost/damaged/withdrawn/notforloan items via API
> 6 - You should not be able to place the hold
> 7 - Attempt to place club holds on these items - it should not be possible
> 8 - Attempt to place holds via ILSDI - it should not be possible

With these patches applied, I was able to create holds via API for all of these items, despite their statuses. I did get blocked from placing holds beyond the holds-per-record limit in the circ rules, but otherwise everything was permitted.

Also, after the patches, in the staff client the lost and withdrawn items are showing new wording in the first column of the item selection table when placing a hold.
Comment 8 Nick Clemens (kidclamp) 2023-10-13 19:30:53 UTC
Created attachment 157133 [details] [review]
Bug 32702: Move item status checks to CanItemBeReserved

This patch moves item status checks from IsAvailableForItemLevelRequest to
CanItemBeReserved.

Changes to existing calls highlighted below.

Existing Calls for CanItemBeReserved/CanBookBeReserved:
Call in Circulation.pm
checkHighHolds - previously would have allowed damaged, notforloan, etc items to fill holds
CanBookBeRenewed - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

Call in ILSDI/Services.pm
HoldItem - would have allowed placing holds on damaged/notforloan/etc items
GetServices - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

C4/Reserves:
What is ItemsAnyAvailableAndNotRestricted used for

C4/SIP/ILS/Transaction/Hold.pm:
do_hold - would have allowed hold on lost/damaged/etc.

Koha/Club/Hold/pm:
add - would have allowed etc.

Koha/REST/V1/Holds.pm:
add - would have allowed ...

opac/opac-reserve.pl:
Was already combo of CanItemBeReserved and IsAvailableForItemLevelRequest

reserve/placerequest:
Items would have been filtered on reserve/request.pl by IsAvailableForItemLevelRequest

In this case and opac, I believe before this a hold could have been forced with the right parameters

Existing Calls for IsAvailableForItemLevelRequest
C4/Circulation - see above

Call in opac/opac-reserve.pl:
Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

Call in reserve/request.pl:
Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

To test:
1 - Setup a record with items to have:
    1 available item
    1 lost item
    1 item with positive not for loan status
    1 item with negative not for loan status
    1 item withdrawn
    1 item damaged
    1 item not for loan by itemtype
2 - Attempt to place hold on staff and opac and note the statuses
3 - Apply patch
4 - Confirm the statuses have not changed
5 - Attempt to hold lost/damaged/withdrawn/notforloan items via API
6 - You should not be able to place the hold
7 - Attempt to place club holds on these items - it should not be possible
8 - Attempt to place holds via ILSDI - it should not be possible
Comment 9 Nick Clemens (kidclamp) 2023-10-13 19:30:56 UTC
Created attachment 157134 [details] [review]
Bug 32702: Unit tests
Comment 10 Nick Clemens (kidclamp) 2023-10-13 19:32:03 UTC
(In reply to Andrew Fuerste-Henry from comment #7)
> (In reply to Nick Clemens from comment #4)
> With these patches applied, I was able to create holds via API for all of
> these items, despite their statuses. I did get blocked from placing holds
> beyond the holds-per-record limit in the circ rules, but otherwise
> everything was permitted.

See bug 35053 - if you only pas the item_id to the API it should work correctly

> 
> Also, after the patches, in the staff client the lost and withdrawn items
> are showing new wording in the first column of the item selection table when
> placing a hold.

Added values to the template to handle new values
Comment 11 Tomás Cohen Arazi 2023-10-17 12:33:35 UTC
(In reply to Nick Clemens from comment #10)
> (In reply to Andrew Fuerste-Henry from comment #7)
> > (In reply to Nick Clemens from comment #4)
> > With these patches applied, I was able to create holds via API for all of
> > these items, despite their statuses. I did get blocked from placing holds
> > beyond the holds-per-record limit in the circ rules, but otherwise
> > everything was permitted.
> 
> See bug 35053 - if you only pas the item_id to the API it should work
> correctly

Submitted a fix on that one. Please test!
Comment 12 Andrew Fuerste-Henry 2024-02-02 20:20:36 UTC
Created attachment 161725 [details] [review]
Bug 32702: Move item status checks to CanItemBeReserved

This patch moves item status checks from IsAvailableForItemLevelRequest to
CanItemBeReserved.

Changes to existing calls highlighted below.

Existing Calls for CanItemBeReserved/CanBookBeReserved:
Call in Circulation.pm
checkHighHolds - previously would have allowed damaged, notforloan, etc items to fill holds
CanBookBeRenewed - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

Call in ILSDI/Services.pm
HoldItem - would have allowed placing holds on damaged/notforloan/etc items
GetServices - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

C4/Reserves:
What is ItemsAnyAvailableAndNotRestricted used for

C4/SIP/ILS/Transaction/Hold.pm:
do_hold - would have allowed hold on lost/damaged/etc.

Koha/Club/Hold/pm:
add - would have allowed etc.

Koha/REST/V1/Holds.pm:
add - would have allowed ...

opac/opac-reserve.pl:
Was already combo of CanItemBeReserved and IsAvailableForItemLevelRequest

reserve/placerequest:
Items would have been filtered on reserve/request.pl by IsAvailableForItemLevelRequest

In this case and opac, I believe before this a hold could have been forced with the right parameters

Existing Calls for IsAvailableForItemLevelRequest
C4/Circulation - see above

Call in opac/opac-reserve.pl:
Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

Call in reserve/request.pl:
Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

To test:
1 - Setup a record with items to have:
    1 available item
    1 lost item
    1 item with positive not for loan status
    1 item with negative not for loan status
    1 item withdrawn
    1 item damaged
    1 item not for loan by itemtype
2 - Attempt to place hold on staff and opac and note the statuses
3 - Apply patch
4 - Confirm the statuses have not changed
5 - Attempt to hold lost/damaged/withdrawn/notforloan items via API
6 - You should not be able to place the hold
7 - Attempt to place club holds on these items - it should not be possible
8 - Attempt to place holds via ILSDI - it should not be possible

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Comment 13 Andrew Fuerste-Henry 2024-02-02 20:20:38 UTC
Created attachment 161726 [details] [review]
Bug 32702: Unit tests

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Comment 14 Victor Grousset/tuxayo 2024-02-18 05:29:56 UTC
In CanItemBeReserved, every current return uses the cache that is setup for this sub.

In this patch, the moved returns are just like `return { status => 'notforloan' }`
Pretty sure they should be like `return _cache { status => 'notforloan' }`
Comment 15 Julian Maurice 2024-04-03 13:15:43 UTC
There is at least 3 places where IsAvailableForItemLevelRequest is called and CanItemBeReserved is not:

* opac/opac-ISBDdetail.pl
* opac/opac-MARCdetail.pl
* opac/opac-detail.pl

(side note: it's the same code in those 3 scripts and it's used to set a template variable used only in opac-detail-sidebar.inc; should be moved into a TT plugin ?)

Removing checks from IsAvailableForItemLevelRequest means that those checks won't be done in these places.

Instead, do you think it's possible to call IsAvailableForItemLevelRequest from CanItemBeReserved ? (and remove calls to IsAvailableForItemLevelRequest whenever CanItemBeReserved is called)
Comment 16 Nick Clemens (kidclamp) 2024-04-03 16:05:39 UTC
(In reply to Julian Maurice from comment #15)
> There is at least 3 places where IsAvailableForItemLevelRequest is called
> and CanItemBeReserved is not:
> 
> * opac/opac-ISBDdetail.pl
> * opac/opac-MARCdetail.pl
> * opac/opac-detail.pl
> 
> (side note: it's the same code in those 3 scripts and it's used to set a
> template variable used only in opac-detail-sidebar.inc; should be moved into
> a TT plugin ?)

Indeed, I just added those checks on bug 34886
 
> Removing checks from IsAvailableForItemLevelRequest means that those checks
> won't be done in these places.
> 
> Instead, do you think it's possible to call IsAvailableForItemLevelRequest
> from CanItemBeReserved ? (and remove calls to IsAvailableForItemLevelRequest
> whenever CanItemBeReserved is called)

See comment 1 - I think that's the ultimate goal

Would you be willing to accept a smaller change in those scripts for now?
$can_holds_be_placed = IsAvailableForItemLevelRequest($item, $patron, undef) && CanItemBeReserved($patron, $item, undef);
Comment 17 Julian Maurice 2024-04-04 08:45:49 UTC
(In reply to Nick Clemens from comment #16)
> Would you be willing to accept a smaller change in those scripts for now?
> $can_holds_be_placed = IsAvailableForItemLevelRequest($item, $patron, undef)
> && CanItemBeReserved($patron, $item, undef);

Yes, that should work.
Comment 18 Nick Clemens (kidclamp) 2024-08-08 11:22:53 UTC
Created attachment 170156 [details] [review]
Bug 32702: Move item status checks to CanItemBeReserved

This patch moves item status checks from IsAvailableForItemLevelRequest to
CanItemBeReserved.

Changes to existing calls highlighted below.

Existing Calls for CanItemBeReserved/CanBookBeReserved:
Call in Circulation.pm
checkHighHolds - previously would have allowed damaged, notforloan, etc items to fill holds
CanBookBeRenewed - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

Call in ILSDI/Services.pm
HoldItem - would have allowed placing holds on damaged/notforloan/etc items
GetServices - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

C4/Reserves:
What is ItemsAnyAvailableAndNotRestricted used for

C4/SIP/ILS/Transaction/Hold.pm:
do_hold - would have allowed hold on lost/damaged/etc.

Koha/Club/Hold/pm:
add - would have allowed etc.

Koha/REST/V1/Holds.pm:
add - would have allowed ...

opac/opac-reserve.pl:
Was already combo of CanItemBeReserved and IsAvailableForItemLevelRequest

reserve/placerequest:
Items would have been filtered on reserve/request.pl by IsAvailableForItemLevelRequest

In this case and opac, I believe before this a hold could have been forced with the right parameters

Existing Calls for IsAvailableForItemLevelRequest
C4/Circulation - see above

Call in opac/opac-reserve.pl:
Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

Call in reserve/request.pl:
Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest

To test:
1 - Setup a record with items to have:
    1 available item
    1 lost item
    1 item with positive not for loan status
    1 item with negative not for loan status
    1 item withdrawn
    1 item damaged
    1 item not for loan by itemtype
2 - Attempt to place hold on staff and opac and note the statuses
3 - Apply patch
4 - Confirm the statuses have not changed
5 - Attempt to hold lost/damaged/withdrawn/notforloan items via API
6 - You should not be able to place the hold
7 - Attempt to place club holds on these items - it should not be possible
8 - Attempt to place holds via ILSDI - it should not be possible

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Comment 19 Nick Clemens (kidclamp) 2024-08-08 11:22:55 UTC
Created attachment 170157 [details] [review]
Bug 32702: Unit tests

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Comment 20 Nick Clemens (kidclamp) 2024-08-08 11:22:58 UTC
Created attachment 170158 [details] [review]
Bug 32702: (follow-up) Add CanItemBeReserved checks to opac scripts
Comment 21 Victor Grousset/tuxayo 2024-09-16 02:35:38 UTC
comment 14 is still valid, the patch is missing the use of the cache mechanism on return.