From 338ba1d7692c1f75af7b0fb75eb5aac00a04a3df Mon Sep 17 00:00:00 2001
From: =?utf8?q?Joonas=20Kylm=C3=A4l=C3=A4?= <joonas.kylmala@helsinki.fi>
Date: Wed, 17 Feb 2021 14:17:36 +0200
Subject: [PATCH] Bug 25690: Remove duplicated logic in SIP2 checkouts

Because AllowItemsOnHoldCheckoutSIP only affects the checkoutability
of non-attached, i.e. RESERVED holds in SIP2 we can therefore use the
common code from CanBookBeIssued and ignore only the RESERVED confirmation
message case in SIP2 checkout code.

This slightly changes the checkout error message given for "In
processing" holds that someone other than the holdee tries to
checkout. Otherwise there is no logic changes. The message that this
changes is "Item is on hold for another patron." vs. now "Item cannot
be issued: $confirmation". It is easier to create follow-up patch to
properly add INPROCESSING confirmation to CanBookBeIssued and then
show correct message based on the CanBookBeIssued return value.

To test:
 1) Apply all patches from bug 25690 to get latest Transaction.t version
 2) prove t/db_dependent/SIP/Transaction.t => passes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
---
 C4/SIP/ILS/Transaction/Checkout.pm | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/C4/SIP/ILS/Transaction/Checkout.pm b/C4/SIP/ILS/Transaction/Checkout.pm
index 251a38f695..1be4974fe7 100644
--- a/C4/SIP/ILS/Transaction/Checkout.pm
+++ b/C4/SIP/ILS/Transaction/Checkout.pm
@@ -55,9 +55,7 @@ sub do_checkout {
     my $overridden_duedate; # usually passed as undef to AddIssue
     my $prevcheckout_block_checkout  = $account->{prevcheckout_block_checkout};
     $debug and warn "do_checkout borrower: . " . $patron->borrowernumber;
-    my ($issuingimpossible, $needsconfirmation) = _can_we_issue($patron, $barcode,
-        C4::Context->preference("AllowItemsOnHoldCheckoutSIP")
-    );
+    my ($issuingimpossible, $needsconfirmation) = _can_we_issue($patron, $barcode, 0);
 
     my $noerror=1;  # If set to zero we block the issue
     if (keys %{$issuingimpossible}) {
@@ -71,14 +69,15 @@ sub do_checkout {
         foreach my $confirmation (keys %{$needsconfirmation}) {
             if ($confirmation eq 'RENEW_ISSUE'){
                 $self->screen_msg("Item already checked out to you: renewing item.");
-            } elsif ($confirmation eq 'RESERVED' or $confirmation eq 'RESERVE_WAITING' or $confirmation eq 'TRANSFERRED') {
-                my $x = $self->{item}->available($patron->borrowernumber);
-                if ($x) {
-                    $self->screen_msg("Item was reserved for you.");
-                } else {
-                    $self->screen_msg("Item is reserved for another patron upon return.");
-                    $noerror = 0;
-                }
+            } elsif ($confirmation eq 'RESERVED' and !C4::Context->preference("AllowItemsOnHoldCheckoutSIP")) {
+                $self->screen_msg("Item is reserved for another patron upon return.");
+                $noerror = 0;
+            } elsif ($confirmation eq 'RESERVED' and C4::Context->preference("AllowItemsOnHoldCheckoutSIP")) {
+                next;
+            } elsif ($confirmation eq 'RESERVE_WAITING' or $confirmation eq 'TRANSFERRED') {
+               $debug and warn "Item is on hold for another patron.";
+               $self->screen_msg("Item is on hold for another patron.");
+               $noerror = 0;
             } elsif ($confirmation eq 'ISSUED_TO_ANOTHER') {
                 $self->screen_msg("Item already checked out to another patron.  Please return item for check-in.");
                 $noerror = 0;
@@ -113,15 +112,6 @@ sub do_checkout {
         }
     }
     my $itemnumber = $self->{item}->{itemnumber};
-    foreach (@$shelf) {
-        $debug and warn sprintf( "shelf has (%s for %s). this is (%is, %s)", $_->{itemnumber}, $_->{borrowernumber}, $itemnumber, $patron->borrowernumber );
-        ($_->{itemnumber} eq $itemnumber) or next;    # skip it if not this item
-        ($_->{borrowernumber} == $patron->borrowernumber) and last;
-            # if item was waiting for this patron, we're done.  AddIssue takes care of the "W" hold.
-        $debug and warn "Item is on hold for another patron.";
-        $self->screen_msg("Item is on hold for another patron.");
-        $noerror = 0;
-    }
     my ($fee, undef) = GetIssuingCharges($itemnumber, $patron->borrowernumber);
     if ( $fee > 0 ) {
         $self->{sip_fee_type} = '06';
-- 
2.39.5