Merge pull request #12350 from grafana/12240_save_modal_regression

Fix regressions after save modal changes

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);
+  }
 }

public/app/features/dashboard/save_modal.ts

@@ -1,5 +1,4 @@
 import coreModule from 'app/core/core_module';
-import _ from 'lodash';
 
 const template = `
 <div class="modal-body">
@@ -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();
   }
 }
 

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');
     });
   });
 });

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('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('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 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);
+      });
     });
   });
 });