Send operations to project-history when deleting comments (#17198)

* Make Operation.fromRaw parse AddComment operations

* Send operations to project-history when deleting comments

* remove RedisManager.updateDocument call

* format fix

* Revert "remove RedisManager.updateDocument call"

This reverts commit 5d45f34b5919bf5a85e8475e5a450bcb213d3f82.

* remove versions from deleteComment op

* pass userId to deleteComment operation

* revert userId from chat service

* revert userid from chat tests

* format:fix

* document updater fix tests

* format:fix

* fix web unit test

* deleteThread test fix

* send only if historyRangesSupport is true

* use headers to pass userId

* set historyRangesSupport in tests

* optional headers

* Revert "use headers to pass userId"

This reverts commit e916c469f95b1ac166e30e12e735171eb814af01.

---------

Co-authored-by: Eric Mc Sween <5454374+emcsween@users.noreply.github.com>
GitOrigin-RevId: 34ab7e4ea6dd1b5fd749f9accaf4a6e16ae1d840

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

@@ -422,7 +422,7 @@ const DocumentManager = {
     )
   },
 
-  deleteComment(projectId, docId, commentId, _callback) {
+  deleteComment(projectId, docId, commentId, userId, _callback) {
     const timer = new Metrics.Timer('docManager.deleteComment')
     const callback = (...args) => {
       timer.done()
@@ -432,7 +432,17 @@ const DocumentManager = {
     DocumentManager.getDoc(
       projectId,
       docId,
-      (error, lines, version, ranges) => {
+      (
+        error,
+        lines,
+        version,
+        ranges,
+        pathname,
+        projectHistoryId,
+        unflushedTime,
+        alreadyLoaded,
+        historyRangesSupport
+      ) => {
         if (error) {
           return callback(error)
         }
@@ -461,7 +471,27 @@ const DocumentManager = {
             if (error) {
               return callback(error)
             }
-            callback()
+            if (historyRangesSupport) {
+              ProjectHistoryRedisManager.queueOps(
+                projectId,
+                JSON.stringify({
+                  pathname,
+                  deleteComment: commentId,
+                  meta: {
+                    ts: new Date(),
+                    user_id: userId,
+                  },
+                }),
+                error => {
+                  if (error) {
+                    return callback(error)
+                  }
+                  callback()
+                }
+              )
+            } else {
+              callback()
+            }
           }
         )
       }
@@ -659,13 +689,14 @@ const DocumentManager = {
     )
   },
 
-  deleteCommentWithLock(projectId, docId, threadId, callback) {
+  deleteCommentWithLock(projectId, docId, threadId, userId, callback) {
     const UpdateManager = require('./UpdateManager')
     UpdateManager.lockUpdatesAndDo(
       DocumentManager.deleteComment,
       projectId,
       docId,
       threadId,
+      userId,
       callback
     )
   },

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

@@ -307,16 +307,23 @@ function deleteComment(req, res, next) {
     doc_id: docId,
     comment_id: commentId,
   } = req.params
+  const userId = req.body.user_id
   logger.debug({ projectId, docId, commentId }, 'deleting comment via http')
   const timer = new Metrics.Timer('http.deleteComment')
-  DocumentManager.deleteCommentWithLock(projectId, docId, commentId, error => {
-    timer.done()
-    if (error) {
-      return next(error)
+  DocumentManager.deleteCommentWithLock(
+    projectId,
+    docId,
+    commentId,
+    userId,
+    error => {
+      timer.done()
+      if (error) {
+        return next(error)
+      }
+      logger.debug({ projectId, docId, commentId }, 'deleted comment via http')
+      res.sendStatus(204) // No Content
     }
-    logger.debug({ projectId, docId, commentId }, 'deleted comment via http')
-    res.sendStatus(204) // No Content
-  })
+  )
 }
 
 function updateProject(req, res, next) {

services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js

@@ -833,13 +833,25 @@ describe('DocumentManager', function () {
       this.lines = ['original', 'lines']
       this.ranges = { comments: ['one', 'two', 'three'] }
       this.updated_ranges = { comments: ['one', 'three'] }
+      this.historyRangesSupport = true
       this.DocumentManager.getDoc = sinon
         .stub()
-        .yields(null, this.lines, this.version, this.ranges)
+        .yields(
+          null,
+          this.lines,
+          this.version,
+          this.ranges,
+          this.pathname,
+          this.projectHistoryId,
+          Date.now() - 1e9,
+          true,
+          this.historyRangesSupport
+        )
       this.RangesManager.deleteComment = sinon
         .stub()
         .returns(this.updated_ranges)
       this.RedisManager.updateDocument = sinon.stub().yields()
+      this.ProjectHistoryRedisManager.queueOps = sinon.stub().yields()
     })
 
     describe('successfully', function () {
@@ -848,6 +860,7 @@ describe('DocumentManager', function () {
           this.project_id,
           this.doc_id,
           this.comment_id,
+          this.user_id,
           this.callback
         )
       })
@@ -878,6 +891,23 @@ describe('DocumentManager', function () {
           .should.equal(true)
       })
 
+      it('should queue the delete comment operation', function () {
+        this.ProjectHistoryRedisManager.queueOps
+          .calledWith(
+            this.project_id,
+            JSON.stringify({
+              pathname: this.pathname,
+              deleteComment: this.comment_id,
+              meta: {
+                ts: new Date(),
+                user_id: this.user_id,
+              },
+            }),
+            sinon.match.func
+          )
+          .should.equal(true)
+      })
+
       it('should call the callback', function () {
         this.callback.called.should.equal(true)
       })

services/document-updater/test/unit/js/HttpController/HttpControllerTests.js

@@ -11,6 +11,7 @@ describe('HttpController', function () {
         './HistoryManager': (this.HistoryManager = {
           flushProjectChangesAsync: sinon.stub(),
         }),
+        './ProjectHistoryRedisManager': (this.ProjectHistoryRedisManager = {}),
         './ProjectManager': (this.ProjectManager = {}),
         './ProjectFlusher': { flushAllProjects() {} },
         './DeleteQueueManager': (this.DeleteQueueManager = {}),
@@ -651,6 +652,7 @@ describe('HttpController', function () {
 
   describe('deleteComment', function () {
     beforeEach(function () {
+      this.user_id = 'user-id-123'
       this.req = {
         params: {
           project_id: this.project_id,
@@ -658,7 +660,9 @@ describe('HttpController', function () {
           comment_id: (this.comment_id = 'mock-comment-id'),
         },
         query: {},
-        body: {},
+        body: {
+          user_id: this.user_id,
+        },
       }
     })
 
@@ -666,13 +670,20 @@ describe('HttpController', function () {
       beforeEach(function () {
         this.DocumentManager.deleteCommentWithLock = sinon
           .stub()
-          .callsArgWith(3)
+          .callsArgWith(4)
+
+        this.ProjectHistoryRedisManager.queueOps = sinon.stub()
         this.HttpController.deleteComment(this.req, this.res, this.next)
       })
 
       it('should accept the change', function () {
         this.DocumentManager.deleteCommentWithLock
-          .calledWith(this.project_id, this.doc_id, this.comment_id)
+          .calledWith(
+            this.project_id,
+            this.doc_id,
+            this.comment_id,
+            this.user_id
+          )
           .should.equal(true)
       })
 
@@ -702,7 +713,7 @@ describe('HttpController', function () {
       beforeEach(function () {
         this.DocumentManager.deleteCommentWithLock = sinon
           .stub()
-          .callsArgWith(3, new Error('oops'))
+          .callsArgWith(4, new Error('oops'))
         this.HttpController.deleteComment(this.req, this.res, this.next)
       })
 

services/project-history/app/js/UpdateTranslator.js

@@ -14,6 +14,7 @@ import * as OperationsCompressor from './OperationsCompressor.js'
  * @typedef {import('./types.ts').Op} Op
  * @typedef {import('./types.ts').RenameUpdate} RenameUpdate
  * @typedef {import('./types.ts').TextUpdate} TextUpdate
+ * @typedef {import('./types.ts').DeleteCommentUpdate} DeleteCommentUpdate
  * @typedef {import('./types.ts').Update} Update
  * @typedef {import('./types.ts').UpdateWithBlob} UpdateWithBlob
  */
@@ -77,6 +78,13 @@ function _convertToChange(projectId, updateWithBlob) {
     if (update.v != null) {
       v2DocVersions[update.doc] = { pathname, v: update.v }
     }
+  } else if (isDeleteCommentUpdate(update)) {
+    operations = [
+      {
+        pathname: _convertPathname(update.pathname),
+        deleteComment: update.deleteComment,
+      },
+    ]
   } else {
     const error = new Errors.UpdateWithUnknownFormatError(
       'update with unknown format',
@@ -180,6 +188,14 @@ export function isAddUpdate(update) {
   return _isAddDocUpdate(update) || _isAddFileUpdate(update)
 }
 
+/**
+ * @param {Update} update
+ * @returns {update is DeleteCommentUpdate}
+ */
+export function isDeleteCommentUpdate(update) {
+  return 'deleteComment' in update
+}
+
 export function _convertPathname(pathname) {
   // Strip leading /
   pathname = pathname.replace(/^\//, '')

services/project-history/app/js/types.ts

@@ -1,4 +1,4 @@
-export type Update = TextUpdate | AddDocUpdate | AddFileUpdate | RenameUpdate
+export type Update = TextUpdate | AddDocUpdate | AddFileUpdate | RenameUpdate | DeleteCommentUpdate
 
 export type UpdateMeta = {
   user_id: string
@@ -18,6 +18,12 @@ export type TextUpdate = {
   }
 }
 
+export type DeleteCommentUpdate = {
+  pathname: string
+  deleteComment: string
+  meta: UpdateMeta
+}
+
 type ProjectUpdateBase = {
   version: string
   projectHistoryId: string

services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js

@@ -212,11 +212,14 @@ function acceptChanges(projectId, docId, changeIds, callback) {
   )
 }
 
-function deleteThread(projectId, docId, threadId, callback) {
+function deleteThread(projectId, docId, threadId, userId, callback) {
   _makeRequest(
     {
       path: `/project/${projectId}/doc/${docId}/comment/${threadId}`,
       method: 'DELETE',
+      json: {
+        user_id: userId,
+      },
     },
     projectId,
     'delete-thread',

services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js

@@ -766,6 +766,7 @@ describe('DocumentUpdaterHandler', function () {
           this.project_id,
           this.doc_id,
           this.thread_id,
+          this.user_id,
           this.callback
         )
       })
@@ -796,6 +797,7 @@ describe('DocumentUpdaterHandler', function () {
           this.project_id,
           this.doc_id,
           this.thread_id,
+          this.user_id,
           this.callback
         )
       })
@@ -814,6 +816,7 @@ describe('DocumentUpdaterHandler', function () {
           this.project_id,
           this.doc_id,
           this.thread_id,
+          this.user_id,
           this.callback
         )
       })