Attachment 137706 Details for Bug 29094 – Bug 29094: Adding hold via SIP should check if patron can hold item first

[patch] Bug 29094: Adding hold via SIP should check if patron can hold item first

Bug-29094-Adding-hold-via-SIP-should-check-if-patr.patch (text/plain), 5.41 KB, created by Martin Renvoize (ashimema) on 2022-07-14 09:55:08 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2022-07-14 09:55:08 UTC

Size: 5.41 KB

patch

obsolete

>From 3c8c0e35862d8a263387c81fbc297ab1d7c8a4a4 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
>
>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 abc441a748..e96dd4ce55 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 f11efa8038..b17b33e635 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 => 14;
>+use Test::More tests => 15;
> 
> 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.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 29094: 125189 | 136570 | 136611 | 137706 | 139830