From fcd0c382b6e064eb91e8a01572da9d8f0db2224b Mon Sep 17 00:00:00 2001 From: "Grot (@grafanabot)" <43478413+grafanabot@users.noreply.github.com> Date: Mon, 22 Nov 2021 02:04:34 -0500 Subject: [PATCH] Variables: Make renamed or missing variable section expandable (#41964) (#42025) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Variables: Make renamed or missing variable section expandable * Chore: feedback after PR comments (cherry picked from commit 44d7d6546f1a87cfb71e6c32875f79743772659d) Co-authored-by: Hugo Häggmark --- .../Collapse/CollapsableSection.tsx | 12 +- .../editor/VariableEditorContainer.tsx | 5 +- .../app/features/variables/editor/actions.ts | 6 +- .../inspect/VariablesUnknownButton.tsx | 9 +- .../inspect/VariablesUnknownTable.test.tsx | 137 +++++++++++++++ .../inspect/VariablesUnknownTable.tsx | 156 +++++++++++++----- .../variables/inspect/reducer.test.ts | 6 - .../app/features/variables/inspect/reducer.ts | 22 +-- .../app/features/variables/inspect/utils.ts | 52 ++++-- 9 files changed, 309 insertions(+), 96 deletions(-) create mode 100644 public/app/features/variables/inspect/VariablesUnknownTable.test.tsx diff --git a/packages/grafana-ui/src/components/Collapse/CollapsableSection.tsx b/packages/grafana-ui/src/components/Collapse/CollapsableSection.tsx index 1ef09c8ff7b..3e2ac0adc99 100644 --- a/packages/grafana-ui/src/components/Collapse/CollapsableSection.tsx +++ b/packages/grafana-ui/src/components/Collapse/CollapsableSection.tsx @@ -5,20 +5,26 @@ import { Icon } from '..'; import { GrafanaTheme2 } from '@grafana/data'; export interface Props { - label: string; + label: ReactNode; isOpen: boolean; + /** Callback for the toggle functionality */ + onToggle?: (isOpen: boolean) => void; children: ReactNode; } -export const CollapsableSection: FC = ({ label, isOpen, children }) => { +export const CollapsableSection: FC = ({ label, isOpen, onToggle, children }) => { const [open, toggleOpen] = useState(isOpen); const styles = useStyles2(collapsableSectionStyles); const headerStyle = open ? styles.header : styles.headerCollapsed; const tooltip = `Click to ${open ? 'collapse' : 'expand'}`; + const onClick = () => { + onToggle?.(!open); + toggleOpen(!open); + }; return (
-
toggleOpen(!open)} className={headerStyle} title={tooltip}> +
{label}
diff --git a/public/app/features/variables/editor/VariableEditorContainer.tsx b/public/app/features/variables/editor/VariableEditorContainer.tsx index 2ebafabb99b..047ba76059e 100644 --- a/public/app/features/variables/editor/VariableEditorContainer.tsx +++ b/public/app/features/variables/editor/VariableEditorContainer.tsx @@ -17,10 +17,7 @@ const mapStateToProps = (state: StoreState) => ({ variables: getEditorVariables(state), idInEditor: state.templating.editor.id, dashboard: state.dashboard.getModel(), - unknownsNetwork: state.templating.inspect.unknownsNetwork, - unknownExists: state.templating.inspect.unknownExits, usagesNetwork: state.templating.inspect.usagesNetwork, - unknown: state.templating.inspect.unknown, usages: state.templating.inspect.usages, }); @@ -119,7 +116,7 @@ class VariableEditorContainerUnconnected extends PureComponent { usages={this.props.usages} usagesNetwork={this.props.usagesNetwork} /> - {this.props.unknownExists ? : null} + )} {variableToEdit && } diff --git a/public/app/features/variables/editor/actions.ts b/public/app/features/variables/editor/actions.ts index ad922516815..67c5ae91227 100644 --- a/public/app/features/variables/editor/actions.ts +++ b/public/app/features/variables/editor/actions.ts @@ -109,12 +109,10 @@ export const switchToListMode = (): ThunkResult => (dispatch, getState) => const state = getState(); const variables = getEditorVariables(state); const dashboard = state.dashboard.getModel(); - const { unknown, usages } = createUsagesNetwork(variables, dashboard); - const unknownsNetwork = transformUsagesToNetwork(unknown); - const unknownExits = Object.keys(unknown).length > 0; + const { usages } = createUsagesNetwork(variables, dashboard); const usagesNetwork = transformUsagesToNetwork(usages); - dispatch(initInspect({ unknown, usages, usagesNetwork, unknownsNetwork, unknownExits })); + dispatch(initInspect({ usages, usagesNetwork })); }; export function getNextAvailableId(type: VariableType, variables: VariableModel[]): string { diff --git a/public/app/features/variables/inspect/VariablesUnknownButton.tsx b/public/app/features/variables/inspect/VariablesUnknownButton.tsx index 596d40268c0..a8ec1998118 100644 --- a/public/app/features/variables/inspect/VariablesUnknownButton.tsx +++ b/public/app/features/variables/inspect/VariablesUnknownButton.tsx @@ -25,7 +25,14 @@ export const VariablesUnknownButton: FC = ({ id, usages }) => { return ( {({ showModal }) => { - return showModal()} name="code-branch" title="Show usages" />; + return ( + showModal()} + name="code-branch" + title="Show usages" + data-testid="VariablesUnknownButton" + /> + ); }} ); diff --git a/public/app/features/variables/inspect/VariablesUnknownTable.test.tsx b/public/app/features/variables/inspect/VariablesUnknownTable.test.tsx new file mode 100644 index 00000000000..7fb888ca539 --- /dev/null +++ b/public/app/features/variables/inspect/VariablesUnknownTable.test.tsx @@ -0,0 +1,137 @@ +import React from 'react'; +import * as runtime from '@grafana/runtime'; +import { render, screen, waitFor, waitForElementToBeRemoved } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +import { VariablesUnknownTable, VariablesUnknownTableProps } from './VariablesUnknownTable'; +import { customBuilder } from '../shared/testing/builders'; +import * as utils from './utils'; +import { UsagesToNetwork } from './utils'; + +async function getTestContext( + overrides: Partial | undefined = {}, + usages: UsagesToNetwork[] = [] +) { + jest.clearAllMocks(); + const reportInteractionSpy = jest.spyOn(runtime, 'reportInteraction').mockImplementation(); + const getUnknownsNetworkSpy = jest.spyOn(utils, 'getUnknownsNetwork').mockResolvedValue(usages); + const defaults: VariablesUnknownTableProps = { + variables: [], + dashboard: null, + }; + const props = { ...defaults, ...overrides }; + const { rerender } = render(); + await waitFor(() => + expect(screen.getByRole('heading', { name: /renamed or missing variables/i })).toBeInTheDocument() + ); + + return { reportInteractionSpy, getUnknownsNetworkSpy, rerender }; +} + +describe('VariablesUnknownTable', () => { + describe('when rendered', () => { + it('then it should render the section header', async () => { + await getTestContext(); + }); + }); + + describe('when expanding the section', () => { + it('then it should show loading spinner', async () => { + await getTestContext(); + + userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i })); + await waitFor(() => expect(screen.getByText('Loading...')).toBeInTheDocument()); + }); + + it('then it should call getUnknownsNetwork', async () => { + const { getUnknownsNetworkSpy } = await getTestContext(); + + userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i })); + await waitFor(() => expect(getUnknownsNetworkSpy).toHaveBeenCalledTimes(1)); + }); + + it('then it should report the interaction', async () => { + const { reportInteractionSpy } = await getTestContext(); + + userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i })); + await waitFor(() => expect(screen.getByText('Loading...')).toBeInTheDocument()); + + expect(reportInteractionSpy).toHaveBeenCalledTimes(1); + expect(reportInteractionSpy).toHaveBeenCalledWith('Unknown variables section expanded'); + }); + + describe('but when expanding it again without changes to variables or dashboard', () => { + it('then it should not call getUnknownsNetwork', async () => { + const { getUnknownsNetworkSpy } = await getTestContext(); + + userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i })); + await waitFor(() => expect(screen.getByTitle('Click to collapse')).toBeInTheDocument()); + expect(getUnknownsNetworkSpy).toHaveBeenCalledTimes(1); + + userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i })); + await waitFor(() => expect(screen.getByTitle('Click to expand')).toBeInTheDocument()); + + userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i })); + await waitFor(() => expect(screen.getByTitle('Click to collapse')).toBeInTheDocument()); + + expect(getUnknownsNetworkSpy).toHaveBeenCalledTimes(1); + }); + }); + + describe('and there are no renamed or missing variables', () => { + it('then it should render the correct message', async () => { + await getTestContext(); + + userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i })); + await waitForElementToBeRemoved(() => screen.getByText('Loading...')); + + expect(screen.getByText('No renamed or missing variables found.')).toBeInTheDocument(); + }); + }); + + describe('and there are renamed or missing variables', () => { + it('then it should render the table', async () => { + const variable = customBuilder().withId('Renamed Variable').withName('Renamed Variable').build(); + const usages = [{ variable, nodes: [], edges: [], showGraph: false }]; + const { reportInteractionSpy } = await getTestContext({}, usages); + + userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i })); + await waitForElementToBeRemoved(() => screen.getByText('Loading...')); + + expect(screen.queryByText('No renamed or missing variables found.')).not.toBeInTheDocument(); + expect(screen.getByText('Renamed Variable')).toBeInTheDocument(); + expect(screen.getAllByTestId('VariablesUnknownButton')).toHaveLength(1); + + // make sure we don't report the interaction for slow expansion + expect(reportInteractionSpy).toHaveBeenCalledTimes(1); + expect(reportInteractionSpy).toHaveBeenCalledWith('Unknown variables section expanded'); + }); + + describe('but when the unknown processing takes a while', () => { + const origDateNow = Date.now; + + afterEach(() => { + Date.now = origDateNow; + }); + + it('then it should report slow expansion', async () => { + const variable = customBuilder().withId('Renamed Variable').withName('Renamed Variable').build(); + const usages = [{ variable, nodes: [], edges: [], showGraph: false }]; + const { reportInteractionSpy } = await getTestContext({}, usages); + const dateNowStart = 1000; + const dateNowStop = 2000; + Date.now = jest.fn().mockReturnValueOnce(dateNowStart).mockReturnValue(dateNowStop); + + userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i })); + await waitForElementToBeRemoved(() => screen.getByText('Loading...')); + + // make sure we report the interaction for slow expansion + expect(reportInteractionSpy).toHaveBeenCalledTimes(2); + expect(reportInteractionSpy.mock.calls[0][0]).toEqual('Unknown variables section expanded'); + expect(reportInteractionSpy.mock.calls[1][0]).toEqual('Slow unknown variables expansion'); + expect(reportInteractionSpy.mock.calls[1][1]).toEqual({ elapsed: 1000 }); + }); + }); + }); + }); +}); diff --git a/public/app/features/variables/inspect/VariablesUnknownTable.tsx b/public/app/features/variables/inspect/VariablesUnknownTable.tsx index 7a2435c81df..5c45b6a5b88 100644 --- a/public/app/features/variables/inspect/VariablesUnknownTable.tsx +++ b/public/app/features/variables/inspect/VariablesUnknownTable.tsx @@ -1,63 +1,129 @@ -import React, { FC } from 'react'; +import React, { ReactElement, useEffect, useState } from 'react'; import { css } from '@emotion/css'; -import { Icon, Tooltip, useStyles } from '@grafana/ui'; +import { useAsync } from 'react-use'; +import { CollapsableSection, HorizontalGroup, Icon, Spinner, Tooltip, useStyles, VerticalGroup } from '@grafana/ui'; import { GrafanaTheme } from '@grafana/data'; -import { UsagesToNetwork } from './utils'; -import { VariablesUnknownButton } from './VariablesUnknownButton'; +import { reportInteraction } from '@grafana/runtime'; -interface Props { - usages: UsagesToNetwork[]; +import { VariableModel } from '../types'; +import { DashboardModel } from '../../dashboard/state'; +import { VariablesUnknownButton } from './VariablesUnknownButton'; +import { getUnknownsNetwork, UsagesToNetwork } from './utils'; + +export const SLOW_VARIABLES_EXPANSION_THRESHOLD = 1000; + +export interface VariablesUnknownTableProps { + variables: VariableModel[]; + dashboard: DashboardModel | null; } -export const VariablesUnknownTable: FC = ({ usages }) => { +export function VariablesUnknownTable({ variables, dashboard }: VariablesUnknownTableProps): ReactElement { + const [open, setOpen] = useState(false); + const [changed, setChanged] = useState(0); + const [usages, setUsages] = useState([]); const style = useStyles(getStyles); + useEffect(() => setChanged((prevState) => prevState + 1), [variables, dashboard]); + const { loading } = useAsync(async () => { + if (open && changed > 0) { + // make sure we only fetch when opened and variables or dashboard have changed + const start = Date.now(); + const unknownsNetwork = await getUnknownsNetwork(variables, dashboard); + const stop = Date.now(); + const elapsed = stop - start; + if (elapsed >= SLOW_VARIABLES_EXPANSION_THRESHOLD) { + reportInteraction('Slow unknown variables expansion', { elapsed }); + } + setChanged(0); + setUsages(unknownsNetwork); + return unknownsNetwork; + } + + return []; + }, [variables, dashboard, open, changed]); + + const onToggle = (isOpen: boolean) => { + if (isOpen) { + reportInteraction('Unknown variables section expanded'); + } + + setOpen(isOpen); + }; + return (
-
- Unknown Variables - - - -
- -
- - - - - - - - {usages.map((usage) => { - const { variable } = usage; - const { id, name } = variable; - return ( - - - - - ); - })} - -
Variable -
- {name} - - - - - -
-
+ } isOpen={open} onToggle={onToggle}> + {loading && ( + + + Loading... + + + + )} + {!loading && usages && ( + <> + {usages.length === 0 && } + {usages.length > 0 && } + + )} +
); -}; +} + +function CollapseLabel(): ReactElement { + const style = useStyles(getStyles); + return ( +
+ Renamed or missing variables + + + +
+ ); +} + +function NoUnknowns(): ReactElement { + return No renamed or missing variables found.; +} + +function UnknownTable({ usages }: { usages: UsagesToNetwork[] }): ReactElement { + const style = useStyles(getStyles); + return ( + + + + + + + + {usages.map((usage) => { + const { variable } = usage; + const { id, name } = variable; + return ( + + + + + ); + })} + +
Variable +
+ {name} + + + + + +
+ ); +} const getStyles = (theme: GrafanaTheme) => ({ container: css` margin-top: ${theme.spacing.xl}; padding-top: ${theme.spacing.xl}; - border-top: 1px solid ${theme.colors.panelBorder}; `, infoIcon: css` margin-left: ${theme.spacing.sm}; diff --git a/public/app/features/variables/inspect/reducer.test.ts b/public/app/features/variables/inspect/reducer.test.ts index e7d8c763d1a..75218bcb366 100644 --- a/public/app/features/variables/inspect/reducer.test.ts +++ b/public/app/features/variables/inspect/reducer.test.ts @@ -10,19 +10,13 @@ describe('variableInspectReducer', () => { .givenReducer(variableInspectReducer, { ...initialVariableInspectState }) .whenActionIsDispatched( initInspect({ - unknownExits: true, - unknownsNetwork: [{ edges: [], nodes: [], showGraph: true, variable }], usagesNetwork: [{ edges: [], nodes: [], showGraph: true, variable }], usages: [{ variable, tree: {} }], - unknown: [{ variable, tree: {} }], }) ) .thenStateShouldEqual({ - unknownExits: true, - unknownsNetwork: [{ edges: [], nodes: [], showGraph: true, variable }], usagesNetwork: [{ edges: [], nodes: [], showGraph: true, variable }], usages: [{ variable, tree: {} }], - unknown: [{ variable, tree: {} }], }); }); }); diff --git a/public/app/features/variables/inspect/reducer.ts b/public/app/features/variables/inspect/reducer.ts index e1101727db8..8ec775d4d1a 100644 --- a/public/app/features/variables/inspect/reducer.ts +++ b/public/app/features/variables/inspect/reducer.ts @@ -2,40 +2,22 @@ import { UsagesToNetwork, VariableUsageTree } from './utils'; import { createSlice, PayloadAction } from '@reduxjs/toolkit'; export interface VariableInspectState { - unknown: VariableUsageTree[]; usages: VariableUsageTree[]; - unknownsNetwork: UsagesToNetwork[]; usagesNetwork: UsagesToNetwork[]; - unknownExits: boolean; } export const initialVariableInspectState: VariableInspectState = { - unknown: [], usages: [], - unknownsNetwork: [], usagesNetwork: [], - unknownExits: false, }; const variableInspectReducerSlice = createSlice({ name: 'templating/inspect', initialState: initialVariableInspectState, reducers: { - initInspect: ( - state, - action: PayloadAction<{ - unknown: VariableUsageTree[]; - usages: VariableUsageTree[]; - unknownsNetwork: UsagesToNetwork[]; - usagesNetwork: UsagesToNetwork[]; - unknownExits: boolean; - }> - ) => { - const { unknown, usages, unknownExits, unknownsNetwork, usagesNetwork } = action.payload; + initInspect: (state, action: PayloadAction<{ usages: VariableUsageTree[]; usagesNetwork: UsagesToNetwork[] }>) => { + const { usages, usagesNetwork } = action.payload; state.usages = usages; - state.unknown = unknown; - state.unknownsNetwork = unknownsNetwork; - state.unknownExits = unknownExits; state.usagesNetwork = usagesNetwork; }, }, diff --git a/public/app/features/variables/inspect/utils.ts b/public/app/features/variables/inspect/utils.ts index 35f77855016..45943f15c4f 100644 --- a/public/app/features/variables/inspect/utils.ts +++ b/public/app/features/variables/inspect/utils.ts @@ -179,29 +179,18 @@ export interface VariableUsageTree { export interface VariableUsages { unUsed: VariableModel[]; - unknown: VariableUsageTree[]; usages: VariableUsageTree[]; } export const createUsagesNetwork = (variables: VariableModel[], dashboard: DashboardModel | null): VariableUsages => { if (!dashboard) { - return { unUsed: [], unknown: [], usages: [] }; + return { unUsed: [], usages: [] }; } const unUsed: VariableModel[] = []; let usages: VariableUsageTree[] = []; - let unknown: VariableUsageTree[] = []; const model = dashboard.getSaveModelClone(); - const unknownVariables = getUnknownVariableStrings(variables, model); - for (const unknownVariable of unknownVariables) { - const props = getPropsWithVariable(unknownVariable, { key: 'model', value: model }, {}); - if (Object.keys(props).length) { - const variable = ({ id: unknownVariable, name: unknownVariable } as unknown) as VariableModel; - unknown.push({ variable, tree: props }); - } - } - for (const variable of variables) { const variableId = variable.id; const props = getPropsWithVariable(variableId, { key: 'model', value: model }, {}); @@ -214,9 +203,46 @@ export const createUsagesNetwork = (variables: VariableModel[], dashboard: Dashb } } - return { unUsed, unknown, usages }; + return { unUsed, usages }; }; +export async function getUnknownsNetwork( + variables: VariableModel[], + dashboard: DashboardModel | null +): Promise { + return new Promise((resolve, reject) => { + // can be an expensive call so we avoid blocking the main thread + setTimeout(() => { + try { + const unknowns = createUnknownsNetwork(variables, dashboard); + resolve(transformUsagesToNetwork(unknowns)); + } catch (e) { + reject(e); + } + }, 200); + }); +} + +function createUnknownsNetwork(variables: VariableModel[], dashboard: DashboardModel | null): VariableUsageTree[] { + if (!dashboard) { + return []; + } + + let unknown: VariableUsageTree[] = []; + const model = dashboard.getSaveModelClone(); + + const unknownVariables = getUnknownVariableStrings(variables, model); + for (const unknownVariable of unknownVariables) { + const props = getPropsWithVariable(unknownVariable, { key: 'model', value: model }, {}); + if (Object.keys(props).length) { + const variable = ({ id: unknownVariable, name: unknownVariable } as unknown) as VariableModel; + unknown.push({ variable, tree: props }); + } + } + + return unknown; +} + /* getAllAffectedPanelIdsForVariableChange is a function that extracts all the panel ids that are affected by a single variable change. It will traverse all chained variables to identify all cascading changes too.