Bug 14673

Summary: Renewals cause Sipserver to crash
Product: Koha Reporter: Colin Campbell <colin.campbell>
Component: SIP2Assignee: Colin Campbell <colin.campbell>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P3 CC: barton, colin.campbell, f.demians, gwilliams, jonathan.druart, katrin.fischer, kyle, liz, magnus, tomascohen, ulrich.kleiber
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Proposed patch
Bug 14673 Work around change to AddIssue return
Revised patch
Bug 14673 Work around change to AddIssue return
[PASSED QA] Bug 14673 Work around change to AddIssue return

Description Colin Campbell 2015-08-11 14:21:19 UTC
Bug 13315 Changed the return of C4::Circulation::AddIssue to a hash reflecting the added issue ; However if AddIssue is called with the borrower currently having the item on loan a renewal is carried out nut as no issue is created in the db undef is returned rather than the issue structure. The same bug made the first thing the sipserver does is to extract the due_date from that non-existent structure causing a fatal error if it is a renewal. 

This is potentially a major bug as almost all self-issue units use checkout requests rather than renewal requests when renewing borrowers items
Comment 1 Colin Campbell 2015-08-11 16:11:05 UTC Comment hidden (obsolete)
Comment 2 Colin Campbell 2015-09-03 15:22:58 UTC
Upping priority as this needs applying to releases from 3.18.7 to stop communication errors on renew. The change causing the error was bug 13315
Comment 3 Chris Cormack 2015-09-03 21:03:29 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2015-09-15 08:05:28 UTC
Colin,
Wouldn't it possible to move the duedatefromissue subroutine to a method in C4::SIP::ILS::Transaction?
It'd avoid c/p code.
Comment 5 Magnus Enger 2015-10-01 07:42:31 UTC
Colin: What do you think of the proposal from Jonathan? 
Jonathan: Could you provide a counter patch? Avoiding copypasta sounds like a good idea.
Comment 6 Jonathan Druart 2015-10-01 11:04:58 UTC
(In reply to Magnus Enger from comment #5)
> Colin: What do you think of the proposal from Jonathan? 
> Jonathan: Could you provide a counter patch? Avoiding copypasta sounds like
> a good idea.

No, I would like to keep my QA token.
Comment 7 Colin Campbell 2015-10-01 13:08:25 UTC
(In reply to Magnus Enger from comment #5)
> Colin: What do you think of the proposal from Jonathan? 
> Jonathan: Could you provide a counter patch? Avoiding copypasta sounds like
> a good idea.

(In reply to Jonathan Druart from comment #6)
> (In reply to Magnus Enger from comment #5)
> > Colin: What do you think of the proposal from Jonathan? 
> > Jonathan: Could you provide a counter patch? Avoiding copypasta sounds like
> > a good idea.
> 
> No, I would like to keep my QA token.

Its not really a transaction method but a utility but will attempt to test that solution
Comment 8 Colin Campbell 2015-10-01 14:22:56 UTC Comment hidden (obsolete)
Comment 9 Colin Campbell 2015-10-01 14:24:42 UTC
Revised patch to inherit method from base class - works for me with Checkin and Renewal requests - please test
Comment 10 Frédéric Demians 2015-11-03 11:18:13 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2015-11-06 12:18:25 UTC
Created attachment 44548 [details] [review]
[PASSED QA] Bug 14673 Work around change to AddIssue return

Return from AddIssue used to be due date or undef.
Now it is less straightforward returning am issue object
if an issue row is created or undef. If the issue is a renewal
undef is returned. As that case was not handled properly it
caused the server site to crash the listener causing a
communications error on the client.

Signed-off-by: Frederic Demians <f.demians@tamil.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Tomás Cohen Arazi 2015-11-06 15:12:18 UTC
Patch pushed to master.

Thanks Colin!
Comment 13 Frédéric Demians 2015-11-29 20:24:41 UTC
This patch has been pushed to 3.20.x, will be in 3.20.6.
Comment 14 Barton Chittenden 2015-12-01 15:18:31 UTC
(In reply to Colin Campbell from comment #2)
> Upping priority as this needs applying to releases from 3.18.7 to stop
> communication errors on renew. The change causing the error was bug 13315

I see that this has been pushed to 3.20.x, can we get it back-ported to 3.18.x?
Comment 15 Colin Campbell 2015-12-10 14:09:15 UTC
(In reply to Barton Chittenden from comment #14)
> (In reply to Colin Campbell from comment #2)
> > Upping priority as this needs applying to releases from 3.18.7 to stop
> > communication errors on renew. The change causing the error was bug 13315
> 
> I see that this has been pushed to 3.20.x, can we get it back-ported to
> 3.18.x?

Patch as is should apply to 3.18.x
Comment 16 Liz Rea 2016-01-06 21:05:44 UTC
Pushed to 3.18.13, and released.