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

Sentinel needs to support these patient_id use cases #3166

Closed
3 tasks done
SCdF opened this issue Feb 27, 2017 · 25 comments
Closed
3 tasks done

Sentinel needs to support these patient_id use cases #3166

SCdF opened this issue Feb 27, 2017 · 25 comments
Labels
Type: Feature Add something new

Comments

@SCdF
Copy link
Contributor

SCdF commented Feb 27, 2017

There are various ways that patient_id gets set, we need to support them all. We currently support:

  • from SMS -> new patient id, new contact
  • migration -> existing internal[1] patient id onto new contact

We need to also support:

This ticket may have code against it, or it may just link to other tickets that take care of this.

[1] internal being a patient id we generate, external being one that we are given. At least for this ticket we are presuming that a) we are given unique patient ids, and b) they fit whatever validation scheme is configured.

@SCdF SCdF added the Type: Feature Add something new label Feb 27, 2017
@SCdF
Copy link
Contributor Author

SCdF commented Feb 27, 2017

We will need to change sentinel so it processes person documents. We should remove the document restriction, and just update all of the existing transitions to ignore documents appropriately (some may already do so by the nature of their filters)

@abbyad
Copy link
Contributor

abbyad commented Feb 27, 2017

If we were to use the same trigger, the person creation forms could be easily modified to result in a doc.form field. The resulting config could use this form value to assign the trigger to the doc:

"registrations": [
    {
      "form": "person_create",
      "events": [
        {
          "name": "on_create",
          "trigger": "add_patient",
          "params": "",
          "bool_expr": ""
        }
      ]
    },

@sglangevin sglangevin added this to the February 28th - March 14th milestone Feb 28, 2017
@SCdF SCdF self-assigned this Mar 2, 2017
SCdF added a commit to medic/medic-sentinel that referenced this issue Mar 3, 2017
This allows us to use sentinel for all normal (not deleted or ddoc)
documents.
SCdF added a commit to medic/medic-sentinel that referenced this issue Mar 3, 2017
@SCdF
Copy link
Contributor Author

SCdF commented Mar 3, 2017

Step 1: change sentinel so it passes allows transitions to transition all types of documents.

@garethbowen please review and merge medic/medic-sentinel#111 when ready.

I'm keeping this ticket open and working on it, but let's get this chunk out and done with.

garethbowen pushed a commit to medic/medic-sentinel that referenced this issue Mar 5, 2017
This allows us to use sentinel for all normal (not deleted or ddoc)
documents.

medic/cht-core#3166
@garethbowen
Copy link
Member

Nice. Merged. Assigning back to you,

@SCdF
Copy link
Contributor Author

SCdF commented Mar 6, 2017

I think adding patient_id to contacts created in Enketo is a separate transition, @abbyad would you agree? You don't actually expect any kind of registration action to be performed right? You'd just expect that if you created a patient contact in the UI a patient_id would be generated (unless you provided one).

@SCdF
Copy link
Contributor Author

SCdF commented Mar 6, 2017

Actually @abbyad would you expect this to just run on any person that was created? That would generate ids for CHPs as well, right?

SCdF added a commit to medic/medic-sentinel that referenced this issue Mar 6, 2017
@SCdF
Copy link
Contributor Author

SCdF commented Mar 7, 2017

@abbyad for the third bullet point (using Enketo -> existing external[1] patient id onto new contact.) do we need to do anything here? Can enketo forms already be configured to store something at doc.patient_id in those situations?

@abbyad
Copy link
Contributor

abbyad commented Mar 7, 2017

I think adding patient_id to contacts created in Enketo is a separate transition, @abbyad would you agree? You don't actually expect any kind of registration action to be performed right? You'd just expect that if you created a patient contact in the UI a patient_id would be generated (unless you provided one).

Correct. Although I don't see harm in it coming from the same transition add_patient, it could be a separate transition add_patient_id (but not that name to avoid confusion with previous trigger of same name).

Actually @abbyad would you expect this to just run on any person that was created? That would generate ids for CHPs as well, right?

Projects that use IDs for CHWs are more likely to use an external ID, such as their "Health Worker Registry" number. Given that, I don't think we would need to create an ID for everyone in every project. If we did we might exhaust possible numbers a bit quicker, and it would mean that app workflow only projects get an unnecessary ID for patients. That may not be a problem, and may make things more consistent that every person and place has a short form ID to refer to it.

@abbyad for the third bullet point (using Enketo -> existing external[1] patient id onto new contact.) do we need to do anything here? Can enketo forms already be configured to store something at doc.patient_id in those situations?

Yes, we can already do this. In this case we couldn't verify that it is unique, but that is likely fine.

@SCdF
Copy link
Contributor Author

SCdF commented Mar 13, 2017

Added a WIP branch to check in code I'm working through that allows support for third party ids: https://github.com/medic/medic-sentinel/tree/3166-external-id-support

SCdF added a commit to medic/medic-sentinel that referenced this issue Mar 14, 2017
* Generate patient_id on patients

New sentinel transition that adds a patient_id to each person document that
does not already contain one. This allows patients created via XForms to be used
via SMS. It will also allow CHWs to be referred to via SMS in the future if a
partner wishes to do so.

* Auto-lengthen ids generated

To support an increase in ids, and to remove a production issue where sentinel
runs out of generatable ids, we now auto-lengthen the patient ids we generate.

We attempt to create 5 randomly generated ids, and if all of them are already
used by patients we up the length of ids we're generating by 1 and try again.
This will auto-increase as they are used up to a maximum of 14, which is in the
area of a million billion ids, so I'm sure it's fine for now™.

* Drop support for id_format

Dropping support for this setting allows us to more easily support ids
that auto-lengthen when required. id_format was not used by any existing
partners, with the exception of fixing the aforementioned issue with Sentinel
running out of ids.

Tickets:
 - medic/cht-core#3166
 - medic/cht-core#3000
 - medic/cht-core#3230
@SCdF
Copy link
Contributor Author

SCdF commented Mar 15, 2017

@garethbowen can you please review the code: medic/medic-sentinel#116

I have no idea where to put the following documentation. I started writing out markdown documentation to cover things, and then realised that it's sort of half-covered by kanso.json. Things you can specifically configure are sort of covered, but also sort of not (eg a short phrase is encouraged by the format), and there isn't a way to document things that aren't specifically tied to a static piece of configuration.

Additional documentation

add_patient_id

add_patient_id event has been deprecated in favour of add_patient

add_patient

Can now be passed an externally generated patient id instead of generating one. Can be configured like so:

{
  "name": "on_create",
  "trigger": "add_patient",
  "params": {
    "patient_id": "external_id"
  }
}

The document passed would then be expected to have a patient id in the fields.external_id field.

If params.patient exists but that field cannot be found on the document a sys.no_provided_patient_id error is recorded and SMS sent.
if the given external patient_id is not unique a sys.provided_patient_id_not_unique error is recorded and SMS sent.

@abbyad
Copy link
Contributor

abbyad commented Mar 16, 2017

I don't think there is sufficient place to document this in kanso.json. We can explain "triggers", but not each one individually there. Linking to our doc from there would be useful (eg "more info on triggers"), as is being done here for contact summary.

@SCdF
Copy link
Contributor Author

SCdF commented Mar 17, 2017

Also: #3260

Just some message changes, with a note to talk to @abbyad about how we should change the default config (and others??) to support auto-extending ids.

SCdF added a commit to medic/medic-docs that referenced this issue Mar 22, 2017
SCdF added a commit that referenced this issue Mar 22, 2017
@SCdF
Copy link
Contributor Author

SCdF commented Mar 22, 2017

OK let's get this all clear, because there are lots of fragmented things flying around.

@garethbowen, can you approve (you may have seen this stuff before):

Once these are cool I'm going to merge medic/medic-sentinel#117 into medic/medic-sentinel#116, then hopefully be able to test properly with a good local environment, and then merge into master.

@garethbowen
Copy link
Member

@SCdF Nice one. One small change requested on each of the last two PRs. Merge when fixed!

@garethbowen garethbowen removed their assignment Mar 22, 2017
SCdF added a commit to medic/medic-sentinel that referenced this issue Mar 24, 2017
* Generates patient_id on patients medic/cht-core#3166

We now support external ids instead of using our own.

* Better optimisation of id generation medic/cht-core#3231

ID generation is now encapsulated in an ES6 generator, which manages state, id length and id validity compared to existing ids.

The generator caches chunks of up to 100 ids: 100 are randomly generated and then validated to not already be used by CouchDB.

Id length is increased automatically if all 100 of the ids that were randomly generated are already used in CouchDB.

NB: this implies that this generator is system-wide, and is the only tool generating ids.
SCdF added a commit that referenced this issue Mar 24, 2017
* Adding release notes
* Adding translations

Issues: #3230 and #3166
@SCdF
Copy link
Contributor Author

SCdF commented Mar 24, 2017

OK everything is merged with the exception of the documentation stuff, which Gareth and I will talk about on Monday, and then I'll almost certainly merge it then.

SCdF added a commit to medic/medic-docs that referenced this issue Mar 27, 2017
@SCdF
Copy link
Contributor Author

SCdF commented Mar 27, 2017

OK merged documentation. Donnnnnnnnnnnne.

@ngaruko
Copy link
Contributor

ngaruko commented Aug 9, 2017

Any clue on how to AT this @abbyad ?

@abbyad
Copy link
Contributor

abbyad commented Aug 9, 2017

Has this already been released?

@ngaruko
Copy link
Contributor

ngaruko commented Aug 14, 2017

Looks like it has been release @abbyad....

@SCdF
Copy link
Contributor Author

SCdF commented Aug 15, 2017

Yes this has been released, looks like 2.11.1. However, it hasn't been AT'd ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Add something new
Projects
None yet
Development

No branches or pull requests

5 participants