Improve timezone handling: #617
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. Load all date/datetime values with moment.utc() which doesn't apply current timezone to them. 2. Don't use toLocaleString to format strings (which was converting them to current timezone as well). Ref #411.
Closed
|
Thanks, @arikfr! This helps for those of us who configure the DB for UTC. However, if a DB is configured for a specific time zone, then the results will return in terms of that. I would think re:dash should use the adapter to determine this time zone before each DB session and store that in each query result. |
|
@stanhu maybe instead of storing it in the query result, it should apply it to the date/time values it has in the results? (if there are any) |
|
Good point. Normalizing to UTC internally in the query results is a good thing. |
|
True. Although before adding more code to handle this, I would start with documenting that re:dash expects timestamps with either timezone data, or it assumes the data is UTC. And of course to implement #618. |
dairyo
pushed a commit
to KiiCorp/redash
that referenced
this pull request
Mar 1, 2019
Improve timezone handling:
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
moment.utc()instead ofmoment(), which was assuming user's local timezone when time zone information wasn't available.toLocaleStringto format strings (which was converting them to current timezone as well). Due to the need to allow showing dates with different formatting (DD/MM vs. MM/DD), there is a new setting to define the date format.Ideally we should add a setting to the user's profile to allow him to select his default timezone and date/time format, which we can than use when showing timestamps.
Closes #411.