Set timezone for creation and modification dates of comments

[instification]

Currently comment creation_date and modification_date are stored as timezone naive objects which cause them to display incorrectly in most places.

This PR sets the timezone when creating/modifying comments.

There is an upgrade step to walk existing comments and apply a timezone.

https://github.com/plone/plone.restapi/pull/1520 is also required in order for the full plone tests to pass.

[mister-roboto]

@instification thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

[instification]

@jenkins-plone-ort please run jobs

[davisagli]

@instification Thanks for working on this.

I'm slightly concerned about the backwards compatibility of solving it this way, since the dates have been stored this way for a long time and there are likely custom views and such in the wild that assume the creation_date and modification_date are timezone-naive.

Can you be more specific about where you noticed they're being displayed incorrectly? I'm wondering how hard it would be to fix them at display / serialization time rather than changing what is stored.

[instification]

@davisagli thanks for looking at this.

I probably should have checked against 6.0.0b3 as it does appear to be dealt with in the classic view there. In 6.0.0b1 the classic view displays the date in utc regardless of local settings.

In latest volto however it is still an issue. This is what the comment looks like immediately after saving (my timezone is currently utc+1).

I agree it might be problematic for custom views etc but it's also an argument to say that it's not great to force developers of volto and those building new solutions to have to to deal with timezone naive dates/times. It's further exacerbated by the fact that the catalogued values are stored with a timezone.

The rest api gives naive dates as well:

{
  "@id": "http://localhost:8080/Plone/++api++/test/@comments", 
  "items": [
    {
      "@id": "http://localhost:8080/Plone/test/@comments/1666168843608935", 
      ...
      "creation_date": "2022-10-19T08:40:43", 
      ...
      "modification_date": "2022-10-19T08:40:43", 
      ...
    }
  ]
}

Perhaps the best solution would be to make a major breaking release just for plone 6?

The argument being that it's still in beta so a change like this is more expected. I'm happy to write some documentation here and in any upgrade guide/release for plone 6 explaining the change and the impact on existing code.

cc @jensens @avoinea and @tisto for opinions?

[instification]

@pnicolli in your talk at the conference you mentioned using p.a.discussion to handle messaging in volto. Did the timezone issue come up for you there? If so, how did you resolve it?

[jensens]

Currently comment creation_date and modification_date are stored as timezone naive objects which cause them to display incorrectly in most places.

This is wrong and need a fix. But storing as UTC by default is wrong too. The correct way is to get the configured portal timezone. There is a utility in plone.app.event.base https://github.com/plone/plone.app.event/blob/master/plone/app/event/base.py#L433 you can use.

It's further exacerbated by the fact that the catalogued values are stored with a timezone.

Because you can not mix naive and non-naive datetimes in comparisons. Thus, if mixed the index can not be used for sorting. I would guess this was at some point in time a hack to fix the problem on index time.

So I think the indexer as it is here https://github.com/plone/plone.app.discussion/blob/dd0255fd5db6662a1b1b4cb7046785038d0d6b71/plone/app/discussion/catalog.py#L103-L113 is not needed anymore. AFAIK the index does not care if it gets a Zope or a Python datetime (it can can deal with both).

The other datetime related indexers and its attributes and further how the attributes are constructed would possibly need a check too.

[davisagli]

@instification Okay, it's helpful to have the REST API / Volto use case in mind specifically.

I looked at some other content (i.e. not comments) and in the REST API it has created and modified with a timezone offset. My first inclination would be to fix the REST API serialization of creation_date and modification_date on comments to include this offset, but not change what is stored on the backend. Then it's Volto's responsibility to convert from the timezone-aware datetime in the REST API response to the browser's local time (I haven't checked whether it already does that or not).

I'm not entirely opposed to the idea of starting to store timezone-aware datetimes in the backend as well; it just feels like a riskier approach especially as we're hopefully getting close to final release candidates of Plone 6.

But storing as UTC by default is wrong too. The correct way is to get the configured portal timezone.

@jensens I don't think it matters what timezone is stored as long as we know what timezone it is; it's going to need to be converted sometimes anyway, depending on the timezone of a specific user.

[jensens]

I don't think it matters what timezone is stored as long as we know what timezone it is; it's going to need to be converted sometimes anyway, depending on the timezone of a specific user.

For creation/modification/effective/expired it does not matter much, but it does not hurt and adds additional information about the intend of the author. It saves recalculation processing on display, where one wants to see it in the configured timezone anyway or - if we leverage this in future - in a users configured timezone (which would need calculations again anyway). But in fact UTC is just one timezone. I would never take the intended timezone an calculate it to UTC. For what? It is pointless. Always store the intended timezone.

But (out of scope here, but keep this in mind) for start/end timezone does matter (hint: recurring dates over different DST borders).

[davisagli]

@jensens Yeah, okay. Point taken.

[jensens]

REST API it has created and modified with a timezone offset.

I hope it stores a named timezone and not an offset. An Offset is not a timezone. I.e with an offset one can not do calculations of recurring event over daylight saving borders. It it really stores an offset this is just wrong and would need to be fixed.

[davisagli]

I hope it stores a named timezone and not an offset. An Offset is not a timezone. I.e with an offset one can not do calculations of recurring event over daylight saving borders. It it really stores an offset this is just wrong and would need to be fixed.

I meant that it's serialized with e.g. "+00:00" at the end. For Dexterity content I believe the creation date and modification date are stored internally as a Zope DateTime. I think that means it's based on a named timezone but I honestly can't remember...

[instification]

So can we get a consensus?

I would like to push for putting timezones in over just masking it in serialisation/deserialisation, but I'll obviously go consensus. I think fixing it in the restapi output will be enough to make it display correctly in Volto.

If it helps to make the case (whataboutery isn't the best argument but still), I noticed that Pillow made a breaking change in 9.2 by converting to camelcase. This has caused us issues with code we rely on and was brought in via upgraded dependencies between beta versions of Plone 6. So it's not exactly uncommon to have to deal with breaking changes in custom code.

As a final argument, I'd say that there's not going to be a better opportunity to make this change than before Plone 6, even if we are approaching release candidates.

[davisagli]

What if we went ahead with the switch to storing timezone-aware datetimes, but also implemented a getter to convert from timezone-naive to timezone-aware at read time if it isn't already timezone-aware? That way if someone is doing an inplace migration of a site with many comments, they could choose to defer the migration step.

I'm personally leaning toward going ahead with your change, especially since you've already done a lot of the work. @mauritsvanrees how do you feel about including it at this point in the release cycle?

[instification]

but also implemented a getter to convert from timezone-naive to timezone-aware at read tim

Agreed, I think this is a great solution to anyone missing the upgrade step and presumably it would flow into collective.exportimport. I'll try and put that together as soon as I can.

I'm not sure if there's an additional step needed to include it in the site upgrade steps needed when upgrading Plone, perhaps it needs to get added in Products.CMFPlone too? I'd say that most people would be picking up a new version of p.a.discussion through a plone upgrade rather than manually upgrading it.

[jensens]

convert from timezone-naive to timezone-aware at read time

I do this in in-line updates in custom code, but having intended write-on-reads in core code make me feel uncomfortable.

I agree that we need an upgrade step running with the version upgrade of CMFPlone. An example how to do this for a different package in p.a.upgrade is here is https://github.com/plone/plone.app.upgrade/blob/2e4a3ad9c74eae4c3a81be62a676f0e17e5ec790/plone/app/upgrade/v60/alphas.py#L162-L164

[davisagli]

No, I don’t mean a write on read. I meant converting the value when it is read but not updating the stored value unless the admin runs the upgrade step.

On Thu, Oct 20, 2022 at 6:48 AM Jens W. Klein @.***> wrote:

convert from timezone-naive to timezone-aware at read time

I do this in in-line updates in custom code, but having intended write-on-reads in core code make me feel uncomfortable.

I agree that we need an upgrade step running with the version upgrade of CMFPlone. An example how to do this for a different package in p.a.upgrade is here is https://github.com/plone/plone.app.upgrade/blob/2e4a3ad9c74eae4c3a81be62a676f0e17e5ec790/plone/app/upgrade/v60/alphas.py#L162-L164

— Reply to this email directly, view it on GitHub https://github.com/plone/plone.app.discussion/pull/204#issuecomment-1285577208, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAL65R6ILKAMZKVDLZT6WTWEFEUBANCNFSM6AAAAAARIELFEU . You are receiving this because you were mentioned.Message ID: <plone/plone .@.***>

[mauritsvanrees]

Including this change in beta makes me a bit uncomfortable. But the current released code seems a bit wrong, even though for classic it works. We should check that it still works afterwards in classic, maybe a template there can be simplified now, not sure. So I would indeed say, go ahead and fix it and we'll include it in the next beta or release candidate.

An upgrade step here will be picked up automatically when you run @@plone-upgrade, so that seems fine.

Seems best to me to use the timezone of the portal when storing. But I say this with my programmer-hat on, not my release-manager-hat. ;-)

[instification]

@mauritsvanrees thanks for your comments.

I think the biggest risk of it breaking is in custom code that is assuming it is timezone naive and does a date comparison. ie. if someone wanted a 'x hours ago' type string and aren't using a library that would check first. I don't think it will have any impact to classic or core other than maybe changing the value that is displayed, but in any case that would be to display it correctly when previously it was wrong.

Seems best to me to use the timezone of the portal when storing. But I say this with my programmer-hat on, not my release-manager-hat. ;-)

My understanding is, as long as there's a timezone it shouldn't matter. But by storing the timezone of the person/site that is saving the comment, it will better accommodate daylight savings.

[instification]

@jenkins-plone-org please run jobs

[instification]

@jensens @davisagli I have taken both of your comments on board and implemented the following changes:

• set timezone as the current local value (as returned by p.a.events.utils.localized_now, which checks a bunch of places for the best timezone)
• created a custom getter, so old naive attributes will be returned with utc (as previously they were stored utc)
• updated the tests accordingly
• tried to ensure the tests take daylight savings into account

I'd be grateful if you could review & let me know what you think. And many thanks for the assistance so far.

[instification]

@davisagli Looks like I need to do some more work on the tests :( I'll let you know when it's passing.

[pnicolli]

@pnicolli in your talk at the conference you mentioned using p.a.discussion to handle messaging in volto. Did the timezone issue come up for you there? If so, how did you resolve it?

Sorry if I'm a little late with the reply here. We had no timezone issues, we probably did not realize since we don't show comment dates there.

[instification]

@davisagli @jensens this pr is now ready for a (hopefully) final review & merge. Many thanks for the advice & support.

The final changes I made to get the tests working were:

• Adding the utc timezone in the custom getter. I was originally converting to UTC which was not the correct behaviour as it assumes the saved time was local not utc.
• Fixed a couple of tests that were subtracting the current time from the modified time, leading to a negative timedelta. For some reason, locally comparing < some_negative_timedelta returns true but in jenkins it was returning false. Handy I guess as it exposed the bug in the test.

As well as the passing tests, I've also done some sanity testing in Classic and Volto locally and it all seems to be working fine.

[instification]

@davisagli I'll implement your suggestions over in https://github.com/plone/plone.restapi/pull/1520 then re-run the jenkins jobs here.

[instification]

@davisagli ok I think we're ready for (another) review :)

[davisagli]

@instification Thanks!