From 7517a818b2f74003f7069213090fe5f890c1f2eb Mon Sep 17 00:00:00 2001 From: Thomas Date: Thu, 12 Aug 2021 15:07:34 +0100 Subject: [PATCH] Initialise full project history for old projects when project opened (#4687) * Initialise full project history for old projects when project opened This begins a second attempt at initialising the full project history in the background for projects without a full project history id. The original web-internal#4345 was reverted in web-internal#4353. This commit reverts the revert, and adds an additional flush of the project before initialising full project history. GitOrigin-RevId: ac263dca8cf0d80186fee916a76e5572ec5649d4 --- .../src/Features/History/HistoryManager.js | 14 +++++ .../src/Features/Project/ProjectController.js | 19 +++++++ .../Features/Project/ProjectHistoryHandler.js | 56 ++++++++++++------- .../src/mocks/MockProjectHistoryApi.js | 4 ++ .../src/Project/ProjectControllerTests.js | 16 ++++++ .../src/Project/ProjectHistoryHandlerTests.js | 25 ++++++--- 6 files changed, 104 insertions(+), 30 deletions(-) diff --git a/services/web/app/src/Features/History/HistoryManager.js b/services/web/app/src/Features/History/HistoryManager.js index 4ac4a09390..293a062697 100644 --- a/services/web/app/src/Features/History/HistoryManager.js +++ b/services/web/app/src/Features/History/HistoryManager.js @@ -8,12 +8,14 @@ module.exports = { initializeProject: callbackify(initializeProject), flushProject: callbackify(flushProject), resyncProject: callbackify(resyncProject), + forceResyncProject: callbackify(forceResyncProject), deleteProject: callbackify(deleteProject), injectUserDetails: callbackify(injectUserDetails), promises: { initializeProject, flushProject, resyncProject, + forceResyncProject, deleteProject, injectUserDetails, }, @@ -65,6 +67,18 @@ async function resyncProject(projectId) { } } +async function forceResyncProject(projectId) { + try { + await request.post({ + url: `${settings.apis.project_history.url}/project/${projectId}/resync?force=true`, + }) + } catch (err) { + throw OError.tag(err, 'failed to force resync project history', { + projectId, + }) + } +} + async function deleteProject(projectId, historyId) { try { const tasks = [ diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index 47cc292062..3f58582905 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -9,6 +9,7 @@ const { ObjectId } = require('mongodb') const ProjectDeleter = require('./ProjectDeleter') const ProjectDuplicator = require('./ProjectDuplicator') const ProjectCreationHandler = require('./ProjectCreationHandler') +const ProjectHistoryHandler = require('./ProjectHistoryHandler') const EditorController = require('../Editor/EditorController') const ProjectHelper = require('./ProjectHelper') const metrics = require('@overleaf/metrics') @@ -680,6 +681,24 @@ const ProjectController = { activate(cb) { InactiveProjectManager.reactivateProjectIfRequired(projectId, cb) }, + ensureHistoryExists(cb) { + // enable full project history in background for older projects + if (!Settings.apis.project_history || !Features.hasFeature('saas')) { + return cb() + } + ProjectHistoryHandler.ensureHistoryExistsForProject( + projectId, + err => { + if (err) { + logger.error( + { err, projectId }, + 'error ensuring history exists for project' + ) + } + cb() + } + ) + }, markAsOpened(cb) { // don't need to wait for this to complete ProjectUpdateHandler.markAsOpened(projectId, () => {}) diff --git a/services/web/app/src/Features/Project/ProjectHistoryHandler.js b/services/web/app/src/Features/Project/ProjectHistoryHandler.js index f3f7c39c5b..fd1f3afd97 100644 --- a/services/web/app/src/Features/Project/ProjectHistoryHandler.js +++ b/services/web/app/src/Features/Project/ProjectHistoryHandler.js @@ -19,6 +19,7 @@ const logger = require('logger-sharelatex') const settings = require('@overleaf/settings') const HistoryManager = require('../History/HistoryManager') const ProjectEntityUpdateHandler = require('./ProjectEntityUpdateHandler') +const DocumentUpdaterHandler = require('../DocumentUpdater/DocumentUpdaterHandler') const { promisifyAll } = require('../../util/promises') const ProjectHistoryHandler = { @@ -134,31 +135,44 @@ const ProjectHistoryHandler = { if (history_id != null) { return callback() } // history already exists, success - return HistoryManager.initializeProject(function (err, history) { + return HistoryManager.flushProject(project_id, function (err) { if (err != null) { return callback(err) } - if (!(history != null ? history.overleaf_id : undefined)) { - return callback(new Error('failed to initialize history id')) - } - return ProjectHistoryHandler.setHistoryId( - project_id, - history.overleaf_id, - function (err) { - if (err != null) { - return callback(err) - } - return ProjectEntityUpdateHandler.resyncProjectHistory( - project_id, - function (err) { - if (err != null) { - return callback(err) - } - return HistoryManager.flushProject(project_id, callback) - } - ) + return HistoryManager.initializeProject(function (err, history) { + if (err != null) { + return callback(err) } - ) + if (!history || !history.overleaf_id) { + return callback(new Error('failed to initialize history id')) + } + return ProjectHistoryHandler.setHistoryId( + project_id, + history.overleaf_id, + function (err) { + if (err != null) { + return callback(err) + } + return DocumentUpdaterHandler.flushProjectToMongoAndDelete( + project_id, + function (err) { + if (err != null) { + return callback(err) + } + return HistoryManager.forceResyncProject( + project_id, + function (err) { + if (err != null) { + return callback(err) + } + return HistoryManager.flushProject(project_id, callback) + } + ) + } + ) + } + ) + }) }) } ) diff --git a/services/web/test/acceptance/src/mocks/MockProjectHistoryApi.js b/services/web/test/acceptance/src/mocks/MockProjectHistoryApi.js index 2468aacb0a..671aed949b 100644 --- a/services/web/test/acceptance/src/mocks/MockProjectHistoryApi.js +++ b/services/web/test/acceptance/src/mocks/MockProjectHistoryApi.js @@ -128,6 +128,10 @@ class MockProjectHistoryApi extends AbstractMockApi { this.app.post('/project/:projectId/flush', (req, res) => { res.sendStatus(200) }) + + this.app.post('/project/:projectId/resync', (req, res) => { + res.sendStatus(204) + }) } } diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index 3fec7ffbbb..56373accd4 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectControllerTests.js @@ -54,6 +54,9 @@ describe('ProjectController', function () { .stub() .callsArgWith(2, null, { _id: this.project_id }), } + this.ProjectHistoryHandler = { + ensureHistoryExistsForProject: sinon.stub().callsArg(1), + } this.SubscriptionLocator = { getUsersSubscription: sinon.stub() } this.LimitationsManager = { hasPaidSubscription: sinon.stub() } this.TagsHandler = { getAllTags: sinon.stub() } @@ -140,6 +143,7 @@ describe('ProjectController', function () { './ProjectDeleter': this.ProjectDeleter, './ProjectDuplicator': this.ProjectDuplicator, './ProjectCreationHandler': this.ProjectCreationHandler, + './ProjectHistoryHandler': this.ProjectHistoryHandler, '../Editor/EditorController': this.EditorController, '../User/UserController': this.UserController, './ProjectHelper': this.ProjectHelper, @@ -1024,6 +1028,18 @@ describe('ProjectController', function () { this.ProjectController.loadEditor(this.req, this.res) }) + it('should ensureHistoryExistsForProject if saas and project_history enabled', function (done) { + this.Features.hasFeature.withArgs('saas').returns(true) + this.settings.apis.project_history = 'enabled' + this.res.render = (pageName, opts) => { + this.ProjectHistoryHandler.ensureHistoryExistsForProject + .calledWith(this.project_id) + .should.equal(true) + done() + } + this.ProjectController.loadEditor(this.req, this.res) + }) + it('should mark project as opened', function (done) { this.res.render = (pageName, opts) => { this.ProjectUpdateHandler.markAsOpened diff --git a/services/web/test/unit/src/Project/ProjectHistoryHandlerTests.js b/services/web/test/unit/src/Project/ProjectHistoryHandlerTests.js index 3a8451e282..6f5fc7f2e3 100644 --- a/services/web/test/unit/src/Project/ProjectHistoryHandlerTests.js +++ b/services/web/test/unit/src/Project/ProjectHistoryHandlerTests.js @@ -51,6 +51,7 @@ describe('ProjectHistoryHandler', function () { './ProjectDetailsHandler': (this.ProjectDetailsHandler = {}), '../History/HistoryManager': (this.HistoryManager = {}), './ProjectEntityUpdateHandler': (this.ProjectEntityUpdateHandler = {}), + '../DocumentUpdater/DocumentUpdaterHandler': (this.DocumentUpdaterHandler = {}), }, })) }) @@ -62,9 +63,10 @@ describe('ProjectHistoryHandler', function () { .stub() .callsArgWith(0, null, { overleaf_id: this.newHistoryId }) this.HistoryManager.flushProject = sinon.stub().callsArg(1) - return (this.ProjectEntityUpdateHandler.resyncProjectHistory = sinon + this.HistoryManager.forceResyncProject = sinon.stub().callsArg(1) + this.DocumentUpdaterHandler.flushProjectToMongoAndDelete = sinon .stub() - .callsArg(1)) + .callsArg(1) }) describe('when the history does not already exist', function () { @@ -101,14 +103,21 @@ describe('ProjectHistoryHandler', function () { .should.equal(true) }) - it('should resync the project history', function () { - return this.ProjectEntityUpdateHandler.resyncProjectHistory + it('should trigger a hard resync of the project history', function () { + return this.HistoryManager.forceResyncProject .calledWith(project_id) .should.equal(true) }) - it('should flush the project history', function () { + it('should flush the project history (twice)', function () { + this.HistoryManager.flushProject.calledTwice.should.equal(true) return this.HistoryManager.flushProject + .alwaysCalledWith(project_id) + .should.equal(true) + }) + + it('should tell docupdater to flush and delete', function () { + return this.DocumentUpdaterHandler.flushProjectToMongoAndDelete .calledWith(project_id) .should.equal(true) }) @@ -146,10 +155,8 @@ describe('ProjectHistoryHandler', function () { return this.ProjectModel.updateOne.called.should.equal(false) }) - it('should not resync the project history', function () { - return this.ProjectEntityUpdateHandler.resyncProjectHistory.called.should.equal( - false - ) + it('should not trigger a hard resync of the project history', function () { + return this.HistoryManager.forceResyncProject.called.should.equal(false) }) it('should not flush the project history', function () {