diff --git a/libraries/overleaf-editor-core/index.js b/libraries/overleaf-editor-core/index.js index 028eefabf9..72a11f76f0 100644 --- a/libraries/overleaf-editor-core/index.js +++ b/libraries/overleaf-editor-core/index.js @@ -7,6 +7,7 @@ const ChangeRequest = require('./lib/change_request') const ChangeNote = require('./lib/change_note') const Chunk = require('./lib/chunk') const ChunkResponse = require('./lib/chunk_response') +const Comment = require('./lib/comment') const DeleteCommentOperation = require('./lib/operation/delete_comment_operation') const File = require('./lib/file') const FileMap = require('./lib/file_map') @@ -48,6 +49,7 @@ exports.ChangeRequest = ChangeRequest exports.ChangeNote = ChangeNote exports.Chunk = Chunk exports.ChunkResponse = ChunkResponse +exports.Comment = Comment exports.DeleteCommentOperation = DeleteCommentOperation exports.File = File exports.FileMap = FileMap diff --git a/libraries/overleaf-editor-core/lib/file_data/comment_list.js b/libraries/overleaf-editor-core/lib/file_data/comment_list.js index 80b89dd434..13300b44c2 100644 --- a/libraries/overleaf-editor-core/lib/file_data/comment_list.js +++ b/libraries/overleaf-editor-core/lib/file_data/comment_list.js @@ -21,6 +21,15 @@ class CommentList { return this.comments.values() } + /** + * Returns the contents of this list in an array + * + * @returns {Comment[]} + */ + toArray() { + return Array.from(this) + } + /** * Return the length of the comment list * diff --git a/libraries/overleaf-editor-core/lib/range.js b/libraries/overleaf-editor-core/lib/range.js index 4984f129c7..7a92f55ec9 100644 --- a/libraries/overleaf-editor-core/lib/range.js +++ b/libraries/overleaf-editor-core/lib/range.js @@ -25,6 +25,16 @@ class Range { return this.pos + this.length } + /** + * Is this range equal to the given range? + * + * @param {Range} other + * @returns {boolean} + */ + equals(other) { + return this.pos === other.pos && this.length === other.length + } + /** * @param {Range} range * @returns {boolean} diff --git a/libraries/overleaf-editor-core/test/comments_list.test.js b/libraries/overleaf-editor-core/test/comments_list.test.js index 31f7ac8626..c7c4765a7e 100644 --- a/libraries/overleaf-editor-core/test/comments_list.test.js +++ b/libraries/overleaf-editor-core/test/comments_list.test.js @@ -128,6 +128,12 @@ describe('commentList', function () { ]) }) + it('should be iterable', function () { + const comment = new Comment('comm1', [new Range(5, 10)]) + const commentList = new CommentList([comment]) + expect(Array.from(commentList)).to.deep.equal([comment]) + }) + describe('inserting a comment between ranges', function () { it('should expand comment on the left', function () { const commentList = CommentList.fromRaw([ diff --git a/services/project-history/app/js/SnapshotManager.js b/services/project-history/app/js/SnapshotManager.js index 0f35c36de6..48754b3d0b 100644 --- a/services/project-history/app/js/SnapshotManager.js +++ b/services/project-history/app/js/SnapshotManager.js @@ -82,7 +82,7 @@ async function getRangesSnapshot(projectId, version, pathname) { throw new Error('Unable to read file contents') } const trackedChanges = file.getTrackedChanges().asSorted() - const comments = file.getComments() + const comments = file.getComments().toArray() const docUpdaterCompatibleTrackedChanges = [] let trackedDeletionOffset = 0 diff --git a/services/project-history/app/js/SyncManager.js b/services/project-history/app/js/SyncManager.js index 18161e3148..7864029108 100644 --- a/services/project-history/app/js/SyncManager.js +++ b/services/project-history/app/js/SyncManager.js @@ -1,3 +1,5 @@ +// @ts-check + import _ from 'lodash' import { callbackify, promisify } from 'util' import { callbackifyMultiResult } from '@overleaf/promise-utils' @@ -18,6 +20,13 @@ import * as RedisManager from './RedisManager.js' import * as HistoryStoreManager from './HistoryStoreManager.js' import * as HashManager from './HashManager.js' +/** + * @typedef {import('overleaf-editor-core').Comment} HistoryComment + * @typedef {import('./types').Comment} Comment + * @typedef {import('./types').Entity} Entity + * @typedef {import('./types').ResyncDocContentUpdate} ResyncDocContentUpdate + * @typedef {import('./types').Update} Update + */ const MAX_RESYNC_HISTORY_RECORDS = 100 // keep this many records of previous resyncs const EXPIRE_RESYNC_HISTORY_INTERVAL_MS = 90 * 24 * 3600 * 1000 // 90 days @@ -347,6 +356,13 @@ class SyncState { } class SyncUpdateExpander { + /** + * Build a SyncUpdateExpander + * + * @param {string} projectId + * @param {Record} snapshotFiles + * @param {string} origin + */ constructor(projectId, snapshotFiles, origin) { this.projectId = projectId this.files = snapshotFiles @@ -374,8 +390,11 @@ class SyncUpdateExpander { return !matchedExpectedFile } + /** + * @param {Update} update + */ async expandUpdate(update) { - if (update.resyncProjectStructure != null) { + if ('resyncProjectStructure' in update) { logger.debug( { projectId: this.projectId, update }, 'expanding resyncProjectStructure update' @@ -441,12 +460,12 @@ class SyncUpdateExpander { expectedBinaryFiles, persistedBinaryFiles ) - } else if (update.resyncDocContent != null) { + } else if ('resyncDocContent' in update) { logger.debug( { projectId: this.projectId, update }, 'expanding resyncDocContent update' ) - await this.queueTextOpForOutOfSyncContents(update) + await this.expandResyncDocContentUpdate(update) } else { this.expandedUpdates.push(update) } @@ -456,6 +475,10 @@ class SyncUpdateExpander { return this.expandedUpdates } + /** + * @param {Entity[]} expectedFiles + * @param {{ path: string }[]} persistedFiles + */ queueRemoveOpsForUnexpectedFiles(update, expectedFiles, persistedFiles) { const unexpectedFiles = _.differenceBy( persistedFiles, @@ -479,6 +502,10 @@ class SyncUpdateExpander { } } + /** + * @param {Entity[]} expectedFiles + * @param {{ path: string }[]} persistedFiles + */ queueAddOpsForMissingFiles(update, expectedFiles, persistedFiles) { const missingFiles = _.differenceBy(expectedFiles, persistedFiles, 'path') for (const entity of missingFiles) { @@ -491,7 +518,7 @@ class SyncUpdateExpander { }, } - if (entity.doc != null) { + if ('doc' in entity) { update.doc = entity.doc update.docLines = '' // we have to create a dummy entry here because later we will need the content in the diff computation @@ -553,10 +580,16 @@ class SyncUpdateExpander { } } - async queueTextOpForOutOfSyncContents(update) { + /** + * Expand a resyncDocContentUpdate + * + * @param {ResyncDocContentUpdate} update + */ + async expandResyncDocContentUpdate(update) { const pathname = UpdateTranslator._convertPathname(update.path) const snapshotFile = this.files[pathname] const expectedFile = update.resyncDocContent + const expectedContent = expectedFile.content if (!snapshotFile) { throw new OError('unrecognised file: not in snapshot') @@ -567,63 +600,79 @@ class SyncUpdateExpander { // Note getHash() returns the hash only when the persisted file has // no changes in the snapshot, the hash is null if there are changes // that apply to it. - const persistedHash = - typeof snapshotFile.getHash === 'function' - ? snapshotFile.getHash() - : undefined + let hashesMatch = false + const persistedHash = snapshotFile.getHash() if (persistedHash != null) { - const expectedHash = HashManager._getBlobHashFromString( - expectedFile.content - ) + const expectedHash = HashManager._getBlobHashFromString(expectedContent) if (persistedHash === expectedHash) { logger.debug( { projectId: this.projectId, persistedHash, expectedHash }, 'skipping diff because hashes match and persisted file has no ops' ) - return + hashesMatch = true } } else { logger.debug('cannot compare hashes, will retrieve content') } - const expectedContent = update.resyncDocContent.content - - let persistedContent // compute the difference between the expected and persisted content - if (snapshotFile.load != null) { - const historyId = await WebApiManager.promises.getHistoryId( - this.projectId - ) - const file = await snapshotFile.load( - 'eager', - HistoryStoreManager.getBlobStore(historyId) - ) - persistedContent = file.getContent() - } else if (snapshotFile.content != null) { - // use dummy content from queueAddOpsForMissingFiles for added missing files - persistedContent = snapshotFile.content - } else { - throw new OError('unrecognised file') + const historyId = await WebApiManager.promises.getHistoryId(this.projectId) + const file = await snapshotFile.load( + 'eager', + HistoryStoreManager.getBlobStore(historyId) + ) + const persistedContent = file.getContent() + if (persistedContent == null) { + // This should not happen given that we loaded the file eagerly. We could + // probably refine the types in overleaf-editor-core so that this check + // wouldn't be necessary. + throw new Error('File was not properly loaded') } - let op + if (!hashesMatch) { + await this.queueUpdateForOutOfSyncContent( + update, + pathname, + persistedContent, + expectedContent + ) + } + + const persistedComments = file.getComments().toArray() + await this.queueUpdateForOutOfSyncComments( + update, + pathname, + persistedContent, + persistedComments + ) + } + + /** + * Queue update for out of sync content + * + * @param {ResyncDocContentUpdate} update + * @param {string} pathname + * @param {string} persistedContent + * @param {string} expectedContent + */ + async queueUpdateForOutOfSyncContent( + update, + pathname, + persistedContent, + expectedContent + ) { logger.debug( { projectId: this.projectId, persistedContent, expectedContent }, 'diffing doc contents' ) - try { - op = UpdateCompressor.diffAsShareJsOps(persistedContent, expectedContent) - } catch (error) { - throw OError.tag(error, 'error from diffAsShareJsOps', { - projectId: this.projectId, - persistedContent, - expectedContent, - }) - } + const op = UpdateCompressor.diffAsShareJsOps( + persistedContent, + expectedContent + ) if (op.length === 0) { return } - update = { + const expandedUpdate = { doc: update.doc, op, meta: { @@ -638,11 +687,105 @@ class SyncUpdateExpander { { projectId: this.projectId, diffCount: op.length }, 'doc contents differ' ) - this.expandedUpdates.push(update) + this.expandedUpdates.push(expandedUpdate) Metrics.inc('project_history_resync_operation', 1, { status: 'update text file contents', }) } + + /** + * Queue update for out of sync comments + * + * @param {ResyncDocContentUpdate} update + * @param {string} pathname + * @param {string} persistedContent + * @param {HistoryComment[]} persistedComments + */ + async queueUpdateForOutOfSyncComments( + update, + pathname, + persistedContent, + persistedComments + ) { + const expectedComments = update.resyncDocContent.ranges?.comments ?? [] + const resolvedComments = new Set( + update.resyncDocContent.resolvedComments ?? [] + ) + const expectedCommentsById = new Map( + expectedComments.map(comment => [comment.id, comment]) + ) + const persistedCommentsById = new Map( + persistedComments.map(comment => [comment.id, comment]) + ) + + // Delete any persisted comment that is not in the expected comment list. + for (const persistedComment of persistedComments) { + if (!expectedCommentsById.has(persistedComment.id)) { + this.expandedUpdates.push({ + pathname, + deleteComment: persistedComment.id, + meta: { + resync: true, + origin: this.origin, + ts: update.meta.ts, + }, + }) + } + } + + for (const expectedComment of expectedComments) { + const persistedComment = persistedCommentsById.get(expectedComment.id) + if ( + persistedComment != null && + commentRangesAreInSync(persistedComment, expectedComment) + ) { + const expectedCommentResolved = resolvedComments.has(expectedComment.id) + if (expectedCommentResolved === persistedComment.resolved) { + // Both comments are identical; do nothing + } else { + // Only the resolved state differs + this.expandedUpdates.push({ + pathname, + commentId: expectedComment.id, + resolved: expectedCommentResolved, + }) + } + } else { + // New comment or ranges differ + this.expandedUpdates.push({ + doc: update.doc, + op: [expectedComment.op], + meta: { + resync: true, + origin: this.origin, + ts: update.meta.ts, + pathname, + doc_length: persistedContent.length, + }, + }) + } + } + } +} + +/** + * Compares the ranges in the persisted and expected comments + * + * @param {HistoryComment} persistedComment + * @param {Comment} expectedComment + */ +function commentRangesAreInSync(persistedComment, expectedComment) { + if (persistedComment.ranges.length !== 1) { + // The editor only supports single range comments + return false + } + const persistedRange = persistedComment.ranges[0] + const expectedPos = expectedComment.op.hpos ?? expectedComment.op.p + const expectedLength = expectedComment.op.hlen ?? expectedComment.op.c.length + return ( + persistedRange.pos === expectedPos && + persistedRange.length === expectedLength + ) } // EXPORTS diff --git a/services/project-history/app/js/UpdateTranslator.js b/services/project-history/app/js/UpdateTranslator.js index 8150ba8d96..cc118505c2 100644 --- a/services/project-history/app/js/UpdateTranslator.js +++ b/services/project-history/app/js/UpdateTranslator.js @@ -9,7 +9,7 @@ import * as OperationsCompressor from './OperationsCompressor.js' * @typedef {import('./types').AddDocUpdate} AddDocUpdate * @typedef {import('./types').AddFileUpdate} AddFileUpdate * @typedef {import('./types').CommentOp} CommentOp - * @typedef {import('./types').DeleteOp} DeleteCommentUpdate + * @typedef {import('./types').DeleteCommentUpdate} DeleteCommentUpdate * @typedef {import('./types').DeleteOp} DeleteOp * @typedef {import('./types').InsertOp} InsertOp * @typedef {import('./types').RetainOp} RetainOp @@ -266,7 +266,7 @@ class OperationsBuilder { /** * @param {Op} op - * @param {Update} update + * @param {TextUpdate} update * @returns {void} */ addOp(op, update) { diff --git a/services/project-history/app/js/types.ts b/services/project-history/app/js/types.ts index f5ac040ac2..ec93ad340e 100644 --- a/services/project-history/app/js/types.ts +++ b/services/project-history/app/js/types.ts @@ -5,6 +5,8 @@ export type Update = | RenameUpdate | DeleteCommentUpdate | SetCommentStateUpdate + | ResyncProjectStructureUpdate + | ResyncDocContentUpdate export type UpdateMeta = { user_id: string @@ -13,6 +15,7 @@ export type UpdateMeta = { type?: string origin?: RawOrigin tc?: string + resync?: boolean } export type TextUpdate = { @@ -62,6 +65,32 @@ export type RenameUpdate = ProjectUpdateBase & { new_pathname: string } +export type ResyncProjectStructureUpdate = { + resyncProjectStructure: { + docs: Doc[] + files: File[] + } + projectHistoryId: string + meta: { + ts: string + } +} + +export type ResyncDocContentUpdate = { + resyncDocContent: { + content: string + version: number + ranges?: Ranges + resolvedComments?: string[] + } + projectHistoryId: string + path: string + doc: string + meta: { + ts: string + } +} + export type Op = RetainOp | InsertOp | DeleteOp | CommentOp export type RetainOp = { @@ -146,3 +175,39 @@ export type RangesSnapshot = { changes: TrackedChangeSnapshot[] comments: CommentSnapshot[] } + +export type Doc = { + doc: string + path: string +} + +export type File = { + file: string + url: string + path: string +} + +export type Entity = Doc | File + +export type Ranges = { + comments?: Comment[] + changes?: TrackedChange[] +} + +export type Comment = { + id: string + op: CommentOp + metadata: { + user_id: string + ts: string + } +} + +export type TrackedChange = { + id: string + op: Op + metadata: { + user_id: string + ts: string + } +} diff --git a/services/project-history/test/acceptance/js/SyncTests.js b/services/project-history/test/acceptance/js/SyncTests.js index df25407a35..3d6a4b8f45 100644 --- a/services/project-history/test/acceptance/js/SyncTests.js +++ b/services/project-history/test/acceptance/js/SyncTests.js @@ -360,7 +360,7 @@ describe('Syncing with web and doc-updater', function () { }) describe("when a doc's contents is not up to date", function () { - it('should send test updates to the history store', function (done) { + beforeEach(function () { MockHistoryStore() .get(`/api/projects/${historyId}/latest/history`) .reply(200, { @@ -385,7 +385,9 @@ describe('Syncing with web and doc-updater', function () { `/api/projects/${historyId}/blobs/0a207c060e61f3b88eaee0a8cd0696f46fb155eb` ) .reply(200, 'a\nb') + }) + it('should send test updates to the history store', function (done) { const addFile = MockHistoryStore() .post(`/api/projects/${historyId}/legacy_changes`, body => { expect(body).to.deep.equal([ @@ -457,31 +459,6 @@ describe('Syncing with web and doc-updater', function () { }) it('should strip non-BMP characters in updates before sending to the history store', function (done) { - MockHistoryStore() - .get(`/api/projects/${historyId}/latest/history`) - .reply(200, { - chunk: { - history: { - snapshot: { - files: { - 'main.tex': { - hash: '0a207c060e61f3b88eaee0a8cd0696f46fb155eb', - stringLength: 3, - }, - }, - }, - changes: [], - }, - startVersion: 0, - }, - }) - - MockHistoryStore() - .get( - `/api/projects/${historyId}/blobs/0a207c060e61f3b88eaee0a8cd0696f46fb155eb` - ) - .reply(200, 'a\nb') - const addFile = MockHistoryStore() .post(`/api/projects/${historyId}/legacy_changes`, body => { expect(body).to.deep.equal([ @@ -551,6 +528,98 @@ describe('Syncing with web and doc-updater', function () { } ) }) + + it('should fix comments in the history store', function (done) { + const commentId = 'comment-id' + const addComment = MockHistoryStore() + .post(`/api/projects/${historyId}/legacy_changes`, body => { + expect(body).to.deep.equal([ + { + v2Authors: [], + authors: [], + timestamp: this.timestamp.toJSON(), + operations: [ + { + pathname: 'main.tex', + commentId, + ranges: [{ pos: 1, length: 10 }], + resolved: false, + }, + ], + origin: { kind: 'test-origin' }, + }, + ]) + return true + }) + .query({ end_version: 0 }) + .reply(204) + + async.series( + [ + cb => { + ProjectHistoryClient.resyncHistory(this.project_id, cb) + }, + cb => { + const update = { + projectHistoryId: historyId, + resyncProjectStructure: { + docs: [{ path: '/main.tex' }], + files: [], + }, + meta: { + ts: this.timestamp, + }, + } + ProjectHistoryClient.pushRawUpdate(this.project_id, update, cb) + }, + cb => { + const update = { + path: '/main.tex', + projectHistoryId: historyId, + resyncDocContent: { + content: 'a\nb', + ranges: { + comments: [ + { + id: commentId, + op: { + c: 'a', + p: 0, + hpos: 1, + hlen: 10, + t: commentId, + }, + meta: { + user_id: 'user-id', + ts: this.timestamp, + }, + }, + ], + }, + }, + doc: this.doc_id, + meta: { + ts: this.timestamp, + }, + } + ProjectHistoryClient.pushRawUpdate(this.project_id, update, cb) + }, + cb => { + ProjectHistoryClient.flushProject(this.project_id, cb) + }, + ], + error => { + if (error) { + return done(error) + } + assert( + addComment.isDone(), + `/api/projects/${historyId}/changes should have been called` + ) + done() + } + ) + }) }) }) }) diff --git a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js b/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js index f249e77829..512034988e 100644 --- a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js +++ b/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js @@ -1,7 +1,10 @@ +// @ts-check + import sinon from 'sinon' import { expect } from 'chai' import mongodb from 'mongodb-legacy' import tk from 'timekeeper' +import { Comment, Range } from 'overleaf-editor-core' import { strict as esmock } from 'esmock' const { ObjectId } = mongodb @@ -9,26 +12,47 @@ const MODULE_PATH = '../../../../app/js/SyncManager.js' const timestamp = new Date() -const resyncProjectStructureUpdate = (docs, files) => ({ - resyncProjectStructure: { docs, files }, +function resyncProjectStructureUpdate(docs, files) { + return { + resyncProjectStructure: { docs, files }, - meta: { - ts: timestamp, - }, -}) + meta: { + ts: timestamp, + }, + } +} -const docContentSyncUpdate = (doc, content) => ({ - path: doc.path, - doc: doc.doc, +function docContentSyncUpdate( + doc, + content, + ranges = {}, + resolvedComments = [] +) { + return { + path: doc.path, + doc: doc.doc, - resyncDocContent: { - content, - }, + resyncDocContent: { + content, + ranges, + resolvedComments, + }, - meta: { - ts: timestamp, - }, -}) + meta: { + ts: timestamp, + }, + } +} + +function makeComment(commentId, pos, text) { + return { + id: commentId, + op: { p: pos, c: text, t: commentId }, + meta: { + ts: timestamp, + }, + } +} describe('SyncManager', function () { beforeEach(async function () { @@ -54,7 +78,7 @@ describe('SyncManager', function () { } this.UpdateCompressor = { - diffAsShareJsOps: sinon.stub(), + diffAsShareJsOps: sinon.stub().returns([]), } this.UpdateTranslator = { @@ -451,22 +475,33 @@ describe('SyncManager', function () { describe('expandSyncUpdates', function () { beforeEach(function () { this.persistedDoc = { - doc: { data: { hash: 'abcdef' } }, + doc: 'doc-id', path: 'main.tex', - content: 'asdf', } + this.persistedDocContent = 'the quick brown fox jumps over the lazy fox' this.persistedFile = { - file: { data: { hash: '123456789a' } }, + file: 'file-id', path: '1.png', + _hash: 'abcde', + } + this.loadedSnapshotDoc = { + isEditable: sinon.stub().returns(true), + getContent: sinon.stub().returns(this.persistedDocContent), + getComments: sinon + .stub() + .returns({ toArray: sinon.stub().returns([]) }), + getHash: sinon.stub().returns(null), } this.fileMap = { 'main.tex': { isEditable: sinon.stub().returns(true), - content: this.persistedDoc.content, + getContent: sinon.stub().returns(null), + getHash: sinon.stub().returns(null), + load: sinon.stub().resolves(this.loadedSnapshotDoc), }, '1.png': { isEditable: sinon.stub().returns(false), - data: { hash: this.persistedFile.file.data.hash }, + data: { hash: this.persistedFile._hash }, }, } this.UpdateTranslator._convertPathname @@ -777,7 +812,7 @@ describe('SyncManager', function () { }) it('preserves other updates', async function () { - const update = 'mock-update' + const update = { mock: 'update' } const updates = [ update, resyncProjectStructureUpdate( @@ -807,7 +842,7 @@ describe('SyncManager', function () { [this.persistedDoc], [this.persistedFile] ), - docContentSyncUpdate(this.persistedDoc, this.persistedDoc.content), + docContentSyncUpdate(this.persistedDoc, this.persistedDocContent), ] await expect( this.SyncManager.promises.expandSyncUpdates( @@ -838,7 +873,7 @@ describe('SyncManager', function () { [this.persistedDoc], [this.persistedFile] ), - docContentSyncUpdate(this.persistedDoc, this.persistedDoc.content), + docContentSyncUpdate(this.persistedDoc, this.persistedDocContent), ] this.UpdateCompressor.diffAsShareJsOps.returns([]) const expandedUpdates = @@ -859,9 +894,14 @@ describe('SyncManager', function () { [this.persistedDoc], [this.persistedFile] ), - docContentSyncUpdate(this.persistedDoc, 'a'), + docContentSyncUpdate( + this.persistedDoc, + 'the fox jumps over the lazy dog' + ), ] - this.UpdateCompressor.diffAsShareJsOps.returns([{ d: 'sdf', p: 1 }]) + this.UpdateCompressor.diffAsShareJsOps.returns([ + { d: 'quick brown ', p: 4 }, + ]) const expandedUpdates = await this.SyncManager.promises.expandSyncUpdates( this.projectId, @@ -873,10 +913,10 @@ describe('SyncManager', function () { expect(expandedUpdates).to.deep.equal([ { doc: this.persistedDoc.doc, - op: [{ d: 'sdf', p: 1 }], + op: [{ d: 'quick brown ', p: 4 }], meta: { pathname: this.persistedDoc.path, - doc_length: 4, + doc_length: this.persistedDocContent.length, resync: true, ts: timestamp, origin: { kind: 'history-resync' }, @@ -891,14 +931,14 @@ describe('SyncManager', function () { const newDoc = { path: 'another.tex', doc: new ObjectId().toString(), - content: 'a', } + const newDocContent = 'a' const updates = [ resyncProjectStructureUpdate( [this.persistedDoc, newDoc], [this.persistedFile] ), - docContentSyncUpdate(newDoc, newDoc.content), + docContentSyncUpdate(newDoc, newDocContent), ] const expandedUpdates = await this.SyncManager.promises.expandSyncUpdates( @@ -935,7 +975,7 @@ describe('SyncManager', function () { }) it('skips text updates for docs when hashes match', async function () { - this.fileMap['main.tex'].getHash = sinon.stub().returns('special-hash') + this.fileMap['main.tex'].getHash.returns('special-hash') this.HashManager._getBlobHashFromString.returns('special-hash') const updates = [ resyncProjectStructureUpdate( @@ -957,7 +997,7 @@ describe('SyncManager', function () { }) it('computes text updates for docs when hashes differ', async function () { - this.fileMap['main.tex'].getHash = sinon.stub().returns('first-hash') + this.fileMap['main.tex'].getHash.returns('first-hash') this.HashManager._getBlobHashFromString.returns('second-hash') this.UpdateCompressor.diffAsShareJsOps.returns([ { i: 'test diff', p: 0 }, @@ -983,7 +1023,7 @@ describe('SyncManager', function () { op: [{ i: 'test diff', p: 0 }], meta: { pathname: this.persistedDoc.path, - doc_length: 4, + doc_length: this.persistedDocContent.length, resync: true, ts: timestamp, origin: { kind: 'history-resync' }, @@ -1002,8 +1042,7 @@ describe('SyncManager', function () { ), docContentSyncUpdate(this.persistedDoc, 'a'), ] - const file = { getContent: sinon.stub().returns('stored content') } - this.fileMap['main.tex'].load = sinon.stub().resolves(file) + this.loadedSnapshotDoc.getContent.returns('stored content') this.UpdateCompressor.diffAsShareJsOps.returns([{ d: 'sdf', p: 1 }]) this.expandedUpdates = await this.SyncManager.promises.expandSyncUpdates( @@ -1032,5 +1071,195 @@ describe('SyncManager', function () { }) }) }) + + describe('syncing comments', function () { + beforeEach(function () { + this.loadedSnapshotDoc.getComments.returns({ + toArray: sinon + .stub() + .returns([ + new Comment('comment1', [new Range(4, 5)]), + new Comment('comment2', [new Range(10, 5)], true), + ]), + }) + this.comments = [ + makeComment('comment1', 4, 'quick'), + makeComment('comment2', 10, 'brown'), + ] + this.resolvedComments = ['comment2'] + }) + + it('does nothing if comments have not changed', async function () { + const updates = [ + docContentSyncUpdate( + this.persistedDoc, + this.persistedDocContent, + { + comments: this.comments, + }, + this.resolvedComments + ), + ] + const expandedUpdates = + await this.SyncManager.promises.expandSyncUpdates( + this.projectId, + this.historyId, + updates, + this.extendLock + ) + expect(expandedUpdates).to.deep.equal([]) + }) + + it('adds missing comments', async function () { + this.comments.push(makeComment('comment3', 20, 'jumps')) + const updates = [ + docContentSyncUpdate( + this.persistedDoc, + this.persistedDocContent, + { + comments: this.comments, + }, + this.resolvedComments + ), + ] + const expandedUpdates = + await this.SyncManager.promises.expandSyncUpdates( + this.projectId, + this.historyId, + updates, + this.extendLock + ) + expect(expandedUpdates).to.deep.equal([ + { + doc: this.persistedDoc.doc, + op: [ + { + c: 'jumps', + p: 20, + t: 'comment3', + }, + ], + meta: { + origin: { + kind: 'history-resync', + }, + pathname: this.persistedDoc.path, + resync: true, + ts: timestamp, + doc_length: this.persistedDocContent.length, + }, + }, + ]) + }) + + it('deletes extra comments', async function () { + this.comments.splice(0, 1) + const updates = [ + docContentSyncUpdate( + this.persistedDoc, + this.persistedDocContent, + { + comments: this.comments, + }, + this.resolvedComments + ), + ] + const expandedUpdates = + await this.SyncManager.promises.expandSyncUpdates( + this.projectId, + this.historyId, + updates, + this.extendLock + ) + expect(expandedUpdates).to.deep.equal([ + { + pathname: this.persistedDoc.path, + deleteComment: 'comment1', + meta: { + origin: { + kind: 'history-resync', + }, + resync: true, + ts: timestamp, + }, + }, + ]) + }) + + it('updates comments when ranges differ', async function () { + this.comments[1] = makeComment('comment2', 16, 'fox') + const updates = [ + docContentSyncUpdate( + this.persistedDoc, + this.persistedDocContent, + { + comments: this.comments, + }, + this.resolvedComments + ), + ] + const expandedUpdates = + await this.SyncManager.promises.expandSyncUpdates( + this.projectId, + this.historyId, + updates, + this.extendLock + ) + expect(expandedUpdates).to.deep.equal([ + { + doc: 'doc-id', + op: [ + { + c: 'fox', + p: 16, + t: 'comment2', + }, + ], + meta: { + origin: { + kind: 'history-resync', + }, + resync: true, + ts: timestamp, + pathname: this.persistedDoc.path, + doc_length: this.persistedDocContent.length, + }, + }, + ]) + }) + + it('sets the resolved state when it differs', async function () { + this.resolvedComments = ['comment1'] + const updates = [ + docContentSyncUpdate( + this.persistedDoc, + this.persistedDocContent, + { + comments: this.comments, + }, + this.resolvedComments + ), + ] + const expandedUpdates = + await this.SyncManager.promises.expandSyncUpdates( + this.projectId, + this.historyId, + updates, + this.extendLock + ) + expect(expandedUpdates).to.have.deep.members([ + { + pathname: this.persistedDoc.path, + commentId: 'comment1', + resolved: true, + }, + { + pathname: this.persistedDoc.path, + commentId: 'comment2', + resolved: false, + }, + ]) + }) + }) }) })