fix: update timestamps [DHIS2-14516] #452
Conversation
|
🚀 Deployed on https://pr-452--dhis2-scheduler.netlify.app |
package.json
Outdated
| "test": "TZ=Etc/UTC d2-app-scripts test --coverage", | ||
| "test:watch": "TZ=Etc/UTC d2-app-scripts test --watch", |
There was a problem hiding this comment.
I would just mock this in the test instead of globally. Otherwise it's not clear why this is being set. Plus it means that in the test you have to search for where this is being set.
There was a problem hiding this comment.
See how we do it here for example: https://github.com/dhis2/ui/blob/master/components/select/src/multi-select/__tests__/selection-list.test.js#L10-L29
There was a problem hiding this comment.
The time zone is being mocked as part of the mock of the useTimeZoneConversion hook, so we don't actually need to set it generally for the test (I think this is left over from some other approach I did not use). I've just removed this.
There was a problem hiding this comment.
Maybe I missed this discussion, but why did we change from relative to absolute time? Relative time seems simpler to parse for a user to me. We could have the actual time in a tooltip as the more detailed option.
edit: ok, I see I did miss some discussion, and that Joe and Jan B. ok'ed this. Tbh., I would find the inverse more user friendly, but if this is not up for discussion at this stage I'll defer to the existing consensus. It just doesn't seem very user friendly to me as the information that seems easier to me to understand (the relative time) is presented as the secondary option.
src/components/JobTable/NextRun.js
Outdated
| /** | ||
| * The recommendation is to run dhis2 on a server set to UTC time. | ||
| * In that case this timestamp is also UTC. If those recommendations | ||
| * weren't followed the time could be off, but there's nothing | ||
| * we can do to detect that. | ||
| */ | ||
| const nextRun = moment.utc(nextExecutionTime) | ||
| const nextRunIsInPast = nextRun.isSameOrBefore(now, 'minute') | ||
| const nextRun = fromServerDate(nextExecutionTime) | ||
| const nextRunIsInPast = nextRun.getTime() <= now |
There was a problem hiding this comment.
Is this comment still accurate now that we're converting between timezones? It should correct for a server set in a different TZ right?
There was a problem hiding this comment.
What I'm saying is, this could be removed, right?
If those recommendations
* weren't followed the time could be off, but there's nothing
* we can do to detect that.
Since we're now detecting TZ differences and correcting for them. Or not? (And the rest of the comment reworded)
There was a problem hiding this comment.
Yes; going to remove 👍 (when I get around to updating the PR)
There was a problem hiding this comment.
Updated the comment to say Adjust for client/sever time zone difference. (rather than say we cannot detect it)
## [100.2.10](v100.2.9...v100.2.10) (2023-03-01) ### Bug Fixes * adjust for server timezone [DHIS2-14516] ([8ce5e40](8ce5e40)) * clean up tests [DHIS2-14516] ([9b82265](9b82265)) * retrigger CI ([466c00e](466c00e)) * update from PR comments ([6a04f3f](6a04f3f)) * update timestamps [DHIS2-14516] ([#452](#452)) ([b8564e1](b8564e1))
|
🎉 This PR is included in version 100.2.10 🎉 The release is available on: Your semantic-release bot 📦🚀 |
See DHIS2-14516
before
the time was displayed as a relative time, but we assumed the timestamps from the server were in UTC. So for example, if my server timezone is in Europe/Oslo and it is currently 19:41 and my next job is scheduled at 45 minutes past the hour (19:45), this would display as
in 1 hourbecause our code (+ moment) adjusted the timestamp for the assumed 1 hour time difference between CET+1 and UTC timezonesafter
time stamps are displayed as absolute time (
YYYY-MM-DD HH:MM:SS (server timezone)) with the relative time in a tooltip (this has been okayed by Joe and Jan B., see ticket)