-
Notifications
You must be signed in to change notification settings - Fork 180
Fix: Conversion MedicationRequest 30-40 unmapped fields #2066
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
base: master
Are you sure you want to change the base?
Conversation
@@ -439,61 +436,4 @@ public void copyDomainResource(org.hl7.fhir.r4.model.DomainResource src, | |||
.map(Extension30_40::convertExtension) | |||
.forEach(tgt::addModifierExtension); | |||
} | |||
|
|||
public static org.hl7.fhir.r4.model.MedicationRequest convertMedicationRequest(org.hl7.fhir.dstu3.model.MedicationRequest src) throws FHIRException { |
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.
Moved to MedicationRequest30_40.java
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2066 +/- ##
============================================
+ Coverage 12.67% 12.80% +0.12%
- Complexity 35317 35697 +380
============================================
Files 2449 2454 +5
Lines 724249 725730 +1481
Branches 210848 211185 +337
============================================
+ Hits 91800 92928 +1128
+ Misses 600621 600616 -5
- Partials 31828 32186 +358 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
91bc846
to
8153274
Compare
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.
See comments. The new test is excellent, as it gives us coverage for existing code, but it should also specifically target any added conversion logic. (I realize this is additional work, and do appreciate what has been done so far).
for (org.hl7.fhir.r4.model.Identifier t : src.getIdentifier()) | ||
tgt.addIdentifier(Identifier30_40.convertIdentifier(t)); | ||
for (org.hl7.fhir.r4.model.Reference t : src.getBasedOn()) tgt.addBasedOn(Reference30_40.convertReference(t)); | ||
for (org.hl7.fhir.r4.model.CanonicalType t : src.getInstantiatesCanonical()) |
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 do not see instantiatesCanonical
added to the test anywhere. The new test is great, and very welcome, but we should check that the logic added in the PR works.
for (org.hl7.fhir.r4.model.Reference t : src.getBasedOn()) tgt.addBasedOn(Reference30_40.convertReference(t)); | ||
for (org.hl7.fhir.r4.model.CanonicalType t : src.getInstantiatesCanonical()) | ||
tgt.getDefinition().add(Reference30_40.convertCanonicalToReference(t)); | ||
for (org.hl7.fhir.r4.model.Reference t : src.getBasedOn()) |
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.
Needs test for basedOn
if (src.hasGroupIdentifier()) | ||
tgt.setGroupIdentifier(Identifier30_40.convertIdentifier(src.getGroupIdentifier())); | ||
if (src.hasStatus()) | ||
tgt.setStatusElement(convertMedicationRequestStatus(src.getStatusElement())); | ||
if (src.hasIntent()) | ||
tgt.setIntentElement(convertMedicationRequestIntent(src.getIntentElement())); | ||
if (src.hasCategory()) | ||
tgt.setCategory(CodeableConcept30_40.convertCodeableConcept(src.getCategory().get(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.
Needs test for category
@@ -62,17 +76,79 @@ public static org.hl7.fhir.dstu3.model.MedicationRequest convertMedicationReques | |||
tgt.addDetectedIssue(Reference30_40.convertReference(t)); | |||
for (org.hl7.fhir.r4.model.Reference t : src.getEventHistory()) | |||
tgt.addEventHistory(Reference30_40.convertReference(t)); | |||
|
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.
Same issues as above. If there's added R3 -> R4 functionality for a field, it should be included in the test.
@dotasek can you have another look at this PR? All your remarks should have been processed |
(Sorry for auto formatting, best to review with hide whitespace enabled)