Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The update_notifications transition is not muting schedules #3362

Closed
abbyad opened this issue Apr 10, 2017 · 10 comments
Closed

The update_notifications transition is not muting schedules #3362

abbyad opened this issue Apr 10, 2017 · 10 comments
Assignees
Labels
Priority: 2 - Medium Normal priority Sentinel Affects the Sentinel service Type: Bug Fix something that isn't working as intended

Comments

@abbyad
Copy link
Contributor

abbyad commented Apr 10, 2017

Reported in https://github.com/medic/medic-projects/issues/1813, the ON and OFF forms are not getting auto responses, nor are they muting/unmuting schedules.

I was hoping this was a simple config issue but was not able to find the source of the problem. I have put further notes in the original issue.

@abbyad abbyad added Priority: 1 - High Blocks the next release. Regression Affects a feature that worked in a previous release labels Apr 10, 2017
@sglangevin sglangevin added this to the March 28th - April 11th milestone Apr 10, 2017
@garethbowen garethbowen self-assigned this Apr 10, 2017
@garethbowen
Copy link
Member

Related to #3190

@abbyad
Copy link
Contributor Author

abbyad commented Apr 11, 2017

This is in fact (partly) a config issue, and medic/medic-projects#1813 can be resolved for the time being by reverting to the older way of defining messages with message, not translation_key.

The larger issue of ON/OFF forms not working as expected is still valid, and not fully resolved by #3190.

Messages appear as deprecated, but are in fact required

image

Report not processed if autoreply is omitted

If the notifications section is missing the messages.message field, or even the whole messages object, the transition detects that and stops processing the form altogether. As a result, the form:

  • does not have any outgoing messages (which is expected since there isn't a message!)
  • does not provide any of the validation errors
  • does not provide any ID not found errors
  • provides no error in Sentinel logs
  • provides no error in UI (on the contrary, it shows a valid green circle)
  • does not do the mute/unmute action even though it appears as valid

To avoid this we will make sure projects have notifications with messages (or translation_key after 3190). I am removing the priority label and marking as a bug because although it could be considered a feature request to mute/unmute without messages, it is a UX bug to not do the mute/unmute action while not providing any errors.

Report shows as valid even if the patient ID is missing

As reported by Bishwas, the usual validations for patient ID like regex('^[0-9]{5}$') and lenMin(5) do not work for notification forms if the patient ID field is missing. The way to deal with this is to make the field required in the JSON definition of the form.

edited to reflect #3362 (comment)

@abbyad abbyad added Type: Bug Fix something that isn't working as intended and removed Priority: 1 - High Blocks the next release. Regression Affects a feature that worked in a previous release labels Apr 11, 2017
@abbyad
Copy link
Contributor Author

abbyad commented Apr 11, 2017

I have also removed this from the 2.11 project as it is no longer a blocker for that release.

@bishwasBhatta
Copy link

@abbyad regarding the validations and ID not found errors not showing up, it is happening even when the message object is present: https://github.com/medic/medic-projects/issues/1825

@abbyad
Copy link
Contributor Author

abbyad commented Apr 11, 2017

Thanks @bishwas-medic, just tested that and you are right, that is a problem. I will update the comment above to include that as another way the forms don't behave as expected.

@garethbowen
Copy link
Member

Closing as @abbyad reports it's a configuration issue - details in medic/medic-projects#1813

Reopen if this is incorrect!

@garethbowen garethbowen added Won’t fix: Not an issue Working as expected and removed 2 - Active Work labels Apr 17, 2017
@garethbowen garethbowen removed their assignment Apr 17, 2017
@garethbowen garethbowen removed the Type: Bug Fix something that isn't working as intended label Apr 17, 2017
@abbyad
Copy link
Contributor Author

abbyad commented Apr 17, 2017

Although it can be partly mitigated in configuration, it is problematic to show a notification report as valid when the intended mute/unmute action is not taken. Because of that we should:

  1. make necessary change so that reports are completely processed even if outgoing messages are missing
    OR
  2. document that the notification messages are required in app_settings and config docs

If we chose the latter option we should also consider proper handling when messages are missing (eg reports marked as invalid, and errors in Sentinel logs), or as part of a more comprehensive error handling of the configuration.

@abbyad abbyad reopened this Apr 17, 2017
@garethbowen garethbowen added 1 - Scheduled Type: Bug Fix something that isn't working as intended and removed Won’t fix: Not an issue Working as expected labels Apr 17, 2017
@garethbowen
Copy link
Member

@dianabarsan Do you think you work for #4767 will fix this one too?

@dianabarsan
Copy link
Member

@garethbowen not yet, I did not change that part of the behavior. Working on it.

@dianabarsan dianabarsan self-assigned this Nov 5, 2018
@dianabarsan dianabarsan added Status: 2 - Active work Sentinel Affects the Sentinel service Priority: 2 - Medium Normal priority and removed Status: 1 - Triaged labels Nov 5, 2018
@dianabarsan dianabarsan mentioned this issue Nov 5, 2018
5 tasks
dianabarsan added a commit that referenced this issue Nov 6, 2018
Adds muting transition & corresponding webapp style changes for muted contacts.
Deprecates update_notifications transition in favor of muting.
Updates lineage shared library to hydrate patient by uuid as well, not just by patient_id.

#4767
#4768
#3362
#4649
@dianabarsan dianabarsan removed their assignment Nov 6, 2018
@ngaruko ngaruko self-assigned this Nov 12, 2018
@ngaruko
Copy link
Contributor

ngaruko commented Nov 13, 2018

LGTM

@newtewt newtewt closed this as completed Nov 14, 2018
tookam pushed a commit that referenced this issue Nov 19, 2018
Adds muting transition & corresponding webapp style changes for muted contacts.
Deprecates update_notifications transition in favor of muting.
Updates lineage shared library to hydrate patient by uuid as well, not just by patient_id.

#4767
#4768
#3362
#4649
@garethbowen garethbowen changed the title The update_notifications transition is not working as expected The update_notifications transition is not muting schedules Dec 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 2 - Medium Normal priority Sentinel Affects the Sentinel service Type: Bug Fix something that isn't working as intended
Projects
None yet
Development

No branches or pull requests

7 participants