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
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!
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 :(
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>
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>
Works as expected.. I added some regressions tests for you. Signing off
(In reply to Martin Renvoize from comment #5) > Works as expected.. I added some regressions tests for you. > > Signing off Thanks Martin!
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.
Pushed to master for 21.05, thanks to everybody involved!
Pushed to 20.11.x for 20.11.04
Pushed to 20.05.x for 20.05.10
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.