Merge pull request #34 from sharelatex/pr-bulk-actions

Add methods to bulk accept changes.

services/document-updater/app.coffee

@@ -37,14 +37,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.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.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.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)->
 	timer = new Metrics.Timer("http.allDocList")	

services/document-updater/app/coffee/DocumentManager.coffee

@@ -130,9 +130,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...)
@@ -141,7 +141,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) ->
+			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?
@@ -183,9 +183,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
+		UpdateManager.lockUpdatesAndDo DocumentManager.acceptChanges, project_id, doc_id, change_ids, callback
 
 	deleteCommentWithLock: (project_id, doc_id, thread_id, callback = (error) ->) ->
 		UpdateManager = require "./UpdateManager"

services/document-updater/app/coffee/HttpController.coffee

@@ -95,15 +95,18 @@ 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.acceptChangeWithLock project_id, doc_id, change_id, (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.acceptChanges")
+		DocumentManager.acceptChangesWithLock project_id, doc_id, change_ids, (error) ->
 			timer.done()
 			return next(error) if error?
-			logger.log {project_id, doc_id, change_id}, "accepted change via http"
+			logger.log {project_id, doc_id}, "accepted #{ change_ids.length } changes via http"
 			res.send 204 # No Content
 	
 	deleteComment: (req, res, next = (error) ->) ->

services/document-updater/app/coffee/RangesManager.coffee

@@ -33,11 +33,11 @@ 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) ->) ->
+	acceptChanges: (change_ids, ranges, callback = (error, ranges) ->) ->
 		{changes, comments} = ranges
-		logger.log {change_id}, "accepting change in ranges"
+		logger.log "accepting #{ change_ids.length } changes in ranges"
 		rangesTracker = new RangesTracker(changes, comments)
-		rangesTracker.removeChangeId(change_id)
+		rangesTracker.removeChangeIds(change_ids)
 		response = RangesManager._getRanges(rangesTracker)
 		callback null, response
 

services/document-updater/app/coffee/RangesTracker.coffee

@@ -96,10 +96,42 @@ 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?
 			@_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
+
+			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

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
 

services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee

@@ -319,17 +319,18 @@ 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" }
 			@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", ->
+		describe "successfully with a single change", ->
 			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
@@ -337,8 +338,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", ->
@@ -349,10 +350,19 @@ 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)
-				@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
@@ -397,7 +407,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

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:
@@ -343,14 +343,14 @@ describe "HttpController", ->
 					doc_id: @doc_id
 					change_id: @change_id = "mock-change-od-1"
 
-		describe "successfully", ->
+		describe "successfully with a single change", ->
 			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,16 +360,34 @@ 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", ->
 				@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.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

services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee

@@ -10,7 +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"
@@ -178,4 +178,108 @@ 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 \")")
+				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 ->
+			@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 (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 (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]
+