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

* 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: 14c10669e43d76883dbaaa8ab55e102b5ebadd38
2024-11-21 20:47:08 -05:00 · 2024-03-11 17:24:43 +01:00 · 2024-03-11 17:24:43 +01:00 · 3e701f1388
commit 3e701f1388
parent 51994579cd
8 changed files with 125 additions and 18 deletions
--- a/services/document-updater/app/js/DocumentManager.js
+++ b/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)
        }
@ -457,6 +467,21 @@ const DocumentManager = {
          [],
          newRanges,
          {},
+          error => {
+            if (error) {
+              return callback(error)
+            }
+            if (historyRangesSupport) {
+              ProjectHistoryRedisManager.queueOps(
+                projectId,
+                JSON.stringify({
+                  pathname,
+                  deleteComment: commentId,
+                  meta: {
+                    ts: new Date(),
+                    user_id: userId,
+                  },
+                }),
                error => {
                  if (error) {
                    return callback(error)
@ -464,6 +489,11 @@ const DocumentManager = {
                  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
    )
  },
--- a/services/document-updater/app/js/HttpController.js
+++ b/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 => {
+  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
-  })
+    }
+  )
 }

 function updateProject(req, res, next) {
--- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js
+++ b/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)
      })
--- a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js
+++ b/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)
      })

--- a/services/project-history/app/js/UpdateTranslator.js
+++ b/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(/^\//, '')
--- a/services/project-history/app/js/types.ts
+++ b/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
--- a/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js
+++ b/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',
--- a/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js
+++ b/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
        )
      })