Refactor diff manager

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

@@ -4,36 +4,42 @@ DiffGenerator = require "./DiffGenerator"
 logger = require "logger-sharelatex"
 
 module.exports = DiffManager =
-	getLatestDocAndUpdates: (project_id, doc_id, fromVersion, toVersion, callback = (error, lines, version, updates) ->) ->
+	getLatestDocAndUpdates: (project_id, doc_id, fromVersion, toVersion, callback = (error, content, version, updates) ->) ->
 		UpdatesManager.getUpdatesWithUserInfo doc_id, from: fromVersion, to: toVersion, (error, updates) ->
 			return callback(error) if error?
-			DocumentUpdaterManager.getDocument project_id, doc_id, (error, lines, version) ->
+			DocumentUpdaterManager.getDocument project_id, doc_id, (error, content, version) ->
 				return callback(error) if error?
-				callback(null, lines, version, updates)
+				callback(null, content, version, updates)
 	
 	getDiff: (project_id, doc_id, fromVersion, toVersion, callback = (error, diff) ->) ->
 		logger.log project_id: project_id, doc_id: doc_id, from: fromVersion, to: toVersion, "getting diff"
-		DiffManager.getLatestDocAndUpdates project_id, doc_id, fromVersion, null, (error, lines, version, updates) ->
+		DiffManager.getDocumentBeforeVersion project_id, doc_id, fromVersion, (error, startingContent, updates) ->
 			return callback(error) if error?
 
-			logger.log lines: lines, version: version, updates: updates, "got doc and updates"
-
-			lastUpdate = updates[0]
-			if lastUpdate? and lastUpdate.v != version - 1
-				return callback new Error("latest update version, #{lastUpdate.v}, does not match doc version, #{version}")
-
 			updatesToApply = []
-			for update in updates.reverse()
+			for update in updates.slice().reverse()
 				if update.v <= toVersion
 					updatesToApply.push update
 
-			logger.log project_id: project_id, doc_id: doc_id, updatesToApply: updatesToApply, "got updates to apply"
-
 			try
-				startingContent = DiffGenerator.rewindUpdates lines.join("\n"), updates
-				logger.log project_id: project_id, doc_id: doc_id, startingContent: startingContent, "rewound doc"
 				diff = DiffGenerator.buildDiff startingContent, updatesToApply
 			catch e
 				return callback(e)
 			
-			callback(null, diff)
+			callback(null, diff)
+
+	getDocumentBeforeVersion: (project_id, doc_id, version, callback = (error, document, rewoundUpdates) ->) ->
+		logger.log project_id: project_id, doc_id: doc_id, version: version, "getting document before version"
+		DiffManager.getLatestDocAndUpdates project_id, doc_id, version, null, (error, content, version, updates) ->
+			return callback(error) if error?
+
+			lastUpdate = updates[0]
+			if lastUpdate? and lastUpdate.v != version - 1
+				return callback new Error("latest update version, #{lastUpdate.v}, does not match doc version, #{version}")
+
+			try
+				startingContent = DiffGenerator.rewindUpdates content, updates.slice().reverse()
+			catch e
+				return callback(e)
+			
+			callback(null, startingContent, updates)

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

@@ -14,7 +14,7 @@ module.exports = DocumentUpdaterManager =
 					body = JSON.parse(body)
 				catch error
 					return callback(error)
-				callback null, body.lines, body.version
+				callback null, body.lines.join("\n"), body.version
 			else
 				error = new Error("doc updater returned a non-success status code: #{res.statusCode}")
 				logger.error err: error, project_id:project_id, doc_id:doc_id, url: url, "error accessing doc updater"

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

@@ -14,7 +14,6 @@ module.exports = UpdatesManager =
 
 		MongoManager.popLastCompressedUpdate doc_id, (error, lastCompressedUpdate) ->
 			return callback(error) if error?
-			logger.log doc_id: doc_id, "popped last update"
 
 			# Ensure that raw updates start where lastCompressedUpdate left off
 			if lastCompressedUpdate?
@@ -39,11 +38,9 @@ module.exports = UpdatesManager =
 
 	REDIS_READ_BATCH_SIZE: 100
 	processUncompressedUpdates: (doc_id, callback = (error) ->) ->
-		logger.log "processUncompressedUpdates"
 		RedisManager.getOldestRawUpdates doc_id, UpdatesManager.REDIS_READ_BATCH_SIZE, (error, rawUpdates) ->
 			return callback(error) if error?
 			length = rawUpdates.length
-			logger.log doc_id: doc_id, length: length, "got raw updates from redis"
 			UpdatesManager.compressAndSaveRawUpdates doc_id, rawUpdates, (error) ->
 				return callback(error) if error?
 				logger.log doc_id: doc_id, "compressed and saved doc updates"

services/track-changes/test/unit/coffee/DiffManager/DiffManagerTests.coffee

@@ -20,11 +20,11 @@ describe "DiffManager", ->
 
 	describe "getLatestDocAndUpdates", ->
 		beforeEach ->
-			@lines = [ "hello", "world" ]
+			@content = "hello world"
 			@version = 42
 			@updates = [ "mock-update-1", "mock-update-2" ]
 
-			@DocumentUpdaterManager.getDocument = sinon.stub().callsArgWith(2, null, @lines, @version)
+			@DocumentUpdaterManager.getDocument = sinon.stub().callsArgWith(2, null, @content, @version)
 			@UpdatesManager.getUpdatesWithUserInfo = sinon.stub().callsArgWith(2, null, @updates)
 			@DiffManager.getLatestDocAndUpdates @project_id, @doc_id, @from, @to, @callback
 
@@ -38,12 +38,12 @@ describe "DiffManager", ->
 				.calledWith(@doc_id, from: @from, to: @to)
 				.should.equal true
 
-		it "should call the callback with the lines, version and updates", ->
-			@callback.calledWith(null, @lines, @version, @updates).should.equal true
+		it "should call the callback with the content, version and updates", ->
+			@callback.calledWith(null, @content, @version, @updates).should.equal true
 
 	describe "getDiff", ->
 		beforeEach ->
-			@lines = [ "hello", "world" ]
+			@content = "hello world"
 			# Op versions are the version they were applied to, so doc is always one version
 			# ahead.s
 			@version = 43
@@ -61,35 +61,75 @@ describe "DiffManager", ->
 			
 		describe "with matching versions", ->
 			beforeEach ->
-				@DiffManager.getLatestDocAndUpdates = sinon.stub().callsArgWith(4, null, @lines, @version, @updates)
-				@DiffGenerator.rewindUpdates = sinon.stub().returns(@rewound_content)
+				@DiffManager.getDocumentBeforeVersion = sinon.stub().callsArgWith(3, null, @rewound_content, @updates)
 				@DiffGenerator.buildDiff = sinon.stub().returns(@diff)
 				@DiffManager.getDiff @project_id, @doc_id, @fromVersion, @toVersion, @callback
 
 			it "should get the latest doc and version with all recent updates", ->
-				@DiffManager.getLatestDocAndUpdates
-					.calledWith(@project_id, @doc_id, @fromVersion, null)
-					.should.equal true
-
-			it "should rewind the diff", ->
-				@DiffGenerator.rewindUpdates
-					.calledWith(@lines.join("\n"), @updates)
+				@DiffManager.getDocumentBeforeVersion
+					.calledWith(@project_id, @doc_id, @fromVersion)
 					.should.equal true
 
 			it "should generate the diff", ->
 				@DiffGenerator.buildDiff
-					.calledWith(@rewound_content, @diffed_updates.reverse())
+					.calledWith(@rewound_content, @diffed_updates.slice().reverse())
 					.should.equal true
 
 			it "should call the callback with the diff", ->
 				@callback.calledWith(null, @diff).should.equal true
 
+		describe "when the updates are inconsistent", ->
+			beforeEach ->
+				@DiffManager.getLatestDocAndUpdates = sinon.stub().callsArgWith(4, null, @content, @version, @updates)
+				@DiffGenerator.buildDiff = sinon.stub().throws(@error = new Error("inconsistent!"))
+				@DiffManager.getDiff @project_id, @doc_id, @fromVersion, @toVersion, @callback
+
+			it "should call the callback with an error", ->
+				@callback
+					.calledWith(@error)
+					.should.equal true
+
+	describe "getDocumentBeforeVersion", ->
+		beforeEach ->
+			@content = "hello world"
+			# Op versions are the version they were applied to, so doc is always one version
+			# ahead.s
+			@version = 43
+			@updates = [
+				{ op: "mock-4", v: 42, meta: { start_ts: new Date(@to.getTime() + 20)} }
+				{ op: "mock-3", v: 41, meta: { start_ts: new Date(@to.getTime() + 10)} }
+				{ op: "mock-2", v: 40, meta: { start_ts: new Date(@to.getTime() - 10)} }
+				{ op: "mock-1", v: 39, meta: { start_ts: new Date(@to.getTime() - 20)} }
+			]
+			@fromVersion = 39
+			@rewound_content = "rewound-content"
+			@diff = [ u: "mock-diff" ]
+			
+		describe "with matching versions", ->
+			beforeEach ->
+				@DiffManager.getLatestDocAndUpdates = sinon.stub().callsArgWith(4, null, @content, @version, @updates)
+				@DiffGenerator.rewindUpdates = sinon.stub().returns(@rewound_content)
+				@DiffManager.getDocumentBeforeVersion @project_id, @doc_id, @fromVersion, @callback
+
+			it "should get the latest doc and version with all recent updates", ->
+				@DiffManager.getLatestDocAndUpdates
+					.calledWith(@project_id, @doc_id, @fromVersion, null)
+					.should.equal true
+
+			it "should rewind the diff", ->
+				@DiffGenerator.rewindUpdates
+					.calledWith(@content, @updates.slice().reverse())
+					.should.equal true
+
+			it "should call the callback with the rewound document and updates", ->
+				@callback.calledWith(null, @rewound_content, @updates).should.equal true
+
 		describe "with mismatching versions", ->
 			beforeEach ->
 				@version = 50
 				@updates = [ { op: "mock-1", v: 40 }, { op: "mock-1", v: 39 } ]
-				@DiffManager.getLatestDocAndUpdates = sinon.stub().callsArgWith(4, null, @lines, @version, @updates)
-				@DiffManager.getDiff @project_id, @doc_id, @fromVersion, @toVersion, @callback
+				@DiffManager.getLatestDocAndUpdates = sinon.stub().callsArgWith(4, null, @content, @version, @updates)
+				@DiffManager.getDocumentBeforeVersion @project_id, @doc_id, @fromVersion, @callback
 
 			it "should call the callback with an error", ->
 				@callback
@@ -98,12 +138,11 @@ describe "DiffManager", ->
 
 		describe "when the updates are inconsistent", ->
 			beforeEach ->
-				@DiffManager.getLatestDocAndUpdates = sinon.stub().callsArgWith(4, null, @lines, @version, @updates)
+				@DiffManager.getLatestDocAndUpdates = sinon.stub().callsArgWith(4, null, @content, @version, @updates)
 				@DiffGenerator.rewindUpdates = sinon.stub().throws(@error = new Error("inconsistent!"))
-				@DiffManager.getDiff @project_id, @doc_id, @fromVersion, @toVersion, @callback
+				@DiffManager.getDocumentBeforeVersion @project_id, @doc_id, @fromVersion, @callback
 
 			it "should call the callback with an error", ->
 				@callback
 					.calledWith(@error)
 					.should.equal true
-

services/track-changes/test/unit/coffee/DocumentUpdaterManager/DocumentUpdaterManagerTests.coffee

@@ -30,8 +30,8 @@ describe "DocumentUpdaterManager", ->
 				url = "#{@settings.apis.documentupdater.url}/project/#{@project_id}/doc/#{@doc_id}"
 				@request.get.calledWith(url).should.equal true
 
-			it "should call the callback with the lines and version", ->
-				@callback.calledWith(null, @lines, @version, @ops).should.equal true
+			it "should call the callback with the content and version", ->
+				@callback.calledWith(null, @lines.join("\n"), @version, @ops).should.equal true
 
 		describe "when the document updater API returns an error", ->
 			beforeEach ->