Dashboard: Unsaved changes warning shown for anyone that can save (#33145)

* Dashboard: Unsaved changes warning shown for anyone that can save

* Refactor: changes the order in function

public/app/core/services/context_srv.ts

@@ -110,9 +110,16 @@ export class ContextSrv {
   }
 }
 
-const contextSrv = new ContextSrv();
+let contextSrv = new ContextSrv();
 export { contextSrv };
 
+export const setContextSrv = (override: ContextSrv) => {
+  if (process.env.NODE_ENV !== 'test') {
+    throw new Error('contextSrv can be only overriden in test environment');
+  }
+  contextSrv = override;
+};
+
 coreModule.factory('contextSrv', () => {
   return contextSrv;
 });

public/app/features/dashboard/services/ChangeTracker.test.ts

@@ -1,20 +1,10 @@
 import { ChangeTracker } from './ChangeTracker';
 import { DashboardModel } from '../state/DashboardModel';
 import { PanelModel } from '../state/PanelModel';
+import { setContextSrv } from '../../../core/services/context_srv';
 
-jest.mock('app/core/services/context_srv', () => ({
+function getDefaultDashboardModel(): DashboardModel {
-  contextSrv: {
+  return new DashboardModel({
-    user: { orgId: 1 },
-  },
-}));
-
-describe('ChangeTracker', () => {
-  let tracker: ChangeTracker;
-  let dash: any;
-  let original: any;
-
-  beforeEach(() => {
-    dash = new DashboardModel({
     refresh: false,
     panels: [
       {
@@ -36,21 +26,32 @@ describe('ChangeTracker', () => {
       { id: 5, type: 'row', gridPos: { x: 0, y: 6, w: 1, h: 1 } },
     ],
   });
+}
 
-    tracker = new ChangeTracker();
+function getTestContext() {
-    original = dash.getSaveModelClone();
+  const contextSrv: any = { isSignedIn: true, isEditor: true };
-  });
+  setContextSrv(contextSrv);
+  const dash: any = getDefaultDashboardModel();
+  const tracker = new ChangeTracker();
+  const original: any = dash.getSaveModelClone();
 
+  return { dash, tracker, original, contextSrv };
+}
+
+describe('ChangeTracker', () => {
   it('No changes should not have changes', () => {
+    const { tracker, original, dash } = getTestContext();
     expect(tracker.hasChanges(dash, original)).toBe(false);
   });
 
   it('Simple change should be registered', () => {
+    const { tracker, original, dash } = getTestContext();
     dash.title = 'google';
     expect(tracker.hasChanges(dash, original)).toBe(true);
   });
 
   it('Should ignore a lot of changes', () => {
+    const { tracker, original, dash } = getTestContext();
     dash.time = { from: '1h' };
     dash.refresh = true;
     dash.schemaVersion = 10;
@@ -58,23 +59,104 @@ describe('ChangeTracker', () => {
   });
 
   it('Should ignore .iteration changes', () => {
+    const { tracker, original, dash } = getTestContext();
     dash.iteration = new Date().getTime() + 1;
     expect(tracker.hasChanges(dash, original)).toBe(false);
   });
 
   it('Should ignore row collapse change', () => {
+    const { tracker, original, dash } = getTestContext();
     dash.toggleRow(dash.panels[1]);
     expect(tracker.hasChanges(dash, original)).toBe(false);
   });
 
   it('Should ignore panel legend changes', () => {
+    const { tracker, original, dash } = getTestContext();
     dash.panels[0].legend.sortDesc = true;
     dash.panels[0].legend.sort = 'avg';
     expect(tracker.hasChanges(dash, original)).toBe(false);
   });
 
   it('Should ignore panel repeats', () => {
+    const { tracker, original, dash } = getTestContext();
     dash.panels.push(new PanelModel({ repeatPanelId: 10 }));
     expect(tracker.hasChanges(dash, original)).toBe(false);
   });
+
+  describe('ignoreChanges', () => {
+    describe('when called without original dashboard', () => {
+      it('then it should return true', () => {
+        const { tracker, dash } = getTestContext();
+        expect(tracker.ignoreChanges(dash, null)).toBe(true);
+      });
+    });
+
+    describe('when called without current dashboard', () => {
+      it('then it should return true', () => {
+        const { tracker, original } = getTestContext();
+        expect(tracker.ignoreChanges((null as unknown) as DashboardModel, original)).toBe(true);
+      });
+    });
+
+    describe('when called without meta in current dashboard', () => {
+      it('then it should return true', () => {
+        const { tracker, original, dash } = getTestContext();
+        expect(tracker.ignoreChanges({ ...dash, meta: undefined }, original)).toBe(true);
+      });
+    });
+
+    describe('when called for a viewer without save permissions', () => {
+      it('then it should return true', () => {
+        const { tracker, original, dash, contextSrv } = getTestContext();
+        contextSrv.isEditor = false;
+        expect(tracker.ignoreChanges({ ...dash, meta: { canSave: false } }, original)).toBe(true);
+      });
+    });
+
+    describe('when called for a viewer with save permissions', () => {
+      it('then it should return undefined', () => {
+        const { tracker, original, dash, contextSrv } = getTestContext();
+        contextSrv.isEditor = false;
+        expect(tracker.ignoreChanges({ ...dash, meta: { canSave: true } }, original)).toBe(undefined);
+      });
+    });
+
+    describe('when called for an user that is not signed in', () => {
+      it('then it should return true', () => {
+        const { tracker, original, dash, contextSrv } = getTestContext();
+        contextSrv.isSignedIn = false;
+        expect(tracker.ignoreChanges({ ...dash, meta: { canSave: true } }, original)).toBe(true);
+      });
+    });
+
+    describe('when called with fromScript', () => {
+      it('then it should return true', () => {
+        const { tracker, original, dash } = getTestContext();
+        expect(
+          tracker.ignoreChanges({ ...dash, meta: { canSave: true, fromScript: true, fromFile: undefined } }, original)
+        ).toBe(true);
+      });
+    });
+
+    describe('when called with fromFile', () => {
+      it('then it should return true', () => {
+        const { tracker, original, dash } = getTestContext();
+        expect(
+          tracker.ignoreChanges({ ...dash, meta: { canSave: true, fromScript: undefined, fromFile: true } }, original)
+        ).toBe(true);
+      });
+    });
+
+    describe('when called with canSave but without fromScript and fromFile', () => {
+      it('then it should return false', () => {
+        const { tracker, original, dash } = getTestContext();
+        expect(
+          tracker.ignoreChanges(
+            { ...dash, meta: { canSave: true, fromScript: undefined, fromFile: undefined } },
+            original
+          )
+        ).toBe(undefined);
+      });
+    });
+  });
 });

public/app/features/dashboard/services/ChangeTracker.ts

@@ -78,7 +78,8 @@ export class ChangeTracker {
       return true;
     }
 
-    if (!contextSrv.isEditor) {
+    // Ignore changes if the user has been signed out
+    if (!contextSrv.isSignedIn) {
       return true;
     }
 
@@ -86,13 +87,12 @@ export class ChangeTracker {
       return true;
     }
 
-    // Ignore changes if the user has been signed out
+    const { canSave, fromScript, fromFile } = current.meta;
-    if (!contextSrv.isSignedIn) {
+    if (!contextSrv.isEditor && !canSave) {
       return true;
     }
 
-    const meta = current.meta;
+    return !canSave || fromScript || fromFile;
-    return !meta.canSave || meta.fromScript || meta.fromFile;
   }
 
   // remove stuff that should not count in diff