From ef8adf34fde2352680934ba6a36de89e3f905a36 Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
Date: Thu, 20 Apr 2023 15:44:53 +0000
Subject: [PATCH] Bug 33447: (follow-up) Fix tests and make assumption explicit

The patches made an assumption that patron would always be passed. It is
within Koha, but not in the Biblios tests.

There is no scenario where we can determine pickup locations that are not in
reference to a patron (who is picking it up?) so we should always have
this parameter

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
---
 Koha/Biblio.pm                |  7 ++++---
 Koha/Biblios.pm               |  4 +++-
 Koha/Item.pm                  | 11 ++++++-----
 t/db_dependent/Koha/Biblios.t |  4 +++-
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/Koha/Biblio.pm b/Koha/Biblio.pm
index a94665ddc8..cb898f4bc3 100644
--- a/Koha/Biblio.pm
+++ b/Koha/Biblio.pm
@@ -255,11 +255,12 @@ sub can_be_transferred {
 
 =head3 pickup_locations
 
-    my $pickup_locations = $biblio->pickup_locations( {patron => $patron } );
+    my $pickup_locations = $biblio->pickup_locations( { patron => $patron } );
 
 Returns a Koha::Libraries set of possible pickup locations for this biblio's items,
-according to patron's home library (if patron is defined and holds are allowed
-only from hold groups) and if item can be transferred to each pickup location.
+according to patron's home library and if item can be transferred to each pickup location.
+
+Patron is a required parameter.
 
 =cut
 
diff --git a/Koha/Biblios.pm b/Koha/Biblios.pm
index cf1cbdb83a..af8de37cb1 100644
--- a/Koha/Biblios.pm
+++ b/Koha/Biblios.pm
@@ -40,7 +40,9 @@ Koha::Biblios - Koha Biblio object set class
     my $biblios = Koha::Biblios->search(...);
     my $pickup_locations = $biblios->pickup_locations({ patron => $patron });
 
-For a given resultset, it returns all the pickup locations
+For a given resultset, it returns all the pickup locations.
+
+Patron is a required parameter.
 
 =cut
 
diff --git a/Koha/Item.pm b/Koha/Item.pm
index 0d29655938..5ed04bcca4 100644
--- a/Koha/Item.pm
+++ b/Koha/Item.pm
@@ -736,25 +736,26 @@ sub can_be_transferred {
 
 $pickup_locations = $item->pickup_locations( {patron => $patron } )
 
-Returns possible pickup locations for this item, according to patron's home library (if patron is defined and holds are allowed only from hold groups)
+Returns possible pickup locations for this item, according to patron's home library
 and if item can be transferred to each pickup location.
 
+Patron parameter is required.
+
 =cut
 
 sub pickup_locations {
     my ($self, $params) = @_;
 
     my $patron = $params->{patron};
+    # FIXME We should throw an exception if not passed
 
     my $circ_control_branch =
       C4::Reserves::GetReservesControlBranch( $self->unblessed(), $patron->unblessed );
     my $branchitemrule =
       C4::Circulation::GetBranchItemRule( $circ_control_branch, $self->itype );
 
-    if(defined $patron) {
-        return Koha::Libraries->new()->empty if $branchitemrule->{holdallowed} eq 'from_local_hold_group' && !$self->home_branch->validate_hold_sibling( {branchcode => $patron->branchcode} );
-        return Koha::Libraries->new()->empty if $branchitemrule->{holdallowed} eq 'from_home_library' && $self->home_branch->branchcode ne $patron->branchcode;
-    }
+    return Koha::Libraries->new()->empty if $branchitemrule->{holdallowed} eq 'from_local_hold_group' && !$self->home_branch->validate_hold_sibling( {branchcode => $patron->branchcode} );
+    return Koha::Libraries->new()->empty if $branchitemrule->{holdallowed} eq 'from_home_library' && $self->home_branch->branchcode ne $patron->branchcode;
 
     my $pickup_libraries = Koha::Libraries->search();
     if ($branchitemrule->{hold_fulfillment_policy} eq 'holdgroup') {
diff --git a/t/db_dependent/Koha/Biblios.t b/t/db_dependent/Koha/Biblios.t
index b93efebb21..bf9d36d216 100755
--- a/t/db_dependent/Koha/Biblios.t
+++ b/t/db_dependent/Koha/Biblios.t
@@ -278,6 +278,8 @@ subtest 'pickup_locations() tests' => sub {
     my $item_2_1 = $builder->build_sample_item({ biblionumber => $biblio_2->biblionumber });
     my $item_2_2 = $builder->build_sample_item({ biblionumber => $biblio_2->biblionumber });
 
+    my $patron = $builder->build_object({ class => 'Koha::Patrons' });
+
     my $biblios = Koha::Biblios->search(
         {
             biblionumber => [ $biblio_1->biblionumber, $biblio_2->biblionumber ]
@@ -298,7 +300,7 @@ subtest 'pickup_locations() tests' => sub {
     ];
 
     my $pickup_locations_ids = [
-        $biblios->pickup_locations->_resultset->get_column('branchcode')->all
+        $biblios->pickup_locations({ patron => $patron })->_resultset->get_column('branchcode')->all
     ];
 
     is_deeply(
-- 
2.39.5