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

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

Merged
merged 9 commits into from
Aug 5, 2020
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

1. [19043](https://github.com/influxdata/influxdb/pull/19043): Enforce all influx CLI flag args are valid
1. [19188](https://github.com/influxdata/influxdb/pull/19188): Dashboard cells correctly map results when multiple queries exist
1. [19146](https://github.com/influxdata/influxdb/pull/19146): Dashboard cells and overlay use UTC as query time when toggling to UTC timezone

## v2.0.0-beta.15 [2020-07-23]

Expand Down
102 changes: 92 additions & 10 deletions ui/src/dashboards/selectors/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
// Funcs
import {mocked} from 'ts-jest/utils'
import {
getTimeRange,
getTimeRangeWithTimezone,
} from 'src/dashboards/selectors/index'
import moment from 'moment'
import {getTimezoneOffset} from 'src/dashboards/utils/getTimezoneOffset'

jest.mock('src/dashboards/utils/getTimezoneOffset')

// Types
import {RangeState} from 'src/dashboards/reducers/ranges'
Expand All @@ -30,22 +33,37 @@ const untypedGetTimeRangeWithTimeZone = getTimeRangeWithTimezone as (a: {
}) => TimeRange

describe('Dashboards.Selector', () => {
beforeEach(() => {
jest.clearAllMocks()
})
const dashboardIDs = [
'04c6f3976f4b8001',
'04c6f3976f4b8000',
'04c6f3976f4b8002',
'04c6f3976f4b8003',
'04c6f3976f4b8004',
]
const lower = `2020-05-05T10:00:00${moment().format('Z')}`
const upper = `2020-05-05T11:00:00${moment().format('Z')}`
const customTimeRange = {
lower,
upper,
const customTimeRangePST = {
lower: '2020-05-05T10:00:00-07:00',
upper: '2020-05-05T11:00:00-07:00',
type: 'custom',
} as CustomTimeRange
const customTimeRangeCET = {
lower: '2020-05-05T10:00:00+02:00',
upper: '2020-05-05T11:00:00+02:00',
type: 'custom',
} as CustomTimeRange
const customTimeRangeGMT = {
lower: '2020-05-05T10:00:00+00:00',
upper: '2020-05-05T11:00:00+00:00',
type: 'custom',
} as CustomTimeRange
const ranges: RangeState = {
[dashboardIDs[0]]: pastFifteenMinTimeRange,
[dashboardIDs[1]]: pastHourTimeRange,
[dashboardIDs[2]]: customTimeRange,
[dashboardIDs[2]]: customTimeRangePST,
[dashboardIDs[3]]: customTimeRangeCET,
[dashboardIDs[4]]: customTimeRangeGMT,
}

it('should return the correct range when a matching dashboard ID is found', () => {
Expand All @@ -72,7 +90,7 @@ describe('Dashboards.Selector', () => {
).toEqual(DEFAULT_TIME_RANGE)
})

it('should return the an unmodified version of the timeRange when the timeZone is local', () => {
it('should return an unmodified version of the timeRange when the timeZone is local', () => {
const currentDashboard = {id: dashboardIDs[2]}
const app: AppPresentationState = {
ephemeral: {
Expand All @@ -91,10 +109,10 @@ describe('Dashboards.Selector', () => {

expect(
untypedGetTimeRangeWithTimeZone({ranges, currentDashboard, app})
).toEqual(customTimeRange)
).toEqual(customTimeRangePST)
})

it('should return the timeRange for the same hour with a UTC timezone when the timeZone is UTC', () => {
it('should return the timeRange for the same hour with a UTC timezone when the timeZone is UTC and the locale is 7 timezones behind UTC', () => {
const currentDashboard = {id: dashboardIDs[2]}

const app: AppPresentationState = {
Expand All @@ -117,6 +135,70 @@ describe('Dashboards.Selector', () => {
upper: `2020-05-05T11:00:00Z`,
type: 'custom',
}
// Offset for PST
mocked(getTimezoneOffset).mockImplementation(() => 420)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


expect(
untypedGetTimeRangeWithTimeZone({ranges, currentDashboard, app})
).toEqual(expected)
})

it('should return the timeRange for the same hour with a UTC timezone when the timeZone is UTC and the locale is 2 timezones ahead of UTC', () => {
const currentDashboard = {id: dashboardIDs[3]}

const app: AppPresentationState = {
ephemeral: {
inPresentationMode: false,
hasUpdatedTimeRangeInVEO: false,
},
persisted: {
autoRefresh: 0,
showTemplateControlBar: false,
navBarState: 'expanded',
notebookMiniMapState: 'expanded',
timeZone: 'UTC' as TimeZone,
theme: 'dark',
},
}

const expected = {
lower: `2020-05-05T10:00:00Z`,
upper: `2020-05-05T11:00:00Z`,
type: 'custom',
}
// Offset for CET
mocked(getTimezoneOffset).mockImplementation(() => -120)
Copy link
Contributor

@hoorayimhelping hoorayimhelping Aug 4, 2020

Choose a reason for hiding this comment

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

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


expect(
untypedGetTimeRangeWithTimeZone({ranges, currentDashboard, app})
).toEqual(expected)
})

it('should return the timeRange when the timezone has no offset', () => {
const currentDashboard = {id: dashboardIDs[4]}

const app: AppPresentationState = {
ephemeral: {
inPresentationMode: false,
hasUpdatedTimeRangeInVEO: false,
},
persisted: {
autoRefresh: 0,
showTemplateControlBar: false,
navBarState: 'expanded',
notebookMiniMapState: 'expanded',
timeZone: 'UTC' as TimeZone,
theme: 'dark',
},
}

const expected = {
lower: `2020-05-05T10:00:00Z`,
upper: `2020-05-05T11:00:00Z`,
type: 'custom',
}

mocked(getTimezoneOffset).mockImplementation(() => 0)

expect(
untypedGetTimeRangeWithTimeZone({ranges, currentDashboard, app})
Expand Down
29 changes: 13 additions & 16 deletions ui/src/dashboards/selectors/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
// Libraries
import {get} from 'lodash'
import moment from 'moment'

// Types
import {
AppState,
Check,
Expand All @@ -9,7 +12,10 @@ import {
View,
ViewType,
} from 'src/types'

// Utility
import {currentContext} from 'src/shared/selectors/currentContext'
import {getTimezoneOffset} from 'src/dashboards/utils/getTimezoneOffset'

// Constants
import {DEFAULT_TIME_RANGE} from 'src/shared/constants/timeRanges'
Expand Down Expand Up @@ -53,25 +59,16 @@ export const isCurrentPageDashboard = (state: AppState): boolean =>
// from the local time to the same time in UTC if UTC is selected from the
// timezone dropdown. This is feature was original requested here:
// https://github.com/influxdata/influxdb/issues/17877
// and finalized across the dashboards & the data explorer here:
// https://github.com/influxdata/influxdb/pull/19146
// Example: user selected 10-11:00am and sets the dropdown to UTC
// Query should run against 10-11:00am UTC rather than querying
// 10-11:00am local time (offset depending on timezone)
export const setTimeToUTC = (date: string): string => {
const offset = new Date(date).getTimezoneOffset()
if (offset > 0) {
return moment
.utc(date)
.subtract(offset, 'minutes')
.format()
}
if (offset < 0) {
return moment
.utc(date)
.add(offset, 'minutes')
.format()
}
return moment.utc(date).format()
}
export const setTimeToUTC = (date: string): string =>
moment
.utc(date)
.subtract(getTimezoneOffset(), 'minutes')
.format()

export const getTimeZone = (state: AppState): TimeZone => {
return state.app.persisted.timeZone || 'Local'
Expand Down
13 changes: 13 additions & 0 deletions ui/src/dashboards/utils/getTimezoneOffset.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/**
* This files has been created as a way to effectively test
* the getTimeRangeWithTimezone function since current system (circleCI, Jenkins)
* and JS Date limitations prevent us from fully testing out its dependent functions
*
* It should be noted that the native getTimezoneOffset function returns a number
* that represents the number of minutes (not hours) the "local" timezone is offset
* where locations West of UTC are positive (+420) and locations East of UTC are negative (-120):
*
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/getTimezoneOffset
**/

export const getTimezoneOffset = (): number => new Date().getTimezoneOffset()
4 changes: 2 additions & 2 deletions ui/src/shared/components/RefreshingView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import CellEvent from 'src/perf/components/CellEvent'

// Utils
import {GlobalAutoRefresher} from 'src/utils/AutoRefresher'
import {getTimeRange} from 'src/dashboards/selectors'
import {getTimeRangeWithTimezone} from 'src/dashboards/selectors'
import {checkResultsLength} from 'src/shared/utils/vis'
import {getActiveTimeRange} from 'src/timeMachine/selectors/index'

Expand Down Expand Up @@ -147,7 +147,7 @@ class RefreshingView extends PureComponent<Props, State> {
}

const mstp = (state: AppState, ownProps: OwnProps) => {
const timeRange = getTimeRange(state)
const timeRange = getTimeRangeWithTimezone(state)
const ranges = getActiveTimeRange(timeRange, ownProps.properties.queries)
const {timeZone, theme} = state.app.persisted

Expand Down
4 changes: 2 additions & 2 deletions ui/src/shared/components/TimeSeries.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {getCachedResultsThunk} from 'src/shared/apis/queryCache'

// Utils
import {
getTimeRange,
getTimeRangeWithTimezone,
isCurrentPageDashboard as isCurrentPageDashboardSelector,
} from 'src/dashboards/selectors'
import {getVariables, asAssignment} from 'src/variables/selectors'
Expand Down Expand Up @@ -369,7 +369,7 @@ class TimeSeries extends Component<Props, State> {
}

const mstp = (state: AppState, props: OwnProps) => {
const timeRange = getTimeRange(state)
const timeRange = getTimeRangeWithTimezone(state)

// NOTE: cannot use getAllVariables here because the TimeSeries
// component appends it automatically. That should be fixed
Expand Down
4 changes: 2 additions & 2 deletions ui/src/timeMachine/components/Vis.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {
getFillColumnsSelection,
getSymbolColumnsSelection,
} from 'src/timeMachine/selectors'
import {getTimeRange, getTimeZone} from 'src/dashboards/selectors'
import {getTimeRangeWithTimezone, getTimeZone} from 'src/dashboards/selectors'

// Types
import {RemoteDataState, AppState} from 'src/types'
Expand Down Expand Up @@ -126,7 +126,7 @@ const mstp = (state: AppState) => {
statuses,
},
} = activeTimeMachine
const timeRange = getTimeRange(state)
const timeRange = getTimeRangeWithTimezone(state)
const {
alertBuilder: {type: checkType, thresholds: checkThresholds},
} = state
Expand Down