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

feat(custom-timerange-tz): timeZone dropdown is now integrated in setting custom time ranges for queries #17992

Merged
merged 15 commits into from
May 7, 2020
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
### Features

1. [17934](https://github.com/influxdata/influxdb/pull/17934): Add ability to delete a stack and all the resources associated with it
1. [17992](https://github.com/influxdata/influxdb/pull/17992): Add ability to specify UTC when making custom time range queries
1. [17941](https://github.com/influxdata/influxdb/pull/17941): Enforce DNS name compliance on all pkger resources' metadata.name field
1. [17989](https://github.com/influxdata/influxdb/pull/17989): Add stateful pkg management with stacks

Expand Down
3 changes: 2 additions & 1 deletion ui/src/checks/components/CheckHistory.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {STATUS_FIELDS} from 'src/alerting/constants/history'
// Utils
import {loadStatuses, getInitialState} from 'src/alerting/utils/history'
import {getCheckIDs} from 'src/checks/selectors'
import {getTimeZone} from 'src/dashboards/selectors'

// Types
import {ResourceIDs} from 'src/checks/reducers'
Expand Down Expand Up @@ -101,7 +102,7 @@ const CheckHistory: FC<Props> = ({
}

const mstp = (state: AppState, props: OwnProps) => {
const timeZone = state.app.persisted.timeZone
const timeZone = getTimeZone(state)
const checkIDs = getCheckIDs(state)
const check = getByID<Check>(state, ResourceType.Checks, props.params.checkID)

Expand Down
67 changes: 61 additions & 6 deletions ui/src/dashboards/selectors/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
// Funcs
import {getTimeRange} from 'src/dashboards/selectors/index'
import {
getTimeRange,
getTimeRangeWithTimezone,
} from 'src/dashboards/selectors/index'

// Types
import {RangeState} from 'src/dashboards/reducers/ranges'
import {CurrentDashboardState} from 'src/shared/reducers/currentDashboard'
import {TimeRange} from 'src/types'
import {TimeRange, CustomTimeRange} from 'src/types'
import {AppState} from 'src/shared/reducers/app'

// Constants
import {
Expand All @@ -18,34 +22,85 @@ const untypedGetTimeRangeByDashboardID = getTimeRange as (a: {
currentDashboard: CurrentDashboardState
}) => TimeRange

const untypedGetTimeRangeWithTimeZone = getTimeRangeWithTimezone as (a: {
ranges: RangeState
currentDashboard: CurrentDashboardState
app: AppState
}) => TimeRange

describe('Dashboards.Selector', () => {
const dashboardIDs = ['04c6f3976f4b8001', '04c6f3976f4b8000']
const dashboardIDs = [
'04c6f3976f4b8001',
'04c6f3976f4b8000',
'04c6f3976f4b8002',
]
const customTimeRange = {
lower: '2020-05-05T10:00:00-07:00',
upper: '2020-05-05T11:00:00-07:00',
type: 'custom',
} as CustomTimeRange
const ranges: RangeState = {
[dashboardIDs[0]]: pastFifteenMinTimeRange,
[dashboardIDs[1]]: pastHourTimeRange,
[dashboardIDs[2]]: customTimeRange,
}

it('should return the the correct range when a matching dashboard ID is found', () => {
it('should return the correct range when a matching dashboard ID is found', () => {
const currentDashboard = {id: dashboardIDs[0]}

expect(
untypedGetTimeRangeByDashboardID({ranges, currentDashboard})
).toEqual(pastFifteenMinTimeRange)
})

it('should return the the default range when no matching dashboard ID is found', () => {
it('should return the default range when no matching dashboard ID is found', () => {
const currentDashboard = {id: 'Oogum Boogum'}

expect(
untypedGetTimeRangeByDashboardID({ranges, currentDashboard})
).toEqual(DEFAULT_TIME_RANGE)
})

it('should return the the default range when no ranges are passed in', () => {
it('should return the default range when no ranges are passed in', () => {
const currentDashboard = {id: dashboardIDs[0]}

expect(
untypedGetTimeRangeByDashboardID({ranges: {}, currentDashboard})
).toEqual(DEFAULT_TIME_RANGE)
})

it('should return the an unmodified version of the timeRange when the timeZone is local', () => {
const currentDashboard = {id: dashboardIDs[2]}

const app = {
persisted: {
timeZone: 'Local',
},
} as AppState

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

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

const app = {
persisted: {
timeZone: 'UTC',
},
} as AppState

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

expect(app.persisted.timeZone).toEqual('UTC')
expect(
untypedGetTimeRangeWithTimeZone({ranges, currentDashboard, app})
).toEqual(expected)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

these tests look fine, and are wise in that they don't reference like today or now but instead solid dates in the past.

but having said that, a career of having tests around checking date/times that fail randomly has made me extremely wary and gunshy of them. just keep an eye on tests that involve dates and time. they can fail and blow up in weird and unexpected and painful ways.

})
45 changes: 43 additions & 2 deletions ui/src/dashboards/selectors/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {get} from 'lodash'

import {AppState, View, Check, ViewType, TimeRange} from 'src/types'
import moment from 'moment'
import {AppState, View, Check, ViewType, TimeRange, TimeZone} from 'src/types'
import {currentContext} from 'src/shared/selectors/currentContext'

// Constants
Expand All @@ -15,6 +15,47 @@ export const getTimeRange = (state: AppState): TimeRange => {
return state.ranges[contextID] || DEFAULT_TIME_RANGE
}

export const getTimeRangeWithTimezone = (state: AppState): TimeRange => {
const timeRange = getTimeRange(state)
const timeZone = getTimeZone(state)
// create a copy of the timeRange so as not to mutate the original timeRange
const newTimeRange = Object.assign({}, timeRange)
asalem1 marked this conversation as resolved.
Show resolved Hide resolved
if (timeRange.type === 'custom' && timeZone === 'UTC') {
// check to see if the timeRange has an offset
asalem1 marked this conversation as resolved.
Show resolved Hide resolved
newTimeRange.lower = setTimeToUTC(newTimeRange.lower)
newTimeRange.upper = setTimeToUTC(newTimeRange.upper)
}
return newTimeRange
}

export const setTimeToUTC = (date: string): string => {
asalem1 marked this conversation as resolved.
Show resolved Hide resolved
const offset = new Date(date).getTimezoneOffset()
// check if date has offset
if (offset === 0) {
return date
}
let offsetDate = date
if (offset > 0) {
// subtract tz minute difference
Copy link
Contributor

Choose a reason for hiding this comment

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

OH! is that what the subtract function does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

;)

offsetDate = moment
.utc(date)
.subtract(offset, 'minutes')
.format()
}
if (offset < 0) {
// add tz minute difference
offsetDate = moment
.utc(date)
.add(offset, 'minutes')
.format()
}
return offsetDate
}
asalem1 marked this conversation as resolved.
Show resolved Hide resolved

export const getTimeZone = (state: AppState): TimeZone => {
return state.app.persisted.timeZone || 'Local'
}

export const getCheckForView = (
state: AppState,
view: View
Expand Down
5 changes: 3 additions & 2 deletions ui/src/shared/components/TimeZoneDropdown.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ import React, {FunctionComponent} from 'react'
import {connect} from 'react-redux'
import {SelectDropdown, IconFont} from '@influxdata/clockface'

// Actions
// Actions & Selectors
import {setTimeZone} from 'src/shared/actions/app'
import {getTimeZone} from 'src/dashboards/selectors'

// Constants
import {TIME_ZONES} from 'src/shared/constants/timeZones'
Expand Down Expand Up @@ -38,7 +39,7 @@ const TimeZoneDropdown: FunctionComponent<Props> = ({
}

const mstp = (state: AppState): StateProps => {
return {timeZone: state.app.persisted.timeZone || 'Local'}
return {timeZone: getTimeZone(state)}
}

const mdtp = {onSetTimeZone: setTimeZone}
Expand Down
7 changes: 4 additions & 3 deletions ui/src/timeMachine/actions/queryBuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {fetchDemoDataBuckets} from 'src/cloud/apis/demodata'

// Utils
import {getActiveQuery, getActiveTimeMachine} from 'src/timeMachine/selectors'
import {getTimeRange} from 'src/dashboards/selectors'
import {getTimeRangeWithTimezone} from 'src/dashboards/selectors'

// Types
import {
Expand Down Expand Up @@ -222,7 +222,8 @@ export const loadTagSelector = (index: number) => async (
const orgID = get(foundBucket, 'orgID', getOrg(getState()).id)

try {
const timeRange = getTimeRange(state)
const timeRange = getTimeRangeWithTimezone(state)

const searchTerm = getActiveTimeMachine(state).queryBuilder.tags[index]
.keysSearchTerm

Expand Down Expand Up @@ -287,7 +288,7 @@ const loadTagSelectorValues = (index: number) => async (
dispatch(setBuilderTagValuesStatus(index, RemoteDataState.Loading))

try {
const timeRange = getTimeRange(state)
const timeRange = getTimeRangeWithTimezone(state)
const key = getActiveQuery(getState()).builderConfig.tags[index].key
const searchTerm = getActiveTimeMachine(getState()).queryBuilder.tags[index]
.valuesSearchTerm
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} from 'src/dashboards/selectors'
import {getTimeRange, getTimeZone} from 'src/dashboards/selectors'

// Types
import {
Expand Down Expand Up @@ -164,7 +164,7 @@ const mstp = (state: AppState): StateProps => {
const fillColumns = getFillColumnsSelection(state)
const symbolColumns = getSymbolColumnsSelection(state)

const timeZone = state.app.persisted.timeZone
const timeZone = getTimeZone(state)
Copy link
Contributor

Choose a reason for hiding this comment

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

so the line this is replacing wouldn't set the timeZone to local if it state.app.persisted.timeZone didn't exist. this function will. is that okay for this component? i imagine it is, but just want to be explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also noticed that. It's weird because we set local as the default in only one place, but I think it's good to have a default for this value, especially if it's a required property. I'm very wary of assigning defaultProps, but in this case I think it's safe to assign a default even if none were specified


return {
loading,
Expand Down
5 changes: 5 additions & 0 deletions ui/src/variables/selectors/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ import {
import {AppState} from 'src/types'

const MOCKSTATE = ({
app: {
persisted: {
timeZone: 'UTC',
},
},
currentDashboard: {
id: '',
},
Expand Down
7 changes: 2 additions & 5 deletions ui/src/variables/selectors/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {get} from 'lodash'
// Utils
import {getActiveQuery} from 'src/timeMachine/selectors'
import {getRangeVariable} from 'src/variables/utils/getTimeRangeVars'
import {getTimeRange} from 'src/dashboards/selectors'
import {getTimeRange, getTimeRangeWithTimezone} from 'src/dashboards/selectors'
import {getWindowPeriodVariable} from 'src/variables/utils/getWindowVars'
import {
TIME_RANGE_START,
Expand Down Expand Up @@ -108,11 +108,9 @@ export const getAllVariables = (
.concat([TIME_RANGE_START, TIME_RANGE_STOP, WINDOW_PERIOD])
.reduce((prev, curr) => {
prev.push(getVariable(state, curr))

return prev
}, [])
.filter(v => !!v)

return vars
}

Expand All @@ -126,8 +124,7 @@ export const getVariable = (state: AppState, variableID: string): Variable => {
}

if (variableID === TIME_RANGE_START || variableID === TIME_RANGE_STOP) {
const timeRange = getTimeRange(state)

const timeRange = getTimeRangeWithTimezone(state)
vari = getRangeVariable(variableID, timeRange)
}

Expand Down