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!
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.
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?
Confirmed.
Created attachment 95703 [details] [review] Bug 23403: Remove cardnumber from SIP
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.
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!
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
Created attachment 96404 [details] [review] Bug 23403: Remove cardnumber from SIP
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.
Created attachment 101303 [details] [review] Bug 23403: Remove cardnumber from SIP Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Created attachment 101304 [details] [review] Bug 23403: Remove cardnumber from SIP Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Created attachment 101445 [details] [review] Bug 23403: Fix whitespace
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
Created attachment 101456 [details] [review] Bug 23403: List borrowernumber in the ILS::Patron fields
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
Created attachment 102859 [details] [review] Bug 23403: Catch other cases of ->{patron}
IIUC the test plan is - comment 0 - prove t/db_dependent/SIP/*
(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
> 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
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!
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>
Created attachment 104314 [details] [review] Bug 23403: Fix whitespace Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 104315 [details] [review] Bug 23403: List borrowernumber in the ILS::Patron fields Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 104316 [details] [review] Bug 23403: Catch other cases of ->{patron} Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
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)
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>
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>
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>
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>
Nick, how confident are you with this change? Do you think it should be part of 20.05?
Nice work everyone! Pushed to master for 20.05
Does not apply to 19.11.x- please rebase if needed.
(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
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
(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?
backported to 19.11.x for 19.11.06
Backported to 19.05.x branch for 19.05.12