From b55bdf7cedb962adeb9b5651f90a3ae8139dd245 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Fri, 16 Feb 2024 07:40:13 -0500 Subject: [PATCH] Merge pull request #17128 from overleaf/em-filter-out-comments Do not send comments to project-history when ranges support is disabled GitOrigin-RevId: 0c5e5e2c98ea3c2830ba4d5d114bf4730b440440 --- libraries/ranges-tracker/index.cjs | 18 +- .../document-updater/app/js/RangesManager.js | 14 +- .../document-updater/app/js/UpdateManager.js | 11 +- .../js/RangesManager/RangesManagerTests.js | 354 ++++++++++-------- .../js/UpdateManager/UpdateManagerTests.js | 8 +- .../ide-react/editor/document-container.ts | 2 +- 6 files changed, 226 insertions(+), 181 deletions(-) diff --git a/libraries/ranges-tracker/index.cjs b/libraries/ranges-tracker/index.cjs index b6efdb6557..b7bf3330ab 100644 --- a/libraries/ranges-tracker/index.cjs +++ b/libraries/ranges-tracker/index.cjs @@ -1,3 +1,4 @@ +// @ts-check /** * The purpose of this class is to track a set of inserts and deletes to a document, like * track changes in Word. We store these as a set of ShareJs style ranges: @@ -44,8 +45,21 @@ class RangesTracker { comments = [] } this.comments = comments - this.setIdSeed(RangesTracker.generateIdSeed()) - this.resetDirtyState() + this.track_changes = false + this.id_seed = RangesTracker.generateIdSeed() + this.id_increment = 0 + this._dirtyState = { + comment: { + moved: {}, + removed: {}, + added: {}, + }, + change: { + moved: {}, + removed: {}, + added: {}, + }, + } } getIdSeed() { diff --git a/services/document-updater/app/js/RangesManager.js b/services/document-updater/app/js/RangesManager.js index 53f1c13ddc..c9d0367f71 100644 --- a/services/document-updater/app/js/RangesManager.js +++ b/services/document-updater/app/js/RangesManager.js @@ -37,9 +37,11 @@ const RangesManager = { * @param {Ranges} ranges - ranges before the updates were applied * @param {Update[]} updates * @param {string[]} newDocLines - the document lines after the updates were applied + * @param {object} opts + * @param {boolean} [opts.historyRangesSupport] - whether history ranges support is enabled * @returns {{ newRanges: Ranges, rangesWereCollapsed: boolean, historyUpdates: HistoryUpdate[] }} */ - applyUpdate(projectId, docId, ranges, updates, newDocLines) { + applyUpdate(projectId, docId, ranges, updates, newDocLines, opts = {}) { if (ranges == null) { ranges = {} } @@ -58,9 +60,13 @@ const RangesManager = { } const historyOps = [] for (const op of update.op) { - historyOps.push( - getHistoryOp(op, rangesTracker.comments, rangesTracker.changes) - ) + if (opts.historyRangesSupport) { + historyOps.push( + getHistoryOp(op, rangesTracker.comments, rangesTracker.changes) + ) + } else if (isInsert(op) || isDelete(op)) { + historyOps.push(op) + } rangesTracker.applyOp(op, { user_id: update.meta?.user_id }) } historyUpdates.push({ ...update, op: historyOps }) diff --git a/services/document-updater/app/js/UpdateManager.js b/services/document-updater/app/js/UpdateManager.js index f68ae79ec9..a086cb62e4 100644 --- a/services/document-updater/app/js/UpdateManager.js +++ b/services/document-updater/app/js/UpdateManager.js @@ -141,20 +141,15 @@ const UpdateManager = { sync: incomingUpdateVersion === previousVersion, }) - let { newRanges, rangesWereCollapsed, historyUpdates } = + const { newRanges, rangesWereCollapsed, historyUpdates } = RangesManager.applyUpdate( projectId, docId, ranges, appliedOps, - updatedDocLines + updatedDocLines, + { historyRangesSupport } ) - if (!historyRangesSupport) { - // The document has not been transitioned to include comments and - // tracked changes in its history. Send regular updates rather than the - // full history updates. - historyUpdates = appliedOps - } profile.log('RangesManager.applyUpdate', { sync: true }) await RedisManager.promises.updateDocument( diff --git a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js index 7eb7203990..2032c21273 100644 --- a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js +++ b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js @@ -237,25 +237,13 @@ describe('RangesManager', function () { }) }) - describe('inserts among tracked deletes', function () { + describe('with comment updates', function () { beforeEach(function () { - // original text is "on[1]e[22] [333](three) fo[4444]ur five" - // [] denotes tracked deletes - // () denotes tracked inserts - this.ranges = { - changes: makeRanges([ - { d: '1', p: 2 }, - { d: '22', p: 3 }, - { d: '333', p: 4 }, - { i: 'three', p: 4 }, - { d: '4444', p: 12 }, - ]), - } this.updates = makeUpdates([ - { i: 'zero ', p: 0 }, - { i: 'two ', p: 9, u: true }, + { i: 'two ', p: 4 }, + { c: 'one', p: 0 }, ]) - this.newDocLines = ['zero one two three four five'] + this.ranges = {} this.result = this.RangesManager.applyUpdate( this.project_id, this.doc_id, @@ -265,168 +253,210 @@ describe('RangesManager', function () { ) }) - it('should offset the hpos by the length of tracked deletes before the insert', function () { - expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ - [{ i: 'zero ', p: 0 }], - // 'two' is added just before the "333" tracked delete - [{ i: 'two ', p: 9, u: true, hpos: 12 }], + it('should not send comments to the history', function () { + expect(this.result.historyUpdates[0].op).to.deep.equal([ + { i: 'two ', p: 4 }, ]) }) }) - describe('tracked delete rejections', function () { - beforeEach(function () { - // original text is "one [two ]three four five" - // [] denotes tracked deletes - this.ranges = { - changes: makeRanges([{ d: 'two ', p: 4 }]), - } - this.updates = makeUpdates([{ i: 'tw', p: 4, u: true }]) - this.newDocLines = ['one twthree four five'] - this.result = this.RangesManager.applyUpdate( - this.project_id, - this.doc_id, - this.ranges, - this.updates, - this.newDocLines - ) + describe('with history ranges support', function () { + describe('inserts among tracked deletes', function () { + beforeEach(function () { + // original text is "on[1]e[22] [333](three) fo[4444]ur five" + // [] denotes tracked deletes + // () denotes tracked inserts + this.ranges = { + changes: makeRanges([ + { d: '1', p: 2 }, + { d: '22', p: 3 }, + { d: '333', p: 4 }, + { i: 'three', p: 4 }, + { d: '4444', p: 12 }, + ]), + } + this.updates = makeUpdates([ + { i: 'zero ', p: 0 }, + { i: 'two ', p: 9, u: true }, + ]) + this.newDocLines = ['zero one two three four five'] + this.result = this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.ranges, + this.updates, + this.newDocLines, + { historyRangesSupport: true } + ) + }) + + it('should offset the hpos by the length of tracked deletes before the insert', function () { + expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ + [{ i: 'zero ', p: 0 }], + // 'two' is added just before the "333" tracked delete + [{ i: 'two ', p: 9, u: true, hpos: 12 }], + ]) + }) }) - it('should mark the insert as a tracked delete rejection where appropriate', function () { - expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ - [{ i: 'tw', p: 4, u: true, trackedDeleteRejection: true }], - ]) - }) - }) + describe('tracked delete rejections', function () { + beforeEach(function () { + // original text is "one [two ]three four five" + // [] denotes tracked deletes + this.ranges = { + changes: makeRanges([{ d: 'two ', p: 4 }]), + } + this.updates = makeUpdates([{ i: 'tw', p: 4, u: true }]) + this.newDocLines = ['one twthree four five'] + this.result = this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.ranges, + this.updates, + this.newDocLines, + { historyRangesSupport: true } + ) + }) - describe('deletes among tracked deletes', function () { - beforeEach(function () { - // original text is "on[1]e [22](three) f[333]ou[4444]r [55555]five" - // [] denotes tracked deletes - // () denotes tracked inserts - this.ranges = { - comments: [], - changes: makeRanges([ - { d: '1', p: 2 }, - { d: '22', p: 4 }, - { i: 'three', p: 4 }, - { d: '333', p: 11 }, - { d: '4444', p: 13 }, - { d: '55555', p: 15 }, - ]), - } - this.updates = makeUpdates([ - { d: 'four ', p: 10 }, - { d: 'three ', p: 4 }, - ]) - this.newDocLines = ['one five'] - this.result = this.RangesManager.applyUpdate( - this.project_id, - this.doc_id, - this.ranges, - this.updates, - this.newDocLines - ) + it('should mark the insert as a tracked delete rejection where appropriate', function () { + expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ + [{ i: 'tw', p: 4, u: true, trackedDeleteRejection: true }], + ]) + }) }) - it('should split and offset deletes appropriately', function () { - expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ - [ - // the "four" delete has tracked deletes inside it, add splits - { - d: 'four ', - p: 10, - hpos: 13, - hsplits: [ - { offset: 1, length: 3 }, - { offset: 3, length: 4 }, - ], - }, - ], + describe('deletes among tracked deletes', function () { + beforeEach(function () { + // original text is "on[1]e [22](three) f[333]ou[4444]r [55555]five" + // [] denotes tracked deletes + // () denotes tracked inserts + this.ranges = { + comments: [], + changes: makeRanges([ + { d: '1', p: 2 }, + { d: '22', p: 4 }, + { i: 'three', p: 4 }, + { d: '333', p: 11 }, + { d: '4444', p: 13 }, + { d: '55555', p: 15 }, + ]), + } + this.updates = makeUpdates([ + { d: 'four ', p: 10 }, + { d: 'three ', p: 4 }, + ]) + this.newDocLines = ['one five'] + this.result = this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.ranges, + this.updates, + this.newDocLines, + { historyRangesSupport: true } + ) + }) - // the "three" delete is offset to the right by the two first tracked - // deletes - [{ d: 'three ', p: 4, hpos: 7 }], - ]) - }) - }) + it('should split and offset deletes appropriately', function () { + expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ + [ + // the "four" delete has tracked deletes inside it, add splits + { + d: 'four ', + p: 10, + hpos: 13, + hsplits: [ + { offset: 1, length: 3 }, + { offset: 3, length: 4 }, + ], + }, + ], - describe('comments among tracked deletes', function () { - beforeEach(function () { - // original text is "on[1]e[22] [333](three) fo[4444]ur five" - // [] denotes tracked deletes - // () denotes tracked inserts - this.ranges = { - changes: makeRanges([ - { d: '1', p: 2 }, - { d: '22', p: 3 }, - { d: '333', p: 4 }, - { i: 'three', p: 4 }, - { d: '4444', p: 12 }, - ]), - } - this.updates = makeUpdates([ - { c: 'three ', p: 4 }, - { c: 'four ', p: 10 }, - ]) - this.newDocLines = ['one three four five'] - this.result = this.RangesManager.applyUpdate( - this.project_id, - this.doc_id, - this.ranges, - this.updates, - this.newDocLines - ) + // the "three" delete is offset to the right by the two first tracked + // deletes + [{ d: 'three ', p: 4, hpos: 7 }], + ]) + }) }) - it('should offset the hpos by the length of tracked deletes before the insert', function () { - expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ - [{ c: 'three ', p: 4, hpos: 10 }], - [{ c: 'four ', p: 10, hpos: 16, hlen: 9 }], - ]) - }) - }) + describe('comments among tracked deletes', function () { + beforeEach(function () { + // original text is "on[1]e[22] [333](three) fo[4444]ur five" + // [] denotes tracked deletes + // () denotes tracked inserts + this.ranges = { + changes: makeRanges([ + { d: '1', p: 2 }, + { d: '22', p: 3 }, + { d: '333', p: 4 }, + { i: 'three', p: 4 }, + { d: '4444', p: 12 }, + ]), + } + this.updates = makeUpdates([ + { c: 'three ', p: 4 }, + { c: 'four ', p: 10 }, + ]) + this.newDocLines = ['one three four five'] + this.result = this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.ranges, + this.updates, + this.newDocLines, + { historyRangesSupport: true } + ) + }) - describe('inserts inside comments', function () { - beforeEach(function () { - // original text is "one three four five" - this.ranges = { - comments: makeRanges([ - { c: 'three', p: 4, t: 'comment-id-1' }, - { c: 'ree four', p: 6, t: 'comment-id-2' }, - ]), - } - this.updates = makeUpdates([ - { i: '[before]', p: 4 }, - { i: '[inside]', p: 13 }, // 4 + 8 + 1 - { i: '[overlap]', p: 23 }, // 13 + 8 + 2 - { i: '[after]', p: 39 }, // 23 + 9 + 7 - ]) - this.newDocLines = [ - 'one [before]t[inside]hr[overlap]ee four[after] five', - ] - this.result = this.RangesManager.applyUpdate( - this.project_id, - this.doc_id, - this.ranges, - this.updates, - this.newDocLines - ) + it('should offset the hpos by the length of tracked deletes before the insert', function () { + expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ + [{ c: 'three ', p: 4, hpos: 10 }], + [{ c: 'four ', p: 10, hpos: 16, hlen: 9 }], + ]) + }) }) - it('should add the proper commentIds properties to ops', function () { - expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ - [{ i: '[before]', p: 4 }], - [{ i: '[inside]', p: 13, commentIds: ['comment-id-1'] }], - [ - { - i: '[overlap]', - p: 23, - commentIds: ['comment-id-1', 'comment-id-2'], - }, - ], - [{ i: '[after]', p: 39 }], - ]) + describe('inserts inside comments', function () { + beforeEach(function () { + // original text is "one three four five" + this.ranges = { + comments: makeRanges([ + { c: 'three', p: 4, t: 'comment-id-1' }, + { c: 'ree four', p: 6, t: 'comment-id-2' }, + ]), + } + this.updates = makeUpdates([ + { i: '[before]', p: 4 }, + { i: '[inside]', p: 13 }, // 4 + 8 + 1 + { i: '[overlap]', p: 23 }, // 13 + 8 + 2 + { i: '[after]', p: 39 }, // 23 + 9 + 7 + ]) + this.newDocLines = [ + 'one [before]t[inside]hr[overlap]ee four[after] five', + ] + this.result = this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.ranges, + this.updates, + this.newDocLines, + { historyRangesSupport: true } + ) + }) + + it('should add the proper commentIds properties to ops', function () { + expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ + [{ i: '[before]', p: 4 }], + [{ i: '[inside]', p: 13, commentIds: ['comment-id-1'] }], + [ + { + i: '[overlap]', + p: 23, + commentIds: ['comment-id-1', 'comment-id-2'], + }, + ], + [{ i: '[after]', p: 39 }], + ]) + }) }) }) }) diff --git a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js index 9bcbf0445b..20dd9a817d 100644 --- a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js +++ b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js @@ -396,7 +396,7 @@ describe('UpdateManager', function () { it('should add metadata to the ops', function () { this.UpdateManager.promises._addMetadataToHistoryUpdates.should.have.been.calledWith( - this.appliedOps, + this.historyUpdates, this.pathname, this.projectHistoryId, this.lines @@ -406,12 +406,12 @@ describe('UpdateManager', function () { it('should push the applied ops into the history queue', function () { this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWith( this.project_id, - ...this.appliedOps.map(op => JSON.stringify(op)) + ...this.historyUpdates.map(op => JSON.stringify(op)) ) this.HistoryManager.recordAndFlushHistoryOps.should.have.been.calledWith( this.project_id, - this.appliedOps, - this.appliedOps.length + this.historyUpdates, + this.historyUpdates.length ) }) }) diff --git a/services/web/frontend/js/features/ide-react/editor/document-container.ts b/services/web/frontend/js/features/ide-react/editor/document-container.ts index 5987ad2887..0afa883f13 100644 --- a/services/web/frontend/js/features/ide-react/editor/document-container.ts +++ b/services/web/frontend/js/features/ide-react/editor/document-container.ts @@ -722,7 +722,7 @@ export class DocumentContainer extends EventEmitter { this.emit('ranges:clear') this.ranges!.changes = changes this.ranges!.comments = comments - this.ranges!.track_changes = this.doc?.track_changes + this.ranges!.track_changes = this.doc?.track_changes ?? false for (const op of this.filterOps(this.doc?.getInflightOp() || [])) { this.ranges!.setIdSeed(this.doc?.track_changes_id_seeds?.inflight) this.ranges!.applyOp(op, { user_id: this.track_changes_as })