Refactor to take doc ops from an HTTP request from doc updater

services/track-changes/app.coffee

@@ -5,10 +5,11 @@ ConversoinManager = require "./app/js/ConversionManager"
 logger = require "logger-sharelatex"
 logger.initialize("history")
 
-app.post "/doc/:doc_id/flush", (req, res, next) ->
+app.post express.bodyParser(), "/doc/:doc_id/flush", (req, res, next) ->
 	project_id = req.params.project_id
+	docOps = req.body.docOps
 	logger.log doc_id: doc_id, "compressing doc history"
-	ConversionManager.convertOldRawUpdates doc_id, (error) ->
+	ConversionManager.convertAndSaveRawOps doc_id, docOps, (error) ->
 		return next(error) if error?
 		res.send 204 # No content
 

services/track-changes/app/coffee/ConversionManager.coffee

@@ -1,11 +1,11 @@
 {db, ObjectId} = require "./mongojs"
-ConcatManager = require "./ConcatManager"
+HistoryBuilder = require "./HistoryBuilder"
 logger = require "logger-sharelatex"
 
 module.exports = ConversionManager =
 	OPS_TO_LEAVE: 100
 
-	popLatestCompressedUpdate: (doc_id, callback = (error, update) ->) ->
+	popLastCompressedOp: (doc_id, callback = (error, op) ->) ->
 		db.docHistory.findAndModify
 			query: { doc_id: ObjectId(doc_id) }
 			fields: { docOps: { $slice: -1 } }
@@ -14,68 +14,35 @@ module.exports = ConversionManager =
 			return callback(error) if error?
 			callback null, history.docOps[0]
 
-	insertCompressedUpdates: (doc_id, updates, callback = (error) ->) ->
-		db.docHistory.update { doc_id: ObjectId(doc_id) }, { $push: { docOps: { $each: updates } } }, { upsert: true }, callback
+	insertCompressedOps: (doc_id, docOps, callback = (error) ->) ->
+		db.docHistory.update {
+			doc_id: ObjectId(doc_id)
+		}, {
+			$push:
+				docOps:
+					$each: docOps
+		}, {
+			upsert: true
+		}, callback
 
-	popOldRawUpdates: (doc_id, callback = (error, updates) ->) ->
-		db.docOps.find { doc_id: ObjectId(doc_id) }, { version: true, tailVersion: true }, (error, docs) ->
-			return callback(error) if error?
-			return callback(new Error("doc not found")) if docs.length == 0
-			doc = docs[0]
-			currentVersion = doc.version
-			tailVersion = doc.tailVersion or 0
-			if currentVersion - tailVersion > ConversionManager.OPS_TO_LEAVE
-				db.docOps.findAndModify
-					query:
-						doc_id: ObjectId(doc_id)
-						version: currentVersion
-					update:
-						$push:
-							docOps:
-								$each: []
-								$slice: - ConversionManager.OPS_TO_LEAVE
-						$set:
-							tailVersion: currentVersion - ConversionManager.OPS_TO_LEAVE
-					fields:
-						docOps:
-							$slice: currentVersion - tailVersion - ConversionManager.OPS_TO_LEAVE
-				, (error, doc) ->
-					return callback(error) if error?
-					if !doc?
-						# Version was modified since so try again
-						return ConversionManager.popOldRawUpdates doc_id, callback
-					else
-						return callback null, doc.docOps
-						
-			else
-				callback null, []
+	convertAndSaveRawOps: (doc_id, rawOps, callback = (error) ->) ->
+		length = rawOps.length
+		if length == 0
+			return callback()
 
-	convertOldRawUpdates: (doc_id, callback = (error) ->) ->
-		ConversionManager.popOldRawUpdates doc_id, (error, rawUpdates) ->
+
+		ConversionManager.popLastCompressedOp doc_id, (error, lastCompressedOp) ->
 			return callback(error) if error?
 
-			length = rawUpdates.length
+			if !lastCompressedOp?
+				rawOps = rawOps.slice(0) # Clone so we can modify in place
+				lastCompressedOp = rawOps.shift()
 
-			normalizedRawUpdates = []
-			for rawUpdate in rawUpdates
-				normalizedRawUpdates = normalizedRawUpdates.concat ConcatManager.normalizeUpdate(rawUpdate)
-			rawUpdates = normalizedRawUpdates
+			uncompressedOps = [lastCompressedOp].concat rawOps
+			compressedOps = HistoryBuilder.compressOps uncompressedOps
 
-			ConversionManager.popLatestCompressedUpdate doc_id, (error, lastCompressedUpdate) ->
+			ConversionManager.insertCompressedOps doc_id, compressedOps, (error) ->
 				return callback(error) if error?
-
-				if !lastCompressedUpdate?
-					lastCompressedUpdate = rawUpdates.shift()
-
-				if !lastCompressedUpdate?
-					# No saved versions, no raw updates, nothing to do
-					return callback()
-
-				uncompressedUpdates = [lastCompressedUpdate].concat rawUpdates
-				compressedUpdates = ConcatManager.compressUpdates uncompressedUpdates
-
-				ConversionManager.insertCompressedUpdates doc_id, compressedUpdates, (error) ->
-					return callback(error) if error?
-					logger.log doc_id: doc_id, rawOpsLength: length, compressedOpsLength: compressedUpdates.length, "compressed doc ops"
-					callback()
+				logger.log doc_id: doc_id, rawOpsLength: length, compressedOpsLength: compressedOps.length, "compressed doc ops"
+				callback()
 

services/track-changes/app/coffee/HistoryBuilder.coffee

@@ -1,8 +1,11 @@
 strInject = (s1, pos, s2) -> s1[...pos] + s2 + s1[pos..]
 strRemove = (s1, pos, length) -> s1[...pos] + s1[(pos + length)..]
 
-module.exports = ConcatManager =
+module.exports = HistoryBuilder =
 	normalizeUpdate: (update) ->
+		if update.meta.start_ts?
+			return [update] # already normalized
+
 		updates = []
 		for op in update.op
 			updates.push
@@ -15,11 +18,16 @@ module.exports = ConcatManager =
 
 	compressUpdates: (rawUpdates) ->
 		return [] if rawUpdates.length == 0
-		firstPass = [rawUpdates.shift()]
-		for update in rawUpdates
+		normalizedUpdates = []
+		for rawUpdate in rawUpdates
+			normalizedUpdates = normalizedUpdates.concat HistoryBuilder.normalizeUpdate(rawUpdate)
+
+		return [] if normalizedUpdates.length == 0
+		firstPass = [normalizedUpdates.shift()]
+		for update in normalizedUpdates
 			lastCompressedUpdate = firstPass.pop()
 			if lastCompressedUpdate?
-				firstPass = firstPass.concat ConcatManager._concatTwoUpdates lastCompressedUpdate, update, false
+				firstPass = firstPass.concat HistoryBuilder._concatTwoUpdates lastCompressedUpdate, update, false
 			else
 				firstPass.push rawUpdate
 
@@ -28,7 +36,7 @@ module.exports = ConcatManager =
 		for update in firstPass
 			lastCompressedUpdate = secondPass.pop()
 			if lastCompressedUpdate?
-				secondPass = secondPass.concat ConcatManager._concatTwoUpdates lastCompressedUpdate, update, true
+				secondPass = secondPass.concat HistoryBuilder._concatTwoUpdates lastCompressedUpdate, update, true
 			else
 				secondPass.push update
 
@@ -53,7 +61,7 @@ module.exports = ConcatManager =
 		if firstUpdate.meta.user_id != secondUpdate.meta.user_id
 			return [firstUpdate, secondUpdate]
 
-		if secondUpdate.meta.start_ts - firstUpdate.meta.end_ts > ConcatManager.MAX_TIME_BETWEEN_UPDATES
+		if secondUpdate.meta.start_ts - firstUpdate.meta.end_ts > HistoryBuilder.MAX_TIME_BETWEEN_UPDATES
 			return [firstUpdate, secondUpdate]
 
 		firstOp = firstUpdate.op[0]

services/track-changes/test/unit/coffee/ConversionManager/ConversionManagerTests.coffee

@@ -0,0 +1,87 @@
+sinon = require('sinon')
+chai = require('chai')
+should = chai.should()
+expect = chai.expect
+modulePath = "../../../../app/js/ConversionManager.js"
+SandboxedModule = require('sandboxed-module')
+
+describe "ConversionManager", ->
+	beforeEach ->
+		@ConversionManager = SandboxedModule.require modulePath, requires:
+			"./HistoryBuilder": @HistoryBuilder = {}
+			"./mongojs" : {}
+			"logger-sharelatex": { log: sinon.stub() }
+		@doc_id = "doc-id-123"
+		@callback = sinon.stub()
+
+	describe "when there are no raw ops", ->
+		beforeEach ->
+			@ConversionManager.popLastCompressedOp = sinon.stub()
+			@ConversionManager.insertCompressedOps = sinon.stub()
+			@ConversionManager.convertAndSaveRawOps @doc_id, [], @callback
+
+		it "should not need to access the database", ->
+			@ConversionManager.popLastCompressedOp.called.should.equal false
+			@ConversionManager.insertCompressedOps.called.should.equal false
+
+		it "should call the callback", ->
+			@callback.called.should.equal true
+
+	describe "when there is no compressed history to begin with", ->
+		beforeEach ->
+			@rawOps = ["mock-raw-op-1", "mock-raw-op-2"]
+			@compressedOps = ["mock-compressed-op"]
+
+			@ConversionManager.popLastCompressedOp = sinon.stub().callsArgWith(1, null, null)
+			@ConversionManager.insertCompressedOps = sinon.stub().callsArg(2)
+			@HistoryBuilder.compressOps = sinon.stub().returns(@compressedOps)
+			@ConversionManager.convertAndSaveRawOps @doc_id, @rawOps, @callback
+
+		it "should try to pop the last compressed op", ->
+			@ConversionManager.popLastCompressedOp
+				.calledWith(@doc_id)
+				.should.equal true
+		
+		it "should compress the raw ops", ->
+			@HistoryBuilder.compressOps
+				.calledWith(@rawOps)
+				.should.equal true
+		
+		it "should save the compressed ops", ->
+			@ConversionManager.insertCompressedOps
+				.calledWith(@doc_id, @compressedOps)
+				.should.equal true
+
+		it "should call the callback", ->
+			@callback.called.should.equal true
+
+	describe "when the raw ops need appending to existing history", ->
+		beforeEach ->
+			@rawOps = ["mock-raw-op-1", "mock-raw-op-2"]
+			@lastCompressedOp = "mock-last-compressed-op-0"
+			@compressedOps = ["mock-compressed-op-1"]
+
+			@ConversionManager.popLastCompressedOp = sinon.stub().callsArgWith(1, null, @lastCompressedOp)
+			@ConversionManager.insertCompressedOps = sinon.stub().callsArg(2)
+			@HistoryBuilder.compressOps = sinon.stub().returns(@compressedOps)
+			@ConversionManager.convertAndSaveRawOps @doc_id, @rawOps, @callback
+
+		it "should try to pop the last compressed op", ->
+			@ConversionManager.popLastCompressedOp
+				.calledWith(@doc_id)
+				.should.equal true
+		
+		it "should compress the last compressed op and the raw ops", ->
+			@HistoryBuilder.compressOps
+				.calledWith([@lastCompressedOp].concat(@rawOps))
+				.should.equal true
+		
+		it "should save the compressed ops", ->
+			@ConversionManager.insertCompressedOps
+				.calledWith(@doc_id, @compressedOps)
+				.should.equal true
+
+		it "should call the callback", ->
+			@callback.called.should.equal true
+		
+

services/track-changes/test/unit/coffee/HistoryBuilder/HistoryBuilderTests.coffee

@@ -2,12 +2,12 @@ sinon = require('sinon')
 chai = require('chai')
 should = chai.should()
 expect = chai.expect
-modulePath = "../../../../app/js/ConcatManager.js"
+modulePath = "../../../../app/js/HistoryBuilder.js"
 SandboxedModule = require('sandboxed-module')
 
-describe "ConcatManager", ->
+describe "HistoryBuilder", ->
 	beforeEach ->
-		@ConcatManager = SandboxedModule.require modulePath
+		@HistoryBuilder = SandboxedModule.require modulePath
 		@user_id = "user-id-1"
 		@other_user_id = "user-id-2"
 		@ts1 = Date.now()
@@ -16,7 +16,7 @@ describe "ConcatManager", ->
 	describe "compress", ->
 		describe "insert - insert", ->
 			it "should append one insert to the other", ->
-				expect(@ConcatManager.compressUpdates [{
+				expect(@HistoryBuilder.compressUpdates [{
 					op: [ p: 3, i: "foo" ]
 					meta: ts: @ts1, user_id: @user_id
 				}, {
@@ -29,7 +29,7 @@ describe "ConcatManager", ->
 				}]
 
 			it "should insert one insert inside the other", ->
-				expect(@ConcatManager.compressUpdates [{
+				expect(@HistoryBuilder.compressUpdates [{
 					op: [ p: 3, i: "foo" ]
 					meta: ts: @ts1, user_id: @user_id
 				}, {
@@ -42,7 +42,7 @@ describe "ConcatManager", ->
 				}]
 
 			it "should not append separated inserts", ->
-				expect(@ConcatManager.compressUpdates [{
+				expect(@HistoryBuilder.compressUpdates [{
 					op: [ p: 3, i: "foo" ]
 					meta: ts: @ts1, user_id: @user_id
 				}, {
@@ -59,7 +59,7 @@ describe "ConcatManager", ->
 
 		describe "delete - delete", ->
 			it "should append one delete to the other", ->
-				expect(@ConcatManager.compressUpdates [{
+				expect(@HistoryBuilder.compressUpdates [{
 					op: [ p: 3, d: "foo" ]
 					meta: ts: @ts1, user_id: @user_id
 				}, {
@@ -72,7 +72,7 @@ describe "ConcatManager", ->
 				}]
 				
 			it "should insert one delete inside the other", ->
-				expect(@ConcatManager.compressUpdates [{
+				expect(@HistoryBuilder.compressUpdates [{
 					op: [ p: 3, d: "foo" ]
 					meta: ts: @ts1, user_id: @user_id
 				}, {
@@ -85,7 +85,7 @@ describe "ConcatManager", ->
 				}]
 				
 			it "should not append separated deletes", ->
-				expect(@ConcatManager.compressUpdates [{
+				expect(@HistoryBuilder.compressUpdates [{
 					op: [ p: 3, d: "foo" ]
 					meta: ts: @ts1, user_id: @user_id
 				}, {
@@ -102,7 +102,7 @@ describe "ConcatManager", ->
 
 		describe "insert - delete", ->
 			it "should undo a previous insert", ->
-				expect(@ConcatManager.compressUpdates [{
+				expect(@HistoryBuilder.compressUpdates [{
 					op: [ p: 3, i: "foo" ]
 					meta: ts: @ts1, user_id: @user_id
 				}, {
@@ -115,7 +115,7 @@ describe "ConcatManager", ->
 				}]
 				
 			it "should remove part of an insert from the middle", ->
-				expect(@ConcatManager.compressUpdates [{
+				expect(@HistoryBuilder.compressUpdates [{
 					op: [ p: 3, i: "fobaro" ]
 					meta: ts: @ts1, user_id: @user_id
 				}, {
@@ -128,7 +128,7 @@ describe "ConcatManager", ->
 				}]
 
 			it "should cancel out two opposite updates", ->
-				expect(@ConcatManager.compressUpdates [{
+				expect(@HistoryBuilder.compressUpdates [{
 					op: [ p: 3, i: "foo" ]
 					meta: ts: @ts1, user_id: @user_id
 				}, {
@@ -138,7 +138,7 @@ describe "ConcatManager", ->
 				.to.deep.equal []
 				
 			it "should not combine separated updates", ->
-				expect(@ConcatManager.compressUpdates [{
+				expect(@HistoryBuilder.compressUpdates [{
 					op: [ p: 3, i: "foo" ]
 					meta: ts: @ts1, user_id: @user_id
 				}, {
@@ -155,7 +155,7 @@ describe "ConcatManager", ->
 
 		describe "delete - insert", ->
 			it "should redo a previous delete at the beginning", ->
-				expect(@ConcatManager.compressUpdates [{
+				expect(@HistoryBuilder.compressUpdates [{
 					op: [ p: 3, d: "foo" ]
 					meta: ts: @ts1, user_id: @user_id
 				}, {
@@ -168,7 +168,7 @@ describe "ConcatManager", ->
 				}]
 
 			it "should redo a previous delete from halfway through", ->
-				expect(@ConcatManager.compressUpdates [{
+				expect(@HistoryBuilder.compressUpdates [{
 					op: [ p: 3, d: "foobar" ]
 					meta: ts: @ts1, user_id: @user_id
 				}, {
@@ -184,7 +184,7 @@ describe "ConcatManager", ->
 				}]
 
 			it "should keep words together", ->
-				expect(@ConcatManager.compressUpdates [{
+				expect(@HistoryBuilder.compressUpdates [{
 					op: [ p: 3, d: "abcdefghijklmnopqrstuvwxyz hello world" ]
 					meta: ts: @ts1, user_id: @user_id
 				}, {
@@ -210,7 +210,7 @@ describe "ConcatManager", ->
 				
 
 			it "should not combine the ops if the insert text does not match the delete text", ->
-				expect(@ConcatManager.compressUpdates [{
+				expect(@HistoryBuilder.compressUpdates [{
 					op: [ p: 3, d: "foobar" ]
 					meta: ts: @ts1, user_id: @user_id
 				}, {
@@ -226,7 +226,7 @@ describe "ConcatManager", ->
 				}]
 
 			it "should cancel two equal updates", ->
-				expect(@ConcatManager.compressUpdates [{
+				expect(@HistoryBuilder.compressUpdates [{
 					op: [ p: 3, d: "foo" ]
 					meta: ts: @ts1, user_id: @user_id
 				}, {