From 0293c773c5a45d72d1250e37bd06600fa550d475 Mon Sep 17 00:00:00 2001 From: Nick Clemens 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 --- 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 a94665ddc81..cb898f4bc3e 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 cf1cbdb83ad..af8de37cb1b 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 0d29655938d..5ed04bcca44 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 b93efebb212..bf9d36d216f 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.34.1