From d530ee9501eba8e51cf6a30cefa2f3b5811f73c7 Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Thu, 4 May 2017 15:32:54 +0100 Subject: [PATCH 01/11] Add methods to bulk accept changes. --- services/document-updater/app.coffee | 15 +++++++------- .../app/coffee/DocumentManager.coffee | 20 +++++++++++++++++++ .../app/coffee/HttpController.coffee | 12 +++++++++++ .../app/coffee/RangesManager.coffee | 9 +++++++++ 4 files changed, 49 insertions(+), 7 deletions(-) diff --git a/services/document-updater/app.coffee b/services/document-updater/app.coffee index 36c0cb3a72..1672d1b3b5 100644 --- a/services/document-updater/app.coffee +++ b/services/document-updater/app.coffee @@ -42,14 +42,15 @@ app.param 'doc_id', (req, res, next, doc_id) -> else next new Error("invalid doc id") -app.get '/project/:project_id/doc/:doc_id', HttpController.getDoc -app.post '/project/:project_id/doc/:doc_id', HttpController.setDoc -app.post '/project/:project_id/doc/:doc_id/flush', HttpController.flushDocIfLoaded -app.delete '/project/:project_id/doc/:doc_id', HttpController.flushAndDeleteDoc -app.delete '/project/:project_id', HttpController.deleteProject -app.post '/project/:project_id/flush', HttpController.flushProject +app.get '/project/:project_id/doc/:doc_id', HttpController.getDoc +app.post '/project/:project_id/doc/:doc_id', HttpController.setDoc +app.post '/project/:project_id/doc/:doc_id/flush', HttpController.flushDocIfLoaded +app.delete '/project/:project_id/doc/:doc_id', HttpController.flushAndDeleteDoc +app.delete '/project/:project_id', HttpController.deleteProject +app.post '/project/:project_id/flush', HttpController.flushProject app.post '/project/:project_id/doc/:doc_id/change/:change_id/accept', HttpController.acceptChange -app.del '/project/:project_id/doc/:doc_id/comment/:comment_id', HttpController.deleteComment +app.post '/project/:project_id/doc/:doc_id/change/accept', HttpController.bulkAcceptChanges +app.del '/project/:project_id/doc/:doc_id/comment/:comment_id', HttpController.deleteComment app.get '/total', (req, res)-> timer = new Metrics.Timer("http.allDocList") diff --git a/services/document-updater/app/coffee/DocumentManager.coffee b/services/document-updater/app/coffee/DocumentManager.coffee index be47ec4c8c..a327f04af4 100644 --- a/services/document-updater/app/coffee/DocumentManager.coffee +++ b/services/document-updater/app/coffee/DocumentManager.coffee @@ -139,6 +139,22 @@ module.exports = DocumentManager = RedisManager.updateDocument doc_id, lines, version, [], new_ranges, (error) -> return callback(error) if error? callback() + + bulkAcceptChanges: (project_id, doc_id, change_ids, _callback = (error) ->) -> + timer = new Metrics.Timer("docManager.bulkAcceptChanges") + callback = (args...) -> + timer.done() + _callback(args...) + + DocumentManager.getDoc project_id, doc_id, (error, lines, version, ranges) -> + return callback(error) if error? + if !lines? or !version? + return callback(new Errors.NotFoundError("document not found: #{doc_id}")) + RangesManager.bulkAcceptChanges change_ids, ranges, (error, new_ranges) -> + return callback(error) if error? + RedisManager.updateDocument doc_id, lines, version, [], new_ranges, (error) -> + return callback(error) if error? + callback() deleteComment: (project_id, doc_id, comment_id, _callback = (error) ->) -> timer = new Metrics.Timer("docManager.deleteComment") @@ -180,6 +196,10 @@ module.exports = DocumentManager = UpdateManager = require "./UpdateManager" UpdateManager.lockUpdatesAndDo DocumentManager.acceptChange, project_id, doc_id, change_id, callback + bulkAcceptChangesWithLock: (project_id, doc_id, change_ids, callback = (error) ->) -> + UpdateManager = require "./UpdateManager" + UpdateManager.lockUpdatesAndDo DocumentManager.bulkAcceptChanges, project_id, doc_id, change_ids, callback + deleteCommentWithLock: (project_id, doc_id, thread_id, callback = (error) ->) -> UpdateManager = require "./UpdateManager" UpdateManager.lockUpdatesAndDo DocumentManager.deleteComment, project_id, doc_id, thread_id, callback diff --git a/services/document-updater/app/coffee/HttpController.coffee b/services/document-updater/app/coffee/HttpController.coffee index aae2b51f8e..075d1f8e40 100644 --- a/services/document-updater/app/coffee/HttpController.coffee +++ b/services/document-updater/app/coffee/HttpController.coffee @@ -105,6 +105,18 @@ module.exports = HttpController = return next(error) if error? logger.log {project_id, doc_id, change_id}, "accepted change via http" res.send 204 # No Content + + bulkAcceptChanges: (req, res, next = (error) ->) -> + {project_id, doc_id} = req.params + {change_ids} = req.body + change_ids ?= [] + logger.log {project_id, doc_id}, "accepting #{ change_ids.length } changes via http" + timer = new Metrics.Timer("http.bulkAcceptChanges") + DocumentManager.bulkAcceptChangesWithLock project_id, doc_id, change_ids, (error) -> + timer.done() + return next(error) if error? + logger.log {project_id, doc_id}, "accepted #{ change_ids.length } changes via http" + res.send 204 # No Content deleteComment: (req, res, next = (error) ->) -> {project_id, doc_id, comment_id} = req.params diff --git a/services/document-updater/app/coffee/RangesManager.coffee b/services/document-updater/app/coffee/RangesManager.coffee index ebef566424..559cd18da4 100644 --- a/services/document-updater/app/coffee/RangesManager.coffee +++ b/services/document-updater/app/coffee/RangesManager.coffee @@ -41,6 +41,15 @@ module.exports = RangesManager = response = RangesManager._getRanges(rangesTracker) callback null, response + bulkAcceptChanges: (change_ids, ranges, callback = (error, ranges) ->) -> + {changes, comments} = ranges + logger.log {change_id}, "accepting #{ change_ids.length } changes in ranges" + rangesTracker = new RangesTracker(changes, comments) + for change_id in change_ids + rangesTracker.removeChangeId(change_id) + response = RangesManager._getRanges(rangesTracker) + callback null, response + deleteComment: (comment_id, ranges, callback = (error, ranges) ->) -> {changes, comments} = ranges logger.log {comment_id}, "deleting comment in ranges" From 9b8a2c3030fa0354d1a56fc7ccb6e9e3f9fd8462 Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Fri, 5 May 2017 15:12:06 +0100 Subject: [PATCH 02/11] Consolidate methods to accept changes. --- services/document-updater/app.coffee | 4 +-- .../app/coffee/DocumentManager.coffee | 32 ++++--------------- .../app/coffee/HttpController.coffee | 11 ++++--- .../app/coffee/RangesManager.coffee | 10 +----- 4 files changed, 15 insertions(+), 42 deletions(-) diff --git a/services/document-updater/app.coffee b/services/document-updater/app.coffee index 1672d1b3b5..9e96d2b859 100644 --- a/services/document-updater/app.coffee +++ b/services/document-updater/app.coffee @@ -48,8 +48,8 @@ app.post '/project/:project_id/doc/:doc_id/flush', HttpCont app.delete '/project/:project_id/doc/:doc_id', HttpController.flushAndDeleteDoc app.delete '/project/:project_id', HttpController.deleteProject app.post '/project/:project_id/flush', HttpController.flushProject -app.post '/project/:project_id/doc/:doc_id/change/:change_id/accept', HttpController.acceptChange -app.post '/project/:project_id/doc/:doc_id/change/accept', HttpController.bulkAcceptChanges +app.post '/project/:project_id/doc/:doc_id/change/:change_id/accept', HttpController.acceptChanges +app.post '/project/:project_id/doc/:doc_id/change/accept', HttpController.acceptChanges app.del '/project/:project_id/doc/:doc_id/comment/:comment_id', HttpController.deleteComment app.get '/total', (req, res)-> diff --git a/services/document-updater/app/coffee/DocumentManager.coffee b/services/document-updater/app/coffee/DocumentManager.coffee index a327f04af4..a608331003 100644 --- a/services/document-updater/app/coffee/DocumentManager.coffee +++ b/services/document-updater/app/coffee/DocumentManager.coffee @@ -123,9 +123,9 @@ module.exports = DocumentManager = RedisManager.removeDocFromMemory project_id, doc_id, (error) -> return callback(error) if error? callback null - - acceptChange: (project_id, doc_id, change_id, _callback = (error) ->) -> - timer = new Metrics.Timer("docManager.acceptChange") + + acceptChanges: (project_id, doc_id, change_ids = [], _callback = (error) ->) -> + timer = new Metrics.Timer("docManager.acceptChanges") callback = (args...) -> timer.done() _callback(args...) @@ -134,23 +134,7 @@ module.exports = DocumentManager = return callback(error) if error? if !lines? or !version? return callback(new Errors.NotFoundError("document not found: #{doc_id}")) - RangesManager.acceptChange change_id, ranges, (error, new_ranges) -> - return callback(error) if error? - RedisManager.updateDocument doc_id, lines, version, [], new_ranges, (error) -> - return callback(error) if error? - callback() - - bulkAcceptChanges: (project_id, doc_id, change_ids, _callback = (error) ->) -> - timer = new Metrics.Timer("docManager.bulkAcceptChanges") - callback = (args...) -> - timer.done() - _callback(args...) - - DocumentManager.getDoc project_id, doc_id, (error, lines, version, ranges) -> - return callback(error) if error? - if !lines? or !version? - return callback(new Errors.NotFoundError("document not found: #{doc_id}")) - RangesManager.bulkAcceptChanges change_ids, ranges, (error, new_ranges) -> + RangesManager.acceptChanges change_ids, ranges, (error, new_ranges) -> return callback(error) if error? RedisManager.updateDocument doc_id, lines, version, [], new_ranges, (error) -> return callback(error) if error? @@ -192,13 +176,9 @@ module.exports = DocumentManager = UpdateManager = require "./UpdateManager" UpdateManager.lockUpdatesAndDo DocumentManager.flushAndDeleteDoc, project_id, doc_id, callback - acceptChangeWithLock: (project_id, doc_id, change_id, callback = (error) ->) -> + acceptChangesWithLock: (project_id, doc_id, change_ids, callback = (error) ->) -> UpdateManager = require "./UpdateManager" - UpdateManager.lockUpdatesAndDo DocumentManager.acceptChange, project_id, doc_id, change_id, callback - - bulkAcceptChangesWithLock: (project_id, doc_id, change_ids, callback = (error) ->) -> - UpdateManager = require "./UpdateManager" - UpdateManager.lockUpdatesAndDo DocumentManager.bulkAcceptChanges, project_id, doc_id, change_ids, callback + UpdateManager.lockUpdatesAndDo DocumentManager.acceptChanges, project_id, doc_id, change_ids, callback deleteCommentWithLock: (project_id, doc_id, thread_id, callback = (error) ->) -> UpdateManager = require "./UpdateManager" diff --git a/services/document-updater/app/coffee/HttpController.coffee b/services/document-updater/app/coffee/HttpController.coffee index 075d1f8e40..8267df38f5 100644 --- a/services/document-updater/app/coffee/HttpController.coffee +++ b/services/document-updater/app/coffee/HttpController.coffee @@ -100,19 +100,20 @@ module.exports = HttpController = {project_id, doc_id, change_id} = req.params logger.log {project_id, doc_id, change_id}, "accepting change via http" timer = new Metrics.Timer("http.acceptChange") - DocumentManager.acceptChangeWithLock project_id, doc_id, change_id, (error) -> + DocumentManager.acceptChangesWithLock project_id, doc_id, [ change_id ], (error) -> timer.done() return next(error) if error? logger.log {project_id, doc_id, change_id}, "accepted change via http" res.send 204 # No Content - bulkAcceptChanges: (req, res, next = (error) ->) -> + acceptChanges: (req, res, next = (error) ->) -> {project_id, doc_id} = req.params {change_ids} = req.body - change_ids ?= [] + if !change_ids? + change_ids = [ req.params.change_id ] logger.log {project_id, doc_id}, "accepting #{ change_ids.length } changes via http" - timer = new Metrics.Timer("http.bulkAcceptChanges") - DocumentManager.bulkAcceptChangesWithLock project_id, doc_id, change_ids, (error) -> + timer = new Metrics.Timer("http.acceptChanges") + DocumentManager.acceptChangesWithLock project_id, doc_id, change_ids, (error) -> timer.done() return next(error) if error? logger.log {project_id, doc_id}, "accepted #{ change_ids.length } changes via http" diff --git a/services/document-updater/app/coffee/RangesManager.coffee b/services/document-updater/app/coffee/RangesManager.coffee index 559cd18da4..977790f825 100644 --- a/services/document-updater/app/coffee/RangesManager.coffee +++ b/services/document-updater/app/coffee/RangesManager.coffee @@ -33,15 +33,7 @@ module.exports = RangesManager = logger.log {project_id, doc_id, changesCount: response.changes?.length, commentsCount: response.comments?.length}, "applied updates to ranges" callback null, response - acceptChange: (change_id, ranges, callback = (error, ranges) ->) -> - {changes, comments} = ranges - logger.log {change_id}, "accepting change in ranges" - rangesTracker = new RangesTracker(changes, comments) - rangesTracker.removeChangeId(change_id) - response = RangesManager._getRanges(rangesTracker) - callback null, response - - bulkAcceptChanges: (change_ids, ranges, callback = (error, ranges) ->) -> + acceptChanges: (change_ids, ranges, callback = (error, ranges) ->) -> {changes, comments} = ranges logger.log {change_id}, "accepting #{ change_ids.length } changes in ranges" rangesTracker = new RangesTracker(changes, comments) From e96e6d6a2903385c6ebeadbf69035c0a3181dd9d Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Fri, 5 May 2017 15:17:19 +0100 Subject: [PATCH 03/11] Remove unused method. --- .../document-updater/app/coffee/HttpController.coffee | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/services/document-updater/app/coffee/HttpController.coffee b/services/document-updater/app/coffee/HttpController.coffee index 8267df38f5..cac024a802 100644 --- a/services/document-updater/app/coffee/HttpController.coffee +++ b/services/document-updater/app/coffee/HttpController.coffee @@ -95,16 +95,6 @@ module.exports = HttpController = return next(error) if error? logger.log project_id: project_id, "deleted project via http" res.send 204 # No Content - - acceptChange: (req, res, next = (error) ->) -> - {project_id, doc_id, change_id} = req.params - logger.log {project_id, doc_id, change_id}, "accepting change via http" - timer = new Metrics.Timer("http.acceptChange") - DocumentManager.acceptChangesWithLock project_id, doc_id, [ change_id ], (error) -> - timer.done() - return next(error) if error? - logger.log {project_id, doc_id, change_id}, "accepted change via http" - res.send 204 # No Content acceptChanges: (req, res, next = (error) ->) -> {project_id, doc_id} = req.params From 70341e4e8045eb8aed4abb037538e637c872de8d Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Mon, 8 May 2017 16:08:34 +0100 Subject: [PATCH 04/11] Add bulk method from range tracker. --- .../app/coffee/RangesTracker.coffee | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/services/document-updater/app/coffee/RangesTracker.coffee b/services/document-updater/app/coffee/RangesTracker.coffee index 14193f628d..f589098440 100644 --- a/services/document-updater/app/coffee/RangesTracker.coffee +++ b/services/document-updater/app/coffee/RangesTracker.coffee @@ -100,6 +100,21 @@ load = () -> change = @getChange(change_id) return if !change? @_removeChange(change) + + removeChangeIds: (change_to_remove_ids) -> + return if !change_to_remove_ids?.length > 0 + i = @changes.length + remove_change_id = {} + for change_id in change_to_remove_ids + remove_change_id[change_id] = true + + while (i--) + if remove_change_id[@changes[i].id] + delete remove_change_id[@changes[i].id] + removed_change = @changes.splice(i, 1)[0] + @_markAsDirty removed_change, "change", "removed" + if Object.keys(remove_change_id).length == 0 + break validate: (text) -> for change in @changes From d7ac57d054f7ff51d92ee0080d0500dfcf4ef7d6 Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Mon, 8 May 2017 16:08:42 +0100 Subject: [PATCH 05/11] Use new bulk method. --- services/document-updater/app/coffee/RangesManager.coffee | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/services/document-updater/app/coffee/RangesManager.coffee b/services/document-updater/app/coffee/RangesManager.coffee index 977790f825..1fa56559b3 100644 --- a/services/document-updater/app/coffee/RangesManager.coffee +++ b/services/document-updater/app/coffee/RangesManager.coffee @@ -37,8 +37,7 @@ module.exports = RangesManager = {changes, comments} = ranges logger.log {change_id}, "accepting #{ change_ids.length } changes in ranges" rangesTracker = new RangesTracker(changes, comments) - for change_id in change_ids - rangesTracker.removeChangeId(change_id) + rangesTracker.removeChangeIds(change_ids) response = RangesManager._getRanges(rangesTracker) callback null, response From 144804e516dbc9e83493cb6ba37c7ba6e2f62c3a Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Mon, 8 May 2017 16:34:17 +0100 Subject: [PATCH 06/11] Update tests with new methods. --- .../app/coffee/HttpController.coffee | 2 +- .../app/coffee/RangesManager.coffee | 2 +- .../DocumentManager/DocumentManagerTests.coffee | 12 ++++++------ .../HttpController/HttpControllerTests.coffee | 16 ++++++++-------- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/services/document-updater/app/coffee/HttpController.coffee b/services/document-updater/app/coffee/HttpController.coffee index cac024a802..13e618e734 100644 --- a/services/document-updater/app/coffee/HttpController.coffee +++ b/services/document-updater/app/coffee/HttpController.coffee @@ -98,7 +98,7 @@ module.exports = HttpController = acceptChanges: (req, res, next = (error) ->) -> {project_id, doc_id} = req.params - {change_ids} = req.body + change_ids = req.body?.change_ids if !change_ids? change_ids = [ req.params.change_id ] logger.log {project_id, doc_id}, "accepting #{ change_ids.length } changes via http" diff --git a/services/document-updater/app/coffee/RangesManager.coffee b/services/document-updater/app/coffee/RangesManager.coffee index 1fa56559b3..d0653bb6a2 100644 --- a/services/document-updater/app/coffee/RangesManager.coffee +++ b/services/document-updater/app/coffee/RangesManager.coffee @@ -35,7 +35,7 @@ module.exports = RangesManager = acceptChanges: (change_ids, ranges, callback = (error, ranges) ->) -> {changes, comments} = ranges - logger.log {change_id}, "accepting #{ change_ids.length } changes in ranges" + logger.log "accepting #{ change_ids.length } changes in ranges" rangesTracker = new RangesTracker(changes, comments) rangesTracker.removeChangeIds(change_ids) response = RangesManager._getRanges(rangesTracker) diff --git a/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee b/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee index 47fbde021b..7050f0f370 100644 --- a/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee @@ -283,12 +283,12 @@ describe "DocumentManager", -> @ranges = { entries: "mock", comments: "mock" } @updated_ranges = { entries: "updated", comments: "updated" } @DocumentManager.getDoc = sinon.stub().yields(null, @lines, @version, @ranges) - @RangesManager.acceptChange = sinon.stub().yields(null, @updated_ranges) + @RangesManager.acceptChanges = sinon.stub().yields(null, @updated_ranges) @RedisManager.updateDocument = sinon.stub().yields() describe "successfully", -> beforeEach -> - @DocumentManager.acceptChange @project_id, @doc_id, @change_id, @callback + @DocumentManager.acceptChanges @project_id, @doc_id, [ @change_id ], @callback it "should get the document's current ranges", -> @DocumentManager.getDoc @@ -296,8 +296,8 @@ describe "DocumentManager", -> .should.equal true it "should apply the accept change to the ranges", -> - @RangesManager.acceptChange - .calledWith(@change_id, @ranges) + @RangesManager.acceptChanges + .calledWith([ @change_id ], @ranges) .should.equal true it "should save the updated ranges", -> @@ -311,7 +311,7 @@ describe "DocumentManager", -> describe "when the doc is not found", -> beforeEach -> @DocumentManager.getDoc = sinon.stub().yields(null, null, null, null) - @DocumentManager.acceptChange @project_id, @doc_id, @change_id, @callback + @DocumentManager.acceptChanges @project_id, @doc_id, [ @change_id ], @callback it "should not save anything", -> @RedisManager.updateDocument.called.should.equal false @@ -356,7 +356,7 @@ describe "DocumentManager", -> describe "when the doc is not found", -> beforeEach -> @DocumentManager.getDoc = sinon.stub().yields(null, null, null, null) - @DocumentManager.acceptChange @project_id, @doc_id, @comment_id, @callback + @DocumentManager.acceptChanges @project_id, @doc_id, [ @comment_id ], @callback it "should not save anything", -> @RedisManager.updateDocument.called.should.equal false diff --git a/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee b/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee index 69b40c85d2..ec910c9519 100644 --- a/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee +++ b/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee @@ -335,7 +335,7 @@ describe "HttpController", -> .calledWith(new Error("oops")) .should.equal true - describe "acceptChange", -> + describe "acceptChanges", -> beforeEach -> @req = params: @@ -345,12 +345,12 @@ describe "HttpController", -> describe "successfully", -> beforeEach -> - @DocumentManager.acceptChangeWithLock = sinon.stub().callsArgWith(3) - @HttpController.acceptChange(@req, @res, @next) + @DocumentManager.acceptChangesWithLock = sinon.stub().callsArgWith(3) + @HttpController.acceptChanges(@req, @res, @next) it "should accept the change", -> - @DocumentManager.acceptChangeWithLock - .calledWith(@project_id, @doc_id, @change_id) + @DocumentManager.acceptChangesWithLock + .calledWith(@project_id, @doc_id, [ @change_id ]) .should.equal true it "should return a successful No Content response", -> @@ -360,7 +360,7 @@ describe "HttpController", -> it "should log the request", -> @logger.log - .calledWith({@project_id, @doc_id, @change_id}, "accepting change via http") + .calledWith({@project_id, @doc_id}, "accepting 1 changes via http") .should.equal true it "should time the request", -> @@ -368,8 +368,8 @@ describe "HttpController", -> describe "when an errors occurs", -> beforeEach -> - @DocumentManager.acceptChangeWithLock = sinon.stub().callsArgWith(3, new Error("oops")) - @HttpController.acceptChange(@req, @res, @next) + @DocumentManager.acceptChangesWithLock = sinon.stub().callsArgWith(3, new Error("oops")) + @HttpController.acceptChanges(@req, @res, @next) it "should call next with the error", -> @next From f690ef078aa823ea09c1e344fc6c8f254662240d Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Tue, 9 May 2017 16:16:25 +0100 Subject: [PATCH 07/11] Update tests; add new ones for multiple changes. --- .../DocumentManagerTests.coffee | 12 ++++++++- .../HttpController/HttpControllerTests.coffee | 20 +++++++++++++- .../RangesManager/RangesManagerTests.coffee | 27 +++++++++++++++++-- 3 files changed, 55 insertions(+), 4 deletions(-) diff --git a/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee b/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee index 7050f0f370..4a29f54321 100644 --- a/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee @@ -278,6 +278,7 @@ describe "DocumentManager", -> describe "acceptChanges", -> beforeEach -> @change_id = "mock-change-id" + @change_ids = [ "mock-change-id-1", "mock-change-id-2", "mock-change-id-3", "mock-change-id-4" ] @version = 34 @lines = ["original", "lines"] @ranges = { entries: "mock", comments: "mock" } @@ -286,7 +287,7 @@ describe "DocumentManager", -> @RangesManager.acceptChanges = sinon.stub().yields(null, @updated_ranges) @RedisManager.updateDocument = sinon.stub().yields() - describe "successfully", -> + describe "successfully with a single change", -> beforeEach -> @DocumentManager.acceptChanges @project_id, @doc_id, [ @change_id ], @callback @@ -308,6 +309,15 @@ describe "DocumentManager", -> it "should call the callback", -> @callback.called.should.equal true + describe "successfully with multiple changes", -> + beforeEach -> + @DocumentManager.acceptChanges @project_id, @doc_id, @change_ids, @callback + + it "should apply the accept change to the ranges", -> + @RangesManager.acceptChanges + .calledWith(@change_ids, @ranges) + .should.equal true + describe "when the doc is not found", -> beforeEach -> @DocumentManager.getDoc = sinon.stub().yields(null, null, null, null) diff --git a/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee b/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee index ec910c9519..617146e787 100644 --- a/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee +++ b/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee @@ -343,7 +343,7 @@ describe "HttpController", -> doc_id: @doc_id change_id: @change_id = "mock-change-od-1" - describe "successfully", -> + describe "successfully with a single change", -> beforeEach -> @DocumentManager.acceptChangesWithLock = sinon.stub().callsArgWith(3) @HttpController.acceptChanges(@req, @res, @next) @@ -366,6 +366,24 @@ describe "HttpController", -> it "should time the request", -> @Metrics.Timer::done.called.should.equal true + describe "succesfully with with multiple changes", -> + beforeEach -> + @change_ids = [ "mock-change-od-1", "mock-change-od-2", "mock-change-od-3", "mock-change-od-4" ] + @req.body = + change_ids: @change_ids + @DocumentManager.acceptChangesWithLock = sinon.stub().callsArgWith(3) + @HttpController.acceptChanges(@req, @res, @next) + + it "should accept the changes in the body payload", -> + @DocumentManager.acceptChangesWithLock + .calledWith(@project_id, @doc_id, @change_ids) + .should.equal true + + it "should log the request with the correct number of changes", -> + @logger.log + .calledWith({@project_id, @doc_id}, "accepting #{ @change_ids.length } changes via http") + .should.equal true + describe "when an errors occurs", -> beforeEach -> @DocumentManager.acceptChangesWithLock = sinon.stub().callsArgWith(3, new Error("oops")) diff --git a/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee b/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee index fd3bc8faec..bdb075ebe6 100644 --- a/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee @@ -10,7 +10,6 @@ describe "RangesManager", -> @RangesManager = SandboxedModule.require modulePath, requires: "logger-sharelatex": @logger = { error: sinon.stub(), log: sinon.stub(), warn: sinon.stub() } - @doc_id = "doc-id-123" @project_id = "project-id-123" @user_id = "user-id-123" @@ -178,4 +177,28 @@ describe "RangesManager", -> @callback.called.should.equal true [error, entries] = @callback.args[0] expect(error).to.not.be.null - expect(error.message).to.equal("Change ({\"op\":{\"i\":\"five\",\"p\":15},\"metadata\":{\"user_id\":\"user-id-123\"}}) doesn't match text (\"our \")") \ No newline at end of file + expect(error.message).to.equal("Change ({\"op\":{\"i\":\"five\",\"p\":15},\"metadata\":{\"user_id\":\"user-id-123\"}}) doesn't match text (\"our \")") + + describe "acceptChanges", -> + beforeEach -> + @ranges = { entries: "mock", comments: "mock" } + + describe "successfully with a single change", -> + beforeEach -> + @change_id = "mock-change-id" + @RangesManager.acceptChanges [ @change_id ], @ranges + + it "should log the call with the correct number of changes", -> + @logger.log + .calledWith("accepting 1 changes in ranges") + .should.equal true + + describe "successfully with multiple changes", -> + beforeEach -> + @change_ids = [ "mock-change-id-1", "mock-change-id-2", "mock-change-id-3", "mock-change-id-4" ] + @RangesManager.acceptChanges @change_ids, @ranges + + it "should log the call with the correct number of changes", -> + @logger.log + .calledWith("accepting #{ @change_ids.length } changes in ranges") + .should.equal true From 5677c7ad9af7668ad6b9d5f8e0892df9d0198032 Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Thu, 11 May 2017 11:31:19 +0100 Subject: [PATCH 08/11] Update range tracker. --- .../app/coffee/RangesTracker.coffee | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/services/document-updater/app/coffee/RangesTracker.coffee b/services/document-updater/app/coffee/RangesTracker.coffee index f589098440..4174dcdab0 100644 --- a/services/document-updater/app/coffee/RangesTracker.coffee +++ b/services/document-updater/app/coffee/RangesTracker.coffee @@ -108,13 +108,16 @@ load = () -> for change_id in change_to_remove_ids remove_change_id[change_id] = true - while (i--) - if remove_change_id[@changes[i].id] - delete remove_change_id[@changes[i].id] - removed_change = @changes.splice(i, 1)[0] - @_markAsDirty removed_change, "change", "removed" - if Object.keys(remove_change_id).length == 0 - break + remaining_changes = [] + + for change in @changes + if remove_change_id[change.id] + delete remove_change_id[change.id] + @_markAsDirty change, "change", "removed" + else + remaining_changes.push change + + @changes = remaining_changes validate: (text) -> for change in @changes From d53266ea2d947aa5a8e5e295021cf1b1322a7e20 Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Thu, 11 May 2017 12:00:34 +0100 Subject: [PATCH 09/11] Update web API stub. --- .../test/acceptance/coffee/FlushingDocsTests.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/document-updater/test/acceptance/coffee/FlushingDocsTests.coffee b/services/document-updater/test/acceptance/coffee/FlushingDocsTests.coffee index 47dddcd19a..f732d69bb6 100644 --- a/services/document-updater/test/acceptance/coffee/FlushingDocsTests.coffee +++ b/services/document-updater/test/acceptance/coffee/FlushingDocsTests.coffee @@ -63,7 +63,7 @@ describe "Flushing a doc to Mongo", -> lines: @lines version: @version } - sinon.stub MockWebApi, "setDocument", (project_id, doc_id, lines, version, callback = (error) ->) -> + sinon.stub MockWebApi, "setDocument", (project_id, doc_id, lines, version, ranges, callback = (error) ->) -> setTimeout callback, 30000 DocUpdaterClient.preloadDoc @project_id, @doc_id, done From 36d16a76e0e30b19e7d95a81f9387519482ac19e Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Fri, 12 May 2017 14:42:40 +0100 Subject: [PATCH 10/11] Unit test accept changes in the ranges manager. --- .../RangesManager/RangesManagerTests.coffee | 95 +++++++++++++++++-- 1 file changed, 88 insertions(+), 7 deletions(-) diff --git a/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee b/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee index bdb075ebe6..b11c73489e 100644 --- a/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee @@ -10,6 +10,7 @@ describe "RangesManager", -> @RangesManager = SandboxedModule.require modulePath, requires: "logger-sharelatex": @logger = { error: sinon.stub(), log: sinon.stub(), warn: sinon.stub() } + @doc_id = "doc-id-123" @project_id = "project-id-123" @user_id = "user-id-123" @@ -181,24 +182,104 @@ describe "RangesManager", -> describe "acceptChanges", -> beforeEach -> - @ranges = { entries: "mock", comments: "mock" } + @RangesManager = SandboxedModule.require modulePath, + requires: + "logger-sharelatex": @logger = { error: sinon.stub(), log: sinon.stub(), warn: sinon.stub() } + "./RangesTracker":@RangesTracker = SandboxedModule.require "../../../../app/js/RangesTracker.js" + + @ranges = { + comments: [] + changes: [{ + id: "a1" + op: + i: "lorem" + p: 0 + }, { + id: "a2" + op: + i: "ipsum" + p: 10 + }, { + id: "a3" + op: + i: "dolor" + p: 20 + }, { + id: "a4" + op: + i: "sit" + p: 30 + }, { + id: "a5" + op: + i: "amet" + p: 40 + }] + } + @removeChangeIdsSpy = sinon.spy @RangesTracker.prototype, "removeChangeIds" describe "successfully with a single change", -> - beforeEach -> - @change_id = "mock-change-id" - @RangesManager.acceptChanges [ @change_id ], @ranges + beforeEach (done) -> + @change_ids = [ @ranges.changes[1].id ] + @RangesManager.acceptChanges @change_ids, @ranges, (err, ranges) => + @rangesResponse = ranges + done() it "should log the call with the correct number of changes", -> @logger.log .calledWith("accepting 1 changes in ranges") .should.equal true + it "should delegate the change removal to the ranges tracker", -> + @removeChangeIdsSpy + .calledWith(@change_ids) + .should.equal true + + it "should remove the change", -> + expect(@rangesResponse.changes + .find((change) => change.id == @ranges.changes[1].id)) + .to.be.undefined + + it "should return the original number of changes minus 1", -> + @rangesResponse.changes.length + .should.equal @ranges.changes.length - 1 + + it "should not touch other changes", -> + for i in [ 0, 2, 3, 4] + expect(@rangesResponse.changes + .find((change) => change.id == @ranges.changes[i].id)) + .to.deep.equal @ranges.changes[i] + describe "successfully with multiple changes", -> - beforeEach -> - @change_ids = [ "mock-change-id-1", "mock-change-id-2", "mock-change-id-3", "mock-change-id-4" ] - @RangesManager.acceptChanges @change_ids, @ranges + beforeEach (done) -> + @change_ids = [ @ranges.changes[1].id, @ranges.changes[3].id, @ranges.changes[4].id ] + @RangesManager.acceptChanges @change_ids, @ranges, (err, ranges) => + @rangesResponse = ranges + done() it "should log the call with the correct number of changes", -> @logger.log .calledWith("accepting #{ @change_ids.length } changes in ranges") .should.equal true + + it "should delegate the change removal to the ranges tracker", -> + @removeChangeIdsSpy + .calledWith(@change_ids) + .should.equal true + + it "should remove the changes", -> + for i in [ 1, 3, 4] + expect(@rangesResponse.changes + .find((change) => change.id == @ranges.changes[1].id)) + .to.be.undefined + + it "should return the original number of changes minus the number of accepted changes", -> + @rangesResponse.changes.length + .should.equal @ranges.changes.length - 3 + + it "should not touch other changes", -> + for i in [ 0, 2 ] + expect(@rangesResponse.changes + .find((change) => change.id == @ranges.changes[i].id)) + .to.deep.equal @ranges.changes[i] + From ef7fa5925747fc131b3013ff342c282348764d9d Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Mon, 15 May 2017 11:11:14 +0100 Subject: [PATCH 11/11] Update ranges tracker. --- .../app/coffee/RangesTracker.coffee | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/services/document-updater/app/coffee/RangesTracker.coffee b/services/document-updater/app/coffee/RangesTracker.coffee index 4174dcdab0..869d63159b 100644 --- a/services/document-updater/app/coffee/RangesTracker.coffee +++ b/services/document-updater/app/coffee/RangesTracker.coffee @@ -96,6 +96,20 @@ load = () -> break return change + getChanges: (change_ids) -> + changes_response = [] + ids_map = {} + + for change_id in change_ids + ids_map[change_id] = true + + for change in @changes + if ids_map[change.id] + delete ids_map[change.id] + changes_response.push change + + return changes_response + removeChangeId: (change_id) -> change = @getChange(change_id) return if !change?