Attachment 153473 Details for Bug 33471 – Bug 33471: Add validate_pickup_location routine to Koha::Biblio

[patch] Bug 33471: Add validate_pickup_location routine to Koha::Biblio

Bug-33471-Add-validatepickuplocation-routine-to-Ko.patch (text/plain), 5.82 KB, created by Nick Clemens (kidclamp) on 2023-07-14 11:38:45 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

Created: 2023-07-14 11:38:45 UTC

Size: 5.82 KB

patch

obsolete

>From 130c35b03667f843b858f5a76ff53ce1d2f3c99a Mon Sep 17 00:00:00 2001
>From: Nick Clemens <nick@bywatersolutions.com>
>Date: Sun, 9 Apr 2023 20:06:05 +0000
>Subject: [PATCH] Bug 33471: Add validate_pickup_location routine to
> Koha::Biblio
>
>This patch adds a new routine, similar to pickup_locations except that
>it short circuits once a location has been found as acceptable.
>
>To test:
>1 - Attempt placing a hold for an acceptable pickup location via the API
>2 - Attempt to change pickup location to an invalid location, you are blocked
>3 - Attempt to place a another hold via API for an invalid locatoin, you are blocked
>4 - Apply patch
>5 - Repeat
>6 - Results should be the same
>
>Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
>---
> Koha/Biblio.pm                | 35 +++++++++++++++++++++++++++++++++++
> Koha/Hold.pm                  |  4 +++-
> Koha/REST/V1/Holds.pm         |  4 +---
> t/db_dependent/Koha/Biblio.t  | 11 ++++++++++-
> t/db_dependent/api/v1/holds.t |  8 ++++----
> 5 files changed, 53 insertions(+), 9 deletions(-)
>
>diff --git a/Koha/Biblio.pm b/Koha/Biblio.pm
>index 26dc83f2d4..82f586c8d6 100644
>--- a/Koha/Biblio.pm
>+++ b/Koha/Biblio.pm
>@@ -285,6 +285,41 @@ sub can_be_transferred {
> }
> 
> 
>+=head3 validate_pickup_location
>+
>+    $biblio->validate_pickup_location( {patron => $patron, pickup_location_id => $branchcode } );
>+
>+Returns 1 if the pickup location is valid, nothing if it is not. This function is similar to, but distinct
>+from pickup_locations in that it does not need to calculate all valid locations, but simply to check until
>+1 item is found that validates the passed location. Rather than confusing the logic in that sub, some code
>+is duplicated.
>+
>+=cut
>+
>+sub validate_pickup_location {
>+    my ( $self, $params ) = @_;
>+
>+    my $patron     = $params->{patron};
>+    my $branchcode = $params->{pickup_location_id};
>+
>+    my %seen;
>+    foreach my $item ( $self->items->as_list ) {
>+
>+        # The 5 variables below determine the valid locations for any item, no need to check twice
>+        my $cache_key = sprintf "Pickup_locations:%s:%s:%s:%s:%s",
>+            $item->itype, $item->homebranch, $item->holdingbranch, $item->ccode || "", $patron->branchcode || "";
>+        next if $seen{$cache_key};
>+
>+        my @item_pickup_locations =
>+            $item->pickup_locations( { patron => $patron } )->_resultset->get_column('branchcode')->all;
>+        $seen{$cache_key} = 1;
>+
>+        return 1 if grep { $branchcode eq $_ } @item_pickup_locations;
>+    }
>+
>+    return;
>+}
>+
> =head3 pickup_locations
> 
>     my $pickup_locations = $biblio->pickup_locations({ patron => $patron });
>diff --git a/Koha/Hold.pm b/Koha/Hold.pm
>index f275530d97..bd614035fe 100644
>--- a/Koha/Hold.pm
>+++ b/Koha/Hold.pm
>@@ -298,12 +298,14 @@ sub is_pickup_location_valid {
> 
>     if ( $self->itemnumber ) { # item-level
>         $pickup_locations = $self->item->pickup_locations({ patron => $self->patron });
>+        return any { $_->branchcode eq $params->{library_id} } $pickup_locations->as_list;
>     }
>     else { # biblio-level
>         $pickup_locations = $self->biblio->pickup_locations({ patron => $self->patron });
>+        return $self->biblio->validate_pickup_location(
>+            { patron => $self->patron, pickup_location_id => $params->{library_id} } );
>     }
> 
>-    return any { $_->branchcode eq $params->{library_id} } $pickup_locations->as_list;
> }
> 
> =head3 set_pickup_location
>diff --git a/Koha/REST/V1/Holds.pm b/Koha/REST/V1/Holds.pm
>index a8a998ef12..7c55b1e635 100644
>--- a/Koha/REST/V1/Holds.pm
>+++ b/Koha/REST/V1/Holds.pm
>@@ -155,9 +155,7 @@ sub add {
>             }
>             else {
>                 $valid_pickup_location =
>-                  any { $_->branchcode eq $pickup_library_id }
>-                $biblio->pickup_locations(
>-                    { patron => $patron } )->as_list;
>+                  $biblio->validate_pickup_location({ patron => $patron, pickup_location_id  => $pickup_library_id });
>             }
> 
>             return $c->render(
>diff --git a/t/db_dependent/Koha/Biblio.t b/t/db_dependent/Koha/Biblio.t
>index 4bc53195ec..235c63dff9 100755
>--- a/t/db_dependent/Koha/Biblio.t
>+++ b/t/db_dependent/Koha/Biblio.t
>@@ -217,7 +217,7 @@ subtest 'is_serial() tests' => sub {
> 
> subtest 'pickup_locations() tests' => sub {
> 
>-    plan tests => 11;
>+    plan tests => 75;
> 
>     $schema->storage->txn_begin;
> 
>@@ -442,6 +442,15 @@ subtest 'pickup_locations() tests' => sub {
>               . ' but returns '
>               . scalar(@pl)
>         );
>+
>+        foreach my $branchcode (@branchcodes) {
>+            my $valid;
>+            $valid = 1 if grep { $branchcode eq $_ } @pl;
>+            is(
>+                $valid, $biblio->validate_pickup_location( { patron => $patron, pickup_location_id => $branchcode } ),
>+                "Validate pickup location returns the expected result"
>+            );
>+        }
>     }
> 
>     foreach my $cbranch ('ItemHomeLibrary','PatronLibrary') {
>diff --git a/t/db_dependent/api/v1/holds.t b/t/db_dependent/api/v1/holds.t
>index adc5429660..0e8d2c946b 100755
>--- a/t/db_dependent/api/v1/holds.t
>+++ b/t/db_dependent/api/v1/holds.t
>@@ -1248,8 +1248,8 @@ subtest 'add() tests' => sub {
>       ->status_is(201);
> 
>     # empty cases
>-    $mock_biblio->mock( 'pickup_locations', sub {
>-        return Koha::Libraries->new->empty;
>+    $mock_biblio->mock( 'validate_pickup_location', sub {
>+        return;
>     });
> 
>     $t->post_ok( "//$userid:$password@/api/v1/holds" => json => $biblio_hold_data )
>@@ -1257,8 +1257,8 @@ subtest 'add() tests' => sub {
>       ->json_is({ error => 'The supplied pickup location is not valid' });
> 
>     # empty cases
>-    $mock_item->mock( 'pickup_locations', sub {
>-        return Koha::Libraries->new->empty;
>+    $mock_item->mock( 'validate_pickup_location', sub {
>+        return;
>     });
> 
>     $t->post_ok( "//$userid:$password@/api/v1/holds" => json => $item_hold_data )
>-- 
>2.30.2

Actions: View | Diff | Splinter Review

Attachments on bug 33471: 149379 | 149411 | 153473