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

HTML-755: UIFR to expose time zones support API. #65

Closed
wants to merge 10 commits into from
23 changes: 18 additions & 5 deletions api/src/main/java/org/openmrs/ui/framework/FormatterImpl.java
Original file line number Diff line number Diff line change
@@ -1,18 +1,17 @@
package org.openmrs.ui.framework;

import java.text.DateFormat;
import java.util.ArrayList;
import java.util.Calendar;
import java.util.Date;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util. Map;
import java.util. Calendar;
import java.util. List;
import java.util. ArrayList;

import org.apache.commons.beanutils.BeanUtils;
import org.apache.commons.beanutils.MethodUtils;
import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang.time.DateFormatUtils;
import org.openmrs.Concept;
import org.openmrs.ConceptName;
import org.openmrs.ConceptNumeric;
Expand All @@ -34,6 +33,8 @@
import org.openmrs.ui.framework.formatter.FormatterService;
import org.springframework.context.MessageSource;

import static org.openmrs.util.TimeZoneUtil.toRFC3339;

/**
* Contains default formatting for most OpenMRS classes, which can be override with {@link FormatterFactory} instances.
* Do not construct this class directly, but rather use {@link FormatterService#getFormatter()}.
Expand All @@ -42,6 +43,15 @@ public class FormatterImpl implements Formatter {

private MessageSource messageSource;
private AdministrationService administrationService;
private UiUtils ui = new BasicUiUtils();
Copy link
Member

Choose a reason for hiding this comment

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

@mogoodrich is it ok to access UiUtils from here and in that way?
I would think that this was not the intent of this class, but maybe it's ok and since it works...

Copy link
Member

Choose a reason for hiding this comment

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

In general, I'm okay with it, but isn't a bit weird/recursive in this case, because the UiUtils has the formatter wired into it? ie the format method below is usually accessed via "uiUtils.format()" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can always remove that UiUtils and get the GP directly... Instead of using ui.handleTimeZones().


public UiUtils getUi() {
return ui;
}

public void setUi(UiUtils ui) {
this.ui = ui;
}

/**
* Map from fully-qualified classname, to the formatter to use for this class
Expand Down Expand Up @@ -113,6 +123,9 @@ private boolean wholeNumber(Number n) {

private String format(Date d, Locale locale) {
DateFormat df;
if(ui.handleTimeZones()){
return (toRFC3339(d));
}
Copy link
Member

Choose a reason for hiding this comment

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

The "format" method is primarily about formatting something for display... are we saying when "handleTimeZones" is enabled we never want to format for display on the server side, but always render in RFC3339 format?

Copy link
Contributor Author

@icrc-jfigueiredo icrc-jfigueiredo Mar 15, 2021

Choose a reason for hiding this comment

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

Exactly that, because, if timezones are enabled, we always need to format on the client side, with client timezone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the UiUtils there and now we are getting the GP directly here

if (hasTimeComponent(d)) {
df = UiFrameworkUtil.getDateTimeFormat(administrationService, locale);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ public class UiFrameworkConstants {
public static final String GP_FORMATTER_TIME_FORMAT = "uiframework.formatter.timeFormat";
public static final String GP_FORMATTER_JS_DATETIME_FORMAT = "uiframework.formatter.JSdateAndTimeFormat";
public static final String GP_FORMATTER_JS_DATE_FORMAT = "uiframework.formatter.JSdateFormat";

public final static String GP_HANDLE_TIMEZONES = "uiframework.handleTimezones";

public static final String MAP_RESOURCE_EXTENSION_POINT_ID = "org.openmrs.ui.framework.mapResource";


Expand Down
26 changes: 26 additions & 0 deletions api/src/main/java/org/openmrs/ui/framework/UiUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -586,4 +586,30 @@ public Locale getLocale() {
public void setLocale(Locale locale) {
this.locale = locale;
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to add the method description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but that is something that was create in 2013, I added a little description here

Copy link
Member

@dkayiwa dkayiwa Mar 17, 2021

Choose a reason for hiding this comment

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

oh, ok.

* @return the value of the Global Propriety Handle Timezones
*/
icrc-jfigueiredo marked this conversation as resolved.
Show resolved Hide resolved
public boolean handleTimeZones(){
Copy link
Member

Choose a reason for hiding this comment

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

@mogoodrich are you ok with this name?
Or perhaps hasTimeZonesSupport() or timeZonesSupport()?

Would be good to know early because this will be referred to in many modules.

@ibacher you're great at this kind of thing ;-) Your ideas would be more than welcome.

Copy link
Member

Choose a reason for hiding this comment

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

I know we've talked about this ad nauseum but I always forget the exact details... "handle" time zones feels a bit vague... I'd be in favor of something more specific... per my above comment are we says "never format timezones for display on the server side, force RFC3339 format" (not my suggestion for a GP.. :) )

Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like: sendDatetimeAsUTC? Not my favourite phrasing, but accurate.

Copy link
Member

Choose a reason for hiding this comment

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

That's decent @ibacher ....

return Boolean.parseBoolean(
Context.getAdministrationService().getGlobalProperty(UiFrameworkConstants.GP_HANDLE_TIMEZONES));
}

public String getJSDatetimeFormat(){
return Context.getAdministrationService().getGlobalProperty(UiFrameworkConstants.GP_FORMATTER_JS_DATETIME_FORMAT);
}
public String getJSDateFormat(){
return Context.getAdministrationService().getGlobalProperty(UiFrameworkConstants.GP_FORMATTER_JS_DATE_FORMAT);
}
public String getDatetimeFormat(){
return Context.getAdministrationService().getGlobalProperty(UiFrameworkConstants.GP_FORMATTER_DATETIME_FORMAT);
}
public String getDateFormat(){
return Context.getAdministrationService().getGlobalProperty(UiFrameworkConstants.GP_FORMATTER_DATE_FORMAT);
}

public String getTimeFormat(){
return Context.getAdministrationService().getGlobalProperty(UiFrameworkConstants.GP_FORMATTER_TIME_FORMAT);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import java.util.Date;

import org.openmrs.ui.framework.WebConstants;
import org.openmrs.util.TimeZoneUtil;
import org.springframework.core.convert.ConversionFailedException;
import org.springframework.core.convert.TypeDescriptor;
import org.springframework.core.convert.converter.Converter;
Expand All @@ -17,7 +18,13 @@ public Date convert(String ymdhmsms) {
if (ymdhmsms.isEmpty()) {
return null;
}


//try to parse date with format ISO 8601
try {
return TimeZoneUtil.fromISO8601(ymdhmsms);
}
catch (Exception ex) {}

try {
SimpleDateFormat sdf = new SimpleDateFormat(WebConstants.DATE_FORMAT_TIMESTAMP);
return sdf.parse(ymdhmsms);
Expand Down
63 changes: 63 additions & 0 deletions api/src/main/java/org/openmrs/util/TimeZoneUtil.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/**
* This Source Code Form is subject to the terms of the Mozilla Public License,
* v. 2.0. If a copy of the MPL was not distributed with this file, You can
* obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under
* the terms of the Healthcare Disclaimer located at http://openmrs.org/license.
*
* Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS
* graphic logo is a trademark of OpenMRS Inc.
*/
package org.openmrs.util;

import org.joda.time.DateTime;
import org.joda.time.format.ISODateTimeFormat;

import java.util.Calendar;
import java.util.Date;

import static org.joda.time.DateTimeZone.UTC;

/**
* Helps provide tools to support recommended OpenMRS time zones conventions.
*
* @see https://wiki.openmrs.org/display/docs/Time+Zones+Conventions
*/
public class TimeZoneUtil {

/**
* Formats a date as its RFC 3339 string representation.
*
* @param date The date.
* @return The date formated as RFC 3339.
*/
public static String toRFC3339(Date date) {
return ISODateTimeFormat.dateTime().print(new DateTime(date.getTime(), UTC));
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this just treat the date as if it were a UTC date rather than a server time-zone specific date?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it convert to UTC and use the ISO8601 format, that in this case its the same as RFC3339, which is what client side is expecting.

}

/**
* Gets the Calendar instance for the date set in UTC. This always returns a GregorianCalendar
* subclass.
*
* @param date The date.
* @return The GregorianCalendar set in UTC for the date.
*/
public static Calendar toUTCCalendar(Date date) {
return new DateTime(date.getTime(), UTC).toGregorianCalendar();
}


/**
* Get a Date out of its ISO 8601 string representation.
*
* @param s A date formatted as ISO 8601.
* @return The Date object.
* @Throws IllegalArgumentException – if string parameter does not conform to lexical value space
*/
public static Date fromISO8601(String s) throws
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind using a more descriptive parameter name than s?

Copy link
Contributor Author

@icrc-jfigueiredo icrc-jfigueiredo Mar 17, 2021

Choose a reason for hiding this comment

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

I change it to public static Date fromISO8601(String isoDateString) throws here, is this ok to you?

Copy link
Member

Choose a reason for hiding this comment

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

Looks good.

IllegalArgumentException {
return javax.xml.bind.DatatypeConverter.parseDateTime(s).getTime();
}

}


Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,14 @@ public class FormatterImplTest {

AdministrationService administrationService;
MockMessageSource messageSource;

UiUtils ui;
FormatterImpl formatter;

@Before
public void setUp() {
administrationService = mock(AdministrationService.class);
messageSource = new MockMessageSource();

ui = mock(UiUtils.class);
formatter = new FormatterImpl(messageSource, administrationService);
}

Expand Down Expand Up @@ -139,6 +139,8 @@ public void testFormattingRoleWithOverriddenMetadataName() throws Exception {
@Test
public void testFormattingADateWithNoTime() throws Exception {
when(administrationService.getGlobalProperty(UiFrameworkConstants.GP_FORMATTER_DATE_FORMAT, "dd.MMM.yyyy")).thenReturn("dd.MMM.yyyy");
formatter.setUi(ui);
when(ui.handleTimeZones()).thenReturn(false);

Locale locale = Locale.ENGLISH;
Date date = new SimpleDateFormat("yyyy-MM-dd").parse("2003-02-01");
Expand All @@ -150,6 +152,8 @@ public void testFormattingADateWithNoTime() throws Exception {
@Test
public void testFormattingADateWithATime() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about the test method name, what is this test about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is not something we introduce, its an old test, that see if the date is well formatted by FormatterImpl.java

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough 😊

when(administrationService.getGlobalProperty(UiFrameworkConstants.GP_FORMATTER_DATETIME_FORMAT, "dd.MMM.yyyy, HH:mm:ss")).thenReturn("dd.MMM.yyyy, HH:mm:ss");
formatter.setUi(ui);
when(ui.handleTimeZones()).thenReturn(false);

Locale locale = Locale.ENGLISH;
Date date = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").parse("2003-02-01 14:25:07.123");
Expand Down
7 changes: 7 additions & 0 deletions omod/src/main/resources/config.xml
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,13 @@
Format used by Javascript functions for dates that have a time component
</description>
</globalProperty>

<globalProperty>
<property>uiframework.handleTimezones</property>
<defaultValue>true</defaultValue>
<description>When set to true dates are sent from the server to the client as UTC dates and parsed from the client to the server as holding the client timezone information.
</description>
</globalProperty>

<servlet>
<servlet-name>resource</servlet-name>
Expand Down
10 changes: 10 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,16 @@
<version>3.19.0-GA</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>joda-time</groupId>
<artifactId>joda-time</artifactId>
<version>2.9.2</version>
</dependency>
<dependency>
<groupId>org.joda</groupId>
<artifactId>joda-convert</artifactId>
<version>1.2</version>
</dependency>
Comment on lines +64 to +73
Copy link
Member

Choose a reason for hiding this comment

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

@icrc-jfigueiredo you sure you need this, Joda-Time is normally coming with Core.

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 could not found Joda-Time in Core, that is why I added it here.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are still using 2.3, once we update it to 2.4 I can remove it.

</dependencies>

<dependencyManagement>
Expand Down