diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index cdfdadc071..11b71f601b 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -331,6 +331,7 @@ const DocumentManager = { if (changeIds == null) { changeIds = [] } + let changeContributors = [] const { lines, @@ -374,7 +375,7 @@ const DocumentManager = { }) if (historyUpdates.length === 0) { - return + return changeContributors } await ProjectHistoryRedisManager.promises.queueOps( @@ -382,6 +383,11 @@ const DocumentManager = { ...historyUpdates.map(op => JSON.stringify(op)) ) } + changeContributors = (ranges.changes || []) + .filter(change => changeIds.includes(change.id)) + .map(change => change?.metadata?.user_id) + .filter(userId => userId) + return changeContributors }, async rejectChanges(projectId, docId, changeIds, userId) { @@ -708,12 +714,13 @@ const DocumentManager = { async acceptChangesWithLock(projectId, docId, changeIds) { const UpdateManager = require('./UpdateManager') - await UpdateManager.promises.lockUpdatesAndDo( + const changeContributors = await UpdateManager.promises.lockUpdatesAndDo( DocumentManager.acceptChanges, projectId, docId, changeIds ) + return changeContributors }, async rejectChangesWithLock(projectId, docId, changeIds, userId) { diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index c45876ed68..a09e3618d6 100644 --- a/services/document-updater/app/js/HttpController.js +++ b/services/document-updater/app/js/HttpController.js @@ -355,17 +355,19 @@ async function acceptChanges(req, res) { `accepting ${changeIds.length} changes via http` ) const timer = new Metrics.Timer('http.acceptChanges') - await DocumentManager.promises.acceptChangesWithLock( - projectId, - docId, - changeIds - ) + const changeContributors = + await DocumentManager.promises.acceptChangesWithLock( + projectId, + docId, + changeIds + ) timer.done() logger.debug( { projectId, docId }, `accepted ${changeIds.length} changes via http` ) - res.sendStatus(204) // No Content + + res.status(200).json({ changeContributors }) } async function rejectChanges(req, res) { diff --git a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js index da99b37f52..0d4b2fd33d 100644 --- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js +++ b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js @@ -756,7 +756,18 @@ describe('DocumentManager', function () { ] this.version = 34 this.lines = ['original', 'lines'] - this.ranges = { entries: 'mock', comments: 'mock' } + this.ranges = { + entries: 'mock', + comments: 'mock', + changes: [ + { id: 'mock-change-id', metadata: { user_id: 'mock-user-id-0' } }, + { id: 'mock-change-id-1', metadata: { user_id: 'mock-user-id-1' } }, + { id: 'mock-change-id-2', metadata: { user_id: 'mock-user-id-2' } }, + { id: 'mock-change-id-3', metadata: { user_id: 'mock-user-id-3' } }, + { id: 'mock-change-id-4', metadata: { user_id: 'mock-user-id-4' } }, + { id: 'other-change-id', metadata: { user_id: 'other-user-id' } }, + ], + } this.updated_ranges = { entries: 'updated', comments: 'updated' } this.DocumentManager.promises.getDoc = sinon.stub().resolves({ lines: this.lines, @@ -768,7 +779,7 @@ describe('DocumentManager', function () { describe('successfully with a single change', function () { beforeEach(async function () { - await this.DocumentManager.promises.acceptChanges( + this.result = await this.DocumentManager.promises.acceptChanges( this.project_id, this.doc_id, [this.change_id] @@ -803,11 +814,15 @@ describe('DocumentManager', function () { ) .should.equal(true) }) + + it('should return the change contributors', function () { + expect(this.result).to.deep.equal(['mock-user-id-0']) + }) }) describe('successfully with multiple changes', function () { beforeEach(async function () { - await this.DocumentManager.promises.acceptChanges( + this.result = await this.DocumentManager.promises.acceptChanges( this.project_id, this.doc_id, this.change_ids @@ -824,6 +839,15 @@ describe('DocumentManager', function () { ) .should.equal(true) }) + + it('should return the change contributors', function () { + expect(this.result).to.deep.equal([ + 'mock-user-id-1', + 'mock-user-id-2', + 'mock-user-id-3', + 'mock-user-id-4', + ]) + }) }) describe('when the doc is not found', function () { diff --git a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js index 58db0a80f8..f293687646 100644 --- a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js +++ b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js @@ -14,6 +14,7 @@ describe('HttpController', function () { send: sinon.stub(), sendStatus: sinon.stub(), json: sinon.stub(), + status: sinon.stub().returnsThis(), } this.DocumentManager = { @@ -753,6 +754,10 @@ describe('HttpController', function () { describe('successfully with a single change', function () { beforeEach(async function () { + this.changeContributors = ['user-id-1', 'user-id-2'] + this.DocumentManager.promises.acceptChangesWithLock.resolves( + this.changeContributors + ) await this.HttpController.acceptChanges(this.req, this.res, this.next) }) @@ -764,8 +769,11 @@ describe('HttpController', function () { ) }) - it('should return a successful No Content response', function () { - this.res.sendStatus.calledWith(204).should.equal(true) + it('should return a successful 200 with a list of the change contributors', function () { + this.res.status.should.have.been.calledWith(200) + this.res.json.should.have.been.calledWith({ + changeContributors: this.changeContributors, + }) }) it('should log the request', function () { diff --git a/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.mjs b/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.mjs index c9cc737b46..ba1a8f205e 100644 --- a/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.mjs +++ b/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.mjs @@ -202,9 +202,10 @@ async function clearProjectState(projectId) { * @param {string} projectId * @param {string} docId * @param {string[]} changeIds + * @param {string} userId */ async function acceptChanges(projectId, docId, changeIds, userId) { - await fetchNothing( + const { changeContributors } = await fetchJson( `${BASE_URL}/project/${projectId}/doc/${docId}/change/accept`, { method: 'POST', @@ -212,8 +213,13 @@ async function acceptChanges(projectId, docId, changeIds, userId) { signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS), } ) - - await Modules.promises.hooks.fire('changesAccepted', projectId, docId, userId) + await Modules.promises.hooks.fire( + 'changesAccepted', + projectId, + docId, + userId, + changeContributors + ) } /** diff --git a/services/web/test/acceptance/src/mocks/MockDocUpdaterApi.mjs b/services/web/test/acceptance/src/mocks/MockDocUpdaterApi.mjs index 9ae253311e..02414c7c03 100644 --- a/services/web/test/acceptance/src/mocks/MockDocUpdaterApi.mjs +++ b/services/web/test/acceptance/src/mocks/MockDocUpdaterApi.mjs @@ -47,7 +47,10 @@ class MockDocUpdaterApi extends AbstractMockApi { this.app.post( '/project/:projectId/doc/:docId/change/accept', (req, res) => { - res.sendStatus(204) + res.status(200).json({ + // todo: return a list of change contributors based on doc ranges accepted similar to DocumentManager, and require tests to set real changes onto a doc before calling accept + changeContributors: [], + }) } ) diff --git a/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandler.test.mjs b/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandler.test.mjs index b4c6d3ddd8..b3a565abfe 100644 --- a/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandler.test.mjs +++ b/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandler.test.mjs @@ -68,11 +68,20 @@ describe('DocumentUpdaterHandler', function () { default: {}, })) + vi.doMock('@overleaf/metrics', () => ({ + default: { + Timer: class { + done() {} + }, + }, + })) + + ctx.modulesHooksFire = sinon.stub().resolves() vi.doMock('../../../../app/src/infrastructure/Modules', () => ({ default: { promises: { hooks: { - fire: sinon.stub().resolves(), + fire: ctx.modulesHooksFire, }, }, }, @@ -540,23 +549,71 @@ describe('DocumentUpdaterHandler', function () { describe('acceptChanges', function () { beforeEach(function (ctx) { ctx.change_id = 'mock-change-id-1' + ctx.change_contributors = ['mock-user-id-1', 'mock-user-id-2'] }) describe('successfully', function () { - beforeEach(function (ctx) { + beforeEach(async function (ctx) { ctx.docUpdaterMock .post(`/project/${ctx.project_id}/doc/${ctx.doc_id}/change/accept`, { change_ids: [ctx.change_id], }) - .reply(200) + .reply(200, { changeContributors: ctx.change_contributors }) + await ctx.handler.promises.acceptChanges( + ctx.project_id, + ctx.doc_id, + [ctx.change_id], + ctx.user_id + ) }) - it('should accept the change in the document updater', async function (ctx) { - await ctx.handler.promises.acceptChanges(ctx.project_id, ctx.doc_id, [ - ctx.change_id, - ]) + it('should accept the change in the document updater', function (ctx) { expect(ctx.docUpdaterMock.isDone()).to.be.true }) + + it('should fire the changesAccepted hook with change contributors', function (ctx) { + expect(ctx.modulesHooksFire).to.have.been.calledWith( + 'changesAccepted', + ctx.project_id, + ctx.doc_id, + ctx.user_id, + ctx.change_contributors + ) + }) + }) + + describe('when there are no change contributors', function () { + beforeEach(async function (ctx) { + ctx.docUpdaterMock + .post(`/project/${ctx.project_id}/doc/${ctx.doc_id}/change/accept`) + .reply(200, { changeContributors: [] }) + await ctx.handler.promises.acceptChanges( + ctx.project_id, + ctx.doc_id, + [ctx.change_id], + ctx.user_id + ) + }) + }) + + describe('when there are duplicate change contributors', function () { + beforeEach(async function (ctx) { + ctx.docUpdaterMock + .post(`/project/${ctx.project_id}/doc/${ctx.doc_id}/change/accept`) + .reply(200, { + changeContributors: [ + 'mock-user-id-1', + 'mock-user-id-1', + 'mock-user-id-2', + ], + }) + await ctx.handler.promises.acceptChanges( + ctx.project_id, + ctx.doc_id, + [ctx.change_id], + ctx.user_id + ) + }) }) describe('when the document updater API returns an error', function () { diff --git a/services/web/types/module-hooks.ts b/services/web/types/module-hooks.ts index bb0b713759..caba7f7b52 100644 --- a/services/web/types/module-hooks.ts +++ b/services/web/types/module-hooks.ts @@ -2,6 +2,13 @@ * Types for module hook events fired across the application */ +export type TrackChangesAcceptedEvent = { + projectId: string + docId: string + userId: string + changeContributors: string[] +} + export type CommentAddedEvent = { projectId: string userId: string