fix(dashboard-timezone-selection): toggling timezone should update queries timerange to respect timezone selection #19146

asalem1 · 2020-07-29T19:43:27Z

Closes #17877

Problem

The problem with this was twofold:

Toggling between timezones in the explorer (VEO / DE) was incorrectly adjusting times when timezones were East of GMT.
Toggling between timezones on the dashboard page was not updating the timerange parameters to respect the timezone in dashboards
Timerange selections with a timezone were not being respected in data visualization

Solution

The solutions to these issues were:

Update the offset to be an absolute value rather than a negative, since that was incorrectly adjusting the times (and was leading to confusing assumptions)
Updated the timerange being used for the cells (in the TimeSeries component) to account for timezone selections
Updated the timeRange that was being passed to the Views (data visualizations) to respect the timezone selections

CHANGELOG.md updated with a link to the PR (not the Issue)

hoorayimhelping

how about adding some test cases for this bug and its fix: https://github.com/influxdata/influxdb/blob/master/ui/src/dashboards/selectors/index.test.ts

hoorayimhelping · 2020-07-30T16:36:27Z

ui/src/dashboards/selectors/index.ts

@@ -59,7 +59,7 @@ export const setTimeToUTC = (date: string): string => {
  if (offset < 0) {
    return moment
      .utc(date)
-      .add(offset, 'minutes')
+      .add(Math.abs(offset), 'minutes')


Suggested change

.add(Math.abs(offset), 'minutes')

.subtract(offset, 'minutes')

seems odd to me to explicitly check for a negative number, then take the absolute value of it 🤷

@hoorayimhelping I hear what you're saying and it's something I considered but wasn't bold enough to try. Let me give it a shot and see if it has the same outcome

@asalem1 here's a method i've used in the past to gain confidence:

write a test that triggers the bug

swap out your fix and code from master while writing the test if it helps writing the test

confirm the test fails against master

apply your fix and get passing tests reliably

add your scary negative subtraction

if tests pass, confidence. if tests fail, put it back the way it works 👍

this works because you go through the steps of triggering the bug against master, then fixing it. that is what gives you the confidence in your solutions and your tests.

Totally a great idea, unfortunately testing relative time based on a timerange here is not as straightforward as it seems due to the constraints of our current testing framework

asalem1 · 2020-07-30T16:46:33Z

how about adding some test cases for this bug and its fix: https://github.com/influxdata/influxdb/blob/master/ui/src/dashboards/selectors/index.test.ts

Definitely hear where you're coming from but am a little a loss on how to add some tests for stability here, since it would require mocking out date locales in order to ensure that this feature works across different timezones (which is something that we weren't able to do originally).

If you'll recall from the previous PR that's related to this issue, this wasn't actually possible due to configuration variables that cannot be manipulated within out current testing framework

ebb-tide · 2020-08-03T17:50:14Z

ui/src/dashboards/selectors/index.ts

-      .subtract(offset, 'minutes')
-      .format()
+  if (offset === 0) {
+    return moment.utc(date).format()


Is this needed? if offset = 0, would .subtract(offset, 'minutes') not just return zero? I don't know the answer just thought I'd check

That's a great question / point. I think I had originally implemented this so that it wouldn't overwrite formatted data unnecessarily. Let me just check real quick to see if it makes a difference

Great point, totally didn't consider this. You're 100% correct

ebb-tide

It's tricky that you can't test in different time zones. :/

asalem1 · 2020-08-04T20:36:53Z

The most recent PR addresses some of the limitations that the previous testing suite could not. In order to do so, I had to mock out the getTimezoneOffset functionality for testing purposes. This decision was made after making attempts to mock out the Date object's timezone reference within the test suite itself. However, this proved to be an unlikely path. The most compelling argument I found can be seen here:

https://stackoverflow.com/a/15171030/7963795

hoorayimhelping

good work on those tests

hoorayimhelping · 2020-08-04T22:00:37Z

ui/src/dashboards/selectors/index.test.ts

@@ -118,6 +136,70 @@ describe('Dashboards.Selector', () => {
      type: 'custom',
    }

+    mocked(getTimezoneOffset).mockImplementation(() => 420)


~~can you add a comment here saying this is the value returned for california in daylight savings time?~~

actually ignore the above, it's subtly wrong. this is the value returned for -7 UTC timezone, which is the timezone california is in during daylight savings time. in the winter time, this will be 360.

hoorayimhelping · 2020-08-04T22:01:42Z

ui/src/dashboards/selectors/index.test.ts

+      type: 'custom',
+    }
+
+    mocked(getTimezoneOffset).mockImplementation(() => -120)


same thing here about adding a comment for whichever timezone is +2 utc

hoorayimhelping · 2020-08-04T22:06:52Z

ui/src/dashboards/utils/getTimezoneOffset.ts

@@ -0,0 +1,14 @@
+/**
+ * PLEASE READ


suggestion: ✂️ the 'please read' part

a big comment implies it should be read. 'please read' almost comes off like the comment is begging for attention, like pls respond

…ment Still need to address dashboards refreshing when toggling timezones

…election as well

…e logic

…ce it no longer makes a difference

asalem1 requested a review from hoorayimhelping July 29, 2020 19:43

hoorayimhelping requested a review from ebb-tide July 30, 2020 16:28

hoorayimhelping suggested changes Jul 30, 2020

View reviewed changes

asalem1 requested a review from hoorayimhelping July 30, 2020 16:58

ebb-tide reviewed Aug 3, 2020

View reviewed changes

ebb-tide approved these changes Aug 3, 2020

View reviewed changes

asalem1 force-pushed the timezone-offset-issue branch 2 times, most recently from 1362db0 to 975c732 Compare August 4, 2020 20:34

hoorayimhelping approved these changes Aug 4, 2020

View reviewed changes

asalem1 force-pushed the timezone-offset-issue branch from 7098d6d to 33bb672 Compare August 5, 2020 11:55

asalem1 added 9 commits August 5, 2020 07:59

fix(custom-timerange): fixes UTC selection data mismatch and misalign…

3dbf42f

…ment Still need to address dashboards refreshing when toggling timezones

fix(dashboard-cell): updated dashboard cells to conform to timezone s…

0957caa

…election as well

chore(timezone-selector): simplified timezone selector to utilize lik…

0cd3759

…e logic

chore(removed-offset-check): removed conditional check for offset sin…

641f3e6

…ce it no longer makes a difference

chore(CHANGELOG): updated CHANGELOG

c213f7c

feat(test): mocked the offset and added more testing to ensure stability

11d6bb5

chore(test): fixed breaking test

dab43b4

fix(lint): fixed linter errors

57ce56d

chore(updated-comments): updated comments on tests and getTimezoneOffset

5dedd66

asalem1 force-pushed the timezone-offset-issue branch from 33bb672 to 5dedd66 Compare August 5, 2020 14:59

asalem1 merged commit 2397f7f into master Aug 5, 2020

asalem1 deleted the timezone-offset-issue branch August 5, 2020 15:14

asalem1 mentioned this pull request Sep 2, 2020

Timezone selector is not linked to date picker influxdata/ui#18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dashboard-timezone-selection): toggling timezone should update queries timerange to respect timezone selection #19146

fix(dashboard-timezone-selection): toggling timezone should update queries timerange to respect timezone selection #19146

asalem1 commented Jul 29, 2020

hoorayimhelping left a comment

hoorayimhelping Jul 30, 2020

asalem1 Jul 30, 2020

hoorayimhelping Jul 30, 2020

asalem1 Jul 30, 2020

asalem1 commented Jul 30, 2020

ebb-tide Aug 3, 2020

asalem1 Aug 3, 2020

asalem1 Aug 3, 2020

ebb-tide left a comment

asalem1 commented Aug 4, 2020

hoorayimhelping left a comment

hoorayimhelping Aug 4, 2020

hoorayimhelping Aug 4, 2020 •

edited

Loading

hoorayimhelping Aug 4, 2020

asalem1 Aug 5, 2020

	.add(Math.abs(offset), 'minutes')
	.subtract(offset, 'minutes')

fix(dashboard-timezone-selection): toggling timezone should update queries timerange to respect timezone selection #19146

fix(dashboard-timezone-selection): toggling timezone should update queries timerange to respect timezone selection #19146

Conversation

asalem1 commented Jul 29, 2020

Problem

Solution

hoorayimhelping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asalem1 commented Jul 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebb-tide left a comment

Choose a reason for hiding this comment

asalem1 commented Aug 4, 2020

hoorayimhelping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoorayimhelping Aug 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoorayimhelping Aug 4, 2020 •

edited

Loading