Bug 15524: (QA follow-up) Change Can[Book|Item]BeReserved to return hashref, pass...
authorKyle M Hall <kyle@bywatersolutiosn.com>
Fri, 10 Aug 2018 14:36:36 +0000 (10:36 -0400)
committerNick Clemens <nick@bywatersolutions.com>
Fri, 24 Aug 2018 16:27:27 +0000 (16:27 +0000)
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

C4/ILSDI/Services.pm
C4/Reserves.pm
Koha/REST/V1/Hold.pm
opac/opac-reserve.pl
reserve/request.pl
t/db_dependent/Holds.t
t/db_dependent/Reserves.t
t/db_dependent/Reserves/MultiplePerRecord.t

index 9498a77..53292a5 100644 (file)
@@ -542,7 +542,7 @@ sub GetServices {
     # Reserve level management
     my $biblionumber = $item->{'biblionumber'};
     my $canbookbereserved = CanBookBeReserved( $borrower, $biblionumber );
-    if ($canbookbereserved eq 'OK') {
+    if ($canbookbereserved->{status} eq 'OK') {
         push @availablefor, 'title level hold';
         my $canitembereserved = IsAvailableForItemLevelRequest($item, $borrower);
         if ($canitembereserved) {
@@ -662,7 +662,7 @@ sub HoldTitle {
     my $title = $biblio ? $biblio->title : '';
 
     # Check if the biblio can be reserved
-    return { code => 'NotHoldable' } unless CanBookBeReserved( $borrowernumber, $biblionumber ) eq 'OK';
+    return { code => 'NotHoldable' } unless CanBookBeReserved( $borrowernumber, $biblionumber )->{status} eq 'OK';
 
     my $branch;
 
@@ -740,7 +740,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 eq 'OK' and $canitembereserved eq 'OK';
+    return { code => 'NotHoldable' } unless $canbookbereserved->{status} eq 'OK' and $canitembereserved->{status} eq 'OK';
 
     # Pickup branch management
     my $branch;
index 16ad20d..38f9953 100644 (file)
@@ -285,7 +285,7 @@ sub CanBookBeReserved{
     my $canReserve;
     foreach my $item (@$items) {
         $canReserve = CanItemBeReserved( $borrowernumber, $item );
-        return 'OK' if $canReserve eq 'OK';
+        return $canReserve if $canReserve->{status} eq 'OK';
     }
     return $canReserve;
 }
@@ -293,14 +293,14 @@ sub CanBookBeReserved{
 =head2 CanItemBeReserved
 
   $canReserve = &CanItemBeReserved($borrowernumber, $itemnumber)
-  if ($canReserve eq 'OK') { #We can reserve this Item! }
+  if ($canReserve->{status} eq 'OK') { #We can reserve this Item! }
 
-@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
+@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.
+         { status => cannotReserveFromOtherBranches }, if syspref 'canreservefromotherbranches' is OK.
+         { status => tooManyReserves, limit => $limit }, if the borrower has exceeded his maximum reserve amount.
+         { status => notReservable },   if holds on this item are not allowed
 
 =cut
 
@@ -320,17 +320,17 @@ sub CanItemBeReserved {
     my $borrower = $patron->unblessed;
 
     # If an item is damaged and we don't allow holds on damaged items, we can stop right here
-    return 'damaged'
+    return { status =>'damaged' }
       if ( $item->{damaged}
         && !C4::Context->preference('AllowHoldsOnDamagedItems') );
 
     # Check for the age restriction
     my ( $ageRestriction, $daysToAgeRestriction ) =
       C4::Circulation::GetAgeRestriction( $biblio->biblioitem->agerestriction, $borrower );
-    return 'ageRestricted' if $daysToAgeRestriction && $daysToAgeRestriction > 0;
+    return { status => 'ageRestricted' } if $daysToAgeRestriction && $daysToAgeRestriction > 0;
 
     # Check that the patron doesn't have an item level hold on this item already
-    return 'itemAlreadyOnHold'
+    return { status =>'itemAlreadyOnHold' }
       if Koha::Holds->search( { borrowernumber => $borrowernumber, itemnumber => $itemnumber } )->count();
 
     my $controlbranch = C4::Context->preference('ReservesControlBranch');
@@ -375,7 +375,7 @@ sub CanItemBeReserved {
         }
     );
     if ( $holds->count() >= $holds_per_record ) {
-        return "tooManyHoldsForThisRecord";
+        return { status => "tooManyHoldsForThisRecord", limit => $holds_per_record };
     }
 
     # we retrieve count
@@ -406,7 +406,7 @@ sub CanItemBeReserved {
 
     # we check if it's ok or not
     if ( $reservecount >= $allowedreserves ) {
-        return 'tooManyReserves';
+        return { status => 'tooManyReserves', limit => $allowedreserves };
     }
 
     # Now we need to check hold limits by patron category
@@ -429,7 +429,7 @@ sub CanItemBeReserved {
             }
         )->count();
 
-        return 'tooManyReserves' if $total_holds_count >= $rule->max_holds;
+        return { status => 'tooManyReserves', limit => $rule->max_holds } if $total_holds_count >= $rule->max_holds;
     }
 
     my $circ_control_branch =
@@ -438,13 +438,13 @@ sub CanItemBeReserved {
       C4::Circulation::GetBranchItemRule( $circ_control_branch, $item->itype );
 
     if ( $branchitemrule->{holdallowed} == 0 ) {
-        return 'notReservable';
+        return { status => 'notReservable' };
     }
 
     if (   $branchitemrule->{holdallowed} == 1
         && $borrower->{branchcode} ne $item->homebranch )
     {
-        return 'cannotReserveFromOtherBranches';
+        return { status => 'cannotReserveFromOtherBranches' };
     }
 
     # If reservecount is ok, we check item branch if IndependentBranches is ON
@@ -454,11 +454,11 @@ sub CanItemBeReserved {
     {
         my $itembranch = $item->homebranch;
         if ( $itembranch ne $borrower->{branchcode} ) {
-            return 'cannotReserveFromOtherBranches';
+            return { status => 'cannotReserveFromOtherBranches' };
         }
     }
 
-    return 'OK';
+    return { status => 'OK' };
 }
 
 =head2 CanReserveBeCanceledFromOpac
index 2833f39..c152fb7 100644 (file)
@@ -89,7 +89,7 @@ sub add {
       ? CanItemBeReserved( $borrowernumber, $itemnumber )
       : CanBookBeReserved( $borrowernumber, $biblionumber );
 
-    unless ($can_reserve eq 'OK') {
+    unless ($can_reserve->{status} eq 'OK') {
         return $c->render( status => 403, openapi => {
             error => "Reserve cannot be placed. Reason: $can_reserve"
         } );
index a9af8d1..92989a2 100755 (executable)
@@ -269,10 +269,10 @@ if ( $query->param('place_reserve') ) {
 
         my $rank = $biblioData->{rank};
         if ( $itemNum ne '' ) {
-            $canreserve = 1 if CanItemBeReserved( $borrowernumber, $itemNum ) eq 'OK';
+            $canreserve = 1 if CanItemBeReserved( $borrowernumber, $itemNum )->{status} eq 'OK';
         }
         else {
-            $canreserve = 1 if CanBookBeReserved( $borrowernumber, $biblioNum ) eq 'OK';
+            $canreserve = 1 if CanBookBeReserved( $borrowernumber, $biblioNum )->{status} eq 'OK';
 
             # Inserts a null into the 'itemnumber' field of 'reserves' table.
             $itemNum = undef;
@@ -525,7 +525,7 @@ foreach my $biblioNum (@biblionumbers) {
         my $policy_holdallowed = !$itemLoopIter->{already_reserved};
         $policy_holdallowed &&=
             IsAvailableForItemLevelRequest($itemInfo,$patron_unblessed) &&
-            CanItemBeReserved($borrowernumber,$itemNum) eq 'OK';
+            CanItemBeReserved($borrowernumber,$itemNum)->{status} eq 'OK';
 
         if ($policy_holdallowed) {
             my $opac_hold_policy = Koha::IssuingRules->get_opacitemholds_policy( { item => $item, patron => $patron } );
@@ -585,7 +585,7 @@ foreach my $biblioNum (@biblionumbers) {
         }
     }
 
-    $biblioLoopIter{holdable} &&= CanBookBeReserved($borrowernumber,$biblioNum) eq 'OK';
+    $biblioLoopIter{holdable} &&= CanBookBeReserved($borrowernumber,$biblioNum)->{status} eq 'OK';
 
     # For multiple holds per record, if a patron has previously placed a hold,
     # the patron can only place more holds of the same type. That is, if the
index 2902de0..2fd36e8 100755 (executable)
@@ -211,24 +211,25 @@ foreach my $biblionumber (@biblionumbers) {
     if ( $patron ) {
         { # CanBookBeReserved
             my $canReserve = CanBookBeReserved( $patron->borrowernumber, $biblionumber );
-            $canReserve //= '';
-            if ( $canReserve eq 'OK' ) {
+            $canReserve->{status} //= '';
+            if ( $canReserve->{status} eq 'OK' ) {
 
                 #All is OK and we can continue
             }
-            elsif ( $canReserve eq 'tooManyReserves' ) {
+            elsif ( $canReserve->{status} eq 'tooManyReserves' ) {
                 $exceeded_maxreserves = 1;
+                $template->param( maxreserves => $canReserve->{limit} );
             }
-            elsif ( $canReserve eq 'tooManyHoldsForThisRecord' ) {
+            elsif ( $canReserve->{status} eq 'tooManyHoldsForThisRecord' ) {
                 $exceeded_holds_per_record = 1;
-                $biblioloopiter{$canReserve} = 1;
+                $biblioloopiter{ $canReserve->{status} } = 1;
             }
-            elsif ( $canReserve eq 'ageRestricted' ) {
-                $template->param( $canReserve => 1 );
-                $biblioloopiter{$canReserve} = 1;
+            elsif ( $canReserve->{status} eq 'ageRestricted' ) {
+                $template->param( $canReserve->{status} => 1 );
+                $biblioloopiter{ $canReserve->{status} } = 1;
             }
             else {
-                $biblioloopiter{$canReserve} = 1;
+                $biblioloopiter{ $canReserve->{status} } = 1;
             }
         }
 
@@ -462,7 +463,7 @@ foreach my $biblionumber (@biblionumbers) {
                 $item->{'holdallowed'} = $branchitemrule->{'holdallowed'};
 
                 my $can_item_be_reserved = CanItemBeReserved( $patron->borrowernumber, $itemnumber );
-                $item->{not_holdable} = $can_item_be_reserved unless ( $can_item_be_reserved eq 'OK' );
+                $item->{not_holdable} = $can_item_be_reserved->{status} unless ( $can_item_be_reserved->{status} eq 'OK' );
 
                 $item->{item_level_holds} = Koha::IssuingRules->get_opacitemholds_policy( { item => $item_object, patron => $patron } );
 
index 8f1eaf2..27151ae 100755 (executable)
@@ -7,7 +7,7 @@ use t::lib::TestBuilder;
 
 use C4::Context;
 
-use Test::More tests => 56;
+use Test::More tests => 55;
 use MARC::Record;
 use Koha::Patrons;
 use C4::Items;
@@ -261,7 +261,7 @@ t::lib::Mocks::mock_preference('item-level_itypes', 1);
 # if IndependentBranches is OFF, a $branch_1 patron can reserve an $branch_2 item
 t::lib::Mocks::mock_preference('IndependentBranches', 0);
 ok(
-    CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber) eq 'OK',
+    CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber)->{status} eq 'OK',
     '$branch_1 patron allowed to reserve $branch_2 item with IndependentBranches OFF (bug 2394)'
 );
 
@@ -269,14 +269,14 @@ ok(
 t::lib::Mocks::mock_preference('IndependentBranches', 1);
 t::lib::Mocks::mock_preference('canreservefromotherbranches', 0);
 ok(
-    CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber) eq 'cannotReserveFromOtherBranches',
+    CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber)->{status} eq 'cannotReserveFromOtherBranches',
     '$branch_1 patron NOT allowed to reserve $branch_2 item with IndependentBranches ON ... (bug 2394)'
 );
 
 # ... unless canreservefromotherbranches is ON
 t::lib::Mocks::mock_preference('canreservefromotherbranches', 1);
 ok(
-    CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber) eq 'OK',
+    CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber)->{status} eq 'OK',
     '... unless canreservefromotherbranches is ON (bug 2394)'
 );
 
@@ -299,7 +299,7 @@ ok(
 
 ModItem({ damaged => 1 }, $item_bibnum, $itemnumber);
 t::lib::Mocks::mock_preference( 'AllowHoldsOnDamagedItems', 1 );
-is( CanItemBeReserved( $borrowernumbers[0], $itemnumber), 'OK', "Patron can reserve damaged item with AllowHoldsOnDamagedItems enabled" );
+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" );
 
 $hold = Koha::Hold->new(
@@ -309,13 +309,13 @@ $hold = Koha::Hold->new(
         biblionumber   => $item_bibnum,
     }
 )->store();
-is( CanItemBeReserved( $borrowernumbers[0], $itemnumber ),
+is( CanItemBeReserved( $borrowernumbers[0], $itemnumber )->{status},
     'itemAlreadyOnHold',
     "Patron cannot place a second item level hold for a given item" );
 $hold->delete();
 
 t::lib::Mocks::mock_preference( 'AllowHoldsOnDamagedItems', 0 );
-ok( CanItemBeReserved( $borrowernumbers[0], $itemnumber) eq 'damaged', "Patron cannot reserve damaged item with AllowHoldsOnDamagedItems disabled" );
+ok( CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status} 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
@@ -329,19 +329,19 @@ AddReserve(
     1,
 );
 is(
-    CanItemBeReserved( $borrowernumbers[0], $itemnumber), 'tooManyReserves',
+    CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status}, '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) eq 'OK',
+    CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status} 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',
+    CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status} eq 'tooManyReserves',
     "cannot request item if policy that matches on bib-level item type forbids it (bug 9532)"
 );
 
@@ -370,18 +370,18 @@ $dbh->do(q{
 ($bibnum, $title, $bibitemnum) = create_helper_biblio('CANNOT');
 ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem(
     { homebranch => $branch_1, holdingbranch => $branch_1, itype => 'CANNOT' } , $bibnum);
-is(CanItemBeReserved($borrowernumbers[0], $itemnumber), 'notReservable',
+is(CanItemBeReserved($borrowernumbers[0], $itemnumber)->{status}, 'notReservable',
     "CanItemBeReserved should return 'notReservable'");
 
 ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem(
     { homebranch => $branch_2, holdingbranch => $branch_1, itype => 'CAN' } , $bibnum);
-is(CanItemBeReserved($borrowernumbers[0], $itemnumber),
+is(CanItemBeReserved($borrowernumbers[0], $itemnumber)->{status},
     'cannotReserveFromOtherBranches',
     "CanItemBeReserved should return 'cannotReserveFromOtherBranches'");
 
 ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem(
     { homebranch => $branch_1, holdingbranch => $branch_1, itype => 'CAN' } , $bibnum);
-is(CanItemBeReserved($borrowernumbers[0], $itemnumber), 'OK',
+is(CanItemBeReserved($borrowernumbers[0], $itemnumber)->{status}, 'OK',
     "CanItemBeReserved should return 'OK'");
 
 # Bug 12632
@@ -403,12 +403,12 @@ $dbh->do(
     {},
     '*', '*', 'ONLY1', 1, 99
 );
-is( CanItemBeReserved( $borrowernumbers[0], $itemnumber ),
+is( CanItemBeReserved( $borrowernumbers[0], $itemnumber )->{status},
     'OK', 'Patron can reserve item with hold limit of 1, no holds placed' );
 
 my $res_id = AddReserve( $branch_1, $borrowernumbers[0], $bibnum, '', 1, );
 
-is( CanItemBeReserved( $borrowernumbers[0], $itemnumber ),
+is( CanItemBeReserved( $borrowernumbers[0], $itemnumber )->{status},
     'tooManyReserves', 'Patron cannot reserve item with hold limit of 1, 1 bib level hold placed' );
 
 subtest 'Test max_holds per library/patron category' => sub {
@@ -438,7 +438,7 @@ subtest 'Test max_holds per library/patron category' => sub {
     is( $count, 3, 'Patron now has 3 holds' );
 
     my $ret = CanItemBeReserved( $borrowernumbers[0], $itemnumber );
-    is( $ret, 'OK', 'Patron can place hold with no borrower circ rules' );
+    is( $ret->{status}, 'OK', 'Patron can place hold with no borrower circ rules' );
 
     my $rule_all = $schema->resultset('DefaultBorrowerCircRule')->new(
         {
@@ -456,19 +456,19 @@ subtest 'Test max_holds per library/patron category' => sub {
     )->insert();
 
     $ret = CanItemBeReserved( $borrowernumbers[0], $itemnumber );
-    is( $ret, 'OK', 'Patron can place hold with branch/category rule of 5, category rule of 3' );
+    is( $ret->{status}, 'OK', 'Patron can place hold with branch/category rule of 5, category rule of 3' );
 
     $rule_branch->delete();
 
     $ret = CanItemBeReserved( $borrowernumbers[0], $itemnumber );
-    is( $ret, 'tooManyReserves', 'Patron cannot place hold with only a category rule of 3' );
+    is( $ret->{status}, 'tooManyReserves', 'Patron cannot place hold with only a category rule of 3' );
 
     $rule_all->delete();
     $rule_branch->max_holds(3);
     $rule_branch->insert();
 
     $ret = CanItemBeReserved( $borrowernumbers[0], $itemnumber );
-    is( $ret, 'tooManyReserves', 'Patron cannot place hold with only a branch/category rule of 3' );
+    is( $ret->{status}, 'tooManyReserves', 'Patron cannot place hold with only a branch/category rule of 3' );
 
     $rule_branch->max_holds(5);
     $rule_branch->update();
@@ -476,7 +476,7 @@ subtest 'Test max_holds per library/patron category' => sub {
     $rule_all->insert();
 
     $ret = CanItemBeReserved( $borrowernumbers[0], $itemnumber );
-    is( $ret, 'OK', 'Patron can place hold with branch/category rule of 5, category rule of 5' );
+    is( $ret->{status}, 'OK', 'Patron can place hold with branch/category rule of 5, category rule of 5' );
 };
 
 # Helper method to set up a Biblio.
index 406e450..9fe293d 100755 (executable)
@@ -522,19 +522,19 @@ 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, $frameworkcode );
 
-is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , 'OK', "Reserving an ageRestricted Biblio without a borrower dateofbirth succeeds" );
+is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber)->{status} , 'OK', "Reserving an ageRestricted Biblio without a borrower dateofbirth succeeds" );
 
 #Set the dateofbirth for the Borrower making them "too young".
 $borrower->{dateofbirth} = DateTime->now->add( years => -15 );
 Koha::Patrons->find( $borrowernumber )->set({ dateofbirth => $borrower->{dateofbirth} })->store;
 
-is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , 'ageRestricted', "Reserving a 'PEGI 16' Biblio by a 15 year old borrower fails");
+is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber)->{status} , 'ageRestricted', "Reserving a 'PEGI 16' Biblio by a 15 year old borrower fails");
 
 #Set the dateofbirth for the Borrower making them "too old".
 $borrower->{dateofbirth} = DateTime->now->add( years => -30 );
 Koha::Patrons->find( $borrowernumber )->set({ dateofbirth => $borrower->{dateofbirth} })->store;
 
-is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , 'OK', "Reserving a 'PEGI 16' Biblio by a 30 year old borrower succeeds");
+is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber)->{status} , 'OK', "Reserving a 'PEGI 16' Biblio by a 30 year old borrower succeeds");
        ####
 ####### EO Bug 13113 <<<
        ####
index 70f2600..7f639ce 100755 (executable)
@@ -277,17 +277,17 @@ $rule = $rules_rs->new(
 )->insert();
 
 my $can = CanBookBeReserved($patron->{borrowernumber}, $biblio->{biblionumber});
-is( $can, 'OK', 'Hold can be placed with 0 holds' );
+is( $can->{status}, 'OK', 'Hold can be placed with 0 holds' );
 my $hold_id = AddReserve( $library->{branchcode}, $patron->{borrowernumber}, $biblio->{biblionumber}, '', 1 );
 ok( $hold_id, 'First hold was placed' );
 
 $can = CanBookBeReserved($patron->{borrowernumber}, $biblio->{biblionumber});
-is( $can, 'OK', 'Hold can be placed with 1 hold' );
+is( $can->{status}, 'OK', 'Hold can be placed with 1 hold' );
 $hold_id = AddReserve( $library->{branchcode}, $patron->{borrowernumber}, $biblio->{biblionumber}, '', 1 );
 ok( $hold_id, 'Second hold was placed' );
 
 $can = CanBookBeReserved($patron->{borrowernumber}, $biblio->{biblionumber});
-is( $can, 'tooManyHoldsForThisRecord', 'Third hold exceeds limit of holds per record' );
+is( $can->{status}, 'tooManyHoldsForThisRecord', 'Third hold exceeds limit of holds per record' );
 
 $schema->storage->txn_rollback;