diff --git a/services/document-updater/app/js/RangesManager.js b/services/document-updater/app/js/RangesManager.js index c146afda60..162f161829 100644 --- a/services/document-updater/app/js/RangesManager.js +++ b/services/document-updater/app/js/RangesManager.js @@ -29,7 +29,7 @@ const RangesManager = { * @param {string[]} newDocLines - the document lines after the updates were applied * @param {object} opts * @param {boolean} [opts.historyRangesSupport] - whether history ranges support is enabled - * @returns {{ newRanges: Ranges, rangesWereCollapsed: boolean, historyUpdates: HistoryUpdate[] }} + * @returns {{ newRanges: Ranges, rangesWereCollapsed: boolean, historyUpdates: HistoryUpdate[], removedChangeIds: string[] }} */ applyUpdate(projectId, docId, ranges, updates, newDocLines, opts = {}) { if (ranges == null) { @@ -114,6 +114,9 @@ const RangesManager = { ) } const newRanges = RangesManager._getRanges(rangesTracker) + const removedChangeIds = Object.keys( + rangesTracker.getDirtyState().change.removed + ) logger.debug( { projectId, @@ -124,7 +127,7 @@ const RangesManager = { }, 'applied updates to ranges' ) - return { newRanges, rangesWereCollapsed, historyUpdates } + return { newRanges, rangesWereCollapsed, historyUpdates, removedChangeIds } }, acceptChanges(projectId, docId, changeIds, ranges, lines) { diff --git a/services/document-updater/app/js/UpdateManager.js b/services/document-updater/app/js/UpdateManager.js index e5df48575e..3fb9ec0fde 100644 --- a/services/document-updater/app/js/UpdateManager.js +++ b/services/document-updater/app/js/UpdateManager.js @@ -13,6 +13,7 @@ const Errors = require('./Errors') const DocumentManager = require('./DocumentManager') const RangesManager = require('./RangesManager') const SnapshotManager = require('./SnapshotManager') +const WebApiManager = require('./WebApiManager') const Profiler = require('./Profiler') const { isInsert, isDelete, getDocLength, computeDocHash } = require('./Utils') const HistoryOTUpdateManager = require('./HistoryOTUpdateManager') @@ -143,15 +144,19 @@ const UpdateManager = { sync: incomingUpdateVersion === previousVersion, }) - const { newRanges, rangesWereCollapsed, historyUpdates } = - RangesManager.applyUpdate( - projectId, - docId, - ranges, - appliedOps, - updatedDocLines, - { historyRangesSupport } - ) + const { + newRanges, + rangesWereCollapsed, + historyUpdates, + removedChangeIds, + } = RangesManager.applyUpdate( + projectId, + docId, + ranges, + appliedOps, + updatedDocLines, + { historyRangesSupport } + ) profile.log('RangesManager.applyUpdate', { sync: true }) await RedisManager.promises.updateDocument( @@ -197,6 +202,27 @@ const UpdateManager = { } } + // applyUpdate is not triggered by accept change operations, so any + // tracked change removed by the ops we just applied was rejected. + if (removedChangeIds.length > 0) { + // Fire-and-forget without awaiting because + // we hold the doc lock here, and the result of the + // notification doesn't affect the update + WebApiManager.promises + .notifyTrackChangesRejected( + projectId, + docId, + removedChangeIds, + update.meta?.user_id + ) + .catch(err => { + logger.warn( + { err, projectId, docId, removedChangeIds }, + 'failed to notify web of rejected track changes' + ) + }) + } + if (rangesWereCollapsed) { Metrics.inc('doc-snapshot') logger.debug( diff --git a/services/document-updater/app/js/WebApiManager.js b/services/document-updater/app/js/WebApiManager.js new file mode 100644 index 0000000000..c94ffacf8b --- /dev/null +++ b/services/document-updater/app/js/WebApiManager.js @@ -0,0 +1,39 @@ +// @ts-check +const Settings = require('@overleaf/settings') +const { fetchNothing } = require('@overleaf/fetch-utils') + +const MAX_HTTP_REQUEST_LENGTH = 5000 + +/** + * @param {string} projectId + * @param {string} docId + * @param {string[]} rejectedChangeIds + * @param {string | undefined} userId + */ +async function notifyTrackChangesRejected( + projectId, + docId, + rejectedChangeIds, + userId +) { + const url = new URL( + `/project/${projectId}/doc/${docId}/changes/reject`, + Settings.apis.web.url + ) + + await fetchNothing(url, { + method: 'POST', + json: { rejectedChangeIds, userId }, + basicAuth: { + user: Settings.apis.web.user, + password: Settings.apis.web.pass, + }, + signal: AbortSignal.timeout(MAX_HTTP_REQUEST_LENGTH), + }) +} + +module.exports = { + promises: { + notifyTrackChangesRejected, + }, +} diff --git a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js index 4053aafb01..4782763c98 100644 --- a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js +++ b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js @@ -258,6 +258,39 @@ describe('RangesManager', function () { }) }) + describe('removedChangeIds', function () { + it('should be empty when no tracked changes are removed', function () { + const result = this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.ranges, + this.updates, + this.newDocLines + ) + expect(result.removedChangeIds).to.deep.equal([]) + }) + + it('should report the ids of tracked changes that were removed by the update', function () { + // original text is "one [two ]three four five" + // [] denotes tracked deletes; rejecting it by re-inserting the + // matching text drops the tracked change. + const ranges = { + changes: makeRanges([{ d: 'two ', p: 4 }]), + } + const updates = makeUpdates([{ i: 'two ', p: 4, u: true }]) + const newDocLines = ['one two three four five'] + const result = this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + ranges, + updates, + newDocLines, + { historyRangesSupport: true } + ) + expect(result.removedChangeIds).to.deep.equal(['1']) + }) + }) + describe('with history ranges support', function () { describe('inserts among tracked deletes', function () { beforeEach(function () { diff --git a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js index 912707e01d..8532443fdf 100644 --- a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js +++ b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js @@ -74,6 +74,12 @@ describe('UpdateManager', function () { }, } + this.WebApiManager = { + promises: { + notifyTrackChangesRejected: sinon.stub().resolves(), + }, + } + this.ProjectHistoryRedisManager = { promises: { queueOps: sinon @@ -94,6 +100,7 @@ describe('UpdateManager', function () { './DocumentManager': this.DocumentManager, './RangesManager': this.RangesManager, './SnapshotManager': this.SnapshotManager, + './WebApiManager': this.WebApiManager, './Profiler': this.Profiler, './ProjectHistoryRedisManager': this.ProjectHistoryRedisManager, }, @@ -337,6 +344,7 @@ describe('UpdateManager', function () { newRanges: this.updated_ranges, rangesWereCollapsed: false, historyUpdates: this.historyUpdates, + removedChangeIds: [], }) this.ShareJsUpdateManager.promises.applyUpdate = sinon.stub().resolves({ updatedDocLines: this.updatedDocLines, @@ -468,6 +476,7 @@ describe('UpdateManager', function () { newRanges: this.updated_ranges, rangesWereCollapsed: true, historyUpdates: this.historyUpdates, + removedChangeIds: [], }) await this.UpdateManager.promises.applyUpdate( this.project_id, @@ -524,6 +533,48 @@ describe('UpdateManager', function () { ) }) }) + + describe('when tracked changes are rejected', function () { + beforeEach(async function () { + this.removedChangeIds = ['change-1', 'change-2'] + this.RangesManager.applyUpdate.returns({ + newRanges: this.updated_ranges, + rangesWereCollapsed: false, + historyUpdates: this.historyUpdates, + removedChangeIds: this.removedChangeIds, + }) + await this.UpdateManager.promises.applyUpdate( + this.project_id, + this.doc_id, + this.update + ) + }) + + it('should notify web of the rejected tracked changes', function () { + this.WebApiManager.promises.notifyTrackChangesRejected.should.have.been.calledWith( + this.project_id, + this.doc_id, + this.removedChangeIds, + this.updateMeta.user_id + ) + }) + }) + + describe('when no tracked changes are rejected', function () { + beforeEach(async function () { + await this.UpdateManager.promises.applyUpdate( + this.project_id, + this.doc_id, + this.update + ) + }) + + it('should not notify web', function () { + this.WebApiManager.promises.notifyTrackChangesRejected.called.should.equal( + false + ) + }) + }) }) describe('_adjustHistoryUpdatesMetadata', function () { diff --git a/services/web/app/src/Features/Documents/DocumentController.mjs b/services/web/app/src/Features/Documents/DocumentController.mjs index 92f848110b..7ad0d781de 100644 --- a/services/web/app/src/Features/Documents/DocumentController.mjs +++ b/services/web/app/src/Features/Documents/DocumentController.mjs @@ -105,7 +105,21 @@ async function setDocument(req, res) { res.json(result) } +async function trackChangesRejected(req, res) { + const { Project_id: projectId, doc_id: docId } = req.params + const { rejectedChangeIds, userId } = req.body + await Modules.promises.hooks.fire( + 'trackChangesRejected', + projectId, + docId, + rejectedChangeIds, + userId + ) + res.sendStatus(204) +} + export default { getDocument: expressify(getDocument), setDocument: expressify(setDocument), + trackChangesRejected: expressify(trackChangesRejected), } diff --git a/services/web/app/src/router.mjs b/services/web/app/src/router.mjs index 965ed5daa1..8e27a91563 100644 --- a/services/web/app/src/router.mjs +++ b/services/web/app/src/router.mjs @@ -941,6 +941,11 @@ async function initialize(webRouter, privateApiRouter, publicApiRouter) { AuthenticationController.requirePrivateApiAuth(), DocumentController.setDocument ) + privateApiRouter.post( + '/project/:Project_id/doc/:doc_id/changes/reject', + AuthenticationController.requirePrivateApiAuth(), + DocumentController.trackChangesRejected + ) privateApiRouter.post( '/user/:user_id/project/new',