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