FHIR Chat · docs / PR #109 Formally document hook definitions · cds hooks/committers

Stream: cds hooks/committers

Topic: docs / PR #109 Formally document hook definitions

---

Github Notifications (Oct 30 2017 at 20:43):

isaacvetter opened PR #109
from iss39-formally-document-hook-definitions to master

Fixes #75
Fixes #39

Github Notifications (Oct 30 2017 at 20:43):

isaacvetter review_requested PR #109

Github Notifications (Oct 30 2017 at 20:43):

isaacvetter review_requested PR #109

Github Notifications (Oct 30 2017 at 20:46):

isaacvetter edited PR #109
from iss39-formally-document-hook-definitions to master

Fixes #39

Github Notifications (Nov 19 2017 at 18:18):

kpshek synchronized PR #109

Github Notifications (Nov 21 2017 at 22:24):

kpshek synchronized PR #109

Github Notifications (Nov 22 2017 at 14:04):

brettmarquard commented on PR #109

I completely agree with this comment:

• each hook should have a complete example of the context object.

Github Notifications (Nov 24 2017 at 18:31):

isaacvetter synchronized PR #109

Github Notifications (Nov 24 2017 at 18:40):

isaacvetter synchronized PR #109

Github Notifications (Nov 24 2017 at 18:47):

isaacvetter synchronized PR #109

Github Notifications (Nov 24 2017 at 18:54):

isaacvetter synchronized PR #109

Github Notifications (Nov 24 2017 at 19:02):

isaacvetter commented on PR #109

@kpshek - can you figure out why Travis won't build for this PR? I thought that it was because I had outstanding reviews from you and Bryn. But even after getting rid of those, Travis returns this:

The command "mkdocs build --verbose --clean --strict" exited with 0.
Skipping a deployment with the pages provider because this branch is not permitted
Done. Your build exited with 0.

It's something to do with the travis provider configuration.

Github Notifications (Nov 24 2017 at 19:18):

isaacvetter synchronized PR #109

Github Notifications (Nov 25 2017 at 12:02):

kpshek commented on PR #109

@isaacvetter - Travis CI is building your branch just fine. Exiting with a status of 0 means that the mkdocs build command was successful. The "skipping deployment" message is expected because branches aren't deployed to the gh-pages branch. As long as you see a ✅ next to your commit hash, that means the Travis CI build succeeded.

Our review apps (that "View deployment" button) are built through Heroku, not Travis CI.

To recap:
- Travis CI builds the project via mkdocs build
- Travis CI will deploy the build to the gh-pages branch (but only if the commit is on master
- Heroku builds pull request commits and deploys them to unique review apps

Github Notifications (Nov 28 2017 at 19:33):

isaacvetter synchronized PR #109

Github Notifications (Nov 30 2017 at 00:33):

kpshek commented on PR #109

@isaacvetter - Rather than having a cardinality column, I think labeling it required with a boolean value would be more appropriate. Cardinality has implications that don't apply here. For instance, medication-prescribe has a medications-in-progress field with a cardinality of 1..*. This implies that you could have a JSON structure like:

{
   "patient": "123",
   "medications-in-progress": { // a FHIR medication },
   "medications-in-progress": { // another FHIR medication }
}

However, what we're really trying to communicate is that medications-in-progress is required and it is an array of FHIR medication objects. Here is a concrete proposal that tweaks what you have today:

Key	Required	JSON Type	Data
patient	Yes	String	FHIR Patient.id
encounter	No	String	FHIR Encounter.id
medications-in-progress	Yes	Array	DSTU2 - MedicationOrder<br />STU3 - MedicationRequest

Github Notifications (Dec 04 2017 at 17:00):

isaacvetter synchronized PR #109

Github Notifications (Dec 04 2017 at 19:17):

isaacvetter commented on PR #109

Hey @kpshek - your example of cardinality is contrary to how the FHIR spec uses the term, but I do think that splitting the two concepts is to understand.

By introducing "JSON data type", we now have boolean, number, null? Fundamentally, the relevant data types are:
* FHIR resource
* FHIR identifier
* string
* array
* object
* then less importantly: number

By specifying json, we lose the primary data types.

Github Notifications (Dec 04 2017 at 19:48):

isaacvetter synchronized PR #109

Github Notifications (Dec 11 2017 at 21:26):

isaacvetter synchronized PR #109

Github Notifications (Dec 11 2017 at 22:43):

brynrhodes commented on PR #109

I agree with the proposed change to add the JSON type. Yes, that means for resources it will be something like Array, with the actual resources identified by the Description, but at least with the JSON type we have the beginnings of something computable.

Github Notifications (Jan 27 2018 at 23:43):

kpshek synchronized PR #109

Github Notifications (Jan 28 2018 at 16:01):

kpshek synchronized PR #109

Github Notifications (Jan 28 2018 at 16:35):

mattberther synchronized PR #109

Github Notifications (Jan 28 2018 at 16:39):

kpshek commented on PR #109

In an offline discussion with @mattberther, he is going to revert 951197b as we will make that change on #153 :smile:

Github Notifications (Jan 28 2018 at 16:40):

isaacvetter commented on PR #109

Hey @kpshek - the example you use to differentiate pre-fetch and context is a poor example:

+When the context data relates to a FHIR data type, it is important not to conflate context and prefetch. For instance, imagine a hook for opening a patient's chart. This hook should include the FHIR identifier of the patient whose chart is being opened, not the full patient FHIR resource. In this case, the FHIR identifier of the patient is appropriate as CDS Services may not be interested in details about the patient resource but instead other data related to this patient. Or, a CDS Service may only need the full patient resource in certain scenarios. Therefore, including the full patient resource in context would be unnecessary. For CDS Services that want the full patient resource, they can request it to be prefetched or fetch it as need from the FHIR server.

A better example would be something like - a pgx cds service will always ask for genomic marker Observations in pre-fetch, but a zika service will never use this information -- even when the same hook is used by both services.

Isaac

Github Notifications (Jan 28 2018 at 16:41):

mattberther synchronized PR #109

Github Notifications (Jan 28 2018 at 17:10):

isaacvetter commented on PR #109

I really like the new prefetch context reference syntax! I think that it's important to call out that:

Only the first level fields in context may be considered for tokens.
Means that a hook whose context includes a full FHIR resource disallows a prefetch definition from referring to a field within that resource.

As an example, if a hook's context includes a full Patient resource, it's not possible for the prefetch definition to reference the resource identifier. This can practically be worked around by always using both the full FHIR resource and a FHIR identifier as two separate elements in the context.

Github Notifications (Jan 28 2018 at 17:29):

isaacvetter commented on PR #109

Pertaining to context hook definition, while it will be useful to additionally enable arbitrary key/value pairs, the primary use-case for context is and should be FHIR identifiers and FHIR resources.

The reason that the initial PR included both a FHIR resource name and FHIR resource name is that resource name is version-specific .... This isn't addressed in the current PR.

To recognize this, it would make sense for the documentation's example context to include a FHIR resource and/or a FHIR id.

Github Notifications (Jan 28 2018 at 17:39):

isaacvetter commented on PR #109

Fyi - I opened #155 related to context on order-review

Github Notifications (Jan 28 2018 at 20:20):

kpshek synchronized PR #109

Github Notifications (Jan 28 2018 at 20:40):

kpshek synchronized PR #109

Github Notifications (Jan 28 2018 at 20:50):

kpshek commented on PR #109

@isaacvetter - Thanks for the great suggestions! I've addressed each of your three suggestions as additional commits on this PR. Can you take a look now?

Github Notifications (Jan 28 2018 at 21:01):

kpshek commented on PR #109

These changes also are intended to address #151

Github Notifications (Jan 28 2018 at 21:14):

mattberther review_requested PR #109

Github Notifications (Jan 28 2018 at 21:35):

isaacvetter commented on PR #109

Approved!

Github Notifications (Jan 28 2018 at 23:15):

kpshek closed PR #109

---

Last updated: Apr 12 2022 at 19:14 UTC