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.

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,
+      saveVariables: true,
-      saveTimerange: false,
+      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) {
+        let current = copy.templating.list[i];
-          copy.templating.list[i].current = this.originalTemplating[i].current;
+        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.timeChange = this.dashboardSrv.getCurrent().hasTimeChanged();
-
+    this.variableValueChange = this.dashboardSrv.getCurrent().hasVariableValuesChanged();
-    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);
-    }
   }
 
   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', () => {
+  describe('Given model with time', () => {
-    let model;
+    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] = {
+      expect(model.hasVariableValuesChanged()).toBeFalsy();
-        name: 'Server',
+    });
+
+    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 = {
+      expect(model.hasVariableValuesChanged()).toBeFalsy();
-        from: 'now-3h',
-        to: 'now',
-      };
     });
 
-    it('should not save variables and timeline', () => {
+    it('hasVariableValuesChanged should be false when removing existing template variable', () => {
-      let options = {
+      model.templating.list = [];
-        saveVariables: false,
+      expect(model.hasVariableValuesChanged()).toBeFalsy();
-        saveTimerange: false,
+    });
-      };
+
+    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', () => {
+    it('getSaveModelClone should return updated variable when saveVariables=true', () => {
-      let options = {
+      model.templating.list[0].current.text = 'server_002';
-        saveVariables: true,
+
-        saveTimerange: true,
+      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', () => {
+  describe('Given time and template variable values have not changed', () => {
-    it('should show checkboxes', () => {
+    setup(false, false, (dash, ctrl: SaveDashboardModalCtrl) => {
-      let fakeDashboardSrv = {
+      it('When creating ctrl should set time and template variable values changed', () => {
-        dash: {
+        expect(ctrl.timeChange).toBeFalsy();
-          templating: {
+        expect(ctrl.variableValueChange).toBeFalsy();
-            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);
     });
+  });
 
-    it('should hide checkboxes', () => {
+  describe('Given time and template variable values have changed', () => {
-      let fakeDashboardSrv = {
+    setup(true, true, (dash, ctrl: SaveDashboardModalCtrl) => {
-        dash: {
+      it('When creating ctrl should set time and template variable values changed', () => {
-          templating: {
+        expect(ctrl.timeChange).toBeTruthy();
-            list: [
+        expect(ctrl.variableValueChange).toBeTruthy();
-              {
+      });
-                current: {
+
-                  selected: true,
+      it('When save time and variable value changes disabled and saving should reset original time and template variable values', async () => {
-                  text: 'server_002',
+        ctrl.saveTimerange = false;
-                  value: 'server_002',
+        ctrl.saveVariables = false;
-                },
+        await ctrl.save();
-                name: 'Server',
+        expect(dash.resetOriginalTime).toHaveBeenCalledTimes(0);
-              },
+        expect(dash.resetOriginalVariables).toHaveBeenCalledTimes(0);
-            ],
+      });
-          },
-          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);
-    });
 
-    it('should hide variable checkboxes', () => {
+      it('When save time and variable value changes enabled and saving should reset original time and template variable values', async () => {
-      let fakeDashboardSrv = {
+        ctrl.saveTimerange = true;
-        dash: {
+        ctrl.saveVariables = true;
-          templating: {
+        await ctrl.save();
-            list: [
+        expect(dash.resetOriginalTime).toHaveBeenCalledTimes(1);
-              {
+        expect(dash.resetOriginalVariables).toHaveBeenCalledTimes(1);
-                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);
     });
   });
 });