permissions: remove client validation and handle server validation

71a0a298 · Marcus Efraimsson · f76b98d2 · 71a0a298 · 71a0a298 · 71a0a298
Commit 71a0a298 authored Feb 27, 2018 by Marcus Efraimsson
4 changed files
--- a/public/app/core/components/Permissions/AddPermissions.jest.tsx
+++ b/public/app/core/components/Permissions/AddPermissions.jest.tsx
@@ -17,7 +17,7 @@ describe('AddPermissions', () => {
      ])
    );
-    backendSrv.post = jest.fn();
+    backendSrv.post = jest.fn(() => Promise.resolve({}));
    store = RootStore.create(
      {},

--- a/public/app/core/components/Permissions/AddPermissions.tsx
+++ b/public/app/core/components/Permissions/AddPermissions.tsx
@@ -135,14 +135,6 @@ class AddPermissions extends Component<IProps, any> {
            </div>
          </div>
        </form>
-        {permissions.error ? (
-          <div className="gf-form width-17">
-            <span ng-if="ctrl.error" className="text-error p-l-1">
-              <i className="fa fa-warning" />
-              {permissions.error}
-            </span>
-          </div>
-        ) : null}
      </div>
    );
  }

--- a/public/app/stores/PermissionsStore/PermissionsStore.jest.ts
+++ b/public/app/stores/PermissionsStore/PermissionsStore.jest.ts
-import { PermissionsStore, aclTypeValues } from './PermissionsStore';
+import { PermissionsStore } from './PermissionsStore';
 import { backendSrv } from 'test/mocks/common';
 describe('PermissionsStore', () => {
  let store;
-  beforeEach(() => {
+  beforeEach(async () => {
    backendSrv.get.mockReturnValue(
      Promise.resolve([
        { id: 2, dashboardId: 1, role: 'Viewer', permission: 1, permissionName: 'View' },
@@ -20,7 +20,7 @@ describe('PermissionsStore', () => {
      ])
    );
-    backendSrv.post = jest.fn();
+    backendSrv.post = jest.fn(() => Promise.resolve({}));
    store = PermissionsStore.create(
      {
@@ -32,14 +32,14 @@ describe('PermissionsStore', () => {
      }
    );
-    return store.load(1, false, false);
+    await store.load(1, false, false);
  });
-  it('should save update on permission change', () => {
+  it('should save update on permission change', async () => {
    expect(store.items[0].permission).toBe(1);
    expect(store.items[0].permissionName).toBe('View');
-    store.updatePermissionOnIndex(0, 2, 'Edit');
+    await store.updatePermissionOnIndex(0, 2, 'Edit');
    expect(store.items[0].permission).toBe(2);
    expect(store.items[0].permissionName).toBe('Edit');
@@ -47,69 +47,18 @@ describe('PermissionsStore', () => {
    expect(backendSrv.post.mock.calls[0][0]).toBe('/api/dashboards/id/1/permissions');
  });
-  it('should save removed permissions automatically', () => {
+  it('should save removed permissions automatically', async () => {
    expect(store.items.length).toBe(3);
-    store.removeStoreItem(2);
+    await store.removeStoreItem(2);
    expect(store.items.length).toBe(2);
    expect(backendSrv.post.mock.calls.length).toBe(1);
    expect(backendSrv.post.mock.calls[0][0]).toBe('/api/dashboards/id/1/permissions');
  });
-  describe('when duplicate team permissions are added', () => {
-    beforeEach(() => {
-      const newItem = {
-        teamId: 10,
-        team: 'tester-team',
-        permission: 1,
-        dashboardId: 1,
-      };
-      store.resetNewType();
-      store.newItem.setTeam(newItem.teamId, newItem.team);
-      store.newItem.setPermission(newItem.permission);
-      store.addStoreItem();
-      store.newItem.setTeam(newItem.teamId, newItem.team);
-      store.newItem.setPermission(newItem.permission);
-      store.addStoreItem();
-    });
-    it('should return a validation error', () => {
-      expect(store.items.length).toBe(4);
-      expect(store.error).toBe('This permission exists already.');
-      expect(backendSrv.post.mock.calls.length).toBe(1);
-    });
-  });
-  describe('when duplicate user permissions are added', () => {
-    beforeEach(() => {
-      expect(store.items.length).toBe(3);
-      const newItem = {
-        userId: 10,
-        userLogin: 'tester1',
-        permission: 1,
-        dashboardId: 1,
-      };
-      store.setNewType(aclTypeValues.USER.value);
-      store.newItem.setUser(newItem.userId, newItem.userLogin);
-      store.newItem.setPermission(newItem.permission);
-      store.addStoreItem();
-      store.setNewType(aclTypeValues.USER.value);
-      store.newItem.setUser(newItem.userId, newItem.userLogin);
-      store.newItem.setPermission(newItem.permission);
-      store.addStoreItem();
-    });
-    it('should return a validation error', () => {
-      expect(store.items.length).toBe(4);
-      expect(store.error).toBe('This permission exists already.');
-      expect(backendSrv.post.mock.calls.length).toBe(1);
-    });
-  });
  describe('when one inherited and one not inherited team permission are added', () => {
-    beforeEach(() => {
+    beforeEach(async () => {
      const overridingItemForChildDashboard = {
        team: 'MyTestTeam',
        dashboardId: 1,
@@ -120,11 +69,7 @@ describe('PermissionsStore', () => {
      store.resetNewType();
      store.newItem.setTeam(overridingItemForChildDashboard.teamId, overridingItemForChildDashboard.team);
      store.newItem.setPermission(overridingItemForChildDashboard.permission);
-      store.addStoreItem();
+      await store.addStoreItem();
-    });
-    it('should allowing overriding the inherited permission and not throw a validation error', () => {
-      expect(store.error).toBe(null);
    });
    it('should add new overriding permission', () => {

--- a/public/app/stores/PermissionsStore/PermissionsStore.ts
+++ b/public/app/stores/PermissionsStore/PermissionsStore.ts
 import { types, getEnv, flow } from 'mobx-state-tree';
 import { PermissionsStoreItem } from './PermissionsStoreItem';
-const duplicateError = 'This permission exists already.';
 export const permissionOptions = [
  { value: 1, label: 'View', description: 'Can view dashboards.' },
  { value: 2, label: 'Edit', description: 'Can add, edit and delete dashboards.' },
@@ -75,7 +73,6 @@ export const PermissionsStore = types
    isFolder: types.maybe(types.boolean),
    dashboardId: types.maybe(types.number),
    items: types.optional(types.array(PermissionsStoreItem), []),
-    error: types.maybe(types.string),
    originalItems: types.optional(types.array(PermissionsStoreItem), []),
    newType: types.optional(types.string, defaultNewType),
    newItem: types.maybe(NewPermissionsItem),
@@ -88,7 +85,6 @@ export const PermissionsStore = types
        return isDuplicate(it, item);
      });
      if (dupe) {
-        self.error = duplicateError;
        return false;
      }
@@ -96,8 +92,7 @@ export const PermissionsStore = types
    },
  }))
  .actions(self => {
-    const resetNewType = () => {
+    const resetNewTypeInternal = () => {
-      self.error = null;
      self.newItem = NewPermissionsItem.create();
    };
@@ -115,11 +110,9 @@ export const PermissionsStore = types
        self.items = items;
        self.originalItems = items;
        self.fetching = false;
-        self.error = null;
      }),
      addStoreItem: flow(function* addStoreItem() {
-        self.error = null;
        let item = {
          type: self.newItem.type,
          permission: self.newItem.permission,
@@ -147,19 +140,21 @@ export const PermissionsStore = types
            throw Error('Unknown type: ' + self.newItem.type);
        }
-        if (!self.isValid(item)) {
+        const updatedItems = self.items.peek();
-          return undefined;
+        const newItem = prepareItem(item, self.dashboardId, self.isFolder, self.isInRoot);
-        }
+        updatedItems.push(newItem);
-        self.items.push(prepareItem(item, self.dashboardId, self.isFolder, self.isInRoot));
+        try {
-        resetNewType();
+          yield updateItems(self, updatedItems);
-        return updateItems(self);
+          self.items.push(newItem);
+          resetNewTypeInternal();
+        } catch {}
+        yield Promise.resolve();
      }),
      removeStoreItem: flow(function* removeStoreItem(idx: number) {
-        self.error = null;
        self.items.splice(idx, 1);
-        return updateItems(self);
+        yield updateItems(self, self.items.peek());
      }),
      updatePermissionOnIndex: flow(function* updatePermissionOnIndex(
@@ -167,9 +162,8 @@ export const PermissionsStore = types
        permission: number,
        permissionName: string
      ) {
-        self.error = null;
        self.items[idx].updatePermission(permission, permissionName);
-        return updateItems(self);
+        yield updateItems(self, self.items.peek());
      }),
      setNewType(newType: string) {
@@ -177,7 +171,7 @@ export const PermissionsStore = types
      },
      resetNewType() {
-        resetNewType();
+        resetNewTypeInternal();
      },
      toggleAddPermissions() {
@@ -190,12 +184,10 @@ export const PermissionsStore = types
    };
  });
-const updateItems = self => {
+const updateItems = (self, items) => {
-  self.error = null;
  const backendSrv = getEnv(self).backendSrv;
  const updated = [];
-  for (let item of self.items) {
+  for (let item of items) {
    if (item.inherited) {
      continue;
    }
@@ -208,16 +200,9 @@ const updateItems = self => {
    });
  }
-  let res;
+  return backendSrv.post(`/api/dashboards/id/${self.dashboardId}/permissions`, {
-  try {
-    res = backendSrv.post(`/api/dashboards/id/${self.dashboardId}/permissions`, {
    items: updated,
  });
-  } catch (error) {
-    self.error = error;
-  }
-  return res;
 };
 const prepareServerResponse = (response, dashboardId: number, isFolder: boolean, isInRoot: boolean) => {