Send operations to project-history when resolving/unresolving comments (#17540)

* Send operations to project-history when resolving/unresolving comments * small fixes * added doc_id in web unit test * Revert "added doc_id in web unit test" This reverts commit f0b8251abfce17965d5e1b0e45d8784fcf1d9eed. * fix mocked dependency in test * wip: web unit tests * document updater, reopen test * document-updater tests * format fix * fix typo * fix callsArgWith * fix reopenThread calls in doc updater tests * fix typos * log error if chat api resolve failes * log error when reopening thread * sendStatus calls done() in tests * using OError instead of logging * removed timers * preserve legacy endpoints * update after merge * Remove timer check in HttpControllerTest * prettier * added "legacy" in log * remove metrics.timer * fix promisify issues * remove unused cb GitOrigin-RevId: 849538c86996973a065c727835e93028e5429344
2024-11-21 20:47:08 -05:00 · 2024-04-09 11:15:06 +02:00 · 2024-04-09 11:15:06 +02:00 · 8bde496da4
commit 8bde496da4
parent 1557338775
12 changed files with 485 additions and 10 deletions
--- a/services/document-updater/app.js
+++ b/services/document-updater/app.js
@ -171,6 +171,14 @@ app.post(
  '/project/:project_id/doc/:doc_id/change/accept',
  HttpController.acceptChanges
 )
+app.post(
+  '/project/:project_id/doc/:doc_id/comment/:comment_id/resolve',
+  HttpController.resolveComment
+)
+app.post(
+  '/project/:project_id/doc/:doc_id/comment/:comment_id/reopen',
+  HttpController.reopenComment
+)
 app.delete(
  '/project/:project_id/doc/:doc_id/comment/:comment_id',
  HttpController.deleteComment
--- a/services/document-updater/app/js/DocumentManager.js
+++ b/services/document-updater/app/js/DocumentManager.js
@ -258,6 +258,30 @@ const DocumentManager = {
    )
  },

+  async updateCommentState(projectId, docId, commentId, userId, resolved) {
+    const { lines, version, pathname, historyRangesSupport } =
+      await DocumentManager.getDoc(projectId, docId)
+
+    if (lines == null || version == null) {
+      throw new Errors.NotFoundError(`document not found: ${docId}`)
+    }
+
+    if (historyRangesSupport) {
+      await ProjectHistoryRedisManager.promises.queueOps(
+        projectId,
+        JSON.stringify({
+          pathname,
+          commentId,
+          resolved,
+          meta: {
+            ts: new Date(),
+            user_id: userId,
+          },
+        })
+      )
+    }
+  },
+
  async deleteComment(projectId, docId, commentId, userId) {
    const { lines, version, ranges, pathname, historyRangesSupport } =
      await DocumentManager.getDoc(projectId, docId)
@ -427,6 +451,24 @@ const DocumentManager = {
    )
  },

+  async updateCommentStateWithLock(
+    projectId,
+    docId,
+    threadId,
+    userId,
+    resolved
+  ) {
+    const UpdateManager = require('./UpdateManager')
+    await UpdateManager.promises.lockUpdatesAndDo(
+      DocumentManager.updateCommentState,
+      projectId,
+      docId,
+      threadId,
+      userId,
+      resolved
+    )
+  },
+
  async deleteCommentWithLock(projectId, docId, threadId, userId) {
    const UpdateManager = require('./UpdateManager')
    await UpdateManager.promises.lockUpdatesAndDo(
--- a/services/document-updater/app/js/HttpController.js
+++ b/services/document-updater/app/js/HttpController.js
@ -23,6 +23,8 @@ module.exports = {
  deleteProject,
  deleteMultipleProjects,
  acceptChanges,
+  resolveComment,
+  reopenComment,
  deleteComment,
  updateProject,
  resyncProjectHistory,
@ -301,6 +303,54 @@ function acceptChanges(req, res, next) {
  })
 }

+function resolveComment(req, res, next) {
+  const {
+    project_id: projectId,
+    doc_id: docId,
+    comment_id: commentId,
+  } = req.params
+  const userId = req.body.user_id
+  logger.debug({ projectId, docId, commentId }, 'resolving comment via http')
+  DocumentManager.updateCommentStateWithLock(
+    projectId,
+    docId,
+    commentId,
+    userId,
+    true,
+    error => {
+      if (error) {
+        return next(error)
+      }
+      logger.debug({ projectId, docId, commentId }, 'resolved comment via http')
+      res.sendStatus(204) // No Content
+    }
+  )
+}
+
+function reopenComment(req, res, next) {
+  const {
+    project_id: projectId,
+    doc_id: docId,
+    comment_id: commentId,
+  } = req.params
+  const userId = req.body.user_id
+  logger.debug({ projectId, docId, commentId }, 'reopening comment via http')
+  DocumentManager.updateCommentStateWithLock(
+    projectId,
+    docId,
+    commentId,
+    userId,
+    false,
+    error => {
+      if (error) {
+        return next(error)
+      }
+      logger.debug({ projectId, docId, commentId }, 'reopened comment via http')
+      res.sendStatus(204) // No Content
+    }
+  )
+}
+
 function deleteComment(req, res, next) {
  const {
    project_id: projectId,
--- a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js
+++ b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js
@ -650,6 +650,149 @@ describe('HttpController', function () {
    })
  })

+  describe('resolveComment', function () {
+    beforeEach(function () {
+      this.user_id = 'user-id-123'
+      this.req = {
+        params: {
+          project_id: this.project_id,
+          doc_id: this.doc_id,
+          comment_id: (this.comment_id = 'mock-comment-id'),
+        },
+        query: {},
+        body: {
+          user_id: this.user_id,
+        },
+      }
+      this.resolved = true
+    })
+
+    describe('successfully', function () {
+      beforeEach(function (done) {
+        this.DocumentManager.updateCommentStateWithLock = sinon
+          .stub()
+          .callsArgWith(5)
+
+        this.ProjectHistoryRedisManager.queueOps = sinon.stub()
+        this.res.sendStatus.callsFake(() => done())
+        this.HttpController.resolveComment(this.req, this.res, this.next)
+      })
+
+      it('should accept the change', function () {
+        this.DocumentManager.updateCommentStateWithLock
+          .calledWith(
+            this.project_id,
+            this.doc_id,
+            this.comment_id,
+            this.user_id,
+            this.resolved
+          )
+          .should.equal(true)
+      })
+
+      it('should return a successful No Content response', function () {
+        this.res.sendStatus.calledWith(204).should.equal(true)
+      })
+
+      it('should log the request', function () {
+        this.logger.debug
+          .calledWith(
+            {
+              projectId: this.project_id,
+              docId: this.doc_id,
+              commentId: this.comment_id,
+            },
+            'resolving comment via http'
+          )
+          .should.equal(true)
+      })
+    })
+
+    describe('when an errors occurs', function () {
+      beforeEach(function () {
+        this.DocumentManager.updateCommentStateWithLock = sinon
+          .stub()
+          .callsArgWith(5, new Error('oops'))
+        this.HttpController.resolveComment(this.req, this.res, this.next)
+      })
+
+      it('should call next with the error', function () {
+        this.next.calledWith(sinon.match.instanceOf(Error)).should.equal(true)
+      })
+    })
+  })
+
+  describe('reopenComment', function () {
+    beforeEach(function () {
+      this.user_id = 'user-id-123'
+      this.req = {
+        params: {
+          project_id: this.project_id,
+          doc_id: this.doc_id,
+          comment_id: (this.comment_id = 'mock-comment-id'),
+        },
+        query: {},
+        body: {
+          user_id: this.user_id,
+        },
+      }
+      this.resolved = false
+    })
+
+    describe('successfully', function () {
+      beforeEach(function () {
+        this.DocumentManager.updateCommentStateWithLock = sinon
+          .stub()
+          .callsArgWith(5)
+
+        this.ProjectHistoryRedisManager.queueOps = sinon.stub()
+        this.HttpController.reopenComment(this.req, this.res, this.next)
+      })
+
+      it('should accept the change', function () {
+        this.DocumentManager.updateCommentStateWithLock
+          .calledWith(
+            this.project_id,
+            this.doc_id,
+            this.comment_id,
+            this.user_id,
+            this.resolved
+          )
+          .should.equal(true)
+      })
+
+      it('should return a successful No Content response', function () {
+        this.res.sendStatus.calledWith(204).should.equal(true)
+      })
+
+      it('should log the request', function () {
+        this.logger.debug
+          .calledWith(
+            {
+              projectId: this.project_id,
+              docId: this.doc_id,
+              commentId: this.comment_id,
+            },
+            'reopening comment via http'
+          )
+          .should.equal(true)
+      })
+    })
+
+    describe('when an errors occurs', function () {
+      beforeEach(function () {
+        this.DocumentManager.updateCommentStateWithLock = sinon
+          .stub()
+          .callsArgWith(5, new Error('oops'))
+        this.HttpController.reopenComment(this.req, this.res, this.next)
+      })
+
+      it('should call next with the error', function () {
+        this.next.calledWith(sinon.match.instanceOf(Error)).should.equal(true)
+      })
+    })
+  })
+
  describe('deleteComment', function () {
    beforeEach(function () {
      this.user_id = 'user-id-123'
--- a/services/project-history/app/js/UpdateTranslator.js
+++ b/services/project-history/app/js/UpdateTranslator.js
@ -17,6 +17,7 @@ import * as OperationsCompressor from './OperationsCompressor.js'
 * @typedef {import('./types').RenameUpdate} RenameUpdate
 * @typedef {import('./types').TextUpdate} TextUpdate
 * @typedef {import('./types').TrackingProps} TrackingProps
+ * @typedef {import('./types').SetCommentStateUpdate} SetCommentStateUpdate
 * @typedef {import('./types').Update} Update
 * @typedef {import('./types').UpdateWithBlob} UpdateWithBlob
 */
@ -79,6 +80,14 @@ function _convertToChange(projectId, updateWithBlob) {
    if (update.v != null) {
      v2DocVersions[update.doc] = { pathname, v: update.v }
    }
+  } else if (isSetCommentStateUpdate(update)) {
+    operations = [
+      {
+        pathname: _convertPathname(update.pathname),
+        commentId: update.commentId,
+        resolved: update.resolved,
+      },
+    ]
  } else if (isDeleteCommentUpdate(update)) {
    operations = [
      {
@ -189,6 +198,14 @@ export function isAddUpdate(update) {
  return _isAddDocUpdate(update) || _isAddFileUpdate(update)
 }

+/**
+ * @param {Update} update
+ * @returns {update is SetCommentStateUpdate}
+ */
+export function isSetCommentStateUpdate(update) {
+  return 'commentId' in update && 'resolved' in update
+}
+
 /**
 * @param {Update} update
 * @returns {update is DeleteCommentUpdate}
--- a/services/project-history/app/js/types.ts
+++ b/services/project-history/app/js/types.ts
@ -4,6 +4,7 @@ export type Update =
  | AddFileUpdate
  | RenameUpdate
  | DeleteCommentUpdate
+  | SetCommentStateUpdate

 export type UpdateMeta = {
  user_id: string
@ -25,6 +26,13 @@ export type TextUpdate = {
  }
 }

+export type SetCommentStateUpdate = {
+  pathname: string
+  commentId: string
+  resolved: boolean
+  meta: UpdateMeta
+}
+
 export type DeleteCommentUpdate = {
  pathname: string
  deleteComment: string
--- a/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js
+++ b/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js
@ -19,6 +19,8 @@ module.exports = {
  getProjectDocsIfMatch,
  clearProjectState,
  acceptChanges,
+  resolveThread,
+  reopenThread,
  deleteThread,
  resyncProjectHistory,
  updateProjectStructure,
@ -38,6 +40,8 @@ module.exports = {
    getProjectDocsIfMatch: promisify(getProjectDocsIfMatch),
    clearProjectState: promisify(clearProjectState),
    acceptChanges: promisify(acceptChanges),
+    resolveThread: promisify(resolveThread),
+    reopenThread: promisify(reopenThread),
    deleteThread: promisify(deleteThread),
    resyncProjectHistory: promisify(resyncProjectHistory),
    updateProjectStructure: promisify(updateProjectStructure),
@ -212,6 +216,36 @@ function acceptChanges(projectId, docId, changeIds, callback) {
  )
 }

+function resolveThread(projectId, docId, threadId, userId, callback) {
+  _makeRequest(
+    {
+      path: `/project/${projectId}/doc/${docId}/comment/${threadId}/resolve`,
+      method: 'POST',
+      json: {
+        user_id: userId,
+      },
+    },
+    projectId,
+    'resolve-thread',
+    callback
+  )
+}
+
+function reopenThread(projectId, docId, threadId, userId, callback) {
+  _makeRequest(
+    {
+      path: `/project/${projectId}/doc/${docId}/comment/${threadId}/reopen`,
+      method: 'POST',
+      json: {
+        user_id: userId,
+      },
+    },
+    projectId,
+    'reopen-thread',
+    callback
+  )
+}
+
 function deleteThread(projectId, docId, threadId, userId, callback) {
  _makeRequest(
    {
--- a/services/web/frontend/js/features/ide-react/context/review-panel/hooks/use-review-panel-state.ts
+++ b/services/web/frontend/js/features/ide-react/context/review-panel/hooks/use-review-panel-state.ts
@ -898,7 +898,9 @@ function useReviewPanelState(): ReviewPanel.ReviewPanelState {
        },
      }))

-      postJSON(`/project/${projectId}/thread/${entry.thread_id}/resolve`)
+      postJSON(
+        `/project/${projectId}/doc/${docId}/thread/${entry.thread_id}/resolve`
+      )
      onCommentResolved(entry.thread_id, user)
      sendMB('rp-comment-resolve', { view })
    },
@ -925,9 +927,9 @@ function useReviewPanelState(): ReviewPanel.ReviewPanelState {
  )

  const unresolveComment = useCallback(
-    (threadId: ThreadId) => {
+    (docId: DocId, threadId: ThreadId) => {
      onCommentReopened(threadId)
-      const url = `/project/${projectId}/thread/${threadId}/reopen`
+      const url = `/project/${projectId}/doc/${docId}/thread/${threadId}/reopen`
      postJSON(url).catch(debugConsole.error)
      sendMB('rp-comment-reopen')
    },
--- a/services/web/frontend/js/features/source-editor/components/review-panel/entries/resolved-comment-entry.tsx
+++ b/services/web/frontend/js/features/source-editor/components/review-panel/entries/resolved-comment-entry.tsx
@ -38,7 +38,7 @@ function ResolvedCommentEntry({
    : thread.content

  const handleUnresolve = () => {
-    unresolveComment(thread.threadId)
+    unresolveComment(thread.docId, thread.threadId)
  }

  const handleDelete = () => {
--- a/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts
+++ b/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts
@ -71,7 +71,7 @@ export interface ReviewPanelState {
      commentId: CommentId,
      content: string
    ) => void
-    unresolveComment: (threadId: ThreadId) => void
+    unresolveComment: (docId: DocId, threadId: ThreadId) => void
    deleteThread: (docId: DocId, threadId: ThreadId) => void
    refreshResolvedCommentsDropdown: () => Promise<
      void | ReviewPanelDocEntries[]
--- a/services/web/frontend/js/ide/review-panel/controllers/ReviewPanelController.js
+++ b/services/web/frontend/js/ide/review-panel/controllers/ReviewPanelController.js
@ -849,7 +849,7 @@ export default App.controller('ReviewPanelController', [
      const entry = getDocEntries(doc_id)[entry_id]
      entry.focused = false
      $http.post(
-        `/project/${$scope.project_id}/thread/${entry.thread_id}/resolve`,
+        `/project/${$scope.project_id}/doc/${doc_id}/thread/${entry.thread_id}/resolve`,
        { _csrf: window.csrfToken }
      )
      _onCommentResolved(entry.thread_id, ide.$scope.user)
@ -860,11 +860,14 @@ export default App.controller('ReviewPanelController', [
      })
    }

-    ide.$scope.unresolveComment = function (thread_id) {
+    ide.$scope.unresolveComment = function (doc_id, thread_id) {
      _onCommentReopened(thread_id)
-      $http.post(`/project/${$scope.project_id}/thread/${thread_id}/reopen`, {
-        _csrf: window.csrfToken,
-      })
+      $http.post(
+        `/project/${$scope.project_id}/doc/${doc_id}/thread/${thread_id}/reopen`,
+        {
+          _csrf: window.csrfToken,
+        }
+      )
      eventTracking.sendMB('rp-comment-reopen')
    }

--- a/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js
+++ b/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js
@ -838,6 +838,174 @@ describe('DocumentUpdaterHandler', function () {
    })
  })

+  describe('resolveThread', function () {
+    beforeEach(function () {
+      this.thread_id = 'mock-thread-id-1'
+    })
+
+    describe('successfully', function () {
+      beforeEach(function () {
+        this.request.callsArgWith(1, null, { statusCode: 200 }, this.body)
+        this.handler.resolveThread(
+          this.project_id,
+          this.doc_id,
+          this.thread_id,
+          this.user_id,
+          this.callback
+        )
+      })
+
+      it('should resolve the thread in the document updater', function () {
+        this.request
+          .calledWithMatch({
+            url: `${this.settings.apis.documentupdater.url}/project/${this.project_id}/doc/${this.doc_id}/comment/${this.thread_id}/resolve`,
+            method: 'POST',
+          })
+          .should.equal(true)
+      })
+
+      it('should call the callback', function () {
+        this.callback.calledWith(null).should.equal(true)
+      })
+    })
+
+    describe('when the document updater API returns an error', function () {
+      beforeEach(function () {
+        this.request.callsArgWith(
+          1,
+          new Error('something went wrong'),
+          null,
+          null
+        )
+        this.handler.resolveThread(
+          this.project_id,
+          this.doc_id,
+          this.thread_id,
+          this.user_id,
+          this.callback
+        )
+      })
+
+      it('should return an error to the callback', function () {
+        this.callback
+          .calledWith(sinon.match.instanceOf(Error))
+          .should.equal(true)
+      })
+    })
+
+    describe('when the document updater returns a failure error code', function () {
+      beforeEach(function () {
+        this.request.callsArgWith(1, null, { statusCode: 500 }, '')
+        this.handler.resolveThread(
+          this.project_id,
+          this.doc_id,
+          this.thread_id,
+          this.user_id,
+          this.callback
+        )
+      })
+
+      it('should return the callback with an error', function () {
+        this.callback
+          .calledWith(
+            sinon.match
+              .instanceOf(Error)
+              .and(
+                sinon.match.has(
+                  'message',
+                  'document updater returned a failure status code: 500'
+                )
+              )
+          )
+          .should.equal(true)
+      })
+    })
+  })
+
+  describe('reopenThread', function () {
+    beforeEach(function () {
+      this.thread_id = 'mock-thread-id-1'
+    })
+
+    describe('successfully', function () {
+      beforeEach(function () {
+        this.request.callsArgWith(1, null, { statusCode: 200 }, this.body)
+        this.handler.reopenThread(
+          this.project_id,
+          this.doc_id,
+          this.thread_id,
+          this.user_id,
+          this.callback
+        )
+      })
+
+      it('should reopen the thread in the document updater', function () {
+        this.request
+          .calledWithMatch({
+            url: `${this.settings.apis.documentupdater.url}/project/${this.project_id}/doc/${this.doc_id}/comment/${this.thread_id}/reopen`,
+            method: 'POST',
+          })
+          .should.equal(true)
+      })
+
+      it('should call the callback', function () {
+        this.callback.calledWith(null).should.equal(true)
+      })
+    })
+
+    describe('when the document updater API returns an error', function () {
+      beforeEach(function () {
+        this.request.callsArgWith(
+          1,
+          new Error('something went wrong'),
+          null,
+          null
+        )
+        this.handler.reopenThread(
+          this.project_id,
+          this.doc_id,
+          this.thread_id,
+          this.user_id,
+          this.callback
+        )
+      })
+
+      it('should return an error to the callback', function () {
+        this.callback
+          .calledWith(sinon.match.instanceOf(Error))
+          .should.equal(true)
+      })
+    })
+
+    describe('when the document updater returns a failure error code', function () {
+      beforeEach(function () {
+        this.request.callsArgWith(1, null, { statusCode: 500 }, '')
+        this.handler.reopenThread(
+          this.project_id,
+          this.doc_id,
+          this.thread_id,
+          this.user_id,
+          this.callback
+        )
+      })
+
+      it('should return the callback with an error', function () {
+        this.callback
+          .calledWith(
+            sinon.match
+              .instanceOf(Error)
+              .and(
+                sinon.match.has(
+                  'message',
+                  'document updater returned a failure status code: 500'
+                )
+              )
+          )
+          .should.equal(true)
+      })
+    })
+  })
+
  describe('updateProjectStructure ', function () {
    beforeEach(function () {
      this.user_id = 1234