Skip to content
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

Merged
merged 1 commit into from Nov 6, 2017

Conversation

buchi
Copy link
Member

@buchi buchi commented Oct 22, 2017

Fixes #253

@coveralls
Copy link

coveralls commented Oct 22, 2017

Coverage Status

Coverage increased (+0.01%) to 96.576% when pulling 51bec0b on fix-datetime-deserialization into 88a6779 on master.

@buchi buchi force-pushed the fix-datetime-deserialization branch from 51bec0b to 4c810cb Compare October 22, 2017 22:42
@buchi
Copy link
Member Author

buchi commented Oct 22, 2017

@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.
If the field doesn't have a value, the given value is stored as datetime naive object (same behavior as before).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 96.576% when pulling 4c810cb on fix-datetime-deserialization into 88a6779 on master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 96.576% when pulling 4c810cb on fix-datetime-deserialization into 88a6779 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 96.576% when pulling 4c810cb on fix-datetime-deserialization into 88a6779 on master.

Copy link
Sponsor Member

@tisto tisto left a 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 )

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 96.576% when pulling b40c992 on fix-datetime-deserialization into 7fa16c9 on master.

1 similar comment
@coveralls
Copy link

coveralls commented Oct 25, 2017

Coverage Status

Coverage increased (+0.01%) to 96.576% when pulling b40c992 on fix-datetime-deserialization into 7fa16c9 on master.

@buchi buchi force-pushed the fix-datetime-deserialization branch from b40c992 to e559c10 Compare October 30, 2017 08:24
@coveralls
Copy link

coveralls commented Oct 30, 2017

Coverage Status

Coverage increased (+0.01%) to 96.65% when pulling e559c10 on fix-datetime-deserialization into 8a84333 on master.

@tisto
Copy link
Sponsor Member

tisto commented Nov 3, 2017

@sneridagh could you please review this pr? I would like to make a new release soon and include this one if possible.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9487194 on fix-datetime-deserialization into ** on master**.

1 similar comment
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9487194 on fix-datetime-deserialization into ** on master**.

@coveralls
Copy link

coveralls commented Nov 3, 2017

Coverage Status

Coverage increased (+0.01%) to 96.651% when pulling 9487194 on fix-datetime-deserialization into c9be8e3 on master.

@tisto tisto added this to the 1.0b1 milestone Nov 4, 2017
@buchi buchi force-pushed the fix-datetime-deserialization branch from 9487194 to f7a7f21 Compare November 6, 2017 09:01
@coveralls
Copy link

coveralls commented Nov 6, 2017

Coverage Status

Coverage increased (+0.01%) to 96.654% when pulling f7a7f21 on fix-datetime-deserialization into 3aaf76e on master.

Copy link
Member

@sneridagh sneridagh left a 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants