Attachment 114374 Details for Bug 27209 – Bug 27209: (follow-up) Add ->is_pickup_location_valid

[patch] Bug 27209: (follow-up) Add ->is_pickup_location_valid

Bug-27209-follow-up-Add--ispickuplocationvalid.patch (text/plain), 5.44 KB, created by Tomás Cohen Arazi (tcohen) on 2020-12-14 10:42:34 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tomás Cohen Arazi (tcohen)

Created: 2020-12-14 10:42:34 UTC

Size: 5.44 KB

patch

obsolete

>From 8e7cf335a84b8a6133d011db72c6e7a4234ed991 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>
>---
> 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 d848962204c..892fb20cd53 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 dfe0c379c55..a6a5c4a7d69 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.29.2

Actions: View | Diff | Splinter Review

Attachments on bug 27209: 114356 | 114357 | 114374 | 114378 | 114379 | 114380 | 114443 | 114444 | 114445 | 114448 | 114449 | 114450