From 24bdda72fbf7eed224061e39edb4d243396c39be Mon Sep 17 00:00:00 2001 From: Josh Hunt Date: Tue, 21 Jun 2022 23:45:37 +0100 Subject: [PATCH] Barchart: Fix warning not showing (#51190) --- .../plugins/panel/barchart/BarChartPanel.tsx | 8 +-- public/app/plugins/panel/barchart/module.tsx | 2 +- public/app/plugins/panel/barchart/types.ts | 8 +-- .../app/plugins/panel/barchart/utils.test.ts | 51 ++++++++++++------- public/app/plugins/panel/barchart/utils.ts | 16 +++--- 5 files changed, 52 insertions(+), 33 deletions(-) diff --git a/public/app/plugins/panel/barchart/BarChartPanel.tsx b/public/app/plugins/panel/barchart/BarChartPanel.tsx index b8058520cdd..0a58e65beaa 100644 --- a/public/app/plugins/panel/barchart/BarChartPanel.tsx +++ b/public/app/plugins/panel/barchart/BarChartPanel.tsx @@ -102,6 +102,8 @@ export const BarChartPanel: React.FunctionComponent = ({ const frame0Ref = useRef(); const info = useMemo(() => prepareBarChartDisplayValues(data?.series, theme, options), [data, theme, options]); + const chartDisplay = 'viz' in info ? info : null; + const structureRef = useRef(10000); useMemo(() => { structureRef.current++; @@ -109,14 +111,14 @@ export const BarChartPanel: React.FunctionComponent = ({ }, [options]); // change every time the options object changes (while editing) const structureRev = useMemo(() => { - const f0 = info.viz[0]; + const f0 = chartDisplay?.viz[0]; const f1 = frame0Ref.current; if (!(f0 && f1 && compareDataFrameStructures(f0, f1, true))) { structureRef.current++; } frame0Ref.current = f0; return (data.structureRev ?? 0) + structureRef.current; - }, [info, data.structureRev]); + }, [chartDisplay, data.structureRev]); const orientation = useMemo(() => { if (!options.orientation || options.orientation === VizOrientation.Auto) { @@ -142,7 +144,7 @@ export const BarChartPanel: React.FunctionComponent = ({ } }, [height, options.xTickLabelRotation, options.xTickLabelMaxLength]); - if (!info.viz[0]?.fields.length) { + if ('warn' in info) { return ( (BarChar .setPanelOptions((builder, context) => { const disp = prepareBarChartDisplayValues(context.data, config.theme2, context.options ?? ({} as any)); let xaxisPlaceholder = 'First string or time field'; - const viz = disp.viz ? disp.viz[0] : undefined; + const viz = 'viz' in disp ? disp.viz[0] : undefined; if (viz?.fields?.length) { const first = viz.fields[0]; xaxisPlaceholder += ` (${getFieldDisplayName(first, viz)})`; diff --git a/public/app/plugins/panel/barchart/types.ts b/public/app/plugins/panel/barchart/types.ts index c91940d7c09..f694f369a19 100644 --- a/public/app/plugins/panel/barchart/types.ts +++ b/public/app/plugins/panel/barchart/types.ts @@ -1,9 +1,6 @@ import { DataFrame, Field } from '@grafana/data'; export interface BarChartDisplayValues { - /** When the data can not display, this will be returned */ - warn?: string; - /** All fields joined */ aligned: DataFrame; @@ -16,3 +13,8 @@ export interface BarChartDisplayValues { /** Potentialy color by a field value */ colorByField?: Field; } + +export interface BarChartDisplayWarning { + /** When the data can not display, this will be returned */ + warn: string; +} diff --git a/public/app/plugins/panel/barchart/utils.test.ts b/public/app/plugins/panel/barchart/utils.test.ts index 7f21f76c68b..124fc030518 100644 --- a/public/app/plugins/panel/barchart/utils.test.ts +++ b/public/app/plugins/panel/barchart/utils.test.ts @@ -1,3 +1,5 @@ +import { assertIsDefined } from 'test/helpers/asserts'; + import { createTheme, DefaultTimeZone, @@ -67,7 +69,13 @@ function mockDataFrame() { state: {}, }); - return prepareBarChartDisplayValues([df1], createTheme(), {} as any).aligned; + const info = prepareBarChartDisplayValues([df1], createTheme(), {} as any); + + if (!('aligned' in info)) { + throw new Error('Bar chart not prepared correctly'); + } + + return info.aligned; } jest.mock('@grafana/data', () => ({ @@ -150,7 +158,9 @@ describe('BarChart utils', () => { describe('prepareGraphableFrames', () => { it('will warn when there is no data in the response', () => { const result = prepareBarChartDisplayValues([], createTheme(), { stacking: StackingMode.None } as any); - expect(result.warn).toEqual('No data in response'); + const warning = assertIsDefined('warn' in result ? result : null); + + expect(warning.warn).toEqual('No data in response'); }); it('will warn when there is no string or time field', () => { @@ -161,8 +171,9 @@ describe('BarChart utils', () => { ], }); const result = prepareBarChartDisplayValues([df], createTheme(), { stacking: StackingMode.None } as any); - expect(result.warn).toEqual('Bar charts requires a string or time field'); - expect(result.viz).toBeUndefined(); + const warning = assertIsDefined('warn' in result ? result : null); + expect(warning.warn).toEqual('Bar charts requires a string or time field'); + expect(warning).not.toHaveProperty('viz'); }); it('will warn when there are no numeric fields in the response', () => { @@ -173,8 +184,9 @@ describe('BarChart utils', () => { ], }); const result = prepareBarChartDisplayValues([df], createTheme(), { stacking: StackingMode.None } as any); - expect(result.warn).toEqual('No numeric fields found'); - expect(result.viz).toBeUndefined(); + const warning = assertIsDefined('warn' in result ? result : null); + expect(warning.warn).toEqual('No numeric fields found'); + expect(warning).not.toHaveProperty('viz'); }); it('will convert NaN and Infinty to nulls', () => { @@ -185,9 +197,10 @@ describe('BarChart utils', () => { ], }); const result = prepareBarChartDisplayValues([df], createTheme(), { stacking: StackingMode.None } as any); + const displayValues = assertIsDefined('viz' in result ? result : null); - const field = result.viz[0].fields[1]; - expect(field!.values.toArray()).toMatchInlineSnapshot(` + const field = displayValues.viz[0].fields[1]; + expect(field.values.toArray()).toMatchInlineSnapshot(` Array [ -10, null, @@ -210,19 +223,21 @@ describe('BarChart utils', () => { const resultAsc = prepareBarChartDisplayValues([frame], createTheme(), { legend: { sortBy: 'Min', sortDesc: false }, - } as any).viz[0]; - expect(resultAsc.fields[0].type).toBe(FieldType.string); - expect(resultAsc.fields[1].name).toBe('a'); - expect(resultAsc.fields[2].name).toBe('c'); - expect(resultAsc.fields[3].name).toBe('b'); + } as any); + const displayValuesAsc = assertIsDefined('viz' in resultAsc ? resultAsc : null).viz[0]; + expect(displayValuesAsc.fields[0].type).toBe(FieldType.string); + expect(displayValuesAsc.fields[1].name).toBe('a'); + expect(displayValuesAsc.fields[2].name).toBe('c'); + expect(displayValuesAsc.fields[3].name).toBe('b'); const resultDesc = prepareBarChartDisplayValues([frame], createTheme(), { legend: { sortBy: 'Min', sortDesc: true }, - } as any).viz[0]; - expect(resultDesc.fields[0].type).toBe(FieldType.string); - expect(resultDesc.fields[1].name).toBe('b'); - expect(resultDesc.fields[2].name).toBe('c'); - expect(resultDesc.fields[3].name).toBe('a'); + } as any); + const displayValuesDesc = assertIsDefined('viz' in resultDesc ? resultDesc : null).viz[0]; + expect(displayValuesDesc.fields[0].type).toBe(FieldType.string); + expect(displayValuesDesc.fields[1].name).toBe('b'); + expect(displayValuesDesc.fields[2].name).toBe('c'); + expect(displayValuesDesc.fields[3].name).toBe('a'); }); }); }); diff --git a/public/app/plugins/panel/barchart/utils.ts b/public/app/plugins/panel/barchart/utils.ts index 77057d7a89c..1a45b42a2bb 100644 --- a/public/app/plugins/panel/barchart/utils.ts +++ b/public/app/plugins/panel/barchart/utils.ts @@ -30,7 +30,7 @@ import { findField } from 'app/features/dimensions'; import { BarsOptions, getConfig } from './bars'; import { BarChartFieldConfig, PanelOptions, defaultBarChartFieldConfig } from './models.gen'; -import { BarChartDisplayValues } from './types'; +import { BarChartDisplayValues, BarChartDisplayWarning } from './types'; function getBarCharScaleOrientation(orientation: VizOrientation) { if (orientation === VizOrientation.Vertical) { @@ -298,9 +298,9 @@ export function prepareBarChartDisplayValues( series: DataFrame[], theme: GrafanaTheme2, options: PanelOptions -): BarChartDisplayValues { +): BarChartDisplayValues | BarChartDisplayWarning { if (!series?.length) { - return { warn: 'No data in response' } as BarChartDisplayValues; + return { warn: 'No data in response' }; } // Bar chart requires a single frame @@ -312,7 +312,7 @@ export function prepareBarChartDisplayValues( ) : outerJoinDataFrames({ frames: series }); if (!frame) { - return { warn: 'Unable to join data' } as BarChartDisplayValues; + return { warn: 'Unable to join data' }; } // Color by a field different than the input @@ -320,7 +320,7 @@ export function prepareBarChartDisplayValues( if (options.colorByField) { colorByField = findField(frame, options.colorByField); if (!colorByField) { - return { warn: 'Color field not found' } as BarChartDisplayValues; + return { warn: 'Color field not found' }; } } @@ -328,7 +328,7 @@ export function prepareBarChartDisplayValues( if (options.xField) { xField = findField(frame, options.xField); if (!xField) { - return { warn: 'Configured x field not found' } as BarChartDisplayValues; + return { warn: 'Configured x field not found' }; } } @@ -398,13 +398,13 @@ export function prepareBarChartDisplayValues( if (!firstField) { return { warn: 'Bar charts requires a string or time field', - } as BarChartDisplayValues; + }; } if (!fields.length) { return { warn: 'No numeric fields found', - } as BarChartDisplayValues; + }; } // Show the first number value