Bug 27166 - SIP2 Connection is killed when an item that was not issued is checked in and generates a transfer
Summary: SIP2 Connection is killed when an item that was not issued is checked in and ...
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 21020
Blocks:
  Show dependency treegraph
 
Reported: 2020-12-07 20:16 UTC by Nick Clemens
Modified: 2020-12-24 14:46 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.01,20.05.07,19.11.13


Attachments
Bug 27166: Unit tests (1.58 KB, patch)
2020-12-08 14:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27166: Set destination_loc to item->homebranch (1.58 KB, patch)
2020-12-08 14:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27166: Unit tests (1.64 KB, patch)
2020-12-09 14:47 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 27166: Set destination_loc to item->homebranch (1.64 KB, patch)
2020-12-09 14:47 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 27166: Unit tests (1.70 KB, patch)
2020-12-10 12:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27166: Set destination_loc to item->homebranch (1.70 KB, patch)
2020-12-10 12:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27166: [20.05.x] Set destination_loc to item->homebranch (1.78 KB, patch)
2020-12-18 19:53 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-07 20:16:18 UTC
bug 21020 updated the SIP item with the destination branch by fetching it from the 'issue' object - if the item doesn't have an issue then the connection dies:

problematic line in C4/SIP/ILS/Transaction/Checkin.pm:
$self->{item}->destination_loc($issue->item->homebranch);

To recreate:
1 - Have an item belonging to branch A
2 - Check it in via sip at branch B:
    perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 --item CHESS1 -l FPL -m checkin
3 - The read command is blank and there is a warn about uninitialized data
Comment 1 Nick Clemens 2020-12-08 14:22:06 UTC
Created attachment 114257 [details] [review]
Bug 27166: Unit tests
Comment 2 Nick Clemens 2020-12-08 14:22:09 UTC
Created attachment 114258 [details] [review]
Bug 27166: Set destination_loc to item->homebranch

In bug 21020 we used the issue to fetch the item and then the homebranch.
This fails when the item is not issued. We fetch the item already, so we
can use it directly

To test:
1 - Have an item belonging to branch A
2 - Check it in via sip at branch B:
    perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 --item CHESS1 -l FPL -m checkin
3 - The read command is blank and there is a warn about uninitialized data
4 - Apply patch
5 - Restart sip server
6 - Check the item in at branch A using the sip cli emulator
7 - Check it in at branch B
8 - This time the checkin should succeed and the output msg should be included
Comment 3 Victor Grousset/tuxayo 2020-12-09 14:47:25 UTC
Created attachment 114281 [details] [review]
Bug 27166: Unit tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 4 Victor Grousset/tuxayo 2020-12-09 14:47:28 UTC
Created attachment 114282 [details] [review]
Bug 27166: Set destination_loc to item->homebranch

In bug 21020 we used the issue to fetch the item and then the homebranch.
This fails when the item is not issued. We fetch the item already, so we
can use it directly

To test:
1 - Have an item belonging to branch A
2 - Check it in via sip at branch B:
    perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 --item CHESS1 -l FPL -m checkin
3 - The read command is blank and there is a warn about uninitialized data
4 - Apply patch
5 - Restart sip server
6 - Check the item in at branch A using the sip cli emulator
7 - Check it in at branch B
8 - This time the checkin should succeed and the output msg should be included

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 5 Martin Renvoize 2020-12-10 12:06:12 UTC
Created attachment 114301 [details] [review]
Bug 27166: Unit tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2020-12-10 12:06:15 UTC
Created attachment 114302 [details] [review]
Bug 27166: Set destination_loc to item->homebranch

In bug 21020 we used the issue to fetch the item and then the homebranch.
This fails when the item is not issued. We fetch the item already, so we
can use it directly

To test:
1 - Have an item belonging to branch A
2 - Check it in via sip at branch B:
    perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 --item CHESS1 -l FPL -m checkin
3 - The read command is blank and there is a warn about uninitialized data
4 - Apply patch
5 - Restart sip server
6 - Check the item in at branch A using the sip cli emulator
7 - Check it in at branch B
8 - This time the checkin should succeed and the output msg should be included

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2020-12-10 12:07:00 UTC
This makes sense and also provides a minor performance boost (not double fetching the item).

Tests pass, QA script passes... Passing QA
Comment 8 Jonathan Druart 2020-12-14 13:18:19 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 9 Fridolin Somers 2020-12-14 17:17:55 UTC
Pushed to 20.11.x for 20.11.01
Comment 10 Nick Clemens 2020-12-18 19:53:48 UTC
Created attachment 114536 [details] [review]
Bug 27166: [20.05.x] Set destination_loc to item->homebranch

In bug 21020 we used the issue to fetch the item and then the homebranch.
This fails when the item is not issued. We fetch the item already, so we
can use it directly

In 20.05.x we don't have the item object, so we can just use the SIP transaction item

To test:
1 - Have an item belonging to branch A
2 - Check it in via sip at branch B:
    perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 --item CHESS1 -l FPL -m checkin
3 - The read command is blank and there is a warn about uninitialized data
4 - Apply patch
5 - Restart sip server
6 - Check the item in at branch A using the sip cli emulator
7 - Check it in at branch B
8 - This time the checkin should succeed and the output msg should be included

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Andrew Fuerste-Henry 2020-12-18 20:07:09 UTC
Pushed to 20.05.x for 20.05.07
Comment 12 Victor Grousset/tuxayo 2020-12-21 18:55:28 UTC
The tests pass in 19.11.x but I have an error message not in 20.05.x

kohadev-koha@9ddfdbbbd877:/kohadevbox/koha$ prove t/db_dependent/SIP/Transaction.t
t/db_dependent/SIP/Transaction.t .. 7/10 AddReturn error: branch 'SIP2' not found.  Reverting to bH8GNQ at /kohadevbox/koha/C4/Circulation.pm line 1844.
AddReturn error: branch 'SIP2' not found.  Reverting to bH8GNQ at /kohadevbox/koha/C4/Circulation.pm line 1844.
t/db_dependent/SIP/Transaction.t .. 9/10 AddReturn error: branch 'SIP2' not found.  Reverting to lw0u4bBY02 at /kohadevbox/koha/C4/Circulation.pm line 1844.
AddReturn error: branch 'SIP2' not found.  Reverting to lw0u4bBY02 at /kohadevbox/koha/C4/Circulation.pm line 1844.
t/db_dependent/SIP/Transaction.t .. ok     
All tests successful.
Files=1, Tests=10,  6 wallclock secs ( 0.04 usr  0.00 sys +  4.78 cusr  0.78 csys =  5.60 CPU)
Result: PASS

That doesn't look good right?
Comment 13 Nick Clemens 2020-12-23 13:36:12 UTC
(In reply to Victor Grousset/tuxayo from comment #12)
> The tests pass in 19.11.x but I have an error message not in 20.05.x
> 
> kohadev-koha@9ddfdbbbd877:/kohadevbox/koha$ prove
> t/db_dependent/SIP/Transaction.t
> t/db_dependent/SIP/Transaction.t .. 7/10 AddReturn error: branch 'SIP2' not
> found.  Reverting to bH8GNQ at /kohadevbox/koha/C4/Circulation.pm line 1844.
> AddReturn error: branch 'SIP2' not found.  Reverting to bH8GNQ at
> /kohadevbox/koha/C4/Circulation.pm line 1844.
> t/db_dependent/SIP/Transaction.t .. 9/10 AddReturn error: branch 'SIP2' not
> found.  Reverting to lw0u4bBY02 at /kohadevbox/koha/C4/Circulation.pm line
> 1844.
> AddReturn error: branch 'SIP2' not found.  Reverting to lw0u4bBY02 at
> /kohadevbox/koha/C4/Circulation.pm line 1844.
> t/db_dependent/SIP/Transaction.t .. ok     
> All tests successful.
> Files=1, Tests=10,  6 wallclock secs ( 0.04 usr  0.00 sys +  4.78 cusr  0.78
> csys =  5.60 CPU)
> Result: PASS
> 
> That doesn't look good right?

I get the same warnings before this patch
Comment 14 Victor Grousset/tuxayo 2020-12-24 14:46:27 UTC
My bad, thanks for doing the right check.

Backported to 19.11.x branch for 19.11.13 :D