Merge pull request #2480 from overleaf/ta-refresh-features-script-improve

Refresh Features Script Improvements

GitOrigin-RevId: 1cd0fc3b689cf85760d9a22804bf9cab19e22409

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

@@ -1,17 +1,3 @@
-/* eslint-disable
-    camelcase,
-    handle-callback-err,
-    max-len,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const async = require('async')
 const PlansLocator = require('./PlansLocator')
 const _ = require('underscore')
@@ -24,50 +10,45 @@ const V1SubscriptionManager = require('./V1SubscriptionManager')
 const InstitutionsFeatures = require('../Institutions/InstitutionsFeatures')
 const UserGetter = require('../User/UserGetter')
 
-const oneMonthInSeconds = 60 * 60 * 24 * 30
-
 const FeaturesUpdater = {
-  refreshFeatures(user_id, callback) {
+  refreshFeatures(userId, callback = () => {}) {
-    if (callback == null) {
+    FeaturesUpdater._computeFeatures(userId, (error, features) => {
-      callback = function(error, features, featuresChanged) {}
-    }
-    FeaturesUpdater._computeFeatures(user_id, (error, features) => {
       if (error) {
         return callback(error)
       }
-      logger.log({ user_id, features }, 'updating user features')
+      logger.log({ userId, features }, 'updating user features')
-      UserFeaturesUpdater.updateFeatures(user_id, features, callback)
+      UserFeaturesUpdater.updateFeatures(userId, features, callback)
     })
   },
 
-  _computeFeatures(user_id, callback) {
+  _computeFeatures(userId, callback) {
     const jobs = {
       individualFeatures(cb) {
-        return FeaturesUpdater._getIndividualFeatures(user_id, cb)
+        FeaturesUpdater._getIndividualFeatures(userId, cb)
       },
       groupFeatureSets(cb) {
-        return FeaturesUpdater._getGroupFeatureSets(user_id, cb)
+        FeaturesUpdater._getGroupFeatureSets(userId, cb)
       },
       institutionFeatures(cb) {
-        return InstitutionsFeatures.getInstitutionsFeatures(user_id, cb)
+        InstitutionsFeatures.getInstitutionsFeatures(userId, cb)
       },
       v1Features(cb) {
-        return FeaturesUpdater._getV1Features(user_id, cb)
+        FeaturesUpdater._getV1Features(userId, cb)
       },
       bonusFeatures(cb) {
-        return ReferalFeatures.getBonusFeatures(user_id, cb)
+        ReferalFeatures.getBonusFeatures(userId, cb)
       },
       samlFeatures(cb) {
-        return FeaturesUpdater._getSamlFeatures(user_id, cb)
+        FeaturesUpdater._getSamlFeatures(userId, cb)
       },
       featuresOverrides(cb) {
-        return FeaturesUpdater._getFeaturesOverrides(user_id, cb)
+        FeaturesUpdater._getFeaturesOverrides(userId, cb)
       }
     }
-    return async.series(jobs, function(err, results) {
+    async.series(jobs, function(err, results) {
-      if (err != null) {
+      if (err) {
         logger.warn(
-          { err, user_id },
+          { err, userId },
           'error getting subscription or group for refreshFeatures'
         )
         return callback(err)
@@ -84,7 +65,7 @@ const FeaturesUpdater = {
       } = results
       logger.log(
         {
-          user_id,
+          userId,
           individualFeatures,
           groupFeatureSets,
           institutionFeatures,
@@ -112,28 +93,20 @@ const FeaturesUpdater = {
     })
   },
 
-  _getIndividualFeatures(user_id, callback) {
+  _getIndividualFeatures(userId, callback) {
-    if (callback == null) {
+    SubscriptionLocator.getUsersSubscription(userId, (err, sub) =>
-      callback = function(error, features) {}
-    }
-    return SubscriptionLocator.getUsersSubscription(user_id, (err, sub) =>
       callback(err, FeaturesUpdater._subscriptionToFeatures(sub))
     )
   },
 
-  _getGroupFeatureSets(user_id, callback) {
+  _getGroupFeatureSets(userId, callback) {
-    if (callback == null) {
+    SubscriptionLocator.getGroupSubscriptionsMemberOf(userId, (err, subs) =>
-      callback = function(error, featureSets) {}
+      callback(err, (subs || []).map(FeaturesUpdater._subscriptionToFeatures))
-    }
-    return SubscriptionLocator.getGroupSubscriptionsMemberOf(
-      user_id,
-      (err, subs) =>
-        callback(err, (subs || []).map(FeaturesUpdater._subscriptionToFeatures))
     )
   },
 
-  _getSamlFeatures(user_id, callback) {
+  _getSamlFeatures(userId, callback) {
-    UserGetter.getUser(user_id, (err, user) => {
+    UserGetter.getUser(userId, (err, user) => {
       if (err) {
         return callback(err)
       }
@@ -152,12 +125,12 @@ const FeaturesUpdater = {
           )
         }
       }
-      return callback(null, {})
+      callback(null, {})
     })
   },
 
-  _getFeaturesOverrides(user_id, callback) {
+  _getFeaturesOverrides(userId, callback) {
-    UserGetter.getUser(user_id, { featuresOverrides: 1 }, (error, user) => {
+    UserGetter.getUser(userId, { featuresOverrides: 1 }, (error, user) => {
       if (error) {
         return callback(error)
       }
@@ -182,27 +155,24 @@ const FeaturesUpdater = {
         FeaturesUpdater._mergeFeatures,
         {}
       )
-      return callback(null, features)
+      callback(null, features)
     })
   },
 
-  _getV1Features(user_id, callback) {
+  _getV1Features(userId, callback) {
-    if (callback == null) {
+    V1SubscriptionManager.getPlanCodeFromV1(userId, function(
-      callback = function(error, features) {}
-    }
-    return V1SubscriptionManager.getPlanCodeFromV1(user_id, function(
       err,
       planCode,
       v1Id
     ) {
-      if (err != null) {
+      if (err) {
-        if ((err != null ? err.name : undefined) === 'NotFoundError') {
+        if ((err ? err.name : undefined) === 'NotFoundError') {
           return callback(null, [])
         }
         return callback(err)
       }
 
-      return callback(
+      callback(
         err,
         FeaturesUpdater._mergeFeatures(
           V1SubscriptionManager.getGrandfatheredFeaturesForV1User(v1Id) || {},
@@ -216,7 +186,6 @@ const FeaturesUpdater = {
     const features = Object.assign({}, featuresA)
     for (let key in featuresB) {
       // Special merging logic for non-boolean features
-      const value = featuresB[key]
       if (key === 'compileGroup') {
         if (
           features['compileGroup'] === 'priority' ||
@@ -253,27 +222,69 @@ const FeaturesUpdater = {
 
   _subscriptionToFeatures(subscription) {
     return FeaturesUpdater._planCodeToFeatures(
-      subscription != null ? subscription.planCode : undefined
+      subscription ? subscription.planCode : undefined
     )
   },
 
   _planCodeToFeatures(planCode) {
-    if (planCode == null) {
+    if (!planCode) {
       return {}
     }
     const plan = PlansLocator.findLocalPlanInSettings(planCode)
-    if (plan == null) {
+    if (!plan) {
       return {}
     } else {
       return plan.features
     }
+  },
+
+  compareFeatures(currentFeatures, expectedFeatures) {
+    currentFeatures = _.clone(currentFeatures)
+    expectedFeatures = _.clone(expectedFeatures)
+    if (_.isEqual(currentFeatures, expectedFeatures)) {
+      return {}
+    }
+
+    let 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
   }
 }
 
-const refreshFeaturesPromise = user_id =>
+const refreshFeaturesPromise = userId =>
   new Promise(function(resolve, reject) {
     FeaturesUpdater.refreshFeatures(
-      user_id,
+      userId,
       (error, features, featuresChanged) => {
         if (error) {
           reject(error)

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

@@ -17,6 +17,7 @@ const { Subscription } = require('../../models/Subscription')
 const { DeletedSubscription } = require('../../models/DeletedSubscription')
 const logger = require('logger-sharelatex')
 const { ObjectId } = require('mongoose').Types
+require('./GroupPlansData') // make sure dynamic group plans are loaded
 
 const SubscriptionLocator = {
   getUsersSubscription(user_or_id, callback) {

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

@@ -1,43 +1,23 @@
-/* eslint-disable
-    camelcase,
-    handle-callback-err,
-    max-len,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const { User } = require('../../models/User')
 
 module.exports = {
-  updateFeatures(user_id, features, callback) {
+  updateFeatures(userId, features, callback) {
-    if (callback == null) {
+    const conditions = { _id: userId }
-      callback = function(err, features, featuresChanged) {}
-    }
-    const conditions = { _id: user_id }
     const update = {}
     for (let key in features) {
       const value = features[key]
       update[`features.${key}`] = value
     }
-    return User.update(conditions, update, (err, result) =>
+    User.update(conditions, update, (err, result) =>
-      callback(
+      callback(err, features, (result ? result.nModified : 0) === 1)
-        err,
-        features,
-        (result != null ? result.nModified : undefined) === 1
-      )
     )
   },
 
-  overrideFeatures(user_id, features, callback) {
+  overrideFeatures(userId, features, callback) {
-    const conditions = { _id: user_id }
+    const conditions = { _id: userId }
     const update = { features }
-    return User.update(conditions, update, (err, result) =>
+    User.update(conditions, update, (err, result) =>
-      callback(err, (result != null ? result.nModified : undefined) === 1)
+      callback(err, (result ? result.nModified : 0) === 1)
     )
   }
 }

services/web/scripts/refresh_features.js

@@ -5,58 +5,53 @@ const async = require('async')
 const FeaturesUpdater = require('../app/src/Features/Subscription/FeaturesUpdater')
 const UserFeaturesUpdater = require('../app/src/Features/Subscription/UserFeaturesUpdater')
 
-const getMismatchReasons = (currentFeatures, expectedFeatures) => {
+const ScriptLogger = {
-  currentFeatures = _.clone(currentFeatures)
+  checkedUsersCount: 0,
-  expectedFeatures = _.clone(expectedFeatures)
+  mismatchUsersCount: 0,
-  if (_.isEqual(currentFeatures, expectedFeatures)) {
+  allDaysSinceLastLoggedIn: [],
-    return {}
+  allMismatchReasons: {},
-  }
 
-  let mismatchReasons = {}
+  recordMismatch: (user, mismatchReasons) => {
-  Object.keys(currentFeatures)
+    const mismatchReasonsString = JSON.stringify(mismatchReasons)
-    .sort()
+    if (ScriptLogger.allMismatchReasons[mismatchReasonsString]) {
-    .forEach(key => {
+      ScriptLogger.allMismatchReasons[mismatchReasonsString].push(user._id)
-      if (expectedFeatures[key] !== currentFeatures[key]) {
+    } else {
-        mismatchReasons[key] = expectedFeatures[key]
+      ScriptLogger.allMismatchReasons[mismatchReasonsString] = [user._id]
-      }
-    })
-
-  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
+    ScriptLogger.mismatchUsersCount += 1
+
+    if (user.lastLoggedIn) {
+      let daysSinceLastLoggedIn =
+        (new Date() - user.lastLoggedIn) / 1000 / 3600 / 24
+      ScriptLogger.allDaysSinceLastLoggedIn.push(daysSinceLastLoggedIn)
     }
-    mismatchReasons.collaborators =
+  },
-      expectedFeatures.collaborators - currentFeatures.collaborators
-  }
 
-  return mismatchReasons
+  printProgress: () => {
-}
+    console.warn(
+      `Users checked: ${ScriptLogger.checkedUsersCount}. Mismatches: ${
+        ScriptLogger.mismatchUsersCount
+      }`
+    )
+  },
 
-const recordMismatch = (user, mismatchReasons) => {
+  printSummary: () => {
-  const mismatchReasonsString = JSON.stringify(mismatchReasons)
+    console.log('All Mismatch Reasons:', ScriptLogger.allMismatchReasons)
-  if (allMismatchReasons[mismatchReasonsString]) {
+    console.log('Mismatch Users Count', ScriptLogger.mismatchUsersCount)
-    allMismatchReasons[mismatchReasonsString] += 1
+    console.log(
-  } else {
+      'Average Last Logged In (Days):',
-    allMismatchReasons[mismatchReasonsString] = 1
+      _.sum(ScriptLogger.allDaysSinceLastLoggedIn) /
-  }
+        ScriptLogger.allDaysSinceLastLoggedIn.length
-
+    )
-  mismatchUsersCount += 1
+    console.log(
-
+      'Recent Logged In (Last 7 Days):',
-  if (user.lastLoggedIn) {
+      _.filter(ScriptLogger.allDaysSinceLastLoggedIn, a => a < 7).length
-    let daysSinceLastLoggedIn =
+    )
-      (new Date() - user.lastLoggedIn) / 1000 / 3600 / 24
+    console.log(
-    allDaysSinceLastLoggedIn.push(daysSinceLastLoggedIn)
+      'Recent Logged In (Last 30 Days):',
+      _.filter(ScriptLogger.allDaysSinceLastLoggedIn, a => a < 30).length
+    )
   }
 }
 
@@ -66,13 +61,16 @@ const checkAndUpdateUser = (user, callback) =>
       return callback(error)
     }
 
-    let mismatchReasons = getMismatchReasons(user.features, freshFeatures)
+    let mismatchReasons = FeaturesUpdater.compareFeatures(
+      user.features,
+      freshFeatures
+    )
     if (Object.keys(mismatchReasons).length === 0) {
       // features are matching; nothing else to do
       return callback()
     }
 
-    recordMismatch(user, mismatchReasons)
+    ScriptLogger.recordMismatch(user, mismatchReasons)
 
     if (!COMMIT) {
       // not saving features; nothing else to do
@@ -82,83 +80,64 @@ const checkAndUpdateUser = (user, callback) =>
     UserFeaturesUpdater.overrideFeatures(user._id, freshFeatures, callback)
   })
 
-const updateUsers = (users, callback) =>
+const checkAndUpdateUsers = (users, callback) =>
-  async.eachLimit(users, ASYNC_LIMIT, checkAndUpdateUser, error => {
+  async.eachLimit(users, ASYNC_LIMIT, checkAndUpdateUser, callback)
-    if (error) {
-      return callback(error)
-    }
-    checkedUsersCount += users.length
-    console.log(
-      `Users checked: ${checkedUsersCount}. Mismatches: ${mismatchUsersCount}`
-    )
-    callback()
-  })
 
-const loopForUsers = (lastUserId, callback) => {
+const loopForUsers = (skip, callback) => {
-  const query = {}
-  if (lastUserId) {
-    query['_id'] = { $gt: lastUserId }
-  }
   db.users
-    .find(query, { features: 1, lastLoggedIn: 1 })
+    .find({}, { features: 1, lastLoggedIn: 1 })
     .sort('_id')
+    .skip(skip)
     .limit(FETCH_LIMIT, (error, users) => {
       if (error) {
         return callback(error)
       }
 
       if (users.length === 0) {
-        console.log('DONE')
+        console.warn('DONE')
         return callback()
       }
 
-      updateUsers(users, error => {
+      checkAndUpdateUsers(users, error => {
         if (error) {
           return callback(error)
         }
-        const lastUserId = users[users.length - 1]._id
+        ScriptLogger.checkedUsersCount += users.length
-        loopForUsers(lastUserId, callback)
+        retryCounter = 0
+        ScriptLogger.printProgress()
+        ScriptLogger.printSummary()
+        loopForUsers(MONGO_SKIP + ScriptLogger.checkedUsersCount, callback)
       })
     })
 }
 
-const printSummary = () => {
+let retryCounter = 0
-  console.log({ allMismatchReasons })
-  console.log(
-    'Average Last Logged In (Days):',
-    _.sum(allDaysSinceLastLoggedIn) / allDaysSinceLastLoggedIn.length
-  )
-  console.log(
-    'Recent Logged In (Last 7 Days):',
-    _.filter(allDaysSinceLastLoggedIn, a => a < 7).length
-  )
-  console.log(
-    'Recent Logged In (Last 30 Days):',
-    _.filter(allDaysSinceLastLoggedIn, a => a < 30).length
-  )
-}
-
-let checkedUsersCount = 0
-let mismatchUsersCount = 0
-let allDaysSinceLastLoggedIn = []
-let allMismatchReasons = {}
 const run = () =>
-  loopForUsers(null, error => {
+  loopForUsers(MONGO_SKIP + ScriptLogger.checkedUsersCount, error => {
     if (error) {
+      if (retryCounter < 3) {
+        console.error(error)
+        retryCounter += 1
+        console.warn(`RETRYING IN 60 SECONDS. (${retryCounter}/3)`)
+        return setTimeout(run, 6000)
+      }
       throw error
     }
-    printSummary()
     process.exit()
   })
 
-let FETCH_LIMIT, ASYNC_LIMIT, COMMIT
+let FETCH_LIMIT, ASYNC_LIMIT, COMMIT, MONGO_SKIP
 const setup = () => {
   const argv = minimist(process.argv.slice(2))
   FETCH_LIMIT = argv.fetch ? argv.fetch : 100
   ASYNC_LIMIT = argv.async ? argv.async : 10
+  MONGO_SKIP = argv.skip ? argv.skip : 0
   COMMIT = argv.commit !== undefined
   if (!COMMIT) {
-    console.log('Doing dry run without --commit')
+    console.warn('Doing dry run without --commit')
+  }
+  if (MONGO_SKIP) {
+    console.warn(`Skipping first ${MONGO_SKIP} records`)
   }
 }