diff --git a/addons/addon-base-raas-ui/packages/base-raas-ui/src/models/studies/StudyPermissionsStore.js b/addons/addon-base-raas-ui/packages/base-raas-ui/src/models/studies/StudyPermissionsStore.js index 0414a2d42b..1e525cbcae 100644 --- a/addons/addon-base-raas-ui/packages/base-raas-ui/src/models/studies/StudyPermissionsStore.js +++ b/addons/addon-base-raas-ui/packages/base-raas-ui/src/models/studies/StudyPermissionsStore.js @@ -55,7 +55,7 @@ const StudyPermissionsStore = BaseStore.named('StudyPermissionsStore') superCleanup(); }, - update: async selectedUserIds => { + update: async (selectedUserIds, staleUserIds) => { const updateRequest = { usersToAdd: [], usersToRemove: [] }; const parent = getParent(self, 1); @@ -63,7 +63,10 @@ const StudyPermissionsStore = BaseStore.named('StudyPermissionsStore') const userToRequestFormat = uid => ({ uid, permissionLevel: type }); // Set selected users as "usersToAdd" (API is idempotent) - updateRequest.usersToAdd.push(..._.map(selectedUserIds[type], userToRequestFormat)); + // And remove staleUserIds from usersToAdd list + updateRequest.usersToAdd.push( + ..._.differenceWith(selectedUserIds[type], staleUserIds[type], _.isEqual).map(userToRequestFormat), + ); // Set removed users as "usersToRemove" updateRequest.usersToRemove.push( @@ -71,6 +74,8 @@ const StudyPermissionsStore = BaseStore.named('StudyPermissionsStore') userToRequestFormat, ), ); + // Add staleUserIds to usersToRemove + updateRequest.usersToRemove.push(..._.map(staleUserIds[type], userToRequestFormat)); }); // Perform update and reload store diff --git a/addons/addon-base-raas-ui/packages/base-raas-ui/src/models/users/UsersStore.js b/addons/addon-base-raas-ui/packages/base-raas-ui/src/models/users/UsersStore.js index 248a2d72e5..13f5ed5a07 100644 --- a/addons/addon-base-raas-ui/packages/base-raas-ui/src/models/users/UsersStore.js +++ b/addons/addon-base-raas-ui/packages/base-raas-ui/src/models/users/UsersStore.js @@ -158,7 +158,17 @@ const UsersStore = BaseStore.named('UsersStore') if (user) { result.push(user); } else { - result.push(User.create(getSnapshot(userIdentifier))); + let userSnapshot; + try { + userSnapshot = getSnapshot(userIdentifier); + } catch (error) { + // Note that user might be already deleted. In order to prevent UI from crashing log the error instead + // and not add it to User list + console.log(`User ${userIdentifier.uid} doesn't exist`, error); + } + if (userSnapshot) { + result.push(User.create(userSnapshot)); + } } // this could happen in the employee is no longer active or with the company } }); diff --git a/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/studies/StudyPermissionsTable.js b/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/studies/StudyPermissionsTable.js index 340c269683..1e91be1866 100644 --- a/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/studies/StudyPermissionsTable.js +++ b/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/studies/StudyPermissionsTable.js @@ -59,8 +59,10 @@ class StudyPermissionsTable extends React.Component { // Set users who currently have permission to the study as the selected users this.study.userTypes.forEach(userType => { this.selectedUserIds[userType] = this.permissionsStore.studyPermissions[`${userType}Users`]; - }); + // determine staleUserIds + this.staleUserIds[userType] = getStaleUsers(this.selectedUserIds[userType], this.usersStore); + }); // Show edit dropdowns via observable this.editModeOn = true; }; @@ -69,6 +71,7 @@ class StudyPermissionsTable extends React.Component { this.editModeOn = false; this.isProcessing = false; this.selectedUserIds = {}; + this.staleUserIds = {}; }; submitUpdate = async () => { @@ -78,7 +81,7 @@ class StudyPermissionsTable extends React.Component { // Perform update try { - await this.permissionsStore.update(this.selectedUserIds); + await this.permissionsStore.update(this.selectedUserIds, this.staleUserIds); displaySuccess('Update Succeeded'); runInAction(() => { this.resetForm(); @@ -195,13 +198,28 @@ class StudyPermissionsTable extends React.Component { } } +function getStaleUsers(selectedUserIds, usersStore) { + const userIdentifiers = _.map(selectedUserIds, uid => ({ uid })); + const users = usersStore.asUserObjects(userIdentifiers); + const userIdDict = Object.assign({}, ...users.map(user => ({ [user.id]: user }))); + const invalidUserIds = []; + selectedUserIds.forEach(userId => { + if (!(userId in userIdDict)) { + invalidUserIds.push(userId); + } + }); + return invalidUserIds; +} + decorate(StudyPermissionsTable, { editModeOn: observable, isProcessing: observable, selectedUserIds: observable, + staleUserIds: observable, enableEditMode: action, resetForm: action, submitUpdate: action, }); export default inject('userStore', 'usersStore')(observer(StudyPermissionsTable)); +export { getStaleUsers }; diff --git a/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/studies/__tests__/StudyPermissionsTable.test.js b/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/studies/__tests__/StudyPermissionsTable.test.js new file mode 100644 index 0000000000..23d41d6c94 --- /dev/null +++ b/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/studies/__tests__/StudyPermissionsTable.test.js @@ -0,0 +1,35 @@ +import { getStaleUsers } from '../StudyPermissionsTable'; + +describe('StudyPermissionsTable tests', () => { + describe('getStaleUsers', () => { + it('Test stale users when no users are stale', async () => { + const usersStore = {}; + usersStore.asUserObjects = jest.fn().mockImplementationOnce(() => { + return [{ id: 1 }, { id: 2 }]; + }); + const staleUsers = getStaleUsers([1, 2], usersStore); + expect(staleUsers).toEqual([]); + expect(usersStore.asUserObjects).toHaveBeenCalledTimes(1); + }); + + it('Test stale users when all users are stale', async () => { + const usersStore = {}; + usersStore.asUserObjects = jest.fn().mockImplementationOnce(() => { + return []; + }); + const staleUsers = getStaleUsers([1, 2], usersStore); + expect(staleUsers).toEqual([1, 2]); + expect(usersStore.asUserObjects).toHaveBeenCalledTimes(1); + }); + + it('Test stale users when one user is stale', async () => { + const usersStore = {}; + usersStore.asUserObjects = jest.fn().mockImplementationOnce(() => { + return [{ id: 1 }, { id: 4 }, { id: 3 }]; + }); + const staleUsers = getStaleUsers([1, 2, 4, 3], usersStore); + expect(staleUsers).toEqual([2]); + expect(usersStore.asUserObjects).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/users/UpdateUser.js b/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/users/UpdateUser.js index 6d5ba0176d..abd8533786 100644 --- a/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/users/UpdateUser.js +++ b/addons/addon-base-raas-ui/packages/base-raas-ui/src/parts/users/UpdateUser.js @@ -149,18 +149,6 @@ class UpdateUser extends React.Component { disabled: this.processing, }); - const deleteButton = - // TODO: deletion actions should be confirmed by user first - this.view === 'detail' - ? makeButton({ - label: 'Delete', - floated: 'right', - color: 'red', - onClick: this.handleDeleteClick, - disabled: currentUser.isRootUser || this.processing, - }) - : ''; - const activeButton = this.props.user.status === 'pending' || this.props.user.status === 'inactive' ? makeButton({ @@ -191,7 +179,6 @@ class UpdateUser extends React.Component {
{cancelButton} - {deleteButton} {deactiveButton} {activeButton} {editButton} @@ -380,19 +367,6 @@ class UpdateUser extends React.Component { } }; - handleDeleteClick = async () => { - try { - this.processing = true; - await this.usersStore.deleteUser(this.getCurrentUser()); - } catch (error) { - displayError(error); - } - runInAction(() => { - this.processing = false; - }); - this.handleClose(); - }; - handleApproveDisapproveClick = async status => { try { this.processing = true; diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/study/__tests__/study-permission-service.test.js b/addons/addon-base-raas/packages/base-raas-services/lib/study/__tests__/study-permission-service.test.js index 33a008941e..385c220586 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/study/__tests__/study-permission-service.test.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/study/__tests__/study-permission-service.test.js @@ -42,6 +42,7 @@ const { getEmptyUserPermissions } = require('../helpers/entities/user-permission describe('StudyPermissionService', () => { let service; let dbService; + let lockService; beforeEach(async () => { // Initialize services container and register dependencies @@ -62,6 +63,7 @@ describe('StudyPermissionService', () => { service = await container.find('studyPermissionService'); dbService = await container.find('dbService'); + lockService = await container.find('lockService'); }); describe('findStudyPermissions', () => { @@ -602,5 +604,72 @@ describe('StudyPermissionService', () => { expect.objectContaining({ boom: true, code: 'badRequest', safe: true, message: 'Input has validation errors' }), ); }); + + it('should fail since the user id to add belongs to usersToAdd and fails assertValidUsers', async () => { + // BUILD + const uid = 'u-admin1'; + const requestContext = { + principalIdentifier: { uid }, + principal: { userRole: 'researcher', status: 'active' }, + }; + const studyEntity = { + id: 'study1', + }; + const updateRequest = { + usersToAdd: [{ uid: 'uid-1', permissionLevel: 'readonly' }], + usersToRemove: [{ uid: 'uid-2', permissionLevel: 'readwrite' }], + }; + lockService.tryWriteLockAndRun = jest.fn((params, callback) => callback()); + service.findStudyPermissions = jest.fn().mockImplementationOnce(() => { + return { + adminUsers: ['u-admin1'], + readonlyUsers: [], + readwriteUsers: ['uid-2'], + writeonlyUsers: [], + }; + }); + service.assertValidUsers = jest.fn().mockImplementationOnce(() => { + throw Error('Invalid Users'); + }); + // OPERATE + await expect(service.update(requestContext, studyEntity, updateRequest)).rejects.toThrow( + // CHECK + expect.objectContaining({ message: 'Invalid Users' }), + ); + expect(service.findStudyPermissions).toHaveBeenCalledWith(requestContext, studyEntity); + expect(service.findStudyPermissions).toHaveBeenCalledTimes(1); + expect(service.assertValidUsers).toHaveBeenCalledWith(['uid-1']); + expect(service.assertValidUsers).toHaveBeenCalledTimes(1); + }); + + it('should not fail assertValidUsers check since invalid user is part of usersToRemove', async () => { + // BUILD + const uid = 'u-admin1'; + const requestContext = { + principalIdentifier: { uid }, + principal: { userRole: 'researcher', status: 'active' }, + }; + const studyEntity = { + id: 'study1', + }; + const updateRequest = { + usersToAdd: [{ uid: 'uid-1', permissionLevel: 'readonly' }], + usersToRemove: [{ uid: 'uid-2', permissionLevel: 'readwrite' }], + }; + lockService.tryWriteLockAndRun = jest.fn((params, callback) => callback()); + service.findStudyPermissions = jest.fn().mockImplementationOnce(() => { + return { + adminUsers: ['u-admin1'], + readonlyUsers: [], + readwriteUsers: [], + writeonlyUsers: [], + }; + }); + service.assertValidUsers = jest.fn().mockImplementationOnce(() => {}); + // OPERATE + await service.update(requestContext, studyEntity, updateRequest); + expect(service.findStudyPermissions).toHaveBeenCalledWith(requestContext, studyEntity); + expect(service.assertValidUsers).toHaveBeenCalledWith(['uid-1']); + }); }); }); diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/study/study-permission-service.js b/addons/addon-base-raas/packages/base-raas-services/lib/study/study-permission-service.js index e4ee0f97e1..f17702c850 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/study/study-permission-service.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/study/study-permission-service.js @@ -339,7 +339,9 @@ class StudyPermissionService extends Service { applyUpdateRequest(studyPermissionsEntity, updateRequest); - const userIds = getImpactedUsers(updateRequest); + // impacted users for update are only those users who are being added. Don't validate if users being removed + // exist or not since their permissions are being removed + const userIds = _.map(updateRequest.usersToAdd, item => item.uid); if (_.size(userIds) > 100) { // To protect against a large number throw this.boom.badRequest('You can only change permissions for up to 100 users', true); diff --git a/addons/addon-base-ui/packages/base-ui/src/models/users/UsersStore.js b/addons/addon-base-ui/packages/base-ui/src/models/users/UsersStore.js index a0cc628726..f530377f42 100644 --- a/addons/addon-base-ui/packages/base-ui/src/models/users/UsersStore.js +++ b/addons/addon-base-ui/packages/base-ui/src/models/users/UsersStore.js @@ -131,7 +131,17 @@ const UsersStore = BaseStore.named('UsersStore') if (user) { result.push(user); } else { - result.push(User.create(getSnapshot(userIdentifier))); + let userSnapshot; + try { + userSnapshot = getSnapshot(userIdentifier); + } catch (error) { + // Note that user might be already deleted. In order to prevent UI from crashing log the error instead + // and not add it to User list + console.log(`User ${userIdentifier.uid} doesn't exist`, error); + } + if (userSnapshot) { + result.push(User.create(userSnapshot)); + } } } }); diff --git a/openapi.yaml b/openapi.yaml index 212df37bfb..3470bfaf7c 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -264,7 +264,9 @@ paths: summary: Delete a User operationId: DeleteUser description: | - Delete a User + Delete a User. Note that this API doesn't check for outstanding resources like studies, environments, projects + etc. the user might have permissions to. Take caution while using this API since those resources might stop + functioning as expected. Consider using update API instead and change 'state' to 'inactive'. responses: '200': description: User deleted