Bug 35353: Add REST API endpoint to retrieve old holds

Same as checkout but for holds, we need to provide a way to retrieve old
holds for a patron.

Test plan:
Create some holds for a patron, cancel and fulfill some, then use the
REST API endpoint with the new 'old' flag set to 1
  /api/v1/patrons/42/holds?old=1

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
(cherry picked from commit 7a32231a52)
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Koha/Hold.pm

@@ -574,13 +574,25 @@ sub item_group {
 
 =head3 branch
 
-Returns the related Koha::Library object for this Hold
+Returns the related Koha::Library object for this hold
+
+DEPRECATED
 
 =cut
 
 sub branch {
+    return shift->pickup_library(@_);
+}
+
+=head3 pickup_library
+
+Returns the related Koha::Library object for this hold
+
+=cut
+
+sub pickup_library {
     my ($self) = @_;
-    my $rs = $self->_result->branchcode;
+    my $rs = $self->_result->pickup_library;
     return Koha::Library->_new_from_dbic($rs);
 }
 

Koha/Old/Hold.pm

@@ -47,6 +47,32 @@ sub biblio {
     return Koha::Biblio->_new_from_dbic($rs);
 }
 
+=head3 item
+
+Returns the related Koha::Item object for this old Hold
+
+=cut
+
+sub item {
+    my ($self) = @_;
+    my $rs = $self->_result->itemnumber;
+    return unless $rs;
+    return Koha::Item->_new_from_dbic($rs);
+}
+
+=head3 pickup_library
+
+Returns the related Koha::Biblio object for this old hold
+
+=cut
+
+sub pickup_library {
+    my ($self) = @_;
+    my $rs = $self->_result->pickup_library;
+    return unless $rs;
+    return Koha::Library->_new_from_dbic($rs);
+}
+
 =head3 anonymize
 
     $old_hold->anonymize();

Koha/REST/V1/Holds.pm

@@ -26,6 +26,7 @@ use C4::Reserves;
 use Koha::Items;
 use Koha::Patrons;
 use Koha::Holds;
+use Koha::Old::Holds;
 use Koha::DateUtils qw( dt_from_string );
 
 use List::MoreUtils qw( any );
@@ -44,11 +45,18 @@ Method that handles listing Koha::Hold objects
 sub list {
     my $c = shift->openapi->valid_input or return;
 
+    my $old = $c->param('old');
+    $c->req->params->remove('old');
+
     return try {
-        my $holds = $c->objects->search( Koha::Holds->new );
+        my $holds_set =
+              $old
+            ? Koha::Old::Holds->new
+            : Koha::Holds->new;
+
+        my $holds = $c->objects->search($holds_set);
         return $c->render( status => 200, openapi => $holds );
-    }
+    } catch {
-    catch {
         $c->unhandled_exception($_);
     };
 }

Koha/REST/V1/Patrons/Holds.pm

@@ -49,16 +49,18 @@ sub list {
         );
     }
 
+    my $old = $c->param('old');
+    $c->req->params->remove('old');
+
     return try {
+        my $holds_set =
+            $old
+            ? $patron->old_holds
+            : $patron->holds;
 
-        my $holds = $c->objects->search( $patron->holds );
+        my $holds = $c->objects->search( $holds_set );
-
+        return $c->render( status => 200, openapi => $holds );
-        return $c->render(
+    } catch {
-            status  => 200,
-            openapi => $holds
-        );
-    }
-    catch {
         $c->unhandled_exception($_);
     };
 }

api/v1/swagger/definitions/hold.yaml

@@ -108,5 +108,20 @@ properties:
       - boolean
       - "null"
     description: Cancellation requests count for the hold (x-koha-embed)
+  biblio:
+    type:
+      - object
+      - "null"
+    description: Bibliographic record
+  item:
+    type:
+      - object
+      - "null"
+    description: The item
+  pickup_library:
+    type:
+      - object
+      - "null"
+    description: Pickup library
 
 additionalProperties: false

api/v1/swagger/paths/holds.yaml

@@ -88,6 +88,11 @@
       - $ref: "../swagger.yaml#/parameters/q_param"
       - $ref: "../swagger.yaml#/parameters/q_body"
       - $ref: "../swagger.yaml#/parameters/request_id_header"
+      - name: old
+        in: query
+        description: By default, current holds are returned, when this is true then
+          old holds are returned as result.
+        type: boolean
       - name: x-koha-embed
         in: header
         required: false
@@ -97,6 +102,8 @@
           type: string
           enum:
             - cancellation_requested
+            - biblio
+            - pickup_library
           collectionFormat: csv
     produces:
       - application/json

api/v1/swagger/paths/patrons_holds.yaml

@@ -15,6 +15,11 @@
       - $ref: "../swagger.yaml#/parameters/q_param"
       - $ref: "../swagger.yaml#/parameters/q_body"
       - $ref: "../swagger.yaml#/parameters/request_id_header"
+      - name: old
+        in: query
+        description: By default, current holds are returned, when this is true then
+          old holds are returned as result.
+        type: boolean
       - name: x-koha-embed
         in: header
         required: false
@@ -24,6 +29,10 @@
           type: string
           enum:
             - cancellation_requested
+            - biblio
+            - item
+            - pickup_library
+            - pickup_library.branchname
         collectionFormat: csv
     produces:
       - application/json

t/db_dependent/Koha/Hold.t

@@ -19,7 +19,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 13;
+use Test::More tests => 14;
 
 use Test::Exception;
 use Test::MockModule;
@@ -87,6 +87,23 @@ subtest 'biblio() tests' => sub {
     $schema->storage->txn_rollback;
 };
 
+subtest 'pickup_library/branch tests' => sub {
+
+    plan tests => 1;
+
+    $schema->storage->txn_begin;
+
+    my $hold = $builder->build_object(
+        {
+            class => 'Koha::Holds',
+        }
+    );
+
+    is( ref( $hold->pickup_library ), 'Koha::Library', '->pickup_library should return a Koha::Library object' );
+
+    $schema->storage->txn_rollback;
+};
+
 subtest 'fill() tests' => sub {
 
     plan tests => 14;

t/db_dependent/Koha/Old/Hold.t

@@ -17,7 +17,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 2;
+use Test::More tests => 3;
 use Test::Exception;
 
 use Koha::Database;
@@ -118,3 +118,28 @@ subtest 'biblio() tests' => sub {
 
     $schema->storage->txn_rollback;
 };
+
+subtest 'item/pickup_library() tests' => sub {
+
+    plan tests => 4;
+
+    $schema->storage->txn_begin;
+
+    my $old_hold = $builder->build_object(
+        {
+            class => 'Koha::Old::Holds',
+        }
+    );
+    is( ref( $old_hold->item ),           'Koha::Item',    '->item should return a Koha::Item object' );
+    is( ref( $old_hold->pickup_library ), 'Koha::Library', '->pickup_library should return a Koha::Library object' );
+
+    $old_hold->item->delete;
+    $old_hold->pickup_library->delete;
+
+    $old_hold = $old_hold->get_from_storage;    # Be on the safe side
+
+    is( $old_hold->item,           undef, 'Item has been deleted, ->item should return undef' );
+    is( $old_hold->pickup_library, undef, 'Library has been deleted, ->pickup_library should return undef' );
+
+    $schema->storage->txn_rollback;
+};

t/db_dependent/api/v1/patrons_holds.t

@@ -35,7 +35,7 @@ t::lib::Mocks::mock_preference( 'RESTBasicAuth', 1 );
 
 subtest 'list() tests' => sub {
 
-    plan tests => 9;
+    plan tests => 18;
 
     $schema->storage->txn_begin;
 
@@ -51,12 +51,33 @@ subtest 'list() tests' => sub {
       ->status_is( 200, 'SWAGGER3.2.2' )
       ->json_is( [] );
 
-    my $hold_1 = $builder->build_object({ class => 'Koha::Holds', value => { borrowernumber => $patron->id } });
+    my $hold_1 = $builder->build_object( { class => 'Koha::Holds', value => { borrowernumber => $patron->id } } );
-    my $hold_2 = $builder->build_object({ class => 'Koha::Holds', value => { borrowernumber => $patron->id } });
+    my $hold_2 = $builder->build_object( { class => 'Koha::Holds', value => { borrowernumber => $patron->id } } );
+    my $hold_3 = $builder->build_object( { class => 'Koha::Holds', value => { borrowernumber => $patron->id } } );
 
-    $t->get_ok("//$userid:$password@/api/v1/patrons/" . $patron->id . '/holds?_order_by=+me.hold_id')
+    $t->get_ok( "//$userid:$password@/api/v1/patrons/" . $patron->id . '/holds?_order_by=+me.hold_id' )
         ->status_is( 200, 'SWAGGER3.2.2' )
-      ->json_is( '' => [ $hold_1->to_api, $hold_2->to_api ], 'Holds retrieved' );
+        ->json_is( '' => [ $hold_1->to_api, $hold_2->to_api, $hold_3->to_api ], 'Holds retrieved' );
+
+    $hold_1->fill;
+    $hold_3->fill;
+
+    $t->get_ok( "//$userid:$password@/api/v1/patrons/" . $patron->id . '/holds?_order_by=+me.hold_id' )
+        ->status_is( 200, 'SWAGGER3.2.2' )->json_is( '' => [ $hold_2->to_api ], 'Only current holds retrieved' );
+
+    $t->get_ok( "//$userid:$password@/api/v1/patrons/" . $patron->id . '/holds?old=1&_order_by=+me.hold_id' )
+        ->status_is( 200, 'SWAGGER3.2.2' )
+        ->json_is( '' => [ $hold_1->to_api, $hold_3->to_api ], 'Only old holds retrieved' );
+
+    my $old_hold_1 = Koha::Old::Holds->find( $hold_1->id );
+    $old_hold_1->item->delete;
+    $old_hold_1->pickup_library->delete;
+
+    $t->get_ok( "//$userid:$password@/api/v1/patrons/" . $patron->id . '/holds?old=1&_order_by=+me.hold_id' )
+        ->status_is( 200, 'SWAGGER3.2.2' )->json_is(
+        '' => [ $old_hold_1->get_from_storage->to_api, $hold_3->to_api ],
+        'Old holds even after item and library removed'
+        );
 
     my $non_existent_patron = $builder->build_object({ class => 'Koha::Patrons' });
     my $non_existent_patron_id = $non_existent_patron->id;