From 702585a897fa959223616471da2f31f0682f1a78 Mon Sep 17 00:00:00 2001
From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com>
Date: Wed, 14 Feb 2024 07:40:51 -0500
Subject: [PATCH] Merge pull request #17037 from overleaf/em-comment-ids

Add commentIds property to insert updates sent to history

GitOrigin-RevId: 1fdfc21e1cceff2693975ed6d037e60557106e67
---
 .../document-updater/app/js/RangesManager.js  | 29 +++++++++++--
 services/document-updater/app/js/types.ts     |  1 +
 .../js/RangesManager/RangesManagerTests.js    | 43 +++++++++++++++++++
 3 files changed, 69 insertions(+), 4 deletions(-)

diff --git a/services/document-updater/app/js/RangesManager.js b/services/document-updater/app/js/RangesManager.js
index b0a399f8b0..53f1c13ddc 100644
--- a/services/document-updater/app/js/RangesManager.js
+++ b/services/document-updater/app/js/RangesManager.js
@@ -8,6 +8,7 @@ const _ = require('lodash')
 const { isInsert, isDelete, isComment } = require('./Utils')
 
 /**
+ * @typedef {import('./types').Comment} Comment
  * @typedef {import('./types').CommentOp} CommentOp
  * @typedef {import('./types').DeleteOp} DeleteOp
  * @typedef {import('./types').HistoryCommentOp} HistoryCommentOp
@@ -57,7 +58,9 @@ const RangesManager = {
       }
       const historyOps = []
       for (const op of update.op) {
-        historyOps.push(getHistoryOp(op, rangesTracker.changes))
+        historyOps.push(
+          getHistoryOp(op, rangesTracker.comments, rangesTracker.changes)
+        )
         rangesTracker.applyOp(op, { user_id: update.meta?.user_id })
       }
       historyUpdates.push({ ...update, op: historyOps })
@@ -172,9 +175,9 @@ const RangesManager = {
  *        RangesTracker is ordered by position.
  * @returns {HistoryOp}
  */
-function getHistoryOp(op, changes, opts = {}) {
+function getHistoryOp(op, comments, changes, opts = {}) {
   if (isInsert(op)) {
-    return getHistoryOpForInsert(op, changes)
+    return getHistoryOpForInsert(op, comments, changes)
   } else if (isDelete(op)) {
     return getHistoryOpForDelete(op, changes)
   } else if (isComment(op)) {
@@ -191,13 +194,28 @@ function getHistoryOp(op, changes, opts = {}) {
  * op. When an insert is made at the same position as a tracked delete, the
  * insert is placed before the tracked delete.
  *
+ * We also add a commentIds property when inserts are made inside a comment.
+ * The current behaviour is to include the insert in the comment only if the
+ * insert is made strictly inside the comment. Inserts made at the edges are
+ * not included in the comment.
+ *
  * @param {InsertOp} op
+ * @param {Comment[]} comments
  * @param {TrackedChange[]} changes
  * @returns {HistoryInsertOp}
  */
-function getHistoryOpForInsert(op, changes) {
+function getHistoryOpForInsert(op, comments, changes) {
   let hpos = op.p
   let trackedDeleteRejection = false
+  const commentIds = new Set()
+
+  for (const comment of comments) {
+    if (comment.op.p < op.p && op.p < comment.op.p + comment.op.c.length) {
+      // Insert is inside the comment; add the comment id
+      commentIds.add(comment.op.t)
+    }
+  }
+
   for (const change of changes) {
     if (!isDelete(change.op)) {
       // We're only interested in tracked deletes
@@ -227,6 +245,9 @@ function getHistoryOpForInsert(op, changes) {
 
   /** @type {HistoryInsertOp} */
   const historyOp = { ...op }
+  if (commentIds.size > 0) {
+    historyOp.commentIds = Array.from(commentIds)
+  }
   if (hpos !== op.p) {
     historyOp.hpos = hpos
   }
diff --git a/services/document-updater/app/js/types.ts b/services/document-updater/app/js/types.ts
index 8739723070..937803eed3 100644
--- a/services/document-updater/app/js/types.ts
+++ b/services/document-updater/app/js/types.ts
@@ -55,6 +55,7 @@ export type TrackedChange = {
 export type HistoryOp = HistoryInsertOp | HistoryDeleteOp | HistoryCommentOp
 
 export type HistoryInsertOp = InsertOp & {
+  commentIds?: string[]
   hpos?: number
   trackedDeleteRejection?: boolean
 }
diff --git a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js
index b835de4783..7eb7203990 100644
--- a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js
+++ b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js
@@ -386,6 +386,49 @@ describe('RangesManager', function () {
         ])
       })
     })
+
+    describe('inserts inside comments', function () {
+      beforeEach(function () {
+        // original text is "one three four five"
+        this.ranges = {
+          comments: makeRanges([
+            { c: 'three', p: 4, t: 'comment-id-1' },
+            { c: 'ree four', p: 6, t: 'comment-id-2' },
+          ]),
+        }
+        this.updates = makeUpdates([
+          { i: '[before]', p: 4 },
+          { i: '[inside]', p: 13 }, // 4 + 8 + 1
+          { i: '[overlap]', p: 23 }, // 13 + 8 + 2
+          { i: '[after]', p: 39 }, // 23 + 9 + 7
+        ])
+        this.newDocLines = [
+          'one [before]t[inside]hr[overlap]ee four[after] five',
+        ]
+        this.result = this.RangesManager.applyUpdate(
+          this.project_id,
+          this.doc_id,
+          this.ranges,
+          this.updates,
+          this.newDocLines
+        )
+      })
+
+      it('should add the proper commentIds properties to ops', function () {
+        expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([
+          [{ i: '[before]', p: 4 }],
+          [{ i: '[inside]', p: 13, commentIds: ['comment-id-1'] }],
+          [
+            {
+              i: '[overlap]',
+              p: 23,
+              commentIds: ['comment-id-1', 'comment-id-2'],
+            },
+          ],
+          [{ i: '[after]', p: 39 }],
+        ])
+      })
+    })
   })
 
   describe('acceptChanges', function () {