[web] fetch user details for chat messages/ranges in a single batch (#26342)

GitOrigin-RevId: 5a4238e30388bb7f58b7a93dda9e43338ab94f6e

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,
   },
 }

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
     })
   })
 })