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(ui): adding times in script editor will default start and endtime for timerange queries #16201

Merged
merged 7 commits into from
Dec 12, 2019
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
1. [16175](https://github.com/influxdata/influxdb/pull/16175): Added delete functionality to note cells so that they can be deleted
1. [16204](https://github.com/influxdata/influxdb/pull/16204): Fix failure to create labels when creating telegraf configs
1. [16207](https://github.com/influxdata/influxdb/pull/16207): Fix crash when editing a Telegraf config
1. [16201](https://github.com/influxdata/influxdb/pull/16201): Updated start/endtime functionality so that custom script timeranges overwrite dropdown selections

### UI Improvements

Expand Down
16 changes: 9 additions & 7 deletions ui/src/shared/components/HeatmapPlot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,25 @@ import {DEFAULT_LINE_COLORS} from 'src/shared/constants/graphColorPalettes'
import {INVALID_DATA_COPY} from 'src/shared/copy/cell'

// Types
import {RemoteDataState, HeatmapViewProperties, TimeZone} from 'src/types'
import {
RemoteDataState,
HeatmapViewProperties,
TimeZone,
TimeRange,
} from 'src/types'

interface Props {
endTime: number
loading: RemoteDataState
startTime: number
timeRange: TimeRange | null
asalem1 marked this conversation as resolved.
Show resolved Hide resolved
table: Table
timeZone: TimeZone
viewProperties: HeatmapViewProperties
children: (config: Config) => JSX.Element
}

const HeatmapPlot: FunctionComponent<Props> = ({
endTime,
loading,
startTime,
timeRange,
table,
timeZone,
viewProperties: {
Expand All @@ -56,8 +59,7 @@ const HeatmapPlot: FunctionComponent<Props> = ({
const [xDomain, onSetXDomain, onResetXDomain] = useVisDomainSettings(
storedXDomain,
table.getColumn(xColumn, 'number'),
startTime,
endTime
timeRange
asalem1 marked this conversation as resolved.
Show resolved Hide resolved
)

const [yDomain, onSetYDomain, onResetYDomain] = useVisDomainSettings(
Expand Down
34 changes: 11 additions & 23 deletions ui/src/shared/components/RefreshingView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,12 @@ import ViewSwitcher from 'src/shared/components/ViewSwitcher'
// Utils
import {GlobalAutoRefresher} from 'src/utils/AutoRefresher'
import {getTimeRangeVars} from 'src/variables/utils/getTimeRangeVars'
import {getVariableAssignments} from 'src/variables/selectors'
import {getDashboardValuesStatus} from 'src/variables/selectors'
import {
getVariableAssignments,
getDashboardValuesStatus,
} from 'src/variables/selectors'
import {checkResultsLength} from 'src/shared/utils/vis'

// Selectors
import {getEndTime, getStartTime} from 'src/timeMachine/selectors/index'
import {getTimeRangeByDashboardID} from 'src/dashboards/selectors/index'
import {getActiveTimeRange} from 'src/timeMachine/selectors/index'

// Types
import {
Expand All @@ -39,8 +38,7 @@ interface OwnProps {
}

interface StateProps {
endTime: number
startTime: number
ranges: TimeRange | null
timeZone: TimeZone
variableAssignments: VariableAssignment[]
variablesStatus: RemoteDataState
Expand Down Expand Up @@ -73,14 +71,7 @@ class RefreshingView extends PureComponent<Props, State> {
}

public render() {
const {
check,
endTime,
properties,
manualRefresh,
startTime,
timeZone,
} = this.props
const {check, ranges, properties, manualRefresh, timeZone} = this.props
const {submitToken} = this.state

return (
Expand Down Expand Up @@ -111,12 +102,11 @@ class RefreshingView extends PureComponent<Props, State> {
>
<ViewSwitcher
check={check}
endTime={endTime}
files={files}
giraffeResult={giraffeResult}
loading={loading}
properties={properties}
startTime={startTime}
timeRange={ranges}
statuses={statuses}
timeZone={timeZone}
/>
Expand Down Expand Up @@ -168,15 +158,13 @@ const mstp = (state: AppState, ownProps: OwnProps): StateProps => {
state,
ownProps.dashboardID
)
const timeRange = getTimeRangeByDashboardID(state, ownProps.dashboardID)

const valuesStatus = getDashboardValuesStatus(state, ownProps.dashboardID)

const {properties} = ownProps
const timeRange = getActiveTimeRange(ownProps.timeRange, properties.queries)
const timeZone = state.app.persisted.timeZone

return {
endTime: getEndTime(timeRange),
startTime: getStartTime(timeRange),
ranges: timeRange,
Copy link
Contributor

Choose a reason for hiding this comment

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

let's keep calling this timeRange to signal to the next programmer that this is the same old timeRange that they know from the rest of the app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but we have timeRange in the OwnProps :(

Copy link
Contributor

Choose a reason for hiding this comment

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

why are we passing the same prop to the same component twice? What is the difference between them? Which one is redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The difference between them is that the timeRange refers to the dropdown selection while the ranges refer to the specific ranges in the query script editor

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct.

timeZone,
variableAssignments,
variablesStatus: valuesStatus,
Expand Down
16 changes: 9 additions & 7 deletions ui/src/shared/components/ScatterPlot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,24 +20,27 @@ import {DEFAULT_LINE_COLORS} from 'src/shared/constants/graphColorPalettes'
import {INVALID_DATA_COPY} from 'src/shared/copy/cell'

// Types
import {RemoteDataState, ScatterViewProperties, TimeZone} from 'src/types'
import {
RemoteDataState,
ScatterViewProperties,
TimeZone,
TimeRange,
} from 'src/types'

interface Props {
children: (config: Config) => JSX.Element
endTime: number
fluxGroupKeyUnion?: string[]
loading: RemoteDataState
startTime: number
timeRange: TimeRange | null
table: Table
timeZone: TimeZone
viewProperties: ScatterViewProperties
}

const ScatterPlot: FunctionComponent<Props> = ({
children,
endTime,
loading,
startTime,
timeRange,
timeZone,
table,
viewProperties: {
Expand Down Expand Up @@ -68,8 +71,7 @@ const ScatterPlot: FunctionComponent<Props> = ({
const [xDomain, onSetXDomain, onResetXDomain] = useVisDomainSettings(
storedXDomain,
table.getColumn(xColumn, 'number'),
startTime,
endTime
timeRange
)

const [yDomain, onSetYDomain, onResetYDomain] = useVisDomainSettings(
Expand Down
19 changes: 7 additions & 12 deletions ui/src/shared/components/ViewSwitcher.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
StatusRow,
TimeZone,
XYViewProperties,
TimeRange,
} from 'src/types'

interface Props {
Expand All @@ -34,18 +35,16 @@ interface Props {
properties: QueryViewProperties | CheckViewProperties
timeZone: TimeZone
statuses: StatusRow[][]
endTime: number
startTime: number
timeRange: TimeRange | null
}

const ViewSwitcher: FunctionComponent<Props> = ({
properties,
check,
loading,
endTime,
timeRange,
files,
giraffeResult: {table, fluxGroupKeyUnion},
startTime,
timeZone,
statuses,
}) => {
Expand Down Expand Up @@ -83,10 +82,9 @@ const ViewSwitcher: FunctionComponent<Props> = ({
case 'xy':
return (
<XYPlot
endTime={endTime}
timeRange={timeRange}
fluxGroupKeyUnion={fluxGroupKeyUnion}
loading={loading}
startTime={startTime}
table={table}
timeZone={timeZone}
viewProperties={properties}
Expand All @@ -111,10 +109,9 @@ const ViewSwitcher: FunctionComponent<Props> = ({

return (
<XYPlot
endTime={endTime}
timeRange={timeRange}
fluxGroupKeyUnion={fluxGroupKeyUnion}
loading={loading}
startTime={startTime}
table={table}
timeZone={timeZone}
viewProperties={xyProperties}
Expand Down Expand Up @@ -153,9 +150,8 @@ const ViewSwitcher: FunctionComponent<Props> = ({
case 'heatmap':
return (
<HeatmapPlot
endTime={endTime}
timeRange={timeRange}
loading={loading}
startTime={startTime}
table={table}
timeZone={timeZone}
viewProperties={properties}
Expand All @@ -167,9 +163,8 @@ const ViewSwitcher: FunctionComponent<Props> = ({
case 'scatter':
return (
<ScatterPlot
endTime={endTime}
timeRange={timeRange}
loading={loading}
startTime={startTime}
table={table}
viewProperties={properties}
timeZone={timeZone}
Expand Down
11 changes: 4 additions & 7 deletions ui/src/shared/components/XYPlot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,25 +23,23 @@ import {DEFAULT_LINE_COLORS} from 'src/shared/constants/graphColorPalettes'
import {INVALID_DATA_COPY} from 'src/shared/copy/cell'

// Types
import {RemoteDataState, XYViewProperties, TimeZone} from 'src/types'
import {RemoteDataState, XYViewProperties, TimeZone, TimeRange} from 'src/types'

interface Props {
children: (config: Config) => JSX.Element
endTime: number
fluxGroupKeyUnion: string[]
loading: RemoteDataState
startTime: number
timeRange: TimeRange | null
table: Table
timeZone: TimeZone
viewProperties: XYViewProperties
}

const XYPlot: FunctionComponent<Props> = ({
children,
endTime,
fluxGroupKeyUnion,
loading,
startTime,
timeRange,
table,
timeZone,
viewProperties: {
Expand Down Expand Up @@ -81,8 +79,7 @@ const XYPlot: FunctionComponent<Props> = ({
const [xDomain, onSetXDomain, onResetXDomain] = useVisDomainSettings(
storedXDomain,
table.getColumn(xColumn, 'number'),
startTime,
endTime
timeRange
)

const [yDomain, onSetYDomain, onResetYDomain] = useVisDomainSettings(
Expand Down
43 changes: 32 additions & 11 deletions ui/src/shared/utils/useVisDomainSettings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,29 +3,50 @@ import {getValidRange} from 'src/shared/utils/useVisDomainSettings'

// Types
import {numericColumnData as data} from 'mocks/dummyData'
import {CustomTimeRange} from 'src/types/queries'

describe('getValidRange', () => {
const startTime: number = 1573123611000
const endTime: number = 1574981211000
const startTime: string = 'Nov 07 2019 02:46:51 GMT-0800'
const unixStart: number = 1573123611000
const endTime: string = 'Nov 28 2019 14:46:51 GMT-0800'
const unixEnd: number = 1574981211000
it('should return null when no parameters are input', () => {
expect(getValidRange()).toEqual(null)
expect(getValidRange(undefined, undefined)).toEqual(null)
})
it('should return null when no data is passed', () => {
expect(getValidRange([], startTime, endTime)).toEqual(null)
const timeRange: CustomTimeRange = {
type: 'custom',
lower: startTime,
upper: endTime,
}
expect(getValidRange([], timeRange)).toEqual(null)
})
it("should return the startTime as startTime if it's before the first time in the data array", () => {
const [start] = getValidRange(data, startTime, endTime)
expect(start).toEqual(startTime)
const [beginning] = getValidRange(data, endTime, endTime)
const timeRange: CustomTimeRange = {
type: 'custom',
lower: startTime,
upper: endTime,
}
const [start] = getValidRange(data, timeRange)
expect(start).toEqual(unixStart)
timeRange.lower = endTime
const [beginning] = getValidRange(data, timeRange)
expect(beginning).toEqual(data[0])
})
it("should return the endTime as endTime if it's before the last time in the data array", () => {
const range = getValidRange(data, startTime, endTime)
expect(range[1]).toEqual(endTime)
const newRange = getValidRange(data, endTime, startTime)
const timeRange: CustomTimeRange = {
type: 'custom',
lower: startTime,
upper: endTime,
}
const range = getValidRange(data, timeRange)
expect(range[1]).toEqual(unixEnd)
timeRange.lower = endTime
timeRange.upper = startTime
const newRange = getValidRange(data, timeRange)
expect(newRange[1]).toEqual(data[data.length - 1])
})
it('should return the the start and end times based on the data array if no start / endTime are passed', () => {
expect(getValidRange(data)).toEqual([data[0], data[data.length - 1]])
expect(getValidRange(data, null)).toEqual([data[0], data[data.length - 1]])
})
})
16 changes: 11 additions & 5 deletions ui/src/shared/utils/useVisDomainSettings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ import {NumericColumnData} from '@influxdata/giraffe'
// Utils
import {useOneWayState} from 'src/shared/utils/useOneWayState'
import {extent} from 'src/shared/utils/vis'
import {getStartTime, getEndTime} from 'src/timeMachine/selectors/index'

// Types
import {TimeRange} from 'src/types'
/*
This hook helps map the domain setting stored for line graph to the
appropriate settings on a @influxdata/giraffe `Config` object.
Expand All @@ -16,11 +19,15 @@ import {extent} from 'src/shared/utils/vis'
*/
export const getValidRange = (
data: NumericColumnData = [],
startTime: number = Infinity,
endTime: number = -Infinity
timeRange: TimeRange | null
) => {
const range = extent((data as number[]) || [])
if (!timeRange) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure- but I wonder if this should be _.isNull(timeRange) so that we don't also return range in a situation when timeRange is unexpectedly undefined. hmm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wouldn't we want to return the range is timeRange is undefined?

Copy link
Contributor

Choose a reason for hiding this comment

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

that might happen for a variety of reasons. we don't know what to do if timeRange is undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's reasonable

return range
}
if (range && range.length >= 2) {
const startTime = getStartTime(timeRange)
const endTime = getEndTime(timeRange)
const start = Math.min(startTime, range[0])
const end = Math.max(endTime, range[1])
return [start, end]
Expand All @@ -31,15 +38,14 @@ export const getValidRange = (
export const useVisDomainSettings = (
storedDomain: number[],
data: NumericColumnData,
startTime: number = Infinity,
endTime: number = -Infinity
timeRange: TimeRange | null = null
asalem1 marked this conversation as resolved.
Show resolved Hide resolved
) => {
const initialDomain = useMemo(() => {
if (storedDomain) {
return storedDomain
}

return getValidRange(data, startTime, endTime)
return getValidRange(data, timeRange)
}, [storedDomain, data])

const [domain, setDomain] = useOneWayState(initialDomain)
Expand Down
Loading