From 9af0cd5844fa3028279f6decf79f1a486bd3cde4 Mon Sep 17 00:00:00 2001 From: "grafana-delivery-bot[bot]" <132647405+grafana-delivery-bot[bot]@users.noreply.github.com> Date: Fri, 3 May 2024 18:55:18 +0300 Subject: [PATCH] [v11.0.x] datatrails: recently loaded trails modify parent variables when making new steps (#87333) datatrails: recently loaded trails modify parent variables when making new steps (#87284) * fix: loaded trails modified parent var on new step This ensures that recently loaded trails won't have variable changes which create new steps modify the previous step. (cherry picked from commit 046eedaa4ca05bc2a1594e43d552b7147c3aa76d) Co-authored-by: Darren Janeczek <38694490+darrenjaneczek@users.noreply.github.com> --- public/app/features/trails/DataTrail.test.tsx | 100 ++++++++++-- public/app/features/trails/DataTrail.tsx | 4 + .../trails/TrailStore/TrailStore.test.ts | 145 +++++++++++++++++- 3 files changed, 228 insertions(+), 21 deletions(-) diff --git a/public/app/features/trails/DataTrail.test.tsx b/public/app/features/trails/DataTrail.test.tsx index a6ccc31f354..e8617751909 100644 --- a/public/app/features/trails/DataTrail.test.tsx +++ b/public/app/features/trails/DataTrail.test.tsx @@ -1,8 +1,8 @@ import { locationService, setDataSourceSrv } from '@grafana/runtime'; import { AdHocFiltersVariable, sceneGraph } from '@grafana/scenes'; +import { DataSourceType } from 'app/features/alerting/unified/utils/datasource'; import { MockDataSourceSrv, mockDataSource } from '../alerting/unified/mocks'; -import { DataSourceType } from '../alerting/unified/utils/datasource'; import { activateFullSceneTree } from '../dashboard-scene/utils/test-utils'; import { DataTrail } from './DataTrail'; @@ -26,6 +26,22 @@ describe('DataTrail', () => { let trail: DataTrail; const preTrailUrl = '/'; + function getFilterVar() { + const variable = sceneGraph.lookupVariable(VAR_FILTERS, trail); + if (variable instanceof AdHocFiltersVariable) { + return variable; + } + throw new Error('getFilterVar failed'); + } + + function getStepFilterVar(step: number) { + const variable = trail.state.history.state.steps[step].trailState.$variables?.getByName(VAR_FILTERS); + if (variable instanceof AdHocFiltersVariable) { + return variable; + } + throw new Error(`getStepFilterVar failed for step ${step}`); + } + beforeEach(() => { trail = new DataTrail({}); locationService.push(preTrailUrl); @@ -199,22 +215,6 @@ describe('DataTrail', () => { }); }); - function getFilterVar() { - const variable = sceneGraph.lookupVariable(VAR_FILTERS, trail); - if (variable instanceof AdHocFiltersVariable) { - return variable; - } - throw new Error('getFilterVar failed'); - } - - function getStepFilterVar(step: number) { - const variable = trail.state.history.state.steps[step].trailState.$variables?.getByName(VAR_FILTERS); - if (variable instanceof AdHocFiltersVariable) { - return variable; - } - throw new Error(`getStepFilterVar failed for step ${step}`); - } - it('Should have default empty filter', () => { expect(getFilterVar().state.filters.length).toBe(0); }); @@ -410,5 +410,71 @@ describe('DataTrail', () => { expect(locationService.getSearch().has('metric')).toBe(false); }); }); + + it('Filter should be empty', () => { + expect(getStepFilterVar(0).state.filters.length).toBe(0); + }); + + describe('And filter is added zone=a', () => { + beforeEach(() => { + getFilterVar().setState({ filters: [{ key: 'zone', operator: '=', value: 'a' }] }); + }); + + it('Filter of trail should be zone=a', () => { + expect(getFilterVar().state.filters[0].key).toBe('zone'); + expect(getFilterVar().state.filters[0].value).toBe('a'); + }); + + it('Filter of step 1 should be zone=a', () => { + expect(getStepFilterVar(1).state.filters[0].key).toBe('zone'); + expect(getStepFilterVar(1).state.filters[0].value).toBe('a'); + }); + + it('Filter of step 0 should empty', () => { + expect(getStepFilterVar(0).state.filters.length).toBe(0); + }); + + describe('When returning to step 0', () => { + beforeEach(() => { + trail.state.history.goBackToStep(0); + }); + + it('Filter of trail should be empty', () => { + expect(getFilterVar().state.filters.length).toBe(0); + }); + }); + }); + + it('Time range `from` should be now-6h', () => { + expect(trail.state.$timeRange?.state.from).toBe('now-6h'); + }); + + describe('And time range is changed to now-15m to now', () => { + beforeEach(() => { + trail.state.$timeRange?.setState({ from: 'now-15m' }); + }); + + it('Time range `from` should be now-15m', () => { + expect(trail.state.$timeRange?.state.from).toBe('now-15m'); + }); + + it('Time range `from` of step 1 should be now-15m', () => { + expect(trail.state.history.state.steps[1].trailState.$timeRange?.state.from).toBe('now-15m'); + }); + + it('Time range `from` of step 0 should be now-6h', () => { + expect(trail.state.history.state.steps[0].trailState.$timeRange?.state.from).toBe('now-6h'); + }); + + describe('When returning to step 0', () => { + beforeEach(() => { + trail.state.history.goBackToStep(0); + }); + + it('Time range `from` should be now-6h', () => { + expect(trail.state.$timeRange?.state.from).toBe('now-6h'); + }); + }); + }); }); }); diff --git a/public/app/features/trails/DataTrail.tsx b/public/app/features/trails/DataTrail.tsx index 4b3820f5f96..840a89878ab 100644 --- a/public/app/features/trails/DataTrail.tsx +++ b/public/app/features/trails/DataTrail.tsx @@ -93,6 +93,10 @@ export class DataTrail extends SceneObjectBase { ); } + // Disconnects the current step history state from the current state, to prevent changes affecting history state + const currentState = this.state.history.state.steps[this.state.history.state.currentStep].trailState; + this.restoreFromHistoryStep(currentState); + this.enableUrlSync(); // Save the current trail as a recent if the browser closes or reloads diff --git a/public/app/features/trails/TrailStore/TrailStore.test.ts b/public/app/features/trails/TrailStore/TrailStore.test.ts index 8fe31ff9181..1a3bccd567c 100644 --- a/public/app/features/trails/TrailStore/TrailStore.test.ts +++ b/public/app/features/trails/TrailStore/TrailStore.test.ts @@ -1,4 +1,10 @@ -import { BOOKMARKED_TRAILS_KEY, RECENT_TRAILS_KEY } from '../shared'; +import { locationService, setDataSourceSrv } from '@grafana/runtime'; +import { AdHocFiltersVariable, getUrlSyncManager, sceneGraph } from '@grafana/scenes'; +import { DataSourceType } from 'app/features/alerting/unified/utils/datasource'; + +import { MockDataSourceSrv, mockDataSource } from '../../alerting/unified/mocks'; +import { DataTrail } from '../DataTrail'; +import { BOOKMARKED_TRAILS_KEY, RECENT_TRAILS_KEY, VAR_FILTERS } from '../shared'; import { SerializedTrail, getTrailStore } from './TrailStore'; @@ -21,6 +27,17 @@ describe('TrailStore', () => { global.localStorage = localStorageMock as unknown as Storage; jest.useFakeTimers(); + + // Having the mock service set up is required for activating the loaded trails + setDataSourceSrv( + new MockDataSourceSrv({ + prom: mockDataSource({ + name: 'Prometheus', + type: DataSourceType.Prometheus, + uid: 'ds', + }), + }) + ); }); describe('Empty store', () => { @@ -35,7 +52,7 @@ describe('TrailStore', () => { }); }); - describe('Initialize store with one recent trail', () => { + describe('Initialize store with one recent trail with final current step', () => { const history: SerializedTrail['history'] = [ { urlValues: { @@ -134,7 +151,7 @@ describe('TrailStore', () => { ['metric', 'different_metric'], ['from', 'now-1y'], ['to', 'now-30m'], - ['var-ds', '1234'], + ['var-ds', 'ds'], ['var-groupby', 'job'], ['var-filters', 'cluster|=|dev-eu-west-2'], ])(`new recent trails with a different '%p' value should insert new entry`, (key, differentValue) => { @@ -253,6 +270,126 @@ describe('TrailStore', () => { expect(trail.state.history.state.currentStep).toBe(1); }); + function getFilterVar(trail: DataTrail) { + const variable = sceneGraph.lookupVariable(VAR_FILTERS, trail); + if (variable instanceof AdHocFiltersVariable) { + return variable; + } + throw new Error('getFilterVar failed'); + } + + function getStepFilterVar(trail: DataTrail, step: number) { + const variable = trail.state.history.state.steps[step].trailState.$variables?.getByName(VAR_FILTERS); + if (variable instanceof AdHocFiltersVariable) { + return variable; + } + throw new Error(`getStepFilterVar failed for step ${step}`); + } + + it('Recent trail filter should be empty at current step 1', () => { + const store = getTrailStore(); + const trail = store.recent[0].resolve(); + + expect(getStepFilterVar(trail, 1).state.filters.length).toBe(0); + expect(trail.state.history.state.currentStep).toBe(1); + expect(trail.state.history.state.steps.length).toBe(3); + }); + + describe('And filter is added zone=a', () => { + let trail: DataTrail; + beforeEach(() => { + localStorage.clear(); + localStorage.setItem(RECENT_TRAILS_KEY, JSON.stringify([{ history, currentStep: 1 }])); + getTrailStore().load(); + const store = getTrailStore(); + trail = store.recent[0].resolve(); + const urlState = getUrlSyncManager().getUrlState(trail); + locationService.partial(urlState); + trail.activate(); + trail.state.history.activate(); + getFilterVar(trail).setState({ filters: [{ key: 'zone', operator: '=', value: 'a' }] }); + }); + + it('This should create step 3', () => { + expect(trail.state.history.state.steps.length).toBe(4); + expect(trail.state.history.state.currentStep).toBe(3); + }); + + it('Filter of trail should be zone=a', () => { + expect(getFilterVar(trail).state.filters[0].key).toBe('zone'); + expect(getFilterVar(trail).state.filters[0].value).toBe('a'); + }); + + it('Filter of step 3 should be zone=a', () => { + expect(getStepFilterVar(trail, 3).state.filters[0].key).toBe('zone'); + expect(getStepFilterVar(trail, 3).state.filters[0].value).toBe('a'); + }); + + it('Filter of step 1 should be empty', () => { + expect(getStepFilterVar(trail, 1).state.filters.length).toBe(0); + }); + + describe('When returning to step 1', () => { + beforeEach(() => { + trail.state.history.goBackToStep(1); + }); + + it('Filter of trail should be empty', () => { + expect(getFilterVar(trail).state.filters.length).toBe(0); + }); + }); + }); + + it('Time range `from` should be now-1h', () => { + const store = getTrailStore(); + const trail = store.recent[0].resolve(); + + expect(trail.state.$timeRange?.state.from).toBe('now-1h'); + }); + + describe('And time range is changed to now-15m to now', () => { + let trail: DataTrail; + beforeEach(() => { + localStorage.clear(); + localStorage.setItem(RECENT_TRAILS_KEY, JSON.stringify([{ history, currentStep: 1 }])); + getTrailStore().load(); + const store = getTrailStore(); + trail = store.recent[0].resolve(); + const urlState = getUrlSyncManager().getUrlState(trail); + locationService.partial(urlState); + trail.activate(); + trail.state.history.activate(); + trail.state.$timeRange?.setState({ from: 'now-15m' }); + }); + + it('This should create step 3', () => { + expect(trail.state.history.state.steps.length).toBe(4); + expect(trail.state.history.state.currentStep).toBe(3); + }); + + it('Time range `from` should be now-15m', () => { + expect(trail.state.$timeRange?.state.from).toBe('now-15m'); + }); + + it('Time range `from` of step 2 should be now-15m', () => { + expect(trail.state.history.state.steps[3].trailState.$timeRange?.state.from).toBe('now-15m'); + }); + + it('Time range `from` of step 1 should be now-1h', () => { + expect(trail.state.history.state.steps[1].trailState.$timeRange?.state.from).toBe('now-1h'); + }); + + describe('When returning to step 1', () => { + beforeEach(() => { + trail.state.history.goBackToStep(1); + }); + + it('Time range `from` should be now-1h', () => { + expect(trail.state.$timeRange?.state.from).toBe('now-1h'); + }); + }); + }); + it('should have no bookmarked trails', () => { const store = getTrailStore(); expect(store.bookmarks.length).toBe(0); @@ -309,7 +446,7 @@ describe('TrailStore', () => { ['metric', 'different_metric'], ['from', 'now-1y'], ['to', 'now-30m'], - ['var-ds', '1234'], + ['var-ds', 'different'], ['var-groupby', 'job'], ['var-filters', 'cluster|=|dev-eu-west-2'], ])(`new recent trails with a different '%p' value should insert new entry`, (key, differentValue) => {