Sfoglia il codice sorgente

Reject tracked changes notifications (#32917)

* [web] Reject tracked changes notifications

feat: adding new tests

feat: adding rejected changes notifications

feat: adding tests for rejectchanges

feat: updating tests for rejecting notifications;

feat: adding in rejecting user, and improving subject and activity line

fix: moving to a params object instead of positionals for email building

feat: updating to use events triggered from applyUpdate in document-updater

feat: updating to send rejected author ids with rejected change notification instead of change ids

feat: moving rejected author notification determination to updateManager instead of RangesManager, which is used by other paths

feat: only map to author if changes were made

* fix: gate by user status not project status

* fix: unit tests post-rebase

---------

Co-authored-by: Kristina Hjertberg <kristina.hjertberg@overleaf.com>
GitOrigin-RevId: f992e1885c47d1a6cf776740769d6d4763f3cb7c
Jimmy Domagala-Tang 3 mesi fa
parent
commit
bc2f5ae746

+ 8 - 2
services/document-updater/app/js/UpdateManager.js

@@ -204,7 +204,13 @@ const UpdateManager = {
 
       // applyUpdate is not triggered by accept change operations, so any
       // tracked change removed by the ops we just applied was rejected.
+      // Look up the authors of those rejected changes from the pre-update
+      // ranges so we can notify web below.
       if (removedChangeIds.length > 0) {
+        const rejectedChangeAuthorIds = (ranges?.changes || [])
+          .filter(change => removedChangeIds.includes(change.id))
+          .map(change => change.metadata.user_id)
+
         // Fire-and-forget without awaiting because
         // we hold the doc lock here, and the result of the
         // notification doesn't affect the update
@@ -212,12 +218,12 @@ const UpdateManager = {
           .notifyTrackChangesRejected(
             projectId,
             docId,
-            removedChangeIds,
+            rejectedChangeAuthorIds,
             update.meta?.user_id
           )
           .catch(err => {
             logger.warn(
-              { err, projectId, docId, removedChangeIds },
+              { err, projectId, docId, rejectedChangeAuthorIds },
               'failed to notify web of rejected track changes'
             )
           })

+ 3 - 3
services/document-updater/app/js/WebApiManager.js

@@ -7,13 +7,13 @@ const MAX_HTTP_REQUEST_LENGTH = 5000
 /**
  * @param {string} projectId
  * @param {string} docId
- * @param {string[]} rejectedChangeIds
+ * @param {string[]} rejectedChangeAuthorIds
  * @param {string | undefined} userId
  */
 async function notifyTrackChangesRejected(
   projectId,
   docId,
-  rejectedChangeIds,
+  rejectedChangeAuthorIds,
   userId
 ) {
   const url = new URL(
@@ -23,7 +23,7 @@ async function notifyTrackChangesRejected(
 
   await fetchNothing(url, {
     method: 'POST',
-    json: { rejectedChangeIds, userId },
+    json: { rejectedChangeAuthorIds, userId },
     basicAuth: {
       user: Settings.apis.web.user,
       password: Settings.apis.web.pass,

+ 1 - 1
services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js

@@ -287,7 +287,7 @@ describe('RangesManager', function () {
           newDocLines,
           { historyRangesSupport: true }
         )
-        expect(result.removedChangeIds).to.deep.equal(['1'])
+        expect(result.removedChangeIds).to.deep.equal([ranges.changes[0].id])
       })
     })
 

+ 31 - 3
services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js

@@ -536,12 +536,40 @@ describe('UpdateManager', function () {
 
     describe('when tracked changes are rejected', function () {
       beforeEach(async function () {
-        this.removedChangeIds = ['change-1', 'change-2']
+        this.rejectedChangeAuthorIds = ['author-1', 'author-2']
+        // The ranges that getDoc returned must include the changes whose IDs
+        // RangesManager reports as removed so UpdateManager can look up the
+        // authors locally.
+        this.ranges = {
+          changes: [
+            {
+              id: 'change-1',
+              metadata: { user_id: 'author-1' },
+            },
+            {
+              id: 'change-2',
+              metadata: { user_id: 'author-2' },
+            },
+            {
+              id: 'change-untouched',
+              metadata: { user_id: 'author-3' },
+            },
+          ],
+        }
+        this.DocumentManager.promises.getDoc.resolves({
+          lines: this.lines,
+          version: this.version,
+          ranges: this.ranges,
+          pathname: this.pathname,
+          projectHistoryId: this.projectHistoryId,
+          historyRangesSupport: false,
+          type: 'sharejs-text-ot',
+        })
         this.RangesManager.applyUpdate.returns({
           newRanges: this.updated_ranges,
           rangesWereCollapsed: false,
           historyUpdates: this.historyUpdates,
-          removedChangeIds: this.removedChangeIds,
+          removedChangeIds: ['change-1', 'change-2'],
         })
         await this.UpdateManager.promises.applyUpdate(
           this.project_id,
@@ -554,7 +582,7 @@ describe('UpdateManager', function () {
         this.WebApiManager.promises.notifyTrackChangesRejected.should.have.been.calledWith(
           this.project_id,
           this.doc_id,
-          this.removedChangeIds,
+          this.rejectedChangeAuthorIds,
           this.updateMeta.user_id
         )
       })

+ 3 - 3
services/web/app/src/Features/Documents/DocumentController.mjs

@@ -107,13 +107,13 @@ async function setDocument(req, res) {
 
 async function trackChangesRejected(req, res) {
   const { Project_id: projectId, doc_id: docId } = req.params
-  const { rejectedChangeIds, userId } = req.body
+  const { rejectedChangeAuthorIds, userId } = req.body
   await Modules.promises.hooks.fire(
     'trackChangesRejected',
     projectId,
     docId,
-    rejectedChangeIds,
-    userId
+    userId,
+    rejectedChangeAuthorIds
   )
   res.sendStatus(204)
 }

+ 7 - 0
services/web/types/module-hooks.ts

@@ -9,6 +9,13 @@ export type TrackChangesAcceptedEvent = {
   changeContributors: string[]
 }
 
+export type TrackChangesRejectedEvent = {
+  projectId: string
+  docId: string
+  userId: string
+  changeContributors: string[]
+}
+
 export type CommentAddedEvent = {
   projectId: string
   userId: string