Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out
Summary: Koha's SIP server returns not ok for checking in items that are not checked out
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 15221
  Show dependency treegraph
 
Reported: 2014-12-08 18:47 UTC by Kyle M Hall
Modified: 2017-07-28 12:33 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out (4.85 KB, patch)
2014-12-08 18:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out (4.95 KB, patch)
2015-02-04 20:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out (4.95 KB, patch)
2015-02-05 11:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out (5.03 KB, patch)
2015-02-05 12:38 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out (5.01 KB, patch)
2015-03-02 10:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13411 [QA Followup] - Add logging (981 bytes, patch)
2015-03-02 14:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13411 [QA Followup] - Add logging (1.41 KB, patch)
2015-03-03 17:15 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out (5.15 KB, patch)
2015-07-12 19:41 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 13411 [QA Followup] - Add logging (1007 bytes, patch)
2015-07-12 19:57 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out (3.66 KB, patch)
2015-08-27 14:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13411 [QA Followup] - Add logging (997 bytes, patch)
2015-08-27 14:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out (5.08 KB, patch)
2015-08-28 14:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13411 [QA Followup] - Add logging (997 bytes, patch)
2015-08-28 14:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out (5.08 KB, patch)
2015-10-19 13:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13411 [QA Followup] - Add logging (997 bytes, patch)
2015-10-19 13:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13411 - Koha's SIP server returns not ok for checking in items that are not checked out (5.16 KB, patch)
2015-11-19 20:14 UTC, Brendan Gallagher
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13411 [QA Followup] - Add logging (1.03 KB, patch)
2015-11-19 20:15 UTC, Brendan Gallagher
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2014-12-08 18:47:08 UTC
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.
Comment 1 Kyle M Hall 2014-12-08 18:53:46 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2014-12-08 18:54:42 UTC
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.
Comment 3 Colin Campbell 2015-01-08 15:10:10 UTC
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 )
Comment 4 Kyle M Hall 2015-02-04 20:03:28 UTC Comment hidden (obsolete)
Comment 5 Benjamin Rokseth 2015-02-05 11:06:03 UTC
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.
Comment 6 Kyle M Hall 2015-02-05 11:15:33 UTC Comment hidden (obsolete)
Comment 7 Benjamin Rokseth 2015-02-05 12:38:03 UTC Comment hidden (obsolete)
Comment 8 Benjamin Rokseth 2015-02-10 08:26:47 UTC
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.
Comment 9 Jonathan Druart 2015-02-12 10:42:24 UTC
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.
Comment 10 Katrin Fischer 2015-02-28 22:23:28 UTC
There is a conflict in ILS.pm - please rebase!
Comment 11 Kyle M Hall 2015-03-02 10:58:22 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2015-03-02 14:08:33 UTC Comment hidden (obsolete)
Comment 13 Benjamin Rokseth 2015-03-03 09:57:53 UTC
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
Comment 14 Benjamin Rokseth 2015-03-03 17:15:57 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2015-03-05 07:10:54 UTC
Colin, could you take a look at this again please?
Comment 16 Colin Campbell 2015-03-23 15:10:00 UTC
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
Comment 17 Benjamin Rokseth 2015-03-27 07:21:27 UTC
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
Comment 18 Kyle M Hall 2015-04-15 19:01:54 UTC
(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!
Comment 19 Katrin Fischer 2015-05-04 05:28:02 UTC
Colin, could you take a look at the last 2 comments and update the status here? Thx!
Comment 20 Benjamin Rokseth 2015-07-12 19:35:29 UTC
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
Comment 21 Benjamin Rokseth 2015-07-12 19:41:29 UTC Comment hidden (obsolete)
Comment 22 Benjamin Rokseth 2015-07-12 19:57:38 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2015-08-27 14:58:06 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2015-08-27 14:58:13 UTC Comment hidden (obsolete)
Comment 25 Colin Campbell 2015-08-28 09:12:11 UTC
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
Comment 26 Kyle M Hall 2015-08-28 14:03:19 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2015-08-28 14:03:26 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2015-08-28 14:04:50 UTC
(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!
Comment 29 Colin Campbell 2015-10-07 12:00:01 UTC
having applied the patch against current master server terminates when processing an issue transaction
Comment 30 Kyle M Hall 2015-10-09 13:11:32 UTC
(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?
Comment 31 Colin Campbell 2015-10-15 09:54:18 UTC
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?
Comment 32 Kyle M Hall 2015-10-15 11:30:12 UTC
(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!
Comment 33 Jonathan Druart 2015-10-19 11:37:48 UTC
(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.
Comment 34 Kyle M Hall 2015-10-19 13:05:04 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2015-10-19 13:05:12 UTC Comment hidden (obsolete)
Comment 36 Jonathan Druart 2015-10-30 10:28:29 UTC
(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?
Comment 37 Kyle M Hall 2015-10-30 12:43:26 UTC
(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.
Comment 38 Brendan Gallagher 2015-11-19 20:14:45 UTC
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>
Comment 39 Brendan Gallagher 2015-11-19 20:15:08 UTC
Created attachment 45028 [details] [review]
[SIGNED-OFF] Bug 13411 [QA Followup] - Add logging

Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>
Comment 40 Tomás Cohen Arazi 2015-11-19 23:25:02 UTC
Patches pushed to master.

Thanks Kyle!
Comment 41 Frédéric Demians 2015-11-29 21:11:24 UTC
This patch has been pushed to 3.20.x, will be in 3.20.6.
Comment 42 Liz Rea 2016-01-06 21:05:25 UTC
Pushed to 3.18.13, and released.
Comment 43 Marcel de Rooy 2017-07-28 12:33:16 UTC
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.