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

REPORT-906: Add pagination support to ReportRequest read methods #249

Closed
wants to merge 5 commits into from
Closed

REPORT-906: Add pagination support to ReportRequest read methods #249

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aca2b85
O3-1494: Added method to get report requests with pagination
druchniewicz Dec 21, 2023
533d2b5
O3-1494: Changes after 1st code review
pwargulak Feb 21, 2024
28d54b4
O3-1494: Changes after 2nd code review
pwargulak Feb 22, 2024
7649ee7
O3-1494: Removed ReportRequestPageDTO
pwargulak Feb 23, 2024
5b5e77e
O3-1494: Adjusted Java Docs
pwargulak Feb 27, 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
31 changes: 31 additions & 0 deletions api/src/main/java/org/openmrs/module/reporting/report/ReportRequestDTO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package org.openmrs.module.reporting.report;

import java.util.List;

public class ReportRequestDTO {

private List<ReportRequest> reportRequests;

private Long reportRequestCount;

public ReportRequestDTO(List<ReportRequest> reportRequests, Long reportRequestCount) {
this.reportRequests = reportRequests;
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just get the reportRequestsCount from reportRequests.size()? Or can we change the variable names to make things clearer? 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed the class name to ReportRequestPageDTO and reportRequestsCount to totalCount, so hopefully it should be easily read as "this is some subset of report requests and total count indicates the size of total set"/ " one page from totalCount items".

this.reportRequestCount = reportRequestCount;
}

public List<ReportRequest> getReportRequests() {
return reportRequests;
}

public void setReportRequests(List<ReportRequest> reportRequests) {
this.reportRequests = reportRequests;
}

public Long getReportRequestCount() {
return reportRequestCount;
}

public void setReportRequestCount(Long reportRequestCount) {
this.reportRequestCount = reportRequestCount;
}
}
7 changes: 7 additions & 0 deletions api/src/main/java/org/openmrs/module/reporting/report/service/ReportService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.openmrs.module.reporting.report.ReportProcessorConfiguration;
import org.openmrs.module.reporting.report.ReportRequest;
import org.openmrs.module.reporting.report.ReportRequest.Status;
import org.openmrs.module.reporting.report.ReportRequestDTO;
import org.openmrs.module.reporting.report.definition.ReportDefinition;
import org.openmrs.module.reporting.report.processor.ReportProcessor;
import org.openmrs.module.reporting.report.renderer.RenderingMode;
Expand Down Expand Up @@ -140,6 +141,12 @@ public interface ReportService extends OpenmrsService {
@Transactional(readOnly = true)
public List<ReportRequest> getReportRequests(ReportDefinition reportDefinition, Date requestOnOrAfter, Date requestOnOrBefore, Integer mostRecentNum, Status...statuses);

/**
Copy link
Member

Choose a reason for hiding this comment

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

Can we add method and param descriptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some JavaDocs, also to related methods.

* @return {@link ReportRequestDTO} object which contains report requests and total count data
*/
@Transactional(readOnly = true)
ReportRequestDTO getReportsWithPagination(ReportDefinition reportDefinition, Date requestOnOrAfter, Date requestOnOrBefore, Integer pageNumber, Integer pageSize, Status...statuses);
Copy link
Member

Choose a reason for hiding this comment

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

Can we add the since annotation? https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-NewPublicMethodsandClasses

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the new method name, does it mean we already have a getReports method without pagination?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two getReportRequests methods without pagination, both return a List.
The getReportsWithPagination returns 'wrapper' object - a reason for different name.

We'll change the name to getReportRequestsWithPagination as suggested in some of the later comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Annotation added, method name changed.


/**
* Deletes the passed {@link ReportRequest}
*/
Expand Down
10 changes: 10 additions & 0 deletions api/src/main/java/org/openmrs/module/reporting/report/service/ReportServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.openmrs.module.reporting.report.ReportRequest.Priority;
import org.openmrs.module.reporting.report.ReportRequest.PriorityComparator;
import org.openmrs.module.reporting.report.ReportRequest.Status;
import org.openmrs.module.reporting.report.ReportRequestDTO;
import org.openmrs.module.reporting.report.definition.ReportDefinition;
import org.openmrs.module.reporting.report.definition.service.ReportDefinitionService;
import org.openmrs.module.reporting.report.processor.ReportProcessor;
Expand Down Expand Up @@ -223,6 +224,15 @@ public List<ReportRequest> getReportRequests(ReportDefinition reportDefinition,
return reportDAO.getReportRequests(reportDefinition, requestOnOrAfter, requestOnOrBefore, mostRecentNum, statuses);
}

/**
* @see ReportService#getReportsWithPagination(ReportDefinition, Date, Date, Integer, Integer, Status...)
*/
@Override
@Transactional(readOnly = true)
public ReportRequestDTO getReportsWithPagination(ReportDefinition reportDefinition, Date requestOnOrAfter, Date requestOnOrBefore, Integer pageNumber, Integer pageSize, Status... statuses) {
return reportDAO.getReportsWithPagination(reportDefinition, requestOnOrAfter, requestOnOrBefore, pageNumber, pageSize, statuses);
}

/**
* @see ReportService#purgeReportRequest(ReportRequest)
*/
Expand Down
41 changes: 40 additions & 1 deletion api/src/main/java/org/openmrs/module/reporting/report/service/db/HibernateReportDAO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,15 @@
import org.hibernate.Query;
import org.hibernate.criterion.Expression;
import org.hibernate.criterion.Order;
import org.hibernate.criterion.Projections;
import org.hibernate.criterion.Restrictions;
import org.openmrs.api.db.DAOException;
import org.openmrs.api.db.hibernate.DbSessionFactory;
import org.openmrs.module.reporting.report.ReportDesign;
import org.openmrs.module.reporting.report.ReportProcessorConfiguration;
import org.openmrs.module.reporting.report.ReportRequest;
import org.openmrs.module.reporting.report.ReportRequest.Status;
import org.openmrs.module.reporting.report.ReportRequestDTO;
import org.openmrs.module.reporting.report.definition.ReportDefinition;
import org.openmrs.module.reporting.report.renderer.ReportRenderer;

Expand Down Expand Up @@ -214,7 +216,44 @@ public List<ReportRequest> getReportRequests(ReportDefinition reportDefinition,
}
return c.list();
}

Copy link
Member

Choose a reason for hiding this comment

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

Add a method here that returns an Integer called "getReportRequestCount" or something, that takes in your parameters and returns the rowcount projection only, then modify the below method to just return the List that match your parameters including the pageNumber and pageSize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the method, also removed the "pagination concept", and replaced with more general first result and max results - the page calculation will be added to reporting-rest.

Copy link
Member

Choose a reason for hiding this comment

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

This looks good, thanks @pwargulak


/**
* @see ReportDAO#getReportsWithPagination(ReportDefinition, Date, Date, Integer, Integer, Status...)
*/
public ReportRequestDTO getReportsWithPagination(ReportDefinition reportDefinition, Date requestOnOrAfter, Date requestOnOrBefore, Integer pageNumber, Integer pageSize, Status...statuses) {
Copy link
Member

Choose a reason for hiding this comment

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

Since you are returning requests, getReportRequests looks better than getReports. Not so? 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to getReportRequestsWithPagination, although the suffix WithPagination might not be great.

Criteria c = sessionFactory.getCurrentSession().createCriteria(ReportRequest.class);

if (reportDefinition != null) {
c.add(Restrictions.eq("reportDefinition.definition", reportDefinition.getUuid()));
}
if (requestOnOrAfter != null) {
c.add(Restrictions.ge("requestDate", requestOnOrAfter));
}
if (requestOnOrBefore != null) {
c.add(Restrictions.le("requestDate", requestOnOrBefore));
}
if (statuses != null && statuses.length > 0) {
c.add(Restrictions.in("status", statuses));
}
c.addOrder(Order.desc("evaluateCompleteDatetime"));
c.addOrder(Order.desc("evaluateStartDatetime"));
c.addOrder(Order.desc("priority"));
c.addOrder(Order.desc("requestDate"));

c.setProjection(Projections.rowCount());
Long count = (Long) c.uniqueResult();
c.setProjection(null);

if (pageNumber != null && pageSize != null) {
c.setFirstResult((pageNumber - 1) * pageSize);
c.setMaxResults(pageSize);
}

List<ReportRequest> reportRequests = c.list();

return new ReportRequestDTO(reportRequests, count);
}

/**
* @see ReportDAO#purgeReportRequest(ReportRequest)
*/
Expand Down
6 changes: 6 additions & 0 deletions api/src/main/java/org/openmrs/module/reporting/report/service/db/ReportDAO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.openmrs.module.reporting.report.ReportProcessorConfiguration;
import org.openmrs.module.reporting.report.ReportRequest;
import org.openmrs.module.reporting.report.ReportRequest.Status;
import org.openmrs.module.reporting.report.ReportRequestDTO;
import org.openmrs.module.reporting.report.definition.ReportDefinition;
import org.openmrs.module.reporting.report.renderer.ReportRenderer;

Expand Down Expand Up @@ -123,6 +124,11 @@ public List<ReportDesign> getReportDesigns(ReportDefinition reportDefinition, Cl
*/
public List<ReportRequest> getReportRequests(ReportDefinition reportDefinition, Date requestOnOrAfter, Date requestOnOrBefore, Integer mostRecentNum, Status...statuses);

/**
* @return {@link ReportRequestDTO} object which contains report requests and total count data
Copy link
Member

@dkayiwa dkayiwa Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can we also document this fully? Or at least use a @see annotation pointing to where it is properly documented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added see

*/
ReportRequestDTO getReportsWithPagination(ReportDefinition reportDefinition, Date requestOnOrAfter, Date requestOnOrBefore, Integer pageNumber, Integer pageSize, Status...statuses);

/**
* Deletes the passed {@link ReportRequest}
*/
Expand Down
Loading