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.

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

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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
5 changes: 4 additions & 1 deletion omod/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,15 @@
<artifactId>openmrs-test</artifactId>
<type>pom</type>
</dependency>

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

@dkayiwa dkayiwa Feb 20, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, not sure why you need this?

Copy link
Author

Choose a reason for hiding this comment

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

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

</dependency>
</dependencies>

<build>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

import org.apache.commons.logging.LogFactory;
import org.openmrs.Location;
import org.openmrs.api.context.Context;
import org.openmrs.module.reporting.cohort.definition.CohortDefinition;
import org.openmrs.module.reporting.evaluation.parameter.Mapped;
import org.openmrs.module.reporting.evaluation.parameter.Parameter;
import org.openmrs.module.reporting.report.Report;
import org.openmrs.module.reporting.report.ReportRequest;
import org.openmrs.module.reporting.report.definition.ReportDefinition;
import org.openmrs.module.reporting.report.definition.service.ReportDefinitionService;
import org.openmrs.module.reporting.report.renderer.RenderingMode;
import org.openmrs.module.reporting.report.service.ReportService;
import org.openmrs.module.reporting.web.renderers.WebReportRenderer;
import org.openmrs.module.reportingrest.web.wrapper.RunReportRequest;
import org.openmrs.module.webservices.rest.web.RestConstants;
import org.openmrs.module.webservices.rest.web.v1_0.controller.MainResourceController;
import org.openmrs.ui.framework.page.FileDownload;
import org.springframework.http.HttpStatus;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestMethod;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.ResponseStatus;

import java.text.ParseException;
import java.util.ArrayList;
import java.util.Date;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

/**
* Controller for {@link CohortDefinition}s
Expand All @@ -28,11 +57,139 @@ public class ReportingRestController extends MainResourceController {

public static final String REPORTING_REST_NAMESPACE = "/reportingrest";

private static final Log LOGGER = LogFactory.getLog(ReportingRestController.class);

/**
* @see org.openmrs.module.webservices.rest.web.v1_0.controller.BaseRestController#getNamespace()
*/
@Override
public String getNamespace() {
return RestConstants.VERSION_1 + REPORTING_REST_NAMESPACE;
}

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

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

ReportDefinition reportDefinition = Context.getService(ReportDefinitionService.class)
.getDefinitionByUuid(runReportRequest.getReportDefinitionUuid());

ReportService reportService = getReportService();

Map<String, Object> parameterValues = new HashMap<String, Object>();
for (Parameter parameter : reportDefinition.getParameters()) {
Object convertedObj =
convertParamValueToObject(runReportRequest.getReportParameters().get(parameter.getName()), parameter.getType());
parameterValues.put(parameter.getName(), convertedObj);
}

List<RenderingMode> renderingModes = reportService.getRenderingModes(reportDefinition);
RenderingMode renderingMode = null;
for (RenderingMode mode : renderingModes) {
if (StringUtils.equals(mode.getArgument(), runReportRequest.getRenderModeUuid())) {
renderingMode = mode;
break;
}
}

final ReportRequest reportRequest;
if(StringUtils.isNotBlank(runReportRequest.getExistingRequestUuid())) {
reportRequest = reportService.getReportRequestByUuid(runReportRequest.getExistingRequestUuid());
} else {
reportRequest = new ReportRequest();
}
reportRequest.setReportDefinition(new Mapped<ReportDefinition>(reportDefinition, parameterValues));
reportRequest.setRenderingMode(renderingMode);
reportRequest.setPriority(ReportRequest.Priority.NORMAL);
reportRequest.setSchedule(runReportRequest.getSchedule());

reportService.queueReport(reportRequest);
reportService.processNextQueuedReports();
}

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

Choose a reason for hiding this comment

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

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?

ReportService reportService = getReportService();
ReportRequest reportRequest = reportService.getReportRequestByUuid(reportRequestUuid);
if (reportRequest != null) {
reportService.purgeReportRequest(reportRequest);
}
}

@RequestMapping(value = "/preserveReport", method = RequestMethod.POST)
Copy link
Member

Choose a reason for hiding this comment

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

How different is preserveReport from saveReport?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

@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()?

@ResponseStatus(HttpStatus.OK)
public void preserveReport(@RequestParam String reportRequestUuid) {
ReportService reportService = getReportService();
ReportRequest reportRequest = reportService.getReportRequestByUuid(reportRequestUuid);
if (ReportRequest.Status.COMPLETED.equals(reportRequest.getStatus())) {
Report report = reportService.loadReport(reportRequest);
reportService.saveReport(report, StringUtils.EMPTY);
}
}

Copy link
Member

Choose a reason for hiding this comment

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

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

@RequestMapping(value = "/downloadReport", method = RequestMethod.GET)
@ResponseStatus(HttpStatus.OK)
public FileDownload downloadReport(@RequestParam String reportRequestUuid) {
return processAndDownloadReport(reportRequestUuid, getReportService());
}

@RequestMapping(value = "/downloadMultipleReports", method = RequestMethod.GET)
@ResponseStatus(HttpStatus.OK)
public List<FileDownload> downloadMultipleReports(@RequestParam String reportRequestUuids) {
List<FileDownload> fileDownloadList = new ArrayList<FileDownload>();
ReportService reportService = getReportService();
for (String reportRequestUuid : reportRequestUuids.split(",")) {
fileDownloadList.add(processAndDownloadReport(reportRequestUuid, reportService));
}

return fileDownloadList;
}

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

Choose a reason for hiding this comment

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

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

}

RenderingMode renderingMode = reportRequest.getRenderingMode();
if (renderingMode.getRenderer() instanceof WebReportRenderer) {
throw new IllegalStateException("Web Renderers not implemented yet");
}

String fileName = renderingMode.getRenderer().getFilename(reportRequest).replace(" ", "_");
String contentType = renderingMode.getRenderer().getRenderedContentType(reportRequest);
byte[] fileContent = reportService.loadRenderedOutput(reportRequest);

if (fileContent == null) {
throw new IllegalStateException("Error during loading rendered output");
} else {
return new FileDownload(fileName, contentType, fileContent);
}
}

private Object convertParamValueToObject(Object value, Class<?> type) {
Object convertedObject = value;

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

Choose a reason for hiding this comment

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

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

} catch (ParseException e) {
LOGGER.error("Error while parsing date");
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 pass the exception as the second parameter of the above?

}
}

if (type.equals(Integer.class)) {
Copy link
Member

@dkayiwa dkayiwa Feb 21, 2024

Choose a reason for hiding this comment

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

Why not use ConversionUtil.convert?

convertedObject = Integer.valueOf((String) value);
}

if (type.equals(Location.class)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not use ConversionUtil.convert?

convertedObject = Context.getLocationService().getLocationByUuid((String) value);
}

return convertedObject;
}

private ReportService getReportService() {
return Context.getService(ReportService.class);
}
}
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

* compliance with the License. You may obtain a copy of the License at
* http://license.openmrs.org
*
* Software distributed under the License is distributed on an "AS IS"
* basis, WITHOUT WARRANTY OF ANY KIND, either express or implied. See the
* License for the specific language governing rights and limitations
* under the License.
*
* Copyright (C) OpenMRS, LLC. All Rights Reserved.
*/

package org.openmrs.module.reportingrest.web.resource;

import org.apache.commons.lang3.StringUtils;
import org.openmrs.api.context.Context;
import org.openmrs.module.reporting.report.ReportDesign;
import org.openmrs.module.reporting.report.definition.ReportDefinition;
import org.openmrs.module.reporting.report.definition.service.ReportDefinitionService;
import org.openmrs.module.reporting.report.service.ReportService;
import org.openmrs.module.reportingrest.web.controller.ReportingRestController;
import org.openmrs.module.webservices.rest.web.RequestContext;
import org.openmrs.module.webservices.rest.web.RestConstants;
import org.openmrs.module.webservices.rest.web.annotation.Resource;
import org.openmrs.module.webservices.rest.web.representation.RefRepresentation;
import org.openmrs.module.webservices.rest.web.representation.Representation;
import org.openmrs.module.webservices.rest.web.resource.api.PageableResult;
import org.openmrs.module.webservices.rest.web.resource.impl.DelegatingCrudResource;
import org.openmrs.module.webservices.rest.web.resource.impl.DelegatingResourceDescription;
import org.openmrs.module.webservices.rest.web.resource.impl.NeedsPaging;
import org.openmrs.module.webservices.rest.web.response.ResponseException;

import java.util.List;

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

Choose a reason for hiding this comment

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

How about reportDesign instead of designs?

Copy link
Member

Choose a reason for hiding this comment

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

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

supportedClass = ReportDesign.class, supportedOpenmrsVersions = {"1.8.* - 9.9.*"})
public class ReportDesignResource extends DelegatingCrudResource<ReportDesign> {

@Override
public ReportDesign newDelegate() {
return new ReportDesign();
}

@Override
public ReportDesign getByUniqueId(String uuid) {
return getReportService().getReportDesignByUuid(uuid);
}

@Override
protected PageableResult doSearch(RequestContext context) {
String reportDefinitionUuid = context.getParameter("reportDefinitionUuid");
if (StringUtils.isBlank(reportDefinitionUuid)) {
throw new IllegalArgumentException("reportDefinitionUuid parameter is required");
}

ReportDefinition reportDefinition = getReportDefinitionService().getDefinitionByUuid(reportDefinitionUuid);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

List<ReportDesign> reportDesigns = getReportService().getReportDesigns(reportDefinition, null, false);

return new NeedsPaging<ReportDesign>(reportDesigns, context);
}

@Override
protected void delete(ReportDesign reportDesign, String reason, RequestContext requestContext) throws ResponseException {
purge(reportDesign, requestContext);
}

@Override
public ReportDesign save(ReportDesign reportDesign) {
return getReportService().saveReportDesign(reportDesign);
}

@Override
public void purge(ReportDesign reportDesign, RequestContext requestContext) throws ResponseException {
if (reportDesign != null) {
getReportService().purgeReportDesign(reportDesign);
}
}

@Override
public DelegatingResourceDescription getCreatableProperties() {
return new DelegatingResourceDescription();
}

@Override
public DelegatingResourceDescription getRepresentationDescription(Representation representation) {
DelegatingResourceDescription description = null;

if (representation instanceof RefRepresentation) {
description = new DelegatingResourceDescription();
description.addProperty("uuid");
description.addProperty("name");
description.addProperty("rendererType");
description.addSelfLink();
}

return description;
}

private ReportService getReportService() {
return Context.getService(ReportService.class);
}

private ReportDefinitionService getReportDefinitionService() {
return Context.getService(ReportDefinitionService.class);
}
}
Loading