Bug 17758 - SIP checkin does not handle holds correctly
Summary: SIP checkin does not handle holds correctly
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 14695
Blocks:
  Show dependency treegraph
 
Reported: 2016-12-12 08:39 UTC by Petter Goksøyr Åsen
Modified: 2017-05-03 03:53 UTC (History)
6 users (show)

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


Attachments
Bug 17758 - SIP checkin does not handle holds correctly (1.85 KB, patch)
2016-12-13 12:40 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17758 - SIP checkin does not handle holds correctly (1.91 KB, patch)
2017-03-21 01:22 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 17758 - SIP checkin does not handle holds correctly (1.95 KB, patch)
2017-03-29 15:07 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 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 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