The problem occurs when we find a hold during SIP checkin - we check the message form AddReturn for the hold value - but for a waiting hold at wrong location the hold has been reverted and, in the case of a title level hold, has no itemnumber We partially succeed in setting the hold to transit and leave the hold in limbo This is due to passing $messages->{ResFound}->{itemnumber} to ModReserveAffect rather than $item->itemnumber which we have available To recreate: 1 - Place a next available hold for delivery at library A 2 - Check the item in at library A to set it waiting 3 - Check the item in via SIP at library B perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 --item BARCODE -l FPL -m checkin 4 - The SIP emulator dies: Use of uninitialized value $data in concatenation (.) or string at misc/sip_cli_emulator.pl line 344, <GEN0> chunk 1. READ: 5 - Check the hold in the db: SELECT * FROM reserves WHERE itemnumber IS NULL and found IS NOT NULL; 5 - Try to load the biblio - ISE Can't call method holdingbranch on an undefined object
Internal server error mentioned is: Can't call method "holdingbranch" on an undefined value at /kohadevbox/koha/reserve/request.pl line 654
What's the expected behaviour?
Created attachment 114387 [details] [review] Bug 27196: Don't explode if item that is waiting if checked in by SIP
Can you detail what's the expected behaviour, Nick? I am not sure this fix is correct (ie. do we want to start the transfer on a SIP operation?)
Created attachment 114392 [details] [review] Bug 27196: Don't explode if item that is waiting if checked in by SIP Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Jonathan Druart from comment #4) > Can you detail what's the expected behaviour, Nick? > I am not sure this fix is correct (ie. do we want to start the transfer on a > SIP operation?) I believe this is the expected behaviour, it should mimic what happens when a hold is found in general via sip - all you have done is prevent an error, not changed behaviour
Created attachment 114622 [details] [review] Bug 27196: Don't explode if item that is waiting if checked in by SIP Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
As a step towards QA, here is another signoff :)
My test plan: 0. koha-sip --restart kohadev 1. Go to http://localhost:8081/cgi-bin/koha/reserve/request.pl?borrowernumber=51&biblionumber=29 2. Click "Place hold" 3. koha-shell kohadev 4. perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 --item 39999000001310 -l FPL -m checkin 5. Note that SIP emulator does not die and you get the following output: Attempting socket connection to localhost:6001...connected! SEND: 9300CNterm1|COterm1|CPFPL| READ: 941 Trying 'checkin' SEND: 09N20201223 23152620201223 231526APFPL|AOFPL|AB39999000001310|ACterm1|BIN| READ: 101YNY20201223 231526AOFPL|AB39999000001310|AQCPL|AJGairm.|CY42|CTCPL|DAkoha|CRREF|CV02| 6. Go to http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51 7. Note that the hold displays normally as in transit from Fairview 8. Go to http://localhost:8081/cgi-bin/koha/reserve/request.pl?biblionumbers=29/ 9. Note that the hold displays normally as being transferred to Centerville
prove t/db_dependent/SIP/Transaction.t t/db_dependent/SIP/Transaction.t .. ok All tests successful. Files=1, Tests=11, 5 wallclock secs ( 0.04 usr 0.00 sys + 3.66 cusr 0.40 csys = 4.10 CPU) Result: PASS -- qa -c 2 -v 2 testing 2 commit(s) (applied to 470c760 '69 Bug 26941: Fix OPAC password recov') Processing files before patches |========================>| 2 / 2 (100.00%) Processing files after patches |========================>| 2 / 2 (100.00%) OK C4/SIP/ILS/Transaction/Checkin.pm OK t/db_dependent/SIP/Transaction.t Processing additional checks OK! -- Code review looks good to me. Sensible change.
Created attachment 114693 [details] [review] Bug 27196: Don't explode if item that is waiting if checked in by SIP Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: David Cook <dcook@prosentient.com.au>
Pushed to master for 21.05, thanks to everybody involved!
Patch removes a # in C4/SIP/ILS/Transaction/Checkin.pm : https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114693&action=diff#a/C4/SIP/ILS/Transaction/Checkin.pm_sec1 Is this on purpose ?
(In reply to Fridolin Somers from comment #13) > Patch removes a # in C4/SIP/ILS/Transaction/Checkin.pm : > > https://bugs.koha-community.org/bugzilla3/attachment. > cgi?id=114693&action=diff#a/C4/SIP/ILS/Transaction/Checkin.pm_sec1 > > Is this on purpose ? It was not, but shouldn't hurt anything
(In reply to Nick Clemens from comment #14) > (In reply to Fridolin Somers from comment #13) > > Patch removes a # in C4/SIP/ILS/Transaction/Checkin.pm : > > > > https://bugs.koha-community.org/bugzilla3/attachment. > > cgi?id=114693&action=diff#a/C4/SIP/ILS/Transaction/Checkin.pm_sec1 > > > > Is this on purpose ? > > It was not, but shouldn't hurt anything OK no pb.
Pushed to 20.11.x for 20.11.01
Gives some errors on 20.05, please rebase. qa -c 1 -v 2 testing 1 commit(s) (applied to 4d16cdd '7c Bug 27256: Update click handler fo') Processing files before patches |========================>| 2 / 2 (100.00%) Processing files after patches |========================>| 2 / 2 (100.00%) FAIL C4/SIP/ILS/Transaction/Checkin.pm FAIL valid Global symbol "$item" requires explicit package name (did you forget to declare "my $item"?) Global symbol "$item" requires explicit package name (did you forget to declare "my $item"?) C4/SIP/ILS/Transaction/Checkin.pm had compilation errors. FAIL t/db_dependent/SIP/Transaction.t FAIL valid Global symbol "$item" requires explicit package name (did you forget to declare "my $item"?) Global symbol "$item" requires explicit package name (did you forget to declare "my $item"?) Compilation failed in require Compilation failed in require BEGIN failed--compilation aborted
Created attachment 115711 [details] [review] Bug 27196:[20.05.x] Don't explode if item that is waiting if checked in by SIP Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: David Cook <dcook@prosentient.com.au>
Pushed to 20.05.x for 20.05.08
Can't backport to 19.11.x, a test is failing, help needed if this patch is wanted in 19.11.x # Failed test 'No tests run for subtest "Checkin an in transit item"' # at t/db_dependent/SIP/Transaction.t line 338. # Looks like you failed 1 test of 12. t/db_dependent/SIP/Transaction.t .. 8/10 # Failed test 'do_checkin' # at t/db_dependent/SIP/Transaction.t line 339. DBIx::Class::Row::store_column(): No such column 'biblionumber' on Koha::Schema::Result::Branch at /kohadevbox/koha/Koha/Object.pm line 77
Created attachment 115757 [details] [review] Bug 27196:[19.11.x] Don't explode if item that is waiting if checked in by SIP AddReserve doesn't take a hash in 19.11 - amended the call in the tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: David Cook <dcook@prosentient.com.au>
Thanks Nick :D Backported: Pushed to 19.11.x branch for 19.11.15