From 924cc0bf7390b0a297595bfe9f9033c6d351b8b4 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Wed, 20 May 2020 15:31:41 -0400 Subject: [PATCH 01/10] Decaf cleanup: simplify null checks --- .../ApplyingUpdatesToProjectStructureTests.js | 51 +++++++------------ 1 file changed, 18 insertions(+), 33 deletions(-) diff --git a/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js b/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js index 793a9fa5a8..c43ad15ae6 100644 --- a/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js +++ b/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js @@ -8,7 +8,6 @@ /* * decaffeinate suggestions: * 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 sinon = require('sinon') @@ -41,7 +40,7 @@ describe("Applying updates to a project's structure", function () { } this.fileUpdates = [this.fileUpdate] return DocUpdaterApp.ensureRunning((error) => { - if (error != null) { + if (error) { throw error } return DocUpdaterClient.sendProjectUpdate( @@ -51,7 +50,7 @@ describe("Applying updates to a project's structure", function () { this.fileUpdates, this.version, (error) => { - if (error != null) { + if (error) { throw error } return setTimeout(done, 200) @@ -66,7 +65,7 @@ describe("Applying updates to a project's structure", function () { 0, -1, (error, updates) => { - if (error != null) { + if (error) { throw error } @@ -81,7 +80,6 @@ describe("Applying updates to a project's structure", function () { return done() } ) - return null }) }) @@ -105,13 +103,12 @@ describe("Applying updates to a project's structure", function () { [], this.version, (error) => { - if (error != null) { + if (error) { throw error } return setTimeout(done, 200) } ) - return null }) return it('should push the applied doc renames to the project history api', function (done) { @@ -120,7 +117,7 @@ describe("Applying updates to a project's structure", function () { 0, -1, (error, updates) => { - if (error != null) { + if (error) { throw error } @@ -135,7 +132,6 @@ describe("Applying updates to a project's structure", function () { return done() } ) - return null }) }) @@ -147,7 +143,7 @@ describe("Applying updates to a project's structure", function () { this.project_id, this.docUpdate.id, (error) => { - if (error != null) { + if (error) { throw error } sinon.spy(MockWebApi, 'getDocument') @@ -158,7 +154,7 @@ describe("Applying updates to a project's structure", function () { [], this.version, (error) => { - if (error != null) { + if (error) { throw error } return setTimeout(done, 200) @@ -166,7 +162,6 @@ describe("Applying updates to a project's structure", function () { ) } ) - return null }) after(function () { @@ -182,7 +177,6 @@ describe("Applying updates to a project's structure", function () { return done() } ) - return null }) return it('should push the applied doc renames to the project history api', function (done) { @@ -191,7 +185,7 @@ describe("Applying updates to a project's structure", function () { 0, -1, (error, updates) => { - if (error != null) { + if (error) { throw error } @@ -206,7 +200,6 @@ describe("Applying updates to a project's structure", function () { return done() } ) - return null }) }) }) @@ -247,13 +240,12 @@ describe("Applying updates to a project's structure", function () { this.fileUpdates, this.version, (error) => { - if (error != null) { + if (error) { throw error } return setTimeout(done, 200) } ) - return null }) return it('should push the applied doc renames to the project history api', function (done) { @@ -262,7 +254,7 @@ describe("Applying updates to a project's structure", function () { 0, -1, (error, updates) => { - if (error != null) { + if (error) { throw error } @@ -301,7 +293,6 @@ describe("Applying updates to a project's structure", function () { return done() } ) - return null }) }) }) @@ -322,13 +313,12 @@ describe("Applying updates to a project's structure", function () { this.fileUpdates, this.version, (error) => { - if (error != null) { + if (error) { throw error } return setTimeout(done, 200) } ) - return null }) return it('should push the file addition to the project history api', function (done) { @@ -337,7 +327,7 @@ describe("Applying updates to a project's structure", function () { 0, -1, (error, updates) => { - if (error != null) { + if (error) { throw error } @@ -352,7 +342,6 @@ describe("Applying updates to a project's structure", function () { return done() } ) - return null }) }) @@ -372,13 +361,12 @@ describe("Applying updates to a project's structure", function () { [], this.version, (error) => { - if (error != null) { + if (error) { throw error } return setTimeout(done, 200) } ) - return null }) return it('should push the doc addition to the project history api', function (done) { @@ -387,7 +375,7 @@ describe("Applying updates to a project's structure", function () { 0, -1, (error, updates) => { - if (error != null) { + if (error) { throw error } @@ -402,7 +390,6 @@ describe("Applying updates to a project's structure", function () { return done() } ) - return null }) }) @@ -434,7 +421,7 @@ describe("Applying updates to a project's structure", function () { [], this.version0, function (error) { - if (error != null) { + if (error) { throw error } return DocUpdaterClient.sendProjectUpdate( @@ -444,7 +431,7 @@ describe("Applying updates to a project's structure", function () { [], this.version1, (error) => { - if (error != null) { + if (error) { throw error } return setTimeout(done, 2000) @@ -452,7 +439,6 @@ describe("Applying updates to a project's structure", function () { ) } ) - return null }) after(function () { @@ -495,7 +481,7 @@ describe("Applying updates to a project's structure", function () { [], this.version0, function (error) { - if (error != null) { + if (error) { throw error } return DocUpdaterClient.sendProjectUpdate( @@ -505,7 +491,7 @@ describe("Applying updates to a project's structure", function () { [], this.version1, (error) => { - if (error != null) { + if (error) { throw error } return setTimeout(done, 2000) @@ -513,7 +499,6 @@ describe("Applying updates to a project's structure", function () { ) } ) - return null }) after(function () { From cfc0d45ccd46f78530272f571b3d4f81f965471a Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Wed, 20 May 2020 15:32:57 -0400 Subject: [PATCH 02/10] Decaf cleanup: unnecessary returns --- .../ApplyingUpdatesToProjectStructureTests.js | 85 +++++++++---------- 1 file changed, 39 insertions(+), 46 deletions(-) diff --git a/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js b/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js index c43ad15ae6..4efa4c63e6 100644 --- a/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js +++ b/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js @@ -3,13 +3,6 @@ handle-callback-err, no-return-assign, */ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * 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 chai = require('chai') chai.should() @@ -27,7 +20,7 @@ const DocUpdaterApp = require('./helpers/DocUpdaterApp') describe("Applying updates to a project's structure", function () { before(function () { this.user_id = 'user-id-123' - return (this.version = 1234) + this.version = 1234 }) describe('renaming a file', function () { @@ -39,11 +32,11 @@ describe("Applying updates to a project's structure", function () { newPathname: '/new-file-path' } this.fileUpdates = [this.fileUpdate] - return DocUpdaterApp.ensureRunning((error) => { + DocUpdaterApp.ensureRunning((error) => { if (error) { throw error } - return DocUpdaterClient.sendProjectUpdate( + DocUpdaterClient.sendProjectUpdate( this.project_id, this.user_id, [], @@ -53,13 +46,13 @@ describe("Applying updates to a project's structure", function () { if (error) { throw error } - return setTimeout(done, 200) + setTimeout(done, 200) } ) }) }) - return it('should push the applied file renames to the project history api', function (done) { + it('should push the applied file renames to the project history api', function (done) { rclient_project_history.lrange( ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), 0, @@ -77,7 +70,7 @@ describe("Applying updates to a project's structure", function () { update.meta.ts.should.be.a('string') update.version.should.equal(`${this.version}.0`) - return done() + done() } ) }) @@ -90,7 +83,7 @@ describe("Applying updates to a project's structure", function () { pathname: '/doc-path', newPathname: '/new-doc-path' } - return (this.docUpdates = [this.docUpdate]) + this.docUpdates = [this.docUpdate] }) describe('when the document is not loaded', function () { @@ -106,12 +99,12 @@ describe("Applying updates to a project's structure", function () { if (error) { throw error } - return setTimeout(done, 200) + setTimeout(done, 200) } ) }) - return it('should push the applied doc renames to the project history api', function (done) { + it('should push the applied doc renames to the project history api', function (done) { rclient_project_history.lrange( ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), 0, @@ -129,13 +122,13 @@ describe("Applying updates to a project's structure", function () { update.meta.ts.should.be.a('string') update.version.should.equal(`${this.version}.0`) - return done() + done() } ) }) }) - return describe('when the document is loaded', function () { + describe('when the document is loaded', function () { before(function (done) { this.project_id = DocUpdaterClient.randomId() MockWebApi.insertDoc(this.project_id, this.docUpdate.id, {}) @@ -147,7 +140,7 @@ describe("Applying updates to a project's structure", function () { throw error } sinon.spy(MockWebApi, 'getDocument') - return DocUpdaterClient.sendProjectUpdate( + DocUpdaterClient.sendProjectUpdate( this.project_id, this.user_id, this.docUpdates, @@ -157,7 +150,7 @@ describe("Applying updates to a project's structure", function () { if (error) { throw error } - return setTimeout(done, 200) + setTimeout(done, 200) } ) } @@ -165,7 +158,7 @@ describe("Applying updates to a project's structure", function () { }) after(function () { - return MockWebApi.getDocument.restore() + MockWebApi.getDocument.restore() }) it('should update the doc', function (done) { @@ -174,12 +167,12 @@ describe("Applying updates to a project's structure", function () { this.docUpdate.id, (error, res, doc) => { doc.pathname.should.equal(this.docUpdate.newPathname) - return done() + done() } ) }) - return it('should push the applied doc renames to the project history api', function (done) { + it('should push the applied doc renames to the project history api', function (done) { rclient_project_history.lrange( ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), 0, @@ -197,7 +190,7 @@ describe("Applying updates to a project's structure", function () { update.meta.ts.should.be.a('string') update.version.should.equal(`${this.version}.0`) - return done() + done() } ) }) @@ -227,10 +220,10 @@ describe("Applying updates to a project's structure", function () { pathname: '/file-path1', newPathname: '/new-file-path1' } - return (this.fileUpdates = [this.fileUpdate0, this.fileUpdate1]) + this.fileUpdates = [this.fileUpdate0, this.fileUpdate1] }) - return describe('when the documents are not loaded', function () { + describe('when the documents are not loaded', function () { before(function (done) { this.project_id = DocUpdaterClient.randomId() DocUpdaterClient.sendProjectUpdate( @@ -243,12 +236,12 @@ describe("Applying updates to a project's structure", function () { if (error) { throw error } - return setTimeout(done, 200) + setTimeout(done, 200) } ) }) - return it('should push the applied doc renames to the project history api', function (done) { + it('should push the applied doc renames to the project history api', function (done) { rclient_project_history.lrange( ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), 0, @@ -290,7 +283,7 @@ describe("Applying updates to a project's structure", function () { update.meta.ts.should.be.a('string') update.version.should.equal(`${this.version}.3`) - return done() + done() } ) }) @@ -316,12 +309,12 @@ describe("Applying updates to a project's structure", function () { if (error) { throw error } - return setTimeout(done, 200) + setTimeout(done, 200) } ) }) - return it('should push the file addition to the project history api', function (done) { + it('should push the file addition to the project history api', function (done) { rclient_project_history.lrange( ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), 0, @@ -339,7 +332,7 @@ describe("Applying updates to a project's structure", function () { update.meta.ts.should.be.a('string') update.version.should.equal(`${this.version}.0`) - return done() + done() } ) }) @@ -364,12 +357,12 @@ describe("Applying updates to a project's structure", function () { if (error) { throw error } - return setTimeout(done, 200) + setTimeout(done, 200) } ) }) - return it('should push the doc addition to the project history api', function (done) { + it('should push the doc addition to the project history api', function (done) { rclient_project_history.lrange( ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), 0, @@ -387,7 +380,7 @@ describe("Applying updates to a project's structure", function () { update.meta.ts.should.be.a('string') update.version.should.equal(`${this.version}.0`) - return done() + done() } ) }) @@ -424,7 +417,7 @@ describe("Applying updates to a project's structure", function () { if (error) { throw error } - return DocUpdaterClient.sendProjectUpdate( + DocUpdaterClient.sendProjectUpdate( projectId, userId, updates.slice(250), @@ -434,7 +427,7 @@ describe("Applying updates to a project's structure", function () { if (error) { throw error } - return setTimeout(done, 2000) + setTimeout(done, 2000) } ) } @@ -442,17 +435,17 @@ describe("Applying updates to a project's structure", function () { }) after(function () { - return MockProjectHistoryApi.flushProject.restore() + MockProjectHistoryApi.flushProject.restore() }) - return it('should flush project history', function () { - return MockProjectHistoryApi.flushProject + it('should flush project history', function () { + MockProjectHistoryApi.flushProject .calledWith(this.project_id) .should.equal(true) }) }) - return describe('with too few updates to flush to the history service', function () { + describe('with too few updates to flush to the history service', function () { before(function (done) { this.project_id = DocUpdaterClient.randomId() this.user_id = DocUpdaterClient.randomId() @@ -484,7 +477,7 @@ describe("Applying updates to a project's structure", function () { if (error) { throw error } - return DocUpdaterClient.sendProjectUpdate( + DocUpdaterClient.sendProjectUpdate( projectId, userId, updates.slice(10), @@ -494,7 +487,7 @@ describe("Applying updates to a project's structure", function () { if (error) { throw error } - return setTimeout(done, 2000) + setTimeout(done, 2000) } ) } @@ -502,11 +495,11 @@ describe("Applying updates to a project's structure", function () { }) after(function () { - return MockProjectHistoryApi.flushProject.restore() + MockProjectHistoryApi.flushProject.restore() }) - return it('should not flush project history', function () { - return MockProjectHistoryApi.flushProject + it('should not flush project history', function () { + MockProjectHistoryApi.flushProject .calledWith(this.project_id) .should.equal(false) }) From e41836028a632fdd18cab8cf323d6af24b4df1e1 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Wed, 20 May 2020 15:34:28 -0400 Subject: [PATCH 03/10] Decaf cleanup: error handling --- .../ApplyingUpdatesToProjectStructureTests.js | 41 ++++++++++--------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js b/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js index 4efa4c63e6..d9e1365a5a 100644 --- a/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js +++ b/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js @@ -1,7 +1,5 @@ /* eslint-disable camelcase, - handle-callback-err, - no-return-assign, */ const sinon = require('sinon') const chai = require('chai') @@ -34,7 +32,7 @@ describe("Applying updates to a project's structure", function () { this.fileUpdates = [this.fileUpdate] DocUpdaterApp.ensureRunning((error) => { if (error) { - throw error + return done(error) } DocUpdaterClient.sendProjectUpdate( this.project_id, @@ -44,7 +42,7 @@ describe("Applying updates to a project's structure", function () { this.version, (error) => { if (error) { - throw error + return done(error) } setTimeout(done, 200) } @@ -59,7 +57,7 @@ describe("Applying updates to a project's structure", function () { -1, (error, updates) => { if (error) { - throw error + return done(error) } const update = JSON.parse(updates[0]) @@ -97,7 +95,7 @@ describe("Applying updates to a project's structure", function () { this.version, (error) => { if (error) { - throw error + return done(error) } setTimeout(done, 200) } @@ -111,7 +109,7 @@ describe("Applying updates to a project's structure", function () { -1, (error, updates) => { if (error) { - throw error + return done(error) } const update = JSON.parse(updates[0]) @@ -137,7 +135,7 @@ describe("Applying updates to a project's structure", function () { this.docUpdate.id, (error) => { if (error) { - throw error + return done(error) } sinon.spy(MockWebApi, 'getDocument') DocUpdaterClient.sendProjectUpdate( @@ -148,7 +146,7 @@ describe("Applying updates to a project's structure", function () { this.version, (error) => { if (error) { - throw error + return done(error) } setTimeout(done, 200) } @@ -166,6 +164,9 @@ describe("Applying updates to a project's structure", function () { this.project_id, this.docUpdate.id, (error, res, doc) => { + if (error) { + return done(error) + } doc.pathname.should.equal(this.docUpdate.newPathname) done() } @@ -179,7 +180,7 @@ describe("Applying updates to a project's structure", function () { -1, (error, updates) => { if (error) { - throw error + return done(error) } const update = JSON.parse(updates[0]) @@ -234,7 +235,7 @@ describe("Applying updates to a project's structure", function () { this.version, (error) => { if (error) { - throw error + return done(error) } setTimeout(done, 200) } @@ -248,7 +249,7 @@ describe("Applying updates to a project's structure", function () { -1, (error, updates) => { if (error) { - throw error + return done(error) } let update = JSON.parse(updates[0]) @@ -307,7 +308,7 @@ describe("Applying updates to a project's structure", function () { this.version, (error) => { if (error) { - throw error + return done(error) } setTimeout(done, 200) } @@ -321,7 +322,7 @@ describe("Applying updates to a project's structure", function () { -1, (error, updates) => { if (error) { - throw error + return done(error) } const update = JSON.parse(updates[0]) @@ -355,7 +356,7 @@ describe("Applying updates to a project's structure", function () { this.version, (error) => { if (error) { - throw error + return done(error) } setTimeout(done, 200) } @@ -369,7 +370,7 @@ describe("Applying updates to a project's structure", function () { -1, (error, updates) => { if (error) { - throw error + return done(error) } const update = JSON.parse(updates[0]) @@ -415,7 +416,7 @@ describe("Applying updates to a project's structure", function () { this.version0, function (error) { if (error) { - throw error + return done(error) } DocUpdaterClient.sendProjectUpdate( projectId, @@ -425,7 +426,7 @@ describe("Applying updates to a project's structure", function () { this.version1, (error) => { if (error) { - throw error + return done(error) } setTimeout(done, 2000) } @@ -475,7 +476,7 @@ describe("Applying updates to a project's structure", function () { this.version0, function (error) { if (error) { - throw error + return done(error) } DocUpdaterClient.sendProjectUpdate( projectId, @@ -485,7 +486,7 @@ describe("Applying updates to a project's structure", function () { this.version1, (error) => { if (error) { - throw error + return done(error) } setTimeout(done, 2000) } From 3830b8029ad2c46e3eb7bd60a4409ab2b99cf5b1 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Wed, 20 May 2020 15:35:12 -0400 Subject: [PATCH 04/10] Decaf cleanup: camel case variables --- .../ApplyingUpdatesToProjectStructureTests.js | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js b/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js index d9e1365a5a..8be9291fdf 100644 --- a/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js +++ b/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js @@ -1,11 +1,8 @@ -/* eslint-disable - camelcase, -*/ const sinon = require('sinon') const chai = require('chai') chai.should() const Settings = require('settings-sharelatex') -const rclient_project_history = require('redis-sharelatex').createClient( +const rclientProjectHistory = require('redis-sharelatex').createClient( Settings.redis.project_history ) const ProjectHistoryKeys = Settings.redis.project_history.key_schema @@ -51,7 +48,7 @@ describe("Applying updates to a project's structure", function () { }) it('should push the applied file renames to the project history api', function (done) { - rclient_project_history.lrange( + rclientProjectHistory.lrange( ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), 0, -1, @@ -103,7 +100,7 @@ describe("Applying updates to a project's structure", function () { }) it('should push the applied doc renames to the project history api', function (done) { - rclient_project_history.lrange( + rclientProjectHistory.lrange( ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), 0, -1, @@ -174,7 +171,7 @@ describe("Applying updates to a project's structure", function () { }) it('should push the applied doc renames to the project history api', function (done) { - rclient_project_history.lrange( + rclientProjectHistory.lrange( ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), 0, -1, @@ -243,7 +240,7 @@ describe("Applying updates to a project's structure", function () { }) it('should push the applied doc renames to the project history api', function (done) { - rclient_project_history.lrange( + rclientProjectHistory.lrange( ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), 0, -1, @@ -316,7 +313,7 @@ describe("Applying updates to a project's structure", function () { }) it('should push the file addition to the project history api', function (done) { - rclient_project_history.lrange( + rclientProjectHistory.lrange( ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), 0, -1, @@ -364,7 +361,7 @@ describe("Applying updates to a project's structure", function () { }) it('should push the doc addition to the project history api', function (done) { - rclient_project_history.lrange( + rclientProjectHistory.lrange( ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), 0, -1, From 8bbfd25d477c8a3c7d8bf2e3a26616d4762a10bb Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Wed, 20 May 2020 15:53:06 -0400 Subject: [PATCH 05/10] Decaf cleanup: simplify null checks --- .../acceptance/js/helpers/DocUpdaterClient.js | 58 +------------------ 1 file changed, 3 insertions(+), 55 deletions(-) diff --git a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js index 53793135eb..74e04733f5 100644 --- a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js +++ b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js @@ -8,7 +8,6 @@ * 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 */ let DocUpdaterClient @@ -35,26 +34,20 @@ module.exports = DocUpdaterClient = { }, subscribeToAppliedOps(callback) { - if (callback == null) { - callback = function (message) {} - } return rclient_sub.on('message', callback) }, sendUpdate(project_id, doc_id, update, callback) { - if (callback == null) { - callback = function (error) {} - } return rclient.rpush( keys.pendingUpdates({ doc_id }), JSON.stringify(update), (error) => { - if (error != null) { + if (error) { return callback(error) } const doc_key = `${project_id}:${doc_id}` return rclient.sadd('DocsWithPendingUpdates', doc_key, (error) => { - if (error != null) { + if (error) { return callback(error) } return rclient.rpush('pending-updates-list', doc_key, callback) @@ -64,11 +57,8 @@ module.exports = DocUpdaterClient = { }, sendUpdates(project_id, doc_id, updates, callback) { - if (callback == null) { - callback = function (error) {} - } return DocUpdaterClient.preloadDoc(project_id, doc_id, (error) => { - if (error != null) { + if (error) { return callback(error) } const jobs = [] @@ -100,9 +90,6 @@ module.exports = DocUpdaterClient = { }, getDoc(project_id, doc_id, callback) { - if (callback == null) { - callback = function (error, res, body) {} - } return request.get( `http://localhost:3003/project/${project_id}/doc/${doc_id}`, (error, res, body) => { @@ -115,9 +102,6 @@ module.exports = DocUpdaterClient = { }, getDocAndRecentOps(project_id, doc_id, fromVersion, callback) { - if (callback == null) { - callback = function (error, res, body) {} - } return request.get( `http://localhost:3003/project/${project_id}/doc/${doc_id}?fromVersion=${fromVersion}`, (error, res, body) => { @@ -130,16 +114,10 @@ module.exports = DocUpdaterClient = { }, preloadDoc(project_id, doc_id, callback) { - if (callback == null) { - callback = function (error) {} - } return DocUpdaterClient.getDoc(project_id, doc_id, callback) }, flushDoc(project_id, doc_id, callback) { - if (callback == null) { - callback = function (error) {} - } return request.post( `http://localhost:3003/project/${project_id}/doc/${doc_id}/flush`, (error, res, body) => callback(error, res, body) @@ -147,9 +125,6 @@ module.exports = DocUpdaterClient = { }, setDocLines(project_id, doc_id, lines, source, user_id, undoing, callback) { - if (callback == null) { - callback = function (error) {} - } return request.post( { url: `http://localhost:3003/project/${project_id}/doc/${doc_id}`, @@ -165,9 +140,6 @@ module.exports = DocUpdaterClient = { }, deleteDoc(project_id, doc_id, callback) { - if (callback == null) { - callback = function (error) {} - } return request.del( `http://localhost:3003/project/${project_id}/doc/${doc_id}`, (error, res, body) => callback(error, res, body) @@ -175,9 +147,6 @@ module.exports = DocUpdaterClient = { }, flushProject(project_id, callback) { - if (callback == null) { - callback = function () {} - } return request.post( `http://localhost:3003/project/${project_id}/flush`, callback @@ -185,16 +154,10 @@ module.exports = DocUpdaterClient = { }, deleteProject(project_id, callback) { - if (callback == null) { - callback = function () {} - } return request.del(`http://localhost:3003/project/${project_id}`, callback) }, deleteProjectOnShutdown(project_id, callback) { - if (callback == null) { - callback = function () {} - } return request.del( `http://localhost:3003/project/${project_id}?background=true&shutdown=true`, callback @@ -202,9 +165,6 @@ module.exports = DocUpdaterClient = { }, flushOldProjects(callback) { - if (callback == null) { - callback = function () {} - } return request.get( 'http://localhost:3003/flush_queued_projects?min_delete_age=1', callback @@ -212,9 +172,6 @@ module.exports = DocUpdaterClient = { }, acceptChange(project_id, doc_id, change_id, callback) { - if (callback == null) { - callback = function () {} - } return request.post( `http://localhost:3003/project/${project_id}/doc/${doc_id}/change/${change_id}/accept`, callback @@ -222,9 +179,6 @@ module.exports = DocUpdaterClient = { }, removeComment(project_id, doc_id, comment, callback) { - if (callback == null) { - callback = function () {} - } return request.del( `http://localhost:3003/project/${project_id}/doc/${doc_id}/comment/${comment}`, callback @@ -232,9 +186,6 @@ module.exports = DocUpdaterClient = { }, getProjectDocs(project_id, projectStateHash, callback) { - if (callback == null) { - callback = function () {} - } return request.get( `http://localhost:3003/project/${project_id}/doc?state=${projectStateHash}`, (error, res, body) => { @@ -254,9 +205,6 @@ module.exports = DocUpdaterClient = { version, callback ) { - if (callback == null) { - callback = function (error) {} - } return request.post( { url: `http://localhost:3003/project/${project_id}`, From e9df9714e59d2f6af1d2ab90fc52025877868a05 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Wed, 20 May 2020 15:54:36 -0400 Subject: [PATCH 06/10] Decaf cleanup: unnecessary returns --- .../acceptance/js/helpers/DocUpdaterClient.js | 56 +++++++++---------- 1 file changed, 26 insertions(+), 30 deletions(-) diff --git a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js index 74e04733f5..89cc9cd74c 100644 --- a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js +++ b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js @@ -7,7 +7,6 @@ /* * 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 */ let DocUpdaterClient @@ -34,11 +33,11 @@ module.exports = DocUpdaterClient = { }, subscribeToAppliedOps(callback) { - return rclient_sub.on('message', callback) + rclient_sub.on('message', callback) }, sendUpdate(project_id, doc_id, update, callback) { - return rclient.rpush( + rclient.rpush( keys.pendingUpdates({ doc_id }), JSON.stringify(update), (error) => { @@ -46,18 +45,18 @@ module.exports = DocUpdaterClient = { return callback(error) } const doc_key = `${project_id}:${doc_id}` - return rclient.sadd('DocsWithPendingUpdates', doc_key, (error) => { + rclient.sadd('DocsWithPendingUpdates', doc_key, (error) => { if (error) { return callback(error) } - return rclient.rpush('pending-updates-list', doc_key, callback) + rclient.rpush('pending-updates-list', doc_key, callback) }) } ) }, sendUpdates(project_id, doc_id, updates, callback) { - return DocUpdaterClient.preloadDoc(project_id, doc_id, (error) => { + DocUpdaterClient.preloadDoc(project_id, doc_id, (error) => { if (error) { return callback(error) } @@ -68,21 +67,21 @@ module.exports = DocUpdaterClient = { DocUpdaterClient.sendUpdate(project_id, doc_id, update, callback) ))(update) } - return async.series(jobs, (err) => + async.series(jobs, (err) => DocUpdaterClient.waitForPendingUpdates(project_id, doc_id, callback) ) }) }, waitForPendingUpdates(project_id, doc_id, callback) { - return async.retry( + async.retry( { times: 30, interval: 100 }, (cb) => rclient.llen(keys.pendingUpdates({ doc_id }), (err, length) => { if (length > 0) { - return cb(new Error('updates still pending')) + cb(new Error('updates still pending')) } else { - return cb() + cb() } }), callback @@ -90,42 +89,42 @@ module.exports = DocUpdaterClient = { }, getDoc(project_id, doc_id, callback) { - return request.get( + request.get( `http://localhost:3003/project/${project_id}/doc/${doc_id}`, (error, res, body) => { if (body != null && res.statusCode >= 200 && res.statusCode < 300) { body = JSON.parse(body) } - return callback(error, res, body) + callback(error, res, body) } ) }, getDocAndRecentOps(project_id, doc_id, fromVersion, callback) { - return request.get( + request.get( `http://localhost:3003/project/${project_id}/doc/${doc_id}?fromVersion=${fromVersion}`, (error, res, body) => { if (body != null && res.statusCode >= 200 && res.statusCode < 300) { body = JSON.parse(body) } - return callback(error, res, body) + callback(error, res, body) } ) }, preloadDoc(project_id, doc_id, callback) { - return DocUpdaterClient.getDoc(project_id, doc_id, callback) + DocUpdaterClient.getDoc(project_id, doc_id, callback) }, flushDoc(project_id, doc_id, callback) { - return request.post( + request.post( `http://localhost:3003/project/${project_id}/doc/${doc_id}/flush`, (error, res, body) => callback(error, res, body) ) }, setDocLines(project_id, doc_id, lines, source, user_id, undoing, callback) { - return request.post( + request.post( { url: `http://localhost:3003/project/${project_id}/doc/${doc_id}`, json: { @@ -140,59 +139,56 @@ module.exports = DocUpdaterClient = { }, deleteDoc(project_id, doc_id, callback) { - return request.del( + request.del( `http://localhost:3003/project/${project_id}/doc/${doc_id}`, (error, res, body) => callback(error, res, body) ) }, flushProject(project_id, callback) { - return request.post( - `http://localhost:3003/project/${project_id}/flush`, - callback - ) + request.post(`http://localhost:3003/project/${project_id}/flush`, callback) }, deleteProject(project_id, callback) { - return request.del(`http://localhost:3003/project/${project_id}`, callback) + request.del(`http://localhost:3003/project/${project_id}`, callback) }, deleteProjectOnShutdown(project_id, callback) { - return request.del( + request.del( `http://localhost:3003/project/${project_id}?background=true&shutdown=true`, callback ) }, flushOldProjects(callback) { - return request.get( + request.get( 'http://localhost:3003/flush_queued_projects?min_delete_age=1', callback ) }, acceptChange(project_id, doc_id, change_id, callback) { - return request.post( + request.post( `http://localhost:3003/project/${project_id}/doc/${doc_id}/change/${change_id}/accept`, callback ) }, removeComment(project_id, doc_id, comment, callback) { - return request.del( + request.del( `http://localhost:3003/project/${project_id}/doc/${doc_id}/comment/${comment}`, callback ) }, getProjectDocs(project_id, projectStateHash, callback) { - return request.get( + request.get( `http://localhost:3003/project/${project_id}/doc?state=${projectStateHash}`, (error, res, body) => { if (body != null && res.statusCode >= 200 && res.statusCode < 300) { body = JSON.parse(body) } - return callback(error, res, body) + callback(error, res, body) } ) }, @@ -205,7 +201,7 @@ module.exports = DocUpdaterClient = { version, callback ) { - return request.post( + request.post( { url: `http://localhost:3003/project/${project_id}`, json: { userId, docUpdates, fileUpdates, version } From 05a2cf829c8e5c88697ec3cc641d5c27768aa6db Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Wed, 20 May 2020 16:06:42 -0400 Subject: [PATCH 07/10] Decaf cleanup: simplify loops --- .../acceptance/js/helpers/DocUpdaterClient.js | 36 +++++-------------- 1 file changed, 8 insertions(+), 28 deletions(-) diff --git a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js index 89cc9cd74c..ebd1a5b1a0 100644 --- a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js +++ b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js @@ -2,13 +2,6 @@ camelcase, handle-callback-err, */ -// 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 - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ let DocUpdaterClient const Settings = require('settings-sharelatex') const rclient = require('redis-sharelatex').createClient( @@ -26,10 +19,11 @@ rclient_sub.setMaxListeners(0) module.exports = DocUpdaterClient = { randomId() { - const chars = __range__(1, 24, true).map( - (i) => Math.random().toString(16)[2] - ) - return chars.join('') + let str = '' + for (let i = 0; i < 24; i++) { + str += Math.floor(Math.random() * 16).toString(16) + } + return str }, subscribeToAppliedOps(callback) { @@ -60,13 +54,9 @@ module.exports = DocUpdaterClient = { if (error) { return callback(error) } - const jobs = [] - for (const update of Array.from(updates)) { - ;((update) => - jobs.push((callback) => - DocUpdaterClient.sendUpdate(project_id, doc_id, update, callback) - ))(update) - } + const jobs = updates.map((update) => (callback) => { + DocUpdaterClient.sendUpdate(project_id, doc_id, update, callback) + }) async.series(jobs, (err) => DocUpdaterClient.waitForPendingUpdates(project_id, doc_id, callback) ) @@ -210,13 +200,3 @@ module.exports = DocUpdaterClient = { ) } } - -function __range__(left, right, inclusive) { - const range = [] - const ascending = left < right - const end = !inclusive ? right : ascending ? right + 1 : right - 1 - for (let i = left; ascending ? i < end : i > end; ascending ? i++ : i--) { - range.push(i) - } - return range -} From abb7e8fa20908700c839b2600dd8b3489e43d56a Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Wed, 20 May 2020 16:08:03 -0400 Subject: [PATCH 08/10] Decaf cleanup: error handling --- .../test/acceptance/js/helpers/DocUpdaterClient.js | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js index ebd1a5b1a0..a3b15942d5 100644 --- a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js +++ b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js @@ -1,6 +1,5 @@ /* eslint-disable camelcase, - handle-callback-err, */ let DocUpdaterClient const Settings = require('settings-sharelatex') @@ -57,9 +56,12 @@ module.exports = DocUpdaterClient = { const jobs = updates.map((update) => (callback) => { DocUpdaterClient.sendUpdate(project_id, doc_id, update, callback) }) - async.series(jobs, (err) => + async.series(jobs, (err) => { + if (err) { + return callback(err) + } DocUpdaterClient.waitForPendingUpdates(project_id, doc_id, callback) - ) + }) }) }, @@ -68,6 +70,9 @@ module.exports = DocUpdaterClient = { { times: 30, interval: 100 }, (cb) => rclient.llen(keys.pendingUpdates({ doc_id }), (err, length) => { + if (err) { + return cb(err) + } if (length > 0) { cb(new Error('updates still pending')) } else { From c018fee72c192e58e9ac79cc5f21108154236b42 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Wed, 20 May 2020 16:12:27 -0400 Subject: [PATCH 09/10] Decaf cleanup: camel case variables --- .../acceptance/js/helpers/DocUpdaterClient.js | 87 +++++++++---------- 1 file changed, 42 insertions(+), 45 deletions(-) diff --git a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js index a3b15942d5..10813e72fa 100644 --- a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js +++ b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js @@ -1,6 +1,3 @@ -/* eslint-disable - camelcase, -*/ let DocUpdaterClient const Settings = require('settings-sharelatex') const rclient = require('redis-sharelatex').createClient( @@ -10,11 +7,11 @@ const keys = Settings.redis.documentupdater.key_schema const request = require('request').defaults({ jar: false }) const async = require('async') -const rclient_sub = require('redis-sharelatex').createClient( +const rclientSub = require('redis-sharelatex').createClient( Settings.redis.pubsub ) -rclient_sub.subscribe('applied-ops') -rclient_sub.setMaxListeners(0) +rclientSub.subscribe('applied-ops') +rclientSub.setMaxListeners(0) module.exports = DocUpdaterClient = { randomId() { @@ -26,50 +23,50 @@ module.exports = DocUpdaterClient = { }, subscribeToAppliedOps(callback) { - rclient_sub.on('message', callback) + rclientSub.on('message', callback) }, - sendUpdate(project_id, doc_id, update, callback) { + sendUpdate(projectId, docId, update, callback) { rclient.rpush( - keys.pendingUpdates({ doc_id }), + keys.pendingUpdates({ doc_id: docId }), JSON.stringify(update), (error) => { if (error) { return callback(error) } - const doc_key = `${project_id}:${doc_id}` - rclient.sadd('DocsWithPendingUpdates', doc_key, (error) => { + const docKey = `${projectId}:${docId}` + rclient.sadd('DocsWithPendingUpdates', docKey, (error) => { if (error) { return callback(error) } - rclient.rpush('pending-updates-list', doc_key, callback) + rclient.rpush('pending-updates-list', docKey, callback) }) } ) }, - sendUpdates(project_id, doc_id, updates, callback) { - DocUpdaterClient.preloadDoc(project_id, doc_id, (error) => { + sendUpdates(projectId, docId, updates, callback) { + DocUpdaterClient.preloadDoc(projectId, docId, (error) => { if (error) { return callback(error) } const jobs = updates.map((update) => (callback) => { - DocUpdaterClient.sendUpdate(project_id, doc_id, update, callback) + DocUpdaterClient.sendUpdate(projectId, docId, update, callback) }) async.series(jobs, (err) => { if (err) { return callback(err) } - DocUpdaterClient.waitForPendingUpdates(project_id, doc_id, callback) + DocUpdaterClient.waitForPendingUpdates(projectId, docId, callback) }) }) }, - waitForPendingUpdates(project_id, doc_id, callback) { + waitForPendingUpdates(projectId, docId, callback) { async.retry( { times: 30, interval: 100 }, (cb) => - rclient.llen(keys.pendingUpdates({ doc_id }), (err, length) => { + rclient.llen(keys.pendingUpdates({ doc_id: docId }), (err, length) => { if (err) { return cb(err) } @@ -83,9 +80,9 @@ module.exports = DocUpdaterClient = { ) }, - getDoc(project_id, doc_id, callback) { + getDoc(projectId, docId, callback) { request.get( - `http://localhost:3003/project/${project_id}/doc/${doc_id}`, + `http://localhost:3003/project/${projectId}/doc/${docId}`, (error, res, body) => { if (body != null && res.statusCode >= 200 && res.statusCode < 300) { body = JSON.parse(body) @@ -95,9 +92,9 @@ module.exports = DocUpdaterClient = { ) }, - getDocAndRecentOps(project_id, doc_id, fromVersion, callback) { + getDocAndRecentOps(projectId, docId, fromVersion, callback) { request.get( - `http://localhost:3003/project/${project_id}/doc/${doc_id}?fromVersion=${fromVersion}`, + `http://localhost:3003/project/${projectId}/doc/${docId}?fromVersion=${fromVersion}`, (error, res, body) => { if (body != null && res.statusCode >= 200 && res.statusCode < 300) { body = JSON.parse(body) @@ -107,25 +104,25 @@ module.exports = DocUpdaterClient = { ) }, - preloadDoc(project_id, doc_id, callback) { - DocUpdaterClient.getDoc(project_id, doc_id, callback) + preloadDoc(projectId, docId, callback) { + DocUpdaterClient.getDoc(projectId, docId, callback) }, - flushDoc(project_id, doc_id, callback) { + flushDoc(projectId, docId, callback) { request.post( - `http://localhost:3003/project/${project_id}/doc/${doc_id}/flush`, + `http://localhost:3003/project/${projectId}/doc/${docId}/flush`, (error, res, body) => callback(error, res, body) ) }, - setDocLines(project_id, doc_id, lines, source, user_id, undoing, callback) { + setDocLines(projectId, docId, lines, source, userId, undoing, callback) { request.post( { - url: `http://localhost:3003/project/${project_id}/doc/${doc_id}`, + url: `http://localhost:3003/project/${projectId}/doc/${docId}`, json: { lines, source, - user_id, + user_id: userId, undoing } }, @@ -133,24 +130,24 @@ module.exports = DocUpdaterClient = { ) }, - deleteDoc(project_id, doc_id, callback) { + deleteDoc(projectId, docId, callback) { request.del( - `http://localhost:3003/project/${project_id}/doc/${doc_id}`, + `http://localhost:3003/project/${projectId}/doc/${docId}`, (error, res, body) => callback(error, res, body) ) }, - flushProject(project_id, callback) { - request.post(`http://localhost:3003/project/${project_id}/flush`, callback) + flushProject(projectId, callback) { + request.post(`http://localhost:3003/project/${projectId}/flush`, callback) }, - deleteProject(project_id, callback) { - request.del(`http://localhost:3003/project/${project_id}`, callback) + deleteProject(projectId, callback) { + request.del(`http://localhost:3003/project/${projectId}`, callback) }, - deleteProjectOnShutdown(project_id, callback) { + deleteProjectOnShutdown(projectId, callback) { request.del( - `http://localhost:3003/project/${project_id}?background=true&shutdown=true`, + `http://localhost:3003/project/${projectId}?background=true&shutdown=true`, callback ) }, @@ -162,23 +159,23 @@ module.exports = DocUpdaterClient = { ) }, - acceptChange(project_id, doc_id, change_id, callback) { + acceptChange(projectId, docId, changeId, callback) { request.post( - `http://localhost:3003/project/${project_id}/doc/${doc_id}/change/${change_id}/accept`, + `http://localhost:3003/project/${projectId}/doc/${docId}/change/${changeId}/accept`, callback ) }, - removeComment(project_id, doc_id, comment, callback) { + removeComment(projectId, docId, comment, callback) { request.del( - `http://localhost:3003/project/${project_id}/doc/${doc_id}/comment/${comment}`, + `http://localhost:3003/project/${projectId}/doc/${docId}/comment/${comment}`, callback ) }, - getProjectDocs(project_id, projectStateHash, callback) { + getProjectDocs(projectId, projectStateHash, callback) { request.get( - `http://localhost:3003/project/${project_id}/doc?state=${projectStateHash}`, + `http://localhost:3003/project/${projectId}/doc?state=${projectStateHash}`, (error, res, body) => { if (body != null && res.statusCode >= 200 && res.statusCode < 300) { body = JSON.parse(body) @@ -189,7 +186,7 @@ module.exports = DocUpdaterClient = { }, sendProjectUpdate( - project_id, + projectId, userId, docUpdates, fileUpdates, @@ -198,7 +195,7 @@ module.exports = DocUpdaterClient = { ) { request.post( { - url: `http://localhost:3003/project/${project_id}`, + url: `http://localhost:3003/project/${projectId}`, json: { userId, docUpdates, fileUpdates, version } }, (error, res, body) => callback(error, res, body) From 1d1f2040214f119f5199655aa9c41724958cfc10 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Wed, 20 May 2020 16:26:22 -0400 Subject: [PATCH 10/10] Remove backwards-compat project update API The project update endpoint accepted updates both in two array params: docUpdates and fileUpdates, and in a single array: updates. This commit removes the docUpdates/fileUpdates params now that web uses the updates param. --- .../document-updater/app/js/HttpController.js | 41 +-------- .../ApplyingUpdatesToProjectStructureTests.js | 62 ++++++------- .../acceptance/js/helpers/DocUpdaterClient.js | 11 +-- .../js/HttpController/HttpControllerTests.js | 87 +------------------ 4 files changed, 39 insertions(+), 162 deletions(-) diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index d2904eb898..5e47cf5bf1 100644 --- a/services/document-updater/app/js/HttpController.js +++ b/services/document-updater/app/js/HttpController.js @@ -325,28 +325,13 @@ function deleteComment(req, res, next) { function updateProject(req, res, next) { const timer = new Metrics.Timer('http.updateProject') const projectId = req.params.project_id - const { - projectHistoryId, - userId, - docUpdates, - fileUpdates, - updates, - version - } = req.body - logger.log( - { projectId, updates, docUpdates, fileUpdates, version }, - 'updating project via http' - ) - const allUpdates = _mergeUpdates( - docUpdates || [], - fileUpdates || [], - updates || [] - ) + const { projectHistoryId, userId, updates = [], version } = req.body + logger.log({ projectId, updates, version }, 'updating project via http') ProjectManager.updateProjectWithLocks( projectId, projectHistoryId, userId, - allUpdates, + updates, version, (error) => { timer.done() @@ -416,23 +401,3 @@ function flushQueuedProjects(req, res, next) { } }) } - -/** - * Merge updates from the previous project update interface (docUpdates + - * fileUpdates) and the new update interface (updates). - */ -function _mergeUpdates(docUpdates, fileUpdates, updates) { - const mergedUpdates = [] - for (const update of docUpdates) { - const type = update.docLines != null ? 'add-doc' : 'rename-doc' - mergedUpdates.push({ type, ...update }) - } - for (const update of fileUpdates) { - const type = update.url != null ? 'add-file' : 'rename-file' - mergedUpdates.push({ type, ...update }) - } - for (const update of updates) { - mergedUpdates.push(update) - } - return mergedUpdates -} diff --git a/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js b/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js index 8be9291fdf..58fe5d13eb 100644 --- a/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js +++ b/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js @@ -22,11 +22,12 @@ describe("Applying updates to a project's structure", function () { before(function (done) { this.project_id = DocUpdaterClient.randomId() this.fileUpdate = { + type: 'rename-file', id: DocUpdaterClient.randomId(), pathname: '/file-path', newPathname: '/new-file-path' } - this.fileUpdates = [this.fileUpdate] + this.updates = [this.fileUpdate] DocUpdaterApp.ensureRunning((error) => { if (error) { return done(error) @@ -34,8 +35,7 @@ describe("Applying updates to a project's structure", function () { DocUpdaterClient.sendProjectUpdate( this.project_id, this.user_id, - [], - this.fileUpdates, + this.updates, this.version, (error) => { if (error) { @@ -73,12 +73,13 @@ describe("Applying updates to a project's structure", function () { describe('renaming a document', function () { before(function () { - this.docUpdate = { + this.update = { + type: 'rename-doc', id: DocUpdaterClient.randomId(), pathname: '/doc-path', newPathname: '/new-doc-path' } - this.docUpdates = [this.docUpdate] + this.updates = [this.update] }) describe('when the document is not loaded', function () { @@ -87,8 +88,7 @@ describe("Applying updates to a project's structure", function () { DocUpdaterClient.sendProjectUpdate( this.project_id, this.user_id, - this.docUpdates, - [], + this.updates, this.version, (error) => { if (error) { @@ -110,7 +110,7 @@ describe("Applying updates to a project's structure", function () { } const update = JSON.parse(updates[0]) - update.doc.should.equal(this.docUpdate.id) + update.doc.should.equal(this.update.id) update.pathname.should.equal('/doc-path') update.new_pathname.should.equal('/new-doc-path') update.meta.user_id.should.equal(this.user_id) @@ -126,10 +126,10 @@ describe("Applying updates to a project's structure", function () { describe('when the document is loaded', function () { before(function (done) { this.project_id = DocUpdaterClient.randomId() - MockWebApi.insertDoc(this.project_id, this.docUpdate.id, {}) + MockWebApi.insertDoc(this.project_id, this.update.id, {}) DocUpdaterClient.preloadDoc( this.project_id, - this.docUpdate.id, + this.update.id, (error) => { if (error) { return done(error) @@ -138,8 +138,7 @@ describe("Applying updates to a project's structure", function () { DocUpdaterClient.sendProjectUpdate( this.project_id, this.user_id, - this.docUpdates, - [], + this.updates, this.version, (error) => { if (error) { @@ -159,12 +158,12 @@ describe("Applying updates to a project's structure", function () { it('should update the doc', function (done) { DocUpdaterClient.getDoc( this.project_id, - this.docUpdate.id, + this.update.id, (error, res, doc) => { if (error) { return done(error) } - doc.pathname.should.equal(this.docUpdate.newPathname) + doc.pathname.should.equal(this.update.newPathname) done() } ) @@ -181,7 +180,7 @@ describe("Applying updates to a project's structure", function () { } const update = JSON.parse(updates[0]) - update.doc.should.equal(this.docUpdate.id) + update.doc.should.equal(this.update.id) update.pathname.should.equal('/doc-path') update.new_pathname.should.equal('/new-doc-path') update.meta.user_id.should.equal(this.user_id) @@ -198,27 +197,35 @@ describe("Applying updates to a project's structure", function () { describe('renaming multiple documents and files', function () { before(function () { this.docUpdate0 = { + type: 'rename-doc', id: DocUpdaterClient.randomId(), pathname: '/doc-path0', newPathname: '/new-doc-path0' } this.docUpdate1 = { + type: 'rename-doc', id: DocUpdaterClient.randomId(), pathname: '/doc-path1', newPathname: '/new-doc-path1' } - this.docUpdates = [this.docUpdate0, this.docUpdate1] this.fileUpdate0 = { + type: 'rename-file', id: DocUpdaterClient.randomId(), pathname: '/file-path0', newPathname: '/new-file-path0' } this.fileUpdate1 = { + type: 'rename-file', id: DocUpdaterClient.randomId(), pathname: '/file-path1', newPathname: '/new-file-path1' } - this.fileUpdates = [this.fileUpdate0, this.fileUpdate1] + this.updates = [ + this.docUpdate0, + this.docUpdate1, + this.fileUpdate0, + this.fileUpdate1 + ] }) describe('when the documents are not loaded', function () { @@ -227,8 +234,7 @@ describe("Applying updates to a project's structure", function () { DocUpdaterClient.sendProjectUpdate( this.project_id, this.user_id, - this.docUpdates, - this.fileUpdates, + this.updates, this.version, (error) => { if (error) { @@ -292,16 +298,16 @@ describe("Applying updates to a project's structure", function () { before(function (done) { this.project_id = DocUpdaterClient.randomId() this.fileUpdate = { + type: 'add-file', id: DocUpdaterClient.randomId(), pathname: '/file-path', url: 'filestore.example.com' } - this.fileUpdates = [this.fileUpdate] + this.updates = [this.fileUpdate] DocUpdaterClient.sendProjectUpdate( this.project_id, this.user_id, - [], - this.fileUpdates, + this.updates, this.version, (error) => { if (error) { @@ -340,16 +346,16 @@ describe("Applying updates to a project's structure", function () { before(function (done) { this.project_id = DocUpdaterClient.randomId() this.docUpdate = { + type: 'add-doc', id: DocUpdaterClient.randomId(), pathname: '/file-path', docLines: 'a\nb' } - this.docUpdates = [this.docUpdate] + this.updates = [this.docUpdate] DocUpdaterClient.sendProjectUpdate( this.project_id, this.user_id, - this.docUpdates, - [], + this.updates, this.version, (error) => { if (error) { @@ -394,6 +400,7 @@ describe("Applying updates to a project's structure", function () { for (let v = 0; v <= 599; v++) { // Should flush after 500 ops updates.push({ + type: 'add-doc', id: DocUpdaterClient.randomId(), pathname: '/file-' + v, docLines: 'a\nb' @@ -409,7 +416,6 @@ describe("Applying updates to a project's structure", function () { projectId, userId, updates.slice(0, 250), - [], this.version0, function (error) { if (error) { @@ -419,7 +425,6 @@ describe("Applying updates to a project's structure", function () { projectId, userId, updates.slice(250), - [], this.version1, (error) => { if (error) { @@ -454,6 +459,7 @@ describe("Applying updates to a project's structure", function () { for (let v = 0; v <= 42; v++) { // Should flush after 500 ops updates.push({ + type: 'add-doc', id: DocUpdaterClient.randomId(), pathname: '/file-' + v, docLines: 'a\nb' @@ -469,7 +475,6 @@ describe("Applying updates to a project's structure", function () { projectId, userId, updates.slice(0, 10), - [], this.version0, function (error) { if (error) { @@ -479,7 +484,6 @@ describe("Applying updates to a project's structure", function () { projectId, userId, updates.slice(10), - [], this.version1, (error) => { if (error) { diff --git a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js index 10813e72fa..9e0ee6462f 100644 --- a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js +++ b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js @@ -185,18 +185,11 @@ module.exports = DocUpdaterClient = { ) }, - sendProjectUpdate( - projectId, - userId, - docUpdates, - fileUpdates, - version, - callback - ) { + sendProjectUpdate(projectId, userId, updates, version, callback) { request.post( { url: `http://localhost:3003/project/${projectId}`, - json: { userId, docUpdates, fileUpdates, version } + json: { userId, updates, version } }, (error, res, body) => callback(error, res, body) ) diff --git a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js index 4d9790926a..07e9d93c9a 100644 --- a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js +++ b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js @@ -809,92 +809,7 @@ describe('HttpController', function () { }) }) - describe('updateProject (split doc and file updates)', function () { - beforeEach(function () { - this.projectHistoryId = 'history-id-123' - this.userId = 'user-id-123' - this.docUpdates = [ - { id: 1, pathname: 'thesis.tex', newPathname: 'book.tex' }, - { id: 2, pathname: 'article.tex', docLines: 'hello' } - ] - this.fileUpdates = [ - { id: 3, pathname: 'apple.png', newPathname: 'banana.png' }, - { id: 4, url: 'filestore.example.com/4' } - ] - this.expectedUpdates = [ - { - type: 'rename-doc', - id: 1, - pathname: 'thesis.tex', - newPathname: 'book.tex' - }, - { type: 'add-doc', id: 2, pathname: 'article.tex', docLines: 'hello' }, - { - type: 'rename-file', - id: 3, - pathname: 'apple.png', - newPathname: 'banana.png' - }, - { type: 'add-file', id: 4, url: 'filestore.example.com/4' } - ] - this.version = 1234567 - this.req = { - query: {}, - body: { - projectHistoryId: this.projectHistoryId, - userId: this.userId, - docUpdates: this.docUpdates, - fileUpdates: this.fileUpdates, - version: this.version - }, - params: { - project_id: this.project_id - } - } - }) - - describe('successfully', function () { - beforeEach(function () { - this.ProjectManager.updateProjectWithLocks = sinon.stub().yields() - this.HttpController.updateProject(this.req, this.res, this.next) - }) - - it('should accept the change', function () { - this.ProjectManager.updateProjectWithLocks - .calledWith( - this.project_id, - this.projectHistoryId, - this.userId, - this.expectedUpdates, - this.version - ) - .should.equal(true) - }) - - it('should return a successful No Content response', function () { - this.res.sendStatus.calledWith(204).should.equal(true) - }) - - it('should time the request', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) - }) - }) - - describe('when an errors occurs', function () { - beforeEach(function () { - this.ProjectManager.updateProjectWithLocks = sinon - .stub() - .yields(new Error('oops')) - this.HttpController.updateProject(this.req, this.res, this.next) - }) - - it('should call next with the error', function () { - this.next.calledWith(sinon.match.instanceOf(Error)).should.equal(true) - }) - }) - }) - - describe('updateProject (single updates parameter)', function () { + describe('updateProject', function () { beforeEach(function () { this.projectHistoryId = 'history-id-123' this.userId = 'user-id-123'