Merge pull request #16788 from overleaf/dp-mongoose-callback-user-membership-handler

Promisify UserMembershipHandler and UserMembershipHandlerTests

GitOrigin-RevId: bb33110ee750364754db53fb075a5700be003ecc

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

@@ -15,7 +15,11 @@ const UserMembershipHandler = require('./UserMembershipHandler')
 const Errors = require('../Errors/Errors')
 const EmailHelper = require('../Helpers/EmailHelper')
 const { csvAttachment } = require('../../infrastructure/Response')
-const { UserIsManagerError } = require('./UserMembershipErrors')
+const {
+  UserIsManagerError,
+  UserAlreadyAddedError,
+  UserNotFoundError,
+} = require('./UserMembershipErrors')
 const { SSOConfig } = require('../../models/SSOConfig')
 const CSVParser = require('json2csv').Parser
 
@@ -136,7 +140,7 @@ module.exports = {
       entityConfig,
       email,
       function (error, user) {
-        if (error != null ? error.alreadyAdded : undefined) {
+        if (error && error instanceof UserAlreadyAddedError) {
           return res.status(400).json({
             error: {
               code: 'user_already_added',
@@ -144,7 +148,7 @@ module.exports = {
             },
           })
         }
-        if (error != null ? error.userNotFound : undefined) {
+        if (error && error instanceof UserNotFoundError) {
           return res.status(404).json({
             error: {
               code: 'user_not_found',

services/web/app/src/Features/UserMembership/UserMembershipErrors.js

@@ -1,7 +1,11 @@
 const OError = require('@overleaf/o-error')
 
 class UserIsManagerError extends OError {}
+class UserNotFoundError extends OError {}
+class UserAlreadyAddedError extends OError {}
 
 module.exports = {
   UserIsManagerError,
+  UserNotFoundError,
+  UserAlreadyAddedError,
 }

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

@@ -1,20 +1,5 @@
-/* eslint-disable
-    n/handle-callback-err,
-    max-len,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS101: Remove unnecessary use of Array.from
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const { ObjectId } = require('mongodb')
-const async = require('async')
-const { promisifyAll } = require('@overleaf/promise-utils')
-const Errors = require('../Errors/Errors')
+const { promisifyAll, callbackify } = require('@overleaf/promise-utils')
 const EntityModels = {
   Institution: require('../../models/Institution').Institution,
   Subscription: require('../../models/Subscription').Subscription,
@@ -22,82 +7,70 @@ const EntityModels = {
 }
 const UserMembershipViewModel = require('./UserMembershipViewModel')
 const UserGetter = require('../User/UserGetter')
-const logger = require('@overleaf/logger')
-const UserMembershipEntityConfigs = require('./UserMembershipEntityConfigs')
-const { UserIsManagerError } = require('./UserMembershipErrors')
+const {
+  UserIsManagerError,
+  UserNotFoundError,
+  UserAlreadyAddedError,
+} = require('./UserMembershipErrors')
 
 const UserMembershipHandler = {
-  getEntityWithoutAuthorizationCheck(entityId, entityConfig, callback) {
-    if (callback == null) {
-      callback = function () {}
-    }
+  async getEntityWithoutAuthorizationCheck(entityId, entityConfig) {
     const query = buildEntityQuery(entityId, entityConfig)
-    EntityModels[entityConfig.modelName].findOne(query, callback)
+    return await EntityModels[entityConfig.modelName].findOne(query).exec()
   },
 
-  createEntity(entityId, entityConfig, callback) {
-    if (callback == null) {
-      callback = function () {}
-    }
+  async createEntity(entityId, entityConfig) {
     const data = buildEntityQuery(entityId, entityConfig)
-    EntityModels[entityConfig.modelName].create(data, callback)
+    return await EntityModels[entityConfig.modelName].create(data).exec()
   },
 
-  getUsers(entity, entityConfig, callback) {
-    if (callback == null) {
-      callback = function () {}
-    }
+  async getUsers(entity, entityConfig) {
     const attributes = entityConfig.fields.read
-    getPopulatedListOfMembers(entity, attributes, callback)
+    return await getPopulatedListOfMembers(entity, attributes)
   },
 
-  addUser(entity, entityConfig, email, callback) {
-    if (callback == null) {
-      callback = function () {}
-    }
+  async addUser(entity, entityConfig, email) {
     const attribute = entityConfig.fields.write
-    UserGetter.getUserByAnyEmail(email, function (error, user) {
-      if (error != null) {
-        return callback(error)
-      }
-      if (!user) {
-        const err = { userNotFound: true }
-        return callback(err)
-      }
-      if (entity[attribute].some(managerId => managerId.equals(user._id))) {
-        error = { alreadyAdded: true }
-        return callback(error)
-      }
-
-      addUserToEntity(entity, attribute, user, error =>
-        callback(error, UserMembershipViewModel.build(user))
-      )
-    })
-  },
+    const user = await UserGetter.promises.getUserByAnyEmail(email)
 
-  removeUser(entity, entityConfig, userId, callback) {
-    if (callback == null) {
-      callback = function () {}
+    if (!user) {
+      throw new UserNotFoundError()
     }
+
+    if (entity[attribute].some(managerId => managerId.equals(user._id))) {
+      throw new UserAlreadyAddedError()
+    }
+
+    await addUserToEntity(entity, attribute, user)
+    return UserMembershipViewModel.build(user)
+  },
+
+  async removeUser(entity, entityConfig, userId) {
     const attribute = entityConfig.fields.write
-    if (entity.admin_id != null ? entity.admin_id.equals(userId) : undefined) {
-      return callback(new UserIsManagerError())
+    if (entity.admin_id ? entity.admin_id.equals(userId) : undefined) {
+      throw new UserIsManagerError()
     }
-    removeUserFromEntity(entity, attribute, userId, callback)
+    return await removeUserFromEntity(entity, attribute, userId)
   },
 }
 
 UserMembershipHandler.promises = promisifyAll(UserMembershipHandler)
-module.exports = UserMembershipHandler
+module.exports = {
+  getEntityWithoutAuthorizationCheck: callbackify(
+    UserMembershipHandler.getEntityWithoutAuthorizationCheck
+  ),
+  createEntity: callbackify(UserMembershipHandler.createEntity),
+  getUsers: callbackify(UserMembershipHandler.getUsers),
+  addUser: callbackify(UserMembershipHandler.addUser),
+  removeUser: callbackify(UserMembershipHandler.removeUser),
+  promises: UserMembershipHandler,
+}
 
-function getPopulatedListOfMembers(entity, attributes, callback) {
-  if (callback == null) {
-    callback = function () {}
-  }
+async function getPopulatedListOfMembers(entity, attributes) {
   const userObjects = []
 
-  for (const attribute of Array.from(attributes)) {
-    for (const userObject of Array.from(entity[attribute] || [])) {
+  for (const attribute of attributes) {
+    for (const userObject of entity[attribute] || []) {
       // userObject can be an email as String, a user id as ObjectId or an
       // invite as Object with an email attribute as String. We want to pass to
       // UserMembershipViewModel either an email as (String) or a user id (ObjectId)
@@ -106,42 +79,38 @@ function getPopulatedListOfMembers(entity, attributes, callback) {
     }
   }
 
-  async.map(userObjects, UserMembershipViewModel.buildAsync, (err, users) => {
-    if (err) {
-      return callback(err)
-    }
-    for (const user of users) {
-      if (
-        user?._id &&
-        entity?.admin_id &&
-        user._id.toString() === entity.admin_id.toString()
-      ) {
-        user.isEntityAdmin = true
-      }
+  const users = await Promise.all(
+    userObjects.map(userObject =>
+      UserMembershipViewModel.promises.buildAsync(userObject)
+    )
+  )
+
+  for (const user of users) {
+    if (
+      user?._id &&
+      entity?.admin_id &&
+      user._id.toString() === entity.admin_id.toString()
+    ) {
+      user.isEntityAdmin = true
     }
-    callback(null, users)
-  })
+  }
+
+  return users
 }
 
-function addUserToEntity(entity, attribute, user, callback) {
-  if (callback == null) {
-    callback = function () {}
-  }
+async function addUserToEntity(entity, attribute, user) {
   const fieldUpdate = {}
   fieldUpdate[attribute] = user._id
-  entity.updateOne({ $addToSet: fieldUpdate }, callback)
+  return await entity.updateOne({ $addToSet: fieldUpdate }).exec()
 }
 
-function removeUserFromEntity(entity, attribute, userId, callback) {
-  if (callback == null) {
-    callback = function () {}
-  }
+async function removeUserFromEntity(entity, attribute, userId) {
   const fieldUpdate = {}
   fieldUpdate[attribute] = userId
-  entity.updateOne({ $pull: fieldUpdate }, callback)
+  return await entity.updateOne({ $pull: fieldUpdate }).exec()
 }
 
-function buildEntityQuery(entityId, entityConfig, loggedInUser) {
+function buildEntityQuery(entityId, entityConfig) {
   if (ObjectId.isValid(entityId.toString())) {
     entityId = new ObjectId(entityId)
   }

services/web/app/src/Features/UserMembership/UserMembershipViewModel.js

@@ -10,11 +10,11 @@
  * DS207: Consider shorter variations of null checks
  * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
  */
-let UserMembershipViewModel
 const UserGetter = require('../User/UserGetter')
 const { isObjectIdInstance } = require('../Helpers/Mongo')
+const { promisify } = require('@overleaf/promise-utils')
 
-module.exports = UserMembershipViewModel = {
+const UserMembershipViewModel = {
   build(userOrEmail) {
     if (userOrEmail._id) {
       return buildUserViewModel(userOrEmail)
@@ -75,3 +75,9 @@ function buildUserViewModel(user, isInvite) {
 const buildUserViewModelWithEmail = email => buildUserViewModel({ email }, true)
 
 const buildUserViewModelWithId = id => buildUserViewModel({ _id: id }, false)
+
+UserMembershipViewModel.promises = {
+  buildAsync: promisify(UserMembershipViewModel.buildAsync),
+}
+
+module.exports = UserMembershipViewModel

services/web/test/unit/src/UserMembership/UserMembershipControllerTests.js

@@ -23,6 +23,8 @@ const EntityConfigs = require('../../../../app/src/Features/UserMembership/UserM
 const Errors = require('../../../../app/src/Features/Errors/Errors')
 const {
   UserIsManagerError,
+  UserNotFoundError,
+  UserAlreadyAddedError,
 } = require('../../../../app/src/Features/UserMembership/UserMembershipErrors')
 
 describe('UserMembershipController', function () {
@@ -91,7 +93,11 @@ describe('UserMembershipController', function () {
       modulePath,
       {
         requires: {
-          './UserMembershipErrors': { UserIsManagerError },
+          './UserMembershipErrors': {
+            UserIsManagerError,
+            UserNotFoundError,
+            UserAlreadyAddedError,
+          },
           '../Authentication/SessionManager': this.SessionManager,
           '../SplitTests/SplitTestHandler': this.SplitTestHandler,
           './UserMembershipHandler': this.UserMembershipHandler,
@@ -212,7 +218,7 @@ describe('UserMembershipController', function () {
     })
 
     it('handle user already added', function (done) {
-      this.UserMembershipHandler.addUser.yields({ alreadyAdded: true })
+      this.UserMembershipHandler.addUser.yields(new UserAlreadyAddedError())
       return this.UserMembershipController.add(this.req, {
         status: () => ({
           json: payload => {
@@ -224,7 +230,7 @@ describe('UserMembershipController', function () {
     })
 
     it('handle user not found', function (done) {
-      this.UserMembershipHandler.addUser.yields({ userNotFound: true })
+      this.UserMembershipHandler.addUser.yields(new UserNotFoundError())
       return this.UserMembershipController.add(this.req, {
         status: () => ({
           json: payload => {

services/web/test/unit/src/UserMembership/UserMembershipHandlerTests.js

@@ -1,28 +1,15 @@
-/* eslint-disable
-    n/handle-callback-err,
-    max-len,
-    no-return-assign,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const { expect } = require('chai')
 const sinon = require('sinon')
 const assertCalledWith = sinon.assert.calledWith
-const assertNotCalled = sinon.assert.notCalled
 const { ObjectId } = require('mongodb')
 const modulePath =
   '../../../../app/src/Features/UserMembership/UserMembershipHandler'
 const SandboxedModule = require('sandboxed-module')
-const Errors = require('../../../../app/src/Features/Errors/Errors')
 const EntityConfigs = require('../../../../app/src/Features/UserMembership/UserMembershipEntityConfigs')
 const {
   UserIsManagerError,
+  UserNotFoundError,
+  UserAlreadyAddedError,
 } = require('../../../../app/src/Features/UserMembership/UserMembershipErrors')
 
 describe('UserMembershipHandler', function () {
@@ -38,40 +25,64 @@ describe('UserMembershipHandler', function () {
       manager_ids: [new ObjectId()],
       invited_emails: ['mock-email-1@foo.com'],
       teamInvites: [{ email: 'mock-email-1@bar.com' }],
-      update: sinon.stub().yields(null),
+      update: sinon.stub().returns({
+        exec: sinon.stub().resolves(),
+      }),
     }
     this.institution = {
       _id: 'mock-institution-id',
       v1Id: 123,
       managerIds: [new ObjectId(), new ObjectId(), new ObjectId()],
-      updateOne: sinon.stub().yields(null),
+      updateOne: sinon.stub().returns({
+        exec: sinon.stub().resolves(),
+      }),
     }
     this.publisher = {
       _id: 'mock-publisher-id',
       slug: 'slug',
       managerIds: [new ObjectId(), new ObjectId()],
-      updateOne: sinon.stub().yields(null),
+      updateOne: sinon.stub().returns({
+        exec: sinon.stub().resolves(),
+      }),
     }
 
     this.UserMembershipViewModel = {
-      buildAsync: sinon.stub().yields(null, { _id: 'mock-member-id' }),
+      promises: {
+        buildAsync: sinon.stub().resolves({ _id: 'mock-member-id' }),
+      },
       build: sinon.stub().returns(this.newUser),
     }
     this.UserGetter = {
-      getUserByAnyEmail: sinon.stub().yields(null, this.newUser),
+      promises: {
+        getUserByAnyEmail: sinon.stub().resolves(this.newUser),
+      },
+    }
+    this.Institution = {
+      findOne: sinon.stub().returns({
+        exec: sinon.stub().resolves(this.institution),
+      }),
     }
-    this.Institution = { findOne: sinon.stub().yields(null, this.institution) }
     this.Subscription = {
-      findOne: sinon.stub().yields(null, this.subscription),
+      findOne: sinon.stub().returns({
+        exec: sinon.stub().resolves(this.subscription),
+      }),
     }
     this.Publisher = {
-      findOne: sinon.stub().yields(null, this.publisher),
-      create: sinon.stub().yields(null, this.publisher),
+      findOne: sinon.stub().returns({
+        exec: sinon.stub().resolves(this.publisher),
+      }),
+      create: sinon.stub().returns({
+        exec: sinon.stub().resolves(this.publisher),
+      }),
     }
-    return (this.UserMembershipHandler = SandboxedModule.require(modulePath, {
+    this.UserMembershipHandler = SandboxedModule.require(modulePath, {
       requires: {
         mongodb: { ObjectId },
-        './UserMembershipErrors': { UserIsManagerError },
+        './UserMembershipErrors': {
+          UserIsManagerError,
+          UserNotFoundError,
+          UserAlreadyAddedError,
+        },
         './UserMembershipViewModel': this.UserMembershipViewModel,
         '../User/UserGetter': this.UserGetter,
         '../../models/Institution': {
@@ -84,195 +95,158 @@ describe('UserMembershipHandler', function () {
           Publisher: this.Publisher,
         },
       },
-    }))
+    })
   })
 
   describe('getEntityWithoutAuthorizationCheck', function () {
-    it('get publisher', function (done) {
-      return this.UserMembershipHandler.getEntityWithoutAuthorizationCheck(
-        this.fakeEntityId,
-        EntityConfigs.publisher,
-        (error, subscription) => {
-          expect(error).not.to.exist
-          const expectedQuery = { slug: this.fakeEntityId }
-          assertCalledWith(this.Publisher.findOne, expectedQuery)
-          expect(subscription).to.equal(this.publisher)
-          return done()
-        }
-      )
+    it('get publisher', async function () {
+      const subscription =
+        await this.UserMembershipHandler.promises.getEntityWithoutAuthorizationCheck(
+          this.fakeEntityId,
+          EntityConfigs.publisher
+        )
+      const expectedQuery = { slug: this.fakeEntityId }
+      assertCalledWith(this.Publisher.findOne, expectedQuery)
+      expect(subscription).to.equal(this.publisher)
     })
   })
 
   describe('getUsers', function () {
     describe('group', function () {
-      it('build view model for all users', function (done) {
-        return this.UserMembershipHandler.getUsers(
+      it('build view model for all users', async function () {
+        await this.UserMembershipHandler.promises.getUsers(
           this.subscription,
-          EntityConfigs.group,
-          (error, users) => {
-            const expectedCallcount =
-              this.subscription.member_ids.length +
-              this.subscription.invited_emails.length +
-              this.subscription.teamInvites.length
-            expect(this.UserMembershipViewModel.buildAsync.callCount).to.equal(
-              expectedCallcount
-            )
-            return done()
-          }
+          EntityConfigs.group
         )
+        const expectedCallcount =
+          this.subscription.member_ids.length +
+          this.subscription.invited_emails.length +
+          this.subscription.teamInvites.length
+        expect(
+          this.UserMembershipViewModel.promises.buildAsync.callCount
+        ).to.equal(expectedCallcount)
       })
     })
 
     describe('group mamagers', function () {
-      it('build view model for all managers', function (done) {
-        return this.UserMembershipHandler.getUsers(
+      it('build view model for all managers', async function () {
+        await this.UserMembershipHandler.promises.getUsers(
           this.subscription,
-          EntityConfigs.groupManagers,
-          (error, users) => {
-            const expectedCallcount = this.subscription.manager_ids.length
-            expect(this.UserMembershipViewModel.buildAsync.callCount).to.equal(
-              expectedCallcount
-            )
-            return done()
-          }
+          EntityConfigs.groupManagers
         )
+        const expectedCallcount = this.subscription.manager_ids.length
+        expect(
+          this.UserMembershipViewModel.promises.buildAsync.callCount
+        ).to.equal(expectedCallcount)
       })
     })
 
     describe('institution', function () {
-      it('build view model for all managers', function (done) {
-        return this.UserMembershipHandler.getUsers(
+      it('build view model for all managers', async function () {
+        await this.UserMembershipHandler.promises.getUsers(
           this.institution,
-          EntityConfigs.institution,
-          (error, users) => {
-            const expectedCallcount = this.institution.managerIds.length
-            expect(this.UserMembershipViewModel.buildAsync.callCount).to.equal(
-              expectedCallcount
-            )
-            return done()
-          }
+          EntityConfigs.institution
         )
+
+        const expectedCallcount = this.institution.managerIds.length
+        expect(
+          this.UserMembershipViewModel.promises.buildAsync.callCount
+        ).to.equal(expectedCallcount)
       })
     })
   })
 
   describe('createEntity', function () {
-    it('creates publisher', function (done) {
-      return this.UserMembershipHandler.createEntity(
+    it('creates publisher', async function () {
+      await this.UserMembershipHandler.promises.createEntity(
         this.fakeEntityId,
-        EntityConfigs.publisher,
-        (error, publisher) => {
-          expect(error).not.to.exist
-          assertCalledWith(this.Publisher.create, { slug: this.fakeEntityId })
-          return done()
-        }
+        EntityConfigs.publisher
       )
+      assertCalledWith(this.Publisher.create, { slug: this.fakeEntityId })
     })
   })
 
   describe('addUser', function () {
     beforeEach(function () {
-      return (this.email = this.newUser.email)
+      this.email = this.newUser.email
     })
 
     describe('institution', function () {
-      it('get user', function (done) {
-        return this.UserMembershipHandler.addUser(
+      it('get user', async function () {
+        await this.UserMembershipHandler.promises.addUser(
           this.institution,
           EntityConfigs.institution,
-          this.email,
-          (error, user) => {
-            assertCalledWith(this.UserGetter.getUserByAnyEmail, this.email)
-            return done()
-          }
+          this.email
         )
+        assertCalledWith(this.UserGetter.promises.getUserByAnyEmail, this.email)
       })
 
-      it('handle user not found', function (done) {
-        this.UserGetter.getUserByAnyEmail.yields(null, null)
-        return this.UserMembershipHandler.addUser(
-          this.institution,
-          EntityConfigs.institution,
-          this.email,
-          error => {
-            expect(error).to.exist
-            expect(error.userNotFound).to.equal(true)
-            return done()
-          }
-        )
+      it('handle user not found', async function () {
+        this.UserGetter.promises.getUserByAnyEmail.resolves(null)
+        expect(
+          this.UserMembershipHandler.promises.addUser(
+            this.institution,
+            EntityConfigs.institution,
+            this.email
+          )
+        ).to.be.rejectedWith(UserNotFoundError)
       })
 
-      it('handle user already added', function (done) {
+      it('handle user already added', async function () {
         this.institution.managerIds.push(this.newUser._id)
-        return this.UserMembershipHandler.addUser(
-          this.institution,
-          EntityConfigs.institution,
-          this.email,
-          (error, users) => {
-            expect(error).to.exist
-            expect(error.alreadyAdded).to.equal(true)
-            return done()
-          }
-        )
+        expect(
+          this.UserMembershipHandler.promises.addUser(
+            this.institution,
+            EntityConfigs.institution,
+            this.email
+          )
+        ).to.be.rejectedWith(UserAlreadyAddedError)
       })
 
-      it('add user to institution', function (done) {
-        return this.UserMembershipHandler.addUser(
+      it('add user to institution', async function () {
+        await this.UserMembershipHandler.promises.addUser(
           this.institution,
           EntityConfigs.institution,
-          this.email,
-          (error, user) => {
-            assertCalledWith(this.institution.updateOne, {
-              $addToSet: { managerIds: this.newUser._id },
-            })
-            return done()
-          }
+          this.email
         )
+        assertCalledWith(this.institution.updateOne, {
+          $addToSet: { managerIds: this.newUser._id },
+        })
       })
 
-      it('return user view', function (done) {
-        return this.UserMembershipHandler.addUser(
+      it('return user view', async function () {
+        const user = await this.UserMembershipHandler.promises.addUser(
           this.institution,
           EntityConfigs.institution,
-          this.email,
-          (error, user) => {
-            user.should.equal(this.newUser)
-            return done()
-          }
+          this.email
         )
+        user.should.equal(this.newUser)
       })
     })
   })
 
   describe('removeUser', function () {
     describe('institution', function () {
-      it('remove user from institution', function (done) {
-        return this.UserMembershipHandler.removeUser(
+      it('remove user from institution', async function () {
+        await this.UserMembershipHandler.promises.removeUser(
           this.institution,
           EntityConfigs.institution,
-          this.newUser._id,
-          (error, user) => {
-            const { lastCall } = this.institution.updateOne
-            assertCalledWith(this.institution.updateOne, {
-              $pull: { managerIds: this.newUser._id },
-            })
-            return done()
-          }
+          this.newUser._id
         )
+        assertCalledWith(this.institution.updateOne, {
+          $pull: { managerIds: this.newUser._id },
+        })
       })
 
-      it('handle admin', function (done) {
+      it('handle admin', async function () {
         this.subscription.admin_id = this.newUser._id
-        return this.UserMembershipHandler.removeUser(
-          this.subscription,
-          EntityConfigs.groupManagers,
-          this.newUser._id,
-          (error, user) => {
-            expect(error).to.exist
-            expect(error).to.be.instanceof(UserIsManagerError)
-            return done()
-          }
-        )
+        expect(
+          this.UserMembershipHandler.promises.removeUser(
+            this.subscription,
+            EntityConfigs.groupManagers,
+            this.newUser._id
+          )
+        ).to.be.rejectedWith(UserIsManagerError)
       })
     })
   })