Attachment 149379 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.59 KB, created by Nick Clemens (kidclamp) on 2023-04-09 20:09:38 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

Created: 2023-04-09 20:09:38 UTC

Size: 5.59 KB

patch

obsolete

>From 962e59e58e13a0539b70c1a65b21ef72f99046c9 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
>---
> Koha/Biblio.pm                | 32 ++++++++++++++++++++++++++++++++
> Koha/Hold.pm                  |  3 ++-
> Koha/REST/V1/Holds.pm         |  4 +---
> t/db_dependent/Koha/Biblio.t  |  9 ++++++++-
> t/db_dependent/api/v1/holds.t |  8 ++++----
> 5 files changed, 47 insertions(+), 9 deletions(-)
>
>diff --git a/Koha/Biblio.pm b/Koha/Biblio.pm
>index a94665ddc8..19b11bdb91 100644
>--- a/Koha/Biblio.pm
>+++ b/Koha/Biblio.pm
>@@ -252,6 +252,38 @@ sub can_be_transferred {
>     return 0;
> }
> 
>+=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
> 
>diff --git a/Koha/Hold.pm b/Koha/Hold.pm
>index a5a9d03003..162635f39e 100644
>--- a/Koha/Hold.pm
>+++ b/Koha/Hold.pm
>@@ -298,12 +298,13 @@ 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 e3957a7a67..4156c877d3 100755
>--- a/t/db_dependent/Koha/Biblio.t
>+++ b/t/db_dependent/Koha/Biblio.t
>@@ -215,7 +215,7 @@ subtest 'is_serial() tests' => sub {
> };
> 
> subtest 'pickup_locations' => sub {
>-    plan tests => 9;
>+    plan tests => 73;
> 
>     $schema->storage->txn_begin;
> 
>@@ -433,6 +433,13 @@ subtest 'pickup_locations' => 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