Change the serialization of the datetime objects, and store them as o… #394

sneridagh · 2017-08-11T15:11:51Z

…ffset-aware UTC-based objects

sneridagh · 2017-08-11T15:12:59Z

This fixes #253

csenger · 2017-08-11T15:21:42Z

Looks good to me.

sneridagh · 2017-08-11T15:26:14Z

src/plone/restapi/deserializer/dxfields.py

@@ -46,8 +46,7 @@ def __call__(self, value):
        try:
            # Parse ISO 8601 string with Zope's DateTime module
            # and convert to a timezone naive datetime in local time
-            value = DateTime(value).toZone(DateTime().localZone()).asdatetime(
-            ).replace(tzinfo=None)
+            value = DateTime(value).toZone('UTC').asdatetime()


@tisto @buchi @lukasgraf

I just tried a new approach and change the code to store the datetime objects as datetime offset-aware and referenced always to UTC. It felt bad to convert it to the local time of the server each time, and it caused a bad offset when the API waas used by people in other timezones than the server's one.

That is perfect, It should always be in an isoformat with UTC timezone.

sneridagh · 2017-08-11T15:27:12Z

docs/source/serialization.rst

@@ -24,6 +24,10 @@ Python                                  JSON
 ``DateTime('2015/11/23 19:45:55')``     ``'2015-11-23T19:45:55'``
 ======================================= ======================================

+.. warning::


The only thing to keep in mind by using this approach is when used in combination with zope.schema date validators. If that's the case, you should also use a datetime object that also contains offset-aware object as the validator value.

coveralls · 2017-08-11T15:40:28Z

Coverage decreased (-0.4%) to 96.004% when pulling d6f3edd on fix-253-add-time-zone-to-serializer into 5b4435e on master.

coveralls · 2017-08-11T16:22:04Z

Coverage decreased (-0.4%) to 96.004% when pulling f7c4dec on fix-253-add-time-zone-to-serializer into 5b4435e on master.

coveralls · 2017-08-11T18:32:08Z

Coverage decreased (-0.4%) to 96.004% when pulling 776ff65 on fix-253-add-time-zone-to-serializer into 5b4435e on master.

coveralls · 2017-08-24T15:14:50Z

Coverage increased (+0.004%) to 96.438% when pulling 8ee4b32 on fix-253-add-time-zone-to-serializer into 5b4435e on master.

coveralls · 2017-08-24T15:14:50Z

Coverage increased (+0.004%) to 96.438% when pulling 8ee4b32 on fix-253-add-time-zone-to-serializer into 5b4435e on master.

coveralls · 2017-08-24T16:14:17Z

Coverage increased (+0.007%) to 96.441% when pulling d578263 on fix-253-add-time-zone-to-serializer into 5b4435e on master.

coveralls · 2017-08-25T09:50:37Z

Coverage increased (+0.007%) to 96.441% when pulling 4dd3ff0 on fix-253-add-time-zone-to-serializer into 5b4435e on master.

coveralls · 2017-08-25T09:50:37Z

Coverage increased (+0.007%) to 96.441% when pulling 4dd3ff0 on fix-253-add-time-zone-to-serializer into 5b4435e on master.

coveralls · 2017-08-25T09:50:37Z

Coverage increased (+0.007%) to 96.441% when pulling 4dd3ff0 on fix-253-add-time-zone-to-serializer into 5b4435e on master.

tisto · 2017-08-29T08:20:04Z

@thet any chance you have time to have a look at this?

loechel

One issue with doing it on try except base, I suggested an alternative that should be more explicite and solves it too.

loechel · 2017-08-29T08:35:43Z

src/plone/restapi/deserializer/dxfields.py

@@ -46,8 +46,7 @@ def __call__(self, value):
        try:
            # Parse ISO 8601 string with Zope's DateTime module
            # and convert to a timezone naive datetime in local time
-            value = DateTime(value).toZone(DateTime().localZone()).asdatetime(
-            ).replace(tzinfo=None)
+            value = DateTime(value).toZone('UTC').asdatetime()


That is perfect, It should always be in an isoformat with UTC timezone.

loechel · 2017-08-29T08:42:19Z

src/plone/restapi/deserializer/dxcontent.py

+                            if value != dm.get():
+                                dm.set(value)
+                                modified = True
+                        except TypeError:


I am sorry, but I did not like the usage of an Error Handler to apply a value

I would suggest something like:

dm_value = dm.get() if value.tzinfo is None: value = value.replace(tzinfo=pytz.UTC) else: value = value.asdatetime(pytz.UTC) if dm_value.tzinfo is None: dm_value = dm_value.replace(tzinfo=pytz.UTC) else: dm_value = dm_value.asdatetime(pytz.UTC) if value != dm_value: dm.set(value) modified = True

instead of the try except clause.

Ok, but you have to detect first if dm.get() returns a datetime right? if not, value.tzinfo will fail. I wanted to prevent to infer more conditional clauses here, which in turn, increases the complexity ratio of the method (and then plone.recipe.codeanalysis complains about it).

yes you are right, there needs to be an if isinstance(value, datetime) and isinstance(dm_value, datetime): that wraps both if else clause.

Well the complexity of an aditional if level is better than an try except. It could also be an idea to split this test aus into a separate function.

@sneridagh @loechel For me the main objective of this code snippet is readability. I don't mind a try/except if it makes the code more readable. Though, without a few more comments I don't really understand what goes on here anyways. :)

…ed on update

coveralls · 2017-08-30T09:31:25Z

Coverage increased (+0.007%) to 96.441% when pulling 7f5dd84 on fix-253-add-time-zone-to-serializer into 5b4435e on master.

coveralls · 2017-08-30T09:32:53Z

Coverage increased (+0.007%) to 96.441% when pulling 7f5dd84 on fix-253-add-time-zone-to-serializer into 5b4435e on master.

coveralls · 2017-08-30T09:32:53Z

Coverage increased (+0.007%) to 96.441% when pulling 7f5dd84 on fix-253-add-time-zone-to-serializer into 5b4435e on master.

coveralls · 2017-08-30T09:32:53Z

Coverage increased (+0.007%) to 96.441% when pulling 7f5dd84 on fix-253-add-time-zone-to-serializer into 5b4435e on master.

sneridagh · 2017-08-30T09:50:55Z

@loechel Please check again the current implementation. It takes into account your comments.

thet · 2017-08-30T09:42:10Z

docs/source/deserialization.rst

+
+and builds the `start` and `end` fields with the proper timezone, depending on the `timezone` field. It also returns the datetime object with the proper timezone offset.
+
+If using Plone 4 and p.a.event 1.x you should construct the Event type using this approach, otherwise the Event object will be created with a wrong timezones.


typo: -> "with a wrong timezone."

thet · 2017-08-30T09:59:19Z

src/plone/restapi/deserializer/dxcontent.py

+                            if value.tzinfo is None:
+                                value = value.replace(tzinfo=pytz.UTC)
+                            if dm_value.tzinfo is None:
+                                dm_value = dm_value.replace(tzinfo=pytz.UTC)


Wouldn't it be better to localize the datetime values to the portal's timezone?

Something like:

from plone.app.event.base import default_timezone dtz = default_timezone(self.context, as_tzinfo=True) dtz.localize(value)

Or if you have to avoid a dependency on plone.app.event here:

value = DateTime( value.year(), value.month(), value.day(), value.hour(), value.minute() ).asdatetime()

That uses the default timezone of DateTime, which is the servers timezone (which should actually be set to UTC but often is set in the local zone).

I would always go for UTC as convention.

thet · 2017-08-30T10:00:08Z

src/plone/restapi/deserializer/dxfields.py

-            value = DateTime(value).toZone(DateTime().localZone()).asdatetime(
-            ).replace(tzinfo=None)
+            # and convert to a UTC based timezone as datetime
+            value = DateTime(value).toZone('UTC').asdatetime()


Again like above, I wouldn't use 'UTC' but the portal's default timezone.

I think that the concept of having a "portal" timezone is wrong. In which case is p.a.event using it? It's ok if you always have users in the same timezone, but this is hardly the case in most user cases.

All datetimes should be referenced using a common baseline. Then the UI convert it to the user's timezone. If not, a user in India will see always the dates offsetted with the portal timezone, which is pretty useless for him. This is what happened before and that's the reason of this change.

@thet I thought you agree on that too when you said: #253 (comment)

Yes, true. I was suggesting that because I thought you do not have a timezone information in the JSON object available - besides the offset.
But if you have timezone-naive datetimes (UTC is not timezone-naive), like in the code above - and you convert those to UTC, then you make a decision which could be wrong. Unless you always exchange dates in UTC per convention.

Fact is, plone.app.event 2.x does not convert it's datetimes to UTC but stores them in the default timezone - which is:

The event's timezone (not implemented yet)

The user's timezone (probably most often not set)

The portal's timezone (You have to set this on portal creation time)

I agree that users from anywhere want to have the option to show the datetimes in their local zone. But that's a UI thing and should be done via JS with the option to show the datetimes in the original zone.

So: converting to UTC and storing in UTC is OK, if you have no timezone identifier (offsets are a bit useless) and exchange in UTC anyways.
IMO it's not OK if the datetimes are "naive" - without any timezone information.

@sneridagh If you want, we can have a short hangout if you think I can help here...

@thet sure, whenever you are available, we can have a talk about it. We should find a common agreement on this.

Agreed that the implementation makes an assumption that p.a.event is not (yet) doing but IMHO should do and we should fix it upstream in Plone. That is, save the internal dates with timezone information under a common frame of reference (say UTC).

Clients (and existing UI) should provide always datetimes depending on the user's timezone (we are already doing that in a client's project all Angular UI. Current libraries always transform to UTC on serializing and it's easy to be user's timezone aware.

Setting a portal default timezone (or even a user timezone) it's useless and error prone. The current implementation now it's broken, as you can see, the provided tests are not passing using the current version of plone.restapi.

The more sensible solution (as agreed in #253 seems to store always the timezone information) but then, use a common reference for that (using server's timezone will also be error prone and dates could not be the same if you migrate one server from timezone).

@tisto @lukasgraf @buchi any insights on this?

I would say, if the convention in plone.restapi is to always export dates as UTC that is an convention. If other modules for other export formats uses another convetion that might not be consistent, but possible.

I would prefer to always export datetime objects as timezone aware objects, and that transformed to UTC, but also to always import all native datetime input as new timezone aware datetime objects. Presentation in the UI should be bound to user settings.

coveralls · 2017-08-30T10:15:26Z

Coverage decreased (-0.01%) to 96.42% when pulling ba84c0f on fix-253-add-time-zone-to-serializer into 5b4435e on master.

coveralls · 2017-08-30T10:15:26Z

Coverage decreased (-0.01%) to 96.42% when pulling ba84c0f on fix-253-add-time-zone-to-serializer into 5b4435e on master.

coveralls · 2017-08-30T10:15:26Z

Coverage decreased (-0.01%) to 96.42% when pulling ba84c0f on fix-253-add-time-zone-to-serializer into 5b4435e on master.

coveralls · 2017-08-30T10:39:41Z

Coverage decreased (-0.01%) to 96.42% when pulling 7169bee on fix-253-add-time-zone-to-serializer into 5b4435e on master.

coveralls · 2017-08-30T10:39:41Z

Coverage decreased (-0.01%) to 96.42% when pulling 7169bee on fix-253-add-time-zone-to-serializer into 5b4435e on master.

loechel

I still have a few typos found, and one potential critical case on init.py --> if an pkg_resources.DistributionNotFound Error occure following import errors will occure as then HAS_PLONE_APP_EVENT_20 is undefined.

But overall it now looks very good.

loechel · 2017-08-30T21:47:07Z

docs/source/deserialization.rst

+Deserialization
+===============
+
+It's worth to note an special case when deserializing Datetimes objects, and how plone.restapi will handle them.


Still a typo: "to note an special" --> to note a special"

loechel · 2017-08-30T21:48:20Z

docs/source/deserialization.rst

+
+It's worth to note an special case when deserializing Datetimes objects, and how plone.restapi will handle them.
+
+Although not supported by Plone itself yet, plone.restapi will store all the Datetimes that will be handling along with its timezone converted to UTC.


"all the Datetimes" --> "datetime objects"

loechel · 2017-08-30T21:50:42Z

docs/source/deserialization.rst

+
+If using Plone 4 and p.a.event 1.x you should construct the Event type using this approach, otherwise the Event object will be created with a wrong timezone.
+
+This approach was counterintuitive, and thus, it was changed it Plone 5 version of p.a.event.


maybe better: it was changed for Plone 5 by p.a.event version 2

loechel · 2017-08-30T21:51:38Z

docs/source/deserialization.rst

+p.a.event 2.x in Plone 5
+------------------------
+
+The implementation of p.a.event in 2.x no longer requires to provide a `timezone` schema property, because the timezone is computed taking the timezone already existent in dates supplied::


"p.a.event in 2.x no longer" --> p.a.event 2.x no longer"

loechel · 2017-08-30T21:54:23Z

src/plone/restapi/__init__.py

+        HAS_PLONE_APP_EVENT_20 = True
+    else:
+        HAS_PLONE_APP_EVENT_20 = False
+
 except pkg_resources.DistributionNotFound:  # pragma: no cover
    HAS_PLONE_APP_CONTENTTYPES = False


shouldn't in the except case also HAS_PLONE_APP_EVENT_20 = False be added, as else you do not have that value later on.

loechel · 2017-08-30T21:55:53Z

src/plone/restapi/deserializer/dxcontent.py

+                            if value.tzinfo is None:
+                                value = value.replace(tzinfo=pytz.UTC)
+                            if dm_value.tzinfo is None:
+                                dm_value = dm_value.replace(tzinfo=pytz.UTC)


I would always go for UTC as convention.

loechel · 2017-08-30T22:04:04Z

src/plone/restapi/deserializer/dxcontent.py

+                            if value.tzinfo is None:
+                                value = value.replace(tzinfo=pytz.UTC)
+                            if dm_value.tzinfo is None:
+                                dm_value = dm_value.replace(tzinfo=pytz.UTC)


good catch that the second case in my proposal is unnessesary, two datetime objects that have the same time in utc are equal even if the contain different timezone infos.

coveralls · 2017-09-05T15:03:30Z

Coverage decreased (-0.01%) to 96.42% when pulling 66d3e22 on fix-253-add-time-zone-to-serializer into 5c34f4d on master.

coveralls · 2017-09-05T15:03:31Z

Coverage decreased (-0.01%) to 96.42% when pulling 66d3e22 on fix-253-add-time-zone-to-serializer into 5c34f4d on master.

loechel · 2017-10-03T16:51:32Z

@tisto could you please make a last check and merge, and also close #396

Thanks

coveralls · 2017-10-03T18:02:02Z

Coverage decreased (-0.1%) to 96.436% when pulling 00e5871 on fix-253-add-time-zone-to-serializer into 88a6779 on master.

coveralls · 2017-10-03T18:02:02Z

Coverage decreased (-0.1%) to 96.436% when pulling 00e5871 on fix-253-add-time-zone-to-serializer into 88a6779 on master.

buchi · 2017-10-22T10:01:50Z

This doesn't work for me. I'v done the following test with Plone 5.0 with p.a.event 2.0:

Create an event TTW with:

start: 2017-10-01T10:00
end: 2017-10-31T10:00
effective: 2017-10-02T10:00
expires: 2017-10-30T10:00

Event information shown in event view:

01.10.2017 10:00 to 31.10.2017 10:00 (Europe/Berlin / UTC200)
Effective: 2 Oktober, 2017 10:00
Expires: 30 Oktober, 2017, 10:00

Create the "same" event with REST API:

{
    "@type": "Event",
    "title": "A REST API event",
    "effective": "2017-10-02T10:00:00",
    "end": "2017-10-31T10:00:00+01:00",
    "expires": "2017-10-30T10:00:00",
    "start": "2017-10-01T10:00:00+02:00"
}

Event information shown in event view:

01.10.2017 08:00 to 31.10.2017 09:00 (UTC / UTC0)
Effective: 2 Oktober, 2017 10:00
Expires: 30 Oktober, 2017, 10:00

Both events represent the same dates but timezone information is wrong when the event is created using the REST API.

Editing the REST API event TTW by adding e description results in:

01.10.2017 08:00 to 31.10.2017 09:00 (Europe/Berlin / UTC200)
Effective: 2 Oktober, 2017 10:00
Expires: 30 Oktober, 2017, 10:00

Now the timezone information gets added, but no timezone conversion occurs. Thus the dates are now wrong. Even worse, they get modified though the user didn't change them.

sneridagh · 2017-11-07T08:37:44Z

This is superseeded by #415 , closing.

Change the serialization of the datetime objects, and store them as o…

d6f3edd

…ffset-aware UTC-based objects

sneridagh requested review from buchi, lukasgraf and tisto August 11, 2017 15:12

sneridagh mentioned this pull request Aug 11, 2017

Propose a fix for the issue #253 by not comparing IDatetime fields wh… #258

Closed

sneridagh commented Aug 11, 2017

View reviewed changes

Fix typo

f7c4dec

Improve tests and documentation

776ff65

sneridagh mentioned this pull request Aug 23, 2017

p.a.event: Cannot compare offset-naive and offset-aware datetimes #253

Closed

Add conditional tests for Plone 5

8ee4b32

Add solution for dates comparision in PATCH operations and related test

d578263

Add proper documentation explaining both behaviors of p.a.event

4dd3ff0

sneridagh requested a review from csenger August 25, 2017 09:10

tisto requested a review from thet August 28, 2017 14:36

sneridagh mentioned this pull request Aug 29, 2017

Missing TimeZone info from deserialized value #396

Closed

sneridagh requested a review from loechel August 29, 2017 08:33

loechel requested changes Aug 29, 2017

View reviewed changes

sneridagh added 2 commits August 30, 2017 10:51

Correct old comment for a proper new one

7f5dd84

Implement a better handling of the use case when two dates are compar…

ba84c0f

…ed on update

sneridagh requested review from tisto and loechel August 30, 2017 09:51

thet reviewed Aug 30, 2017

View reviewed changes

Correct another typo

7169bee

loechel approved these changes Aug 30, 2017

View reviewed changes

Merge branch 'master' into fix-253-add-time-zone-to-serializer

66d3e22

loechel and others added 2 commits October 3, 2017 18:48

fix requested typos and error on except case

d7290c0

Merge branch 'master' into fix-253-add-time-zone-to-serializer

00e5871

tisto added this to the 1.0b1 milestone Nov 4, 2017

sneridagh closed this Nov 7, 2017

jensens deleted the fix-253-add-time-zone-to-serializer branch April 7, 2020 18:13


		and builds the `start` and `end` fields with the proper timezone, depending on the `timezone` field. It also returns the datetime object with the proper timezone offset.

		If using Plone 4 and p.a.event 1.x you should construct the Event type using this approach, otherwise the Event object will be created with a wrong timezones.


		It's worth to note an special case when deserializing Datetimes objects, and how plone.restapi will handle them.

		Although not supported by Plone itself yet, plone.restapi will store all the Datetimes that will be handling along with its timezone converted to UTC.


		If using Plone 4 and p.a.event 1.x you should construct the Event type using this approach, otherwise the Event object will be created with a wrong timezone.

		This approach was counterintuitive, and thus, it was changed it Plone 5 version of p.a.event.

Change the serialization of the datetime objects, and store them as o… #394

Change the serialization of the datetime objects, and store them as o… #394

Conversation

sneridagh commented Aug 11, 2017

sneridagh commented Aug 11, 2017

csenger commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 11, 2017

coveralls commented Aug 11, 2017

coveralls commented Aug 11, 2017

coveralls commented Aug 24, 2017

coveralls commented Aug 24, 2017 • edited Loading

coveralls commented Aug 24, 2017 • edited Loading

coveralls commented Aug 25, 2017

coveralls commented Aug 25, 2017

coveralls commented Aug 25, 2017

tisto commented Aug 29, 2017

loechel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 30, 2017 • edited Loading

coveralls commented Aug 30, 2017

coveralls commented Aug 30, 2017

coveralls commented Aug 30, 2017

sneridagh commented Aug 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thet Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 30, 2017

coveralls commented Aug 30, 2017

coveralls commented Aug 30, 2017

coveralls commented Aug 30, 2017

coveralls commented Aug 30, 2017 • edited Loading

loechel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 5, 2017 • edited Loading

coveralls commented Sep 5, 2017

loechel commented Oct 3, 2017

coveralls commented Oct 3, 2017 • edited Loading

coveralls commented Oct 3, 2017

buchi commented Oct 22, 2017

sneridagh commented Nov 7, 2017

coveralls commented Aug 24, 2017 •

edited

Loading

coveralls commented Aug 24, 2017 •

edited

Loading

coveralls commented Aug 30, 2017 •

edited

Loading

thet Aug 30, 2017 •

edited

Loading

coveralls commented Aug 30, 2017 •

edited

Loading

coveralls commented Sep 5, 2017 •

edited

Loading

coveralls commented Oct 3, 2017 •

edited

Loading