Bug 17758

Summary:	SIP checkin does not handle holds correctly
Product:	Koha	Reporter:	Petter Goksøyr Åsen <boutrosboutrosboutros>
Component:	SIP2	Assignee:	Bugs List <koha-bugs>
Status:	CLOSED FIXED	QA Contact:	Testopia <testopia>
Severity:	major
Priority:	P5 - low	CC:	benjamin.rokseth, colin.campbell, jonathan.druart, kyle, mtj
Version:	Main
Hardware:	All
OS:	All
See Also:	https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17766
Change sponsored?:	---	Patch complexity:	---
Documentation contact:		Documentation submission:
Text to go in the release notes:		Version(s) released in:
Circulation function:
Bug Depends on:	14695
Bug Blocks:
Attachments:	Bug 17758 - SIP checkin does not handle holds correctly [SIGNED-OFF] Bug 17758 - SIP checkin does not handle holds correctly Bug 17758 - SIP checkin does not handle holds correctly

Description Petter Goksøyr Åsen 2016-12-12 08:39:39 UTC

We have been investingating some strange behaviour from our SIP transactions:

Given a biblio with to items x1 & x2, and two holds on the biblo.
- Checkin (via SIP) x1 on the pickup branch - hold is filled and notified in SIP message.
- Checkin (via SIP) x2 on another branch - get notified of hold again, but this time the hold is swapped so that now item x2 fullfils the hold, and x1 is again available.

If do the same transactions in the web-interface - no error occurs.

I belive bug #14695 is the origin of this behaviour. If you look at commit https://github.com/Koha-Community/Koha/commit/6e76e342cb6ac6036738d79418ff068ad84346cc which changes the signature of ModReserveAffect (without updating the documentation btw) but fails to update the corresponding usage of it in the SIP::ILS::Transaction::Checkin.

Comment 1 Benjamin Rokseth 2016-12-13 11:55:41 UTC

on further investigation, SIP checkin only lacks the reserve_id param to ModReserveAffect, which is rather weird, since it is the unique key on a reserve.

Also ModReserveAffect and its called method _koha_notify_reserve should be rewritten to use Koha::Holds, and use the reserve_id but I will do this in a separate bug.

Comment 2 Benjamin Rokseth 2016-12-13 12:40:12 UTC

Created attachment 58154 [details] [review]
Bug 17758 - SIP checkin does not handle holds correctly

This patch updates SIP checkin to use reserve_id as param, to fix regression
introduced by Bug 14695.

To test:

Given 1 patron and 1 biblio with two items attached:

1) create two record level holds on one biblio on same user
Using SIP client or telnet:
2) checkin item x on pickup branch, observe item is waiting and hold 1 is filled
3) checkin item y on pickup branch, observe item y now is waiting and x is available
4) apply this patch and repeat 1-3, now expected behaviour should be
   that both holds are filled and both items waiting

Comment 3 Srdjan Jankovic 2017-03-21 01:22:38 UTC

Created attachment 61347 [details] [review]
[SIGNED-OFF] Bug 17758 - SIP checkin does not handle holds correctly

This patch updates SIP checkin to use reserve_id as param, to fix regression
introduced by Bug 14695.

To test:

Given 1 patron and 1 biblio with two items attached:

1) create two record level holds on one biblio on same user
Using SIP client or telnet:
2) checkin item x on pickup branch, observe item is waiting and hold 1 is filled
3) checkin item y on pickup branch, observe item y now is waiting and x is available
4) apply this patch and repeat 1-3, now expected behaviour should be
   that both holds are filled and both items waiting

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>

Comment 4 Nick Clemens (kidclamp) 2017-03-29 15:07:07 UTC

Created attachment 61686 [details] [review]
Bug 17758 - SIP checkin does not handle holds correctly

This patch updates SIP checkin to use reserve_id as param, to fix regression
introduced by Bug 14695.

To test:

Given 1 patron and 1 biblio with two items attached:

1) create two record level holds on one biblio on same user
Using SIP client or telnet:
2) checkin item x on pickup branch, observe item is waiting and hold 1 is filled
3) checkin item y on pickup branch, observe item y now is waiting and x is available
4) apply this patch and repeat 1-3, now expected behaviour should be
   that both holds are filled and both items waiting

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Comment 5 Kyle M Hall (khall) 2017-03-31 13:14:42 UTC

Pushed to master for 17.05, thanks Ben!

Comment 6 Katrin Fischer 2017-04-02 16:53:26 UTC

Hm, must have missed this before? 

This patch has been pushed to 16.11.x and will be in 16.11.07.

Comment 7 Mason James 2017-05-03 03:53:40 UTC

Pushed to 16.05.x, for 16.05.12 release