Bug 33447: Make *->pickup_locations methods throw an exception on missing parameter

This patch fixes the FIXME for making the methods throw an exception.

Tests are added, and POD is adapted as well.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/patrons* \
           t/db_dependent/api/v1/holds.t \
           t/db_dependent/Reserves* \
           t/db_dependent/Hold* \
           t/db_dependent/Koha/Hold* \
           t/db_dependent/Items*

=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Koha/Biblio.pm

@@ -38,6 +38,7 @@ use Koha::Biblioitems;
 use Koha::Cache::Memory::Lite;
 use Koha::Checkouts;
 use Koha::CirculationRules;
+use Koha::Exceptions;
 use Koha::Item::Transfer::Limits;
 use Koha::Items;
 use Koha::Libraries;
@@ -255,18 +256,22 @@ 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 and if item can be transferred to each pickup location.
 
-Patron is a required parameter.
+Throws a I<Koha::Exceptions::MissingParameter> exception if the B<mandatory> parameter I<patron>
+is not passed.
 
 =cut
 
 sub pickup_locations {
     my ( $self, $params ) = @_;
 
+    Koha::Exceptions::MissingParameter->throw( parameter => 'patron' )
+      unless exists $params->{patron};
+
     my $patron = $params->{patron};
 
     my $memory_cache = Koha::Cache::Memory::Lite->get_instance();

Koha/Biblios.pm

@@ -42,13 +42,17 @@ Koha::Biblios - Koha Biblio object set class
 
 For a given resultset, it returns all the pickup locations.
 
-Patron is a required parameter.
+Throws a I<Koha::Exceptions::MissingParameter> exception if the B<mandatory> parameter I<patron>
+is not passed.
 
 =cut
 
 sub pickup_locations {
     my ( $self, $params ) = @_;
 
+    Koha::Exceptions::MissingParameter->throw( parameter => 'patron' )
+      unless exists $params->{patron};
+
     my $patron = $params->{patron};
 
     my @pickup_locations;

Koha/Item.pm

@@ -36,6 +36,7 @@ use Koha::Biblio::ItemGroups;
 use Koha::Checkouts;
 use Koha::CirculationRules;
 use Koha::CoverImages;
+use Koha::Exceptions;
 use Koha::Exceptions::Checkin;
 use Koha::Exceptions::Item::Bundle;
 use Koha::Exceptions::Item::Transfer;
@@ -734,20 +735,23 @@ sub can_be_transferred {
 
 =head3 pickup_locations
 
-$pickup_locations = $item->pickup_locations( {patron => $patron } )
+    my $pickup_locations = $item->pickup_locations({ patron => $patron })
 
 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.
+Throws a I<Koha::Exceptions::MissingParameter> exception if the B<mandatory> parameter I<patron>
+is not passed.
 
 =cut
 
 sub pickup_locations {
     my ($self, $params) = @_;
 
+    Koha::Exceptions::MissingParameter->throw( parameter => 'patron' )
+      unless exists $params->{patron};
+
     my $patron = $params->{patron};
-    # FIXME We should throw an exception if not passed
 
     my $circ_control_branch =
       C4::Reserves::GetReservesControlBranch( $self->unblessed(), $patron->unblessed );

t/db_dependent/Koha/Biblio.t

@@ -18,6 +18,7 @@
 use Modern::Perl;
 
 use Test::More tests => 23; # +1
+use Test::Exception;
 use Test::Warn;
 
 use C4::Biblio qw( AddBiblio ModBiblio ModBiblioMarc );
@@ -214,8 +215,9 @@ subtest 'is_serial() tests' => sub {
     $schema->storage->txn_rollback;
 };
 
-subtest 'pickup_locations' => sub {
-    plan tests => 9;
+subtest 'pickup_locations() tests' => sub {
+
+    plan tests => 11;
 
     $schema->storage->txn_begin;
 
@@ -357,6 +359,13 @@ subtest 'pickup_locations' => sub {
     my $biblio1  = $builder->build_sample_biblio({ title => '1' });
     my $biblio2  = $builder->build_sample_biblio({ title => '2' });
 
+    throws_ok
+      { $biblio1->pickup_locations }
+      'Koha::Exceptions::MissingParameter',
+      'Exception thrown on missing parameter';
+
+    is( $@->parameter, 'patron', 'Exception param correctly set' );
+
     my $item1_1  = $builder->build_sample_item({
         biblionumber     => $biblio1->biblionumber,
         homebranch       => $library1->branchcode,

t/db_dependent/Koha/Biblios.t

@@ -234,7 +234,7 @@ $schema->storage->txn_rollback;
 
 subtest 'pickup_locations() tests' => sub {
 
-    plan tests => 1;
+    plan tests => 3;
 
     $schema->storage->txn_begin;
 
@@ -286,6 +286,13 @@ subtest 'pickup_locations() tests' => sub {
         }
     );
 
+    throws_ok
+      { $biblios->pickup_locations }
+      'Koha::Exceptions::MissingParameter',
+      'Exception thrown on missing parameter';
+
+    is( $@->parameter, 'patron', 'Exception param correctly set' );
+
     my $library_ids = [
         Koha::Libraries->search(
             {

t/db_dependent/Koha/Item.t

@@ -443,8 +443,9 @@ subtest "as_marc_field() tests" => sub {
     Koha::Caches->get_instance->clear_from_cache( "MarcStructure-1-" );
 };
 
-subtest 'pickup_locations' => sub {
-    plan tests => 66;
+subtest 'pickup_locations() tests' => sub {
+
+    plan tests => 68;
 
     $schema->storage->txn_begin;
 
@@ -496,6 +497,12 @@ subtest 'pickup_locations' => sub {
         }
     );
 
+    throws_ok
+      { $item1->pickup_locations }
+      'Koha::Exceptions::MissingParameter',
+      'Exception thrown on missing parameter';
+
+    is( $@->parameter, 'patron', 'Exception param correctly set' );
 
     my $patron1 = $builder->build_object( { class => 'Koha::Patrons', value => { branchcode => $library1->branchcode, firstname => '1' } } );
     my $patron4 = $builder->build_object( { class => 'Koha::Patrons', value => { branchcode => $library4->branchcode, firstname => '4' } } );