[web] Promisify UserMembershipController (#29189)

* Promisify UserMembershipController

* Remove nested promisification from UserMembershipHandler

* Move export to the bottom of the file

* Remove manual promises

GitOrigin-RevId: a72b9ee4973da7a04b1ffeb588bcae3db696602f

services/web/app/src/Features/UserMembership/UserMembershipController.mjs

@@ -171,96 +171,98 @@ async function exportCsv(req, res) {
   csvAttachment(res, csvParser.parse(users), 'Group.csv')
 }
 
-export default {
-  manageGroupMembers: expressify(manageGroupMembers),
-  manageGroupManagers: expressify(manageGroupManagers),
-  manageInstitutionManagers: expressify(manageInstitutionManagers),
-  managePublisherManagers: expressify(managePublisherManagers),
-  add(req, res, next) {
-    const { entity, entityConfig } = req
-    const email = EmailHelper.parseEmail(req.body.email)
-    if (email == null) {
+async function add(req, res) {
+  const { entity, entityConfig } = req
+  const email = EmailHelper.parseEmail(req.body.email)
+  if (email == null) {
+    return res.status(400).json({
+      error: {
+        code: 'invalid_email',
+        message: req.i18n.translate('invalid_email'),
+      },
+    })
+  }
+  if (entityConfig.readOnly) {
+    throw new Errors.NotFoundError('Cannot add users to entity')
+  }
+  let user
+  try {
+    user = await UserMembershipHandler.promises.addUser(
+      entity,
+      entityConfig,
+      email
+    )
+  } catch (err) {
+    if (err instanceof UserMembershipErrors.UserAlreadyAddedError) {
       return res.status(400).json({
         error: {
-          code: 'invalid_email',
-          message: req.i18n.translate('invalid_email'),
+          code: 'user_already_added',
+          message: req.i18n.translate('user_already_added'),
         },
       })
     }
-
-    if (entityConfig.readOnly) {
-      return next(new Errors.NotFoundError('Cannot add users to entity'))
+    if (err instanceof UserMembershipErrors.UserNotFoundError) {
+      return res.status(404).json({
+        error: {
+          code: 'user_not_found',
+          message: req.i18n.translate('add_manager_user_not_found'),
+        },
+      })
     }
+    throw err
+  }
+  res.json({ user })
+}
 
-    UserMembershipHandler.addUser(
+async function remove(req, res) {
+  const { entity, entityConfig } = req
+  const { userId } = req.params
+  if (entityConfig.readOnly) {
+    throw new Errors.NotFoundError('Cannot remove users from entity')
+  }
+  const loggedInUserId = SessionManager.getLoggedInUserId(req.session)
+  if (loggedInUserId === userId) {
+    return res.status(400).json({
+      error: {
+        code: 'managers_cannot_remove_self',
+        message: req.i18n.translate('managers_cannot_remove_self'),
+      },
+    })
+  }
+  try {
+    await UserMembershipHandler.promises.removeUser(
       entity,
       entityConfig,
-      email,
-      function (error, user) {
-        if (
-          error &&
-          error instanceof UserMembershipErrors.UserAlreadyAddedError
-        ) {
-          return res.status(400).json({
-            error: {
-              code: 'user_already_added',
-              message: req.i18n.translate('user_already_added'),
-            },
-          })
-        }
-        if (error && error instanceof UserMembershipErrors.UserNotFoundError) {
-          return res.status(404).json({
-            error: {
-              code: 'user_not_found',
-              message: req.i18n.translate('add_manager_user_not_found'),
-            },
-          })
-        }
-        if (error != null) {
-          return next(error)
-        }
-        res.json({ user })
-      }
+      userId
     )
-  },
-  remove(req, res, next) {
-    const { entity, entityConfig } = req
-    const { userId } = req.params
-
-    if (entityConfig.readOnly) {
-      return next(new Errors.NotFoundError('Cannot remove users from entity'))
-    }
-
-    const loggedInUserId = SessionManager.getLoggedInUserId(req.session)
-    if (loggedInUserId === userId) {
+  } catch (err) {
+    if (err instanceof UserMembershipErrors.UserIsManagerError) {
       return res.status(400).json({
         error: {
-          code: 'managers_cannot_remove_self',
-          message: req.i18n.translate('managers_cannot_remove_self'),
+          code: 'managers_cannot_remove_admin',
+          message: req.i18n.translate('managers_cannot_remove_admin'),
         },
       })
     }
+    throw err
+  }
+  res.sendStatus(200)
+}
 
-    UserMembershipHandler.removeUser(
-      entity,
-      entityConfig,
-      userId,
-      function (error, user) {
-        if (error && error instanceof UserMembershipErrors.UserIsManagerError) {
-          return res.status(400).json({
-            error: {
-              code: 'managers_cannot_remove_admin',
-              message: req.i18n.translate('managers_cannot_remove_admin'),
-            },
-          })
-        }
-        if (error != null) {
-          return next(error)
-        }
-        res.sendStatus(200)
-      }
-    )
-  },
+async function create(req, res) {
+  const entityId = req.params.id
+  const entityConfig = req.entityConfig
+  await UserMembershipHandler.promises.createEntity(entityId, entityConfig)
+  res.redirect(entityConfig.pathsFor(entityId).index)
+}
+
+export default {
+  manageGroupMembers: expressify(manageGroupMembers),
+  manageGroupManagers: expressify(manageGroupManagers),
+  manageInstitutionManagers: expressify(manageInstitutionManagers),
+  managePublisherManagers: expressify(managePublisherManagers),
+  add: expressify(add),
+  remove: expressify(remove),
   exportCsv: expressify(exportCsv),
   new(req, res, next) {
     res.render('user_membership/new', {
@@ -268,19 +270,5 @@ export default {
       entityId: req.params.id,
     })
   },
-  create(req, res, next) {
-    const entityId = req.params.id
-    const entityConfig = req.entityConfig
-
-    UserMembershipHandler.createEntity(
-      entityId,
-      entityConfig,
-      function (error, entity) {
-        if (error != null) {
-          return next(error)
-        }
-        res.redirect(entityConfig.pathsFor(entityId).index)
-      }
-    )
-  },
+  create: expressify(create),
 }

services/web/app/src/Features/UserMembership/UserMembershipHandler.mjs

@@ -1,5 +1,5 @@
 import mongodb from 'mongodb-legacy'
-import { promisifyAll, callbackify } from '@overleaf/promise-utils'
+import { callbackify } from '@overleaf/promise-utils'
 import { Institution } from '../../models/Institution.js'
 import { Subscription } from '../../models/Subscription.js'
 import { Publisher } from '../../models/Publisher.js'
@@ -52,19 +52,6 @@ const UserMembershipHandler = {
   },
 }
 
-UserMembershipHandler.promises = promisifyAll(UserMembershipHandler)
-
-export default {
-  getEntityWithoutAuthorizationCheck: callbackify(
-    UserMembershipHandler.getEntityWithoutAuthorizationCheck
-  ),
-  createEntity: callbackify(UserMembershipHandler.createEntity),
-  getUsers: callbackify(UserMembershipHandler.getUsers),
-  addUser: callbackify(UserMembershipHandler.addUser),
-  removeUser: callbackify(UserMembershipHandler.removeUser),
-  promises: UserMembershipHandler,
-}
-
 async function getPopulatedListOfMembers(entity, attributes) {
   const userObjects = []
 
@@ -113,3 +100,14 @@ function buildEntityQuery(entityId, entityConfig) {
   query[entityConfig.fields.primaryKey] = entityId
   return query
 }
+
+export default {
+  getEntityWithoutAuthorizationCheck: callbackify(
+    UserMembershipHandler.getEntityWithoutAuthorizationCheck
+  ),
+  createEntity: callbackify(UserMembershipHandler.createEntity),
+  getUsers: callbackify(UserMembershipHandler.getUsers),
+  addUser: callbackify(UserMembershipHandler.addUser),
+  removeUser: callbackify(UserMembershipHandler.removeUser),
+  promises: UserMembershipHandler,
+}

services/web/test/unit/src/UserMembership/UserMembershipController.test.mjs

@@ -132,6 +132,9 @@ describe('UserMembershipController', () => {
       ),
       promises: {
         getUsers: vi.fn().mockResolvedValue(ctx.users),
+        addUser: vi.fn().mockResolvedValue(ctx.newUser),
+        removeUser: vi.fn().mockResolvedValue(),
+        createEntity: vi.fn().mockResolvedValue(ctx.institution),
       },
     }
     ctx.SplitTestHandler = {
@@ -326,29 +329,25 @@ describe('UserMembershipController', () => {
       newUser,
     }) => {
       expect.assertions(1)
-      await new Promise(resolve => {
-        UserMembershipController.add(req, {
-          json: () => {
-            expect(UserMembershipHandler.addUser).toHaveBeenCalledWith(
-              subscription,
-              {
-                modelName: 'Subscription',
-                baseQuery: { groupPlan: true },
-                fields: {
-                  access: 'manager_ids',
-                  membership: 'member_ids',
-                  name: 'teamName',
-                  primaryKey: '_id',
-                  read: ['manager_ids'],
-                  write: 'manager_ids',
-                },
+      await UserMembershipController.add(req, {
+        json: () => {
+          expect(UserMembershipHandler.promises.addUser).toHaveBeenCalledWith(
+            subscription,
+            {
+              modelName: 'Subscription',
+              baseQuery: { groupPlan: true },
+              fields: {
+                access: 'manager_ids',
+                membership: 'member_ids',
+                name: 'teamName',
+                primaryKey: '_id',
+                read: ['manager_ids'],
+                write: 'manager_ids',
               },
-              newUser.email,
-              expect.any(Function)
-            )
-            resolve()
-          },
-        })
+            },
+            newUser.email
+          )
+        },
       })
     })
 
@@ -358,25 +357,19 @@ describe('UserMembershipController', () => {
       newUser,
     }) => {
       expect.assertions(1)
-      await new Promise(resolve => {
-        UserMembershipController.add(req, {
-          json: payload => {
-            expect(payload.user).to.equal(newUser)
-            resolve()
-          },
-        })
+      await UserMembershipController.add(req, {
+        json: payload => {
+          expect(payload.user).to.equal(newUser)
+        },
       })
     })
 
     it('handle readOnly entity', async ({ UserMembershipController, req }) => {
       expect.assertions(2)
       req.entityConfig = EntityConfigs.group
-      await new Promise(resolve => {
-        UserMembershipController.add(req, null, error => {
-          expect(error).to.exist
-          expect(error).to.be.an.instanceof(Errors.NotFoundError)
-          resolve()
-        })
+      await UserMembershipController.add(req, null, error => {
+        expect(error).to.exist
+        expect(error).to.be.an.instanceof(Errors.NotFoundError)
       })
     })
 
@@ -386,26 +379,21 @@ describe('UserMembershipController', () => {
       UserMembershipHandler,
     }) => {
       expect.assertions(1)
-      UserMembershipHandler.addUser.mockImplementation(
-        (_entity, _options, _email, callback) => {
-          callback(new UserMembershipErrors.UserAlreadyAddedError())
-        }
+      UserMembershipHandler.promises.addUser.mockRejectedValue(
+        new UserMembershipErrors.UserAlreadyAddedError()
       )
-      await new Promise(resolve => {
-        UserMembershipController.add(
-          req,
-          {
-            status: () => ({
-              json: payload => {
-                expect(payload.error.code).to.equal('user_already_added')
-                resolve()
-              },
-            }),
-          },
+      await UserMembershipController.add(
+        req,
+        {
+          status: () => ({
+            json: payload => {
+              expect(payload.error.code).to.equal('user_already_added')
+            },
+          }),
+        },
 
-          () => {}
-        )
-      })
+        () => {}
+      )
     })
 
     it('handle user not found', async ({
@@ -414,35 +402,27 @@ describe('UserMembershipController', () => {
       UserMembershipHandler,
     }) => {
       expect.assertions(1)
-      UserMembershipHandler.addUser.mockImplementation(
-        (_entity, _options, _email, callback) => {
-          callback(new UserMembershipErrors.UserNotFoundError())
-        }
+      UserMembershipHandler.promises.addUser.mockRejectedValue(
+        new UserMembershipErrors.UserNotFoundError()
       )
-      await new Promise(resolve => {
-        UserMembershipController.add(req, {
-          status: () => ({
-            json: payload => {
-              expect(payload.error.code).to.equal('user_not_found')
-              resolve()
-            },
-          }),
-        })
+      await UserMembershipController.add(req, {
+        status: () => ({
+          json: payload => {
+            expect(payload.error.code).to.equal('user_not_found')
+          },
+        }),
       })
     })
 
     it('handle invalid email', async ({ UserMembershipController, req }) => {
       expect.assertions(1)
       req.body.email = 'not_valid_email'
-      await new Promise(resolve => {
-        UserMembershipController.add(req, {
-          status: () => ({
-            json: payload => {
-              expect(payload.error.code).to.equal('invalid_email')
-              resolve()
-            },
-          }),
-        })
+      await UserMembershipController.add(req, {
+        status: () => ({
+          json: payload => {
+            expect(payload.error.code).to.equal('invalid_email')
+          },
+        }),
       })
     })
   })
@@ -464,7 +444,9 @@ describe('UserMembershipController', () => {
       expect.assertions(1)
       await UserMembershipController.remove(req, {
         sendStatus: () => {
-          expect(UserMembershipHandler.removeUser).toHaveBeenCalledWith(
+          expect(
+            UserMembershipHandler.promises.removeUser
+          ).toHaveBeenCalledWith(
             subscription,
             {
               modelName: 'Subscription',
@@ -481,8 +463,7 @@ describe('UserMembershipController', () => {
                 write: 'manager_ids',
               },
             },
-            newUser._id,
-            expect.any(Function)
+            newUser._id
           )
         },
       })
@@ -491,12 +472,9 @@ describe('UserMembershipController', () => {
     it('handle readOnly entity', async ({ UserMembershipController, req }) => {
       expect.assertions(2)
       req.entityConfig = EntityConfigs.group
-      await new Promise(resolve => {
-        UserMembershipController.remove(req, null, error => {
-          expect(error).to.exist
-          expect(error).to.be.an.instanceof(Errors.NotFoundError)
-          resolve()
-        })
+      await UserMembershipController.remove(req, null, error => {
+        expect(error).to.exist
+        expect(error).to.be.an.instanceof(Errors.NotFoundError)
       })
     })
 
@@ -522,10 +500,8 @@ describe('UserMembershipController', () => {
       UserMembershipHandler,
     }) => {
       expect.assertions(1)
-      UserMembershipHandler.removeUser.mockImplementation(
-        (_entity, _options, _userId, callback) => {
-          callback(new UserMembershipErrors.UserIsManagerError())
-        }
+      UserMembershipHandler.promises.removeUser.mockRejectedValue(
+        new UserMembershipErrors.UserIsManagerError()
       )
       await UserMembershipController.remove(req, {
         status: () => ({
@@ -671,22 +647,20 @@ describe('UserMembershipController', () => {
       await UserMembershipController.create(req, {
         redirect: path => {
           expect(path).to.eq(EntityConfigs.institution.pathsFor(123).index)
-          expect(UserMembershipHandler.createEntity).toHaveBeenCalledWith(
-            123,
-            {
-              fields: {
-                access: 'managerIds',
-                membership: 'member_ids',
-                name: 'name',
-                primaryKey: 'v1Id',
-                read: ['managerIds'],
-                write: 'managerIds',
-              },
-              modelName: 'Institution',
-              pathsFor: EntityConfigs.institution.pathsFor,
+          expect(
+            UserMembershipHandler.promises.createEntity
+          ).toHaveBeenCalledWith(123, {
+            fields: {
+              access: 'managerIds',
+              membership: 'member_ids',
+              name: 'name',
+              primaryKey: 'v1Id',
+              read: ['managerIds'],
+              write: 'managerIds',
             },
-            expect.any(Function)
-          )
+            modelName: 'Institution',
+            pathsFor: EntityConfigs.institution.pathsFor,
+          })
         },
       })
     })