Skip to content

Conversation

@julianxcarter
Copy link
Contributor

Summary

Adds support for a string value of all within the Patient Extractor constructorArgs.

New behavior

A string with a value of all can now be included in the Patient Extractor config to mask all supported Patient fields.

Code changes

  1. The mask property in config.schema.json has been updated to allow both strings and arrays.
  2. New format added to configUtils.js to validate that any string included in the mask field is the all flag.
  3. Support for the all flag added to the FHIRPatientExtractor and CSVPatientExtractor.

Testing guidance

  1. Ensure that using the all flag in the config actually masks all properties.
  2. Ensure that including any other string as a value for the mask property in the config causes config validation to fail.
  3. Ensure that the current array-based option still works as expected.

Copy link
Contributor

@jafeltra jafeltra left a 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! The only thing I thought of is if there is a test we could add to ensure that a provided 'all' value masks values, but I couldn't think of where that fit in with any of our existing tests since you reused the maskPatientData field so well!

@jafeltra
Copy link
Contributor

It looks like this just needs a rebase and then it should be good to be merged!

@julianxcarter
Copy link
Contributor Author

I spoke with @jafeltra and we both thought it made a bit more sense to localize this change to the maskPatientData function. That way we'd be able to add a unit test in the helpers test suite and also continue keeping the array of supported masking fields in a single file.

@jafeltra
Copy link
Contributor

The changes here look really good! The only thing that I think might be off is the commits. It seems like there are a lot more commits than expected after the rebase. Did you maybe rebase off of main instead of develop? That's my only idea of what's going on but something seems off.

@julianxcarter
Copy link
Contributor Author

That's exactly what I did, the git weirdness should be fixed now!

Copy link
Contributor

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I really like the refactor and masterful git work!

@jafeltra jafeltra merged commit a9e3e18 into develop Oct 12, 2021
@jafeltra jafeltra deleted the masking-all-option branch October 12, 2021 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants