Attachment 170156 Details for Bug 32702 – Bug 32702: Move item status checks to CanItemBeReserved

[patch] Bug 32702: Move item status checks to CanItemBeReserved

Bug-32702-Move-item-status-checks-to-CanItemBeRese.patch (text/plain), 6.89 KB, created by Nick Clemens (kidclamp) on 2024-08-08 11:22:53 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

Created: 2024-08-08 11:22:53 UTC

Size: 6.89 KB

patch

obsolete

>From 316af311016333c75994bfcdd6e66a69a3858f47 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, 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

Actions: View | Diff | Splinter Review

Attachments on bug 32702: 145590 | 149762 | 149763 | 157133 | 157134 | 161725 | 161726 | 170156 | 170157 | 170158