From 83e1452991eaac21a4f3ad7f7d621713a4631a0b Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Fri, 24 May 2024 07:55:42 -0400 Subject: [PATCH] Merge pull request #18398 from overleaf/em-web-resolved-comment-ids Return resolved comment ids with getDocument() GitOrigin-RevId: 30234f72d52b90b313821290b6c63aa6cc6cb243 --- libraries/fetch-utils/index.js | 12 ++--- .../app/src/Features/Chat/ChatApiHandler.js | 9 ++++ .../Features/Documents/DocumentController.js | 13 +++++ .../src/Documents/DocumentControllerTests.js | 53 ++++++++++++++----- 4 files changed, 69 insertions(+), 18 deletions(-) diff --git a/libraries/fetch-utils/index.js b/libraries/fetch-utils/index.js index 5e2cf16d1d..5ac0fc2ce5 100644 --- a/libraries/fetch-utils/index.js +++ b/libraries/fetch-utils/index.js @@ -9,8 +9,8 @@ const https = require('https') * Make a request and return the parsed JSON response. * * @param {string | URL} url - request URL - * @param {object} opts - fetch options - * @return {Promise} the parsed JSON response + * @param {any} [opts] - fetch options + * @return {Promise} the parsed JSON response * @throws {RequestFailedError} if the response has a failure status code */ async function fetchJson(url, opts = {}) { @@ -39,7 +39,7 @@ async function fetchJsonWithResponse(url, opts = {}) { * If the response body is destroyed, the request is aborted. * * @param {string | URL} url - request URL - * @param {object} opts - fetch options + * @param {any} [opts] - fetch options * @return {Promise} * @throws {RequestFailedError} if the response has a failure status code */ @@ -67,7 +67,7 @@ async function fetchStreamWithResponse(url, opts = {}) { * Make a request and discard the response. * * @param {string | URL} url - request URL - * @param {object} opts - fetch options + * @param {any} [opts] - fetch options * @return {Promise} * @throws {RequestFailedError} if the response has a failure status code */ @@ -86,7 +86,7 @@ async function fetchNothing(url, opts = {}) { * Make a request and extract the redirect from the response. * * @param {string | URL} url - request URL - * @param {object} opts - fetch options + * @param {any} [opts] - fetch options * @return {Promise} * @throws {RequestFailedError} if the response has a non redirect status code or missing Location header */ @@ -115,7 +115,7 @@ async function fetchRedirect(url, opts = {}) { * Make a request and return a string. * * @param {string | URL} url - request URL - * @param {object} opts - fetch options + * @param {any} [opts] - fetch options * @return {Promise} * @throws {RequestFailedError} if the response has a failure status code */ diff --git a/services/web/app/src/Features/Chat/ChatApiHandler.js b/services/web/app/src/Features/Chat/ChatApiHandler.js index 16edb81fea..76c58d7e49 100644 --- a/services/web/app/src/Features/Chat/ChatApiHandler.js +++ b/services/web/app/src/Features/Chat/ChatApiHandler.js @@ -90,6 +90,13 @@ async function deleteMessage(projectId, threadId, messageId) { ) } +async function getResolvedThreadIds(projectId) { + const body = await fetchJson( + chatApiUrl(`/project/${projectId}/resolved-thread-ids`) + ) + return body.resolvedThreadIds +} + function chatApiUrl(path) { return new URL(path, settings.apis.chat.internal_url) } @@ -105,6 +112,7 @@ module.exports = { deleteThread: callbackify(deleteThread), editMessage: callbackify(editMessage), deleteMessage: callbackify(deleteMessage), + getResolvedThreadIds: callbackify(getResolvedThreadIds), promises: { getThreads, destroyProject, @@ -116,5 +124,6 @@ module.exports = { deleteThread, editMessage, deleteMessage, + getResolvedThreadIds, }, } diff --git a/services/web/app/src/Features/Documents/DocumentController.js b/services/web/app/src/Features/Documents/DocumentController.js index a54475b1b3..a3191881c4 100644 --- a/services/web/app/src/Features/Documents/DocumentController.js +++ b/services/web/app/src/Features/Documents/DocumentController.js @@ -1,3 +1,4 @@ +const ChatApiHandler = require('../Chat/ChatApiHandler') const ProjectGetter = require('../Project/ProjectGetter') const ProjectLocator = require('../Project/ProjectLocator') const ProjectEntityHandler = require('../Project/ProjectEntityHandler') @@ -31,6 +32,17 @@ async function getDocument(req, res) { { peek } ) + const resolvedCommentIdsInProject = + await ChatApiHandler.promises.getResolvedThreadIds(projectId) + + const commentIdsInDoc = new Set( + ranges.comments?.map(comment => comment.id) ?? [] + ) + + const resolvedCommentIds = resolvedCommentIdsInProject.filter(commentId => + commentIdsInDoc.has(commentId) + ) + if (plain) { plainTextResponse(res, lines.join('\n')) } else { @@ -53,6 +65,7 @@ async function getDocument(req, res) { projectHistoryId, projectHistoryType, historyRangesSupport, + resolvedCommentIds, }) } } diff --git a/services/web/test/unit/src/Documents/DocumentControllerTests.js b/services/web/test/unit/src/Documents/DocumentControllerTests.js index b3c3f14fa1..d4c12de619 100644 --- a/services/web/test/unit/src/Documents/DocumentControllerTests.js +++ b/services/web/test/unit/src/Documents/DocumentControllerTests.js @@ -15,7 +15,26 @@ describe('DocumentController', function () { this.doc = { _id: 'doc-id-123' } this.doc_lines = ['one', 'two', 'three'] this.version = 42 - this.ranges = { mock: 'ranges' } + this.ranges = { + comments: [ + { + id: 'comment1', + op: { + c: 'foo', + p: 123, + t: 'comment1', + }, + }, + { + id: 'comment2', + op: { + c: 'bar', + p: 456, + t: 'comment2', + }, + }, + ], + } this.pathname = '/a/b/c/file.tex' this.lastUpdatedAt = new Date().getTime() this.lastUpdatedBy = 'fake-last-updater-id' @@ -29,6 +48,10 @@ describe('DocumentController', function () { }, }, } + this.resolvedThreadIds = [ + 'comment2', + 'comment4', // Comment in project but not in doc + ] this.ProjectGetter = { promises: { @@ -58,6 +81,12 @@ describe('DocumentController', function () { }, } + this.ChatApiHandler = { + promises: { + getResolvedThreadIds: sinon.stub().resolves(this.resolvedThreadIds), + }, + } + this.DocumentController = SandboxedModule.require(MODULE_PATH, { requires: { '../Project/ProjectGetter': this.ProjectGetter, @@ -65,6 +94,7 @@ describe('DocumentController', function () { '../Project/ProjectEntityHandler': this.ProjectEntityHandler, '../Project/ProjectEntityUpdateHandler': this.ProjectEntityUpdateHandler, + '../Chat/ChatApiHandler': this.ChatApiHandler, }, }) }) @@ -87,17 +117,16 @@ describe('DocumentController', function () { it('should return the history id and display setting to the client as JSON', function () { this.res.type.should.equal('application/json') - this.res.body.should.equal( - JSON.stringify({ - lines: this.doc_lines, - version: this.version, - ranges: this.ranges, - pathname: this.pathname, - projectHistoryId: this.project.overleaf.history.id, - projectHistoryType: 'project-history', - historyRangesSupport: false, - }) - ) + JSON.parse(this.res.body).should.deep.equal({ + lines: this.doc_lines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.project.overleaf.history.id, + projectHistoryType: 'project-history', + resolvedCommentIds: ['comment2'], + historyRangesSupport: false, + }) }) })