Cht-core: Do not know if patient ID is valid when processing Registrations/Report Actions

[abbyad]

In a person centered SMS workflow a registration report is submitted with the person's patient ID, and the registrations transition is used to create new SMS schedules, while the accept_patient_reports transition is used to clear any previous ones. This works well when the patient ID is valid. When it is invalid it is problematic because the registrations transition proceeds even though the patient ID is invalid.

The result is that:
1) If the patient ID passes the regex validation in Registrations then SMS schedules are created even if no patient is associated to that ID
1) If the patient ID _does not_ pass the regex validation in Registrations then two error messages are being sent, one from each transition:

Spawned off from https://github.com/medic/medic-webapp/issues/3074#issuecomment-275481953, and related to #3075.

[abbyad]

Below is an example of symptom 1 listed above. The report does not pass validation because it does not have the ID of an existing person, but the schedule is still created:

[sglangevin]

To reproduce:

1. Send a pregnancy registration form via SMS P [invalid patient_id] [weeks since LMP].
2. You will get an error SMS that says the ID is invalid but a schedule will still be created for reminders (see screenshot immediately above this comment).

Note that an invalid ID can be either an ID that doesn't exist in the Medic system or and ID that is not of the correct format.

Test cases:

1. P [valid patient_id] [LMP] which should return a confirmation SMS that the pregnancy was registered and create a schedule for the person's pregnancy
2. P [invalid patient_id] [LMP] which should return an error SMS that the ID was invalid and should NOT create a schedule for the person

[sglangevin]

app_settings are here: https://github.com/medic/medic-projects/blob/master/default-sms-workflow/standard/app_settings.json

[abbyad]

Here are some inputs and expected outputs that I have been using as tests while trying to find a way to do this with changes in configuration:

| Test | Input | Expected Output |
| --- | --- | --- |
|Valid LMP| P 66837 2|1 auto reply confirming reception, create LMP ANC schedule|
|No LMP| P 66837 | 1 auto reply confirming reception, create ANC schedule|
|Invalid ID format|P 668370 2|1 error reply for invalid ID format, no schedule|
|Unknown ID|P 12345 2|1 error reply for unknown ID, no schedule|
|Invalid LMP|P 66837 0|1 error reply for invalid LMP, no schedule|
|Invalid LMP|P 66837 43|1 error reply for invalid LMP, no schedule|
|Invalid LMP|P 66837 AB|1 error reply for invalid LMP, no schedule|
|Invalid LMP, Unknown ID|P 12345 AB|1 error reply with unknown ID and invalid LMP, no schedule|
|Invalid LMP & ID|P 668370 AB|1 error reply with invalid ID and invalid LMP, no schedule|

Contents of my csv that I use with load_messages.js:

message, from
P 66837 2, [phone]
P 66837, [phone]
P 668370 2, [phone]
P 12345 2, [phone]
P 66837 0, [phone]
P 66837 43, [phone]
P 66837 AB, [phone]
P 12345 AB, [phone]
P 668370 AB, [phone]

[abbyad]

@garethbowen helped clarify this issue. Since the transitions are meant to run in order, we can rely on accept_patient_reports running before registrations if ordered accordingly. If that is the case then we can prevent the schedule creation by checking that there are no errors added by previous transitions (eg using something like (doc.errors && doc.errors.length === 0) in the bool_expr for the event).

It was also mentioned that it may be worth stopping other transitions from running if any errors are encountered, which would avoid this issue altogether. @mandric, can you think of situations where you'd want other transitions to run even if there was an error?

[mandric]

I think transitions should continue running even when errors occur. Any transition can do anything, like use some random API. If that random API is down that doesn't mean simpler transitions should also fail/never run. If an errors occurs in a transition the doc continues towards a complete state. If transitions want to be dependent on each other they can coordinate via the transitions property. The failed transition should mark itself as failed in the doc and the transition handles that state during backlog processing or the next change.

[SCdF]

OK this is way outta my paygrade. Sounds like I need to talk to @mandric or @garethbowen, were I to do this I'd need some serious pairing on it I think.

[SCdF]

3075 will fix accept_patient_reports. This ticket needs to also fix registrations, such that if a registration is passing in a patient id (not generating a new one), that id is valid.

[SCdF]

@garethbowen please take a look at https://github.com/medic/medic-sentinel/pull/105

Note that this PR bases against the patiently_patient PRs for ease of reading. Once you're happy with it all I'll squash merge it in the correct order.

[abbyad]

We should document the changes required in app_settings to make use of this. I can make some guesses for acceptance testing but it's not entirely evident.

[SCdF]

@abbyad no changes required. the use of the create_patient_id is deprecated in favour of the create_patient trigger, but they will both work for now.

[abbyad]

Testing the V form, which is only in "patient_reports", and it is not accepted if the ID is incorrect -- which is the correct behaviour.

Testing failed however for the P {{patient_id}} form, which is in "registrations" only to assign a schedule, and in "patient_reports" because it clears schedules and should only be accepted for valid contact person IDs. A form with an unknown ID (eg P 12345) is still accepted, but shouldn't be.

Tested on standard.app with 2.10.0-alpha.4865. Returning for further investigation.

[SCdF]

@abbyad can you give me details (privately if you're not comfortable doing it here) about the server etc? Just so I make sure I duplicate the config properly

[abbyad]

Here is the app_settings file

[garethbowen]

I've code reviewed and merged this so it's ready to go back into testing.

[abbyad]

Better! Some of this may need fixes in config rather than code... but not clear what changes are needed.

Should not create schedule due to validation error

P 66837 AB
P 66837 43

Two errors messages

P 12345 AB
P 12345 2
P 668370 2
P 668370 AB

Examples

[abbyad]

I tried fixing this in config by moving validations around between the registrations and accept_patient_reports sections, but always end up with the same issue manifested in different ways: the validation and report_accepted sections acting independently so you can accept a report on one but not the other unless you duplicate the validation. If you do, you end up sending the error messages twice.

[abbyad]

Now that registrations can check for an existing patient ID and generate a registration_not_found message, we could do without the accept_patient_reports section altogether for this issue -- if we can add the schedule silencing to the registrations group. Being able to do that would likely completely resolve this issue.

From the app_settings perspective we would need to move the following from accept_patient_reports:

"silence_type": "ANC Reminders",
"silence_for": "8 days"

To registrations:

      "form": "P",
      "events": [
        {
          /* other triggers that need to happen for a registration form */
        },
        {
          "name": "on_create",
          "trigger": "silence",
          "params": { 
             "silence_type": "ANC Reminders",
             "silence_for": "8 days"
          },
          "bool_expr": ""
        }

Unlike the other triggers we would have 2 params, which isn't necessarily as easy as adding an object like that, but included for demonstration purposes only. Aside from being able to process an additional param, there is also of course the rest of the code that needs to be ported over.

Doing this would also do what Milan suggested in https://github.com/medic/medic-webapp/issues/3074#issuecomment-275183846

[garethbowen]

Just like Milan said in that comment that's too big to do in this release. I'm also really uncomfortable with merging all the transitions into one super transition.

I'm wondering if we should check for errors before assigning a schedule, but it's not guaranteed that the accept_patient_reports transition runs before the registrations transition. This would fix the issue where the schedule is generated even though the patient_id isn't found, but is not guaranteed to fix the issue with the LMP validation.

As far as I can tell this is an existing bug and so I think we can safely put it off for another release, perhaps solved by massive refactoring of sentinel (including ordered transitions so validation runs first, and stopping transition execution on error, etc).

@abbyad Thoughts?

[abbyad]

Agreed that doing a huge refactor for this release seems unwise. Will report back within 12hrs to see if some of the issues (eg necessary duplicate config causing duplicate errors, or not clearing any schedules with a registration form) can be lived with for this release or if this is a blocker. Since we are doing pure ANC only with the first projects, we might be able to release without this fixed.

[abbyad]

In acceptance testing I've noticed that if the patient_id is not found the outgoing message is sys.registration_not_found.

With accept_patient_reports we can specify different messages:

"messages": [
        {
          "translation_key": "messages.v.report_accepted",
          "event_type": "report_accepted",
          "recipient": "reporting_unit"
        },
        {
          "translation_key": "messages.generic.registration_not_found",
          "event_type": "registration_not_found",
          "recipient": "reporting_unit"
        }
      ]

If I add the messages with event_type in registrations it does not help since all the messages go out, even when the report is accepted.

[abbyad]

PRs ready for review: https://github.com/medic/medic-sentinel/issues/109 + https://github.com/medic/medic-webapp/pull/3153

Could perhaps (_always_) use some tests, but hopefully lightweight enough to include in this release if it passes code review.

[abbyad]

Tested again in alpha with the latest modifications and seems to work well. @ngaruko, let me know if you need tips on further testing.