Summary: | Renewals cause Sipserver to crash | ||
---|---|---|---|
Product: | Koha | Reporter: | Colin Campbell <colin.campbell> |
Component: | SIP2 | Assignee: | 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
Created attachment 41452 [details] [review] Proposed patch Not the most elegant solution, but this should allow renewals to proceed without causing 'Communications Error' at the client end 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 Created attachment 42355 [details] [review] 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: Chris Cormack <chrisc@catalyst.net.nz> We have this patch in production, it does solve the issue Colin, Wouldn't it possible to move the duedatefromissue subroutine to a method in C4::SIP::ILS::Transaction? It'd avoid c/p code. 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 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. (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 Created attachment 43014 [details] [review] Revised patch Revised to remove duplication of method Revised patch to inherit method from base class - works for me with Checkin and Renewal requests - please test Created attachment 44328 [details] [review] 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> 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> Patch pushed to master. Thanks Colin! This patch has been pushed to 3.20.x, will be in 3.20.6. (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? (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 Pushed to 3.18.13, and released. |