From bc2f5ae746f05a44dc0c0f6b171ec43d2a0a936b Mon Sep 17 00:00:00 2001 From: Jimmy Domagala-Tang Date: Wed, 13 May 2026 04:22:47 -0400 Subject: [PATCH] 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 GitOrigin-RevId: f992e1885c47d1a6cf776740769d6d4763f3cb7c --- .../document-updater/app/js/UpdateManager.js | 10 ++++-- .../document-updater/app/js/WebApiManager.js | 6 ++-- .../js/RangesManager/RangesManagerTests.js | 2 +- .../js/UpdateManager/UpdateManagerTests.js | 34 +++++++++++++++++-- .../Features/Documents/DocumentController.mjs | 6 ++-- services/web/types/module-hooks.ts | 7 ++++ 6 files changed, 53 insertions(+), 12 deletions(-) diff --git a/services/document-updater/app/js/UpdateManager.js b/services/document-updater/app/js/UpdateManager.js index 3fb9ec0fde..6faafbab06 100644 --- a/services/document-updater/app/js/UpdateManager.js +++ b/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' ) }) diff --git a/services/document-updater/app/js/WebApiManager.js b/services/document-updater/app/js/WebApiManager.js index c94ffacf8b..8ceef1ed6d 100644 --- a/services/document-updater/app/js/WebApiManager.js +++ b/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, diff --git a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js index 4782763c98..980a985948 100644 --- a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js +++ b/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]) }) }) diff --git a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js index 8532443fdf..8073d47afb 100644 --- a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js +++ b/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 ) }) diff --git a/services/web/app/src/Features/Documents/DocumentController.mjs b/services/web/app/src/Features/Documents/DocumentController.mjs index 7ad0d781de..cdb968cd2f 100644 --- a/services/web/app/src/Features/Documents/DocumentController.mjs +++ b/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) } diff --git a/services/web/types/module-hooks.ts b/services/web/types/module-hooks.ts index caba7f7b52..6c1ac8228a 100644 --- a/services/web/types/module-hooks.ts +++ b/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