From 0d20cf9bd03b83f2d50b84dd2c7c4eaf34977dcf 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 | 35 ++++++++++++++- .../prog/en/modules/reserve/request.tt | 4 +- reserve/request.pl | 4 +- t/db_dependent/Holds.t | 52 +++++++++++++++------- 4 files changed, 76 insertions(+), 19 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 50982306a1..2c355974c9 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -291,6 +291,21 @@ See CanItemBeReserved() for possible return values. sub CanBookBeReserved{ my ($borrowernumber, $biblionumber) = @_; + # Check if borrower already has reserved the same biblio. + my $patron = Koha::Patrons->find($borrowernumber); + my $holds = $patron->holds; + while (my $hold = $holds->next) { + if ($hold->biblionumber == $biblionumber) { + return 'alreadyReserved'; + } + } + + # Check if borrower has reached the maximum number of holds allowed + my $maxreserves = C4::Context->preference('maxreserves'); + if ($maxreserves && $holds->count >= $maxreserves) { + return 'tooManyReserves'; + } + my $items = GetItemnumbersForBiblio($biblionumber); #get items linked via host records my @hostitems = get_hostitemnumbers_of($biblionumber); @@ -303,7 +318,8 @@ sub CanBookBeReserved{ $canReserve = CanItemBeReserved( $borrowernumber, $item ); return 'OK' if $canReserve eq 'OK'; } - return $canReserve; + + return 'none_available'; } =head2 CanItemBeReserved @@ -317,6 +333,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 @@ -349,6 +366,22 @@ sub CanItemBeReserved { return 'itemAlreadyOnHold' if Koha::Holds->search( { borrowernumber => $borrowernumber, itemnumber => $itemnumber } )->count(); + # Check if borrower already has reserved the same item or biblio. + my $holds = $patron->holds; + while (my $hold = $holds->next) { + if ( $hold->itemnumber == $itemnumber + or $hold->biblionumber == $item->{biblionumber} ) + { + return 'alreadyReserved'; + } + } + + # Check if borrower has reached the maximum number of holds allowed + my $maxreserves = C4::Context->preference('maxreserves'); + if ($maxreserves && $holds->count >= $maxreserves) { + return 'tooManyReserves'; + } + my $controlbranch = C4::Context->preference('ReservesControlBranch'); my $querycount = q{ 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 27dd2de440..00baa836e5 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt @@ -270,7 +270,7 @@ function checkMultiHold() { [% ELSIF NOT noitems %] -[% IF ( exceeded_maxreserves || exceeded_holds_per_record || alreadyreserved || none_available || alreadypossession || ageRestricted ) %] +[% IF ( exceeded_maxreserves || exceeded_holds_per_record || alreadyReserved || none_available || alreadypossession || ageRestricted ) %]
[% UNLESS ( multi_hold ) %] @@ -282,7 +282,7 @@ function checkMultiHold() {
  • Too many holds for this record: [% borrowerfirstname %] [% borrowersurname %] can only place a maximum of [% max_holds_for_record %] hold(s) on this record.
  • [% 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 7cda03dce2..a9f38c4da2 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -233,7 +233,9 @@ foreach my $biblionumber (@biblionumbers) { $exceeded_holds_per_record = 1; $biblioloopiter{$canReserve} = 1; } - elsif ( $canReserve eq 'ageRestricted' ) { + elsif ( grep { $canReserve eq $_ } + (qw(ageRestricted alreadyReserved none_available)) ) + { $template->param( $canReserve => 1 ); $biblioloopiter{$canReserve} = 1; } diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index ad87b26129..ce7a01f686 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -7,7 +7,7 @@ use t::lib::TestBuilder; use C4::Context; -use Test::More tests => 58; +use Test::More tests => 61; use MARC::Record; use C4::Biblio; use C4::Items; @@ -348,10 +348,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 ); is( CanItemBeReserved( $borrowernumbers[0], $itemnumber), 'OK', "Patron can reserve damaged item with AllowHoldsOnDamagedItems enabled" ); -ok( defined( ( CheckReserves($itemnumber) )[1] ), "Hold can be trapped for damaged item with AllowHoldsOnDamagedItems enabled" ); $hold = Koha::Hold->new( { @@ -379,23 +381,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); is( - CanItemBeReserved( $borrowernumbers[0], $itemnumber), 'tooManyReserves', + CanItemBeReserved( $borrowernumbers[0], $itemnumber2), '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); -ok( - CanItemBeReserved( $borrowernumbers[0], $itemnumber) eq 'tooManyReserves', +ModItem({ itype => undef }, $item_bibnum2, $itemnumber2); +is( + CanItemBeReserved( $borrowernumbers[0], $itemnumber2), 'tooManyReserves', "cannot request item if policy that matches on bib-level item type forbids it (bug 9532)" ); +is(CanBookBeReserved($borrowernumbers[0], $bibnum), 'OK'); + +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 @@ -460,7 +473,7 @@ is( CanItemBeReserved( $borrowernumbers[0], $itemnumber ), my $res_id = AddReserve( $branch_1, $borrowernumbers[0], $bibnum, '', 1, ); is( CanItemBeReserved( $borrowernumbers[0], $itemnumber ), - 'tooManyReserves', 'Patron cannot reserve item with hold limit of 1, 1 bib level hold placed' ); + 'tooManyHoldsForThisRecord', 'Patron cannot reserve item with hold limit of 1, 1 bib level hold placed' ); # Helper method to set up a Biblio. @@ -468,10 +481,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.11.0