From 71a0a298bf7a5010bd9b6dfdebc692d3ea631098 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Tue, 27 Feb 2018 16:04:26 +0100 Subject: [PATCH] permissions: remove client validation and handle server validation --- .../Permissions/AddPermissions.jest.tsx | 2 +- .../components/Permissions/AddPermissions.tsx | 8 -- .../PermissionsStore/PermissionsStore.jest.ts | 75 +++---------------- .../PermissionsStore/PermissionsStore.ts | 51 +++++-------- 4 files changed, 29 insertions(+), 107 deletions(-) diff --git a/public/app/core/components/Permissions/AddPermissions.jest.tsx b/public/app/core/components/Permissions/AddPermissions.jest.tsx index 9c01eee70b1..fe97c4c7e62 100644 --- 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( {}, diff --git a/public/app/core/components/Permissions/AddPermissions.tsx b/public/app/core/components/Permissions/AddPermissions.tsx index 94afa7c1180..07ccfdbbef5 100644 --- a/public/app/core/components/Permissions/AddPermissions.tsx +++ b/public/app/core/components/Permissions/AddPermissions.tsx @@ -135,14 +135,6 @@ class AddPermissions extends Component { - {permissions.error ? ( -
- - - {permissions.error} - -
- ) : null} ); } diff --git a/public/app/stores/PermissionsStore/PermissionsStore.jest.ts b/public/app/stores/PermissionsStore/PermissionsStore.jest.ts index f7332516bef..c3bc6016e50 100644 --- a/public/app/stores/PermissionsStore/PermissionsStore.jest.ts +++ b/public/app/stores/PermissionsStore/PermissionsStore.jest.ts @@ -1,10 +1,10 @@ -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(); - }); - - it('should allowing overriding the inherited permission and not throw a validation error', () => { - expect(store.error).toBe(null); + await store.addStoreItem(); }); it('should add new overriding permission', () => { diff --git a/public/app/stores/PermissionsStore/PermissionsStore.ts b/public/app/stores/PermissionsStore/PermissionsStore.ts index 88b26a2886f..79df593f06e 100644 --- a/public/app/stores/PermissionsStore/PermissionsStore.ts +++ b/public/app/stores/PermissionsStore/PermissionsStore.ts @@ -1,8 +1,6 @@ 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 = () => { - self.error = null; + const resetNewTypeInternal = () => { 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)) { - return undefined; - } + const updatedItems = self.items.peek(); + const newItem = prepareItem(item, self.dashboardId, self.isFolder, self.isInRoot); + updatedItems.push(newItem); - self.items.push(prepareItem(item, self.dashboardId, self.isFolder, self.isInRoot)); - resetNewType(); - return updateItems(self); + try { + yield updateItems(self, updatedItems); + 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 => { - self.error = null; - +const updateItems = (self, items) => { 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; - try { - res = backendSrv.post(`/api/dashboards/id/${self.dashboardId}/permissions`, { - items: updated, - }); - } catch (error) { - self.error = error; - } - - return res; + return backendSrv.post(`/api/dashboards/id/${self.dashboardId}/permissions`, { + items: updated, + }); }; const prepareServerResponse = (response, dashboardId: number, isFolder: boolean, isInRoot: boolean) => {