From 9a8c3cb61c324ff90b2ab017dc42845b34cdc6c5 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 22 Aug 2014 13:29:28 -0400 Subject: [PATCH] Bug 12632: Hold limits ignored for record level holds with item level itemtypes The crux of the issue is that if you are using item level itemtypes, but are allowing biblio levels holds, those holds do not have items. So, in CanItemBeReserved, when Koha counts the number of holds to compare against the given rule, it will always give 0 ( except of course for found holds, and the occasional item-level hold ). So the query is saying "link each of these reserves to the reserved item, and count the number of reserves this patron where the itemtype is DVD". However, since these are all record level reserves, there are no items to link to, and so when it looks for all reserves this and item whose itemtype is DVD, it finds zero reserves! This patch solves the problem by looking first at the item level itemtype, and if it does not exist, then it looks at the record level itemtype. For installations using record level itemtypes, the behavior remains unchanged. Test plan: 1) Enable item level itemtypes 2) Create two records with one item each of a given itemtype 3) Create a single issuing rule and limit the holds allowed for that itemtype to 1 4) Place a record level hold on your first record 5) Attempt to place a record level hold for the same patron on your second record. You should not be able to but you can! 6) Apply this patch 7) Repeat step 5, note you can no longer place the hold! Signed-off-by: Paola Rossi Signed-off-by: Jonathan Druart Signed-off-by: Tomas Cohen Arazi (cherry picked from commit 6f95acd1a6cf5113992d28173dece96e8afc6cef) Signed-off-by: Chris Cormack --- C4/Reserves.pm | 10 ++++++++-- t/db_dependent/Holds.t | 38 +++++++++++++++++++++++++++++++++----- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 9c760b6e28..9eac77afd5 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -513,7 +513,6 @@ sub CanItemBeReserved{ return 'ageRestricted' if $daysToAgeRestriction && $daysToAgeRestriction > 0; my $controlbranch = C4::Context->preference('ReservesControlBranch'); - my $itemtypefield = C4::Context->preference('item-level_itypes') ? "itype" : "itemtype"; # we retrieve user rights on this itemtype and branchcode my $sth = $dbh->prepare("SELECT categorycode, itemtype, branchcode, reservesallowed @@ -561,7 +560,14 @@ sub CanItemBeReserved{ $querycount .= "AND $branchfield = ?"; - $querycount .= " AND $itemtypefield = ?" if ($ruleitemtype ne "*"); + # If using item-level itypes, fall back to the record + # level itemtype if the hold has no associated item + $querycount .= + C4::Context->preference('item-level_itypes') + ? " AND COALESCE( itype, itemtype ) = ?" + : " AND itemtype = ?" + if ( $ruleitemtype ne "*" ); + my $sthcount = $dbh->prepare($querycount); if($ruleitemtype eq "*"){ diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index 5a480d81d2..18a4ef5b8c 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -6,7 +6,7 @@ use t::lib::Mocks; use C4::Context; use C4::Branch; -use Test::More tests => 41; +use Test::More tests => 51; use MARC::Record; use C4::Biblio; use C4::Items; @@ -34,6 +34,7 @@ my $insert_sth = $dbh->prepare('INSERT INTO itemtypes (itemtype) VALUES (?)'); $insert_sth->execute('CAN'); $insert_sth->execute('CANNOT'); $insert_sth->execute('DUMMY'); +$insert_sth->execute('ONLY1'); # Setup Test------------------------ # Create a biblio instance for testing @@ -193,19 +194,19 @@ my ($foreign_item_bibnum, $foreign_item_bibitemnum, $foreign_itemnumber) $dbh->do('DELETE FROM issuingrules'); $dbh->do( q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed) - VALUES (?, ?, ?, ?)}, + VALUES (?, ?, ?, ?)}, {}, '*', '*', '*', 25 ); $dbh->do( q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed) - VALUES (?, ?, ?, ?)}, + VALUES (?, ?, ?, ?)}, {}, - '*', '*', 'CANNOT', 0 + '*', '*', 'CANNOT', 0 ); # make sure some basic sysprefs are set -t::lib::Mocks::mock_preference('ReservesControlBranch', 'homebranch'); +t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); t::lib::Mocks::mock_preference('item-level_itypes', 1); # if IndependentBranches is OFF, a CPL patron can reserve an MPL item @@ -405,6 +406,33 @@ CancelExpiredReserves(); $count = $dbh->selectrow_array("SELECT COUNT(*) FROM reserves WHERE reserve_id = ?", undef, $reserve_id ); is( $count, 0, "Reserve with manual expiration date canceled correctly" ); +# Bug 12632 +t::lib::Mocks::mock_preference( 'item-level_itypes', 1 ); +t::lib::Mocks::mock_preference( 'ReservesControlBranch', 'PatronLibrary' ); + +$dbh->do('DELETE FROM reserves'); +$dbh->do('DELETE FROM issues'); +$dbh->do('DELETE FROM items'); +$dbh->do('DELETE FROM biblio'); + +( $bibnum, $title, $bibitemnum ) = create_helper_biblio('ONLY1'); +( $item_bibnum, $item_bibitemnum, $itemnumber ) = AddItem( { homebranch => 'CPL', holdingbranch => 'CPL' }, $bibnum ); + +$dbh->do( + q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed) + VALUES (?, ?, ?, ?)}, + {}, + '*', '*', 'ONLY1', 1 +); +is( CanItemBeReserved( $borrowernumbers[0], $itemnumber ), + 'OK', 'Patron can reserve item with hold limit of 1, no holds placed' ); + +my $res_id = AddReserve( $branch, $borrowernumbers[0], $bibnum, 'a', '', 1, ); + +is( CanItemBeReserved( $borrowernumbers[0], $itemnumber ), + 'tooManyReserves', 'Patron cannot reserve item with hold limit of 1, 1 bib level hold placed' ); + + # Helper method to set up a Biblio. sub create_helper_biblio { my $itemtype = shift; -- 2.39.5