Skip to content

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Corne
Copy link
Contributor

@Corne Corne commented Jun 27, 2025

  • Support mapping src.instantiatesCanonical <-> tgt.definition;
  • Support mapping src.basedOn <-> tgt.basedOn
  • Support mapping src.category <-> tgt.category;
  • Support mapping of requester with behalfof extension v4 to v3
  • Moved the medication request conversion from v3 to v4 to the corresponding file.
  • Added tests for MedicationRequest mapping

(Sorry for auto formatting, best to review with hide whitespace enabled)

@@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

codecov bot commented Jun 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 12.80%. Comparing base (98ea1d2) to head (5860625).
Report is 72 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Corne Corne force-pushed the MedicationRequestConversion3040 branch from 91bc846 to 8153274 Compare July 2, 2025 16:08
Copy link
Collaborator

@dotasek dotasek left a 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())
Copy link
Collaborator

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())
Copy link
Collaborator

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)));
Copy link
Collaborator

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));

Copy link
Collaborator

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.

@Corne Corne requested a review from dotasek July 4, 2025 20:41
@Corne
Copy link
Contributor Author

Corne commented Jul 15, 2025

@dotasek can you have another look at this PR? All your remarks should have been processed

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.

2 participants