plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
481 stars 653 forks source link

User time zone is not used when creating events #1923

Open jackahl opened 4 years ago

jackahl commented 4 years ago

Steps to reproduce: as admin

  1. add new event
  2. set some start and end time for the event
  3. save. --> the times for the event are of (except you are located in UTC±00:00).

I think this happens because the information Volto is send to the API always has +00:00 set as timezone (e.g. 2020-10-20T12:00:00+00:00). As the frontend then displays it modified for the users time zone its shifted. Responsible to set this should be the Datetimewidget Component afaik. I could not figure out how to inject the correct timezone there.

jackahl commented 4 years ago

I suspect 5d1e2c72b250e53d74d8a32d1c47254064f6661f might have something to do with it because reverting the datetimewidget before that seems to fix the issue. @razvanMiu could you take another look on this?

sneridagh commented 4 years ago

@razvanMiu can you elaborate why this was needed in the first place? I remember left it working well (in the first place).

/cc @tiberiuichim

tiberiuichim commented 4 years ago

@sneridagh @jackahl I haven't been involved with this change, maybe @avoinea but I'll flag my colleague about this

razvanMiu commented 4 years ago

Hi, I will look into it.

razvanMiu commented 4 years ago

@sneridagh @jackahl The idea was to show the start/end dates in the timezone specified in the config file and in the timezone-definitions file:

From what I see the DateTimeWidgets shows the date as expected. Ex: If the timezone is 'CET' (+2) and the event has:

start: "2020-10-20T09:00:00+00:00", 
end: "2020-10-20T10:00:00+00:00" 

I will see:

start: "2020-10-20T11:00:00+02:00", 
end: "2020-10-20T12:00:00+02:00" 

Screenshot_20201020_181704

Now..this block https://github.com/plone/volto/blob/master/src/components/theme/View/EventView.jsx which actually display the event in 'View mode' converts the start/end dates in local timezone and not in the timezone specified in the config file -> https://github.com/plone/volto/blob/master/src/components/theme/View/EventDatesInfo.jsx#L10 I hope I understood you correctly and if this is the issue then we either use the same approch that I used in DateTimeWidget to show the datetime in a specified timezone, or we just show the local datetime.

sneridagh commented 4 years ago

@razvanMiu I'm aware that this subject is complex, and easily could spend hours debating this, and we are not even a multi-planet species ;) yet...

The rationale behind the initial implementation is not to have a TZ specified in the server, and save all events "normalized" in UTC. Then, when it's displayed to the user, the client will adjust to the local TZ of the user.

There's no value for me if I live in Europe to see an event happening in the TZ where the server supposed to "live", as a user I want to know at a glance when it happens in my local time.

We discussed with @thet and @buchi years ago in the Beethoven sprint, and agreed on the UTC assumption.

Am I missing something?

avoinea commented 4 years ago

I'm not sure, but I think sending UTC to RestAPI while Plone backend is configure otherwise can lead to issues.

sneridagh commented 4 years ago

@razvanMiu Just tried 7.4.0 version (prior to the merge of this PR) and works as intended.

@avoinea it seems indeed that changing the Plone config for the server TZ gives unstable results, gitches I would say, when setting some timezones, I have no clue why. But in general, the outcome is the one intended, UTC.

@razvanMiu @avoinea @tiberiuichim We agreed that Volto will bypass the Plone TZ setting, and eventually this would go away in Plone. The agreed behavior is the one I reasoned in the previous comment.

@razvanMiu Should be an specific use case that you need since you sent the PR in the first place. Can we talk about that use case, and discuss how to proceed? Maybe we can use an opt-in config for use the local server TZ if required.

/cc @tisto

tisto commented 4 years ago

@razvanMiu @avoinea @tiberiuichim we have some time pressure here to fix this since this used to work in older Volto versions. We are open to doing a call today on short notice to discuss this if you have time.

razvanMiu commented 4 years ago

@sneridagh @tisto I will remove the changes that I made in 1 hour. Sorry for creating this issue.

tisto commented 4 years ago

@razvanMiu all good! There is really no need to apologize! If anyone is to blame for this it is me, since I reviewed and merged the PR without taking our previous discussions on that topic into account. There is no way you could have known about all this. :)

sneridagh commented 4 years ago

@razvanMiu as said it might be still a sensible and valid use case, but I would implement it as an opt-in. Feel free to rework the PR to support it!

razvanMiu commented 4 years ago

https://github.com/plone/volto/pull/1927