diff --git a/services/web/app/src/Features/Chat/ChatManager.js b/services/web/app/src/Features/Chat/ChatManager.js index 9625881dd8..7eab6039d8 100644 --- a/services/web/app/src/Features/Chat/ChatManager.js +++ b/services/web/app/src/Features/Chat/ChatManager.js @@ -1,61 +1,46 @@ -const async = require('async') -const UserInfoManager = require('../User/UserInfoManager') const UserInfoController = require('../User/UserInfoController') -const { promisify } = require('@overleaf/promise-utils') +const UserGetter = require('../User/UserGetter') +const { callbackify } = require('@overleaf/promise-utils') -function injectUserInfoIntoThreads(threads, callback) { - // There will be a lot of repitition of user_ids, so first build a list - // of unique ones to perform db look ups on, then use these to populate the - // user fields - let message, thread, threadId, userId - if (callback == null) { - callback = function () {} - } - const userIds = {} - for (threadId in threads) { - thread = threads[threadId] +async function injectUserInfoIntoThreads(threads) { + const userIds = new Set() + for (const thread of Object.values(threads)) { if (thread.resolved) { - userIds[thread.resolved_by_user_id] = true + userIds.add(thread.resolved_by_user_id) } - for (message of Array.from(thread.messages)) { - userIds[message.user_id] = true + for (const message of thread.messages) { + userIds.add(message.user_id) } } - const jobs = [] - const users = {} - for (userId in userIds) { - ;(userId => - jobs.push(cb => - UserInfoManager.getPersonalInfo(userId, function (error, user) { - if (error != null) return cb(error) - user = UserInfoController.formatPersonalInfo(user) - users[userId] = user - cb() - }) - ))(userId) + const projection = { + _id: true, + first_name: true, + last_name: true, + email: true, } - - return async.series(jobs, function (error) { - if (error != null) { - return callback(error) + const users = await UserGetter.promises.getUsers(userIds, projection) + const usersById = new Map() + for (const user of users) { + usersById.set( + user._id.toString(), + UserInfoController.formatPersonalInfo(user) + ) + } + for (const thread of Object.values(threads)) { + if (thread.resolved) { + thread.resolved_by_user = usersById.get(thread.resolved_by_user_id) } - for (threadId in threads) { - thread = threads[threadId] - if (thread.resolved) { - thread.resolved_by_user = users[thread.resolved_by_user_id] - } - for (message of Array.from(thread.messages)) { - message.user = users[message.user_id] - } + for (const message of thread.messages) { + message.user = usersById.get(message.user_id) } - return callback(null, threads) - }) + } + return threads } module.exports = { - injectUserInfoIntoThreads, + injectUserInfoIntoThreads: callbackify(injectUserInfoIntoThreads), promises: { - injectUserInfoIntoThreads: promisify(injectUserInfoIntoThreads), + injectUserInfoIntoThreads, }, } diff --git a/services/web/test/unit/src/Chat/ChatManagerTests.js b/services/web/test/unit/src/Chat/ChatManagerTests.js index bdd3042513..5578b8b167 100644 --- a/services/web/test/unit/src/Chat/ChatManagerTests.js +++ b/services/web/test/unit/src/Chat/ChatManagerTests.js @@ -12,7 +12,7 @@ describe('ChatManager', function () { this.user_id = 'mock-user-id' this.ChatManager = SandboxedModule.require(modulePath, { requires: { - '../User/UserInfoManager': (this.UserInfoManager = {}), + '../User/UserGetter': (this.UserGetter = { promises: {} }), '../User/UserInfoController': (this.UserInfoController = {}), }, }) @@ -32,18 +32,22 @@ describe('ChatManager', function () { beforeEach(function () { this.users = { user_id_1: { - mock: 'user_1', + _id: 'user_id_1', }, user_id_2: { - mock: 'user_2', + _id: 'user_id_2', }, } - this.UserInfoManager.getPersonalInfo = (userId, callback) => { - return callback(null, this.users[userId]) - } - sinon.spy(this.UserInfoManager, 'getPersonalInfo') + this.UserGetter.promises.getUsers = userIds => + Promise.resolve( + Array.from(userIds) + .map(id => this.users[id]) + .filter(u => !!u) + ) + + sinon.spy(this.UserGetter.promises, 'getUsers') return (this.UserInfoController.formatPersonalInfo = user => ({ - formatted: user.mock, + formatted: { id: user._id.toString() }, })) }) @@ -79,16 +83,16 @@ describe('ChatManager', function () { thread1: { resolved: true, resolved_by_user_id: 'user_id_1', - resolved_by_user: { formatted: 'user_1' }, + resolved_by_user: { formatted: { id: 'user_id_1' } }, messages: [ { user_id: 'user_id_1', - user: { formatted: 'user_1' }, + user: { formatted: { id: 'user_id_1' } }, content: 'foo', }, { user_id: 'user_id_2', - user: { formatted: 'user_2' }, + user: { formatted: { id: 'user_id_2' } }, content: 'bar', }, ], @@ -97,7 +101,7 @@ describe('ChatManager', function () { messages: [ { user_id: 'user_id_1', - user: { formatted: 'user_1' }, + user: { formatted: { id: 'user_id_1' } }, content: 'baz', }, ], @@ -105,7 +109,7 @@ describe('ChatManager', function () { }) }) - it('should only need to look up each user once', async function () { + it('should lookup all users in a single batch', async function () { await this.ChatManager.promises.injectUserInfoIntoThreads([ { messages: [ @@ -121,7 +125,7 @@ describe('ChatManager', function () { }, ]) - this.UserInfoManager.getPersonalInfo.calledOnce.should.equal(true) + this.UserGetter.promises.getUsers.should.have.been.calledOnce }) }) })