Skip to content

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.

2 participants