From 148f6d6f96a7bf2ac614983f388fd0c9720bba70 Mon Sep 17 00:00:00 2001 From: David <33458145+davidmcpowell@users.noreply.github.com> Date: Mon, 19 Feb 2024 09:21:14 +0000 Subject: [PATCH] Merge pull request #17065 from overleaf/dp-mongoose-callback-inactive-project-manager Promisify InactiveProjectManager and InactiveProjectManagerTests GitOrigin-RevId: 985d0d4c80bfd1e46fa3c85c98203432459bdc84 --- .../InactiveData/InactiveProjectManager.js | 178 ++++++++---------- .../InactiveProjectManagerTests.js | 168 +++++++---------- 2 files changed, 154 insertions(+), 192 deletions(-) diff --git a/services/web/app/src/Features/InactiveData/InactiveProjectManager.js b/services/web/app/src/Features/InactiveData/InactiveProjectManager.js index 40f6b33f24..34a4f47fc5 100644 --- a/services/web/app/src/Features/InactiveData/InactiveProjectManager.js +++ b/services/web/app/src/Features/InactiveData/InactiveProjectManager.js @@ -1,17 +1,4 @@ -/* eslint-disable - max-len, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -let InactiveProjectManager const OError = require('@overleaf/o-error') -const async = require('async') -const _ = require('lodash') const logger = require('@overleaf/logger') const DocstoreManager = require('../Docstore/DocstoreManager') const ProjectGetter = require('../Project/ProjectGetter') @@ -19,43 +6,45 @@ const ProjectUpdateHandler = require('../Project/ProjectUpdateHandler') const { Project } = require('../../models/Project') const { ObjectId } = require('mongodb') const { READ_PREFERENCE_SECONDARY } = require('../../infrastructure/mongodb') +const { callbackifyAll } = require('@overleaf/promise-utils') const MILISECONDS_IN_DAY = 86400000 -module.exports = InactiveProjectManager = { - reactivateProjectIfRequired(projectId, callback) { - ProjectGetter.getProject( - projectId, - { active: true }, - function (err, project) { - if (err != null) { - OError.tag(err, 'error getting project', { - project_id: projectId, - }) - return callback(err) - } - logger.debug( - { projectId, active: project.active }, - 'seeing if need to reactivate project' - ) +const InactiveProjectManager = { + async reactivateProjectIfRequired(projectId) { + let project + try { + project = await ProjectGetter.promises.getProject(projectId, { + active: true, + }) + } catch (err) { + OError.tag(err, 'error getting project', { + project_id: projectId, + }) + throw err + } - if (project.active) { - return callback() - } - - DocstoreManager.unarchiveProject(projectId, function (err) { - if (err != null) { - OError.tag(err, 'error reactivating project in docstore', { - project_id: projectId, - }) - return callback(err) - } - ProjectUpdateHandler.markAsActive(projectId, callback) - }) - } + logger.debug( + { projectId, active: project.active }, + 'seeing if need to reactivate project' ) + + if (project.active) { + return + } + + try { + await DocstoreManager.promises.unarchiveProject(projectId) + } catch (err) { + OError.tag(err, 'error reactivating project in docstore', { + project_id: projectId, + }) + throw err + } + + await ProjectUpdateHandler.promises.markAsActive(projectId) }, - deactivateOldProjects(limit, daysOld, callback) { + async deactivateOldProjects(limit, daysOld) { if (limit == null) { limit = 10 } @@ -63,60 +52,59 @@ module.exports = InactiveProjectManager = { daysOld = 360 } const oldProjectDate = new Date() - MILISECONDS_IN_DAY * daysOld - // use $not $gt to catch non-opened projects where lastOpened is null - Project.find({ lastOpened: { $not: { $gt: oldProjectDate } } }) - .where('_id') - .lt(ObjectId.createFromTime(oldProjectDate / 1000)) - .where('active') - .equals(true) - .select('_id') - .sort({ _id: 1 }) - .limit(limit) - .read(READ_PREFERENCE_SECONDARY) - .exec(function (err, projects) { - if (err != null) { - logger.err({ err }, 'could not get projects for deactivating') - } - const jobs = _.map( - projects, - project => cb => - InactiveProjectManager.deactivateProject( - project._id, - function (err) { - if (err) { - logger.err( - { projectId: project._id, err }, - 'unable to deactivate project' - ) - } - cb() - } - ) - ) - logger.debug( - { numberOfProjects: projects && projects.length }, - 'deactivating projects' - ) - async.series(jobs, function (err) { - if (err != null) { - logger.warn({ err }, 'error deactivating projects') - } - callback(err, projects) - }) + + let projects + try { + // use $not $gt to catch non-opened projects where lastOpened is null + projects = await Project.find({ + lastOpened: { $not: { $gt: oldProjectDate } }, }) + .where('_id') + .lt(ObjectId.createFromTime(oldProjectDate / 1000)) + .where('active') + .equals(true) + .select('_id') + .sort({ _id: 1 }) + .limit(limit) + .read(READ_PREFERENCE_SECONDARY) + .exec() + } catch (err) { + logger.err({ err }, 'could not get projects for deactivating') + } + + logger.debug( + { numberOfProjects: projects && projects.length }, + 'deactivating projects' + ) + + for (const project of projects) { + try { + await InactiveProjectManager.deactivateProject(project._id) + } catch (err) { + logger.err( + { projectId: project._id, err }, + 'unable to deactivate project' + ) + } + } + + return projects }, - deactivateProject(projectId, callback) { + async deactivateProject(projectId) { logger.debug({ projectId }, 'deactivating inactive project') - const jobs = [ - cb => DocstoreManager.archiveProject(projectId, cb), - cb => ProjectUpdateHandler.markAsInactive(projectId, cb), - ] - async.series(jobs, function (err) { - if (err != null) { - logger.warn({ err, projectId }, 'error deactivating project') - } - callback(err) - }) + + try { + await DocstoreManager.promises.archiveProject(projectId) + await ProjectUpdateHandler.promises.markAsInactive(projectId) + } catch (err) { + logger.warn({ err, projectId }, 'error deactivating project') + throw err + } }, } + +module.exports = { + ...callbackifyAll(InactiveProjectManager), + promises: InactiveProjectManager, +} diff --git a/services/web/test/unit/src/InactiveData/InactiveProjectManagerTests.js b/services/web/test/unit/src/InactiveData/InactiveProjectManagerTests.js index 3c8f97cc7f..470e9eb2ed 100644 --- a/services/web/test/unit/src/InactiveData/InactiveProjectManagerTests.js +++ b/services/web/test/unit/src/InactiveData/InactiveProjectManagerTests.js @@ -1,39 +1,29 @@ -/* eslint-disable - n/handle-callback-err, - max-len, - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const SandboxedModule = require('sandboxed-module') -const assert = require('assert') const path = require('path') const sinon = require('sinon') const modulePath = path.join( __dirname, '../../../../app/src/Features/InactiveData/InactiveProjectManager' ) -const { expect } = require('chai') const { ObjectId, ReadPreference } = require('mongodb') +const { expect } = require('chai') describe('InactiveProjectManager', function () { beforeEach(function () { this.settings = {} this.DocstoreManager = { - unarchiveProject: sinon.stub(), - archiveProject: sinon.stub(), + promises: { + unarchiveProject: sinon.stub(), + archiveProject: sinon.stub(), + }, } this.ProjectUpdateHandler = { - markAsActive: sinon.stub(), - markAsInactive: sinon.stub(), + promises: { + markAsActive: sinon.stub(), + markAsInactive: sinon.stub(), + }, } - this.ProjectGetter = { getProject: sinon.stub() } + this.ProjectGetter = { promises: { getProject: sinon.stub() } } this.InactiveProjectManager = SandboxedModule.require(modulePath, { requires: { mongodb: { ObjectId }, @@ -48,106 +38,90 @@ describe('InactiveProjectManager', function () { }, }, }) - return (this.project_id = '1234') + this.project_id = '1234' }) describe('reactivateProjectIfRequired', function () { beforeEach(function () { this.project = { active: false } - this.ProjectGetter.getProject.callsArgWith(2, null, this.project) - return this.ProjectUpdateHandler.markAsActive.callsArgWith(1) + this.ProjectGetter.promises.getProject.resolves(this.project) + this.ProjectUpdateHandler.promises.markAsActive.resolves() }) - it('should call unarchiveProject', function (done) { - this.DocstoreManager.unarchiveProject.callsArgWith(1) - return this.InactiveProjectManager.reactivateProjectIfRequired( - this.project_id, - err => { - this.DocstoreManager.unarchiveProject - .calledWith(this.project_id) - .should.equal(true) - this.ProjectUpdateHandler.markAsActive - .calledWith(this.project_id) - .should.equal(true) - return done() - } + it('should call unarchiveProject', async function () { + this.DocstoreManager.promises.unarchiveProject.resolves() + await this.InactiveProjectManager.promises.reactivateProjectIfRequired( + this.project_id ) + + this.DocstoreManager.promises.unarchiveProject + .calledWith(this.project_id) + .should.equal(true) + this.ProjectUpdateHandler.promises.markAsActive + .calledWith(this.project_id) + .should.equal(true) }) - it('should not mark project as active if error with unarchiving', function (done) { - const error = new Error('error') - this.DocstoreManager.unarchiveProject.callsArgWith(1, error) - return this.InactiveProjectManager.reactivateProjectIfRequired( - this.project_id, - err => { - err.should.equal(error) - this.DocstoreManager.unarchiveProject - .calledWith(this.project_id) - .should.equal(true) - this.ProjectUpdateHandler.markAsActive - .calledWith(this.project_id) - .should.equal(false) - return done() - } - ) + it('should not mark project as active if error with unarchiving', async function () { + this.DocstoreManager.promises.unarchiveProject.rejects() + await expect( + this.InactiveProjectManager.promises.reactivateProjectIfRequired( + this.project_id + ) + ).to.be.rejected + + this.DocstoreManager.promises.unarchiveProject + .calledWith(this.project_id) + .should.equal(true) + this.ProjectUpdateHandler.promises.markAsActive + .calledWith(this.project_id) + .should.equal(false) }) - it('should not call unarchiveProject if it is active', function (done) { + it('should not call unarchiveProject if it is active', async function () { this.project.active = true - this.DocstoreManager.unarchiveProject.callsArgWith(1) - return this.InactiveProjectManager.reactivateProjectIfRequired( - this.project_id, - err => { - this.DocstoreManager.unarchiveProject - .calledWith(this.project_id) - .should.equal(false) - this.ProjectUpdateHandler.markAsActive - .calledWith(this.project_id) - .should.equal(false) - return done() - } + this.DocstoreManager.promises.unarchiveProject.resolves() + await this.InactiveProjectManager.promises.reactivateProjectIfRequired( + this.project_id ) + this.DocstoreManager.promises.unarchiveProject + .calledWith(this.project_id) + .should.equal(false) + this.ProjectUpdateHandler.promises.markAsActive + .calledWith(this.project_id) + .should.equal(false) }) }) describe('deactivateProject', function () { - it('should call unarchiveProject and markAsInactive', function (done) { - this.DocstoreManager.archiveProject.callsArgWith(1) + it('should call unarchiveProject and markAsInactive', async function () { + this.DocstoreManager.promises.archiveProject.resolves() + this.ProjectUpdateHandler.promises.markAsInactive.resolves() - this.ProjectUpdateHandler.markAsInactive.callsArgWith(1) - - return this.InactiveProjectManager.deactivateProject( - this.project_id, - err => { - this.DocstoreManager.archiveProject - .calledWith(this.project_id) - .should.equal(true) - this.ProjectUpdateHandler.markAsInactive - .calledWith(this.project_id) - .should.equal(true) - return done() - } + await this.InactiveProjectManager.promises.deactivateProject( + this.project_id ) + this.DocstoreManager.promises.archiveProject + .calledWith(this.project_id) + .should.equal(true) + this.ProjectUpdateHandler.promises.markAsInactive + .calledWith(this.project_id) + .should.equal(true) }) - it('should not call markAsInactive if there was a problem archiving in docstore', function (done) { - this.DocstoreManager.archiveProject.callsArgWith(1, 'errorrr') + it('should not call markAsInactive if there was a problem archiving in docstore', async function () { + this.DocstoreManager.promises.archiveProject.rejects() + this.ProjectUpdateHandler.promises.markAsInactive.resolves() - this.ProjectUpdateHandler.markAsInactive.callsArgWith(1) - - return this.InactiveProjectManager.deactivateProject( - this.project_id, - err => { - err.should.equal('errorrr') - this.DocstoreManager.archiveProject - .calledWith(this.project_id) - .should.equal(true) - this.ProjectUpdateHandler.markAsInactive - .calledWith(this.project_id) - .should.equal(false) - return done() - } - ) + await expect( + this.InactiveProjectManager.promises.deactivateProject(this.project_id) + ).to.be.rejected + this.DocstoreManager.promises.archiveProject + .calledWith(this.project_id) + .should.equal(true) + this.ProjectUpdateHandler.promises.markAsInactive + .calledWith(this.project_id) + .should.equal(false) }) }) })