From b0b7ee378b5c33940875f6f8196b65a7c084eacb Mon Sep 17 00:00:00 2001
From: Kyle M Hall <kyle@bywatersolutions.com>
Date: Tue, 17 Sep 2024 18:28:11 +0000
Subject: [PATCH] Bug 37582: Add ability to for SIP to convert control and
 separator characters to spaces

This came up with a SIP vendor recently - when reading responses they were only getting part of a patron's information.
It seemed to have been caused by patrons with multiple restrictions and a new line in the response

This patch also does minor refactoring to write_msg to pass the server variable that contains
all the previously passed parameters, and removes the never used "file" parameter.

Test Plan:
1) Add multiple debarments to a patron
2) Run a sip patron information response, note the presence of newlines
3) Apply this patch
4) Add the new option convert_control_characters to your SIP account,
   set the value to " -- "
5) Restart all the things!
7) Run a sip patron information response, note the newlines have been replaced!

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
---
 C4/SIP/Sip.pm         | 19 +++++++++----------
 C4/SIP/Sip/MsgType.pm | 34 +++++++++++++++++-----------------
 etc/SIPconfig.xml     |  2 ++
 3 files changed, 28 insertions(+), 27 deletions(-)

diff --git a/C4/SIP/Sip.pm b/C4/SIP/Sip.pm
index c29bff282f..cfaa202dac 100644
--- a/C4/SIP/Sip.pm
+++ b/C4/SIP/Sip.pm
@@ -187,11 +187,16 @@ sub boolspace {
 #
 
 sub write_msg {
-    my ($self, $msg, $file, $terminator, $encoding) = @_;
+    my ($self, $msg, $server) = @_;
+    my $terminator = $server->{account}->{terminator};
+    my $encoding = $server->{account}->{encoding};
 
     $terminator ||= q{};
     $terminator = ( $terminator eq 'CR' ) ? $CR : $CRLF;
 
+    my $separator = $server->{account}->{convert_nonprinting_characters};
+    $msg =~ s/[^[:print:]]/$separator/g if defined $separator;
+
     $msg = encode($encoding, $msg) if ( $encoding );
 
     my $cksum;
@@ -206,15 +211,9 @@ sub write_msg {
         $msg .= sprintf('%04.4X', $cksum);
     }
 
-
-    if ($file) {
-        $file->autoflush(1);
-        print $file $msg, $terminator;
-    } else {
-        STDOUT->autoflush(1);
-        print $msg, $terminator;
-        siplog("LOG_INFO", "OUTPUT MSG: '$msg'");
-    }
+    STDOUT->autoflush(1);
+    print $msg, $terminator;
+    siplog("LOG_INFO", "OUTPUT MSG: '$msg'");
 
     $last_response = $msg;
 }
diff --git a/C4/SIP/Sip/MsgType.pm b/C4/SIP/Sip/MsgType.pm
index 12bfd526f5..6ad48672fd 100644
--- a/C4/SIP/Sip/MsgType.pm
+++ b/C4/SIP/Sip/MsgType.pm
@@ -509,7 +509,7 @@ sub handle_patron_status {
         $koha_patron->update_lastseen('connection');
     }
     $resp = build_patron_status( $patron, $lang, $fields, $server );
-    $self->write_msg( $resp, undef, $server->{account}->{terminator}, $server->{account}->{encoding} );
+    $self->write_msg( $resp, $server );
     return (PATRON_STATUS_REQ);
 }
 
@@ -649,7 +649,7 @@ sub handle_checkout {
         }
     }
 
-    $self->write_msg( $resp, undef, $server->{account}->{terminator}, $server->{account}->{encoding} );
+    $self->write_msg( $resp, $server );
     return (CHECKOUT);
 }
 
@@ -748,7 +748,7 @@ sub handle_checkin {
     $resp .= maybe_add( FID_SCREEN_MSG, $status->screen_msg, $server );
     $resp .= maybe_add( FID_PRINT_LINE, $status->print_line, $server );
 
-    $self->write_msg( $resp, undef, $server->{account}->{terminator}, $server->{account}->{encoding} );
+    $self->write_msg( $resp, $server );
 
     return (CHECKIN);
 }
@@ -795,7 +795,7 @@ sub handle_block_patron {
     }
 
     $resp = build_patron_status( $patron, $patron->language, $fields, $server );
-    $self->write_msg( $resp, undef, $server->{account}->{terminator}, $server->{account}->{encoding} );
+    $self->write_msg( $resp, $server );
     return (BLOCK_PATRON);
 }
 
@@ -836,7 +836,7 @@ sub handle_request_acs_resend {
 
         # We haven't sent anything yet, so respond with a
         # REQUEST_SC_RESEND msg (p. 16)
-        $self->write_msg( REQUEST_SC_RESEND, undef, $server->{account}->{terminator}, $server->{account}->{encoding} );
+        $self->write_msg( REQUEST_SC_RESEND, $server );
     } elsif ( ( length($last_response) < 9 )
         || substr( $last_response, -9, 2 ) ne 'AY' ) {
 
@@ -850,7 +850,7 @@ sub handle_request_acs_resend {
         # Cut out the sequence number and checksum, since the old
         # checksum is wrong for the resent message.
         my $rebuilt = substr( $last_response, 0, -9 );
-        $self->write_msg( $rebuilt, undef, $server->{account}->{terminator}, $server->{account}->{encoding} );
+        $self->write_msg( $rebuilt, $server );
     }
 
     return REQUEST_ACS_RESEND;
@@ -932,7 +932,7 @@ sub handle_login {
         $status = login_core( $server, $uid, $pwd );
     }
 
-    $self->write_msg( LOGIN_RESP . $status, undef, $server->{account}->{terminator}, $server->{account}->{encoding} );
+    $self->write_msg( LOGIN_RESP . $status, $server );
     return $status ? LOGIN : '';
 }
 
@@ -1110,7 +1110,7 @@ sub handle_patron_info {
         $resp .= maybe_add( FID_SCREEN_MSG, INVALID_CARD, $server );
     }
 
-    $self->write_msg( $resp, undef, $server->{account}->{terminator}, $server->{account}->{encoding} );
+    $self->write_msg( $resp, $server );
     return (PATRON_INFO);
 }
 
@@ -1137,7 +1137,7 @@ sub handle_end_patron_session {
     $resp .= maybe_add( FID_SCREEN_MSG, $screen_msg, $server );
     $resp .= maybe_add( FID_PRINT_LINE, $print_line, $server );
 
-    $self->write_msg( $resp, undef, $server->{account}->{terminator}, $server->{account}->{encoding} );
+    $self->write_msg( $resp, $server );
 
     return (END_PATRON_SESSION);
 }
@@ -1219,7 +1219,7 @@ sub handle_fee_paid {
     $resp .= maybe_add( FID_SCREEN_MSG,     $status->screen_msg, $server );
     $resp .= maybe_add( FID_PRINT_LINE,     $status->print_line, $server );
 
-    $self->write_msg( $resp, undef, $server->{account}->{terminator}, $server->{account}->{encoding} );
+    $self->write_msg( $resp, $server );
 
     return (FEE_PAID);
 }
@@ -1312,7 +1312,7 @@ sub handle_item_information {
         $resp .= $item->build_custom_field_string( $server );
     }
 
-    $self->write_msg( $resp, undef, $server->{account}->{terminator}, $server->{account}->{encoding} );
+    $self->write_msg( $resp, $server );
 
     return (ITEM_INFORMATION);
 }
@@ -1362,7 +1362,7 @@ sub handle_item_status_update {
     $resp .= maybe_add( FID_SCREEN_MSG, $status->screen_msg, $server );
     $resp .= maybe_add( FID_PRINT_LINE, $status->print_line, $server );
 
-    $self->write_msg( $resp, undef, $server->{account}->{terminator}, $server->{account}->{encoding} );
+    $self->write_msg( $resp, $server );
 
     return (ITEM_STATUS_UPDATE);
 }
@@ -1416,7 +1416,7 @@ sub handle_patron_enable {
 
     $resp .= add_field( FID_INST_ID, $ils->institution, $server );
 
-    $self->write_msg( $resp, undef, $server->{account}->{terminator}, $server->{account}->{encoding} );
+    $self->write_msg( $resp, $server );
 
     return (PATRON_ENABLE);
 }
@@ -1481,7 +1481,7 @@ sub handle_hold {
     $resp .= maybe_add( FID_SCREEN_MSG, $status->screen_msg, $server );
     $resp .= maybe_add( FID_PRINT_LINE, $status->print_line, $server );
 
-    $self->write_msg( $resp, undef, $server->{account}->{terminator}, $server->{account}->{encoding} );
+    $self->write_msg( $resp, $server );
 
     return (HOLD);
 }
@@ -1568,7 +1568,7 @@ sub handle_renew {
     $resp .= maybe_add( FID_SCREEN_MSG, $status->screen_msg, $server );
     $resp .= maybe_add( FID_PRINT_LINE, $status->print_line, $server );
 
-    $self->write_msg( $resp, undef, $server->{account}->{terminator}, $server->{account}->{encoding} );
+    $self->write_msg( $resp, $server );
 
     return (RENEW);
 }
@@ -1621,7 +1621,7 @@ sub handle_renew_all {
     $resp .= maybe_add( FID_SCREEN_MSG, $status->screen_msg, $server );
     $resp .= maybe_add( FID_PRINT_LINE, $status->print_line, $server );
 
-    $self->write_msg( $resp, undef, $server->{account}->{terminator}, $server->{account}->{encoding} );
+    $self->write_msg( $resp, $server );
 
     return (RENEW_ALL);
 }
@@ -1727,7 +1727,7 @@ sub send_acs_status {
 
     # Do we want to tell the terminal its location?
 
-    $self->write_msg( $msg, undef, $server->{account}->{terminator}, $server->{account}->{encoding} );
+    $self->write_msg( $msg, $server );
     return 1;
 }
 
diff --git a/etc/SIPconfig.xml b/etc/SIPconfig.xml
index a1432b13f3..22f2f0faf0 100644
--- a/etc/SIPconfig.xml
+++ b/etc/SIPconfig.xml
@@ -59,6 +59,7 @@
       <login id="lpl-sc" password="1234" institution="LPL" allow_fields="AO,AA,AE"/>
       <!-- allow_fields hides all fields not in the list, it is the inverse of hide_fields ( hide_fields takes precedence ) -->
       <login id="lpl-sc-beacock" password="xyzzy"
+             convert_nonprinting_characters=" "
              delimiter="|" error-detect="enabled" institution="LPL"
              send_patron_home_library_in_af="1"
              cv_send_00_on_success="1"
@@ -84,6 +85,7 @@
              lost_status_for_missing="4"
              blocked_item_types="VM|MU"
              seen_on_item_information="mark_found"> <!-- could be "keep_lost", empty to disable -->
+             <!-- convert_nonprinting_characters: Convert control and non-space separator characters into the givens string -->
              <!-- show_checkin_message: If enabled, successful checking responses will contain an AF screen message -->
              <!-- lost_block_checkout sets flag if patron has more than the given current checkouts that are lost ( itemlost > 0 by default ) -->
              <!-- lost_block_checkout_value determines the minimum lost item value to count ( that is, the value in items.itemlost ) -->
-- 
2.39.5