From 738edd06348ad53c9e9220a0b918ee093cef2df3 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 | 48 ++++++++++++++++------ 4 files changed, 74 insertions(+), 31 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 4b10ff9..a14a222 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -412,6 +412,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); @@ -424,7 +438,8 @@ sub CanBookBeReserved{ $canReserve = CanItemBeReserved( $borrowernumber, $item ); return 'OK' if $canReserve eq 'OK'; } - return $canReserve; + + return 'none_available'; } =head2 CanItemBeReserved @@ -438,6 +453,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 @@ -447,7 +463,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); @@ -461,6 +477,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'); # we retrieve user rights on this itemtype and branchcode 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 bf85bf6..3be1474 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt @@ -298,7 +298,7 @@ function checkMultiHold() { [% ELSE %] -[% IF ( exceeded_maxreserves || alreadyreserved || none_available || alreadypossession || ageRestricted ) %] +[% IF ( exceeded_maxreserves || alreadyReserved || none_available || alreadypossession || ageRestricted ) %]
[% UNLESS ( multi_hold ) %] @@ -308,7 +308,7 @@ function checkMultiHold() {
  • Too many holds: [% borrowerfirstname %] [% borrowersurname %] can only place a maximum of [% maxreserves %] total holds.
  • [% ELSIF ( alreadypossession ) %]
  • [% borrowerfirstname %] [% borrowersurname %] is already in possession of one item
  • - [% ELSIF ( alreadyreserved ) %] + [% ELSIF ( alreadyReserved ) %]
  • [% borrowerfirstname %] [% borrowersurname %] already has a hold on this item
  • [% ELSIF ( ageRestricted ) %]
  • Age restricted
  • diff --git a/reserve/request.pl b/reserve/request.pl index 80cf0d6..40f7f96 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -226,7 +226,9 @@ foreach my $biblionumber (@biblionumbers) { elsif ( $canReserve eq 'tooManyReserves' ) { $exceeded_maxreserves = 1; } - elsif ( $canReserve eq 'ageRestricted' ) { + elsif ( grep { $canReserve eq $_ } + (qw(ageRestricted alreadyReserved none_available)) ) + { $template->param( $canReserve => 1 ); $biblioloopiter{$canReserve} = 1; } @@ -243,27 +245,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 f219f87..e7df136 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -8,7 +8,7 @@ use t::lib::TestBuilder; use C4::Context; use C4::Branch; -use Test::More tests => 56; +use Test::More tests => 59; use MARC::Record; use C4::Biblio; use C4::Items; @@ -314,10 +314,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" ); @@ -332,23 +334,34 @@ AddReserve( '', 1, ); +my ($bibnum2, $title2, $bibitemnum2) = create_helper_biblio('CANNOT'); +my ($item_bibnum2, $item_bibitemnum2, $itemnumber2) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1, 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 @@ -458,10 +471,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