diff --git a/services/document-updater/app/coffee/DocumentManager.coffee b/services/document-updater/app/coffee/DocumentManager.coffee index 8e69989d09..6574c4c4b1 100644 --- a/services/document-updater/app/coffee/DocumentManager.coffee +++ b/services/document-updater/app/coffee/DocumentManager.coffee @@ -89,6 +89,13 @@ module.exports = DocumentManager = callback null else DocumentManager.flushAndDeleteDoc project_id, doc_id, (error) -> + # Flush in the background since it requires a http request. We + # want to flush project history if the previous call only failed + # to delete the doc from Redis. There is no harm in flushing + # project history if the previous call failed to flush at all. So + # do this before checking errors. + HistoryManager.flushProjectChangesAsync project_id + return callback(error) if error? callback null @@ -118,7 +125,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..6d567234da 100644 --- a/services/document-updater/app/coffee/HistoryManager.coffee +++ b/services/document-updater/app/coffee/HistoryManager.coffee @@ -4,10 +4,8 @@ logger = require "logger-sharelatex" HistoryRedisManager = require "./HistoryRedisManager" module.exports = HistoryManager = - flushChangesAsync: (project_id, doc_id) -> + flushDocChangesAsync: (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) -> if !Settings.apis?.trackchanges? @@ -23,7 +21,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" diff --git a/services/document-updater/app/coffee/HttpController.coffee b/services/document-updater/app/coffee/HttpController.coffee index ef9f860552..069cfc889e 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,13 @@ module.exports = HttpController = timer = new Metrics.Timer("http.deleteDoc") DocumentManager.flushAndDeleteDocWithLock project_id, doc_id, (error) -> timer.done() + # Flush in the background since it requires a http request. We + # want to flush project history if the previous call only failed + # to delete the doc from Redis. There is no harm in flushing + # project history if the previous call failed to flush at all. So + # do this before checking errors. + 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..a82d88b4a6 100644 --- a/services/document-updater/app/coffee/ProjectManager.coffee +++ b/services/document-updater/app/coffee/ProjectManager.coffee @@ -59,6 +59,12 @@ module.exports = ProjectManager = logger.log project_id: project_id, doc_ids: doc_ids, "deleting docs" async.series jobs, () -> + # Flush in the background since it requires a htpt request. If we + # flushed and deleted only some docs successfully then we should still + # flush project history. If no docs succeeded then there is still no + # harm flushing project history. So do this before checking errors. + 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..161b7afd44 100644 --- a/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee @@ -21,23 +21,6 @@ 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", -> beforeEach -> @request.post = sinon.stub().callsArgWith(1, null, statusCode: 204) 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 - -