From 110b83aea01617bb18c4344505d95990144f1277 Mon Sep 17 00:00:00 2001 From: Mathias Jakobsen Date: Fri, 7 Jun 2024 12:07:45 +0100 Subject: [PATCH] Merge pull request #18710 from overleaf/mj-web-chat-send-thread-data [chat+web] Inform frontend when duplicating threads GitOrigin-RevId: 285afee8f5a016a8e7ac58e9538cc3ec8362681d --- .../Messages/MessageHttpController.js | 16 +++ .../app/js/Features/Threads/ThreadManager.js | 9 ++ services/chat/chat.yaml | 27 ++++ .../app/src/Features/Chat/ChatApiHandler.js | 12 ++ .../app/src/Features/Chat/ChatController.js | 56 +------- .../web/app/src/Features/Chat/ChatManager.js | 61 ++++++++ .../src/Features/History/RestoreManager.js | 15 ++ .../hooks/use-review-panel-state.ts | 21 +++ .../test/unit/src/Chat/ChatControllerTests.js | 108 +------------- .../test/unit/src/Chat/ChatManagerTests.js | 135 ++++++++++++++++++ .../unit/src/History/RestoreManagerTests.js | 10 ++ 11 files changed, 312 insertions(+), 158 deletions(-) create mode 100644 services/web/app/src/Features/Chat/ChatManager.js create mode 100644 services/web/test/unit/src/Chat/ChatManagerTests.js diff --git a/services/chat/app/js/Features/Messages/MessageHttpController.js b/services/chat/app/js/Features/Messages/MessageHttpController.js index b9f513643b..a20d005864 100644 --- a/services/chat/app/js/Features/Messages/MessageHttpController.js +++ b/services/chat/app/js/Features/Messages/MessageHttpController.js @@ -86,6 +86,10 @@ export async function duplicateCommentThreads(context) { return await callMessageHttpController(context, _duplicateCommentThreads) } +export async function generateThreadData(context) { + return await callMessageHttpController(context, _generateThreadData) +} + export async function getStatus(context) { const message = 'chat is alive' context.res.status(200).setBody(message) @@ -124,6 +128,18 @@ const _getAllThreads = async (req, res) => { res.json(threads) } +const _generateThreadData = async (req, res) => { + const { projectId } = req.params + const { threads } = req.body + logger.debug({ projectId }, 'getting all threads') + const rooms = await ThreadManager.findThreadsById(projectId, threads) + const roomIds = rooms.map(r => r._id) + const messages = await MessageManager.findAllMessagesInRooms(roomIds) + logger.debug({ rooms, messages }, 'looked up messages in the rooms') + const threadData = MessageFormatter.groupMessagesByThreads(rooms, messages) + res.json(threadData) +} + const _resolveThread = async (req, res) => { const { projectId, threadId } = req.params const { user_id: userId } = req.body diff --git a/services/chat/app/js/Features/Threads/ThreadManager.js b/services/chat/app/js/Features/Threads/ThreadManager.js index d7a666f082..5697b39393 100644 --- a/services/chat/app/js/Features/Threads/ThreadManager.js +++ b/services/chat/app/js/Features/Threads/ThreadManager.js @@ -146,3 +146,12 @@ export async function duplicateThread(projectId, threadId) { newRoom._id = confirmation.insertedId return { oldRoom: room, newRoom } } + +export async function findThreadsById(projectId, threadIds) { + return await db.rooms + .find({ + project_id: new ObjectId(projectId), + thread_id: { $in: threadIds.map(id => new ObjectId(id)) }, + }) + .toArray() +} diff --git a/services/chat/chat.yaml b/services/chat/chat.yaml index 5ffbab17b4..3ccdf9bc30 100644 --- a/services/chat/chat.yaml +++ b/services/chat/chat.yaml @@ -334,6 +334,33 @@ paths: type: object description: Mapping of old thread ids to their duplicated thread ids description: Duplicate a list of comment threads + '/project/{projectId}/generate-thread-data': + parameters: + - schema: + type: string + name: projectId + in: path + required: true + post: + summary: Generate thread data to load into the frontend + operationId: generateThreadData + requestBody: + content: + application/json: + schema: + type: object + properties: + threads: + type: array + items: + type: string + responses: + '200': + content: + application/json: + schema: + type: object + description: Load threads and generate a json blob containing all messages in all the threads components: schemas: Message: diff --git a/services/web/app/src/Features/Chat/ChatApiHandler.js b/services/web/app/src/Features/Chat/ChatApiHandler.js index a871a2adfc..d581827a89 100644 --- a/services/web/app/src/Features/Chat/ChatApiHandler.js +++ b/services/web/app/src/Features/Chat/ChatApiHandler.js @@ -109,6 +109,16 @@ async function duplicateCommentThreads(projectId, threads) { ) } +async function generateThreadData(projectId, threads) { + return await fetchJson( + chatApiUrl(`/project/${projectId}/generate-thread-data`), + { + method: 'POST', + json: { threads }, + } + ) +} + function chatApiUrl(path) { return new URL(path, settings.apis.chat.internal_url) } @@ -126,6 +136,7 @@ module.exports = { deleteMessage: callbackify(deleteMessage), getResolvedThreadIds: callbackify(getResolvedThreadIds), duplicateCommentThreads: callbackify(duplicateCommentThreads), + generateThreadData: callbackify(generateThreadData), promises: { getThreads, destroyProject, @@ -139,5 +150,6 @@ module.exports = { deleteMessage, getResolvedThreadIds, duplicateCommentThreads, + generateThreadData, }, } diff --git a/services/web/app/src/Features/Chat/ChatController.js b/services/web/app/src/Features/Chat/ChatController.js index 6050d8e22c..51d217ed9e 100644 --- a/services/web/app/src/Features/Chat/ChatController.js +++ b/services/web/app/src/Features/Chat/ChatController.js @@ -18,7 +18,8 @@ const EditorRealTimeController = require('../Editor/EditorRealTimeController') const SessionManager = require('../Authentication/SessionManager') const UserInfoManager = require('../User/UserInfoManager') const UserInfoController = require('../User/UserInfoController') -const async = require('async') +const ChatManager = require('./ChatManager') +const logger = require('@overleaf/logger') module.exports = ChatController = { sendMessage(req, res, next) { @@ -68,7 +69,7 @@ module.exports = ChatController = { if (err != null) { return next(err) } - return ChatController._injectUserInfoIntoThreads( + return ChatManager.injectUserInfoIntoThreads( { global: { messages } }, function (err) { if (err != null) { @@ -80,55 +81,4 @@ module.exports = ChatController = { } ) }, - - _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] - if (thread.resolved) { - userIds[thread.resolved_by_user_id] = true - } - for (message of Array.from(thread.messages)) { - userIds[message.user_id] = true - } - } - - const jobs = [] - const users = {} - for (userId in userIds) { - const _ = userIds[userId] - ;(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) - } - - return async.series(jobs, function (error) { - if (error != null) { - return callback(error) - } - 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] - } - } - return callback(null, threads) - }) - }, } diff --git a/services/web/app/src/Features/Chat/ChatManager.js b/services/web/app/src/Features/Chat/ChatManager.js new file mode 100644 index 0000000000..9625881dd8 --- /dev/null +++ b/services/web/app/src/Features/Chat/ChatManager.js @@ -0,0 +1,61 @@ +const async = require('async') +const UserInfoManager = require('../User/UserInfoManager') +const UserInfoController = require('../User/UserInfoController') +const { promisify } = 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] + if (thread.resolved) { + userIds[thread.resolved_by_user_id] = true + } + for (message of Array.from(thread.messages)) { + userIds[message.user_id] = true + } + } + + 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) + } + + return async.series(jobs, function (error) { + if (error != null) { + return callback(error) + } + 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] + } + } + return callback(null, threads) + }) +} + +module.exports = { + injectUserInfoIntoThreads, + promises: { + injectUserInfoIntoThreads: promisify(injectUserInfoIntoThreads), + }, +} diff --git a/services/web/app/src/Features/History/RestoreManager.js b/services/web/app/src/Features/History/RestoreManager.js index a6c3c9b00f..1d210b2fdc 100644 --- a/services/web/app/src/Features/History/RestoreManager.js +++ b/services/web/app/src/Features/History/RestoreManager.js @@ -12,6 +12,8 @@ const DocumentUpdaterHandler = require('../DocumentUpdater/DocumentUpdaterHandle const ChatApiHandler = require('../Chat/ChatApiHandler') const DocstoreManager = require('../Docstore/DocstoreManager') const logger = require('@overleaf/logger') +const EditorRealTimeController = require('../Editor/EditorRealTimeController') +const ChatManager = require('../Chat/ChatManager') const RestoreManager = { async restoreFileFromV2(userId, projectId, version, pathname) { @@ -158,6 +160,19 @@ const RestoreManager = { newRanges.comments = ranges.comments } + const newCommentThreadData = + await ChatApiHandler.promises.generateThreadData( + projectId, + newRanges.comments.map(({ op: { t } }) => t) + ) + await ChatManager.promises.injectUserInfoIntoThreads(newCommentThreadData) + logger.debug({ newCommentThreadData }, 'emitting new comment threads') + EditorRealTimeController.emitToRoom( + projectId, + 'new-comment-threads', + newCommentThreadData + ) + return await EditorController.promises.addDocWithRanges( projectId, parentFolderId, diff --git a/services/web/frontend/js/features/ide-react/context/review-panel/hooks/use-review-panel-state.ts b/services/web/frontend/js/features/ide-react/context/review-panel/hooks/use-review-panel-state.ts index 3fdfb8c624..1e7b981597 100644 --- a/services/web/frontend/js/features/ide-react/context/review-panel/hooks/use-review-panel-state.ts +++ b/services/web/frontend/js/features/ide-react/context/review-panel/hooks/use-review-panel-state.ts @@ -1452,6 +1452,27 @@ function useReviewPanelState(): ReviewPanel.ReviewPanelState { [getThread] ) ) + useSocketListener( + socket, + 'new-comment-threads', + useCallback( + (threads: ReviewPanelCommentThreadsApi) => { + setCommentThreads(prevState => { + const newThreads = { ...prevState } + for (const threadIdString of Object.keys(threads)) { + const threadId = threadIdString as ThreadId + const { submitting: _, ...thread } = getThread(threadId) + // Replace already loaded messages with the server provided ones + thread.messages = threads[threadId].messages.map(formatComment) + newThreads[threadId] = thread + } + return newThreads + }) + handleLayoutChange({ async: true }) + }, + [getThread] + ) + ) const openSubView = useRef('cur_file') useEffect(() => { diff --git a/services/web/test/unit/src/Chat/ChatControllerTests.js b/services/web/test/unit/src/Chat/ChatControllerTests.js index ca7a5e18d9..12cd2d81d8 100644 --- a/services/web/test/unit/src/Chat/ChatControllerTests.js +++ b/services/web/test/unit/src/Chat/ChatControllerTests.js @@ -19,13 +19,13 @@ const modulePath = path.join( __dirname, '../../../../app/src/Features/Chat/ChatController' ) -const { expect } = require('chai') describe('ChatController', function () { beforeEach(function () { this.user_id = 'mock-user-id' this.settings = {} this.ChatApiHandler = {} + this.ChatManager = {} this.EditorRealTimeController = { emitToRoom: sinon.stub() } this.SessionManager = { getLoggedInUserId: sinon.stub().returns(this.user_id), @@ -34,6 +34,7 @@ describe('ChatController', function () { requires: { '@overleaf/settings': this.settings, './ChatApiHandler': this.ChatApiHandler, + './ChatManager': this.ChatManager, '../Editor/EditorRealTimeController': this.EditorRealTimeController, '../Authentication/SessionManager': this.SessionManager, '../User/UserInfoManager': (this.UserInfoManager = {}), @@ -106,7 +107,7 @@ describe('ChatController', function () { limit: (this.limit = '30'), before: (this.before = '12345'), } - this.ChatController._injectUserInfoIntoThreads = sinon.stub().yields() + this.ChatManager.injectUserInfoIntoThreads = sinon.stub().yields() this.ChatApiHandler.getGlobalMessages = sinon .stub() .yields(null, (this.messages = ['mock', 'messages'])) @@ -123,107 +124,4 @@ describe('ChatController', function () { return this.res.json.calledWith(this.messages).should.equal(true) }) }) - - describe('_injectUserInfoIntoThreads', function () { - beforeEach(function () { - this.users = { - user_id_1: { - mock: 'user_1', - }, - user_id_2: { - mock: 'user_2', - }, - } - this.UserInfoManager.getPersonalInfo = (userId, callback) => { - return callback(null, this.users[userId]) - } - sinon.spy(this.UserInfoManager, 'getPersonalInfo') - return (this.UserInfoController.formatPersonalInfo = user => ({ - formatted: user.mock, - })) - }) - - it('should inject a user object into messaged and resolved data', function (done) { - return this.ChatController._injectUserInfoIntoThreads( - { - thread1: { - resolved: true, - resolved_by_user_id: 'user_id_1', - messages: [ - { - user_id: 'user_id_1', - content: 'foo', - }, - { - user_id: 'user_id_2', - content: 'bar', - }, - ], - }, - thread2: { - messages: [ - { - user_id: 'user_id_1', - content: 'baz', - }, - ], - }, - }, - (error, threads) => { - expect(threads).to.deep.equal({ - thread1: { - resolved: true, - resolved_by_user_id: 'user_id_1', - resolved_by_user: { formatted: 'user_1' }, - messages: [ - { - user_id: 'user_id_1', - user: { formatted: 'user_1' }, - content: 'foo', - }, - { - user_id: 'user_id_2', - user: { formatted: 'user_2' }, - content: 'bar', - }, - ], - }, - thread2: { - messages: [ - { - user_id: 'user_id_1', - user: { formatted: 'user_1' }, - content: 'baz', - }, - ], - }, - }) - return done() - } - ) - }) - - it('should only need to look up each user once', function (done) { - return this.ChatController._injectUserInfoIntoThreads( - [ - { - messages: [ - { - user_id: 'user_id_1', - content: 'foo', - }, - { - user_id: 'user_id_1', - content: 'bar', - }, - ], - }, - ], - (error, threads) => { - this.UserInfoManager.getPersonalInfo.calledOnce.should.equal(true) - return done() - } - ) - }) - }) }) diff --git a/services/web/test/unit/src/Chat/ChatManagerTests.js b/services/web/test/unit/src/Chat/ChatManagerTests.js new file mode 100644 index 0000000000..76d9b79c4b --- /dev/null +++ b/services/web/test/unit/src/Chat/ChatManagerTests.js @@ -0,0 +1,135 @@ +const SandboxedModule = require('sandboxed-module') +const path = require('path') +const sinon = require('sinon') +const modulePath = path.join( + __dirname, + '../../../../app/src/Features/Chat/ChatManager' +) +const { expect } = require('chai') + +describe('ChatManager', function () { + beforeEach(function () { + this.user_id = 'mock-user-id' + this.ChatManager = SandboxedModule.require(modulePath, { + requires: { + '../User/UserInfoManager': (this.UserInfoManager = {}), + '../User/UserInfoController': (this.UserInfoController = {}), + }, + }) + this.req = { + params: { + project_id: this.project_id, + }, + } + this.res = { + json: sinon.stub(), + send: sinon.stub(), + sendStatus: sinon.stub(), + } + }) + + describe('injectUserInfoIntoThreads', function () { + beforeEach(function () { + this.users = { + user_id_1: { + mock: 'user_1', + }, + user_id_2: { + mock: 'user_2', + }, + } + this.UserInfoManager.getPersonalInfo = (userId, callback) => { + return callback(null, this.users[userId]) + } + sinon.spy(this.UserInfoManager, 'getPersonalInfo') + return (this.UserInfoController.formatPersonalInfo = user => ({ + formatted: user.mock, + })) + }) + + it('should inject a user object into messaged and resolved data', function (done) { + return this.ChatManager.injectUserInfoIntoThreads( + { + thread1: { + resolved: true, + resolved_by_user_id: 'user_id_1', + messages: [ + { + user_id: 'user_id_1', + content: 'foo', + }, + { + user_id: 'user_id_2', + content: 'bar', + }, + ], + }, + thread2: { + messages: [ + { + user_id: 'user_id_1', + content: 'baz', + }, + ], + }, + }, + (error, threads) => { + expect(error).to.be.null + expect(threads).to.deep.equal({ + thread1: { + resolved: true, + resolved_by_user_id: 'user_id_1', + resolved_by_user: { formatted: 'user_1' }, + messages: [ + { + user_id: 'user_id_1', + user: { formatted: 'user_1' }, + content: 'foo', + }, + { + user_id: 'user_id_2', + user: { formatted: 'user_2' }, + content: 'bar', + }, + ], + }, + thread2: { + messages: [ + { + user_id: 'user_id_1', + user: { formatted: 'user_1' }, + content: 'baz', + }, + ], + }, + }) + return done() + } + ) + }) + + it('should only need to look up each user once', function (done) { + return this.ChatManager.injectUserInfoIntoThreads( + [ + { + messages: [ + { + user_id: 'user_id_1', + content: 'foo', + }, + { + user_id: 'user_id_1', + content: 'bar', + }, + ], + }, + ], + (error, threads) => { + expect(error).to.be.null + this.UserInfoManager.getPersonalInfo.calledOnce.should.equal(true) + return done() + } + ) + }) + }) +}) diff --git a/services/web/test/unit/src/History/RestoreManagerTests.js b/services/web/test/unit/src/History/RestoreManagerTests.js index 384f19b4ce..93929cc64e 100644 --- a/services/web/test/unit/src/History/RestoreManagerTests.js +++ b/services/web/test/unit/src/History/RestoreManagerTests.js @@ -31,6 +31,9 @@ describe('RestoreManager', function () { promises: {}, }), '../Chat/ChatApiHandler': (this.ChatApiHandler = { promises: {} }), + '../Chat/ChatManager': (this.ChatManager = { promises: {} }), + '../Editor/EditorRealTimeController': (this.EditorRealTimeController = + {}), }, }) this.user_id = 'mock-user-id' @@ -324,6 +327,13 @@ describe('RestoreManager', function () { }, }, }) + this.ChatApiHandler.promises.generateThreadData = sinon + .stub() + .resolves({}) + this.ChatManager.promises.injectUserInfoIntoThreads = sinon + .stub() + .resolves() + this.EditorRealTimeController.emitToRoom = sinon.stub() this.tracked_changes = [ { op: { pos: 4, i: 'bar' },