FM2-675: Enhancing FHIR2 Diagnostic Report #583
Conversation
- added column - order_id (ref FK to order.order_id) - added column - conclusion - added status enums - amended, cancelled API - updated translator to map order ref - introduced new ServiceRequestReferenceTranslator that looks up the order - added tests
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #583 +/- ##
============================================
+ Coverage 77.84% 78.72% +0.87%
+ Complexity 2683 2626 -57
============================================
Files 239 249 +10
Lines 7452 8167 +715
Branches 901 1079 +178
============================================
+ Hits 5801 6429 +628
- Misses 1115 1130 +15
- Partials 536 608 +72 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ibacher
left a comment
ibacher
left a comment
There was a problem hiding this comment.
Thanks @angshu!
I'm not sure about naming of things here. This adds stuff about GenericServiceRequest, but in FHIR terms, the objects its operating on are all Requests (not every type of FHIR request, but a MedicationRequest has no relationship to a "ServiceRequest" in FHIR terminology). They are both really about "Orders" in OpenMRS terms, and should probably be configured that way.
SImilarly, "ServiceRequestReferenceTranslator" is really an "OrderReferenceTranslator". It would also be better if this "OrderReferenceTranslator" deferred to the existing "MedicationRequestReferenceTranslator" for MedicationRequests. There are too many interfaces in this module, but that's down to want to provide a high degree of overridability via implementations providing their own definitions of those, so it's best if we keep things defined in a single place.
Finally, it would be good if the tests for the translator better covered the set of edge-cases its meant to, mostly to ensure that it's functioning correctly (I think there's a subtle bug in the toOpenmrsType() implementation.
| Optional.ofNullable(fhirDiagnosticReport.getConclusion()).ifPresent(value -> diagnosticReport.setConclusion(value)); | ||
| Optional.ofNullable(fhirDiagnosticReport.getOrder()).ifPresent((value -> { | ||
| diagnosticReport.addBasedOn(serviceRequestReferenceTranslator.toFhirResource(value)); | ||
| })); |
There was a problem hiding this comment.
Using Optional in this way feels unidiomatic to me.
There was a problem hiding this comment.
first one, will change. second one, wanted to guard against the serviceRequestTranslator not handling null. Yeah, the "ServiceRequestReferenceTranslatorImpl" does do a null check, but ToFhirTranslator.toFhirResource() really expects a notnull parameter.
| if (order instanceof DrugOrder) { | ||
| return ReferenceHandlingTranslator.createDrugOrderReference((DrugOrder) order); | ||
| } else { | ||
| return new Reference().setReference(FhirConstants.SERVICE_REQUEST + "/" + order.getUuid()) |
There was a problem hiding this comment.
It probably would've been more consistent to just use ReferenceHandlingTranslator.createOrderReference() and modify that as necessary.
There was a problem hiding this comment.
thought about it. Didn't want to disturb that. Will change and update
There was a problem hiding this comment.
there are 2 test cases which break and I am not sure if they should be asserted/tested the way they are written currently.
ObservationBasedOnReferenceTranslatorImplTest.toFhirType_shouldReturnNullForUnknownOrderType()
translator.toFhirResource(order)- does not check against the Order.OrderType - but rather checks against the java type of the order (Order, TestOrder, DrugOrder etc). So this test case is redundant really, if I change the implementation atReferenceHandlingTranslator
ObservationBasedOnReferenceTranslatorImplTest.toFhirResource_shouldReturnNullIfOrderTypeIsNull()
- same case here as well.
If the check against order.orderType is really what was expected, then the implementation in ObservationBasedOnReferenceTranslatorImpl should really override and check against the order.orderType
Let me know what you want me to do. I can remove or ignore the tests.
There was a problem hiding this comment.
Also not sure if - ReferenceHandlingTranslatorTest.shouldReturnNullForUnknownOrderSubclass() - was introduced intended to only handle Order as being Lab Orders (e.g path/hema/micro labs) or Medication Requests? We model orders of different types - surgical procedures, Physiological evaluations ...
If we put a check that order.orderType must be specified - that would break other test cases, but if thats never likely to happen - I can put that condition and try to fix the tests
whereas - ReferenceHandlingTranslatorTest.shouldReturnNullForRawOrder() - seems to guard against base Order
There was a problem hiding this comment.
I think this was more a case of ensuring that we weren't accidentally translating order type (classes) that we didn't explicitly support since. Basically, anything descended from, e.g., TestOrder has, at a minimum, all of those fields that we mapped in ServiceRequest, but some other random descendant of Order may not. I don't know that keeping them is essential.
| Optional<String> referenceType = getReferenceType(reference); | ||
| if (referenceType | ||
| .map(ref -> !(ref.equals(FhirConstants.SERVICE_REQUEST) || ref.equals(FhirConstants.MEDICATION_REQUEST))) | ||
| .orElse(false)) { |
There was a problem hiding this comment.
Shouldn't this be:
| .orElse(false)) { | |
| .orElse(true)) { |
api/src/main/resources/liquibase.xml
Outdated
| </not> | ||
| </preConditions> | ||
| <addColumn tableName="fhir_diagnostic_report"> | ||
| <column name="conclusion" type="VARCHAR(512)"/> |
There was a problem hiding this comment.
Wouldn't it be somewhat better to just store this as a CLOB? Then we don't need to worry so much about the conclusion size.
There was a problem hiding this comment.
Allocating too much VARCHAR /TEXT/CLOB for a column can lead to inefficient storage and a DB Server may overestimate the memory required for queries, potentially leading to excessive memory allocation. It's generally better to allocate a size that closely matches the expected data length to optimize performance and resource usage. We can always expand the size if required later. Would 1024 be enough?
There was a problem hiding this comment.
Yeah, 1024 seems better. 512 characters requires things to be pretty terse.
| @JoinColumn(name = "order_id", referencedColumnName = "order_id") | ||
| private Order order; | ||
|
|
||
| @Column(name = "conclusion") |
There was a problem hiding this comment.
length defaults to 255, so if we're using 512, we should specify that here.
There was a problem hiding this comment.
I deliberately didn't mention the size in case we want to change the db column size later. I can add - see related comment, if you would want little more capacity (e.g. 1024)
There was a problem hiding this comment.
I suppose we don't really use the Hibernate metadata for anything, so it's probably fine.
There was a problem hiding this comment.
I thought we were using it to validate field lengths: https://github.com/openmrs/openmrs-core/blob/2.8.2/api/src/main/java/org/openmrs/api/db/hibernate/HibernateAdministrationDAO.java#L222-L238
👍 I wasn't happy with the naming and the way I had to implement. Also, I didn't want to change a lot of things there. Unfortunately, existing code doesn't make it easy to reuse code.
if we were to truly support the ideas here and model through ServiceRequest and MedicationRequest, then we really need to bring in flexibility of allowing implementers to map through OrderType and delegate to their own translations and mappings. if we would to do this, it would require fair amount of refactoring. I can give it a shot, but it may take sometime due to my availability. Let me know your thoughts. |
|
I will, for now, just renamed the classes. However, re-using and changing the code in ReferenceHandlingTranslator will mean that any Order as a fallback will be a ServiceRequest reference, which for many wouldn't work - as the FhirServiceRequestServiceImpl will only understand TestOrder. I am inclined to not use ReferenceHandlingTranslator for now, and live with the duplicates for now. When fhir2 moves over to core 2.5, (currently 2.4.1), we probably can refactor to check against ReferralOrder, TestOrder, ServiceOrder, and Order hierarchy. Or maybe refactor the FhirServiceRequestServiceImpl to make it more generic (by using Order.OrderType) instead by providing extensions/hooks for implementations to override behavior. |
|
I have pushed the code. Please check. Note, the test cases ignored in
This is due to reuse of the Let me know if you prefer this way, or just resolve the reference in the |
This is sort of intentional, i.e., the only Order type core knows about is |
| private Set<Obs> results = new HashSet<>(); | ||
|
|
||
| @OneToOne | ||
| @JoinColumn(name = "order_id", referencedColumnName = "order_id") |
There was a problem hiding this comment.
Do we have a convention for the since annotation when dealing with new fields taken care of by lombok? https://openmrs.atlassian.net/wiki/spaces/docs/pages/25477044/Java+Conventions#New-Public-Methods-and-Classes
There was a problem hiding this comment.
done. marked since 2.8.1, assuming you are ok to make a point release
| package org.openmrs.module.fhir2.api.dao.impl; | ||
|
|
||
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.hamcrest.Matchers.*; |
There was a problem hiding this comment.
| assertThat(reference, notNullValue()); | ||
| } | ||
|
|
||
| @Ignore("this test case is no longer valid, as all other order subclasses now return a reference to ServiceRequest") |
| assertThat(result.getType(), equalTo(FhirConstants.MEDICATION_REQUEST)); | ||
| } | ||
|
|
||
| @Ignore("this test case is not valid as the underlying implementation does not check against order.orderType") |
There was a problem hiding this comment.
Was this invalidated by the changes you introduced in this pull request? Or was it invalid even before?
There was a problem hiding this comment.
For the title of the test, I can only infer the intention and see that the implementation was not asserting against the order.orderType, but rather based on the class of the order - whether TestOrder or DrugOrder; otherwise returning null. As I changed the implementation to always return a ServiceRequest reference for an order, this has started failing.
I do not know whether this was done to guard against a specific case. But just going by the test name, imo, the test is not right.
| assertThat(result, nullValue()); | ||
| } | ||
|
|
||
| @Ignore("this test case is not valid as the underlying implementation does not check against order.orderType") |
There was a problem hiding this comment.
yeah, same as above. for "toFhirType_shouldReturnNullForUnknownOrderType()", ObservationBasedOnReferenceTranslatorImpl was never checking the orderType. If I have to do the right one, I have to inject "OrderService" in ObservationBasedOnReferenceTranslatorImpl, and check whether the orderType is a valid one. And in this case, I am not sure whether that should be the responsibility of this translator at all. This translator's job is to return a service request reference for observation - and imo, it should not go about checking the order type for the related order.
Do you want to keep it so? Imho, Openmrs core from the begining, very much allowed modelling other order types (e.g. surgical procedure, counselling requests can be modelled as Fhir ServiceRequest) through its base order model and most of the time thats enough. An order also can have order attribute. An implementation can always extend the order model through the additional attributes - and write their own translators as they deem fit, as you mentioned. ServiceRequestTranslatorImpl - actually does not need TestOrder - it does not use any field like laterality, specimen_source, location, frequency or number_of_repeats! So it could have just used "Order" (and just differentiation of DrugOrder). Anyway, in this card context - we are only talking about reference to the order - which imo, does not need anything other than an Order reference. |
|
@ibacher @dkayiwa We want to change the relationship of Diagnostic report to service request to one to many, and therefore introduced a mapping table. Initially, we were thinking that a single reference to an order is enough, but in reality in many contexts multiple service requests maybe reported using a single report. e.g. Radiology tests (radiologist/physician asking for additional views, and multiple pathology test orders maybe reported using a single panel report. This model is supported in FHIR, and seems a real one. Please provide your thoughts and ideas, and whether you forsee any issues with it. I can update this PR anytime. Also added more tests. btw, let me know if you have additional comments on the Order reference resolution above, and the effected tests that I mentioned. If you are unsure, I propose that OrderReferenceTranslator.java does not reuse code from ReferenceHandlingTranslator. It would be a minor code duplication, but we can resolve that in future when we think of the larger order type mapping. If the contention is only about resolving reference to a service request, I think that would be prudent (at least for now) |
…able). Added integration tests.
3 participants
API
see https://issues.openmrs.org/browse/FM2-675
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
I ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
All new and existing tests passed.
My pull request is based on the latest changes of the master branch.