-
Notifications
You must be signed in to change notification settings - Fork 2
Adding ability to mask fields in Patient resource #92
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
Conversation
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 PR looks really good to me! I had two small suggestions that I've made a note of.
I tested this with the Epic-MEF as well and it worked great for the most part. I do think we'll need a smaller PR over there to handle cases within the EpicCancerDiseaseStatusExtractor and EpicTreatmentPlanChangeExtractor where they could potentially try to pull the patient's name for those resources after a user has specified it to be masked. I'm not sure if that's in scope for this task, but just something to consider.
Scratch that, I actually think we could make this change within the getPatientName function in the MEF? We should definitely discuss if that function should return a Masked for privacy reasons value or something like that.
e2ea180 to
9cdcab4
Compare
Is the getPatientName function actually used anywhere in the MEF? I only see it in the patientUtils file and then in the test file. Knowing the context would definitely help with figuring out how it should act when the field is masked. |
| */ | ||
| function getPatientName(name) { | ||
| return `${name[0].given.join(' ')} ${name[0].family}`; | ||
| return ('extension' in name[0]) ? 'masked' : `${name[0].given.join(' ')} ${name[0].family}`; |
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.
Looking for opinions on what should actually be returned here when the name field is masked (this is used in EpicCancerDiseaseStatus and EpicTreatmentPlanChange Extractors for context)
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 think the string 'masked' is good! I don't think it's safe to return null or undefined since it might get applied to a required field, and I think it might be too much for right now to support returning a full dataAbsent extesion.
jafeltra
left a comment
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 really good! I also just had one small comment, but this is a really cool feature!
| */ | ||
| function getPatientName(name) { | ||
| return `${name[0].given.join(' ')} ${name[0].family}`; | ||
| return ('extension' in name[0]) ? 'masked' : `${name[0].given.join(' ')} ${name[0].family}`; |
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 think the string 'masked' is good! I don't think it's safe to return null or undefined since it might get applied to a required field, and I think it might be too much for right now to support returning a full dataAbsent extesion.
|
I think all comments have been addressed now, give it another look and let me know if anything else comes up |
jafeltra
left a comment
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 awesome!
julianxcarter
left a comment
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 great to me!
Summary
A set number of fields can now be specified to be masked with dataAbsentReason extensions in a Patient resource
New behavior
The CSV Patient Extractor and FHIR Patient Extractor can now take an optional constructor argument "mask", which is an array containing any number of fields that are capable of being masked. The specified fields will have their values replaced by dataAbsentReason extensions with the code 'masked'. The fields that can currently be masked are any values the CSV Patient Extractor can take as input (currently "gender", "mrn", "name", "address", "birthDate", "language", "ethnicity", "birthsex" and "race")
Code changes
patientUtils.jsnow has a function calledmaskPatientDatathat takes a FHIR bundle with a Patient resource and an array of fields to mask. maskPatientData will modify the bundle to have dataAbesentReason extensions where necessaryCSVPatientExtractor.jsandFHIRPatientExtractor.jsnow take an optional argument of a 'mask' array and callmaskPatientDatawith the mask array as the last step of the extraction process.patientUtils.test.jsnow contains tests formaskPatientDataTesting guidance
Example config:
For mcode-extraction-framework:
"extractors": [ { "label": "patient", "type": "CSVPatientExtractor", "constructorArgs": { "filePath": "./test/sample-client-data/patient-information.csv", "mask": ["gender","mrn","name","address","birthDate","language","ethnicity","birthsex","race"] } } ]For epic-mcode-extraction-framework:
"extractors": [ { "label": "patientExtractor", "type": "FHIRPatientExtractor", "constructorArgs": { "mask": ["gender","mrn","name","address","birthDate","language","ethnicity","birthsex","race"] } } ](In both configs
maskcontains all possible fields, feel free to try any combination)