Skip to content

LUI-198: Optimize patient dashboard loading by implementing pagination for observations #209

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.

Sign up for GitHub

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
ODORA0 wants to merge 16 commits into
base: master
Choose a base branch
from
Open

LUI-198: Optimize patient dashboard loading by implementing pagination for observations #209

Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b03e0de
(Enhancement) Optimize patient dashboard loading by implementing pagi…
ODORA0 Nov 13, 2024
a06309a
Refactor to focus on pagination
ODORA0 Nov 14, 2024
ba7a0fc
DB-level pagination to only load in memory obs for current page
ODORA0 Nov 29, 2024
fa01899
Limit loaded obs to MostRecentN
ODORA0 Dec 4, 2024
e1c2f60
Remove hard coding and keep values dynamic based on GP set with defau…
ODORA0 Dec 4, 2024
9456377
Change naming conventions
ODORA0 Dec 5, 2024
98301a5
Create seperate GP for observations
ODORA0 Dec 6, 2024
639a616
Remove extra formatting
ODORA0 Dec 6, 2024
3426cb9
Remove extra formatting
ODORA0 Dec 6, 2024
1bf5bea
Remove false as default value
ODORA0 Dec 6, 2024
6bcaeda
Add StartIndex to getObservation api
ODORA0 Dec 6, 2024
d7e5866
Add proper null checks using StringUtils.hasText(), update getEncount…
ODORA0 Dec 8, 2024
a6b840c
Keep obs set for BMI calculations, remove unused limit param from model
ODORA0 Dec 9, 2024
b763c92
Enhance obs to load latest weight and height
ODORA0 Dec 13, 2024
568c1f9
Remove commented out code
ODORA0 Dec 13, 2024
9bf378f
Add model for latestHeight and weight
ODORA0 Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 73 additions & 21 deletions omod/src/main/java/org/openmrs/web/controller/PortletController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,14 @@

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.stream.Collectors;

import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
Expand All @@ -29,6 +31,7 @@
import org.openmrs.Concept;
import org.openmrs.ConceptNumeric;
import org.openmrs.Encounter;
import org.openmrs.Location;
import org.openmrs.Obs;
import org.openmrs.Patient;
import org.openmrs.Person;
Expand All @@ -39,6 +42,9 @@
import org.openmrs.api.ConceptService;
import org.openmrs.api.context.Context;
import org.openmrs.module.legacyui.GeneralUtils;
import org.openmrs.parameter.EncounterSearchCriteria;
import org.openmrs.parameter.EncounterSearchCriteriaBuilder;
import org.openmrs.util.OpenmrsConstants.PERSON_TYPE;
import org.openmrs.util.PrivilegeConstants;
import org.openmrs.web.WebConstants;
import org.springframework.util.StringUtils;
Expand All @@ -49,6 +55,8 @@ public class PortletController implements Controller {

protected Log log = LogFactory.getLog(this.getClass());

private static final int DEFAULT_PAGE_SIZE = 50;

/**
* This method produces a model containing the following mappings:
*
Expand Down Expand Up @@ -106,14 +114,15 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
Object uri = request.getAttribute("javax.servlet.include.servlet_path");
String portletPath = "";
Map<String, Object> model = null;

{
HttpSession session = request.getSession();
String uniqueRequestId = (String) request.getAttribute(WebConstants.INIT_REQ_UNIQUE_ID);
String lastRequestId = (String) session.getAttribute(WebConstants.OPENMRS_PORTLET_LAST_REQ_ID);
if (uniqueRequestId.equals(lastRequestId)) {
model = (Map<String, Object>) session.getAttribute(WebConstants.OPENMRS_PORTLET_CACHED_MODEL);

// remove cached parameters
// remove cached parameters
List<String> parameterKeys = (List<String>) model.get("parameterKeys");
if (parameterKeys != null) {
for (String key : parameterKeys) {
Expand Down Expand Up @@ -161,7 +170,8 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
}
model.put("parameterKeys", parameterKeys); // so we can clean these up in the next request

// if there's an authenticated user, put them, and their patient set, in the model
// if there's an authenticated user, put them, and their patient set, in the
// model
if (Context.getAuthenticatedUser() != null) {
model.put("authenticatedUser", Context.getAuthenticatedUser());
}
Expand All @@ -183,7 +193,10 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons

// add encounters if this user can view them
if (Context.hasPrivilege(PrivilegeConstants.GET_ENCOUNTERS)) {
model.put("patientEncounters", Context.getEncounterService().getEncountersByPatient(p));
model.put(
"patientEncounters",
Context.getEncounterService().getEncounters(p.getPatientIdentifier().getIdentifier(), 0, 100,
Copy link

@eudson eudson Dec 4, 2024

Choose a reason for hiding this comment

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

@ODORA0 why are you hardcoding the start(0) and length(100) ? My understanding of these two variables is that they can help paginate the results, the start being the page and the length the number of records per page.

Copy link
Member Author

@ODORA0 ODORA0 Dec 4, 2024

Choose a reason for hiding this comment

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

My bad here, I might have forgotten to make the change but changed so that we using the proper pagination parameters using the configured page size in the maximumNumberToShow GP or default to only 50 when empty and allow users to paginate through encounters.

false));
}

// add visits if this user can view them
Expand All @@ -195,8 +208,18 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
}

if (Context.hasPrivilege(PrivilegeConstants.GET_OBS)) {
List<Obs> patientObs = Context.getObsService().getObservationsByPerson(p);
model.put("patientObs", patientObs);
Integer pageSize = getPageSizeParameter(request, as);

Person person = (Person) p;
List<Person> persons = Collections.singletonList(person);

List<Obs> paginatedObs = Context.getObsService().getObservations(persons, null, null, null, null,
Copy link
Member

@dkayiwa dkayiwa Dec 6, 2024

Choose a reason for hiding this comment

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

According to the user interface you shared, what do you do with the startIndex?

Copy link
Member Author

@ODORA0 ODORA0 Dec 6, 2024

Choose a reason for hiding this comment

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

Select the page size

Copy link
Member

@dkayiwa dkayiwa Dec 6, 2024

Choose a reason for hiding this comment

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

Is the page size the same as startIndex?

Copy link
Member Author

@ODORA0 ODORA0 Dec 6, 2024

Choose a reason for hiding this comment

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

Yes, based on the link you shared I renamed pageSize to that

Copy link
Member

@dkayiwa dkayiwa Dec 6, 2024

Choose a reason for hiding this comment

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

Where are you passing the two parameters in the getObservations api call?

Copy link
Member Author

@ODORA0 ODORA0 Dec 6, 2024

Choose a reason for hiding this comment

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

We are only passing the limit and not startIndex directly in that method call

Copy link
Member

@dkayiwa dkayiwa Dec 6, 2024

Choose a reason for hiding this comment

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

Which means that if you are on the last page, you still load all obs from the database to memory.

Copy link
Member Author

@ODORA0 ODORA0 Dec 6, 2024

Choose a reason for hiding this comment

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

@dkayiwa Yes that's right, added that

null, Collections.singletonList("obsDatetime desc"), pageSize,
Copy link

@eudson eudson Dec 4, 2024

Choose a reason for hiding this comment

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

Can we apply the same concept here?

Copy link
Member Author

@ODORA0 ODORA0 Dec 4, 2024

Choose a reason for hiding this comment

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

So I am using the using proper ObsService method signature for observations and using mostRecentN for pagination with the pageSize parameter, using same pageSize from global property or default which is set to 50 thus showing most recent observations up to pageSize limit

null, null, null, false);

model.put("pageSize", pageSize);
model.put("patientObs", paginatedObs);

Obs latestWeight = null;
Obs latestHeight = null;
String bmiAsString = "?";
Expand All @@ -211,25 +234,35 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
if (StringUtils.hasLength(heightString)) {
heightConcept = cs.getConceptNumeric(GeneralUtils.getConcept(heightString).getConceptId());
}
for (Obs obs : patientObs) {
Copy link
Member

@dkayiwa dkayiwa Dec 8, 2024

Choose a reason for hiding this comment

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

Did you intentionally remove this for loop? Is it related to the pagination?

Copy link
Member Author

@ODORA0 ODORA0 Dec 8, 2024

Choose a reason for hiding this comment

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

Rather than loading all observations and finding the latest in memory, we would leverage the database to do this work more efficiently. Not directly related to pagination but aligns with the optimization goals since we no longer need to iterate over all patientObs to extract weight and height.

Copy link
Member

@dkayiwa dkayiwa Dec 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

With performance optimisations, unless you have tested and confirmed, you can be very shocked to find a different outcome. The database itself being an expensive resource, and you have anyway already fetched these observations in the above call, you can find that actually iterating through the collection in memory may be faster than making another expensive database call. So i would remove that loop only after i have done some confirmations.

Copy link
Member Author

@ODORA0 ODORA0 Dec 9, 2024

Choose a reason for hiding this comment

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

The queries for weight and height are correctly implemented, unless there are other dependencies on the for loop logic that exist that are not unaccounted for I think I will put this back to be on a safer side.

Copy link
Member Author

@ODORA0 ODORA0 Dec 9, 2024

Choose a reason for hiding this comment

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

@dkayiwa The paginated list only contains a limited number of observations (default 50), therefore BMI calculation couldn't find the necessary values, resulting in "?" if at all we add back the for loop while paginating. Now for this to work I would have to do something like this

// Paginated observations for display
List<Obs> paginatedObs = Context.getObsService().getObservations(..., limit, startIndex, ...);

// Additional query to get ALL observations for BMI calculation
List<Obs> allObs = Context.getObsService().getObservations(persons, null, null, null, null,
    null, null, null, null, null, null, false);

// Finding weight/height in complete set of observations
for (Obs obs : allObs) {
    // ... looking for weight/height
}

In essence, we'd now be using:

  • Paginated query for display purposes (performance optimization)
  • Complete query for BMI calculation (functionality requirement)

Does this sound an approach to use?

Copy link
Member

@dkayiwa dkayiwa Dec 9, 2024

Choose a reason for hiding this comment

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

What do you see as the best approach?

Copy link
Member Author

@ODORA0 ODORA0 Dec 9, 2024

Choose a reason for hiding this comment

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

I think I will go with this option I have suggested, in the long run we have separation of concerns. Change made

if (obs.getConcept().equals(weightConcept)) {
if (latestWeight == null
|| obs.getObsDatetime().compareTo(latestWeight.getObsDatetime()) > 0) {
latestWeight = obs;
}
} else if (obs.getConcept().equals(heightConcept)
&& (latestHeight == null || obs.getObsDatetime().compareTo(
latestHeight.getObsDatetime()) > 0)) {
latestHeight = obs;

if (weightConcept != null) {
List<Concept> weightQuestions = Collections.singletonList(weightConcept);
List<String> weightSort = Collections.singletonList("obsDatetime desc");

List<Obs> weightObs = Context.getObsService().getObservations(persons, null,
weightQuestions, null, null, null, weightSort, 1, null, null, null, false);

if (!weightObs.isEmpty()) {
latestWeight = weightObs.get(0);
}
}
if (latestWeight != null) {
Copy link
Member

@dkayiwa dkayiwa Dec 13, 2024

Choose a reason for hiding this comment

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

According to the original code, latestHeight or latestWeight was put in the model as long as it was not null. Did you intentionally change it?

Copy link
Member

@dkayiwa dkayiwa Dec 13, 2024

Choose a reason for hiding this comment

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

Did you see the above comment?

model.put("patientWeight", latestWeight);
}
if (latestHeight != null) {
model.put("patientHeight", latestHeight);

if (heightConcept != null) {
List<Concept> heightQuestions = Collections.singletonList(heightConcept);
List<String> heightSort = Collections.singletonList("obsDatetime desc");

List<Obs> heightObs = Context.getObsService().getObservations(persons, null,
heightQuestions, null, null, null, heightSort, 1, null, null, null, false);

if (!heightObs.isEmpty()) {
latestHeight = heightObs.get(0);
}
}

if (latestWeight != null && latestHeight != null) {
model.put("patientWeight", latestWeight);
Copy link
Member

@dkayiwa dkayiwa Dec 8, 2024

Choose a reason for hiding this comment

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

What happens to the model when latestHeight is null but latestWeight is not null?

Copy link
Member Author

@ODORA0 ODORA0 Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No BMI in the model, since both latestWeight and latestHeight are required for BMI calculation, the model will not have patientBmi or patientBmiAsString if either is null. he model will only include the observation that is not null (patientWeight in this case). But since I will revert this, I think any changes like setting a default value or log warning wouldn't be necessary

model.put("patientHeight", latestHeight);

double weightInKg;
double heightInM;
if (weightConcept.getUnits().equalsIgnoreCase("kg")) {
Expand Down Expand Up @@ -354,7 +387,8 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
}
}

// if an encounter id is available, put "encounter" and "encounterObs" in the model
// if an encounter id is available, put "encounter" and "encounterObs" in the
// model
o = request.getAttribute("org.openmrs.portlet.encounterId");
if (o != null && !model.containsKey("encounterId")) {
if (!model.containsKey("encounter") && Context.hasPrivilege(PrivilegeConstants.GET_ENCOUNTERS)) {
Expand Down Expand Up @@ -425,4 +459,22 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
protected void populateModel(HttpServletRequest request, Map<String, Object> model) {
}

private Integer getPageSizeParameter(HttpServletRequest request, AdministrationService as) {
try {
String pageSizeParam = request.getParameter("pageSize");
if (pageSizeParam != null) {
return Integer.parseInt(pageSizeParam);
}

String globalPageSize = as.getGlobalProperty("dashboard.encounters.maximumNumberToShow");
Copy link
Member

@dkayiwa dkayiwa Dec 5, 2024

Choose a reason for hiding this comment

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

Is this pull request about encounters or observations?

Copy link
Member

@dkayiwa dkayiwa Dec 5, 2024

Choose a reason for hiding this comment

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

And just in case you find this helpful: https://openmrs.atlassian.net/browse/LUI-188

Copy link
Member Author

@ODORA0 ODORA0 Dec 5, 2024

Choose a reason for hiding this comment

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

Yes, we thought we could align encounters as well to limit it to based max numbers to show or default to the fallback property 50. cc @eudson
Encounters fetched are now limited to the value of the GP

Copy link
Member

@dkayiwa dkayiwa Dec 5, 2024

Choose a reason for hiding this comment

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

The global property that you are using here is specifically for encounters. You can take a look at the ticket link that i shared above to get some brief historical background about what i am driving at. 😊

Copy link
Member Author

@ODORA0 ODORA0 Dec 5, 2024

Choose a reason for hiding this comment

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

Okay so that solution effectively decouples the configuration for the Form Entry tab and the Encounters tab which tackles both usability and configurability needs without disrupting existing any setups. Are you trying to say we employ the same thing but for the patientEncounters jsp, LOL, I am having a brain fart right now with looking at old tech am not as familiar with

Copy link
Member

@dkayiwa dkayiwa Dec 5, 2024

Choose a reason for hiding this comment

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

One should be able to set different values for encounters and observations.

Copy link
Member Author

@ODORA0 ODORA0 Dec 5, 2024

Choose a reason for hiding this comment

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

This involves creating a different GP for Obs as in maximumNumberObservationsToShow? Please advise

Copy link
Member

@dkayiwa dkayiwa Dec 5, 2024

Choose a reason for hiding this comment

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

Yes you need to create a separate global property for it.

if (globalPageSize != null) {
return Integer.parseInt(globalPageSize);
}
}
catch (NumberFormatException e) {
log.debug("Unable to parse page size parameter, using default", e);
}
return DEFAULT_PAGE_SIZE;
}

}