From 2cebfd45c52aa9fc7bb63151a3a54e41d3cb44a7 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 +++++++++++++++++++++++++++++- reserve/request.pl | 6 +++-- t/db_dependent/Holds.t | 59 +++++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 96 insertions(+), 4 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 5fc68f1359..f57b428e1f 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -274,7 +274,23 @@ See CanItemBeReserved() for possible return values. sub CanBookBeReserved{ my ($borrowernumber, $biblionumber, $pickup_branchcode) = @_; + # 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 { status => 'alreadyReserved' }; + } + } + + # Check if borrower has reached the maximum number of holds allowed + my $maxreserves = C4::Context->preference('maxreserves'); + if ($maxreserves && $holds->count >= $maxreserves) { + return { status => 'tooManyReserves' }; + } + my @itemnumbers = Koha::Items->search({ biblionumber => $biblionumber})->get_column("itemnumber"); + #get items linked via host records my @hostitems = get_hostitemnumbers_of($biblionumber); if (@hostitems){ @@ -286,7 +302,8 @@ sub CanBookBeReserved{ $canReserve = CanItemBeReserved( $borrowernumber, $itemnumber, $pickup_branchcode ); return { status => 'OK' } if $canReserve->{status} eq 'OK'; } - return $canReserve; + + return { status => 'none_available' }; } =head2 CanItemBeReserved @@ -336,6 +353,22 @@ sub CanItemBeReserved { return { status =>'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/reserve/request.pl b/reserve/request.pl index df9cae861d..3d6a1e4706 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -240,8 +240,10 @@ foreach my $biblionumber (@biblionumbers) { $exceeded_holds_per_record = 1; $biblioloopiter{ $canReserve->{status} } = 1; } - elsif ( $canReserve->{status} eq 'ageRestricted' ) { - $template->param( $canReserve->{status} => 1 ); + elsif ( grep { $canReserve->{status} eq $_ } + (qw(ageRestricted alreadyReserved none_available)) ) + { + $template->param( $canReserve->{status} => 1 ); $biblioloopiter{ $canReserve->{status} } = 1; } else { diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index 1515b4bef4..a5d9aa42ac 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -299,7 +299,10 @@ ok( is( $hold3->discard_changes->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)->{status}, 'OK', "Patron can reserve damaged item with AllowHoldsOnDamagedItems enabled" ); ok( defined( ( CheckReserves($itemnumber) )[1] ), "Hold can be trapped for damaged item with AllowHoldsOnDamagedItems enabled" ); @@ -330,23 +333,48 @@ 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( +<<<<<<< HEAD CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status}, 'tooManyReserves', +======= + CanItemBeReserved( $borrowernumbers[0], $itemnumber2), 'tooManyReserves', +>>>>>>> Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved "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( +<<<<<<< HEAD CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status} eq 'OK', +======= + CanItemBeReserved( $borrowernumbers[0], $itemnumber2) eq 'OK', +>>>>>>> Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved "can request item if policy that matches on item type allows it" ); t::lib::Mocks::mock_preference('item-level_itypes', 0); +<<<<<<< HEAD ModItem({ itype => undef }, $item_bibnum, $itemnumber); ok( CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status} eq 'tooManyReserves', +======= +ModItem({ itype => undef }, $item_bibnum2, $itemnumber2); +is( + CanItemBeReserved( $borrowernumbers[0], $itemnumber2), 'tooManyReserves', +>>>>>>> Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved "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 @@ -415,8 +443,13 @@ is( CanItemBeReserved( $borrowernumbers[0], $itemnumber )->{status}, my $res_id = AddReserve( $branch_1, $borrowernumbers[0], $biblio->biblionumber, '', 1, ); +<<<<<<< HEAD is( CanItemBeReserved( $borrowernumbers[0], $itemnumber )->{status}, 'tooManyReserves', 'Patron cannot reserve item with hold limit of 1, 1 bib level hold placed' ); +======= +is( CanItemBeReserved( $borrowernumbers[0], $itemnumber ), + 'tooManyHoldsForThisRecord', 'Patron cannot reserve item with hold limit of 1, 1 bib level hold placed' ); +>>>>>>> Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved #results should be the same for both ReservesControlBranch settings t::lib::Mocks::mock_preference( 'ReservesControlBranch', 'ItemHomeLibrary' ); @@ -425,6 +458,7 @@ is( CanItemBeReserved( $borrowernumbers[0], $itemnumber )->{status}, #reset for further tests t::lib::Mocks::mock_preference( 'ReservesControlBranch', 'PatronLibrary' ); +<<<<<<< HEAD subtest 'Test max_holds per library/patron category' => sub { plan tests => 6; @@ -675,3 +709,26 @@ subtest 'CanItemBeReserved / holds_per_day tests' => sub { $schema->storage->txn_rollback; }; +======= +# Helper method to set up a Biblio. +sub create_helper_biblio { + my $itemtype = shift; + my $bib = MARC::Record->new(); + my $title = 'Silence in the library'; + 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, ''); +} +>>>>>>> Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved -- 2.11.0