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 time zone names in time zone dropdown #2395

Merged
merged 2 commits into from Jun 29, 2016

Conversation

Projects
None yet
2 participants
@edmundoa
Member

edmundoa commented Jun 21, 2016

Some time zones may not have an area, and we were putting those into the Etc area, which is incorrect and failed when trying to get that time zone back in the server.

Fixes #2393

The changes should also be merged into master.

edmundoa added some commits Jun 21, 2016

Reformat TimezoneSelect
Also use more accurate names for area, and location variables.
https://en.wikipedia.org/wiki/Tz_database#Names_of_time_zones
Do not add Etc area to time zones without area
Adding Etc is incorrect and fails when parsint the zone in the server.
Instead we are adding an unclassified section for organisational purposes,
where we can place all time zones without area.

The area is now removed from those entries in the dropdown and will not
be send to the server.

Fixes #2393

@edmundoa edmundoa added this to the 2.0.4 milestone Jun 21, 2016

// Some time zones are not stored into any areas, this is the group we use to put them apart in the dropdown
// https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
_UNCLASSIFIED_AREA: 'Unclassified',
_formatTimezones() {
const timezones = {};
moment.tz.names().forEach((timezone) => {

This comment has been minimized.

@joschi

joschi Jun 22, 2016

Contributor

Just a remark: Wouldn't it be better to export the list of supported time zones in Joda-Time once and use that instead of what Moment.js returns?

This comment has been minimized.

@edmundoa

edmundoa Jun 22, 2016

Member

@joschi I think that would only move the problem to another place: when we use the time zone to convert UTC times into local times, as that is mostly done in the frontend nowadays. So, in the end, moment.js and Joda-Time need to support the same time zones, or we need to have a layer between them that does some conversion for us. So far I didn't find any example that breaks one of the libraries, so I assume they do use the same names currently.

This comment has been minimized.

@joschi

joschi Jun 29, 2016

Contributor

@joschi joschi self-assigned this Jun 29, 2016

@joschi

This comment has been minimized.

Contributor

joschi commented Jun 29, 2016

LGTM. 👍

@joschi joschi merged commit adeb32a into 2.0 Jun 29, 2016

4 checks passed

ci-server-integration Jenkins build graylog2-server-integration-pr 1005 has succeeded
Details
ci-web-linter Jenkins build graylog-pr-linter-check 491 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@joschi joschi deleted the issue-2393 branch Jun 29, 2016

joschi added a commit that referenced this pull request Jun 29, 2016

Fix time zone names in time zone dropdown (#2395)
* Reformat TimezoneSelect

Also use more accurate names for area, and location variables.
https://en.wikipedia.org/wiki/Tz_database#Names_of_time_zones

* Do not add Etc area to time zones without area

Adding Etc is incorrect and fails when parsint the zone in the server.
Instead we are adding an unclassified section for organisational purposes,
where we can place all time zones without area.

The area is now removed from those entries in the dropdown and will not
be send to the server.

Fixes #2393
(cherry picked from commit adeb32a)

@joschi joschi modified the milestones: 2.0.4, 2.1.0 Aug 11, 2016

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