From a6d0ad958c57a326af8464d993b75a34bea12584 Mon Sep 17 00:00:00 2001 From: Julian Maurice Date: Wed, 26 Mar 2014 10:51:59 +0100 Subject: [PATCH] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved - Check if borrower already has reserved the same biblio (or item). - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower 3/ Try to hold the same biblio/item with the same borrower through ILS-DI and note that you can Apply the patch 4/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 5/ Try to hold the same biblio/item with the same borrower through ILS-DI. You shouldn't be able to do that. 6/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Signed-off-by: Alex Sassmannshausen --- C4/Reserves.pm | 36 ++++++++++++++- .../prog/en/modules/reserve/request.tt | 4 +- reserve/request.pl | 17 ++----- t/db_dependent/Holds.t | 54 +++++++++++++++------- 4 files changed, 77 insertions(+), 34 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 2beea87..bc1af9e 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -463,6 +463,20 @@ See CanItemBeReserved() for possible return values. sub CanBookBeReserved{ my ($borrowernumber, $biblionumber) = @_; + # Check if borrower already has reserved the same biblio. + my @reserves = GetReservesFromBorrowernumber($borrowernumber); + foreach my $reserve (@reserves) { + if ($reserve->{biblionumber} == $biblionumber) { + return 'alreadyReserved'; + } + } + + # Check if borrower has reached the maximum number of holds allowed + my $maxreserves = C4::Context->preference('maxreserves'); + if ($maxreserves && scalar(@reserves) >= $maxreserves) { + return 'tooManyReserves'; + } + my $items = GetItemnumbersForBiblio($biblionumber); #get items linked via host records my @hostitems = get_hostitemnumbers_of($biblionumber); @@ -475,7 +489,8 @@ sub CanBookBeReserved{ $canReserve = CanItemBeReserved( $borrowernumber, $item ); return 'OK' if $canReserve eq 'OK'; } - return $canReserve; + + return 'none_available'; } =head2 CanItemBeReserved @@ -489,6 +504,7 @@ sub CanBookBeReserved{ cannotReserveFromOtherBranches, if syspref 'canreservefromotherbranches' is OK. tooManyReserves, if the borrower has exceeded his maximum reserve amount. notReservable, if holds on this item are not allowed + alreadyReserved, if the borrower has already reserved this item. =cut @@ -498,7 +514,7 @@ sub CanItemBeReserved{ my $dbh = C4::Context->dbh; my $ruleitemtype; # itemtype of the matching issuing rule my $allowedreserves = 0; - + # we retrieve borrowers and items informations # # item->{itype} will come for biblioitems if necessery my $item = GetItem($itemnumber); @@ -512,6 +528,22 @@ sub CanItemBeReserved{ my ($ageRestriction, $daysToAgeRestriction) = C4::Circulation::GetAgeRestriction( $biblioData->{agerestriction}, $borrower ); return 'ageRestricted' if $daysToAgeRestriction && $daysToAgeRestriction > 0; + # Check if borrower already has reserved the same item or biblio. + my @reserves = GetReservesFromBorrowernumber($borrowernumber); + foreach my $reserve (@reserves) { + if ( $reserve->{itemnumber} == $itemnumber + or $reserve->{biblionumber} == $item->{biblionumber} ) + { + return 'alreadyReserved'; + } + } + + # Check if borrower has reached the maximum number of holds allowed + my $maxreserves = C4::Context->preference('maxreserves'); + if ($maxreserves && scalar(@reserves) >= $maxreserves) { + return 'tooManyReserves'; + } + my $controlbranch = C4::Context->preference('ReservesControlBranch'); my $itemtypefield = C4::Context->preference('item-level_itypes') ? "itype" : "itemtype"; 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 80be359..666cbfa 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt @@ -297,7 +297,7 @@ function checkMultiHold() { [% ELSE %] -[% IF ( maxreserves || alreadyreserved || none_available || alreadypossession || ageRestricted ) %] +[% IF ( maxreserves || alreadyReserved || none_available || alreadypossession || ageRestricted ) %]
[% UNLESS ( multi_hold ) %] @@ -309,7 +309,7 @@ function checkMultiHold() { [% IF ( ageRestricted ) %]
  • Age restricted
  • [% END %] - [% IF ( alreadyreserved ) %] + [% IF ( alreadyReserved ) %]
  • [% borrowerfirstname %] [% borrowersurname %] already has a hold on this item
  • [% END %] [% IF ( none_available ) %] diff --git a/reserve/request.pl b/reserve/request.pl index 62f82a8..7b2741c 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -208,7 +208,9 @@ foreach my $biblionumber (@biblionumbers) { elsif ( $canReserve eq 'tooManyReserves' ) { $maxreserves = 1; } - elsif ( $canReserve eq 'ageRestricted' ) { + elsif ( grep { $canReserve eq $_ } + (qw(ageRestricted alreadyReserved none_available)) ) + { $template->param( $canReserve => 1 ); $biblioloopiter{$canReserve} = 1; } @@ -225,27 +227,14 @@ foreach my $biblionumber (@biblionumbers) { my $reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 }); my $count = scalar( @$reserves ); my $totalcount = $count; - my $holds_count = 0; - my $alreadyreserved = 0; foreach my $res (@$reserves) { if ( defined $res->{found} ) { # found can be 'W' or 'T' $count--; } - - if ( defined $borrowerinfo && defined($borrowerinfo->{borrowernumber}) && ($borrowerinfo->{borrowernumber} eq $res->{borrowernumber}) ) { - $holds_count++; - } - } - - if ( $holds_count ) { - $alreadyreserved = 1; - $biblioloopiter{warn} = 1; - $biblioloopiter{alreadyres} = 1; } $template->param( - alreadyreserved => $alreadyreserved, alreadypossession => $alreadypossession, ); diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index 01e67db..cf86e2a 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 => 49; +use Test::More tests => 52; use MARC::Record; use C4::Biblio; use C4::Items; @@ -59,8 +59,8 @@ foreach (1..$borrowers_count) { my $biblionumber = $bibnum; -my @branches = GetBranchesLoop(); -my $branch = $branches[0][0]{value}; +my $branches = GetBranchesLoop(); +my $branch = $branches->[0]{value}; # Create five item level holds foreach my $borrowernumber ( @borrowernumbers ) { @@ -222,7 +222,7 @@ $dbh->do( ); # 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 @@ -310,10 +310,12 @@ ModReserve({ reserve_id => $reserveid2, rank => 'del' }); $reserve3 = GetReserve( $reserveid3 ); is( $reserve3->{priority}, 1, "After ModReserve, the 3rd reserve becomes the first on the waiting list" ); +ok( defined( ( CheckReserves($itemnumber) )[1] ), "Hold can be trapped for damaged item with AllowHoldsOnDamagedItems enabled" ); + ModItem({ damaged => 1 }, $item_bibnum, $itemnumber); +CancelReserve({reserve_id => $reserveid3}); C4::Context->set_preference( 'AllowHoldsOnDamagedItems', 1 ); ok( CanItemBeReserved( $borrowernumbers[0], $itemnumber) eq 'OK', "Patron can reserve damaged item with AllowHoldsOnDamagedItems enabled" ); -ok( defined( ( CheckReserves($itemnumber) )[1] ), "Hold can be trapped for damaged item with AllowHoldsOnDamagedItems enabled" ); C4::Context->set_preference( 'AllowHoldsOnDamagedItems', 0 ); ok( CanItemBeReserved( $borrowernumbers[0], $itemnumber) eq 'damaged', "Patron cannot reserve damaged item with AllowHoldsOnDamagedItems disabled" ); ok( !defined( ( CheckReserves($itemnumber) )[1] ), "Hold cannot be trapped for damaged item with AllowHoldsOnDamagedItems disabled" ); @@ -329,23 +331,34 @@ AddReserve( '', 1, ); +my ($bibnum2, $title2, $bibitemnum2) = create_helper_biblio('CANNOT'); +my ($item_bibnum2, $item_bibitemnum2, $itemnumber2) = AddItem({ homebranch => $branch, holdingbranch => $branch, itype => 'CANNOT' } , $bibnum2); ok( - CanItemBeReserved( $borrowernumbers[0], $itemnumber) eq 'tooManyReserves', + CanItemBeReserved( $borrowernumbers[0], $itemnumber2) eq 'tooManyReserves', "cannot request item if policy that matches on item-level item type forbids it" ); -ModItem({ itype => 'CAN' }, $item_bibnum, $itemnumber); +ModItem({ itype => 'CAN' }, $item_bibnum2, $itemnumber2); ok( - CanItemBeReserved( $borrowernumbers[0], $itemnumber) eq 'OK', + CanItemBeReserved( $borrowernumbers[0], $itemnumber2) eq 'OK', "can request item if policy that matches on item type allows it" ); t::lib::Mocks::mock_preference('item-level_itypes', 0); -ModItem({ itype => undef }, $item_bibnum, $itemnumber); +ModItem({ itype => undef }, $item_bibnum2, $itemnumber2); ok( - CanItemBeReserved( $borrowernumbers[0], $itemnumber) eq 'tooManyReserves', + CanItemBeReserved( $borrowernumbers[0], $itemnumber2) eq 'tooManyReserves', "cannot request item if policy that matches on bib-level item type forbids it (bug 9532)" ); +is(CanBookBeReserved($borrowernumbers[0], $bibnum), 'alreadyReserved'); + +C4::Context->set_preference('maxreserves', 1); +ok(CanBookBeReserved($borrowernumbers[0], $biblionumber) eq 'tooManyReserves'); + +C4::Context->set_preference('maxreserves', 0); +t::lib::Mocks::mock_preference('IndependentBranches', 1); +t::lib::Mocks::mock_preference('canreservefromotherbranches', 0); +ok(CanBookBeReserved($borrowernumbers[0], $foreign_bibnum) eq 'none_available'); # Test branch item rules @@ -427,10 +440,19 @@ sub create_helper_biblio { my $itemtype = shift; my $bib = MARC::Record->new(); my $title = 'Silence in the library'; - $bib->append_fields( - MARC::Field->new('100', ' ', ' ', a => 'Moffat, Steven'), - MARC::Field->new('245', ' ', ' ', a => $title), - MARC::Field->new('942', ' ', ' ', c => $itemtype), - ); - return ($bibnum, $title, $bibitemnum) = AddBiblio($bib, ''); + if (C4::Context->preference('marcflavour') eq 'UNIMARC') { + $bib->append_fields( + MARC::Field->new('700', ' ', '0', a => 'Moffat, Steven'), + MARC::Field->new('200', ' ', ' ', a => $title), + MARC::Field->new('099', ' ', ' ', t => $itemtype), + ); + } else { + $bib->append_fields( + MARC::Field->new('100', ' ', ' ', a => 'Moffat, Steven'), + MARC::Field->new('245', ' ', ' ', a => $title), + MARC::Field->new('942', ' ', ' ', c => $itemtype), + ); + } + + return AddBiblio($bib, ''); } -- 1.9.1