From 865a1dab5c346920c61237c4b7593202328e04f8 Mon Sep 17 00:00:00 2001
From: Tomas Cohen Arazi <tomascohen@theke.io>
Date: Mon, 14 Dec 2020 07:37:53 -0300
Subject: [PATCH] Bug 27209: (follow-up) Add ->is_pickup_location_valid

This patch simply refactors the pickup location check into a method that
can be called on its own. Tests are added, and the tests for
->set_pickup_location should pass unmodified.

To test:
1. Apply the first two patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: tests pass
3. Apply this refactoring patch
4. Notice the tests are similar, but check for boolean output
5. Repeat 2
=> SUCCESS: New tests pass, set_pickup_location() behavior unchanged.
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
---
 Koha/Hold.pm               | 33 +++++++++++++++++----
 t/db_dependent/Koha/Hold.t | 60 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 86 insertions(+), 7 deletions(-)

diff --git a/Koha/Hold.pm b/Koha/Hold.pm
index d848962204..892fb20cd5 100644
--- a/Koha/Hold.pm
+++ b/Koha/Hold.pm
@@ -222,16 +222,19 @@ sub set_waiting {
     return $self;
 }
 
-=head3 set_pickup_location
+=head3 is_pickup_location_valid
 
-    $hold->set_pickup_location({ library_id => $library->id });
+    if ($hold->is_pickup_location_valid({ library_id => $library->id }) ) {
+        ...
+    }
 
-Updates the hold pickup location. It throws a I<Koha::Exceptions::Hold::InvalidPickupLocation> if
-the passed pickup location is not valid.
+Returns a I<boolean> representing if the passed pickup location is valid for the hold.
+It throws a I<Koha::Exceptions::_MissingParameter> if the library_id parameter is not
+passed.
 
 =cut
 
-sub set_pickup_location {
+sub is_pickup_location_valid {
     my ( $self, $params ) = @_;
 
     Koha::Exceptions::MissingParameter->throw('The library_id parameter is mandatory')
@@ -246,7 +249,25 @@ sub set_pickup_location {
         @pickup_locations = $self->biblio->pickup_locations({ patron => $self->patron });
     }
 
-    if ( any { $_->branchcode eq $params->{library_id} } @pickup_locations ) {
+    return any { $_->branchcode eq $params->{library_id} } @pickup_locations;
+}
+
+=head3 set_pickup_location
+
+    $hold->set_pickup_location({ library_id => $library->id });
+
+Updates the hold pickup location. It throws a I<Koha::Exceptions::Hold::InvalidPickupLocation> if
+the passed pickup location is not valid.
+
+=cut
+
+sub set_pickup_location {
+    my ( $self, $params ) = @_;
+
+    Koha::Exceptions::MissingParameter->throw('The library_id parameter is mandatory')
+        unless $params->{library_id};
+
+    if ( $self->is_pickup_location_valid({ library_id => $params->{library_id} }) ) {
         # all good, set the new pickup location
         $self->branchcode( $params->{library_id} )->store;
     }
diff --git a/t/db_dependent/Koha/Hold.t b/t/db_dependent/Koha/Hold.t
index dfe0c379c5..a6a5c4a7d6 100755
--- a/t/db_dependent/Koha/Hold.t
+++ b/t/db_dependent/Koha/Hold.t
@@ -19,7 +19,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 2;
+use Test::More tests => 3;
 
 use Test::Exception;
 use Test::MockModule;
@@ -140,3 +140,61 @@ subtest 'set_pickup_location() tests' => sub {
 
     $schema->storage->txn_rollback;
 };
+
+subtest 'is_pickup_location_valid() tests' => sub {
+
+    plan tests => 4;
+
+    $schema->storage->txn_begin;
+
+    my $mock_biblio = Test::MockModule->new('Koha::Biblio');
+    my $mock_item   = Test::MockModule->new('Koha::Item');
+
+    my $library_1 = $builder->build_object({ class => 'Koha::Libraries' });
+    my $library_2 = $builder->build_object({ class => 'Koha::Libraries' });
+    my $library_3 = $builder->build_object({ class => 'Koha::Libraries' });
+
+    # let's control what Koha::Biblio->pickup_locations returns, for testing
+    $mock_biblio->mock( 'pickup_locations', sub {
+        return Koha::Libraries->search( { branchcode => [ $library_2->branchcode, $library_3->branchcode ] } );
+    });
+    # let's mock what Koha::Item->pickup_locations returns, for testing
+    $mock_item->mock( 'pickup_locations', sub {
+        return Koha::Libraries->search( { branchcode => [ $library_2->branchcode, $library_3->branchcode ] } );
+    });
+
+    my $biblio = $builder->build_sample_biblio;
+    my $item   = $builder->build_sample_item({ biblionumber => $biblio->biblionumber });
+
+    # Test biblio-level holds
+    my $biblio_hold = $builder->build_object(
+        {
+            class => "Koha::Holds",
+            value => {
+                biblionumber => $biblio->biblionumber,
+                branchcode   => $library_3->branchcode,
+                itemnumber   => undef,
+            }
+        }
+    );
+
+    ok( !$biblio_hold->is_pickup_location_valid({ library_id => $library_1->branchcode }), 'Pickup location invalid');
+    ok( $biblio_hold->is_pickup_location_valid({ library_id => $library_2->id }), 'Pickup location valid');
+
+    # Test item-level holds
+    my $item_hold = $builder->build_object(
+        {
+            class => "Koha::Holds",
+            value => {
+                biblionumber => $biblio->biblionumber,
+                branchcode   => $library_3->branchcode,
+                itemnumber   => $item->itemnumber,
+            }
+        }
+    );
+
+    ok( !$item_hold->is_pickup_location_valid({ library_id => $library_1->branchcode }), 'Pickup location invalid');
+    ok( $item_hold->is_pickup_location_valid({ library_id => $library_2->id }), 'Pickup location valid' );
+
+    $schema->storage->txn_rollback;
+};
-- 
2.39.5