Bug 27014 - SIP2 cannot find patrons at checkin
Summary: SIP2 cannot find patrons at checkin
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 23403
Blocks:
  Show dependency treegraph
 
Reported: 2020-11-13 12:12 UTC by Nick Clemens
Modified: 2021-03-19 00:05 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.04,20.05.10


Attachments
Bug 27014: Enable C4::SIP::ILS::Patron::new to accept a hash (2.74 KB, patch)
2020-11-13 13:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27014: Enable C4::SIP::ILS::Patron::new to accept a hash (2.79 KB, patch)
2021-01-12 11:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27014: (QA follow-up) Add regression tests (2.21 KB, patch)
2021-01-12 11:21 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 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 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 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 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 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 2021-01-12 11:21:32 UTC
Works as expected.. I added some regressions tests for you.

Signing off
Comment 6 Kyle M Hall 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 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.