Bug 3638 - Status of hold not changed when item checked in via SIP2 Interface
Summary: Status of hold not changed when item checked in via SIP2 Interface
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: 3.8
Hardware: All All
: P2 normal
Assignee: Colin Campbell
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-16 08:22 UTC by Colin Campbell
Modified: 2013-12-05 19:59 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Proposed Patch (1.61 KB, patch)
2010-02-16 17:15 UTC, Chris Cormack
Details | Diff | Splinter Review
Improved Patch (5.08 KB, patch)
2011-07-07 09:09 UTC, Colin Campbell
Details | Diff | Splinter Review
test script example (2.28 KB, text/plain)
2011-08-15 15:37 UTC, Colin Campbell
Details
Proposed patch (5.08 KB, patch)
2011-09-14 12:10 UTC, Colin Campbell
Details | Diff | Splinter Review
Proposed Patch (4.21 KB, patch)
2011-09-14 12:11 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 3638 Self Check Should Capture Hold Items (5.09 KB, patch)
2012-06-02 07:51 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 3638 : Captured Holds may need to generate a transfer (4.22 KB, patch)
2012-06-02 07:51 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:13:09 UTC


---- Reported by colin.campbell@ptfs-europe.com 2009-09-16 08:22:11 ----

When an Item is returned via SIP2 the unit is being correctly alerted that it is required for a hold and the details of the hold. However unlike a normal check in the status of the hold does not reflect that the item has been captured.
This causes problems in the work flow, forcing sip2 checked in items to be manually checked in before the hold is shown as waiting or in transit to pickup location.



---- Additional Comments From colin.campbell@ptfs-europe.com 2009-09-16 08:28:00 ----

Patch sent. 





---- Additional Comments From colin.campbell@ptfs-europe.com 2010-02-16 17:15:28 ----

Created an attachment
Proposed Patch

Patch added to bug report



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:13 UTC  ---

This bug was previously known as _bug_ 3638 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3638
Imported an attachment (id=1471)

Actual time not defined. Setting to 0.0
The original submitter of attachment 1471 [details] [review] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.

Comment 1 Katrin Fischer 2010-12-17 10:10:42 UTC
What is the current status of this bug? 

There was some discussion about it on koha-patches, see below:

-----Original Message-----
From: koha-patches-bounces@lists.koha.org [mailto:koha-patches-
bounces@lists.koha.org] On Behalf Of Colin Campbell
Sent: Wednesday, September 16, 2009 1:48 PM
To: Galen Charlton
Cc: patches@koha.org
Subject: Re: [Koha-patches] [PATCH] bug 3638 On CheckIn via SIP update
the Hold status to waiting if Reserved

On 09/16/2009 12:19 PM, Galen Charlton wrote:
> I have a question about the workflow.  Are most or all SIP2 
> self-checkout devices capable of making sure that there's enough 
> notice given to the circulation desk so that the item actually gets 
> picked up and placed on the hold shelf or transferred?
>
Same concern occurred to me. You can assume if its captured at a circ desk it probably goes to the holdshelf, but for a selfcheck you lack that certainty. That said, most devices now have some kind of capturing and the recently added support for the extensions is basically info on the hold for that. Some libraries are using SIP2 to implement RFID so they are forced to check-in twice.
   I'm not sure of the best implementation for all workflows. It might be a case of needing a per account or institution flag as people could have both situations. One where the device checks-in and routes holds and transits as if a normal checkin and one where it checks in the book but needs a second manual checkin before placing the book on the hold-shelf (much as if the pickup location was another branch).
   Comments and suggestions welcome.

C.
Comment 2 Katrin Fischer 2011-04-06 09:45:00 UTC
I am not sure how this can be tested. 

For us it's a big problem that checked-in books at the selfcheck don't change their status to waiting. It means another patron can check it out, because the hold has not been triggered on check-in. 

Would this problem be fixed by the patch?
Comment 3 Colin Campbell 2011-07-07 09:09:50 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2011-08-14 18:23:40 UTC
I am very interested in testing this and I see that some other SIP patches are pending in 'Needs Signoff'. 
Is it possible to setup a test environment for SIP2 that works with a local Koha installation?
Are there tools that can be used for that? Any hints appreciated.
Comment 5 Dobrica Pavlinusic 2011-08-15 13:16:38 UTC
A while ago, I wrote very simple SIP2 implementation which enables me to send messages directly to Koha SIP2 server.

https://github.com/dpavlin/Biblio-SIP2

We are using it to push same messages which we collected from network dumps with 3M SIP2 self-check implementation. They are just a few messages from complete SIP2 implementation, so this approach worked for us.

sc-emulator.pl contain a lot of hard-coded values specific to our Koha installation, but can be easily modified, I hope :-)

acs-proxy.pl is simple protocol wrapper which can be inserted in-between SC and ACS server to provide protocol dumps.

We also experimented with 3M\ SIP2\ Development\ Kit/Program/SCEmul.exe running under Wine on Linux, but it's a complex emulator, and we don't see most of messages on wire anyway.

We have some notes in Croatian at http://saturn.ffzg.hr/koha/index.cgi?sip2 but hopefully examples can be useful (and Google translate does a fair job of replacing few Croatian words with something understandable).
Comment 6 Colin Campbell 2011-08-15 13:36:50 UTC
I've got a test directory with some SIP config files and startup and shutdown scripts so I can run up the SIP software using these config files in development branches.
I've got a number of test scripts that open a socket to these sip ports then do basic sip operations charge/discharge etc. The sip messages are built up from the docs or if I'm looking at a specific problem its easy enough to copy the contents of messages from the logs. (really vim id my test tool)
I'd second Dobrica's comments about not using the development kit emulator. I've used it in the past but found it didn't support lots of the fields that are important to libraries just doing very basic transactions.
Comment 7 Katrin Fischer 2011-08-15 13:51:16 UTC
Hi Dobrica and Colin, thx for your comments! 
Colin - could you share one of your test scripts?
Comment 8 Colin Campbell 2011-08-15 15:37:06 UTC
Created attachment 5019 [details]
test script example

And example of a test script. This one issues two items then discharges them. Other requests like the various patron info request could be substituted.
Comment 9 Colin Campbell 2011-09-14 12:10:06 UTC Comment hidden (obsolete)
Comment 10 Colin Campbell 2011-09-14 12:11:08 UTC Comment hidden (obsolete)
Comment 11 Colin Campbell 2011-09-14 12:17:00 UTC
Testing in production showed that the initial patch while setting the status correctly did not address the whole of the problem. If the item is captured at branch X for a hold set to pickup at branch Y in needs to generate a branchtransfer to that branch otherwise the status is easily lost by staff and general dissatisfaction ensues have added a second patch which builds on the first, initiating the transfer
Comment 12 Ian Walls 2011-10-19 19:26:32 UTC
Colin,


Are the two patches currently shown competing implementations, or complements to each other?
Comment 13 Colin Campbell 2011-11-21 11:15:33 UTC
Patches complement each other see previous comment
Comment 14 Chris Cormack 2012-06-02 07:51:19 UTC
Created attachment 9901 [details] [review]
Bug 3638 Self Check Should Capture Hold Items

Shelf Check was receiving messages saying item was wanted
for a hold but the item was discharged to the shelf
not associated with the hold or transited to
the pickup location. The message was also being sent
on discharge of items when a suitable item had already
been captured.
Checkin now associates the item with the hold and sets
the appropriate data for a correct checkin response

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 15 Chris Cormack 2012-06-02 07:51:46 UTC
Created attachment 9902 [details] [review]
Bug 3638 : Captured Holds may need to generate a transfer

If a discharged item is captured for a hold at another branch
the selfcheck should generate a branchtransfer for it.
This was complicated as the checkin routine was setting its own
location from a field not set in many scm machines (e.g. 3M)
also in ILS::Transaction the test for location was incorrect
assuming that an empty string or undefined was a valid location

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 16 Chris Cormack 2012-06-02 07:52:25 UTC
Thanks for the test script example Colin, was very handy for testing.
Comment 17 Ian Walls 2012-06-04 20:14:23 UTC
First patch:  modifies some subroutines in SIP/ILS/Item.pm to take borrowernumber/branchcode params, and set them for the hold.  Moves hold handling to do_checkin instead of handle_checkin, and adds the processing from C4::Reserves we'd normally get in the staff client.  Also strips out some commented lines.

Marking as Passed QA.

Second patch:  Better handling of conditional (separate lines and shift instead of a Perl one-liner).  Adds return_date to do_checkin call.  Adds a call to ModItemTransfer where applicable.  Bulk of work is to set a default current location, as the self check machine may not.

Also marked Passed QA.
Comment 18 Paul Poulain 2012-06-05 14:14:22 UTC
QA comment: I couldn't test the patch with a true selfcheck machine, but patch pushed.

Colin, before the patch there was some perlcritic problems, they are still here after the patch.

If you can submit a follow-up to fix them, that would be nice. Should be very easy (removal or "return undef" in favour of "return" and removing prototyping of subs)

16:11 ~/koha.dev/koha-community (new/bug_3638 $%)$ perlcritic C4/SIP/ILS/Item.pm C4/SIP/ILS/Transaction/Checkin.pm C4/SIP/Sip/MsgType.pm
C4/SIP/ILS/Item.pm: "return" statement with explicit "undef" at line 89, column 3.  See page 199 of PBP.  (Severity: 5)
C4/SIP/ILS/Item.pm: Subroutine prototypes used at line 350, column 1.  See page 194 of PBP.  (Severity: 5)
C4/SIP/ILS/Item.pm: "return" statement with explicit "undef" at line 352, column 26.  See page 199 of PBP.  (Severity: 5)
C4/SIP/ILS/Item.pm: "return" statement with explicit "undef" at line 353, column 51.  See page 199 of PBP.  (Severity: 5)
C4/SIP/ILS/Item.pm: Subroutine prototypes used at line 356, column 1.  See page 194 of PBP.  (Severity: 5)
C4/SIP/ILS/Item.pm: "return" statement with explicit "undef" at line 359, column 28.  See page 199 of PBP.  (Severity: 5)
C4/SIP/ILS/Item.pm: "return" statement with explicit "undef" at line 360, column 28.  See page 199 of PBP.  (Severity: 5)
C4/SIP/ILS/Item.pm: "return" statement with explicit "undef" at line 361, column 61.  See page 199 of PBP.  (Severity: 5)
C4/SIP/ILS/Item.pm: Subroutine prototypes used at line 364, column 1.  See page 194 of PBP.  (Severity: 5)
C4/SIP/ILS/Item.pm: "return" statement with explicit "undef" at line 366, column 25.  See page 199 of PBP.  (Severity: 5)
C4/SIP/ILS/Item.pm: "return" statement with explicit "undef" at line 368, column 24.  See page 199 of PBP.  (Severity: 5)
C4/SIP/ILS/Transaction/Checkin.pm source OK
C4/SIP/Sip/MsgType.pm: "return" statement with explicit "undef" at line 408, column 3.  See page 199 of PBP.  (Severity: 5)
C4/SIP/Sip/MsgType.pm: Subroutine prototypes used at line 797, column 1.  See page 194 of PBP.  (Severity: 5)
C4/SIP/Sip/MsgType.pm: "return" statement with explicit "undef" at line 798, column 24.  See page 199 of PBP.  (Severity: 5)
Comment 19 Chris Cormack 2012-06-06 05:00:09 UTC
Pushed to 3.8.x, will be in 3.8.2