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

Timezone from client #303

Merged
merged 15 commits into from
Sep 2, 2021
Merged

Timezone from client #303

merged 15 commits into from
Sep 2, 2021

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented Jul 5, 2021

This is #4 re-opened in response to user report at https://forum.image.sc/t/omero-webclient-import-date-modify-timezone/54384

Current situation (before this PR):

  • Bug: time-stamps are formatted according to UTC e.g. d.getUTCHours() etc, so if your timezone is not UTC, the time-stamps will appear wrong (as reported above).
  • Most dates displayed in the webclient are in the right panel, formatted in a couple of different ways:
    • Creation dates (and acquisition dates) for P/D/I etc are formatted by Django e.g.{{ manager.dataset.getDate|date:"Y-m-d H:i:s" }} I believe these will be formatted according to Django's TIME_ZONE which is Europe/London, so anyone in a different time zone will see incorrect times.
    • Annotation date-times are set in JSON via datetime.isoformat() + "Z". Then formatted via JavaScript with OME.formatDate(). E.g. OME.formatDate("2021-07-09T13:29:32Z") -> "2021-07-09 13:29:32". This works for UK. I assume it's also broken in other time-zones. OME.formatDate() formats to UTC. *Note: Without the Z we get a formatted string showing an hour earlier since the date is not understood to be UK time-zone (British Summer Time), so formatting gives an hour earlier. OME.formatDate("2021-07-09T13:29:32") -> "2021-07-09 12:29:32".
    • Other dates are on e.g. search results page and history page are also formatted by Django

This PR:

  • We now use OME.formatDate() in JavaScript to format all dates in webclient (unless I've missed some)? This now formats date according to the browser's time zone.
  • P/D/I creation dates are first formatted in html with Django's built-in date formatting {{ manager.dataset.getDate|date:"r" }} (see docs which includes time-zone offset info, although the time-stamp is UTC data from the server). E.g. "2020-03-12T21:02:43+00:00" for Europe/London (default). These are then converted to browser time-zone with JavaScript OME.formatDate()
  • Annotation dates in JSON are also formatted in the JSON to include time-zone offset. This allows other clients to choose whether to use the time-zone offset in their handling of dates.

The intended behaviour for all time-stamps would be as described by @stephenogg below: "The display of the dates changes when I change the timezone setting on my computer, but NOT when I change the omero.web.time_zone setting. They all display in America/Edmonton time when the computer's timezone is set to America/Edmonton and in Europe/London time when the computer's timezone is set to Europe/London, irrespective of the omero.web.time_zone setting.

Pros / Cons to this PR:

Pros:

  • All Date formatting is handled by OME.formatDate() so it will be consistent across the webclient.
  • All times shown should match the wall clock. So if you add a comment, import an Image, create a Dataset etc, all the times shown will match current time and time-zone. Fixes the bug reported above.

Cons:

  • If you import an Image etc. at e.g. 9:00am on 24th August, then travel to a different timezone and look at the same Image, the import date-time will look different.
  • The history page doesn't take local time-zone into account when showing events on different dates, so it's possible to get a mismatch between the date that an event shows in the history calendar and the time-stamp shown for it in the centre panel (which is now time-zone corrected). To fix this, we could try to take local time-zone into account when displaying events by date?

NB: Whether you consider this is a bug-fix or a breaking change depends on whether you think the previous behaviour was a feature or a bug.

manics and others added 10 commits December 17, 2019 10:55
This reverts commit cefe7eca24999ca3e09e8d5070f06e95e5817f16.
Following some investigation into OMERO timezones it's apparent that when OMERO.server returns a datetime to OMERO.web it's unclear what timezone it's in. Therefore we shouldn't make assumptions about whether it should be converted into a different timezone for the client of not, so default to UTC.

Since this is now configurable an OMERO.web admin can change the default timezone for OMERO.web.
Requires that the dates from the backend API are in UTC
@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/omero-webclient-import-date-modify-timezone/54384/3

@will-moore
Copy link
Member Author

See comment from @stephenogg: #4 (comment)

This PR is causing a test failure:
https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-
integration/822/testReport/OmeroWeb.test.integration.test_tree/TestTree/test_marshal_images_dataset_date/

because the test expects this timestamp to be rendered to this string:

    utcAcq = 1444129810716
    acqDate = '2015-10-06T12:10:10Z'

which happens in tree.py

    d = datetime.fromtimestamp(old_div(time, 1000))
    return d.isoformat() + "Z"

This fails because the Django setting TIME_ZONE is now UTC instead of previous value of TIME_ZONE = 'Europe/London'.

@will-moore
Copy link
Member Author

Currently in released omero-web, the TIME_ZONE = 'Europe/London' setting is not configurable, so that anyone running omero-web in a different Time Zone would have an incorrect setting. Hence bugs as reported by Steve in the forum link above.

If most users already have this value "incorrect" (not matching their time zone) is there anything wrong with making it configurable, with the default being UTC? cc @chris-allan.

Opened a PR to fix the integration test failure: ome/openmicroscopy#6284

@chris-allan
Copy link
Member

I'm not adverse to having it be configurable though I do think the default should be left alone (ie. Europe/London).

@stephenogg
Copy link

Hi Will-
I appreciate all the work you're doing to get this sorted. Thanks!
I've been testing your most recent commit, e4c95e3 and there is different behaviour of the import date display of an image versus the display of creation dates of projects/datasets/annotations. I tested two variables with two settings. I set the omero.web.time_zone to either "Europe/London" or "America/Edmonton" and I changed the timezone on the computer that was running omero-web client to be "Europe/London" or "America/Edmonton". I tested all four combinations. The display of the import date of an image only depends on the omero.web.time_zone setting. It displays in Europe/London time when the the setting is Europe/London and displays in America/Edmonton time when the setting is America/Edmonton. It doesn't change when I change the timezone of the computer running the omero-web client.
The creation date of projects/images/annotations (I only tested tags) has the opposite behaviour. The display of the dates changes when I change the timezone setting on my computer, but NOT when I change the omero.web.time_zone setting. They all display in America/Edmonton time when the computer's timezone is set to America/Edmonton and in Europe/London time when the computer's timezone is set to Europe/London, irrespective of the omero.web.time_zone setting.

I think this is because in the omeroweb/webclient/templates/webclient/annotations/metadata_general.html file, the display dates of the projects/datasets and annotations are modified with the "data-isodate", while the import date display is from the
omeroweb/webclient/templates/webclient/annotations/includes/core_metatdata.html, and doesn't reference data-isodate. Somehow this influences the output. But I don't know why this changes the behaviour, or which behaviour is preferable, or how to change it so they behave consistently in the client.

@will-moore
Copy link
Member Author

Thanks @stephenogg. I fixed the image acquisition and import date display to be consistent with creation and annotation dates.
For me, testing in my default UK time zone, these are consistent with Insight:

Screenshot 2021-07-09 at 22 02 37

@will-moore
Copy link
Member Author

@stephenogg All time-stamps should now behave as you describe: "The display of the dates changes when I change the timezone setting on my computer, but NOT when I change the omero.web.time_zone setting. They all display in America/Edmonton time when the computer's timezone is set to America/Edmonton and in Europe/London time when the computer's timezone is set to Europe/London, irrespective of the omero.web.time_zone setting.

Is this OK, or do we need an option for clients to see time-stamps according to the omero.web.time_zone setting? I guess this could be useful. If so, then the obvious next question: what should the default be?

@stephenogg
Copy link

@will-moore Yes all the timestamps in the right hand-panel behave as noted above. I want to point out that timestamps that aren't in the right-hand panel, e.g., the dates displayed in the middle panel when a user clicks on the history tab, or dates displayed in the middle-panel when users are searching their data from the search function, haven't been updated to use the OME.formatDate(), and therefore display inconsistently with those dates in the right hand panel. Should that be addressed as part of this PR?

To have true multi-timezone support, I think you would have to assign a "home" timezone to users, maybe as part of their profile, and then images uploaded by each user would have that home timezone collected as part of the import process. Then, the timestamp, along with the user's timezone would absolutely determine when and in which timezone the image was imported. I'm not proposing this, just saying that you need more information if you want to be deterministic.

For me, having the display dates of images/datasets/objects match the time when I think I imported/created them is what's important. I think that your currently proposed behaviour (timestamp displayed in the timezone that the machine running the webclient is set to) is fine, because in most instances that will match the user's timezone. So there isn't a need to see timestamps according to the omero.web.time_zone setting.

Many instances will have one webclient installed along with the omero-server in a single-node configuration and users from all timezones will connect from their local machine to the same webclient running on a remote computer. That configuration is slightly different than what I've been testing, where I'm using a webclient running on my local computer to connect to a remote omero-server running on a different machine. So unlike the test situation I've created, the timezone setting of a user's computer won't affect the timestamp display. Only the timezone setting of the machine running the webclient, I think. But I don't know enough about where the timezone information is set/stored/used to know whether this would affect the timestamp display?

@will-moore
Copy link
Member Author

@stephenogg Thanks for the feedback. It might be a while before I do more on this - away for a couple of weeks now, but will look at the middle panel dates next...

@stephenogg
Copy link

@will-moore That didn't take long. I haven't tested extensively, but it seems that the date displays are all now consistent. Thanks.

@sbesson
Copy link
Member

sbesson commented Jul 29, 2021

Note the OMERO integration tests have been periodically failing since the inclusion of this change and will need to be adjusted accordingly. Temporarily excluding this PR until @will-moore comes back from annual leave.

@will-moore
Copy link
Member Author

@sbesson Apologies, ome/openmicroscopy#6284 has the test changes but was being excluded. I've just fixed the build so it should be included now (and the tests should pass with this PR included, so please don't exclude this now, Thanks).

@will-moore
Copy link
Member Author

will-moore commented Aug 12, 2021

@stephenogg Apologies for the delay. To answer your last question.
I believe the location of the OMERO.server and the location of the OMERO.web server don't have any effect on this, since all timestamps will be in UTC.
The TIME_ZONE setting on the web server (omero.web.time_zone) will affect the time-zone of the date in the JSON, but not it's absolute value. e.g. "2020-03-12T21:02:43+00:00" for "Europe/London" (default) will be "2020-03-12T22:02:43+01:00" after $ omero config set omero.web.time_zone "Europe/Paris", and so both will be rendered the same by the browser, depending ONLY on the browser's time zone.

@pwalczysko
Copy link
Member

@will-moore On merge-ci, I have, being on my Mac in Dundee, with timezone set as below, following experience

Screenshot 2021-08-19 at 15 28 41

Tested following timestamps in OMERO.web:

  • import time of freshly imported image
  • creation dates of freshly created P/D
  • creation dates of freshly create FilaAnn, KVP, Comment, Tag

All times checked matched my wall clock and my expectations.

How valuable would that be to change the timezone on my Mac and go with the tests again ? Is that necessary ?

@will-moore
Copy link
Member Author

Re: @sbesson "by local time of the browser, assuming two clients accessing the same data on the same server via Web from two different timezones, do you mean the dates should render differently"

Yes

@pwalczysko
Copy link
Member

pwalczysko commented Aug 20, 2021

Changed my timezone on the Mac to Edmonton/Canada. Tried to create a new D, and also add attachment and comment. All times are now in Edmonton/Canada in OMERO.web (the times shown yesterday in UK/Dundee have indeed changed to E/Canada as well), which matches the wall clock in Edmonton/Canada and the expectation as highlighted in #303 (comment) cc @sbesson . It kind of also matches my intutive expectation.

Screen Shot 2021-08-20 at 4 09 30 AM

@pwalczysko
Copy link
Member

I think I understand now the behaviour, the obvious question is
omero.web.time_zone setting seems to be expected to have no influence now. If it has still some influence on some timestamps, which ones ? If it does not have any influence, what is it good for ?

@will-moore
Copy link
Member Author

Partly it's been left in this PR since it was originally needed for different behaviour.
Also, because clients (webclient or other clients using the JSON data) can access the time-zone within the timestamps, it gives them the possibility of displaying time-stamps according to the configured time zone (even if we're not doing it in this PR).

@joshmoore
Copy link
Member

Reviewing the pros & cons:

  • "All Date formatting is handled by OME.formatDate() so it will be consistent across the webclient." 👍
  • "All times shown should match the wall clock. So if you add a comment" 👍
  • "then travel to a different timezone and look at the same Image, the import date-time will look different." Is the time zone shown and/or discoverable, i.e. just to be explicit?
  • "The history page doesn't take local time-zone into account when showing events on different dates" From my POV, that is pretty low-priority.

So the primary issue as far as I read the current state of this PR is if the server moves locations, right? I can see having that out-of-scope for this PR as long as the choices made here don't prevent us from solving the deeper issue (if we want to).

@joshmoore joshmoore closed this Aug 25, 2021
@joshmoore joshmoore reopened this Aug 25, 2021
@will-moore
Copy link
Member Author

I don't think that server moving locations should have any effect. The UTC time-stamps that it provides the webclient will be unchanged (as far as I understand). The omero-web time-zone will still be Europe/London.

But if you were to set the omero-web time-zone to something else, the time-stamps that are provided to the webclient will have a different time-zone info but the time-stamp will be the same.

For example, I just added a comment on my local server: "UK time in browser at 11:34 on 25th August"
With the default Europe/London timezone the time-stamp in the JSON data is:

"2021-08-25T11:34:46+01:00"

I guess the +01:00 is because we're now in Summer time.
That time-stamp renders correctly with OME.formatDate. You can try this in the browser Console:

OME.formatDate("2021-08-25T11:34:46+01:00") -> "2021-08-25 11:34:46"

When I switch the timezone on OMERO.web:

$ omero config set omero.web.time_zone "Pacific/Auckland"

Now I see the JSON data is:

"2021-08-25T22:34:46+12:00"

But that renders the same in my browser:

OME.formatDate("2021-08-25T22:34:46+12:00") -> "2021-08-25 11:34:46"

So, changing the omero-web time-zone has no effect on what users see in the webclient.

@ome ome deleted a comment from joshmoore Aug 26, 2021
@jburel
Copy link
Member

jburel commented Aug 27, 2021

Suggestion on 27/08 during standup:

  • prepare an rc for people to install and give feeback
    • contact people in US (Erick?), Europe, Japan
  • check what insight and web display. (import is still mainly done via insight for many people)
  • contact Susanne who has been working on a web app for acquisition metatada. Similar to what they have done with mde

@pwalczysko
Copy link
Member

Test insight:

Test 1: Timezone on Mac is UK, London

  1. Open a release insight and import an image to merge-ci. Check timestamp on created D and the import time of the image both in insight and web.

Result of Test1: the times in insight and web match. They also match the wall clock in Dundee.

Test 2: Timezone on Mac is Edmonton, Canada

  1. Open a release insight and import an image to merge ci. Check timestamp on created D and import time of the newly imported image but also on the image imported whilst Mac was on London timezone above.
  2. Create a new Tag in insight and attach it to the image. Check the timestamp of the newly created tag in insight.

Result of Test 2:

  1. Both the image and D previously imported in the "London timezone" as well as the new image imported in "Edmonton" timezone have matching timestamps: it matches between the two I/D pairs, it also matches between web and insight and it matches the wall clock in Edmonton, Canada.
  2. The tag created in insight - timestamp matches the D/I as above, and the wall clock in Edmonton Canada.

All the tests are as expected, all times match.
The only room for confusion here is the fact (not caused by this PR) that insight, unlike web, is showing creation dates of the Tags in the "Info" context menu. Web is showing the date and time of linking of the Tag. cc @dominikl But if you know this small detail, everything matches perfectly.

@will-moore
Copy link
Member Author

As discussed today, it would be good to get this into a release candidate.
Needs merging...

@sbesson sbesson merged commit 6e49e8b into ome:master Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants