Attachment 88987 Details for Bug 19302 – Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest

[patch] Bug 19302: Send koha::objects to C4::Reserves::IsAvailableForItemLevelRequest

Bug-19302-Send-kohaobjects-to-C4ReservesIsAvailabl.patch (text/plain), 20.18 KB, created by Jonathan Druart on 2019-04-27 19:03:43 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2019-04-27 19:03:43 UTC

Size: 20.18 KB

patch

obsolete

>From 16b09fc3e141fc130538f64f85fbf9654bc88b09 Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>Date: Tue, 12 Sep 2017 15:26:27 -0300
>Subject: [PATCH] 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
>---
> C4/Circulation.pm                                  | 20 +++----
> C4/ILSDI/Services.pm                               |  2 +-
> C4/Reserves.pm                                     | 28 +++++-----
> opac/opac-reserve.pl                               |  2 +-
> reserve/request.pl                                 |  2 +-
> .../Holds/DisallowHoldIfItemsAvailable.t           | 62 ++++++++++------------
> t/db_dependent/Reserves.t                          | 20 +++----
> 7 files changed, 62 insertions(+), 74 deletions(-)
>
>diff --git a/C4/Circulation.pm b/C4/Circulation.pm
>index b9ff0f2824..e1b0ed6348 100644
>--- 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 43935bbf8f..8833094109 100644
>--- 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 5fc68f1359..205ed7873a 100644
>--- 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 725b31d07b..7291adb4d4 100755
>--- a/opac/opac-reserve.pl
>+++ b/opac/opac-reserve.pl
>@@ -542,7 +542,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 486c770c75..312c8e612a 100755
>--- a/reserve/request.pl
>+++ b/reserve/request.pl
>@@ -481,7 +481,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 4302170d02..c5a3b8fbb1 100755
>--- 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 27b2701ab6..e78f74014e 100755
>--- 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',
>-- 
>2.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 19302: 67124 | 68845 | 86119 | 88660 | 88987 | 89197 | 89369