From cc0d1822cc8e7a835b2ccc037dc7cb9a82b024fd Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
Date: Mon, 23 Jan 2023 16:48:53 +0000
Subject: [PATCH] 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
---
 C4/Reserves.pm            | 35 +++++++++++++++--------------------
 t/db_dependent/Reserves.t |  7 +------
 2 files changed, 16 insertions(+), 26 deletions(-)

diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index 987543e1cd..d1884995e8 100644
--- a/C4/Reserves.pm
+++ b/C4/Reserves.pm
@@ -436,15 +436,27 @@ sub CanItemBeReserved {
         }
     }
 
-    # we retrieve borrowers and items informations #
-    # item->{itype} will come for biblioitems if necessery
-    my $borrower = $patron->unblessed;
 
+    # must check the notforloan setting of the itemtype
+    # FIXME - a lot of places in the code do this
+    #         or something similar - need to be
+    #         consolidated
+    my $itemtype = $item->effective_itemtype;
+    return { status => 'missing_itemtype' }
+      unless defined $itemtype;
+    my $notforloan_per_itemtype = Koha::ItemTypes->find($itemtype)->notforloan;
+
+    return { status => 'notforloan' } if ( $item->notforloan > 0 || $notforloan_per_itemtype );
+    return { status => 'itemlost' } if $item->itemlost;
+    return { status => 'withdrawn' } if $item->withdrawn;
     # If an item is damaged and we don't allow holds on damaged items, we can stop right here
     return { status =>'damaged' }
       if ( $item->damaged
         && !C4::Context->preference('AllowHoldsOnDamagedItems') );
 
+    # we retrieve borrowers information #
+    my $borrower = $patron->unblessed;
+
     if( GetMarcFromKohaField('biblioitems.agerestriction') ){
         my $biblio = $item->biblio;
         # Check for the age restriction
@@ -1360,23 +1372,6 @@ sub IsAvailableForItemLevelRequest {
     # looped outside of IsAvailableForItemLevelRequest to avoid nested loops:
     my $items_any_available = shift;
 
-    my $dbh = C4::Context->dbh;
-    # must check the notforloan setting of the itemtype
-    # FIXME - a lot of places in the code do this
-    #         or something similar - need to be
-    #         consolidated
-    my $itemtype = $item->effective_itemtype;
-    return 0
-      unless defined $itemtype;
-    my $notforloan_per_itemtype = Koha::ItemTypes->find($itemtype)->notforloan;
-
-    return 0 if
-        $notforloan_per_itemtype ||
-        $item->itemlost        ||
-        $item->notforloan > 0  || # item with negative or zero notforloan value is holdable
-        $item->withdrawn        ||
-        ($item->damaged && !C4::Context->preference('AllowHoldsOnDamagedItems'));
-
     if ($pickup_branchcode) {
         my $destination = Koha::Libraries->find($pickup_branchcode);
         return 0 unless $destination;
diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t
index 3371c9539d..a136c2f9e9 100755
--- a/t/db_dependent/Reserves.t
+++ b/t/db_dependent/Reserves.t
@@ -1455,7 +1455,7 @@ $schema->storage->txn_rollback();
 
 subtest 'IsAvailableForItemLevelRequest() tests' => sub {
 
-    plan tests => 2;
+    plan tests => 1;
 
     $schema->storage->txn_begin;
 
@@ -1468,11 +1468,6 @@ subtest 'IsAvailableForItemLevelRequest() tests' => sub {
 
     my $item = $builder->build_sample_item;
 
-    ok(
-        !C4::Reserves::IsAvailableForItemLevelRequest( $item, $patron ),
-        "Item not available for item-level hold because no effective item type"
-    );
-
     # Weird use case to highlight issue
     $item_type = '0';
     Koha::ItemTypes->search( { itemtype => $item_type } )->delete;
-- 
2.30.2