Bugzilla – Attachment 32627 Details for
Bug 13116
Make it possible to propagate errors from C4::Reserves::CanItemBeReserved() to the web-templates.
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 13116 - Make it possible to propagate errors from C4::Reserves::CanItemBeReserved() to the web-templates.
Bug-13116---Make-it-possible-to-propagate-errors-f.patch (text/plain), 17.92 KB, created by
Olli-Antti Kivilahti
on 2014-10-23 14:46:17 UTC
(
hide
)
Description:
Bug 13116 - Make it possible to propagate errors from C4::Reserves::CanItemBeReserved() to the web-templates.
Filename:
MIME Type:
Creator:
Olli-Antti Kivilahti
Created:
2014-10-23 14:46:17 UTC
Size:
17.92 KB
patch
obsolete
>From 52c95c16ffb04add595b3b3483f284268388717b Mon Sep 17 00:00:00 2001 >From: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> >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 >--- > C4/ILSDI/Services.pm | 6 ++-- > C4/Reserves.pm | 30 +++++++++++++------- > .../prog/en/modules/reserve/request.tt | 5 +++- > .../opac-tmpl/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 0b02bc7..a97c626 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 97990ac..19a8e97 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 6c061e4..ddec2b5 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() { > </form> > [% ELSE %] > >-[% IF ( maxreserves || alreadyreserved || none_available || alreadypossession ) %] >+[% IF ( maxreserves || alreadyreserved || none_available || alreadypossession || ageRestricted ) %] > <div class="dialog alert"> > > [% UNLESS ( multi_hold ) %] >@@ -250,6 +250,9 @@ function checkMultiHold() { > [% IF ( maxreserves ) %] > <li><strong>Too many holds: </strong> <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% borrowernumber %]">[% borrowerfirstname %] [% borrowersurname %] </a> has too many holds.</li> > [% END %] >+ [% IF ( ageRestricted ) %] >+ <li><strong>Age restricted</strong></li> >+ [% END %] > [% IF ( alreadyreserved ) %] > <li><a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% borrowernumber %]">[% borrowerfirstname %] [% borrowersurname %]</a> <strong>already has a hold</strong> on this item </li> > [% 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 a0b86e8..6558e6e 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 @@ > </p> > > [% UNLESS ( bibitemloo.holdable ) %] >+ [% IF ( bibitemloo.ageRestricted ) %] >+ <div class="alert">Sorry, you are too young to reserve this material.</div> >+ [% END %] > [% IF ( bibitemloo.already_reserved ) %] > <div class="alert">You have already requested this title.</div> > [% ELSE %] >diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl >index 45d03bd..b0e366c 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 14340c2..dd271e6 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 ec969bb..3e2861c 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 0bc763b..b666dfe 100755 >--- a/t/db_dependent/Reserves.t >+++ b/t/db_dependent/Reserves.t >@@ -486,7 +486,7 @@ 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(); >@@ -494,14 +494,14 @@ my $duration_15years = DateTime::Duration->new(years => 15); > my $past15yearsAgo = DateTime->now()->subtract_duration($duration_15years); > C4::Members::ModMember( borrowernumber => $borrowernumber, dateofbirth => $past15yearsAgo->ymd() ); > >-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". > my $duration_30years = DateTime::Duration->new(years => 30); > my $past30yearsAgo = DateTime->now()->subtract_duration($duration_30years); > C4::Members::ModMember( borrowernumber => $borrowernumber, dateofbirth => $past30yearsAgo->ymd() ); > >-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 <<< > #### >-- >1.7.9.5
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 13116
:
32544
|
32550
|
32551
|
32552
|
32627
|
32662
|
32920
|
33075
|
33076
|
33077