From 8bde496da46c6d9becef1c22c0a57b0e8ca43a32 Mon Sep 17 00:00:00 2001 From: Domagoj Kriskovic Date: Tue, 9 Apr 2024 11:15:06 +0200 Subject: [PATCH] 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 --- services/document-updater/app.js | 8 + .../app/js/DocumentManager.js | 42 +++++ .../document-updater/app/js/HttpController.js | 50 ++++++ .../js/HttpController/HttpControllerTests.js | 143 +++++++++++++++ .../app/js/UpdateTranslator.js | 17 ++ services/project-history/app/js/types.ts | 8 + .../DocumentUpdater/DocumentUpdaterHandler.js | 34 ++++ .../hooks/use-review-panel-state.ts | 8 +- .../entries/resolved-comment-entry.tsx | 2 +- .../review-panel/types/review-panel-state.ts | 2 +- .../controllers/ReviewPanelController.js | 13 +- .../DocumentUpdaterHandlerTests.js | 168 ++++++++++++++++++ 12 files changed, 485 insertions(+), 10 deletions(-) diff --git a/services/document-updater/app.js b/services/document-updater/app.js index 7fa9743779..7a547bd89f 100644 --- 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 diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index 46c33ef2dc..fe485a52ea 100644 --- 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( diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index bc4fed1287..b39668a35c 100644 --- 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, diff --git a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js index f98c08a45e..b131c774d3 100644 --- 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' diff --git a/services/project-history/app/js/UpdateTranslator.js b/services/project-history/app/js/UpdateTranslator.js index 04841f848c..ded0c4f4d7 100644 --- 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} diff --git a/services/project-history/app/js/types.ts b/services/project-history/app/js/types.ts index 91d1ea6bcf..cf46092d48 100644 --- 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 diff --git a/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js b/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js index c331665c1a..2b17544255 100644 --- 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( { diff --git 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 index ef990876fc..a0143e718d 100644 --- 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') }, diff --git 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 index 609bb8fde3..5a8ef217c8 100644 --- 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 = () => { diff --git 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 index 6531261d53..b64b3f79a6 100644 --- 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[] diff --git a/services/web/frontend/js/ide/review-panel/controllers/ReviewPanelController.js b/services/web/frontend/js/ide/review-panel/controllers/ReviewPanelController.js index ba622f71e2..dcc4853e9b 100644 --- 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') } diff --git a/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js b/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js index 23ec1e6875..4e41873c76 100644 --- 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