Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest

Almost everywhere we call IsAvailableForItemLevelRequest we already have
a Koha::Patron and Koha::Item object. It makes sense to use them to
avoid a refetch

Test plan:
It would be good to test this patch on top of 19300 and 19301 and make
sure everything works as expected

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index b9ff0f2..e1b0ed6 100644 (file)
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -2686,16 +2686,16 @@ sub CanBookBeRenewed {
             # can be filled with available items. We can get the union of the sets simply
             # by pushing all the elements onto an array and removing the duplicates.
             my @reservable;
-            my %borrowers;
-            ITEM: foreach my $i (@itemnumbers) {
-                my $item = Koha::Items->find($i)->unblessed;
-                next if IsItemOnHoldAndFound($i);
-                for my $b (@borrowernumbers) {
-                    my $borr = $borrowers{$b} //= Koha::Patrons->find( $b )->unblessed;
-                    next unless IsAvailableForItemLevelRequest($item, $borr);
-                    next unless CanItemBeReserved($b,$i);
-
-                    push @reservable, $i;
+            my %patrons;
+            ITEM: foreach my $itemnumber (@itemnumbers) {
+                my $item = Koha::Items->find( $itemnumber );
+                next if IsItemOnHoldAndFound( $itemnumber );
+                for my $borrowernumber (@borrowernumbers) {
+                    my $patron = $patrons{$borrowernumber} //= Koha::Patrons->find( $borrowernumber );
+                    next unless IsAvailableForItemLevelRequest($item, $patron);
+                    next unless CanItemBeReserved($borrowernumber,$itemnumber);
+
+                    push @reservable, $itemnumber;
                     if (@reservable >= @borrowernumbers) {
                         $resfound = 0;
                         last ITEM;

diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm
index 43935bb..8833094 100644 (file)
--- a/C4/ILSDI/Services.pm
+++ b/C4/ILSDI/Services.pm
@@ -562,7 +562,7 @@ sub GetServices {
     my $canbookbereserved = CanBookBeReserved( $borrower, $biblionumber );
     if ($canbookbereserved->{status} eq 'OK') {
         push @availablefor, 'title level hold';
-        my $canitembereserved = IsAvailableForItemLevelRequest($item->unblessed, $borrower);
+        my $canitembereserved = IsAvailableForItemLevelRequest($item, $patron);
         if ($canitembereserved) {
             push @availablefor, 'item level hold';
         }

diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index 5fc68f1..205ed78 100644 (file)
--- a/C4/Reserves.pm
+++ b/C4/Reserves.pm
@@ -1163,46 +1163,42 @@ and canreservefromotherbranches.
 =cut
 
 sub IsAvailableForItemLevelRequest {
-    my $item = shift;
-    my $borrower = shift;
-    my $pickup_branchcode = shift;
+    my ( $item, $patron, $pickup_branchcode ) = @_;
 
     my $dbh = C4::Context->dbh;
     # must check the notforloan setting of the itemtype
     # FIXME - a lot of places in the code do this
     #         or something similar - need to be
     #         consolidated
-    my $patron = Koha::Patrons->find( $borrower->{borrowernumber} );
-    my $item_object = Koha::Items->find( $item->{itemnumber } );
-    my $itemtype = $item_object->effective_itemtype;
+    my $itemtype = $item->effective_itemtype;
     my $notforloan_per_itemtype = Koha::ItemTypes->find($itemtype)->notforloan;
 
     return 0 if
         $notforloan_per_itemtype ||
-        $item->{itemlost}        ||
-        $item->{notforloan} > 0  ||
-        $item->{withdrawn}        ||
-        ($item->{damaged} && !C4::Context->preference('AllowHoldsOnDamagedItems'));
+        $item->itemlost        ||
+        $item->notforloan > 0  ||
+        $item->withdrawn        ||
+        ($item->damaged && !C4::Context->preference('AllowHoldsOnDamagedItems'));
 
-    my $on_shelf_holds = Koha::IssuingRules->get_onshelfholds_policy( { item => $item_object, patron => $patron } );
+    my $on_shelf_holds = Koha::IssuingRules->get_onshelfholds_policy( { item => $item, patron => $patron } );
 
     if ($pickup_branchcode) {
         my $destination = Koha::Libraries->find($pickup_branchcode);
         return 0 unless $destination;
         return 0 unless $destination->pickup_location;
-        return 0 unless $item_object->can_be_transferred( { to => $destination } );
+        return 0 unless $item->can_be_transferred( { to => $destination } );
     }
 
     if ( $on_shelf_holds == 1 ) {
         return 1;
     } elsif ( $on_shelf_holds == 2 ) {
         my @items =
-          Koha::Items->search( { biblionumber => $item->{biblionumber} } );
+          Koha::Items->search( { biblionumber => $item->biblionumber } );
 
         my $any_available = 0;
 
         foreach my $i (@items) {
-            my $reserves_control_branch = GetReservesControlBranch( $i->unblessed(), $borrower );
+            my $reserves_control_branch = GetReservesControlBranch( $i->unblessed(), $patron->unblessed );
             my $branchitemrule = C4::Circulation::GetBranchItemRule( $reserves_control_branch, $i->itype );
 
             $any_available = 1
@@ -1214,12 +1210,12 @@ sub IsAvailableForItemLevelRequest {
               || ( $i->damaged
                 && !C4::Context->preference('AllowHoldsOnDamagedItems') )
               || Koha::ItemTypes->find( $i->effective_itemtype() )->notforloan
-              || $branchitemrule->{holdallowed} == 1 && $borrower->{branchcode} ne $i->homebranch;
+              || $branchitemrule->{holdallowed} == 1 && $patron->branchcode ne $i->homebranch;
         }
 
         return $any_available ? 0 : 1;
     } else { # on_shelf_holds == 0 "If any unavailable" (the description is rather cryptic and could still be improved)
-        return $item->{onloan} || IsItemOnHoldAndFound( $item->{itemnumber} );
+        return $item->onloan || IsItemOnHoldAndFound( $item->itemnumber );
     }
 }
 

diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl
index eb017b6..472d0f5 100755 (executable)
--- a/opac/opac-reserve.pl
+++ b/opac/opac-reserve.pl
@@ -537,7 +537,7 @@ foreach my $biblioNum (@biblionumbers) {
 
         my $policy_holdallowed = !$itemLoopIter->{already_reserved};
         $policy_holdallowed &&=
-            IsAvailableForItemLevelRequest($itemInfo,$patron_unblessed) &&
+            IsAvailableForItemLevelRequest($item, $patron) &&
             CanItemBeReserved( $borrowernumber, $itemNum )->{status} eq 'OK';
 
         if ($policy_holdallowed) {

diff --git a/reserve/request.pl b/reserve/request.pl
index df9cae8..f559ed0 100755 (executable)
--- a/reserve/request.pl
+++ b/reserve/request.pl
@@ -497,7 +497,7 @@ foreach my $biblionumber (@biblionumbers) {
                 if (
                        !$item->{cantreserve}
                     && !$exceeded_maxreserves
-                    && IsAvailableForItemLevelRequest($item, $patron_unblessed)
+                    && IsAvailableForItemLevelRequest($item_object, $patron)
                     && $can_item_be_reserved->{status} eq 'OK'
                   )
                 {

diff --git a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
index 4302170..c5a3b8f 100755 (executable)
--- a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
+++ b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
@@ -36,32 +36,31 @@ my $itemtype = $builder->build({
 t::lib::Mocks::mock_userenv({ branchcode => $library1->{branchcode} });
 
 
-my $borrower1 = $builder->build({
-    source => 'Borrower',
+my $patron1 = $builder->build_object({
+    class => 'Koha::Patrons',
     value => {
         branchcode => $library1->{branchcode},
         dateexpiry => '3000-01-01',
     }
 });
+my $borrower1 = $patron1->unblessed;
 
-my $borrower2 = $builder->build({
-    source => 'Borrower',
+my $patron2 = $builder->build_object({
+    class => 'Koha::Patrons',
     value => {
         branchcode => $library1->{branchcode},
         dateexpiry => '3000-01-01',
     }
 });
 
-my $borrower3 = $builder->build({
-    source => 'Borrower',
+my $patron3 = $builder->build_object({
+    class => 'Koha::Patrons',
     value => {
         branchcode => $library2->{branchcode},
         dateexpiry => '3000-01-01',
     }
 });
 
-my $borrowernumber1 = $borrower1->{borrowernumber};
-my $borrowernumber2 = $borrower2->{borrowernumber};
 my $library_A = $library1->{branchcode};
 my $library_B = $library2->{branchcode};
 
@@ -72,18 +71,15 @@ my $item1  = $builder->build_sample_item({
     itype=>$itemtype,
     homebranch => $library_A,
     holdingbranch => $library_A
-})->unblessed;
+});
 my $item2  = $builder->build_sample_item({
     biblionumber=>$biblionumber,
     itype=>$itemtype,
     homebranch => $library_A,
     holdingbranch => $library_A
-})->unblessed;
+});
 
 # Test hold_fulfillment_policy
-
-
-
 my $rule = Koha::IssuingRule->new(
     {
         categorycode => '*',
@@ -97,20 +93,20 @@ my $rule = Koha::IssuingRule->new(
 );
 $rule->store();
 
-my $is = IsAvailableForItemLevelRequest( $item1, $borrower1);
+my $is = IsAvailableForItemLevelRequest( $item1, $patron1);
 is( $is, 0, "Item cannot be held, 2 items available" );
 
-my $issue1 = AddIssue( $borrower2, $item1->{barcode} );
+my $issue1 = AddIssue( $patron2->unblessed, $item1->barcode );
 
-$is = IsAvailableForItemLevelRequest( $item1, $borrower1);
+$is = IsAvailableForItemLevelRequest( $item1, $patron1);
 is( $is, 0, "Item cannot be held, 1 item available" );
 
-AddIssue( $borrower2, $item2->{barcode} );
+AddIssue( $patron2->unblessed, $item2->barcode );
 
-$is = IsAvailableForItemLevelRequest( $item1, $borrower1);
+$is = IsAvailableForItemLevelRequest( $item1, $patron1);
 is( $is, 1, "Item can be held, no items available" );
 
-AddReturn( $item1->{barcode} );
+AddReturn( $item1->barcode );
 
 { # Remove the issue for the first patron, and modify the branch for item1
     subtest 'IsAvailableForItemLevelRequest behaviours depending on ReservesControlBranch + holdallowed' => sub {
@@ -125,9 +121,7 @@ AddReturn( $item1->{barcode} );
         subtest 'Item is available at a different library' => sub {
             plan tests => 7;
 
-            $item1 = Koha::Items->find( $item1->{itemnumber} );
             $item1->set({homebranch => $library_B, holdingbranch => $library_B })->store;
-            $item1 = $item1->unblessed;
             #Scenario is:
             #One shelf holds is 'If all unavailable'/2
             #Item 1 homebranch library B is available
@@ -139,21 +133,21 @@ AddReturn( $item1->{barcode} );
                 $sth_insert_rule->execute( $hold_allowed_from_home_library );
 
                 t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary');
-                $is = IsAvailableForItemLevelRequest( $item1, $borrower1);
+                $is = IsAvailableForItemLevelRequest( $item1, $patron1);
                 is( $is, 1, "Hold allowed from home library + ReservesControlBranch=ItemHomeLibrary, One item is available at different library, not holdable = none available => the hold is allowed at item level" );
-                $is = IsAvailableForItemLevelRequest( $item1, $borrower2);
+                $is = IsAvailableForItemLevelRequest( $item1, $patron2);
                 is( $is, 1, "Hold allowed from home library + ReservesControlBranch=ItemHomeLibrary, One item is available at home library, holdable = one available => the hold is not allowed at item level" );
                 $sth_insert_branch_rule->execute( $library_B, $hold_allowed_from_any_libraries );
                 #Adding a rule for the item's home library affects the availability for a borrower from another library because ReservesControlBranch is set to ItemHomeLibrary
-                $is = IsAvailableForItemLevelRequest( $item1, $borrower1);
+                $is = IsAvailableForItemLevelRequest( $item1, $patron1);
                 is( $is, 0, "Hold allowed from home library + ReservesControlBranch=ItemHomeLibrary, One item is available at different library, holdable = one available => the hold is not allowed at item level" );
 
                 t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary');
-                $is = IsAvailableForItemLevelRequest( $item1, $borrower1);
+                $is = IsAvailableForItemLevelRequest( $item1, $patron1);
                 is( $is, 1, "Hold allowed from home library + ReservesControlBranch=PatronLibrary, One item is available at different library, not holdable = none available => the hold is allowed at item level" );
                 #Adding a rule for the patron's home library affects the availability for an item from another library because ReservesControlBranch is set to PatronLibrary
                 $sth_insert_branch_rule->execute( $library_A, $hold_allowed_from_any_libraries );
-                $is = IsAvailableForItemLevelRequest( $item1, $borrower1);
+                $is = IsAvailableForItemLevelRequest( $item1, $patron1);
                 is( $is, 0, "Hold allowed from home library + ReservesControlBranch=PatronLibrary, One item is available at different library, holdable = one available => the hold is not allowed at item level" );
             }
 
@@ -162,11 +156,11 @@ AddReturn( $item1->{barcode} );
                 $sth_insert_rule->execute( $hold_allowed_from_any_libraries );
 
                 t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary');
-                $is = IsAvailableForItemLevelRequest( $item1, $borrower1);
+                $is = IsAvailableForItemLevelRequest( $item1, $patron1);
                 is( $is, 0, "Hold allowed from any library + ReservesControlBranch=ItemHomeLibrary, One item is available at the diff library, holdable = 1 available => the hold is not allowed at item level" );
 
                 t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary');
-                $is = IsAvailableForItemLevelRequest( $item1, $borrower1);
+                $is = IsAvailableForItemLevelRequest( $item1, $patron1);
                 is( $is, 0, "Hold allowed from any library + ReservesControlBranch=PatronLibrary, One item is available at the diff library, holdable = 1 available => the hold is not allowed at item level" );
             }
         };
@@ -174,9 +168,7 @@ AddReturn( $item1->{barcode} );
         subtest 'Item is available at the same library' => sub {
             plan tests => 4;
 
-            $item1 = Koha::Items->find( $item1->{itemnumber} );
             $item1->set({homebranch => $library_A, holdingbranch => $library_A })->store;
-            $item1 = $item1->unblessed;
             #Scenario is:
             #One shelf holds is 'If all unavailable'/2
             #Item 1 homebranch library A is available
@@ -190,11 +182,11 @@ AddReturn( $item1->{barcode} );
                 $sth_insert_rule->execute( $hold_allowed_from_home_library );
 
                 t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary');
-                $is = IsAvailableForItemLevelRequest( $item1, $borrower1);
+                $is = IsAvailableForItemLevelRequest( $item1, $patron1);
                 is( $is, 0, "Hold allowed from home library + ReservesControlBranch=ItemHomeLibrary, One item is available at the same library, holdable = 1 available  => the hold is not allowed at item level" );
 
                 t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary');
-                $is = IsAvailableForItemLevelRequest( $item1, $borrower1);
+                $is = IsAvailableForItemLevelRequest( $item1, $patron1);
                 is( $is, 0, "Hold allowed from home library + ReservesControlBranch=PatronLibrary, One item is available at the same library, holdable = 1 available  => the hold is not allowed at item level" );
             }
 
@@ -203,11 +195,11 @@ AddReturn( $item1->{barcode} );
                 $sth_insert_rule->execute( $hold_allowed_from_any_libraries );
 
                 t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary');
-                $is = IsAvailableForItemLevelRequest( $item1, $borrower1);
+                $is = IsAvailableForItemLevelRequest( $item1, $patron1);
                 is( $is, 0, "Hold allowed from any library + ReservesControlBranch=ItemHomeLibrary, One item is available at the same library, holdable = 1 available => the hold is not allowed at item level" );
 
                 t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary');
-                $is = IsAvailableForItemLevelRequest( $item1, $borrower1);
+                $is = IsAvailableForItemLevelRequest( $item1, $patron1);
                 is( $is, 0, "Hold allowed from any library + ReservesControlBranch=PatronLibrary, One item is available at the same library, holdable = 1 available  => the hold is not allowed at item level" );
             }
         };
@@ -241,7 +233,7 @@ $rule = Koha::IssuingRule->new(
 );
 $rule->store();
 
-$is = IsAvailableForItemLevelRequest( $item3->unblessed, $borrower1);
+$is = IsAvailableForItemLevelRequest( $item3, $patron1);
 is( $is, 1, "Item can be held, items in transit are not available" );
 
 # Cleanup

diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t
index 27b2701..e78f740 100755 (executable)
--- a/t/db_dependent/Reserves.t
+++ b/t/db_dependent/Reserves.t
@@ -109,7 +109,8 @@ my %data = (
 );
 Koha::Patron::Categories->find($category_1)->set({ enrolmentfee => 0})->store;
 my $borrowernumber = Koha::Patron->new(\%data)->store->borrowernumber;
-my $borrower = Koha::Patrons->find( $borrowernumber )->unblessed;
+my $patron = Koha::Patrons->find( $borrowernumber );
+my $borrower = $patron->unblessed;
 my $biblionumber   = $bibnum;
 my $barcode        = $testbarcode;
 
@@ -520,22 +521,21 @@ is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblio_with_no_item->{bibl
 ####### EO Bug 13113 <<<
        ####
 
-$item = Koha::Items->find($itemnumber)->unblessed;
+$item = Koha::Items->find($itemnumber);
 
-ok( C4::Reserves::IsAvailableForItemLevelRequest($item, $borrower), "Reserving a book on item level" );
+ok( C4::Reserves::IsAvailableForItemLevelRequest($item, $patron), "Reserving a book on item level" );
 
 my $pickup_branch = $builder->build({ source => 'Branch' })->{ branchcode };
 t::lib::Mocks::mock_preference( 'UseBranchTransferLimits',  '1' );
 t::lib::Mocks::mock_preference( 'BranchTransferLimitsType', 'itemtype' );
-my ($item_object) = Koha::Biblios->find($biblionumber)->items->as_list;
 my $limit = Koha::Item::Transfer::Limit->new(
     {
         toBranch   => $pickup_branch,
-        fromBranch => $item_object->holdingbranch,
-        itemtype   => $item_object->effective_itemtype,
+        fromBranch => $item->holdingbranch,
+        itemtype   => $item->effective_itemtype,
     }
 )->store();
-is( C4::Reserves::IsAvailableForItemLevelRequest($item, $borrower, $pickup_branch), 0, "Item level request not available due to transfer limit" );
+is( C4::Reserves::IsAvailableForItemLevelRequest($item, $patron, $pickup_branch), 0, "Item level request not available due to transfer limit" );
 t::lib::Mocks::mock_preference( 'UseBranchTransferLimits',  '0' );
 
 # tests for MoveReserve in relation to ConfirmFutureHolds (BZ 14526)
@@ -643,10 +643,10 @@ subtest '_koha_notify_reserve() tests' => sub {
         })->{borrowernumber};
 
     C4::Reserves::AddReserve(
-        $item->{homebranch}, $hold_borrower,
-        $item->{biblionumber} );
+        $item->homebranch, $hold_borrower,
+        $item->biblionumber );
 
-    ModReserveAffect($item->{itemnumber}, $hold_borrower, 0);
+    ModReserveAffect($item->itemnumber, $hold_borrower, 0);
     my $sms_message_address = $schema->resultset('MessageQueue')->search({
             letter_code     => 'HOLD',
             message_transport_type => 'sms',