From 2fdff8288b405f1e5e471f8afc52cdd32709713a Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Mon, 20 Sep 2021 08:20:33 -0400 Subject: [PATCH] Merge pull request #5088 from overleaf/em-queue-feature-refreshes Perform some user refreshes in the background GitOrigin-RevId: 3aec73c827bf0f7de7bd9caa369dfc653eac5dd0 --- .../Features/Subscription/FeaturesUpdater.js | 17 ++++ .../Subscription/SubscriptionHandler.js | 2 +- .../Subscription/SubscriptionUpdater.js | 14 ++- .../app/src/infrastructure/QueueWorkers.js | 7 ++ services/web/app/src/infrastructure/Queues.js | 8 +- services/web/app/src/util/promises.js | 31 ++++++- .../test/acceptance/src/helpers/InitApp.js | 5 ++ .../src/helpers/RecurlySubscription.js | 13 ++- .../test/acceptance/src/helpers/request.js | 1 + .../acceptance/src/mocks/MockRecurlyApi.js | 2 +- services/web/test/unit/bootstrap.js | 1 + .../Subscription/SubscriptionUpdaterTests.js | 43 ++++----- .../web/test/unit/src/util/promisesTests.js | 90 ++++++++++++++++++- 13 files changed, 202 insertions(+), 32 deletions(-) diff --git a/services/web/app/src/Features/Subscription/FeaturesUpdater.js b/services/web/app/src/Features/Subscription/FeaturesUpdater.js index 3e73f84d99..c1485008a0 100644 --- a/services/web/app/src/Features/Subscription/FeaturesUpdater.js +++ b/services/web/app/src/Features/Subscription/FeaturesUpdater.js @@ -12,7 +12,19 @@ const V1SubscriptionManager = require('./V1SubscriptionManager') const InstitutionsFeatures = require('../Institutions/InstitutionsFeatures') const UserGetter = require('../User/UserGetter') const AnalyticsManager = require('../Analytics/AnalyticsManager') +const Queues = require('../../infrastructure/Queues') +/** + * Enqueue a job for refreshing features for the given user + */ +async function scheduleRefreshFeatures(userId, reason) { + const queue = Queues.getRefreshFeaturesQueue() + await queue.add({ userId, reason }) +} + +/** + * Refresh features for the given user + */ async function refreshFeatures(userId, reason) { const user = await UserGetter.promises.getUser(userId, { _id: 1, @@ -45,6 +57,9 @@ async function refreshFeatures(userId, reason) { return { features: newFeatures, featuresChanged } } +/** + * Return the features that the given user should have. + */ async function computeFeatures(userId) { const individualFeatures = await _getIndividualFeatures(userId) const groupFeatureSets = await _getGroupFeatureSets(userId) @@ -191,9 +206,11 @@ module.exports = { 'features', 'featuresChanged', ]), + scheduleRefreshFeatures: callbackify(scheduleRefreshFeatures), promises: { computeFeatures, refreshFeatures, + scheduleRefreshFeatures, doSyncFromV1, }, } diff --git a/services/web/app/src/Features/Subscription/SubscriptionHandler.js b/services/web/app/src/Features/Subscription/SubscriptionHandler.js index 69c305d184..b9d9372f8b 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionHandler.js +++ b/services/web/app/src/Features/Subscription/SubscriptionHandler.js @@ -287,7 +287,7 @@ function syncSubscription(recurlySubscription, requesterData, callback) { } SubscriptionUpdater.syncSubscription( recurlySubscription, - user != null ? user._id : undefined, + user._id, requesterData, callback ) diff --git a/services/web/app/src/Features/Subscription/SubscriptionUpdater.js b/services/web/app/src/Features/Subscription/SubscriptionUpdater.js index 008d99d1ab..d0d20fa292 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionUpdater.js +++ b/services/web/app/src/Features/Subscription/SubscriptionUpdater.js @@ -110,7 +110,7 @@ async function deleteSubscription(subscription, deleterData) { await Subscription.deleteOne({ _id: subscription._id }).exec() // 3. refresh users features - await refreshUsersFeatures(subscription) + await _scheduleRefreshFeatures(subscription) } async function restoreSubscription(subscriptionId) { @@ -144,6 +144,16 @@ async function refreshUsersFeatures(subscription) { } } +async function _scheduleRefreshFeatures(subscription) { + const userIds = [subscription.admin_id].concat(subscription.member_ids || []) + for (const userId of userIds) { + await FeaturesUpdater.promises.scheduleRefreshFeatures( + userId, + 'subscription-updater' + ) + } +} + async function createDeletedSubscription(subscription, deleterData) { subscription.teamInvites = [] subscription.invited_emails = [] @@ -234,7 +244,7 @@ async function updateSubscriptionFromRecurly( } } await subscription.save() - await refreshUsersFeatures(subscription) + await _scheduleRefreshFeatures(subscription) } async function _sendUserGroupPlanCodeUserProperty(userId) { diff --git a/services/web/app/src/infrastructure/QueueWorkers.js b/services/web/app/src/infrastructure/QueueWorkers.js index 98967d5217..caf00108c2 100644 --- a/services/web/app/src/infrastructure/QueueWorkers.js +++ b/services/web/app/src/infrastructure/QueueWorkers.js @@ -2,6 +2,7 @@ const Features = require('./Features') const Queues = require('./Queues') const UserOnboardingEmailManager = require('../Features/User/UserOnboardingEmailManager') const UserPostRegistrationAnalyticsManager = require('../Features/User/UserPostRegistrationAnalyticsManager') +const FeaturesUpdater = require('../Features/Subscription/FeaturesUpdater') function start() { if (!Features.hasFeature('saas')) { @@ -19,6 +20,12 @@ function start() { const { userId } = job.data await UserPostRegistrationAnalyticsManager.postRegistrationAnalytics(userId) }) + + const refreshFeaturesQueue = Queues.getRefreshFeaturesQueue() + refreshFeaturesQueue.process(async job => { + const { userId, reason } = job.data + await FeaturesUpdater.promises.refreshFeatures(userId, reason) + }) } module.exports = { start } diff --git a/services/web/app/src/infrastructure/Queues.js b/services/web/app/src/infrastructure/Queues.js index d22d0ac61a..8e7014844c 100644 --- a/services/web/app/src/infrastructure/Queues.js +++ b/services/web/app/src/infrastructure/Queues.js @@ -27,6 +27,10 @@ function getAnalyticsUserPropertiesQueue() { } } +function getRefreshFeaturesQueue() { + return getOrCreateQueue('refresh-features', { attempts: 3 }) +} + function getOnboardingEmailsQueue() { return getOrCreateQueue('emails-onboarding') } @@ -35,7 +39,7 @@ function getPostRegistrationAnalyticsQueue() { return getOrCreateQueue('post-registration-analytics') } -function getOrCreateQueue(queueName, defaultJobOptions) { +function getOrCreateQueue(queueName, jobOptions = {}) { if (!queues[queueName]) { queues[queueName] = new Queue(queueName, { redis: Settings.redis.queues, @@ -47,6 +51,7 @@ function getOrCreateQueue(queueName, defaultJobOptions) { type: 'exponential', delay: 3000, }, + ...jobOptions, }, }) } @@ -57,6 +62,7 @@ module.exports = { getAnalyticsEventsQueue, getAnalyticsEditingSessionsQueue, getAnalyticsUserPropertiesQueue, + getRefreshFeaturesQueue, getOnboardingEmailsQueue, getPostRegistrationAnalyticsQueue, } diff --git a/services/web/app/src/util/promises.js b/services/web/app/src/util/promises.js index c1a68784ea..7af3b03759 100644 --- a/services/web/app/src/util/promises.js +++ b/services/web/app/src/util/promises.js @@ -4,6 +4,7 @@ const pLimit = require('p-limit') module.exports = { promisify, promisifyAll, + promisifyClass, promisifyMultiResult, callbackify, callbackifyMultiResult, @@ -57,6 +58,34 @@ function promisifyAll(module, opts = {}) { return promises } +/** + * Promisify all methods in a class. + * + * Options are the same as for promisifyAll + */ +function promisifyClass(cls, opts = {}) { + const promisified = class extends cls {} + const { without = [], multiResult = {} } = opts + for (const propName of Object.getOwnPropertyNames(cls.prototype)) { + if (propName === 'constructor' || without.includes(propName)) { + continue + } + const propValue = cls.prototype[propName] + if (typeof propValue !== 'function') { + continue + } + if (multiResult[propName] != null) { + promisified.prototype[propName] = promisifyMultiResult( + propValue, + multiResult[propName] + ) + } else { + promisified.prototype[propName] = promisify(propValue) + } + } + return promisified +} + /** * Promisify a function that returns multiple results via additional callback * parameters. @@ -78,7 +107,7 @@ function promisifyMultiResult(fn, resultNames) { function promisified(...args) { return new Promise((resolve, reject) => { try { - fn(...args, (err, ...results) => { + fn.bind(this)(...args, (err, ...results) => { if (err != null) { return reject(err) } diff --git a/services/web/test/acceptance/src/helpers/InitApp.js b/services/web/test/acceptance/src/helpers/InitApp.js index 06275d0d8f..d9cfa65621 100644 --- a/services/web/test/acceptance/src/helpers/InitApp.js +++ b/services/web/test/acceptance/src/helpers/InitApp.js @@ -1,4 +1,5 @@ const App = require('../../../../app.js') +const QueueWorkers = require('../../../../app/src/infrastructure/QueueWorkers') const MongoHelper = require('./MongoHelper') const RedisHelper = require('./RedisHelper') const { logger } = require('logger-sharelatex') @@ -14,6 +15,10 @@ before('start main app', function (done) { server = App.listen(3000, 'localhost', done) }) +before('start queue workers', function () { + QueueWorkers.start() +}) + after('stop main app', function (done) { if (!server) { return done() diff --git a/services/web/test/acceptance/src/helpers/RecurlySubscription.js b/services/web/test/acceptance/src/helpers/RecurlySubscription.js index 51dd7f3cb5..8ab1457f7c 100644 --- a/services/web/test/acceptance/src/helpers/RecurlySubscription.js +++ b/services/web/test/acceptance/src/helpers/RecurlySubscription.js @@ -2,6 +2,7 @@ const { ObjectId } = require('mongodb') const Subscription = require('./Subscription') const MockRecurlyApiClass = require('../mocks/MockRecurlyApi') const RecurlyWrapper = require('../../../../app/src/Features/Subscription/RecurlyWrapper') +const { promisifyClass } = require('../../../../app/src/util/promises') let MockRecurlyApi @@ -27,6 +28,7 @@ class RecurlySubscription { email: options.account && options.account.email, hosted_login_token: options.account && options.account.hosted_login_token, } + this.planCode = options.planCode || 'personal' } ensureExists(callback) { @@ -39,13 +41,13 @@ class RecurlySubscription { }) } - buildCallbackXml() { - return RecurlyWrapper._buildXml('expired_subscription_notification', { + buildCallbackXml(event) { + return RecurlyWrapper._buildXml(event, { subscription: { uuid: this.uuid, - state: 'expired', + state: this.state, plan: { - plan_code: 'collaborator', + plan_code: this.planCode, }, }, account: { @@ -56,3 +58,6 @@ class RecurlySubscription { } module.exports = RecurlySubscription +module.exports.promises = promisifyClass(RecurlySubscription, { + without: ['buildCallbackXml'], +}) diff --git a/services/web/test/acceptance/src/helpers/request.js b/services/web/test/acceptance/src/helpers/request.js index 337cd59932..577cf3f0e6 100644 --- a/services/web/test/acceptance/src/helpers/request.js +++ b/services/web/test/acceptance/src/helpers/request.js @@ -7,6 +7,7 @@ const request = require('request').defaults({ }) module.exports = request +module.exports.BASE_URL = BASE_URL module.exports.promises = { request: function (options) { diff --git a/services/web/test/acceptance/src/mocks/MockRecurlyApi.js b/services/web/test/acceptance/src/mocks/MockRecurlyApi.js index 38909c9a29..b522820950 100644 --- a/services/web/test/acceptance/src/mocks/MockRecurlyApi.js +++ b/services/web/test/acceptance/src/mocks/MockRecurlyApi.js @@ -32,7 +32,7 @@ class MockRecurlyApi extends AbstractMockApi { } else { res.send(`\ - ${subscription.plan_code} + ${subscription.planCode} ${subscription.currency} ${subscription.state} ${subscription.tax_in_cents} diff --git a/services/web/test/unit/bootstrap.js b/services/web/test/unit/bootstrap.js index 0dcb497324..132f3ff2ad 100644 --- a/services/web/test/unit/bootstrap.js +++ b/services/web/test/unit/bootstrap.js @@ -60,6 +60,7 @@ function getSandboxedModuleRequires() { ] const externalLibs = [ 'async', + 'bull', 'json2csv', 'lodash', 'marked', diff --git a/services/web/test/unit/src/Subscription/SubscriptionUpdaterTests.js b/services/web/test/unit/src/Subscription/SubscriptionUpdaterTests.js index f60de1d3ae..34c5e6c37c 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionUpdaterTests.js +++ b/services/web/test/unit/src/Subscription/SubscriptionUpdaterTests.js @@ -125,6 +125,7 @@ describe('SubscriptionUpdater', function () { this.FeaturesUpdater = { promises: { + scheduleRefreshFeatures: sinon.stub().resolves(), refreshFeatures: sinon.stub().resolves({}), }, } @@ -249,9 +250,9 @@ describe('SubscriptionUpdater', function () { this.recurlySubscription.plan.plan_code ) this.subscription.save.called.should.equal(true) - this.FeaturesUpdater.promises.refreshFeatures - .calledWith(this.adminUser._id) - .should.equal(true) + expect( + this.FeaturesUpdater.promises.scheduleRefreshFeatures + ).to.have.been.calledWith(this.adminUser._id) }) it('should remove the subscription when expired', async function () { @@ -270,18 +271,18 @@ describe('SubscriptionUpdater', function () { this.subscription, {} ) - this.FeaturesUpdater.promises.refreshFeatures - .calledWith(this.adminUser._id) - .should.equal(true) - this.FeaturesUpdater.promises.refreshFeatures - .calledWith(this.allUserIds[0]) - .should.equal(true) - this.FeaturesUpdater.promises.refreshFeatures - .calledWith(this.allUserIds[1]) - .should.equal(true) - this.FeaturesUpdater.promises.refreshFeatures - .calledWith(this.allUserIds[2]) - .should.equal(true) + expect( + this.FeaturesUpdater.promises.scheduleRefreshFeatures + ).to.have.been.calledWith(this.adminUser._id) + expect( + this.FeaturesUpdater.promises.scheduleRefreshFeatures + ).to.have.been.calledWith(this.allUserIds[0]) + expect( + this.FeaturesUpdater.promises.scheduleRefreshFeatures + ).to.have.been.calledWith(this.allUserIds[1]) + expect( + this.FeaturesUpdater.promises.scheduleRefreshFeatures + ).to.have.been.calledWith(this.allUserIds[2]) }) it('should set group to true and save how many members can be added to group', async function () { @@ -538,16 +539,16 @@ describe('SubscriptionUpdater', function () { }) it('should downgrade the admin_id', function () { - this.FeaturesUpdater.promises.refreshFeatures - .calledWith(this.subscription.admin_id) - .should.equal(true) + expect( + this.FeaturesUpdater.promises.scheduleRefreshFeatures + ).to.have.been.calledWith(this.subscription.admin_id) }) it('should downgrade all of the members', function () { for (const userId of this.subscription.member_ids) { - this.FeaturesUpdater.promises.refreshFeatures - .calledWith(userId) - .should.equal(true) + expect( + this.FeaturesUpdater.promises.scheduleRefreshFeatures + ).to.have.been.calledWith(userId) } }) }) diff --git a/services/web/test/unit/src/util/promisesTests.js b/services/web/test/unit/src/util/promisesTests.js index cecfa395cf..64ffa4cbb1 100644 --- a/services/web/test/unit/src/util/promisesTests.js +++ b/services/web/test/unit/src/util/promisesTests.js @@ -1,6 +1,7 @@ const { expect } = require('chai') const { promisifyAll, + promisifyClass, callbackifyMultiResult, } = require('../../../../app/src/util/promises') @@ -48,7 +49,7 @@ describe('promisifyAll', function () { return a + b }, } - this.promisified = promisifyAll(this.module, { without: 'syncAdd' }) + this.promisified = promisifyAll(this.module, { without: ['syncAdd'] }) }) it('does not promisify excluded functions', function () { @@ -88,6 +89,93 @@ describe('promisifyAll', function () { }) }) +describe('promisifyClass', function () { + describe('basic functionality', function () { + before(function () { + this.Class = class { + constructor(a) { + this.a = a + } + + asyncAdd(b, callback) { + callback(null, this.a + b) + } + } + this.Promisified = promisifyClass(this.Class) + }) + + it('promisifies the class methods', async function () { + const adder = new this.Promisified(1) + const sum = await adder.asyncAdd(2) + expect(sum).to.equal(3) + }) + }) + + describe('without option', function () { + before(function () { + this.Class = class { + constructor(a) { + this.a = a + } + + asyncAdd(b, callback) { + callback(null, this.a + b) + } + + syncAdd(b) { + return this.a + b + } + } + this.Promisified = promisifyClass(this.Class, { without: ['syncAdd'] }) + }) + + it('does not promisify excluded functions', function () { + const adder = new this.Promisified(10) + const sum = adder.syncAdd(12) + expect(sum).to.equal(22) + }) + + it('promisifies other functions', async function () { + const adder = new this.Promisified(23) + const sum = await adder.asyncAdd(3) + expect(sum).to.equal(26) + }) + }) + + describe('multiResult option', function () { + before(function () { + this.Class = class { + constructor(a) { + this.a = a + } + + asyncAdd(b, callback) { + callback(null, this.a + b) + } + + asyncArithmetic(b, callback) { + callback(null, this.a + b, this.a * b) + } + } + this.Promisified = promisifyClass(this.Class, { + multiResult: { asyncArithmetic: ['sum', 'product'] }, + }) + }) + + it('promisifies multi-result functions', async function () { + const adder = new this.Promisified(3) + const result = await adder.asyncArithmetic(6) + expect(result).to.deep.equal({ sum: 9, product: 18 }) + }) + + it('promisifies other functions normally', async function () { + const adder = new this.Promisified(6) + const sum = await adder.asyncAdd(1) + expect(sum).to.equal(7) + }) + }) +}) + describe('callbackifyMultiResult', function () { it('callbackifies a multi-result function', function (done) { async function asyncArithmetic(a, b) {