Bug 27014

Summary:	SIP2 cannot find patrons at checkin
Product:	Koha	Reporter:	Nick Clemens (kidclamp) <nick>
Component:	SIP2	Assignee:	Kyle M Hall (khall) <kyle>
Status:	CLOSED FIXED	QA Contact:	Testopia <testopia>
Severity:	normal
Priority:	P5 - low	CC:	andreas.jonsson, andrew, colin.campbell, fridolin.somers, jonathan.druart, kyle, martin.renvoize, victor
Version:	Main
Hardware:	All
OS:	All
Change sponsored?:	---	Patch complexity:	---
Documentation contact:		Documentation submission:
Text to go in the release notes:		Version(s) released in:	21.05.00,20.11.04,20.05.10
Circulation function:
Bug Depends on:	23403
Bug Blocks:
Attachments:	Bug 27014: Enable C4::SIP::ILS::Patron::new to accept a hash Bug 27014: Enable C4::SIP::ILS::Patron::new to accept a hash Bug 27014: (QA follow-up) Add regression tests

Description Nick Clemens (kidclamp) 2020-11-13 12:12:28 UTC

Caused by bug 23403 - when performing a checkin we lookup the SIP patron using the borrowernumber, however, SIP only knows how to find a patron via cardnumber or userid

The change on 23403 was to avoid using an 'id' that didn't always exist (as some users don't have a userid or cardnumber

When checking in, however, we are not passed a user cardnumber or borrowernumber, so we don't have those on hand to get the patron.

from C4/SIP/ILS.pm
245     } elsif ( $circ->ok ) {
246         $circ->patron( $patron = C4::SIP::ILS::Patron->new( $item->{borrowernumber} ) );
247         delete $item->{borrowernumber};
248         delete $item->{due_date};
249         $patron->{items} = [ grep { $_ ne $item_id } @{ $patron->{items} } ];
250     } else {

I actually don't know if the $patron->{items} call is useful, it seems to be updating the patron item cache, but I don't see where we cache it, or read it, we just seem to create new patrons/transactions when needed

Comment 1 Kyle M Hall (khall) 2020-11-13 13:26:52 UTC

Created attachment 113602 [details] [review]
Bug 27014: Enable C4::SIP::ILS::Patron::new to accept a hash

Caused by bug 23403 - when performing a checkin we lookup the SIP patron
using the borrowernumber, however, SIP only knows how to find a patron
via cardnumber or userid

The change on 23403 was to avoid using an 'id' that didn't always exist
(as some users don't have a userid or cardnumber

When checking in, however, we are not passed a user cardnumber or
borrowernumber, so we don't have those on hand to get the patron.

Test Plan:
1) Check in an item via SIP, note patron is not found
2) Apply this patch
3) Restart all the things!
4) Check in an item via SIP, patron should be found!

Comment 2 Martin Renvoize (ashimema) 2021-01-12 07:06:26 UTC

This looks reasonable enough, I'll give it a t at layer today.

I'm getting the feeling more and more that we should be working together to convert sip to use core Koha objects.. or even using a migration to Koha::SIP or Koha::Patrons::SIP perhaps as an excuse to tidy some of this code up and add unit tests.

Either way, would it be possible to add a regression test for this... It's all too easy to break SIP code at the moment :(

Comment 3 Martin Renvoize (ashimema) 2021-01-12 11:05:15 UTC

Created attachment 115071 [details] [review]
Bug 27014: Enable C4::SIP::ILS::Patron::new to accept a hash

Caused by bug 23403 - when performing a checkin we lookup the SIP patron
using the borrowernumber, however, SIP only knows how to find a patron
via cardnumber or userid

The change on 23403 was to avoid using an 'id' that didn't always exist
(as some users don't have a userid or cardnumber

When checking in, however, we are not passed a user cardnumber or
borrowernumber, so we don't have those on hand to get the patron.

Test Plan:
1) Check in an item via SIP, note patron is not found
2) Apply this patch
3) Restart all the things!
4) Check in an item via SIP, patron should be found!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Comment 4 Martin Renvoize (ashimema) 2021-01-12 11:21:05 UTC

Created attachment 115072 [details] [review]
Bug 27014: (QA follow-up) Add regression tests

This patch adds a regression test for the change introduced in the
patchset.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Comment 5 Martin Renvoize (ashimema) 2021-01-12 11:21:32 UTC

Works as expected.. I added some regressions tests for you.

Signing off

Comment 6 Kyle M Hall (khall) 2021-01-12 11:32:09 UTC

(In reply to Martin Renvoize from comment #5)
> Works as expected.. I added some regressions tests for you.
> 
> Signing off

Thanks Martin!

Comment 7 Martin Renvoize (ashimema) 2021-02-19 13:43:06 UTC

I'm tempted to mark this one as PQA.. it's a pretty trivial change in reality and I'm confident in my testing... and we're lacking in people with the expertise to QA SIP...

Lets go for it.. Jonathan, you can always knock it back if you prefer.. just means trying to coerce someone to get to grips with SIP.. which we should do anyway longer term.

Comment 8 Jonathan Druart 2021-02-23 12:21:37 UTC

Pushed to master for 21.05, thanks to everybody involved!

Comment 9 Fridolin Somers 2021-02-25 09:41:53 UTC

Pushed to 20.11.x for 20.11.04

Comment 10 Andrew Fuerste-Henry 2021-03-05 16:19:10 UTC

Pushed to 20.05.x for 20.05.10

Comment 11 Victor Grousset/tuxayo 2021-03-19 00:05:11 UTC

Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.