From ff380d2a04fd04c98fff48ddc7b60cc8ff0fda49 Mon Sep 17 00:00:00 2001
From: Kyle Hall <kyle@bywatersolutions.com>
Date: Thu, 23 Sep 2021 08:57:30 -0400
Subject: [PATCH] Bug 29094: Adding hold via SIP should check if patron can
 hold item first [21.11.x]

When placing holds via SIP2, there is no holdability check. This seems very incorrect.

Test Plan:
1) Apply this patch
2) prove -r t/db_dependent/SIP

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
---
 C4/SIP/ILS/Transaction/Hold.pm   | 31 ++++++++-------
 t/db_dependent/SIP/ILS.t         |  9 ++++-
 t/db_dependent/SIP/Transaction.t | 66 +++++++++++++++++++++++++++++++-
 3 files changed, 90 insertions(+), 16 deletions(-)

diff --git a/C4/SIP/ILS/Transaction/Hold.pm b/C4/SIP/ILS/Transaction/Hold.pm
index 82fddf1e97..b3dae13860 100644
--- a/C4/SIP/ILS/Transaction/Hold.pm
+++ b/C4/SIP/ILS/Transaction/Hold.pm
@@ -7,7 +7,7 @@ use Modern::Perl;
 
 use C4::SIP::ILS::Transaction;
 
-use C4::Reserves qw( CalculatePriority AddReserve ModReserve );
+use C4::Reserves qw( CalculatePriority AddReserve ModReserve CanItemBeReserved );
 use Koha::Holds;
 use Koha::Patrons;
 use Koha::Items;
@@ -61,18 +61,23 @@ sub do_hold {
         return $self;
     }
 
-    my $priority = C4::Reserves::CalculatePriority($item->biblionumber);
-    AddReserve(
-        {
-            priority       => $priority,
-            branchcode     => $branch,
-            borrowernumber => $patron->borrowernumber,
-            biblionumber   => $item->biblionumber
-        }
-    );
-
-    # unfortunately no meaningful return value
-    $self->ok(1);
+    my $canReserve = CanItemBeReserved($patron, $item, $branch);
+    if ($canReserve->{status} eq 'OK') {
+        my $priority = C4::Reserves::CalculatePriority($item->biblionumber);
+        AddReserve(
+            {
+                priority       => $priority,
+                branchcode     => $branch,
+                borrowernumber => $patron->borrowernumber,
+                biblionumber   => $item->biblionumber
+            }
+        );
+
+        $self->ok(1);
+    } else {
+        $self->ok(0);
+    }
+
     return $self;
 }
 
diff --git a/t/db_dependent/SIP/ILS.t b/t/db_dependent/SIP/ILS.t
index c117e53763..11d52e0ce6 100755
--- a/t/db_dependent/SIP/ILS.t
+++ b/t/db_dependent/SIP/ILS.t
@@ -74,7 +74,14 @@ is( $ils->test_cardnumber_compare( 'A1234', 'b1234' ),
 subtest add_hold => sub {
     plan tests => 4;
 
-    my $library = $builder->build_object( { class => 'Koha::Libraries' } );
+    my $library = $builder->build_object(
+        {
+            class => 'Koha::Libraries',
+            value => {
+                pickup_location => 1
+            }
+        }
+    );
     my $patron = $builder->build_object(
         {
             class => 'Koha::Patrons',
diff --git a/t/db_dependent/SIP/Transaction.t b/t/db_dependent/SIP/Transaction.t
index 1f8cab65f2..bc3d714718 100755
--- a/t/db_dependent/SIP/Transaction.t
+++ b/t/db_dependent/SIP/Transaction.t
@@ -4,7 +4,7 @@
 # Current state is very rudimentary. Please help to extend it!
 
 use Modern::Perl;
-use Test::More tests => 12;
+use Test::More tests => 13;
 
 use Koha::Database;
 use t::lib::TestBuilder;
@@ -212,7 +212,14 @@ subtest cancel_hold => sub {
 subtest do_hold => sub {
     plan tests => 8;
 
-    my $library = $builder->build_object( { class => 'Koha::Libraries' } );
+    my $library = $builder->build_object(
+        {
+            class => 'Koha::Libraries',
+            value => {
+                pickup_location => 1
+            }
+        }
+    );
     my $patron_1 = $builder->build_object(
         {
             class => 'Koha::Patrons',
@@ -271,6 +278,61 @@ subtest do_hold => sub {
     is( $THE_hold->branchcode, $patron_2->branchcode, 'Hold placed from SIP should have the branchcode set' );
 };
 
+subtest "Placing holds via SIP check CanItemBeReserved" => sub {
+    plan tests => 4;
+
+    my $library = $builder->build_object(
+        {
+            class => 'Koha::Libraries',
+            value => {
+                pickup_location => 0
+            }
+        }
+    );
+    my $patron_1 = $builder->build_object(
+        {
+            class => 'Koha::Patrons',
+            value => {
+                branchcode => $library->branchcode,
+            }
+        }
+    );
+    my $patron_2 = $builder->build_object(
+        {
+            class => 'Koha::Patrons',
+            value => {
+                branchcode   => $library->branchcode,
+                categorycode => $patron_1->categorycode,
+            }
+        }
+    );
+
+    t::lib::Mocks::mock_userenv(
+        { branchcode => $library->branchcode, flags => 1 } );
+
+    my $item = $builder->build_sample_item(
+        {
+            library => $library->branchcode,
+        }
+    );
+
+    my $sip_patron  = C4::SIP::ILS::Patron->new( $patron_2->cardnumber );
+    my $sip_item    = C4::SIP::ILS::Item->new( $item->barcode );
+    my $transaction = C4::SIP::ILS::Transaction::Hold->new();
+    is(
+        ref $transaction,
+        "C4::SIP::ILS::Transaction::Hold",
+        "New transaction created"
+    );
+    is( $transaction->patron($sip_patron),
+        $sip_patron, "Patron assigned to transaction" );
+    is( $transaction->item($sip_item),
+        $sip_item, "Item assigned to transaction" );
+    my $hold = $transaction->do_hold();
+
+    is( $transaction->ok, 0, "Hold was not allowed" );
+};
+
 subtest do_checkin => sub {
     plan tests => 12;
 
-- 
2.30.2