Summary: | SIP Checkin Response alert flag set to often set to Y incorrectly | ||
---|---|---|---|
Product: | Koha | Reporter: | Kyle M Hall <kyle> |
Component: | SIP2 | Assignee: | Kyle M Hall <kyle> |
Status: | CLOSED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | normal | ||
Priority: | P5 - low | CC: | colin.campbell, josef.moravec, m.de.rooy, martin.renvoize, nick, sally.healey |
Version: | master | ||
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: | ||
Bug Depends on: | 22016 | ||
Bug Blocks: | 22076 | ||
Attachments: |
Bug 22043: SIP Checkin Response alert flag set to often set to Y incorrectly
Bug 22043: SIP Checkin Response alert flag set to often set to Y incorrectly Bug 22043: (QA Follow-up) Add paramter to control behavior Bug 22043: (QA Follow-up) Add paramter to control behavior Bug 22043: SIP Checkin Response alert flag set to often set to Y incorrectly Bug 22043: (QA follow-up) Add parameter to control behavior |
Description
Kyle M Hall
2018-12-21 18:54:00 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 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> 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. 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. (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. 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. Created attachment 85884 [details] [review] Bug 22043: (QA Follow-up) Add paramter to control behavior Created attachment 85897 [details] [review] Bug 22043: (QA Follow-up) Add paramter to control behavior 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> 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> t/db_dependent/SIP/Message.t | 21 ++++++++++++++++++++- Great ! Awesome work all! Pushed to master for 19.05 Bug with enhancement not in 18.11.x series. *** Bug 21218 has been marked as a duplicate of this bug. *** |