-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
Fix datetime deserialization for timezone aware fields #415
Conversation
51bec0b
to
4c810cb
Compare
@sneridagh I've implemented another approach to fix datetime deserialization. Can you check if this works for your use case? If the field already has a value, the new value will be converted to a timezone aware datetime object with the same timezone. This works also for newly created events as the start and end field both have default values. |
2 similar comments
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.
LGTM. Would like someone else with more in-depth knowledge to have a second look (e.g. @sneridagh )
1 similar comment
b40c992
to
e559c10
Compare
@sneridagh could you please review this pr? I would like to make a new release soon and include this one if possible. |
Changes Unknown when pulling 9487194 on fix-datetime-deserialization into ** on master**. |
1 similar comment
Changes Unknown when pulling 9487194 on fix-datetime-deserialization into ** on master**. |
9487194
to
f7a7f21
Compare
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.
LGTM too! Nice and elegant approach that fixes all the use cases and retains the current behavior.
@tisto @buchi:
However, I implemented some tests in my initial PR that might be interesting:
https://github.com/plone/plone.restapi/pull/394/files#diff-a6bae0e634bde0c6823c506071a52a50R224
and
https://github.com/plone/plone.restapi/pull/394/files#diff-03b868ff4d32963d159a2f06dcbe9588R93
They are working fine but have the drawback that they must be especific for p.a.event <2.0 and above that. Do you think they are interesting to have in place or we can live without them?
Fixes #253