Rc/test epic integration #35125

Robert-Costello · 2024-09-13T22:20:39Z

Product Description

Technical Summary

This is for integrating with Epic as part of SUDCare OCS study.
Details available in this document

Feature Flag

Safety Assurance

Safety story

Behind Feature Flag
Accesses specific case types, patient(host) and appointment(extension) , which limits the radius of influence.
Tested locally and on staging.

Automated test coverage

not covered by automated tests

QA Plan

no QA planned

Rollback instructions

This PR can be reverted after deploy with no further considerations

Labels & Review

Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change

allow merging

catch up to master

Robert-Costello · 2024-09-13T22:25:50Z

More refactoring needs to be done here. This is in an initial test phase using sandbox data from Open Epic.

catch up to master

corehq/motech/fhir/tasks.py

snopoke · 2024-09-17T10:56:47Z

corehq/motech/fhir/tasks.py

+    response = requests.post(url, data=data, headers=headers)
+    if response.status_code == 200:
+        return response.json().get('access_token')
+    elif response.status_code >= 400:


what happens for 3xx and 4xx status codes?

snopoke · 2024-09-17T10:58:37Z

corehq/motech/fhir/tasks.py

+    if response.status_code == 200:
+        response_json = response.json()
+        fhir_id = None
+        entry = response_json.get('entry')[0]


It would be useful to have clearer docs available for what the response format is. It's hard to tell if this is correct. It seems like you expect it to be a list with at least 1 item in it but that item might be falsy (empty list, empty dict, null etc).

This code will error if entry is not in the response or if its an empty list.

From the open epic docs entry is a required property, but may be an empty list. I added a check here 2409a2e

Name Description Is Optional Is Array

entry (Entry) An array of entries included in this bundle. false true

Name Description Is Optional Is Array
entry (Entry) An array of entries included in this bundle. false true

this bundle

Bundles can be paginated. You can find code for iterating resources inside paginated bundles here.

corehq/motech/fhir/tasks.py

snopoke · 2024-09-17T11:08:44Z

corehq/motech/fhir/tasks.py

+    return utc_iso_format
+
+
+def convert_utc_timestamp_to_date_and_time(utc_timestamp):


similar comment to above re TZ conversion using utilities

corehq/motech/fhir/tasks.py

snopoke · 2024-09-17T11:15:53Z

corehq/motech/fhir/tasks.py

+    return date, time
+
+
+def sync_all_appointments_domain(domain):


Do you know the kinds of scale we expect (# patient / appointment cases). I'm just wondering if this should be done in batches.

I did give some thought to batching. I'm not sure about the number of patients, but I'm estimating that the number of appointments per patient will be relatively low for the timeframe of 12 weeks (still need to add the time constraint to the appointment query). One appointment per week seems like the upper end to me. If you double that it's still only 24 per patient. I'll talk to Chantal and see if she thinks those numbers are low and also get a sense of the number of patients.

from Chantal:
the max number of patients enrolled in the study will be 55, with rolling enrollment over the next year
~5 patients per month and they are enrolled for 12 weeks so maybe ~15 open cases at a time in a given month

Robert-Costello · 2024-09-17T19:37:55Z

My reasoning for to this change is that when this case property is updated in CommCare it's formatted with seconds and milliseconds like so: 2016-03-10T17:00:00.000000Z
The value in Epic excludes milliseconds like so: 2016-03-10T17:00:00Z
These equate to the same appointment time, but will never match at the time of syncing appointments and so will always cause a form to be submitted to update that property.
open to other approaches

snopoke · 2024-09-18T07:12:08Z

corehq/motech/fhir/tasks.py

@@ -563,7 +562,7 @@ def sync_all_appointments_domain(domain):
        for appointment in epic_appointment_records:
            appointment_resource = appointment.get('resource')
            appointment_id = appointment_resource.get('id')
-            if appointment_id and appointment_id not in appointment_fhir_ids:
+            if appointment_id and appointment_id not in appointment_map.keys():


You don't actually need to use .keys() here since dicts support key lookup by default

kaapstorm · 2024-10-01T14:25:01Z

corehq/motech/fhir/tasks.py

+    if response.status_code == 200:
+        return response.json()
+    elif response.status_code >= 400:
+        response.raise_for_status()


The response of a POST request can often be a 201 or a 202. I'd return JSON for all success responses. You could also drop the check for error codes, because raise_for_status() won't raise an exception if there was no error.

Suggested change

if response.status_code == 200:

return response.json()

elif response.status_code >= 400:

response.raise_for_status()

if 200 <= response.status_code < 300:

return response.json()

response.raise_for_status()

kaapstorm · 2024-10-01T14:34:22Z

corehq/motech/fhir/tasks.py

+    return token
+
+
+def request_epic_access_token():


Handling authentication and authorization inside the corehq.motech.fhir.tasks module feels wrong. I think this either belongs in corehq.motech.auth, or maybe if this code is specific to only one project, it should go in custom.epic?

kaapstorm · 2024-10-01T14:36:31Z

corehq/motech/fhir/tasks.py

+
+
+def generate_epic_jwt():
+    key = settings.EPIC_PRIVATE_KEY


Did you consider using ConnectionSettings for storing (and encrypting) API auth secrets?

kaapstorm · 2024-10-01T14:40:11Z

corehq/motech/fhir/tasks.py

+    base_url = "https://fhir.epic.com/interconnect-fhir-oauth/api/FHIR/R4/Patient"
+    params = {
+        'birthdate': birthdate,
+        'family': family_name,
+        'given': given_name,
+        '_format': 'json'
+    }


This looks super specific, not just to Epic, but to this particular project. Shouldn't this go in custom.PROJECT_NAME?

kaapstorm · 2024-10-01T14:48:07Z

corehq/motech/fhir/tasks.py

+    headers = {
+        'authorization': f'Bearer {access_token}',
+    }
+    response = requests.get(url, headers=headers)


If you use the requests library directly, you won't get any remote API logging in HQ, and that will make monitoring and debugging a lot more difficult for you. Take a look at how the Requests class gets used. It will make your life much easier later.

You probably also want authentication to be handled for you, instead of handling it every time you make a request. You could subclass AuthManager for Epic, if an existing OAuth2.0 AuthManager subclass doesn't already do what you need.

kaapstorm · 2024-10-01T15:02:08Z

corehq/motech/fhir/tasks.py

+    entry_list = response_json.get('entry')
+    if entry_list and len(entry_list) > 0:
+        entry = entry_list[0]
+    else:
+        entry = None
+    if entry:
+        resource = entry.get('resource')
+        if resource:
+            fhir_id = resource.get('id')
+    return fhir_id


Using JSONPath could make this more readable, and the response that you're expecting would be more obvious.

from jsonpath_ng import parse as parse_jsonpath # ... path = parse_jsonpath('entry[0].resource.id') matches = path.find(response_json) if matches: fhir_id = matches[0].value return fhir_id

kaapstorm · 2024-10-01T15:05:07Z

corehq/motech/fhir/tasks.py

+    url = f'{base_url}?{urlencode(params)}'
+    response = requests.get(url, headers=headers)


Suggested change

url = f'{base_url}?{urlencode(params)}'

response = requests.get(url, headers=headers)

response = requests.get(base_url, params=params, headers=headers)

... but again, don't use requests directly, use the Requests class to get built-in authentication, and logging that you can audit later.

kaapstorm · 2024-10-01T15:14:06Z

corehq/toggles/__init__.py

@@ -2240,6 +2240,13 @@ def _commtrackify(domain_name, toggle_is_enabled):
    help_link="https://confluence.dimagi.com/display/GS/FHIR+API+Documentation",
 )

+MGH_EPIC_STUDY = StaticToggle(


(Note to self: Read all the code before you start to comment.)

Instead of using a feature flag, I'd advocate moving this code into custom.mgh_epic, and enable the custom module in settings.py for the project spaces you want.

kaapstorm · 2024-10-01T15:53:05Z

corehq/motech/fhir/tasks.py

+        epic_appointments_to_add = []
+        epic_appointments_to_update = []
+        # Get all appointments for patient from epic
+        epic_appointment_records = get_epic_appointments_for_patient(patient_fhir_id, access_token)


Did you look at using FHIRImportConfig (documentation)?

Would it be a lot of effort to extend it to allow syncing appointments for this project?

We might need to make some changes to allow for extension cases instead of child cases.

We would probably need to be smarter regarding how we set case ownership.

We would either need all cases to store their corresponding FHIR ID in their external_id case property, or use case search to fetch them.

We probably need to add a (custom?) serializer to handle timestamps for this project.

... and maybe it's a lot more complicated than that.

Robert-Costello added 13 commits August 22, 2024 13:41

add epic secret vars

19ba9a6

generate json web token to epic specs

30fe0d0

add method to request epic access token

234bc5c

raise exception for failed token request

25db683

add method to get appointment data for patient

01281d5

change response format to json

b764511

add FF toggle

c6be7af

Merge branch 'master' into rc/test-epic-integration

0ed677b

allow merging

use patient fhir id for appointment search

de2bac9

add time util methods

173c017

sync patient and appointment data

9dd9c75

add periodic task

f148de9

catch access token failure

9d0c563

Robert-Costello requested a review from snopoke September 13, 2024 22:20

Merge branch 'master' into rc/test-epic-integration

442e112

catch up to master

Robert-Costello added 2 commits September 16, 2024 10:37

lint

9fe01a6

add reason and practitioner properties to new appointment cases

461a163

Robert-Costello added the product/feature-flag Change will only affect users who have a specific feature flag enabled label Sep 16, 2024

Robert-Costello marked this pull request as ready for review September 16, 2024 17:02

Robert-Costello requested a review from kaapstorm as a code owner September 16, 2024 17:02

Robert-Costello added 2 commits September 16, 2024 13:02

Merge branch 'master' into rc/test-epic-integration

cd705bc

catch up to master

lint

e132919

snopoke reviewed Sep 17, 2024

View reviewed changes

Robert-Costello added 6 commits September 17, 2024 10:29

use urlencode for query params

4b57d2c

lint

4a30572

use f-string

6651007

ensure patient entry list is not empty

2409a2e

continue past patient if no fhir id

1d51e47

name formatting

92ba265

Robert-Costello added 3 commits September 17, 2024 13:22

map appointment fhir ids to cases

5a90a7f

add response handler method

7d14e11

account for secs and milli added by commcare

7955b1c

snopoke reviewed Sep 18, 2024

View reviewed changes

Robert-Costello added 6 commits September 18, 2024 09:14

no need to get keys

36a1884

ensure required params are present

ccbf366

fix param names

b7e0699

rename case property

5001b16

default properties to empty strings for case helper

829cbdc

check for correct fhir id property

1f7bf89

kaapstorm reviewed Oct 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rc/test epic integration #35125

Rc/test epic integration #35125

Robert-Costello commented Sep 13, 2024 •

edited

Loading

Robert-Costello commented Sep 13, 2024

snopoke Sep 17, 2024

snopoke Sep 17, 2024

Robert-Costello Sep 17, 2024 •

edited

Loading

kaapstorm Oct 1, 2024

snopoke Sep 17, 2024

snopoke Sep 17, 2024

Robert-Costello Sep 17, 2024

Robert-Costello Sep 17, 2024

Robert-Costello commented Sep 17, 2024 •

edited

Loading

snopoke Sep 18, 2024

kaapstorm Oct 1, 2024

kaapstorm Oct 1, 2024

kaapstorm Oct 1, 2024

kaapstorm Oct 1, 2024

kaapstorm Oct 1, 2024

kaapstorm Oct 1, 2024

kaapstorm Oct 1, 2024

kaapstorm Oct 1, 2024

kaapstorm Oct 1, 2024

		return utc_iso_format


		def convert_utc_timestamp_to_date_and_time(utc_timestamp):

		url = f'{base_url}?{urlencode(params)}'
		response = requests.get(url, headers=headers)

	url = f'{base_url}?{urlencode(params)}'
	response = requests.get(url, headers=headers)
	response = requests.get(base_url, params=params, headers=headers)

Rc/test epic integration #35125

Are you sure you want to change the base?

Rc/test epic integration #35125

Conversation

Robert-Costello commented Sep 13, 2024 • edited Loading

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Rollback instructions

Labels & Review

Robert-Costello commented Sep 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robert-Costello Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robert-Costello commented Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robert-Costello commented Sep 13, 2024 •

edited

Loading

Robert-Costello Sep 17, 2024 •

edited

Loading

Robert-Costello commented Sep 17, 2024 •

edited

Loading