From 1f6c2dbcfb4bb1a09627dfe1a3e180aa22282ceb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Tue, 1 Sep 2020 16:00:38 +0200 Subject: [PATCH] Thresholds: Fixed issue with thresholds in overrides not working after save and reload (#27297) * WIP: Fix null thresholds in overrides when loading * Fix thresholds on load instead of in apply field overrides * simplify expression * fixed ts issue * Updated test * Updated another test * Updated another test --- .../src/field/fieldDisplay.test.ts | 25 +------- .../grafana-data/src/field/fieldOverrides.ts | 13 ---- .../SingleStatBaseOptions.test.ts | 2 +- .../dashboard/state/PanelModel.test.ts | 25 +++++++- .../features/dashboard/state/PanelModel.ts | 64 ++++++++++++++----- .../panel/bargauge/BarGaugeMigrations.test.ts | 2 +- .../panel/gauge/GaugeMigrations.test.ts | 2 +- 7 files changed, 76 insertions(+), 57 deletions(-) diff --git a/packages/grafana-data/src/field/fieldDisplay.test.ts b/packages/grafana-data/src/field/fieldDisplay.test.ts index 162d1a287f1..67651bf1764 100644 --- a/packages/grafana-data/src/field/fieldDisplay.test.ts +++ b/packages/grafana-data/src/field/fieldDisplay.test.ts @@ -2,10 +2,8 @@ import merge from 'lodash/merge'; import { getFieldDisplayValues, GetFieldDisplayValuesOptions } from './fieldDisplay'; import { toDataFrame } from '../dataframe/processDataFrame'; import { ReducerID } from '../transformations/fieldReducer'; -import { ThresholdsMode } from '../types/thresholds'; import { GrafanaTheme } from '../types/theme'; -import { FieldConfig, MappingType } from '../types'; -import { validateFieldConfig } from './fieldOverrides'; +import { MappingType } from '../types'; import { standardFieldConfigEditorRegistry } from './standardFieldConfigEditorRegistry'; describe('FieldDisplay', () => { @@ -74,27 +72,6 @@ describe('FieldDisplay', () => { expect(display.map(v => v.display.numeric)).toEqual([1, 3]); // First 2 are from the first field }); - it('should restore -Infinity value for base threshold', () => { - const config: FieldConfig = { - thresholds: { - mode: ThresholdsMode.Absolute, - steps: [ - { - color: '#73BF69', - value: (null as any) as number, // -Infinity becomes null in JSON - }, - { - color: '#F2495C', - value: 50, - }, - ], - }, - }; - validateFieldConfig(config); - expect(config.thresholds!.steps.length).toEqual(2); - expect(config.thresholds!.steps[0].value).toBe(-Infinity); - }); - it('Should return field thresholds when there is no data', () => { const options = createEmptyDisplayOptions({ fieldConfig: { diff --git a/packages/grafana-data/src/field/fieldOverrides.ts b/packages/grafana-data/src/field/fieldOverrides.ts index 60d7e525883..99ee72711cf 100644 --- a/packages/grafana-data/src/field/fieldOverrides.ts +++ b/packages/grafana-data/src/field/fieldOverrides.ts @@ -4,7 +4,6 @@ import { DataFrame, Field, FieldType, - ThresholdsMode, FieldColorMode, ColorScheme, FieldOverrideContext, @@ -308,18 +307,6 @@ const processFieldConfigValue = ( */ export function validateFieldConfig(config: FieldConfig) { const { thresholds } = config; - if (thresholds) { - if (!thresholds.mode) { - thresholds.mode = ThresholdsMode.Absolute; - } - if (!thresholds.steps) { - thresholds.steps = []; - } else if (thresholds.steps.length) { - // First value is always -Infinity - // JSON saves it as null - thresholds.steps[0].value = -Infinity; - } - } if (!config.color) { if (thresholds) { diff --git a/packages/grafana-ui/src/components/SingleStatShared/SingleStatBaseOptions.test.ts b/packages/grafana-ui/src/components/SingleStatShared/SingleStatBaseOptions.test.ts index 084712e90c4..5458e37eda7 100644 --- a/packages/grafana-ui/src/components/SingleStatShared/SingleStatBaseOptions.test.ts +++ b/packages/grafana-ui/src/components/SingleStatShared/SingleStatBaseOptions.test.ts @@ -17,7 +17,7 @@ describe('sharedSingleStatMigrationHandler', () => { { color: 'green', index: 0, - value: null, + value: -Infinity, }, { color: 'orange', diff --git a/public/app/features/dashboard/state/PanelModel.test.ts b/public/app/features/dashboard/state/PanelModel.test.ts index 62c85753466..aab5130b971 100644 --- a/public/app/features/dashboard/state/PanelModel.test.ts +++ b/public/app/features/dashboard/state/PanelModel.test.ts @@ -96,6 +96,13 @@ describe('PanelModel', () => { fieldConfig: { defaults: { unit: 'mpg', + thresholds: { + mode: 'absolute', + steps: [ + { color: 'green', value: null }, + { color: 'red', value: 80 }, + ], + }, }, overrides: [ { @@ -103,7 +110,18 @@ describe('PanelModel', () => { id: '1', options: {}, }, - properties: [], + properties: [ + { + id: 'thresholds', + value: { + mode: 'absolute', + steps: [ + { color: 'green', value: null }, + { color: 'red', value: 80 }, + ], + }, + }, + ], }, ], }, @@ -146,6 +164,11 @@ describe('PanelModel', () => { expect(model.getOptions().showThresholds).toBeTruthy(); }); + it('should change null thresholds to negative infinity', () => { + expect(model.fieldConfig.defaults.thresholds.steps[0].value).toBe(-Infinity); + expect(model.fieldConfig.overrides[0].properties[0].value.steps[0].value).toBe(-Infinity); + }); + it('should apply option defaults but not override if array is changed', () => { expect(model.getOptions().arrayWith2Values.length).toBe(1); }); diff --git a/public/app/features/dashboard/state/PanelModel.ts b/public/app/features/dashboard/state/PanelModel.ts index 2050274d406..197beba4015 100644 --- a/public/app/features/dashboard/state/PanelModel.ts +++ b/public/app/features/dashboard/state/PanelModel.ts @@ -16,6 +16,8 @@ import { PanelEvents, PanelPlugin, ScopedVars, + ThresholdsConfig, + ThresholdsMode, } from '@grafana/data'; import { EDIT_PANEL_ID } from 'app/core/constants'; import config from 'app/core/config'; @@ -317,22 +319,7 @@ export class PanelModel implements DataConfigSource { } }); - this.fieldConfig = { - defaults: _.mergeWith( - {}, - plugin.fieldConfigDefaults.defaults, - this.fieldConfig ? this.fieldConfig.defaults : {}, - (objValue: any, srcValue: any): any => { - if (_.isArray(srcValue)) { - return srcValue; - } - } - ), - overrides: [ - ...plugin.fieldConfigDefaults.overrides, - ...(this.fieldConfig && this.fieldConfig.overrides ? this.fieldConfig.overrides : []), - ], - }; + this.fieldConfig = applyFieldConfigDefaults(this.fieldConfig, this.plugin!.fieldConfigDefaults); } pluginLoaded(plugin: PanelPlugin) { @@ -503,6 +490,51 @@ export class PanelModel implements DataConfigSource { } } +function applyFieldConfigDefaults(fieldConfig: FieldConfigSource, defaults: FieldConfigSource): FieldConfigSource { + const result: FieldConfigSource = { + defaults: _.mergeWith( + {}, + defaults.defaults, + fieldConfig ? fieldConfig.defaults : {}, + (objValue: any, srcValue: any): any => { + if (_.isArray(srcValue)) { + return srcValue; + } + } + ), + overrides: fieldConfig?.overrides ?? [], + }; + + // Thresholds base values are null in JSON but need to be converted to -Infinity + if (result.defaults.thresholds) { + fixThresholds(result.defaults.thresholds); + } + + for (const override of result.overrides) { + for (const property of override.properties) { + if (property.id === 'thresholds') { + fixThresholds(property.value as ThresholdsConfig); + } + } + } + + return result; +} + +function fixThresholds(thresholds: ThresholdsConfig) { + if (!thresholds.mode) { + thresholds.mode = ThresholdsMode.Absolute; + } + + if (!thresholds.steps) { + thresholds.steps = []; + } else if (thresholds.steps.length) { + // First value is always -Infinity + // JSON saves it as null + thresholds.steps[0].value = -Infinity; + } +} + function getPluginVersion(plugin: PanelPlugin): string { return plugin && plugin.meta.info.version ? plugin.meta.info.version : config.buildInfo.version; } diff --git a/public/app/plugins/panel/bargauge/BarGaugeMigrations.test.ts b/public/app/plugins/panel/bargauge/BarGaugeMigrations.test.ts index bcd4af6b9bd..002552c0a6c 100644 --- a/public/app/plugins/panel/bargauge/BarGaugeMigrations.test.ts +++ b/public/app/plugins/panel/bargauge/BarGaugeMigrations.test.ts @@ -22,7 +22,7 @@ describe('BarGauge Panel Migrations', () => { { color: 'green', index: 0, - value: null, + value: -Infinity, }, { color: 'orange', diff --git a/public/app/plugins/panel/gauge/GaugeMigrations.test.ts b/public/app/plugins/panel/gauge/GaugeMigrations.test.ts index e63f3baf819..1f63ae5641b 100644 --- a/public/app/plugins/panel/gauge/GaugeMigrations.test.ts +++ b/public/app/plugins/panel/gauge/GaugeMigrations.test.ts @@ -22,7 +22,7 @@ describe('Gauge Panel Migrations', () => { { color: 'green', index: 0, - value: null, + value: -Infinity, }, { color: '#EAB839',