(fix): format observation datetime to accurate local time to prevent inaccurate timestamps

[amosmachora]

Requirements

• This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
• My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
• My work includes tests or is validated by existing tests.

Summary

This PR formats the test timestamp in the results panel using the openmrs framework formatDatetime function that accurately handles the users locale. Since the date we had was already formatted to a certain format we first convert it to an ISO string (which was the original format) then format it correctly to the users local time removing the inconsistency.

Screenshots

Results

Visits

Related Issue

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

Other

[donaldkibet]

Why not use the framework formatDatetime here?

[amosmachora]

I did not want to break the existing functionality. In my investigation using formatDatetime returns dates like Today 06:50 am which was not my intention. I wanted to return a string like this 2024-09-04 06:23:23.0 so that wherever the value is being used it doesn't break anything. In short I wanted to return the original date but formatted correctly,

[ojwanganto]

I vote for consistency across the system and I vote for the formatters provided by the framework. Is there a strong reason not to go with the formatted date from the framework's function?

[amosmachora]

Yes @ojwanganto that date object at that point is not for display but rather to make sure it is in the correct timezone and that approach is what made it possible. using formatDateTime or any other format function from the framework did not quite fit the use case.

I am open to suggestions though.

[ibacher]

I think it's fine to operate on time objects, but please use dayjs(), which supports everything you're trying to do here in a much cleaner way and is already part of the framework.

[donaldkibet]

Suggested change

	const formatObsDatetime = (obsDatetime: string) => {
	const isoFormattedString = obsDatetime.replace(' ', 'T').replace('.0', '.000').concat('Z');
	return isoFormattedString;
	};
	const formatObsDatetime = (obsDatetime: string) => {
	return dayjs(obsDatetime).toISOString();
	};

dayjs already provides this conversion to ISO format.

[amosmachora]

The obsDatetime parameter is weird. It's an already formatted date string like this: 2025-01-22 05:28:00.0 Following your suggestion rolls back the date by 3 hours (probably because I am on EAT).

obsDatetime - 2025-01-22 05:28:00.0
isoFormattedString - 2025-01-22T05:28:00.000Z 
formattedWithDayJS - 2025-01-22T02:28:00.000Z

thanks for highlighting though I have seen a use case for it on the tooltip for accuracy purposes

[ibacher]

So, yes, because dayjs interprets it as a local timestamp (which is what that format claims it is). Instead, you're adding an adjustment so this is treated as if it were in UTC, which is not something we should be deciding here, but on the backend.

[mogoodrich]

Agree with @ibacher ... I'm forgetting, when does the server return something like "2025-01-22 05:28:00.0" (ie without specifying time zone).

[amosmachora]

@mogoodrich highly likely that it does not. This is a modification that is most certainly done at the front end, somewhere up the "callstack".

[mogoodrich]

We should certainly consider fixing that @amosmachora @denniskigen , where does the obstree resource live?

[denniskigen]

I'm referring to this guy in the REST-WS module. And this is not in response to Amos' comments from above, but rather about this ticket in general. Per Ian, this specific issue is just another pointer to the known wider issue of how dates and time (specifically timezones) are handled in the backend.

[mogoodrich]

thanks @denniskigen !

@ibacher @dkayiwa @mseaton I'm thinking we want to fix this? specifically, when generating the value_datetime and obs_datetime here:

https://github.com/openmrs/openmrs-module-webservices.rest/blob/master/omod-1.9/src/main/java/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_9/ObsTreeResource1_9.java#L153

... just format in the standard ISO string format instead?

[ibacher]

@mogoodrich Yep. We do need to make allowance for, e.g., implementations that have been storing time in UTC rather than local time. But, yeah, ideally, all conversations through the API are done in ISO timestamps and the ambiguity should mostly go away.

[mogoodrich]

Thanks @ibacher ... isn't that even more of a reason to include an ISO string here? Stripping off the timezone seems to assume that the server time zone is the facility timezone, and we just want to display that time to the user, regardless of what timezone their client is in. (We've done that in the past with things like this: https://github.com/openmrs/openmrs-module-uicommons/blob/master/omod/src/main/webapp/resources/scripts/filters/serverDate.js but this has become problematic when the server is in UTC or some timezone that don't correspond to the clinic timezone)

[ibacher]

I think it's fine to operate on time objects, but please use dayjs(), which supports everything you're trying to do here in a much cleaner way and is already part of the framework.

[ibacher]

So, yes, because dayjs interprets it as a local timestamp (which is what that format claims it is). Instead, you're adding an adjustment so this is treated as if it were in UTC, which is not something we should be deciding here, but on the backend.

[denniskigen]

@amosmachora did you see Ian's latest comments on the ticket?

[amosmachora]

@amosmachora did you see Ian's latest comments on the ticket?

Yes I did Dennis, not gonna lie worked out perfectly for me because I had no clue how to rewrite the code with the dayjs implementation that Ian was suggesting : )