From 7801cf6585c08fc3e3ee2d2b4e5066b5e2dc2aac Mon Sep 17 00:00:00 2001 From: kay delaney <45561153+kaydelaney@users.noreply.github.com> Date: Wed, 10 May 2023 14:12:55 +0100 Subject: [PATCH] Dashboards: Only refresh panels on panel edit exit if refresh is set (#67652) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #66838 Co-authored-by: Torkel Ödegaard --- .betterer.results | 3 +- .../dashboard/services/TimeSrv.test.ts | 19 ++- .../features/dashboard/services/TimeSrv.ts | 23 ++-- .../dashboard/state/DashboardModel.test.ts | 115 ++++++++++-------- .../dashboard/state/DashboardModel.ts | 32 +++-- 5 files changed, 105 insertions(+), 87 deletions(-) diff --git a/.betterer.results b/.betterer.results index ef1bca72e3e..5cd31161bc9 100644 --- a/.betterer.results +++ b/.betterer.results @@ -2372,8 +2372,7 @@ exports[`better eslint`] = { [0, 0, 0, "Unexpected any. Specify a different type.", "1"], [0, 0, 0, "Unexpected any. Specify a different type.", "2"], [0, 0, 0, "Unexpected any. Specify a different type.", "3"], - [0, 0, 0, "Unexpected any. Specify a different type.", "4"], - [0, 0, 0, "Unexpected any. Specify a different type.", "5"] + [0, 0, 0, "Unexpected any. Specify a different type.", "4"] ], "public/app/features/dashboard/state/DashboardMigrator.test.ts:5381": [ [0, 0, 0, "Unexpected any. Specify a different type.", "0"], diff --git a/public/app/features/dashboard/services/TimeSrv.test.ts b/public/app/features/dashboard/services/TimeSrv.test.ts index 571b236d098..fb6a20f19c4 100644 --- a/public/app/features/dashboard/services/TimeSrv.test.ts +++ b/public/app/features/dashboard/services/TimeSrv.test.ts @@ -287,19 +287,16 @@ describe('timeSrv', () => { }); }); - describe('pauseAutoRefresh', () => { - it('should set autoRefreshPaused to true', () => { - _dashboard.refresh = '10s'; - timeSrv.pauseAutoRefresh(); - expect(timeSrv.autoRefreshPaused).toBe(true); - }); - }); - describe('resumeAutoRefresh', () => { - it('should set refresh to empty value', () => { - timeSrv.autoRefreshPaused = true; + it('should set auto-refresh interval', () => { + timeSrv.setAutoRefresh('10s'); + expect(timeSrv.refreshTimer).not.toBeUndefined(); + + timeSrv.stopAutoRefresh(); + expect(timeSrv.refreshTimer).toBeUndefined(); + timeSrv.resumeAutoRefresh(); - expect(timeSrv.autoRefreshPaused).toBe(false); + expect(timeSrv.refreshTimer).not.toBeUndefined(); }); }); diff --git a/public/app/features/dashboard/services/TimeSrv.ts b/public/app/features/dashboard/services/TimeSrv.ts index 42a83a9510c..273c7924bc9 100644 --- a/public/app/features/dashboard/services/TimeSrv.ts +++ b/public/app/features/dashboard/services/TimeSrv.ts @@ -23,9 +23,8 @@ import { getRefreshFromUrl } from '../utils/getRefreshFromUrl'; export class TimeSrv { time: any; - refreshTimer: any; + refreshTimer: number | undefined; refresh: any; - autoRefreshPaused = false; oldRefresh: string | null | undefined; timeModel?: TimeModel; timeAtLoad: any; @@ -235,9 +234,9 @@ export class TimeSrv { const validInterval = this.contextSrv.getValidInterval(interval); const intervalMs = rangeUtil.intervalToMs(validInterval); - this.refreshTimer = setTimeout(() => { + this.refreshTimer = window.setTimeout(() => { this.startNextRefreshTimer(intervalMs); - !this.autoRefreshPaused && this.refreshTimeModel(); + this.refreshTimeModel(); }, intervalMs); const refresh = this.contextSrv.getValidInterval(interval); @@ -252,10 +251,10 @@ export class TimeSrv { } private startNextRefreshTimer(afterMs: number) { - this.refreshTimer = setTimeout(() => { + this.refreshTimer = window.setTimeout(() => { this.startNextRefreshTimer(afterMs); if (this.contextSrv.isGrafanaVisible()) { - !this.autoRefreshPaused && this.refreshTimeModel(); + this.refreshTimeModel(); } else { this.autoRefreshBlocked = true; } @@ -264,18 +263,14 @@ export class TimeSrv { stopAutoRefresh() { clearTimeout(this.refreshTimer); - } - - // store timeModel refresh value and pause auto-refresh in some places - // i.e panel edit - pauseAutoRefresh() { - this.autoRefreshPaused = true; + this.refreshTimer = undefined; } // resume auto-refresh based on old dashboard refresh property resumeAutoRefresh() { - this.autoRefreshPaused = false; - this.refreshTimeModel(); + if (this.timeModel?.refresh) { + this.setAutoRefresh(this.timeModel.refresh); + } } setTime(time: RawTimeRange, updateUrl = true) { diff --git a/public/app/features/dashboard/state/DashboardModel.test.ts b/public/app/features/dashboard/state/DashboardModel.test.ts index bfdfcd9a726..3d7c3ff6c1a 100644 --- a/public/app/features/dashboard/state/DashboardModel.test.ts +++ b/public/app/features/dashboard/state/DashboardModel.test.ts @@ -1,6 +1,6 @@ import { keys as _keys } from 'lodash'; -import { VariableHide } from '@grafana/data'; +import { dateTime, TimeRange, VariableHide } from '@grafana/data'; import { defaultVariableModel } from '@grafana/schema'; import { contextSrv } from 'app/core/services/context_srv'; @@ -1158,8 +1158,35 @@ describe('exitViewPanel', () => { }); }); -describe('exitPanelEditor', () => { - function getTestContext(pauseAutoRefresh = false) { +describe('when initEditPanel is called', () => { + function getTestContext() { + const dashboard = createDashboardModelFixture(); + const timeSrvMock = { + pauseAutoRefresh: jest.fn(), + resumeAutoRefresh: jest.fn(), + stopAutoRefresh: jest.fn(), + } as unknown as TimeSrv; + setTimeSrv(timeSrvMock); + return { dashboard, timeSrvMock }; + } + + it('should set panelInEdit', () => { + const { dashboard } = getTestContext(); + dashboard.addPanel({ type: 'timeseries' }); + dashboard.initEditPanel(dashboard.panels[0]); + expect(dashboard.panelInEdit).not.toBeUndefined(); + }); + + it('should stop auto-refresh', () => { + const { dashboard, timeSrvMock } = getTestContext(); + dashboard.addPanel({ type: 'timeseries' }); + dashboard.initEditPanel(dashboard.panels[0]); + expect(timeSrvMock.stopAutoRefresh).toHaveBeenCalled(); + }); +}); + +describe('when exitPanelEditor is called', () => { + function getTestContext() { const panel = new PanelModel({ destroy: jest.fn() }); const dashboard = createDashboardModelFixture(); const timeSrvMock = { @@ -1169,70 +1196,54 @@ describe('exitPanelEditor', () => { } as unknown as TimeSrv; dashboard.startRefresh = jest.fn(); dashboard.panelInEdit = panel; - if (pauseAutoRefresh) { - timeSrvMock.autoRefreshPaused = true; - } setTimeSrv(timeSrvMock); return { dashboard, panel, timeSrvMock }; } - describe('when called', () => { - it('then panelInEdit is set to undefined', () => { - const { dashboard } = getTestContext(); + it('should set panelInEdit to undefined', () => { + const { dashboard } = getTestContext(); - dashboard.exitPanelEditor(); + dashboard.exitPanelEditor(); - expect(dashboard.panelInEdit).toBeUndefined(); - }); + expect(dashboard.panelInEdit).toBeUndefined(); + }); - it('then destroy is called on panel', () => { - const { dashboard, panel } = getTestContext(); + it('should destroy panel', () => { + const { dashboard, panel } = getTestContext(); - dashboard.exitPanelEditor(); + dashboard.exitPanelEditor(); - expect(panel.destroy).toHaveBeenCalled(); - }); + expect(panel.destroy).toHaveBeenCalled(); + }); - it('then startRefresh is not called', () => { - const { dashboard } = getTestContext(); + it('should not call startRefresh', () => { + const { dashboard } = getTestContext(); - dashboard.exitPanelEditor(); + dashboard.exitPanelEditor(); - expect(dashboard.startRefresh).not.toHaveBeenCalled(); - }); + expect(dashboard.startRefresh).not.toHaveBeenCalled(); + }); - it('then auto refresh property is resumed', () => { - const { dashboard, timeSrvMock } = getTestContext(true); - dashboard.exitPanelEditor(); - expect(timeSrvMock.resumeAutoRefresh).toHaveBeenCalled(); - }); - }); -}); + it('should call startRefresh if time range changed during edit', () => { + const { dashboard } = getTestContext(); -describe('initEditPanel', () => { - function getTestContext() { - const dashboard = createDashboardModelFixture(); - const timeSrvMock = { - pauseAutoRefresh: jest.fn(), - resumeAutoRefresh: jest.fn(), - } as unknown as TimeSrv; - setTimeSrv(timeSrvMock); - return { dashboard, timeSrvMock }; - } + const range: TimeRange = { + from: dateTime(new Date().getTime()).subtract(1, 'minutes'), + to: dateTime(new Date().getTime()), + raw: { + from: 'now-1m', + to: 'now', + }, + }; + dashboard.timeRangeUpdated(range); + dashboard.exitPanelEditor(); - describe('when called', () => { - it('then panelInEdit is not undefined', () => { - const { dashboard } = getTestContext(); - dashboard.addPanel({ type: 'timeseries' }); - dashboard.initEditPanel(dashboard.panels[0]); - expect(dashboard.panelInEdit).not.toBeUndefined(); - }); + expect(dashboard.startRefresh).toHaveBeenCalled(); + }); - it('then auto-refresh is paused', () => { - const { dashboard, timeSrvMock } = getTestContext(); - dashboard.addPanel({ type: 'timeseries' }); - dashboard.initEditPanel(dashboard.panels[0]); - expect(timeSrvMock.pauseAutoRefresh).toHaveBeenCalled(); - }); + it('then auto refresh property is resumed', () => { + const { dashboard, timeSrvMock } = getTestContext(); + dashboard.exitPanelEditor(); + expect(timeSrvMock.resumeAutoRefresh).toHaveBeenCalled(); }); }); diff --git a/public/app/features/dashboard/state/DashboardModel.ts b/public/app/features/dashboard/state/DashboardModel.ts index 588b42dfb6b..a8e9af013a4 100644 --- a/public/app/features/dashboard/state/DashboardModel.ts +++ b/public/app/features/dashboard/state/DashboardModel.ts @@ -101,6 +101,7 @@ export class DashboardModel implements TimeModel { private panelsAffectedByVariableChange: number[] | null; private appEventsSubscription: Subscription; private lastRefresh: number; + private timeRangeUpdatedDuringEdit = false; // ------------------ // not persisted @@ -126,6 +127,7 @@ export class DashboardModel implements TimeModel { appEventsSubscription: true, panelsAffectedByVariableChange: true, lastRefresh: true, + timeRangeUpdatedDuringEdit: true, }; constructor( @@ -379,6 +381,10 @@ export class DashboardModel implements TimeModel { timeRangeUpdated(timeRange: TimeRange) { this.events.publish(new TimeRangeUpdatedEvent(timeRange)); dispatch(onTimeRangeUpdated(this.uid, timeRange)); + + if (this.panelInEdit) { + this.timeRangeUpdatedDuringEdit = true; + } } startRefresh(event: VariablesChangedEvent = { refreshAll: true, panelIds: [] }) { @@ -417,11 +423,28 @@ export class DashboardModel implements TimeModel { } initEditPanel(sourcePanel: PanelModel): PanelModel { - getTimeSrv().pauseAutoRefresh(); + getTimeSrv().stopAutoRefresh(); this.panelInEdit = sourcePanel.getEditClone(); + this.timeRangeUpdatedDuringEdit = false; return this.panelInEdit; } + exitPanelEditor() { + this.panelInEdit!.destroy(); + this.panelInEdit = undefined; + + getTimeSrv().resumeAutoRefresh(); + + if (this.panelsAffectedByVariableChange || this.timeRangeUpdatedDuringEdit) { + this.startRefresh({ + panelIds: this.panelsAffectedByVariableChange ?? [], + refreshAll: this.timeRangeUpdatedDuringEdit, + }); + this.panelsAffectedByVariableChange = null; + this.timeRangeUpdatedDuringEdit = false; + } + } + initViewPanel(panel: PanelModel) { this.panelInView = panel; panel.setIsViewing(true); @@ -433,13 +456,6 @@ export class DashboardModel implements TimeModel { this.refreshIfPanelsAffectedByVariableChange(); } - exitPanelEditor() { - this.panelInEdit!.destroy(); - this.panelInEdit = undefined; - getTimeSrv().resumeAutoRefresh(); - this.refreshIfPanelsAffectedByVariableChange(); - } - private refreshIfPanelsAffectedByVariableChange() { if (!this.panelsAffectedByVariableChange) { return;