From f025f1d0cba04aa790e253c7365cca39c262fa8d Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Fri, 13 Jun 2025 09:02:10 +0200 Subject: [PATCH] [web] let docstore determine a projects comment thread ids (#26364) * [docstore] add endpoint for getting a projects comment thread ids * [web] let docstore determine a projects comment thread ids Also fetch the comment thread ids once when reverting project. GitOrigin-RevId: c3ebab976821509c9627962e58918f9c6ebb0e1d --- services/docstore/app.js | 4 + services/docstore/app/js/DocManager.js | 16 ++ services/docstore/app/js/HttpController.js | 7 + .../test/acceptance/js/GettingAllDocsTests.js | 23 +++ .../acceptance/js/helpers/DocstoreClient.js | 10 + .../src/Features/Docstore/DocstoreManager.js | 10 + .../src/Features/History/RestoreManager.js | 63 ++++-- .../unit/src/History/RestoreManagerTests.js | 187 +++++++++++++++--- 8 files changed, 278 insertions(+), 42 deletions(-) diff --git a/services/docstore/app.js b/services/docstore/app.js index 1adf0ba559..ef755c4bb1 100644 --- a/services/docstore/app.js +++ b/services/docstore/app.js @@ -50,6 +50,10 @@ app.param('doc_id', function (req, res, next, docId) { app.get('/project/:project_id/doc-deleted', HttpController.getAllDeletedDocs) app.get('/project/:project_id/doc', HttpController.getAllDocs) app.get('/project/:project_id/ranges', HttpController.getAllRanges) +app.get( + '/project/:project_id/comment-thread-ids', + HttpController.getCommentThreadIds +) app.get( '/project/:project_id/tracked-changes-user-ids', HttpController.getTrackedChangesUserIds diff --git a/services/docstore/app/js/DocManager.js b/services/docstore/app/js/DocManager.js index 6ecfe29695..00f03436a1 100644 --- a/services/docstore/app/js/DocManager.js +++ b/services/docstore/app/js/DocManager.js @@ -132,6 +132,22 @@ const DocManager = { return docs }, + async getCommentThreadIds(projectId) { + const docs = await DocManager.getAllNonDeletedDocs(projectId, { + _id: true, + ranges: true, + }) + const byDoc = new Map() + for (const doc of docs) { + const ids = new Set() + for (const comment of doc.ranges?.comments || []) { + ids.add(comment.op.t) + } + if (ids.size > 0) byDoc.set(doc._id.toString(), Array.from(ids)) + } + return Object.fromEntries(byDoc.entries()) + }, + async getTrackedChangesUserIds(projectId) { const docs = await DocManager.getAllNonDeletedDocs(projectId, { ranges: true, diff --git a/services/docstore/app/js/HttpController.js b/services/docstore/app/js/HttpController.js index 2e6ec2575a..50c4302aeb 100644 --- a/services/docstore/app/js/HttpController.js +++ b/services/docstore/app/js/HttpController.js @@ -83,6 +83,12 @@ async function getAllRanges(req, res) { res.json(_buildDocsArrayView(projectId, docs)) } +async function getCommentThreadIds(req, res) { + const { project_id: projectId } = req.params + const threadIds = await DocManager.getCommentThreadIds(projectId) + res.json(threadIds) +} + async function getTrackedChangesUserIds(req, res) { const { project_id: projectId } = req.params const userIds = await DocManager.getTrackedChangesUserIds(projectId) @@ -239,6 +245,7 @@ module.exports = { getAllDeletedDocs: expressify(getAllDeletedDocs), getAllRanges: expressify(getAllRanges), getTrackedChangesUserIds: expressify(getTrackedChangesUserIds), + getCommentThreadIds: expressify(getCommentThreadIds), projectHasRanges: expressify(projectHasRanges), updateDoc: expressify(updateDoc), patchDoc: expressify(patchDoc), diff --git a/services/docstore/test/acceptance/js/GettingAllDocsTests.js b/services/docstore/test/acceptance/js/GettingAllDocsTests.js index c5929b78fb..c66c7d15f5 100644 --- a/services/docstore/test/acceptance/js/GettingAllDocsTests.js +++ b/services/docstore/test/acceptance/js/GettingAllDocsTests.js @@ -25,6 +25,9 @@ describe('Getting all docs', function () { _id: new ObjectId(), lines: ['one', 'two', 'three'], ranges: { + comments: [ + { id: new ObjectId().toString(), op: { t: 'thread-id-1' } }, + ], changes: [ { id: new ObjectId().toString(), @@ -51,6 +54,9 @@ describe('Getting all docs', function () { _id: new ObjectId(), lines: ['111', '222', '333'], ranges: { + comments: [ + { id: new ObjectId().toString(), op: { t: 'thread-id-2' } }, + ], changes: [ { id: new ObjectId().toString(), @@ -65,6 +71,7 @@ describe('Getting all docs', function () { _id: new ObjectId(), lines: ['deleted'], ranges: { + comments: [{ id: new ObjectId().toString(), op: { t: 'thread-id-3' } }], changes: [ { id: new ObjectId().toString(), metadata: { user_id: 'user-id-3' } }, ], @@ -147,4 +154,20 @@ describe('Getting all docs', function () { } ) }) + + it('getCommentThreadIds should return all the thread ids from (non-deleted) ranges', function (done) { + DocstoreClient.getCommentThreadIds( + this.project_id, + (error, res, threadIds) => { + if (error != null) { + throw error + } + threadIds.should.deep.equal({ + [this.docs[0]._id.toString()]: ['thread-id-1'], + [this.docs[2]._id.toString()]: ['thread-id-2'], + }) + done() + } + ) + }) }) diff --git a/services/docstore/test/acceptance/js/helpers/DocstoreClient.js b/services/docstore/test/acceptance/js/helpers/DocstoreClient.js index d1833d3503..cb8bce2579 100644 --- a/services/docstore/test/acceptance/js/helpers/DocstoreClient.js +++ b/services/docstore/test/acceptance/js/helpers/DocstoreClient.js @@ -100,6 +100,16 @@ module.exports = DocstoreClient = { ) }, + getCommentThreadIds(projectId, callback) { + request.get( + { + url: `http://127.0.0.1:${settings.internal.docstore.port}/project/${projectId}/comment-thread-ids`, + json: true, + }, + callback + ) + }, + getTrackedChangesUserIds(projectId, callback) { request.get( { diff --git a/services/web/app/src/Features/Docstore/DocstoreManager.js b/services/web/app/src/Features/Docstore/DocstoreManager.js index af84ede5d2..4074b90605 100644 --- a/services/web/app/src/Features/Docstore/DocstoreManager.js +++ b/services/web/app/src/Features/Docstore/DocstoreManager.js @@ -87,6 +87,14 @@ function getAllDeletedDocs(projectId, callback) { }) } +/** + * @param {string} projectId + */ +async function getCommentThreadIds(projectId) { + const url = `${settings.apis.docstore.url}/project/${projectId}/comment-thread-ids` + return fetchJson(url, { signal: AbortSignal.timeout(TIMEOUT) }) +} + /** * @param {string} projectId */ @@ -301,6 +309,7 @@ module.exports = { getAllDeletedDocs, getAllRanges, getDoc, + getCommentThreadIds: callbackify(getCommentThreadIds), getTrackedChangesUserIds: callbackify(getTrackedChangesUserIds), isDocDeleted, updateDoc, @@ -314,6 +323,7 @@ module.exports = { getAllDeletedDocs: promisify(getAllDeletedDocs), getAllRanges: promisify(getAllRanges), getDoc: promisifyMultiResult(getDoc, ['lines', 'rev', 'version', 'ranges']), + getCommentThreadIds, getTrackedChangesUserIds, isDocDeleted: promisify(isDocDeleted), updateDoc: promisifyMultiResult(updateDoc, ['modified', 'rev']), diff --git a/services/web/app/src/Features/History/RestoreManager.js b/services/web/app/src/Features/History/RestoreManager.js index 1db32b998d..0d3e96b97f 100644 --- a/services/web/app/src/Features/History/RestoreManager.js +++ b/services/web/app/src/Features/History/RestoreManager.js @@ -18,6 +18,12 @@ const OError = require('@overleaf/o-error') const ProjectGetter = require('../Project/ProjectGetter') const ProjectEntityHandler = require('../Project/ProjectEntityHandler') +async function getCommentThreadIds(projectId) { + await DocumentUpdaterHandler.promises.flushProjectToMongo(projectId) + const raw = await DocstoreManager.promises.getCommentThreadIds(projectId) + return new Map(Object.entries(raw).map(([doc, ids]) => [doc, new Set(ids)])) +} + const RestoreManager = { async restoreFileFromV2(userId, projectId, version, pathname) { const fsPath = await RestoreManager._writeFileVersionToDisk( @@ -52,6 +58,25 @@ const RestoreManager = { }, async revertFile(userId, projectId, version, pathname, options = {}) { + const threadIds = await getCommentThreadIds(projectId) + return await RestoreManager._revertSingleFile( + userId, + projectId, + version, + pathname, + threadIds, + options + ) + }, + + async _revertSingleFile( + userId, + projectId, + version, + pathname, + threadIds, + options = {} + ) { const project = await ProjectGetter.promises.getProject(projectId, { overleaf: true, }) @@ -115,6 +140,7 @@ const RestoreManager = { origin, userId ) + threadIds.delete(file.element._id.toString()) } const { metadata } = await RestoreManager._getMetadataFromHistory( @@ -154,22 +180,12 @@ const RestoreManager = { const documentCommentIds = new Set( ranges.comments?.map(({ op: { t } }) => t) ) - - await DocumentUpdaterHandler.promises.flushProjectToMongo(projectId) - - const docsWithRanges = - await DocstoreManager.promises.getAllRanges(projectId) - - const nonOrphanedThreadIds = new Set() - for (const { ranges } of docsWithRanges) { - for (const comment of ranges.comments ?? []) { - nonOrphanedThreadIds.add(comment.op.t) + const commentIdsToDuplicate = Array.from(documentCommentIds).filter(id => { + for (const ids of threadIds.values()) { + if (ids.has(id)) return true } - } - - const commentIdsToDuplicate = Array.from(documentCommentIds).filter(id => - nonOrphanedThreadIds.has(id) - ) + return false + }) const newRanges = { changes: ranges.changes, comments: [] } @@ -257,6 +273,11 @@ const RestoreManager = { origin, userId ) + // For revertProject: The next doc that gets reverted will need to duplicate all the threads seen here. + threadIds.set( + _id.toString(), + new Set(newRanges.comments.map(({ op: { t } }) => t)) + ) return { _id, @@ -319,11 +340,17 @@ const RestoreManager = { version, timestamp: new Date(updateAtVersion.meta.end_ts).toISOString(), } + const threadIds = await getCommentThreadIds(projectId) for (const pathname of pathsAtPastVersion) { - await RestoreManager.revertFile(userId, projectId, version, pathname, { - origin, - }) + await RestoreManager._revertSingleFile( + userId, + projectId, + version, + pathname, + threadIds, + { origin } + ) } const entitiesAtLiveVersion = diff --git a/services/web/test/unit/src/History/RestoreManagerTests.js b/services/web/test/unit/src/History/RestoreManagerTests.js index 2474425bfb..a22793de85 100644 --- a/services/web/test/unit/src/History/RestoreManagerTests.js +++ b/services/web/test/unit/src/History/RestoreManagerTests.js @@ -9,6 +9,12 @@ const tk = require('timekeeper') const moment = require('moment') const { expect } = require('chai') +function nestedMapWithSetToObject(m) { + return Object.fromEntries( + Array.from(m.entries()).map(([key, set]) => [key, Array.from(set)]) + ) +} + describe('RestoreManager', function () { beforeEach(function () { tk.freeze(Date.now()) // freeze the time for these tests @@ -28,7 +34,7 @@ describe('RestoreManager', function () { promises: { flushProjectToMongo: sinon.stub().resolves() }, }), '../Docstore/DocstoreManager': (this.DocstoreManager = { - promises: {}, + promises: { getCommentThreadIds: sinon.stub().resolves({}) }, }), '../Chat/ChatApiHandler': (this.ChatApiHandler = { promises: {} }), '../Chat/ChatManager': (this.ChatManager = { promises: {} }), @@ -269,13 +275,9 @@ describe('RestoreManager', function () { { op: { t: 'single-comment', p: 10, c: 'bar' } }, ] this.ProjectLocator.promises.findElementByPath = sinon.stub().rejects() - this.DocstoreManager.promises.getAllRanges = sinon.stub().resolves([ - { - ranges: { - comments: this.comments.slice(0, 1), - }, - }, - ]) + this.DocstoreManager.promises.getCommentThreadIds = sinon + .stub() + .resolves({ 'other-doc': [this.comments[0].op.t] }) this.ChatApiHandler.promises.duplicateCommentThreads = sinon .stub() .resolves({ @@ -355,7 +357,7 @@ describe('RestoreManager', function () { expect( this.DocumentUpdaterHandler.promises.flushProjectToMongo ).to.have.been.calledBefore( - this.DocstoreManager.promises.getAllRanges + this.DocstoreManager.promises.getCommentThreadIds ) }) @@ -451,19 +453,11 @@ describe('RestoreManager', function () { ) }) - it('should delete the document before flushing', function () { - expect( - this.EditorController.promises.deleteEntity - ).to.have.been.calledBefore( - this.DocumentUpdaterHandler.promises.flushProjectToMongo - ) - }) - it('should flush the document before fetching ranges', function () { expect( this.DocumentUpdaterHandler.promises.flushProjectToMongo ).to.have.been.calledBefore( - this.DocstoreManager.promises.getAllRanges + this.DocstoreManager.promises.getCommentThreadIds ) }) @@ -499,6 +493,143 @@ describe('RestoreManager', function () { ) }) }) + + describe('with comments in same doc', function () { + // copy of the above, addition: inject and later inspect threadIds set + beforeEach(async function () { + this.ProjectLocator.promises.findElementByPath = sinon + .stub() + .resolves({ type: 'doc', element: { _id: 'mock-file-id' } }) + this.EditorController.promises.deleteEntity = sinon.stub().resolves() + this.ChatApiHandler.promises.generateThreadData = sinon + .stub() + .resolves( + (this.threadData = { + [this.comments[0].op.t]: { + messages: [ + { + content: 'message', + timestamp: '2024-01-01T00:00:00.000Z', + user_id: 'user-1', + }, + ], + }, + [this.comments[1].op.t]: { + messages: [ + { + content: 'other message', + timestamp: '2024-01-01T00:00:00.000Z', + user_id: 'user-1', + }, + ], + }, + }) + ) + + this.threadIds = new Map([ + [ + 'mock-file-id', + new Set([this.comments[0].op.t, this.comments[1].op.t]), + ], + ]) + // Comments are updated in-place. Look up threads before reverting. + this.afterThreadIds = { + // mock-file-id removed + [this.addedFile._id]: [ + this.comments[0].op.t, + this.comments[1].op.t, + ], + } + this.data = await this.RestoreManager.promises._revertSingleFile( + this.user_id, + this.project_id, + this.version, + this.pathname, + this.threadIds + ) + }) + + it('should import the file with original comments minus the deleted one', function () { + expect( + this.EditorController.promises.addDocWithRanges + ).to.have.been.calledWith( + this.project_id, + this.folder_id, + 'foo.tex', + ['foo', 'bar', 'baz'], + { + changes: this.tracked_changes, + comments: this.comments.slice(0, 2), + }, + { + kind: 'file-restore', + path: this.pathname, + version: this.version, + timestamp: new Date(this.endTs).toISOString(), + } + ) + }) + + it('should add the seen thread ids to the map', function () { + expect(nestedMapWithSetToObject(this.threadIds)).to.deep.equal( + this.afterThreadIds + ) + }) + }) + + describe('with remapped comments during revertProject', function () { + // copy of the above, addition: inject and later inspect threadIds set + beforeEach(async function () { + this.ProjectLocator.promises.findElementByPath = sinon + .stub() + .resolves({ type: 'doc', element: { _id: 'mock-file-id' } }) + this.EditorController.promises.deleteEntity = sinon.stub().resolves() + + this.threadIds = new Map([ + ['other-doc', new Set([this.comments[0].op.t])], + ]) + // Comments are updated in-place. Look up threads before reverting. + this.afterThreadIds = { + // mock-file-id removed + 'other-doc': [this.comments[0].op.t], + [this.addedFile._id]: [ + this.remappedComments[0].op.t, + this.remappedComments[1].op.t, + ], + } + this.data = await this.RestoreManager.promises._revertSingleFile( + this.user_id, + this.project_id, + this.version, + this.pathname, + this.threadIds + ) + }) + + it('should import the file', function () { + expect( + this.EditorController.promises.addDocWithRanges + ).to.have.been.calledWith( + this.project_id, + this.folder_id, + 'foo.tex', + ['foo', 'bar', 'baz'], + { changes: this.tracked_changes, comments: this.remappedComments }, + { + kind: 'file-restore', + path: this.pathname, + version: this.version, + timestamp: new Date(this.endTs).toISOString(), + } + ) + }) + + it('should add the seen thread ids to the map', function () { + expect(nestedMapWithSetToObject(this.threadIds)).to.deep.equal( + this.afterThreadIds + ) + }) + }) }) describe('reverting a file or document with metadata', function () { @@ -524,7 +655,9 @@ describe('RestoreManager', function () { .stub() .resolves((this.addedFile = { _id: 'mock-doc-id', type: 'doc' })) - this.DocstoreManager.promises.getAllRanges = sinon.stub().resolves([]) + this.DocstoreManager.promises.getCommentThreadIds = sinon + .stub() + .resolves({}) this.ChatApiHandler.promises.generateThreadData = sinon .stub() .resolves({}) @@ -741,7 +874,7 @@ describe('RestoreManager', function () { this.ProjectGetter.promises.getProject .withArgs(this.project_id) .resolves({ overleaf: { history: { rangesSupportEnabled: true } } }) - this.RestoreManager.promises.revertFile = sinon.stub().resolves() + this.RestoreManager.promises._revertSingleFile = sinon.stub().resolves() this.RestoreManager.promises._getProjectPathsAtVersion = sinon .stub() .resolves([]) @@ -832,21 +965,27 @@ describe('RestoreManager', function () { }) it('should revert the old files', function () { - expect(this.RestoreManager.promises.revertFile).to.have.been.calledWith( + expect( + this.RestoreManager.promises._revertSingleFile + ).to.have.been.calledWith( this.user_id, this.project_id, this.version, 'main.tex' ) - expect(this.RestoreManager.promises.revertFile).to.have.been.calledWith( + expect( + this.RestoreManager.promises._revertSingleFile + ).to.have.been.calledWith( this.user_id, this.project_id, this.version, 'figures/image.png' ) - expect(this.RestoreManager.promises.revertFile).to.have.been.calledWith( + expect( + this.RestoreManager.promises._revertSingleFile + ).to.have.been.calledWith( this.user_id, this.project_id, this.version, @@ -856,7 +995,7 @@ describe('RestoreManager', function () { it('should not revert the current files', function () { expect( - this.RestoreManager.promises.revertFile + this.RestoreManager.promises._revertSingleFile ).to.not.have.been.calledWith( this.user_id, this.project_id,