EA-207: Implement an API for Fetching Visits with Notes and Diagnosis #250

jayasanka-sack · 2024-10-24T13:57:14Z

Ticket: https://openmrs.atlassian.net/browse/EA-207

We needed to create a new API that retrieves visits along with their associated notes and diagnosis to optimize the current approach. The existing API call we are using to fetch past visits is expensive and returns more data than necessary for the initial UI rendering. The goal is to retrieve only the relevant information (visits, notes, diagnosis) to improve performance and load times.

mogoodrich · 2024-10-24T14:33:20Z

@jayasanka-sack can you write up an EMR-API ticket in Jira describing the use cases on what this PR is intended to accomplish? thanks, fyi @mseaton

jayasanka-sack · 2024-10-24T14:39:24Z

Hey @mogoodrich The ticket is: https://openmrs.atlassian.net/browse/EA-207

This PR is still under WIP. I'll let you know when it is ready for review.

mogoodrich

Thanks @jayasanka-sack ! Good to see we are adding methods to the EMR API to support performant queries in O3.

See my comments and let me know if you have questions... also, can you add a link to this PR to the ticket?

Also, it looks like this is really just returning "Visits With Diagnoses" (not "Visits with Notes and Diagnsoes")... which is fine, just wanted to confirm it is what you were looking for.

also fyi @mseaton

mogoodrich · 2024-12-02T16:36:32Z

api/src/main/java/org/openmrs/module/emrapi/db/HibernateVisitDAO.java

+
+    public List<VisitWithDiagnoses> getVisitsByPatientId(Patient patient, int startIndex, int limit) {
+
+        String visitNoteEncounterTypeUuid = "d7151f82-c1f3-4152-a605-2f9ea7414a79";


Instead of hardcoding the uuid here, we should be fetching this based on the encounter that has the "emr.visitNoteEncounterType" metadata mapping, see:

https://github.com/openmrs/openmrs-module-emrapi/blob/master/api/src/main/java/org/openmrs/module/emrapi/EmrApiProperties.java#L93

And actually, why are we including this subquery at all? Do we only want to include visits that have visit notes (this seems wrong)?

We’re including this to get only note encounters with the visit, but we also need visits that don’t have encounters. I’ve updated the query with a left join and modified the test accordingly.

api/src/main/java/org/openmrs/module/emrapi/visit/VisitWithDiagnoses.java

mogoodrich · 2024-12-02T16:45:27Z

api/src/main/java/org/openmrs/module/emrapi/db/HibernateVisitDAO.java

+    @Autowired
+    private SessionFactory sessionFactory;
+
+    public List<VisitWithDiagnoses> getVisitsByPatientId(Patient patient, int startIndex, int limit) {


let's call this getVisitsWithDiagnosesByPatient to be clear what it returns. Also note that we don't/shouldn't have the "Id" at the end, because this takes a Patient, not a patient Id.

mogoodrich · 2024-12-02T16:46:38Z

api/src/main/java/org/openmrs/module/emrapi/visit/VisitWithDiagnosesService.java

+     * @param patientUuid the UUID of the patient
+     * @return a list of visits that has note encounters and diagnoses
+     */
+    List<VisitWithDiagnoses> getVisitsByPatientId(String patientUuid, int startIndex, int limit);


Same comment here, let's make this "getVisitsWithDiagnosesByPatient" (add "WithDiagnoses" and remove "Id")

mogoodrich · 2024-12-02T16:48:57Z

api/src/main/java/org/openmrs/module/emrapi/visit/VisitWithDiagnosesServiceImpl.java

+    VisitDAO visitDAO;
+
+    @Override
+    public List<VisitWithDiagnoses> getVisitsByPatientId(String patientUuid, int startIndex, int limit) {


let's have this take a Patient patient, not a String patientUuid (see my further comments below)

mogoodrich · 2024-12-02T16:50:13Z

omod/pom.xml

+            <artifactId>junit-jupiter</artifactId>
+            <version>RELEASE</version>
+            <scope>test</scope>
+        </dependency>


I'm not necessarily totally opposed to this, but what Junit functionality is this added that we don't already have available in this module?

Jupiter is JUnit 5's base engine, so I expect this is to be able to write tests in JUnit 5?

We either should choose to use Junit 4 or Junit 5 for the entire module. I don't want us to have to try to support both in one module, there are too many annoying gotchas.

Yes, if it's problematic to support both JUnit 4 and JUnit 5 in a single module, let's just go with JUnit 4 until we feel motivated to update all the tests.

mogoodrich · 2024-12-02T16:56:00Z

omod/src/main/java/org/openmrs/module/emrapi/web/controller/VisitController.java

+    public ResponseEntity<?> getVisitsByPatientId(
+            HttpServletRequest request,
+            HttpServletResponse response,
+            @PathVariable String patientUuid) {


Ah, so normally we do something like this:

@RequestParam("patient") Patient patient

...and there is a converter behind the scenes that handles the conversion from uuid -> Patient (and throws an Exception if no patient found). Did you/can you test if this "automagically" works:

@PathVariable Patient patientUuid

(Then the underlying service method can just be getVisitsByPatient(Patient, int, int)

I’m guessing it needs to be called with ?patient={uuid} but it’s returning a 500 with no body, and I’m having a hard time debugging it. Can you point me to where the conversion happens? I noticed a couple of other endpoints which use the same, but I couldn’t find any tests for them or any part of the frontend that calls them. I tried @PathVariable Patient patientUuid as well.

Unfortunately, it's part of the UI framework, which is almost certainly not the right place for it. I think you could also do @PathVariable("patientUuid") Patient patient with the right converter.

I’m still stuck on this part. I tried using both the path variable and the request parameter, but it still throws a 500 error without any message. Any ideas on how I can debug this?

By the way, this code has been used here:

openmrs-module-emrapi/omod/src/main/java/org/openmrs/module/emrapi/web/controller/ActiveVisitController.java

Line 33 in 7cabb05

public SimpleObject ensureActiveVisit(@RequestParam("patient") Patient patient,

Did you try adding the UIFramework module? The problem is that without the converter, Spring doesn't know how to change the value it gets (a String) into the thing you want (a Patient object).

No real objection, but also no need to get bogged down on this. Just accept a String parameter (which could be a uuid or primary key id), and convert it to a patient in the controller...

Huh, so things like the ActiveVisitController have been dependent on UI Framework module all along, we just never realized it because we've generally always been running EMR API and UI Framework together?

No strong objection to move them... I think I'd have slight preference for moving them to EMR API instead of REST Web Services, but haven't thought it all through. We also could potentially add them to Core? (Though of course the adoption time would be slower that way).

Also, it looks like REST WS may have it's own converters, fwiw...

REST has it's own Converter concept, but it's actually distinct from the Spring converters, I think.

We also could potentially add them to Core? (Though of course the adoption time would be slower that way)

Yeah, I'd like to migrate them to core eventually, but we just released 2.7.0, so it's not the best time to do that. Easy enough to add to a module and then add an @OpenmrsProfile to exclude them once they're available in core.

mogoodrich · 2024-12-02T16:56:38Z

omod/src/main/java/org/openmrs/module/emrapi/web/controller/VisitController.java

+    VisitWithDiagnosesService visitWithDiagnosesService;
+
+    @RequestMapping(method = RequestMethod.GET, value = "/rest/**/emrapi/patient/{patientUuid}/visit")
+    public ResponseEntity<?> getVisitsByPatientId(


let's make this "getVisitsWithDiagnosesByPatient"

And the endpoint should not be /.../visit, but should reflect that it is a /.../visitWithDiagnoses or something

mogoodrich · 2024-12-02T16:58:42Z

api/src/test/resources/metadata/pastVisitSetup.xml

+<!--  <metadatamapping_metadata_term_mapping metadata_term_mapping_id="3945" metadata_source_id="2134"-->
+<!--                                         creator="1" date_created="2023-10-20 12:38:58.0" uuid="111bbad0-d565-11e3-9c1a-0800200c9a665"-->
+<!--                                         retired="false" code="emr.visitNoteEncounterType" name="emr.visitNoteEncounterType"-->
+<!--                                         metadata_uuid="06087e80-d565-11e3-9c1a-0800200c9a66" metadata_class="org.openmrs.EncounterType"/>-->


If this is commented out, let's just not include it?

mogoodrich · 2024-12-02T17:00:17Z

api/src/test/resources/metadata/pastVisitSetup.xml

@@ -0,0 +1,35 @@
+<?xml version='1.0' encoding='UTF-8'?>


Is this class being used, or is everything handled in the other "pastVisitSetup" class below?

Both files are used in tests in both modules.

What is the distinction between them?

Yeah the organization is a little weird. It looks like there may have been some convention for all the metadata to go into a file like this and the data into a file like the other one, but both of them mix data and metadata, so it's confusing why we need the two.

mogoodrich · 2024-12-02T17:07:14Z

api/src/main/java/org/openmrs/module/emrapi/db/VisitDAO.java

+
+import java.util.List;
+
+public interface VisitDAO {


I think we may want to call with EmrApiVisitDAO to distinguish it from the Core one? Thoughts @mseaton @ibacher ?

Do we really need an entirely new DAO class we have to wire up and maintain? Can't we just move to having one DAO for the entire module?

Yeah, I'm fine with using the existing "EmrApiDAO" for this... there are some other DAOs already in the EMR API module with obsolete names ("EmrEncounterDAO"), but we could move away from these in favor of a single DAO.

mogoodrich · 2024-12-02T17:08:05Z

api/src/main/java/org/openmrs/module/emrapi/visit/VisitWithDiagnosesService.java

+
+import java.util.List;
+
+public interface VisitWithDiagnosesService {


Maybe call this EmrApiVisitService (and we can use it for other Visit-related services we add to EMR API)? Thoughts @mseaton @ibacher ?

Yeah, that's a very good idea!

Yes! Let's move towards having fewer distinct DAOs and Services in this module

mseaton · 2024-12-02T17:23:10Z

api/src/main/java/org/openmrs/module/emrapi/db/HibernateVisitDAO.java

+import java.util.Set;
+import java.util.stream.Collectors;
+
+@Repository


I have never seen this annotation actually used in this module before. Are we sure this works? I'd prefer we stick with existing conventions within the module for wiring things up, and then do a change to everything if we want to update to a new mechanism.

mseaton · 2024-12-02T17:24:01Z

api/src/main/java/org/openmrs/module/emrapi/db/HibernateVisitDAO.java

+@Repository
+public class HibernateVisitDAO implements VisitDAO {
+
+    @Autowired


Usually we do not autowire into a DAO AFAIK, but we use xml. There are definitely performance issues to using Autowired in services, and I'm not sure if those carry over to DAOs.

Nice catch and 100% agree... it's annoying, but we need to manually wire services and daos using xml... there should be examples in this module and Core.

They likely don't, but I'd still favour XML over autowiring.

mseaton · 2024-12-02T17:27:34Z

api/src/main/java/org/openmrs/module/emrapi/db/VisitDAO.java

+
+import java.util.List;
+
+public interface VisitDAO {


Do we really need an entirely new DAO class we have to wire up and maintain? Can't we just move to having one DAO for the entire module?

mseaton · 2024-12-02T17:28:52Z

api/src/main/java/org/openmrs/module/emrapi/visit/VisitWithDiagnosesService.java

+
+import java.util.List;
+
+public interface VisitWithDiagnosesService {


Yes! Let's move towards having fewer distinct DAOs and Services in this module

mseaton · 2024-12-02T17:29:51Z

omod/pom.xml

+            <artifactId>junit-jupiter</artifactId>
+            <version>RELEASE</version>
+            <scope>test</scope>
+        </dependency>


We either should choose to use Junit 4 or Junit 5 for the entire module. I don't want us to have to try to support both in one module, there are too many annoying gotchas.

mseaton · 2024-12-02T17:30:53Z

omod/src/main/java/org/openmrs/module/emrapi/web/controller/VisitController.java

+    VisitWithDiagnosesService visitWithDiagnosesService;
+
+    @RequestMapping(method = RequestMethod.GET, value = "/rest/**/emrapi/patient/{patientUuid}/visit")
+    public ResponseEntity<?> getVisitsByPatientId(


And the endpoint should not be /.../visit, but should reflect that it is a /.../visitWithDiagnoses or something

jayasanka-sack · 2024-12-09T05:45:47Z

Appreciate the feedback @mogoodrich and @mseaton! I’ll take a look and follow up with you.

jayasanka-sack · 2024-12-09T11:14:23Z

I still have to address some comments. I'll get them fixed and get back to you.

mogoodrich

Sorry, juts realized I had a couple "pending" comments that I never submitted.

mogoodrich · 2024-12-02T18:39:28Z

api/src/main/java/org/openmrs/module/emrapi/db/HibernateVisitDAO.java

+@Repository
+public class HibernateVisitDAO implements VisitDAO {
+
+    @Autowired


Nice catch and 100% agree... it's annoying, but we need to manually wire services and daos using xml... there should be examples in this module and Core.

mogoodrich · 2024-12-02T18:42:38Z

api/src/main/java/org/openmrs/module/emrapi/db/VisitDAO.java

+
+import java.util.List;
+
+public interface VisitDAO {


Yeah, I'm fine with using the existing "EmrApiDAO" for this... there are some other DAOs already in the EMR API module with obsolete names ("EmrEncounterDAO"), but we could move away from these in favor of a single DAO.

mogoodrich

Thanks @jayasanka-sack , getting close! See my updated thoughts/questions.

mogoodrich · 2024-12-17T15:42:16Z

api/src/main/java/org/openmrs/module/emrapi/db/EmrApiDAOImpl.java

+      String visitNoteEncounterTypeUuid = emrApiProperties.getVisitNoteEncounterType().getUuid();
+
+      String hqlVisit="SELECT DISTINCT v FROM Visit v " +
+              "LEFT JOIN FETCH v.encounters enc " +


Ah, I wasn't aware of this FETCH syntax.... this is saying when fetching the encounters associated with the visit, limit to those which are notes? ie, although this query only returns visits, you can fetch the encounters via visit.encounters, and those will be limited to notes? @jayasanka-sack

Yes, you’re right!

P.S.
I used the FETCH syntax to load encounters eagerly instead of lazily.

mogoodrich · 2024-12-17T17:58:24Z

api/src/test/resources/metadata/pastVisitSetup.xml

@@ -0,0 +1,35 @@
+<?xml version='1.0' encoding='UTF-8'?>


What is the distinction between them?

Jayasanka Weerasinghe and others added 4 commits October 23, 2024 14:39

Implement an API for Fetching Visits with Notes and Diagnosis

876bdee

Handle invalid patient uuids

efa8cb7

Minor updates

539eb15

Add pagination

612175d

jayasanka-sack changed the title ~~Implement an API for Fetching Visits with Notes and Diagnosis~~ WIP: [Do not review] Implement an API for Fetching Visits with Notes and Diagnosis Oct 24, 2024

jayasanka-sack changed the title ~~WIP: [Do not review] Implement an API for Fetching Visits with Notes and Diagnosis~~ WIP: [Do not review] EA-207: Implement an API for Fetching Visits with Notes and Diagnosis Oct 24, 2024

jayasanka-sack added 2 commits October 25, 2024 16:33

Convert the response to a simple object

0828b67

Format the code

2ede16b

jayasanka-sack changed the title ~~WIP: [Do not review] EA-207: Implement an API for Fetching Visits with Notes and Diagnosis~~ WIP: EA-207: Implement an API for Fetching Visits with Notes and Diagnosis Nov 21, 2024

jayasanka-sack requested a review from ibacher November 26, 2024 13:39

Fix tests

a1dd8a0

jayasanka-sack changed the title ~~WIP: EA-207: Implement an API for Fetching Visits with Notes and Diagnosis~~ EA-207: Implement an API for Fetching Visits with Notes and Diagnosis Nov 28, 2024

jayasanka-sack marked this pull request as ready for review November 28, 2024 06:06

jayasanka-sack requested review from mogoodrich and mseaton November 28, 2024 06:06

mogoodrich requested changes Dec 2, 2024

View reviewed changes

mogoodrich reviewed Dec 2, 2024

View reviewed changes

mseaton requested changes Dec 2, 2024

View reviewed changes

jayasanka-sack added 2 commits December 9, 2024 18:51

Address PR reviews

1aea583

Fix test failures

875b405

mogoodrich reviewed Dec 9, 2024

View reviewed changes

jayasanka-sack added 5 commits December 10, 2024 14:09

Fix test failures

8b20d2d

Merge DAOs

7f8fe94

Fix method namings

a23b7c0

Use EmrApiProperties Class

f2fa209

Use XML instead autowiring

cade351

jayasanka-sack added 3 commits December 16, 2024 21:24

Use XML instead autowiring

494e9b9

Extend BaseRestController

e8c79b1

Fix tests

b155ca9

mogoodrich reviewed Dec 17, 2024

View reviewed changes


		public List<VisitWithDiagnoses> getVisitsByPatientId(Patient patient, int startIndex, int limit) {

		String visitNoteEncounterTypeUuid = "d7151f82-c1f3-4152-a605-2f9ea7414a79";


		import java.util.List;

		public interface VisitWithDiagnosesService {

EA-207: Implement an API for Fetching Visits with Notes and Diagnosis #250

Are you sure you want to change the base?

EA-207: Implement an API for Fetching Visits with Notes and Diagnosis #250

Conversation

jayasanka-sack commented Oct 24, 2024 • edited Loading

mogoodrich commented Oct 24, 2024

jayasanka-sack commented Oct 24, 2024 • edited Loading

mogoodrich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayasanka-sack Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibacher Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayasanka-sack commented Dec 9, 2024

jayasanka-sack commented Dec 9, 2024

mogoodrich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mogoodrich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayasanka-sack Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayasanka-sack commented Oct 24, 2024 •

edited

Loading

jayasanka-sack commented Oct 24, 2024 •

edited

Loading

jayasanka-sack Dec 9, 2024 •

edited

Loading

ibacher Dec 16, 2024 •

edited

Loading

jayasanka-sack Dec 18, 2024 •

edited

Loading