Skip to content
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

Jump to bottom

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
3 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
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
115 changes: 95 additions & 20 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,6 +11,7 @@

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.HashSet;
Expand All @@ -29,6 +30,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 +41,7 @@
import org.openmrs.api.ConceptService;
import org.openmrs.api.context.Context;
import org.openmrs.module.legacyui.GeneralUtils;
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 +52,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 +111,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 +167,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 Down Expand Up @@ -195,8 +202,38 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
}

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

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

// Setup parameters for database-level pagination
List<Encounter> encounters = null;
List<Concept> questions = null;
List<Concept> answers = null;
List<PERSON_TYPE> personTypes = null;
List<Location> locations = null;
List<String> sort = Collections.singletonList("obsDatetime desc");
Integer obsGroupId = null;
Date fromDate = null;
Date toDate = null;
boolean includeVoided = false;

// Get observations for the current page
List<Obs> paginatedObs = Context.getObsService().getObservations(persons, encounters, questions,
Copy link
Member

Choose a reason for hiding this comment

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

Though the comment says get observations for the current page, i do not seen anything in the getObservations method call that does so. Was it just a typo?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooh yeah, my bad the getObservations() method doesn't have parameters for offset/limit pagination, let me remove that comment

Copy link
Member

Choose a reason for hiding this comment

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

So then, shall we then have any dashboard loading optimisation if the method loads all of them in memory?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I critically looked at the ObsService interface, didn't notice that it lacks proper pagination parameters (offset/limit). Maybe this should be done as a DAO level implementation so we can have a DB level pagination support, but I would think of doing it this way as well i.e. we can use an existing API features like mostRecentN to limit initial load like so

Suggested change
List<Obs> paginatedObs = Context.getObsService().getObservations(persons, encounters, questions,
List<Obs> paginatedObs = Context.getObsService().getObservations(
persons,
null,
null,
null,
null,
null,
Collections.singletonList("obsDatetime desc"),
pageSize, // mostRecentN - only fetch what we need
null,
null,
null,
false
);

or using date filtering

Suggested change
List<Obs> paginatedObs = Context.getObsService().getObservations(persons, encounters, questions,
Calendar cal = Calendar.getInstance();
cal.add(Calendar.DAY_OF_MONTH, -30);
Date thirtyDaysAgo = cal.getTime();
List<Obs> recentObs = Context.getObsService().getObservations(
persons,
null,
null,
null,
null,
null,
sort,
null,
null,
thirtyDaysAgo,
new Date(),
false
);

Your thoughts @dkayiwa

Copy link
Member

Choose a reason for hiding this comment

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

Limiting the number of loaded obs with mostRecentN makes lots of sense to me. For as long as your use case allows it. That is, as long as you are aware that it behaves differently from page size in the sense that the rest of the obs will never be fetched.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay!
@eudson Any thoughts on this, is it acceptable to only show the most recent observations?

answers, personTypes, locations, sort, null, obsGroupId, fromDate, toDate, includeVoided);

// Get total count for pagination
Integer totalCount = Context.getObsService().getObservationCount(persons, encounters, questions,
answers, personTypes, locations, obsGroupId, fromDate, toDate, includeVoided);

model.put("currentPage", page);
model.put("pageSize", pageSize);
model.put("totalObsCount", totalCount);
model.put("patientObs", paginatedObs);

Obs latestWeight = null;
Obs latestHeight = null;
String bmiAsString = "?";
Expand All @@ -211,25 +248,35 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
if (StringUtils.hasLength(heightString)) {
heightConcept = cs.getConceptNumeric(GeneralUtils.getConcept(heightString).getConceptId());
}
for (Obs obs : patientObs) {
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) {
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);
model.put("patientHeight", latestHeight);

double weightInKg;
double heightInM;
if (weightConcept.getUnits().equalsIgnoreCase("kg")) {
Expand Down Expand Up @@ -354,7 +401,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 +473,31 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
protected void populateModel(HttpServletRequest request, Map<String, Object> model) {
}

private Integer getPageParameter(HttpServletRequest request) {
try {
return Integer.parseInt(request.getParameter("page"));
}
catch (NumberFormatException e) {
return 0;
}
}

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.defaultPageSize");
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;
}

}
Loading