-
-
Notifications
You must be signed in to change notification settings - Fork 218
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
Rc/test epic integration #35125
base: master
Are you sure you want to change the base?
Rc/test epic integration #35125
Conversation
allow merging
catch up to master
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
Outdated
response = requests.post(url, data=data, headers=headers) | ||
if response.status_code == 200: | ||
return response.json().get('access_token') | ||
elif response.status_code >= 400: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens for 3xx and 4xx status codes?
corehq/motech/fhir/tasks.py
Outdated
if response.status_code == 200: | ||
response_json = response.json() | ||
fhir_id = None | ||
entry = response_json.get('entry')[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this bundle
Bundles can be paginated. You can find code for iterating resources inside paginated bundles here.
return utc_iso_format | ||
|
||
|
||
def convert_utc_timestamp_to_date_and_time(utc_timestamp): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar comment to above re TZ conversion using utilities
return date, time | ||
|
||
|
||
def sync_all_appointments_domain(domain): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know the kinds of scale we expect (# patient / appointment cases). I'm just wondering if this should be done in batches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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: |
corehq/motech/fhir/tasks.py
Outdated
@@ -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(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't actually need to use .keys()
here since dicts support key lookup by default
if response.status_code == 200: | ||
return response.json() | ||
elif response.status_code >= 400: | ||
response.raise_for_status() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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() |
return token | ||
|
||
|
||
def request_epic_access_token(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
|
||
|
||
def generate_epic_jwt(): | ||
key = settings.EPIC_PRIVATE_KEY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider using ConnectionSettings
for storing (and encrypting) API auth secrets?
base_url = "https://fhir.epic.com/interconnect-fhir-oauth/api/FHIR/R4/Patient" | ||
params = { | ||
'birthdate': birthdate, | ||
'family': family_name, | ||
'given': given_name, | ||
'_format': 'json' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks super specific, not just to Epic, but to this particular project. Shouldn't this go in custom.PROJECT_NAME
?
headers = { | ||
'authorization': f'Bearer {access_token}', | ||
} | ||
response = requests.get(url, headers=headers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
url = f'{base_url}?{urlencode(params)}' | ||
response = requests.get(url, headers=headers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a look, Norman. This has been in a holding pattern for a while, but I'm coming back to it to do some more work now. I can move this to a custom module.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Product Description
Technical Summary
This is for integrating with Epic as part of SUDCare OCS study.
Details available in this document
Feature Flag
MGH_EPIC_STUDY
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
Labels & Review