From 7179a29feaf994f35de871d06084f71dfc9eebcc Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 11 Nov 2020 08:57:21 +0000 Subject: [PATCH] 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 Signed-off-by: Tomas Cohen Arazi Signed-off-by: Jonathan Druart --- Koha/Item.pm | 114 ++++++++++++------------------------- Koha/Library.pm | 2 +- t/db_dependent/Koha/Item.t | 2 +- 3 files changed, 37 insertions(+), 81 deletions(-) diff --git a/Koha/Item.pm b/Koha/Item.pm index 71f6b7827b..db87a0aaf7 100644 --- a/Koha/Item.pm +++ b/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 \@libs if $branchitemrule->{holdallowed} == 1 && $self->home_branch->branchcode ne $patron->branchcode; + return Koha::Libraries->new()->empty if $branchitemrule->{holdallowed} == 3 && !$self->home_branch->validate_hold_sibling( {branchcode => $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; - push @libs, $self->home_branch unless scalar(@libs) > 0; + $pickup_libraries = $self->home_branch->get_hold_libraries; } elsif ($branchitemrule->{hold_fulfillment_policy} eq 'patrongroup') { my $plib = Koha::Libraries->find({ branchcode => $patron->branchcode}); - @libs = $plib->get_hold_libraries; - push @libs, $self->home_branch unless scalar(@libs) > 0; + $pickup_libraries = $plib->get_hold_libraries; } 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; - } else { - @libs = Koha::Libraries->search({ + $pickup_libraries = Koha::Libraries->search({ branchcode => $self->holdingbranch }); + }; + + return $pickup_libraries->search( + { pickup_location => 1 - }, { + }, + { order_by => ['branchname'] - })->as_list; - } + } + ) unless C4::Context->preference('UseBranchTransferLimits'); - my $pickup_locations = $self->_can_pickup_locations({ - to => \@libs - }); + my $limittype = C4::Context->preference('BranchTransferLimitsType'); + 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 diff --git a/Koha/Library.pm b/Koha/Library.pm index 86291b0c7f..b69fdaaeab 100644 --- a/Koha/Library.pm +++ b/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 diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t index e8fd36a9a8..8cb1b399bd 100755 --- a/t/db_dependent/Koha/Item.t +++ b/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) { -- 2.39.5