fix: Remove delete user feature from UI and handle study permissions …

…which have stale users (#595)

* fix: Remove delete user feature from UI and handle study permissions which have stale users

* chore: (PR feedback) deleted the commented line

addons/addon-base-raas-ui/packages/base-raas-ui/src/models/studies/StudyPermissionsStore.js

@@ -55,22 +55,27 @@ const StudyPermissionsStore = BaseStore.named('StudyPermissionsStore')
         superCleanup();
       },
 
-      update: async selectedUserIds => {
+      update: async (selectedUserIds, staleUserIds) => {
         const updateRequest = { usersToAdd: [], usersToRemove: [] };
 
         const parent = getParent(self, 1);
         parent.userTypes.forEach(type => {
           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(
             ..._.differenceWith(self.studyPermissions[`${type}Users`], selectedUserIds[type], _.isEqual).map(
               userToRequestFormat,
             ),
           );
+          // Add staleUserIds to usersToRemove
+          updateRequest.usersToRemove.push(..._.map(staleUserIds[type], userToRequestFormat));
         });
 
         // Perform update and reload store

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
         }
       });

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 };

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);
+    });
+  });
+});

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 {
       <div className="mt4 mb4">
         <Modal.Actions>
           {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;

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']);
+    });
   });
 });

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);

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));
+            }
           }
         }
       });

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