From 12c9d543c3c492be15af9ecfd7d1f81d30dd6921 Mon Sep 17 00:00:00 2001 From: Arthur Suzuki Date: Thu, 21 Mar 2019 21:52:06 +0100 Subject: [PATCH] Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved Via ILSDI we use the routines mentioned above to determine holdability, however, neither of these routines check on shelf hold policies Test plan : 1 / Check that default circulation have OnShelfHoldsAllowed to Yes 2 / Place a hold on a book with a single item, item being available for loan, verify that hold can be placed. 3 / Set OnShelfHoldsAllowed to any other value, verify that hold cannot be placed for reason "onShelfHoldsNotAllowed" (shown in the item table). --- C4/Reserves.pm | 14 +++++++-- t/db_dependent/Reserves.t | 78 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 87 insertions(+), 5 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 348e6e58db..d5a16e0bf8 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -348,6 +348,7 @@ sub CanBookBeReserved{ current checkout against the high holds threshold @RETURNS { status => OK }, if the Item can be reserved. + { status => onShelfHoldsNotAllowed }, if onShelfHoldsAllowed parameter and item availability combination doesn't allow holds. { status => ageRestricted }, if the Item is age restricted for this borrower. { status => damaged }, if the Item is damaged. { status => cannotReserveFromOtherBranches }, if syspref 'canreservefromotherbranches' is OK. @@ -370,10 +371,12 @@ sub CanItemBeReserved { my $holds_per_record = 1; # Total number of holds allowed for this one given record my $holds_per_day; # Default to unlimited my $opacitemholds = 'Y'; # Itemlevel holds default to allowed + my $on_shelf_holds = 0; # Default to "if any unavailable" my $context = $params->{context} // ''; # we retrieve borrowers and items informations # # item->{itype} will come for biblioitems if necessery + my $item = Koha::Items->find($itemnumber); my $biblio = $item->biblio; my $patron = Koha::Patrons->find( $borrowernumber ); @@ -424,6 +427,7 @@ sub CanItemBeReserved { $holds_per_record = $rights->{holds_per_record} // $holds_per_record; $holds_per_day = $rights->{holds_per_day}; $opacitemholds = $rights->{opacitemholds}; + $on_shelf_holds = $rights->{onshelfholds}; } else { $ruleitemtype = undef; @@ -433,8 +437,13 @@ sub CanItemBeReserved { borrowernumber => $borrowernumber, biblionumber => $item->biblionumber, }; + $search_params->{found} = undef if $params->{ignore_found_holds}; + # Check for item on shelves and OnShelfHoldsAllowed + return { status => 'onShelfHoldsNotAllowed' } + unless IsAvailableForItemLevelRequest($item, $patron, $pickup_branchcode,1); + my $holds = Koha::Holds->search($search_params); if ( $opacitemholds eq "N" && $context ne 'staff' ) { @@ -1285,6 +1294,7 @@ checks with CanItemBeReserved or CanBookBeReserved. =cut + sub IsAvailableForItemLevelRequest { my $item = shift; my $patron = shift; @@ -1326,7 +1336,6 @@ sub IsAvailableForItemLevelRequest { if ( $on_shelf_holds == 1 ) { return 1; } elsif ( $on_shelf_holds == 2 ) { - # if we have this param predefined from outer caller sub, we just need # to return it, so we saving from having loop inside other loop: return $items_any_available ? 0 : 1 @@ -2284,7 +2293,7 @@ sub GetHoldRule { itemtype => $itemtype, categorycode => $categorycode, branchcode => $branchcode, - rules => ['holds_per_record', 'holds_per_day', 'opacitemholds'], + rules => ['holds_per_record', 'holds_per_day', 'opacitemholds', 'onshelfholds'], order_by => { -desc => [ 'categorycode', 'itemtype', 'branchcode' ] } @@ -2293,6 +2302,7 @@ sub GetHoldRule { $rules->{holds_per_record} = $holds_per_x_rules->{holds_per_record}; $rules->{holds_per_day} = $holds_per_x_rules->{holds_per_day}; $rules->{opacitemholds} = $holds_per_x_rules->{opacitemholds} // 'Y'; + $rules->{onshelfholds} = $holds_per_x_rules->{onshelfholds} // '0'; return $rules; } diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index de0ec3f337..7a7d92a15c 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 64; +use Test::More tests => 65; use Test::MockModule; use Test::Warn; @@ -187,6 +187,7 @@ Koha::CirculationRules->set_rules( rules => { reservesallowed => 25, holds_per_record => 1, + onshelfholds => 1, } } ); @@ -1058,10 +1059,10 @@ subtest 'reserves.item_level_hold' => sub { } ); - $canreserve = C4::Reserves::CanBookBeReserved( + $canreserve = C4::Reserves::CanBookBeReserved( $patron->borrowernumber, $item->biblionumber, - ); + ); is( $canreserve->{status}, 'OK', 'record-level holds should be possible with opacitemholds set to "Yes"' ); @@ -1075,6 +1076,7 @@ subtest 'reserves.item_level_hold' => sub { 'item-level holds should be possible with opacitemholds set to "Yes"' ); }; + subtest 'test opacitemholds rules in staff context' => sub { plan tests => 2; @@ -1112,6 +1114,76 @@ subtest 'reserves.item_level_hold' => sub { }; }; +subtest 'OnShelfHoldAllowed test' => sub { + plan tests => 3; + $dbh->do('DELETE FROM circulation_rules'); + my $biblio = $builder->build_sample_biblio({frameworkcode => $frameworkcode})->biblionumber; + + # Create a helper item instance for testing + my $item = $builder->build_sample_item({ biblionumber => $biblio, library => $branch_1, itype => $itemtype }); + + Koha::CirculationRules->set_rules( + { + branchcode => undef, + categorycode => undef, + itemtype => undef, + rules => { + reservesallowed => 25, + opacitemholds => 'Y', + onshelfholds => 1, + } + } + ); + + my $canreserve = C4::Reserves::CanItemBeReserved( + $patron->borrowernumber, + $item->itemnumber, + ); + + is( $canreserve->{status}, 'OK', + 'item-level holds should be possible with onshelfholdallowed set to "Yes"' ); + + Koha::CirculationRules->set_rules( + { + branchcode => undef, + categorycode => undef, + itemtype => undef, + rules => { + reservesallowed => 25, + opacitemholds => 'Y', + onshelfholds => '0', + } + }); + + $canreserve = C4::Reserves::CanItemBeReserved( + $patron->borrowernumber, + $item->itemnumber, + ); + + is( $canreserve->{status}, 'onShelfHoldsNotAllowed', + 'item-level holds should not be possible with onshelfholdallowed set to "If any unavailable"' ); + + Koha::CirculationRules->set_rules( + { + branchcode => undef, + categorycode => undef, + itemtype => undef, + rules => { + reservesallowed => 25, + opacitemholds => 'Y', + onshelfholds => '2', + } + }); + + $canreserve = C4::Reserves::CanItemBeReserved( + $patron->borrowernumber, + $item->itemnumber, + ); + + is( $canreserve->{status}, 'onShelfHoldsNotAllowed', + 'item-level holds should not be possible with onshelfholdallowed set to "If all unavailable"' ); +}; + subtest 'MoveReserve additional test' => sub { plan tests => 4; -- 2.11.0