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

pwargulak · 2024-02-20T15:46:37Z

Summary

This change extends ReportService with new method getReportsWithPagination used to read ReportRequests for specific report, dates, statuses paginated for performance gains.
It was done as part of making of O3 Reports Admin pages.

See the related issue.

Issue

https://openmrs.atlassian.net/browse/REPORT-906

pwargulak · 2024-02-20T15:46:54Z

@druchniewicz CC

dkayiwa · 2024-02-20T22:41:45Z

api/src/main/java/org/openmrs/module/reporting/report/service/ReportService.java

+	 * @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);


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

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

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.

Annotation added, method name changed.

dkayiwa · 2024-02-20T22:43:21Z

api/src/main/java/org/openmrs/module/reporting/report/service/ReportService.java

@@ -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);

+	/**


Can we add method and param descriptions?

Added some JavaDocs, also to related methods.

dkayiwa · 2024-02-20T22:48:04Z

api/src/main/java/org/openmrs/module/reporting/report/service/db/HibernateReportDAO.java

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


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

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

dkayiwa · 2024-02-20T22:50:44Z

What do you think of adding some tests?

pwargulak · 2024-02-21T16:31:46Z

What do you think of adding some tests?

Added couple of tests.

dkayiwa · 2024-02-21T21:23:56Z

...s/src/test/resources/org/openmrs/module/reporting/include/ReportTestDataset-openmrs-1.10.xml

Did you intentionally change the formatting of these xml dataset files? I notice that you reformatted quite a number of them. I thought you would just add your new records and leave the rest intact. 😊

I've added 4th reporting_report_request to all datasets (one for each openMRSMinorVersion) and intentionally formatted the reporting_report_request elements group to have the same style as reporting_report_design elements group just a line above - IMO made sense to do.

dkayiwa · 2024-02-21T21:26:00Z

api/src/main/java/org/openmrs/module/reporting/report/service/ReportService.java

+	 */
+	@Transactional(readOnly = true)
+	public ReportRequestDTO getReportRequestsWithPagination(ReportDefinition reportDefinition, Date requestOnOrAfter,
+																										Date requestOnOrBefore, Integer pageNumber, Integer pageSize, Status...statuses);


Can you look into this formatting?

Made it back into one line.

dkayiwa · 2024-02-21T21:31:07Z

api/src/main/java/org/openmrs/module/reporting/report/service/db/ReportDAO.java

@@ -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


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

dkayiwa · 2024-02-21T21:36:30Z

api/src/main/java/org/openmrs/module/reporting/report/ReportRequestDTO.java

+  private long reportRequestCount;
+
+  public ReportRequestDTO(List<ReportRequest> reportRequests, long reportRequestCount) {
+    this.reportRequests = reportRequests;


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

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".

mseaton · 2024-02-23T03:37:42Z

api/src/main/java/org/openmrs/module/reporting/report/ReportRequestPageDTO.java

+
+import java.util.List;
+
+public class ReportRequestPageDTO {


I'd prefer to see us not introduce this new ReportRequestPageDTO class, but to just have separate DAO methods - one to get a count of all requests that match the criteria, and one to get the current list of requests that match the criteria. Creating a separate shared method to create the common criteria that would be used would be fine.

mseaton · 2024-02-23T03:40:34Z

api/src/main/java/org/openmrs/module/reporting/report/service/db/HibernateReportDAO.java

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


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.

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.

This looks good, thanks @pwargulak

mseaton

This looks good to me @pwargulak , thanks for the rework. I made a few minor comments related to the javadoc that seemed inaccurate, but otherwise I think this is good to go.

mseaton · 2024-02-26T13:50:38Z

api/src/main/java/org/openmrs/module/reporting/report/service/ReportService.java

+	 * @param requestOnOrBefore a Date used to limit result to ReportRequests which ware requested on or before
+	 *                          (lower or equal filter), nullable
+	 * @param firstResult the first result to be retrieved, not null
+	 * @param maxResults a limit upon the number of Report Requests to be retrieved, not null


The maxResults does appear that it can be nullable, based on the dao implementation...

mseaton · 2024-02-26T13:51:19Z

api/src/main/java/org/openmrs/module/reporting/report/service/ReportService.java

+	 *                          (greater or equal filter), nullable
+	 * @param requestOnOrBefore a Date used to limit result to ReportRequests which ware requested on or before
+	 *                          (lower or equal filter), nullable
+	 * @param firstResult the first result to be retrieved, not null


The firstResult does appear that it can be nullable, based on the dao implementation...

mseaton · 2024-02-26T13:52:08Z

api/src/main/java/org/openmrs/module/reporting/report/service/db/HibernateReportDAO.java

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


This looks good, thanks @pwargulak

mseaton

This is ready to merge @pwargulak , but all changes made to the reporting module should have a corresponding ticket in the reporting module JIRA project, and the commit message should link back to that ticket, rather than to an O3 ticket. Can you do that?

pwargulak · 2024-03-13T15:50:59Z

@mseaton Added https://openmrs.atlassian.net/browse/REPORT-906

pwargulak · 2024-03-28T11:57:31Z

Moved to #251 to indicate real source of the PR.

O3-1494: Added method to get report requests with pagination

aca2b85

dkayiwa reviewed Feb 20, 2024

View reviewed changes

O3-1494: Changes after 1st code review

533d2b5

pwargulak mentioned this pull request Feb 21, 2024

O3-1494: Added REST APIs for O3 Report Admin pages openmrs/openmrs-module-reportingrest#33

Draft

dkayiwa reviewed Feb 21, 2024

View reviewed changes

O3-1494: Changes after 2nd code review

28d54b4

mseaton requested changes Feb 23, 2024

View reviewed changes

O3-1494: Removed ReportRequestPageDTO

7649ee7

mseaton approved these changes Feb 26, 2024

View reviewed changes

O3-1494: Adjusted Java Docs

5b5e77e

mseaton approved these changes Feb 27, 2024

View reviewed changes

pwargulak changed the title ~~O3-1494: Added method to get report requests with pagination~~ REPORT-906: Add pagination support to ReportRequest read methods Mar 13, 2024

pwargulak closed this Mar 28, 2024

pwargulak mentioned this pull request Mar 28, 2024

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pwargulak commented Feb 20, 2024 •

edited

Loading

pwargulak commented Feb 20, 2024

dkayiwa Feb 20, 2024

dkayiwa Feb 20, 2024

pwargulak Feb 21, 2024

pwargulak Feb 21, 2024

dkayiwa Feb 20, 2024

pwargulak Feb 21, 2024

dkayiwa Feb 20, 2024

pwargulak Feb 21, 2024

dkayiwa commented Feb 20, 2024

pwargulak commented Feb 21, 2024

dkayiwa Feb 21, 2024 •

edited

Loading

pwargulak Feb 22, 2024

dkayiwa Feb 21, 2024

pwargulak Feb 22, 2024

dkayiwa Feb 21, 2024 •

edited

Loading

pwargulak Feb 22, 2024

dkayiwa Feb 21, 2024

pwargulak Feb 22, 2024

mseaton Feb 23, 2024

pwargulak Feb 23, 2024

mseaton Feb 23, 2024

pwargulak Feb 23, 2024

mseaton Feb 26, 2024

mseaton left a comment

mseaton Feb 26, 2024

pwargulak Feb 27, 2024

mseaton Feb 26, 2024

pwargulak Feb 27, 2024

mseaton Feb 26, 2024

mseaton left a comment

pwargulak commented Mar 13, 2024

pwargulak commented Mar 28, 2024

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

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

Conversation

pwargulak commented Feb 20, 2024 • edited Loading

Summary

Issue

pwargulak commented Feb 20, 2024

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

dkayiwa commented Feb 20, 2024

pwargulak commented Feb 21, 2024

dkayiwa Feb 21, 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

dkayiwa Feb 21, 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

mseaton 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

mseaton left a comment

Choose a reason for hiding this comment

pwargulak commented Mar 13, 2024

pwargulak commented Mar 28, 2024

pwargulak commented Feb 20, 2024 •

edited

Loading

dkayiwa Feb 21, 2024 •

edited

Loading

dkayiwa Feb 21, 2024 •

edited

Loading