From 677b68a6b7cf2516af11567bfbe0b31e2b6482d4 Mon Sep 17 00:00:00 2001 From: Antoine Clausse Date: Mon, 20 Oct 2025 16:32:11 +0200 Subject: [PATCH] [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 --- .../UserMembershipController.mjs | 188 ++++++++---------- .../UserMembership/UserMembershipHandler.mjs | 26 ++- .../UserMembershipController.test.mjs | 180 +++++++---------- 3 files changed, 177 insertions(+), 217 deletions(-) diff --git a/services/web/app/src/Features/UserMembership/UserMembershipController.mjs b/services/web/app/src/Features/UserMembership/UserMembershipController.mjs index 31a85e0c55..3091e44d9e 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipController.mjs +++ b/services/web/app/src/Features/UserMembership/UserMembershipController.mjs @@ -171,96 +171,98 @@ async function exportCsv(req, res) { csvAttachment(res, csvParser.parse(users), 'Group.csv') } +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: 'user_already_added', + message: req.i18n.translate('user_already_added'), + }, + }) + } + 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 }) +} + +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, + userId + ) + } catch (err) { + if (err instanceof UserMembershipErrors.UserIsManagerError) { + return res.status(400).json({ + error: { + code: 'managers_cannot_remove_admin', + message: req.i18n.translate('managers_cannot_remove_admin'), + }, + }) + } + throw err + } + 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(req, res, next) { - 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) { - return next(new Errors.NotFoundError('Cannot add users to entity')) - } - - UserMembershipHandler.addUser( - 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 }) - } - ) - }, - 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) { - return res.status(400).json({ - error: { - code: 'managers_cannot_remove_self', - message: req.i18n.translate('managers_cannot_remove_self'), - }, - }) - } - - 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) - } - ) - }, + 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), } diff --git a/services/web/app/src/Features/UserMembership/UserMembershipHandler.mjs b/services/web/app/src/Features/UserMembership/UserMembershipHandler.mjs index 7cce97ad0a..ed7b5d4cc8 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipHandler.mjs +++ b/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, +} diff --git a/services/web/test/unit/src/UserMembership/UserMembershipController.test.mjs b/services/web/test/unit/src/UserMembership/UserMembershipController.test.mjs index 75fa498e8c..f31dd7d831 100644 --- a/services/web/test/unit/src/UserMembership/UserMembershipController.test.mjs +++ b/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, + }) }, }) })