From 0e9ceb1ffd248c92a6316a02f4bd14d06bb75429 Mon Sep 17 00:00:00 2001 From: David <33458145+davidmcpowell@users.noreply.github.com> Date: Mon, 5 Feb 2024 09:28:20 +0000 Subject: [PATCH] Merge pull request #16788 from overleaf/dp-mongoose-callback-user-membership-handler Promisify UserMembershipHandler and UserMembershipHandlerTests GitOrigin-RevId: bb33110ee750364754db53fb075a5700be003ecc --- .../UserMembershipController.js | 10 +- .../UserMembership/UserMembershipErrors.js | 4 + .../UserMembership/UserMembershipHandler.js | 155 ++++------ .../UserMembership/UserMembershipViewModel.js | 10 +- .../UserMembershipControllerTests.js | 12 +- .../UserMembershipHandlerTests.js | 270 ++++++++---------- 6 files changed, 212 insertions(+), 249 deletions(-) diff --git a/services/web/app/src/Features/UserMembership/UserMembershipController.js b/services/web/app/src/Features/UserMembership/UserMembershipController.js index b0fa97838f..281088b3c1 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipController.js +++ b/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', diff --git a/services/web/app/src/Features/UserMembership/UserMembershipErrors.js b/services/web/app/src/Features/UserMembership/UserMembershipErrors.js index 504f4fa512..6667ced79f 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipErrors.js +++ b/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, } diff --git a/services/web/app/src/Features/UserMembership/UserMembershipHandler.js b/services/web/app/src/Features/UserMembership/UserMembershipHandler.js index 7404b4d6f8..7e03545085 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipHandler.js +++ b/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) - } + const user = await UserGetter.promises.getUserByAnyEmail(email) - addUserToEntity(entity, attribute, user, error => - callback(error, UserMembershipViewModel.build(user)) - ) - }) + 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) }, - removeUser(entity, entityConfig, userId, callback) { - if (callback == null) { - callback = function () {} - } + 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) + 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 } - 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) } diff --git a/services/web/app/src/Features/UserMembership/UserMembershipViewModel.js b/services/web/app/src/Features/UserMembership/UserMembershipViewModel.js index 8b90be3647..6c94f677b7 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipViewModel.js +++ b/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 diff --git a/services/web/test/unit/src/UserMembership/UserMembershipControllerTests.js b/services/web/test/unit/src/UserMembership/UserMembershipControllerTests.js index 87425e0237..e5e45b1445 100644 --- a/services/web/test/unit/src/UserMembership/UserMembershipControllerTests.js +++ b/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 => { diff --git a/services/web/test/unit/src/UserMembership/UserMembershipHandlerTests.js b/services/web/test/unit/src/UserMembership/UserMembershipHandlerTests.js index 6ea0b14a98..b29fb76fd7 100644 --- a/services/web/test/unit/src/UserMembership/UserMembershipHandlerTests.js +++ b/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) }) }) })