From 20dcff0ef3a475df846ba5d99dee59f80ba0e191 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

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
---
 C4/Reserves.pm                                | 32 ++++++++-----------
 .../prog/en/modules/reserve/request.tt        |  4 +++
 t/db_dependent/Reserves.t                     |  7 +---
 3 files changed, 18 insertions(+), 25 deletions(-)

diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index 432ae461dc5..87042320e02 100644
--- a/C4/Reserves.pm
+++ b/C4/Reserves.pm
@@ -486,6 +486,19 @@ sub CanItemBeReserved {
         }
     }
 
+    # 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 _cache { status => 'damaged' }
         if ( $item->damaged
@@ -1356,25 +1369,6 @@ sub IsAvailableForItemLevelRequest {
     my $patron            = shift;
     my $pickup_branchcode = 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/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
index 558b5e0d9f0..f86b3ce0e1d 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
@@ -890,6 +890,10 @@
                                                                 <span>No valid pickup location</span>
                                                             [% ELSIF itemloo.not_holdable == 'notforloan' %]
                                                                 <span>Not for loan</span>
+                                                            [% ELSIF itemloo.not_holdable == 'withdrawn' %]
+                                                                <span>Item has been withdrawn</span>
+                                                            [% ELSIF itemloo.not_holdable == 'itemlost' %]
+                                                                <span>Item has been marked lost</span>
                                                             [% ELSE %]
                                                                 <span>[% itemloo.not_holdable | html %]</span>
                                                             [% END %]
diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t
index 93ab654c183..fdca5ccdb0a 100755
--- a/t/db_dependent/Reserves.t
+++ b/t/db_dependent/Reserves.t
@@ -1540,7 +1540,7 @@ $schema->storage->txn_rollback();
 
 subtest 'IsAvailableForItemLevelRequest() tests' => sub {
 
-    plan tests => 3;
+    plan tests => 2;
 
     $schema->storage->txn_begin;
 
@@ -1553,11 +1553,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.39.5