Bug 23403 - SIP2 lends to wrong patron if cardnumber is missing
Summary: SIP2 lends to wrong patron if cardnumber is missing
Status: NEW
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:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-31 12:59 UTC by Magnus Enger
Modified: 2019-07-31 13:28 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2019-07-31 12:59:11 UTC
To reproduce: 

Make sure you have two borrowers with no cardnumber, but with a userid. Determine which of the two patrons has the *highest* borrowernumber and lend a book to this patron via SIP2. 

If you do not have a SIP2 self check machine on hand you can do it thusly on the command line: 

$ telnet localhost <SIP2 port number>

> 9300CN<SIP2 username>|CO<SIP2 password>|CP<library code>|

> 11NN20190731    14163520190731    141635AO<library code>|AA<userid>|AB<barcode>|ACmy password|AD|

Now check who the book is actually on loan to. In my testing it has been the patron without a cardnumber that has the lowest borrowernumber!
Comment 1 Magnus Enger 2019-07-31 13:21:46 UTC
C4::SIP::ILS::Patron::new() looks for a patron based on either cardnumber or userid, around line 41: 

     my $patron = Koha::Patrons->find( { cardnumber => $patron_id } )
       || Koha::Patrons->find( { userid => $patron_id } );

but then we set the id of the patron to the value of the barcode field, around line 77: 

    id   => $kp->{cardnumber},    # to SIP, the id is the BARCODE, not userid

If there is no barcode, the id will be empty. Then in C4::SIP::ILS::Transaction::Checkout::do_checkout() we use the id to find the patron, around line 53:

    my $patron_barcode = $self->{patron}->id;
    ...
    my $patron = Koha::Patrons->find( { cardnumber => $patron_barcode } );

And I guess ->find( { cardnumber => '' } ) returns the first patron that has an empty cardnumber.
Comment 2 Magnus Enger 2019-07-31 13:28:51 UTC
Possible solution: 

In C4::SIP::ILS::Transaction::Checkout::do_checkout(), find the patron based on either cardnumber or userid (similar to how C4::SIP::ILS::Patron::new() does it).

Or is doing Koha::Patrons->find twice for each checkout just plain wrong and we should be passing the patron object around instead?