-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
Set timezone for creation and modification dates of comments #204
Conversation
@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:
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! |
@jenkins-plone-ort please run jobs |
@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. |
@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:
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. |
@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? |
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.
Because you can not mix naive and non-naive datetimes in comparisons. Thus, if mixed the index can not be used for sorting. So I think the indexer as it is here plone.app.discussion/plone/app/discussion/catalog.py Lines 103 to 113 in dd0255f
The other datetime related indexers and its attributes and further how the attributes are constructed would possibly need a check too. |
@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 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.
@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 (out of scope here, but keep this in mind) for start/end timezone does matter (hint: recurring dates over different DST borders). |
@jensens Yeah, okay. Point taken. |
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... |
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. |
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? |
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. |
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. |
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
<#204 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAL65R6ILKAMZKVDLZT6WTWEFEUBANCNFSM6AAAAAARIELFEU>
.
You are receiving this because you were mentioned.Message ID: <plone/plone
.***@***.***>
|
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 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. ;-) |
…from older Comment objects
…ight savings bounds.
@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.
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. |
@jenkins-plone-org please run jobs |
@jensens @davisagli I have taken both of your comments on board and implemented the following changes:
I'd be grateful if you could review & let me know what you think. And many thanks for the assistance so far. |
@davisagli Looks like I need to do some more work on the tests :( I'll let you know when it's passing. |
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. |
…arison in tests to correct direction (avoid comparing against negative deltas)
@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:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some testing changing the timezone of the portal and of my computer after creating a comment. Seems to work well.
@davisagli I'll implement your suggestions over in plone/plone.restapi#1520 then re-run the jenkins jobs here. |
@davisagli ok I think we're ready for (another) review :) |
@instification Thanks! |
Currently comment
creation_date
andmodification_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.
plone/plone.restapi#1520 is also required in order for the full plone tests to pass.