From 1cb992edc3beb95c82315a4c776987f83db00d22 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. Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Sassmannshausen Signed-off-by: Alex Arnaud --- 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 fbeca06..c3ff373 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -417,6 +417,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); @@ -429,7 +443,8 @@ sub CanBookBeReserved{ $canReserve = CanItemBeReserved( $borrowernumber, $item ); return 'OK' if $canReserve eq 'OK'; } - return $canReserve; + + return 'none_available'; } =head2 CanItemBeReserved @@ -443,6 +458,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 @@ -452,7 +468,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); @@ -466,6 +482,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 473cfb4..6181d61 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt @@ -299,7 +299,7 @@ function checkMultiHold() { [% ELSIF NOT noitems %] -[% IF ( exceeded_maxreserves || alreadyreserved || none_available || alreadypossession || ageRestricted ) %] +[% IF ( exceeded_maxreserves || alreadyReserved || none_available || alreadypossession || ageRestricted ) %]
[% UNLESS ( multi_hold ) %] @@ -309,7 +309,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 970596f..7e8cddd 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 4ba95df..d400b47 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 => 60; +use Test::More tests => 63; use MARC::Record; use C4::Biblio; use C4::Items; @@ -333,10 +333,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}); t::lib::Mocks::mock_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" ); t::lib::Mocks::mock_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" ); @@ -351,23 +353,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 @@ -477,10 +490,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, ''); } -- 2.1.4