Bug 27196 - Waiting title level hold checked in at wrong location via SIP leaves hold in a broken state and drops connection
Summary: Waiting title level hold checked in at wrong location via SIP leaves hold in ...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: David Cook
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-12-10 18:55 UTC by Nick Clemens
Modified: 2021-12-13 21:09 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.01,20.05.08,19.11.15


Attachments
Bug 27196: Don't explode if item that is waiting if checked in by SIP (3.67 KB, patch)
2020-12-14 14:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27196: Don't explode if item that is waiting if checked in by SIP (3.78 KB, patch)
2020-12-14 16:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27196: Don't explode if item that is waiting if checked in by SIP (3.83 KB, patch)
2020-12-22 21:01 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 27196: Don't explode if item that is waiting if checked in by SIP (3.89 KB, patch)
2020-12-23 23:20 UTC, David Cook
Details | Diff | Splinter Review
Bug 27196:[20.05.x] Don't explode if item that is waiting if checked in by SIP (3.88 KB, patch)
2021-01-22 19:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27196:[19.11.x] Don't explode if item that is waiting if checked in by SIP (3.76 KB, patch)
2021-01-25 12:21 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-12-10 18:55:57 UTC
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
Comment 1 Nick Clemens 2020-12-10 19:02:37 UTC
Internal server error mentioned is:
Can't call method "holdingbranch" on an undefined value at /kohadevbox/koha/reserve/request.pl line 654
Comment 2 Jonathan Druart 2020-12-14 14:51:57 UTC
What's the expected behaviour?
Comment 3 Jonathan Druart 2020-12-14 14:55:35 UTC
Created attachment 114387 [details] [review]
Bug 27196: Don't explode if item that is waiting if checked in by SIP
Comment 4 Jonathan Druart 2020-12-14 14:56:32 UTC
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?)
Comment 5 Nick Clemens 2020-12-14 16:23:10 UTC
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>
Comment 6 Nick Clemens 2020-12-14 16:25:45 UTC
(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
Comment 7 Victor Grousset/tuxayo 2020-12-22 21:01:46 UTC
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>
Comment 8 Victor Grousset/tuxayo 2020-12-22 21:03:08 UTC
As a step towards QA, here is another signoff :)
Comment 9 David Cook 2020-12-23 23:16:30 UTC
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
Comment 10 David Cook 2020-12-23 23:19:55 UTC
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.
Comment 11 David Cook 2020-12-23 23:20:54 UTC
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>
Comment 12 Jonathan Druart 2020-12-26 17:15:25 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 13 Fridolin Somers 2020-12-28 15:05:33 UTC
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 ?
Comment 14 Nick Clemens 2020-12-28 15:33:43 UTC
(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
Comment 15 Fridolin Somers 2020-12-30 13:49:50 UTC
(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.
Comment 16 Fridolin Somers 2020-12-30 14:16:19 UTC
Pushed to 20.11.x for 20.11.01
Comment 17 Andrew Fuerste-Henry 2021-01-04 14:39:38 UTC
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
Comment 18 Nick Clemens 2021-01-22 19:11:43 UTC
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>
Comment 19 Andrew Fuerste-Henry 2021-01-22 19:47:48 UTC
Pushed to 20.05.x for 20.05.08
Comment 20 Victor Grousset/tuxayo 2021-01-22 21:25:15 UTC
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
Comment 21 Nick Clemens 2021-01-25 12:21:49 UTC
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>
Comment 22 Victor Grousset/tuxayo 2021-02-01 18:14:40 UTC
Thanks Nick :D

Backported: Pushed to 19.11.x branch for 19.11.15