From 51f0a0b7229c1e76699708a548a9e3e68c9953ad Mon Sep 17 00:00:00 2001 From: Olli-Antti Kivilahti Date: Mon, 20 Oct 2014 19:04:51 +0300 Subject: [PATCH] Bug 13116 - Make it possible to propagate errors from C4::Reserves::CanItemBeReserved() to the web-templates. This patch changes the way CanBookBeReserved() and CanItemBeReserved() return error messages and how they are dealt with in the templates. This change makes it possible to distinguish between different types of reservation failure. Currently only two types of errors are handled, all the way to the user, from the CanItemBeReserved(): -ageRestricted -tooManyReserves which translates to maxreserves ############# - TEST PLAN - ############# ((-- AGE RESTRICTION --)) STAFF CLIENT 1. Find a Record with Items, update the MARC Subfield 521a to "PEGI 16". 2. Get a Borrower who is younger than 16 years. 3. Place a hold for the underage Borrower for the ageRestricted Record. 4. You get a notification, that placing a hold on ageRestricted material is forbidden. (previously you just got a notification about maximum amount of reserves reached) ((-- MAXIMUM RESERVES REACHED --)) 0. Set the maxreserves -syspref to 3 (or any low value) STAFF CLIENT AND OPAC 1. Make a ton of reserves for one borrower. 2. Observe the notification about maximum reserves reached blocking your reservations. ((-- MULTIPLE HOLDS STAFF CLIENT --)) 3. Observe the error notification "Cannot place hold on some items" ((-- MULTIPLE HOLDS OPAC --)) 1. Make a search with many results, of which atleast one is age restricted to the current borrower. 2. Select few results and "Place hold" from to result summary header element. (Not individual results "Place hold") 3. Observe individual Biblios getting the "age restricted"-notification, where others can be reserved just fine. Updated the unit tests to match the new method return values. t/db_dependent/Holds.t & Reserves.t Followed test plan. Works as expected and displays meaningful messages for the reason why placing a hold is not possible. Signed-off-by: Marc Veron Signed-off-by: Tomas Cohen Arazi --- C4/ILSDI/Services.pm | 6 ++-- C4/Reserves.pm | 30 ++++++++++++------- .../prog/en/modules/reserve/request.tt | 5 +++- .../bootstrap/en/modules/opac-reserve.tt | 3 ++ opac/opac-reserve.pl | 17 +++++++---- reserve/request.pl | 17 +++++++++-- t/db_dependent/Holds.t | 16 +++++----- t/db_dependent/Reserves.t | 6 ++-- 8 files changed, 66 insertions(+), 34 deletions(-) diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index 45864ab3de..e33ef6edff 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -489,7 +489,7 @@ sub GetServices { # Reserve level management my $biblionumber = $item->{'biblionumber'}; my $canbookbereserved = CanBookBeReserved( $borrower, $biblionumber ); - if ($canbookbereserved) { + if ($canbookbereserved eq 'OK') { push @availablefor, 'title level hold'; my $canitembereserved = IsAvailableForItemLevelRequest($itemnumber); if ($canitembereserved) { @@ -609,7 +609,7 @@ sub HoldTitle { my $title = $$biblio{title}; # Check if the biblio can be reserved - return { code => 'NotHoldable' } unless CanBookBeReserved( $borrowernumber, $biblionumber ); + return { code => 'NotHoldable' } unless CanBookBeReserved( $borrowernumber, $biblionumber ) eq 'OK'; my $branch; @@ -687,7 +687,7 @@ sub HoldItem { # Check for item disponibility my $canitembereserved = C4::Reserves::CanItemBeReserved( $borrowernumber, $itemnumber ); my $canbookbereserved = C4::Reserves::CanBookBeReserved( $borrowernumber, $biblionumber ); - return { code => 'NotHoldable' } unless $canbookbereserved and $canitembereserved; + return { code => 'NotHoldable' } unless $canbookbereserved eq 'OK' and $canitembereserved eq 'OK'; # Pickup branch management my $branch; diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 28b7105088..f69d6accd5 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -448,7 +448,10 @@ sub GetReservesFromBorrowernumber { #------------------------------------------------------------------------------------- =head2 CanBookBeReserved - $error = &CanBookBeReserved($borrowernumber, $biblionumber) + $canReserve = &CanBookBeReserved($borrowernumber, $biblionumber) + if ($canReserve eq 'OK') { #We can reserve this Item! } + +See CanItemBeReserved() for possible return values. =cut @@ -462,17 +465,24 @@ sub CanBookBeReserved{ push (@$items,@hostitems); } + my $canReserve; foreach my $item (@$items){ - return 1 if CanItemBeReserved($borrowernumber, $item); + $canReserve = CanItemBeReserved($borrowernumber, $item); + return 'OK' if $canReserve eq 'OK'; } - return 0; + return $canReserve; } =head2 CanItemBeReserved - $error = &CanItemBeReserved($borrowernumber, $itemnumber) + $canReserve = &CanItemBeReserved($borrowernumber, $itemnumber) + if ($canReserve eq 'OK') { #We can reserve this Item! } -This function return 1 if an item can be issued by this borrower. +@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. =cut @@ -490,11 +500,11 @@ sub CanItemBeReserved{ my $borrower = C4::Members::GetMember('borrowernumber'=>$borrowernumber); # If an item is damaged and we don't allow holds on damaged items, we can stop right here - return 0 if ( $item->{damaged} && !C4::Context->preference('AllowHoldsOnDamagedItems') ); + return 'damaged' if ( $item->{damaged} && !C4::Context->preference('AllowHoldsOnDamagedItems') ); #Check for the age restriction my ($ageRestriction, $daysToAgeRestriction) = C4::Circulation::GetAgeRestriction( $biblioData->{agerestriction}, $borrower ); - return 0 if $daysToAgeRestriction && $daysToAgeRestriction > 0; + return 'ageRestricted' if $daysToAgeRestriction && $daysToAgeRestriction > 0; my $controlbranch = C4::Context->preference('ReservesControlBranch'); my $itemtypefield = C4::Context->preference('item-level_itypes') ? "itype" : "itemtype"; @@ -561,7 +571,7 @@ sub CanItemBeReserved{ # we check if it's ok or not if( $reservecount >= $allowedreserves ){ - return 0; + return 'tooManyReserves'; } # If reservecount is ok, we check item branch if IndependentBranches is ON @@ -571,11 +581,11 @@ sub CanItemBeReserved{ { my $itembranch = $item->{homebranch}; if ($itembranch ne $borrower->{branchcode}) { - return 0; + return 'cannotReserveFromOtherBranches'; } } - return 1; + return 'OK'; } =head2 CanReserveBeCanceledFromOpac 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 6c061e4611..ddec2b593f 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt @@ -241,7 +241,7 @@ function checkMultiHold() { [% ELSE %] -[% IF ( maxreserves || alreadyreserved || none_available || alreadypossession ) %] +[% IF ( maxreserves || alreadyreserved || none_available || alreadypossession || ageRestricted ) %]
[% UNLESS ( multi_hold ) %] @@ -250,6 +250,9 @@ function checkMultiHold() { [% IF ( maxreserves ) %]
  • Too many holds: [% borrowerfirstname %] [% borrowersurname %] has too many holds.
    • [% END %] + [% IF ( ageRestricted ) %] +
  • Age restricted
    • + [% END %] [% IF ( alreadyreserved ) %]
  • [% borrowerfirstname %] [% borrowersurname %] already has a hold on this item
    • [% END %] diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt index a0b86e8048..6558e6eaf4 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt @@ -143,6 +143,9 @@

    [% UNLESS ( bibitemloo.holdable ) %] + [% IF ( bibitemloo.ageRestricted ) %] +
    Sorry, you are too young to reserve this material.
    + [% END %] [% IF ( bibitemloo.already_reserved ) %]
    You have already requested this title.
    [% ELSE %] diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index ec63976d4e..4343936d8b 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -252,7 +252,7 @@ if ( $query->param('place_reserve') ) { # holdingbranch, force the value $rank and $found. my $rank = $biblioData->{rank}; if ( $itemNum ne '' ) { - $canreserve = 1 if CanItemBeReserved( $borrowernumber, $itemNum ); + $canreserve = 1 if CanItemBeReserved( $borrowernumber, $itemNum ) eq 'OK'; $rank = '0' unless C4::Context->preference('ReservesNeedReturns'); my $item = GetItem($itemNum); if ( $item->{'holdingbranch'} eq $branch ) { @@ -261,7 +261,7 @@ if ( $query->param('place_reserve') ) { } } else { - $canreserve = 1 if CanBookBeReserved( $borrowernumber, $biblioNum ); + $canreserve = 1 if CanBookBeReserved( $borrowernumber, $biblioNum ) eq 'OK'; # Inserts a null into the 'itemnumber' field of 'reserves' table. $itemNum = undef; @@ -524,7 +524,7 @@ foreach my $biblioNum (@biblionumbers) { $policy_holdallowed = 0; } - if (IsAvailableForItemLevelRequest($itemNum) and $policy_holdallowed and CanItemBeReserved($borrowernumber,$itemNum) and ($itemLoopIter->{already_reserved} ne 1)) { + if (IsAvailableForItemLevelRequest($itemNum) and $policy_holdallowed and CanItemBeReserved($borrowernumber,$itemNum) eq 'OK' and ($itemLoopIter->{already_reserved} ne 1)) { $itemLoopIter->{available} = 1; $numCopiesAvailable++; } @@ -548,9 +548,14 @@ foreach my $biblioNum (@biblionumbers) { if ($biblioLoopIter{already_reserved}) { $biblioLoopIter{holdable} = undef; } - if(not CanBookBeReserved($borrowernumber,$biblioNum)){ - $biblioLoopIter{holdable} = undef; - } + my $canReserve = CanBookBeReserved($borrowernumber,$biblioNum); + if ($canReserve eq 'OK') { + #All is OK! + } + else { + $biblioLoopIter{holdable} = undef; + $biblioLoopIter{ $canReserve } = 1; + } if(not C4::Context->preference('AllowHoldsOnPatronsPossessions') and CheckIfIssuedToPatron($borrowernumber,$biblioNum)) { $biblioLoopIter{holdable} = undef; $biblioLoopIter{already_patron_possession} = 1; diff --git a/reserve/request.pl b/reserve/request.pl index 14340c24b3..dd271e6b27 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -191,9 +191,20 @@ foreach my $biblionumber (@biblionumbers) { my $dat = GetBiblioData($biblionumber); - unless ( CanBookBeReserved($borrowerinfo->{borrowernumber}, $biblionumber) ) { - $maxreserves = 1; + my $canReserve = CanBookBeReserved($borrowerinfo->{borrowernumber}, $biblionumber); + if ($canReserve eq 'OK') { + #All is OK and we can continue + } + elsif ( $canReserve eq 'tooManyReserves' ) { + $maxreserves = 1; } + elsif ( $canReserve eq 'ageRestricted' ) { + $template->param( $canReserve => 1 ); + $biblioloopiter{ $canReserve } = 1; + } + else { + $biblioloopiter{ $canReserve } = 1; + } my $alreadypossession; if (not C4::Context->preference('AllowHoldsOnPatronsPossessions') and CheckIfIssuedToPatron($borrowerinfo->{borrowernumber},$biblionumber)) { @@ -415,7 +426,7 @@ foreach my $biblionumber (@biblionumbers) { && IsAvailableForItemLevelRequest($itemnumber) && CanItemBeReserved( $borrowerinfo->{borrowernumber}, $itemnumber - ) + ) eq 'OK' ) { $item->{available} = 1; diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index ec969bb668..3e2861c568 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -210,7 +210,7 @@ t::lib::Mocks::mock_preference('item-level_itypes', 1); # if IndependentBranches is OFF, a CPL patron can reserve an MPL item t::lib::Mocks::mock_preference('IndependentBranches', 0); ok( - CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber), + CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber) eq 'OK', 'CPL patron allowed to reserve MPL item with IndependentBranches OFF (bug 2394)' ); @@ -218,14 +218,14 @@ ok( t::lib::Mocks::mock_preference('IndependentBranches', 1); t::lib::Mocks::mock_preference('canreservefromotherbranches', 0); ok( - ! CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber), + CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber) eq 'cannotReserveFromOtherBranches', 'CPL patron NOT allowed to reserve MPL item with IndependentBranches ON ... (bug 2394)' ); # ... unless canreservefromotherbranches is ON t::lib::Mocks::mock_preference('canreservefromotherbranches', 1); ok( - CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber), + CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber) eq 'OK', '... unless canreservefromotherbranches is ON (bug 2394)' ); @@ -294,10 +294,10 @@ is( $reserve3->{priority}, 1, "After ModReserve, the 3rd reserve becomes the fir ModItem({ damaged => 1 }, $item_bibnum, $itemnumber); C4::Context->set_preference( 'AllowHoldsOnDamagedItems', 1 ); -ok( CanItemBeReserved( $borrowernumbers[0], $itemnumber), "Patron can reserve damaged item with AllowHoldsOnDamagedItems enabled" ); +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), "Patron cannot reserve damaged item with AllowHoldsOnDamagedItems disabled" ); +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" ); # Regression test for bug 9532 @@ -312,19 +312,19 @@ AddReserve( 1, ); ok( - !CanItemBeReserved( $borrowernumbers[0], $itemnumber), + CanItemBeReserved( $borrowernumbers[0], $itemnumber) eq 'tooManyReserves', "cannot request item if policy that matches on item-level item type forbids it" ); ModItem({ itype => 'CAN' }, $item_bibnum, $itemnumber); ok( - CanItemBeReserved( $borrowernumbers[0], $itemnumber), + CanItemBeReserved( $borrowernumbers[0], $itemnumber) 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), + CanItemBeReserved( $borrowernumbers[0], $itemnumber) eq 'tooManyReserves', "cannot request item if policy that matches on bib-level item type forbids it (bug 9532)" ); diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index e969982a4a..4676a60994 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -488,20 +488,20 @@ my ( $ageres_tagid, $ageres_subfieldid ) = GetMarcFromKohaField( "biblioitems.ag $record->append_fields( MARC::Field->new($ageres_tagid, '', '', $ageres_subfieldid => 'PEGI 16') ); C4::Biblio::ModBiblio( $record, $bibnum, '' ); -is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , 1, "Reserving an ageRestricted Biblio without a borrower dateofbirth succeeds" ); +is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , 'OK', "Reserving an ageRestricted Biblio without a borrower dateofbirth succeeds" ); #Set the dateofbirth for the Borrower making him "too young". my $now = DateTime->now(); C4::Members::SetAge( $borrower, '0015-00-00' ); C4::Members::ModMember( borrowernumber => $borrowernumber, dateofbirth => $borrower->{dateofbirth} ); -is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , 0, "Reserving a 'PEGI 16' Biblio by a 15 year old borrower fails"); +is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , 'ageRestricted', "Reserving a 'PEGI 16' Biblio by a 15 year old borrower fails"); #Set the dateofbirth for the Borrower making him "too old". C4::Members::SetAge( $borrower, '0030-00-00' ); C4::Members::ModMember( borrowernumber => $borrowernumber, dateofbirth => $borrower->{dateofbirth} ); -is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , 1, "Reserving a 'PEGI 16' Biblio by a 30 year old borrower succeeds"); +is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , 'OK', "Reserving a 'PEGI 16' Biblio by a 30 year old borrower succeeds"); #### ####### EO Bug 13113 <<< #### -- 2.20.1