From 316af311016333c75994bfcdd6e66a69a3858f47 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, 20 insertions(+), 23 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index bc96dc9c3c1..8d45ce3e0e9 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -484,11 +484,26 @@ 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 && !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 @@ -1330,23 +1345,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 15684309541..8e2cc5ff96a 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt @@ -904,6 +904,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 14fb88d5cbf..3e02d4ec625 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -1452,7 +1452,7 @@ $schema->storage->txn_rollback(); subtest 'IsAvailableForItemLevelRequest() tests' => sub { - plan tests => 3; + plan tests => 2; $schema->storage->txn_begin; @@ -1465,11 +1465,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.2