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