From 20dcff0ef3a475df846ba5d99dee59f80ba0e191 Mon Sep 17 00:00:00 2001 From: Nick Clemens 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 --- 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 @@ No valid pickup location [% ELSIF itemloo.not_holdable == 'notforloan' %] Not for loan + [% ELSIF itemloo.not_holdable == 'withdrawn' %] + Item has been withdrawn + [% ELSIF itemloo.not_holdable == 'itemlost' %] + Item has been marked lost [% ELSE %] [% itemloo.not_holdable | html %] [% 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