Merge pull request #5088 from overleaf/em-queue-feature-refreshes

Perform some user refreshes in the background

GitOrigin-RevId: 3aec73c827bf0f7de7bd9caa369dfc653eac5dd0

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,
   },
 }

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
           )

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) {

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 }

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,
 }

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)
           }

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()

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'],
+})

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) {

services/web/test/acceptance/src/mocks/MockRecurlyApi.js

@@ -32,7 +32,7 @@ class MockRecurlyApi extends AbstractMockApi {
       } else {
         res.send(`\
 <subscription>
-	<plan_code>${subscription.plan_code}</plan_code>
+	<plan><plan_code>${subscription.planCode}</plan_code></plan>
 	<currency>${subscription.currency}</currency>
 	<state>${subscription.state}</state>
 	<tax_in_cents type="integer">${subscription.tax_in_cents}</tax_in_cents>

services/web/test/unit/bootstrap.js

@@ -60,6 +60,7 @@ function getSandboxedModuleRequires() {
   ]
   const externalLibs = [
     'async',
+    'bull',
     'json2csv',
     'lodash',
     'marked',

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)
       }
     })
   })

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) {