Bug 26963: (QA follow-up) Convert to ResultSets

This patch removes the previously introduced private method by
converting the arrayref returns to ResultSets appropriately and inlining
the filter search queries.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Koha/Item.pm

@@ -553,67 +553,6 @@ sub can_be_transferred {
 
 }
 
-=head3 _can_pickup_locations
-
-$item->_can_pickup_locations({ to => $to_libraries, from => $from_library })
-Checks if an item can be transferred to given libraries.
-
-This feature is controlled by two system preferences:
-UseBranchTransferLimits to enable / disable the feature
-BranchTransferLimitsType to use either an itemnumber or ccode as an identifier
-                         for setting the limitations
-
-Takes HASHref that can have the following parameters:
-    MANDATORY PARAMETERS:
-    $to   : Array of Koha::Libraries
-    OPTIONAL PARAMETERS:
-    $from : Koha::Library  # if not given, item holdingbranch
-                           # will be used instead
-
-Returns arry of Koha::Libraries that item can be transferred to $to_library and
-are pickup_locations
-
-If checking only one library please use $item->can_be_transferred.
-
-=cut
-
-sub _can_pickup_locations {
-    my ($self, $params ) = @_;
-
-    my $to   = $params->{to};
-    my $from = $params->{from};
-    $from = defined $from ? $from->branchcode : $self->holdingbranch;
-
-    my @pickup_locations;
-    my @destination_codes;
-    foreach my $lib (@$to){
-        next unless $lib->pickup_location;
-        push @destination_codes, $lib->branchcode;
-        push @pickup_locations, $lib;
-    }
-
-    return \@pickup_locations unless C4::Context->preference('UseBranchTransferLimits');
-
-    my $limittype = C4::Context->preference('BranchTransferLimitsType');
-    my $limiter = $limittype eq 'itemtype' ? $self->effective_itemtype : $self->ccode;
-
-    my $limits = Koha::Item::Transfer::Limits->search({
-                fromBranch => $from,
-                $limittype => $limiter
-            });
-    my @limits = $limits->get_column('toBranch');
-    return \@pickup_locations unless @limits;
-
-    my @can_transfer = Koha::Libraries->search({
-        pickup_location => 1,
-        branchcode => {
-            -in => \@destination_codes,
-            -not_in => \@limits,
-        }
-    });
-    return \@can_transfer;
-}
-
 =head3 pickup_locations
 
 $pickup_locations = $item->pickup_locations( {patron => $patron } )
@@ -633,36 +572,53 @@ sub pickup_locations {
     my $branchitemrule =
       C4::Circulation::GetBranchItemRule( $circ_control_branch, $self->itype );
 
-    my @libs;
     if(defined $patron) {
-        return \@libs if $branchitemrule->{holdallowed} == 3 && !$self->home_branch->validate_hold_sibling( {branchcode => $patron->branchcode} );
+        return Koha::Libraries->new()->empty if $branchitemrule->{holdallowed} == 3 && !$self->home_branch->validate_hold_sibling( {branchcode => $patron->branchcode} );
-        return \@libs if $branchitemrule->{holdallowed} == 1 && $self->home_branch->branchcode ne $patron->branchcode;
+        return Koha::Libraries->new()->empty if $branchitemrule->{holdallowed} == 1 && $self->home_branch->branchcode ne $patron->branchcode;
     }
 
+    my $pickup_libraries = Koha::Libraries->search();
     if ($branchitemrule->{hold_fulfillment_policy} eq 'holdgroup') {
-        @libs  = $self->home_branch->get_hold_libraries;
+        $pickup_libraries = $self->home_branch->get_hold_libraries;
-        push @libs, $self->home_branch unless scalar(@libs) > 0;
     } elsif ($branchitemrule->{hold_fulfillment_policy} eq 'patrongroup') {
         my $plib = Koha::Libraries->find({ branchcode => $patron->branchcode});
-        @libs  = $plib->get_hold_libraries;
+        $pickup_libraries = $plib->get_hold_libraries;
-        push @libs, $self->home_branch unless scalar(@libs) > 0;
     } elsif ($branchitemrule->{hold_fulfillment_policy} eq 'homebranch') {
-        push @libs, $self->home_branch;
+        $pickup_libraries = Koha::Libraries->search({ branchcode => $self->homebranch });
     } elsif ($branchitemrule->{hold_fulfillment_policy} eq 'holdingbranch') {
-        push @libs, $self->holding_branch;
+        $pickup_libraries = Koha::Libraries->search({ branchcode => $self->holdingbranch });
-    } else {
+    };
-        @libs = Koha::Libraries->search({
+
+    return $pickup_libraries->search(
+        {
             pickup_location => 1
-        }, {
+        },
+        {
             order_by => ['branchname']
-        })->as_list;
+        }
-    }
+    ) unless C4::Context->preference('UseBranchTransferLimits');
 
-    my $pickup_locations = $self->_can_pickup_locations({
+    my $limittype = C4::Context->preference('BranchTransferLimitsType');
-        to => \@libs
+    my $limiter = $limittype eq 'itemtype' ? $self->effective_itemtype : $self->ccode;
-    });
+    my $limits = Koha::Item::Transfer::Limits->search(
+        {
+            fromBranch => $self->holdingbranch,
+            $limittype => $limiter
+        },
+        { columns => ['toBranch'] }
+    );
 
-    return $pickup_locations;
+    return $pickup_libraries->search(
+        {
+            pickup_location => 1,
+            branchcode      => {
+                '-not_in' => $limits->_resultset->as_query
+            }
+        },
+        {
+            order_by => ['branchname']
+        }
+    );
 }
 
 =head3 article_request_type

Koha/Library.pm

@@ -229,7 +229,7 @@ sub get_hold_libraries {
     @hold_libraries =
       grep { !$seen{ $_->id }++ } @hold_libraries;
 
-    return @hold_libraries;
+    return Koha::Libraries->search({ branchcode => { '-in' => [ keys %seen ] } });
 }
 
 =head3 validate_hold_sibling

t/db_dependent/Koha/Item.t

@@ -290,7 +290,7 @@ subtest 'pickup_locations' => sub {
                 }
             }
         );
-        my @pl = @{ $item->pickup_locations( { patron => $patron} ) };
+        my @pl = $item->pickup_locations( { patron => $patron} )->as_list;
         my $ha_value=$ha==3?'holdgroup':($ha==2?'any':'homebranch');
 
         foreach my $pickup_location (@pl) {