fix: Row state is now ignored when looking for dashboard changes (#11608)

* fix: Row state is now ignored when looking for dashboawrd changes, had to fix bug in expand logic to make fix work. Also moved the ChangeTracker to it's own file and moved tests to jest, fixes #11208

* removed commented out log calls

public/app/features/dashboard/change_tracker.ts

@@ -0,0 +1,186 @@
+import angular from 'angular';
+import _ from 'lodash';
+import { DashboardModel } from './dashboard_model';
+
+export class ChangeTracker {
+  current: any;
+  originalPath: any;
+  scope: any;
+  original: any;
+  next: any;
+  $window: any;
+
+  /** @ngInject */
+  constructor(
+    dashboard,
+    scope,
+    originalCopyDelay,
+    private $location,
+    $window,
+    private $timeout,
+    private contextSrv,
+    private $rootScope
+  ) {
+    this.$location = $location;
+    this.$window = $window;
+
+    this.current = dashboard;
+    this.originalPath = $location.path();
+    this.scope = scope;
+
+    // register events
+    scope.onAppEvent('dashboard-saved', () => {
+      this.original = this.current.getSaveModelClone();
+      this.originalPath = $location.path();
+    });
+
+    $window.onbeforeunload = () => {
+      if (this.ignoreChanges()) {
+        return undefined;
+      }
+      if (this.hasChanges()) {
+        return 'There are unsaved changes to this dashboard';
+      }
+      return undefined;
+    };
+
+    scope.$on('$locationChangeStart', (event, next) => {
+      // check if we should look for changes
+      if (this.originalPath === $location.path()) {
+        return true;
+      }
+      if (this.ignoreChanges()) {
+        return true;
+      }
+
+      if (this.hasChanges()) {
+        event.preventDefault();
+        this.next = next;
+
+        this.$timeout(() => {
+          this.open_modal();
+        });
+      }
+      return false;
+    });
+
+    if (originalCopyDelay) {
+      this.$timeout(() => {
+        // wait for different services to patch the dashboard (missing properties)
+        this.original = dashboard.getSaveModelClone();
+      }, originalCopyDelay);
+    } else {
+      this.original = dashboard.getSaveModelClone();
+    }
+  }
+
+  // for some dashboards and users
+  // changes should be ignored
+  ignoreChanges() {
+    if (!this.original) {
+      return true;
+    }
+    if (!this.contextSrv.isEditor) {
+      return true;
+    }
+    if (!this.current || !this.current.meta) {
+      return true;
+    }
+
+    var meta = this.current.meta;
+    return !meta.canSave || meta.fromScript || meta.fromFile;
+  }
+
+  // remove stuff that should not count in diff
+  cleanDashboardFromIgnoredChanges(dashData) {
+    // need to new up the domain model class to get access to expand / collapse row logic
+    let model = new DashboardModel(dashData);
+
+    // Expand all rows before making comparison. This is required because row expand / collapse
+    // change order of panel array and panel positions.
+    model.expandRows();
+
+    let dash = model.getSaveModelClone();
+
+    // ignore time and refresh
+    dash.time = 0;
+    dash.refresh = 0;
+    dash.schemaVersion = 0;
+
+    // ignore iteration property
+    delete dash.iteration;
+
+    dash.panels = _.filter(dash.panels, panel => {
+      if (panel.repeatPanelId) {
+        return false;
+      }
+
+      // remove scopedVars
+      panel.scopedVars = null;
+
+      // ignore panel legend sort
+      if (panel.legend) {
+        delete panel.legend.sort;
+        delete panel.legend.sortDesc;
+      }
+
+      return true;
+    });
+
+    // ignore template variable values
+    _.each(dash.templating.list, function(value) {
+      value.current = null;
+      value.options = null;
+      value.filters = null;
+    });
+
+    return dash;
+  }
+
+  hasChanges() {
+    let current = this.cleanDashboardFromIgnoredChanges(this.current.getSaveModelClone());
+    let original = this.cleanDashboardFromIgnoredChanges(this.original);
+
+    var currentTimepicker = _.find(current.nav, { type: 'timepicker' });
+    var originalTimepicker = _.find(original.nav, { type: 'timepicker' });
+
+    if (currentTimepicker && originalTimepicker) {
+      currentTimepicker.now = originalTimepicker.now;
+    }
+
+    var currentJson = angular.toJson(current, true);
+    var originalJson = angular.toJson(original, true);
+
+    return currentJson !== originalJson;
+  }
+
+  discardChanges() {
+    this.original = null;
+    this.gotoNext();
+  }
+
+  open_modal() {
+    this.$rootScope.appEvent('show-modal', {
+      templateHtml: '<unsaved-changes-modal dismiss="dismiss()"></unsaved-changes-modal>',
+      modalClass: 'modal--narrow confirm-modal',
+    });
+  }
+
+  saveChanges() {
+    var self = this;
+    var cancel = this.$rootScope.$on('dashboard-saved', () => {
+      cancel();
+      this.$timeout(() => {
+        self.gotoNext();
+      });
+    });
+
+    this.$rootScope.appEvent('save-dashboard');
+  }
+
+  gotoNext() {
+    var baseLen = this.$location.absUrl().length - this.$location.url().length;
+    var nextUrl = this.next.substring(baseLen);
+    this.$location.url(nextUrl);
+  }
+}

public/app/features/dashboard/dashboard_model.ts

@@ -649,6 +649,7 @@ export class DashboardModel {
 
         for (let panel of row.panels) {
           // make sure y is adjusted (in case row moved while collapsed)
+          // console.log('yDiff', yDiff);
           panel.gridPos.y -= yDiff;
           // insert after row
           this.panels.splice(insertPos, 0, new PanelModel(panel));
@@ -657,7 +658,7 @@ export class DashboardModel {
           yMax = Math.max(yMax, panel.gridPos.y + panel.gridPos.h);
         }
 
-        const pushDownAmount = yMax - row.gridPos.y;
+        const pushDownAmount = yMax - row.gridPos.y - 1;
 
         // push panels below down
         for (let panelIndex = insertPos; panelIndex < this.panels.length; panelIndex++) {

public/app/features/dashboard/specs/change_tracker.jest.ts

@@ -0,0 +1,99 @@
+import { ChangeTracker } from 'app/features/dashboard/change_tracker';
+import { contextSrv } from 'app/core/services/context_srv';
+import { DashboardModel } from '../dashboard_model';
+import { PanelModel } from '../panel_model';
+
+jest.mock('app/core/services/context_srv', () => ({
+  contextSrv: {
+    user: { orgId: 1 },
+  },
+}));
+
+describe('ChangeTracker', () => {
+  let rootScope;
+  let location;
+  let timeout;
+  let tracker: ChangeTracker;
+  let dash;
+  let scope;
+
+  beforeEach(() => {
+    dash = new DashboardModel({
+      refresh: false,
+      panels: [
+        {
+          id: 1,
+          type: 'graph',
+          gridPos: { x: 0, y: 0, w: 24, h: 6 },
+          legend: { sortDesc: false },
+        },
+        {
+          id: 2,
+          type: 'row',
+          gridPos: { x: 0, y: 6, w: 24, h: 2 },
+          collapsed: true,
+          panels: [
+            { id: 3, type: 'graph', gridPos: { x: 0, y: 6, w: 12, h: 2 } },
+            { id: 4, type: 'graph', gridPos: { x: 12, y: 6, w: 12, h: 2 } },
+          ],
+        },
+        { id: 5, type: 'row', gridPos: { x: 0, y: 6, w: 1, h: 1 } },
+      ],
+    });
+
+    scope = {
+      appEvent: jest.fn(),
+      onAppEvent: jest.fn(),
+      $on: jest.fn(),
+    };
+
+    rootScope = {
+      appEvent: jest.fn(),
+      onAppEvent: jest.fn(),
+      $on: jest.fn(),
+    };
+
+    location = {
+      path: jest.fn(),
+    };
+
+    tracker = new ChangeTracker(dash, scope, undefined, location, window, timeout, contextSrv, rootScope);
+  });
+
+  it('No changes should not have changes', () => {
+    expect(tracker.hasChanges()).toBe(false);
+  });
+
+  it('Simple change should be registered', () => {
+    dash.title = 'google';
+    expect(tracker.hasChanges()).toBe(true);
+  });
+
+  it('Should ignore a lot of changes', () => {
+    dash.time = { from: '1h' };
+    dash.refresh = true;
+    dash.schemaVersion = 10;
+    expect(tracker.hasChanges()).toBe(false);
+  });
+
+  it('Should ignore .iteration changes', () => {
+    dash.iteration = new Date().getTime() + 1;
+    expect(tracker.hasChanges()).toBe(false);
+  });
+
+  it('Should ignore row collapse change', () => {
+    dash.toggleRow(dash.panels[1]);
+    expect(tracker.hasChanges()).toBe(false);
+  });
+
+  it('Should ignore panel legend changes', () => {
+    dash.panels[0].legend.sortDesc = true;
+    dash.panels[0].legend.sort = 'avg';
+    expect(tracker.hasChanges()).toBe(false);
+  });
+
+  it('Should ignore panel repeats', () => {
+    dash.panels.push(new PanelModel({ repeatPanelId: 10 }));
+    expect(tracker.hasChanges()).toBe(false);
+  });
+});

public/app/features/dashboard/specs/dashboard_model.jest.ts

@@ -374,14 +374,14 @@ describe('DashboardModel', function() {
           {
             id: 2,
             type: 'row',
-            gridPos: { x: 0, y: 6, w: 24, h: 2 },
+            gridPos: { x: 0, y: 6, w: 24, h: 1 },
             collapsed: true,
             panels: [
-              { id: 3, type: 'graph', gridPos: { x: 0, y: 2, w: 12, h: 2 } },
-              { id: 4, type: 'graph', gridPos: { x: 12, y: 2, w: 12, h: 2 } },
+              { id: 3, type: 'graph', gridPos: { x: 0, y: 7, w: 12, h: 2 } },
+              { id: 4, type: 'graph', gridPos: { x: 12, y: 7, w: 12, h: 2 } },
             ],
           },
-          { id: 5, type: 'row', gridPos: { x: 0, y: 6, w: 1, h: 1 } },
+          { id: 5, type: 'row', gridPos: { x: 0, y: 7, w: 1, h: 1 } },
         ],
       });
       dashboard.toggleRow(dashboard.panels[1]);
@@ -399,16 +399,16 @@ describe('DashboardModel', function() {
     it('should position them below row', function() {
       expect(dashboard.panels[2].gridPos).toMatchObject({
         x: 0,
-        y: 8,
+        y: 7,
         w: 12,
         h: 2,
       });
     });
 
-    it('should move panels below down', function() {
+    it.only('should move panels below down', function() {
       expect(dashboard.panels[4].gridPos).toMatchObject({
         x: 0,
-        y: 10,
+        y: 9,
         w: 1,
         h: 1,
       });

public/app/features/dashboard/specs/unsaved_changes_srv_specs.ts

@@ -1,95 +0,0 @@
-import { describe, beforeEach, it, expect, sinon, angularMocks } from 'test/lib/common';
-import { Tracker } from 'app/features/dashboard/unsaved_changes_srv';
-import 'app/features/dashboard/dashboard_srv';
-import { contextSrv } from 'app/core/core';
-
-describe('unsavedChangesSrv', function() {
-  var _dashboardSrv;
-  var _contextSrvStub = { isEditor: true };
-  var _rootScope;
-  var _location;
-  var _timeout;
-  var _window;
-  var tracker;
-  var dash;
-  var scope;
-
-  beforeEach(angularMocks.module('grafana.core'));
-  beforeEach(angularMocks.module('grafana.services'));
-  beforeEach(
-    angularMocks.module(function($provide) {
-      $provide.value('contextSrv', _contextSrvStub);
-      $provide.value('$window', {});
-    })
-  );
-
-  beforeEach(
-    angularMocks.inject(function($location, $rootScope, dashboardSrv, $timeout, $window) {
-      _dashboardSrv = dashboardSrv;
-      _rootScope = $rootScope;
-      _location = $location;
-      _timeout = $timeout;
-      _window = $window;
-    })
-  );
-
-  beforeEach(function() {
-    dash = _dashboardSrv.create({
-      refresh: false,
-      panels: [{ test: 'asd', legend: {} }],
-      rows: [
-        {
-          panels: [{ test: 'asd', legend: {} }],
-        },
-      ],
-    });
-    scope = _rootScope.$new();
-    scope.appEvent = sinon.spy();
-    scope.onAppEvent = sinon.spy();
-
-    tracker = new Tracker(dash, scope, undefined, _location, _window, _timeout, contextSrv, _rootScope);
-  });
-
-  it('No changes should not have changes', function() {
-    expect(tracker.hasChanges()).to.be(false);
-  });
-
-  it('Simple change should be registered', function() {
-    dash.property = 'google';
-    expect(tracker.hasChanges()).to.be(true);
-  });
-
-  it('Should ignore a lot of changes', function() {
-    dash.time = { from: '1h' };
-    dash.refresh = true;
-    dash.schemaVersion = 10;
-    expect(tracker.hasChanges()).to.be(false);
-  });
-
-  it('Should ignore .iteration changes', () => {
-    dash.iteration = new Date().getTime() + 1;
-    expect(tracker.hasChanges()).to.be(false);
-  });
-
-  it.skip('Should ignore row collapse change', function() {
-    dash.rows[0].collapse = true;
-    expect(tracker.hasChanges()).to.be(false);
-  });
-
-  it('Should ignore panel legend changes', function() {
-    dash.panels[0].legend.sortDesc = true;
-    dash.panels[0].legend.sort = 'avg';
-    expect(tracker.hasChanges()).to.be(false);
-  });
-
-  it.skip('Should ignore panel repeats', function() {
-    dash.rows[0].panels.push({ repeatPanelId: 10 });
-    expect(tracker.hasChanges()).to.be(false);
-  });
-
-  it.skip('Should ignore row repeats', function() {
-    dash.addEmptyRow();
-    dash.rows[1].repeatRowId = 10;
-    expect(tracker.hasChanges()).to.be(false);
-  });
-});

public/app/features/dashboard/unsaved_changes_srv.ts

@@ -1,217 +1,10 @@
 import angular from 'angular';
-import _ from 'lodash';
-
-export class Tracker {
-  current: any;
-  originalPath: any;
-  scope: any;
-  original: any;
-  next: any;
-  $window: any;
-
-  /** @ngInject */
-  constructor(
-    dashboard,
-    scope,
-    originalCopyDelay,
-    private $location,
-    $window,
-    private $timeout,
-    private contextSrv,
-    private $rootScope
-  ) {
-    this.$location = $location;
-    this.$window = $window;
-
-    this.current = dashboard;
-    this.originalPath = $location.path();
-    this.scope = scope;
-
-    // register events
-    scope.onAppEvent('dashboard-saved', () => {
-      this.original = this.current.getSaveModelClone();
-      this.originalPath = $location.path();
-    });
-
-    $window.onbeforeunload = () => {
-      if (this.ignoreChanges()) {
-        return undefined;
-      }
-      if (this.hasChanges()) {
-        return 'There are unsaved changes to this dashboard';
-      }
-      return undefined;
-    };
-
-    scope.$on('$locationChangeStart', (event, next) => {
-      // check if we should look for changes
-      if (this.originalPath === $location.path()) {
-        return true;
-      }
-      if (this.ignoreChanges()) {
-        return true;
-      }
-
-      if (this.hasChanges()) {
-        event.preventDefault();
-        this.next = next;
-
-        this.$timeout(() => {
-          this.open_modal();
-        });
-      }
-      return false;
-    });
-
-    if (originalCopyDelay) {
-      this.$timeout(() => {
-        // wait for different services to patch the dashboard (missing properties)
-        this.original = dashboard.getSaveModelClone();
-      }, originalCopyDelay);
-    } else {
-      this.original = dashboard.getSaveModelClone();
-    }
-  }
-
-  // for some dashboards and users
-  // changes should be ignored
-  ignoreChanges() {
-    if (!this.original) {
-      return true;
-    }
-    if (!this.contextSrv.isEditor) {
-      return true;
-    }
-    if (!this.current || !this.current.meta) {
-      return true;
-    }
-
-    var meta = this.current.meta;
-    return !meta.canSave || meta.fromScript || meta.fromFile;
-  }
-
-  // remove stuff that should not count in diff
-  cleanDashboardFromIgnoredChanges(dash) {
-    // ignore time and refresh
-    dash.time = 0;
-    dash.refresh = 0;
-    dash.schemaVersion = 0;
-
-    // ignore iteration property
-    delete dash.iteration;
-
-    // filter row and panels properties that should be ignored
-    dash.rows = _.filter(dash.rows, function(row) {
-      if (row.repeatRowId) {
-        return false;
-      }
-
-      row.panels = _.filter(row.panels, function(panel) {
-        if (panel.repeatPanelId) {
-          return false;
-        }
-
-        // remove scopedVars
-        panel.scopedVars = null;
-
-        // ignore span changes
-        panel.span = null;
-
-        // ignore panel legend sort
-        if (panel.legend) {
-          delete panel.legend.sort;
-          delete panel.legend.sortDesc;
-        }
-
-        return true;
-      });
-
-      // ignore collapse state
-      row.collapse = false;
-      return true;
-    });
-
-    dash.panels = _.filter(dash.panels, panel => {
-      if (panel.repeatPanelId) {
-        return false;
-      }
-
-      // remove scopedVars
-      panel.scopedVars = null;
-
-      // ignore panel legend sort
-      if (panel.legend) {
-        delete panel.legend.sort;
-        delete panel.legend.sortDesc;
-      }
-
-      return true;
-    });
-
-    // ignore template variable values
-    _.each(dash.templating.list, function(value) {
-      value.current = null;
-      value.options = null;
-      value.filters = null;
-    });
-  }
-
-  hasChanges() {
-    var current = this.current.getSaveModelClone();
-    var original = this.original;
-
-    this.cleanDashboardFromIgnoredChanges(current);
-    this.cleanDashboardFromIgnoredChanges(original);
-
-    var currentTimepicker = _.find(current.nav, { type: 'timepicker' });
-    var originalTimepicker = _.find(original.nav, { type: 'timepicker' });
-
-    if (currentTimepicker && originalTimepicker) {
-      currentTimepicker.now = originalTimepicker.now;
-    }
-
-    var currentJson = angular.toJson(current);
-    var originalJson = angular.toJson(original);
-
-    return currentJson !== originalJson;
-  }
-
-  discardChanges() {
-    this.original = null;
-    this.gotoNext();
-  }
-
-  open_modal() {
-    this.$rootScope.appEvent('show-modal', {
-      templateHtml: '<unsaved-changes-modal dismiss="dismiss()"></unsaved-changes-modal>',
-      modalClass: 'modal--narrow confirm-modal',
-    });
-  }
-
-  saveChanges() {
-    var self = this;
-    var cancel = this.$rootScope.$on('dashboard-saved', () => {
-      cancel();
-      this.$timeout(() => {
-        self.gotoNext();
-      });
-    });
-
-    this.$rootScope.appEvent('save-dashboard');
-  }
-
-  gotoNext() {
-    var baseLen = this.$location.absUrl().length - this.$location.url().length;
-    var nextUrl = this.next.substring(baseLen);
-    this.$location.url(nextUrl);
-  }
-}
+import { ChangeTracker } from './change_tracker';
 
 /** @ngInject */
 export function unsavedChangesSrv($rootScope, $q, $location, $timeout, contextSrv, dashboardSrv, $window) {
-  this.Tracker = Tracker;
   this.init = function(dashboard, scope) {
-    this.tracker = new Tracker(dashboard, scope, 1000, $location, $window, $timeout, contextSrv, $rootScope);
+    this.tracker = new ChangeTracker(dashboard, scope, 1000, $location, $window, $timeout, contextSrv, $rootScope);
     return this.tracker;
   };
 }

public/app/features/templating/specs/query_variable.jest.ts

@@ -91,7 +91,6 @@ describe('QueryVariable', () => {
 
       it('should return in same order', () => {
         var i = 0;
-        console.log(result);
         expect(result.length).toBe(11);
         expect(result[i++].text).toBe('');
         expect(result[i++].text).toBe('0');