From b01cbc7aef5e8634688983c08b2391c301156cf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Fri, 13 Oct 2023 16:23:23 +0200 Subject: [PATCH] Dashboard: Fixes save drawer always comparing changes against first loaded version (#76506) * Dashboard: Fixes save changes diff after first save * Lots of type issues * better fix * Update some more places to use new function * Fix * Update * Update * remove console.log * Update --- .betterer.results | 18 +-- .../app/features/alerting/TestRuleResult.tsx | 2 +- .../DashExportModal/DashboardExporter.ts | 2 +- .../DashboardPrompt/DashboardPrompt.test.tsx | 14 +-- .../DashboardPrompt/DashboardPrompt.tsx | 25 ++-- .../EmbeddedDashboard/SaveDashboardDrawer.tsx | 7 +- .../EmbeddedDashboard/SaveDashboardForm.tsx | 3 +- .../dashboard/components/GenAI/utils.ts | 5 +- .../SaveDashboard/DashboardValidation.tsx | 2 +- .../SaveDashboard/SaveDashboardDrawer.tsx | 7 +- .../SaveDashboard/SaveDashboardErrorProxy.tsx | 10 +- .../forms/SaveDashboardAsForm.tsx | 9 +- .../forms/SaveDashboardForm.test.tsx | 24 ++-- .../SaveDashboard/forms/SaveDashboardForm.tsx | 11 +- .../components/SaveDashboard/types.ts | 5 +- .../SaveDashboard/useDashboardSave.tsx | 9 +- .../components/ShareModal/ShareSnapshot.tsx | 2 +- .../containers/EmbeddedDashboardPage.tsx | 4 +- .../dashboard/state/DashboardModel.test.ts | 78 ++++++------ .../dashboard/state/DashboardModel.ts | 116 ++++++++---------- .../dashboard/utils/panelMerge.test.ts | 2 +- public/app/features/runtime/init.ts | 2 +- .../app/features/variables/inspect/utils.ts | 4 +- 23 files changed, 175 insertions(+), 186 deletions(-) diff --git a/.betterer.results b/.betterer.results index e768dc7999e..f99801e80ad 100644 --- a/.betterer.results +++ b/.betterer.results @@ -3112,14 +3112,13 @@ exports[`better eslint`] = { ], "public/app/features/dashboard/components/DashboardPrompt/DashboardPrompt.tsx:5381": [ [0, 0, 0, "Do not use any type assertions.", "0"], - [0, 0, 0, "Unexpected any. Specify a different type.", "1"], + [0, 0, 0, "Do not use any type assertions.", "1"], [0, 0, 0, "Do not use any type assertions.", "2"], [0, 0, 0, "Do not use any type assertions.", "3"], [0, 0, 0, "Do not use any type assertions.", "4"], - [0, 0, 0, "Do not use any type assertions.", "5"], - [0, 0, 0, "Unexpected any. Specify a different type.", "6"], - [0, 0, 0, "Do not use any type assertions.", "7"], - [0, 0, 0, "Unexpected any. Specify a different type.", "8"] + [0, 0, 0, "Unexpected any. Specify a different type.", "5"], + [0, 0, 0, "Do not use any type assertions.", "6"], + [0, 0, 0, "Unexpected any. Specify a different type.", "7"] ], "public/app/features/dashboard/components/DashboardRow/DashboardRow.test.tsx:5381": [ [0, 0, 0, "Unexpected any. Specify a different type.", "0"] @@ -3550,13 +3549,8 @@ exports[`better eslint`] = { [0, 0, 0, "Unexpected any. Specify a different type.", "23"], [0, 0, 0, "Unexpected any. Specify a different type.", "24"], [0, 0, 0, "Unexpected any. Specify a different type.", "25"], - [0, 0, 0, "Unexpected any. Specify a different type.", "26"], - [0, 0, 0, "Do not use any type assertions.", "27"], - [0, 0, 0, "Unexpected any. Specify a different type.", "28"], - [0, 0, 0, "Unexpected any. Specify a different type.", "29"], - [0, 0, 0, "Unexpected any. Specify a different type.", "30"], - [0, 0, 0, "Unexpected any. Specify a different type.", "31"], - [0, 0, 0, "Unexpected any. Specify a different type.", "32"] + [0, 0, 0, "Do not use any type assertions.", "26"], + [0, 0, 0, "Unexpected any. Specify a different type.", "27"] ], "public/app/features/dashboard/state/PanelModel.test.ts:5381": [ [0, 0, 0, "Unexpected any. Specify a different type.", "0"], diff --git a/public/app/features/alerting/TestRuleResult.tsx b/public/app/features/alerting/TestRuleResult.tsx index dc1504f916f..a2c86c917c9 100644 --- a/public/app/features/alerting/TestRuleResult.tsx +++ b/public/app/features/alerting/TestRuleResult.tsx @@ -43,7 +43,7 @@ class UnThemedTestRuleResult extends PureComponent { const { dashboard, panel } = this.props; // dashboard save model - const model = dashboard.getSaveModelClone(); + const model = dashboard.getSaveModelCloneOld(); // now replace panel to get current edits model.panels = model.panels.map((dashPanel) => { diff --git a/public/app/features/dashboard/components/DashExportModal/DashboardExporter.ts b/public/app/features/dashboard/components/DashExportModal/DashboardExporter.ts index 9dc25f22ffd..1e68a587f8e 100644 --- a/public/app/features/dashboard/components/DashExportModal/DashboardExporter.ts +++ b/public/app/features/dashboard/components/DashExportModal/DashboardExporter.ts @@ -83,7 +83,7 @@ export class DashboardExporter { // this is pretty hacky and needs to be changed dashboard.cleanUpRepeats(); - const saveModel = dashboard.getSaveModelClone(); + const saveModel = dashboard.getSaveModelCloneOld(); saveModel.id = null; // undo repeat cleanup diff --git a/public/app/features/dashboard/components/DashboardPrompt/DashboardPrompt.test.tsx b/public/app/features/dashboard/components/DashboardPrompt/DashboardPrompt.test.tsx index 94a437bf93d..ce8b511ce66 100644 --- a/public/app/features/dashboard/components/DashboardPrompt/DashboardPrompt.test.tsx +++ b/public/app/features/dashboard/components/DashboardPrompt/DashboardPrompt.test.tsx @@ -35,7 +35,7 @@ function getTestContext() { const contextSrv = { isSignedIn: true, isEditor: true } as ContextSrv; setContextSrv(contextSrv); const dash = getDefaultDashboardModel(); - const original = dash.getSaveModelClone(); + const original = dash.getSaveModelCloneOld(); return { dash, original, contextSrv }; } @@ -98,7 +98,7 @@ describe('DashboardPrompt', () => { it('then it should return true', () => { const { contextSrv } = getTestContext(); const dash = createDashboardModelFixture({}, { canSave: false }); - const original = dash.getSaveModelClone(); + const original = dash.getSaveModelCloneOld(); contextSrv.isEditor = false; expect(ignoreChanges(dash, original)).toBe(true); }); @@ -108,7 +108,7 @@ describe('DashboardPrompt', () => { it('then it should return undefined', () => { const { contextSrv } = getTestContext(); const dash = createDashboardModelFixture({}, { canSave: true }); - const original = dash.getSaveModelClone(); + const original = dash.getSaveModelCloneOld(); contextSrv.isEditor = false; expect(ignoreChanges(dash, original)).toBe(undefined); }); @@ -118,7 +118,7 @@ describe('DashboardPrompt', () => { it('then it should return true', () => { const { contextSrv } = getTestContext(); const dash = createDashboardModelFixture({}, { canSave: true }); - const original = dash.getSaveModelClone(); + const original = dash.getSaveModelCloneOld(); contextSrv.isSignedIn = false; expect(ignoreChanges(dash, original)).toBe(true); }); @@ -127,7 +127,7 @@ describe('DashboardPrompt', () => { describe('when called with fromScript', () => { it('then it should return true', () => { const dash = createDashboardModelFixture({}, { canSave: true, fromScript: true, fromFile: undefined }); - const original = dash.getSaveModelClone(); + const original = dash.getSaveModelCloneOld(); expect(ignoreChanges(dash, original)).toBe(true); }); }); @@ -146,7 +146,7 @@ describe('DashboardPrompt', () => { describe('when called with fromFile', () => { it('then it should return true', () => { const dash = createDashboardModelFixture({}, { canSave: true, fromScript: undefined, fromFile: true }); - const original = dash.getSaveModelClone(); + const original = dash.getSaveModelCloneOld(); expect(ignoreChanges(dash, original)).toBe(true); }); }); @@ -154,7 +154,7 @@ describe('DashboardPrompt', () => { describe('when called with canSave but without fromScript and fromFile', () => { it('then it should return false', () => { const dash = createDashboardModelFixture({}, { canSave: true, fromScript: undefined, fromFile: undefined }); - const original = dash.getSaveModelClone(); + const original = dash.getSaveModelCloneOld(); expect(ignoreChanges(dash, original)).toBe(undefined); }); }); diff --git a/public/app/features/dashboard/components/DashboardPrompt/DashboardPrompt.tsx b/public/app/features/dashboard/components/DashboardPrompt/DashboardPrompt.tsx index fc3e87a2f30..c5602a8cc44 100644 --- a/public/app/features/dashboard/components/DashboardPrompt/DashboardPrompt.tsx +++ b/public/app/features/dashboard/components/DashboardPrompt/DashboardPrompt.tsx @@ -1,5 +1,5 @@ import * as H from 'history'; -import { each, find } from 'lodash'; +import { find } from 'lodash'; import React, { useContext, useEffect, useState } from 'react'; import { Prompt } from 'react-router-dom'; @@ -37,12 +37,12 @@ export const DashboardPrompt = React.memo(({ dashboard }: Props) => { // This is to minimize unsaved changes warnings due to automatic schema migrations const timeoutId = setTimeout(() => { const originalPath = locationService.getLocation().pathname; - const original = dashboard.getSaveModelClone(); + const original = dashboard.getSaveModelCloneOld(); setState({ originalPath, original }); }, 1000); const savedEventUnsub = appEvents.subscribe(DashboardSavedEvent, () => { - const original = dashboard.getSaveModelClone(); + const original = dashboard.getSaveModelCloneOld(); setState({ originalPath, original }); }); @@ -177,19 +177,22 @@ function cleanDashboardFromIgnoredChanges(dashData: Dashboard) { const dash = model.getSaveModelClone(); // ignore time and refresh - dash.time = 0; + delete dash.time; dash.refresh = ''; dash.schemaVersion = 0; - dash.timezone = 0; + delete dash.timezone; dash.panels = []; // ignore template variable values - each(dash.getVariables(), (variable: any) => { - variable.current = null; - variable.options = null; - variable.filters = null; - }); + if (dash.templating?.list) { + for (const variable of dash.templating.list) { + delete variable.current; + delete variable.options; + // @ts-expect-error + delete variable.filters; + } + } return dash; } @@ -200,7 +203,7 @@ export function hasChanges(current: DashboardModel, original: unknown) { return true; } // TODO: Make getSaveModelClone return Dashboard type instead - const currentClean = cleanDashboardFromIgnoredChanges(current.getSaveModelClone() as unknown as Dashboard); + const currentClean = cleanDashboardFromIgnoredChanges(current.getSaveModelCloneOld() as unknown as Dashboard); const originalClean = cleanDashboardFromIgnoredChanges(original as Dashboard); const currentTimepicker = find((currentClean as any).nav, { type: 'timepicker' }); diff --git a/public/app/features/dashboard/components/EmbeddedDashboard/SaveDashboardDrawer.tsx b/public/app/features/dashboard/components/EmbeddedDashboard/SaveDashboardDrawer.tsx index 105c855a547..c54ca33fe93 100644 --- a/public/app/features/dashboard/components/EmbeddedDashboard/SaveDashboardDrawer.tsx +++ b/public/app/features/dashboard/components/EmbeddedDashboard/SaveDashboardDrawer.tsx @@ -1,6 +1,7 @@ import React, { useMemo, useState } from 'react'; import { config } from '@grafana/runtime'; +import { Dashboard } from '@grafana/schema'; import { Drawer, Tab, TabsBar } from '@grafana/ui'; import { DashboardModel } from '../../state'; @@ -15,16 +16,14 @@ type SaveDashboardDrawerProps = { dashboard: DashboardModel; onDismiss: () => void; dashboardJson: string; - onSave: (clone: DashboardModel) => Promise; + onSave: (clone: Dashboard) => Promise; }; export const SaveDashboardDrawer = ({ dashboard, onDismiss, dashboardJson, onSave }: SaveDashboardDrawerProps) => { const data = useMemo(() => { const clone = dashboard.getSaveModelClone(); - const cloneJSON = JSON.stringify(clone, null, 2); - const cloneSafe = JSON.parse(cloneJSON); // avoids undefined issues - const diff = jsonDiff(JSON.parse(JSON.stringify(dashboardJson, null, 2)), cloneSafe); + const diff = jsonDiff(JSON.parse(JSON.stringify(dashboardJson, null, 2)), clone); let diffCount = 0; for (const d of Object.values(diff)) { diffCount += d.length; diff --git a/public/app/features/dashboard/components/EmbeddedDashboard/SaveDashboardForm.tsx b/public/app/features/dashboard/components/EmbeddedDashboard/SaveDashboardForm.tsx index c8e4f6d2e01..910486e18a0 100644 --- a/public/app/features/dashboard/components/EmbeddedDashboard/SaveDashboardForm.tsx +++ b/public/app/features/dashboard/components/EmbeddedDashboard/SaveDashboardForm.tsx @@ -1,6 +1,7 @@ import React, { useMemo, useState } from 'react'; import { Stack } from '@grafana/experimental'; +import { Dashboard } from '@grafana/schema'; import { Button, Form } from '@grafana/ui'; import { useAppNotification } from 'app/core/copy/appNotification'; @@ -10,7 +11,7 @@ import { SaveDashboardData } from '../SaveDashboard/types'; interface SaveDashboardProps { dashboard: DashboardModel; onCancel: () => void; - onSubmit?: (clone: DashboardModel) => Promise; + onSubmit?: (clone: Dashboard) => Promise; onSuccess: () => void; saveModel: SaveDashboardData; } diff --git a/public/app/features/dashboard/components/GenAI/utils.ts b/public/app/features/dashboard/components/GenAI/utils.ts index 956beb0ac93..f8fc219234c 100644 --- a/public/app/features/dashboard/components/GenAI/utils.ts +++ b/public/app/features/dashboard/components/GenAI/utils.ts @@ -45,9 +45,10 @@ export function getDashboardChanges(dashboard: DashboardModel): { migrationChanges: Diffs; } { // Re-parse the dashboard to remove functions and other non-serializable properties - const currentDashboard = JSON.parse(JSON.stringify(dashboard.getSaveModelClone())); + const currentDashboard = dashboard.getSaveModelClone(); const originalDashboard = dashboard.getOriginalDashboard()!; - const dashboardAfterMigration = JSON.parse(JSON.stringify(new DashboardModel(originalDashboard).getSaveModelClone())); + + const dashboardAfterMigration = new DashboardModel(originalDashboard).getSaveModelClone(); return { userChanges: jsonDiff(dashboardAfterMigration, currentDashboard), diff --git a/public/app/features/dashboard/components/SaveDashboard/DashboardValidation.tsx b/public/app/features/dashboard/components/SaveDashboard/DashboardValidation.tsx index 2f59a6f8b12..6225baac685 100644 --- a/public/app/features/dashboard/components/SaveDashboard/DashboardValidation.tsx +++ b/public/app/features/dashboard/components/SaveDashboard/DashboardValidation.tsx @@ -18,7 +18,7 @@ type ValidationResponse = Awaited { - const saveModel = dashboard.getSaveModelClone(); + const saveModel = dashboard.getSaveModelCloneOld(); const respPromise = backendSrv .validateDashboard(saveModel) // API returns schema validation errors in 4xx range, so resolve them rather than throwing diff --git a/public/app/features/dashboard/components/SaveDashboard/SaveDashboardDrawer.tsx b/public/app/features/dashboard/components/SaveDashboard/SaveDashboardDrawer.tsx index 8bc3e5dda69..5b81085c3a0 100644 --- a/public/app/features/dashboard/components/SaveDashboard/SaveDashboardDrawer.tsx +++ b/public/app/features/dashboard/components/SaveDashboard/SaveDashboardDrawer.tsx @@ -30,10 +30,7 @@ export const SaveDashboardDrawer = ({ dashboard, onDismiss, onSaveSuccess, isCop return { clone, diff: {}, diffCount: 0, hasChanges: false }; } - const cloneJSON = JSON.stringify(clone, null, 2); - const cloneSafe = JSON.parse(cloneJSON); // avoids undefined issues - - const diff = jsonDiff(previous, cloneSafe); + const diff = jsonDiff(previous, clone); let diffCount = 0; for (const d of Object.values(diff)) { diffCount += d.length; @@ -48,7 +45,7 @@ export const SaveDashboardDrawer = ({ dashboard, onDismiss, onSaveSuccess, isCop }, [dashboard, previous, options, isNew]); const [showDiff, setShowDiff] = useState(false); - const { state, onDashboardSave } = useDashboardSave(dashboard, isCopy); + const { state, onDashboardSave } = useDashboardSave(isCopy); const onSuccess = onSaveSuccess ? () => { onDismiss(); diff --git a/public/app/features/dashboard/components/SaveDashboard/SaveDashboardErrorProxy.tsx b/public/app/features/dashboard/components/SaveDashboard/SaveDashboardErrorProxy.tsx index 21af0812755..23882f5af60 100644 --- a/public/app/features/dashboard/components/SaveDashboard/SaveDashboardErrorProxy.tsx +++ b/public/app/features/dashboard/components/SaveDashboard/SaveDashboardErrorProxy.tsx @@ -3,8 +3,10 @@ import React, { useEffect } from 'react'; import { GrafanaTheme2 } from '@grafana/data'; import { FetchError } from '@grafana/runtime'; +import { Dashboard } from '@grafana/schema'; import { Button, ConfirmModal, Modal, useStyles2 } from '@grafana/ui'; -import { DashboardModel } from 'app/features/dashboard/state'; + +import { DashboardModel } from '../../state/DashboardModel'; import { SaveDashboardAsButton } from './SaveDashboardButton'; import { SaveDashboardModalProps } from './types'; @@ -14,7 +16,7 @@ interface SaveDashboardErrorProxyProps { /** original dashboard */ dashboard: DashboardModel; /** dashboard save model with applied modifications, i.e. title */ - dashboardSaveModel: DashboardModel; + dashboardSaveModel: Dashboard; error: FetchError; onDismiss: () => void; } @@ -25,7 +27,7 @@ export const SaveDashboardErrorProxy = ({ error, onDismiss, }: SaveDashboardErrorProxyProps) => { - const { onDashboardSave } = useDashboardSave(dashboard); + const { onDashboardSave } = useDashboardSave(); useEffect(() => { if (error.data && proxyHandlesError(error.data.status)) { @@ -78,7 +80,7 @@ export const SaveDashboardErrorProxy = ({ }; const ConfirmPluginDashboardSaveModal = ({ onDismiss, dashboard }: SaveDashboardModalProps) => { - const { onDashboardSave } = useDashboardSave(dashboard); + const { onDashboardSave } = useDashboardSave(); const styles = useStyles2(getConfirmPluginDashboardSaveModalStyles); return ( diff --git a/public/app/features/dashboard/components/SaveDashboard/forms/SaveDashboardAsForm.tsx b/public/app/features/dashboard/components/SaveDashboard/forms/SaveDashboardAsForm.tsx index 834ed63d3fc..0e86fa849d3 100644 --- a/public/app/features/dashboard/components/SaveDashboard/forms/SaveDashboardAsForm.tsx +++ b/public/app/features/dashboard/components/SaveDashboard/forms/SaveDashboardAsForm.tsx @@ -3,7 +3,7 @@ import React, { ChangeEvent } from 'react'; import { config } from '@grafana/runtime'; import { Button, Input, Switch, Form, Field, InputControl, HorizontalGroup, Label, TextArea } from '@grafana/ui'; import { FolderPicker } from 'app/core/components/Select/FolderPicker'; -import { DashboardModel, PanelModel } from 'app/features/dashboard/state'; +import { DashboardModel } from 'app/features/dashboard/state'; import { validationSrv } from 'app/features/manage-dashboards/services/ValidationSrv'; import { GenAIDashDescriptionButton } from '../../GenAI/GenAIDashDescriptionButton'; @@ -26,11 +26,14 @@ const getSaveAsDashboardClone = (dashboard: DashboardModel) => { // remove alerts if source dashboard is already persisted // do not want to create alert dupes - if (dashboard.id > 0) { - clone.panels.forEach((panel: PanelModel) => { + if (dashboard.id > 0 && clone.panels) { + clone.panels.forEach((panel) => { + // @ts-expect-error if (panel.type === 'graph' && panel.alert) { + // @ts-expect-error delete panel.thresholds; } + // @ts-expect-error delete panel.alert; }); } diff --git a/public/app/features/dashboard/components/SaveDashboard/forms/SaveDashboardForm.test.tsx b/public/app/features/dashboard/components/SaveDashboard/forms/SaveDashboardForm.test.tsx index bcbad4676d7..cf52a3984dc 100644 --- a/public/app/features/dashboard/components/SaveDashboard/forms/SaveDashboardForm.test.tsx +++ b/public/app/features/dashboard/components/SaveDashboard/forms/SaveDashboardForm.test.tsx @@ -2,6 +2,7 @@ import { screen, render } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import React from 'react'; +import { Dashboard } from '@grafana/schema'; import { DashboardModel } from 'app/features/dashboard/state'; import { createDashboardModelFixture } from 'app/features/dashboard/state/__fixtures__/dashboardFixtures'; @@ -15,22 +16,23 @@ const prepareDashboardMock = ( resetTimeSpy: jest.Mock, resetVarsSpy: jest.Mock ) => { - const json = { + const json: Dashboard = { title: 'name', - hasTimeChanged: jest.fn().mockReturnValue(timeChanged), - hasVariableValuesChanged: jest.fn().mockReturnValue(variableValuesChanged), - resetOriginalTime: () => resetTimeSpy(), - resetOriginalVariables: () => resetVarsSpy(), - getSaveModelClone: jest.fn().mockReturnValue({}), + id: 5, + schemaVersion: 30, }; return { - id: 5, - meta: {}, ...json, + meta: {}, + hasTimeChanged: jest.fn().mockReturnValue(timeChanged), + hasVariablesChanged: jest.fn().mockReturnValue(variableValuesChanged), + resetOriginalTime: () => resetTimeSpy(), + resetOriginalVariables: () => resetVarsSpy(), getSaveModelClone: () => json, } as unknown as DashboardModel; }; + const renderAndSubmitForm = async (dashboard: DashboardModel, submitSpy: jest.Mock) => { render( { return {}; }} saveModel={{ - clone: prepareDashboardMock(true, true, jest.fn(), jest.fn()), + clone: { id: 1, schemaVersion: 3 }, diff: {}, diffCount: 0, hasChanges: true, @@ -134,7 +136,7 @@ describe('SaveDashboardAsForm', () => { return {}; }} saveModel={{ - clone: createDashboardModelFixture(), + clone: createDashboardModelFixture().getSaveModelClone(), diff: {}, diffCount: 0, hasChanges: true, diff --git a/public/app/features/dashboard/components/SaveDashboard/forms/SaveDashboardForm.tsx b/public/app/features/dashboard/components/SaveDashboard/forms/SaveDashboardForm.tsx index 908e918f85a..2ffbb0abd99 100644 --- a/public/app/features/dashboard/components/SaveDashboard/forms/SaveDashboardForm.tsx +++ b/public/app/features/dashboard/components/SaveDashboard/forms/SaveDashboardForm.tsx @@ -5,6 +5,7 @@ import { GrafanaTheme2 } from '@grafana/data'; import { selectors } from '@grafana/e2e-selectors'; import { Stack } from '@grafana/experimental'; import { config } from '@grafana/runtime'; +import { Dashboard } from '@grafana/schema'; import { Button, Checkbox, Form, TextArea, useStyles2 } from '@grafana/ui'; import { DashboardModel } from 'app/features/dashboard/state'; @@ -21,7 +22,7 @@ export type SaveProps = { saveModel: SaveDashboardData; // already cloned onCancel: () => void; onSuccess: () => void; - onSubmit?: (clone: DashboardModel, options: SaveDashboardOptions, dashboard: DashboardModel) => Promise; + onSubmit?: (saveModel: Dashboard, options: SaveDashboardOptions, dashboard: DashboardModel) => Promise; options: SaveDashboardOptions; onOptionsChange: (opts: SaveDashboardOptions) => void; }; @@ -37,7 +38,7 @@ export const SaveDashboardForm = ({ onOptionsChange, }: SaveProps) => { const hasTimeChanged = useMemo(() => dashboard.hasTimeChanged(), [dashboard]); - const hasVariableChanged = useMemo(() => dashboard.hasVariableValuesChanged(), [dashboard]); + const hasVariableChanged = useMemo(() => dashboard.hasVariablesChanged(), [dashboard]); const [saving, setSaving] = useState(false); const [message, setMessage] = useState(options.message); @@ -53,12 +54,6 @@ export const SaveDashboardForm = ({ options = { ...options, message }; const result = await onSubmit(saveModel.clone, options, dashboard); if (result.status === 'success') { - if (options.saveVariables) { - dashboard.resetOriginalVariables(); - } - if (options.saveTimerange) { - dashboard.resetOriginalTime(); - } onSuccess(); } else { setSaving(false); diff --git a/public/app/features/dashboard/components/SaveDashboard/types.ts b/public/app/features/dashboard/components/SaveDashboard/types.ts index 1e757c0bad0..0baff9a2d89 100644 --- a/public/app/features/dashboard/components/SaveDashboard/types.ts +++ b/public/app/features/dashboard/components/SaveDashboard/types.ts @@ -1,10 +1,11 @@ +import { Dashboard } from '@grafana/schema'; import { CloneOptions, DashboardModel } from 'app/features/dashboard/state/DashboardModel'; import { DashboardDataDTO } from 'app/types'; import { Diffs } from '../VersionHistory/utils'; export interface SaveDashboardData { - clone: DashboardModel; // cloned copy + clone: Dashboard; // cloned copy diff: Diffs; diffCount: number; // cumulative count hasChanges: boolean; // not new and has changes @@ -29,7 +30,7 @@ export interface SaveDashboardFormProps { isLoading: boolean; onCancel: () => void; onSuccess: () => void; - onSubmit?: (clone: DashboardModel, options: SaveDashboardOptions, dashboard: DashboardModel) => Promise; + onSubmit?: (saveModel: Dashboard, options: SaveDashboardOptions, dashboard: DashboardModel) => Promise; } export interface SaveDashboardModalProps { diff --git a/public/app/features/dashboard/components/SaveDashboard/useDashboardSave.tsx b/public/app/features/dashboard/components/SaveDashboard/useDashboardSave.tsx index 8aa6db501de..da61281c296 100644 --- a/public/app/features/dashboard/components/SaveDashboard/useDashboardSave.tsx +++ b/public/app/features/dashboard/components/SaveDashboard/useDashboardSave.tsx @@ -2,6 +2,7 @@ import { useAsyncFn } from 'react-use'; import { locationUtil } from '@grafana/data'; import { locationService, reportInteraction } from '@grafana/runtime'; +import { Dashboard } from '@grafana/schema'; import appEvents from 'app/core/app_events'; import { useAppNotification } from 'app/core/copy/appNotification'; import { contextSrv } from 'app/core/core'; @@ -49,16 +50,18 @@ const saveDashboard = async ( } }; -export const useDashboardSave = (dashboard: DashboardModel, isCopy = false) => { +export const useDashboardSave = (isCopy = false) => { const dispatch = useDispatch(); const notifyApp = useAppNotification(); const [saveDashboardRtkQuery] = useSaveDashboardMutation(); const [state, onDashboardSave] = useAsyncFn( - async (clone: DashboardModel, options: SaveDashboardOptions, dashboard: DashboardModel) => { + async (clone: Dashboard, options: SaveDashboardOptions, dashboard: DashboardModel) => { try { const result = await saveDashboard(clone, options, dashboard, saveDashboardRtkQuery); dashboard.version = result.version; - dashboard.clearUnsavedChanges(); + + clone.version = result.version; + dashboard.clearUnsavedChanges(clone, options); // important that these happen before location redirect below appEvents.publish(new DashboardSavedEvent()); diff --git a/public/app/features/dashboard/components/ShareModal/ShareSnapshot.tsx b/public/app/features/dashboard/components/ShareModal/ShareSnapshot.tsx index 1923a0fb30f..d0f7b3baa8f 100644 --- a/public/app/features/dashboard/components/ShareModal/ShareSnapshot.tsx +++ b/public/app/features/dashboard/components/ShareModal/ShareSnapshot.tsx @@ -97,7 +97,7 @@ export class ShareSnapshot extends PureComponent { saveSnapshot = async (dashboard: DashboardModel, external?: boolean) => { const { snapshotExpires } = this.state; - const dash = this.dashboard.getSaveModelClone(); + const dash = this.dashboard.getSaveModelCloneOld(); this.scrubDashboard(dash); diff --git a/public/app/features/dashboard/containers/EmbeddedDashboardPage.tsx b/public/app/features/dashboard/containers/EmbeddedDashboardPage.tsx index 86a61bc4ea5..d33fbfd1c90 100644 --- a/public/app/features/dashboard/containers/EmbeddedDashboardPage.tsx +++ b/public/app/features/dashboard/containers/EmbeddedDashboardPage.tsx @@ -3,7 +3,7 @@ import React, { useEffect, useState } from 'react'; import { GrafanaTheme2, PageLayoutType } from '@grafana/data'; import { getBackendSrv, locationService } from '@grafana/runtime'; -import { TimeZone } from '@grafana/schema'; +import { Dashboard, TimeZone } from '@grafana/schema'; import { Button, ModalsController, PageToolbar, useStyles2 } from '@grafana/ui'; import { Page } from 'app/core/components/Page/Page'; import { useGrafana } from 'app/core/context/GrafanaContext'; @@ -105,7 +105,7 @@ const Toolbar = ({ dashboard, dashboardJson }: ToolbarProps) => { dispatch(updateTimeZoneForSession(timeZone)); }; - const saveDashboard = async (clone: DashboardModel) => { + const saveDashboard = async (clone: Dashboard) => { const params = locationService.getSearch(); const serverPort = params.get('serverPort'); if (!clone || !serverPort) { diff --git a/public/app/features/dashboard/state/DashboardModel.test.ts b/public/app/features/dashboard/state/DashboardModel.test.ts index 7db392e38d7..45294c5df9e 100644 --- a/public/app/features/dashboard/state/DashboardModel.test.ts +++ b/public/app/features/dashboard/state/DashboardModel.test.ts @@ -130,7 +130,7 @@ describe('DashboardModel', () => { const saveModel = model.getSaveModelClone(); const panels = saveModel.panels; - expect(panels.length).toBe(1); + expect(panels!.length).toBe(1); }); it('should save model in edit mode', () => { @@ -140,7 +140,7 @@ describe('DashboardModel', () => { const panel = model.initEditPanel(model.panels[0]); panel.title = 'updated'; - const saveModel = model.getSaveModelClone(); + const saveModel = model.getSaveModelCloneOld(); const savedPanel = saveModel.panels[0]; expect(savedPanel.title).toBe('updated'); @@ -204,7 +204,7 @@ describe('DashboardModel', () => { }); it('getSaveModelClone should remove meta', () => { - const clone = model.getSaveModelClone(); + const clone = model.getSaveModelCloneOld(); expect(clone.meta).toBe(undefined); }); }); @@ -608,37 +608,32 @@ describe('DashboardModel', () => { const options = { saveTimerange: false }; const saveModel = model.getSaveModelClone(options); - expect(saveModel.time.from).toBe('now-6h'); - expect(saveModel.time.to).toBe('now'); + expect(saveModel.time!.from).toBe('now-6h'); + expect(saveModel.time!.to).toBe('now'); }); it('getSaveModelClone should return updated time when saveTimerange=true', () => { const options = { saveTimerange: true }; const saveModel = model.getSaveModelClone(options); - expect(saveModel.time.from).toBe('now-3h'); - expect(saveModel.time.to).toBe('now-1h'); - }); - - it('hasTimeChanged should be false when reset original time', () => { - model.resetOriginalTime(); - expect(model.hasTimeChanged()).toBeFalsy(); + expect(saveModel.time!.from).toBe('now-3h'); + expect(saveModel.time!.to).toBe('now-1h'); }); it('getSaveModelClone should return original time when saveTimerange=false', () => { const options = { saveTimerange: false }; const saveModel = model.getSaveModelClone(options); - expect(saveModel.time.from).toBe('now-6h'); - expect(saveModel.time.to).toBe('now'); + expect(saveModel.time!.from).toBe('now-6h'); + expect(saveModel.time!.to).toBe('now'); }); it('getSaveModelClone should return updated time when saveTimerange=true', () => { const options = { saveTimerange: true }; const saveModel = model.getSaveModelClone(options); - expect(saveModel.time.from).toBe('now-3h'); - expect(saveModel.time.to).toBe('now-1h'); + expect(saveModel.time!.from).toBe('now-3h'); + expect(saveModel.time!.to).toBe('now-1h'); }); it('getSaveModelClone should remove repeated panels and scopedVars', () => { @@ -684,13 +679,13 @@ describe('DashboardModel', () => { expect(model.panels.find((x) => x.type !== 'row')?.scopedVars?.dc?.value).toBe('dc1'); expect(model.panels.find((x) => x.type !== 'row')?.scopedVars?.app?.value).toBe('se1'); - const saveModel = model.getSaveModelClone(); + const saveModel = model.getSaveModelCloneOld(); expect(saveModel.panels.length).toBe(2); expect(saveModel.panels[0].scopedVars).toBe(undefined); expect(saveModel.panels[1].scopedVars).toBe(undefined); model.collapseRows(); - const savedModelWithCollapsedRows = model.getSaveModelClone(); + const savedModelWithCollapsedRows = model.getSaveModelCloneOld(); expect(savedModelWithCollapsedRows.panels[0].panels!.length).toBe(1); }); @@ -738,14 +733,14 @@ describe('DashboardModel', () => { expect(model.panels.find((x) => x.type !== 'row')?.scopedVars?.app?.value).toBe('se1'); model.snapshot = { timestamp: new Date() }; - const saveModel = model.getSaveModelClone(); + const saveModel = model.getSaveModelCloneOld(); expect(saveModel.panels.filter((x) => x.type === 'row')).toHaveLength(2); expect(saveModel.panels.filter((x) => x.type !== 'row')).toHaveLength(4); expect(saveModel.panels.find((x) => x.type !== 'row')?.scopedVars?.dc?.value).toBe('dc1'); expect(saveModel.panels.find((x) => x.type !== 'row')?.scopedVars?.app?.value).toBe('se1'); model.collapseRows(); - const savedModelWithCollapsedRows = model.getSaveModelClone(); + const savedModelWithCollapsedRows = model.getSaveModelCloneOld(); expect(savedModelWithCollapsedRows.panels[0].panels!.length).toBe(2); }); }); @@ -760,6 +755,8 @@ describe('DashboardModel', () => { { name: 'Server', type: 'query', + refresh: 1, + options: [], current: { selected: true, text: 'server_001', @@ -770,10 +767,10 @@ describe('DashboardModel', () => { }, }; model = getDashboardModel(json); - expect(model.hasVariableValuesChanged()).toBeFalsy(); + expect(model.hasVariablesChanged()).toBeFalsy(); }); - it('hasVariableValuesChanged should be false when adding a template variable', () => { + it('hasVariablesChanged should be false when adding a template variable', () => { model.templating.list.push({ name: 'Server2', type: 'query', @@ -783,24 +780,24 @@ describe('DashboardModel', () => { value: 'server_002', }, }); - expect(model.hasVariableValuesChanged()).toBeFalsy(); + expect(model.hasVariablesChanged()).toBeFalsy(); }); - it('hasVariableValuesChanged should be false when removing existing template variable', () => { + it('hasVariablesChanged should be false when removing existing template variable', () => { model.templating.list = []; - expect(model.hasVariableValuesChanged()).toBeFalsy(); + expect(model.hasVariablesChanged()).toBeFalsy(); }); - it('hasVariableValuesChanged should be true when changing value of template variable', () => { + it('hasVariablesChanged should be true when changing value of template variable', () => { model.templating.list[0].current.text = 'server_002'; - expect(model.hasVariableValuesChanged()).toBeTruthy(); + expect(model.hasVariablesChanged()).toBeTruthy(); }); it('getSaveModelClone should return original variable when saveVariables=false', () => { model.templating.list[0].current.text = 'server_002'; const options = { saveVariables: false }; - const saveModel = model.getSaveModelClone(options); + const saveModel = model.getSaveModelCloneOld(options); expect(saveModel.templating.list[0].current.text).toBe('server_001'); }); @@ -809,7 +806,7 @@ describe('DashboardModel', () => { model.templating.list[0].current.text = 'server_002'; const options = { saveVariables: true }; - const saveModel = model.getSaveModelClone(options); + const saveModel = model.getSaveModelCloneOld(options); expect(saveModel.templating.list[0].current.text).toBe('server_002'); }); @@ -825,6 +822,7 @@ describe('DashboardModel', () => { { name: 'Filter', type: 'adhoc', + refresh: 0, filters: [ { key: '@hostname', @@ -837,10 +835,10 @@ describe('DashboardModel', () => { }, }; model = getDashboardModel(json); - expect(model.hasVariableValuesChanged()).toBeFalsy(); + expect(model.hasVariablesChanged()).toBeFalsy(); }); - it('hasVariableValuesChanged should be false when adding a template variable', () => { + it('hasVariablesChanged should be false when adding a template variable', () => { model.templating.list.push({ name: 'Filter', type: 'adhoc', @@ -852,34 +850,34 @@ describe('DashboardModel', () => { }, ], }); - expect(model.hasVariableValuesChanged()).toBeFalsy(); + expect(model.hasVariablesChanged()).toBeFalsy(); }); - it('hasVariableValuesChanged should be false when removing existing template variable', () => { + it('hasVariablesChanged should be false when removing existing template variable', () => { model.templating.list = []; - expect(model.hasVariableValuesChanged()).toBeFalsy(); + expect(model.hasVariablesChanged()).toBeFalsy(); }); - it('hasVariableValuesChanged should be true when changing value of filter', () => { + it('hasVariablesChanged should be true when changing value of filter', () => { model.templating.list[0].filters[0].value = 'server 1'; - expect(model.hasVariableValuesChanged()).toBeTruthy(); + expect(model.hasVariablesChanged()).toBeTruthy(); }); - it('hasVariableValuesChanged should be true when adding an additional condition', () => { + it('hasVariablesChanged should be true when adding an additional condition', () => { model.templating.list[0].filters[0].condition = 'AND'; model.templating.list[0].filters[1] = { key: '@metric', operator: '=', value: 'logins.count', }; - expect(model.hasVariableValuesChanged()).toBeTruthy(); + expect(model.hasVariablesChanged()).toBeTruthy(); }); it('getSaveModelClone should return original variable when saveVariables=false', () => { model.templating.list[0].filters[0].value = 'server 1'; const options = { saveVariables: false }; - const saveModel = model.getSaveModelClone(options); + const saveModel = model.getSaveModelCloneOld(options); expect(saveModel.templating.list[0].filters[0].value).toBe('server 20'); }); @@ -888,7 +886,7 @@ describe('DashboardModel', () => { model.templating.list[0].filters[0].value = 'server 1'; const options = { saveVariables: true }; - const saveModel = model.getSaveModelClone(options); + const saveModel = model.getSaveModelCloneOld(options); expect(saveModel.templating.list[0].filters[0].value).toBe('server 1'); }); diff --git a/public/app/features/dashboard/state/DashboardModel.ts b/public/app/features/dashboard/state/DashboardModel.ts index d99b0f89628..a42b88837b1 100644 --- a/public/app/features/dashboard/state/DashboardModel.ts +++ b/public/app/features/dashboard/state/DashboardModel.ts @@ -24,7 +24,6 @@ import { GRID_CELL_HEIGHT, GRID_CELL_VMARGIN, GRID_COLUMN_COUNT, REPEAT_DIR_VERT import { contextSrv } from 'app/core/services/context_srv'; import { sortedDeepCloneWithoutNulls } from 'app/core/utils/object'; import { isAngularDatasourcePlugin } from 'app/features/plugins/angularDeprecation/utils'; -import { deepFreeze } from 'app/features/plugins/extensions/utils'; import { variableAdapters } from 'app/features/variables/adapters'; import { onTimeRangeUpdated } from 'app/features/variables/state/actions'; import { GetVariables, getVariablesByKey } from 'app/features/variables/state/selectors'; @@ -175,12 +174,12 @@ export class DashboardModel implements TimeModel { this.panels = map(data.panels ?? [], (panelData: any) => new PanelModel(panelData)); // Deep clone original dashboard to avoid mutations by object reference this.originalDashboard = cloneDeep(data); + this.originalTemplating = cloneDeep(this.templating); + this.originalTime = cloneDeep(this.time); + this.ensurePanelsHaveUniqueIds(); this.formatDate = this.formatDate.bind(this); - this.resetOriginalVariables(true); - this.resetOriginalTime(); - this.initMeta(meta); this.updateSchema(data); @@ -248,9 +247,11 @@ export class DashboardModel implements TimeModel { this.meta = meta; } - // cleans meta data and other non persistent state - getSaveModelClone(options?: CloneOptions): DashboardModel { - const defaults = _defaults(options || {}, { + /** + * @deprecated Returns the wrong type please do not use + */ + getSaveModelCloneOld(options?: CloneOptions): DashboardModel { + const optionsWithDefaults = _defaults(options || {}, { saveVariables: true, saveTimerange: true, }); @@ -265,9 +266,9 @@ export class DashboardModel implements TimeModel { copy[property] = cloneDeep(this[property]); } - this.updateTemplatingSaveModelClone(copy, defaults); + copy.templating = this.getTemplatingSaveModel(optionsWithDefaults); - if (!defaults.saveTimerange) { + if (!optionsWithDefaults.saveTimerange) { copy.time = this.originalTime; } @@ -281,6 +282,19 @@ export class DashboardModel implements TimeModel { return copy; } + /** + * Returns the persisted save model (schema) of the dashboard + */ + getSaveModelClone(options?: CloneOptions): Dashboard { + const clone = this.getSaveModelCloneOld(options); + + // This is a bit messy / hacky but it's how we clean the model of any nulls / undefined / infinity + const cloneJSON = JSON.stringify(clone); + const cloneSafe = JSON.parse(cloneJSON); + + return cloneSafe; + } + /** * This will load a new dashboard, but keep existing panels unchanged * @@ -353,36 +367,35 @@ export class DashboardModel implements TimeModel { }); } - private updateTemplatingSaveModelClone( - copy: any, - defaults: { saveTimerange: boolean; saveVariables: boolean } & CloneOptions - ) { - const originalVariables = this.originalTemplating; + private getTemplatingSaveModel(options: CloneOptions) { + const originalVariables = this.originalTemplating?.list ?? []; const currentVariables = this.getVariablesFromState(this.uid); - copy.templating = { - list: currentVariables.map((variable) => - variableAdapters.get(variable.type).getSaveModel(variable, defaults.saveVariables) - ), - }; + const saveModels = currentVariables.map((variable) => { + const variableSaveModel = variableAdapters.get(variable.type).getSaveModel(variable, options.saveVariables); - if (!defaults.saveVariables) { - for (const current of copy.templating.list) { + if (!options.saveVariables) { const original = originalVariables.find( - ({ name, type }: any) => name === current.name && type === current.type + ({ name, type }: any) => name === variable.name && type === variable.type ); if (!original) { - continue; + return variableSaveModel; } - if (current.type === 'adhoc') { - current.filters = original.filters; + if (variable.type === 'adhoc') { + variableSaveModel.filters = original.filters; } else { - current.current = original.current; + variableSaveModel.current = original.current; + variableSaveModel.options = original.options; } } - } + + return variableSaveModel; + }); + + const saveModelsWithoutNull = sortedDeepCloneWithoutNulls(saveModels); + return { list: saveModelsWithoutNull }; } timeRangeUpdated(timeRange: TimeRange) { @@ -567,7 +580,7 @@ export class DashboardModel implements TimeModel { }); } - clearUnsavedChanges() { + clearUnsavedChanges(savedModel: Dashboard, options: CloneOptions) { for (const panel of this.panels) { panel.configRev = 0; } @@ -577,6 +590,13 @@ export class DashboardModel implements TimeModel { this.panelInEdit.hasSavedPanelEditChange = this.panelInEdit.configRev > 0; this.panelInEdit.configRev = 0; } + + this.originalDashboard = savedModel; + this.originalTemplating = savedModel.templating; + + if (options.saveTimerange) { + this.originalTime = savedModel.time; + } } hasUnsavedChanges() { @@ -1082,10 +1102,6 @@ export class DashboardModel implements TimeModel { migrator.updateSchema(old); } - resetOriginalTime() { - this.originalTime = deepFreeze(this.time); - } - hasTimeChanged() { const { time, originalTime } = this; @@ -1097,19 +1113,6 @@ export class DashboardModel implements TimeModel { ); } - resetOriginalVariables(initial = false) { - if (initial) { - this.originalTemplating = this.cloneVariablesFrom(this.templating.list); - return; - } - - this.originalTemplating = this.cloneVariablesFrom(this.getVariablesFromState(this.uid)); - } - - hasVariableValuesChanged() { - return this.hasVariablesChanged(this.originalTemplating, this.getVariablesFromState(this.uid)); - } - autoFitPanels(viewHeight: number, kioskMode?: UrlQueryValue) { const currentGridHeight = Math.max(...this.panels.map((panel) => panel.gridPos.h + panel.gridPos.y)); @@ -1245,28 +1248,15 @@ export class DashboardModel implements TimeModel { return this.getVariablesFromState(this.uid).length > 0; } - private hasVariablesChanged(originalVariables: any[], currentVariables: any[]): boolean { + public hasVariablesChanged(): boolean { + const originalVariables = this.originalTemplating?.list ?? []; + const currentVariables = this.getTemplatingSaveModel({ saveVariables: true }).list; + if (originalVariables.length !== currentVariables.length) { return false; } - const updated = currentVariables.map((variable: any) => ({ - name: variable.name, - type: variable.type, - current: cloneDeep(variable.current), - filters: cloneDeep(variable.filters), - })); - - return !isEqual(updated, originalVariables); - } - - private cloneVariablesFrom(variables: any[]) { - return variables.map((variable) => ({ - name: variable.name, - type: variable.type, - current: deepFreeze(variable.current), - filters: deepFreeze(variable.filters), - })); + return !isEqual(currentVariables, originalVariables); } private variablesTimeRangeProcessDoneHandler(event: VariablesTimeRangeProcessDone) { diff --git a/public/app/features/dashboard/utils/panelMerge.test.ts b/public/app/features/dashboard/utils/panelMerge.test.ts index 4aa420ee76d..4bc201aa8ad 100644 --- a/public/app/features/dashboard/utils/panelMerge.test.ts +++ b/public/app/features/dashboard/utils/panelMerge.test.ts @@ -41,7 +41,7 @@ describe('Merge dashboard panels', () => { }), ], }); - rawPanels = dashboard.getSaveModelClone().panels; + rawPanels = dashboard.getSaveModelCloneOld().panels; }); it('should load and support noop', () => { diff --git a/public/app/features/runtime/init.ts b/public/app/features/runtime/init.ts index 5ae3e02dce7..1f71bf55154 100644 --- a/public/app/features/runtime/init.ts +++ b/public/app/features/runtime/init.ts @@ -29,7 +29,7 @@ export function initWindowRuntime() { if (!d) { return undefined; } - return d.getSaveModelClone(); + return d.getSaveModelCloneOld(); }, /** The selected time range */ diff --git a/public/app/features/variables/inspect/utils.ts b/public/app/features/variables/inspect/utils.ts index 16e1f3cf88d..281d8df449b 100644 --- a/public/app/features/variables/inspect/utils.ts +++ b/public/app/features/variables/inspect/utils.ts @@ -193,7 +193,7 @@ export const createUsagesNetwork = (variables: VariableModel[], dashboard: Dashb const unUsed: VariableModel[] = []; let usages: VariableUsageTree[] = []; - const model = dashboard.getSaveModelClone(); + const model = dashboard.getSaveModelCloneOld(); for (const variable of variables) { const variableId = variable.id; @@ -233,7 +233,7 @@ function createUnknownsNetwork(variables: VariableModel[], dashboard: DashboardM } let unknown: VariableUsageTree[] = []; - const model = dashboard.getSaveModelClone(); + const model = dashboard.getSaveModelCloneOld(); const unknownVariables = getUnknownVariableStrings(variables, model); for (const unknownVariable of unknownVariables) {