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

Timezone bug: App always shows "updated 15 hours ago" #728

Closed
clyons opened this Issue Apr 8, 2011 · 7 comments

Comments

Projects
None yet
4 participants
@clyons

clyons commented Apr 8, 2011

I'm in Toronto (GMT-5), and have this in my config.inc.php:

$THINKUP_CFG['timezone'] = 'America/Toronto';

.. and it always says that it was updated 3 hours ago (we're 3 hours from LA)

@ginatrapani

This comment has been minimized.

Show comment
Hide comment
@ginatrapani

ginatrapani Apr 11, 2011

Member

Thanks for filing this issue, I believe @waxpancake is looking into it. Appreciate it!

Member

ginatrapani commented Apr 11, 2011

Thanks for filing this issue, I believe @waxpancake is looking into it. Appreciate it!

@ginatrapani

This comment has been minimized.

Show comment
Hide comment
@ginatrapani

ginatrapani May 22, 2011

Member

Hey @waxpancake, are you working on this issue, or should I take it out of the "in progress" list?

Member

ginatrapani commented May 22, 2011

Hey @waxpancake, are you working on this issue, or should I take it out of the "in progress" list?

@waxpancake

This comment has been minimized.

Show comment
Hide comment
@waxpancake

waxpancake May 23, 2011

Contributor

Thanks for the ping. From my own testing, if the timezone in the ThinkUp config is set to the timezone of the server (not the user), then everything works great. (If anyone has set their timezone config to their server's timezone and it's STILL showing wrong times, please let me know!) But this isn't always intuitive, as we've seen.

There are three possible solutions to this problem I can think of:

A. During step 2 of the installation, change the field from "Your Time Zone" to "Your Server's Time Zone." Not ideal, as many people won't know what that means.

B. Display the server's current time adjusted to their browser's timezone, and ask them to adjust the time if it's wrong with a dropdown. With that info, we'll know definitively what timezone their server is actually in.

C. Hardest, but probably the most correct: Move off of DATETIME fields entirely (which store dates in the server's localtime), and switch to storing UTC timestamps for everything. Since we only display relative times, that should work great, but it's a significant amount of work to do right. Aside from the code changes and table migrations, we'd need to write migrations that convert all the existing data to the new timezones.

Any thoughts? I'm inclined to go with B. Seems like the best tradeoff of usability and effort, but I'm open to anything.

Cross-posted to the list here: http://groups.google.com/group/thinkupapp/browse_thread/thread/1758ef71446b83f7?hl=en

Contributor

waxpancake commented May 23, 2011

Thanks for the ping. From my own testing, if the timezone in the ThinkUp config is set to the timezone of the server (not the user), then everything works great. (If anyone has set their timezone config to their server's timezone and it's STILL showing wrong times, please let me know!) But this isn't always intuitive, as we've seen.

There are three possible solutions to this problem I can think of:

A. During step 2 of the installation, change the field from "Your Time Zone" to "Your Server's Time Zone." Not ideal, as many people won't know what that means.

B. Display the server's current time adjusted to their browser's timezone, and ask them to adjust the time if it's wrong with a dropdown. With that info, we'll know definitively what timezone their server is actually in.

C. Hardest, but probably the most correct: Move off of DATETIME fields entirely (which store dates in the server's localtime), and switch to storing UTC timestamps for everything. Since we only display relative times, that should work great, but it's a significant amount of work to do right. Aside from the code changes and table migrations, we'd need to write migrations that convert all the existing data to the new timezones.

Any thoughts? I'm inclined to go with B. Seems like the best tradeoff of usability and effort, but I'm open to anything.

Cross-posted to the list here: http://groups.google.com/group/thinkupapp/browse_thread/thread/1758ef71446b83f7?hl=en

@waxpancake

This comment has been minimized.

Show comment
Hide comment
@waxpancake

waxpancake Jun 2, 2011

Contributor

After much discussion, I think we've decided to go with option A for 1.0, and we'll do the proper changes to UTC later. Not ideal, but not critical for 1.0 either.

Contributor

waxpancake commented Jun 2, 2011

After much discussion, I think we've decided to go with option A for 1.0, and we'll do the proper changes to UTC later. Not ideal, but not critical for 1.0 either.

@markjaquith

This comment has been minimized.

Show comment
Hide comment
@markjaquith

markjaquith Jun 2, 2011

Contributor

DATETIME can represent any time zone you like. In WordPress we store both the UTC DATETIME and the local-at-the-time-of-publishing-the-item DATETIME. That allows you to change the configured local time zone without moving content into different days. We use UTC for absolute ordering of content, but local times for "on {DATE}" and "{time} ago" displays.

Contributor

markjaquith commented Jun 2, 2011

DATETIME can represent any time zone you like. In WordPress we store both the UTC DATETIME and the local-at-the-time-of-publishing-the-item DATETIME. That allows you to change the configured local time zone without moving content into different days. We use UTC for absolute ordering of content, but local times for "on {DATE}" and "{time} ago" displays.

@waxpancake

This comment has been minimized.

Show comment
Hide comment
@waxpancake

waxpancake Jun 4, 2011

Contributor

Thanks, Mark... Your comment, combined with Gina pointing out that crawler_last_run already is a TIMESTAMP field, led me to a fix. By default, MySQL returns dates from timestamp fields formatted in human-readable dates, local to the server's timezone. But you can set the timezone manually on a per-connection basis, which is what I'm now doing. That resolves the "updated 15 hours ago" issue, and anywhere else in the app where the times are actually stored in UTC. No matter what the timezone's set to, it will now properly display the relative date/time.

The bug that this surfaced, however, is that we're not actually storing the tu_posts.pub_date in UTC at all. It comes from Twitter in UTC, but we discard the timezone information and store it with a completely inaccurate Unix timestamp in localtime.

For example, this tweet has a created_at UTC date of "Fri Jun 03 20:34:47 +0000 2011" from the Twitter API. But it was stored in my ThinkUp instance with the Unix timestamp "1307158487", which converts Fri Jun 03 2011 20:34:47 in PACIFIC TIME. But that's actually the UTC time of Jun 4, 2011 03:34:47, which is in the future. This is a Bad Thing.

It means that, as far as I can tell, every post stored in ThinkUp has an inaccurate date, and it hasn't been noticed because we either use relative dates ("3 hours ago") almost everywhere and don't specify timezones when we use absolute dates.

I'll patch the code to always store in UTC, and write a migration that converts every incorrect timestamp. That, combined with setting the timezone in MySQL (the display part of the issue), should close this bug.

Contributor

waxpancake commented Jun 4, 2011

Thanks, Mark... Your comment, combined with Gina pointing out that crawler_last_run already is a TIMESTAMP field, led me to a fix. By default, MySQL returns dates from timestamp fields formatted in human-readable dates, local to the server's timezone. But you can set the timezone manually on a per-connection basis, which is what I'm now doing. That resolves the "updated 15 hours ago" issue, and anywhere else in the app where the times are actually stored in UTC. No matter what the timezone's set to, it will now properly display the relative date/time.

The bug that this surfaced, however, is that we're not actually storing the tu_posts.pub_date in UTC at all. It comes from Twitter in UTC, but we discard the timezone information and store it with a completely inaccurate Unix timestamp in localtime.

For example, this tweet has a created_at UTC date of "Fri Jun 03 20:34:47 +0000 2011" from the Twitter API. But it was stored in my ThinkUp instance with the Unix timestamp "1307158487", which converts Fri Jun 03 2011 20:34:47 in PACIFIC TIME. But that's actually the UTC time of Jun 4, 2011 03:34:47, which is in the future. This is a Bad Thing.

It means that, as far as I can tell, every post stored in ThinkUp has an inaccurate date, and it hasn't been noticed because we either use relative dates ("3 hours ago") almost everywhere and don't specify timezones when we use absolute dates.

I'll patch the code to always store in UTC, and write a migration that converts every incorrect timestamp. That, combined with setting the timezone in MySQL (the display part of the issue), should close this bug.

waxpancake added a commit to waxpancake/ThinkUp that referenced this issue Jun 27, 2011

Issue #728: fix timezone bug by converting post.pub_date to DATETIME,…
… adjusting dates to UTC, and set user timezone on new db connections
@waxpancake

This comment has been minimized.

Show comment
Hide comment
@waxpancake

waxpancake Jun 27, 2011

Contributor

Warning: If you run this migration, don't run the crawler from any other branch or you'll get inconsistent times in your local db.

Contributor

waxpancake commented Jun 27, 2011

Warning: If you run this migration, don't run the crawler from any other branch or you'll get inconsistent times in your local db.

waxpancake added a commit to waxpancake/ThinkUp that referenced this issue Aug 4, 2011

Issue #728: fix timezone bug by converting post.pub_date to DATETIME,…
… adjusting dates to UTC, and set user timezone on new db connections

waxpancake added a commit to waxpancake/ThinkUp that referenced this issue Aug 8, 2011

Issue #728: Fix timezone bug
Convert post.pub_date to DATETIME, adjust dates to UTC, and set user timezone on new DB connections
Added missing timestamp to datetime conversion to migration, rebuild db
Failing test for timezone bugfix

waxpancake added a commit to waxpancake/ThinkUp that referenced this issue Aug 8, 2011

Issue #728: Fix timezone bugs
Avoid "Updated 15 hours ago" or "Updated -123456 secs ago" problem by setting user timezone on every DB connection
Convert post.pub_date TIMESTAMP to DATETIME and adjust dates to UTC
Add documentation
Closes #728

ginatrapani added a commit that referenced this issue Aug 8, 2011

Issue #369: Installation auto-selects timezone
* First, select timezone based on client/browser
* Else default to server TZ, else default to UTC
Related to issue #728, closes #369, closes #897

ginatrapani added a commit that referenced this issue Aug 8, 2011

[DB MIGRATION REQ'D] Issue #728: Fix timezone bugs
Avoid "Updated 15 hours ago" or "Updated -123456 secs ago" problem by setting user timezone on every DB connection
Convert post.pub_date TIMESTAMP to DATETIME and adjust dates to UTC
Add documentation
Closes #728, closes #900
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment