Merge pull request #16928 from overleaf/em-hpos-hlen

Add history metadata to updates sent to project-history

GitOrigin-RevId: 915beaa01f2bbe48869a40b229397be8e0401852

services/document-updater/app/js/RangesManager.js

@@ -1,7 +1,26 @@
+// @ts-check
+
 const RangesTracker = require('@overleaf/ranges-tracker')
 const logger = require('@overleaf/logger')
+const OError = require('@overleaf/o-error')
 const Metrics = require('./Metrics')
 const _ = require('lodash')
+const { isInsert, isDelete, isComment } = require('./Utils')
+
+/**
+ * @typedef {import('./types').CommentOp} CommentOp
+ * @typedef {import('./types').DeleteOp} DeleteOp
+ * @typedef {import('./types').HistoryCommentOp} HistoryCommentOp
+ * @typedef {import('./types').HistoryDeleteOp} HistoryDeleteOp
+ * @typedef {import('./types').HistoryInsertOp} HistoryInsertOp
+ * @typedef {import('./types').HistoryOp} HistoryOp
+ * @typedef {import('./types').HistoryUpdate} HistoryUpdate
+ * @typedef {import('./types').InsertOp} InsertOp
+ * @typedef {import('./types').Op} Op
+ * @typedef {import('./types').Ranges} Ranges
+ * @typedef {import('./types').TrackedChange} TrackedChange
+ * @typedef {import('./types').Update} Update
+ */
 
 const RANGE_DELTA_BUCKETS = [0, 1, 2, 3, 4, 5, 10, 20, 50]
 
@@ -9,25 +28,39 @@ const RangesManager = {
   MAX_COMMENTS: 500,
   MAX_CHANGES: 2000,
 
-  applyUpdate(projectId, docId, entries, updates, newDocLines) {
-    if (entries == null) {
-      entries = {}
+  /**
+   * Apply an update to the given doc (lines and ranges) and return new ranges
+   *
+   * @param {string} projectId
+   * @param {string} docId
+   * @param {Ranges} ranges - ranges before the updates were applied
+   * @param {Update[]} updates
+   * @param {string[]} newDocLines - the document lines after the updates were applied
+   * @returns {{ newRanges: Ranges, rangesWereCollapsed: boolean, historyUpdates: HistoryUpdate[] }}
+   */
+  applyUpdate(projectId, docId, ranges, updates, newDocLines) {
+    if (ranges == null) {
+      ranges = {}
     }
     if (updates == null) {
       updates = []
     }
-    const { changes, comments } = _.cloneDeep(entries)
+    const { changes, comments } = _.cloneDeep(ranges)
     const rangesTracker = new RangesTracker(changes, comments)
     const [emptyRangeCountBefore, totalRangeCountBefore] =
       RangesManager._emptyRangesCount(rangesTracker)
+    const historyUpdates = []
     for (const update of updates) {
-      rangesTracker.track_changes = !!update.meta.tc
-      if (update.meta.tc) {
+      rangesTracker.track_changes = Boolean(update.meta?.tc)
+      if (update.meta?.tc) {
         rangesTracker.setIdSeed(update.meta.tc)
       }
+      const historyOps = []
       for (const op of update.op) {
+        historyOps.push(getHistoryOp(op, rangesTracker.changes))
         rangesTracker.applyOp(op, { user_id: update.meta?.user_id })
       }
+      historyUpdates.push({ ...update, op: historyOps })
     }
 
     if (
@@ -74,7 +107,7 @@ const RangesManager = {
       },
       'applied updates to ranges'
     )
-    return { newRanges, rangesWereCollapsed }
+    return { newRanges, rangesWereCollapsed, historyUpdates }
   },
 
   acceptChanges(changeIds, ranges) {
@@ -98,17 +131,12 @@ const RangesManager = {
   _getRanges(rangesTracker) {
     // Return the minimal data structure needed, since most documents won't have any
     // changes or comments
-    let response = {}
+
+    const response = {}
     if (rangesTracker.changes != null && rangesTracker.changes.length > 0) {
-      if (response == null) {
-        response = {}
-      }
       response.changes = rangesTracker.changes
     }
     if (rangesTracker.comments != null && rangesTracker.comments.length > 0) {
-      if (response == null) {
-        response = {}
-      }
       response.comments = rangesTracker.comments
     }
     return response
@@ -135,4 +163,166 @@ const RangesManager = {
   },
 }
 
+/**
+ * Calculate ops to be sent to the history system.
+ *
+ * @param {Op} op - the editor op
+ * @param {TrackedChange[]} changes - the list of tracked changes in the
+ *        document before the op is applied. That list, coming from
+ *        RangesTracker is ordered by position.
+ * @returns {HistoryOp}
+ */
+function getHistoryOp(op, changes, opts = {}) {
+  if (isInsert(op)) {
+    return getHistoryOpForInsert(op, changes)
+  } else if (isDelete(op)) {
+    return getHistoryOpForDelete(op, changes)
+  } else if (isComment(op)) {
+    return getHistoryOpForComment(op, changes)
+  } else {
+    throw new OError('Unrecognized op', { op })
+  }
+}
+
+/**
+ * Calculate history ops for an insert
+ *
+ * Inserts are moved forward by tracked deletes placed strictly before the
+ * op. When an insert is made at the same position as a tracked delete, the
+ * insert is placed before the tracked delete.
+ *
+ * @param {InsertOp} op
+ * @param {TrackedChange[]} changes
+ * @returns {HistoryInsertOp}
+ */
+function getHistoryOpForInsert(op, changes) {
+  let hpos = op.p
+  let trackedDeleteRejection = false
+  for (const change of changes) {
+    if (!isDelete(change.op)) {
+      // We're only interested in tracked deletes
+      continue
+    }
+
+    if (change.op.p < op.p) {
+      // Tracked delete is before the op. Move the op forward.
+      hpos += change.op.d.length
+    } else if (change.op.p === op.p) {
+      // Tracked delete is at the same position as the op. The insert comes before
+      // the tracked delete so it doesn't move.
+      if (op.u && change.op.d.startsWith(op.i)) {
+        // We're undoing and the insert matches the start of the tracked
+        // delete. RangesManager treats this as a tracked delete rejection. We
+        // will note this in the op so that project-history can take the
+        // appropriate action.
+        trackedDeleteRejection = true
+      }
+    } else {
+      // Tracked delete is after the insert. Tracked deletes are ordered, so
+      // we know that all subsequent tracked deletes will be after the insert
+      // and we can bail out.
+      break
+    }
+  }
+
+  /** @type {HistoryInsertOp} */
+  const historyOp = { ...op }
+  if (hpos !== op.p) {
+    historyOp.hpos = hpos
+  }
+  if (trackedDeleteRejection) {
+    historyOp.trackedDeleteRejection = true
+  }
+  return historyOp
+}
+
+/**
+ * Calculate history op for a delete
+ *
+ * Deletes are moved forward by tracked deletes placed before or at the position of the
+ * op. If a tracked delete is inside the delete, the delete is split in parts
+ * so that characters are deleted around the tracked delete, but the tracked
+ * delete itself is not deleted.
+ *
+ * @param {DeleteOp} op
+ * @param {TrackedChange[]} changes
+ * @returns {HistoryDeleteOp}
+ */
+function getHistoryOpForDelete(op, changes, opts = {}) {
+  let hpos = op.p
+  const hsplits = []
+  for (const change of changes) {
+    if (!isDelete(change.op)) {
+      // We're only interested in tracked deletes
+      continue
+    }
+
+    if (change.op.p <= op.p) {
+      // Tracked delete is before or at the position of the incoming delete.
+      // Move the op forward.
+      hpos += change.op.d.length
+    } else if (change.op.p < op.p + op.d.length) {
+      // Tracked delete inside the deleted text. Record a split for the history system.
+      hsplits.push({ offset: change.op.p - op.p, length: change.op.d.length })
+    } else {
+      // We've seen all tracked deletes before or inside the delete
+      break
+    }
+  }
+
+  /** @type {HistoryDeleteOp} */
+  const historyOp = { ...op }
+  if (hpos !== op.p) {
+    historyOp.hpos = hpos
+  }
+  if (hsplits.length > 0) {
+    historyOp.hsplits = hsplits
+  }
+  return historyOp
+}
+
+/**
+ * Calculate history ops for a comment
+ *
+ * Comments are moved forward by tracked deletes placed before or at the
+ * position of the op. If a tracked delete is inside the comment, the length of
+ * the comment is extended to include the tracked delete.
+ *
+ * @param {CommentOp} op
+ * @param {TrackedChange[]} changes
+ * @returns {HistoryCommentOp}
+ */
+function getHistoryOpForComment(op, changes) {
+  let hpos = op.p
+  let hlen = op.c.length
+  for (const change of changes) {
+    if (!isDelete(change.op)) {
+      // We're only interested in tracked deletes
+      continue
+    }
+
+    if (change.op.p <= op.p) {
+      // Tracked delete is before or at the position of the incoming comment.
+      // Move the op forward.
+      hpos += change.op.d.length
+    } else if (change.op.p < op.p + op.c.length) {
+      // Tracked comment inside the comment. Extend the length
+      hlen += change.op.d.length
+    } else {
+      // We've seen all tracked deletes before or inside the comment
+      break
+    }
+  }
+
+  /** @type {HistoryCommentOp} */
+  const historyOp = { ...op }
+  if (hpos !== op.p) {
+    historyOp.hpos = hpos
+  }
+  if (hlen !== op.c.length) {
+    historyOp.hlen = hlen
+  }
+  return historyOp
+}
+
 module.exports = RangesManager

services/document-updater/app/js/RedisManager.js

@@ -8,7 +8,6 @@ const metrics = require('./Metrics')
 const Errors = require('./Errors')
 const crypto = require('crypto')
 const async = require('async')
-const ProjectHistoryRedisManager = require('./ProjectHistoryRedisManager')
 const { docIsTooLarge } = require('./Limits')
 
 // Sometimes Redis calls take an unexpectedly long time.  We have to be
@@ -477,25 +476,7 @@ const RedisManager = {
           if (error) {
             return callback(error)
           }
-
-          if (jsonOps.length > 0) {
-            metrics.inc('history-queue', 1, { status: 'project-history' })
-            ProjectHistoryRedisManager.queueOps(
-              projectId,
-              ...jsonOps,
-              (error, projectUpdateCount) => {
-                if (error) {
-                  // The full project history can re-sync a project in case
-                  //  updates went missing.
-                  // Just record the error here and acknowledge the write-op.
-                  metrics.inc('history-queue-error')
-                }
-                callback(null, projectUpdateCount)
-              }
-            )
-          } else {
-            callback(null)
-          }
+          callback()
         })
       })
     })

services/document-updater/app/js/UpdateManager.js

@@ -3,6 +3,7 @@
 const { callbackifyAll } = require('@overleaf/promise-utils')
 const LockManager = require('./LockManager')
 const RedisManager = require('./RedisManager')
+const ProjectHistoryRedisManager = require('./ProjectHistoryRedisManager')
 const RealTimeRedisManager = require('./RealTimeRedisManager')
 const ShareJsUpdateManager = require('./ShareJsUpdateManager')
 const HistoryManager = require('./HistoryManager')
@@ -14,6 +15,15 @@ const DocumentManager = require('./DocumentManager')
 const RangesManager = require('./RangesManager')
 const SnapshotManager = require('./SnapshotManager')
 const Profiler = require('./Profiler')
+const { isInsert, isDelete } = require('./Utils')
+
+/**
+ * @typedef {import("./types").DeleteOp} DeleteOp
+ * @typedef {import("./types").InsertOp} InsertOp
+ * @typedef {import("./types").Op} Op
+ * @typedef {import("./types").Ranges} Ranges
+ * @typedef {import("./types").Update} Update
+ */
 
 const UpdateManager = {
   async processOutstandingUpdates(projectId, docId) {
@@ -82,6 +92,13 @@ const UpdateManager = {
     profile.log('async done').end()
   },
 
+  /**
+   * Apply an update to the given document
+   *
+   * @param {string} projectId
+   * @param {string} docId
+   * @param {Update} update
+   */
   async applyUpdate(projectId, docId, update) {
     const profile = new Profiler('applyUpdate', {
       project_id: projectId,
@@ -92,8 +109,14 @@ const UpdateManager = {
     profile.log('sanitizeUpdate', { sync: true })
 
     try {
-      let { lines, version, ranges, pathname, projectHistoryId } =
-        await DocumentManager.promises.getDoc(projectId, docId)
+      let {
+        lines,
+        version,
+        ranges,
+        pathname,
+        projectHistoryId,
+        historyRangesSupport,
+      } = await DocumentManager.promises.getDoc(projectId, docId)
       profile.log('getDoc')
 
       if (lines == null || version == null) {
@@ -117,13 +140,20 @@ const UpdateManager = {
         sync: incomingUpdateVersion === previousVersion,
       })
 
-      const { newRanges, rangesWereCollapsed } = RangesManager.applyUpdate(
-        projectId,
-        docId,
-        ranges,
-        appliedOps,
-        updatedDocLines
-      )
+      let { newRanges, rangesWereCollapsed, historyUpdates } =
+        RangesManager.applyUpdate(
+          projectId,
+          docId,
+          ranges,
+          appliedOps,
+          updatedDocLines
+        )
+      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 })
 
       UpdateManager._addProjectHistoryMetadataToOps(
@@ -132,8 +162,7 @@ const UpdateManager = {
         projectHistoryId,
         lines
       )
-
-      const projectOpsLength = await RedisManager.promises.updateDocument(
+      await RedisManager.promises.updateDocument(
         projectId,
         docId,
         updatedDocLines,
@@ -144,12 +173,27 @@ const UpdateManager = {
       )
       profile.log('RedisManager.updateDocument')
 
-      HistoryManager.recordAndFlushHistoryOps(
-        projectId,
-        appliedOps,
-        projectOpsLength
-      )
-      profile.log('recordAndFlushHistoryOps')
+      if (historyUpdates.length > 0) {
+        Metrics.inc('history-queue', 1, { status: 'project-history' })
+        try {
+          const projectOpsLength =
+            await ProjectHistoryRedisManager.promises.queueOps(
+              projectId,
+              ...historyUpdates.map(op => JSON.stringify(op))
+            )
+          HistoryManager.recordAndFlushHistoryOps(
+            projectId,
+            historyUpdates,
+            projectOpsLength
+          )
+          profile.log('recordAndFlushHistoryOps')
+        } catch (err) {
+          // The full project history can re-sync a project in case
+          //  updates went missing.
+          // Just record the error here and acknowledge the write-op.
+          Metrics.inc('history-queue-error')
+        }
+      }
 
       if (rangesWereCollapsed) {
         Metrics.inc('doc-snapshot')
@@ -259,10 +303,19 @@ const UpdateManager = {
     return update
   },
 
+  /**
+   * Add metadata to ops that will be useful to project history
+   *
+   * @param {Update[]} updates
+   * @param {string} pathname
+   * @param {string} projectHistoryId
+   * @param {string[]} lines
+   */
   _addProjectHistoryMetadataToOps(updates, pathname, projectHistoryId, lines) {
     let docLength = _.reduce(lines, (chars, line) => chars + line.length, 0)
     docLength += lines.length - 1 // count newline characters
-    updates.forEach(function (update) {
+
+    for (const update of updates) {
       update.projectHistoryId = projectHistoryId
       if (!update.meta) {
         update.meta = {}
@@ -279,14 +332,14 @@ const UpdateManager = {
       // before it's ops are applied. However, we need to track any
       // changes to it for the next update.
       for (const op of update.op) {
-        if (op.i != null) {
+        if (isInsert(op)) {
           docLength += op.i.length
         }
-        if (op.d != null) {
+        if (isDelete(op)) {
           docLength -= op.d.length
         }
       }
-    })
+    }
   },
 }
 

services/document-updater/app/js/Utils.js

@@ -0,0 +1,40 @@
+// @ts-check
+
+/**
+ * @typedef {import('./types').CommentOp} CommentOp
+ * @typedef {import('./types').DeleteOp} DeleteOp
+ * @typedef {import('./types').InsertOp} InsertOp
+ * @typedef {import('./types').Op} Op
+ */
+
+/**
+ * Returns true if the op is an insert
+ *
+ * @param {Op} op
+ * @returns {op is InsertOp}
+ */
+function isInsert(op) {
+  return 'i' in op && op.i != null
+}
+
+/**
+ * Returns true if the op is an insert
+ *
+ * @param {Op} op
+ * @returns {op is DeleteOp}
+ */
+function isDelete(op) {
+  return 'd' in op && op.d != null
+}
+
+/**
+ * Returns true if the op is a comment
+ *
+ * @param {Op} op
+ * @returns {op is CommentOp}
+ */
+function isComment(op) {
+  return 'c' in op && op.c != null
+}
+
+module.exports = { isInsert, isDelete, isComment }

services/document-updater/app/js/types.ts

@@ -0,0 +1,89 @@
+export type Update = {
+  op: Op[]
+  v: number
+  meta?: {
+    pathname?: string
+    doc_length?: number
+    tc?: boolean
+    user_id?: string
+  }
+  projectHistoryId?: string
+}
+
+export type Op = InsertOp | DeleteOp | CommentOp
+
+export type InsertOp = {
+  i: string
+  p: number
+  u?: boolean
+}
+
+export type DeleteOp = {
+  d: string
+  p: number
+  u?: boolean
+}
+
+export type CommentOp = {
+  c: string
+  p: number
+  t: string
+  u?: boolean
+}
+
+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
+  }
+}
+
+export type HistoryOp = HistoryInsertOp | HistoryDeleteOp | HistoryCommentOp
+
+export type HistoryInsertOp = InsertOp & {
+  hpos?: number
+  trackedDeleteRejection?: boolean
+}
+
+export type HistoryDeleteOp = DeleteOp & {
+  hpos?: number
+  hsplits?: HistoryDeleteSplit[]
+}
+
+export type HistoryDeleteSplit = {
+  offset: number
+  length: number
+}
+
+export type HistoryCommentOp = CommentOp & {
+  hpos?: number
+  hlen?: number
+}
+
+export type HistoryUpdate = {
+  op: HistoryOp[]
+  v: number
+  meta?: {
+    pathname?: string
+    doc_length?: number
+    tc?: boolean
+    user_id?: string
+  }
+  projectHistoryId?: string
+}

services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js

@@ -1,11 +1,3 @@
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS101: Remove unnecessary use of Array.from
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const sinon = require('sinon')
 const { expect } = require('chai')
 const async = require('async')
@@ -27,14 +19,13 @@ describe('Applying updates to a doc', function () {
   before(function (done) {
     this.lines = ['one', 'two', 'three']
     this.version = 42
+    this.op = {
+      i: 'one and a half\n',
+      p: 4,
+    }
     this.update = {
       doc: this.doc_id,
-      op: [
-        {
-          i: 'one and a half\n',
-          p: 4,
-        },
-      ],
+      op: [this.op],
       v: this.version,
     }
     this.result = ['one', 'one and a half', 'two', 'three']
@@ -43,10 +34,8 @@ describe('Applying updates to a doc', function () {
 
   describe('when the document is not loaded', function () {
     before(function (done) {
-      ;[this.project_id, this.doc_id] = Array.from([
-        DocUpdaterClient.randomId(),
-        DocUpdaterClient.randomId(),
-      ])
+      this.project_id = DocUpdaterClient.randomId()
+      this.doc_id = DocUpdaterClient.randomId()
       sinon.spy(MockWebApi, 'getDocument')
       this.startTime = Date.now()
       MockWebApi.insertDoc(this.project_id, this.doc_id, {
@@ -97,7 +86,7 @@ describe('Applying updates to a doc', function () {
           if (error != null) {
             throw error
           }
-          JSON.parse(updates[0]).op.should.deep.equal(this.update.op)
+          JSON.parse(updates[0]).op.should.deep.equal([this.op])
           done()
         }
       )
@@ -158,10 +147,8 @@ describe('Applying updates to a doc', function () {
 
   describe('when the document is loaded', function () {
     before(function (done) {
-      ;[this.project_id, this.doc_id] = Array.from([
-        DocUpdaterClient.randomId(),
-        DocUpdaterClient.randomId(),
-      ])
+      this.project_id = DocUpdaterClient.randomId()
+      this.doc_id = DocUpdaterClient.randomId()
 
       MockWebApi.insertDoc(this.project_id, this.doc_id, {
         lines: this.lines,
@@ -213,7 +200,7 @@ describe('Applying updates to a doc', function () {
         -1,
         (error, updates) => {
           if (error) return done(error)
-          JSON.parse(updates[0]).op.should.deep.equal(this.update.op)
+          JSON.parse(updates[0]).op.should.deep.equal([this.op])
           done()
         }
       )
@@ -222,10 +209,8 @@ describe('Applying updates to a doc', function () {
 
   describe('when the document is loaded and is using project-history only', function () {
     before(function (done) {
-      ;[this.project_id, this.doc_id] = Array.from([
-        DocUpdaterClient.randomId(),
-        DocUpdaterClient.randomId(),
-      ])
+      this.project_id = DocUpdaterClient.randomId()
+      this.doc_id = DocUpdaterClient.randomId()
 
       MockWebApi.insertDoc(this.project_id, this.doc_id, {
         lines: this.lines,
@@ -273,7 +258,7 @@ describe('Applying updates to a doc', function () {
         -1,
         (error, updates) => {
           if (error) return done(error)
-          JSON.parse(updates[0]).op.should.deep.equal(this.update.op)
+          JSON.parse(updates[0]).op.should.deep.equal([this.op])
           done()
         }
       )
@@ -283,10 +268,8 @@ describe('Applying updates to a doc', function () {
   describe('when the document has been deleted', function () {
     describe('when the ops come in a single linear order', function () {
       before(function (done) {
-        ;[this.project_id, this.doc_id] = Array.from([
-          DocUpdaterClient.randomId(),
-          DocUpdaterClient.randomId(),
-        ])
+        this.project_id = DocUpdaterClient.randomId()
+        this.doc_id = DocUpdaterClient.randomId()
         const lines = ['', '', '']
         MockWebApi.insertDoc(this.project_id, this.doc_id, {
           lines,
@@ -312,7 +295,7 @@ describe('Applying updates to a doc', function () {
       it('should be able to continue applying updates when the project has been deleted', function (done) {
         let update
         const actions = []
-        for (update of Array.from(this.updates.slice(0, 6))) {
+        for (update of this.updates.slice(0, 6)) {
           ;(update => {
             actions.push(callback =>
               DocUpdaterClient.sendUpdate(
@@ -327,7 +310,7 @@ describe('Applying updates to a doc', function () {
         actions.push(callback =>
           DocUpdaterClient.deleteDoc(this.project_id, this.doc_id, callback)
         )
-        for (update of Array.from(this.updates.slice(6))) {
+        for (update of this.updates.slice(6)) {
           ;(update => {
             actions.push(callback =>
               DocUpdaterClient.sendUpdate(
@@ -363,7 +346,7 @@ describe('Applying updates to a doc', function () {
           -1,
           (error, updates) => {
             if (error) return done(error)
-            updates = Array.from(updates).map(u => JSON.parse(u))
+            updates = updates.map(u => JSON.parse(u))
             for (let i = 0; i < this.updates.length; i++) {
               const appliedUpdate = this.updates[i]
               appliedUpdate.op.should.deep.equal(updates[i].op)
@@ -376,10 +359,8 @@ describe('Applying updates to a doc', function () {
 
     describe('when older ops come in after the delete', function () {
       before(function (done) {
-        ;[this.project_id, this.doc_id] = Array.from([
-          DocUpdaterClient.randomId(),
-          DocUpdaterClient.randomId(),
-        ])
+        this.project_id = DocUpdaterClient.randomId()
+        this.doc_id = DocUpdaterClient.randomId()
         const lines = ['', '', '']
         MockWebApi.insertDoc(this.project_id, this.doc_id, {
           lines,
@@ -400,7 +381,7 @@ describe('Applying updates to a doc', function () {
       it('should be able to continue applying updates when the project has been deleted', function (done) {
         let update
         const actions = []
-        for (update of Array.from(this.updates.slice(0, 5))) {
+        for (update of this.updates.slice(0, 5)) {
           ;(update => {
             actions.push(callback =>
               DocUpdaterClient.sendUpdate(
@@ -415,7 +396,7 @@ describe('Applying updates to a doc', function () {
         actions.push(callback =>
           DocUpdaterClient.deleteDoc(this.project_id, this.doc_id, callback)
         )
-        for (update of Array.from(this.updates.slice(5))) {
+        for (update of this.updates.slice(5)) {
           ;(update => {
             actions.push(callback =>
               DocUpdaterClient.sendUpdate(
@@ -448,10 +429,8 @@ describe('Applying updates to a doc', function () {
 
   describe('with a broken update', function () {
     before(function (done) {
-      ;[this.project_id, this.doc_id] = Array.from([
-        DocUpdaterClient.randomId(),
-        DocUpdaterClient.randomId(),
-      ])
+      this.project_id = DocUpdaterClient.randomId()
+      this.doc_id = DocUpdaterClient.randomId()
       this.broken_update = {
         doc_id: this.doc_id,
         v: this.version,
@@ -493,7 +472,7 @@ describe('Applying updates to a doc', function () {
 
     it('should send a message with an error', function () {
       this.messageCallback.called.should.equal(true)
-      const [channel, message] = Array.from(this.messageCallback.args[0])
+      const [channel, message] = this.messageCallback.args[0]
       channel.should.equal('applied-ops')
       JSON.parse(message).should.deep.include({
         project_id: this.project_id,
@@ -505,10 +484,8 @@ describe('Applying updates to a doc', function () {
 
   describe('when there is no version in Mongo', function () {
     before(function (done) {
-      ;[this.project_id, this.doc_id] = Array.from([
-        DocUpdaterClient.randomId(),
-        DocUpdaterClient.randomId(),
-      ])
+      this.project_id = DocUpdaterClient.randomId()
+      this.doc_id = DocUpdaterClient.randomId()
       MockWebApi.insertDoc(this.project_id, this.doc_id, {
         lines: this.lines,
       })
@@ -546,10 +523,8 @@ describe('Applying updates to a doc', function () {
 
   describe('when the sending duplicate ops', function () {
     before(function (done) {
-      ;[this.project_id, this.doc_id] = Array.from([
-        DocUpdaterClient.randomId(),
-        DocUpdaterClient.randomId(),
-      ])
+      this.project_id = DocUpdaterClient.randomId()
+      this.doc_id = DocUpdaterClient.randomId()
       MockWebApi.insertDoc(this.project_id, this.doc_id, {
         lines: this.lines,
         version: this.version,
@@ -633,10 +608,8 @@ describe('Applying updates to a doc', function () {
 
   describe('when sending updates for a non-existing doc id', function () {
     before(function (done) {
-      ;[this.project_id, this.doc_id] = Array.from([
-        DocUpdaterClient.randomId(),
-        DocUpdaterClient.randomId(),
-      ])
+      this.project_id = DocUpdaterClient.randomId()
+      this.doc_id = DocUpdaterClient.randomId()
       this.non_existing = {
         doc_id: this.doc_id,
         v: this.version,
@@ -674,7 +647,7 @@ describe('Applying updates to a doc', function () {
 
     it('should send a message with an error', function () {
       this.messageCallback.called.should.equal(true)
-      const [channel, message] = Array.from(this.messageCallback.args[0])
+      const [channel, message] = this.messageCallback.args[0]
       channel.should.equal('applied-ops')
       JSON.parse(message).should.deep.include({
         project_id: this.project_id,

services/document-updater/test/acceptance/js/RangesTests.js

@@ -1,15 +1,3 @@
-/* eslint-disable
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS101: Remove unnecessary use of Array.from
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const sinon = require('sinon')
 const { expect } = require('chai')
 const async = require('async')
@@ -21,7 +9,7 @@ const DocUpdaterApp = require('./helpers/DocUpdaterApp')
 
 describe('Ranges', function () {
   before(function (done) {
-    return DocUpdaterApp.ensureRunning(done)
+    DocUpdaterApp.ensureRunning(done)
   })
 
   describe('tracking changes from ops', function () {
@@ -58,43 +46,37 @@ describe('Ranges', function () {
         version: 0,
       })
       const jobs = []
-      for (const update of Array.from(this.updates)) {
-        ;(update => {
-          return jobs.push(callback =>
-            DocUpdaterClient.sendUpdate(
-              this.project_id,
-              this.doc.id,
-              update,
-              callback
-            )
+      for (const update of this.updates) {
+        jobs.push(callback =>
+          DocUpdaterClient.sendUpdate(
+            this.project_id,
+            this.doc.id,
+            update,
+            callback
           )
-        })(update)
+        )
       }
 
-      return DocUpdaterApp.ensureRunning(error => {
+      DocUpdaterApp.ensureRunning(error => {
         if (error != null) {
           throw error
         }
-        return DocUpdaterClient.preloadDoc(
-          this.project_id,
-          this.doc.id,
-          error => {
+        DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => {
+          if (error != null) {
+            throw error
+          }
+          async.series(jobs, error => {
             if (error != null) {
               throw error
             }
-            return async.series(jobs, error => {
-              if (error != null) {
-                throw error
-              }
-              return done()
-            })
-          }
-        )
+            done()
+          })
+        })
       })
     })
 
     it('should update the ranges', function (done) {
-      return DocUpdaterClient.getDoc(
+      DocUpdaterClient.getDoc(
         this.project_id,
         this.doc.id,
         (error, res, data) => {
@@ -106,12 +88,12 @@ describe('Ranges', function () {
           change.op.should.deep.equal({ i: '456', p: 3 })
           change.id.should.equal(this.id_seed + '000001')
           change.metadata.user_id.should.equal(this.user_id)
-          return done()
+          done()
         }
       )
     })
 
-    return describe('Adding comments', function () {
+    describe('Adding comments', function () {
       describe('standalone', function () {
         before(function (done) {
           this.project_id = DocUpdaterClient.randomId()
@@ -134,37 +116,31 @@ describe('Ranges', function () {
             version: 0,
           })
           const jobs = []
-          for (const update of Array.from(this.updates)) {
-            ;(update => {
-              return jobs.push(callback =>
-                DocUpdaterClient.sendUpdate(
-                  this.project_id,
-                  this.doc.id,
-                  update,
-                  callback
-                )
+          for (const update of this.updates) {
+            jobs.push(callback =>
+              DocUpdaterClient.sendUpdate(
+                this.project_id,
+                this.doc.id,
+                update,
+                callback
               )
-            })(update)
+            )
           }
-          return DocUpdaterClient.preloadDoc(
-            this.project_id,
-            this.doc.id,
-            error => {
+          DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => {
+            if (error != null) {
+              throw error
+            }
+            async.series(jobs, error => {
               if (error != null) {
                 throw error
               }
-              return async.series(jobs, error => {
-                if (error != null) {
-                  throw error
-                }
-                return setTimeout(done, 200)
-              })
-            }
-          )
+              setTimeout(done, 200)
+            })
+          })
         })
 
-        return it('should update the ranges', function (done) {
-          return DocUpdaterClient.getDoc(
+        it('should update the ranges', function (done) {
+          DocUpdaterClient.getDoc(
             this.project_id,
             this.doc.id,
             (error, res, data) => {
@@ -175,13 +151,13 @@ describe('Ranges', function () {
               const comment = ranges.comments[0]
               comment.op.should.deep.equal({ c: 'bar', p: 4, t: this.tid })
               comment.id.should.equal(this.tid)
-              return done()
+              done()
             }
           )
         })
       })
 
-      return describe('with conflicting ops needing OT', function () {
+      describe('with conflicting ops needing OT', function () {
         before(function (done) {
           this.project_id = DocUpdaterClient.randomId()
           this.user_id = DocUpdaterClient.randomId()
@@ -209,37 +185,31 @@ describe('Ranges', function () {
             version: 0,
           })
           const jobs = []
-          for (const update of Array.from(this.updates)) {
-            ;(update => {
-              return jobs.push(callback =>
-                DocUpdaterClient.sendUpdate(
-                  this.project_id,
-                  this.doc.id,
-                  update,
-                  callback
-                )
+          for (const update of this.updates) {
+            jobs.push(callback =>
+              DocUpdaterClient.sendUpdate(
+                this.project_id,
+                this.doc.id,
+                update,
+                callback
               )
-            })(update)
+            )
           }
-          return DocUpdaterClient.preloadDoc(
-            this.project_id,
-            this.doc.id,
-            error => {
+          DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => {
+            if (error != null) {
+              throw error
+            }
+            async.series(jobs, error => {
               if (error != null) {
                 throw error
               }
-              return async.series(jobs, error => {
-                if (error != null) {
-                  throw error
-                }
-                return setTimeout(done, 200)
-              })
-            }
-          )
+              setTimeout(done, 200)
+            })
+          })
         })
 
-        return it('should update the comments with the OT shifted comment', function (done) {
-          return DocUpdaterClient.getDoc(
+        it('should update the comments with the OT shifted comment', function (done) {
+          DocUpdaterClient.getDoc(
             this.project_id,
             this.doc.id,
             (error, res, data) => {
@@ -249,7 +219,7 @@ describe('Ranges', function () {
               const { ranges } = data
               const comment = ranges.comments[0]
               comment.op.should.deep.equal({ c: 'bar', p: 7, t: this.tid })
-              return done()
+              done()
             }
           )
         })
@@ -287,30 +257,26 @@ describe('Ranges', function () {
           ],
         },
       })
-      return DocUpdaterClient.preloadDoc(
-        this.project_id,
-        this.doc.id,
-        error => {
-          if (error != null) {
-            throw error
-          }
-          return DocUpdaterClient.sendUpdate(
-            this.project_id,
-            this.doc.id,
-            this.update,
-            error => {
-              if (error != null) {
-                throw error
-              }
-              return setTimeout(done, 200)
-            }
-          )
+      DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => {
+        if (error != null) {
+          throw error
         }
-      )
+        DocUpdaterClient.sendUpdate(
+          this.project_id,
+          this.doc.id,
+          this.update,
+          error => {
+            if (error != null) {
+              throw error
+            }
+            setTimeout(done, 200)
+          }
+        )
+      })
     })
 
     it('should have preloaded the existing ranges', function (done) {
-      return DocUpdaterClient.getDoc(
+      DocUpdaterClient.getDoc(
         this.project_id,
         this.doc.id,
         (error, res, data) => {
@@ -320,27 +286,23 @@ describe('Ranges', function () {
           const { changes } = data.ranges
           changes[0].op.should.deep.equal({ i: '123', p: 1 })
           changes[1].op.should.deep.equal({ i: '456', p: 5 })
-          return done()
+          done()
         }
       )
     })
 
-    return it('should flush the ranges to the persistence layer again', function (done) {
-      return DocUpdaterClient.flushDoc(this.project_id, this.doc.id, error => {
+    it('should flush the ranges to the persistence layer again', function (done) {
+      DocUpdaterClient.flushDoc(this.project_id, this.doc.id, error => {
         if (error != null) {
           throw error
         }
-        return MockWebApi.getDocument(
-          this.project_id,
-          this.doc.id,
-          (error, doc) => {
-            if (error) return done(error)
-            const { changes } = doc.ranges
-            changes[0].op.should.deep.equal({ i: '123', p: 1 })
-            changes[1].op.should.deep.equal({ i: '456', p: 5 })
-            return done()
-          }
-        )
+        MockWebApi.getDocument(this.project_id, this.doc.id, (error, doc) => {
+          if (error) return done(error)
+          const { changes } = doc.ranges
+          changes[0].op.should.deep.equal({ i: '123', p: 1 })
+          changes[1].op.should.deep.equal({ i: '456', p: 5 })
+          done()
+        })
       })
     })
   })
@@ -365,49 +327,45 @@ describe('Ranges', function () {
         lines: this.doc.lines,
         version: 0,
       })
-      return DocUpdaterClient.preloadDoc(
-        this.project_id,
-        this.doc.id,
-        error => {
-          if (error != null) {
-            throw error
-          }
-          return DocUpdaterClient.sendUpdate(
-            this.project_id,
-            this.doc.id,
-            this.update,
-            error => {
-              if (error != null) {
-                throw error
-              }
-              return setTimeout(() => {
-                return DocUpdaterClient.getDoc(
-                  this.project_id,
-                  this.doc.id,
-                  (error, res, data) => {
-                    if (error != null) {
-                      throw error
-                    }
-                    const { ranges } = data
-                    const change = ranges.changes[0]
-                    change.op.should.deep.equal({ i: '456', p: 1 })
-                    change.id.should.equal(this.id_seed + '000001')
-                    change.metadata.user_id.should.equal(this.user_id)
-                    return done()
-                  }
-                )
-              }, 200)
-            }
-          )
+      DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => {
+        if (error != null) {
+          throw error
         }
-      )
+        DocUpdaterClient.sendUpdate(
+          this.project_id,
+          this.doc.id,
+          this.update,
+          error => {
+            if (error != null) {
+              throw error
+            }
+            setTimeout(() => {
+              DocUpdaterClient.getDoc(
+                this.project_id,
+                this.doc.id,
+                (error, res, data) => {
+                  if (error != null) {
+                    throw error
+                  }
+                  const { ranges } = data
+                  const change = ranges.changes[0]
+                  change.op.should.deep.equal({ i: '456', p: 1 })
+                  change.id.should.equal(this.id_seed + '000001')
+                  change.metadata.user_id.should.equal(this.user_id)
+                  done()
+                }
+              )
+            }, 200)
+          }
+        )
+      })
     })
     afterEach(function () {
       MockWebApi.setDocument.restore()
     })
 
     it('should remove the change after accepting', function (done) {
-      return DocUpdaterClient.acceptChange(
+      DocUpdaterClient.acceptChange(
         this.project_id,
         this.doc.id,
         this.id_seed + '000001',
@@ -415,7 +373,7 @@ describe('Ranges', function () {
           if (error != null) {
             throw error
           }
-          return DocUpdaterClient.getDoc(
+          DocUpdaterClient.getDoc(
             this.project_id,
             this.doc.id,
             (error, res, data) => {
@@ -423,7 +381,7 @@ describe('Ranges', function () {
                 throw error
               }
               expect(data.ranges.changes).to.be.undefined
-              return done()
+              done()
             }
           )
         }
@@ -483,45 +441,41 @@ describe('Ranges', function () {
         lines: this.doc.lines,
         version: 0,
       })
-      return DocUpdaterClient.preloadDoc(
-        this.project_id,
-        this.doc.id,
-        error => {
-          if (error != null) {
-            throw error
-          }
-          return DocUpdaterClient.sendUpdate(
-            this.project_id,
-            this.doc.id,
-            this.update,
-            error => {
-              if (error != null) {
-                throw error
-              }
-              return setTimeout(() => {
-                return DocUpdaterClient.getDoc(
-                  this.project_id,
-                  this.doc.id,
-                  (error, res, data) => {
-                    if (error != null) {
-                      throw error
-                    }
-                    const { ranges } = data
-                    const change = ranges.comments[0]
-                    change.op.should.deep.equal({ c: 'bar', p: 4, t: this.tid })
-                    change.id.should.equal(this.tid)
-                    return done()
-                  }
-                )
-              }, 200)
-            }
-          )
+      DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => {
+        if (error != null) {
+          throw error
         }
-      )
+        DocUpdaterClient.sendUpdate(
+          this.project_id,
+          this.doc.id,
+          this.update,
+          error => {
+            if (error != null) {
+              throw error
+            }
+            setTimeout(() => {
+              DocUpdaterClient.getDoc(
+                this.project_id,
+                this.doc.id,
+                (error, res, data) => {
+                  if (error != null) {
+                    throw error
+                  }
+                  const { ranges } = data
+                  const change = ranges.comments[0]
+                  change.op.should.deep.equal({ c: 'bar', p: 4, t: this.tid })
+                  change.id.should.equal(this.tid)
+                  done()
+                }
+              )
+            }, 200)
+          }
+        )
+      })
     })
 
-    return it('should remove the comment range', function (done) {
-      return DocUpdaterClient.removeComment(
+    it('should remove the comment range', function (done) {
+      DocUpdaterClient.removeComment(
         this.project_id,
         this.doc.id,
         this.tid,
@@ -530,7 +484,7 @@ describe('Ranges', function () {
             throw error
           }
           expect(res.statusCode).to.equal(204)
-          return DocUpdaterClient.getDoc(
+          DocUpdaterClient.getDoc(
             this.project_id,
             this.doc.id,
             (error, res, data) => {
@@ -538,7 +492,7 @@ describe('Ranges', function () {
                 throw error
               }
               expect(data.ranges.comments).to.be.undefined
-              return done()
+              done()
             }
           )
         }
@@ -569,37 +523,31 @@ describe('Ranges', function () {
         version: 0,
       })
       const jobs = []
-      for (const update of Array.from(this.updates)) {
-        ;(update => {
-          return jobs.push(callback =>
-            DocUpdaterClient.sendUpdate(
-              this.project_id,
-              this.doc.id,
-              update,
-              callback
-            )
+      for (const update of this.updates) {
+        jobs.push(callback =>
+          DocUpdaterClient.sendUpdate(
+            this.project_id,
+            this.doc.id,
+            update,
+            callback
           )
-        })(update)
+        )
       }
-      return DocUpdaterClient.preloadDoc(
-        this.project_id,
-        this.doc.id,
-        error => {
+      DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => {
+        if (error != null) {
+          throw error
+        }
+        async.series(jobs, error => {
           if (error != null) {
             throw error
           }
-          return async.series(jobs, error => {
-            if (error != null) {
-              throw error
-            }
-            return setTimeout(done, 200)
-          })
-        }
-      )
+          setTimeout(done, 200)
+        })
+      })
     })
 
-    return it('should not update the ranges', function (done) {
-      return DocUpdaterClient.getDoc(
+    it('should not update the ranges', function (done) {
+      DocUpdaterClient.getDoc(
         this.project_id,
         this.doc.id,
         (error, res, data) => {
@@ -608,13 +556,13 @@ describe('Ranges', function () {
           }
           const { ranges } = data
           expect(ranges.changes).to.be.undefined
-          return done()
+          done()
         }
       )
     })
   })
 
-  return describe('deleting text surrounding a comment', function () {
+  describe('deleting text surrounding a comment', function () {
     before(function (done) {
       this.project_id = DocUpdaterClient.randomId()
       this.user_id = DocUpdaterClient.randomId()
@@ -653,48 +601,42 @@ describe('Ranges', function () {
         },
       ]
       const jobs = []
-      for (const update of Array.from(this.updates)) {
-        ;(update => {
-          return jobs.push(callback =>
-            DocUpdaterClient.sendUpdate(
-              this.project_id,
-              this.doc_id,
-              update,
-              callback
-            )
+      for (const update of this.updates) {
+        jobs.push(callback =>
+          DocUpdaterClient.sendUpdate(
+            this.project_id,
+            this.doc_id,
+            update,
+            callback
           )
-        })(update)
+        )
       }
-      return DocUpdaterClient.preloadDoc(
-        this.project_id,
-        this.doc_id,
-        error => {
+      DocUpdaterClient.preloadDoc(this.project_id, this.doc_id, error => {
+        if (error != null) {
+          throw error
+        }
+        async.series(jobs, function (error) {
           if (error != null) {
             throw error
           }
-          return async.series(jobs, function (error) {
-            if (error != null) {
-              throw error
-            }
-            return setTimeout(() => {
-              return DocUpdaterClient.getDoc(
-                this.project_id,
-                this.doc_id,
-                (error, res, data) => {
-                  if (error != null) {
-                    throw error
-                  }
-                  return done()
+          setTimeout(() => {
+            DocUpdaterClient.getDoc(
+              this.project_id,
+              this.doc_id,
+              (error, res, data) => {
+                if (error != null) {
+                  throw error
                 }
-              )
-            }, 200)
-          })
-        }
-      )
+                done()
+              }
+            )
+          }, 200)
+        })
+      })
     })
 
-    return it('should write a snapshot from before the destructive change', function (done) {
-      return DocUpdaterClient.getDoc(
+    it('should write a snapshot from before the destructive change', function (done) {
+      DocUpdaterClient.getDoc(
         this.project_id,
         this.doc_id,
         (error, res, data) => {
@@ -718,7 +660,7 @@ describe('Ranges', function () {
                 p: 1,
                 tid: this.tid,
               })
-              return done()
+              done()
             })
         }
       )

services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js

@@ -3,6 +3,7 @@ const { expect } = require('chai')
 const SandboxedModule = require('sandboxed-module')
 
 const MODULE_PATH = '../../../../app/js/RangesManager.js'
+const TEST_USER_ID = 'user-id-123'
 
 describe('RangesManager', function () {
   beforeEach(function () {
@@ -14,57 +15,29 @@ describe('RangesManager', function () {
 
     this.doc_id = 'doc-id-123'
     this.project_id = 'project-id-123'
-    this.user_id = 'user-id-123'
+    this.user_id = TEST_USER_ID
   })
 
   describe('applyUpdate', function () {
     beforeEach(function () {
-      this.updates = [
-        {
-          meta: {
-            user_id: this.user_id,
-          },
-          op: [
-            {
-              i: 'two ',
-              p: 4,
-            },
-          ],
-        },
-      ]
-      this.entries = {
-        comments: [
-          {
-            op: {
-              c: 'three ',
-              p: 4,
-            },
-            metadata: {
-              user_id: this.user_id,
-            },
-          },
-        ],
-        changes: [
-          {
-            op: {
-              i: 'five',
-              p: 15,
-            },
-            metadata: {
-              user_id: this.user_id,
-            },
-          },
-        ],
+      this.ops = [{ i: 'two ', p: 4 }]
+      this.historyOps = [{ i: 'two ', p: 4, hpos: 4 }]
+      this.meta = { user_id: this.user_id }
+      this.updates = [{ meta: this.meta, op: this.ops }]
+      this.ranges = {
+        comments: makeRanges([{ c: 'three ', p: 4 }]),
+        changes: makeRanges([{ i: 'five', p: 15 }]),
       }
       this.newDocLines = ['one two three four five']
-    }) // old is "one three four five"
+      // old is "one three four five"
+    })
 
     describe('successfully', function () {
       beforeEach(function () {
         this.result = this.RangesManager.applyUpdate(
           this.project_id,
           this.doc_id,
-          this.entries,
+          this.ranges,
           this.updates,
           this.newDocLines
         )
@@ -81,15 +54,19 @@ describe('RangesManager', function () {
           p: 19,
         })
       })
+
+      it('should return unmodified updates for the history', function () {
+        expect(this.result.historyUpdates).to.deep.equal(this.updates)
+      })
     })
 
     describe('with empty comments', function () {
       beforeEach(function () {
-        this.entries.comments = []
+        this.ranges.comments = []
         this.result = this.RangesManager.applyUpdate(
           this.project_id,
           this.doc_id,
-          this.entries,
+          this.ranges,
           this.updates,
           this.newDocLines
         )
@@ -99,15 +76,19 @@ describe('RangesManager', function () {
         // Save space in redis and don't store just {}
         expect(this.result.newRanges.comments).to.be.undefined
       })
+
+      it('should return unmodified updates for the history', function () {
+        expect(this.result.historyUpdates).to.deep.equal(this.updates)
+      })
     })
 
     describe('with empty changes', function () {
       beforeEach(function () {
-        this.entries.changes = []
+        this.ranges.changes = []
         this.result = this.RangesManager.applyUpdate(
           this.project_id,
           this.doc_id,
-          this.entries,
+          this.ranges,
           this.updates,
           this.newDocLines
         )
@@ -117,48 +98,21 @@ describe('RangesManager', function () {
         // Save space in redis and don't store just {}
         expect(this.result.newRanges.changes).to.be.undefined
       })
+
+      it('should return unmodified updates for the history', function () {
+        expect(this.result.historyUpdates).to.deep.equal(this.updates)
+      })
     })
 
     describe('with too many comments', function () {
       beforeEach(function () {
         this.RangesManager.MAX_COMMENTS = 2
-        this.updates = [
-          {
-            meta: {
-              user_id: this.user_id,
-            },
-            op: [
-              {
-                c: 'one',
-                p: 0,
-                t: 'thread-id-1',
-              },
-            ],
-          },
-        ]
-        this.entries = {
-          comments: [
-            {
-              op: {
-                c: 'three ',
-                p: 4,
-                t: 'thread-id-2',
-              },
-              metadata: {
-                user_id: this.user_id,
-              },
-            },
-            {
-              op: {
-                c: 'four ',
-                p: 10,
-                t: 'thread-id-3',
-              },
-              metadata: {
-                user_id: this.user_id,
-              },
-            },
-          ],
+        this.updates = makeUpdates([{ c: 'one', p: 0, t: 'thread-id-1' }])
+        this.ranges = {
+          comments: makeRanges([
+            { c: 'three ', p: 4, t: 'thread-id-2' },
+            { c: 'four ', p: 10, t: 'thread-id-3' },
+          ]),
           changes: [],
         }
       })
@@ -168,7 +122,7 @@ describe('RangesManager', function () {
           this.RangesManager.applyUpdate(
             this.project_id,
             this.doc_id,
-            this.entries,
+            this.ranges,
             this.updates,
             this.newDocLines
           )
@@ -179,41 +133,20 @@ describe('RangesManager', function () {
     describe('with too many changes', function () {
       beforeEach(function () {
         this.RangesManager.MAX_CHANGES = 2
-        this.updates = [
-          {
-            meta: {
-              user_id: this.user_id,
-              tc: 'track-changes-id-yes',
-            },
-            op: [
-              {
-                i: 'one ',
-                p: 0,
-              },
-            ],
-          },
-        ]
-        this.entries = {
-          changes: [
+        this.updates = makeUpdates([{ i: 'one ', p: 0 }], {
+          tc: 'track-changes-id-yes',
+        })
+        this.ranges = {
+          changes: makeRanges([
             {
-              op: {
-                i: 'three',
-                p: 4,
-              },
-              metadata: {
-                user_id: this.user_id,
-              },
+              i: 'three',
+              p: 4,
             },
             {
-              op: {
-                i: 'four',
-                p: 10,
-              },
-              metadata: {
-                user_id: this.user_id,
-              },
+              i: 'four',
+              p: 10,
             },
-          ],
+          ]),
           comments: [],
         }
         this.newDocLines = ['one two three four']
@@ -224,7 +157,7 @@ describe('RangesManager', function () {
           this.RangesManager.applyUpdate(
             this.project_id,
             this.doc_id,
-            this.entries,
+            this.ranges,
             this.updates,
             this.newDocLines
           )
@@ -234,19 +167,7 @@ describe('RangesManager', function () {
 
     describe('inconsistent changes', function () {
       beforeEach(function () {
-        this.updates = [
-          {
-            meta: {
-              user_id: this.user_id,
-            },
-            op: [
-              {
-                c: "doesn't match",
-                p: 0,
-              },
-            ],
-          },
-        ]
+        this.updates = makeUpdates([{ c: "doesn't match", p: 0 }])
       })
 
       it('should throw an error', function () {
@@ -254,51 +175,33 @@ describe('RangesManager', function () {
           this.RangesManager.applyUpdate(
             this.project_id,
             this.doc_id,
-            this.entries,
+            this.ranges,
             this.updates,
             this.newDocLines
           )
         }).to.throw(
-          'Change ({"op":{"i":"five","p":15},"metadata":{"user_id":"user-id-123"}}) doesn\'t match text ("our ")'
+          'Change ({"id":"1","op":{"i":"five","p":15},"metadata":{"user_id":"user-id-123"}}) doesn\'t match text ("our ")'
         )
       })
     })
 
     describe('with an update that collapses a range', function () {
       beforeEach(function () {
-        this.updates = [
-          {
-            meta: {
-              user_id: this.user_id,
-            },
-            op: [
-              {
-                d: 'one',
-                p: 0,
-                t: 'thread-id-1',
-              },
-            ],
-          },
-        ]
-        this.entries = {
-          comments: [
+        this.updates = makeUpdates([{ d: 'one', p: 0, t: 'thread-id-1' }])
+        this.ranges = {
+          comments: makeRanges([
             {
-              op: {
-                c: 'n',
-                p: 1,
-                t: 'thread-id-2',
-              },
-              metadata: {
-                user_id: this.user_id,
-              },
+              c: 'n',
+              p: 1,
+              t: 'thread-id-2',
             },
-          ],
+          ]),
           changes: [],
         }
         this.result = this.RangesManager.applyUpdate(
           this.project_id,
           this.doc_id,
-          this.entries,
+          this.ranges,
           this.updates,
           this.newDocLines
         )
@@ -311,49 +214,15 @@ describe('RangesManager', function () {
 
     describe('with an update that deletes ranges', function () {
       beforeEach(function () {
-        this.updates = [
-          {
-            meta: {
-              user_id: this.user_id,
-            },
-            op: [
-              {
-                d: 'one two three four five',
-                p: 0,
-              },
-            ],
-          },
-        ]
-        this.entries = {
-          comments: [
-            {
-              op: {
-                c: 'n',
-                p: 1,
-                t: 'thread-id-2',
-              },
-              metadata: {
-                user_id: this.user_id,
-              },
-            },
-          ],
-          changes: [
-            {
-              op: {
-                i: 'hello',
-                p: 1,
-                t: 'thread-id-2',
-              },
-              metadata: {
-                user_id: this.user_id,
-              },
-            },
-          ],
+        this.updates = makeUpdates([{ d: 'one two three four five', p: 0 }])
+        this.ranges = {
+          comments: makeRanges([{ c: 'n', p: 1, t: 'thread-id-2' }]),
+          changes: makeRanges([{ i: 'hello', p: 1, t: 'thread-id-2' }]),
         }
         this.result = this.RangesManager.applyUpdate(
           this.project_id,
           this.doc_id,
-          this.entries,
+          this.ranges,
           this.updates,
           this.newDocLines
         )
@@ -367,6 +236,156 @@ describe('RangesManager', function () {
         expect(this.result.rangesWereCollapsed).to.equal(true)
       })
     })
+
+    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
+        )
+      })
+
+      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 }],
+        ])
+      })
+    })
+
+    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
+        )
+      })
+
+      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('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 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 },
+              ],
+            },
+          ],
+
+          // the "three" delete is offset to the right by the two first tracked
+          // deletes
+          [{ d: 'three ', p: 4, hpos: 7 }],
+        ])
+      })
+    })
+
+    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
+        )
+      })
+
+      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('acceptChanges', function () {
@@ -381,43 +400,13 @@ describe('RangesManager', function () {
 
       this.ranges = {
         comments: [],
-        changes: [
-          {
-            id: 'a1',
-            op: {
-              i: 'lorem',
-              p: 0,
-            },
-          },
-          {
-            id: 'a2',
-            op: {
-              i: 'ipsum',
-              p: 10,
-            },
-          },
-          {
-            id: 'a3',
-            op: {
-              i: 'dolor',
-              p: 20,
-            },
-          },
-          {
-            id: 'a4',
-            op: {
-              i: 'sit',
-              p: 30,
-            },
-          },
-          {
-            id: 'a5',
-            op: {
-              i: 'amet',
-              p: 40,
-            },
-          },
-        ],
+        changes: makeRanges([
+          { i: 'lorem', p: 0 },
+          { i: 'ipsum', p: 10 },
+          { i: 'dolor', p: 20 },
+          { i: 'sit', p: 30 },
+          { i: 'amet', p: 40 },
+        ]),
       }
       this.removeChangeIdsSpy = sinon.spy(
         this.RangesTracker.prototype,
@@ -516,3 +505,28 @@ describe('RangesManager', function () {
     })
   })
 })
+
+function makeRanges(ops) {
+  let id = 1
+  const changes = []
+  for (const op of ops) {
+    changes.push({
+      id: id.toString(),
+      op,
+      metadata: { user_id: TEST_USER_ID },
+    })
+    id += 1
+  }
+  return changes
+}
+
+function makeUpdates(ops, meta = {}) {
+  const updates = []
+  for (const op of ops) {
+    updates.push({
+      meta: { user_id: TEST_USER_ID, ...meta },
+      op: [op],
+    })
+  }
+  return updates
+}

services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js

@@ -1,18 +1,18 @@
 const sinon = require('sinon')
-const modulePath = '../../../../app/js/RedisManager.js'
 const SandboxedModule = require('sandboxed-module')
 const Errors = require('../../../../app/js/Errors')
 const crypto = require('crypto')
 const tk = require('timekeeper')
 
+const MODULE_PATH = '../../../../app/js/RedisManager.js'
+
 describe('RedisManager', function () {
   beforeEach(function () {
     this.multi = { exec: sinon.stub().yields() }
     this.rclient = { multi: () => this.multi, srem: sinon.stub().yields() }
     tk.freeze(new Date())
-    this.RedisManager = SandboxedModule.require(modulePath, {
+    this.RedisManager = SandboxedModule.require(MODULE_PATH, {
       requires: {
-        './ProjectHistoryRedisManager': (this.ProjectHistoryRedisManager = {}),
         '@overleaf/settings': (this.settings = {
           documentupdater: { logHashErrors: { write: true, read: true } },
           redis: {
@@ -468,13 +468,6 @@ describe('RedisManager', function () {
           null,
           null,
         ])
-      this.ProjectHistoryRedisManager.queueOps = sinon
-        .stub()
-        .callsArgWith(
-          this.ops.length + 1,
-          null,
-          this.project_update_list_length
-        )
     })
 
     describe('with a consistent version', function () {
@@ -545,16 +538,8 @@ describe('RedisManager', function () {
           .should.equal(true)
       })
 
-      it('should push the updates into the project history ops list', function () {
-        this.ProjectHistoryRedisManager.queueOps
-          .calledWith(this.project_id, JSON.stringify(this.ops[0]))
-          .should.equal(true)
-      })
-
       it('should call the callback', function () {
-        this.callback
-          .calledWith(null, this.project_update_list_length)
-          .should.equal(true)
+        this.callback.should.have.been.called
       })
 
       it('should not log any errors', function () {
@@ -578,16 +563,8 @@ describe('RedisManager', function () {
           )
         })
 
-        it('should push the updates into the project history ops list', function () {
-          this.ProjectHistoryRedisManager.queueOps
-            .calledWith(this.project_id, JSON.stringify(this.ops[0]))
-            .should.equal(true)
-        })
-
-        it('should call the callback with the project update count only', function () {
-          this.callback
-            .calledWith(null, this.project_update_list_length)
-            .should.equal(true)
+        it('should call the callback', function () {
+          this.callback.should.have.been.called
         })
       })
     })
@@ -647,10 +624,6 @@ describe('RedisManager', function () {
         this.multi.rpush.called.should.equal(false)
       })
 
-      it('should not try to enqueue project updates', function () {
-        this.ProjectHistoryRedisManager.queueOps.called.should.equal(false)
-      })
-
       it('should still set the doclines', function () {
         this.multi.mset
           .calledWith({
@@ -1041,9 +1014,6 @@ describe('RedisManager', function () {
         this.RedisManager.getDoc = sinon
           .stub()
           .callsArgWith(2, null, 'lines', 'version')
-        this.ProjectHistoryRedisManager.queueRenameEntity = sinon
-          .stub()
-          .yields()
         this.RedisManager.renameDoc(
           this.project_id,
           this.docId,
@@ -1066,9 +1036,6 @@ describe('RedisManager', function () {
         this.RedisManager.getDoc = sinon
           .stub()
           .callsArgWith(2, null, null, null)
-        this.ProjectHistoryRedisManager.queueRenameEntity = sinon
-          .stub()
-          .yields()
         this.RedisManager.renameDoc(
           this.project_id,
           this.docId,

services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js

@@ -74,6 +74,14 @@ describe('UpdateManager', function () {
       },
     }
 
+    this.ProjectHistoryRedisManager = {
+      promises: {
+        queueOps: sinon
+          .stub()
+          .callsFake(async (projectId, ...ops) => ops.length),
+      },
+    }
+
     this.UpdateManager = SandboxedModule.require(MODULE_PATH, {
       requires: {
         './LockManager': this.LockManager,
@@ -87,6 +95,7 @@ describe('UpdateManager', function () {
         './RangesManager': this.RangesManager,
         './SnapshotManager': this.SnapshotManager,
         './Profiler': this.Profiler,
+        './ProjectHistoryRedisManager': this.ProjectHistoryRedisManager,
       },
     })
   })
@@ -309,6 +318,11 @@ describe('UpdateManager', function () {
         { v: 42, op: 'mock-op-42' },
         { v: 45, op: 'mock-op-45' },
       ]
+      this.historyUpdates = [
+        'history-update-1',
+        'history-update-2',
+        'history-update-3',
+      ]
       this.project_ops_length = 123
       this.pathname = '/a/b/c.tex'
       this.DocumentManager.promises.getDoc.resolves({
@@ -317,19 +331,19 @@ describe('UpdateManager', function () {
         ranges: this.ranges,
         pathname: this.pathname,
         projectHistoryId: this.projectHistoryId,
+        historyRangesSupport: false,
       })
       this.RangesManager.applyUpdate.returns({
         newRanges: this.updated_ranges,
         rangesWereCollapsed: false,
+        historyUpdates: this.historyUpdates,
       })
       this.ShareJsUpdateManager.promises.applyUpdate = sinon.stub().resolves({
         updatedDocLines: this.updatedDocLines,
         version: this.version,
         appliedOps: this.appliedOps,
       })
-      this.RedisManager.promises.updateDocument.resolves(
-        this.project_ops_length
-      )
+      this.RedisManager.promises.updateDocument.resolves()
       this.UpdateManager.promises._addProjectHistoryMetadataToOps = sinon.stub()
     })
 
@@ -390,9 +404,15 @@ describe('UpdateManager', function () {
       })
 
       it('should push the applied ops into the history queue', function () {
-        this.HistoryManager.recordAndFlushHistoryOps
-          .calledWith(this.project_id, this.appliedOps, this.project_ops_length)
-          .should.equal(true)
+        this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWith(
+          this.project_id,
+          ...this.appliedOps.map(op => JSON.stringify(op))
+        )
+        this.HistoryManager.recordAndFlushHistoryOps.should.have.been.calledWith(
+          this.project_id,
+          this.appliedOps,
+          this.appliedOps.length
+        )
       })
     })
 
@@ -445,6 +465,7 @@ describe('UpdateManager', function () {
         this.RangesManager.applyUpdate.returns({
           newRanges: this.updated_ranges,
           rangesWereCollapsed: true,
+          historyUpdates: this.historyUpdates,
         })
         await this.UpdateManager.promises.applyUpdate(
           this.project_id,
@@ -470,6 +491,36 @@ describe('UpdateManager', function () {
           .should.equal(true)
       })
     })
+
+    describe('when history ranges are supported', function () {
+      beforeEach(async function () {
+        this.DocumentManager.promises.getDoc.resolves({
+          lines: this.lines,
+          version: this.version,
+          ranges: this.ranges,
+          pathname: this.pathname,
+          projectHistoryId: this.projectHistoryId,
+          historyRangesSupport: true,
+        })
+        await this.UpdateManager.promises.applyUpdate(
+          this.project_id,
+          this.doc_id,
+          this.update
+        )
+      })
+
+      it('should push the history updates into the history queue', function () {
+        this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWith(
+          this.project_id,
+          ...this.historyUpdates.map(op => JSON.stringify(op))
+        )
+        this.HistoryManager.recordAndFlushHistoryOps.should.have.been.calledWith(
+          this.project_id,
+          this.historyUpdates,
+          this.historyUpdates.length
+        )
+      })
+    })
   })
 
   describe('_addProjectHistoryMetadataToOps', function () {