From 3e701f13883b0b45d12c33d0f01b079332e630f4 Mon Sep 17 00:00:00 2001 From: Domagoj Kriskovic Date: Mon, 11 Mar 2024 17:24:43 +0100 Subject: [PATCH] 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 --- .../app/js/DocumentManager.js | 39 +++++++++++++++++-- .../document-updater/app/js/HttpController.js | 21 ++++++---- .../DocumentManager/DocumentManagerTests.js | 32 ++++++++++++++- .../js/HttpController/HttpControllerTests.js | 19 +++++++-- .../app/js/UpdateTranslator.js | 16 ++++++++ services/project-history/app/js/types.ts | 8 +++- .../DocumentUpdater/DocumentUpdaterHandler.js | 5 ++- .../DocumentUpdaterHandlerTests.js | 3 ++ 8 files changed, 125 insertions(+), 18 deletions(-) diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index 8a27c18c69..8b6ca6e565 100644 --- 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) } @@ -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 ) }, diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index b23302121d..bc4fed1287 100644 --- 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 => { - 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) { diff --git a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js index 2e0e071ac9..93d204d38e 100644 --- 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) }) diff --git a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js index adfe461ea7..f98c08a45e 100644 --- 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) }) diff --git a/services/project-history/app/js/UpdateTranslator.js b/services/project-history/app/js/UpdateTranslator.js index 5f46cd723f..f17d42801a 100644 --- 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(/^\//, '') diff --git a/services/project-history/app/js/types.ts b/services/project-history/app/js/types.ts index e508f3cd35..b1af671335 100644 --- 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 diff --git a/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js b/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js index f077c28313..c331665c1a 100644 --- 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', diff --git a/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js b/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js index 820642d124..23ec1e6875 100644 --- 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 ) })