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

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

Open
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 1 commit
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
45 changes: 11 additions & 34 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 @@ -18,6 +18,7 @@
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 @@ -41,6 +42,8 @@
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;
Expand Down Expand Up @@ -190,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

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

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 @@ -202,36 +208,16 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
}

if (Context.hasPrivilege(PrivilegeConstants.GET_OBS)) {
// 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,
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);
List<Obs> paginatedObs = Context.getObsService().getObservations(persons, null, null, null, null,
Copy link
Member

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

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

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

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

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

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

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

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

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

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("currentPage", page);
model.put("pageSize", pageSize);
model.put("totalObsCount", totalCount);
model.put("patientObs", paginatedObs);

Obs latestWeight = null;
Expand Down Expand Up @@ -473,23 +459,14 @@ 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");
String globalPageSize = as.getGlobalProperty("dashboard.encounters.maximumNumberToShow");
Copy link
Member

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

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

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

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

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

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

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

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);
}
Expand Down
Loading