Bug 22043 - SIP Checkin Response alert flag set to often set to Y incorrectly
Summary: SIP Checkin Response alert flag set to often set to Y incorrectly
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
: 21218 (view as bug list)
Depends on: 22016
Blocks: 22076
  Show dependency treegraph
 
Reported: 2018-12-21 18:54 UTC by Kyle M Hall
Modified: 2019-04-11 14:16 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 22043: SIP Checkin Response alert flag set to often set to Y incorrectly (2.18 KB, patch)
2018-12-21 18:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22043: SIP Checkin Response alert flag set to often set to Y incorrectly (2.29 KB, patch)
2019-01-02 13:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22043: (QA Follow-up) Add paramter to control behavior (4.02 KB, patch)
2019-02-28 21:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22043: (QA Follow-up) Add paramter to control behavior (5.88 KB, patch)
2019-03-01 11:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22043: SIP Checkin Response alert flag set to often set to Y incorrectly (2.37 KB, patch)
2019-03-15 11:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22043: (QA follow-up) Add parameter to control behavior (5.96 KB, patch)
2019-03-15 11:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2018-12-21 18:54:00 UTC
If a checkin has any messages at all, even informational like LocalUse, the alert flag ( 4th character of the response ) will be set to Y, even if we do not prove a corresponding alert code in the CV field. We should only set the alert flag if we are going to send a value in the CV field.
Comment 1 Kyle M Hall 2018-12-21 18:56:23 UTC
Created attachment 83468 [details] [review]
Bug 22043: SIP Checkin Response alert flag set to often set to Y incorrectly

If a checkin has any messages at all, even informational like LocalUse, the alert flag ( 4th character of the response ) will be set to Y, even if we do not prove a corresponding alert code in the CV field. We should only set the alert flag if we are going to send a value in the CV field.If a checkin has any messages at all, even informational like LocalUse, the alert flag ( 4th character of the response ) will be set to Y, even if we do not prove a corresponding alert code in the CV field. We should only set the alert flag if we are going to send a value in the CV field.

Test Plan:
1) Set the checked_in_ok sip account flag to 1 for an account
2) Check in an already checked in item via SIP
3) Note the alert flag is set to Y
4) Apply this patch
5) Restart SIP
6) Check the item in again
7) Note the alert flag is set to N
Comment 2 Kyle M Hall 2019-01-02 13:21:05 UTC
Created attachment 83578 [details] [review]
Bug 22043: SIP Checkin Response alert flag set to often set to Y incorrectly

If a checkin has any messages at all, even informational like LocalUse, the alert flag ( 4th character of the response ) will be set to Y, even if we do not prove a corresponding alert code in the CV field. We should only set the alert flag if we are going to send a value in the CV field.If a checkin has any messages at all, even informational like LocalUse, the alert flag ( 4th character of the response ) will be set to Y, even if we do not prove a corresponding alert code in the CV field. We should only set the alert flag if we are going to send a value in the CV field.

Test Plan:
1) Set the checked_in_ok sip account flag to 1 for an account
2) Check in an already checked in item via SIP
3) Note the alert flag is set to Y
4) Apply this patch
5) Restart SIP
6) Check the item in again
7) Note the alert flag is set to N

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jill Kleven <jill.kleven@pueblolibrary.org>
Comment 3 Marcel de Rooy 2019-02-15 08:56:28 UTC
QA Comment:

-    $self->alert(!$return);

Although this seems to be okay, I am not fully sure about it. Do we overlook some codes? Or will this present a problem if we add a code but forgot to update SIP?
@Colin: What is your opinion about this one ?

Temporary parking into FQA for feedback.
Comment 4 Kyle M Hall 2019-02-19 17:49:59 UTC
Still waiting on feedback. Can we get this moving forward if we don't hear back by the end of the week? I can move the functionality behind a server parameter if necessary, but I as far as I can tell we are violating the standard with our current behavior.

(In reply to Marcel de Rooy from comment #3)
> QA Comment:
> 
> -    $self->alert(!$return);
> 
> Although this seems to be okay, I am not fully sure about it. Do we overlook
> some codes? Or will this present a problem if we add a code but forgot to
> update SIP?
> @Colin: What is your opinion about this one ?
> 
> Temporary parking into FQA for feedback.
Comment 5 Colin Campbell 2019-02-27 16:31:42 UTC
(In reply to Kyle M Hall from comment #4)
> Still waiting on feedback. Can we get this moving forward if we don't hear
> back by the end of the week? I can move the functionality behind a server
> parameter if necessary, but I as far as I can tell we are violating the
> standard with our current behavior.
> 
> (In reply to Marcel de Rooy from comment #3)
> > QA Comment:
> > 
> > -    $self->alert(!$return);
> > 
> > Although this seems to be okay, I am not fully sure about it. Do we overlook
> > some codes? Or will this present a problem if we add a code but forgot to
> > update SIP?
> > @Colin: What is your opinion about this one ?
> > 
> > Temporary parking into FQA for feedback.

No it doesnt violate the standard, the link between there being a message and the alert is a convention that the original code adopted. Similarly the assumption that CV must be present, CV was not part of the original spec, and will be ignored by some clients. Unfortunately its become more complex as since then we've added/suppressed various messages in the circ routines, and informational messages have been added. That said I suspect nowhere is reliant on the alert flag's current behaviour.
Comment 6 Kyle M Hall 2019-02-28 20:57:15 UTC
In that case, I'll add a SIP config parameter to make this configurable.

(In reply to Colin Campbell from comment #5)
> No it doesnt violate the standard, the link between there being a message
> and the alert is a convention that the original code adopted. Similarly the
> assumption that CV must be present, CV was not part of the original spec,
> and will be ignored by some clients. Unfortunately its become more complex
> as since then we've added/suppressed various messages in the circ routines,
> and informational messages have been added. That said I suspect nowhere is
> reliant on the alert flag's current behaviour.
Comment 7 Kyle M Hall 2019-02-28 21:13:28 UTC
Created attachment 85884 [details] [review]
Bug 22043: (QA Follow-up) Add paramter to control behavior
Comment 8 Kyle M Hall 2019-03-01 11:53:59 UTC
Created attachment 85897 [details] [review]
Bug 22043: (QA Follow-up) Add paramter to control behavior
Comment 9 Marcel de Rooy 2019-03-15 11:06:08 UTC
Created attachment 86668 [details] [review]
Bug 22043: SIP Checkin Response alert flag set to often set to Y incorrectly

If a checkin has any messages at all, even informational like LocalUse, the alert flag ( 4th character of the response ) will be set to Y, even if we do not prove a corresponding alert code in the CV field. We should only set the alert flag if we are going to send a value in the CV field.If a checkin has any messages at all, even informational like LocalUse, the alert flag ( 4th character of the response ) will be set to Y, even if we do not prove a corresponding alert code in the CV field. We should only set the alert flag if we are going to send a value in the CV field.

Test Plan:
1) Set the checked_in_ok sip account flag to 1 for an account
2) Check in an already checked in item via SIP
3) Note the alert flag is set to Y
4) Apply this patch
5) Restart SIP
6) Check the item in again
7) Note the alert flag is set to N

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jill Kleven <jill.kleven@pueblolibrary.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2019-03-15 11:06:13 UTC
Created attachment 86669 [details] [review]
Bug 22043: (QA follow-up) Add parameter to control behavior

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2019-03-15 11:07:43 UTC
t/db_dependent/SIP/Message.t      | 21 ++++++++++++++++++++-

Great !
Comment 12 Nick Clemens 2019-03-15 12:47:55 UTC
Awesome work all!

Pushed to master for 19.05
Comment 13 Martin Renvoize 2019-03-19 10:07:46 UTC
Bug with enhancement not in 18.11.x series.
Comment 14 Colin Campbell 2019-04-11 14:16:32 UTC
*** Bug 21218 has been marked as a duplicate of this bug. ***