-
Notifications
You must be signed in to change notification settings - Fork 110
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
FM2-312: Map the order number as an identifier for MedicationRequest and ServiceRequest #306
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #306 +/- ##
============================================
- Coverage 84.22% 84.19% -0.03%
- Complexity 2084 2095 +11
============================================
Files 188 188
Lines 5305 5325 +20
Branches 602 604 +2
============================================
+ Hits 4468 4483 +15
- Misses 502 504 +2
- Partials 335 338 +3
Continue to review full report at Codecov.
|
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.
@Ruhanga Thanks for this! It looks mostly good and I'm sorry it's taken me a bit to get back to you. In addition to the below suggestions, I would look into adding a BaseOrderDao
class that both FhirServiceRequestDaoImpl
and FhirMedicationRequestDaoImpl
can inherit from so we only need one definition of the the handleOrderNumber()
logic (and can add other common logic for orders as needed).
@@ -25,6 +25,11 @@ | |||
@Override | |||
MedicationRequest get(@Nonnull String uuid); | |||
|
|||
IBundleProvider searchForMedicationRequests(ReferenceAndListParam patientReference, |
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.
Ideally, we only have on search
method per service. I would just extend the existing method to also support identifier
.
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.
Could there not be some code already depending on the current method signature elsewhere? I was worried of the backwards compatibility by introducing another search
.
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.
No. This is a very closed set so far and, in essence, no one outside of the FHIR2 module should be depending on this (at least yet).
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.
Ohh, ok. Thank you for clarifying this.
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.
However, there are very many tests that are so tightly coupled to the method signature scattered all over the project. I see a huge refactor if I refactored the method(for example), could I go ahead @ibacher? Thanks.
api/src/main/java/org/openmrs/module/fhir2/api/dao/impl/FhirMedicationRequestDaoImpl.java
Show resolved
Hide resolved
api/src/main/java/org/openmrs/module/fhir2/api/dao/impl/FhirServiceRequestDaoImpl.java
Show resolved
Hide resolved
@@ -60,6 +61,9 @@ | |||
@Autowired | |||
private DosageTranslator dosageTranslator; | |||
|
|||
@Autowired | |||
private OrderIdentifierTranslator orderIdentifierTranslator; |
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.
We should also have a mapping for this in toOpenmrsType()
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'll let the method throw an exceptions at the moment since an order-number is read only and that it is of a string type.
|
||
orderIdentifier.setValue(order.getOrderNumber()); | ||
|
||
orderIdentifier.setUse(Identifier.IdentifierUse.OFFICIAL); |
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 think this should be left as USUAL
.
@Component | ||
public class OrderIdentifierTranslatorImpl implements OrderIdentifierTranslator { | ||
|
||
private static String ORDER_ID_TYPE = "Order Number"; |
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 tend to prefer that constants like this go in the FhirConstants
class.
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.
Ok.
|
||
Identifier orderIdentifier = new Identifier(); | ||
|
||
orderIdentifier.setType(new CodeableConcept().setText(ORDER_ID_TYPE)); |
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.
Technically, I think this should be the PLAC
type:
Coding placCoding = new Coding().setSystem("http://terminology.hl7.org/CodeSystem/v2-0203").setCode("PLAC").setDisplay("Placer Identifier");
orderIdentifier.setType(new CodeableConcept().addCoding(placCoding));
We can even define the CodeableConcept
as a constant.
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.
Resolved.
import org.hl7.fhir.r4.model.Identifier; | ||
import org.openmrs.Order; | ||
|
||
public interface OrderIdentifierTranslator { |
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.
This should really extend ToFhirTranslator<Order>
or, better yet:
public interface OrderIdentifierTranslator<T extends Order> extends OpenmrsFhirTranslator<T, Identifier>
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'd go with:
public interface OrderIdentifierTranslator extends OpenmrsFhirTranslator<Order, Identifier>
…and ServiceRequest
…and ServiceRequest
…and ServiceRequest
…and ServiceRequest
… parameter + fixing failing tests
Description of what I changed
Issue I worked on
see https://issues.openmrs.org/browse/FM2-312
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
I 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 package
right 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.