Merge pull request #5087 from overleaf/em-promisify-subscription-updater

Promisify FeaturesUpdater and SubscriptionHandler

GitOrigin-RevId: 1a9725afa119c0eaee3d975a11197b6f702f1307

services/web/app/src/Features/Institutions/InstitutionsFeatures.js

@@ -2,6 +2,7 @@ let InstitutionsFeatures
 const UserGetter = require('../User/UserGetter')
 const PlansLocator = require('../Subscription/PlansLocator')
 const Settings = require('@overleaf/settings')
+const { promisifyAll } = require('../../util/promises')
 
 module.exports = InstitutionsFeatures = {
   getInstitutionsFeatures(userId, callback) {
@@ -44,3 +45,5 @@ module.exports = InstitutionsFeatures = {
     })
   },
 }
+
+module.exports.promises = promisifyAll(module.exports)

services/web/app/src/Features/Institutions/InstitutionsManager.js

@@ -7,6 +7,7 @@ const {
   promises: InstitutionsAPIPromises,
 } = require('./InstitutionsAPI')
 const FeaturesUpdater = require('../Subscription/FeaturesUpdater')
+const FeaturesHelper = require('../Subscription/FeaturesHelper')
 const UserGetter = require('../User/UserGetter')
 const NotificationsBuilder = require('../Notifications/NotificationsBuilder')
 const SubscriptionLocator = require('../Subscription/SubscriptionLocator')
@@ -55,7 +56,7 @@ async function _checkUsersFeatures(userIds) {
   }
 
   users.forEach(user => {
-    const hasProFeaturesOrBetter = FeaturesUpdater.isFeatureSetBetter(
+    const hasProFeaturesOrBetter = FeaturesHelper.isFeatureSetBetter(
       user.features,
       Settings.features.professional
     )

services/web/app/src/Features/Referal/ReferalFeatures.js

@@ -1,4 +1,5 @@
 const _ = require('underscore')
+const { promisify } = require('util')
 const { User } = require('../../models/User')
 const Settings = require('@overleaf/settings')
 
@@ -47,3 +48,7 @@ module.exports = ReferalFeatures = {
     return highestBonusLevel
   },
 }
+
+module.exports.promises = {
+  getBonusFeatures: promisify(module.exports.getBonusFeatures),
+}

services/web/app/src/Features/Subscription/FeaturesHelper.js

@@ -0,0 +1,94 @@
+const _ = require('lodash')
+
+/**
+ * Merge feature sets coming from different sources
+ */
+function mergeFeatures(featuresA, featuresB) {
+  const features = Object.assign({}, featuresA)
+  for (const key in featuresB) {
+    // Special merging logic for non-boolean features
+    if (key === 'compileGroup') {
+      if (
+        features.compileGroup === 'priority' ||
+        featuresB.compileGroup === 'priority'
+      ) {
+        features.compileGroup = 'priority'
+      } else {
+        features.compileGroup = 'standard'
+      }
+    } else if (key === 'collaborators') {
+      if (features.collaborators === -1 || featuresB.collaborators === -1) {
+        features.collaborators = -1
+      } else {
+        features.collaborators = Math.max(
+          features.collaborators || 0,
+          featuresB.collaborators || 0
+        )
+      }
+    } else if (key === 'compileTimeout') {
+      features.compileTimeout = Math.max(
+        features.compileTimeout || 0,
+        featuresB.compileTimeout || 0
+      )
+    } else {
+      // Boolean keys, true is better
+      features[key] = features[key] || featuresB[key]
+    }
+  }
+  return features
+}
+
+/**
+ * Returns whether `featuresA` is a better feature set than `featuresB`
+ */
+function isFeatureSetBetter(featuresA, featuresB) {
+  const mergedFeatures = mergeFeatures(featuresA, featuresB)
+  return _.isEqual(featuresA, mergedFeatures)
+}
+
+/**
+ * Return what's missing from `currentFeatures` to equal `expectedFeatures`
+ */
+function compareFeatures(currentFeatures, expectedFeatures) {
+  currentFeatures = _.clone(currentFeatures)
+  expectedFeatures = _.clone(expectedFeatures)
+  if (_.isEqual(currentFeatures, expectedFeatures)) {
+    return {}
+  }
+
+  const mismatchReasons = {}
+  const featureKeys = [
+    ...new Set([
+      ...Object.keys(currentFeatures),
+      ...Object.keys(expectedFeatures),
+    ]),
+  ]
+  featureKeys.sort().forEach(key => {
+    if (expectedFeatures[key] !== currentFeatures[key]) {
+      mismatchReasons[key] = expectedFeatures[key]
+    }
+  })
+
+  if (mismatchReasons.compileTimeout) {
+    // store the compile timeout difference instead of the new compile timeout
+    mismatchReasons.compileTimeout =
+      expectedFeatures.compileTimeout - currentFeatures.compileTimeout
+  }
+
+  if (mismatchReasons.collaborators) {
+    // store the collaborators difference instead of the new number only
+    // replace -1 by 100 to make it clearer
+    if (expectedFeatures.collaborators === -1) {
+      expectedFeatures.collaborators = 100
+    }
+    if (currentFeatures.collaborators === -1) {
+      currentFeatures.collaborators = 100
+    }
+    mismatchReasons.collaborators =
+      expectedFeatures.collaborators - currentFeatures.collaborators
+  }
+
+  return mismatchReasons
+}
+
+module.exports = { mergeFeatures, isFeatureSetBetter, compareFeatures }

services/web/app/src/Features/Subscription/FeaturesUpdater.js

@@ -1,9 +1,10 @@
-const async = require('async')
-const OError = require('@overleaf/o-error')
-const PlansLocator = require('./PlansLocator')
 const _ = require('lodash')
+const { callbackify } = require('util')
+const { callbackifyMultiResult } = require('../../util/promises')
+const PlansLocator = require('./PlansLocator')
 const SubscriptionLocator = require('./SubscriptionLocator')
 const UserFeaturesUpdater = require('./UserFeaturesUpdater')
+const FeaturesHelper = require('./FeaturesHelper')
 const Settings = require('@overleaf/settings')
 const logger = require('logger-sharelatex')
 const ReferalFeatures = require('../Referal/ReferalFeatures')
@@ -12,339 +13,187 @@ const InstitutionsFeatures = require('../Institutions/InstitutionsFeatures')
 const UserGetter = require('../User/UserGetter')
 const AnalyticsManager = require('../Analytics/AnalyticsManager')
 
-const FeaturesUpdater = {
-  refreshFeatures(userId, reason, callback = () => {}) {
-    UserGetter.getUser(userId, { _id: 1, features: 1 }, (err, user) => {
-      if (err) {
-        return callback(err)
-      }
-      const oldFeatures = _.clone(user.features)
-      FeaturesUpdater._computeFeatures(userId, (error, features) => {
-        if (error) {
-          return callback(error)
-        }
-        logger.log({ userId, features }, 'updating user features')
-
-        const matchedFeatureSet = FeaturesUpdater._getMatchedFeatureSet(
-          features
-        )
-        AnalyticsManager.setUserPropertyForUser(
-          userId,
-          'feature-set',
-          matchedFeatureSet
-        )
-
-        UserFeaturesUpdater.updateFeatures(
-          userId,
-          features,
-          (err, newFeatures, featuresChanged) => {
-            if (err) {
-              return callback(err)
-            }
-            if (oldFeatures.dropbox === true && features.dropbox === false) {
-              logger.log({ userId }, '[FeaturesUpdater] must unlink dropbox')
-              const Modules = require('../../infrastructure/Modules')
-              Modules.hooks.fire('removeDropbox', userId, reason, err => {
-                if (err) {
-                  logger.error(err)
-                }
-
-                return callback(null, newFeatures, featuresChanged)
-              })
-            } else {
-              return callback(null, newFeatures, featuresChanged)
-            }
-          }
-        )
-      })
-    })
-  },
-
-  _computeFeatures(userId, callback) {
-    const jobs = {
-      individualFeatures(cb) {
-        FeaturesUpdater._getIndividualFeatures(userId, cb)
-      },
-      groupFeatureSets(cb) {
-        FeaturesUpdater._getGroupFeatureSets(userId, cb)
-      },
-      institutionFeatures(cb) {
-        InstitutionsFeatures.getInstitutionsFeatures(userId, cb)
-      },
-      v1Features(cb) {
-        FeaturesUpdater._getV1Features(userId, cb)
-      },
-      bonusFeatures(cb) {
-        ReferalFeatures.getBonusFeatures(userId, cb)
-      },
-      featuresOverrides(cb) {
-        FeaturesUpdater._getFeaturesOverrides(userId, cb)
-      },
+async function refreshFeatures(userId, reason) {
+  const user = await UserGetter.promises.getUser(userId, {
+    _id: 1,
+    features: 1,
+  })
+  const oldFeatures = _.clone(user.features)
+  const features = await computeFeatures(userId)
+  logger.log({ userId, features }, 'updating user features')
+
+  const matchedFeatureSet = _getMatchedFeatureSet(features)
+  AnalyticsManager.setUserPropertyForUser(
+    userId,
+    'feature-set',
+    matchedFeatureSet
+  )
+
+  const {
+    features: newFeatures,
+    featuresChanged,
+  } = await UserFeaturesUpdater.promises.updateFeatures(userId, features)
+  if (oldFeatures.dropbox === true && features.dropbox === false) {
+    logger.log({ userId }, '[FeaturesUpdater] must unlink dropbox')
+    const Modules = require('../../infrastructure/Modules')
+    try {
+      await Modules.promises.hooks.fire('removeDropbox', userId, reason)
+    } catch (err) {
+      logger.error(err)
     }
-    async.series(jobs, function (err, results) {
-      if (err) {
-        OError.tag(
-          err,
-          'error getting subscription or group for refreshFeatures',
-          {
-            userId,
-          }
-        )
-        return callback(err)
-      }
-
-      const {
-        individualFeatures,
-        groupFeatureSets,
-        institutionFeatures,
-        v1Features,
-        bonusFeatures,
-        featuresOverrides,
-      } = results
-      logger.log(
-        {
-          userId,
-          individualFeatures,
-          groupFeatureSets,
-          institutionFeatures,
-          v1Features,
-          bonusFeatures,
-          featuresOverrides,
-        },
-        'merging user features'
-      )
-      const featureSets = groupFeatureSets.concat([
-        individualFeatures,
-        institutionFeatures,
-        v1Features,
-        bonusFeatures,
-        featuresOverrides,
-      ])
-      const features = _.reduce(
-        featureSets,
-        FeaturesUpdater._mergeFeatures,
-        Settings.defaultFeatures
-      )
-      callback(null, features)
-    })
-  },
-
-  _getIndividualFeatures(userId, callback) {
-    SubscriptionLocator.getUserIndividualSubscription(userId, (err, sub) =>
-      callback(err, FeaturesUpdater._subscriptionToFeatures(sub))
-    )
-  },
-
-  _getGroupFeatureSets(userId, callback) {
-    SubscriptionLocator.getGroupSubscriptionsMemberOf(userId, (err, subs) =>
-      callback(err, (subs || []).map(FeaturesUpdater._subscriptionToFeatures))
-    )
-  },
-
-  _getFeaturesOverrides(userId, callback) {
-    UserGetter.getUser(userId, { featuresOverrides: 1 }, (error, user) => {
-      if (error) {
-        return callback(error)
-      }
-      if (
-        !user ||
-        !user.featuresOverrides ||
-        user.featuresOverrides.length === 0
-      ) {
-        return callback(null, {})
-      }
-      const activeFeaturesOverrides = []
-      for (const featuresOverride of user.featuresOverrides) {
-        if (
-          !featuresOverride.expiresAt ||
-          featuresOverride.expiresAt > new Date()
-        ) {
-          activeFeaturesOverrides.push(featuresOverride.features)
-        }
-      }
-      const features = _.reduce(
-        activeFeaturesOverrides,
-        FeaturesUpdater._mergeFeatures,
-        {}
-      )
-      callback(null, features)
-    })
-  },
+  }
+  return { features: newFeatures, featuresChanged }
+}
 
-  _getV1Features(userId, callback) {
-    V1SubscriptionManager.getPlanCodeFromV1(
+async function computeFeatures(userId) {
+  const individualFeatures = await _getIndividualFeatures(userId)
+  const groupFeatureSets = await _getGroupFeatureSets(userId)
+  const institutionFeatures = await InstitutionsFeatures.promises.getInstitutionsFeatures(
+    userId
+  )
+  const v1Features = await _getV1Features(userId)
+  const bonusFeatures = await ReferalFeatures.promises.getBonusFeatures(userId)
+  const featuresOverrides = await _getFeaturesOverrides(userId)
+  logger.log(
+    {
       userId,
-      function (err, planCode, v1Id) {
-        if (err) {
-          if ((err ? err.name : undefined) === 'NotFoundError') {
-            return callback(null, [])
-          }
-          return callback(err)
-        }
-
-        callback(
-          err,
-          FeaturesUpdater._mergeFeatures(
-            V1SubscriptionManager.getGrandfatheredFeaturesForV1User(v1Id) || {},
-            FeaturesUpdater.planCodeToFeatures(planCode)
-          )
-        )
-      }
-    )
-  },
-
-  _mergeFeatures(featuresA, featuresB) {
-    const features = Object.assign({}, featuresA)
-    for (const key in featuresB) {
-      // Special merging logic for non-boolean features
-      if (key === 'compileGroup') {
-        if (
-          features.compileGroup === 'priority' ||
-          featuresB.compileGroup === 'priority'
-        ) {
-          features.compileGroup = 'priority'
-        } else {
-          features.compileGroup = 'standard'
-        }
-      } else if (key === 'collaborators') {
-        if (features.collaborators === -1 || featuresB.collaborators === -1) {
-          features.collaborators = -1
-        } else {
-          features.collaborators = Math.max(
-            features.collaborators || 0,
-            featuresB.collaborators || 0
-          )
-        }
-      } else if (key === 'compileTimeout') {
-        features.compileTimeout = Math.max(
-          features.compileTimeout || 0,
-          featuresB.compileTimeout || 0
-        )
-      } else {
-        // Boolean keys, true is better
-        features[key] = features[key] || featuresB[key]
-      }
-    }
-    return features
-  },
+      individualFeatures,
+      groupFeatureSets,
+      institutionFeatures,
+      v1Features,
+      bonusFeatures,
+      featuresOverrides,
+    },
+    'merging user features'
+  )
+  const featureSets = groupFeatureSets.concat([
+    individualFeatures,
+    institutionFeatures,
+    v1Features,
+    bonusFeatures,
+    featuresOverrides,
+  ])
+  const features = _.reduce(
+    featureSets,
+    FeaturesHelper.mergeFeatures,
+    Settings.defaultFeatures
+  )
+  return features
+}
 
-  isFeatureSetBetter(featuresA, featuresB) {
-    const mergedFeatures = FeaturesUpdater._mergeFeatures(featuresA, featuresB)
-    return _.isEqual(featuresA, mergedFeatures)
-  },
+async function _getIndividualFeatures(userId) {
+  const sub = await SubscriptionLocator.promises.getUserIndividualSubscription(
+    userId
+  )
+  return _subscriptionToFeatures(sub)
+}
 
-  _subscriptionToFeatures(subscription) {
-    return FeaturesUpdater.planCodeToFeatures(
-      subscription ? subscription.planCode : undefined
-    )
-  },
+async function _getGroupFeatureSets(userId) {
+  const subs = await SubscriptionLocator.promises.getGroupSubscriptionsMemberOf(
+    userId
+  )
+  return (subs || []).map(_subscriptionToFeatures)
+}
 
-  planCodeToFeatures(planCode) {
-    if (!planCode) {
-      return {}
-    }
-    const plan = PlansLocator.findLocalPlanInSettings(planCode)
-    if (!plan) {
-      return {}
-    } else {
-      return plan.features
+async function _getFeaturesOverrides(userId) {
+  const user = await UserGetter.promises.getUser(userId, {
+    featuresOverrides: 1,
+  })
+  if (!user || !user.featuresOverrides || user.featuresOverrides.length === 0) {
+    return {}
+  }
+  const activeFeaturesOverrides = []
+  for (const featuresOverride of user.featuresOverrides) {
+    if (
+      !featuresOverride.expiresAt ||
+      featuresOverride.expiresAt > new Date()
+    ) {
+      activeFeaturesOverrides.push(featuresOverride.features)
     }
-  },
+  }
+  const features = _.reduce(
+    activeFeaturesOverrides,
+    FeaturesHelper.mergeFeatures,
+    {}
+  )
+  return features
+}
 
-  compareFeatures(currentFeatures, expectedFeatures) {
-    currentFeatures = _.clone(currentFeatures)
-    expectedFeatures = _.clone(expectedFeatures)
-    if (_.isEqual(currentFeatures, expectedFeatures)) {
+async function _getV1Features(userId) {
+  let planCode, v1Id
+  try {
+    ;({
+      planCode,
+      v1Id,
+    } = await V1SubscriptionManager.promises.getPlanCodeFromV1(userId))
+  } catch (err) {
+    if (err.name === 'NotFoundError') {
       return {}
+    } else {
+      throw err
     }
+  }
+  return FeaturesHelper.mergeFeatures(
+    V1SubscriptionManager.getGrandfatheredFeaturesForV1User(v1Id) || {},
+    _planCodeToFeatures(planCode)
+  )
+}
 
-    const mismatchReasons = {}
-    const featureKeys = [
-      ...new Set([
-        ...Object.keys(currentFeatures),
-        ...Object.keys(expectedFeatures),
-      ]),
-    ]
-    featureKeys.sort().forEach(key => {
-      if (expectedFeatures[key] !== currentFeatures[key]) {
-        mismatchReasons[key] = expectedFeatures[key]
-      }
-    })
-
-    if (mismatchReasons.compileTimeout) {
-      // store the compile timeout difference instead of the new compile timeout
-      mismatchReasons.compileTimeout =
-        expectedFeatures.compileTimeout - currentFeatures.compileTimeout
-    }
-
-    if (mismatchReasons.collaborators) {
-      // store the collaborators difference instead of the new number only
-      // replace -1 by 100 to make it clearer
-      if (expectedFeatures.collaborators === -1) {
-        expectedFeatures.collaborators = 100
-      }
-      if (currentFeatures.collaborators === -1) {
-        currentFeatures.collaborators = 100
-      }
-      mismatchReasons.collaborators =
-        expectedFeatures.collaborators - currentFeatures.collaborators
-    }
+function _subscriptionToFeatures(subscription) {
+  return _planCodeToFeatures(subscription && subscription.planCode)
+}
 
-    return mismatchReasons
-  },
+function _planCodeToFeatures(planCode) {
+  if (!planCode) {
+    return {}
+  }
+  const plan = PlansLocator.findLocalPlanInSettings(planCode)
+  if (!plan) {
+    return {}
+  } else {
+    return plan.features
+  }
+}
 
-  doSyncFromV1(v1UserId, callback) {
-    logger.log({ v1UserId }, '[AccountSync] starting account sync')
-    return UserGetter.getUser(
-      { 'overleaf.id': v1UserId },
-      { _id: 1 },
-      function (err, user) {
-        if (err != null) {
-          OError.tag(err, '[AccountSync] error getting user', {
-            v1UserId,
-          })
-          return callback(err)
-        }
-        if ((user != null ? user._id : undefined) == null) {
-          logger.warn({ v1UserId }, '[AccountSync] no user found for v1 id')
-          return callback(null)
-        }
-        logger.log(
-          { v1UserId, userId: user._id },
-          '[AccountSync] updating user subscription and features'
-        )
-        return FeaturesUpdater.refreshFeatures(user._id, 'sync-v1', callback)
-      }
-    )
-  },
+async function doSyncFromV1(v1UserId) {
+  logger.log({ v1UserId }, '[AccountSync] starting account sync')
+  const user = await UserGetter.promises.getUser(
+    { 'overleaf.id': v1UserId },
+    { _id: 1 }
+  )
+  if (user == null) {
+    logger.warn({ v1UserId }, '[AccountSync] no user found for v1 id')
+    return
+  }
+  logger.log(
+    { v1UserId, userId: user._id },
+    '[AccountSync] updating user subscription and features'
+  )
+  return refreshFeatures(user._id, 'sync-v1')
+}
 
-  _getMatchedFeatureSet(features) {
-    for (const [name, featureSet] of Object.entries(Settings.features)) {
-      if (_.isEqual(features, featureSet)) {
-        return name
-      }
+function _getMatchedFeatureSet(features) {
+  for (const [name, featureSet] of Object.entries(Settings.features)) {
+    if (_.isEqual(features, featureSet)) {
+      return name
     }
-    return 'mixed'
-  },
+  }
+  return 'mixed'
 }
 
-const refreshFeaturesPromise = (userId, reason) =>
-  new Promise(function (resolve, reject) {
-    FeaturesUpdater.refreshFeatures(
-      userId,
-      reason,
-      (error, features, featuresChanged) => {
-        if (error) {
-          reject(error)
-        } else {
-          resolve({ features, featuresChanged })
-        }
-      }
-    )
-  })
-
-FeaturesUpdater.promises = {
-  refreshFeatures: refreshFeaturesPromise,
+module.exports = {
+  computeFeatures: callbackify(computeFeatures),
+  refreshFeatures: callbackifyMultiResult(refreshFeatures, [
+    'features',
+    'featuresChanged',
+  ]),
+  doSyncFromV1: callbackifyMultiResult(doSyncFromV1, [
+    'features',
+    'featuresChanged',
+  ]),
+  promises: {
+    computeFeatures,
+    refreshFeatures,
+    doSyncFromV1,
+  },
 }
-
-module.exports = FeaturesUpdater

services/web/app/src/Features/Subscription/SubscriptionController.js

@@ -447,7 +447,7 @@ function recurlyNotificationParser(req, res, next) {
 
 async function refreshUserFeatures(req, res) {
   const { user_id: userId } = req.params
-  await FeaturesUpdater.promises.refreshFeatures(userId)
+  await FeaturesUpdater.promises.refreshFeatures(userId, 'acceptance-test')
   res.sendStatus(200)
 }
 

services/web/app/src/Features/Subscription/SubscriptionHandler.js

@@ -1,8 +1,8 @@
 const async = require('async')
+const { promisify } = require('util')
 const RecurlyWrapper = require('./RecurlyWrapper')
 const RecurlyClient = require('./RecurlyClient')
 const { User } = require('../../models/User')
-const { promisifyAll } = require('../../util/promises')
 const logger = require('logger-sharelatex')
 const SubscriptionUpdater = require('./SubscriptionUpdater')
 const LimitationsManager = require('./LimitationsManager')
@@ -10,334 +10,352 @@ const EmailHandler = require('../Email/EmailHandler')
 const PlansLocator = require('./PlansLocator')
 const SubscriptionHelper = require('./SubscriptionHelper')
 
-const SubscriptionHandler = {
-  validateNoSubscriptionInRecurly(userId, callback) {
-    if (callback == null) {
-      callback = function () {}
-    }
-    RecurlyWrapper.listAccountActiveSubscriptions(
-      userId,
-      function (error, subscriptions) {
-        if (subscriptions == null) {
-          subscriptions = []
-        }
-        if (error != null) {
-          return callback(error)
-        }
-        if (subscriptions.length > 0) {
-          SubscriptionUpdater.syncSubscription(
-            subscriptions[0],
-            userId,
-            function (error) {
-              if (error != null) {
-                return callback(error)
-              }
-              callback(null, false)
+function validateNoSubscriptionInRecurly(userId, callback) {
+  if (callback == null) {
+    callback = function () {}
+  }
+  RecurlyWrapper.listAccountActiveSubscriptions(
+    userId,
+    function (error, subscriptions) {
+      if (subscriptions == null) {
+        subscriptions = []
+      }
+      if (error != null) {
+        return callback(error)
+      }
+      if (subscriptions.length > 0) {
+        SubscriptionUpdater.syncSubscription(
+          subscriptions[0],
+          userId,
+          function (error) {
+            if (error != null) {
+              return callback(error)
             }
-          )
-        } else {
-          callback(null, true)
-        }
+            callback(null, false)
+          }
+        )
+      } else {
+        callback(null, true)
       }
-    )
-  },
+    }
+  )
+}
 
-  createSubscription(user, subscriptionDetails, recurlyTokenIds, callback) {
-    SubscriptionHandler.validateNoSubscriptionInRecurly(
-      user._id,
-      function (error, valid) {
+function createSubscription(
+  user,
+  subscriptionDetails,
+  recurlyTokenIds,
+  callback
+) {
+  validateNoSubscriptionInRecurly(user._id, function (error, valid) {
+    if (error != null) {
+      return callback(error)
+    }
+    if (!valid) {
+      return callback(new Error('user already has subscription in recurly'))
+    }
+    RecurlyWrapper.createSubscription(
+      user,
+      subscriptionDetails,
+      recurlyTokenIds,
+      function (error, recurlySubscription) {
         if (error != null) {
           return callback(error)
         }
-        if (!valid) {
-          return callback(new Error('user already has subscription in recurly'))
-        }
-        RecurlyWrapper.createSubscription(
-          user,
-          subscriptionDetails,
-          recurlyTokenIds,
-          function (error, recurlySubscription) {
+        return SubscriptionUpdater.syncSubscription(
+          recurlySubscription,
+          user._id,
+          function (error) {
             if (error != null) {
               return callback(error)
             }
-            return SubscriptionUpdater.syncSubscription(
-              recurlySubscription,
-              user._id,
-              function (error) {
-                if (error != null) {
-                  return callback(error)
-                }
-                return callback()
-              }
-            )
+            return callback()
           }
         )
       }
     )
-  },
+  })
+}
 
-  updateSubscription(user, planCode, couponCode, callback) {
-    LimitationsManager.userHasV2Subscription(
-      user,
-      function (err, hasSubscription, subscription) {
-        if (err) {
-          logger.warn(
-            { err, user_id: user._id, hasSubscription },
-            'there was an error checking user v2 subscription'
-          )
-        }
-        if (!hasSubscription) {
-          return callback()
-        } else {
-          return async.series(
-            [
-              function (cb) {
-                if (couponCode == null) {
-                  return cb()
-                }
-                RecurlyWrapper.getSubscription(
-                  subscription.recurlySubscription_id,
-                  { includeAccount: true },
-                  function (err, usersSubscription) {
-                    if (err != null) {
-                      return cb(err)
-                    }
-                    RecurlyWrapper.redeemCoupon(
-                      usersSubscription.account.account_code,
-                      couponCode,
-                      cb
-                    )
+function updateSubscription(user, planCode, couponCode, callback) {
+  LimitationsManager.userHasV2Subscription(
+    user,
+    function (err, hasSubscription, subscription) {
+      if (err) {
+        logger.warn(
+          { err, user_id: user._id, hasSubscription },
+          'there was an error checking user v2 subscription'
+        )
+      }
+      if (!hasSubscription) {
+        return callback()
+      } else {
+        return async.series(
+          [
+            function (cb) {
+              if (couponCode == null) {
+                return cb()
+              }
+              RecurlyWrapper.getSubscription(
+                subscription.recurlySubscription_id,
+                { includeAccount: true },
+                function (err, usersSubscription) {
+                  if (err != null) {
+                    return cb(err)
                   }
+                  RecurlyWrapper.redeemCoupon(
+                    usersSubscription.account.account_code,
+                    couponCode,
+                    cb
+                  )
+                }
+              )
+            },
+            function (cb) {
+              let changeAtTermEnd
+              const currentPlan = PlansLocator.findLocalPlanInSettings(
+                subscription.planCode
+              )
+              const newPlan = PlansLocator.findLocalPlanInSettings(planCode)
+              if (currentPlan && newPlan) {
+                changeAtTermEnd = SubscriptionHelper.shouldPlanChangeAtTermEnd(
+                  currentPlan,
+                  newPlan
                 )
-              },
-              function (cb) {
-                let changeAtTermEnd
-                const currentPlan = PlansLocator.findLocalPlanInSettings(
-                  subscription.planCode
+              } else {
+                logger.error(
+                  { currentPlan: subscription.planCode, newPlan: planCode },
+                  'unable to locate both plans in settings'
                 )
-                const newPlan = PlansLocator.findLocalPlanInSettings(planCode)
-                if (currentPlan && newPlan) {
-                  changeAtTermEnd = SubscriptionHelper.shouldPlanChangeAtTermEnd(
-                    currentPlan,
-                    newPlan
-                  )
-                } else {
-                  logger.error(
-                    { currentPlan: subscription.planCode, newPlan: planCode },
-                    'unable to locate both plans in settings'
-                  )
-                  return cb(
-                    new Error('unable to locate both plans in settings')
+                return cb(new Error('unable to locate both plans in settings'))
+              }
+              const timeframe = changeAtTermEnd ? 'term_end' : 'now'
+              RecurlyClient.changeSubscriptionByUuid(
+                subscription.recurlySubscription_id,
+                { planCode: planCode, timeframe: timeframe },
+                function (error, subscriptionChange) {
+                  if (error != null) {
+                    return cb(error)
+                  }
+                  // v2 recurly API wants a UUID, but UUID isn't included in the subscription change response
+                  // we got the UUID from the DB using userHasV2Subscription() - it is the only property
+                  // we need to be able to build a 'recurlySubscription' object for syncSubscription()
+                  syncSubscription(
+                    { uuid: subscription.recurlySubscription_id },
+                    user._id,
+                    cb
                   )
                 }
-                const timeframe = changeAtTermEnd ? 'term_end' : 'now'
-                RecurlyClient.changeSubscriptionByUuid(
-                  subscription.recurlySubscription_id,
-                  { planCode: planCode, timeframe: timeframe },
-                  function (error, subscriptionChange) {
-                    if (error != null) {
-                      return cb(error)
-                    }
-                    // v2 recurly API wants a UUID, but UUID isn't included in the subscription change response
-                    // we got the UUID from the DB using userHasV2Subscription() - it is the only property
-                    // we need to be able to build a 'recurlySubscription' object for syncSubscription()
-                    SubscriptionHandler.syncSubscription(
-                      { uuid: subscription.recurlySubscription_id },
-                      user._id,
-                      cb
-                    )
-                  }
-                )
-              },
-            ],
-            callback
-          )
-        }
+              )
+            },
+          ],
+          callback
+        )
       }
-    )
-  },
+    }
+  )
+}
 
-  cancelPendingSubscriptionChange(user, callback) {
-    LimitationsManager.userHasV2Subscription(
-      user,
-      function (err, hasSubscription, subscription) {
-        if (err) {
-          return callback(err)
-        }
-        if (hasSubscription) {
-          RecurlyClient.removeSubscriptionChangeByUuid(
-            subscription.recurlySubscription_id,
-            function (error) {
-              if (error != null) {
-                return callback(error)
-              }
-              callback()
-            }
-          )
-        } else {
-          callback()
-        }
+function cancelPendingSubscriptionChange(user, callback) {
+  LimitationsManager.userHasV2Subscription(
+    user,
+    function (err, hasSubscription, subscription) {
+      if (err) {
+        return callback(err)
       }
-    )
-  },
-
-  cancelSubscription(user, callback) {
-    LimitationsManager.userHasV2Subscription(
-      user,
-      function (err, hasSubscription, subscription) {
-        if (err) {
-          logger.warn(
-            { err, user_id: user._id, hasSubscription },
-            'there was an error checking user v2 subscription'
-          )
-        }
-        if (hasSubscription) {
-          RecurlyClient.cancelSubscriptionByUuid(
-            subscription.recurlySubscription_id,
-            function (error) {
-              if (error != null) {
-                return callback(error)
-              }
-              const emailOpts = {
-                to: user.email,
-                first_name: user.first_name,
-              }
-              const ONE_HOUR_IN_MS = 1000 * 60 * 60
-              setTimeout(
-                () =>
-                  EmailHandler.sendEmail(
-                    'canceledSubscription',
-                    emailOpts,
-                    err => {
-                      if (err != null) {
-                        logger.warn(
-                          { err },
-                          'failed to send confirmation email for subscription cancellation'
-                        )
-                      }
-                    }
-                  ),
-                ONE_HOUR_IN_MS
-              )
-              callback()
+      if (hasSubscription) {
+        RecurlyClient.removeSubscriptionChangeByUuid(
+          subscription.recurlySubscription_id,
+          function (error) {
+            if (error != null) {
+              return callback(error)
             }
-          )
-        } else {
-          callback()
-        }
+            callback()
+          }
+        )
+      } else {
+        callback()
       }
-    )
-  },
+    }
+  )
+}
 
-  reactivateSubscription(user, callback) {
-    LimitationsManager.userHasV2Subscription(
-      user,
-      function (err, hasSubscription, subscription) {
-        if (err) {
-          logger.warn(
-            { err, user_id: user._id, hasSubscription },
-            'there was an error checking user v2 subscription'
-          )
-        }
-        if (hasSubscription) {
-          RecurlyClient.reactivateSubscriptionByUuid(
-            subscription.recurlySubscription_id,
-            function (error) {
-              if (error != null) {
-                return callback(error)
-              }
-              EmailHandler.sendEmail(
-                'reactivatedSubscription',
-                { to: user.email },
-                err => {
-                  if (err != null) {
-                    logger.warn(
-                      { err },
-                      'failed to send reactivation confirmation email'
-                    )
-                  }
-                }
-              )
-              callback()
+function cancelSubscription(user, callback) {
+  LimitationsManager.userHasV2Subscription(
+    user,
+    function (err, hasSubscription, subscription) {
+      if (err) {
+        logger.warn(
+          { err, user_id: user._id, hasSubscription },
+          'there was an error checking user v2 subscription'
+        )
+      }
+      if (hasSubscription) {
+        RecurlyClient.cancelSubscriptionByUuid(
+          subscription.recurlySubscription_id,
+          function (error) {
+            if (error != null) {
+              return callback(error)
             }
-          )
-        } else {
-          callback()
-        }
+            const emailOpts = {
+              to: user.email,
+              first_name: user.first_name,
+            }
+            const ONE_HOUR_IN_MS = 1000 * 60 * 60
+            setTimeout(
+              () =>
+                EmailHandler.sendEmail(
+                  'canceledSubscription',
+                  emailOpts,
+                  err => {
+                    if (err != null) {
+                      logger.warn(
+                        { err },
+                        'failed to send confirmation email for subscription cancellation'
+                      )
+                    }
+                  }
+                ),
+              ONE_HOUR_IN_MS
+            )
+            callback()
+          }
+        )
+      } else {
+        callback()
       }
-    )
-  },
+    }
+  )
+}
 
-  syncSubscription(recurlySubscription, requesterData, callback) {
-    RecurlyWrapper.getSubscription(
-      recurlySubscription.uuid,
-      { includeAccount: true },
-      function (error, recurlySubscription) {
-        if (error != null) {
-          return callback(error)
-        }
-        User.findById(
-          recurlySubscription.account.account_code,
-          { _id: 1 },
-          function (error, user) {
+function reactivateSubscription(user, callback) {
+  LimitationsManager.userHasV2Subscription(
+    user,
+    function (err, hasSubscription, subscription) {
+      if (err) {
+        logger.warn(
+          { err, user_id: user._id, hasSubscription },
+          'there was an error checking user v2 subscription'
+        )
+      }
+      if (hasSubscription) {
+        RecurlyClient.reactivateSubscriptionByUuid(
+          subscription.recurlySubscription_id,
+          function (error) {
             if (error != null) {
               return callback(error)
             }
-            if (user == null) {
-              return callback(new Error('no user found'))
-            }
-            SubscriptionUpdater.syncSubscription(
-              recurlySubscription,
-              user != null ? user._id : undefined,
-              requesterData,
-              callback
+            EmailHandler.sendEmail(
+              'reactivatedSubscription',
+              { to: user.email },
+              err => {
+                if (err != null) {
+                  logger.warn(
+                    { err },
+                    'failed to send reactivation confirmation email'
+                  )
+                }
+              }
             )
+            callback()
           }
         )
+      } else {
+        callback()
       }
-    )
-  },
+    }
+  )
+}
 
-  // attempt to collect past due invoice for customer. Only do that when a) the
-  // customer is using Paypal and b) there is only one past due invoice.
-  // This is used because Recurly doesn't always attempt collection of paast due
-  // invoices after Paypal billing info were updated.
-  attemptPaypalInvoiceCollection(recurlyAccountCode, callback) {
-    RecurlyWrapper.getBillingInfo(recurlyAccountCode, (error, billingInfo) => {
-      if (error) {
+function syncSubscription(recurlySubscription, requesterData, callback) {
+  RecurlyWrapper.getSubscription(
+    recurlySubscription.uuid,
+    { includeAccount: true },
+    function (error, recurlySubscription) {
+      if (error != null) {
         return callback(error)
       }
-      if (!billingInfo.paypal_billing_agreement_id) {
-        // this is not a Paypal user
-        return callback()
-      }
-      RecurlyWrapper.getAccountPastDueInvoices(
-        recurlyAccountCode,
-        (error, pastDueInvoices) => {
-          if (error) {
+      User.findById(
+        recurlySubscription.account.account_code,
+        { _id: 1 },
+        function (error, user) {
+          if (error != null) {
             return callback(error)
           }
-          if (pastDueInvoices.length !== 1) {
-            // no past due invoices, or more than one. Ignore.
-            return callback()
+          if (user == null) {
+            return callback(new Error('no user found'))
           }
-          RecurlyWrapper.attemptInvoiceCollection(
-            pastDueInvoices[0].invoice_number,
+          SubscriptionUpdater.syncSubscription(
+            recurlySubscription,
+            user != null ? user._id : undefined,
+            requesterData,
             callback
           )
         }
       )
-    })
-  },
+    }
+  )
+}
 
-  extendTrial(subscription, daysToExend, callback) {
-    return RecurlyWrapper.extendTrial(
-      subscription.recurlySubscription_id,
-      daysToExend,
-      callback
+// attempt to collect past due invoice for customer. Only do that when a) the
+// customer is using Paypal and b) there is only one past due invoice.
+// This is used because Recurly doesn't always attempt collection of paast due
+// invoices after Paypal billing info were updated.
+function attemptPaypalInvoiceCollection(recurlyAccountCode, callback) {
+  RecurlyWrapper.getBillingInfo(recurlyAccountCode, (error, billingInfo) => {
+    if (error) {
+      return callback(error)
+    }
+    if (!billingInfo.paypal_billing_agreement_id) {
+      // this is not a Paypal user
+      return callback()
+    }
+    RecurlyWrapper.getAccountPastDueInvoices(
+      recurlyAccountCode,
+      (error, pastDueInvoices) => {
+        if (error) {
+          return callback(error)
+        }
+        if (pastDueInvoices.length !== 1) {
+          // no past due invoices, or more than one. Ignore.
+          return callback()
+        }
+        RecurlyWrapper.attemptInvoiceCollection(
+          pastDueInvoices[0].invoice_number,
+          callback
+        )
+      }
     )
-  },
+  })
 }
 
-SubscriptionHandler.promises = promisifyAll(SubscriptionHandler)
-module.exports = SubscriptionHandler
+function extendTrial(subscription, daysToExend, callback) {
+  return RecurlyWrapper.extendTrial(
+    subscription.recurlySubscription_id,
+    daysToExend,
+    callback
+  )
+}
+
+module.exports = {
+  validateNoSubscriptionInRecurly,
+  createSubscription,
+  updateSubscription,
+  cancelPendingSubscriptionChange,
+  cancelSubscription,
+  reactivateSubscription,
+  syncSubscription,
+  attemptPaypalInvoiceCollection,
+  extendTrial,
+  promises: {
+    validateNoSubscriptionInRecurly: promisify(validateNoSubscriptionInRecurly),
+    createSubscription: promisify(createSubscription),
+    updateSubscription: promisify(updateSubscription),
+    cancelPendingSubscriptionChange: promisify(cancelPendingSubscriptionChange),
+    cancelSubscription: promisify(cancelSubscription),
+    reactivateSubscription: promisify(reactivateSubscription),
+    syncSubscription: promisify(syncSubscription),
+    attemptPaypalInvoiceCollection: promisify(attemptPaypalInvoiceCollection),
+    extendTrial: promisify(extendTrial),
+  },
+}

services/web/app/src/Features/Subscription/SubscriptionUpdater.js

@@ -1,12 +1,10 @@
 const { db, ObjectId } = require('../../infrastructure/mongodb')
-const OError = require('@overleaf/o-error')
-const async = require('async')
-const { promisify, callbackify } = require('../../util/promises')
+const { callbackify } = require('../../util/promises')
 const { Subscription } = require('../../models/Subscription')
 const SubscriptionLocator = require('./SubscriptionLocator')
-const UserGetter = require('../User/UserGetter')
 const PlansLocator = require('./PlansLocator')
 const FeaturesUpdater = require('./FeaturesUpdater')
+const FeaturesHelper = require('./FeaturesHelper')
 const AnalyticsManager = require('../Analytics/AnalyticsManager')
 const { DeletedSubscription } = require('../../models/DeletedSubscription')
 const logger = require('logger-sharelatex')
@@ -25,7 +23,7 @@ const logger = require('logger-sharelatex')
  *
  * If the subscription is Recurly, we silently do nothing.
  */
-function updateAdmin(subscription, adminId, callback) {
+async function updateAdmin(subscription, adminId) {
   const query = {
     _id: ObjectId(subscription._id),
     customAccount: true,
@@ -38,207 +36,115 @@ function updateAdmin(subscription, adminId, callback) {
   } else {
     update.$set.manager_ids = [ObjectId(adminId)]
   }
-  Subscription.updateOne(query, update, callback)
+  await Subscription.updateOne(query, update).exec()
 }
 
-function syncSubscription(
+async function syncSubscription(
   recurlySubscription,
   adminUserId,
-  requesterData,
-  callback
+  requesterData = {}
 ) {
-  if (!callback) {
-    callback = requesterData
-    requesterData = {}
+  let subscription = await SubscriptionLocator.promises.getUsersSubscription(
+    adminUserId
+  )
+  if (subscription == null) {
+    subscription = await _createNewSubscription(adminUserId)
   }
-  SubscriptionLocator.getUsersSubscription(
-    adminUserId,
-    function (err, subscription) {
-      if (err != null) {
-        return callback(err)
-      }
-      if (subscription != null) {
-        updateSubscriptionFromRecurly(
-          recurlySubscription,
-          subscription,
-          requesterData,
-          callback
-        )
-      } else {
-        _createNewSubscription(adminUserId, function (err, subscription) {
-          if (err != null) {
-            return callback(err)
-          }
-          updateSubscriptionFromRecurly(
-            recurlySubscription,
-            subscription,
-            requesterData,
-            callback
-          )
-        })
-      }
-    }
+  await updateSubscriptionFromRecurly(
+    recurlySubscription,
+    subscription,
+    requesterData
   )
 }
 
-function addUserToGroup(subscriptionId, userId, callback) {
-  Subscription.updateOne(
+async function addUserToGroup(subscriptionId, userId) {
+  await Subscription.updateOne(
     { _id: subscriptionId },
-    { $addToSet: { member_ids: userId } },
-    function (err) {
-      if (err != null) {
-        return callback(err)
-      }
-      FeaturesUpdater.refreshFeatures(userId, 'add-to-group', function () {
-        callbackify(_sendUserGroupPlanCodeUserProperty)(userId, callback)
-      })
-    }
-  )
+    { $addToSet: { member_ids: userId } }
+  ).exec()
+  await FeaturesUpdater.promises.refreshFeatures(userId, 'add-to-group')
+  await _sendUserGroupPlanCodeUserProperty(userId)
 }
 
-function removeUserFromGroup(subscriptionId, userId, callback) {
-  Subscription.updateOne(
+async function removeUserFromGroup(subscriptionId, userId) {
+  await Subscription.updateOne(
     { _id: subscriptionId },
-    { $pull: { member_ids: userId } },
-    function (error) {
-      if (error) {
-        OError.tag(error, 'error removing user from group', {
-          subscriptionId,
-          userId,
-        })
-        return callback(error)
-      }
-      UserGetter.getUser(userId, function (error, user) {
-        if (error) {
-          return callback(error)
-        }
-        FeaturesUpdater.refreshFeatures(
-          userId,
-          'remove-user-from-group',
-          function () {
-            callbackify(_sendUserGroupPlanCodeUserProperty)(userId, callback)
-          }
-        )
-      })
-    }
+    { $pull: { member_ids: userId } }
+  ).exec()
+  await FeaturesUpdater.promises.refreshFeatures(
+    userId,
+    'remove-user-from-group'
   )
+  await _sendUserGroupPlanCodeUserProperty(userId)
 }
 
-function removeUserFromAllGroups(userId, callback) {
-  SubscriptionLocator.getMemberSubscriptions(
+async function removeUserFromAllGroups(userId) {
+  const subscriptions = await SubscriptionLocator.promises.getMemberSubscriptions(
+    userId
+  )
+  if (subscriptions.length === 0) {
+    return
+  }
+  const subscriptionIds = subscriptions.map(sub => sub._id)
+  const removeOperation = { $pull: { member_ids: userId } }
+  await Subscription.updateMany(
+    { _id: subscriptionIds },
+    removeOperation
+  ).exec()
+  await FeaturesUpdater.promises.refreshFeatures(
     userId,
-    function (error, subscriptions) {
-      if (error) {
-        return callback(error)
-      }
-      if (!subscriptions) {
-        return callback()
-      }
-      const subscriptionIds = subscriptions.map(sub => sub._id)
-      const removeOperation = { $pull: { member_ids: userId } }
-      Subscription.updateMany(
-        { _id: subscriptionIds },
-        removeOperation,
-        function (error) {
-          if (error) {
-            OError.tag(error, 'error removing user from groups', {
-              userId,
-              subscriptionIds,
-            })
-            return callback(error)
-          }
-          UserGetter.getUser(userId, function (error, user) {
-            if (error) {
-              return callback(error)
-            }
-            FeaturesUpdater.refreshFeatures(
-              userId,
-              'remove-user-from-groups',
-              function () {
-                callbackify(_sendUserGroupPlanCodeUserProperty)(
-                  userId,
-                  callback
-                )
-              }
-            )
-          })
-        }
-      )
-    }
+    'remove-user-from-groups'
   )
+  await _sendUserGroupPlanCodeUserProperty(userId)
 }
 
-function deleteWithV1Id(v1TeamId, callback) {
-  Subscription.deleteOne({ 'overleaf.id': v1TeamId }, callback)
+async function deleteWithV1Id(v1TeamId) {
+  await Subscription.deleteOne({ 'overleaf.id': v1TeamId }).exec()
 }
 
-function deleteSubscription(subscription, deleterData, callback) {
-  if (callback == null) {
-    callback = function () {}
-  }
-  async.series(
-    [
-      cb =>
-        // 1. create deletedSubscription
-        createDeletedSubscription(subscription, deleterData, cb),
-      cb =>
-        // 2. remove subscription
-        Subscription.deleteOne({ _id: subscription._id }, cb),
-      cb =>
-        // 3. refresh users features
-        refreshUsersFeatures(subscription, cb),
-    ],
-    callback
-  )
+async function deleteSubscription(subscription, deleterData) {
+  // 1. create deletedSubscription
+  await createDeletedSubscription(subscription, deleterData)
+
+  // 2. remove subscription
+  await Subscription.deleteOne({ _id: subscription._id }).exec()
+
+  // 3. refresh users features
+  await refreshUsersFeatures(subscription)
 }
 
-function restoreSubscription(subscriptionId, callback) {
-  SubscriptionLocator.getDeletedSubscription(
-    subscriptionId,
-    function (err, deletedSubscription) {
-      if (err) {
-        return callback(err)
-      }
-      const subscription = deletedSubscription.subscription
-      async.series(
-        [
-          cb =>
-            // 1. upsert subscription
-            db.subscriptions.updateOne(
-              { _id: subscription._id },
-              subscription,
-              { upsert: true },
-              cb
-            ),
-          cb =>
-            // 2. refresh users features. Do this before removing the
-            //    subscription so the restore can be retried if this fails
-            refreshUsersFeatures(subscription, cb),
-          cb =>
-            // 3. remove deleted subscription
-            DeletedSubscription.deleteOne(
-              { 'subscription._id': subscription._id },
-              callback
-            ),
-        ],
-        callback
-      )
-    }
+async function restoreSubscription(subscriptionId) {
+  const deletedSubscription = await SubscriptionLocator.promises.getDeletedSubscription(
+    subscriptionId
   )
+  const subscription = deletedSubscription.subscription
+
+  // 1. upsert subscription
+  await db.subscriptions.updateOne({ _id: subscription._id }, subscription, {
+    upsert: true,
+  })
+
+  // 2. refresh users features. Do this before removing the
+  //    subscription so the restore can be retried if this fails
+  await refreshUsersFeatures(subscription)
+
+  // 3. remove deleted subscription
+  await DeletedSubscription.deleteOne({
+    'subscription._id': subscription._id,
+  }).exec()
 }
 
-function refreshUsersFeatures(subscription, callback) {
+async function refreshUsersFeatures(subscription) {
   const userIds = [subscription.admin_id].concat(subscription.member_ids || [])
-  async.mapSeries(
-    userIds,
-    function (userId, cb) {
-      FeaturesUpdater.refreshFeatures(userId, 'subscription-updater', cb)
-    },
-    callback
-  )
+  for (const userId of userIds) {
+    await FeaturesUpdater.promises.refreshFeatures(
+      userId,
+      'subscription-updater'
+    )
+  }
 }
 
-function createDeletedSubscription(subscription, deleterData, callback) {
+async function createDeletedSubscription(subscription, deleterData) {
   subscription.teamInvites = []
   subscription.invited_emails = []
   const filter = { 'subscription._id': subscription._id }
@@ -250,65 +156,56 @@ function createDeletedSubscription(subscription, deleterData, callback) {
     subscription: subscription,
   }
   const options = { upsert: true, new: true, setDefaultsOnInsert: true }
-  DeletedSubscription.findOneAndUpdate(filter, data, options, callback)
+  await DeletedSubscription.findOneAndUpdate(filter, data, options).exec()
 }
 
-function _createNewSubscription(adminUserId, callback) {
+async function _createNewSubscription(adminUserId) {
   const subscription = new Subscription({
     admin_id: adminUserId,
     manager_ids: [adminUserId],
   })
-  subscription.save(err => callback(err, subscription))
+  await subscription.save()
+  return subscription
 }
 
-function _deleteAndReplaceSubscriptionFromRecurly(
+async function _deleteAndReplaceSubscriptionFromRecurly(
   recurlySubscription,
   subscription,
-  requesterData,
-  callback
+  requesterData
 ) {
   const adminUserId = subscription.admin_id
-  deleteSubscription(subscription, requesterData, err => {
-    if (err) {
-      return callback(err)
-    }
-    _createNewSubscription(adminUserId, (err, newSubscription) => {
-      if (err) {
-        return callback(err)
-      }
-      updateSubscriptionFromRecurly(
-        recurlySubscription,
-        newSubscription,
-        requesterData,
-        callback
-      )
-    })
-  })
+  await deleteSubscription(subscription, requesterData)
+  const newSubscription = await _createNewSubscription(adminUserId)
+  await updateSubscriptionFromRecurly(
+    recurlySubscription,
+    newSubscription,
+    requesterData
+  )
 }
 
-function updateSubscriptionFromRecurly(
+async function updateSubscriptionFromRecurly(
   recurlySubscription,
   subscription,
-  requesterData,
-  callback
+  requesterData
 ) {
   if (recurlySubscription.state === 'expired') {
-    return deleteSubscription(subscription, requesterData, callback)
+    await deleteSubscription(subscription, requesterData)
+    return
   }
   const updatedPlanCode = recurlySubscription.plan.plan_code
   const plan = PlansLocator.findLocalPlanInSettings(updatedPlanCode)
 
   if (plan == null) {
-    return callback(new Error(`plan code not found: ${updatedPlanCode}`))
+    throw new Error(`plan code not found: ${updatedPlanCode}`)
   }
   if (!plan.groupPlan && subscription.groupPlan) {
     // If downgrading from group to individual plan, delete group sub and create a new one
-    return _deleteAndReplaceSubscriptionFromRecurly(
+    await _deleteAndReplaceSubscriptionFromRecurly(
       recurlySubscription,
       subscription,
-      requesterData,
-      callback
+      requesterData
     )
+    return
   }
 
   subscription.recurlySubscription_id = recurlySubscription.uuid
@@ -336,25 +233,22 @@ function updateSubscriptionFromRecurly(
       })
     }
   }
-  subscription.save(function (error) {
-    if (error) {
-      return callback(error)
-    }
-    refreshUsersFeatures(subscription, callback)
-  })
+  await subscription.save()
+  await refreshUsersFeatures(subscription)
 }
 
 async function _sendUserGroupPlanCodeUserProperty(userId) {
   try {
-    const subscriptions =
-      (await SubscriptionLocator.promises.getMemberSubscriptions(userId)) || []
+    const subscriptions = await SubscriptionLocator.promises.getMemberSubscriptions(
+      userId
+    )
     let bestPlanCode = null
     let bestFeatures = {}
     for (const subscription of subscriptions) {
       const plan = PlansLocator.findLocalPlanInSettings(subscription.planCode)
       if (
         plan &&
-        FeaturesUpdater.isFeatureSetBetter(plan.features, bestFeatures)
+        FeaturesHelper.isFeatureSetBetter(plan.features, bestFeatures)
       ) {
         bestPlanCode = plan.planCode
         bestFeatures = plan.features
@@ -374,28 +268,28 @@ async function _sendUserGroupPlanCodeUserProperty(userId) {
 }
 
 module.exports = {
-  updateAdmin,
-  syncSubscription,
-  deleteSubscription,
-  createDeletedSubscription,
-  addUserToGroup,
-  refreshUsersFeatures,
-  removeUserFromGroup,
-  removeUserFromAllGroups,
-  deleteWithV1Id,
-  restoreSubscription,
-  updateSubscriptionFromRecurly,
+  updateAdmin: callbackify(updateAdmin),
+  syncSubscription: callbackify(syncSubscription),
+  deleteSubscription: callbackify(deleteSubscription),
+  createDeletedSubscription: callbackify(createDeletedSubscription),
+  addUserToGroup: callbackify(addUserToGroup),
+  refreshUsersFeatures: callbackify(refreshUsersFeatures),
+  removeUserFromGroup: callbackify(removeUserFromGroup),
+  removeUserFromAllGroups: callbackify(removeUserFromAllGroups),
+  deleteWithV1Id: callbackify(deleteWithV1Id),
+  restoreSubscription: callbackify(restoreSubscription),
+  updateSubscriptionFromRecurly: callbackify(updateSubscriptionFromRecurly),
   promises: {
-    updateAdmin: promisify(updateAdmin),
-    syncSubscription: promisify(syncSubscription),
-    addUserToGroup: promisify(addUserToGroup),
-    refreshUsersFeatures: promisify(refreshUsersFeatures),
-    removeUserFromGroup: promisify(removeUserFromGroup),
-    removeUserFromAllGroups: promisify(removeUserFromAllGroups),
-    deleteSubscription: promisify(deleteSubscription),
-    createDeletedSubscription: promisify(createDeletedSubscription),
-    deleteWithV1Id: promisify(deleteWithV1Id),
-    restoreSubscription: promisify(restoreSubscription),
-    updateSubscriptionFromRecurly: promisify(updateSubscriptionFromRecurly),
+    updateAdmin,
+    syncSubscription,
+    addUserToGroup,
+    refreshUsersFeatures,
+    removeUserFromGroup,
+    removeUserFromAllGroups,
+    deleteSubscription,
+    createDeletedSubscription,
+    deleteWithV1Id,
+    restoreSubscription,
+    updateSubscriptionFromRecurly,
   },
 }

services/web/app/src/Features/Subscription/UserFeaturesUpdater.js

@@ -1,4 +1,5 @@
 const { User } = require('../../models/User')
+const { promisifyAll } = require('../../util/promises')
 
 function _featuresChanged(newFeatures, featuresBefore) {
   for (const feature in newFeatures) {
@@ -39,3 +40,9 @@ module.exports = {
     })
   },
 }
+
+module.exports.promises = promisifyAll(module.exports, {
+  multiResult: {
+    updateFeatures: ['features', 'featuresChanged'],
+  },
+})

services/web/app/src/Features/Subscription/V1SubscriptionManager.js

@@ -16,6 +16,7 @@ const UserGetter = require('../User/UserGetter')
 const request = require('request')
 const settings = require('@overleaf/settings')
 const { V1ConnectionError, NotFoundError } = require('../Errors/Errors')
+const { promisifyAll } = require('../../util/promises')
 
 module.exports = V1SubscriptionManager = {
   // Returned planCode = 'v1_pro' | 'v1_pro_plus' | 'v1_student' | 'v1_free' | null
@@ -214,3 +215,10 @@ function __guard__(value, transform) {
     ? transform(value)
     : undefined
 }
+
+module.exports.promises = promisifyAll(module.exports, {
+  without: ['getGrandfatheredFeaturesForV1User'],
+  multiResult: {
+    getPlanCodeFromV1: ['planCode', 'v1Id'],
+  },
+})

services/web/scripts/refresh_features.js

@@ -3,6 +3,7 @@ const minimist = require('minimist')
 const _ = require('lodash')
 const async = require('async')
 const FeaturesUpdater = require('../app/src/Features/Subscription/FeaturesUpdater')
+const FeaturesHelper = require('../app/src/Features/Subscription/FeaturesHelper')
 const UserFeaturesUpdater = require('../app/src/Features/Subscription/UserFeaturesUpdater')
 
 const ScriptLogger = {
@@ -54,12 +55,12 @@ const ScriptLogger = {
 }
 
 const checkAndUpdateUser = (user, callback) =>
-  FeaturesUpdater._computeFeatures(user._id, (error, freshFeatures) => {
+  FeaturesUpdater.computeFeatures(user._id, (error, freshFeatures) => {
     if (error) {
       return callback(error)
     }
 
-    const mismatchReasons = FeaturesUpdater.compareFeatures(
+    const mismatchReasons = FeaturesHelper.compareFeatures(
       user.features,
       freshFeatures
     )

services/web/test/unit/src/Institutions/InstitutionsManagerTests.js

@@ -106,6 +106,8 @@ describe('InstitutionsManager', function () {
         },
         '../Subscription/FeaturesUpdater': {
           refreshFeatures: this.refreshFeatures,
+        },
+        '../Subscription/FeaturesHelper': {
           isFeatureSetBetter: (this.isFeatureSetBetter = sinon.stub()),
         },
         '../User/UserGetter': this.UserGetter,

services/web/test/unit/src/Subscription/FeaturesHelperTests.js

@@ -0,0 +1,126 @@
+const SandboxedModule = require('sandboxed-module')
+const { expect } = require('chai')
+
+const MODULE_PATH = '../../../../app/src/Features/Subscription/FeaturesHelper'
+
+describe('FeaturesHelper', function () {
+  beforeEach(function () {
+    this.FeaturesHelper = SandboxedModule.require(MODULE_PATH)
+  })
+
+  describe('mergeFeatures', function () {
+    it('should prefer priority over standard for compileGroup', function () {
+      expect(
+        this.FeaturesHelper.mergeFeatures(
+          { compileGroup: 'priority' },
+          { compileGroup: 'standard' }
+        )
+      ).to.deep.equal({ compileGroup: 'priority' })
+      expect(
+        this.FeaturesHelper.mergeFeatures(
+          { compileGroup: 'standard' },
+          { compileGroup: 'priority' }
+        )
+      ).to.deep.equal({ compileGroup: 'priority' })
+      expect(
+        this.FeaturesHelper.mergeFeatures(
+          { compileGroup: 'priority' },
+          { compileGroup: 'priority' }
+        )
+      ).to.deep.equal({ compileGroup: 'priority' })
+      expect(
+        this.FeaturesHelper.mergeFeatures(
+          { compileGroup: 'standard' },
+          { compileGroup: 'standard' }
+        )
+      ).to.deep.equal({ compileGroup: 'standard' })
+    })
+
+    it('should prefer -1 over any other for collaborators', function () {
+      expect(
+        this.FeaturesHelper.mergeFeatures(
+          { collaborators: -1 },
+          { collaborators: 10 }
+        )
+      ).to.deep.equal({ collaborators: -1 })
+      expect(
+        this.FeaturesHelper.mergeFeatures(
+          { collaborators: 10 },
+          { collaborators: -1 }
+        )
+      ).to.deep.equal({ collaborators: -1 })
+      expect(
+        this.FeaturesHelper.mergeFeatures(
+          { collaborators: 4 },
+          { collaborators: 10 }
+        )
+      ).to.deep.equal({ collaborators: 10 })
+    })
+
+    it('should prefer the higher of compileTimeout', function () {
+      expect(
+        this.FeaturesHelper.mergeFeatures(
+          { compileTimeout: 20 },
+          { compileTimeout: 10 }
+        )
+      ).to.deep.equal({ compileTimeout: 20 })
+      expect(
+        this.FeaturesHelper.mergeFeatures(
+          { compileTimeout: 10 },
+          { compileTimeout: 20 }
+        )
+      ).to.deep.equal({ compileTimeout: 20 })
+    })
+
+    it('should prefer the true over false for other keys', function () {
+      expect(
+        this.FeaturesHelper.mergeFeatures({ github: true }, { github: false })
+      ).to.deep.equal({ github: true })
+      expect(
+        this.FeaturesHelper.mergeFeatures({ github: false }, { github: true })
+      ).to.deep.equal({ github: true })
+      expect(
+        this.FeaturesHelper.mergeFeatures({ github: true }, { github: true })
+      ).to.deep.equal({ github: true })
+      expect(
+        this.FeaturesHelper.mergeFeatures({ github: false }, { github: false })
+      ).to.deep.equal({ github: false })
+    })
+  })
+
+  describe('isFeatureSetBetter', function () {
+    it('simple comparisons', function () {
+      const result1 = this.FeaturesHelper.isFeatureSetBetter(
+        { dropbox: true },
+        { dropbox: false }
+      )
+      expect(result1).to.be.true
+
+      const result2 = this.FeaturesHelper.isFeatureSetBetter(
+        { dropbox: false },
+        { dropbox: true }
+      )
+      expect(result2).to.be.false
+    })
+
+    it('compound comparisons with same features', function () {
+      const result1 = this.FeaturesHelper.isFeatureSetBetter(
+        { collaborators: 9, dropbox: true },
+        { collaborators: 10, dropbox: true }
+      )
+      expect(result1).to.be.false
+
+      const result2 = this.FeaturesHelper.isFeatureSetBetter(
+        { collaborators: -1, dropbox: true },
+        { collaborators: 10, dropbox: true }
+      )
+      expect(result2).to.be.true
+
+      const result3 = this.FeaturesHelper.isFeatureSetBetter(
+        { collaborators: -1, compileTimeout: 60, dropbox: true },
+        { collaborators: 10, compileTimeout: 60, dropbox: true }
+      )
+      expect(result3).to.be.true
+    })
+  })
+})

services/web/test/unit/src/Subscription/FeaturesUpdaterTests.js

@@ -1,556 +1,244 @@
 const SandboxedModule = require('sandboxed-module')
-const { assert, expect } = require('chai')
+const { expect } = require('chai')
 const sinon = require('sinon')
-const modulePath = '../../../../app/src/Features/Subscription/FeaturesUpdater'
 const { ObjectId } = require('mongodb')
 
+const MODULE_PATH = '../../../../app/src/Features/Subscription/FeaturesUpdater'
+
 describe('FeaturesUpdater', function () {
   beforeEach(function () {
-    this.user_id = ObjectId().toString()
+    this.user = {
+      _id: new ObjectId(),
+      features: {},
+    }
+    this.v1UserId = 12345
+    this.subscriptions = {
+      individual: { planCode: 'individual-plan' },
+      group1: { planCode: 'group-plan-1' },
+      group2: { planCode: 'group-plan-2' },
+      noDropbox: { planCode: 'no-dropbox' },
+    }
+
+    this.UserFeaturesUpdater = {
+      promises: {
+        updateFeatures: sinon
+          .stub()
+          .resolves({ features: { some: 'features' }, featuresChanged: true }),
+      },
+    }
+
+    this.SubscriptionLocator = {
+      promises: {
+        getUserIndividualSubscription: sinon.stub(),
+        getGroupSubscriptionsMemberOf: sinon.stub(),
+      },
+    }
+    this.SubscriptionLocator.promises.getUserIndividualSubscription
+      .withArgs(this.user._id)
+      .resolves(this.subscriptions.individual)
+    this.SubscriptionLocator.promises.getGroupSubscriptionsMemberOf
+      .withArgs(this.user._id)
+      .resolves([this.subscriptions.group1, this.subscriptions.group2])
+
+    this.Settings = {
+      defaultFeatures: { default: 'features' },
+      plans: [
+        { planCode: 'individual-plan', features: { individual: 'features' } },
+        { planCode: 'group-plan-1', features: { group1: 'features' } },
+        { planCode: 'group-plan-2', features: { group2: 'features' } },
+        { planCode: 'v1-plan', features: { v1: 'features' } },
+        { planCode: 'no-dropbox', features: { dropbox: false } },
+      ],
+      features: {
+        all: {
+          default: 'features',
+          individual: 'features',
+          group1: 'features',
+          group2: 'features',
+          institutions: 'features',
+          v1: 'features',
+          grandfathered: 'features',
+          bonus: 'features',
+        },
+      },
+    }
 
-    this.FeaturesUpdater = SandboxedModule.require(modulePath, {
+    this.ReferalFeatures = {
+      promises: {
+        getBonusFeatures: sinon.stub().resolves({ bonus: 'features' }),
+      },
+    }
+    this.V1SubscriptionManager = {
+      getGrandfatheredFeaturesForV1User: sinon.stub(),
+      promises: {
+        getPlanCodeFromV1: sinon.stub(),
+      },
+    }
+    this.V1SubscriptionManager.promises.getPlanCodeFromV1
+      .withArgs(this.user._id)
+      .resolves({ planCode: 'v1-plan', v1Id: this.v1UserId })
+    this.V1SubscriptionManager.getGrandfatheredFeaturesForV1User
+      .withArgs(this.v1UserId)
+      .returns({ grandfathered: 'features' })
+
+    this.InstitutionsFeatures = {
+      promises: {
+        getInstitutionsFeatures: sinon
+          .stub()
+          .resolves({ institutions: 'features' }),
+      },
+    }
+
+    this.UserGetter = {
+      promises: {
+        getUser: sinon.stub().resolves(null),
+      },
+    }
+    this.UserGetter.promises.getUser.withArgs(this.user._id).resolves(this.user)
+    this.UserGetter.promises.getUser
+      .withArgs({ 'overleaf.id': this.v1UserId })
+      .resolves(this.user)
+
+    this.AnalyticsManager = {
+      setUserPropertyForUser: sinon.stub(),
+    }
+    this.Modules = {
+      promises: { hooks: { fire: sinon.stub().resolves() } },
+    }
+    this.FeaturesHelper = {
+      mergeFeatures: sinon.stub().callsFake((a, b) => ({ ...a, ...b })),
+    }
+
+    this.FeaturesUpdater = SandboxedModule.require(MODULE_PATH, {
       requires: {
-        './UserFeaturesUpdater': (this.UserFeaturesUpdater = {}),
-        './SubscriptionLocator': (this.SubscriptionLocator = {}),
-        './PlansLocator': (this.PlansLocator = {}),
-        '@overleaf/settings': (this.Settings = {
-          features: {
-            personal: {
-              collaborators: 1,
-              dropbox: false,
-              compileTimeout: 60,
-              compileGroup: 'standard',
-            },
-            collaborator: {
-              collaborators: 10,
-              dropbox: true,
-              compileTimeout: 240,
-              compileGroup: 'priority',
-            },
-            professional: {
-              collaborators: -1,
-              dropbox: true,
-              compileTimeout: 240,
-              compileGroup: 'priority',
-            },
-          },
-        }),
-        '../Referal/ReferalFeatures': (this.ReferalFeatures = {}),
-        './V1SubscriptionManager': (this.V1SubscriptionManager = {}),
-        '../Institutions/InstitutionsFeatures': (this.InstitutionsFeatures = {}),
-        '../User/UserGetter': (this.UserGetter = {}),
-        '../Analytics/AnalyticsManager': (this.AnalyticsManager = {
-          setUserPropertyForUser: sinon.stub(),
-        }),
-        '../../infrastructure/Modules': (this.Modules = {
-          hooks: { fire: sinon.stub() },
-        }),
+        './UserFeaturesUpdater': this.UserFeaturesUpdater,
+        './SubscriptionLocator': this.SubscriptionLocator,
+        './FeaturesHelper': this.FeaturesHelper,
+        '@overleaf/settings': this.Settings,
+        '../Referal/ReferalFeatures': this.ReferalFeatures,
+        './V1SubscriptionManager': this.V1SubscriptionManager,
+        '../Institutions/InstitutionsFeatures': this.InstitutionsFeatures,
+        '../User/UserGetter': this.UserGetter,
+        '../Analytics/AnalyticsManager': this.AnalyticsManager,
+        '../../infrastructure/Modules': this.Modules,
       },
     })
   })
 
   describe('refreshFeatures', function () {
-    beforeEach(function () {
-      this.user = {
-        _id: this.user_id,
-        features: {},
-      }
-      this.UserFeaturesUpdater.updateFeatures = sinon
-        .stub()
-        .yields(null, { some: 'features' }, true)
-      this.FeaturesUpdater._getIndividualFeatures = sinon
-        .stub()
-        .yields(null, { individual: 'features' })
-      this.FeaturesUpdater._getGroupFeatureSets = sinon
-        .stub()
-        .yields(null, [{ group: 'features' }, { group: 'features2' }])
-      this.InstitutionsFeatures.getInstitutionsFeatures = sinon
-        .stub()
-        .yields(null, { institutions: 'features' })
-      this.FeaturesUpdater._getV1Features = sinon
-        .stub()
-        .yields(null, { v1: 'features' })
-      this.ReferalFeatures.getBonusFeatures = sinon
-        .stub()
-        .yields(null, { bonus: 'features' })
-      this.FeaturesUpdater._mergeFeatures = sinon
-        .stub()
-        .returns({ merged: 'features' })
-      this.UserGetter.getUser = sinon.stub().yields(null, this.user)
-      this.callback = sinon.stub()
-    })
-    it('should return features and featuresChanged', function () {
-      this.FeaturesUpdater.refreshFeatures(
-        this.user_id,
-        'test',
-        (err, features, featuresChanged) => {
-          expect(err).to.not.exist
-          expect(features).to.exist
-          expect(featuresChanged).to.exist
-        }
+    it('should return features and featuresChanged', async function () {
+      const {
+        features,
+        featuresChanged,
+      } = await this.FeaturesUpdater.promises.refreshFeatures(
+        this.user._id,
+        'test'
       )
+      expect(features).to.exist
+      expect(featuresChanged).to.exist
     })
+
     describe('normally', function () {
-      beforeEach(function () {
-        this.FeaturesUpdater.refreshFeatures(
-          this.user_id,
-          'test',
-          this.callback
+      beforeEach(async function () {
+        await this.FeaturesUpdater.promises.refreshFeatures(
+          this.user._id,
+          'test'
         )
       })
-      it('should get the individual features', function () {
-        this.FeaturesUpdater._getIndividualFeatures
-          .calledWith(this.user_id)
-          .should.equal(true)
-      })
-      it('should get the group features', function () {
-        this.FeaturesUpdater._getGroupFeatureSets
-          .calledWith(this.user_id)
-          .should.equal(true)
-      })
-      it('should get the institution features', function () {
-        this.InstitutionsFeatures.getInstitutionsFeatures
-          .calledWith(this.user_id)
-          .should.equal(true)
-      })
-      it('should get the v1 features', function () {
-        this.FeaturesUpdater._getV1Features
-          .calledWith(this.user_id)
-          .should.equal(true)
-      })
-      it('should get the bonus features', function () {
-        this.ReferalFeatures.getBonusFeatures
-          .calledWith(this.user_id)
-          .should.equal(true)
-      })
-      it('should merge from the default features', function () {
-        this.FeaturesUpdater._mergeFeatures
-          .calledWith(this.Settings.defaultFeatures)
-          .should.equal(true)
-      })
-
-      it('should merge the individual features', function () {
-        this.FeaturesUpdater._mergeFeatures
-          .calledWith(sinon.match.any, { individual: 'features' })
-          .should.equal(true)
-      })
-
-      it('should merge the group features', function () {
-        this.FeaturesUpdater._mergeFeatures
-          .calledWith(sinon.match.any, { group: 'features' })
-          .should.equal(true)
-        this.FeaturesUpdater._mergeFeatures
-          .calledWith(sinon.match.any, { group: 'features2' })
-          .should.equal(true)
-      })
-
-      it('should merge the institutions features', function () {
-        this.FeaturesUpdater._mergeFeatures
-          .calledWith(sinon.match.any, { institutions: 'features' })
-          .should.equal(true)
-      })
-
-      it('should merge the v1 features', function () {
-        this.FeaturesUpdater._mergeFeatures
-          .calledWith(sinon.match.any, { v1: 'features' })
-          .should.equal(true)
-      })
-
-      it('should merge the bonus features', function () {
-        this.FeaturesUpdater._mergeFeatures
-          .calledWith(sinon.match.any, { bonus: 'features' })
-          .should.equal(true)
-      })
 
       it('should update the user with the merged features', function () {
-        this.UserFeaturesUpdater.updateFeatures
-          .calledWith(this.user_id, { merged: 'features' })
-          .should.equal(true)
+        expect(
+          this.UserFeaturesUpdater.promises.updateFeatures
+        ).to.have.been.calledWith(this.user._id, this.Settings.features.all)
       })
-    })
 
-    describe('analytics user properties', function () {
       it('should send the corresponding feature set user property', function () {
-        this.FeaturesUpdater._mergeFeatures = sinon
-          .stub()
-          .returns(this.Settings.features.personal)
-
-        this.FeaturesUpdater.refreshFeatures(
-          this.user_id,
-          'test',
-          this.callback
-        )
+        expect(
+          this.AnalyticsManager.setUserPropertyForUser
+        ).to.have.been.calledWith(this.user._id, 'feature-set', 'all')
+      })
+    })
 
-        sinon.assert.calledWith(
-          this.AnalyticsManager.setUserPropertyForUser,
-          this.user_id,
-          'feature-set',
-          'personal'
+    describe('with a non-standard feature set', async function () {
+      beforeEach(async function () {
+        this.SubscriptionLocator.promises.getGroupSubscriptionsMemberOf
+          .withArgs(this.user._id)
+          .resolves(null)
+        await this.FeaturesUpdater.promises.refreshFeatures(
+          this.user._id,
+          'test'
         )
       })
 
       it('should send mixed feature set user property', function () {
-        this.FeaturesUpdater._mergeFeatures = sinon
-          .stub()
-          .returns({ dropbox: true, feature: 'some' })
-
-        this.FeaturesUpdater.refreshFeatures(
-          this.user_id,
-          'test',
-          this.callback
-        )
-
         sinon.assert.calledWith(
           this.AnalyticsManager.setUserPropertyForUser,
-          this.user_id,
+          this.user._id,
           'feature-set',
           'mixed'
         )
       })
     })
 
-    describe('when losing dropbox feature', function () {
-      beforeEach(function () {
-        this.user = {
-          _id: this.user_id,
-          features: { dropbox: true },
-        }
-        this.UserGetter.getUser = sinon.stub().yields(null, this.user)
-        this.FeaturesUpdater._mergeFeatures = sinon
-          .stub()
-          .returns({ dropbox: false })
-        this.FeaturesUpdater.refreshFeatures(
-          this.user_id,
-          'test',
-          this.callback
+    describe('when losing dropbox feature', async function () {
+      beforeEach(async function () {
+        this.user.features = { dropbox: true }
+        this.SubscriptionLocator.promises.getUserIndividualSubscription
+          .withArgs(this.user._id)
+          .resolves(this.subscriptions.noDropbox)
+        await this.FeaturesUpdater.promises.refreshFeatures(
+          this.user._id,
+          'test'
         )
       })
-      it('should fire module hook to unlink dropbox', function () {
-        this.Modules.hooks.fire
-          .calledWith('removeDropbox', this.user._id, 'test')
-          .should.equal(true)
-      })
-    })
-  })
 
-  describe('_mergeFeatures', function () {
-    it('should prefer priority over standard for compileGroup', function () {
-      expect(
-        this.FeaturesUpdater._mergeFeatures(
-          {
-            compileGroup: 'priority',
-          },
-          {
-            compileGroup: 'standard',
-          }
-        )
-      ).to.deep.equal({
-        compileGroup: 'priority',
-      })
-      expect(
-        this.FeaturesUpdater._mergeFeatures(
-          {
-            compileGroup: 'standard',
-          },
-          {
-            compileGroup: 'priority',
-          }
-        )
-      ).to.deep.equal({
-        compileGroup: 'priority',
-      })
-      expect(
-        this.FeaturesUpdater._mergeFeatures(
-          {
-            compileGroup: 'priority',
-          },
-          {
-            compileGroup: 'priority',
-          }
-        )
-      ).to.deep.equal({
-        compileGroup: 'priority',
-      })
-      expect(
-        this.FeaturesUpdater._mergeFeatures(
-          {
-            compileGroup: 'standard',
-          },
-          {
-            compileGroup: 'standard',
-          }
-        )
-      ).to.deep.equal({
-        compileGroup: 'standard',
-      })
-    })
-
-    it('should prefer -1 over any other for collaborators', function () {
-      expect(
-        this.FeaturesUpdater._mergeFeatures(
-          {
-            collaborators: -1,
-          },
-          {
-            collaborators: 10,
-          }
-        )
-      ).to.deep.equal({
-        collaborators: -1,
-      })
-      expect(
-        this.FeaturesUpdater._mergeFeatures(
-          {
-            collaborators: 10,
-          },
-          {
-            collaborators: -1,
-          }
-        )
-      ).to.deep.equal({
-        collaborators: -1,
-      })
-      expect(
-        this.FeaturesUpdater._mergeFeatures(
-          {
-            collaborators: 4,
-          },
-          {
-            collaborators: 10,
-          }
-        )
-      ).to.deep.equal({
-        collaborators: 10,
-      })
-    })
-
-    it('should prefer the higher of compileTimeout', function () {
-      expect(
-        this.FeaturesUpdater._mergeFeatures(
-          {
-            compileTimeout: 20,
-          },
-          {
-            compileTimeout: 10,
-          }
-        )
-      ).to.deep.equal({
-        compileTimeout: 20,
-      })
-      expect(
-        this.FeaturesUpdater._mergeFeatures(
-          {
-            compileTimeout: 10,
-          },
-          {
-            compileTimeout: 20,
-          }
-        )
-      ).to.deep.equal({
-        compileTimeout: 20,
-      })
-    })
-
-    it('should prefer the true over false for other keys', function () {
-      expect(
-        this.FeaturesUpdater._mergeFeatures(
-          {
-            github: true,
-          },
-          {
-            github: false,
-          }
-        )
-      ).to.deep.equal({
-        github: true,
-      })
-      expect(
-        this.FeaturesUpdater._mergeFeatures(
-          {
-            github: false,
-          },
-          {
-            github: true,
-          }
-        )
-      ).to.deep.equal({
-        github: true,
-      })
-      expect(
-        this.FeaturesUpdater._mergeFeatures(
-          {
-            github: true,
-          },
-          {
-            github: true,
-          }
-        )
-      ).to.deep.equal({
-        github: true,
-      })
-      expect(
-        this.FeaturesUpdater._mergeFeatures(
-          {
-            github: false,
-          },
-          {
-            github: false,
-          }
+      it('should fire module hook to unlink dropbox', function () {
+        expect(this.Modules.promises.hooks.fire).to.have.been.calledWith(
+          'removeDropbox',
+          this.user._id,
+          'test'
         )
-      ).to.deep.equal({
-        github: false,
       })
     })
   })
 
   describe('doSyncFromV1', function () {
-    beforeEach(function () {
-      this.v1UserId = 1
-      this.user = {
-        _id: this.user_id,
-        email: 'user@example.com',
-        overleaf: {
-          id: this.v1UserId,
-        },
-      }
-
-      this.UserGetter.getUser = sinon.stub().callsArgWith(2, null, this.user)
-      this.FeaturesUpdater.refreshFeatures = sinon.stub().yields(null)
-      this.call = cb => {
-        this.FeaturesUpdater.doSyncFromV1(this.v1UserId, cb)
-      }
-    })
-
     describe('when all goes well', function () {
-      it('should call getUser', function (done) {
-        this.call(() => {
-          expect(this.UserGetter.getUser.callCount).to.equal(1)
-          expect(
-            this.UserGetter.getUser.calledWith({ 'overleaf.id': this.v1UserId })
-          ).to.equal(true)
-          done()
-        })
-      })
-
-      it('should call refreshFeatures', function (done) {
-        this.call(() => {
-          expect(this.FeaturesUpdater.refreshFeatures.callCount).to.equal(1)
-          expect(
-            this.FeaturesUpdater.refreshFeatures.calledWith(this.user_id)
-          ).to.equal(true)
-          done()
-        })
+      beforeEach(async function () {
+        await this.FeaturesUpdater.promises.doSyncFromV1(this.v1UserId)
       })
 
-      it('should not produce an error', function (done) {
-        this.call(err => {
-          expect(err).to.not.exist
-          done()
-        })
+      it('should update the user with the merged features', function () {
+        expect(
+          this.UserFeaturesUpdater.promises.updateFeatures
+        ).to.have.been.calledWith(this.user._id, this.Settings.features.all)
       })
     })
 
     describe('when getUser produces an error', function () {
       beforeEach(function () {
-        this.UserGetter.getUser = sinon
-          .stub()
-          .callsArgWith(2, new Error('woops'))
-      })
-
-      it('should not call refreshFeatures', function () {
-        expect(this.FeaturesUpdater.refreshFeatures.callCount).to.equal(0)
+        this.UserGetter.promises.getUser.rejects(new Error('woops'))
       })
 
-      it('should produce an error', function (done) {
-        this.call(err => {
-          expect(err).to.exist
-          done()
-        })
+      it('should propagate the error', async function () {
+        const someId = 9090
+        await expect(this.FeaturesUpdater.promises.doSyncFromV1(someId)).to.be
+          .rejected
+        expect(this.UserFeaturesUpdater.promises.updateFeatures).not.to.have
+          .been.called
       })
     })
 
     describe('when getUser does not find a user', function () {
-      beforeEach(function () {
-        this.UserGetter.getUser = sinon.stub().callsArgWith(2, null, null)
-      })
-
-      it('should not call refreshFeatures', function (done) {
-        this.call(() => {
-          expect(this.FeaturesUpdater.refreshFeatures.callCount).to.equal(0)
-          done()
-        })
+      beforeEach(async function () {
+        const someOtherId = 987
+        await this.FeaturesUpdater.promises.doSyncFromV1(someOtherId)
       })
 
-      it('should not produce an error', function (done) {
-        this.call(err => {
-          expect(err).to.not.exist
-          done()
-        })
+      it('should not update the user', function () {
+        expect(this.UserFeaturesUpdater.promises.updateFeatures).not.to.have
+          .been.called
       })
     })
   })
-
-  describe('isFeatureSetBetter', function () {
-    it('simple comparisons', function () {
-      const result1 = this.FeaturesUpdater.isFeatureSetBetter(
-        {
-          dropbox: true,
-        },
-        {
-          dropbox: false,
-        }
-      )
-      assert.isTrue(result1)
-
-      const result2 = this.FeaturesUpdater.isFeatureSetBetter(
-        {
-          dropbox: false,
-        },
-        {
-          dropbox: true,
-        }
-      )
-      assert.isFalse(result2)
-    })
-
-    it('compound comparisons with same features', function () {
-      const result1 = this.FeaturesUpdater.isFeatureSetBetter(
-        {
-          collaborators: 9,
-          dropbox: true,
-        },
-        {
-          collaborators: 10,
-          dropbox: true,
-        }
-      )
-      assert.isFalse(result1)
-
-      const result2 = this.FeaturesUpdater.isFeatureSetBetter(
-        {
-          collaborators: -1,
-          dropbox: true,
-        },
-        {
-          collaborators: 10,
-          dropbox: true,
-        }
-      )
-      assert.isTrue(result2)
-
-      const result3 = this.FeaturesUpdater.isFeatureSetBetter(
-        {
-          collaborators: -1,
-          compileTimeout: 60,
-          dropbox: true,
-        },
-        {
-          collaborators: 10,
-          compileTimeout: 60,
-          dropbox: true,
-        }
-      )
-      assert.isTrue(result3)
-    })
-  })
 })

services/web/test/unit/src/Subscription/SubscriptionHandlerTests.js

@@ -2,7 +2,8 @@ const SandboxedModule = require('sandboxed-module')
 const sinon = require('sinon')
 const chai = require('chai')
 const { expect } = chai
-const modulePath =
+
+const MODULE_PATH =
   '../../../../app/src/Features/Subscription/SubscriptionHandler'
 
 const mockRecurlySubscriptions = {
@@ -47,6 +48,8 @@ const mockSubscriptionChanges = {
 
 describe('SubscriptionHandler', function () {
   beforeEach(function () {
+    this.callback = sinon.stub()
+
     this.Settings = {
       plans: [
         {
@@ -85,6 +88,7 @@ describe('SubscriptionHandler', function () {
       getBillingInfo: sinon.stub().yields(),
       getAccountPastDueInvoices: sinon.stub().yields(),
       attemptInvoiceCollection: sinon.stub().yields(),
+      listAccountActiveSubscriptions: sinon.stub().yields(null, []),
     }
     this.RecurlyClient = {
       changeSubscriptionByUuid: sinon
@@ -118,7 +122,7 @@ describe('SubscriptionHandler', function () {
       shouldPlanChangeAtTermEnd: sinon.stub(),
     }
 
-    this.SubscriptionHandler = SandboxedModule.require(modulePath, {
+    this.SubscriptionHandler = SandboxedModule.require(MODULE_PATH, {
       requires: {
         './RecurlyWrapper': this.RecurlyWrapper,
         './RecurlyClient': this.RecurlyClient,
@@ -134,23 +138,15 @@ describe('SubscriptionHandler', function () {
         './SubscriptionHelper': this.SubscriptionHelper,
       },
     })
-
-    this.SubscriptionHandler.syncSubscriptionToUser = sinon
-      .stub()
-      .callsArgWith(2)
   })
 
   describe('createSubscription', function () {
     beforeEach(function () {
-      this.callback = sinon.stub()
       this.subscriptionDetails = {
         cvv: '123',
         number: '12345',
       }
       this.recurlyTokenIds = { billing: '45555666' }
-      this.SubscriptionHandler.validateNoSubscriptionInRecurly = sinon
-        .stub()
-        .yields(null, true)
     })
 
     describe('successfully', function () {
@@ -182,9 +178,9 @@ describe('SubscriptionHandler', function () {
 
     describe('when there is already a subscription in Recurly', function () {
       beforeEach(function () {
-        this.SubscriptionHandler.validateNoSubscriptionInRecurly = sinon
-          .stub()
-          .yields(null, false)
+        this.RecurlyWrapper.listAccountActiveSubscriptions.yields(null, [
+          this.subscription,
+        ])
         this.SubscriptionHandler.createSubscription(
           this.user,
           this.subscriptionDetails,
@@ -235,9 +231,9 @@ describe('SubscriptionHandler', function () {
             callback(null, this.user)
           }
           this.plan_code = 'collaborator'
-          this.SubscriptionHelper.shouldPlanChangeAtTermEnd = sinon
-            .stub()
-            .returns(shouldPlanChangeAtTermEnd)
+          this.SubscriptionHelper.shouldPlanChangeAtTermEnd.returns(
+            shouldPlanChangeAtTermEnd
+          )
           this.LimitationsManager.userHasV2Subscription.callsArgWith(
             1,
             null,
@@ -277,14 +273,13 @@ describe('SubscriptionHandler', function () {
             callback(null, this.user)
           }
           this.plan_code = 'collaborator'
-          this.PlansLocator.findLocalPlanInSettings = sinon.stub().returns(null)
+          this.PlansLocator.findLocalPlanInSettings.returns(null)
           this.LimitationsManager.userHasV2Subscription.callsArgWith(
             1,
             null,
             true,
             this.subscription
           )
-          this.callback = sinon.stub()
           this.SubscriptionHandler.updateSubscription(
             this.user,
             this.plan_code,
@@ -322,9 +317,7 @@ describe('SubscriptionHandler', function () {
 
       it('should redirect to the subscription dashboard', function () {
         this.RecurlyClient.changeSubscriptionByUuid.called.should.equal(false)
-        this.SubscriptionHandler.syncSubscriptionToUser.called.should.equal(
-          false
-        )
+        this.SubscriptionUpdater.syncSubscription.called.should.equal(false)
       })
     })
 
@@ -561,18 +554,11 @@ describe('SubscriptionHandler', function () {
   })
 
   describe('validateNoSubscriptionInRecurly', function () {
-    beforeEach(function () {
-      this.subscriptions = []
-      this.RecurlyWrapper.listAccountActiveSubscriptions = sinon
-        .stub()
-        .yields(null, this.subscriptions)
-      this.SubscriptionUpdater.syncSubscription = sinon.stub().yields()
-      this.callback = sinon.stub()
-    })
-
-    describe('with no subscription in recurly', function () {
+    describe('with a subscription in recurly', function () {
       beforeEach(function () {
-        this.subscriptions.push((this.subscription = { mock: 'subscription' }))
+        this.RecurlyWrapper.listAccountActiveSubscriptions.yields(null, [
+          this.subscription,
+        ])
         this.SubscriptionHandler.validateNoSubscriptionInRecurly(
           this.user_id,
           this.callback
@@ -596,7 +582,7 @@ describe('SubscriptionHandler', function () {
       })
     })
 
-    describe('with a subscription in recurly', function () {
+    describe('with no subscription in recurly', function () {
       beforeEach(function () {
         this.SubscriptionHandler.validateNoSubscriptionInRecurly(
           this.user_id,

services/web/test/unit/src/Subscription/SubscriptionUpdaterTests.js

@@ -7,12 +7,14 @@ const { ObjectId } = require('mongodb')
 
 describe('SubscriptionUpdater', function () {
   beforeEach(function () {
+    this.recurlyPlan = { planCode: 'recurly-plan' }
     this.recurlySubscription = {
       uuid: '1238uoijdasjhd',
       plan: {
-        plan_code: 'kjhsakjds',
+        plan_code: this.recurlyPlan.planCode,
       },
     }
+
     this.adminUser = { _id: (this.adminuser_id = '5208dd34438843e2db000007') }
     this.otherUserId = '5208dd34438842e2db000005'
     this.allUserIds = ['13213', 'dsadas', 'djsaiud89']
@@ -21,7 +23,7 @@ describe('SubscriptionUpdater', function () {
       admin_id: this.adminUser._id,
       manager_ids: [this.adminUser._id],
       member_ids: [],
-      save: sinon.stub().callsArgWith(0),
+      save: sinon.stub().resolves(),
       planCode: 'student_or_something',
     }
     this.user_id = this.adminuser_id
@@ -31,7 +33,7 @@ describe('SubscriptionUpdater', function () {
       admin_id: this.adminUser._id,
       manager_ids: [this.adminUser._id],
       member_ids: this.allUserIds,
-      save: sinon.stub().callsArgWith(0),
+      save: sinon.stub().resolves(),
       groupPlan: true,
       planCode: 'group_subscription',
     }
@@ -40,17 +42,11 @@ describe('SubscriptionUpdater', function () {
       admin_id: this.adminUser._id,
       manager_ids: [this.adminUser._id],
       member_ids: [this.otherUserId],
-      save: sinon.stub().callsArgWith(0),
+      save: sinon.stub().resolves(),
       groupPlan: true,
       planCode: 'better_group_subscription',
     }
 
-    this.updateStub = sinon.stub().callsArgWith(2, null)
-    this.updateManyStub = sinon.stub().callsArgWith(2, null)
-    this.findOneAndUpdateStub = sinon
-      .stub()
-      .callsArgWith(2, null, this.subscription)
-
     const subscription = this.subscription
     this.SubscriptionModel = class {
       constructor(opts) {
@@ -61,62 +57,93 @@ describe('SubscriptionUpdater', function () {
       }
 
       save() {
-        return subscription
+        return Promise.resolve(subscription)
       }
     }
-    this.SubscriptionModel.deleteOne = sinon.stub().yields()
-    this.SubscriptionModel.updateOne = this.updateStub
-    this.SubscriptionModel.updateMany = this.updateManyStub
-    this.SubscriptionModel.findOneAndUpdate = this.findOneAndUpdateStub
+    this.SubscriptionModel.deleteOne = sinon
+      .stub()
+      .returns({ exec: sinon.stub().resolves() })
+    this.SubscriptionModel.updateOne = sinon
+      .stub()
+      .returns({ exec: sinon.stub().resolves() })
+    this.SubscriptionModel.updateMany = sinon
+      .stub()
+      .returns({ exec: sinon.stub().resolves() })
+    this.SubscriptionModel.findOneAndUpdate = sinon.stub().returns({
+      exec: sinon.stub().resolves(this.subscription),
+    })
 
     this.SubscriptionLocator = {
-      getUsersSubscription: sinon.stub(),
-      getGroupSubscriptionMemberOf: sinon.stub(),
-      getMemberSubscriptions: sinon.stub().yields(null, []),
       promises: {
-        getMemberSubscriptions: sinon.stub().returns([this.groupSubscription]),
+        getUsersSubscription: sinon.stub(),
+        getGroupSubscriptionMemberOf: sinon.stub(),
+        getMemberSubscriptions: sinon.stub().resolves([]),
+        getSubscription: sinon.stub(),
       },
     }
 
+    this.SubscriptionLocator.promises.getSubscription
+      .withArgs(this.subscription._id)
+      .resolves(this.subscription)
+
     this.Settings = {
       defaultPlanCode: 'personal',
       defaultFeatures: { default: 'features' },
+      plans: [
+        this.recurlyPlan,
+        { planCode: this.subscription.planCode, features: {} },
+        {
+          planCode: this.groupSubscription.planCode,
+          features: {
+            collaborators: 10,
+            compileTimeout: 60,
+            dropbox: true,
+          },
+        },
+        {
+          planCode: this.betterGroupSubscription.planCode,
+          features: {
+            collaborators: -1,
+            compileTimeout: 240,
+            dropbox: true,
+          },
+        },
+      ],
     }
 
-    this.UserFeaturesUpdater = { updateFeatures: sinon.stub().yields() }
-
-    this.PlansLocator = { findLocalPlanInSettings: sinon.stub().returns({}) }
+    this.UserFeaturesUpdater = {
+      promises: {
+        updateFeatures: sinon.stub().resolves(),
+      },
+    }
 
-    this.UserGetter = {
-      getUsers(memberIds, projection, callback) {
-        const users = memberIds.map(id => ({ _id: id }))
-        callback(null, users)
+    this.ReferalFeatures = {
+      promises: {
+        getBonusFeatures: sinon.stub().resolves(),
       },
-      getUser: sinon.stub(),
     }
 
-    this.ReferalFeatures = { getBonusFeatures: sinon.stub().callsArgWith(1) }
-    this.Modules = { hooks: { fire: sinon.stub().callsArgWith(2, null, null) } }
     this.FeaturesUpdater = {
-      refreshFeatures: sinon.stub().yields(),
-      planCodeToFeatures: sinon.stub(),
+      promises: {
+        refreshFeatures: sinon.stub().resolves({}),
+      },
     }
+
     this.DeletedSubscription = {
-      findOneAndUpdate: sinon.stub().yields(),
+      findOneAndUpdate: sinon.stub().returns({ exec: sinon.stub().resolves() }),
     }
+
     this.AnalyticsManager = {
       setUserPropertyForUser: sinon.stub(),
     }
+
     this.SubscriptionUpdater = SandboxedModule.require(modulePath, {
       requires: {
-        mongodb: { ObjectId },
         '../../models/Subscription': {
           Subscription: this.SubscriptionModel,
         },
         './UserFeaturesUpdater': this.UserFeaturesUpdater,
         './SubscriptionLocator': this.SubscriptionLocator,
-        '../User/UserGetter': this.UserGetter,
-        './PlansLocator': this.PlansLocator,
         '@overleaf/settings': this.Settings,
         '../../infrastructure/mongodb': { db: {}, ObjectId },
         './FeaturesUpdater': this.FeaturesUpdater,
@@ -129,266 +156,184 @@ describe('SubscriptionUpdater', function () {
   })
 
   describe('updateAdmin', function () {
-    it('should update the subscription admin', function (done) {
+    it('should update the subscription admin', async function () {
       this.subscription.groupPlan = true
-      this.SubscriptionUpdater.updateAdmin(
+      await this.SubscriptionUpdater.promises.updateAdmin(
         this.subscription,
-        this.otherUserId,
-        err => {
-          if (err != null) {
-            return done(err)
-          }
-          const query = {
-            _id: ObjectId(this.subscription._id),
-            customAccount: true,
-          }
-          const update = {
-            $set: { admin_id: ObjectId(this.otherUserId) },
-            $addToSet: { manager_ids: ObjectId(this.otherUserId) },
-          }
-          this.SubscriptionModel.updateOne.should.have.been.calledOnce
-          this.SubscriptionModel.updateOne.should.have.been.calledWith(
-            query,
-            update
-          )
-          done()
-        }
+        this.otherUserId
+      )
+      const query = {
+        _id: ObjectId(this.subscription._id),
+        customAccount: true,
+      }
+      const update = {
+        $set: { admin_id: ObjectId(this.otherUserId) },
+        $addToSet: { manager_ids: ObjectId(this.otherUserId) },
+      }
+      this.SubscriptionModel.updateOne.should.have.been.calledOnce
+      this.SubscriptionModel.updateOne.should.have.been.calledWith(
+        query,
+        update
       )
     })
 
-    it('should remove the manager for non-group subscriptions', function (done) {
-      this.SubscriptionUpdater.updateAdmin(
+    it('should remove the manager for non-group subscriptions', async function () {
+      await this.SubscriptionUpdater.promises.updateAdmin(
         this.subscription,
-        this.otherUserId,
-        err => {
-          if (err != null) {
-            return done(err)
-          }
-          const query = {
-            _id: ObjectId(this.subscription._id),
-            customAccount: true,
-          }
-          const update = {
-            $set: {
-              admin_id: ObjectId(this.otherUserId),
-              manager_ids: [ObjectId(this.otherUserId)],
-            },
-          }
-          this.SubscriptionModel.updateOne.should.have.been.calledOnce
-          this.SubscriptionModel.updateOne.should.have.been.calledWith(
-            query,
-            update
-          )
-          done()
-        }
+        this.otherUserId
+      )
+      const query = {
+        _id: ObjectId(this.subscription._id),
+        customAccount: true,
+      }
+      const update = {
+        $set: {
+          admin_id: ObjectId(this.otherUserId),
+          manager_ids: [ObjectId(this.otherUserId)],
+        },
+      }
+      this.SubscriptionModel.updateOne.should.have.been.calledOnce
+      this.SubscriptionModel.updateOne.should.have.been.calledWith(
+        query,
+        update
       )
     })
   })
 
   describe('syncSubscription', function () {
     beforeEach(function () {
-      this.SubscriptionLocator.getUsersSubscription.callsArgWith(
-        1,
-        null,
+      this.SubscriptionLocator.promises.getUsersSubscription.resolves(
         this.subscription
       )
-      this.SubscriptionUpdater.updateSubscriptionFromRecurly = sinon
-        .stub()
-        .yields()
     })
 
-    it('should update the subscription if the user already is admin of one', function (done) {
-      this.SubscriptionUpdater.syncSubscription(
+    it('should update the subscription if the user already is admin of one', async function () {
+      await this.SubscriptionUpdater.promises.syncSubscription(
         this.recurlySubscription,
-        this.adminUser._id,
-        err => {
-          if (err != null) {
-            return done(err)
-          }
-          this.SubscriptionLocator.getUsersSubscription
-            .calledWith(this.adminUser._id)
-            .should.equal(true)
-          done()
-        }
+        this.adminUser._id
       )
+      this.SubscriptionLocator.promises.getUsersSubscription
+        .calledWith(this.adminUser._id)
+        .should.equal(true)
     })
 
-    it('should not call updateFeatures with group subscription if recurly subscription is not expired', function (done) {
-      this.SubscriptionUpdater.syncSubscription(
+    it('should not call updateFeatures with group subscription if recurly subscription is not expired', async function () {
+      await this.SubscriptionUpdater.promises.syncSubscription(
         this.recurlySubscription,
-        this.adminUser._id,
-        err => {
-          if (err != null) {
-            return done(err)
-          }
-          this.SubscriptionLocator.getUsersSubscription
-            .calledWith(this.adminUser._id)
-            .should.equal(true)
-          this.UserFeaturesUpdater.updateFeatures.called.should.equal(false)
-          done()
-        }
+        this.adminUser._id
+      )
+      this.SubscriptionLocator.promises.getUsersSubscription
+        .calledWith(this.adminUser._id)
+        .should.equal(true)
+      this.UserFeaturesUpdater.promises.updateFeatures.called.should.equal(
+        false
       )
     })
   })
 
   describe('updateSubscriptionFromRecurly', function () {
-    beforeEach(function () {
-      this.FeaturesUpdater.refreshFeatures = sinon.stub().callsArgWith(2)
-    })
-
     afterEach(function () {
       this.subscription.member_ids = []
     })
 
-    it('should update the subscription with token etc when not expired', function (done) {
-      this.SubscriptionUpdater.updateSubscriptionFromRecurly(
+    it('should update the subscription with token etc when not expired', async function () {
+      await this.SubscriptionUpdater.promises.updateSubscriptionFromRecurly(
         this.recurlySubscription,
         this.subscription,
-        {},
-        err => {
-          if (err != null) {
-            return done(err)
-          }
-          this.subscription.recurlySubscription_id.should.equal(
-            this.recurlySubscription.uuid
-          )
-          this.subscription.planCode.should.equal(
-            this.recurlySubscription.plan.plan_code
-          )
-          this.subscription.save.called.should.equal(true)
-          this.FeaturesUpdater.refreshFeatures
-            .calledWith(this.adminUser._id)
-            .should.equal(true)
-          done()
-        }
+        {}
+      )
+      this.subscription.recurlySubscription_id.should.equal(
+        this.recurlySubscription.uuid
+      )
+      this.subscription.planCode.should.equal(
+        this.recurlySubscription.plan.plan_code
       )
+      this.subscription.save.called.should.equal(true)
+      this.FeaturesUpdater.promises.refreshFeatures
+        .calledWith(this.adminUser._id)
+        .should.equal(true)
     })
 
-    it('should remove the subscription when expired', function (done) {
+    it('should remove the subscription when expired', async function () {
       this.recurlySubscription.state = 'expired'
-      this.SubscriptionUpdater.updateSubscriptionFromRecurly(
+      await this.SubscriptionUpdater.promises.updateSubscriptionFromRecurly(
         this.recurlySubscription,
         this.subscription,
-        {},
-        err => {
-          if (err != null) {
-            return done(err)
-          }
-          done()
-        }
+        {}
       )
     })
 
-    it('should update all the users features', function (done) {
+    it('should update all the users features', async function () {
       this.subscription.member_ids = this.allUserIds
-      this.SubscriptionUpdater.updateSubscriptionFromRecurly(
+      await this.SubscriptionUpdater.promises.updateSubscriptionFromRecurly(
         this.recurlySubscription,
         this.subscription,
-        {},
-        err => {
-          if (err != null) {
-            return done(err)
-          }
-          this.FeaturesUpdater.refreshFeatures
-            .calledWith(this.adminUser._id)
-            .should.equal(true)
-          this.FeaturesUpdater.refreshFeatures
-            .calledWith(this.allUserIds[0])
-            .should.equal(true)
-          this.FeaturesUpdater.refreshFeatures
-            .calledWith(this.allUserIds[1])
-            .should.equal(true)
-          this.FeaturesUpdater.refreshFeatures
-            .calledWith(this.allUserIds[2])
-            .should.equal(true)
-          done()
-        }
+        {}
       )
+      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)
     })
 
-    it('should set group to true and save how many members can be added to group', function (done) {
-      this.PlansLocator.findLocalPlanInSettings
-        .withArgs(this.recurlySubscription.plan.plan_code)
-        .returns({ groupPlan: true, membersLimit: 5 })
-      this.SubscriptionUpdater.updateSubscriptionFromRecurly(
+    it('should set group to true and save how many members can be added to group', async function () {
+      this.recurlyPlan.groupPlan = true
+      this.recurlyPlan.membersLimit = 5
+      await this.SubscriptionUpdater.promises.updateSubscriptionFromRecurly(
         this.recurlySubscription,
         this.subscription,
-        {},
-        err => {
-          if (err != null) {
-            return done(err)
-          }
-          this.subscription.membersLimit.should.equal(5)
-          this.subscription.groupPlan.should.equal(true)
-          this.subscription.member_ids.should.deep.equal([
-            this.subscription.admin_id,
-          ])
-          done()
-        }
+        {}
       )
+      this.subscription.membersLimit.should.equal(5)
+      this.subscription.groupPlan.should.equal(true)
+      this.subscription.member_ids.should.deep.equal([
+        this.subscription.admin_id,
+      ])
     })
 
-    it('should delete and replace subscription when downgrading from group to individual plan', function (done) {
-      this.PlansLocator.findLocalPlanInSettings
-        .withArgs(this.recurlySubscription.plan.plan_code)
-        .returns({ groupPlan: false })
-      this.SubscriptionUpdater._deleteAndReplaceSubscriptionFromRecurly = sinon
-        .stub()
-        .yields()
-      this.SubscriptionUpdater.updateSubscriptionFromRecurly(
+    it('should delete and replace subscription when downgrading from group to individual plan', async function () {
+      this.recurlyPlan.groupPlan = false
+      await this.SubscriptionUpdater.promises.updateSubscriptionFromRecurly(
         this.recurlySubscription,
         this.groupSubscription,
-        {},
-        err => {
-          if (err != null) {
-            return done(err)
-          }
-          done()
-        }
+        {}
       )
     })
 
-    it('should not set group to true or set groupPlan', function (done) {
-      this.SubscriptionUpdater.updateSubscriptionFromRecurly(
+    it('should not set group to true or set groupPlan', async function () {
+      await this.SubscriptionUpdater.promises.updateSubscriptionFromRecurly(
         this.recurlySubscription,
         this.subscription,
-        {},
-        err => {
-          if (err != null) {
-            return done(err)
-          }
-          assert.notEqual(this.subscription.membersLimit, 5)
-          assert.notEqual(this.subscription.groupPlan, true)
-          done()
-        }
+        {}
       )
+      assert.notEqual(this.subscription.membersLimit, 5)
+      assert.notEqual(this.subscription.groupPlan, true)
     })
 
     describe('when the plan allows adding more seats', function () {
       beforeEach(function () {
         this.membersLimitAddOn = 'add_on1'
-        this.PlansLocator.findLocalPlanInSettings
-          .withArgs(this.recurlySubscription.plan.plan_code)
-          .returns({
-            groupPlan: true,
-            membersLimit: 5,
-            membersLimitAddOn: this.membersLimitAddOn,
-          })
+        this.recurlyPlan.groupPlan = true
+        this.recurlyPlan.membersLimit = 5
+        this.recurlyPlan.membersLimitAddOn = this.membersLimitAddOn
       })
 
       function expectMembersLimit(limit) {
-        it('should set the membersLimit accordingly', function (done) {
-          this.SubscriptionUpdater.updateSubscriptionFromRecurly(
+        it('should set the membersLimit accordingly', async function () {
+          await this.SubscriptionUpdater.promises.updateSubscriptionFromRecurly(
             this.recurlySubscription,
             this.subscription,
-            {},
-            error => {
-              if (error) return done(error)
-
-              expect(this.subscription.membersLimit).to.equal(limit)
-              done()
-            }
+            {}
           )
+          expect(this.subscription.membersLimit).to.equal(limit)
         })
       }
 
@@ -431,117 +376,34 @@ describe('SubscriptionUpdater', function () {
   })
 
   describe('addUserToGroup', function () {
-    beforeEach(function () {
-      this.FeaturesUpdater.refreshFeatures = sinon.stub().yields(null, {})
-      this.FeaturesUpdater.isFeatureSetBetter = sinon.stub()
-      this.FeaturesUpdater.isFeatureSetBetter
-        .withArgs(
-          {
-            collaborators: 10,
-            compileTimeout: 60,
-            dropbox: true,
-          },
-          {}
-        )
-        .returns(true)
-      this.FeaturesUpdater.isFeatureSetBetter
-        .withArgs(
-          {
-            collaborators: -1,
-            compileTimeout: 240,
-            dropbox: true,
-          },
-          {
-            collaborators: 10,
-            compileTimeout: 60,
-            dropbox: true,
-          }
-        )
-        .returns(true)
-      this.FeaturesUpdater.isFeatureSetBetter
-        .withArgs(
-          {
-            collaborators: -1,
-            compileTimeout: 240,
-            dropbox: true,
-          },
-          {}
-        )
-        .returns(true)
-      this.PlansLocator.findLocalPlanInSettings = sinon.stub()
-      this.PlansLocator.findLocalPlanInSettings
-        .withArgs(this.groupSubscription.planCode)
-        .returns({
-          planCode: this.groupSubscription.planCode,
-          features: {
-            collaborators: 10,
-            compileTimeout: 60,
-            dropbox: true,
-          },
-        })
-      this.PlansLocator.findLocalPlanInSettings
-        .withArgs(this.betterGroupSubscription.planCode)
-        .returns({
-          planCode: this.betterGroupSubscription.planCode,
-          features: {
-            collaborators: -1,
-            compileTimeout: 240,
-            dropbox: true,
-          },
-        })
-    })
-
-    it('should add the user ids to the group as a set', function (done) {
-      this.SubscriptionUpdater.addUserToGroup(
-        this.subscription._id,
-        this.otherUserId,
-        () => {
-          const searchOps = { _id: this.subscription._id }
-          const insertOperation = {
-            $addToSet: { member_ids: this.otherUserId },
-          }
-          this.updateStub
-            .calledWith(searchOps, insertOperation)
-            .should.equal(true)
-          done()
-        }
-      )
-    })
-
-    it('should update the users features', function (done) {
-      this.SubscriptionUpdater.addUserToGroup(
+    it('should add the user ids to the group as a set', async function () {
+      await this.SubscriptionUpdater.promises.addUserToGroup(
         this.subscription._id,
-        this.otherUserId,
-        () => {
-          this.FeaturesUpdater.refreshFeatures
-            .calledWith(this.otherUserId)
-            .should.equal(true)
-          done()
-        }
+        this.otherUserId
       )
+      const searchOps = { _id: this.subscription._id }
+      const insertOperation = {
+        $addToSet: { member_ids: this.otherUserId },
+      }
+      this.SubscriptionModel.updateOne
+        .calledWith(searchOps, insertOperation)
+        .should.equal(true)
     })
 
-    it('should set the group plan code user property', function (done) {
-      this.SubscriptionUpdater.addUserToGroup(
+    it('should update the users features', async function () {
+      await this.SubscriptionUpdater.promises.addUserToGroup(
         this.subscription._id,
-        this.otherUserId,
-        () => {
-          sinon.assert.calledWith(
-            this.AnalyticsManager.setUserPropertyForUser,
-            this.otherUserId,
-            'group-subscription-plan-code',
-            'group_subscription'
-          )
-          done()
-        }
+        this.otherUserId
       )
+      this.FeaturesUpdater.promises.refreshFeatures
+        .calledWith(this.otherUserId)
+        .should.equal(true)
     })
 
     it('should set the group plan code user property to the best plan with 1 group subscription', async function () {
-      this.SubscriptionLocator.promises.getMemberSubscriptions = sinon
-        .stub()
+      this.SubscriptionLocator.promises.getMemberSubscriptions
         .withArgs(this.otherUserId)
-        .returns([this.groupSubscription])
+        .resolves([this.groupSubscription])
       await this.SubscriptionUpdater.promises.addUserToGroup(
         this.groupSubscription._id,
         this.otherUserId
@@ -555,10 +417,9 @@ describe('SubscriptionUpdater', function () {
     })
 
     it('should set the group plan code user property to the best plan with 2 group subscriptions', async function () {
-      this.SubscriptionLocator.promises.getMemberSubscriptions = sinon
-        .stub()
+      this.SubscriptionLocator.promises.getMemberSubscriptions
         .withArgs(this.otherUserId)
-        .returns([this.groupSubscription, this.betterGroupSubscription])
+        .resolves([this.groupSubscription, this.betterGroupSubscription])
       await this.SubscriptionUpdater.promises.addUserToGroup(
         this.betterGroupSubscription._id,
         this.otherUserId
@@ -572,10 +433,9 @@ describe('SubscriptionUpdater', function () {
     })
 
     it('should set the group plan code user property to the best plan with 2 group subscriptions in reverse order', async function () {
-      this.SubscriptionLocator.promises.getMemberSubscriptions = sinon
-        .stub()
+      this.SubscriptionLocator.promises.getMemberSubscriptions
         .withArgs(this.otherUserId)
-        .returns([this.betterGroupSubscription, this.groupSubscription])
+        .resolves([this.betterGroupSubscription, this.groupSubscription])
       await this.SubscriptionUpdater.promises.addUserToGroup(
         this.betterGroupSubscription._id,
         this.otherUserId
@@ -591,95 +451,84 @@ describe('SubscriptionUpdater', function () {
 
   describe('removeUserFromGroups', function () {
     beforeEach(function () {
-      this.FeaturesUpdater.refreshFeatures = sinon.stub().yields(null, {})
-      this.FeaturesUpdater.isFeatureSetBetter = sinon.stub().returns(true)
-      this.UserGetter.getUser.yields(null, {})
       this.fakeSubscriptions = [{ _id: 'fake-id-1' }, { _id: 'fake-id-2' }]
-      this.SubscriptionLocator.getMemberSubscriptions.yields(
-        null,
+      this.SubscriptionLocator.promises.getMemberSubscriptions.resolves(
         this.fakeSubscriptions
       )
-      this.SubscriptionLocator.promises.getMemberSubscriptions.returns([])
     })
 
-    it('should pull the users id from the group', function (done) {
-      this.SubscriptionUpdater.removeUserFromGroup(
+    it('should pull the users id from the group', async function () {
+      await this.SubscriptionUpdater.promises.removeUserFromGroup(
         this.subscription._id,
-        this.otherUserId,
-        () => {
-          const removeOperation = { $pull: { member_ids: this.otherUserId } }
-          this.updateStub
-            .calledWith({ _id: this.subscription._id }, removeOperation)
-            .should.equal(true)
-          done()
-        }
+        this.otherUserId
       )
+      const removeOperation = { $pull: { member_ids: this.otherUserId } }
+      this.SubscriptionModel.updateOne
+        .calledWith({ _id: this.subscription._id }, removeOperation)
+        .should.equal(true)
     })
 
-    it('should set the group plan code user property when removing user from group', function (done) {
-      this.SubscriptionUpdater.removeUserFromGroup(
+    it('should set the group plan code user property when removing user from group', async function () {
+      await this.SubscriptionUpdater.promises.removeUserFromGroup(
         this.subscription._id,
+        this.otherUserId
+      )
+      sinon.assert.calledWith(
+        this.AnalyticsManager.setUserPropertyForUser,
         this.otherUserId,
-        () => {
-          sinon.assert.calledWith(
-            this.AnalyticsManager.setUserPropertyForUser,
-            this.otherUserId,
-            'group-subscription-plan-code',
-            null
-          )
-          done()
-        }
+        'group-subscription-plan-code',
+        null
       )
     })
 
-    it('should set the group plan code user property when removing user from all groups', function (done) {
-      this.SubscriptionUpdater.removeUserFromAllGroups(this.otherUserId, () => {
-        sinon.assert.calledWith(
-          this.AnalyticsManager.setUserPropertyForUser,
-          this.otherUserId,
-          'group-subscription-plan-code',
-          null
-        )
-        done()
-      })
+    it('should set the group plan code user property when removing user from all groups', async function () {
+      await this.SubscriptionUpdater.promises.removeUserFromAllGroups(
+        this.otherUserId
+      )
+      sinon.assert.calledWith(
+        this.AnalyticsManager.setUserPropertyForUser,
+        this.otherUserId,
+        'group-subscription-plan-code',
+        null
+      )
     })
 
-    it('should pull the users id from all groups', function (done) {
-      this.SubscriptionUpdater.removeUserFromAllGroups(this.otherUserId, () => {
-        const filter = { _id: ['fake-id-1', 'fake-id-2'] }
-        const removeOperation = { $pull: { member_ids: this.otherUserId } }
-        sinon.assert.calledWith(this.updateManyStub, filter, removeOperation)
-        done()
-      })
+    it('should pull the users id from all groups', async function () {
+      await this.SubscriptionUpdater.promises.removeUserFromAllGroups(
+        this.otherUserId
+      )
+      const filter = { _id: ['fake-id-1', 'fake-id-2'] }
+      const removeOperation = { $pull: { member_ids: this.otherUserId } }
+      sinon.assert.calledWith(
+        this.SubscriptionModel.updateMany,
+        filter,
+        removeOperation
+      )
     })
 
-    it('should update the users features', function (done) {
-      this.SubscriptionUpdater.removeUserFromGroup(
+    it('should update the users features', async function () {
+      await this.SubscriptionUpdater.promises.removeUserFromGroup(
         this.subscription._id,
-        this.otherUserId,
-        () => {
-          this.FeaturesUpdater.refreshFeatures
-            .calledWith(this.otherUserId)
-            .should.equal(true)
-          done()
-        }
+        this.otherUserId
       )
+      this.FeaturesUpdater.promises.refreshFeatures
+        .calledWith(this.otherUserId)
+        .should.equal(true)
     })
   })
 
   describe('deleteSubscription', function () {
-    beforeEach(function (done) {
+    beforeEach(async function () {
       this.subscription = {
         _id: ObjectId().toString(),
         mock: 'subscription',
         admin_id: ObjectId(),
         member_ids: [ObjectId(), ObjectId(), ObjectId()],
       }
-      this.SubscriptionLocator.getSubscription = sinon
-        .stub()
-        .yields(null, this.subscription)
-      this.FeaturesUpdater.refreshFeatures = sinon.stub().yields()
-      this.SubscriptionUpdater.deleteSubscription(this.subscription, {}, done)
+      await this.SubscriptionUpdater.promises.deleteSubscription(
+        this.subscription,
+        {}
+      )
     })
 
     it('should remove the subscription', function () {
@@ -689,14 +538,14 @@ describe('SubscriptionUpdater', function () {
     })
 
     it('should downgrade the admin_id', function () {
-      this.FeaturesUpdater.refreshFeatures
+      this.FeaturesUpdater.promises.refreshFeatures
         .calledWith(this.subscription.admin_id)
         .should.equal(true)
     })
 
     it('should downgrade all of the members', function () {
       for (const userId of this.subscription.member_ids) {
-        this.FeaturesUpdater.refreshFeatures
+        this.FeaturesUpdater.promises.refreshFeatures
           .calledWith(userId)
           .should.equal(true)
       }