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

Force UTC datetimes #102

Closed
wants to merge 2 commits into from
Closed

Force UTC datetimes #102

wants to merge 2 commits into from

Conversation

gertjanol
Copy link

The Intercom API docs states that all UNIX timestamp fields are treated as UTC. This (excellent!) client library uses time- and date functions that assume localtime (time.mktime, datetime.fromtimestamp, datetime.timetuple).

This means that when you feed UTC-timestamps to Intercom, but your localtime is not GMT, you'll get wrong dates back.

This PR fixes this issue by replacing all functions that assume local time with their UTC-counterparts (calendar.timegm, datetime.utcfromtimestamp, datetime.utctimetuple) and forcing a UTC-timezone on returned datetimes.

Another solution would be to skip the user friendliness of converting timestamps to datetimes, and leave this to the user. He'll know whether he stored timestamps using UTC or localtimes so he can do the conversion correctly.

@gertjanol
Copy link
Author

@jkeyes ping

@jkeyes
Copy link
Contributor

jkeyes commented Sep 15, 2015

Hi Gertjan,
Thanks for the contribution. I'll get around to looking at it soon. I've been working on the latest version on the 3.0 branch.
I'm going to freeze development on the 2.0 version.
I'm not sure about introducing another dependency, but I see your point, either support dates properly or don't.
Thanks again. 

@jkeyes
Copy link
Contributor

jkeyes commented Nov 15, 2016

Hi @gertjanol, I'm not going to make any changes to version 2. I know it's been ages since I've been active here. If you want you can apply your fixes to the version-3 branch. If not, I'll look at getting this in there myself. Thanks.

jkeyes added a commit that referenced this pull request Nov 16, 2016
jkeyes added a commit that referenced this pull request Nov 16, 2016
jkeyes added a commit that referenced this pull request Nov 16, 2016
@jkeyes
Copy link
Contributor

jkeyes commented Nov 16, 2016

Hey @gertjanol I've ported your hard work into the version-3 branch. Thanks!

#130

@jkeyes jkeyes closed this Nov 16, 2016
@gertjanol
Copy link
Author

Awesome, thanks! We're still on a fork of the 2-branch, with my pr merged in. I'll see if I can make some time to convert things to 3.

@jkeyes
Copy link
Contributor

jkeyes commented Nov 16, 2016

I won't be updating version 2, one version of python-intercom is enough to maintain. Be warned that Intercom are deprecating API keys in January, so version 2 won't work with that.

@gertjanol
Copy link
Author

I wasn't aware of that 😨 . Thanks for the headsup! Looks like migrating to python-intercom 3 just bumped to high prio ;).

@jkeyes
Copy link
Contributor

jkeyes commented Nov 16, 2016

@gertjanol Actually, you'll still be okay with v2, just set the app id to the PAT, and leave the app key empty. Maybe no need to prioritise it just yet.

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

Successfully merging this pull request may close these issues.

None yet

2 participants