Merge pull request #18342 from overleaf/em-tracking-props-none

Separate handling of "none" tracking type in operations GitOrigin-RevId: b07ee5320ac1a9c63e3b0059aade1a1564819943
2024-09-16 02:52:31 -04:00 · 2024-05-16 08:23:57 -04:00 · 2024-05-16 08:23:57 -04:00 · 5d0190da0f
commit 5d0190da0f
parent f5aa0a7a93
17 changed files with 139 additions and 114 deletions
--- a/libraries/overleaf-editor-core/lib/file_data/clear_tracking_props.js
+++ b/libraries/overleaf-editor-core/lib/file_data/clear_tracking_props.js
@ -0,0 +1,28 @@
+// @ts-check
+
+/**
+ * @typedef {import('../types').ClearTrackingPropsRawData} ClearTrackingPropsRawData
+ */
+
+class ClearTrackingProps {
+  constructor() {
+    this.type = 'none'
+  }
+
+  /**
+   * @param {any} other
+   * @returns {boolean}
+   */
+  equals(other) {
+    return other instanceof ClearTrackingProps
+  }
+
+  /**
+   * @returns {ClearTrackingPropsRawData}
+   */
+  toRaw() {
+    return { type: 'none' }
+  }
+}
+
+module.exports = ClearTrackingProps
--- a/libraries/overleaf-editor-core/lib/file_data/tracked_change_list.js
+++ b/libraries/overleaf-editor-core/lib/file_data/tracked_change_list.js
@ -1,10 +1,11 @@
 // @ts-check
 const Range = require('../range')
 const TrackedChange = require('./tracked_change')
+const TrackingProps = require('../file_data/tracking_props')

 /**
+ * @typedef {import("../types").TrackingDirective} TrackingDirective
 * @typedef {import("../types").TrackedChangeRawData} TrackedChangeRawData
- * @typedef {import("../file_data/tracking_props")} TrackingProps
 */

 class TrackedChangeList {
@ -211,7 +212,7 @@ class TrackedChangeList {
  /**
   * @param {number} cursor
   * @param {number} length
-   * @param {{tracking?: TrackingProps}} opts
+   * @param {{tracking?: TrackingDirective}} opts
   */
  applyRetain(cursor, length, opts = {}) {
    // If there's no tracking info, leave everything as-is
@ -263,7 +264,7 @@ class TrackedChangeList {
        newTrackedChanges.push(trackedChange)
      }
    }
-    if (opts.tracking?.type === 'delete' || opts.tracking?.type === 'insert') {
+    if (opts.tracking instanceof TrackingProps) {
      // This is a new tracked change
      const newTrackedChange = new TrackedChange(retainedRange, opts.tracking)
      newTrackedChanges.push(newTrackedChange)
--- a/libraries/overleaf-editor-core/lib/file_data/tracking_props.js
+++ b/libraries/overleaf-editor-core/lib/file_data/tracking_props.js
@ -6,14 +6,14 @@
 class TrackingProps {
  /**
   *
-   * @param {'insert' | 'delete' | 'none'} type
+   * @param {'insert' | 'delete'} type
   * @param {string} userId
   * @param {Date} ts
   */
  constructor(type, userId, ts) {
    /**
     * @readonly
-     * @type {'insert' | 'delete' | 'none'}
+     * @type {'insert' | 'delete'}
     */
    this.type = type
    /**
--- a/libraries/overleaf-editor-core/lib/operation/scan_op.js
+++ b/libraries/overleaf-editor-core/lib/operation/scan_op.js
@ -5,6 +5,7 @@ const {
  InvalidInsertionError,
  UnprocessableError,
 } = require('../errors')
+const ClearTrackingProps = require('../file_data/clear_tracking_props')
 const TrackingProps = require('../file_data/tracking_props')

 /**
@ -13,6 +14,7 @@ const TrackingProps = require('../file_data/tracking_props')
 * @typedef {import('../types').RawInsertOp} RawInsertOp
 * @typedef {import('../types').RawRetainOp} RawRetainOp
 * @typedef {import('../types').RawRemoveOp} RawRemoveOp
+ * @typedef {import('../types').TrackingDirective} TrackingDirective
 */

 class ScanOp {
@ -218,7 +220,7 @@ class InsertOp extends ScanOp {
 class RetainOp extends ScanOp {
  /**
   * @param {number} length
-   * @param {TrackingProps | undefined} tracking
+   * @param {TrackingDirective | undefined} tracking
   */
  constructor(length, tracking = undefined) {
    super()
@ -227,7 +229,7 @@ class RetainOp extends ScanOp {
    }
    /** @type {number} */
    this.length = length
-    /** @type {TrackingProps | undefined} */
+    /** @type {TrackingDirective | undefined} */
    this.tracking = tracking
  }

@ -263,7 +265,11 @@ class RetainOp extends ScanOp {
      throw new Error('retain operation must have a number property')
    }
    if (op.tracking) {
-      return new RetainOp(op.r, TrackingProps.fromRaw(op.tracking))
+      const tracking =
+        op.tracking.type === 'none'
+          ? new ClearTrackingProps()
+          : TrackingProps.fromRaw(op.tracking)
+      return new RetainOp(op.r, tracking)
    }
    return new RetainOp(op.r)
  }
--- a/libraries/overleaf-editor-core/lib/operation/text_operation.js
+++ b/libraries/overleaf-editor-core/lib/operation/text_operation.js
@ -26,12 +26,15 @@ const {
  TooLongError,
 } = require('../errors')
 const Range = require('../range')
+const ClearTrackingProps = require('../file_data/clear_tracking_props')
 const TrackingProps = require('../file_data/tracking_props')
+
 /**
 * @typedef {import('../file_data/string_file_data')} StringFileData
 * @typedef {import('../types').RawTextOperation} RawTextOperation
 * @typedef {import('../operation/scan_op').ScanOp} ScanOp
 * @typedef {import('../file_data/tracked_change_list')} TrackedChangeList
+ * @typedef {import('../types').TrackingDirective} TrackingDirective
 */

 /**
@ -91,7 +94,7 @@ class TextOperation extends EditOperation {
  /**
   * Skip over a given number of characters.
   * @param {number | {r: number}} n
-   * @param {{tracking?: TrackingProps}} opts
+   * @param {{tracking?: TrackingDirective}} opts
   * @returns {TextOperation}
   */
  retain(n, opts = {}) {
@ -376,11 +379,7 @@ class TextOperation extends EditOperation {

        let removeTrackingInfoIfNeeded
        if (op.tracking) {
-          removeTrackingInfoIfNeeded = new TrackingProps(
-            'none',
-            op.tracking.userId,
-            op.tracking.ts
-          )
+          removeTrackingInfoIfNeeded = new ClearTrackingProps()
        }

        for (const trackedChange of previousRanges) {
@ -575,10 +574,15 @@ class TextOperation extends EditOperation {
        }
      } else if (op1 instanceof InsertOp && op2 instanceof RetainOp) {
        const opts = {
-          // Prefer the latter tracking info
-          tracking: op2.tracking ?? op1.tracking,
          commentIds: op1.commentIds,
        }
+        if (op2.tracking instanceof TrackingProps) {
+          // Prefer the tracking info on the second operation
+          opts.tracking = op2.tracking
+        } else if (!(op2.tracking instanceof ClearTrackingProps)) {
+          // The second operation does not cancel the first operation's tracking
+          opts.tracking = op1.tracking
+        }
        if (op1.insertion.length > op2.length) {
          operation.insert(op1.insertion.slice(0, op2.length), opts)
          op1 = new InsertOp(
@ -692,9 +696,9 @@ class TextOperation extends EditOperation {
        // Simple case: retain/retain

        // If both have tracking info, we use the one from op1
-        /** @type {TrackingProps | undefined} */
+        /** @type {TrackingProps | ClearTrackingProps | undefined} */
        let operation1primeTracking
-        /** @type {TrackingProps | undefined} */
+        /** @type {TrackingProps | ClearTrackingProps | undefined} */
        let operation2primeTracking
        if (op1.tracking) {
          operation1primeTracking = op1.tracking
--- a/libraries/overleaf-editor-core/lib/types.ts
+++ b/libraries/overleaf-editor-core/lib/types.ts
@ -1,4 +1,6 @@
 import Blob from './blob'
+import TrackingProps from './file_data/tracking_props'
+import ClearTrackingProps from './file_data/clear_tracking_props'

 export type BlobStore = {
  getBlob(hash: string): Promise<Blob | null>
@ -32,11 +34,17 @@ export type TrackedChangeRawData = {
 }

 export type TrackingPropsRawData = {
-  type: 'insert' | 'delete' | 'none'
+  type: 'insert' | 'delete'
  userId: string
  ts: string
 }

+export type ClearTrackingPropsRawData = {
+  type: 'none'
+}
+
+export type TrackingDirective = TrackingProps | ClearTrackingProps
+
 export type StringFileRawData = {
  content: string
  comments?: CommentRawData[]
@ -58,7 +66,7 @@ export type RawRetainOp =
  | {
      r: number
      commentIds?: string[]
-      tracking?: TrackingPropsRawData
+      tracking?: TrackingPropsRawData | ClearTrackingPropsRawData
    }
  | number

--- a/libraries/overleaf-editor-core/test/support/random_text_operation.js
+++ b/libraries/overleaf-editor-core/test/support/random_text_operation.js
@ -1,4 +1,5 @@
 const TrackingProps = require('../../lib/file_data/tracking_props')
+const ClearTrackingProps = require('../../lib/file_data/clear_tracking_props')
 const TextOperation = require('../../lib/operation/text_operation')
 const random = require('./random')

@ -19,7 +20,7 @@ function randomTextOperation(str, commentIds) {
    const trackedChange =
      Math.random() < 0.1
        ? new TrackingProps(
-            random.element(['insert', 'delete', 'none']),
+            random.element(['insert', 'delete']),
            random.element(['user1', 'user2', 'user3']),
            new Date(
              random.element([
@ -41,6 +42,8 @@ function randomTextOperation(str, commentIds) {
      })
    } else if (r < 0.4) {
      operation.remove(l)
+    } else if (r < 0.5) {
+      operation.retain(l, { tracking: new ClearTrackingProps() })
    } else {
      operation.retain(l, { tracking: trackedChange })
    }
--- a/libraries/overleaf-editor-core/test/text_operation.test.js
+++ b/libraries/overleaf-editor-core/test/text_operation.test.js
@ -15,6 +15,7 @@ const TextOperation = ot.TextOperation
 const StringFileData = require('../lib/file_data/string_file_data')
 const { RetainOp, InsertOp, RemoveOp } = require('../lib/operation/scan_op')
 const TrackingProps = require('../lib/file_data/tracking_props')
+const ClearTrackingProps = require('../lib/file_data/clear_tracking_props')

 describe('TextOperation', function () {
  const numTrials = 500
@ -496,11 +497,7 @@ describe('TextOperation', function () {
          new TextOperation()
            .retain(4)
            .retain(4, {
-              tracking: TrackingProps.fromRaw({
-                ts: '2024-01-01T00:00:00.000Z',
-                type: 'none',
-                userId: 'user2',
-              }),
+              tracking: new ClearTrackingProps(),
            })
            .retain(3)
        )
@ -508,6 +505,42 @@ describe('TextOperation', function () {
        content: 'foo bar baz',
      })
    })
+
+    it('it removes the tracking from an insert if operation 2 resolves it', function () {
+      expect(
+        compose(
+          new StringFileData('foo bar baz'),
+          new TextOperation()
+            .retain(4)
+            .insert('quux ', {
+              tracking: TrackingProps.fromRaw({
+                ts: '2023-01-01T00:00:00.000Z',
+                type: 'insert',
+                userId: 'user1',
+              }),
+            })
+            .retain(7),
+          new TextOperation()
+            .retain(6)
+            .retain(5, {
+              tracking: new ClearTrackingProps(),
+            })
+            .retain(5)
+        )
+      ).to.deep.equal({
+        content: 'foo quux bar baz',
+        trackedChanges: [
+          {
+            range: { pos: 4, length: 2 },
+            tracking: {
+              ts: '2023-01-01T00:00:00.000Z',
+              type: 'insert',
+              userId: 'user1',
+            },
+          },
+        ],
+      })
+    })
  })

  describe('transform', function () {
--- a/libraries/overleaf-editor-core/test/tracked_change_list.test.js
+++ b/libraries/overleaf-editor-core/test/tracked_change_list.test.js
@ -1,6 +1,7 @@
 // @ts-check
 const TrackedChangeList = require('../lib/file_data/tracked_change_list')
 const TrackingProps = require('../lib/file_data/tracking_props')
+const ClearTrackingProps = require('../lib/file_data/clear_tracking_props')
 const { expect } = require('chai')
 /** @typedef {import('../lib/types').TrackedChangeRawData} TrackedChangeRawData */

@ -766,11 +767,7 @@ describe('TrackedChangeList', function () {
        },
      ])
      trackedChanges.applyRetain(0, 10, {
-        tracking: TrackingProps.fromRaw({
-          type: 'none',
-          userId: 'user1',
-          ts: '2024-01-01T00:00:00.000Z',
-        }),
+        tracking: new ClearTrackingProps(),
      })
      expect(trackedChanges.length).to.equal(0)
      expect(trackedChanges.toRaw()).to.deep.equal([])
@ -788,11 +785,7 @@ describe('TrackedChangeList', function () {
        },
      ])
      trackedChanges.applyRetain(0, 10, {
-        tracking: TrackingProps.fromRaw({
-          type: 'none',
-          userId: 'user2',
-          ts: '2024-01-01T00:00:00.000Z',
-        }),
+        tracking: new ClearTrackingProps(),
      })
      expect(trackedChanges.length).to.equal(0)
      expect(trackedChanges.toRaw()).to.deep.equal([])
@ -810,11 +803,7 @@ describe('TrackedChangeList', function () {
        },
      ])
      trackedChanges.applyRetain(0, 10, {
-        tracking: TrackingProps.fromRaw({
-          type: 'none',
-          userId: 'user1',
-          ts: '2024-01-01T00:00:00.000Z',
-        }),
+        tracking: new ClearTrackingProps(),
      })
      expect(trackedChanges.length).to.equal(0)
      expect(trackedChanges.toRaw()).to.deep.equal([])
@ -832,11 +821,7 @@ describe('TrackedChangeList', function () {
        },
      ])
      trackedChanges.applyRetain(0, 10, {
-        tracking: TrackingProps.fromRaw({
-          type: 'none',
-          userId: 'user2',
-          ts: '2024-01-01T00:00:00.000Z',
-        }),
+        tracking: new ClearTrackingProps(),
      })
      expect(trackedChanges.length).to.equal(0)
      expect(trackedChanges.toRaw()).to.deep.equal([])
--- a/services/document-updater/app/js/RangesManager.js
+++ b/services/document-updater/app/js/RangesManager.js
@ -208,11 +208,7 @@ const RangesManager = {
          op = {
            p: change.op.p,
            r: change.op.i,
-            tracking: {
-              type: 'none',
-              userId: change.metadata.user_id,
-              ts: change.metadata.ts,
-            },
+            tracking: { type: 'none' },
          }
          if (unacceptedDeletes > 0) {
            op.hpos = op.p + unacceptedDeletes
--- a/services/document-updater/app/js/types.ts
+++ b/services/document-updater/app/js/types.ts
@ -1,4 +1,7 @@
-import { TrackingPropsRawData } from 'overleaf-editor-core/lib/types'
+import {
+  TrackingPropsRawData,
+  ClearTrackingPropsRawData,
+} from 'overleaf-editor-core/lib/types'

 /**
 * An update coming from the editor
@ -97,7 +100,7 @@ export type HistoryInsertOp = InsertOp & {

 export type HistoryRetainOp = RetainOp & {
  hpos?: number
-  tracking?: TrackingPropsRawData
+  tracking?: TrackingPropsRawData | ClearTrackingPropsRawData
 }

 export type HistoryDeleteOp = DeleteOp & {
--- a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js
+++ b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js
@ -682,11 +682,7 @@ describe('RangesManager', function () {
            {
              r: 'lorem',
              p: 0,
-              tracking: {
-                type: 'none',
-                userId: this.user_id,
-                ts: ranges.changes[0].metadata.ts,
-              },
+              tracking: { type: 'none' },
            },
          ],
        },
@ -702,11 +698,7 @@ describe('RangesManager', function () {
            {
              r: 'ipsum',
              p: 15,
-              tracking: {
-                type: 'none',
-                userId: this.user_id,
-                ts: ranges.changes[1].metadata.ts,
-              },
+              tracking: { type: 'none' },
            },
          ],
        },
@ -868,11 +860,7 @@ describe('RangesManager', function () {
              r: 'xxx ',
              p: 6,
              hpos: 11,
-              tracking: {
-                type: 'none',
-                userId: this.user_id,
-                ts: ranges.changes[1].metadata.ts,
-              },
+              tracking: { type: 'none' },
            },
          ],
        },
--- a/services/project-history/app/js/UpdateTranslator.js
+++ b/services/project-history/app/js/UpdateTranslator.js
@ -309,11 +309,7 @@ class OperationsBuilder {
    if (isInsert(op)) {
      if (op.trackedDeleteRejection) {
        this.retain(op.i.length, {
-          tracking: {
-            type: 'none',
-            userId: update.meta.user_id,
-            ts: new Date(update.meta.ts).toISOString(),
-          },
+          tracking: { type: 'none' },
        })
      } else {
        const opts = {}
--- a/services/project-history/app/js/types.ts
+++ b/services/project-history/app/js/types.ts
@ -140,11 +140,13 @@ export type RawOrigin = {
  kind: string
 }

-export type TrackingProps = {
-  type: 'insert' | 'delete' | 'none'
-  userId: string
-  ts: string
-}
+export type TrackingProps =
+  | { type: 'none' }
+  | {
+      type: 'insert' | 'delete'
+      userId: string
+      ts: string
+    }

 export type RawScanOp =
  | number
--- a/services/project-history/test/unit/js/ChunkTranslator/ChunkTranslatorTests.js
+++ b/services/project-history/test/unit/js/ChunkTranslator/ChunkTranslatorTests.js
@ -2142,11 +2142,7 @@ describe('ChunkTranslator', function () {
                        4, //  Hell|o [planet ]world, this is a test
                        {
                          r: 4,
-                          tracking: {
-                            type: 'none',
-                            userId: this.author1.id,
-                            ts: '2024-01-01T00:00:00.000Z',
-                          },
+                          tracking: { type: 'none' },
                        }, //  Hello pl|[anet ]world, this is a test
                        {
                          r: 3,
@ -2375,11 +2371,7 @@ describe('ChunkTranslator', function () {
                        2, // Heo |{TEST }planet world, [this] is a test
                        {
                          r: 5,
-                          tracking: {
-                            type: 'none',
-                            userId: '65f1baeb3cee7c3563fbd1d1',
-                            ts: '2024-03-27T13:20:23.253Z',
-                          },
+                          tracking: { type: 'none' },
                        }, // Heo TEST| planet world, [this] is a test
                        14, // Heo TEST planet world, |[this] is a test
                        -4, // Heo TEST planet world, | is a test
@ -2557,11 +2549,7 @@ describe('ChunkTranslator', function () {
                        -2, // He|o planet world, [this] {TEST }is a test
                        {
                          r: 39,
-                          tracking: {
-                            type: 'none',
-                            userId: this.author1.id,
-                            ts: this.date.toISOString(),
-                          },
+                          tracking: { type: 'none' },
                        },
                      ], // He|o planet world, this TEST is a test
                    },
--- a/services/project-history/test/unit/js/UpdateCompressor/UpdateCompressorTests.js
+++ b/services/project-history/test/unit/js/UpdateCompressor/UpdateCompressorTests.js
@ -106,7 +106,7 @@ describe('UpdateCompressor', function () {
              (this.op2 = {
                p: 9,
                r: 'baz',
-                tracking: { type: 'none', ts: this.ts1, userId: this.user_id },
+                tracking: { type: 'none' },
              }),
              (this.op3 = { p: 6, i: 'bar' }),
            ],
--- a/services/project-history/test/unit/js/UpdateTranslator/UpdateTranslatorTests.js
+++ b/services/project-history/test/unit/js/UpdateTranslator/UpdateTranslatorTests.js
@ -625,11 +625,7 @@ describe('UpdateTranslator', function () {
                {
                  p: 3,
                  r: 'lo',
-                  tracking: {
-                    type: 'none',
-                    ts: this.timestamp,
-                    userId: this.user_id,
-                  },
+                  tracking: { type: 'none' },
                },
              ],
              v: this.version,
@ -658,11 +654,7 @@ describe('UpdateTranslator', function () {
                  3,
                  {
                    r: 2,
-                    tracking: {
-                      type: 'none',
-                      ts: this.timestamp,
-                      userId: this.user_id,
-                    },
+                    tracking: { type: 'none' },
                  },
                  15,
                ],
@ -1044,11 +1036,7 @@ describe('UpdateTranslator', function () {
                  3,
                  {
                    r: 3,
-                    tracking: {
-                      type: 'none',
-                      userId: this.user_id,
-                      ts: new Date(this.timestamp).toISOString(),
-                    },
+                    tracking: { type: 'none' },
                  },
                  14,
                ],
@ -1123,11 +1111,7 @@ describe('UpdateTranslator', function () {
                  3,
                  {
                    r: 17,
-                    tracking: {
-                      type: 'none',
-                      userId: this.user_id,
-                      ts: new Date(this.timestamp).toISOString(),
-                    },
+                    tracking: { type: 'none' },
                  },
                  3,
                  -18,