Bug 9865

Summary: Make SIP2 message encoding configurable via SIPconfig.xml
Product: Koha Reporter: Adrien SAURAT <adrien.saurat>
Component: SIP2Assignee: Christophe Croullebois <christophe.croullebois>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P5 - low CC: boutrosboutrosboutros, christophe.croullebois, colin.campbell, fridolin.somers, gmcharlt, gwilliams, katrin.fischer, kyle, magnus, mtj
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 9531    
Bug Blocks:    
Attachments: proposed patch #1
proposed patch
Bug 9865: SIP msg encoding configurable via SIPconfig.xml
[SIGNED-OFF] Sorry about the confusion. I forgot to start the SIP server manually, using the koha-start-sip command instead. So naturally then I couldnt see any effect.
Bug 9865: SIP msg encoding configurable via SIPconfig.xml
Bug 9865 [QA Followup] - Give better usage examples in example SIPconfig.xml

Description Adrien SAURAT 2013-03-20 10:10:01 UTC
Based on bug 9531 by Kyle Hall.

We sometimes need to change the output charset to a specific encoding (mostly utf8). Being able to choose an encoding in the account line of SIPconfig.xml would be very useful.
Comment 1 Adrien SAURAT 2013-03-20 11:29:03 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-03-20 13:37:32 UTC
$ git am -3 /tmp/proposed-patch-1-z_97sy.patch
Applying: Bug 9865: SIP msg encoding configurable via SIPconfig.xml
fatal: sha1 information is lacking or useless (C4/SIP/Sip.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 9865: SIP msg encoding configurable via SIPconfig.xml
Comment 3 Adrien SAURAT 2013-03-20 14:40:33 UTC
Yes, you can't apply it yet directly on the master. I noted your patch 9531 as a prerequisite, and you should be able to test this if you install the 9531 before.

But this can wait, I did set a "Needs Signoff" status but in fact it's in "stand by" until the 9531 is merged in master, so we can leave this as "doesn't apply" for now.

It seemed to me easier to handle this as a successor to 9531 because both patches add a new parameter when calling the "write_msg" function. Seems better to know once and for all which order will be used.
Comment 4 Kyle M Hall 2013-03-25 12:25:41 UTC
That makes sense. Thanks!

(In reply to comment #3)
> Yes, you can't apply it yet directly on the master. I noted your patch 9531
> as a prerequisite, and you should be able to test this if you install the
> 9531 before.
> 
> But this can wait, I did set a "Needs Signoff" status but in fact it's in
> "stand by" until the 9531 is merged in master, so we can leave this as
> "doesn't apply" for now.
> 
> It seemed to me easier to handle this as a successor to 9531 because both
> patches add a new parameter when calling the "write_msg" function. Seems
> better to know once and for all which order will be used.
Comment 5 Magnus Enger 2013-03-25 13:06:07 UTC
Sounds like this is BLOCKED, then.
Comment 6 Kyle M Hall 2013-08-01 11:25:45 UTC
9531 has passed qa at this point, so I think it's time to move forward with this now.
Comment 7 Kyle M Hall 2013-08-01 11:36:52 UTC
I'm still getting
fatal: sha1 information is lacking or useless (C4/SIP/Sip.pm)

Could you upload a new copy of the patch?

Also, a minor stylistic suggestions:
What do you think about changing

$encoding ||= q{};
	
my $cksum;

if ($encoding ne '') {
    $msg = encode($encoding, $msg);
}

to

$msg = encode($encoding, $msg) if ( $encoding );
	
my $cksum;

as it groups the code a bit more logically and tersely ( and maybe a picosecond faster ). With this, the possible undef doesn't need to be converted to an empty string, as there is no need for a string comparison.
Comment 8 Christophe Croullebois 2013-11-08 11:36:58 UTC Comment hidden (obsolete)
Comment 9 Christophe Croullebois 2013-11-08 11:39:05 UTC
Kyle,
I include your suggest on :
$msg = encode($encoding, $msg) if ( $encoding );
Comment 10 Colin Campbell 2013-11-08 15:24:56 UTC
Christophe would it be a good idea to ensure that an encoding attribute exists in the Account object, set to q{} if none exists in the config file? That way existing sip configurations can be used without runtime warnings being generated because the encoding attribute is not present
Comment 11 Chris Cormack 2013-12-22 21:35:22 UTC Comment hidden (obsolete)
Comment 12 Petter Goksøyr Åsen 2014-01-02 15:22:50 UTC
Thank you for this patch!
It works great for item titles, but I still get mangled chars if there is a non ascii char in the Patron name; for example if I make a patron status request:

REQUEST: 6301220131216    135338          YAOKOHA|AA1|ACBLA|ADpass|BP1|BQ99|
RESPONSE 64              01220140102    161959000200000003000000000000AOHUTL|AA1|AEKeyser S�ze|BLY|CQN|CC5|PCADMIN|PIY|AFGreetings from Koha. |

"Keyser S�ze" should be "Keyser Söze"
Comment 13 Katrin Fischer 2014-01-02 15:30:10 UTC
Hi Petter, if you find a problem with a patch feel free to set "Failed QA" - this status can be set by everyone.
Comment 14 Petter Goksøyr Åsen 2014-01-02 17:31:18 UTC
Thanks Katrin - I wasn't aware of that.

But anyway - please disregard my last comment about Patron name mangled chars. It may be a problem with my setup. I checked again now and it seems my items also have problems with accents in titles. I only checked with with foreign scripts like russian or arabic alphabets - but they appear to work even without this patch. So clearly there is something funky about my testresults. 

I will investigate again tomorrow!
Comment 15 Petter Goksøyr Åsen 2014-01-09 14:29:31 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2014-01-10 13:34:31 UTC
Created attachment 24124 [details] [review]
Bug 9865: SIP msg encoding configurable via SIPconfig.xml

The accounts->login tag in SIPconfig.xml can now accept a new
parameter, "encoding". It will be mostly used to encore to utf8.
For this, simply add the parameter:
encoding="utf8"
note: this patch was made by Adrien Saurat I have simply rebased it

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Works as advertised, does nothing if encoding is not set.
Blows up all the machines that can't handled utf8 if it is set :) But
that's not Koha's fault. :)

Signed-off-by: Petter Goksoyr Asen <boutrosboutrosboutros@gmail.com>

But now I did it the right way! And I can confirm that this patch solves
all issues with mangled characters in SIP messages. Confirmed that it
looks good with Norwegian characters in patron name and in book titles.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2014-01-10 13:37:23 UTC
Created attachment 24127 [details] [review]
Bug 9865 [QA Followup] - Give better usage examples in example SIPconfig.xml
Comment 18 Galen Charlton 2014-04-30 16:34:45 UTC
Pushed to master.  Thanks, Adrien and Christophe!
Comment 19 Fridolin Somers 2014-08-01 13:58:32 UTC
Pushed to 3.14.x, will be in 3.14.10
Comment 20 Mason James 2015-02-25 13:29:41 UTC
(In reply to Galen Charlton from comment #18)
> Pushed to master.  Thanks, Adrien and Christophe!

Patch already included in 3.16.x branch