From 85121e55c9ee2bf3401416b52ff6e4aeb0a70d67 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Torkel=20=C3=96degaard?= <torkel@grafana.com>
Date: Tue, 17 Apr 2018 09:41:07 +0200
Subject: [PATCH] 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
---
 .../app/features/dashboard/change_tracker.ts  | 186 +++++++++++++++
 .../app/features/dashboard/dashboard_model.ts |   3 +-
 .../dashboard/specs/change_tracker.jest.ts    |  99 ++++++++
 .../dashboard/specs/dashboard_model.jest.ts   |  14 +-
 .../specs/unsaved_changes_srv_specs.ts        |  95 --------
 .../features/dashboard/unsaved_changes_srv.ts | 211 +-----------------
 .../templating/specs/query_variable.jest.ts   |   1 -
 7 files changed, 296 insertions(+), 313 deletions(-)
 create mode 100644 public/app/features/dashboard/change_tracker.ts
 create mode 100644 public/app/features/dashboard/specs/change_tracker.jest.ts
 delete mode 100644 public/app/features/dashboard/specs/unsaved_changes_srv_specs.ts

diff --git a/public/app/features/dashboard/change_tracker.ts b/public/app/features/dashboard/change_tracker.ts
new file mode 100644
index 00000000000..745b76ce347
--- /dev/null
+++ b/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);
+  }
+}
diff --git a/public/app/features/dashboard/dashboard_model.ts b/public/app/features/dashboard/dashboard_model.ts
index 9130cb7e806..8a300a80341 100644
--- a/public/app/features/dashboard/dashboard_model.ts
+++ b/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++) {
diff --git a/public/app/features/dashboard/specs/change_tracker.jest.ts b/public/app/features/dashboard/specs/change_tracker.jest.ts
new file mode 100644
index 00000000000..5ec84aadbd0
--- /dev/null
+++ b/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);
+  });
+});
diff --git a/public/app/features/dashboard/specs/dashboard_model.jest.ts b/public/app/features/dashboard/specs/dashboard_model.jest.ts
index 99fe727c49d..feede679018 100644
--- a/public/app/features/dashboard/specs/dashboard_model.jest.ts
+++ b/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,
       });
diff --git a/public/app/features/dashboard/specs/unsaved_changes_srv_specs.ts b/public/app/features/dashboard/specs/unsaved_changes_srv_specs.ts
deleted file mode 100644
index 8bd639de681..00000000000
--- a/public/app/features/dashboard/specs/unsaved_changes_srv_specs.ts
+++ /dev/null
@@ -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);
-  });
-});
diff --git a/public/app/features/dashboard/unsaved_changes_srv.ts b/public/app/features/dashboard/unsaved_changes_srv.ts
index d4c12b8bcd6..0406e6a55d7 100644
--- a/public/app/features/dashboard/unsaved_changes_srv.ts
+++ b/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;
   };
 }
diff --git a/public/app/features/templating/specs/query_variable.jest.ts b/public/app/features/templating/specs/query_variable.jest.ts
index ce753a4b205..39c51874586 100644
--- a/public/app/features/templating/specs/query_variable.jest.ts
+++ b/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');