Merge pull request #20 from sharelatex/hof-reduce-project-history-flushes

reduce the number of times we flush project history

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

@@ -89,6 +89,10 @@ module.exports = DocumentManager =
 							callback null
 					else
 						DocumentManager.flushAndDeleteDoc project_id, doc_id, (error) ->
+							# There is no harm in flushing project history if the previous
+							# call failed and sometimes it is required
+							HistoryManager.flushProjectChangesAsync project_id
+
 							return callback(error) if error?
 							callback null
 
@@ -118,7 +122,7 @@ module.exports = DocumentManager =
 			return callback(error) if error?
 
 			# Flush in the background since it requires a http request
-			HistoryManager.flushChangesAsync project_id, doc_id
+			HistoryManager.flushDocChangesAsync project_id, doc_id
 
 			RedisManager.removeDocFromMemory project_id, doc_id, (error) ->
 				return callback(error) if error?

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

@@ -4,12 +4,7 @@ logger = require "logger-sharelatex"
 HistoryRedisManager = require "./HistoryRedisManager"
 
 module.exports = HistoryManager =
-	flushChangesAsync: (project_id, doc_id) ->
-		HistoryManager._flushDocChangesAsync project_id, doc_id
-		if Settings.apis?.project_history?.enabled
-			HistoryManager.flushProjectChangesAsync project_id
-
-	_flushDocChangesAsync: (project_id, doc_id) ->
+	flushDocChangesAsync: (project_id, doc_id) ->
 		if !Settings.apis?.trackchanges?
 			logger.warn { doc_id }, "track changes API is not configured, so not flushing"
 			return
@@ -23,7 +18,7 @@ module.exports = HistoryManager =
 				logger.error { doc_id, project_id }, "track changes api returned a failure status code: #{res.statusCode}"
 
 	flushProjectChangesAsync: (project_id) ->
-		return if !Settings.apis?.project_history?
+		return if !Settings.apis?.project_history?.enabled
 
 		url = "#{Settings.apis.project_history.url}/project/#{project_id}/flush"
 		logger.log { project_id, url }, "flushing doc in project history api"
@@ -53,7 +48,7 @@ module.exports = HistoryManager =
 				# Do this in the background since it uses HTTP and so may be too
 				# slow to wait for when processing a doc update.
 				logger.log { doc_ops_length, doc_id, project_id }, "flushing track changes api"
-				HistoryManager._flushDocChangesAsync project_id, doc_id
+				HistoryManager.flushDocChangesAsync project_id, doc_id
 			callback()
 
 	shouldFlushHistoryOps: (length, ops_length, threshold) ->

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

@@ -1,4 +1,5 @@
 DocumentManager = require "./DocumentManager"
+HistoryManager = require "./HistoryManager"
 ProjectManager = require "./ProjectManager"
 Errors = require "./Errors"
 logger = require "logger-sharelatex"
@@ -106,6 +107,10 @@ module.exports = HttpController =
 		timer = new Metrics.Timer("http.deleteDoc")
 		DocumentManager.flushAndDeleteDocWithLock project_id, doc_id, (error) ->
 			timer.done()
+			# There is no harm in flushing project history if the previous call
+			# failed and sometimes it is required
+			HistoryManager.flushProjectChangesAsync project_id
+
 			return next(error) if error?
 			logger.log project_id: project_id, doc_id: doc_id, "deleted doc via http"
 			res.send 204 # No Content

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

@@ -59,6 +59,10 @@ module.exports = ProjectManager =
 
 			logger.log project_id: project_id, doc_ids: doc_ids, "deleting docs"
 			async.series jobs, () ->
+				# There is no harm in flushing project history if the previous call
+				# failed and sometimes it is required
+				HistoryManager.flushProjectChangesAsync project_id
+
 				if errors.length > 0
 					callback new Error("Errors deleting docs. See log for details")
 				else

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

@@ -11,7 +11,9 @@ describe "DocumentManager", ->
 		@DocumentManager = SandboxedModule.require modulePath, requires:
 			"./RedisManager": @RedisManager = {}
 			"./PersistenceManager": @PersistenceManager = {}
-			"./HistoryManager": @HistoryManager = {}
+			"./HistoryManager": @HistoryManager =
+				flushDocChangesAsync: sinon.stub()
+				flushProjectChangesAsync: sinon.stub()
 			"logger-sharelatex": @logger = {log: sinon.stub()}
 			"./DocOpsManager": @DocOpsManager = {}
 			"./Metrics": @Metrics =
@@ -36,7 +38,6 @@ describe "DocumentManager", ->
 			beforeEach ->
 				@RedisManager.removeDocFromMemory = sinon.stub().callsArg(2)
 				@DocumentManager.flushDocIfLoaded = sinon.stub().callsArgWith(2)
-				@HistoryManager.flushChangesAsync = sinon.stub()
 				@DocumentManager.flushAndDeleteDoc @project_id, @doc_id, @callback
 
 			it "should flush the doc", ->
@@ -56,8 +57,8 @@ describe "DocumentManager", ->
 				@Metrics.Timer::done.called.should.equal true
 
 			it "should flush to the history api", ->
-				@HistoryManager.flushChangesAsync
-					.calledWith(@project_id, @doc_id)
+				@HistoryManager.flushDocChangesAsync
+					.calledWithExactly(@project_id, @doc_id)
 					.should.equal true
 
 	describe "flushDocIfLoaded", ->
@@ -243,6 +244,10 @@ describe "DocumentManager", ->
 						.calledWith(@project_id, @doc_id)
 						.should.equal true
 
+				it "should not flush the project history", ->
+					@HistoryManager.flushProjectChangesAsync
+						.called.should.equal false
+
 				it "should call the callback", ->
 					@callback.calledWith(null).should.equal true
 
@@ -259,6 +264,11 @@ describe "DocumentManager", ->
 						.calledWith(@project_id, @doc_id)
 						.should.equal true
 
+				it "should not flush the project history", ->
+					@HistoryManager.flushProjectChangesAsync
+						.calledWithExactly(@project_id)
+						.should.equal true
+
 			describe "without new lines", ->
 				beforeEach ->
 					@DocumentManager.setDoc @project_id, @doc_id, null, @source, @user_id, false, @callback

services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee

@@ -21,28 +21,11 @@ describe "HistoryManager", ->
 		@doc_id = "mock-doc-id"
 		@callback = sinon.stub()
 
-	describe "flushChangesAsync", ->
-		beforeEach ->
-			@HistoryManager._flushDocChangesAsync = sinon.stub()
-			@HistoryManager.flushProjectChangesAsync = sinon.stub()
-
-			@HistoryManager.flushChangesAsync(@project_id, @doc_id)
-
-		it "flushes doc changes", ->
-			@HistoryManager._flushDocChangesAsync
-				.calledWith(@project_id, @doc_id)
-				.should.equal true
-
-		it "flushes project changes", ->
-			@HistoryManager.flushProjectChangesAsync
-				.calledWith(@project_id)
-				.should.equal true
-
-	describe "_flushDocChangesAsync", ->
+	describe "flushDocChangesAsync", ->
 		beforeEach ->
 			@request.post = sinon.stub().callsArgWith(1, null, statusCode: 204)
 
-			@HistoryManager._flushDocChangesAsync @project_id, @doc_id
+			@HistoryManager.flushDocChangesAsync @project_id, @doc_id
 
 		it "should send a request to the track changes api", ->
 			@request.post
@@ -68,7 +51,7 @@ describe "HistoryManager", ->
 
 			@HistoryManager.flushProjectChangesAsync = sinon.stub()
 			@HistoryRedisManager.recordDocHasHistoryOps = sinon.stub().callsArg(3)
-			@HistoryManager._flushDocChangesAsync = sinon.stub()
+			@HistoryManager.flushDocChangesAsync = sinon.stub()
 
 		describe "with no ops", ->
 			beforeEach ->
@@ -83,7 +66,7 @@ describe "HistoryManager", ->
 				@HistoryRedisManager.recordDocHasHistoryOps.called.should.equal false
 
 			it "should not flush doc changes", ->
-				@HistoryManager._flushDocChangesAsync.called.should.equal false
+				@HistoryManager.flushDocChangesAsync.called.should.equal false
 
 			it "should call the callback", ->
 				@callback.called.should.equal true
@@ -108,7 +91,7 @@ describe "HistoryManager", ->
 					.calledWith(@project_id, @doc_id, @ops)
 
 			it "should not flush doc changes", ->
-				@HistoryManager._flushDocChangesAsync.called.should.equal false
+				@HistoryManager.flushDocChangesAsync.called.should.equal false
 
 			it "should call the callback", ->
 				@callback.called.should.equal true
@@ -131,7 +114,7 @@ describe "HistoryManager", ->
 					.calledWith(@project_id, @doc_id, @ops)
 
 			it "should flush doc changes", ->
-				@HistoryManager._flushDocChangesAsync
+				@HistoryManager.flushDocChangesAsync
 					.calledWith(@project_id, @doc_id)
 					.should.equal true
 
@@ -149,7 +132,7 @@ describe "HistoryManager", ->
 				)
 
 			it "should not flush doc changes", ->
-				@HistoryManager._flushDocChangesAsync.called.should.equal false
+				@HistoryManager.flushDocChangesAsync.called.should.equal false
 
 			it "should call the callback with the error", ->
 				@callback.calledWith(@error).should.equal true

services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee

@@ -9,6 +9,8 @@ describe "HttpController", ->
 	beforeEach ->
 		@HttpController = SandboxedModule.require modulePath, requires:
 			"./DocumentManager": @DocumentManager = {}
+			"./HistoryManager": @HistoryManager =
+				flushProjectChangesAsync: sinon.stub()
 			"./ProjectManager": @ProjectManager = {}
 			"logger-sharelatex" : @logger = { log: sinon.stub() }
 			"./Metrics": @Metrics = {}
@@ -275,6 +277,11 @@ describe "HttpController", ->
 					.calledWith(@project_id, @doc_id)
 					.should.equal true
 
+			it "should flush project history", ->
+				@HistoryManager.flushProjectChangesAsync
+					.calledWithExactly(@project_id)
+					.should.equal true
+
 			it "should return a successful No Content response", ->
 				@res.send
 					.calledWith(204)
@@ -293,6 +300,11 @@ describe "HttpController", ->
 				@DocumentManager.flushAndDeleteDocWithLock = sinon.stub().callsArgWith(2, new Error("oops"))
 				@HttpController.flushAndDeleteDoc(@req, @res, @next)
 
+			it "should flush project history", ->
+				@HistoryManager.flushProjectChangesAsync
+					.calledWithExactly(@project_id)
+					.should.equal true
+
 			it "should call next with the error", ->
 				@next
 					.calledWith(new Error("oops"))

services/document-updater/test/unit/coffee/ProjectManager/flushAndDeleteProjectTests.coffee

@@ -10,7 +10,8 @@ describe "ProjectManager - flushAndDeleteProject", ->
 			"./RedisManager": @RedisManager = {}
 			"./DocumentManager": @DocumentManager = {}
 			"logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub() }
-			"./HistoryManager": @HistoryManager = {}
+			"./HistoryManager": @HistoryManager =
+				flushProjectChangesAsync: sinon.stub()
 			"./Metrics": @Metrics =
 				Timer: class Timer
 					done: sinon.stub()
@@ -30,13 +31,18 @@ describe "ProjectManager - flushAndDeleteProject", ->
 			@RedisManager.getDocIdsInProject
 				.calledWith(@project_id)
 				.should.equal true
-		
+
 		it "should delete each doc in the project", ->
 			for doc_id in @doc_ids
 				@DocumentManager.flushAndDeleteDocWithLock
 					.calledWith(@project_id, doc_id)
 					.should.equal true
 
+		it "should flush project history", ->
+			@HistoryManager.flushProjectChangesAsync
+				.calledWithExactly(@project_id)
+				.should.equal true
+
 		it "should call the callback without error", ->
 			@callback.calledWith(null).should.equal true
 
@@ -55,13 +61,18 @@ describe "ProjectManager - flushAndDeleteProject", ->
 			@ProjectManager.flushAndDeleteProjectWithLocks @project_id, (error) =>
 				@callback(error)
 				done()
-			
+
 		it "should still flush each doc in the project", ->
 			for doc_id in @doc_ids
 				@DocumentManager.flushAndDeleteDocWithLock
 					.calledWith(@project_id, doc_id)
 					.should.equal true
 
+		it "should still flush project history", ->
+			@HistoryManager.flushProjectChangesAsync
+				.calledWithExactly(@project_id)
+				.should.equal true
+
 		it "should record the error", ->
 			@logger.error
 				.calledWith(err: @error, project_id: @project_id, doc_id: "doc-id-1", "error deleting doc")
@@ -72,5 +83,3 @@ describe "ProjectManager - flushAndDeleteProject", ->
 
 		it "should time the execution", ->
 			@Metrics.Timer::done.called.should.equal true
-
-