FM-276: Update DiagnosticReport to retrieved by accession number #221

ibacher · 2019-12-05T07:27:50Z

Description of what I changed

These changes allow the FHIR module to identify DiagnosticReports by the accession number, since this is a better identifier for orders than the encounter ID

Issue I worked on

see https://issues.openmrs.org/browse/FM-276

Checklist: I completed these to help reviewers :)

My pull request only contains ONE single commit (not done to provide proper credit to Angshuman
My IDE is configured to follow the code style of this project.
I have added tests to cover my changes.
I ran mvn clean package right before creating this pull request and
added 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.

dkayiwa · 2019-12-08T07:24:57Z

@ibacher thanks for working on this. 😊
There are quite a number of changes in this pull request. Would it be possible to separate the retrieval by accession number changes from those that deal with refactoring, cleanup, and others that i see in this same pull request?

ibacher · 2019-12-08T08:22:59Z

@dkayiwa Thanks for taking the time to look this over. I've tried to separate out the different concerns.

api/src/main/java/org/openmrs/module/fhir/api/db/FHIRDAO.java

api/src/main/java/org/openmrs/module/fhir/api/impl/DiagnosticReportServiceImpl.java

dkayiwa · 2019-12-08T12:02:06Z

api/src/main/java/org/openmrs/module/fhir/api/impl/DiagnosticReportServiceImpl.java

+		List<Order> orders = dao.getOrdersByAccessionNumber(reportId);
+		int orderId = 0;
+		if ((orders != null) && !orders.isEmpty()) {
+			orderId = orders.get(0).getOrderId();


Is there a possibility of orders having more than one item?

@dkayiwa: Yes, but that's out of scope for what we're currently trying to do. At a future point, we're probably going to have to look at adding a model for DiagnosticReport directly, since multiple orders can correspond to a single accession number and multiple diagnostic reports can also correspond to a single accession number, but this is something to be solved when we get the interface to create and update accession numbers working. For right now, we're just trying to get some sensible data returned.

api/src/main/java/org/openmrs/module/fhir/api/impl/DiagnosticReportServiceImpl.java

dkayiwa · 2019-12-08T12:05:43Z

Do we leave this untouched? https://github.com/openmrs/openmrs-module-fhir/blob/master/api/src/main/java/org/openmrs/module/fhir/api/diagnosticreport/DiagnosticReportHandler.java

api/src/main/resources/moduleApplicationContext.xml

dkayiwa · 2019-12-08T12:24:41Z

Thanks @ibacher for responding to this faster than i expected. 😊
I still see quite a number of changes that are not necessary for what is required by the ticket.
They are good changes though, but make the pull request bigger and hence harder to review.
In order not to delay the merging of this asked for functionality, do you mind doing them in a separate pull request?

Adding some unit tests would also not be a bad idea. 😊

ibacher · 2019-12-09T10:20:22Z

@dkayiwa Thanks for taking the time to review this again! I think I've minimized the change surface for now.

On the unit tests, I agree they would be a good idea, but the current state is pretty tightly coupled to, e.g., Context.get...Service() and I'm primarily focusing my energy on a re-imagined version of this module that's fully unit testable 😄.

dkayiwa · 2019-12-09T10:24:11Z

@corneliouzbett do you mind reviewing this pull request? 😊

api/src/main/java/org/openmrs/module/fhir/api/db/hibernate/HibernateFHIRDAO.java

api/src/main/java/org/openmrs/module/fhir/api/impl/DiagnosticReportServiceImpl.java

…umber

ibacher force-pushed the FM-276 branch 2 times, most recently from a568d75 to 0699910 Compare December 8, 2019 08:22