|Summary:||3M self checkout don't work in France|
|Product:||Koha||Reporter:||Paul Poulain <paul.poulain>|
|Component:||SIP2||Assignee:||Colin Campbell <colin.campbell>|
|Status:||CLOSED FIXED||QA Contact:||Ian Walls <koha.sekjal>|
|Priority:||P2||CC:||adrien.saurat, chris, colin.campbell, frederic, gmcharlt, katrin.fischer, koha, marc, mtj, thatcher.rea, wizzyrea|
|Change sponsored?:||---||Patch complexity:||---|
|Who signed the patch off:||Text to go in the release notes:|
|Version(s) released in:|
|Bug Depends on:||5586|
patch fixing french 3M self-checkout
Bug 7619 Use CRLF as default terminator
Description Paul Poulain 2012-02-29 11:00:12 UTC
The 3M self checkout used to work perfectly for Koha 3.2 in France. Since Koha 3.6, they don't work anymore. I've asked some english natives, they don't have any problem with this feature, it seems it's french specific ? However, the patch to fix this problem is tiny, even if it was very hard to spot what/where was the problem. Frederic = you're encouraged to test & signoff this patch and confirm it fixes the problem. Galen = if i'm not wrong, you should be interested to test this patch, also for Evergreen. ALL ENGLISH ppl = you *MUST* also test that this patch don't *break* anything I don't understand why replacing a \r by a CRLF fixes the problem, and it may cause some problems to non-french self checkouts ! (I won't push this patch until i've strong feedback it don't break anything for others)
Comment 1 Paul Poulain 2012-02-29 11:01:37 UTC Comment hidden (obsolete)
Created attachment 7937 [details] [review] patch fixing french 3M self-checkout
Comment 2 Liz Rea 2012-02-29 17:35:15 UTC
Didn't we get rid of the \r on purpose some time back? Let me try to find the bug...
Comment 3 Liz Rea 2012-02-29 17:36:15 UTC
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5586 This one, maybe?
Comment 4 Katrin Fischer 2012-03-17 17:03:42 UTC
I think Liz is right - we changed it on purpose in the past. We updated one of our libraries since then and had no problems - it seems the self checks of this vendor support both. I am not sure if this should be a configuration option? Perhaps there is a way to configure the 3M self check machines to work with the new line endings?
Comment 5 Marc Balmer 2012-03-22 11:48:34 UTC
Shouldn't we adhere to the SIP2 specs? Or at least, if we do things differently add that as an option? We could define $EOL and set it either to CR or CR-LF, depending on a configuration otion, e.g. If that would be a good idea, I can try to add that (since I am working on the SIP server roght now anyways).
Comment 6 Paul Poulain 2012-03-23 09:29:48 UTC
(In reply to comment #5) > Shouldn't we adhere to the SIP2 specs? Or at least, if we do things > differently add that as an option? yes we should adhere to the SIP2 specs. But it seems 3M don't respect his own standard in France, we have to deal with this problem. > We could define $EOL and set it either to CR or CR-LF, depending on a > configuration otion, e.g. that's an option, right. My first request was to have someone testing the patch to see if it *break* SIP2 for others or not. I don't request a signoff to push the patch. (In reply to comment #4) > I think Liz is right - we changed it on purpose in the past. Yes I know, but it break french 3M hardware :( > We updated one > of our libraries since then and had no problems - it seems the self checks > of this vendor support both. I am not sure if this should be a configuration > option? Perhaps there is a way to configure the 3M self check machines to > work with the new line endings? We have investigated the machine and couldn't found anything about that. Lyon3 was supposed to ask 3M, I just added them in the cc list of this bug Siwtching back to need signoff and, I repeat, I won't push this patch without community agreement, I just want some feedback for now.
Comment 7 Katrin Fischer 2012-04-04 15:14:12 UTC
A short update on this: We updated one of our libraries to 3.6.3 this week and got reports about bad response times at their 3M self check stations. Checkouts and checkins were taking much longer than before. Applying Paul's patch fixed the problem. We suspect the station was waiting for CRLF.
Comment 8 Colin Campbell 2012-04-05 13:42:29 UTC
There are two problems one is the non flushing of output buffers which is done on CRLF and not if you just past \r. a second problem is that most clients are windows boxes and seem to lazily assume that everything is windows (or HTTP) and will send crlf. Although crlf is non what the standard dictates this will work for most clients (and if you use telnet you get crlf anyway!!). I've found one machine that it kills completely if you use crlf. I'm going to test a version of this patch but add the ability to override it and be strictly compliant in the SIPConfig which should mean that it shouldnt break any machine that is currently working. Longer term the code should separate the handling out of the sip data message and the transport (which Sip 3.0 really requires and sidesteps this issue)
Comment 9 Ian Walls 2012-04-16 18:35:27 UTC
This seems to also be related to bug 6620. $CRLF is used in C4/SIP/SIPServer.pm in the telnet_transport... but not anywhere else. Personally I think we should code for the standard as it's defined, not as it's implemented, but that's apt to make a lot of people running non-standard implementations very unhappy. If we can find a compromise that doesn't break SIP for one half of the world or the other, I'd be quite happy. Perhaps an entry in SIPConfig.xml?
Comment 10 Mason James 2012-05-01 05:03:02 UTC
(In reply to comment #9) > This seems to also be related to bug 6620. > > $CRLF is used in C4/SIP/SIPServer.pm in the telnet_transport... but not > anywhere else. > > Personally I think we should code for the standard as it's defined, not as > it's implemented, but that's apt to make a lot of people running > non-standard implementations very unhappy. If we can find a compromise that > doesn't break SIP for one half of the world or the other, I'd be quite > happy. > Perhaps an entry in SIPConfig.xml? i agree with Ian's idea for a SIPConfig.xml option, any other opinions?
Comment 11 Colin Campbell 2012-06-09 11:19:40 UTC
Telnet uses CRLF because thats the telnet protocol. The fact SIP is protocol agnostic (i.e. you can use sip by carrier pidgeon if you want) obscures the problem.
Comment 12 Colin Campbell 2012-06-09 14:31:19 UTC Comment hidden (obsolete)
Created attachment 10031 [details] [review] Revised Patch I was tempted to sign this off as I've had to implement my own version of Paul's patch on two or three sites. However I've abstracted it sightly using a constant for the message terminator and adding comments describing how to reset this. In the longer term we should extract this out to allow the parameter to be settable by the admin user.
Comment 13 Chris Cormack 2012-06-24 07:04:03 UTC
We have a SIPConfig.xml file, could we put the terminator in there? If no one can sees a floor in that plan, I'll go ahead and do it now.
Comment 14 Paul Poulain 2012-06-25 07:38:39 UTC
(In reply to comment #13) > We have a SIPConfig.xml file, could we put the terminator in there? If no > one can sees a floor in that plan, I'll go ahead and do it now. +1, that's the easiest & best option (even if, as the file is in gitted, it will require a patch, but that's not a big deal)
Comment 15 Chris Cormack 2012-06-25 07:42:46 UTC
Created attachment 10456 [details] [review] Bug 7619 Use CRLF as default terminator Most units require or tolerate CRLF as the terminator Using only \r causes many including 3M units to signal errors due to delay this resets CRLF to be default but adds a single constant variable and explanation so that if you have a "strictly conforming" unit (the 'Baby' is the only I have encountered you know where to make the change Signed-off-by: Chris Cormack <email@example.com>
Comment 16 Chris Cormack 2012-06-25 07:44:04 UTC
Have signed off the first patch, since this works and fixes the immediate problem. Working on a follow up to shift setting the line end character out of the .pm into the config file.
Comment 17 Chris Cormack 2012-06-25 08:45:13 UTC
Actually, the config object is not passed through to the write_msg sub, so it would take quite a bit of a rewrite. So for now, people will need to change the variable in the C4/SIP/Sip.pm. This works well enough, so I think it should move on to QA. The followup would still be nice, but I don't think we should hold up pushing this fix until then.
Comment 18 Mason James 2012-07-06 01:50:13 UTC
(In reply to comment #17) > Actually, the config object is not passed through to the write_msg sub, so > it would take quite a bit of a rewrite. > > So for now, people will need to change the variable in the C4/SIP/Sip.pm. > This works well enough, so I think it should move on to QA. The followup > would still be nice, but I don't think we should hold up pushing this fix > until then. Passing QA on this... - patch applies clean to master 'ce138ac96f3a5a729568f6d7db2e50ce3653b7bb' nice team-work everyone :)
Comment 19 Chris Cormack 2012-07-06 21:19:25 UTC
Pushed to 3.8.x will be in 3.8.3
Comment 20 Thatcher Rea 2012-07-31 15:53:18 UTC
US Customers using Overdrive are experiencing problems with this patch - we can fix as per Chris' suggestion to modify C4/SIP/Sip.pm in order to resolve the problem manually. +1 for rewriting this patch in the future to be configurable. :T
Comment 21 Paul Poulain 2012-08-27 11:54:40 UTC
(In reply to comment #20) > US Customers using Overdrive are experiencing problems with this patch - we > can fix as per Chris' suggestion to modify C4/SIP/Sip.pm in order to resolve > the problem manually. > > +1 for rewriting this patch in the future to be configurable. > > :T Hi Thatcher, What is "Overdrive" ? (a self-checkout vendor ?) Was everything OK before this patch is applied ?
Comment 22 Owen Leonard 2012-08-27 12:29:24 UTC
OverDrive (http://www.overdrive.com/) is an eBook vendor. After upgrading to 3.8.3 our patrons lost the ability to log in to OverDrive using SIP. The problem was fixed by reverting this patch.
Comment 23 Chris Cormack 2012-09-11 23:29:23 UTC
I can confirm this, however it can be fixed by just switching the variable. Having it in the SIPconfig.xml is still the best idea.