Unique IDs added to several CSVs #109
Conversation
julianxcarter
requested review from
Dtphelan1,
dmendelowitz,
jafeltra and
mgramigna
as code owners
There was a problem hiding this comment.
This looks good. I had one question about an id in the procedure CSVs, which I left inline. The one other CSV I had a question about after looking through them was in the sample-client-data/staging-information.csv, which I couldn't comment on since there are no changes in it. I think it makes sense to have the conditionIds unique per patient there. I think patient 789 could have multiple staging data entries for their one cancer condition, but that condition should probably be different than patient 456's. Do you agree?
docs/procedure.csv
Outdated
| mrn,procedureId,conditionId,status,code,codeSystem,displayName,reasonCode,reasonCodeSystem,reasonDisplayName,effectiveDate,bodySite,laterality,treatmentIntent | ||
| mrn-1,example-id,example-condition-id,example-status,example-code,example-system,example-name,example-reason-code,example-system,example-reason-display,YYYY-MM-DD,example-site,example-laterality,example-treatment-intent | ||
| mrn-2,example-id,example-condition-id,example-status,example-code,example-system,example-name,example-reason-code,example-system,example-reason-display,YYYY-MM-DD,example-site,example-laterality,example-treatment-intent | ||
| mrn-1,example-id-1,example-condition-id,example-status,example-code,example-system,example-name,example-reason-code,example-system,example-reason-display,YYYY-MM-DD,example-site,example-laterality,example-treatment-intent |
There was a problem hiding this comment.
I think the conditionId is a reference to a condition, so do you think it makes sense to have those be example-condition-id-1 and example-condition-id-2?
There was a problem hiding this comment.
I think this makes sense! I was kinda on the fence about this one since I thought two procedures could in theory reference the same condition without there being problems in the bundle. In practice though, the conditionId will probably be different in most cases I'll make this change.
There was a problem hiding this comment.
Hm that's a great point. I think they'd likely be the same condition for the same patient though, and since there's two different patients in this file, maybe it makes sense to update here to be unique. But maybe it makes sense to add another entry to the sample-client-data/procedures.csv that has the same patient, different procedure id, and same condition id in order to represent multiple procedures for one condition?
There was a problem hiding this comment.
I think this makes a lot of sense! Having multiple procedures for a single condition is something I imagine happens a lot in the real world, so it'd be good to add that example in. I'll add that in another commit.
| 123,procedure-id,condition-id,completed,152198000,http://snomed.info/sct,Brachytherapy (procedure),363346000,http://snomed.info/sct,Malignant tumor,2020-01-01,41224006,51440002,373808002 | ||
| 456,procedure-id,condition-id,in-progress,174337000,http://snomed.info/sct,Destruction of lesion,363346000,http://snomed.info/sct,Malignant tumor,2020-01-12,41224006,51440002,373808002 | ||
| 789,procedure-id,condition-id,completed,172043006,http://snomed.info/sct,Total mastectomy,363346000,http://snomed.info/sct,Malignant tumor,2020-06-30,41224006,51440002,373808002 | ||
| 123,procedure-id-1,condition-id,completed,152198000,http://snomed.info/sct,Brachytherapy (procedure),363346000,http://snomed.info/sct,Malignant tumor,2020-01-01,41224006,51440002,373808002 |
There was a problem hiding this comment.
If you agree with the other comment above, it probably makes sense to update here too!
I also agree that the |
dmendelowitz
left a comment
dmendelowitz
left a comment
There was a problem hiding this comment.
This looks good to me!
julianxcarter
force-pushed
the
cleaning-up-sample-data
branch
from
a0f610b to
dd3ca17
Compare
Summary
This PR changes some of the example CSVs in the
docsandsample-client-datadirectory to have values more representative of what the CSVModule is expecting.New behavior
No new behavior.
Code changes
test/sample-client-data, and also to the adverse event, procedure, and observation CSVs within thedocsfolder.Testing guidance
Ensure that I'm not missing anything that could potentially mislead users as they're creating their own CSVs. The only thing I saw that I thought required changing were some of the
idfields not being unique when they should be in the context of FHIR, but I could be missing something.