From 41ac8d4cd56a9986b258e0c4600b7761d55c800c Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Wed, 20 Jun 2018 00:08:52 +0200 Subject: [PATCH] fix regressions after save modal changes of not storing time and variables per default Fix problem with adhoc variable filters not handled. Fix problem with saving variables and time per default when saving a dashboard as/first time. Fix updating dashboard model after save with saving time/variables enabled so that next time you save you won't get checkboxes for save time/variables unless any values changed. Tests validating correctness if time/variable values has changed. --- .../app/features/dashboard/dashboard_model.ts | 69 +++++-- public/app/features/dashboard/save_modal.ts | 52 ++--- .../dashboard/specs/dashboard_model.jest.ts | 192 +++++++++++++++--- .../dashboard/specs/save_modal.jest.ts | 165 +++++---------- 4 files changed, 286 insertions(+), 192 deletions(-) diff --git a/public/app/features/dashboard/dashboard_model.ts b/public/app/features/dashboard/dashboard_model.ts index a37e753bd89..976e4213920 100644 --- a/public/app/features/dashboard/dashboard_model.ts +++ b/public/app/features/dashboard/dashboard_model.ts @@ -22,10 +22,10 @@ export class DashboardModel { editable: any; graphTooltip: any; time: any; - originalTime: any; + private originalTime: any; timepicker: any; templating: any; - originalTemplating: any; + private originalTemplating: any; annotations: any; refresh: any; snapshot: any; @@ -50,6 +50,8 @@ export class DashboardModel { meta: true, panels: true, // needs special handling templating: true, // needs special handling + originalTime: true, + originalTemplating: true, }; constructor(data, meta?) { @@ -70,12 +72,8 @@ export class DashboardModel { this.editable = data.editable !== false; this.graphTooltip = data.graphTooltip || 0; this.time = data.time || { from: 'now-6h', to: 'now' }; - this.originalTime = _.cloneDeep(this.time); this.timepicker = data.timepicker || {}; this.templating = this.ensureListExist(data.templating); - this.originalTemplating = _.map(this.templating.list, variable => { - return { name: variable.name, current: _.clone(variable.current) }; - }); this.annotations = this.ensureListExist(data.annotations); this.refresh = data.refresh; this.snapshot = data.snapshot; @@ -85,6 +83,9 @@ export class DashboardModel { this.gnetId = data.gnetId || null; this.panels = _.map(data.panels || [], panelData => new PanelModel(panelData)); + this.resetOriginalVariables(); + this.resetOriginalTime(); + this.initMeta(meta); this.updateSchema(data); @@ -138,8 +139,8 @@ export class DashboardModel { // cleans meta data and other non persistent state getSaveModelClone(options?) { let defaults = _.defaults(options || {}, { - saveVariables: false, - saveTimerange: false, + saveVariables: true, + saveTimerange: true, }); // make clone @@ -153,15 +154,23 @@ export class DashboardModel { } // get variable save models - //console.log(this.templating.list); copy.templating = { list: _.map(this.templating.list, variable => (variable.getSaveModel ? variable.getSaveModel() : variable)), }; - if (!defaults.saveVariables && copy.templating.list.length === this.originalTemplating.length) { + if (!defaults.saveVariables) { for (let i = 0; i < copy.templating.list.length; i++) { - if (copy.templating.list[i].name === this.originalTemplating[i].name) { - copy.templating.list[i].current = this.originalTemplating[i].current; + let current = copy.templating.list[i]; + let original = _.find(this.originalTemplating, { name: current.name, type: current.type }); + + if (!original) { + continue; + } + + if (current.type === 'adhoc') { + copy.templating.list[i].filters = original.filters; + } else { + copy.templating.list[i].current = original.current; } } } @@ -785,4 +794,40 @@ export class DashboardModel { let migrator = new DashboardMigrator(this); migrator.updateSchema(old); } + + resetOriginalTime() { + this.originalTime = _.cloneDeep(this.time); + } + + hasTimeChanged() { + return !_.isEqual(this.time, this.originalTime); + } + + resetOriginalVariables() { + this.originalTemplating = _.map(this.templating.list, variable => { + return { + name: variable.name, + type: variable.type, + current: _.cloneDeep(variable.current), + filters: _.cloneDeep(variable.filters), + }; + }); + } + + hasVariableValuesChanged() { + if (this.templating.list.length !== this.originalTemplating.length) { + return false; + } + + const updated = _.map(this.templating.list, variable => { + return { + name: variable.name, + type: variable.type, + current: _.cloneDeep(variable.current), + filters: _.cloneDeep(variable.filters), + }; + }); + + return !_.isEqual(updated, this.originalTemplating); + } } diff --git a/public/app/features/dashboard/save_modal.ts b/public/app/features/dashboard/save_modal.ts index 525d2d1cbc3..3afcbab707c 100644 --- a/public/app/features/dashboard/save_modal.ts +++ b/public/app/features/dashboard/save_modal.ts @@ -1,5 +1,4 @@ import coreModule from 'app/core/core_module'; -import _ from 'lodash'; const template = `
@@ -70,7 +69,6 @@ export class SaveDashboardModalCtrl { message: string; saveVariables = false; saveTimerange = false; - templating: any; time: any; originalTime: any; current = []; @@ -87,40 +85,8 @@ export class SaveDashboardModalCtrl { this.message = ''; this.max = 64; this.isSaving = false; - this.templating = dashboardSrv.dash.templating.list; - - this.compareTemplating(); - this.compareTime(); - } - - compareTime() { - if (_.isEqual(this.dashboardSrv.dash.time, this.dashboardSrv.dash.originalTime)) { - this.timeChange = false; - } else { - this.timeChange = true; - } - } - - compareTemplating() { - //checks if variables has been added or removed, if so variables will be saved automatically - if (this.dashboardSrv.dash.originalTemplating.length !== this.dashboardSrv.dash.templating.list.length) { - return (this.variableValueChange = false); - } - - //checks if variable value has changed - if (this.dashboardSrv.dash.templating.list.length > 0) { - for (let i = 0; i < this.dashboardSrv.dash.templating.list.length; i++) { - if ( - this.dashboardSrv.dash.templating.list[i].current.text !== - this.dashboardSrv.dash.originalTemplating[i].current.text - ) { - return (this.variableValueChange = true); - } - } - return (this.variableValueChange = false); - } else { - return (this.variableValueChange = false); - } + this.timeChange = this.dashboardSrv.getCurrent().hasTimeChanged(); + this.variableValueChange = this.dashboardSrv.getCurrent().hasVariableValuesChanged(); } save() { @@ -139,7 +105,19 @@ export class SaveDashboardModalCtrl { this.isSaving = true; - return this.dashboardSrv.save(saveModel, options).then(this.dismiss); + return this.dashboardSrv.save(saveModel, options).then(this.postSave.bind(this, options)); + } + + postSave(options) { + if (options.saveVariables) { + this.dashboardSrv.getCurrent().resetOriginalVariables(); + } + + if (options.saveTimerange) { + this.dashboardSrv.getCurrent().resetOriginalTime(); + } + + this.dismiss(); } } diff --git a/public/app/features/dashboard/specs/dashboard_model.jest.ts b/public/app/features/dashboard/specs/dashboard_model.jest.ts index 3fb5b3c3b73..6ac642cd58e 100644 --- a/public/app/features/dashboard/specs/dashboard_model.jest.ts +++ b/public/app/features/dashboard/specs/dashboard_model.jest.ts @@ -435,8 +435,67 @@ describe('DashboardModel', function() { }); }); - describe('save variables and timeline', () => { - let model; + describe('Given model with time', () => { + let model: DashboardModel; + + beforeEach(() => { + model = new DashboardModel({ + time: { + from: 'now-6h', + to: 'now', + }, + }); + expect(model.hasTimeChanged()).toBeFalsy(); + model.time = { + from: 'now-3h', + to: 'now-1h', + }; + }); + + it('hasTimeChanged should be true', () => { + expect(model.hasTimeChanged()).toBeTruthy(); + }); + + it('getSaveModelClone should return original time when saveTimerange=false', () => { + let options = { saveTimerange: false }; + let saveModel = model.getSaveModelClone(options); + + expect(saveModel.time.from).toBe('now-6h'); + expect(saveModel.time.to).toBe('now'); + }); + + it('getSaveModelClone should return updated time when saveTimerange=true', () => { + let options = { saveTimerange: true }; + let 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(); + }); + + it('getSaveModelClone should return original time when saveTimerange=false', () => { + let options = { saveTimerange: false }; + let saveModel = model.getSaveModelClone(options); + + expect(saveModel.time.from).toBe('now-6h'); + expect(saveModel.time.to).toBe('now'); + }); + + it('getSaveModelClone should return updated time when saveTimerange=true', () => { + let options = { saveTimerange: true }; + let saveModel = model.getSaveModelClone(options); + + expect(saveModel.time.from).toBe('now-3h'); + expect(saveModel.time.to).toBe('now-1h'); + }); + }); + + describe('Given model with template variable of type query', () => { + let model: DashboardModel; beforeEach(() => { model = new DashboardModel({ @@ -444,6 +503,7 @@ describe('DashboardModel', function() { list: [ { name: 'Server', + type: 'query', current: { selected: true, text: 'server_001', @@ -452,45 +512,127 @@ describe('DashboardModel', function() { }, ], }, - time: { - from: 'now-6h', - to: 'now', - }, }); - model.templating.list[0] = { - name: 'Server', + expect(model.hasVariableValuesChanged()).toBeFalsy(); + }); + + it('hasVariableValuesChanged should be false when adding a template variable', () => { + model.templating.list.push({ + name: 'Server2', + type: 'query', current: { selected: true, text: 'server_002', value: 'server_002', }, - }; - model.time = { - from: 'now-3h', - to: 'now', - }; + }); + expect(model.hasVariableValuesChanged()).toBeFalsy(); }); - it('should not save variables and timeline', () => { - let options = { - saveVariables: false, - saveTimerange: false, - }; + it('hasVariableValuesChanged should be false when removing existing template variable', () => { + model.templating.list = []; + expect(model.hasVariableValuesChanged()).toBeFalsy(); + }); + + it('hasVariableValuesChanged should be true when changing value of template variable', () => { + model.templating.list[0].current.text = 'server_002'; + expect(model.hasVariableValuesChanged()).toBeTruthy(); + }); + + it('getSaveModelClone should return original variable when saveVariables=false', () => { + model.templating.list[0].current.text = 'server_002'; + + let options = { saveVariables: false }; let saveModel = model.getSaveModelClone(options); expect(saveModel.templating.list[0].current.text).toBe('server_001'); - expect(saveModel.time.from).toBe('now-6h'); }); - it('should save variables and timeline', () => { - let options = { - saveVariables: true, - saveTimerange: true, - }; + it('getSaveModelClone should return updated variable when saveVariables=true', () => { + model.templating.list[0].current.text = 'server_002'; + + let options = { saveVariables: true }; let saveModel = model.getSaveModelClone(options); expect(saveModel.templating.list[0].current.text).toBe('server_002'); - expect(saveModel.time.from).toBe('now-3h'); + }); + }); + + describe('Given model with template variable of type adhoc', () => { + let model: DashboardModel; + + beforeEach(() => { + model = new DashboardModel({ + templating: { + list: [ + { + name: 'Filter', + type: 'adhoc', + filters: [ + { + key: '@hostname', + operator: '=', + value: 'server 20', + }, + ], + }, + ], + }, + }); + expect(model.hasVariableValuesChanged()).toBeFalsy(); + }); + + it('hasVariableValuesChanged should be false when adding a template variable', () => { + model.templating.list.push({ + name: 'Filter', + type: 'adhoc', + filters: [ + { + key: '@hostname', + operator: '=', + value: 'server 1', + }, + ], + }); + expect(model.hasVariableValuesChanged()).toBeFalsy(); + }); + + it('hasVariableValuesChanged should be false when removing existing template variable', () => { + model.templating.list = []; + expect(model.hasVariableValuesChanged()).toBeFalsy(); + }); + + it('hasVariableValuesChanged should be true when changing value of filter', () => { + model.templating.list[0].filters[0].value = 'server 1'; + expect(model.hasVariableValuesChanged()).toBeTruthy(); + }); + + it('hasVariableValuesChanged 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(); + }); + + it('getSaveModelClone should return original variable when saveVariables=false', () => { + model.templating.list[0].filters[0].value = 'server 1'; + + let options = { saveVariables: false }; + let saveModel = model.getSaveModelClone(options); + + expect(saveModel.templating.list[0].filters[0].value).toBe('server 20'); + }); + + it('getSaveModelClone should return updated variable when saveVariables=true', () => { + model.templating.list[0].filters[0].value = 'server 1'; + + let options = { saveVariables: true }; + let saveModel = model.getSaveModelClone(options); + + expect(saveModel.templating.list[0].filters[0].value).toBe('server 1'); }); }); }); diff --git a/public/app/features/dashboard/specs/save_modal.jest.ts b/public/app/features/dashboard/specs/save_modal.jest.ts index ebae6f0f5bd..669ae43a0ff 100644 --- a/public/app/features/dashboard/specs/save_modal.jest.ts +++ b/public/app/features/dashboard/specs/save_modal.jest.ts @@ -1,128 +1,57 @@ import { SaveDashboardModalCtrl } from '../save_modal'; -jest.mock('app/core/services/context_srv', () => ({})); +const setup = (timeChanged, variableValuesChanged, cb) => { + const dash = { + hasTimeChanged: jest.fn().mockReturnValue(timeChanged), + hasVariableValuesChanged: jest.fn().mockReturnValue(variableValuesChanged), + resetOriginalTime: jest.fn(), + resetOriginalVariables: jest.fn(), + getSaveModelClone: jest.fn().mockReturnValue({}), + }; + const dashboardSrvMock = { + getCurrent: jest.fn().mockReturnValue(dash), + save: jest.fn().mockReturnValue(Promise.resolve()), + }; + const ctrl = new SaveDashboardModalCtrl(dashboardSrvMock); + ctrl.saveForm = { + $valid: true, + }; + ctrl.dismiss = () => Promise.resolve(); + cb(dash, ctrl, dashboardSrvMock); +}; describe('SaveDashboardModal', () => { - describe('save modal checkboxes', () => { - it('should show checkboxes', () => { - let fakeDashboardSrv = { - dash: { - templating: { - list: [ - { - current: { - selected: true, - tags: Array(0), - text: 'server_001', - value: 'server_001', - }, - name: 'Server', - }, - ], - }, - originalTemplating: [ - { - current: { - selected: true, - text: 'server_002', - value: 'server_002', - }, - name: 'Server', - }, - ], - time: { - from: 'now-3h', - to: 'now', - }, - originalTime: { - from: 'now-6h', - to: 'now', - }, - }, - }; - let modal = new SaveDashboardModalCtrl(fakeDashboardSrv); - - expect(modal.timeChange).toBe(true); - expect(modal.variableValueChange).toBe(true); + describe('Given time and template variable values have not changed', () => { + setup(false, false, (dash, ctrl: SaveDashboardModalCtrl) => { + it('When creating ctrl should set time and template variable values changed', () => { + expect(ctrl.timeChange).toBeFalsy(); + expect(ctrl.variableValueChange).toBeFalsy(); + }); }); + }); - it('should hide checkboxes', () => { - let fakeDashboardSrv = { - dash: { - templating: { - list: [ - { - current: { - selected: true, - text: 'server_002', - value: 'server_002', - }, - name: 'Server', - }, - ], - }, - originalTemplating: [ - { - current: { - selected: true, - text: 'server_002', - value: 'server_002', - }, - name: 'Server', - }, - ], - time: { - from: 'now-3h', - to: 'now', - }, - originalTime: { - from: 'now-3h', - to: 'now', - }, - }, - }; - let modal = new SaveDashboardModalCtrl(fakeDashboardSrv); - expect(modal.timeChange).toBe(false); - expect(modal.variableValueChange).toBe(false); - }); + describe('Given time and template variable values have changed', () => { + setup(true, true, (dash, ctrl: SaveDashboardModalCtrl) => { + it('When creating ctrl should set time and template variable values changed', () => { + expect(ctrl.timeChange).toBeTruthy(); + expect(ctrl.variableValueChange).toBeTruthy(); + }); - it('should hide variable checkboxes', () => { - let fakeDashboardSrv = { - dash: { - templating: { - list: [ - { - current: { - selected: true, - text: 'server_002', - value: 'server_002', - }, - name: 'Server', - }, - { - current: { - selected: true, - text: 'web_002', - value: 'web_002', - }, - name: 'Web', - }, - ], - }, - originalTemplating: [ - { - current: { - selected: true, - text: 'server_002', - value: 'server_002', - }, - name: 'Server', - }, - ], - }, - }; - let modal = new SaveDashboardModalCtrl(fakeDashboardSrv); - expect(modal.variableValueChange).toBe(false); + it('When save time and variable value changes disabled and saving should reset original time and template variable values', async () => { + ctrl.saveTimerange = false; + ctrl.saveVariables = false; + await ctrl.save(); + expect(dash.resetOriginalTime).toHaveBeenCalledTimes(0); + expect(dash.resetOriginalVariables).toHaveBeenCalledTimes(0); + }); + + it('When save time and variable value changes enabled and saving should reset original time and template variable values', async () => { + ctrl.saveTimerange = true; + ctrl.saveVariables = true; + await ctrl.save(); + expect(dash.resetOriginalTime).toHaveBeenCalledTimes(1); + expect(dash.resetOriginalVariables).toHaveBeenCalledTimes(1); + }); }); }); });