From e260d13a65f77c67f66b8ab06377b09322498d31 Mon Sep 17 00:00:00 2001 From: Ivan Ortega Alba Date: Fri, 10 May 2024 13:41:30 +0200 Subject: [PATCH] [v11.0.x] Dashboard scene: Discard panel changes disabled/enabled depending of changes (#87570) * DashboardScene: Discard panel changes disabled/enabled depending of changes (#87137) --------- Co-authored-by: Ivan Ortega Alba Co-authored-by: Dominik Prokop (cherry picked from commit c3936bbae2e1327194e66d0bb14e110b52a60609) * Add line number --------- Co-authored-by: Oscar Kilhed --- .betterer.results | 5 +- .../panel-edit/VizPanelManager.test.tsx | 33 +- .../panel-edit/VizPanelManager.tsx | 26 +- .../saving/DashboardSceneChangeTracker.ts | 47 +- .../saving/getDashboardChanges.test.ts | 423 ++++++++++++++++++ .../saving/getDashboardChanges.ts | 120 ++--- .../DashboardDatasourceBehaviour.test.tsx | 32 +- .../scene/NavToolbarActions.tsx | 22 + .../settings/version-history/DiffTitle.tsx | 9 +- .../settings/version-history/utils.test.ts | 8 + .../settings/version-history/utils.ts | 10 +- .../SaveDashboard/SaveDashboardDiff.tsx | 11 +- 12 files changed, 626 insertions(+), 120 deletions(-) create mode 100644 public/app/features/dashboard-scene/saving/getDashboardChanges.test.ts diff --git a/.betterer.results b/.betterer.results index 4ed81e4d5de..983e03cb8ac 100644 --- a/.betterer.results +++ b/.betterer.results @@ -2413,8 +2413,9 @@ exports[`better eslint`] = { [0, 0, 0, "Unexpected any. Specify a different type.", "1"] ], "public/app/features/dashboard-scene/saving/getDashboardChanges.ts:5381": [ - [0, 0, 0, "Do not use any type assertions.", "0"], - [0, 0, 0, "Do not use any type assertions.", "1"] + [0, 0, 0, "Unexpected any. Specify a different type.", "0"], + [0, 0, 0, "Do not use any type assertions.", "1"], + [0, 0, 0, "Do not use any type assertions.", "2"] ], "public/app/features/dashboard-scene/scene/PanelMenuBehavior.tsx:5381": [ [0, 0, 0, "Unexpected any. Specify a different type.", "0"], diff --git a/public/app/features/dashboard-scene/panel-edit/VizPanelManager.test.tsx b/public/app/features/dashboard-scene/panel-edit/VizPanelManager.test.tsx index db2f71c3a6f..590c519956a 100644 --- a/public/app/features/dashboard-scene/panel-edit/VizPanelManager.test.tsx +++ b/public/app/features/dashboard-scene/panel-edit/VizPanelManager.test.tsx @@ -1,6 +1,8 @@ import { map, of } from 'rxjs'; import { DataQueryRequest, DataSourceApi, DataSourceInstanceSettings, LoadingState, PanelData } from '@grafana/data'; +import { calculateFieldTransformer } from '@grafana/data/src/transformations/transformers/calculateField'; +import { mockTransformationsRegistry } from '@grafana/data/src/utils/tests/mockTransformationsRegistry'; import { config, locationService } from '@grafana/runtime'; import { SceneQueryRunner, VizPanel } from '@grafana/scenes'; import { DataQuery, DataSourceJsonData, DataSourceRef } from '@grafana/schema'; @@ -178,12 +180,17 @@ jest.mock('@grafana/runtime', () => ({ }, })); +mockTransformationsRegistry([calculateFieldTransformer]); + +jest.useFakeTimers(); + describe('VizPanelManager', () => { describe('When changing plugin', () => { it('Should successfully change from one viz type to another', () => { const { vizPanelManager } = setupTest('panel-1'); expect(vizPanelManager.state.panel.state.pluginId).toBe('timeseries'); vizPanelManager.changePluginType('table'); + expect(vizPanelManager.state.panel.state.pluginId).toBe('table'); }); @@ -405,6 +412,8 @@ describe('VizPanelManager', () => { datasourceUid: 'gdev-prometheus', }); + jest.runAllTimers(); // The detect panel changes is debounced + expect(vizPanelManager.state.isDirty).toBe(true); expect(vizPanelManager.state.datasource).toEqual(ds2Mock); expect(vizPanelManager.state.dsSettings).toEqual(instance2SettingsMock); }); @@ -472,6 +481,8 @@ describe('VizPanelManager', () => { }, }); + jest.runAllTimers(); // The detect panel changes is debounced + expect(vizPanelManager.state.isDirty).toBe(true); expect((panel.state.$timeRange?.state as PanelTimeRangeState).timeFrom).toBe('2h'); }); @@ -515,7 +526,7 @@ describe('VizPanelManager', () => { }); describe('max data points and interval', () => { - it('max data points', async () => { + it('should update max data points', async () => { const { vizPanelManager } = setupTest('panel-1'); vizPanelManager.activate(); await Promise.resolve(); @@ -534,10 +545,12 @@ describe('VizPanelManager', () => { maxDataPoints: 100, }); + jest.runAllTimers(); // The detect panel changes is debounced + expect(vizPanelManager.state.isDirty).toBe(true); expect(dataObj.state.maxDataPoints).toBe(100); }); - it('max data points', async () => { + it('should update min interval', async () => { const { vizPanelManager } = setupTest('panel-1'); vizPanelManager.activate(); await Promise.resolve(); @@ -556,6 +569,8 @@ describe('VizPanelManager', () => { minInterval: '1s', }); + jest.runAllTimers(); // The detect panel changes is debounced + expect(vizPanelManager.state.isDirty).toBe(true); expect(dataObj.state.minInterval).toBe('1s'); }); }); @@ -579,6 +594,8 @@ describe('VizPanelManager', () => { queries: [], }); + jest.runAllTimers(); // The detect panel changes is debounced + expect(vizPanelManager.state.isDirty).toBe(true); expect(dataObj.state.cacheTimeout).toBe('60'); expect(dataObj.state.queryCachingTTL).toBe(200000); }); @@ -616,6 +633,8 @@ describe('VizPanelManager', () => { }, } as DataSourceInstanceSettings); + jest.runAllTimers(); // The detect panel changes is debounced + expect(vizPanelManager.state.isDirty).toBe(true); expect(vizPanelManager.queryRunner.state.datasource).toEqual({ uid: 'gdev-prometheus', type: 'grafana-prometheus-datasource', @@ -643,6 +662,8 @@ describe('VizPanelManager', () => { }, } as DataSourceInstanceSettings); + jest.runAllTimers(); // The detect panel changes is debounced + expect(vizPanelManager.state.isDirty).toBe(true); expect(vizPanelManager.queryRunner.state.datasource).toEqual({ uid: SHARED_DASHBOARD_QUERY, type: 'datasource', @@ -670,6 +691,8 @@ describe('VizPanelManager', () => { }, } as DataSourceInstanceSettings); + jest.runAllTimers(); // The detect panel changes is debounced + expect(vizPanelManager.state.isDirty).toBe(true); expect(vizPanelManager.queryRunner.state.datasource).toEqual({ uid: 'gdev-prometheus', type: 'grafana-prometheus-datasource', @@ -691,6 +714,8 @@ describe('VizPanelManager', () => { vizPanelManager.dataTransformer.reprocessTransformations = reprocessMock; vizPanelManager.changeTransformations([{ id: 'calculateField', options: {} }]); + jest.runAllTimers(); // The detect panel changes is debounced + expect(vizPanelManager.state.isDirty).toBe(true); expect(reprocessMock).toHaveBeenCalledTimes(1); expect(vizPanelManager.dataTransformer.state.transformations).toEqual([{ id: 'calculateField', options: {} }]); }); @@ -716,6 +741,8 @@ describe('VizPanelManager', () => { }, ]); + jest.runAllTimers(); // The detect panel changes is debounced + expect(vizPanelManager.state.isDirty).toBe(true); expect(vizPanelManager.queryRunner.state.queries).toEqual([ { datasource: { @@ -763,6 +790,8 @@ describe('VizPanelManager', () => { }, ]); + jest.runAllTimers(); // The detect panel changes is debounced + expect(vizPanelManager.state.isDirty).toBe(true); expect(vizPanelManager.queryRunner.state.queries[0].panelId).toBe(panelWithQueriesOnly.id); }); }); diff --git a/public/app/features/dashboard-scene/panel-edit/VizPanelManager.tsx b/public/app/features/dashboard-scene/panel-edit/VizPanelManager.tsx index eae39a8c39d..2892dcd1663 100644 --- a/public/app/features/dashboard-scene/panel-edit/VizPanelManager.tsx +++ b/public/app/features/dashboard-scene/panel-edit/VizPanelManager.tsx @@ -1,4 +1,5 @@ import { css } from '@emotion/css'; +import { debounce } from 'lodash'; import React, { useEffect } from 'react'; import { @@ -20,6 +21,7 @@ import { SceneObjectBase, SceneObjectRef, SceneObjectState, + SceneObjectStateChangedEvent, SceneQueryRunner, VizPanel, sceneUtils, @@ -34,10 +36,12 @@ import { updateQueries } from 'app/features/query/state/updateQueries'; import { GrafanaQuery } from 'app/plugins/datasource/grafana/types'; import { QueryGroupOptions } from 'app/types'; +import { DashboardSceneChangeTracker } from '../saving/DashboardSceneChangeTracker'; +import { getPanelChanges } from '../saving/getDashboardChanges'; import { DashboardGridItem, RepeatDirection } from '../scene/DashboardGridItem'; import { LibraryVizPanel } from '../scene/LibraryVizPanel'; import { PanelTimeRange, PanelTimeRangeState } from '../scene/PanelTimeRange'; -import { gridItemToPanel } from '../serialization/transformSceneToSaveModel'; +import { gridItemToPanel, vizPanelToPanel } from '../serialization/transformSceneToSaveModel'; import { getDashboardSceneFor, getPanelIdForVizPanel, getQueryRunnerFor } from '../utils/utils'; export interface VizPanelManagerState extends SceneObjectState { @@ -49,6 +53,7 @@ export interface VizPanelManagerState extends SceneObjectState { repeat?: string; repeatDirection?: RepeatDirection; maxPerRow?: number; + isDirty?: boolean; } export enum DisplayMode { @@ -95,8 +100,27 @@ export class VizPanelManager extends SceneObjectBase { private _onActivate() { this.loadDataSource(); + const changesSub = this.subscribeToEvent(SceneObjectStateChangedEvent, this._handleStateChange); + + return () => { + changesSub.unsubscribe(); + }; } + private _detectPanelModelChanges = debounce(() => { + const { hasChanges } = getPanelChanges( + vizPanelToPanel(this.state.sourcePanel.resolve()), + vizPanelToPanel(this.state.panel) + ); + this.setState({ isDirty: hasChanges }); + }, 250); + + private _handleStateChange = (event: SceneObjectStateChangedEvent) => { + if (DashboardSceneChangeTracker.isUpdatingPersistedState(event)) { + this._detectPanelModelChanges(); + } + }; + private async loadDataSource() { const dataObj = this.state.panel.state.$data; diff --git a/public/app/features/dashboard-scene/saving/DashboardSceneChangeTracker.ts b/public/app/features/dashboard-scene/saving/DashboardSceneChangeTracker.ts index 26f3287cd7f..4bc186849e9 100644 --- a/public/app/features/dashboard-scene/saving/DashboardSceneChangeTracker.ts +++ b/public/app/features/dashboard-scene/saving/DashboardSceneChangeTracker.ts @@ -36,10 +36,10 @@ export class DashboardSceneChangeTracker { this._dashboard = dashboard; } - private onStateChanged({ payload }: SceneObjectStateChangedEvent) { + static isUpdatingPersistedState({ payload }: SceneObjectStateChangedEvent) { // If there are no changes in the state, the check is not needed if (Object.keys(payload.partialUpdate).length === 0) { - return; + return false; } // Any change in the panel should trigger a change detection @@ -50,7 +50,7 @@ export class DashboardSceneChangeTracker { payload.changedObject instanceof DashboardGridItem || payload.changedObject instanceof PanelTimeRange ) { - return this.detectSaveModelChanges(); + return true; } // VizPanelManager includes the repeat configuration if (payload.changedObject instanceof VizPanelManager) { @@ -59,27 +59,27 @@ export class DashboardSceneChangeTracker { Object.prototype.hasOwnProperty.call(payload.partialUpdate, 'repeatDirection') || Object.prototype.hasOwnProperty.call(payload.partialUpdate, 'maxPerRow') ) { - return this.detectSaveModelChanges(); + return true; } } // SceneQueryRunner includes the DS configuration if (payload.changedObject instanceof SceneQueryRunner) { if (!Object.prototype.hasOwnProperty.call(payload.partialUpdate, 'data')) { - return this.detectSaveModelChanges(); + return true; } } // SceneDataTransformer includes the transformation configuration if (payload.changedObject instanceof SceneDataTransformer) { if (!Object.prototype.hasOwnProperty.call(payload.partialUpdate, 'data')) { - return this.detectSaveModelChanges(); + return true; } } if (payload.changedObject instanceof VizPanelLinks) { - return this.detectSaveModelChanges(); + return true; } if (payload.changedObject instanceof LibraryVizPanel) { if (Object.prototype.hasOwnProperty.call(payload.partialUpdate, 'name')) { - return this.detectSaveModelChanges(); + return true; } } if (payload.changedObject instanceof SceneRefreshPicker) { @@ -87,47 +87,54 @@ export class DashboardSceneChangeTracker { Object.prototype.hasOwnProperty.call(payload.partialUpdate, 'intervals') || Object.prototype.hasOwnProperty.call(payload.partialUpdate, 'refresh') ) { - return this.detectSaveModelChanges(); + return true; } } if (payload.changedObject instanceof behaviors.CursorSync) { - return this.detectSaveModelChanges(); + return true; } if (payload.changedObject instanceof SceneDataLayerSet) { - return this.detectSaveModelChanges(); + return true; } if (payload.changedObject instanceof DashboardGridItem) { - return this.detectSaveModelChanges(); + return true; } if (payload.changedObject instanceof SceneGridLayout) { - return this.detectSaveModelChanges(); + return true; } if (payload.changedObject instanceof DashboardScene) { if (Object.keys(payload.partialUpdate).some((key) => PERSISTED_PROPS.includes(key))) { - return this.detectSaveModelChanges(); + return true; } } if (payload.changedObject instanceof SceneTimeRange) { - return this.detectSaveModelChanges(); + return true; } if (payload.changedObject instanceof DashboardControls) { if (Object.prototype.hasOwnProperty.call(payload.partialUpdate, 'hideTimeControls')) { - return this.detectSaveModelChanges(); + return true; } } if (payload.changedObject instanceof SceneVariableSet) { - return this.detectSaveModelChanges(); + return true; } if (payload.changedObject instanceof DashboardAnnotationsDataLayer) { if (!Object.prototype.hasOwnProperty.call(payload.partialUpdate, 'data')) { - return this.detectSaveModelChanges(); + return true; } } if (payload.changedObject instanceof behaviors.LiveNowTimer) { - return this.detectSaveModelChanges(); + return true; } if (isSceneVariableInstance(payload.changedObject)) { - return this.detectSaveModelChanges(); + return true; + } + return false; + } + + private onStateChanged(event: SceneObjectStateChangedEvent) { + if (DashboardSceneChangeTracker.isUpdatingPersistedState(event)) { + this.detectSaveModelChanges(); } } diff --git a/public/app/features/dashboard-scene/saving/getDashboardChanges.test.ts b/public/app/features/dashboard-scene/saving/getDashboardChanges.test.ts new file mode 100644 index 00000000000..1820d71733f --- /dev/null +++ b/public/app/features/dashboard-scene/saving/getDashboardChanges.test.ts @@ -0,0 +1,423 @@ +import { Dashboard, Panel } from '@grafana/schema'; + +import { getDashboardChanges, getPanelChanges } from './getDashboardChanges'; + +describe('getDashboardChanges', () => { + const initial: Dashboard = { + id: 1, + title: 'Dashboard 1', + time: { + from: 'now-7d', + to: 'now', + }, + refresh: '1h', + version: 1, + schemaVersion: 1, + templating: { + list: [ + { + name: 'var1', + type: 'query', + query: 'query1', + current: { + value: 'value1', + text: 'text1', + }, + options: [], + }, + ], + }, + }; + it('should return the correct result when no changes', () => { + const changed = { ...initial }; + + const expectedChanges = { + initialSaveModel: { + ...initial, + }, + changedSaveModel: { + ...changed, + }, + diffs: {}, + diffCount: 0, + hasChanges: false, + hasTimeChanges: false, + isNew: false, + hasVariableValueChanges: false, + hasRefreshChange: false, + }; + + const result = getDashboardChanges(initial, changed, false, false, false); + + expect(result).toEqual(expectedChanges); + }); + + it('should return the correct result when is new', () => { + const newDashInitial = { + ...initial, + version: 0, + }; + const changed = { + ...newDashInitial, + version: 0, + }; + + const expectedChanges = { + changedSaveModel: { + ...newDashInitial, + }, + initialSaveModel: { + ...changed, + }, + diffs: {}, + diffCount: 0, + hasChanges: false, + hasTimeChanges: false, + isNew: true, + hasVariableValueChanges: false, + hasRefreshChange: false, + }; + + const result = getDashboardChanges(newDashInitial, changed, false, false, false); + + expect(result).toEqual(expectedChanges); + }); + + it('should return the correct result when the time changes but they are not preserved', () => { + const changed = { + ...initial, + time: { + from: 'now-1d', + to: 'now', + }, + }; + + const expectedChanges = { + initialSaveModel: { + ...initial, + }, + changedSaveModel: { + ...initial, + }, + diffs: {}, + diffCount: 0, + hasChanges: false, + hasTimeChanges: true, + isNew: false, + hasVariableValueChanges: false, + hasRefreshChange: false, + }; + + const result = getDashboardChanges(initial, changed, false, false, false); + + expect(result).toEqual(expectedChanges); + }); + + it('should return the correct result when the time changes and they are preserved', () => { + const changed = { + ...initial, + time: { + from: 'now-1d', + to: 'now', + }, + }; + + const expectedChanges = { + initialSaveModel: { + ...initial, + }, + changedSaveModel: { + ...changed, + }, + diffs: { + time: [ + { + endLineNumber: expect.any(Number), + op: 'replace', + originalValue: 'now-7d', + path: ['time', 'from'], + startLineNumber: expect.any(Number), + value: 'now-1d', + }, + ], + }, + diffCount: 1, + hasChanges: true, + hasTimeChanges: true, + isNew: false, + hasVariableValueChanges: false, + hasRefreshChange: false, + }; + + const result = getDashboardChanges(initial, changed, true, false, false); + + expect(result).toEqual(expectedChanges); + }); + + it('should return the correct result when the refresh changes but it is not preserved', () => { + const changed = { + ...initial, + refresh: '2h', + }; + + const expectedChanges = { + initialSaveModel: { + ...initial, + }, + changedSaveModel: { + ...initial, + }, + diffs: {}, + diffCount: 0, + hasChanges: false, + hasTimeChanges: false, + isNew: false, + hasVariableValueChanges: false, + hasRefreshChange: true, + }; + + const result = getDashboardChanges(initial, changed, false, false, false); + + expect(result).toEqual(expectedChanges); + }); + + it('should return the correct result when the refresh changes and it is preserved', () => { + const changed = { + ...initial, + refresh: '2h', + }; + + const expectedChanges = { + initialSaveModel: { + ...initial, + }, + changedSaveModel: { + ...changed, + }, + diffs: { + refresh: [ + { + endLineNumber: expect.any(Number), + op: 'replace', + originalValue: '1h', + path: ['refresh'], + startLineNumber: expect.any(Number), + value: '2h', + }, + ], + }, + diffCount: 1, + hasChanges: true, + hasTimeChanges: false, + isNew: false, + hasVariableValueChanges: false, + hasRefreshChange: true, + }; + + const result = getDashboardChanges(initial, changed, false, false, true); + + expect(result).toEqual(expectedChanges); + }); + + it('should return the correct result when the variable value changes but it is not preserved', () => { + const changed = { + ...initial, + templating: { + list: [ + { + name: 'var1', + type: 'query', + query: 'query1', + current: { + value: 'value2', + text: 'text1', + }, + options: [], + }, + ], + }, + } as Dashboard; + + const expectedChanges = { + initialSaveModel: { + ...initial, + }, + changedSaveModel: { + ...initial, + }, + diffs: {}, + diffCount: 0, + hasChanges: false, + hasTimeChanges: false, + isNew: false, + hasVariableValueChanges: true, + hasRefreshChange: false, + }; + + const result = getDashboardChanges(initial, changed, false, false, false); + + expect(result).toEqual(expectedChanges); + }); + + it('should return the correct result when the variable value changes', () => { + const changed = { + ...initial, + templating: { + list: [ + { + name: 'var1', + type: 'query', + query: 'query1', + current: { + value: 'value2', + text: 'text1', + }, + options: [], + }, + ], + }, + } as Dashboard; + + const expectedChanges = { + initialSaveModel: { + ...initial, + }, + changedSaveModel: { + ...changed, + }, + diffs: { + templating: [ + { + endLineNumber: 17, + op: 'replace', + originalValue: 'value1', + path: ['templating', 'list', '0', 'current', 'value'], + startLineNumber: 17, + value: 'value2', + }, + ], + }, + diffCount: 1, + hasChanges: true, + hasTimeChanges: false, + isNew: false, + hasVariableValueChanges: true, + hasRefreshChange: false, + }; + + const result = getDashboardChanges(initial, changed, false, true, false); + + expect(result).toEqual(expectedChanges); + }); +}); + +describe('getPanelChanges', () => { + const initial: Panel = { + id: 1, + type: 'graph', + title: 'Panel 1', + gridPos: { + x: 0, + y: 0, + w: 12, + h: 8, + }, + targets: [ + { + refId: 'A', + query: 'query1', + }, + ], + }; + + it('should return the correct result when no changes', () => { + const changed = { ...initial }; + + const expectedChanges = { + initialSaveModel: { + ...initial, + }, + changedSaveModel: { + ...changed, + }, + diffs: {}, + diffCount: 0, + hasChanges: false, + }; + + expect(getPanelChanges(initial, changed)).toEqual(expectedChanges); + }); + + it('should return the correct result when there is some changes', () => { + const changed = { + ...initial, + title: 'Panel 2', + type: 'table', + gridPos: { + ...initial.gridPos, + x: 1, + }, + targets: [ + { + refId: 'A', + query: 'query2', + }, + ], + } as Panel; + + const expectedChanges = { + initialSaveModel: { + ...initial, + }, + changedSaveModel: { + ...changed, + }, + diffs: { + title: [ + { + endLineNumber: 3, + op: 'replace', + originalValue: 'Panel 1', + path: ['title'], + startLineNumber: 3, + value: 'Panel 2', + }, + ], + type: [ + { + endLineNumber: 2, + op: 'replace', + originalValue: 'graph', + path: ['type'], + startLineNumber: 2, + value: 'table', + }, + ], + gridPos: [ + { + endLineNumber: 5, + op: 'replace', + originalValue: 0, + path: ['gridPos', 'x'], + startLineNumber: 5, + value: 1, + }, + ], + targets: [ + { + endLineNumber: 13, + op: 'replace', + originalValue: 'query1', + path: ['targets', '0', 'query'], + startLineNumber: 13, + value: 'query2', + }, + ], + }, + diffCount: 4, + hasChanges: true, + }; + + expect(getPanelChanges(changed, initial)).toEqual(expectedChanges); + }); +}); diff --git a/public/app/features/dashboard-scene/saving/getDashboardChanges.ts b/public/app/features/dashboard-scene/saving/getDashboardChanges.ts index 9231ee43f5e..74c71be1b5c 100644 --- a/public/app/features/dashboard-scene/saving/getDashboardChanges.ts +++ b/public/app/features/dashboard-scene/saving/getDashboardChanges.ts @@ -1,10 +1,34 @@ -import { compare, Operation } from 'fast-json-patch'; // @ts-ignore import jsonMap from 'json-source-map'; -import { flow, get, isEqual, sortBy, tail } from 'lodash'; -import { AdHocVariableModel, TypedVariableModel } from '@grafana/data'; -import { Dashboard } from '@grafana/schema'; +import type { AdHocVariableModel, TypedVariableModel } from '@grafana/data'; +import { Dashboard, Panel, VariableOption } from '@grafana/schema'; + +import { jsonDiff } from '../settings/version-history/utils'; + +export function get(obj: any, keys: string[]) { + try { + let val = obj; + for (const key of keys) { + val = val[key]; + } + return val; + } catch (err) { + return undefined; + } +} + +export function deepEqual(a: string | string[], b: string | string[]) { + return ( + typeof a === typeof b && + ((typeof a === 'string' && a === b) || + (Array.isArray(a) && a.length === b.length && a.every((val, i) => val === b[i]))) + ); +} + +export function isEqual(a: VariableOption | undefined, b: VariableOption | undefined) { + return a === b || (a && b && a.selected === b.selected && deepEqual(a.text, b.text) && deepEqual(a.value, b.value)); +} export function getDashboardChanges( initial: Dashboard, @@ -28,11 +52,8 @@ export function getDashboardChanges( } const diff = jsonDiff(initialSaveModel, changedSaveModel); + const diffCount = Object.values(diff).reduce((acc, cur) => acc + cur.length, 0); - let diffCount = 0; - for (const d of Object.values(diff)) { - diffCount += d.length; - } return { changedSaveModel, initialSaveModel, @@ -63,7 +84,7 @@ export function applyVariableChanges(saveModel: Dashboard, originalSaveModel: Da } // Old schema property that never should be in persisted model - if (original.current && Object.hasOwn(original.current, 'selected')) { + if (original.current) { delete original.current.selected; } @@ -75,11 +96,9 @@ export function applyVariableChanges(saveModel: Dashboard, originalSaveModel: Da const typed = variable as TypedVariableModel; if (typed.type === 'adhoc') { typed.filters = (original as AdHocVariableModel).filters; - } else { - if (typed.type !== 'groupby') { - variable.current = original.current; - variable.options = original.options; - } + } else if (typed.type !== 'groupby') { + variable.current = original.current; + variable.options = original.options; } } } @@ -87,68 +106,15 @@ export function applyVariableChanges(saveModel: Dashboard, originalSaveModel: Da return hasVariableValueChanges; } -export type Diff = { - op: 'add' | 'replace' | 'remove' | 'copy' | 'test' | '_get' | 'move'; - value: unknown; - originalValue: unknown; - path: string[]; - startLineNumber: number; -}; +export function getPanelChanges(saveModel: Panel, originalSaveModel: Panel) { + const diff = jsonDiff(originalSaveModel, saveModel); + const diffCount = Object.values(diff).reduce((acc, cur) => acc + cur.length, 0); -export type Diffs = { - [key: string]: Diff[]; -}; - -export type JSONValue = string | Dashboard; - -export const jsonDiff = (lhs: JSONValue, rhs: JSONValue): Diffs => { - const diffs = compare(lhs, rhs); - const lhsMap = jsonMap.stringify(lhs, null, 2); - const rhsMap = jsonMap.stringify(rhs, null, 2); - - const getDiffInformation = (diffs: Operation[]): Diff[] => { - return diffs.map((diff) => { - let originalValue = undefined; - let value = undefined; - let startLineNumber = 0; - - const path = tail(diff.path.split('/')); - - if (diff.op === 'replace' && rhsMap.pointers[diff.path]) { - originalValue = get(lhs, path); - value = diff.value; - startLineNumber = rhsMap.pointers[diff.path].value.line; - } - if (diff.op === 'add' && rhsMap.pointers[diff.path]) { - value = diff.value; - startLineNumber = rhsMap.pointers[diff.path].value.line; - } - if (diff.op === 'remove' && lhsMap.pointers[diff.path]) { - originalValue = get(lhs, path); - startLineNumber = lhsMap.pointers[diff.path].value.line; - } - - return { - op: diff.op, - value, - path, - originalValue, - startLineNumber, - }; - }); + return { + changedSaveModel: saveModel, + initialSaveModel: originalSaveModel, + diffs: diff, + diffCount, + hasChanges: diffCount > 0, }; - - const sortByLineNumber = (diffs: Diff[]) => sortBy(diffs, 'startLineNumber'); - const groupByPath = (diffs: Diff[]) => - diffs.reduce>((acc, value) => { - const groupKey: string = value.path[0]; - if (!acc[groupKey]) { - acc[groupKey] = []; - } - acc[groupKey].push(value); - return acc; - }, {}); - - // return 1; - return flow([getDiffInformation, sortByLineNumber, groupByPath])(diffs); -}; +} diff --git a/public/app/features/dashboard-scene/scene/DashboardDatasourceBehaviour.test.tsx b/public/app/features/dashboard-scene/scene/DashboardDatasourceBehaviour.test.tsx index 86814f13b4b..de5e655eae2 100644 --- a/public/app/features/dashboard-scene/scene/DashboardDatasourceBehaviour.test.tsx +++ b/public/app/features/dashboard-scene/scene/DashboardDatasourceBehaviour.test.tsx @@ -11,7 +11,7 @@ import { } from '@grafana/data'; import { getPanelPlugin } from '@grafana/data/test/__mocks__/pluginMocks'; import { setPluginImportUtils } from '@grafana/runtime'; -import { SceneGridLayout, SceneQueryRunner, VizPanel } from '@grafana/scenes'; +import { SceneDataTransformer, SceneGridLayout, SceneQueryRunner, VizPanel } from '@grafana/scenes'; import { SHARED_DASHBOARD_QUERY } from 'app/plugins/datasource/dashboard'; import { DASHBOARD_DATASOURCE_PLUGIN_ID } from 'app/plugins/datasource/dashboard/types'; @@ -104,12 +104,12 @@ describe('DashboardDatasourceBehaviour', () => { it('Should re-run query of dashboardDS panel when source query re-runs', async () => { // spy on runQueries that will be called by the behaviour - const spy = jest.spyOn(dashboardDSPanel.state.$data as SceneQueryRunner, 'runQueries'); + const spy = jest.spyOn(dashboardDSPanel.state.$data!.state.$data as SceneQueryRunner, 'runQueries'); // deactivate scene to mimic going into panel edit sceneDeactivate(); // run source panel queries and update request ID - (sourcePanel.state.$data as SceneQueryRunner).runQueries(); + (sourcePanel.state.$data!.state.$data as SceneQueryRunner).runQueries(); await new Promise((r) => setTimeout(r, 1)); @@ -121,7 +121,7 @@ describe('DashboardDatasourceBehaviour', () => { it('Should not run query of dashboardDS panel when source panel queries do not change', async () => { // spy on runQueries - const spy = jest.spyOn(dashboardDSPanel.state.$data as SceneQueryRunner, 'runQueries'); + const spy = jest.spyOn(dashboardDSPanel.state.$data!.state.$data as SceneQueryRunner, 'runQueries'); // deactivate scene to mimic going into panel edit sceneDeactivate(); @@ -273,10 +273,10 @@ describe('DashboardDatasourceBehaviour', () => { it('Should exit behaviour early if not in a dashboard scene', async () => { // spy on runQueries - const spy = jest.spyOn(dashboardDSPanel.state.$data as SceneQueryRunner, 'runQueries'); + const spy = jest.spyOn(dashboardDSPanel.state.$data!.state.$data as SceneQueryRunner, 'runQueries'); const vizPanelManager = new VizPanelManager({ - panel: dashboardDSPanel.clone({ $data: undefined }), + panel: dashboardDSPanel.clone(), $data: dashboardDSPanel.state.$data?.clone(), sourcePanel: dashboardDSPanel.getRef(), }); @@ -576,9 +576,12 @@ async function buildTestScene() { title: 'Panel A', pluginId: 'table', key: 'panel-1', - $data: new SceneQueryRunner({ - datasource: { uid: 'grafana' }, - queries: [{ refId: 'A', queryType: 'randomWalk' }], + $data: new SceneDataTransformer({ + transformations: [], + $data: new SceneQueryRunner({ + datasource: { uid: 'grafana' }, + queries: [{ refId: 'A', queryType: 'randomWalk' }], + }), }), }); @@ -586,10 +589,13 @@ async function buildTestScene() { title: 'Panel B', pluginId: 'table', key: 'panel-2', - $data: new SceneQueryRunner({ - datasource: { uid: SHARED_DASHBOARD_QUERY }, - queries: [{ refId: 'A', panelId: 1 }], - $behaviors: [new DashboardDatasourceBehaviour({})], + $data: new SceneDataTransformer({ + transformations: [], + $data: new SceneQueryRunner({ + datasource: { uid: SHARED_DASHBOARD_QUERY }, + queries: [{ refId: 'A', panelId: 1 }], + $behaviors: [new DashboardDatasourceBehaviour({})], + }), }), }); diff --git a/public/app/features/dashboard-scene/scene/NavToolbarActions.tsx b/public/app/features/dashboard-scene/scene/NavToolbarActions.tsx index 7f06537a383..957f78d4724 100644 --- a/public/app/features/dashboard-scene/scene/NavToolbarActions.tsx +++ b/public/app/features/dashboard-scene/scene/NavToolbarActions.tsx @@ -65,6 +65,7 @@ export function ToolbarActions({ dashboard }: Props) { const styles = useStyles2(getStyles); const isEditingPanel = Boolean(editPanel); const isViewingPanel = Boolean(viewPanelScene); + const isEditedPanelDirty = useVizManagerDirty(editPanel); const isEditingLibraryPanel = useEditingLibraryPanel(editPanel); const hasCopiedPanel = Boolean(copiedPanel); // Means we are not in settings view, fullscreen panel or edit panel @@ -412,6 +413,7 @@ export function ToolbarActions({ dashboard }: Props) { onClick={editPanel?.onDiscard} tooltip="Discard panel changes" size="sm" + disabled={!isEditedPanelDirty || !isDirty} key="discard" fill="outline" variant="destructive" @@ -621,6 +623,26 @@ function useEditingLibraryPanel(panelEditor?: PanelEditor) { return isEditingLibraryPanel; } +// This hook handles when panelEditor is not defined to avoid conditionally hook usage +function useVizManagerDirty(panelEditor?: PanelEditor) { + const [isDirty, setIsDirty] = useState(false); + + useEffect(() => { + if (panelEditor) { + const unsub = panelEditor.state.vizManager.subscribeToState((vizManagerState) => + setIsDirty(vizManagerState.isDirty || false) + ); + return () => { + unsub.unsubscribe(); + }; + } + setIsDirty(false); + return; + }, [panelEditor]); + + return isDirty; +} + interface ToolbarAction { group: string; condition?: boolean | string; diff --git a/public/app/features/dashboard-scene/settings/version-history/DiffTitle.tsx b/public/app/features/dashboard-scene/settings/version-history/DiffTitle.tsx index 3716d3534e1..e1bfa6b3aa1 100644 --- a/public/app/features/dashboard-scene/settings/version-history/DiffTitle.tsx +++ b/public/app/features/dashboard-scene/settings/version-history/DiffTitle.tsx @@ -12,7 +12,14 @@ type DiffTitleProps = { title: string; }; -const replaceDiff: Diff = { op: 'replace', originalValue: undefined, path: [''], value: undefined, startLineNumber: 0 }; +const replaceDiff: Diff = { + op: 'replace', + originalValue: undefined, + path: [''], + value: undefined, + startLineNumber: 0, + endLineNumber: 0, +}; export const DiffTitle = ({ diff, title }: DiffTitleProps) => { const styles = useStyles2(getDiffTitleStyles); diff --git a/public/app/features/dashboard-scene/settings/version-history/utils.test.ts b/public/app/features/dashboard-scene/settings/version-history/utils.test.ts index 8d38c5cf6e9..90db4b08325 100644 --- a/public/app/features/dashboard-scene/settings/version-history/utils.test.ts +++ b/public/app/features/dashboard-scene/settings/version-history/utils.test.ts @@ -216,6 +216,7 @@ describe('jsonDiff', () => { const expected = { description: [ { + endLineNumber: 14, op: 'add', originalValue: undefined, path: ['description'], @@ -225,6 +226,7 @@ describe('jsonDiff', () => { ], graphTooltip: [ { + endLineNumber: 17, op: 'replace', originalValue: 0, path: ['graphTooltip'], @@ -234,6 +236,7 @@ describe('jsonDiff', () => { ], panels: [ { + endLineNumber: 23, op: 'add', originalValue: undefined, path: ['panels', '0'], @@ -245,6 +248,7 @@ describe('jsonDiff', () => { ], tags: [ { + endLineNumber: 27, op: 'add', originalValue: undefined, path: ['tags', '0'], @@ -254,6 +258,7 @@ describe('jsonDiff', () => { ], timepicker: [ { + endLineNumber: 49, op: 'add', originalValue: undefined, path: ['timepicker', 'refresh_intervals'], @@ -263,6 +268,7 @@ describe('jsonDiff', () => { ], timezone: [ { + endLineNumber: 51, op: 'replace', originalValue: '', path: ['timezone'], @@ -272,6 +278,7 @@ describe('jsonDiff', () => { ], title: [ { + endLineNumber: 52, op: 'replace', originalValue: 'test dashboard', path: ['title'], @@ -281,6 +288,7 @@ describe('jsonDiff', () => { ], version: [ { + endLineNumber: 54, op: 'replace', originalValue: 2, path: ['version'], diff --git a/public/app/features/dashboard-scene/settings/version-history/utils.ts b/public/app/features/dashboard-scene/settings/version-history/utils.ts index ac036e2acae..ef5d3a21a98 100644 --- a/public/app/features/dashboard-scene/settings/version-history/utils.ts +++ b/public/app/features/dashboard-scene/settings/version-history/utils.ts @@ -3,21 +3,20 @@ import { compare, Operation } from 'fast-json-patch'; import jsonMap from 'json-source-map'; import { flow, get, isArray, isEmpty, last, sortBy, tail, toNumber, isNaN } from 'lodash'; -import { Dashboard } from '@grafana/schema'; - export type Diff = { op: 'add' | 'replace' | 'remove' | 'copy' | 'test' | '_get' | 'move'; value: unknown; originalValue: unknown; path: string[]; startLineNumber: number; + endLineNumber: number; }; export type Diffs = { [key: string]: Diff[]; }; -export type JSONValue = string | Dashboard; +type JSONValue = string | Object; export const jsonDiff = (lhs: JSONValue, rhs: JSONValue): Diffs => { const diffs = compare(lhs, rhs); @@ -29,6 +28,7 @@ export const jsonDiff = (lhs: JSONValue, rhs: JSONValue): Diffs => { let originalValue = undefined; let value = undefined; let startLineNumber = 0; + let endLineNumber = 0; const path = tail(diff.path.split('/')); @@ -36,14 +36,17 @@ export const jsonDiff = (lhs: JSONValue, rhs: JSONValue): Diffs => { originalValue = get(lhs, path); value = diff.value; startLineNumber = rhsMap.pointers[diff.path].value.line; + endLineNumber = rhsMap.pointers[diff.path].valueEnd.line; } if (diff.op === 'add' && rhsMap.pointers[diff.path]) { value = diff.value; startLineNumber = rhsMap.pointers[diff.path].value.line; + endLineNumber = rhsMap.pointers[diff.path].valueEnd.line; } if (diff.op === 'remove' && lhsMap.pointers[diff.path]) { originalValue = get(lhs, path); startLineNumber = lhsMap.pointers[diff.path].value.line; + endLineNumber = lhsMap.pointers[diff.path].valueEnd.line; } return { @@ -52,6 +55,7 @@ export const jsonDiff = (lhs: JSONValue, rhs: JSONValue): Diffs => { path, originalValue, startLineNumber, + endLineNumber, }; }); }; diff --git a/public/app/features/dashboard/components/SaveDashboard/SaveDashboardDiff.tsx b/public/app/features/dashboard/components/SaveDashboard/SaveDashboardDiff.tsx index 0cb291d4e50..8859c59f6cf 100644 --- a/public/app/features/dashboard/components/SaveDashboard/SaveDashboardDiff.tsx +++ b/public/app/features/dashboard/components/SaveDashboard/SaveDashboardDiff.tsx @@ -63,7 +63,16 @@ export const SaveDashboardDiff = ({ {hasFolderChanges && (