From 10a8e6f042db220fdccc187a067f60f62f426a90 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 | 45 +++++++++++++++- .../prog/en/modules/reserve/request.tt | 2 +- reserve/request.pl | 6 ++- t/db_dependent/Holds.t | 61 +++++++++++++++++++++- 4 files changed, 108 insertions(+), 6 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index e2633753c8..be8248c872 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 @@ -294,6 +311,7 @@ sub CanBookBeReserved{ $canReserve = &CanItemBeReserved($borrowernumber, $itemnumber, $branchcode) if ($canReserve->{status} eq 'OK') { #We can reserve this Item! } +<<<<<<< HEAD @RETURNS { status => OK }, if the Item can be reserved. { status => ageRestricted }, if the Item is age restricted for this borrower. { status => damaged }, if the Item is damaged. @@ -303,6 +321,15 @@ sub CanBookBeReserved{ { status => libraryNotFound }, if given branchcode is not an existing library { status => libraryNotPickupLocation }, if given branchcode is not configured to be a pickup location { status => cannotBeTransferred }, if branch transfer limit applies on given item and branchcode +======= +@RETURNS OK, if the Item can be reserved. + ageRestricted, if the Item is age restricted for this borrower. + damaged, if the Item is damaged. + 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. +>>>>>>> Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved =cut @@ -336,6 +363,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/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt index 36d6a30c7f..f1006c3f4c 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt @@ -90,7 +90,7 @@ [% 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 ) %] diff --git a/reserve/request.pl b/reserve/request.pl index 486c770c75..bf9f710bec 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -235,8 +235,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 e212744b43..e8aeea8080 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; @@ -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 @@ -410,8 +438,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' ); @@ -420,6 +453,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; @@ -670,3 +704,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