|
Description
Kyle M Hall (khall)
2014-12-08 18:47:08 UTC
Created attachment 34187 [details] [review] Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out If an item is not checked out when a checkin via SIP2 is attempted, Koha's SIP server sends back an "ok" of 0, and the AF message "Item not checked out". I am not entirely sure this is good and correct behavior by the SIP2 protocol. In particular, this will cause SIP2 book sorting devices to fail on all items that are not checked out, causing them all to be put into the "special handling" been that should be reserved for things like items checked in at the wrong library and items on hold. Test Plan: 1) Apply the patch for bug 13159 so you can use the new enhanced SIP2 command line emulator 2) Use a command similar to the following to check in an item: sip_cli_emulator.pl -a localhost -su <sip user> -sp <sip password> -l <instituation id> --item <barcode> -m checkin 3) Note the 3rd character is 0, and there is an AF field saying the item is not checked out 4) Apply this patch 5) Restart the SIP server 6) Repeat steps 2-3, note that nothing has changed 7) In the SIP config file, Add the parameter checked_in_ok="1" to the SIP account you are using. 8) Restart the SIP server 9) Repeat steps 2-3, note that this time the 4rd character is 1, and you do not recieve the item not checked out message. Note, I believe this is really most likely a bug, but am reticent to change this SIP server's default behavior. Thus, I have made this a configurable option instead. I think this works ok for the self check but it would be good if it logged something in this case as we might need to track odd behaviour (e.g. bug 11039 ) Created attachment 35665 [details] [review] Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out If an item is not checked out when a checkin via SIP2 is attempted, Koha's SIP server sends back an "ok" of 0, and the AF message "Item not checked out". I am not entirely sure this is good and correct behavior by the SIP2 protocol. In particular, this will cause SIP2 book sorting devices to fail on all items that are not checked out, causing them all to be put into the "special handling" been that should be reserved for things like items checked in at the wrong library and items on hold. Test Plan: 1) Apply the patch for bug 13159 so you can use the new enhanced SIP2 command line emulator 2) Use a command similar to the following to check in an item: sip_cli_emulator.pl -a localhost -su <sip user> -sp <sip password> -l <instituation id> --item <barcode> -m checkin 3) Note the 3rd character is 0, and there is an AF field saying the item is not checked out 4) Apply this patch 5) Restart the SIP server 6) Repeat steps 2-3, note that nothing has changed 7) In the SIP config file, Add the parameter checked_in_ok="1" to the SIP account you are using. 8) Restart the SIP server 9) Repeat steps 2-3, note that this time the 4rd character is 1, and you do not recieve the item not checked out message. Not able to confirm expected behaviour. checkin of item checked out works ok: Trying 'checkin' SEND: 09Y201515 115144201515 115144APCPL|AOCPL|AB123456|ACautopass|BIN| READ: 101YNN20150205 115144AOCPL|AB123456|AQCPL|AJEn tittel|AA2| but checkin of not checked out item returns error: Trying 'checkin' SEND: 09Y201515 115147201515 115147APCPL|AOCPL|AB123456|ACautopass|BIN| Use of uninitialized value $data in concatenation (.) or string at ./sip_cli_emulator.pl line 270, <GEN0> chunk 1. READ: command used: perl -I. ./sip_cli_emulator.pl -l CPL -su autouser -sp autopass --port=6001 --address=localhost --item 123456 -m checkin --patron 2 output from SIPServer log: Use of uninitialized value $id in string ne at C4/SIP/ILS.pm line 84. Use of uninitialized value $_[1] in sprintf at /usr/lib/perl/5.14/Sys/Syslog.pm line 368. Use of uninitialized value in negation (-) at /usr/lib/perl5/DateTime.pm line 1764. DBD::mysql::st execute failed: called with 1 bind variables when 0 are needed at C4/Members.pm line 596. Can't use an undefined value as an ARRAY reference at /usr/lib/perl5/DBI.pm line 2054. Created attachment 35667 [details] [review] Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out If an item is not checked out when a checkin via SIP2 is attempted, Koha's SIP server sends back an "ok" of 0, and the AF message "Item not checked out". I am not entirely sure this is good and correct behavior by the SIP2 protocol. In particular, this will cause SIP2 book sorting devices to fail on all items that are not checked out, causing them all to be put into the "special handling" been that should be reserved for things like items checked in at the wrong library and items on hold. Test Plan: 1) Apply the patch for bug 13159 so you can use the new enhanced SIP2 command line emulator 2) Use a command similar to the following to check in an item: sip_cli_emulator.pl -a localhost -su <sip user> -sp <sip password> -l <instituation id> --item <barcode> -m checkin 3) Note the 3rd character is 0, and there is an AF field saying the item is not checked out 4) Apply this patch 5) Restart the SIP server 6) Repeat steps 2-3, note that nothing has changed 7) In the SIP config file, Add the parameter checked_in_ok="1" to the SIP account you are using. 8) Restart the SIP server 9) Repeat steps 2-3, note that this time the 4rd character is 1, and you do not recieve the item not checked out message. Created attachment 35669 [details] [review] Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out If an item is not checked out when a checkin via SIP2 is attempted, Koha's SIP server sends back an "ok" of 0, and the AF message "Item not checked out". I am not entirely sure this is good and correct behavior by the SIP2 protocol. In particular, this will cause SIP2 book sorting devices to fail on all items that are not checked out, causing them all to be put into the "special handling" been that should be reserved for things like items checked in at the wrong library and items on hold. Test Plan: 1) Apply the patch for bug 13159 so you can use the new enhanced SIP2 command line emulator 2) Use a command similar to the following to check in an item: sip_cli_emulator.pl -a localhost -su <sip user> -sp <sip password> -l <instituation id> --item <barcode> -m checkin 3) Note the 3rd character is 0, and there is an AF field saying the item is not checked out 4) Apply this patch 5) Restart the SIP server 6) Repeat steps 2-3, note that nothing has changed 7) In the SIP config file, Add the parameter checked_in_ok="1" to the SIP account you are using. 8) Restart the SIP server 9) Repeat steps 2-3, note that this time the 4rd character is 1, and you do not recieve the item not checked out message. Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> As a sidenote, I can confirm that if the returned character 3in the SIP Checkout Response is 0 for an item that is not checked out, it breaks automation and circulation logic altogether, as any item returned should trigger a transfer if applicable. Therefore I'd like to up the severity of this bug. The code looks good but I am not confident enough with this part of code to mark it as passed QA. Another pair of eyes needed. There is a conflict in ILS.pm - please rebase! Created attachment 36271 [details] [review] Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out If an item is not checked out when a checkin via SIP2 is attempted, Koha's SIP server sends back an "ok" of 0, and the AF message "Item not checked out". I am not entirely sure this is good and correct behavior by the SIP2 protocol. In particular, this will cause SIP2 book sorting devices to fail on all items that are not checked out, causing them all to be put into the "special handling" been that should be reserved for things like items checked in at the wrong library and items on hold. Test Plan: 1) Apply the patch for bug 13159 so you can use the new enhanced SIP2 command line emulator 2) Use a command similar to the following to check in an item: sip_cli_emulator.pl -a localhost -su <sip user> -sp <sip password> -l <instituation id> --item <barcode> -m checkin 3) Note the 3rd character is 0, and there is an AF field saying the item is not checked out 4) Apply this patch 5) Restart the SIP server 6) Repeat steps 2-3, note that nothing has changed 7) In the SIP config file, Add the parameter checked_in_ok="1" to the SIP account you are using. 8) Restart the SIP server 9) Repeat steps 2-3, note that this time the 4rd character is 1, and you do not recieve the item not checked out message. Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Created attachment 36310 [details] [review] Bug 13411 [QA Followup] - Add logging there is a small bug in the rebase on line 202:
Can't locate object method "Patron" via package "ILS" at C4/SIP/ILS.pm line 202.
$circ->patron( $patron = new ILS::Patron $item->{patron} );
should be:
$circ->patron($patron = C4::SIP::ILS::Patron->new( $item->{patron} ));
otherwise it breaks connection on checkin of checked out item
Created attachment 36428 [details] [review] Bug 13411 [QA Followup] - Add logging Added a fix for bug in C4/SIP/ILS.pm line 202 as described above. Tested OK. Setting to Signed Off as it works as previously noted. Colin, could you take a look at this again please? There are a few problems with these patches. First the existing behaviour is consistent with the standard ( 0 because a checkin has not succeeded) however I can see that there is probably no practical distinction between checkin of an unissued book and an issued one especially if some machines are routing returns incorrectly as a result. But we should document it as a change in behaviour so it gets reported as that in release notes (in case there are unforeseen consequences) The commit message also gives a test which does not succeed it says the message is not returned after the patch is applied, but it is (incidently I think it should still be returned as information, so the code is ok the test fails) Finally the patch adds a new parameter into the config files but there is a) no mention of it and b) it doesnt seem to be of any use in the implementation Actually,as Kyle mentioned above I'm also not entirely sure the current implementation is correct as to the SIP2 standard. Checkin is not limited to checked out items, it is, the way I see it,the only way for an ACS to validate the circulation status of an item. (hold,wrong branch,in transfer, etc.) Compare this to a package delivery service where every point involves a checkin. Any critical error in the process should fail the checkin response and give a suitable feedback. The "checked_in_ok" param from SIPConfig.xml is used as param in the checkin routine and fallbacks to old behaviour (item not checked out) if unset. Kyle,please correct me if wrong. This bug is actually a showstopper for us (50+ self-check machines and book sorting device) so I hope it can be resolved shortly (In reply to Benjamin Rokseth from comment #17) > Actually,as Kyle mentioned above I'm also not entirely sure the current > implementation is correct as to the SIP2 standard. Checkin is not limited to > checked out items, it is, the way I see it,the only way for an ACS to > validate the circulation status of an item. (hold,wrong branch,in transfer, > etc.) Compare this to a package delivery service where every point involves > a checkin. > > Any critical error in the process should fail the checkin response and give > a suitable feedback. > > The "checked_in_ok" param from SIPConfig.xml is used as param in the checkin > routine and fallbacks to old behaviour (item not checked out) if unset. > Kyle,please correct me if wrong. > > This bug is actually a showstopper for us (50+ self-check machines and book > sorting device) so I hope it can be resolved shortly You're spot on! Colin, could you take a look at the last 2 comments and update the status here? Thx! Comment on attachment 36310 [details] [review] Bug 13411 [QA Followup] - Add logging moved namespace fix to other patch of this bug. It has nothing to do with logging Created attachment 40952 [details] [review] Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out corrected line $circ->patron( $patron = new ILS::Patron $item->{patron} ); to $circ->patron($patron = C4::SIP::ILS::Patron->new( $item->{patron})); in patch, as it breaks checkin and conforms to namespace used in file Created attachment 40953 [details] [review] Bug 13411 [QA Followup] - Add logging Revert ordering of patches Created attachment 42035 [details] [review] Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out If an item is not checked out when a checkin via SIP2 is attempted, Koha's SIP server sends back an "ok" of 0, and the AF message "Item not checked out". I am not entirely sure this is good and correct behavior by the SIP2 protocol. In particular, this will cause SIP2 book sorting devices to fail on all items that are not checked out, causing them all to be put into the "special handling" been that should be reserved for things like items checked in at the wrong library and items on hold. Test Plan: 1) Apply the patch for bug 13159 so you can use the new enhanced SIP2 command line emulator 2) Use a command similar to the following to check in an item: sip_cli_emulator.pl -a localhost -su <sip user> -sp <sip password> -l <instituation id> --item <barcode> -m checkin 3) Note the 3rd character is 0, and there is an AF field saying the item is not checked out 4) Apply this patch 5) Restart the SIP server 6) Repeat steps 2-3, note that nothing has changed 7) In the SIP config file, Add the parameter checked_in_ok="1" to the SIP account you are using. 8) Restart the SIP server 9) Repeat steps 2-3, note that this time the 4rd character is 1, and you do not recieve the item not checked out message. Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Created attachment 42036 [details] [review] Bug 13411 [QA Followup] - Add logging Patch not working as advertised - in the rebasings it looks like we've lost the check of the config value to pass to checkin so the checked_in_ok variable is never passed Created attachment 42081 [details] [review] Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out If an item is not checked out when a checkin via SIP2 is attempted, Koha's SIP server sends back an "ok" of 0, and the AF message "Item not checked out". I am not entirely sure this is good and correct behavior by the SIP2 protocol. In particular, this will cause SIP2 book sorting devices to fail on all items that are not checked out, causing them all to be put into the "special handling" been that should be reserved for things like items checked in at the wrong library and items on hold. Test Plan: 1) Apply the patch for bug 13159 so you can use the new enhanced SIP2 command line emulator 2) Use a command similar to the following to check in an item: sip_cli_emulator.pl -a localhost -su <sip user> -sp <sip password> -l <instituation id> --item <barcode> -m checkin 3) Note the 3rd character is 0, and there is an AF field saying the item is not checked out 4) Apply this patch 5) Restart the SIP server 6) Repeat steps 2-3, note that nothing has changed 7) In the SIP config file, Add the parameter checked_in_ok="1" to the SIP account you are using. 8) Restart the SIP server 9) Repeat steps 2-3, note that this time the 4rd character is 1, and you do not recieve the item not checked out message. Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Created attachment 42082 [details] [review] Bug 13411 [QA Followup] - Add logging (In reply to Colin Campbell from comment #25) > Patch not working as advertised - in the rebasings it looks like we've lost > the check of the config value to pass to checkin so the checked_in_ok > variable is never passed Thanks for catching that! I've re-rebased the patch to correct that omission! having applied the patch against current master server terminates when processing an issue transaction (In reply to Colin Campbell from comment #29) > having applied the patch against current master server terminates when > processing an issue transaction I have been unable to reproduce this on master. It seems unlikely to be related to this patch since this patch only modifies the checkins, and not checkouts. Can you please retest? Restested and working (??) Message suppressed and ok set to 1 In the test plan step 9 it talks of 4th character but should be 3rd as in step 3 Although discharge is now ok and no message sent the alert flag is being returned as 'Y' which would indicate a condition requiring special action. should this not be set to N in this case? (In reply to Colin Campbell from comment #31) > Restested and working (??) > > Message suppressed and ok set to 1 Excellent! > In the test plan step 9 it talks of 4th character but should be 3rd as in > step 3 Sorry about that! I will revise the test plan. > Although discharge is now ok and no message sent the alert flag is being > returned as 'Y' which would indicate a condition requiring special action. > should this not be set to N in this case? This appears to be out of the scope of this bug. Here is a checkin with the item checked out to a patron SEND: 09N20151015 07240220151015 072402APCPL|AOCPL|AB0140620125|ACstaff|BIN| READ: 101YNY20151015 072402AOCPL|AB0140620125|AQMY|AJWuthering heights /|AA23529000035676|CSPZ3.B7902 W36|CV04| Here is a checkin of the same item without it being checked out: SEND: 09N20151015 07222220151015 072222APCPL|AOCPL|AB0140620125|ACstaff|BIN| READ: 101YNY20151015 072222AOCPL|AB0140620125|AQMY|AJWuthering heights /|CSPZ3.B7902 W36|CV04| Here is the same checkin with a checkout on master: SEND: 09N20151015 07271820151015 072718APCPL|AOCPL|AB0140620125|ACstaff|BIN| READ: 101YNY20151015 072719AOCPL|AB0140620125|AQMY|AJWuthering heights /|AA23529000035676|CSPZ3.B7902 W36|CV04| And on master without a checkout: SEND: 09N20151015 07280320151015 072803APCPL|AOCPL|AB0140620125|ACstaff|BIN| READ: 100YNY20151015 072803AOCPL|AB0140620125|AQMY|AJWuthering heights /|CSPZ3.B7902 W36|CV04|AFItem not checked out| So it appears that Koha always sets the alert flag no matter what. I think we should file a new bug report for this! (In reply to Kyle M Hall from comment #32) > > In the test plan step 9 it talks of 4th character but should be 3rd as in > > step 3 > > Sorry about that! I will revise the test plan. Setting the status accordingly. Created attachment 43590 [details] [review] Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out If an item is not checked out when a checkin via SIP2 is attempted, Koha's SIP server sends back an "ok" of 0, and the AF message "Item not checked out". I am not entirely sure this is good and correct behavior by the SIP2 protocol. In particular, this will cause SIP2 book sorting devices to fail on all items that are not checked out, causing them all to be put into the "special handling" been that should be reserved for things like items checked in at the wrong library and items on hold. Test Plan: 1) Apply the patch for bug 13159 so you can use the new enhanced SIP2 command line emulator 2) Use a command similar to the following to check in an item: sip_cli_emulator.pl -a localhost -su <sip user> -sp <sip password> -l <instituation id> --item <barcode> -m checkin 3) Note the 3rd character is 0, and there is an AF field saying the item is not checked out 4) Apply this patch 5) Restart the SIP server 6) Repeat steps 2-3, note that nothing has changed 7) In the SIP config file, Add the parameter checked_in_ok="1" to the SIP account you are using. 8) Restart the SIP server 9) Repeat steps 2-3, note that this time the 3rd character is 1, and you do not recieve the item not checked out message. Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Created attachment 43591 [details] [review] Bug 13411 [QA Followup] - Add logging (In reply to Kyle M Hall from comment #34) > 9) Repeat steps 2-3, note that this time the 3rd character is 1, and you do > not recieve the item not checked out message. Why don't we want to display the message? (In reply to Jonathan Druart from comment #36) > (In reply to Kyle M Hall from comment #34) > > 9) Repeat steps 2-3, note that this time the 3rd character is 1, and you do > > not recieve the item not checked out message. > > Why don't we want to display the message? Because some SIP2 sorters will behave badly if we send the message. Created attachment 45027 [details] [review] [SIGNED-OFF] Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out If an item is not checked out when a checkin via SIP2 is attempted, Koha's SIP server sends back an "ok" of 0, and the AF message "Item not checked out". I am not entirely sure this is good and correct behavior by the SIP2 protocol. In particular, this will cause SIP2 book sorting devices to fail on all items that are not checked out, causing them all to be put into the "special handling" been that should be reserved for things like items checked in at the wrong library and items on hold. Test Plan: 1) Apply the patch for bug 13159 so you can use the new enhanced SIP2 command line emulator 2) Use a command similar to the following to check in an item: sip_cli_emulator.pl -a localhost -su <sip user> -sp <sip password> -l <instituation id> --item <barcode> -m checkin 3) Note the 3rd character is 0, and there is an AF field saying the item is not checked out 4) Apply this patch 5) Restart the SIP server 6) Repeat steps 2-3, note that nothing has changed 7) In the SIP config file, Add the parameter checked_in_ok="1" to the SIP account you are using. 8) Restart the SIP server 9) Repeat steps 2-3, note that this time the 3rd character is 1, and you do not recieve the item not checked out message. Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com> Created attachment 45028 [details] [review] [SIGNED-OFF] Bug 13411 [QA Followup] - Add logging Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com> Patches pushed to master. Thanks Kyle! This patch has been pushed to 3.20.x, will be in 3.20.6. Pushed to 3.18.13, and released. As I understand this option "checked_in_ok" allows check-ins for items not checked out. Just feel that the name only poorly reflects that. Which is a pity since we won't adjust such option names at upgrade time. The added $circ->ok(...) has an interesting side-effect on bug 18996. Just sharing that I have a patch in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15221 that turns off the alert bit when using checked_in_ok, if that's of interest to people. We're having issues with SIP2 sorters because the alert is causing incorrect sorting for books that haven't been checked out, even when using checked_in_ok (which removes the screen message and marks it as OK). |