diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index 4c20bcbbf5..e7fa040914 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -380,26 +380,29 @@ const DocumentManager = { new Errors.NotFoundError(`document not found: ${docId}`) ) } - RangesManager.acceptChanges(changeIds, ranges, (error, newRanges) => { - if (error) { - return callback(error) - } - RedisManager.updateDocument( - projectId, - docId, - lines, - version, - [], - newRanges, - {}, - error => { - if (error) { - return callback(error) - } - callback() + + let newRanges + try { + newRanges = RangesManager.acceptChanges(changeIds, ranges) + } catch (err) { + return callback(err) + } + + RedisManager.updateDocument( + projectId, + docId, + lines, + version, + [], + newRanges, + {}, + error => { + if (error) { + return callback(error) } - ) - }) + callback() + } + ) } ) }, @@ -423,26 +426,29 @@ const DocumentManager = { new Errors.NotFoundError(`document not found: ${docId}`) ) } - RangesManager.deleteComment(commentId, ranges, (error, newRanges) => { - if (error) { - return callback(error) - } - RedisManager.updateDocument( - projectId, - docId, - lines, - version, - [], - newRanges, - {}, - error => { - if (error) { - return callback(error) - } - callback() + + let newRanges + try { + newRanges = RangesManager.deleteComment(commentId, ranges) + } catch (err) { + return callback(err) + } + + RedisManager.updateDocument( + projectId, + docId, + lines, + version, + [], + newRanges, + {}, + error => { + if (error) { + return callback(error) } - ) - }) + callback() + } + ) } ) }, diff --git a/services/document-updater/app/js/RangesManager.js b/services/document-updater/app/js/RangesManager.js index b87899d2f3..0a114e7bc9 100644 --- a/services/document-updater/app/js/RangesManager.js +++ b/services/document-updater/app/js/RangesManager.js @@ -1,13 +1,3 @@ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS101: Remove unnecessary use of Array.from - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -const { promisifyAll } = require('@overleaf/promise-utils') const RangesTracker = require('@overleaf/ranges-tracker') const logger = require('@overleaf/logger') const Metrics = require('./Metrics') @@ -19,60 +9,44 @@ const RangesManager = { MAX_COMMENTS: 500, MAX_CHANGES: 2000, - applyUpdate(projectId, docId, entries, updates, newDocLines, callback) { - let error + applyUpdate(projectId, docId, entries, updates, newDocLines) { if (entries == null) { entries = {} } if (updates == null) { updates = [] } - if (callback == null) { - callback = function () {} - } const { changes, comments } = _.cloneDeep(entries) const rangesTracker = new RangesTracker(changes, comments) const [emptyRangeCountBefore, totalRangeCountBefore] = RangesManager._emptyRangesCount(rangesTracker) - for (const update of Array.from(updates)) { + for (const update of updates) { rangesTracker.track_changes = !!update.meta.tc if (update.meta.tc) { rangesTracker.setIdSeed(update.meta.tc) } - for (const op of Array.from(update.op)) { - try { - rangesTracker.applyOp(op, { - user_id: update.meta != null ? update.meta.user_id : undefined, - }) - } catch (error1) { - error = error1 - return callback(error) - } + for (const op of update.op) { + rangesTracker.applyOp(op, { user_id: update.meta?.user_id }) } } if ( - (rangesTracker.changes != null - ? rangesTracker.changes.length - : undefined) > RangesManager.MAX_CHANGES || - (rangesTracker.comments != null - ? rangesTracker.comments.length - : undefined) > RangesManager.MAX_COMMENTS + rangesTracker.changes?.length > RangesManager.MAX_CHANGES || + rangesTracker.comments?.length > RangesManager.MAX_COMMENTS ) { - return callback(new Error('too many comments or tracked changes')) + throw new Error('too many comments or tracked changes') } try { // This is a consistency check that all of our ranges and // comments still match the corresponding text rangesTracker.validate(newDocLines.join('\n')) - } catch (error2) { - error = error2 + } catch (err) { logger.error( - { err: error, projectId, docId, newDocLines, updates }, + { err, projectId, docId, newDocLines, updates }, 'error validating ranges' ) - return callback(error) + throw err } const [emptyRangeCountAfter, totalRangeCountAfter] = @@ -89,65 +63,49 @@ const RangesManager = { { status_code: rangesWereCollapsed ? 'saved' : 'unsaved' } ) } - const response = RangesManager._getRanges(rangesTracker) + const newRanges = RangesManager._getRanges(rangesTracker) logger.debug( { projectId, docId, - changesCount: - response.changes != null ? response.changes.length : undefined, - commentsCount: - response.comments != null ? response.comments.length : undefined, + changesCount: newRanges.changes?.length, + commentsCount: newRanges.comments?.length, rangesWereCollapsed, }, 'applied updates to ranges' ) - return callback(null, response, rangesWereCollapsed) + return { newRanges, rangesWereCollapsed } }, - acceptChanges(changeIds, ranges, callback) { - if (callback == null) { - callback = function () {} - } + acceptChanges(changeIds, ranges) { const { changes, comments } = ranges logger.debug(`accepting ${changeIds.length} changes in ranges`) const rangesTracker = new RangesTracker(changes, comments) rangesTracker.removeChangeIds(changeIds) - const response = RangesManager._getRanges(rangesTracker) - return callback(null, response) + const newRanges = RangesManager._getRanges(rangesTracker) + return newRanges }, - deleteComment(commentId, ranges, callback) { - if (callback == null) { - callback = function () {} - } + deleteComment(commentId, ranges) { const { changes, comments } = ranges logger.debug({ commentId }, 'deleting comment in ranges') const rangesTracker = new RangesTracker(changes, comments) rangesTracker.removeCommentId(commentId) - const response = RangesManager._getRanges(rangesTracker) - return callback(null, response) + const newRanges = RangesManager._getRanges(rangesTracker) + return newRanges }, _getRanges(rangesTracker) { // Return the minimal data structure needed, since most documents won't have any // changes or comments let response = {} - if ( - (rangesTracker.changes != null - ? rangesTracker.changes.length - : undefined) > 0 - ) { + if (rangesTracker.changes != null && rangesTracker.changes.length > 0) { if (response == null) { response = {} } response.changes = rangesTracker.changes } - if ( - (rangesTracker.comments != null - ? rangesTracker.comments.length - : undefined) > 0 - ) { + if (rangesTracker.comments != null && rangesTracker.comments.length > 0) { if (response == null) { response = {} } @@ -159,13 +117,13 @@ const RangesManager = { _emptyRangesCount(ranges) { let emptyCount = 0 let totalCount = 0 - for (const comment of Array.from(ranges.comments || [])) { + for (const comment of ranges.comments || []) { totalCount++ if (comment.op.c === '') { emptyCount++ } } - for (const change of Array.from(ranges.changes || [])) { + for (const change of ranges.changes || []) { totalCount++ if (change.op.i != null) { if (change.op.i === '') { @@ -178,9 +136,3 @@ const RangesManager = { } module.exports = RangesManager -module.exports.promises = promisifyAll(RangesManager, { - without: ['_getRanges', '_emptyRangesCount'], - multiResult: { - applyUpdate: ['newRanges', 'rangesWereCollapsed'], - }, -}) diff --git a/services/document-updater/app/js/UpdateManager.js b/services/document-updater/app/js/UpdateManager.js index a53f294df0..5c812379a3 100644 --- a/services/document-updater/app/js/UpdateManager.js +++ b/services/document-updater/app/js/UpdateManager.js @@ -117,14 +117,13 @@ const UpdateManager = { sync: incomingUpdateVersion === previousVersion, }) - const { newRanges, rangesWereCollapsed } = - await RangesManager.promises.applyUpdate( - projectId, - docId, - ranges, - appliedOps, - updatedDocLines - ) + const { newRanges, rangesWereCollapsed } = RangesManager.applyUpdate( + projectId, + docId, + ranges, + appliedOps, + updatedDocLines + ) profile.log('RangesManager.applyUpdate', { sync: true }) UpdateManager._addProjectHistoryMetadataToOps( diff --git a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js index f7aa85499b..78cc98f4d3 100644 --- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js +++ b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js @@ -731,7 +731,7 @@ describe('DocumentManager', function () { .yields(null, this.lines, this.version, this.ranges) this.RangesManager.acceptChanges = sinon .stub() - .yields(null, this.updated_ranges) + .returns(this.updated_ranges) this.RedisManager.updateDocument = sinon.stub().yields() }) @@ -830,7 +830,7 @@ describe('DocumentManager', function () { .yields(null, this.lines, this.version, this.ranges) this.RangesManager.deleteComment = sinon .stub() - .yields(null, this.updated_ranges) + .returns(this.updated_ranges) this.RedisManager.updateDocument = sinon.stub().yields() }) diff --git a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js index a211d133c6..cd70b2f59b 100644 --- a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js +++ b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js @@ -1,23 +1,12 @@ -/* eslint-disable - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS101: Remove unnecessary use of Array.from - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const sinon = require('sinon') const { expect } = require('chai') -const modulePath = '../../../../app/js/RangesManager.js' const SandboxedModule = require('sandboxed-module') +const MODULE_PATH = '../../../../app/js/RangesManager.js' + describe('RangesManager', function () { beforeEach(function () { - this.RangesManager = SandboxedModule.require(modulePath, { + this.RangesManager = SandboxedModule.require(MODULE_PATH, { requires: { '@overleaf/metrics': (this.Metrics = { histogram: sinon.stub() }), }, @@ -26,7 +15,6 @@ describe('RangesManager', function () { this.doc_id = 'doc-id-123' this.project_id = 'project-id-123' this.user_id = 'user-id-123' - return (this.callback = sinon.stub()) }) describe('applyUpdate', function () { @@ -68,33 +56,27 @@ describe('RangesManager', function () { }, ], } - return (this.newDocLines = ['one two three four five']) + this.newDocLines = ['one two three four five'] }) // old is "one three four five" describe('successfully', function () { beforeEach(function () { - return this.RangesManager.applyUpdate( + this.result = this.RangesManager.applyUpdate( this.project_id, this.doc_id, this.entries, this.updates, - this.newDocLines, - this.callback + this.newDocLines ) }) - return it('should return the modified the comments and changes', function () { - this.callback.called.should.equal(true) - const [error, entries, rangesWereCollapsed] = Array.from( - this.callback.args[0] - ) - expect(error).to.be.null - expect(rangesWereCollapsed).to.equal(false) - entries.comments[0].op.should.deep.equal({ + it('should return the modified the comments and changes', function () { + expect(this.result.rangesWereCollapsed).to.equal(false) + this.result.newRanges.comments[0].op.should.deep.equal({ c: 'three ', p: 8, }) - return entries.changes[0].op.should.deep.equal({ + this.result.newRanges.changes[0].op.should.deep.equal({ i: 'five', p: 19, }) @@ -104,44 +86,36 @@ describe('RangesManager', function () { describe('with empty comments', function () { beforeEach(function () { this.entries.comments = [] - return this.RangesManager.applyUpdate( + this.result = this.RangesManager.applyUpdate( this.project_id, this.doc_id, this.entries, this.updates, - this.newDocLines, - this.callback + this.newDocLines ) }) - return it('should return an object with no comments', function () { + it('should return an object with no comments', function () { // Save space in redis and don't store just {} - this.callback.called.should.equal(true) - const [error, entries] = Array.from(this.callback.args[0]) - expect(error).to.be.null - return expect(entries.comments).to.be.undefined + expect(this.result.newRanges.comments).to.be.undefined }) }) describe('with empty changes', function () { beforeEach(function () { this.entries.changes = [] - return this.RangesManager.applyUpdate( + this.result = this.RangesManager.applyUpdate( this.project_id, this.doc_id, this.entries, this.updates, - this.newDocLines, - this.callback + this.newDocLines ) }) - return it('should return an object with no changes', function () { + it('should return an object with no changes', function () { // Save space in redis and don't store just {} - this.callback.called.should.equal(true) - const [error, entries] = Array.from(this.callback.args[0]) - expect(error).to.be.null - return expect(entries.changes).to.be.undefined + expect(this.result.newRanges.changes).to.be.undefined }) }) @@ -187,23 +161,18 @@ describe('RangesManager', function () { ], changes: [], } - return this.RangesManager.applyUpdate( - this.project_id, - this.doc_id, - this.entries, - this.updates, - this.newDocLines, - this.callback - ) }) - return it('should return an error', function () { - this.callback.called.should.equal(true) - const [error, entries] = Array.from(this.callback.args[0]) - expect(error).to.not.be.null - return expect(error.message).to.equal( - 'too many comments or tracked changes' - ) + it('should throw an error', function () { + expect(() => { + this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.entries, + this.updates, + this.newDocLines + ) + }).to.throw('too many comments or tracked changes') }) }) @@ -248,24 +217,18 @@ describe('RangesManager', function () { comments: [], } this.newDocLines = ['one two three four'] - return this.RangesManager.applyUpdate( - this.project_id, - this.doc_id, - this.entries, - this.updates, - this.newDocLines, - this.callback - ) }) - return it('should return an error', function () { - // Save space in redis and don't store just {} - this.callback.called.should.equal(true) - const [error, entries] = Array.from(this.callback.args[0]) - expect(error).to.not.be.null - return expect(error.message).to.equal( - 'too many comments or tracked changes' - ) + it('should throw an error', function () { + expect(() => { + this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.entries, + this.updates, + this.newDocLines + ) + }).to.throw('too many comments or tracked changes') }) }) @@ -284,22 +247,18 @@ describe('RangesManager', function () { ], }, ] - return this.RangesManager.applyUpdate( - this.project_id, - this.doc_id, - this.entries, - this.updates, - this.newDocLines, - this.callback - ) }) - return it('should return an error', function () { - // Save space in redis and don't store just {} - this.callback.called.should.equal(true) - const [error, entries] = Array.from(this.callback.args[0]) - expect(error).to.not.be.null - return expect(error.message).to.equal( + it('should throw an error', function () { + expect(() => { + this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.entries, + this.updates, + this.newDocLines + ) + }).to.throw( 'Change ({"op":{"i":"five","p":15},"metadata":{"user_id":"user-id-123"}}) doesn\'t match text ("our ")' ) }) @@ -336,22 +295,17 @@ describe('RangesManager', function () { ], changes: [], } - return this.RangesManager.applyUpdate( + this.result = this.RangesManager.applyUpdate( this.project_id, this.doc_id, this.entries, this.updates, - this.newDocLines, - this.callback + this.newDocLines ) }) - return it('should return ranges_were_collapsed == true', function () { - this.callback.called.should.equal(true) - const [error, entries, rangesWereCollapsed] = Array.from( - this.callback.args[0] - ) - return expect(rangesWereCollapsed).to.equal(true) + it('should return ranges_were_collapsed == true', function () { + expect(this.result.rangesWereCollapsed).to.equal(true) }) }) @@ -396,13 +350,12 @@ describe('RangesManager', function () { }, ], } - return this.RangesManager.applyUpdate( + this.result = this.RangesManager.applyUpdate( this.project_id, this.doc_id, this.entries, this.updates, - this.newDocLines, - this.callback + this.newDocLines ) }) @@ -410,19 +363,15 @@ describe('RangesManager', function () { this.Metrics.histogram.called.should.equal(true) }) - return it('should return ranges_were_collapsed == true', function () { - this.callback.called.should.equal(true) - const [error, entries, rangesWereCollapsed] = Array.from( - this.callback.args[0] - ) - return expect(rangesWereCollapsed).to.equal(true) + it('should return ranges_were_collapsed == true', function () { + expect(this.result.rangesWereCollapsed).to.equal(true) }) }) }) - return describe('acceptChanges', function () { + describe('acceptChanges', function () { beforeEach(function () { - this.RangesManager = SandboxedModule.require(modulePath, { + this.RangesManager = SandboxedModule.require(MODULE_PATH, { requires: { '@overleaf/ranges-tracker': (this.RangesTracker = SandboxedModule.require('@overleaf/ranges-tracker')), @@ -470,118 +419,99 @@ describe('RangesManager', function () { }, ], } - return (this.removeChangeIdsSpy = sinon.spy( + this.removeChangeIdsSpy = sinon.spy( this.RangesTracker.prototype, 'removeChangeIds' - )) + ) }) describe('successfully with a single change', function () { - beforeEach(function (done) { + beforeEach(function () { this.change_ids = [this.ranges.changes[1].id] - return this.RangesManager.acceptChanges( + this.result = this.RangesManager.acceptChanges( this.change_ids, - this.ranges, - (err, ranges) => { - if (err) return done(err) - this.rangesResponse = ranges - return done() - } + this.ranges ) }) it('should log the call with the correct number of changes', function () { - return this.logger.debug + this.logger.debug .calledWith('accepting 1 changes in ranges') .should.equal(true) }) it('should delegate the change removal to the ranges tracker', function () { - return this.removeChangeIdsSpy - .calledWith(this.change_ids) - .should.equal(true) + this.removeChangeIdsSpy.calledWith(this.change_ids).should.equal(true) }) it('should remove the change', function () { - return expect( - this.rangesResponse.changes.find( + expect( + this.result.changes.find( change => change.id === this.ranges.changes[1].id ) ).to.be.undefined }) it('should return the original number of changes minus 1', function () { - return this.rangesResponse.changes.length.should.equal( - this.ranges.changes.length - 1 - ) + this.result.changes.length.should.equal(this.ranges.changes.length - 1) }) - return it('should not touch other changes', function () { - return [0, 2, 3, 4].map(i => + it('should not touch other changes', function () { + for (const i of [0, 2, 3, 4]) { expect( - this.rangesResponse.changes.find( + this.result.changes.find( change => change.id === this.ranges.changes[i].id ) ).to.deep.equal(this.ranges.changes[i]) - ) + } }) }) - return describe('successfully with multiple changes', function () { - beforeEach(function (done) { + describe('successfully with multiple changes', function () { + beforeEach(function () { this.change_ids = [ this.ranges.changes[1].id, this.ranges.changes[3].id, this.ranges.changes[4].id, ] - return this.RangesManager.acceptChanges( + this.result = this.RangesManager.acceptChanges( this.change_ids, - this.ranges, - (err, ranges) => { - if (err) return done(err) - this.rangesResponse = ranges - return done() - } + this.ranges ) }) it('should log the call with the correct number of changes', function () { - return this.logger.debug + this.logger.debug .calledWith(`accepting ${this.change_ids.length} changes in ranges`) .should.equal(true) }) it('should delegate the change removal to the ranges tracker', function () { - return this.removeChangeIdsSpy - .calledWith(this.change_ids) - .should.equal(true) + this.removeChangeIdsSpy.calledWith(this.change_ids).should.equal(true) }) it('should remove the changes', function () { - return [1, 3, 4].map( - i => - expect( - this.rangesResponse.changes.find( - change => change.id === this.ranges.changes[1].id - ) - ).to.be.undefined - ) + for (const i of [1, 3, 4]) { + expect( + this.result.changes.find( + change => change.id === this.ranges.changes[i].id + ) + ).to.be.undefined + } }) it('should return the original number of changes minus the number of accepted changes', function () { - return this.rangesResponse.changes.length.should.equal( - this.ranges.changes.length - 3 - ) + this.result.changes.length.should.equal(this.ranges.changes.length - 3) }) - return it('should not touch other changes', function () { - return [0, 2].map(i => + it('should not touch other changes', function () { + for (const i of [0, 2]) { expect( - this.rangesResponse.changes.find( + this.result.changes.find( change => change.id === this.ranges.changes[i].id ) ).to.deep.equal(this.ranges.changes[i]) - ) + } }) }) }) diff --git a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js index 932dc472db..16b9d044b3 100644 --- a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js +++ b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js @@ -65,9 +65,7 @@ describe('UpdateManager', function () { } this.RangesManager = { - promises: { - applyUpdate: sinon.stub(), - }, + applyUpdate: sinon.stub(), } this.SnapshotManager = { @@ -320,7 +318,7 @@ describe('UpdateManager', function () { pathname: this.pathname, projectHistoryId: this.projectHistoryId, }) - this.RangesManager.promises.applyUpdate.resolves({ + this.RangesManager.applyUpdate.returns({ newRanges: this.updated_ranges, rangesWereCollapsed: false, }) @@ -357,7 +355,7 @@ describe('UpdateManager', function () { }) it('should update the ranges', function () { - this.RangesManager.promises.applyUpdate + this.RangesManager.applyUpdate .calledWith( this.project_id, this.doc_id, @@ -444,7 +442,7 @@ describe('UpdateManager', function () { describe('when ranges get collapsed', function () { beforeEach(async function () { - this.RangesManager.promises.applyUpdate.resolves({ + this.RangesManager.applyUpdate.returns({ newRanges: this.updated_ranges, rangesWereCollapsed: true, })