diff --git a/services/web/app/src/Features/Cooldown/CooldownManager.js b/services/web/app/src/Features/Cooldown/CooldownManager.js index 26aff88808..873e7723cb 100644 --- a/services/web/app/src/Features/Cooldown/CooldownManager.js +++ b/services/web/app/src/Features/Cooldown/CooldownManager.js @@ -14,6 +14,7 @@ let CooldownManager const RedisWrapper = require('../../infrastructure/RedisWrapper') const rclient = RedisWrapper.client('cooldown') const logger = require('@overleaf/logger') +const { promisifyAll } = require('../../util/promises') const COOLDOWN_IN_SECONDS = 60 * 10 @@ -54,3 +55,7 @@ module.exports = CooldownManager = { ) }, } + +module.exports.promises = promisifyAll(module.exports, { + without: ['_buildKey'], +}) diff --git a/services/web/app/src/Features/Project/ProjectRootDocManager.js b/services/web/app/src/Features/Project/ProjectRootDocManager.js index 3e984276d2..0f712787fb 100644 --- a/services/web/app/src/Features/Project/ProjectRootDocManager.js +++ b/services/web/app/src/Features/Project/ProjectRootDocManager.js @@ -24,6 +24,7 @@ const { promisify } = require('util') const async = require('async') const globby = require('globby') const _ = require('underscore') +const { promisifyAll } = require('../../util/promises') module.exports = ProjectRootDocManager = { setRootDocAutomatically(project_id, callback) { @@ -300,27 +301,10 @@ module.exports = ProjectRootDocManager = { }, } -const promises = { - setRootDocAutomatically: promisify( - ProjectRootDocManager.setRootDocAutomatically - ), - - findRootDocFileFromDirectory: directoryPath => - new Promise((resolve, reject) => { - ProjectRootDocManager.findRootDocFileFromDirectory( - directoryPath, - (error, path, content) => { - if (error) { - reject(error) - } else { - resolve({ path, content }) - } - } - ) - }), - setRootDocFromName: promisify(ProjectRootDocManager.setRootDocFromName), -} - -ProjectRootDocManager.promises = promises - module.exports = ProjectRootDocManager +module.exports.promises = promisifyAll(module.exports, { + without: ['_rootDocSort'], + multiResult: { + findRootDocFileFromDirectory: ['path', 'content'], + }, +}) diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js b/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js index b35d1ff516..072ea8d93e 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js @@ -1,4 +1,4 @@ -const { promisify } = require('util') +const { callbackify } = require('util') const UpdateMerger = require('./UpdateMerger') const logger = require('@overleaf/logger') const NotificationsBuilder = require('../Notifications/NotificationsBuilder') @@ -19,157 +19,135 @@ const ROOT_DOC_TIMEOUT_LENGTH = 30 * 1000 const rootDocResets = new BackgroundTaskTracker('root doc resets') -function newUpdate(userId, projectName, path, updateRequest, source, callback) { - getOrCreateProject(userId, projectName, (err, project) => { - if (err) { - return callback(err) - } - if (project == null) { - return callback() - } - CooldownManager.isProjectOnCooldown( - project._id, - (err, projectIsOnCooldown) => { - if (err) { - return callback(err) - } - if (projectIsOnCooldown) { - return callback( - new Errors.TooManyRequestsError('project on cooldown') - ) - } - FileTypeManager.shouldIgnore(path, (err, shouldIgnore) => { - if (err) { - return callback(err) - } - if (shouldIgnore) { - return callback() - } - UpdateMerger.mergeUpdate( - userId, - project._id, - path, - updateRequest, - source, - callback - ) - }) - } - ) - }) +async function newUpdate(userId, projectName, path, updateRequest, source) { + const project = await getOrCreateProject(userId, projectName) + if (project == null) { + return + } + + const projectIsOnCooldown = + await CooldownManager.promises.isProjectOnCooldown(project._id) + if (projectIsOnCooldown) { + throw new Errors.TooManyRequestsError('project on cooldown') + } + + const shouldIgnore = await FileTypeManager.promises.shouldIgnore(path) + if (shouldIgnore) { + return + } + + await UpdateMerger.promises.mergeUpdate( + userId, + project._id, + path, + updateRequest, + source + ) } -function deleteUpdate(userId, projectName, path, source, callback) { +async function deleteUpdate(userId, projectName, path, source) { logger.debug({ userId, filePath: path }, 'handling delete update from tpds') - ProjectGetter.findUsersProjectsByName( + + const projects = await ProjectGetter.promises.findUsersProjectsByName( userId, - projectName, - (err, projects) => { - if (err) { - return callback(err) - } - const activeProjects = projects.filter( - project => !ProjectHelper.isArchivedOrTrashed(project, userId) - ) - if (activeProjects.length === 0) { - logger.debug( - { userId, filePath: path, projectName }, - 'project not found from tpds update, ignoring folder or project' - ) - return callback() - } - if (projects.length > 1) { - // There is more than one project with that name, and one of them is - // active (previous condition) - return handleDuplicateProjects(userId, projectName, callback) - } - - const project = activeProjects[0] - if (path === '/') { - logger.debug( - { userId, filePath: path, projectName, project_id: project._id }, - 'project found for delete update, path is root so marking project as deleted' - ) - ProjectDeleter.markAsDeletedByExternalSource(project._id, callback) - } else { - UpdateMerger.deleteUpdate(userId, project._id, path, source, err => { - callback(err) - }) - } - } + projectName ) -} - -function getOrCreateProject(userId, projectName, callback) { - ProjectGetter.findUsersProjectsByName( - userId, - projectName, - (err, projects) => { - if (err) { - return callback(err) - } - - if (projects.length === 0) { - // No project with that name -- active, archived or trashed -- has been - // found. Create one. - return ProjectCreationHandler.createBlankProject( - userId, - projectName, - (err, project) => { - // have a crack at setting the root doc after a while, on creation - // we won't have it yet, but should have been sent it it within 30 - // seconds - rootDocResets.add() - setTimeout(() => { - ProjectRootDocManager.setRootDocAutomatically(project._id, () => { - rootDocResets.done() - }) - }, ROOT_DOC_TIMEOUT_LENGTH) - callback(err, project) - } - ) - } - const activeProjects = projects.filter( - project => !ProjectHelper.isArchivedOrTrashed(project, userId) - ) - if (activeProjects.length === 0) { - // All projects with that name are archived or trashed. Ignore. - return callback(null, null) - } - - if (projects.length > 1) { - // There is more than one project with that name, and one of them is - // active (previous condition) - return handleDuplicateProjects(userId, projectName, err => { - if (err) { - return callback(err) - } - callback(null, null) - }) - } - - callback(err, activeProjects[0]) - } + const activeProjects = projects.filter( + project => !ProjectHelper.isArchivedOrTrashed(project, userId) ) -} -function handleDuplicateProjects(userId, projectName, callback) { - Modules.hooks.fire('removeDropbox', userId, 'duplicate-projects', err => { - if (err) { - return callback(err) - } - NotificationsBuilder.dropboxDuplicateProjectNames(userId).create( - projectName, - callback + if (activeProjects.length === 0) { + logger.debug( + { userId, filePath: path, projectName }, + 'project not found from tpds update, ignoring folder or project' ) - }) + return + } + + if (projects.length > 1) { + // There is more than one project with that name, and one of them is + // active (previous condition) + await handleDuplicateProjects(userId, projectName) + return + } + + const project = activeProjects[0] + if (path === '/') { + logger.debug( + { userId, filePath: path, projectName, project_id: project._id }, + 'project found for delete update, path is root so marking project as deleted' + ) + await ProjectDeleter.promises.markAsDeletedByExternalSource(project._id) + } else { + await UpdateMerger.promises.deleteUpdate(userId, project._id, path, source) + } +} + +async function getOrCreateProject(userId, projectName) { + const projects = await ProjectGetter.promises.findUsersProjectsByName( + userId, + projectName + ) + + if (projects.length === 0) { + // No project with that name -- active, archived or trashed -- has been + // found. Create one. + const project = await ProjectCreationHandler.promises.createBlankProject( + userId, + projectName + ) + + // have a crack at setting the root doc after a while, on creation + // we won't have it yet, but should have been sent it it within 30 + // seconds + rootDocResets.add() + setTimeout(() => { + ProjectRootDocManager.promises + .setRootDocAutomatically(project._id) + .then(() => { + rootDocResets.done() + }) + .catch(err => { + logger.warn({ err }, 'failed to set root doc after project creation') + }) + }, ROOT_DOC_TIMEOUT_LENGTH) + return project + } + + const activeProjects = projects.filter( + project => !ProjectHelper.isArchivedOrTrashed(project, userId) + ) + if (activeProjects.length === 0) { + // All projects with that name are archived or trashed. Ignore. + return null + } + + if (projects.length > 1) { + // There is more than one project with that name, and one of them is + // active (previous condition) + await handleDuplicateProjects(userId, projectName) + return null + } + + return activeProjects[0] +} + +async function handleDuplicateProjects(userId, projectName) { + await Modules.promises.hooks.fire( + 'removeDropbox', + userId, + 'duplicate-projects' + ) + await NotificationsBuilder.promises + .dropboxDuplicateProjectNames(userId) + .create(projectName) } module.exports = { - newUpdate, - deleteUpdate, + newUpdate: callbackify(newUpdate), + deleteUpdate: callbackify(deleteUpdate), promises: { - newUpdate: promisify(newUpdate), - deleteUpdate: promisify(deleteUpdate), + newUpdate, + deleteUpdate, }, } diff --git a/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandlerTests.js b/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandlerTests.js index 5fd46ad32d..2d34c74e49 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandlerTests.js +++ b/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandlerTests.js @@ -38,28 +38,42 @@ describe('TpdsUpdateHandler', function () { this.update = {} this.CooldownManager = { - isProjectOnCooldown: sinon.stub().yields(null, false), + promises: { + isProjectOnCooldown: sinon.stub().resolves(false), + }, } this.FileTypeManager = { - shouldIgnore: sinon.stub().yields(null, false), + promises: { + shouldIgnore: sinon.stub().resolves(false), + }, } this.Modules = { - hooks: { fire: sinon.stub().yields() }, + promises: { + hooks: { fire: sinon.stub().resolves() }, + }, } this.notification = { - create: sinon.stub().yields(), + create: sinon.stub().resolves(), } this.NotificationsBuilder = { - dropboxDuplicateProjectNames: sinon.stub().returns(this.notification), + promises: { + dropboxDuplicateProjectNames: sinon.stub().returns(this.notification), + }, } this.ProjectCreationHandler = { - createBlankProject: sinon.stub().yields(null, this.projects.active1), + promises: { + createBlankProject: sinon.stub().resolves(this.projects.active1), + }, } this.ProjectDeleter = { - markAsDeletedByExternalSource: sinon.stub().yields(), + promises: { + markAsDeletedByExternalSource: sinon.stub().resolves(), + }, } this.ProjectGetter = { - findUsersProjectsByName: sinon.stub(), + promises: { + findUsersProjectsByName: sinon.stub(), + }, } this.ProjectHelper = { isArchivedOrTrashed: sinon.stub().returns(false), @@ -70,10 +84,16 @@ describe('TpdsUpdateHandler', function () { this.ProjectHelper.isArchivedOrTrashed .withArgs(this.projects.archived2, this.userId) .returns(true) - this.RootDocManager = { setRootDocAutomatically: sinon.stub() } + this.RootDocManager = { + promises: { + setRootDocAutomatically: sinon.stub().resolves(), + }, + } this.UpdateMerger = { - deleteUpdate: sinon.stub().yields(), - mergeUpdate: sinon.stub().yields(), + promises: { + deleteUpdate: sinon.stub().resolves(), + mergeUpdate: sinon.stub().resolves(), + }, } this.TpdsUpdateHandler = SandboxedModule.require(MODULE_PATH, { @@ -139,10 +159,10 @@ describe('TpdsUpdateHandler', function () { expectDropboxUnlinked() }) - describe('update to a file that should be ignored', function (done) { + describe('update to a file that should be ignored', async function () { setupMatchingProjects(['active1']) beforeEach(function () { - this.FileTypeManager.shouldIgnore.yields(null, true) + this.FileTypeManager.promises.shouldIgnore.resolves(true) }) receiveUpdate() expectProjectNotCreated() @@ -150,21 +170,19 @@ describe('TpdsUpdateHandler', function () { expectDropboxNotUnlinked() }) - describe('update to a project on cooldown', function (done) { + describe('update to a project on cooldown', async function () { setupMatchingProjects(['active1']) setupProjectOnCooldown() - beforeEach(function (done) { - this.TpdsUpdateHandler.newUpdate( - this.userId, - this.projectName, - this.path, - this.update, - this.source, - err => { - expect(err).to.be.instanceof(Errors.TooManyRequestsError) - done() - } - ) + beforeEach(async function () { + await expect( + this.TpdsUpdateHandler.promises.newUpdate( + this.userId, + this.projectName, + this.path, + this.update, + this.source + ) + ).to.be.rejectedWith(Errors.TooManyRequestsError) }) expectUpdateNotProcessed() }) @@ -270,55 +288,52 @@ describe('TpdsUpdateHandler', function () { function setupMatchingProjects(projectKeys) { beforeEach(function () { const projects = projectKeys.map(key => this.projects[key]) - this.ProjectGetter.findUsersProjectsByName + this.ProjectGetter.promises.findUsersProjectsByName .withArgs(this.userId, this.projectName) - .yields(null, projects) + .resolves(projects) }) } function setupProjectOnCooldown() { beforeEach(function () { - this.CooldownManager.isProjectOnCooldown + this.CooldownManager.promises.isProjectOnCooldown .withArgs(this.projects.active1._id) - .yields(null, true) + .resolves(true) }) } /* Test helpers */ function receiveUpdate() { - beforeEach(function (done) { - this.TpdsUpdateHandler.newUpdate( + beforeEach(async function () { + await this.TpdsUpdateHandler.promises.newUpdate( this.userId, this.projectName, this.path, this.update, - this.source, - done + this.source ) }) } function receiveFileDelete() { - beforeEach(function (done) { - this.TpdsUpdateHandler.deleteUpdate( + beforeEach(async function () { + await this.TpdsUpdateHandler.promises.deleteUpdate( this.userId, this.projectName, this.path, - this.source, - done + this.source ) }) } function receiveProjectDelete() { - beforeEach(function (done) { - this.TpdsUpdateHandler.deleteUpdate( + beforeEach(async function () { + await this.TpdsUpdateHandler.promises.deleteUpdate( this.userId, this.projectName, '/', - this.source, - done + this.source ) }) } @@ -328,35 +343,36 @@ function receiveProjectDelete() { function expectProjectCreated() { it('creates a project', function () { expect( - this.ProjectCreationHandler.createBlankProject + this.ProjectCreationHandler.promises.createBlankProject ).to.have.been.calledWith(this.userId, this.projectName) }) it('sets the root doc', function () { // Fire pending timers this.clock.runAll() - expect(this.RootDocManager.setRootDocAutomatically).to.have.been.calledWith( - this.projects.active1._id - ) + expect( + this.RootDocManager.promises.setRootDocAutomatically + ).to.have.been.calledWith(this.projects.active1._id) }) } function expectProjectNotCreated() { it('does not create a project', function () { - expect(this.ProjectCreationHandler.createBlankProject).not.to.have.been - .called + expect(this.ProjectCreationHandler.promises.createBlankProject).not.to.have + .been.called }) it('does not set the root doc', function () { // Fire pending timers this.clock.runAll() - expect(this.RootDocManager.setRootDocAutomatically).not.to.have.been.called + expect(this.RootDocManager.promises.setRootDocAutomatically).not.to.have + .been.called }) } function expectUpdateProcessed() { it('processes the update', function () { - expect(this.UpdateMerger.mergeUpdate).to.have.been.calledWith( + expect(this.UpdateMerger.promises.mergeUpdate).to.have.been.calledWith( this.userId, this.projects.active1._id, this.path, @@ -368,13 +384,13 @@ function expectUpdateProcessed() { function expectUpdateNotProcessed() { it('does not process the update', function () { - expect(this.UpdateMerger.mergeUpdate).not.to.have.been.called + expect(this.UpdateMerger.promises.mergeUpdate).not.to.have.been.called }) } function expectDropboxUnlinked() { it('unlinks Dropbox', function () { - expect(this.Modules.hooks.fire).to.have.been.calledWith( + expect(this.Modules.promises.hooks.fire).to.have.been.calledWith( 'removeDropbox', this.userId, 'duplicate-projects' @@ -383,7 +399,7 @@ function expectDropboxUnlinked() { it('creates a notification that dropbox was unlinked', function () { expect( - this.NotificationsBuilder.dropboxDuplicateProjectNames + this.NotificationsBuilder.promises.dropboxDuplicateProjectNames ).to.have.been.calledWith(this.userId) expect(this.notification.create).to.have.been.calledWith(this.projectName) }) @@ -391,18 +407,18 @@ function expectDropboxUnlinked() { function expectDropboxNotUnlinked() { it('does not unlink Dropbox', function () { - expect(this.Modules.hooks.fire).not.to.have.been.called + expect(this.Modules.promises.hooks.fire).not.to.have.been.called }) it('does not create a notification that dropbox was unlinked', function () { - expect(this.NotificationsBuilder.dropboxDuplicateProjectNames).not.to.have - .been.called + expect(this.NotificationsBuilder.promises.dropboxDuplicateProjectNames).not + .to.have.been.called }) } function expectDeleteProcessed() { it('processes the delete', function () { - expect(this.UpdateMerger.deleteUpdate).to.have.been.calledWith( + expect(this.UpdateMerger.promises.deleteUpdate).to.have.been.calledWith( this.userId, this.projects.active1._id, this.path, @@ -413,21 +429,21 @@ function expectDeleteProcessed() { function expectDeleteNotProcessed() { it('does not process the delete', function () { - expect(this.UpdateMerger.deleteUpdate).not.to.have.been.called + expect(this.UpdateMerger.promises.deleteUpdate).not.to.have.been.called }) } function expectProjectDeleted() { it('deletes the project', function () { expect( - this.ProjectDeleter.markAsDeletedByExternalSource + this.ProjectDeleter.promises.markAsDeletedByExternalSource ).to.have.been.calledWith(this.projects.active1._id) }) } function expectProjectNotDeleted() { it('does not delete the project', function () { - expect(this.ProjectDeleter.markAsDeletedByExternalSource).not.to.have.been - .called + expect(this.ProjectDeleter.promises.markAsDeletedByExternalSource).not.to + .have.been.called }) }