diff --git a/services/document-updater/app/coffee/DocumentManager.coffee b/services/document-updater/app/coffee/DocumentManager.coffee index 8e69989d09..3d3f690b5c 100644 --- a/services/document-updater/app/coffee/DocumentManager.coffee +++ b/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? diff --git a/services/document-updater/app/coffee/HistoryManager.coffee b/services/document-updater/app/coffee/HistoryManager.coffee index 9ec5db2aa3..3ccc42e97f 100644 --- a/services/document-updater/app/coffee/HistoryManager.coffee +++ b/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) -> diff --git a/services/document-updater/app/coffee/HttpController.coffee b/services/document-updater/app/coffee/HttpController.coffee index ef9f860552..650ee07ae2 100644 --- a/services/document-updater/app/coffee/HttpController.coffee +++ b/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 diff --git a/services/document-updater/app/coffee/ProjectManager.coffee b/services/document-updater/app/coffee/ProjectManager.coffee index af2e27c8e5..64293e6985 100644 --- a/services/document-updater/app/coffee/ProjectManager.coffee +++ b/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 diff --git a/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee b/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee index 702617f7ae..c390138cf9 100644 --- a/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee +++ b/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 diff --git a/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee b/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee index 75327a7ae9..6e5010d89c 100644 --- a/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee +++ b/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 diff --git a/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee b/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee index d52956635d..99496332fc 100644 --- a/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee +++ b/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")) diff --git a/services/document-updater/test/unit/coffee/ProjectManager/flushAndDeleteProjectTests.coffee b/services/document-updater/test/unit/coffee/ProjectManager/flushAndDeleteProjectTests.coffee index 74161ca4a2..50a2679953 100644 --- a/services/document-updater/test/unit/coffee/ProjectManager/flushAndDeleteProjectTests.coffee +++ b/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 - -