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

Only set timezone when user setting is a valid timezone #57850

Merged
merged 15 commits into from
Mar 20, 2020

Conversation

jbudz
Copy link
Member

@jbudz jbudz commented Feb 18, 2020

Currently we set the global browser timezone based on the user's
advanced settings. This setting includes a list of timezones and a
non-standard 'Browser' option which can be translated as set the
timezone to my current. In order to avoid warnings and possible future
errors we only set timezone if it exists in moments list of installed
timezones

Closes #38515

Summary

Summarize your PR. If it involves visual changes include a screenshot or gif.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Currently we set the global browser timezone based on the user's
advanced settings.  This setting includes a list of timezones and a
non-standard 'Browser' option which can be translated as set the
timezone to my current.  In order to avoid warnings and possible future
errors we only set timezone if it exists in moments list of installed
timezones

Closes elastic#38515
@jbudz jbudz added review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team release_note:fix v8.0.0 v7.7.0 labels Feb 18, 2020
@jbudz jbudz requested a review from a team as a code owner February 18, 2020 14:53
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

const setDefaultTimezone = (tz: string) => moment.tz.setDefault(tz);
const setDefaultTimezone = (tz: string) => {
const timezones = moment.tz.names();
const hasTimezone = timezones.includes(tz);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like Moment has moment.tz.zone which checks for existance, returning the zone if loaded or null otherwise.

Could something like this work?

moment.tz.setDefault(moment.tz.zone(tz))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave it a try - zone ends up returning a Zone or null, and we have to access name. It ends up fairly close to this, so I could go either way. lemme know

const zone = moment.tz.zone(tz)
if (zone) moment.tz.setDefault(zone.name)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, Zone is an object. I do think it's better to use moment.tz.zone to check for existence, though.


service.start({ uiSettings });
await flushPromises();
expect(momentMock.tz.setDefault).not.toHaveBeenCalledWith('tz4');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add an assertion that it was called with no args?

@@ -47,6 +47,30 @@ describe('MomentService', () => {
expect(momentMock.updateLocale).toHaveBeenCalledWith('default-locale', { week: { dow: 0 } });
});

it('uses the default timezone when a zone is not defined', async () => {
const tz$ = new BehaviorSubject('tz4');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test might be a bit more obvious if the tz name was something like tz-does-not-exist

const setDefaultTimezone = (tz: string) => moment.tz.setDefault(tz);
const setDefaultTimezone = (tz: string) => {
const zone: string | undefined = get(moment.tz.zone(tz), 'name');
moment.tz.setDefault(zone);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could add a test that moment.tz.setDefault(undefined); doesn't remove previously set timezone?

@@ -47,6 +47,26 @@ describe('MomentService', () => {
expect(momentMock.updateLocale).toHaveBeenCalledWith('default-locale', { week: { dow: 0 } });
});

it('uses the default timezone when a zone is not defined', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you mind to update the test suit name? does not set unknown zone

@spalger
Copy link
Contributor

spalger commented Mar 4, 2020

@elasticmachine merge upstream

@jbudz
Copy link
Member Author

jbudz commented Mar 10, 2020

@elasticmachine merge upstream

@tylersmalley
Copy link
Contributor

@jbudz, are we waiting for something with this? We have three approvals and green CI's.

@jbudz
Copy link
Member Author

jbudz commented Mar 19, 2020

@elasticmachine merge upstream

@jbudz
Copy link
Member Author

jbudz commented Mar 19, 2020

It's good, merging on green.

@jbudz
Copy link
Member Author

jbudz commented Mar 19, 2020

jenkins, test it

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jbudz jbudz merged commit 854f242 into elastic:master Mar 20, 2020
jbudz added a commit that referenced this pull request Mar 20, 2020
* Only set timezone when user setting is a valid timezone

Currently we set the global browser timezone based on the user's
advanced settings.  This setting includes a list of timezones and a
non-standard 'Browser' option which can be translated as set the
timezone to my current.  In order to avoid warnings and possible future
errors we only set timezone if it exists in moments list of installed
timezones

Closes #38515

* feedback

* only set timezone if defined

* Update src/core/public/integrations/moment/moment_service.ts

Co-Authored-By: Mikhail Shustov <[email protected]>

* Update src/core/public/integrations/moment/moment_service.ts

Co-Authored-By: Mikhail Shustov <[email protected]>

* Update src/core/public/integrations/moment/moment_service.test.ts

Co-Authored-By: Mikhail Shustov <[email protected]>

* Update src/core/public/integrations/moment/moment_service.test.ts

Co-Authored-By: Mikhail Shustov <[email protected]>

* update test name

Co-authored-by: Mikhail Shustov <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
@jbudz
Copy link
Member Author

jbudz commented Mar 20, 2020

7.x f0059a3

gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 20, 2020
* master: (55 commits)
  Update dependency @elastic/charts to v18.1.0 (elastic#60578)
  Only set timezone when user setting is a valid timezone (elastic#57850)
  [NP] Remove `ui/agg_types` dependencies and move paginated table to kibana_legacy (elastic#60276)
  [SIEM] Fix types in rules tests (elastic#60736)
  [Alerting] prevent flickering when fields are updated in an alert (elastic#60666)
  License checks for actions plugin (elastic#59070)
  Implemented ability to clear and properly validate alert interval (elastic#60571)
  WebElementWrapper: add findByTestSubject/findAllByTestSubject to search with data-test-subj (elastic#60568)
  [Maps] Update layer dependencies to NP (elastic#59585)
  [Discover] Remove StateManagementConfigProvider (elastic#60221)
  [ML] Listing all categorization wizard checks (elastic#60502)
  [Upgrade Assistant] First iteration of batch reindex docs (elastic#59887)
  [SIEM] Export timeline (elastic#58368)
  [SIEM] Add support for actions and throttle in Rules (elastic#59641)
  Fix ace a11y listener (elastic#60639)
  Add addInfo toast to core notifications service (elastic#60574)
  fix test description (elastic#60638)
  [SIEM] Cypress screenshots upload to google cloud (elastic#60556)
  [canvas/shareable_runtime] sync sass loaders with kbn/optimizer (elastic#60653)
  [SIEM] Fixes Modification of ML Rules (elastic#60662)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 20, 2020
* master:
  Only set timezone when user setting is a valid timezone (elastic#57850)
  [NP] Remove `ui/agg_types` dependencies and move paginated table to kibana_legacy (elastic#60276)
  [SIEM] Fix types in rules tests (elastic#60736)
  [Alerting] prevent flickering when fields are updated in an alert (elastic#60666)
  License checks for actions plugin (elastic#59070)
  Implemented ability to clear and properly validate alert interval (elastic#60571)
  WebElementWrapper: add findByTestSubject/findAllByTestSubject to search with data-test-subj (elastic#60568)
  [Maps] Update layer dependencies to NP (elastic#59585)
  [Discover] Remove StateManagementConfigProvider (elastic#60221)
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 57850 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 21, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 57850 or prevent reminders by adding the backport:skip label.

2 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 57850 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 57850 or prevent reminders by adding the backport:skip label.

@jbudz jbudz added the backport:skip This commit does not require backporting label Mar 24, 2020
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:fix review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Moment Timezone has no data for Browser
7 participants