O3-1494: Added REST APIs for O3 Report Admin pages #33

pwargulak · 2024-02-20T15:59:08Z

Summary

This change adds REST APIs used on O3 Admin pages, including:

ReportDesign resource
ScheduledReport pseudo, read-only resource which returns scheduled ReportRequests
extends ReportingRestController with endpoints used to request a report execution, schedule report, cancel, preserve and download

See the related issue.

Issue

https://openmrs.atlassian.net/browse/O3-1494

dkayiwa · 2024-02-20T20:45:28Z

omod/pom.xml

        <dependency>
            <groupId>org.hamcrest</groupId>
            <artifactId>hamcrest-all</artifactId>
            <scope>test</scope>
 		</dependency>
+		<dependency>
+			<groupId>org.openmrs.module</groupId>
+			<artifactId>uiframework-api</artifactId>


Are you intentionally forcing O3 distributions to have to load the uiframework module? It would have been better if we can avoid this. 😊

Agreed, not sure why you need this?

I'll remove it, it was needed for FileDownload class

dkayiwa · 2024-02-20T20:48:39Z

omod/src/main/java/org/openmrs/module/reportingrest/web/controller/ReportingRestController.java

@@ -13,11 +13,40 @@
 */
 package org.openmrs.module.reportingrest.web.controller;

+import org.apache.commons.lang3.StringUtils;
+import org.apache.commons.lang3.time.DateUtils;
+import org.apache.commons.logging.Log;


This is our convention for logging: https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-loggingLogging

dkayiwa · 2024-02-20T21:56:20Z

omod/src/main/java/org/openmrs/module/reportingrest/web/controller/ReportingRestController.java

+        }
+    }
+
+    @RequestMapping(value = "/preserveReport", method = RequestMethod.POST)


How different is preserveReport from saveReport?

Could this not be done as an operation on the existing reportRequest endpoint, to preserve that report request (eg. changing the status)?

@dkayiwa I'll change it to saveReport, preserve came from UI.

@mseaton Generally, this is a quick migration of legacy UI's org.openmrs.module.reportingui.fragment.controller.ReportStatusFragmentController, this directly relates to actions on UI.

The existing ReportRequestResource is a CrudResource - should I just add an endpoint method there?
Or do you mean, that if there is an update call, which changes request's status from COMPLETED to SAVED, then it should run the ReportService.savereport()?

dkayiwa · 2024-02-20T22:00:16Z

omod/src/main/java/org/openmrs/module/reportingrest/web/resource/ReportDesignResource.java

+/**
+ * {@link Resource} for {@link ReportDesign}s, supporting standard CRUD operations
+ */
+@Resource(name = RestConstants.VERSION_1 + ReportingRestController.REPORTING_REST_NAMESPACE + "/designs",


How about reportDesign instead of designs?

Agreed. Please follow existing naming conventions. reportDesign is appropriate.

dkayiwa · 2024-02-20T23:38:52Z

omod/src/main/java/org/openmrs/module/reportingrest/web/resource/ReportDesignResource.java

@@ -0,0 +1,111 @@
+/**
+ * The contents of this file are subject to the OpenMRS Public License
+ * Version 1.0 (the "License"); you may not use this file except in


This is the current license header: https://github.com/openmrs/openmrs-core/blob/2.6.4/api/src/main/java/org/openmrs/Address.java#L1-L9

dkayiwa · 2024-02-20T23:42:12Z

omod/src/main/java/org/openmrs/module/reportingrest/web/controller/ReportingRestController.java

+    private FileDownload processAndDownloadReport(String reportRequestUuid, ReportService reportService) {
+        ReportRequest reportRequest = reportService.getReportRequestByUuid(reportRequestUuid);
+        if (reportRequest == null) {
+            throw new IllegalArgumentException("Report request not found");


It helps to include the report request uuid in the error message.

dkayiwa · 2024-02-20T23:43:07Z

omod/src/main/java/org/openmrs/module/reportingrest/web/controller/ReportingRestController.java

+            try {
+                convertedObject = DateUtils.parseDate((String) value, "MM/dd/yyyy");
+            } catch (ParseException e) {
+                LOGGER.error("Error while parsing date");


Can we pass the exception as the second parameter of the above?

dkayiwa · 2024-02-20T23:44:31Z

omod/src/main/java/org/openmrs/module/reportingrest/web/resource/ReportDesignResource.java

+      throw new IllegalArgumentException("reportDefinitionUuid parameter is required");
+    }
+
+    ReportDefinition reportDefinition = getReportDefinitionService().getDefinitionByUuid(reportDefinitionUuid);


What happens when no report definition is found with the given uuid?

I've changed it to return an empty result, as "no report definition found, then there are no report requests related to that not existing report definition in the system", previously it would return all requests.
But I guess, an exception/404 would be an option too.

dkayiwa · 2024-02-21T00:04:53Z

omod/src/main/java/org/openmrs/module/reportingrest/web/controller/ReportingRestController.java

+
+        if (type.equals(Date.class)) {
+            try {
+                convertedObject = DateUtils.parseDate((String) value, "MM/dd/yyyy");


Why not use org.openmrs.module.webservices.rest.web.ConversionUtil.convert(value, Date.class)

dkayiwa · 2024-02-21T00:08:54Z

omod/src/main/java/org/openmrs/module/reportingrest/web/controller/ReportingRestController.java

+            }
+        }
+
+        if (type.equals(Integer.class)) {


Why not use ConversionUtil.convert?

dkayiwa · 2024-02-21T00:09:18Z

omod/src/main/java/org/openmrs/module/reportingrest/web/controller/ReportingRestController.java

+            convertedObject = Integer.valueOf((String) value);
+        }
+
+        if (type.equals(Location.class)) {


Why not use ConversionUtil.convert?

dkayiwa · 2024-02-21T00:21:08Z

omod/src/main/java/org/openmrs/module/reportingrest/web/resource/ScheduledReportResource.java

+  @Override
+  public DelegatingResourceDescription getRepresentationDescription(Representation representation) {
+    final DelegatingResourceDescription description = new DelegatingResourceDescription();
+    description.addProperty("reportDefinition", Representation.FULL);


Is this the only representation that you support?

Yes, we didn't need anything else to support the O3 UI.

pwargulak · 2024-02-21T16:32:44Z

Changed to DRAFT, becuase it's not adapted to review changes in openmrs/openmrs-module-reporting#249

mseaton · 2024-02-23T03:41:58Z

omod/pom.xml

        <dependency>
            <groupId>org.hamcrest</groupId>
            <artifactId>hamcrest-all</artifactId>
            <scope>test</scope>
 		</dependency>
+		<dependency>
+			<groupId>org.openmrs.module</groupId>
+			<artifactId>uiframework-api</artifactId>


Agreed, not sure why you need this?

mseaton · 2024-02-23T03:51:46Z

omod/src/main/java/org/openmrs/module/reportingrest/web/controller/ReportingRestController.java

+
+    @RequestMapping(value = "/runReport", method = RequestMethod.POST)
+    @ResponseStatus(HttpStatus.OK)
+    public void runReport(@RequestBody RunReportRequest runReportRequest) {


Can you explain what this endpoint does that the existing reportRequest endpoint does not do? Can we improve what we have if needed or do we really need this new endpoint?

Current ReportRequestResource just saves a ReportRequest in DB, as-is.
The runReport creates ReportRequest and adds it to execution queue - maybe better name would be queueReport as from ReportService.queueReport.

Generally, this is migration of org.openmrs.module.reportingui.page.controller.RunReportPageController.

Would it make sense for ReportRequestResource to queue every new ReportRequest?
We were hesitant to modify anything already in use.

@pwargulak - It's been a long time since I've looked at this, but my first reaction is that it definitely would make the most sense for the ReportRequestResource to queue every new ReportRequest. So basically:

If the report request does not already exist with the given uuid, call reportService.queueReport(ReportRequest), otherwise calls reportService.saveReportRequest(ReportRequest);

mseaton · 2024-02-23T03:54:55Z

omod/src/main/java/org/openmrs/module/reportingrest/web/controller/ReportingRestController.java

+
+    @RequestMapping(value = "/cancelReport", method = RequestMethod.DELETE)
+    @ResponseStatus(HttpStatus.OK)
+    public void cancelReport(@RequestParam String reportRequestUuid) {


Could this be handled by the DELETE operation on the existing reportRequest endpoint, maybe based on certain parameters that dictate whether the goal is to cancel or purge the report request? Or maybe as a POST or PUT operation on the reportRequest endpoint that aims to change the status to CANCELLED?

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

omod/src/main/java/org/openmrs/module/reportingrest/web/controller/ReportingRestController.java

+        }
+    }
+
+    @RequestMapping(value = "/preserveReport", method = RequestMethod.POST)


Could this not be done as an operation on the existing reportRequest endpoint, to preserve that report request (eg. changing the status)?

mseaton · 2024-02-23T03:57:41Z

omod/src/main/java/org/openmrs/module/reportingrest/web/controller/ReportingRestController.java

+            reportService.saveReport(report, StringUtils.EMPTY);
+        }
+    }
+


All of these operations that take in a reportRequestUuid seem like they might be better done within the reportRequest resource / endpoint

mseaton · 2024-02-23T04:01:29Z

omod/src/main/java/org/openmrs/module/reportingrest/web/resource/ReportRequestResource.java

 	@Override
 	protected PageableResult doSearch(RequestContext context) {
+		String statusesGroup = context.getParameter("statusesGroup");


I don't love this hard-coding like this with these statusGroups. Why is this needed?

It adds groups to ReportRequest.Status:

ran - all statuses which mean a report was executed

processing - all statuses which mean report is being executed

scheduled - all statuses which mean it's an execution according to schedule

We could change the statusesGroup parameter to status and allow comma-separated list here, moving the grouping of Status to the FE.

@pwargulak - I think that would be preferable. Basically, allow the endpoint to accept multiple values for status and interpret this as an OR condition. I wouldn't accept a csv input, rather I would just get the array of parameter values out of the request for the status parameter name, and treat this as an "or".

mseaton · 2024-02-23T04:03:41Z

omod/src/main/java/org/openmrs/module/reportingrest/web/resource/ReportRequestResource.java

+			}
+
+			return new AlreadyPaged<ReportRequest>(context, reportRequests, reportRequestsDTO.getReportRequestCount() > (long) pageNumber * pageSize, reportRequestsDTO.getReportRequestCount());
+		}


I don't understand the placement of this if/then block. Let's just do things consistently and always use the paging endpoint. If no paging parameters are supplied, it should default to returning all like it currently does.

mseaton · 2024-02-23T04:05:25Z

omod/src/main/java/org/openmrs/module/reportingrest/web/resource/ScheduledReportResource.java

+import java.util.Comparator;
+import java.util.List;
+
+@Resource(name = RestConstants.VERSION_1 + ReportingRestController.REPORTING_REST_NAMESPACE + "/scheduledReport",


Can't this class be handled by hitting the reportRequest endpoint with appropriate parameters to only return scheduled reports? Or can't we make it do so?

Basically, this is reportDefinition left join reportRequest - one request.
With existing APIs, we would need to REST GET reportDefinition and reportRequest then join them on FE side.
Used on a page which shows a table of all Reports with their related execution schedule.

I see. OK. Couldn't we accomplish this without adding the ScheduledReport class, and just return a List where each simple object contains a reportDefinition -> ReportDefinition and scheduledReports -> List<ReportRequest> object?

Then, in the frontend, you'd hit this endpoint and it would give you all available report definitions, and any scheduled reports for them, or an empty list if none.

mseaton · 2024-02-23T04:06:10Z

omod/src/main/resources/config.xml

@@ -15,6 +15,7 @@
 	   	<require_module version="${serializationxstreamVersion}">org.openmrs.module.serialization.xstream</require_module>
 	   	<require_module version="${reportingVersion}">org.openmrs.module.reporting</require_module>
 	   	<require_module version="${webservicesRestVersion}">org.openmrs.module.webservices.rest</require_module>
+	   	<require_module version="${uiframeworkModuleVersion}">org.openmrs.module.uiframework</require_module>


No, this should not require uiframework. This needs to be removed.

mseaton · 2024-02-23T04:06:26Z

pom.xml

+        <artifactId>uiframework-api</artifactId>
+        <version>${uiframeworkModuleVersion}</version>
+        <scope>provided</scope>
+      </dependency>


This dependency needs to be removed.

pwargulak · 2024-03-13T15:56:55Z

@mseaton Just want to confirm with you, that the ReportRequest REST API should perform some additional actions based on ReportRequest's status change, instead having separate endpoints for invoking the actions.

For example,
preserveReport should be removed and the functionality should be invoked when ReportRequest's status is updated to SAVED via REST API.

mseaton · 2024-03-14T19:47:01Z

@mseaton Just want to confirm with you, that the ReportRequest REST API should perform some additional actions based on ReportRequest's status change, instead having separate endpoints for invoking the actions.

For example, preserveReport should be removed and the functionality should be invoked when ReportRequest's status is updated to SAVED via REST API.

I'm fine either way, but would be good if we could make our existing endpoints behave as we think they should.

…ging endpoint

druchniewicz · 2024-12-23T17:43:58Z

@mseaton @dkayiwa @ibacher @djazayeri

we would like to get back to this and make the code ready for merging. We applied previous remarks from code review. Can you take a look at this once again?
This PR and openmrs/openmrs-module-reporting#251 are related to each other and together constitute backend for Reports page in O3 UI - openmrs/openmrs-esm-admin-tools#45 (this UI PR has to be adapted after merging backend PRs)
CC: @pwargulak

O3-1494: Added REST APIs for O3 Report Admin pages

a82878f

dkayiwa reviewed Feb 20, 2024

View reviewed changes

dkayiwa reviewed Feb 21, 2024

View reviewed changes

pwargulak marked this pull request as draft February 21, 2024 16:32

mseaton requested changes Feb 23, 2024

View reviewed changes

pwargulak mentioned this pull request Feb 27, 2024

O3-1494: Added Report Admin pages openmrs/openmrs-esm-admin-tools#45

Draft

O3-1494: Removed uiframework-api, various code quality adjustments

b18c4eb

03-1494: Refactored fetching reports by status, removed redundant pur…

f88990c

…ging endpoint

druchniewicz mentioned this pull request Dec 23, 2024

REPORT-906: Add pagination support to ReportRequest read methods openmrs/openmrs-module-reporting#251

Open

O3-1494: Added REST APIs for O3 Report Admin pages #33

Are you sure you want to change the base?

O3-1494: Added REST APIs for O3 Report Admin pages #33

Conversation

pwargulak commented Feb 20, 2024

Summary

Issue

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

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

pwargulak commented Feb 21, 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

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

pwargulak Feb 27, 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

pwargulak commented Mar 13, 2024

mseaton commented Mar 14, 2024

druchniewicz commented Dec 23, 2024

dkayiwa Feb 20, 2024 •

edited

Loading

dkayiwa Feb 21, 2024 •

edited

Loading

pwargulak Feb 27, 2024 •

edited

Loading