diff --git a/packages/app/obojobo-express/__tests__/express_validators.test.js b/packages/app/obojobo-express/__tests__/express_validators.test.js index 1bed32b95a..23afbe2fc2 100644 --- a/packages/app/obojobo-express/__tests__/express_validators.test.js +++ b/packages/app/obojobo-express/__tests__/express_validators.test.js @@ -519,6 +519,28 @@ describe('current user middleware', () => { }) }) + // requireCanViewAdminPage + + test('requireCanViewAdminPage calls next and has no validation errors', () => { + mockUser.hasPermission = perm => perm === 'canViewAdminPage' + mockReq.requireCurrentUser = jest.fn().mockResolvedValue(mockUser) + return Validators.requireCanViewAdminPage(mockReq, mockRes, mockNext).then(() => { + expect(mockNext).toHaveBeenCalledTimes(1) + expect(mockRes.notAuthorized).toHaveBeenCalledTimes(0) + expect(mockReq._validationErrors).toBeUndefined() + }) + }) + + test('requireCanViewAdminPage doesnt call next and has errors', () => { + mockUser.hasPermission = () => false + mockReq.requireCurrentUser = jest.fn().mockResolvedValue(mockUser) + return Validators.requireCanViewAdminPage(mockReq, mockRes, mockNext).then(() => { + expect(mockNext).toHaveBeenCalledTimes(0) + expect(mockRes.notAuthorized).toHaveBeenCalledTimes(1) + expect(mockReq._validationErrors).toBeUndefined() + }) + }) + // checkValidationRules test('checkValidationRules calls next with no errors', () => { diff --git a/packages/app/obojobo-express/server/models/user.js b/packages/app/obojobo-express/server/models/user.js index 204c950fd7..e2c420667a 100644 --- a/packages/app/obojobo-express/server/models/user.js +++ b/packages/app/obojobo-express/server/models/user.js @@ -102,22 +102,6 @@ class User { }) } - static getAll() { - return db - .manyOrNone( - `SELECT - * - FROM users - ORDER BY first_name, last_name - LIMIT 25` - ) - .then(results => results.map(r => User.dbResultToModel(r))) - .catch(error => { - logger.logError('Error getAll', error) - throw Error('Error fetching all users.') - }) - } - saveOrCreate() { return db .one( diff --git a/packages/app/obojobo-repository/server/models/admin_interface.js b/packages/app/obojobo-repository/server/models/admin_interface.js index 6ac4ca6275..ba1c7677df 100644 --- a/packages/app/obojobo-repository/server/models/admin_interface.js +++ b/packages/app/obojobo-repository/server/models/admin_interface.js @@ -3,72 +3,94 @@ const logger = require('obojobo-express/server/logger') const User = require('obojobo-express/server/models/user') +const { PERMS_PER_ROLE } = require('../../shared/util/implicit-perms') + class AdminInterface { static addPermission(userId, permission) { return new Promise((resolve, reject) => { - // First fetch user User.fetchById(userId) - .then(u => { - let perms = u.perms - if (perms.includes(permission)) return - perms = [...perms, permission] - - // Then add new permission (if it is new) - this._updateUserPerms(userId, perms) - .then(id => resolve(id)) + .then(u => { + let perms = u.perms + if (perms.includes(permission)) return resolve(u) + + perms = [...u.perms, permission] + + const allRolePerms = new Set() + + u.roles.forEach(role => { + PERMS_PER_ROLE[role].forEach(perm => allRolePerms.add(perm)) + }) + + perms = perms.filter(p => !allRolePerms.has(p)) + + const dedupedPerms = new Set([...u.perms, ...perms]) + u.perms = [...dedupedPerms] + + // Then add new permission (if it is new) + this._updateUserPerms(userId, perms) + .then(() => resolve(u)) + .catch(() => { + reject(logger.logError(`AdminInterface error adding permission`)) + }) + }) .catch(() => { - reject() - throw logger.logError(`AdminInterface error adding permission`) + reject(logger.logError(`AdminInterface error finding user with id ${userId}`)) }) - }) - .catch(err => { - reject() - throw logger.logError(`AdminInterface error finding user with id ${userId}`) - }) }) } static removePermission(userId, permission) { return new Promise((resolve, reject) => { - // First fetch user User.fetchById(userId) - .then(u => { - const perms = u.perms + .then(u => { + const allRolePerms = new Set() + + u.roles.forEach(role => { + PERMS_PER_ROLE[role].forEach(perm => allRolePerms.add(perm)) + }) + + const perms = u.perms.filter(p => !allRolePerms.has(p)) + + const ix = perms.indexOf(permission) + if (ix === -1) return resolve(u) + perms.splice(ix, 1) - const ix = perms.indexOf(permission) - if (ix === -1) return - perms.splice(ix, 1) + // add any remaining perms to the 'allRolePerms' set so we can use it to set the user's new combined perms + perms.forEach(p => allRolePerms.add(p)) - // Then remove permission (if present) - this._updateUserPerms(userId, perms) - .then(id => resolve(id)) + u.perms = [...allRolePerms] + + // Then remove permission (if present) + this._updateUserPerms(userId, perms) + .then(() => resolve(u)) + .catch(() => { + reject(logger.logError(`AdminInterface error removing permission`)) + }) + }) .catch(() => { - reject() - throw logger.logError(`AdminInterface error removing permission`) + reject(logger.logError(`AdminInterface error finding user with id ${userId}`)) }) - }) - .catch(() => { - reject() - throw logger.logError(`AdminInterface Error finding user with id ${userId}`) - }) }) } static _updateUserPerms(userId, perms) { return new Promise((resolve, reject) => { - db.oneOrNone(` - UPDATE user_perms - SET perms = $[perms] - WHERE user_id = $[userId] + db.oneOrNone( + ` + INSERT INTO user_perms (user_id, perms) + VALUES ($[userId], $[perms]) + ON CONFLICT (user_id) + DO UPDATE SET perms = $[perms] + WHERE user_perms.user_id = $[userId] `, - { userId, perms }) - .then(() => resolve(userId)) - .catch(error => { - reject() - throw logger.logError('AdminInterface _updateUserPerms Error', error) - }) + { userId, perms } + ) + .then(() => resolve(userId)) + .catch(error => { + reject(logger.logError('AdminInterface _updateUserPerms error', error)) + }) }) } } -module.exports = AdminInterface \ No newline at end of file +module.exports = AdminInterface diff --git a/packages/app/obojobo-repository/server/models/admin_interface.test.js b/packages/app/obojobo-repository/server/models/admin_interface.test.js index d6b7ff67d8..01341aa48e 100644 --- a/packages/app/obojobo-repository/server/models/admin_interface.test.js +++ b/packages/app/obojobo-repository/server/models/admin_interface.test.js @@ -1,67 +1,183 @@ +jest.mock('obojobo-express/server/db') +jest.mock('obojobo-express/server/logger') +jest.mock('obojobo-express/server/models/user') +jest.mock('../../shared/util/implicit-perms', () => ({ + PERMS_PER_ROLE: { + mockRole: ['mockExistingPermission'] + } +})) + +const db = require('obojobo-express/server/db') +const logger = require('obojobo-express/server/logger') + +const User = require('obojobo-express/server/models/user') + +const AdminInterface = require('./admin_interface') + describe('AdminInterface Model', () => { - jest.mock('obojobo-express/server/db') - jest.mock('obojobo-express/server/logger') - let db - let logger - let AdminInterface + let expectedResponseUser - beforeEach(() => { + beforeEach(() => { jest.resetModules() jest.resetAllMocks() - db = require('obojobo-express/server/db') - logger = require('obojobo-express/server/logger') - AdminInterface = require('./admin_interface') + expectedResponseUser = { + perms: [], + roles: [] + } + + // provide this by defualt, override in individual tests if necessary + // if every test ends up overriding this, just remove this one + User.fetchById = jest.fn().mockResolvedValueOnce(expectedResponseUser) + }) + + test('addPermission does nothing if trying to add a permission the given user already has', () => { + expect.assertions(2) + + // set the user's permissions such that they already have the one we're trying to give them + // ideally we could check implicit and explicit perms separately, but they're added to a single + // array inside the User model so our only option is to check the one location + expectedResponseUser.perms = ['someExistingPermission'] + User.fetchById = jest.fn().mockResolvedValueOnce(expectedResponseUser) + + return AdminInterface.addPermission(5, 'someExistingPermission').then(u => { + expect(u).toEqual(expectedResponseUser) + expect(db.oneOrNone).not.toHaveBeenCalled() + }) + }) + + test('addPermission only saves explicitly granted permissions', () => { + expect.assertions(3) + + db.oneOrNone.mockResolvedValueOnce(5) + + // 'mockRole' will account for the 'mockExistingPermission' perm below + expectedResponseUser.roles = ['mockRole'] + expectedResponseUser.perms = ['someExistingPermission', 'mockExistingPermission'] + User.fetchById = jest.fn().mockResolvedValueOnce(expectedResponseUser) + + return AdminInterface.addPermission(5, 'someNewPermission').then(u => { + expect(u).toEqual({ + ...expectedResponseUser, + perms: ['someExistingPermission', 'mockExistingPermission', 'someNewPermission'] + }) + expect(db.oneOrNone).toHaveBeenCalledTimes(1) + // first argument to db function is the query string, no need to check that + expect(db.oneOrNone.mock.calls[0][1]).toEqual({ + userId: 5, + // since 'mockExistingPermission' is a perm-based/implicit perm, + // it should not have been saved explicitly + perms: ['someExistingPermission', 'someNewPermission'] + }) + }) + }) + + test('addPermission catches error when fetching user with invalid id', () => { + User.fetchById = jest.fn().mockRejectedValueOnce('mock-error') + + expect.hasAssertions() + + return AdminInterface.addPermission(123456, 'someNewPermission').catch(() => { + expect(logger.logError).toHaveBeenCalledWith( + 'AdminInterface error finding user with id 123456' + ) + }) + }) + + test('addPermission catches error when updating user perms', () => { + expect.hasAssertions() + + db.oneOrNone.mockRejectedValueOnce('mock-error') + + return AdminInterface.addPermission(5, 'someNewPermission').catch(() => { + expect(logger.logError).toHaveBeenCalledTimes(2) + expect(logger.logError).toHaveBeenCalledWith( + 'AdminInterface _updateUserPerms error', + 'mock-error' + ) + expect(logger.logError).toHaveBeenCalledWith('AdminInterface error adding permission') + }) + }) + + test('removePermission does nothing if trying to remove a permission the given user does not have', () => { + expect.assertions(2) + + // set the user's permissions such that they already have the one we're trying to give them + // ideally we could check implicit and explicit perms separately, but they're added to a single + // array inside the User model so our only option is to check the one location + expectedResponseUser.perms = ['someOtherPermission'] + User.fetchById = jest.fn().mockResolvedValueOnce(expectedResponseUser) + + return AdminInterface.removePermission(5, 'someExistingPermission').then(u => { + expect(u).toEqual(expectedResponseUser) + expect(db.oneOrNone).not.toHaveBeenCalled() + }) + }) + + test('removePermission does nothing if trying to remove a permission the given user has implicitly', () => { + expect.assertions(2) + + expectedResponseUser.roles = ['mockRole'] + expectedResponseUser.perms = ['someExistingPermission', 'mockExistingPermission'] + User.fetchById = jest.fn().mockResolvedValueOnce(expectedResponseUser) + + return AdminInterface.removePermission(5, 'mockExistingPermission').then(u => { + expect(u).toEqual(expectedResponseUser) + expect(db.oneOrNone).not.toHaveBeenCalled() + }) }) - afterEach(() => {}) - - test('addPermission correctly fetches id and adds new permission (if any)', () => { - db.one.mockResolvedValueOnce({ - id: 5, - created_at: 'mocked-date', - email: 'guest@obojobo.ucf.edu', - first_name: 'Guest', - last_name: 'Guest', - roles: [], - username: 'guest' + + test('removePermission saves explicit permissions after removing one from the given user', () => { + db.oneOrNone.mockResolvedValueOnce(5) + + // 'mockRole' will account for the 'mockExistingPermission' perm below + expectedResponseUser.roles = ['mockRole'] + expectedResponseUser.perms = [ + 'someExistingPermission', + 'someOtherExistingPermission', + 'mockExistingPermission' + ] + User.fetchById = jest.fn().mockResolvedValueOnce(expectedResponseUser) + + return AdminInterface.removePermission(5, 'someOtherExistingPermission').then(u => { + expect(u).toEqual({ + ...expectedResponseUser, + perms: ['mockExistingPermission', 'someExistingPermission'] + }) + expect(db.oneOrNone).toHaveBeenCalledTimes(1) + expect(db.oneOrNone.mock.calls[0][1]).toEqual({ + userId: 5, + perms: ['someExistingPermission'] + }) }) + }) + + test('removePermission catches error when fetching user with invalid id', () => { + User.fetchById = jest.fn().mockRejectedValueOnce('mock-error') + + expect.hasAssertions() - db.oneOrNone.mockResolvedValueOnce({}) - - return AdminInterface.addPermission(5, 'canViewAdminPage') - .then(ret => expect(ret).toBe(5)) - }) - - test('addPermission catches error when fetching user with invalid id', () => { - logger.logError = jest.fn() - - db.one.mockRejectedValueOnce('mock-error') - - return AdminInterface.addPermission(123456, 'canViewAdminPage') - .then(ret => { - console.log("ret:") - console.log(ret) - }) - .catch(() => { - console.log("here") - expect(logger.logError).toHaveBeenCalledWith('AdminInterface error finding user with id 123456') - }) - }) - - test('removePermission correctly fetches id and removes permission', () => { - db.one.mockResolvedValueOnce({ - id: 5, - created_at: 'mocked-date', - email: 'guest@obojobo.ucf.edu', - first_name: 'Guest', - last_name: 'Guest', - perms: ['canViewAdminPage'], - username: 'guest' + return AdminInterface.removePermission(123456, 'someExistingPermission').catch(() => { + expect(logger.logError).toHaveBeenCalledWith( + 'AdminInterface error finding user with id 123456' + ) }) + }) + + test('removePermission catches error when updating user perms', () => { + expect.hasAssertions() + + db.oneOrNone.mockRejectedValueOnce('mock-error') - db.oneOrNone.mockResolvedValueOnce({}) - - return AdminInterface.removePermission(5, 'canViewAdminPage') - .then(ret => expect(ret).toBe(5)) - }) -}) \ No newline at end of file + expectedResponseUser.perms = ['someExistingPermission'] + + return AdminInterface.removePermission(5, 'someExistingPermission').catch(() => { + expect(logger.logError).toHaveBeenCalledTimes(2) + expect(logger.logError).toHaveBeenCalledWith( + 'AdminInterface _updateUserPerms error', + 'mock-error' + ) + expect(logger.logError).toHaveBeenCalledWith('AdminInterface error removing permission') + }) + }) +}) diff --git a/packages/app/obojobo-repository/server/routes/api.js b/packages/app/obojobo-repository/server/routes/api.js index b16b87438c..b912aa8688 100644 --- a/packages/app/obojobo-repository/server/routes/api.js +++ b/packages/app/obojobo-repository/server/routes/api.js @@ -16,7 +16,7 @@ const { requireCanDeleteDrafts, check, requireCanViewStatsPage, - requireCanViewAdminPage, + requireCanViewAdminPage } = require('obojobo-express/server/express_validators') const UserModel = require('obojobo-express/server/models/user') const { searchForUserByString } = require('../services/search') @@ -172,47 +172,34 @@ router }) router - .route('/users/all') - .get([requireCanViewAdminPage]) - .get(async (req, res) => { + .route('/permissions/add') + .post([requireCanViewAdminPage]) + .post(async (req, res) => { + const userId = req.body.userId + const perm = req.body.perm + try { - let users = await UserModel.getAll() - users = users.map(u => u.toJSON()) - res.success(users) + const modifiedUser = await AdminInterface.addPermission(userId, perm) + res.success(modifiedUser) } catch (error) { res.unexpected(error) } }) router -.route('/permissions/add') -.post([requireCanViewAdminPage]) -.post(async (req, res) => { - const userId = req.body.userId - const perm = req.body.perm - - try { - await AdminInterface.addPermission(userId, perm) - res.success() - } catch (error) { - res.unexpected(error) - } -}) + .route('/permissions/remove') + .post([requireCanViewAdminPage]) + .post(async (req, res) => { + const userId = req.body.userId + const perm = req.body.perm -router -.route('/permissions/remove') -.post([requireCanViewAdminPage]) -.post(async (req, res) => { - const userId = req.body.userId - const perm = req.body.perm - - try { - await AdminInterface.removePermission(userId, perm) - res.success() - } catch (error) { - res.unexpected(error) - } -}) + try { + const modifiedUser = await AdminInterface.removePermission(userId, perm) + res.success(modifiedUser) + } catch (error) { + res.unexpected(error) + } + }) // Copy a draft to the current user // mounted as /api/drafts/:draftId/copy @@ -541,5 +528,4 @@ router } }) - module.exports = router diff --git a/packages/app/obojobo-repository/server/routes/api.test.js b/packages/app/obojobo-repository/server/routes/api.test.js index db541d935e..538d94debd 100644 --- a/packages/app/obojobo-repository/server/routes/api.test.js +++ b/packages/app/obojobo-repository/server/routes/api.test.js @@ -10,6 +10,7 @@ jest.mock('../services/collections') jest.mock('../services/count') jest.mock('obojobo-express/server/models/user') jest.mock('obojobo-express/server/insert_event') +jest.mock('../models/admin_interface') jest.unmock('fs') // need fs working for view rendering jest.unmock('express') // we'll use supertest + express for this @@ -24,6 +25,7 @@ let CountServices let UserModel let insertEvent let DraftPermissions +let AdminInterface // override requireCurrentUser for tests to provide our own user let mockCurrentUser @@ -98,6 +100,7 @@ describe('repository api route', () => { CountServices = require('../services/count') DraftPermissions = require('../models/draft_permissions') UserModel = require('obojobo-express/server/models/user') + AdminInterface = require('../models/admin_interface') insertEvent = require('obojobo-express/server/insert_event') }) @@ -494,6 +497,64 @@ describe('repository api route', () => { }) }) + test('post /permissions/add returns the expected response', () => { + expect.hasAssertions() + + // this should be a user object but for testing purposes it doesn't matter + AdminInterface.addPermission.mockResolvedValueOnce(true) + + return request(app) + .post('/permissions/add') + .send({ userId: 5, perm: 'someNewPerm' }) + .then(response => { + expect(AdminInterface.addPermission).toHaveBeenCalledTimes(1) + expect(AdminInterface.addPermission).toHaveBeenCalledWith(5, 'someNewPerm') + expect(response.statusCode).toBe(200) + }) + }) + test('post /permissions/add handles thrown errors', () => { + expect.hasAssertions() + + AdminInterface.addPermission.mockRejectedValueOnce('database error') + + return request(app) + .post('/permissions/add') + .send({ userId: 5, perm: 'someNewPerm' }) + .then(response => { + expect(AdminInterface.addPermission).toHaveBeenCalledTimes(1) + expect(AdminInterface.addPermission).toHaveBeenCalledWith(5, 'someNewPerm') + expect(response.statusCode).toBe(500) + }) + }) + test('post /permissions/remove returns the expected response', () => { + expect.hasAssertions() + + AdminInterface.removePermission.mockResolvedValueOnce(true) + + return request(app) + .post('/permissions/remove') + .send({ userId: 5, perm: 'someExistingPerm' }) + .then(response => { + expect(AdminInterface.removePermission).toHaveBeenCalledTimes(1) + expect(AdminInterface.removePermission).toHaveBeenCalledWith(5, 'someExistingPerm') + expect(response.statusCode).toBe(200) + }) + }) + test('post /permissions/remove handles thrown errors', () => { + expect.hasAssertions() + + AdminInterface.removePermission.mockRejectedValueOnce('database error') + + return request(app) + .post('/permissions/remove') + .send({ userId: 5, perm: 'someExistingPerm' }) + .then(response => { + expect(AdminInterface.removePermission).toHaveBeenCalledTimes(1) + expect(AdminInterface.removePermission).toHaveBeenCalledWith(5, 'someExistingPerm') + expect(response.statusCode).toBe(500) + }) + }) + test('post /drafts/:draftId/copy returns the expected response when user can copy draft', () => { expect.hasAssertions() diff --git a/packages/app/obojobo-repository/server/services/search.js b/packages/app/obojobo-repository/server/services/search.js index 19dd0c5080..1ba978780a 100644 --- a/packages/app/obojobo-repository/server/services/search.js +++ b/packages/app/obojobo-repository/server/services/search.js @@ -6,18 +6,21 @@ const searchForUserByString = async searchString => { return db .manyOrNone( `SELECT - id, - first_name AS "firstName", - last_name AS "lastName", - email, - username, - created_at AS "createdAt", - roles + users.id, + users.first_name AS "firstName", + users.last_name AS "lastName", + users.email, + users.username, + users.created_at AS "createdAt", + users.roles, + user_perms.perms FROM users - WHERE obo_immutable_concat_ws(' ', first_name, last_name) ILIKE $[search] - OR email ILIKE $[search] - OR username ILIKE $[search] - ORDER BY first_name, last_name + LEFT JOIN user_perms + ON users.id = user_perms.user_id + WHERE obo_immutable_concat_ws(' ', users.first_name, users.last_name) ILIKE $[search] + OR users.email ILIKE $[search] + OR users.username ILIKE $[search] + ORDER BY users.first_name, users.last_name LIMIT 25`, { search: `%${searchString}%` } ) diff --git a/packages/app/obojobo-repository/server/services/search.test.js b/packages/app/obojobo-repository/server/services/search.test.js index 789f406285..61c748db44 100644 --- a/packages/app/obojobo-repository/server/services/search.test.js +++ b/packages/app/obojobo-repository/server/services/search.test.js @@ -36,18 +36,21 @@ describe('Search Services', () => { expect.hasAssertions() const manyOrNoneQueryString = `SELECT - id, - first_name AS "firstName", - last_name AS "lastName", - email, - username, - created_at AS "createdAt", - roles + users.id, + users.first_name AS "firstName", + users.last_name AS "lastName", + users.email, + users.username, + users.created_at AS "createdAt", + users.roles, + user_perms.perms FROM users - WHERE obo_immutable_concat_ws(' ', first_name, last_name) ILIKE $[search] - OR email ILIKE $[search] - OR username ILIKE $[search] - ORDER BY first_name, last_name + LEFT JOIN user_perms + ON users.id = user_perms.user_id + WHERE obo_immutable_concat_ws(' ', users.first_name, users.last_name) ILIKE $[search] + OR users.email ILIKE $[search] + OR users.username ILIKE $[search] + ORDER BY users.first_name, users.last_name LIMIT 25` return SearchServices.searchForUserByString('searchString').then(response => { diff --git a/packages/app/obojobo-repository/shared/actions/admin-actions.js b/packages/app/obojobo-repository/shared/actions/admin-actions.js index 393aea53c9..ab594a955b 100644 --- a/packages/app/obojobo-repository/shared/actions/admin-actions.js +++ b/packages/app/obojobo-repository/shared/actions/admin-actions.js @@ -1,31 +1,44 @@ -// =================== API ======================= +const debouncePromise = require('debounce-promise') -const apiGetModules = () => { - const JSON_MIME_TYPE = 'application/json' - const fetchOptions = { - method: 'GET', - credentials: 'include', - headers: { - Accept: JSON_MIME_TYPE, - 'Content-Type': JSON_MIME_TYPE - } +const JSON_MIME_TYPE = 'application/json' +const defaultOptions = () => ({ + method: 'GET', + credentials: 'include', + headers: { + Accept: JSON_MIME_TYPE, + 'Content-Type': JSON_MIME_TYPE } - return fetch('/api/drafts', fetchOptions).then(res => res.json()) +}) + +const throwIfNotOk = res => { + if (!res.ok) throw Error(`Error requesting ${res.url}, status code: ${res.status}`) + return res } -const apiGetUsers = () => { - const JSON_MIME_TYPE = 'application/json' - const fetchOptions = { - method: 'GET', - credentials: 'include', - headers: { - Accept: JSON_MIME_TYPE, - 'Content-Type': JSON_MIME_TYPE - } - } - return fetch('/api/users/all', fetchOptions).then(res => res.json()) +const apiSearchForUser = searchString => { + return fetch(`/api/users/search?q=${searchString}`, defaultOptions()) + .then(throwIfNotOk) + .then(res => res.json()) } +const apiSearchForUserDebounced = debouncePromise(apiSearchForUser, 300) + +// =================== API ======================= + +// not using this yet +// const apiGetModules = () => { +// const JSON_MIME_TYPE = 'application/json' +// const fetchOptions = { +// method: 'GET', +// credentials: 'include', +// headers: { +// Accept: JSON_MIME_TYPE, +// 'Content-Type': JSON_MIME_TYPE +// } +// } +// return fetch('/api/drafts', fetchOptions).then(res => res.json()) +// } + const apiAddUserPermission = (userId, perm) => { const JSON_MIME_TYPE = 'application/json' const fetchOptions = { @@ -58,16 +71,17 @@ const apiRemoveUserPermission = (userId, perm) => { // ================== ACTIONS =================== -const LOAD_ALL_MODULES = 'LOAD_ALL_MODULES' -const loadModuleList = () => ({ - type: LOAD_ALL_MODULES, - promise: apiGetModules() -}) +// const LOAD_ALL_MODULES = 'LOAD_ALL_MODULES' +// const loadModuleList = () => ({ +// type: LOAD_ALL_MODULES, +// promise: apiGetModules() +// }) -const LOAD_ALL_USERS = 'LOAD_ALL_USERS' -const loadUserList = () => ({ - type: LOAD_ALL_USERS, - promise: apiGetUsers() +const LOAD_USER_SEARCH = 'LOAD_USER_SEARCH' +const searchForUser = searchString => ({ + type: LOAD_USER_SEARCH, + meta: { searchString }, + promise: apiSearchForUserDebounced(searchString) }) const ADD_USER_PERMISSION = 'ADD_USER_PERMISSION' @@ -82,16 +96,22 @@ const removeUserPermission = (userId, perm) => ({ promise: apiRemoveUserPermission(userId, perm) }) -module.exports = { - LOAD_ALL_MODULES, - loadModuleList, +const CLEAR_PEOPLE_SEARCH_RESULTS = 'CLEAR_PEOPLE_SEARCH_RESULTS' +const clearPeopleSearchResults = () => ({ type: CLEAR_PEOPLE_SEARCH_RESULTS }) - LOAD_ALL_USERS, - loadUserList, +module.exports = { + // LOAD_ALL_MODULES, + // loadModuleList, ADD_USER_PERMISSION, addUserPermission, REMOVE_USER_PERMISSION, removeUserPermission, -} \ No newline at end of file + + LOAD_USER_SEARCH, + searchForUser, + + CLEAR_PEOPLE_SEARCH_RESULTS, + clearPeopleSearchResults +} diff --git a/packages/app/obojobo-repository/shared/actions/admin-actions.test.js b/packages/app/obojobo-repository/shared/actions/admin-actions.test.js index e69de29bb2..07b0fc4491 100644 --- a/packages/app/obojobo-repository/shared/actions/admin-actions.test.js +++ b/packages/app/obojobo-repository/shared/actions/admin-actions.test.js @@ -0,0 +1,163 @@ +const dayjs = require('dayjs') +const advancedFormat = require('dayjs/plugin/advancedFormat') + +jest.mock('./shared-api-methods', () => ({ + apiGetAssessmentDetailsForDraft: () => Promise.resolve() +})) + +dayjs.extend(advancedFormat) + +describe('Admin Actions', () => { + let standardFetchResponse + + let AdminActions + + // this is lifted straight out of admin-actions, for ease of comparison + // barring any better ways of using it + const defaultFetchOptions = { + method: 'GET', + credentials: 'include', + headers: { + Accept: 'application/json', + 'Content-Type': 'application/json' + } + } + + const originalFetch = global.fetch + + beforeAll(() => { + global.fetch = jest.fn() + }) + + beforeEach(() => { + jest.resetModules() + jest.resetAllMocks() + jest.useFakeTimers() + + global.alert = jest.fn() + + delete window.location + window.location = { + reload: jest.fn() + } + + standardFetchResponse = { + ok: true, + json: jest.fn(() => ({ value: 'mockVal' })) + } + + AdminActions = require('./admin-actions') + }) + + afterAll(() => { + global.fetch = originalFetch + }) + + test('searchForUser returns the expected output and calls other functions correctly - server ok', () => { + global.fetch.mockResolvedValueOnce({ ...standardFetchResponse, ok: true }) + + const actionReply = AdminActions.searchForUser('mockSearchString') + jest.runAllTimers() + + expect(global.fetch).toHaveBeenCalledWith( + '/api/users/search?q=mockSearchString', + defaultFetchOptions + ) + + expect(actionReply).toEqual({ + type: AdminActions.LOAD_USER_SEARCH, + meta: { + searchString: 'mockSearchString' + }, + promise: expect.any(Object) + }) + + return actionReply.promise.then(() => { + expect(standardFetchResponse.json).toHaveBeenCalled() + }) + }) + test('searchForUser returns the expected output and calls other functions correctly - server error', () => { + const mockFetchUrl = '/api/users/search?q=mockSearchString' + global.fetch.mockResolvedValueOnce({ + ok: false, + url: mockFetchUrl, + status: 500 + }) + + const actionReply = AdminActions.searchForUser('mockSearchString') + + expect(actionReply).toEqual({ + type: AdminActions.LOAD_USER_SEARCH, + meta: { + searchString: 'mockSearchString' + }, + promise: expect.any(Object) + }) + + jest.runAllTimers() + return actionReply.promise.catch(error => { + expect(error).toBeInstanceOf(Error) + expect(error.message).toBe( + 'Error requesting /api/users/search?q=mockSearchString, status code: 500' + ) + + expect(global.fetch).toHaveBeenCalledWith( + '/api/users/search?q=mockSearchString', + defaultFetchOptions + ) + }) + }) + + test('addUserPermission returns the expected output and calls other functions', () => { + global.fetch.mockResolvedValueOnce({ ...standardFetchResponse, ok: true }) + + const actionReply = AdminActions.addUserPermission(5, 'someNewPermission') + jest.runAllTimers() + + expect(global.fetch).toHaveBeenCalledWith('/api/permissions/add', { + ...defaultFetchOptions, + method: 'POST', + body: JSON.stringify({ userId: 5, perm: 'someNewPermission' }) + }) + + expect(actionReply).toEqual({ + type: AdminActions.ADD_USER_PERMISSION, + promise: expect.any(Object) + }) + + return actionReply.promise.then(() => { + expect(standardFetchResponse.json).toHaveBeenCalled() + }) + }) + + test('removeUserPermission returns the expected output and calls other functions', () => { + global.fetch.mockResolvedValueOnce({ ...standardFetchResponse, ok: true }) + + const actionReply = AdminActions.removeUserPermission(5, 'someExistingPermission') + jest.runAllTimers() + + expect(global.fetch).toHaveBeenCalledWith('/api/permissions/remove', { + ...defaultFetchOptions, + method: 'POST', + body: JSON.stringify({ userId: 5, perm: 'someExistingPermission' }) + }) + + expect(actionReply).toEqual({ + type: AdminActions.REMOVE_USER_PERMISSION, + promise: expect.any(Object) + }) + + return actionReply.promise.then(() => { + expect(standardFetchResponse.json).toHaveBeenCalled() + }) + }) + + test('clearPeopleSearchResults returns the expected output', () => { + const actionReply = AdminActions.clearPeopleSearchResults() + + expect(global.fetch).not.toHaveBeenCalled() + expect(actionReply).toEqual({ + type: AdminActions.CLEAR_PEOPLE_SEARCH_RESULTS + }) + }) +}) diff --git a/packages/app/obojobo-repository/shared/components/__snapshots__/admin.test.js.snap b/packages/app/obojobo-repository/shared/components/__snapshots__/admin.test.js.snap index a08ef7b7ea..485364a0c2 100644 --- a/packages/app/obojobo-repository/shared/components/__snapshots__/admin.test.js.snap +++ b/packages/app/obojobo-repository/shared/components/__snapshots__/admin.test.js.snap @@ -1,6 +1,6 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`Admin Renders "tools loaded" state correctly 1`] = ` +exports[`Admin renders default view correctly 1`] = ` @@ -100,9 +100,23 @@ exports[`Admin Renders "tools loaded" state correctly 1`] = `
-

- Loading... -

+
+

+ Find Users to Manage +

+ +
+
+
    +
diff --git a/packages/app/obojobo-repository/shared/components/admin-hoc.js b/packages/app/obojobo-repository/shared/components/admin-hoc.js index 406cc202c4..25accf38ea 100644 --- a/packages/app/obojobo-repository/shared/components/admin-hoc.js +++ b/packages/app/obojobo-repository/shared/components/admin-hoc.js @@ -1,17 +1,17 @@ const Admin = require('./admin') const connect = require('react-redux').connect const { - loadModuleList, - loadUserList, addUserPermission, - removeUserPermission + removeUserPermission, + searchForUser, + clearPeopleSearchResults } = require('../actions/admin-actions') const mapStoreStateToProps = state => state const mapActionsToProps = { - loadModuleList, - loadUserList, addUserPermission, - removeUserPermission + removeUserPermission, + searchForUser, + clearPeopleSearchResults } module.exports = connect( mapStoreStateToProps, diff --git a/packages/app/obojobo-repository/shared/components/admin-hoc.test.js b/packages/app/obojobo-repository/shared/components/admin-hoc.test.js index dcc7a1b365..50103f49b5 100644 --- a/packages/app/obojobo-repository/shared/components/admin-hoc.test.js +++ b/packages/app/obojobo-repository/shared/components/admin-hoc.test.js @@ -24,8 +24,10 @@ describe('admin HOC', () => { expect(ReactRedux.connect).toHaveBeenCalledTimes(1) expect(ReactRedux.connect).toHaveBeenCalledWith(expect.any(Function), { - loadUserModuleList: AdminActions.loadUserModuleList, - loadModuleAssessmentDetails: AdminActions.loadModuleAssessmentDetails + addUserPermission: AdminActions.addUserPermission, + clearPeopleSearchResults: AdminActions.clearPeopleSearchResults, + removeUserPermission: AdminActions.removeUserPermission, + searchForUser: AdminActions.searchForUser }) expect(mockReduxConnectReturn).toHaveBeenCalledTimes(1) diff --git a/packages/app/obojobo-repository/shared/components/admin.jsx b/packages/app/obojobo-repository/shared/components/admin.jsx index 40843386cc..301378dd7f 100644 --- a/packages/app/obojobo-repository/shared/components/admin.jsx +++ b/packages/app/obojobo-repository/shared/components/admin.jsx @@ -5,70 +5,40 @@ const React = require('react') const { useState, useEffect } = require('react') const RepositoryNav = require('./repository-nav') const RepositoryBanner = require('./repository-banner') +const Search = require('./search') const Button = require('./button') +const PeopleListItem = require('./people-list-item') + +const { POSSIBLE_PERMS, PERMS_PER_ROLE } = require('../util/implicit-perms') -const POSSIBLE_ROLES = [ - 'canViewEditor', - 'canCreateDrafts', - 'canDeleteDrafts', - 'canPreviewDrafts', - 'canViewStatsPage', - 'canViewSystemStats', - 'canViewAdminPage', -] - -const NO_SELECTION_USER = 'no-select-user' -const NO_SELECTION_MODULE = 'no-select-module' const NO_SELECTION_PERMISSION = 'no-select-permission' -function Admin({ - currentUser, - title, - loadUserList, - addUserPermission, - removeUserPermission, -}) { - const [users, setUsers] = useState([]) - const [fetching, setFetching] = useState(true) - const [selectedUser, setSelectedUser] = useState(NO_SELECTION_USER) +const Admin = props => { + // storing selected user in state by choosing one from the list fetched by the search may not work + // perms attached to a user via the search didn't previously include explicitly added perms, just role perms + // that was changed but may slow down the initial fetch due to a join in the query + // may be faster to instead run a new action to fetch the single user explicitly, which will get all perms + // but that would put the selected user in props, as opposed to doing this + const [selectedUser, setSelectedUser] = useState(null) const [selectedPermission, setSelectedPermission] = useState(NO_SELECTION_PERMISSION) + // make sure user/module search results are zero'd out on first render useEffect(() => { - // Load all modules on this obo instance - loadAllUsers() - .then(() => setFetching(null)) + props.clearPeopleSearchResults() }, []) - const loadAllUsers = () => { - return new Promise(async resolve => { - const allUsers = await loadUserList() - if (allUsers && - allUsers.payload && - allUsers.payload.value && - allUsers.payload.value && - allUsers.payload.value.length > 0) { - - setUsers(allUsers.payload.value) - }else { - setUsers([]) - } - - resolve() - }) - } - const onAddUserPermission = async () => { - if (selectedUser === NO_SELECTION_USER) return if (selectedPermission === NO_SELECTION_PERMISSION) return - await addUserPermission(selectedUser, selectedPermission) + const action = await props.addUserPermission(selectedUser.id, selectedPermission) + if (action.payload.status === 'ok') setSelectedUser(action.payload.value) } const onRemoveUserPermission = async () => { - if (selectedUser === NO_SELECTION_USER) return if (selectedPermission === NO_SELECTION_PERMISSION) return - await removeUserPermission(selectedUser, selectedPermission) + const action = await props.removeUserPermission(selectedUser.id, selectedPermission) + if (action.payload.status === 'ok') setSelectedUser(action.payload.value) } const renderPermissionSelectList = () => { @@ -78,78 +48,128 @@ function Admin({ value={selectedPermission} onChange={e => setSelectedPermission(e.target.value)} > - - {POSSIBLE_ROLES.map((r, i) => { + {POSSIBLE_PERMS.map((r, i) => { return ( - + ) })} ) } - const renderUserSelectList = () => { + const renderListItem = u => { + if (u.id !== props.currentUser.id) { + return ( + + + + ) + } + } + + const renderContent = () => { + if (selectedUser) { + const allRolePerms = new Set() + const rolePermsRender = selectedUser.roles.map(role => { + const roles = PERMS_PER_ROLE[role].map(perm => { + allRolePerms.add(perm) + + return
  • {perm}
  • + }) + return ( +
    + + +
    + ) + }) + + const nonRolePerms = selectedUser.perms + .filter(p => !allRolePerms.has(p)) + .map(p =>
  • {p}
  • ) + + return ( + +
    +

    Managing:

    + +
    +
    +

    Manage User Permissions

    +
    + {selectedUser.roles.length > 0 ? ( + + + Current implicit permissions from role{selectedUser.roles.length > 1 ? 's' : ''} + : + + {rolePermsRender} + + ) : ( + 'None' + )} +
    +
    + Current explicit permissions: + {nonRolePerms.length < 1 ? 'None' :
      {nonRolePerms}
    } +
    +
    +

    Select permission:

    + {renderPermissionSelectList()} +
    +
    + + +
    +
    +
    + ) + } return ( - + +
    +

    Find Users to Manage

    + +
    +
    + +
    +
    ) } - // Contains any tools involving modifying module access, user permission, etc - const getTools = () => [ -
    -

    Manage User Permissions

    -
    -

    Select user:

    - {renderUserSelectList()} -
    -
    -

    Select permission:

    - {renderPermissionSelectList()} -
    -
    - - -
    -
    , - ] - return ( - +
    -
    - {fetching ?

    Loading...

    : getTools()} -
    +
    {renderContent()}
    ) diff --git a/packages/app/obojobo-repository/shared/components/admin.scss b/packages/app/obojobo-repository/shared/components/admin.scss index bcc0f6ff49..03367097db 100644 --- a/packages/app/obojobo-repository/shared/components/admin.scss +++ b/packages/app/obojobo-repository/shared/components/admin.scss @@ -35,11 +35,6 @@ min-width: 30%; } - .repository--button { - margin-top: 20px; - margin-bottom: 20px; - } - .row { display: flex; align-items: center; diff --git a/packages/app/obojobo-repository/shared/components/admin.test.js b/packages/app/obojobo-repository/shared/components/admin.test.js index 33ce001a84..819dae0806 100644 --- a/packages/app/obojobo-repository/shared/components/admin.test.js +++ b/packages/app/obojobo-repository/shared/components/admin.test.js @@ -2,16 +2,34 @@ import React from 'react' import { create, act } from 'react-test-renderer' // import Button from './button' +jest.mock('./people-list-item', () => props => { + return {props.children} +}) +import PeopleListItem from './people-list-item' + +jest.mock('./search', () => props => { + return {props.children} +}) +import Search from './search' + +jest.mock('../util/implicit-perms', () => ({ + POSSIBLE_PERMS: ['perm1', 'perm2', 'perm3', 'perm4', 'mockExtraPerm'], + PERMS_PER_ROLE: { + role1: ['perm1', 'perm2', 'perm3'], + role2: ['perm4'] + } +})) + import Admin from './admin' describe('Admin', () => { - beforeEach(() => { + beforeEach(() => { jest.resetAllMocks() }) - const getTestProps = () => ({ - currentUser: { - id: 99, + const defaultProps = { + currentUser: { + id: 99, avatarUrl: '/path/to/avatar', firstName: 'firstName', lastName: 'lastName', @@ -22,13 +40,425 @@ describe('Admin', () => { 'canCreateDrafts', 'canPreviewDrafts' ] - }, - loadUserList: jest.fn() - }) - - test('Renders "tools loaded" state correctly', () => { - const component = create() - const tree = component.toJSON() - expect(tree).toMatchSnapshot() - }) -}) \ No newline at end of file + }, + addUserPermission: jest.fn(), + removeUserPermission: jest.fn(), + searchForUser: jest.fn(), + clearPeopleSearchResults: jest.fn() + } + + test('renders default view correctly', () => { + const component = create() + + // should only be a search input visible by default, no search results + expect(component.root.findAllByType(PeopleListItem).length).toBe(0) + expect(component.root.findByProps({ className: 'user-list' }).children.length).toBe(0) + + const tree = component.toJSON() + expect(tree).toMatchSnapshot() + }) + + test('renders user search results correctly', () => { + const newProps = { ...defaultProps } + newProps.searchUsers = { + items: [ + { id: 1, firstName: 'Mock', lastName: 'User1' }, + { id: 2, firstName: 'Mock', lastName: 'User2' }, + { id: 3, firstName: 'Mock', lastName: 'User3' }, + // just to make sure the current user doesn't show up in the list + { ...defaultProps.currentUser } + ] + } + + const component = create() + const searchResults = component.root.findAllByType(PeopleListItem) + expect(searchResults.length).toBe(3) + // bit inelegant but does the job + searchResults.forEach(i => { + expect(i.props.isMe).toBe(false) + expect(i.props.firstName).not.toBe(defaultProps.currentUser.firstName) + expect(i.props.lastName).not.toBe(defaultProps.currentUser.lastName) + }) + }) + + test('calls functions appropriately when search value changes', () => { + const component = create() + act(() => { + // ordinarily there'd be targets and values to parse, but the Search component does that for us + component.root.findByType(Search).props.onChange('mock-new-search-value') + }) + + expect(defaultProps.searchForUser).toHaveBeenCalledTimes(1) + expect(defaultProps.searchForUser).toHaveBeenCalledWith('mock-new-search-value') + }) + + test('sets selected user correctly - no roles or explicit perms', () => { + const newProps = { ...defaultProps } + newProps.searchUsers = { + items: [ + { + id: 1, + firstName: 'Mock', + lastName: 'User1', + roles: [], + perms: [] + } + ] + } + + const component = create() + + // pre-selection state - make sure the search tools are visible + expect(component.root.findAllByType('mock-Search').length).toBe(1) + expect(component.root.findAllByProps({ className: 'tool' }).length).toBe(0) + + act(() => { + component.root.findByType(PeopleListItem).children[0].children[0].props.onClick() + }) + + // post-selection state - make sure the search tools are no longer available + expect(component.root.findAllByType('mock-Search').length).toBe(0) + expect(component.root.findAllByProps({ className: 'tool' }).length).toBe(1) + + const implicitContainer = component.root.findByProps({ className: 'implicit-perms-container' }) + expect(implicitContainer.findAllByType('ul').length).toBe(0) + expect(implicitContainer.children[implicitContainer.children.length - 1]).toBe('None') + + const explicitContainer = component.root.findByProps({ className: 'explicit-perms-container' }) + expect(explicitContainer.findAllByType('ul').length).toBe(0) + expect(explicitContainer.children[explicitContainer.children.length - 1]).toBe('None') + + // for good measure, make sure the permissions list is populated correctly and the add/remove buttons appear + // this is a bit brute force-y, but at least we know for sure + // also a bit magical since we happen to know what the mocked values are + const expectedSelectOptions = [ + 'no-select-permission', // this will always be there + 'perm1', + 'perm2', + 'perm3', + 'perm4', + 'mockExtraPerm' + ] + const permSelect = component.root.findByType('select') + expectedSelectOptions.forEach((v, i) => { + expect(permSelect.findAllByType('option')[i].props.value).toBe(v) + }) + + const buttons = component.root.findAllByProps({ className: 'tool-button' }) + expect(buttons.length).toBe(2) + expect(buttons[0].children[0].children[0]).toBe('Add') + expect(buttons[1].children[0].children[0]).toBe('Remove') + }) + + test('sets selected user correctly - one role and explicit perms', () => { + const newProps = { ...defaultProps } + newProps.searchUsers = { + items: [ + { + id: 1, + firstName: 'Mock', + lastName: 'User1', + roles: ['role1'], + perms: ['perm1', 'perm2', 'perm3', 'mockExtraPerm'] + } + ] + } + + const component = create() + + act(() => { + component.root.findByType(PeopleListItem).children[0].children[0].props.onClick() + }) + + const implicitContainer = component.root.findByProps({ className: 'implicit-perms-container' }) + // the 'single' rendered string is actually three strings, since the singular/plural part is interpolated + // checking to make sure 'role' isn't 'roles' since any number greater than one requires the extra 's' + expect(implicitContainer.findByType('span').children).toEqual([ + 'Current implicit permissions from role', + '', + ':' + ]) + expect(implicitContainer.findAllByType('ul').length).toBe(1) + // we happen to know what this should be based on our mock of the role-based perms at the top of this file + // ...but ideally this shouldn't be a magical number + expect(implicitContainer.findByType('ul').children.length).toBe(3) + + const explicitContainer = component.root.findByProps({ className: 'explicit-perms-container' }) + expect(explicitContainer.findByType('ul').children.length).toBe(1) + expect(explicitContainer.findByType('ul').children[0].children[0]).toBe('mockExtraPerm') + }) + + test('sets selected user correctly - multiple roles', () => { + const newProps = { ...defaultProps } + newProps.searchUsers = { + items: [ + { + id: 1, + firstName: 'Mock', + lastName: 'User1', + roles: ['role1', 'role2'], + perms: ['perm1', 'perm2', 'perm3', 'perm4', 'mockExtraPerm'] + } + ] + } + + const component = create() + + act(() => { + component.root.findByType(PeopleListItem).children[0].children[0].props.onClick() + }) + + const implicitContainer = component.root.findByProps({ className: 'implicit-perms-container' }) + + expect(implicitContainer.findByType('span').children).toEqual([ + 'Current implicit permissions from role', + 's', + ':' + ]) + expect(implicitContainer.findAllByType('ul').length).toBe(2) + // same situation - we know what order these will be in because we mocked them to show up this way, but... magical + expect(implicitContainer.findAllByType('ul')[0].children.length).toBe(3) + expect(implicitContainer.findAllByType('ul')[0].children[0].children[0]).toBe('perm1') + expect(implicitContainer.findAllByType('ul')[0].children[1].children[0]).toBe('perm2') + expect(implicitContainer.findAllByType('ul')[0].children[2].children[0]).toBe('perm3') + + expect(implicitContainer.findAllByType('ul')[1].children.length).toBe(1) + expect(implicitContainer.findAllByType('ul')[1].children[0].children[0]).toBe('perm4') + + const explicitContainer = component.root.findByProps({ className: 'explicit-perms-container' }) + expect(explicitContainer.findByType('ul').children.length).toBe(1) + expect(explicitContainer.findByType('ul').children[0].children[0]).toBe('mockExtraPerm') + }) + + test('does nothing when trying to add or remove before selecting a valid permission', () => { + const newProps = { ...defaultProps } + newProps.searchUsers = { + items: [ + { + id: 1, + firstName: 'Mock', + lastName: 'User1', + roles: ['role1'], + perms: ['perm1', 'perm2', 'perm3'] + } + ] + } + + const component = create() + + act(() => { + component.root.findByType(PeopleListItem).children[0].children[0].props.onClick() + }) + + // without selecting a new permission, try adding and removing to make sure they don't do anything + const buttons = component.root.findAllByProps({ className: 'tool-button' }) + + // clicking the 'Add' button should do nothing + act(() => { + buttons[0].props.onClick() + }) + expect(newProps.addUserPermission).not.toHaveBeenCalled() + + // clicking the 'Remove' button should do nothing + act(() => { + buttons[1].props.onClick() + }) + expect(newProps.removeUserPermission).not.toHaveBeenCalled() + }) + + test('sends the correct permission to the API utility when adding - success response', async () => { + const mockUser = { + id: 43, + firstName: 'Mock', + lastName: 'User1', + roles: ['role1'], + perms: ['perm1', 'perm2', 'perm3'] + } + + const newProps = { ...defaultProps } + newProps.searchUsers = { + items: [{ ...mockUser }] + } + + newProps.addUserPermission = jest.fn().mockResolvedValueOnce({ + payload: { + status: 'ok', + value: { ...mockUser, perms: ['perm1', 'perm2', 'perm3', 'perm4'] } + } + }) + + const component = create() + + act(() => { + component.root.findByType(PeopleListItem).children[0].children[0].props.onClick() + }) + + // make sure there are no explicit perms before we add one + const explicitContainer = component.root.findByProps({ className: 'explicit-perms-container' }) + expect(explicitContainer.findAllByType('ul').length).toBe(0) + + // as a bonus we're also making sure the permission selection update works properly + // since there's no real way of checking that independently + // make sure clicking the 'Add' button calls functions appropriately + const permSelect = component.root.findByType('select') + const buttons = component.root.findAllByProps({ className: 'tool-button' }) + act(() => { + // ordinarily this value would come from the selected option attached to the change event + // we can just simulate the change event's target's value here + const mockChangeEvent = { target: { value: 'perm4' } } + permSelect.props.onChange(mockChangeEvent) + }) + await act(async () => { + await buttons[0].props.onClick() + }) + + expect(newProps.addUserPermission).toHaveBeenCalledTimes(1) + expect(newProps.addUserPermission).toHaveBeenCalledWith(mockUser.id, 'perm4') + + // on success, the implicit/explicit perms areas should update with any changes + // we added an explicit change this time so we can check that + expect(explicitContainer.findByType('ul').children.length).toBe(1) + expect(explicitContainer.findByType('ul').children[0].children[0]).toBe('perm4') + }) + test('sends the correct permission to the API utility when adding - non-success response', async () => { + const mockUser = { + id: 43, + firstName: 'Mock', + lastName: 'User1', + roles: ['role1'], + perms: ['perm1', 'perm2', 'perm3'] + } + + const newProps = { ...defaultProps } + newProps.searchUsers = { + items: [{ ...mockUser }] + } + + newProps.addUserPermission = jest.fn().mockResolvedValueOnce({ + payload: { status: 'not-ok' } + }) + + const component = create() + + act(() => { + component.root.findByType(PeopleListItem).children[0].children[0].props.onClick() + }) + + // make sure there are no explicit perms before we try adding one + const explicitContainer = component.root.findByProps({ className: 'explicit-perms-container' }) + expect(explicitContainer.findAllByType('ul').length).toBe(0) + + const permSelect = component.root.findByType('select') + const buttons = component.root.findAllByProps({ className: 'tool-button' }) + act(() => { + const mockChangeEvent = { target: { value: 'perm4' } } + permSelect.props.onChange(mockChangeEvent) + }) + await act(async () => { + await buttons[0].props.onClick() + }) + + expect(newProps.addUserPermission).toHaveBeenCalledTimes(1) + expect(newProps.addUserPermission).toHaveBeenCalledWith(mockUser.id, 'perm4') + + // response wasn't successful, make sure nothing changed + expect(explicitContainer.findAllByType('ul').length).toBe(0) + }) + + test('sends the correct permission to the API utility when removing - success response', async () => { + const mockUser = { + id: 43, + firstName: 'Mock', + lastName: 'User1', + roles: ['role1'], + perms: ['perm1', 'perm2', 'perm3', 'perm4'] + } + + const newProps = { ...defaultProps } + newProps.searchUsers = { + items: [{ ...mockUser }] + } + + newProps.removeUserPermission = jest.fn().mockResolvedValueOnce({ + payload: { + status: 'ok', + value: { ...mockUser, perms: ['perm1', 'perm2', 'perm3'] } + } + }) + + const component = create() + + act(() => { + component.root.findByType(PeopleListItem).children[0].children[0].props.onClick() + }) + + // make sure the single explicit perm is shown before we remove it + const explicitContainer = component.root.findByProps({ className: 'explicit-perms-container' }) + expect(explicitContainer.findByType('ul').children.length).toBe(1) + expect(explicitContainer.findByType('ul').children[0].children[0]).toBe('perm4') + + // make sure clicking the 'Remove' button calls functions appropriately + const permSelect = component.root.findByType('select') + const buttons = component.root.findAllByProps({ className: 'tool-button' }) + act(() => { + const mockChangeEvent = { target: { value: 'perm4' } } + permSelect.props.onChange(mockChangeEvent) + }) + await act(async () => { + await buttons[1].props.onClick() + }) + + expect(newProps.removeUserPermission).toHaveBeenCalledTimes(1) + expect(newProps.removeUserPermission).toHaveBeenCalledWith(mockUser.id, 'perm4') + + // we removed the only explicit perm, make sure that's reflected properly + expect(explicitContainer.findAllByType('ul').length).toBe(0) + }) + test('sends the correct permission to the API utility when removing - success response', async () => { + const mockUser = { + id: 43, + firstName: 'Mock', + lastName: 'User1', + roles: ['role1'], + perms: ['perm1', 'perm2', 'perm3', 'perm4'] + } + + const newProps = { ...defaultProps } + newProps.searchUsers = { + items: [{ ...mockUser }] + } + + newProps.removeUserPermission = jest.fn().mockResolvedValueOnce({ + payload: { + status: 'not-ok' + } + }) + + const component = create() + + act(() => { + component.root.findByType(PeopleListItem).children[0].children[0].props.onClick() + }) + + const explicitContainer = component.root.findByProps({ className: 'explicit-perms-container' }) + expect(explicitContainer.findByType('ul').children.length).toBe(1) + expect(explicitContainer.findByType('ul').children[0].children[0]).toBe('perm4') + + const permSelect = component.root.findByType('select') + const buttons = component.root.findAllByProps({ className: 'tool-button' }) + act(() => { + const mockChangeEvent = { target: { value: 'perm4' } } + permSelect.props.onChange(mockChangeEvent) + }) + await act(async () => { + await buttons[1].props.onClick() + }) + + expect(newProps.removeUserPermission).toHaveBeenCalledTimes(1) + expect(newProps.removeUserPermission).toHaveBeenCalledWith(mockUser.id, 'perm4') + + // response wasn't successful, make sure nothing changed + expect(explicitContainer.findByType('ul').children.length).toBe(1) + expect(explicitContainer.findByType('ul').children[0].children[0]).toBe('perm4') + }) +}) diff --git a/packages/app/obojobo-repository/shared/reducers/admin-reducer.js b/packages/app/obojobo-repository/shared/reducers/admin-reducer.js index ad48af3a48..5b4f05f5c8 100644 --- a/packages/app/obojobo-repository/shared/reducers/admin-reducer.js +++ b/packages/app/obojobo-repository/shared/reducers/admin-reducer.js @@ -1,8 +1,18 @@ +const { handle } = require('redux-pack') + +const { LOAD_USER_SEARCH } = require('../actions/admin-actions') + function AdminReducer(state, action) { - switch (action.type) { - default: - return state - } + switch (action.type) { + case LOAD_USER_SEARCH: + return handle(state, action, { + start: prevState => ({ ...prevState, userSearchString: action.meta.searchString }), + success: prevState => ({ ...prevState, searchUsers: { items: action.payload.value } }) + }) + + default: + return state + } } -module.exports = AdminReducer \ No newline at end of file +module.exports = AdminReducer diff --git a/packages/app/obojobo-repository/shared/reducers/admin-reducer.test.js b/packages/app/obojobo-repository/shared/reducers/admin-reducer.test.js index 123059bd44..823cbe7d0d 100644 --- a/packages/app/obojobo-repository/shared/reducers/admin-reducer.test.js +++ b/packages/app/obojobo-repository/shared/reducers/admin-reducer.test.js @@ -1,13 +1,84 @@ +jest.mock('redux-pack', () => { + return { + handle: jest.fn((prevState, action, steps) => ({ prevState, action, steps })) + } +}) +const Pack = require('redux-pack') + +const { LOAD_USER_SEARCH } = require('../actions/admin-actions') + const adminReducer = require('./admin-reducer') +const handleStart = handler => { + return handler.steps.start(handler.prevState) +} +const handleSuccess = handler => { + return handler.steps.success(handler.prevState) +} + describe('Admin Reducer', () => { - test('Admin actions in reducer return the original state', () => { - const initialState = {} - - const action = { - type: 'test-admin-action' - } - - expect(adminReducer(initialState, action)).toBe(initialState) - }) -}) \ No newline at end of file + const defaultSearchResultsState = { + isFetching: false, + hasFetched: false, + items: [] + } + + beforeEach(() => { + Pack.handle.mockClear() + }) + + test('LOAD_USER_SEARCH action modifies state correctly', () => { + const initialState = { + userSearchString: 'oldSearchString', + searchUsers: { ...defaultSearchResultsState } + } + + const mockUserList = [ + { + id: 0, + displayName: 'firstName lastName' + }, + { + id: 1, + displayName: 'firstName2 lastName2' + } + ] + const action = { + type: LOAD_USER_SEARCH, + meta: { + searchString: 'newSearchString' + }, + payload: { + value: mockUserList + } + } + + // asynchronous action - state changes on success + const handler = adminReducer(initialState, action) + let newState + + newState = handleStart(handler) + expect(newState.userSearchString).toEqual('newSearchString') + expect(newState.searchUsers).toEqual(initialState.searchUsers) + + newState = handleSuccess(handler) + expect(newState.searchUsers).not.toEqual(initialState.searchUsers) + expect(newState.searchUsers).toEqual({ + items: mockUserList + }) + }) + + test('unrecognized action types just return the current state', () => { + const initialState = { + key: 'initialValue' + } + + const action = { + type: 'UNSUPPORTED_TYPE', + key: 'someOtherValue' + } + + const newState = adminReducer(initialState, action) + expect(newState.key).toEqual(initialState.key) + }) +}) diff --git a/packages/app/obojobo-repository/shared/util/implicit-perms.js b/packages/app/obojobo-repository/shared/util/implicit-perms.js new file mode 100644 index 0000000000..d809f2527a --- /dev/null +++ b/packages/app/obojobo-repository/shared/util/implicit-perms.js @@ -0,0 +1,17 @@ +// translate all possible perm options from a list and collate them per role +const defaultPermissions = require('obojobo-express/server/config/permission_groups.json').default + +const POSSIBLE_PERMS = Object.keys(defaultPermissions) +const PERMS_PER_ROLE = {} +POSSIBLE_PERMS.forEach(p => { + const roles = defaultPermissions[p] + roles.forEach(r => { + if (!PERMS_PER_ROLE[r]) PERMS_PER_ROLE[r] = [] + PERMS_PER_ROLE[r].push(p) + }) +}) + +module.exports = { + POSSIBLE_PERMS, + PERMS_PER_ROLE +} diff --git a/packages/app/obojobo-repository/shared/util/implicit-perms.test.js b/packages/app/obojobo-repository/shared/util/implicit-perms.test.js new file mode 100644 index 0000000000..bb40ee11bd --- /dev/null +++ b/packages/app/obojobo-repository/shared/util/implicit-perms.test.js @@ -0,0 +1,31 @@ +const mockStructureAsObject = { + mockPerm1: [ + 'Role1', + 'Role2', + 'urn:lti:role:standard/lis/Role1', + 'urn:lti:role:standard/lis/Role2' + ], + mockPerm2: ['Role1', 'Role2', 'urn:lti:role:standard/lis/Role1'], + mockPerm3: ['Role1', 'urn:lti:role:standard/lis/Role1'], + mockPerm4: ['Role1'] +} + +const { POSSIBLE_PERMS, PERMS_PER_ROLE } = require('./implicit-perms') + +jest.mock('obojobo-express/server/config/permission_groups.json', () => ({ + default: mockStructureAsObject +})) + +describe('implicit permission collation shortcut', () => { + test('POSSIBLE_PERMS and PERMS_PER_ROLE are generated correctly based on default role/perm associations', () => { + const expectedPossiblePerms = ['mockPerm1', 'mockPerm2', 'mockPerm3', 'mockPerm4'] + const expectedPermsPerRole = { + Role1: ['mockPerm1', 'mockPerm2', 'mockPerm3', 'mockPerm4'], + Role2: ['mockPerm1', 'mockPerm2'], + 'urn:lti:role:standard/lis/Role1': ['mockPerm1', 'mockPerm2', 'mockPerm3'], + 'urn:lti:role:standard/lis/Role2': ['mockPerm1'] + } + expect(POSSIBLE_PERMS).toEqual(expectedPossiblePerms) + expect(PERMS_PER_ROLE).toEqual(expectedPermsPerRole) + }) +})