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

icrc-jfigueiredo · 2021-02-26T16:17:36Z

Ticket:

https://issues.openmrs.org/browse/HTML-755

What I have done:

Added new GP to switch on/off support for time zones.
Added new UiUtils methods for time zones support and dates formatting.
Added new date parsers to parse from ISO 8601 String to Date.

Add new GP handling timezones. Add new functions on uiutils about timezone and date formats from UiFramework Add new date parses to be able to: Parse from string with format ISO8601 to date Parse Date format to string with RFC3339 format

mks-d

@icrc-jfigueiredo a couple of comments, and importantly we need to address failing tests: https://travis-ci.org/github/openmrs/openmrs-module-uiframework/builds/761266556#L2266-L2273

api/src/main/java/org/openmrs/util/TimeZoneUtil.java

api/src/main/java/org/openmrs/ui/framework/UiUtils.java

mks-d · 2021-03-10T12:05:46Z

api/src/main/java/org/openmrs/ui/framework/UiUtils.java

+	/**
+	 * @return the value of the Global Propriety Handle Timezones 
+	 */
+	public boolean handleTimeZones(){


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

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.. :) )

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

That's decent @ibacher ....

mks-d · 2021-03-10T12:08:00Z

api/src/main/java/org/openmrs/ui/framework/FormatterImpl.java

@@ -42,6 +39,7 @@

    private MessageSource messageSource;
    private AdministrationService administrationService;
+    private UiUtils ui = new BasicUiUtils();


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

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

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

mks-d · 2021-03-10T12:09:03Z

pom.xml

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


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

I could not found Joda-Time in Core, that is why I added it here.

It is in platform 2.4 https://github.com/openmrs/openmrs-core/blob/2.4.0/pom.xml#L574-L578

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

mogoodrich · 2021-03-10T14:23:35Z

Thanks @mks-d for the review request... may take me a couple days to get to it, but I will review.

mogoodrich

Thanks @icrc-jfigueiredo @mks-d I added some initial thoughts... it still makes my head hurt, but this helps me understand things a little better. Are these all the changes we expect to make to the UI Framework module? It might make sense to get them all into the PR because that may make it easier to come up with a naming convention for the GP?

Right now that GP is only being used in one place, the "format" method. "Format" is intended generally used for formatting an element for display. And what we are kind of doing here, if the GP is set to "true", is saying "never format a date for display, keep it in RFC3339 format and let the client handle the formatting, so it can use it's own time zone" (again, not the right name for a GP, but hopefully will help us get closer to a better name... :) )

Hope that's helpful!

mogoodrich · 2021-03-12T23:03:45Z

api/src/main/java/org/openmrs/ui/framework/FormatterImpl.java

@@ -42,6 +39,7 @@

    private MessageSource messageSource;
    private AdministrationService administrationService;
+    private UiUtils ui = new BasicUiUtils();


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

mogoodrich · 2021-03-12T23:08:00Z

api/src/main/java/org/openmrs/ui/framework/FormatterImpl.java

@@ -113,6 +119,9 @@ private boolean wholeNumber(Number n) {

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


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?

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

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

mogoodrich · 2021-03-12T23:09:09Z

api/src/main/java/org/openmrs/ui/framework/UiUtils.java

+	/**
+	 * @return the value of the Global Propriety Handle Timezones 
+	 */
+	public boolean handleTimeZones(){


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.. :) )

api/src/main/java/org/openmrs/ui/framework/FormatterImpl.java

icrc-jfigueiredo · 2021-03-15T09:29:56Z

@mogoodrich the public boolean handleTimeZones(){ it's not only for formatting dates, in the case of the UiFramework it is, but this will be used in other modules (coreApps, HFE-UI, Attachments ...) so they know we are using timezones (to know the GP value). Example here

mogoodrich · 2021-03-15T20:36:22Z

@mogoodrich the public boolean handleTimeZones(){ it's not only for formatting dates, in the case of the UiFramework it is, but this will be used in other modules (coreApps, HFE-UI, Attachments ...) so they know we are using timezones (to know the GP value). Example here

Then maybe something like "handleTimeZonesOnClientSide" or "convertToClientTimeZone"? Not sure I love those either?

ibacher · 2021-03-15T20:55:12Z

api/src/main/java/org/openmrs/util/TimeZoneUtil.java

+	 * @return The date formated as RFC 3339.
+	 */
+	public static String toRFC3339(Date date) {
+		return ISODateTimeFormat.dateTime().print(new DateTime(date.getTime(), UTC));


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

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.

api/src/main/java/org/openmrs/util/TimeZoneUtil.java

icrc-jfigueiredo · 2021-03-16T08:39:03Z

@mogoodrich the public boolean handleTimeZones(){ it's not only for formatting dates, in the case of the UiFramework it is, but this will be used in other modules (coreApps, HFE-UI, Attachments ...) so they know we are using timezones (to know the GP value). Example here

Then maybe something like "handleTimeZonesOnClientSide" or "convertToClientTimeZone"? Not sure I love those either?

I think 'handleTimeZonesOnClientSide' it's a good name

Change function to format date to use fromISO8601

dkayiwa · 2021-03-16T16:25:42Z

api/src/test/java/org/openmrs/ui/framework/FormatterImplTest.java

@@ -150,6 +151,7 @@ public void testFormattingADateWithNoTime() throws Exception {
    @Test
    public void testFormattingADateWithATime() throws Exception {


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

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

Fair enough 😊

dkayiwa · 2021-03-16T16:27:21Z

api/src/main/java/org/openmrs/util/TimeZoneUtil.java

+	 * @return The Date object.
+	 * @Throws IllegalArgumentException – if string parameter does not conform to lexical value space
+	 */
+	public static Date fromISO8601(String s) throws


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

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

Looks good.

dkayiwa · 2021-03-16T16:33:09Z

api/src/main/java/org/openmrs/ui/framework/UiUtils.java

@@ -586,4 +586,30 @@ public Locale getLocale() {
 	public void setLocale(Locale locale) {
 		this.locale = locale;
 	}
+
+	/**


It would be great to add the method description.

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

dkayiwa · 2021-03-16T21:30:12Z

api/src/main/java/org/openmrs/ui/framework/FormatterImpl.java

@@ -113,6 +114,10 @@ private boolean wholeNumber(Number n) {

    private String format(Date d, Locale locale) {
        DateFormat df;
+        if(Boolean.parseBoolean(


How about using BooleanUtils.toBoolean() to also accept on, yes, t, y as true values?

It makes a lot of sense, I changed it to use BooleanUtils.toBoolean() as you can see here and here.

…eBoolean()

icrc-jfigueiredo · 2021-03-25T08:48:45Z

Hello, I created a NEW PR for UiFramework, with a User Propriety to save the client timezone, so the server can send the date formatted on client timezone, instead of doing that on the client side.

icrc-jfigueiredo and others added 2 commits February 27, 2021 03:16

HTML-755 Timezones, change the GP value to true

5ab772c

mks-d changed the title ~~HTML-755 Timezones, add new GP and update UiUtils with date formats~~ HTML-755: UIFR to expose time zones support API. Mar 10, 2021

mks-d requested review from mks-d and mogoodrich March 10, 2021 12:10

mks-d requested changes Mar 10, 2021

View reviewed changes

JFFigueiredo added 2 commits March 10, 2021 14:33

HTML-755 Timezones, change function that convert iso dates

b80befe

HTML-755 Fix tests

ff0a922

mogoodrich reviewed Mar 12, 2021

View reviewed changes

icrc-jfigueiredo added 2 commits March 15, 2021 13:05

HTML-755 Remove wildcard import

f57a83c

HTML-755 Remove wildcard import

6e2421a

ibacher reviewed Mar 15, 2021

View reviewed changes

api/src/main/java/org/openmrs/util/TimeZoneUtil.java Outdated Show resolved Hide resolved

mks-d requested a review from dkayiwa March 16, 2021 10:39

JFFigueiredo added 3 commits March 16, 2021 07:44

HTML-755 Remove empty spaces

d6fd3f0

HTML-755 Remove UiUtils from Formatter

1b7a151

Change function to format date to use fromISO8601

HTML-755 Fix tests

257037b

dkayiwa reviewed Mar 16, 2021

View reviewed changes

HTML-755 Start using BooleanUtils.toBoolean() instead of Boolean.pars…

76f0b88

…eBoolean()

icrc-jfigueiredo closed this Apr 7, 2021

		@@ -150,6 +151,7 @@ public void testFormattingADateWithNoTime() throws Exception {
		@Test
		public void testFormattingADateWithATime() throws Exception {

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

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

Conversation

icrc-jfigueiredo commented Feb 26, 2021 • edited by mks-d Loading

Ticket:

What I have done:

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

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

mogoodrich commented Mar 10, 2021

mogoodrich left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icrc-jfigueiredo Mar 15, 2021 • edited by mks-d Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icrc-jfigueiredo commented Mar 15, 2021

mogoodrich commented Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icrc-jfigueiredo commented Mar 16, 2021

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

icrc-jfigueiredo Mar 17, 2021 • 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 Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

dkayiwa Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icrc-jfigueiredo commented Mar 25, 2021

icrc-jfigueiredo commented Feb 26, 2021 •

edited by mks-d

Loading

mogoodrich left a comment •

edited

Loading

icrc-jfigueiredo Mar 15, 2021 •

edited by mks-d

Loading

mogoodrich commented Mar 15, 2021 •

edited

Loading

icrc-jfigueiredo Mar 17, 2021 •

edited

Loading

dkayiwa Mar 17, 2021 •

edited

Loading

dkayiwa Mar 16, 2021 •

edited

Loading