[Web + Doc-Updater] Add track changes accepted notification (#32752)

* feat: update doc manager to return a list of contributors to the accepted change

* feat: add new notification type for accepting a tracked change

* update email with tracked changes accepted

* feat: update tests

* fix: feedback on consistent api and returns

* feat: adding new tests

* feat: self accepted changes shouldnt trigger notification, and using existing changesAccepted hook

* Add better subject and activity list for track change accepted (#33094)

* feat: add better activity list entry and subject header for accepted changes, to match other notifications

* feat: updating tests

* feat: updating accepting_user_id to just user_id

* fix: adding users in emailBuilder test to userCache

GitOrigin-RevId: 6114f77916b5f503b7bbbb5ca8fed99e58edc31b

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) {

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) {

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 () {

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 () {

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
+  )
 }
 
 /**

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: [],
+        })
       }
     )
 

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 () {

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