From 00a2765d0153391d87d9a7074623b9a9fc59b3ee Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Tue, 28 Jul 2026 11:33:37 +0200 Subject: [PATCH] [document-updater] add track-changes support to setDoc (#35820) GitOrigin-RevId: 5d39dd1945e710ec3f7068830fb2476e59f0f2a5 --- services/document-updater/app.js | 2 + .../app/js/DocumentManager.js | 44 ++- .../document-updater/app/js/HttpController.js | 16 +- .../acceptance/js/SettingADocumentTests.js | 306 ++++++++++++++++++ .../acceptance/js/helpers/DocUpdaterClient.js | 26 +- .../DocumentManager/DocumentManagerTests.js | 33 ++ .../DocumentUpdaterHandler.mjs | 20 +- .../Project/ProjectEntityUpdateHandler.mjs | 47 ++- .../src/Features/Project/ProjectHelper.mjs | 17 + .../test/acceptance/src/TpdsUpdateTests.mjs | 172 ++++++++++ .../src/mocks/MockDocUpdaterApi.mjs | 9 + .../DocumentUpdaterHandler.test.mjs | 54 ++++ .../ProjectEntityUpdateHandler.test.mjs | 128 +++++++- .../unit/src/Project/ProjectHelper.test.mjs | 61 ++++ 14 files changed, 901 insertions(+), 34 deletions(-) diff --git a/services/document-updater/app.js b/services/document-updater/app.js index a5f81aa1a2..f64b9031d2 100644 --- a/services/document-updater/app.js +++ b/services/document-updater/app.js @@ -220,6 +220,8 @@ app.use((error, req, res, next) => { return res.status(413).send('request entity too large') } else if (error instanceof Errors.DocumentValidationError) { return res.sendStatus(422) + } else if (error instanceof Errors.OTTypeMismatchError) { + return res.sendStatus(422) } else { logger.error({ err: error, req }, 'request errored') return res.status(500).send('Oops, something went wrong') diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index 8f1f48db4a..75a531d2a4 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -11,6 +11,7 @@ const RangesManager = require('./RangesManager') const { extractOriginOrSource } = require('./Utils') const { getTotalSizeOfLines } = require('./Limits') const Settings = require('@overleaf/settings') +const RangesTracker = require('@overleaf/ranges-tracker') const { StringFileData } = require('overleaf-editor-core') const { diffAsTextOperation, @@ -134,14 +135,20 @@ const DocumentManager = { } }, - async appendToDoc(projectId, docId, linesToAppend, originOrSource, userId) { + async appendToDoc( + projectId, + docId, + linesToAppend, + originOrSource, + userId, + trackChanges + ) { let { lines: currentLines, type } = await DocumentManager.getDoc( projectId, docId ) if (type === 'history-ot') { const file = StringFileData.fromRaw(currentLines) - // TODO(24596): tc support for history-ot currentLines = file.getLines() } const currentLineSize = getTotalSizeOfLines(currentLines) @@ -161,7 +168,8 @@ const DocumentManager = { originOrSource, userId, false, - false + false, + trackChanges ) }, @@ -172,7 +180,8 @@ const DocumentManager = { originOrSource, userId, undoing, - external + external, + trackChanges ) { if (newLines == null) { throw new Error('No lines were provided to setDoc') @@ -197,7 +206,11 @@ const DocumentManager = { let op if (type === 'history-ot') { const file = StringFileData.fromRaw(oldLines) - const operation = diffAsTextOperation(file, newLines.join('\n')) + const operation = diffAsTextOperation( + file, + newLines.join('\n'), + trackChanges ? { tracking: { userId, ts: new Date() } } : {} + ) if (operation.isNoop()) { op = [] } else { @@ -222,6 +235,9 @@ const DocumentManager = { user_id: userId, }, } + if (type !== 'history-ot' && trackChanges) { + update.meta.tc = RangesTracker.generateIdSeed() + } if (external) { update.meta.type = 'external' } @@ -666,7 +682,8 @@ const DocumentManager = { source, userId, undoing, - external + external, + trackChanges ) { const UpdateManager = require('./UpdateManager') return await UpdateManager.promises.lockUpdatesAndDo( @@ -677,11 +694,19 @@ const DocumentManager = { source, userId, undoing, - external + external, + trackChanges ) }, - async appendToDocWithLock(projectId, docId, lines, source, userId) { + async appendToDocWithLock( + projectId, + docId, + lines, + source, + userId, + trackChanges + ) { const UpdateManager = require('./UpdateManager') return await UpdateManager.promises.lockUpdatesAndDo( DocumentManager.appendToDoc, @@ -689,7 +714,8 @@ const DocumentManager = { docId, lines, source, - userId + userId, + trackChanges ) }, diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index a09e3618d6..8b7ae3088b 100644 --- a/services/document-updater/app/js/HttpController.js +++ b/services/document-updater/app/js/HttpController.js @@ -199,7 +199,10 @@ async function clearProjectState(req, res) { async function setDoc(req, res) { const docId = req.params.doc_id const projectId = req.params.project_id - const { lines, source, user_id: userId, undoing } = req.body + const { lines, source, user_id: userId, undoing, trackChanges } = req.body + if (trackChanges && !userId) { + return res.status(400).send('track changes requires a user id') + } const lineSize = getTotalSizeOfLines(lines) if (lineSize > Settings.max_doc_length) { @@ -222,7 +225,8 @@ async function setDoc(req, res) { source, userId, undoing, - true + true, + trackChanges ) timer.done() logger.debug({ projectId, docId }, 'set doc via http') @@ -236,7 +240,10 @@ async function setDoc(req, res) { async function appendToDoc(req, res) { const docId = req.params.doc_id const projectId = req.params.project_id - const { lines, source, user_id: userId } = req.body + const { lines, source, user_id: userId, trackChanges } = req.body + if (trackChanges && !userId) { + return res.status(400).send('track changes requires a user id') + } const timer = new Metrics.Timer('http.appendToDoc') let result @@ -246,7 +253,8 @@ async function appendToDoc(req, res) { docId, lines, source, - userId + userId, + trackChanges ) } catch (error) { if (error instanceof Errors.FileTooLargeError) { diff --git a/services/document-updater/test/acceptance/js/SettingADocumentTests.js b/services/document-updater/test/acceptance/js/SettingADocumentTests.js index 4d5db8163f..2efda07d23 100644 --- a/services/document-updater/test/acceptance/js/SettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/SettingADocumentTests.js @@ -756,6 +756,312 @@ describe('Setting a document', function () { }) }) } + + describe('with track changes enabled', function () { + async function getDocRaw(docId) { + return JSON.parse( + await docUpdaterRedis.get(Keys.docLines({ doc_id: docId })) + ) + } + + function expectRecentTimestamp(tracking) { + expect(new Date(tracking.ts).getTime()).to.be.closeTo( + Date.now(), + 30_000 + ) + return tracking.ts + } + + describe('when adding content', function () { + beforeEach(async function () { + this.body = await DocUpdaterClient.setDocLines( + this.project_id, + this.doc_id, + ['one', 'INSERT', 'two', 'three'], + this.source, + userId, + false, + true + ) + }) + + it('should record the insertion as a tracked insert', async function () { + const data = await getDocRaw(this.doc_id) + const insertTs = expectRecentTimestamp( + data.trackedChanges[0].tracking + ) + expect(data).to.deep.equal({ + content: 'one\nINSERT\none and a half\ntwo\nthree', + trackedChanges: [ + { + range: { pos: 4, length: 'INSERT\n'.length }, + tracking: { ts: insertTs, type: 'insert', userId }, + }, + { + range: { pos: 'INSERT\n'.length + 4, length: 15 }, + tracking: { ts, type: 'delete', userId }, + }, + ], + }) + }) + }) + + describe('when removing content', function () { + beforeEach(async function () { + this.body = await DocUpdaterClient.setDocLines( + this.project_id, + this.doc_id, + ['one', 'two'], + this.source, + userId, + false, + true + ) + }) + + it('should record the removal as a tracked delete, keeping the content', async function () { + const data = await getDocRaw(this.doc_id) + const deleteTs = expectRecentTimestamp( + data.trackedChanges[1].tracking + ) + expect(data).to.deep.equal({ + content: 'one\none and a half\ntwo\nthree', + trackedChanges: [ + { + range: { pos: 4, length: 15 }, + tracking: { ts, type: 'delete', userId }, + }, + { + range: { pos: 22, length: '\nthree'.length }, + tracking: { ts: deleteTs, type: 'delete', userId }, + }, + ], + }) + }) + + it('should hide the removed content from the doc lines', async function () { + const doc = await DocUpdaterClient.getDoc( + this.project_id, + this.doc_id + ) + expect(doc.lines).to.deep.equal(['one', 'two']) + }) + }) + + describe('when appending content', function () { + beforeEach(async function () { + this.body = await DocUpdaterClient.appendToDoc( + this.project_id, + this.doc_id, + ['four'], + this.source, + userId, + true + ) + }) + + it('should record the appended content as a tracked insert', async function () { + const data = await getDocRaw(this.doc_id) + const insertTs = expectRecentTimestamp( + data.trackedChanges[1].tracking + ) + expect(data).to.deep.equal({ + content: 'one\none and a half\ntwo\nthree\nfour', + trackedChanges: [ + { + range: { pos: 4, length: 15 }, + tracking: { ts, type: 'delete', userId }, + }, + { + range: { pos: 28, length: '\nfour'.length }, + tracking: { ts: insertTs, type: 'insert', userId }, + }, + ], + }) + }) + + it('should include the appended content in the doc lines', async function () { + const doc = await DocUpdaterClient.getDoc( + this.project_id, + this.doc_id + ) + expect(doc.lines).to.deep.equal(['one', 'two', 'three', 'four']) + }) + }) + }) + }) + + describe('with track changes enabled (sharejs-text-ot)', function () { + const lines = ['one', 'one and a half', 'two', 'three'] + const idSeed = '587357bd35e64f6157' + const userId = DocUpdaterClient.randomId() + + beforeEach(async function () { + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() + this.update = { + doc: this.doc_id, + op: [ + { + d: 'one and a half\n', + p: 4, + }, + ], + meta: { + tc: idSeed, + user_id: userId, + }, + v: this.version, + } + MockWebApi.insertDoc(this.project_id, this.doc_id, { + lines, + version: this.version, + }) + await DocUpdaterClient.preloadDoc(this.project_id, this.doc_id) + await DocUpdaterClient.sendUpdate( + this.project_id, + this.doc_id, + this.update + ) + await DocUpdaterClient.waitForPendingUpdates(this.doc_id) + }) + + afterEach(function () { + MockProjectHistoryApi.flushProject.resetHistory() + MockWebApi.setDocument.resetHistory() + }) + + function expectPreservedTrackedDelete(changes) { + const deleteChange = changes.find( + change => change.op.d === 'one and a half\n' + ) + expect(deleteChange.op).to.deep.equal({ d: 'one and a half\n', p: 4 }) + expect(deleteChange.id).to.equal(idSeed + '000001') + expect(deleteChange.metadata.user_id).to.equal(userId) + } + + function expectNewTrackedChange(change, op) { + expect(change.op).to.deep.equal(op) + expect(change.id).to.match(/^[0-9a-f]{24}$/) + expect(change.id.startsWith(idSeed)).to.equal(false) + expect(change.metadata.user_id).to.equal(userId) + expect(new Date(change.metadata.ts).getTime()).to.be.closeTo( + Date.now(), + 30_000 + ) + } + + describe('when adding content', function () { + beforeEach(async function () { + this.body = await DocUpdaterClient.setDocLines( + this.project_id, + this.doc_id, + ['one', 'two', 'INSERT', 'three'], + this.source, + userId, + false, + true + ) + }) + + it('should record the insertion as a tracked insert', async function () { + const doc = await DocUpdaterClient.getDoc(this.project_id, this.doc_id) + expect(doc.lines).to.deep.equal(['one', 'two', 'INSERT', 'three']) + expect(doc.ranges.changes).to.have.length(2) + expectPreservedTrackedDelete(doc.ranges.changes) + const insertChange = doc.ranges.changes.find( + change => change.op.i != null + ) + expectNewTrackedChange(insertChange, { i: 'INSERT\n', p: 8 }) + }) + }) + + describe('when removing content', function () { + beforeEach(async function () { + this.body = await DocUpdaterClient.setDocLines( + this.project_id, + this.doc_id, + ['one', 'two'], + this.source, + userId, + false, + true + ) + }) + + it('should record the removal as a tracked delete', async function () { + const doc = await DocUpdaterClient.getDoc(this.project_id, this.doc_id) + expect(doc.lines).to.deep.equal(['one', 'two']) + expect(doc.ranges.changes).to.have.length(2) + expectPreservedTrackedDelete(doc.ranges.changes) + const newDeleteChange = doc.ranges.changes.find( + change => change.op.d === '\nthree' + ) + expectNewTrackedChange(newDeleteChange, { d: '\nthree', p: 7 }) + }) + }) + + describe('when appending content', function () { + beforeEach(async function () { + this.body = await DocUpdaterClient.appendToDoc( + this.project_id, + this.doc_id, + ['four'], + this.source, + userId, + true + ) + }) + + it('should record the appended content as a tracked insert', async function () { + const doc = await DocUpdaterClient.getDoc(this.project_id, this.doc_id) + expect(doc.lines).to.deep.equal(['one', 'two', 'three', 'four']) + expect(doc.ranges.changes).to.have.length(2) + expectPreservedTrackedDelete(doc.ranges.changes) + const insertChange = doc.ranges.changes.find( + change => change.op.i != null + ) + expectNewTrackedChange(insertChange, { i: '\nfour', p: 13 }) + }) + }) + }) + + describe('when track changes is requested without a user id', function () { + beforeEach(function () { + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() + }) + + it('should reject setting the document with a 400', async function () { + await expect( + DocUpdaterClient.setDocLines( + this.project_id, + this.doc_id, + this.newLines, + this.source, + undefined, + false, + true + ) + ) + .to.be.rejectedWith(RequestFailedError) + .and.eventually.have.nested.property('response.status', 400) + }) + + it('should reject appending to the document with a 400', async function () { + await expect( + DocUpdaterClient.appendToDoc( + this.project_id, + this.doc_id, + this.newLines, + this.source, + undefined, + true + ) + ) + .to.be.rejectedWith(RequestFailedError) + .and.eventually.have.nested.property('response.status', 400) + }) }) describe('when the first request returns a connection error', function () { beforeEach(function () { diff --git a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js index 3c172ee495..60dbe958f9 100644 --- a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js +++ b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js @@ -111,7 +111,15 @@ module.exports = DocUpdaterClient = { ) }, - async setDocLines(projectId, docId, lines, source, userId, undoing) { + async setDocLines( + projectId, + docId, + lines, + source, + userId, + undoing, + trackChanges + ) { return await fetchJson( `http://127.0.0.1:3003/project/${projectId}/doc/${docId}`, { @@ -121,6 +129,22 @@ module.exports = DocUpdaterClient = { source, user_id: userId, undoing, + trackChanges, + }, + } + ) + }, + + async appendToDoc(projectId, docId, lines, source, userId, trackChanges) { + return await fetchJson( + `http://127.0.0.1:3003/project/${projectId}/doc/${docId}/append`, + { + method: 'POST', + json: { + lines, + source, + user_id: userId, + trackChanges, }, } ) diff --git a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js index 0d4b2fd33d..774dcfacfa 100644 --- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js +++ b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js @@ -585,6 +585,39 @@ describe('DocumentManager', function () { }) }) + describe('with track changes enabled', function () { + beforeEach(async function () { + await this.DocumentManager.promises.setDoc( + this.project_id, + this.doc_id, + this.afterLines, + this.source, + this.user_id, + false, + true, + true + ) + }) + + it('should apply the diff with a track changes id seed', function () { + this.UpdateManager.promises.applyUpdate.should.have.been.calledWith( + this.project_id, + this.doc_id, + sinon.match({ + doc: this.doc_id, + v: this.version, + op: this.ops, + meta: sinon.match({ + type: 'external', + source: this.source, + user_id: this.user_id, + tc: sinon.match(/^[0-9a-f]{18}$/), + }), + }) + ) + }) + }) + describe('when not already loaded', function () { beforeEach(async function () { this.DocumentManager.promises.getDoc = sinon.stub().resolves({ diff --git a/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.mjs b/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.mjs index ba1a8f205e..a36e532825 100644 --- a/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.mjs +++ b/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.mjs @@ -114,7 +114,14 @@ async function getDocumentWithHistoryRanges(projectId, docId) { return doc } -async function setDocument(projectId, docId, userId, docLines, source) { +async function setDocument( + projectId, + docId, + userId, + docLines, + source, + trackChanges = false +) { const maybeJson = await fetchString( `${BASE_URL}/project/${projectId}/doc/${docId}`, { @@ -123,6 +130,7 @@ async function setDocument(projectId, docId, userId, docLines, source) { lines: docLines, source, user_id: userId, + trackChanges, }, signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS), } @@ -138,7 +146,14 @@ async function setDocument(projectId, docId, userId, docLines, source) { } } -async function appendToDocument(projectId, docId, userId, lines, source) { +async function appendToDocument( + projectId, + docId, + userId, + lines, + source, + trackChanges = false +) { const maybeJson = await fetchString( `${BASE_URL}/project/${projectId}/doc/${docId}/append`, { @@ -147,6 +162,7 @@ async function appendToDocument(projectId, docId, userId, lines, source) { lines, source, user_id: userId, + trackChanges, }, signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS), } diff --git a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.mjs b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.mjs index 2f53f2dd1d..525f32eff5 100644 --- a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.mjs +++ b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.mjs @@ -13,6 +13,7 @@ import LockManager from '../../infrastructure/LockManager.mjs' import { Project } from '../../models/Project.mjs' import ProjectEntityHandler from './ProjectEntityHandler.mjs' import ProjectGetter from './ProjectGetter.mjs' +import ProjectHelper from './ProjectHelper.mjs' import ProjectLocator from './ProjectLocator.mjs' import ProjectOptionsHandler from './ProjectOptionsHandler.mjs' import ProjectUpdateHandler from './ProjectUpdateHandler.mjs' @@ -388,11 +389,19 @@ const upsertDoc = wrapWithLock( if (!SafePath.isCleanFilename(docName)) { throw new Errors.InvalidNameError('invalid element name') } + const project = await ProjectGetter.promises.getProject(projectId, { + rootFolder: true, + rootDoc_id: true, + track_changes: true, + }) + if (project == null) { + throw new Errors.NotFoundError('project not found') + } let element, folderPath try { ;({ element, path: folderPath } = await ProjectLocator.promises.findElement({ - project_id: projectId, + project, element_id: folderId, type: 'folder', })) @@ -421,7 +430,7 @@ const upsertDoc = wrapWithLock( ) doc.rev = rev - const project = + const updatedProject = await ProjectEntityMongoUpdateHandler.promises.replaceFileWithDoc( projectId, existingFile._id, @@ -433,15 +442,15 @@ const upsertDoc = wrapWithLock( projectId, docId: doc._id, path: filePath, - projectName: project.name, + projectName: updatedProject.name, rev: existingFile.rev + 1, folderId, }) const projectHistoryId = - project.overleaf && - project.overleaf.history && - project.overleaf.history.id + updatedProject.overleaf && + updatedProject.overleaf.history && + updatedProject.overleaf.history.id const newDocs = [ { doc, @@ -459,7 +468,7 @@ const upsertDoc = wrapWithLock( projectId, projectHistoryId, userId, - { oldFiles, newDocs, newProject: project }, + { oldFiles, newDocs, newProject: updatedProject }, source ) @@ -471,12 +480,17 @@ const upsertDoc = wrapWithLock( ) return { doc, isNew: true } } else if (existingDoc) { + const trackChanges = ProjectHelper.isTrackChangesEnabledForUser( + project.track_changes, + userId + ) const result = await DocumentUpdaterHandler.promises.setDocument( projectId, existingDoc._id, userId, docLines, - source + source, + trackChanges ) logger.debug( { projectId, docId: existingDoc._id }, @@ -509,18 +523,31 @@ const upsertDoc = wrapWithLock( const appendToDoc = wrapWithLock( async (projectId, docId, lines, source, userId) => { + const project = await ProjectGetter.promises.getProject(projectId, { + rootFolder: true, + rootDoc_id: true, + track_changes: true, + }) + if (project == null) { + throw new Errors.NotFoundError('project not found') + } const { element } = await ProjectLocator.promises.findElement({ - project_id: projectId, + project, element_id: docId, type: 'doc', }) + const trackChanges = ProjectHelper.isTrackChangesEnabledForUser( + project.track_changes, + userId + ) return await DocumentUpdaterHandler.promises.appendToDocument( projectId, element._id, userId, lines, - source + source, + trackChanges ) } ) diff --git a/services/web/app/src/Features/Project/ProjectHelper.mjs b/services/web/app/src/Features/Project/ProjectHelper.mjs index 1e9b8d8f10..4b3dadc651 100644 --- a/services/web/app/src/Features/Project/ProjectHelper.mjs +++ b/services/web/app/src/Features/Project/ProjectHelper.mjs @@ -23,6 +23,7 @@ export default { isArchived, isTrashed, isArchivedOrTrashed, + isTrackChangesEnabledForUser, getAllowedImagesForUser, ensureNameIsUnique, } @@ -62,6 +63,22 @@ function isArchivedOrTrashed(project, userId) { return isArchived(project, userId) || isTrashed(project, userId) } +/** + * Whether track changes is enabled for the given user, from the project's + * track_changes state: `true` means on for everyone (legacy format), + * otherwise it is an object keyed by user id. + * + * @param {boolean | Record | undefined} trackChangesState + * @param {string | ObjectId | null} userId + * @returns {boolean} + */ +function isTrackChangesEnabledForUser(trackChangesState, userId) { + return ( + trackChangesState === true || + (userId != null && trackChangesState?.[userId.toString()] === true) + ) +} + /** * @param {string[]} nameList * @param {string} name diff --git a/services/web/test/acceptance/src/TpdsUpdateTests.mjs b/services/web/test/acceptance/src/TpdsUpdateTests.mjs index 890fba2585..335342324d 100644 --- a/services/web/test/acceptance/src/TpdsUpdateTests.mjs +++ b/services/web/test/acceptance/src/TpdsUpdateTests.mjs @@ -1,12 +1,20 @@ import { expect } from 'chai' import sinon from 'sinon' +import { Project } from '../../../app/src/models/Project.mjs' import ProjectGetter from '../../../app/src/Features/Project/ProjectGetter.mjs' import ProjectOptionsHandler from '../../../app/src/Features/Project/ProjectOptionsHandler.mjs' import ProjectRootDocManager from '../../../app/src/Features/Project/ProjectRootDocManager.mjs' +import MockDocUpdaterApiClass from './mocks/MockDocUpdaterApi.mjs' import request from './helpers/request.js' import User from './helpers/User.mjs' import { db, ObjectId } from '../../../app/src/infrastructure/mongodb.mjs' +let MockDocUpdaterApi + +before(function () { + MockDocUpdaterApi = MockDocUpdaterApiClass.instance() +}) + describe('TpdsUpdateTests', function () { beforeEach(function () { // Creating a project via TPDS schedules a root doc reset on a 30s timer, @@ -78,6 +86,170 @@ describe('TpdsUpdateTests', function () { }) }) + describe('updating an existing file as a user', function () { + function updateMainTex(userId, done) { + request( + { + method: 'POST', + url: `/user/${userId}/update/test-project/main.tex`, + auth: { + username: 'overleaf', + password: 'password', + sendImmediately: true, + }, + body: 'test one two', + }, + (error, response, body) => { + if (error) { + throw error + } + expect(response.statusCode).to.equal(200) + expect(JSON.parse(response.body).status).to.equal('applied') + done() + } + ) + } + + describe('with track changes disabled', function () { + beforeEach(function (done) { + updateMainTex(this.owner._id, done) + }) + + it('should send the update to document-updater without track changes', function () { + const requests = MockDocUpdaterApi.getReceivedSetDocRequests( + this.projectId + ) + expect(requests).to.have.length(1) + expect(requests[0].body.lines).to.deep.equal(['test one two']) + expect(requests[0].body.user_id).to.equal(this.owner._id.toString()) + expect(requests[0].body.trackChanges).to.equal(false) + }) + }) + + describe('with track changes enabled for the user', function () { + beforeEach(function (done) { + Project.updateOne( + { _id: this.projectId }, + { track_changes: { [this.owner._id.toString()]: true } } + ) + .then(() => updateMainTex(this.owner._id, done)) + .catch(done) + }) + + it('should ask document-updater to record the update as tracked changes', function () { + const requests = MockDocUpdaterApi.getReceivedSetDocRequests( + this.projectId + ) + expect(requests).to.have.length(1) + expect(requests[0].body.user_id).to.equal(this.owner._id.toString()) + expect(requests[0].body.trackChanges).to.equal(true) + }) + }) + + describe('with track changes enabled for another user', function () { + beforeEach(function (done) { + Project.updateOne( + { _id: this.projectId }, + { track_changes: { '5c41deb2b4ca500153340809': true } } + ) + .then(() => updateMainTex(this.owner._id, done)) + .catch(done) + }) + + it('should not ask document-updater to record the update as tracked changes', function () { + const requests = MockDocUpdaterApi.getReceivedSetDocRequests( + this.projectId + ) + expect(requests).to.have.length(1) + expect(requests[0].body.trackChanges).to.equal(false) + }) + }) + }) + + describe('updating an existing file as a user by project id', function () { + beforeEach(function (done) { + Project.updateOne( + { _id: this.projectId }, + { track_changes: { [this.owner._id.toString()]: true } } + ) + .then(() => { + request( + { + method: 'POST', + url: `/project/${this.projectId}/user/${this.owner._id}/update/main.tex`, + auth: { + username: 'overleaf', + password: 'password', + sendImmediately: true, + }, + body: 'test one two', + }, + (error, response, body) => { + if (error) { + throw error + } + expect(response.statusCode).to.equal(200) + expect(JSON.parse(response.body).status).to.equal('applied') + done() + } + ) + }) + .catch(done) + }) + + it('should ask document-updater to record the update as tracked changes', function () { + const requests = MockDocUpdaterApi.getReceivedSetDocRequests( + this.projectId + ) + expect(requests).to.have.length(1) + expect(requests[0].body.user_id).to.equal(this.owner._id.toString()) + expect(requests[0].body.trackChanges).to.equal(true) + }) + }) + + describe('updating an existing file via the project contents route', function () { + // Updates via the project contents route (used by github-sync) carry no + // user id: the synced commits can come from any GitHub user, so they are + // never recorded as tracked changes. + beforeEach(function (done) { + Project.updateOne( + { _id: this.projectId }, + { track_changes: { [this.owner._id.toString()]: true } } + ) + .then(() => { + request( + { + method: 'POST', + url: `/project/${this.projectId}/contents/main.tex`, + auth: { + username: 'overleaf', + password: 'password', + sendImmediately: true, + }, + body: 'test one two', + }, + (error, response, body) => { + if (error) { + throw error + } + expect(response.statusCode).to.equal(200) + done() + } + ) + }) + .catch(done) + }) + + it('should send the update to document-updater without track changes', function () { + const requests = MockDocUpdaterApi.getReceivedSetDocRequests( + this.projectId + ) + expect(requests).to.have.length(1) + expect(requests[0].body.user_id).to.equal(null) + expect(requests[0].body.trackChanges).to.equal(false) + }) + }) + describe('deleting a file', function () { beforeEach(function (done) { request( diff --git a/services/web/test/acceptance/src/mocks/MockDocUpdaterApi.mjs b/services/web/test/acceptance/src/mocks/MockDocUpdaterApi.mjs index 02414c7c03..5cb731b0ab 100644 --- a/services/web/test/acceptance/src/mocks/MockDocUpdaterApi.mjs +++ b/services/web/test/acceptance/src/mocks/MockDocUpdaterApi.mjs @@ -4,6 +4,13 @@ class MockDocUpdaterApi extends AbstractMockApi { reset() { this.updates = {} this.docsByProject = new Map() + this.receivedSetDocRequests = [] + } + + getReceivedSetDocRequests(projectId) { + return this.receivedSetDocRequests.filter( + request => request.projectId === projectId + ) } getProjectStructureUpdates(projectId) { @@ -63,6 +70,8 @@ class MockDocUpdaterApi extends AbstractMockApi { ) this.app.post('/project/:projectId/doc/:doc_id', (req, res) => { + const { projectId, doc_id: docId } = req.params + this.receivedSetDocRequests.push({ projectId, docId, body: req.body }) res.sendStatus(204) }) diff --git a/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandler.test.mjs b/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandler.test.mjs index 358ed4b83c..85408ee0f4 100644 --- a/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandler.test.mjs +++ b/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandler.test.mjs @@ -282,6 +282,7 @@ describe('DocumentUpdaterHandler', function () { lines: ctx.lines, source: ctx.source, user_id: ctx.user_id, + trackChanges: false, }) .reply(204) }) @@ -298,6 +299,31 @@ describe('DocumentUpdaterHandler', function () { }) }) + describe('with track changes enabled', function () { + beforeEach(function (ctx) { + ctx.docUpdaterMock + .post(`/project/${ctx.project_id}/doc/${ctx.doc_id}`, { + lines: ctx.lines, + source: ctx.source, + user_id: ctx.user_id, + trackChanges: true, + }) + .reply(204) + }) + + it('should forward the trackChanges flag', async function (ctx) { + await ctx.handler.promises.setDocument( + ctx.project_id, + ctx.doc_id, + ctx.user_id, + ctx.lines, + ctx.source, + true + ) + expect(ctx.docUpdaterMock.isDone()).to.be.true + }) + }) + describe('when the document updater API returns an error', function () { beforeEach(function (ctx) { ctx.docUpdaterMock @@ -305,6 +331,7 @@ describe('DocumentUpdaterHandler', function () { lines: ctx.lines, source: ctx.source, user_id: ctx.user_id, + trackChanges: false, }) .replyWithError('boom') }) @@ -329,6 +356,7 @@ describe('DocumentUpdaterHandler', function () { lines: ctx.lines, source: ctx.source, user_id: ctx.user_id, + trackChanges: false, }) .reply(500) }) @@ -1603,6 +1631,7 @@ describe('DocumentUpdaterHandler', function () { lines: ctx.lines, source: ctx.source, user_id: ctx.user_id, + trackChanges: false, }) .reply(200) }) @@ -1619,6 +1648,31 @@ describe('DocumentUpdaterHandler', function () { }) }) + describe('with track changes enabled', function () { + beforeEach(function (ctx) { + ctx.docUpdaterMock + .post(`/project/${ctx.project_id}/doc/${ctx.doc_id}/append`, { + lines: ctx.lines, + source: ctx.source, + user_id: ctx.user_id, + trackChanges: true, + }) + .reply(200) + }) + + it('should forward the trackChanges flag', async function (ctx) { + await ctx.handler.promises.appendToDocument( + ctx.project_id, + ctx.doc_id, + ctx.user_id, + ctx.lines, + ctx.source, + true + ) + expect(ctx.docUpdaterMock.isDone()).to.be.true + }) + }) + describe('when the document updater API returns an error', function () { beforeEach(function (ctx) { ctx.docUpdaterMock diff --git a/services/web/test/unit/src/Project/ProjectEntityUpdateHandler.test.mjs b/services/web/test/unit/src/Project/ProjectEntityUpdateHandler.test.mjs index 959c36e1c8..1405b8630d 100644 --- a/services/web/test/unit/src/Project/ProjectEntityUpdateHandler.test.mjs +++ b/services/web/test/unit/src/Project/ProjectEntityUpdateHandler.test.mjs @@ -112,6 +112,9 @@ describe('ProjectEntityUpdateHandler', function () { getProject: sinon.stub(), }, } + ctx.ProjectHelper = { + isTrackChangesEnabledForUser: sinon.stub().returns(false), + } ctx.ProjectLocator = { promises: { findElement: sinon.stub(), @@ -217,6 +220,10 @@ describe('ProjectEntityUpdateHandler', function () { default: ctx.ProjectGetter, })) + vi.doMock('../../../../app/src/Features/Project/ProjectHelper', () => ({ + default: ctx.ProjectHelper, + })) + vi.doMock('../../../../app/src/Features/Project/ProjectLocator', () => ({ default: ctx.ProjectLocator, })) @@ -838,6 +845,10 @@ describe('ProjectEntityUpdateHandler', function () { }) describe('upsertDoc', function () { + beforeEach(function (ctx) { + ctx.ProjectGetter.promises.getProject.resolves(ctx.project) + }) + describe('upserting into an invalid folder', function () { beforeEach(function (ctx) { ctx.ProjectLocator.promises.findElement.resolves({ element: null }) @@ -866,6 +877,7 @@ describe('ProjectEntityUpdateHandler', function () { describe('updating an existing doc', function () { let upsertDocResponse beforeEach(async function (ctx) { + ctx.project.track_changes = { 'some-user-id': true } ctx.existingDoc = { _id: docId, name: ctx.docName } ctx.existingFile = { _id: fileId, @@ -893,16 +905,32 @@ describe('ProjectEntityUpdateHandler', function () { ) }) + it('gets the project with the track changes state', function (ctx) { + ctx.ProjectGetter.promises.getProject + .calledWith(projectId, { + rootFolder: true, + rootDoc_id: true, + track_changes: true, + }) + .should.equal(true) + }) + it('tries to find the folder', function (ctx) { ctx.ProjectLocator.promises.findElement .calledWith({ - project_id: projectId, + project: ctx.project, element_id: folderId, type: 'folder', }) .should.equal(true) }) + it('resolves the track changes state for the user', function (ctx) { + ctx.ProjectHelper.isTrackChangesEnabledForUser + .calledWith(ctx.project.track_changes, userId) + .should.equal(true) + }) + it('updates the doc contents', function (ctx) { ctx.DocumentUpdaterHandler.promises.setDocument .calledWith( @@ -910,7 +938,8 @@ describe('ProjectEntityUpdateHandler', function () { ctx.existingDoc._id, userId, ctx.docLines, - ctx.source + ctx.source, + false ) .should.equal(true) }) @@ -921,6 +950,44 @@ describe('ProjectEntityUpdateHandler', function () { }) }) + describe('updating an existing doc with track changes enabled for the user', function () { + beforeEach(async function (ctx) { + ctx.ProjectHelper.isTrackChangesEnabledForUser.returns(true) + ctx.existingDoc = { _id: docId, name: ctx.docName } + ctx.folder = { + _id: folderId, + docs: [ctx.existingDoc], + fileRefs: [], + } + ctx.ProjectLocator.promises.findElement.resolves({ + element: ctx.folder, + }) + ctx.DocumentUpdaterHandler.promises.setDocument.resolves() + + await ctx.ProjectEntityUpdateHandler.promises.upsertDoc( + projectId, + folderId, + ctx.docName, + ctx.docLines, + ctx.source, + userId + ) + }) + + it('updates the doc contents with track changes', function (ctx) { + ctx.DocumentUpdaterHandler.promises.setDocument + .calledWith( + projectId, + ctx.existingDoc._id, + userId, + ctx.docLines, + ctx.source, + true + ) + .should.equal(true) + }) + }) + describe('creating a new doc', function () { let upsertDocResponse @@ -948,7 +1015,7 @@ describe('ProjectEntityUpdateHandler', function () { it('tries to find the folder', function (ctx) { ctx.ProjectLocator.promises.findElement .calledWith({ - project_id: projectId, + project: ctx.project, element_id: folderId, type: 'folder', }) @@ -1019,7 +1086,7 @@ describe('ProjectEntityUpdateHandler', function () { ctx.filePath = '/path/to/folder/foo.tex' ctx.ProjectLocator.promises.findElement .withArgs({ - project_id: projectId, + project: ctx.project, element_id: ctx.folder._id, type: 'folder', }) @@ -3165,6 +3232,10 @@ describe('ProjectEntityUpdateHandler', function () { }) describe('appendToDoc', function () { + beforeEach(function (ctx) { + ctx.ProjectGetter.promises.getProject.resolves(ctx.project) + }) + describe('when document cannot be found', function () { let appendToDocPromise beforeEach(function (ctx) { @@ -3172,7 +3243,7 @@ describe('ProjectEntityUpdateHandler', function () { ctx.DocumentUpdaterHandler.promises.appendToDocument = sinon.stub() ctx.ProjectLocator.promises.findElement = sinon.stub() ctx.ProjectLocator.promises.findElement - .withArgs({ project_id: projectId, element_id: docId, type: 'doc' }) + .withArgs({ project: ctx.project, element_id: docId, type: 'doc' }) .rejects(new Errors.NotFoundError()) appendToDocPromise = ctx.ProjectEntityUpdateHandler.promises.appendToDocWithPath( @@ -3214,6 +3285,7 @@ describe('ProjectEntityUpdateHandler', function () { describe('when document is found', function () { let appendToDocResult beforeEach(async function (ctx) { + ctx.project.track_changes = { 'some-user-id': true } ctx.appendedLines = ['5678', 'def'] ctx.DocumentUpdaterHandler.promises.appendToDocument = sinon.stub() ctx.DocumentUpdaterHandler.promises.appendToDocument @@ -3221,7 +3293,7 @@ describe('ProjectEntityUpdateHandler', function () { .resolves({ rev: 1 }) ctx.ProjectLocator.promises.findElement = sinon.stub() ctx.ProjectLocator.promises.findElement - .withArgs({ project_id: projectId, element_id: docId, type: 'doc' }) + .withArgs({ project: ctx.project, element_id: docId, type: 'doc' }) .resolves({ element: { _id: docId } }) appendToDocResult = await ctx.ProjectEntityUpdateHandler.promises.appendToDocWithPath( @@ -3233,13 +3305,21 @@ describe('ProjectEntityUpdateHandler', function () { ) }) + it('should resolve the track changes state for the user', function (ctx) { + ctx.ProjectHelper.isTrackChangesEnabledForUser.should.have.been.calledWith( + ctx.project.track_changes, + userId + ) + }) + it('should forward call to DocumentUpdaterHandler.appendToDocument', function (ctx) { ctx.DocumentUpdaterHandler.promises.appendToDocument.should.have.been.calledWith( projectId, docId, userId, ctx.appendedLines, - ctx.source + ctx.source, + false ) }) @@ -3248,6 +3328,38 @@ describe('ProjectEntityUpdateHandler', function () { }) }) + describe('when track changes is enabled for the user', function () { + beforeEach(async function (ctx) { + ctx.ProjectHelper.isTrackChangesEnabledForUser.returns(true) + ctx.appendedLines = ['5678', 'def'] + ctx.DocumentUpdaterHandler.promises.appendToDocument = sinon + .stub() + .resolves({ rev: 1 }) + ctx.ProjectLocator.promises.findElement = sinon.stub() + ctx.ProjectLocator.promises.findElement + .withArgs({ project: ctx.project, element_id: docId, type: 'doc' }) + .resolves({ element: { _id: docId } }) + await ctx.ProjectEntityUpdateHandler.promises.appendToDocWithPath( + projectId, + docId, + ctx.appendedLines, + ctx.source, + userId + ) + }) + + it('should forward the track changes flag', function (ctx) { + ctx.DocumentUpdaterHandler.promises.appendToDocument.should.have.been.calledWith( + projectId, + docId, + userId, + ctx.appendedLines, + ctx.source, + true + ) + }) + }) + describe('when DocumentUpdater throws an error', function () { beforeEach(function (ctx) { ctx.appendedLines = ['5678', 'def'] @@ -3257,7 +3369,7 @@ describe('ProjectEntityUpdateHandler', function () { ) ctx.ProjectLocator.promises.findElement = sinon.stub() ctx.ProjectLocator.promises.findElement - .withArgs({ project_id: projectId, element_id: docId, type: 'doc' }) + .withArgs({ project: ctx.project, element_id: docId, type: 'doc' }) .resolves({ element: { _id: docId } }) }) diff --git a/services/web/test/unit/src/Project/ProjectHelper.test.mjs b/services/web/test/unit/src/Project/ProjectHelper.test.mjs index 8bf51d7a91..822ec711dc 100644 --- a/services/web/test/unit/src/Project/ProjectHelper.test.mjs +++ b/services/web/test/unit/src/Project/ProjectHelper.test.mjs @@ -133,6 +133,67 @@ describe('ProjectHelper', function () { }) }) + describe('isTrackChangesEnabledForUser', function () { + const userId = '588f3ddae8ebc1bac07c9fa4' + + it('returns true for the legacy on-for-everyone format', function (ctx) { + expect( + ctx.ProjectHelper.isTrackChangesEnabledForUser(true, userId) + ).to.equal(true) + }) + + it('returns true when enabled for the user', function (ctx) { + expect( + ctx.ProjectHelper.isTrackChangesEnabledForUser( + { [userId]: true }, + userId + ) + ).to.equal(true) + }) + + it('supports ObjectId user ids', function (ctx) { + expect( + ctx.ProjectHelper.isTrackChangesEnabledForUser( + { [userId]: true }, + new ObjectId(userId) + ) + ).to.equal(true) + }) + + it('returns false when only enabled for another user', function (ctx) { + expect( + ctx.ProjectHelper.isTrackChangesEnabledForUser( + { '5c41deb2b4ca500153340809': true }, + userId + ) + ).to.equal(false) + }) + + it('returns false when only enabled for guests', function (ctx) { + expect( + ctx.ProjectHelper.isTrackChangesEnabledForUser( + { __guests__: true }, + userId + ) + ).to.equal(false) + }) + + it('returns false when disabled', function (ctx) { + expect( + ctx.ProjectHelper.isTrackChangesEnabledForUser(false, userId) + ).to.equal(false) + expect( + ctx.ProjectHelper.isTrackChangesEnabledForUser(undefined, userId) + ).to.equal(false) + }) + + it('returns false without a user id', function (ctx) { + expect( + ctx.ProjectHelper.isTrackChangesEnabledForUser({ [userId]: true }, null) + ).to.equal(false) + }) + }) + describe('compilerFromV1Engine', function () { it('returns the correct engine for latex_dvipdf', function (ctx) { expect(ctx.ProjectHelper.compilerFromV1Engine('latex_dvipdf')).to.equal(