Bug 13666 - Allow SIP2 checkin/checkout to get branch from institution_id field AO
Summary: Allow SIP2 checkin/checkout to get branch from institution_id field AO
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Benjamin Rokseth
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-02-04 14:44 UTC by Benjamin Rokseth
Modified: 2017-10-11 14:55 UTC (History)
6 users (show)

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


Attachments
Bug 13666 - Allow SIP2 checkin/checkout to get branch from institution_id field AO (2.45 KB, patch)
2015-02-05 14:59 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 13666 - Allow SIP2 checkin/checkout to get branch from institution_id field AO (2.49 KB, patch)
2015-07-02 23:49 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 13666 - Allow SIP2 checkin/checkout to get branch from institution_id field AO (2.43 KB, patch)
2015-07-03 01:27 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 13666 - Allow SIP2 checkin/checkout to get branch from institution_id field AO (1.98 KB, patch)
2015-07-03 01:32 UTC, Benjamin Rokseth
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Rokseth 2015-02-04 14:44:15 UTC
Although SIP2 specifies the required AO field to be institution_id and it doesn't neccessarily map to a branch in Koha, this is probably the case for most.

Allowing for AO to set the C4::Context->userenv->{branch} transaction-time would open up for a more flexible transaction scenario:

- SIP2 connections could fetch from a pool of connections, so e.g. ten SIP2 workers could easily serve hundreds of clients
- SIPConfig.xml would not need reconfiguration each time a new client is added

Please advise if this is in collision with other SIP2 logic in Koha
Comment 1 Benjamin Rokseth 2015-02-05 14:59:03 UTC Comment hidden (obsolete)
Comment 2 Olli-Antti Kivilahti 2015-03-03 16:28:08 UTC
Hi! I would need more information about the use-cases.

> - SIPConfig.xml would not need reconfiguration each time a new client is added

I don't understand how this patch has to do with SIPConfig? There are no such followups/dependencies for that either.

> - SIP2 connections could fetch from a pool of connections, so e.g. ten SIP2 workers could easily serve hundreds of clients

SIP2 connections are already reused by the Net::Server?
Comment 3 Benjamin Rokseth 2015-03-05 12:42:32 UTC
Hi Olli!

To recap:  the SIP-server infers the transaction branch from the C4::Context->userenv->{'branch'}, but for some reason only on checkouts. This has to do with how SIP2 uses AddIssue and ModReserve to handle reserves and circulation.

AddIssue uses patron context, while AddReturn takes branch as param. This is probably good enough for intranet use, but not for a service.

Use-case that illustrate this enhancement:

- library has need for a centralised handling of SIP connections, for handling e.g. 10 or, say, 50 concurrent connections. SIP clients range from SelfCheck automats to RFID clients and automated circulation systems.
- this is best handled by a centralised service which maintains a pool of connections (users sippool1, sippool2, ...)
- centralised service handles connections and could easily open and close connections at need, so we don't have to open connection, login, and close for each transaction, (poor performance) or worse, spawn 100+ workers with keep-alive connections
- more dynamic solution than maintaining two sets of "SIP users", one in SIPConfig.xml and the other in Koha
- centralised service could handle retry logic, logging, message rewriting, etc, and SIP server can focus on transactions.

I believe any library with a hope to use SIP2 effectively would benefit from this patch.
Comment 4 Olli-Antti Kivilahti 2015-03-06 14:03:47 UTC
Thanks for the input Benjamin!

Shouldn't this patch be extended to set the AO-field for all SIP2 transactions where it is used?
Then propagate the value to the transaction handlers?

Now this special case is dealed with only for checkouts, which is not logical.
Comment 5 Benjamin Rokseth 2015-03-09 08:59:29 UTC
Actually, AO is respected in checkins already, as AddReturn takes $branch as param, whereas AddIssue does not.

I suppose the only reason AO was not respected in checkout initially was due to the fact that AddIssue doesn't take $branch as param, and this probably was left to be solved in AddIssue at a later stage. The web interface requires user to specify branch before checkout, so C4::Context->userenv->{'branch'} is set which is the same as this patch does.

For Renew, location has no effect, and hold takes its own param, pickup_location (BR), so that's the reason only checkout is handled in this patch, which, in my opinion, should have been handled by AO in the first place, as SIP is a transport protocol and should not have other dependencies than authentication.

Also, the protocol specifies that AO is a required field in all transactions and maps to the library’s institution ID, so SIP clients should adher to this.

Anyway, if AO is left empty, the transaction will fallback to user context, so the patch will not cause anyone trouble
Comment 6 Benjamin Rokseth 2015-07-02 23:49:29 UTC Comment hidden (obsolete)
Comment 7 Benjamin Rokseth 2015-07-03 01:27:44 UTC Comment hidden (obsolete)
Comment 8 Benjamin Rokseth 2015-07-03 01:32:29 UTC
Created attachment 40769 [details] [review]
Bug 13666 - Allow SIP2 checkin/checkout to get branch from institution_id field AO

giving up on patching /etc/SIPconfig.xml ... for now