|
Description
Adrien SAURAT
2013-03-20 10:10:01 UTC
Created attachment 16503 [details] [review] proposed patch #1 MAIN TEST PLAN 1) Apply patch 2) Start SIP server 3) Connect to your SIP server via telnet 3) Do a checkout/checkin on an item containing special characters in the title (see example below) 4) Without any encoding specified, you should see bad characters, like in "L'h�pital de A � Z" 5) Stop SIP server and configure SIPconfig.xml -> add encoding="ut8" in the login tag you're using for your tests. 6) Start SIP Server 7) Connect and do the same checkin/checkout. The accents should display correctly. == Additionnal informations == HOW TO PREPARE AN ITEM AND PATRON FOR THE TEST 1) Choose a biblio containing accents or change the title of an existing biblio, using for example this title : "L'hôpital de A à Z" 2) Note the barcode of an item attached to this biblio. In the following example I'll use the barcode "XX999XX" 3) Note a patron cardnumber (a valid account which can checkout). In the following example I'll use the cardnumber "PATRON01". CONNECTION TO THE SIP SERVER Mode 1: use "telnet localhost 8023", type your login/password (as defined in SIPconfig.xml) Mode 2 (RAW): use "telnet localhost 6001", then type the following request : "9300CNlogin|COpassword|CP" (with the login and password defined in SIPconfig.xml). You should receive a "941" message if you are connected. SIP REQUESTS FOR CHECKOUT/CHECKIN Checkout: 11YN20130320 10225920130230 102259AO|AAPATRON01|ABXXX999XX|AC| You should receive a response beginning with 121 (120 if the checkout failed, but the important thing is that you'll see the title and will be able to notice how the accents are displayed) Checkin: 09N20130320 10225920130130 102259AP|AO|ABXXX999XX|AC| You should receive a response beginning with 101 (100 if the checkin failed). $ 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 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. 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. Sounds like this is BLOCKED, then. 9531 has passed qa at this point, so I think it's time to move forward with this now. 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.
Created attachment 22808 [details] [review] proposed patch Hello Kyle, I have rebased the Adrien's patch, I think all is ok now. thx for testing Kyle, I include your suggest on : $msg = encode($encoding, $msg) if ( $encoding ); 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
Created attachment 23760 [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. :) 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" Hi Petter, if you find a problem with a patch feel free to set "Failed QA" - this status can be set by everyone. 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! Created attachment 24091 [details] [review] [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. 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: Petter Goksoyr Asen <boutrosboutrosboutros@gmail.com> 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> Created attachment 24127 [details] [review] Bug 9865 [QA Followup] - Give better usage examples in example SIPconfig.xml Pushed to master. Thanks, Adrien and Christophe! Pushed to 3.14.x, will be in 3.14.10 (In reply to Galen Charlton from comment #18) > Pushed to master. Thanks, Adrien and Christophe! Patch already included in 3.16.x branch |