From fcd8e322f5248425c89fe28567d10866d0d76a36 Mon Sep 17 00:00:00 2001
From: Colin Campbell <colin.campbell@ptfs-europe.com>
Date: Wed, 24 Aug 2016 13:52:52 +0100
Subject: [PATCH] Bug 15438 - Checking out an on-hold item sends holder's
 borrowernumber in AF (screen message) field.

The returns from C4::Circulation::CanBookBeIssued used
to be structured as a hashref of entries like
   REASON => {
       data => 'foo',
       moredata => 'bar',
   };
Some entries still are. But many are now
   REASON => 1,
   data   => 'foo',
   moredata => 'bar',

The sip Checkout routine still assumed the former, as it
reports any causes it was not aware of (to maintain support for
a changing api) The data fields could leak into the screen message
field of the response. e.g. the borrowernumber or surname of the
borrower who has a hold on an issued title. Some real messages were
getting obscured by this

This patch sanatizes the return from from CanBookBeIssued
by removing keys which are not all uppercase
It also fixes a case where the key's data element was used
for the screen message when we should use the key itself

Updated the documentation of CanBookBeIssued to flag up
the assumption re case and the fact that 3 elements rather
than two may be returned

The loop through the returned keys was a bit bogus
so we now explicitly jump out if noerror is unset

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested quite extensively. Test results put on Bugzilla.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(cherry picked from commit 4d223bdc6156fab0667867f3990854fddfab5684)
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
---
 C4/Circulation.pm                  |  8 +++++-
 C4/SIP/ILS/Transaction/Checkout.pm | 42 ++++++++++++++++++++++--------
 2 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 28196a39e9..ba70d3ea3e 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -564,7 +564,7 @@ sub TooMany {
 
 =head2 CanBookBeIssued
 
-  ( $issuingimpossible, $needsconfirmation ) =  CanBookBeIssued( $borrower, 
+  ( $issuingimpossible, $needsconfirmation, [ $alerts ] ) =  CanBookBeIssued( $borrower,
                       $barcode, $duedate, $inprocess, $ignore_reserves, $params );
 
 Check if a book can be issued.
@@ -661,6 +661,12 @@ sticky due date is invalid or due date in the past
 
 if the borrower borrows to much things
 
+=head3 NB
+
+The assumption by users of the routine is that causes blocking
+the issue are keyed by uppercase labels and other returned
+data is keyed in lower case
+
 =cut
 
 sub CanBookBeIssued {
diff --git a/C4/SIP/ILS/Transaction/Checkout.pm b/C4/SIP/ILS/Transaction/Checkout.pm
index 14a2b66c68..eabe1cde89 100644
--- a/C4/SIP/ILS/Transaction/Checkout.pm
+++ b/C4/SIP/ILS/Transaction/Checkout.pm
@@ -56,19 +56,17 @@ sub do_checkout {
 	$debug and warn "do_checkout: patron (" . $patron_barcode . ")";
 	my $borrower = $self->{patron}->getmemberdetails_object();
 	$debug and warn "do_checkout borrower: . " . Dumper $borrower;
-	my ($issuingimpossible,$needsconfirmation) = CanBookBeIssued(
-        $borrower,
-        $barcode,
-        undef,
-        0,
+    my ($issuingimpossible, $needsconfirmation) = _can_we_issue($borrower, $barcode,
         C4::Context->preference("AllowItemsOnHoldCheckout")
     );
-	my $noerror=1;
-    if (scalar keys %$issuingimpossible) {
-        foreach (keys %$issuingimpossible) {
+
+    my $noerror=1;  # If set to zero we block the issue
+    if (keys %{$issuingimpossible}) {
+        foreach (keys %{$issuingimpossible}) {
             # do something here so we pass these errors
-            $self->screen_msg($_ . ': ' . $issuingimpossible->{$_});
+            $self->screen_msg("Issue failed : $_");
             $noerror = 0;
+            last;
         }
     } else {
         foreach my $confirmation (keys %{$needsconfirmation}) {
@@ -80,25 +78,30 @@ sub do_checkout {
                     $self->screen_msg("Item was reserved for you.");
                 } else {
                     $self->screen_msg("Item is reserved for another patron upon return.");
-                    # $noerror = 0;
+                    $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;
+                last;
             } elsif ($confirmation eq 'DEBT') {
                 $self->screen_msg('Outstanding Fines block issue');
                 $noerror = 0;
+                last;
             } elsif ($confirmation eq 'HIGHHOLDS') {
                 $overridden_duedate = $needsconfirmation->{$confirmation}->{returndate};
                 $self->screen_msg('Loan period reduced for high-demand item');
             } elsif ($confirmation eq 'RENTALCHARGE') {
                 if ($self->{fee_ack} ne 'Y') {
                     $noerror = 0;
+                    last;
                 }
             } else {
-                $self->screen_msg($needsconfirmation->{$confirmation});
+                # We've been returned a case other than those above
+                $self->screen_msg("Item cannot be issued: $confirmation");
                 $noerror = 0;
                 syslog('LOG_DEBUG', "Blocking checkout Reason:$confirmation");
+                last;
             }
         }
     }
@@ -145,5 +148,22 @@ sub do_checkout {
 	return $self;
 }
 
+sub _can_we_issue {
+    my ( $borrower, $barcode, $pref ) = @_;
+
+    my ( $issuingimpossible, $needsconfirmation, $alerts ) =
+      CanBookBeIssued( $borrower, $barcode, undef, 0, $pref );
+    for my $href ( $issuingimpossible, $needsconfirmation ) {
+
+        # some data is returned using lc keys we only
+        foreach my $key ( keys %{$href} ) {
+            if ( $key =~ m/[^A-Z_]/ ) {
+                delete $href->{$key};
+            }
+        }
+    }
+    return ( $issuingimpossible, $needsconfirmation );
+}
+
 1;
 __END__
-- 
2.39.5