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-904: Consider client timezone when processing LocalDateTime values #248

Closed
wants to merge 1 commit into from
Closed

REPORT-904: Consider client timezone when processing LocalDateTime values #248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 deletions api/src/main/java/org/openmrs/module/reporting/report/ReportRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public class ReportRequest extends BaseOpenmrsObject {
private Date renderCompleteDatetime;
private Integer minimumDaysToPreserve;
private String description;
private String clientTimezone;
Copy link
Author

Choose a reason for hiding this comment

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

Added new field so that the requester is able to specify the timezone for the dates on the resulting report

Copy link
Member

Choose a reason for hiding this comment

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

I'm a little wary about this, but I'm not sure I have a much better alternative to offer. The two existing mechanisms that could best support this already on the ReportRequest are:

  1. RenderingMode. If we can adapt the RenderingMode argument or otherwise evolve RenderingMode to allow passing in a timezone, we could potentially use this. But I don't think this will be that straightforward based on how RenderingMode was originally designed.

  2. Parameters. This might be a reasonable alternative. Basically we could say that any parameter passed in which matches the name of an existing "context value" in the evaluation context would override that context value. Then we could add a new default "context value" named "timezone" which would have a value of the server timezone. And this could be overridden by adding a parameter with the same name to the report. Then, we'd ensure this timezone context value from the evaluation context was used whenever dates are rendered.

If we don't think using Parameters in this way is right, another take on 2 would be to add a new column to ReportRequest, but instead of having it be something very specific named "clientTimezone", we'd make it generic and call it "contextValues" or something, and let it define a serialized map or properties of key/values.

What do you think about the second option above?

Copy link
Author

Choose a reason for hiding this comment

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

I assume that adding a parameter (2) will be something like, when creating the request:

ReportRequest reportRequest = new ReportRequest();
reportRequest.setReportDefinition(new Mapped<ReportDefinition>(reportDefinition, Map.of("timezone", "Europe/Zurich")));

Then we would override the "timezone" context value here

The default "timezone" would be set here by adding addContextValue("timezone", ZoneId.systemDefault());

This seems a good approach to me. @mseaton do you think we can proceed with using the existing parameters as above or is it worth creating a new column "contextValues" on the ReportRequest?

Let me know if I've got something wrong or if you have any other input.

cc @ibacher

Copy link
Member

Choose a reason for hiding this comment

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

Sort of. Basically we already have code in a few places in the reporting module that allows context values and parameter values to be used when rendering a report. For example, the ExcelTemplateRenderer runs all values through the EvaluationUtil.evaluateExpression method, which allows values to be converted using a defined syntax (note, we'd need to make a fix here to set the context values first, and then add parameter values, currently this is reversed incorrectly). However, we aren't at all consistent about how these are used (and this goes along with the comment you made below) - we'd have to review all of the different renderers and other ways that report data is output (eg. from the reportingrest module, etc) in order to ensure this was handled consistently, which might be difficult. So this idea probably needs to be reconsidered.


//***** CONSTRUCTORS ******

Expand Down Expand Up @@ -340,6 +341,14 @@ public void setDescription(String description) {
this.description = description;
}

public String getClientTimezone() {
return clientTimezone;
}

public void setClientTimezone(String clientTimezone) {
this.clientTimezone = clientTimezone;
}

//***** ENUMS *****

/**
Expand Down
28 changes: 27 additions & 1 deletion 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 @@ -10,6 +10,7 @@
package org.openmrs.module.reporting.report.service;

import org.apache.commons.io.FileUtils;
import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang.time.StopWatch;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
Expand All @@ -23,6 +24,9 @@
import org.openmrs.module.reporting.common.DateUtil;
import org.openmrs.module.reporting.common.ObjectUtil;
import org.openmrs.module.reporting.common.Timer;
import org.openmrs.module.reporting.dataset.DataSet;
import org.openmrs.module.reporting.dataset.DataSetColumn;
import org.openmrs.module.reporting.dataset.DataSetRow;
import org.openmrs.module.reporting.evaluation.EvaluationContext;
import org.openmrs.module.reporting.evaluation.EvaluationException;
import org.openmrs.module.reporting.report.Report;
Expand Down Expand Up @@ -52,6 +56,8 @@
import java.io.File;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.time.LocalDateTime;
import java.time.ZoneId;
import java.util.ArrayList;
import java.util.Calendar;
import java.util.Collection;
Expand Down Expand Up @@ -479,6 +485,26 @@ public Report runReport(ReportRequest request) {
renderReportDataToBytes = !(renderer instanceof InteractiveReportRenderer);
}

for (Map.Entry<String, DataSet> e : reportData.getDataSets().entrySet()) {
Copy link
Author

@icrc-psousa icrc-psousa Nov 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The dates retrieved from the resultSet are returned as LocalDateTime instead of Date. Not sure if this happens due to Java or jdbc driver version we're using or even if it may return some other type.

Here, I'm converting LocalDateTime to Date considering the timezone on the request so that field is treated as Date on the report. Not sure if it's the right place or way of doing it. Any advice is appreciated

Copy link
Member

Choose a reason for hiding this comment

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

Could we add the above to the code?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the ideal place to put this code. This feels more like something that should be handled at either the time of rendering or the time of evaluation, than something to apply like this as post-processing to an entire report within the service.

If we go with the 2nd option I propose above, which is to have clientTimezone able to be defined as a context value in the evaluation context and overridden by a parameter value in the report request, then the next step would be to create a generic method to render out column values that are of type LocalDate by taking into account the clientTimezone set in the Evaluation Context contextValues.

Copy link
Author

Choose a reason for hiding this comment

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

Regarding the place to do this conversion, I'm kind of divided on whether doing it at the time of evaluation or rendering.

I assume that, timezone conversion at the time of evaluation, for the SQL, would be done here but I don't know about the other evaluators and scenarios. Conversion should be handled on all evaluators other than SqlDataSetEvaluator, no?

If done at the time of rendering would I assume it would be done on the XlsReportRenderer for the Excel but, again, it would have to be done for all the renders, right?

I don't have a definitive opinion on this. @mseaton @ibacher could you provide more input on your thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Great points, and I agree that this is tricky (see my comment above). Probably this does not make the most sense.

DataSet dataset = e.getValue();
for (DataSetRow row : dataset ) {
for (Map.Entry<DataSetColumn, Object> dataSetColumn : row.getColumnValues().entrySet()) {
Object value = dataSetColumn.getValue();
if (value instanceof LocalDateTime) {
LocalDateTime localDateTime = (LocalDateTime) value;
if (StringUtils.isNotEmpty(request.getClientTimezone())) {
ZoneId clientZoneId = ZoneId.of(request.getClientTimezone());
localDateTime = ((LocalDateTime) value).atZone(ZoneId.systemDefault())
.withZoneSameInstant(clientZoneId)
.toLocalDateTime();
}
Date dateValue = Date.from(localDateTime.atZone(ZoneId.systemDefault()).toInstant());
dataSetColumn.setValue(dateValue);
}
}
}
}

if (renderReportDataToBytes) {
logReportMessage(request, "Generating Rendered Report....");
ReportRenderer renderer = request.getRenderingMode().getRenderer();
Expand Down Expand Up @@ -838,4 +864,4 @@ public void setReportData(ReportData reportData) {
this.reportData = reportData;
}
}
}
}
1 change: 1 addition & 0 deletions api/src/main/resources/ReportRequest.hbm.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
<property name="renderCompleteDatetime" type="java.util.Date" column="render_complete_datetime" />
<property name="minimumDaysToPreserve" type="int" column="minimum_days_to_preserve" />
<property name="description" type="string" column="description"/>
<property name="clientTimezone" type="string" column="client_timezone"/>

</class>

Expand Down
12 changes: 12 additions & 0 deletions api/src/main/resources/liquibase.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,18 @@
</addColumn>
</changeSet>

<changeSet id="reporting_report_request_5" author="psousa">
<preConditions onFail="MARK_RAN">
<not><columnExists tableName="reporting_report_request" columnName="client_timezone"/></not>
</preConditions>
<comment>
Add client_timezone property to ReportRequest
</comment>
<addColumn tableName="reporting_report_request">
<column name="client_timezone" type="varchar(255)"/>
</addColumn>
</changeSet>

<!-- REPORTING REPORT PROCESSOR SCHEMA -->

<changeSet id="reporting_report_processor_1" author="mseaton">
Expand Down
Loading