Bug 23403

Summary: SIP2 lends to wrong patron if cardnumber is missing
Product: Koha Reporter: Magnus Enger <magnus>
Component: SIP2Assignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: 1joynelson, colin.campbell, jonathan.druart, kyle, nick, 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:
20.05.00, 19.11.06, 19.05.12
Bug Depends on:    
Bug Blocks: 27014    
Attachments: Bug 23403: Remove cardnumber from SIP
Bug 23403: Remove cardnumber from SIP
Bug 23403: Remove cardnumber from SIP
Bug 23403: Remove cardnumber from SIP
Bug 23403: Fix whitespace
Bug 23403: List borrowernumber in the ILS::Patron fields
Bug 23403: Catch other cases of ->{patron}
Bug 23403: Remove cardnumber from SIP
Bug 23403: Fix whitespace
Bug 23403: List borrowernumber in the ILS::Patron fields
Bug 23403: Catch other cases of ->{patron}
Bug 23403: Remove cardnumber from SIP
Bug 23403: Fix whitespace
Bug 23403: List borrowernumber in the ILS::Patron fields
Bug 23403: Catch other cases of ->{patron}

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?
Comment 3 Jonathan Druart 2019-11-21 17:02:50 UTC
Confirmed.
Comment 4 Jonathan Druart 2019-11-21 17:40:09 UTC
Created attachment 95703 [details] [review]
Bug 23403: Remove cardnumber from SIP
Comment 5 Jonathan Druart 2019-11-21 17:41:33 UTC
I've tried but failed. There are many things broken apparently.

I ended up with that patch, it does not work but I have no idea why, I do not get any useful warnings or errors. Someone with more knowledge in this code may be able to finish it.
Comment 6 Jonathan Druart 2019-12-10 14:20:56 UTC
Kyle, Colin,
Can you have a look at that patch please?
I am stuck at debugging it and founding what's wrong with it.
Thanks!
Comment 7 Nick Clemens 2019-12-17 18:44:57 UTC
It looks like this patch tries to switch to using the Koha patron object for items, but during the checkout we are getting a SIP/ILS patron object, we can't call borrowernumber on that (yet)

I think in general we should stick to the SIP object to avoid confusion and just extend that object to have all the tools it needs (add borrowernumber to the object)- perhaps if the id is unpopulated we can fall back, or we can just always use the borrowernumber internally as a more standard id
Comment 8 Jonathan Druart 2019-12-18 09:17:44 UTC
Created attachment 96404 [details] [review]
Bug 23403: Remove cardnumber from SIP
Comment 9 Jonathan Druart 2019-12-18 09:20:06 UTC
Thanks Nick, that helped a lot!

I think this patch is ready for testing, however I am not sure how it could be tested to cover all the changes. Someone with a good knowledge in this script could review it, then test could be potentially broken.
I think the following SIP commands would be a good test plan: checkin, checkout, renew, place a hold on an item.
Comment 10 Bouzid Fergani 2020-03-20 17:44:42 UTC Comment hidden (obsolete)
Comment 11 Bouzid Fergani 2020-03-20 17:45:34 UTC
Created attachment 101304 [details] [review]
Bug 23403: Remove cardnumber from SIP

Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 12 Kyle M Hall 2020-03-23 12:58:48 UTC
Created attachment 101445 [details] [review]
Bug 23403: Fix whitespace
Comment 13 Kyle M Hall 2020-03-23 12:59:32 UTC
prove t/db_dependent/SIP/*
t/db_dependent/SIP/ILS.t .......... 2/10     # Looks like you planned 5 tests but ran 2.
t/db_dependent/SIP/ILS.t .......... 10/10
#   Failed test 'cancel_hold'
#   at t/db_dependent/SIP/ILS.t line 124.
Cannot access 'borrowernumber' field of class 'C4::SIP::ILS::Patron' at /kohadevbox/koha/C4/SIP/ILS/Transaction/Hold.pm line 81.
# Looks like your test exited with 255 just after 10.
t/db_dependent/SIP/ILS.t .......... Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/10 subtests
t/db_dependent/SIP/Message.t ...... 2/4
    #   Failed test 'Check screen msg'
    #   at t/db_dependent/SIP/Message.t line 507.
    #          got: 'Item not checked out'
    #     expected: 'Checkin failed'
    # Looks like you failed 1 test of 29.
t/db_dependent/SIP/Message.t ...... 3/4
#   Failed test 'Checkin V2'
#   at t/db_dependent/SIP/Message.t line 74.
# Looks like you failed 1 test of 4.
t/db_dependent/SIP/Message.t ...... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/4 subtests
t/db_dependent/SIP/Patron.t ....... ok
t/db_dependent/SIP/SIPServer.t .... ok
t/db_dependent/SIP/Transaction.t .. 4/7 RECORD:69::100 at /kohadevbox/koha/C4/SIP/ILS/Transaction/FeePayment.pm line 55.
t/db_dependent/SIP/Transaction.t .. 5/7     # Looks like you planned 7 tests but ran 5.

#   Failed test 'cancel_hold'
#   at t/db_dependent/SIP/Transaction.t line 210.
Cannot access 'borrowernumber' field of class 'C4::SIP::ILS::Patron' at /kohadevbox/koha/C4/SIP/ILS/Transaction/Hold.pm line 81.
# Looks like your test exited with 255 just after 6.
t/db_dependent/SIP/Transaction.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 2/7 subtests

Test Summary Report
-------------------
t/db_dependent/SIP/ILS.t        (Wstat: 65280 Tests: 10 Failed: 1)
  Failed test:  10
  Non-zero exit status: 255
t/db_dependent/SIP/Message.t    (Wstat: 256 Tests: 4 Failed: 1)
  Failed test:  3
  Non-zero exit status: 1
t/db_dependent/SIP/Transaction.t (Wstat: 65280 Tests: 6 Failed: 1)
  Failed test:  6
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 7 tests but ran 6.
Files=5, Tests=26, 21 wallclock secs ( 0.04 usr  0.02 sys + 11.47 cusr  2.11 csys = 13.64 CPU)
Result: FAIL
Comment 14 Jonathan Druart 2020-03-23 15:03:11 UTC
Created attachment 101456 [details] [review]
Bug 23403: List borrowernumber in the ILS::Patron fields
Comment 15 Jonathan Druart 2020-03-23 15:03:51 UTC
There is still one test missing, I do not manage to fix it.

Could I get some help?

t/db_dependent/SIP/Message.t .. 1/4
    #   Failed test 'Check screen msg'
    #   at t/db_dependent/SIP/Message.t line 507.
    #          got: 'Item not checked out'
    #     expected: 'Checkin failed'
    # Looks like you failed 1 test of 29.
t/db_dependent/SIP/Message.t .. 3/4
Comment 16 Nick Clemens 2020-04-13 15:52:11 UTC
Created attachment 102859 [details] [review]
Bug 23403: Catch other cases of ->{patron}
Comment 17 Victor Grousset/tuxayo 2020-04-26 22:17:08 UTC
IIUC the test plan is
- comment 0
- prove t/db_dependent/SIP/*
Comment 18 Nick Clemens 2020-04-27 10:11:14 UTC
(In reply to Victor Grousset/tuxayo from comment #17)
> IIUC the test plan is
> - comment 0
> - prove t/db_dependent/SIP/*

That covers most of it, I think full testing would be:
1 - Have two patrons with userids and no cardnumber
2 - Note which of these has the higher borrower number
3 - Use the SIP cli emulator to connect and checkout a book to the patron with higher borrowernumber
4 - Note the book may checkout to the wrong patron!
5 - Apply patch
6 - Checkout to both patrons via sip
7 - The patrons get the correct checkouts
Comment 19 Victor Grousset/tuxayo 2020-05-04 15:57:20 UTC
> 3 - Use the SIP cli emulator to connect and checkout a book to the patron with higher borrowernumber

Should something like that do? I'm not sure I'm using well the emulator. And I barely know SIP to manually check whether the command is correct.

./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su koha -sp koha -l CPL --patron 23529000035676 -m checkout --item 39999000001297
Attempting socket connection to 127.0.0.1:6001...connected!
SEND: 9300CNkoha|COkoha|CPCPL|
READ: 941

Trying 'checkout'
SEND: 11YN20200504    15462920200504    154629AOCPL|AA23529000035676|AB39999000001297|ACkoha|BON|BIN|
Use of uninitialized value $data in concatenation (.) or string at ./misc/sip_cli_emulator.pl line 344, <GEN0> chunk 1.
READ: 

https://gitlab.com/koha-community/Koha/-/blob/a6985d167e11eaafee985b51cb0188a693405af5/misc/sip_cli_emulator.pl#L344

Now, trying with telnet by copying the message to read the server response.

kohadev-koha@b5a2119a1209:/kohadevbox/koha$ telnet localhost 6001
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
9300CNkoha|COkoha|CPCPL|
941
11YN20200504    15462920200504    154629AOCPL|AA23529000035676|AB39999000001297|ACkoha|BON|BIN|
Connection closed by foreign host.

That's rude! *pouts*


Doing the same checkout via the UI work.
- same koha librarian user (superlibrarian)
- same barcode
- same patron
Comment 20 Victor Grousset/tuxayo 2020-05-04 15:59:37 UTC
Wait, I had to user the user dedicated to sip.
(term1 in koha-testing-docker)


kohadev-koha@b5a2119a1209:/kohadevbox/koha$ ./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 -m checkout --item 39999000001297
Attempting socket connection to 127.0.0.1:6001...connected!
SEND: 9300CNterm1|COterm1|CPCPL|
READ: 941

Trying 'checkout'
SEND: 11YN20200504    15582620200504    155826AOCPL|AA23529000035676|AB39999000001297|ACterm1|BON|BIN|
READ: 121NNY20200504    155827AOCPL|AA23529000035676|AB39999000001297|AJDream of fair to middling women /|AH20200509    235900|



The testing continues!
Comment 21 Victor Grousset/tuxayo 2020-05-04 16:07:35 UTC
Created attachment 104313 [details] [review]
Bug 23403: Remove cardnumber from SIP

== Test plan ==
1 - Have two patrons with userids and no cardnumber
2 - Note which of these has the higher borrower number
3 - Use the SIP cli emulator to connect and checkout a book to the patron with higher borrowernumber
      See example after
4 - Note the book may checkout to the wrong patron!
5 - Apply patch
6 - Checkout to both patrons via sip
7 - The patrons get the correct checkouts

=== SIP CLI emulator ===
./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 \
-l CPL --patron 23529001000463 -m checkout --item 39999000001259

translation: via the koha user term1, checkout item 39999000001259 to
patron 23529001000463

Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 22 Victor Grousset/tuxayo 2020-05-04 16:07:41 UTC
Created attachment 104314 [details] [review]
Bug 23403: Fix whitespace

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 23 Victor Grousset/tuxayo 2020-05-04 16:07:46 UTC
Created attachment 104315 [details] [review]
Bug 23403: List borrowernumber in the ILS::Patron fields

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 24 Victor Grousset/tuxayo 2020-05-04 16:07:51 UTC
Created attachment 104316 [details] [review]
Bug 23403: Catch other cases of ->{patron}

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 25 Victor Grousset/tuxayo 2020-05-04 16:11:54 UTC
It didn't work and thanks to the patch it works! :D


To next person testing and re-uploading the patches:
I forgot to add in the test plan to run the following:
prove t/db_dependent/SIP/*

(not adding to not spam with email notification again)
Comment 26 Nick Clemens 2020-05-12 11:52:28 UTC
Created attachment 104760 [details] [review]
Bug 23403: Remove cardnumber from SIP

== Test plan ==
1 - Have two patrons with userids and no cardnumber
2 - Note which of these has the higher borrower number
3 - Use the SIP cli emulator to connect and checkout a book to the patron with higher borrowernumber
      See example after
4 - Note the book may checkout to the wrong patron!
5 - Apply patch
6 - Checkout to both patrons via sip
7 - The patrons get the correct checkouts

=== SIP CLI emulator ===
./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 \
-l CPL --patron 23529001000463 -m checkout --item 39999000001259

translation: via the koha user term1, checkout item 39999000001259 to
patron 23529001000463

Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 27 Nick Clemens 2020-05-12 11:52:31 UTC
Created attachment 104761 [details] [review]
Bug 23403: Fix whitespace

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 28 Nick Clemens 2020-05-12 11:52:34 UTC
Created attachment 104762 [details] [review]
Bug 23403: List borrowernumber in the ILS::Patron fields

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 29 Nick Clemens 2020-05-12 11:52:38 UTC
Created attachment 104763 [details] [review]
Bug 23403: Catch other cases of ->{patron}

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 30 Jonathan Druart 2020-05-12 12:34:52 UTC
Nick, how confident are you with this change? Do you think it should be part of 20.05?
Comment 31 Martin Renvoize 2020-05-15 08:58:18 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 32 Joy Nelson 2020-05-19 21:58:46 UTC
Does not apply to 19.11.x- please rebase if needed.
Comment 33 Nick Clemens 2020-05-20 10:13:22 UTC
(In reply to Joy Nelson from comment #32)
> Does not apply to 19.11.x- please rebase if needed.

Applies cleanly to 19.11 for me
Comment 34 Joy Nelson 2020-05-20 20:42:59 UTC
First commit when cherry-picked gives this error:
<<<<<<< HEAD
 51     syslog('LOG_DEBUG', "ILS::Transaction::Checkout performing checkout...");
 52     my $shelf          = $self->{item}->hold_shelf;
 53     my $barcode        = $self->{item}->id;
 54     my $patron_barcode = $self->{patron}->id;
 55         my $overridden_duedate; # usually passed as undef to AddIssue
 56     $debug and warn "do_checkout: patron (" . $patron_barcode . ")";
 57     my $patron = Koha::Patrons->find( { cardnumber => $patron_barcode } );
 58     my $borrower = $patron->unblessed;
 59     $debug and warn "do_checkout borrower: . " . Dumper $borrower;
 60 =======
 61     siplog('LOG_DEBUG', "ILS::Transaction::Checkout performing checkout...");
 62     my $shelf          = $self->{item}->hold_shelf;
 63     my $barcode        = $self->{item}->id;
 64     my $patron         = Koha::Patrons->find($self->{patron}->{borrowernumber});
 65     my $overridden_duedate; # usually passed as undef to AddIssue
 66     $debug and warn "do_checkout borrower: . " . $patron->borrowernumber;
 67 >>>>>>> 23dd6651f8... Bug 23403: Remove cardnumber from SIP
Comment 35 Nick Clemens 2020-05-21 10:56:27 UTC
(In reply to Joy Nelson from comment #34)
> First commit when cherry-picked gives this error:

Indeed, I simply applied the patches directly from the report and that worked, can you try that?
Comment 36 Joy Nelson 2020-05-21 20:53:48 UTC
backported to 19.11.x for 19.11.06
Comment 37 Victor Grousset/tuxayo 2020-06-11 13:36:36 UTC
Backported to 19.05.x branch for 19.05.12