Merge pull request #4219 from overleaf/ab-group-subscription-user-property

Send group subscription user property

GitOrigin-RevId: d1033ab4e0b5061b9d4a781e319d6b690f7e6154

services/web/.gitignore

@@ -74,3 +74,4 @@ modules/**/Makefile
 
 # Intellij
 .idea
+.run

services/web/app/src/Features/Analytics/AnalyticsManager.js

@@ -76,6 +76,13 @@ function setUserProperty(userId, propertyName, propertyValue) {
   if (isAnalyticsDisabled() || isSmokeTestUser(userId)) {
     return
   }
+
+  if (propertyValue === undefined) {
+    throw new Error(
+      'propertyValue cannot be undefined, use null to unset a property'
+    )
+  }
+
   Metrics.analyticsQueue.inc({
     status: 'adding',
     event_type: 'user-property',

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

@@ -1,7 +1,7 @@
 const async = require('async')
 const OError = require('@overleaf/o-error')
 const PlansLocator = require('./PlansLocator')
-const _ = require('underscore')
+const _ = require('lodash')
 const SubscriptionLocator = require('./SubscriptionLocator')
 const UserFeaturesUpdater = require('./UserFeaturesUpdater')
 const Settings = require('settings-sharelatex')
@@ -186,7 +186,7 @@ const FeaturesUpdater = {
           err,
           FeaturesUpdater._mergeFeatures(
             V1SubscriptionManager.getGrandfatheredFeaturesForV1User(v1Id) || {},
-            FeaturesUpdater._planCodeToFeatures(planCode)
+            FeaturesUpdater.planCodeToFeatures(planCode)
           )
         )
       }
@@ -228,13 +228,18 @@ const FeaturesUpdater = {
     return features
   },
 
+  isFeatureSetBetter(featuresA, featuresB) {
+    const mergedFeatures = FeaturesUpdater._mergeFeatures(featuresA, featuresB)
+    return _.isEqual(featuresA, mergedFeatures)
+  },
+
   _subscriptionToFeatures(subscription) {
-    return FeaturesUpdater._planCodeToFeatures(
+    return FeaturesUpdater.planCodeToFeatures(
       subscription ? subscription.planCode : undefined
     )
   },
 
-  _planCodeToFeatures(planCode) {
+  planCodeToFeatures(planCode) {
     if (!planCode) {
       return {}
     }

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

@@ -1,122 +1,112 @@
 const { db, ObjectId } = require('../../infrastructure/mongodb')
 const OError = require('@overleaf/o-error')
 const async = require('async')
-const { promisifyAll } = require('../../util/promises')
+const { promisify, 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 AnalyticsManager = require('../Analytics/AnalyticsManager')
 const { DeletedSubscription } = require('../../models/DeletedSubscription')
+const logger = require('logger-sharelatex')
 
-const SubscriptionUpdater = {
-  /**
-   * Change the admin of the given subscription.
-   *
-   * If the subscription is a group, add the new admin as manager while keeping
-   * the old admin. Otherwise, replace the manager.
-   *
-   * Validation checks are assumed to have been made:
-   *   * subscription exists
-   *   * user exists
-   *   * user does not have another subscription
-   *   * subscription is not a Recurly subscription
-   *
-   * If the subscription is Recurly, we silently do nothing.
-   */
-  updateAdmin(subscription, adminId, callback) {
-    const query = {
-      _id: ObjectId(subscription._id),
-      customAccount: true,
-    }
-    const update = {
-      $set: { admin_id: ObjectId(adminId) },
-    }
-    if (subscription.groupPlan) {
-      update.$addToSet = { manager_ids: ObjectId(adminId) }
-    } else {
-      update.$set.manager_ids = [ObjectId(adminId)]
-    }
-    Subscription.updateOne(query, update, callback)
-  },
+/**
+ * Change the admin of the given subscription.
+ *
+ * If the subscription is a group, add the new admin as manager while keeping
+ * the old admin. Otherwise, replace the manager.
+ *
+ * Validation checks are assumed to have been made:
+ *   * subscription exists
+ *   * user exists
+ *   * user does not have another subscription
+ *   * subscription is not a Recurly subscription
+ *
+ * If the subscription is Recurly, we silently do nothing.
+ */
+function updateAdmin(subscription, adminId, callback) {
+  const query = {
+    _id: ObjectId(subscription._id),
+    customAccount: true,
+  }
+  const update = {
+    $set: { admin_id: ObjectId(adminId) },
+  }
+  if (subscription.groupPlan) {
+    update.$addToSet = { manager_ids: ObjectId(adminId) }
+  } else {
+    update.$set.manager_ids = [ObjectId(adminId)]
+  }
+  Subscription.updateOne(query, update, callback)
+}
 
-  syncSubscription(recurlySubscription, adminUserId, requesterData, callback) {
-    if (!callback) {
-      callback = requesterData
-      requesterData = {}
-    }
-    SubscriptionLocator.getUsersSubscription(
-      adminUserId,
-      function (err, subscription) {
-        if (err != null) {
-          return callback(err)
-        }
-        if (subscription != null) {
-          SubscriptionUpdater._updateSubscriptionFromRecurly(
+function syncSubscription(
+  recurlySubscription,
+  adminUserId,
+  requesterData,
+  callback
+) {
+  if (!callback) {
+    callback = requesterData
+    requesterData = {}
+  }
+  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
           )
-        } else {
-          SubscriptionUpdater._createNewSubscription(
-            adminUserId,
-            function (err, subscription) {
-              if (err != null) {
-                return callback(err)
-              }
-              SubscriptionUpdater._updateSubscriptionFromRecurly(
-                recurlySubscription,
-                subscription,
-                requesterData,
-                callback
-              )
-            }
-          )
-        }
+        })
       }
-    )
-  },
-
-  addUserToGroup(subscriptionId, userId, callback) {
-    SubscriptionUpdater.addUsersToGroup(subscriptionId, [userId], callback)
-  },
+    }
+  )
+}
 
-  addUsersToGroup(subscriptionId, memberIds, callback) {
-    SubscriptionUpdater.addUsersToGroupWithoutFeaturesRefresh(
-      subscriptionId,
-      memberIds,
-      function (err) {
-        if (err != null) {
-          return callback(err)
-        }
-        async.map(
-          memberIds,
-          function (userId, cb) {
-            FeaturesUpdater.refreshFeatures(userId, 'add-to-group', cb)
-          },
-          callback
-        )
+function addUserToGroup(subscriptionId, userId, callback) {
+  Subscription.updateOne(
+    { _id: subscriptionId },
+    { $addToSet: { member_ids: userId } },
+    function (err) {
+      if (err != null) {
+        return callback(err)
       }
-    )
-  },
-
-  addUsersToGroupWithoutFeaturesRefresh(subscriptionId, memberIds, callback) {
-    const searchOps = { _id: subscriptionId }
-    const insertOperation = { $addToSet: { member_ids: { $each: memberIds } } }
-
-    Subscription.updateOne(searchOps, insertOperation, callback)
-  },
+      FeaturesUpdater.refreshFeatures(userId, 'add-to-group', function () {
+        callbackify(_sendUserGroupPlanCodeUserProperty)(userId, callback)
+      })
+    }
+  )
+}
 
-  removeUserFromGroups(filter, userId, callback) {
-    const removeOperation = { $pull: { member_ids: userId } }
-    Subscription.updateMany(filter, removeOperation, function (err) {
-      if (err != null) {
-        OError.tag(err, 'error removing user from groups', {
-          filter,
-          removeOperation,
+function removeUserFromGroup(subscriptionId, userId, callback) {
+  Subscription.updateOne(
+    { _id: subscriptionId },
+    { $pull: { member_ids: userId } },
+    function (error) {
+      if (error) {
+        OError.tag(error, 'error removing user from group', {
+          subscriptionId,
+          userId,
         })
-        return callback(err)
+        return callback(error)
       }
       UserGetter.getUser(userId, function (error, user) {
         if (error) {
@@ -124,230 +114,288 @@ const SubscriptionUpdater = {
         }
         FeaturesUpdater.refreshFeatures(
           userId,
-          'remove-user-from-groups',
-          callback
+          'remove-user-from-group',
+          function () {
+            callbackify(_sendUserGroupPlanCodeUserProperty)(userId, callback)
+          }
         )
       })
-    })
-  },
-
-  removeUserFromGroup(subscriptionId, userId, callback) {
-    SubscriptionUpdater.removeUserFromGroups(
-      { _id: subscriptionId },
-      userId,
-      callback
-    )
-  },
-
-  removeUserFromAllGroups(userId, callback) {
-    SubscriptionLocator.getMemberSubscriptions(
-      userId,
-      function (error, subscriptions) {
-        if (error) {
-          return callback(error)
-        }
-        if (!subscriptions) {
-          return callback()
-        }
-        const subscriptionIds = subscriptions.map(sub => sub._id)
-        SubscriptionUpdater.removeUserFromGroups(
-          { _id: subscriptionIds },
-          userId,
-          callback
-        )
-      }
-    )
-  },
-
-  deleteWithV1Id(v1TeamId, callback) {
-    Subscription.deleteOne({ 'overleaf.id': v1TeamId }, callback)
-  },
-
-  deleteSubscription(subscription, deleterData, callback) {
-    if (callback == null) {
-      callback = function () {}
     }
-    async.series(
-      [
-        cb =>
-          // 1. create deletedSubscription
-          SubscriptionUpdater._createDeletedSubscription(
-            subscription,
-            deleterData,
-            cb
-          ),
-        cb =>
-          // 2. remove subscription
-          Subscription.deleteOne({ _id: subscription._id }, cb),
-        cb =>
-          // 3. refresh users features
-          SubscriptionUpdater._refreshUsersFeatures(subscription, cb),
-      ],
-      callback
-    )
-  },
+  )
+}
 
-  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
-              SubscriptionUpdater._refreshUsersFeatures(subscription, cb),
-            cb =>
-              // 3. remove deleted subscription
-              DeletedSubscription.deleteOne(
-                { 'subscription._id': subscription._id },
-                callback
-              ),
-          ],
-          callback
-        )
+function removeUserFromAllGroups(userId, callback) {
+  SubscriptionLocator.getMemberSubscriptions(
+    userId,
+    function (error, subscriptions) {
+      if (error) {
+        return callback(error)
       }
-    )
-  },
-
-  _refreshUsersFeatures(subscription, callback) {
-    const userIds = [subscription.admin_id].concat(
-      subscription.member_ids || []
-    )
-    async.mapSeries(
-      userIds,
-      function (userId, cb) {
-        FeaturesUpdater.refreshFeatures(userId, 'subscription-updater', cb)
-      },
-      callback
-    )
-  },
-
-  _createDeletedSubscription(subscription, deleterData, callback) {
-    subscription.teamInvites = []
-    subscription.invited_emails = []
-    const filter = { 'subscription._id': subscription._id }
-    const data = {
-      deleterData: {
-        deleterId: deleterData.id,
-        deleterIpAddress: deleterData.ip,
-      },
-      subscription: subscription,
+      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
+                )
+              }
+            )
+          })
+        }
+      )
     }
-    const options = { upsert: true, new: true, setDefaultsOnInsert: true }
-    DeletedSubscription.findOneAndUpdate(filter, data, options, callback)
-  },
+  )
+}
 
-  _createNewSubscription(adminUserId, callback) {
-    const subscription = new Subscription({
-      admin_id: adminUserId,
-      manager_ids: [adminUserId],
-    })
-    subscription.save(err => callback(err, subscription))
-  },
+function deleteWithV1Id(v1TeamId, callback) {
+  Subscription.deleteOne({ 'overleaf.id': v1TeamId }, callback)
+}
 
-  _deleteAndReplaceSubscriptionFromRecurly(
-    recurlySubscription,
-    subscription,
-    requesterData,
+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
-  ) {
-    const adminUserId = subscription.admin_id
-    SubscriptionUpdater.deleteSubscription(subscription, requesterData, err => {
+  )
+}
+
+function restoreSubscription(subscriptionId, callback) {
+  SubscriptionLocator.getDeletedSubscription(
+    subscriptionId,
+    function (err, deletedSubscription) {
       if (err) {
         return callback(err)
       }
-      SubscriptionUpdater._createNewSubscription(
-        adminUserId,
-        (err, newSubscription) => {
-          if (err) {
-            return callback(err)
-          }
-          SubscriptionUpdater._updateSubscriptionFromRecurly(
-            recurlySubscription,
-            newSubscription,
-            requesterData,
-            callback
-          )
-        }
-      )
-    })
-  },
-
-  _updateSubscriptionFromRecurly(
-    recurlySubscription,
-    subscription,
-    requesterData,
-    callback
-  ) {
-    if (recurlySubscription.state === 'expired') {
-      return SubscriptionUpdater.deleteSubscription(
-        subscription,
-        requesterData,
+      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
       )
     }
-    const updatedPlanCode = recurlySubscription.plan.plan_code
-    const plan = PlansLocator.findLocalPlanInSettings(updatedPlanCode)
+  )
+}
 
-    if (plan == null) {
-      return callback(new Error(`plan code not found: ${updatedPlanCode}`))
+function refreshUsersFeatures(subscription, callback) {
+  const userIds = [subscription.admin_id].concat(subscription.member_ids || [])
+  async.mapSeries(
+    userIds,
+    function (userId, cb) {
+      FeaturesUpdater.refreshFeatures(userId, 'subscription-updater', cb)
+    },
+    callback
+  )
+}
+
+function createDeletedSubscription(subscription, deleterData, callback) {
+  subscription.teamInvites = []
+  subscription.invited_emails = []
+  const filter = { 'subscription._id': subscription._id }
+  const data = {
+    deleterData: {
+      deleterId: deleterData.id,
+      deleterIpAddress: deleterData.ip,
+    },
+    subscription: subscription,
+  }
+  const options = { upsert: true, new: true, setDefaultsOnInsert: true }
+  DeletedSubscription.findOneAndUpdate(filter, data, options, callback)
+}
+
+function _createNewSubscription(adminUserId, callback) {
+  const subscription = new Subscription({
+    admin_id: adminUserId,
+    manager_ids: [adminUserId],
+  })
+  subscription.save(err => callback(err, subscription))
+}
+
+function _deleteAndReplaceSubscriptionFromRecurly(
+  recurlySubscription,
+  subscription,
+  requesterData,
+  callback
+) {
+  const adminUserId = subscription.admin_id
+  deleteSubscription(subscription, requesterData, err => {
+    if (err) {
+      return callback(err)
     }
-    if (!plan.groupPlan && subscription.groupPlan) {
-      // If downgrading from group to individual plan, delete group sub and create a new one
-      return SubscriptionUpdater._deleteAndReplaceSubscriptionFromRecurly(
+    _createNewSubscription(adminUserId, (err, newSubscription) => {
+      if (err) {
+        return callback(err)
+      }
+      updateSubscriptionFromRecurly(
         recurlySubscription,
-        subscription,
+        newSubscription,
         requesterData,
         callback
       )
-    }
+    })
+  })
+}
 
-    subscription.recurlySubscription_id = recurlySubscription.uuid
-    subscription.planCode = updatedPlanCode
+function updateSubscriptionFromRecurly(
+  recurlySubscription,
+  subscription,
+  requesterData,
+  callback
+) {
+  if (recurlySubscription.state === 'expired') {
+    return deleteSubscription(subscription, requesterData, callback)
+  }
+  const updatedPlanCode = recurlySubscription.plan.plan_code
+  const plan = PlansLocator.findLocalPlanInSettings(updatedPlanCode)
 
-    if (plan.groupPlan) {
-      if (!subscription.groupPlan) {
-        subscription.member_ids = subscription.member_ids || []
-        subscription.member_ids.push(subscription.admin_id)
-      }
+  if (plan == null) {
+    return callback(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(
+      recurlySubscription,
+      subscription,
+      requesterData,
+      callback
+    )
+  }
+
+  subscription.recurlySubscription_id = recurlySubscription.uuid
+  subscription.planCode = updatedPlanCode
+
+  if (plan.groupPlan) {
+    if (!subscription.groupPlan) {
+      subscription.member_ids = subscription.member_ids || []
+      subscription.member_ids.push(subscription.admin_id)
+    }
 
-      subscription.groupPlan = true
-      subscription.membersLimit = plan.membersLimit
+    subscription.groupPlan = true
+    subscription.membersLimit = plan.membersLimit
 
-      // Some plans allow adding more seats than the base plan provides.
-      // This is recorded as a subscription add on.
+    // Some plans allow adding more seats than the base plan provides.
+    // This is recorded as a subscription add on.
+    if (
+      plan.membersLimitAddOn &&
+      Array.isArray(recurlySubscription.subscription_add_ons)
+    ) {
+      recurlySubscription.subscription_add_ons.forEach(addOn => {
+        if (addOn.add_on_code === plan.membersLimitAddOn) {
+          subscription.membersLimit += addOn.quantity
+        }
+      })
+    }
+  }
+  subscription.save(function (error) {
+    if (error) {
+      return callback(error)
+    }
+    refreshUsersFeatures(subscription, callback)
+  })
+}
+
+async function _sendUserGroupPlanCodeUserProperty(userId) {
+  try {
+    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.membersLimitAddOn &&
-        Array.isArray(recurlySubscription.subscription_add_ons)
+        plan &&
+        FeaturesUpdater.isFeatureSetBetter(plan.features, bestFeatures)
       ) {
-        recurlySubscription.subscription_add_ons.forEach(addOn => {
-          if (addOn.add_on_code === plan.membersLimitAddOn) {
-            subscription.membersLimit += addOn.quantity
-          }
-        })
+        bestPlanCode = plan.planCode
+        bestFeatures = plan.features
       }
     }
-    subscription.save(function (error) {
-      if (error) {
-        return callback(error)
-      }
-      SubscriptionUpdater._refreshUsersFeatures(subscription, callback)
-    })
-  },
+    AnalyticsManager.setUserProperty(
+      userId,
+      'group-subscription-plan-code',
+      bestPlanCode
+    )
+  } catch (error) {
+    logger.error(
+      { err: error },
+      `Failed to update group-subscription-plan-code property for user ${userId}`
+    )
+  }
 }
 
-SubscriptionUpdater.promises = promisifyAll(SubscriptionUpdater)
-module.exports = SubscriptionUpdater
+module.exports = {
+  updateAdmin,
+  syncSubscription,
+  deleteSubscription,
+  createDeletedSubscription,
+  addUserToGroup,
+  refreshUsersFeatures,
+  removeUserFromGroup,
+  removeUserFromAllGroups,
+  deleteWithV1Id,
+  restoreSubscription,
+  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),
+  },
+}

services/web/app/src/util/promises.js

@@ -1,10 +1,11 @@
-const { promisify } = require('util')
+const { promisify, callbackify } = require('util')
 const pLimit = require('p-limit')
 
 module.exports = {
   promisify,
   promisifyAll,
   promisifyMultiResult,
+  callbackify,
   callbackifyMultiResult,
   expressify,
   promiseMapWithLimit,

services/web/scripts/recurly/resync_subscriptions.js

@@ -84,7 +84,7 @@ const syncSubscription = (subscription, callback) => {
         return callback()
       }
 
-      SubscriptionUpdater._updateSubscriptionFromRecurly(
+      SubscriptionUpdater.updateSubscriptionFromRecurly(
         recurlySubscription,
         subscription,
         {},

services/web/test/acceptance/src/SubscriptionDeletionTests.js

@@ -86,7 +86,7 @@ describe('Subscriptions', function () {
       const body = this.recurlySubscription.buildCallbackXml()
 
       // create fake deletedSubscription
-      SubscriptionUpdater._createDeletedSubscription(
+      SubscriptionUpdater.createDeletedSubscription(
         this.subscription,
         {},
         error => {

services/web/test/acceptance/src/helpers/Subscription.js

@@ -52,7 +52,7 @@ class Subscription {
   }
 
   refreshUsersFeatures(callback) {
-    SubscriptionUpdater._refreshUsersFeatures(this, callback)
+    SubscriptionUpdater.refreshUsersFeatures(this, callback)
   }
 
   expectDeleted(deleterData, callback) {

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

@@ -1,5 +1,5 @@
 const SandboxedModule = require('sandboxed-module')
-const { expect } = require('chai')
+const { assert, expect } = require('chai')
 const sinon = require('sinon')
 const modulePath = '../../../../app/src/Features/Subscription/FeaturesUpdater'
 const { ObjectId } = require('mongodb')
@@ -489,4 +489,68 @@ describe('FeaturesUpdater', function () {
       })
     })
   })
+
+  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/SubscriptionUpdaterTests.js

@@ -35,6 +35,15 @@ describe('SubscriptionUpdater', function () {
       groupPlan: true,
       planCode: 'group_subscription',
     }
+    this.betterGroupSubscription = {
+      _id: '999999999999999999999999',
+      admin_id: this.adminUser._id,
+      manager_ids: [this.adminUser._id],
+      member_ids: [this.otherUserId],
+      save: sinon.stub().callsArgWith(0),
+      groupPlan: true,
+      planCode: 'better_group_subscription',
+    }
 
     this.updateStub = sinon.stub().callsArgWith(2, null)
     this.updateManyStub = sinon.stub().callsArgWith(2, null)
@@ -50,6 +59,10 @@ describe('SubscriptionUpdater', function () {
         subscription.manager_ids = [opts.admin_id]
         return subscription
       }
+
+      save() {
+        return subscription
+      }
     }
     this.SubscriptionModel.deleteOne = sinon.stub().yields()
     this.SubscriptionModel.updateOne = this.updateStub
@@ -60,6 +73,9 @@ describe('SubscriptionUpdater', function () {
       getUsersSubscription: sinon.stub(),
       getGroupSubscriptionMemberOf: sinon.stub(),
       getMemberSubscriptions: sinon.stub().yields(null, []),
+      promises: {
+        getMemberSubscriptions: sinon.stub().returns([this.groupSubscription]),
+      },
     }
 
     this.Settings = {
@@ -83,10 +99,14 @@ describe('SubscriptionUpdater', function () {
     this.Modules = { hooks: { fire: sinon.stub().callsArgWith(2, null, null) } }
     this.FeaturesUpdater = {
       refreshFeatures: sinon.stub().yields(),
+      planCodeToFeatures: sinon.stub(),
     }
     this.DeletedSubscription = {
       findOneAndUpdate: sinon.stub().yields(),
     }
+    this.AnalyticsManager = {
+      setUserProperty: sinon.stub(),
+    }
     this.SubscriptionUpdater = SandboxedModule.require(modulePath, {
       requires: {
         mongodb: { ObjectId },
@@ -103,6 +123,7 @@ describe('SubscriptionUpdater', function () {
         '../../models/DeletedSubscription': {
           DeletedSubscription: this.DeletedSubscription,
         },
+        '../Analytics/AnalyticsManager': this.AnalyticsManager,
       },
     })
   })
@@ -171,14 +192,12 @@ describe('SubscriptionUpdater', function () {
         null,
         this.subscription
       )
-      this.SubscriptionUpdater._updateSubscriptionFromRecurly = sinon
+      this.SubscriptionUpdater.updateSubscriptionFromRecurly = sinon
         .stub()
         .yields()
     })
 
     it('should update the subscription if the user already is admin of one', function (done) {
-      this.SubscriptionUpdater._createNewSubscription = sinon.stub()
-
       this.SubscriptionUpdater.syncSubscription(
         this.recurlySubscription,
         this.adminUser._id,
@@ -189,12 +208,6 @@ describe('SubscriptionUpdater', function () {
           this.SubscriptionLocator.getUsersSubscription
             .calledWith(this.adminUser._id)
             .should.equal(true)
-          this.SubscriptionUpdater._updateSubscriptionFromRecurly.called.should.equal(
-            true
-          )
-          this.SubscriptionUpdater._updateSubscriptionFromRecurly
-            .calledWith(this.recurlySubscription, this.subscription)
-            .should.equal(true)
           done()
         }
       )
@@ -211,12 +224,6 @@ describe('SubscriptionUpdater', function () {
           this.SubscriptionLocator.getUsersSubscription
             .calledWith(this.adminUser._id)
             .should.equal(true)
-          this.SubscriptionUpdater._updateSubscriptionFromRecurly.called.should.equal(
-            true
-          )
-          this.SubscriptionUpdater._updateSubscriptionFromRecurly
-            .calledWith(this.recurlySubscription, this.subscription)
-            .should.equal(true)
           this.UserFeaturesUpdater.updateFeatures.called.should.equal(false)
           done()
         }
@@ -224,10 +231,9 @@ describe('SubscriptionUpdater', function () {
     })
   })
 
-  describe('_updateSubscriptionFromRecurly', function () {
+  describe('updateSubscriptionFromRecurly', function () {
     beforeEach(function () {
       this.FeaturesUpdater.refreshFeatures = sinon.stub().callsArgWith(2)
-      this.SubscriptionUpdater.deleteSubscription = sinon.stub().yields()
     })
 
     afterEach(function () {
@@ -235,7 +241,7 @@ describe('SubscriptionUpdater', function () {
     })
 
     it('should update the subscription with token etc when not expired', function (done) {
-      this.SubscriptionUpdater._updateSubscriptionFromRecurly(
+      this.SubscriptionUpdater.updateSubscriptionFromRecurly(
         this.recurlySubscription,
         this.subscription,
         {},
@@ -260,7 +266,7 @@ describe('SubscriptionUpdater', function () {
 
     it('should remove the subscription when expired', function (done) {
       this.recurlySubscription.state = 'expired'
-      this.SubscriptionUpdater._updateSubscriptionFromRecurly(
+      this.SubscriptionUpdater.updateSubscriptionFromRecurly(
         this.recurlySubscription,
         this.subscription,
         {},
@@ -268,9 +274,6 @@ describe('SubscriptionUpdater', function () {
           if (err != null) {
             return done(err)
           }
-          this.SubscriptionUpdater.deleteSubscription
-            .calledWithMatch(this.subscription)
-            .should.equal(true)
           done()
         }
       )
@@ -278,7 +281,7 @@ describe('SubscriptionUpdater', function () {
 
     it('should update all the users features', function (done) {
       this.subscription.member_ids = this.allUserIds
-      this.SubscriptionUpdater._updateSubscriptionFromRecurly(
+      this.SubscriptionUpdater.updateSubscriptionFromRecurly(
         this.recurlySubscription,
         this.subscription,
         {},
@@ -307,7 +310,7 @@ describe('SubscriptionUpdater', function () {
       this.PlansLocator.findLocalPlanInSettings
         .withArgs(this.recurlySubscription.plan.plan_code)
         .returns({ groupPlan: true, membersLimit: 5 })
-      this.SubscriptionUpdater._updateSubscriptionFromRecurly(
+      this.SubscriptionUpdater.updateSubscriptionFromRecurly(
         this.recurlySubscription,
         this.subscription,
         {},
@@ -332,7 +335,7 @@ describe('SubscriptionUpdater', function () {
       this.SubscriptionUpdater._deleteAndReplaceSubscriptionFromRecurly = sinon
         .stub()
         .yields()
-      this.SubscriptionUpdater._updateSubscriptionFromRecurly(
+      this.SubscriptionUpdater.updateSubscriptionFromRecurly(
         this.recurlySubscription,
         this.groupSubscription,
         {},
@@ -340,16 +343,13 @@ describe('SubscriptionUpdater', function () {
           if (err != null) {
             return done(err)
           }
-          this.SubscriptionUpdater._deleteAndReplaceSubscriptionFromRecurly
-            .calledWithMatch(this.recurlySubscription, this.groupSubscription)
-            .should.equal(true)
           done()
         }
       )
     })
 
     it('should not set group to true or set groupPlan', function (done) {
-      this.SubscriptionUpdater._updateSubscriptionFromRecurly(
+      this.SubscriptionUpdater.updateSubscriptionFromRecurly(
         this.recurlySubscription,
         this.subscription,
         {},
@@ -378,7 +378,7 @@ describe('SubscriptionUpdater', function () {
 
       function expectMembersLimit(limit) {
         it('should set the membersLimit accordingly', function (done) {
-          this.SubscriptionUpdater._updateSubscriptionFromRecurly(
+          this.SubscriptionUpdater.updateSubscriptionFromRecurly(
             this.recurlySubscription,
             this.subscription,
             {},
@@ -430,52 +430,75 @@ describe('SubscriptionUpdater', function () {
     })
   })
 
-  describe('_createNewSubscription', function () {
-    it('should create a new subscription then update the subscription', function (done) {
-      this.SubscriptionUpdater._createNewSubscription(
-        this.adminUser._id,
-        () => {
-          this.subscription.admin_id.should.equal(this.adminUser._id)
-          this.subscription.manager_ids.should.deep.equal([this.adminUser._id])
-          this.subscription.save.called.should.equal(true)
-          done()
-        }
-      )
-    })
-  })
-
   describe('addUserToGroup', function () {
     beforeEach(function () {
-      this.SubscriptionUpdater.addUsersToGroup = sinon.stub().yields(null)
+      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('delegates to addUsersToGroup', function (done) {
+    it('should add the user ids to the group as a set', function (done) {
       this.SubscriptionUpdater.addUserToGroup(
         this.subscription._id,
         this.otherUserId,
-        () => {
-          this.SubscriptionUpdater.addUsersToGroup
-            .calledWith(this.subscription._id, [this.otherUserId])
-            .should.equal(true)
-          done()
-        }
-      )
-    })
-  })
-
-  describe('addUsersToGroup', function () {
-    beforeEach(function () {
-      this.FeaturesUpdater.refreshFeatures = sinon.stub().callsArgWith(2)
-    })
-
-    it('should add the user ids to the group as a set', function (done) {
-      this.SubscriptionUpdater.addUsersToGroup(
-        this.subscription._id,
-        [this.otherUserId],
         () => {
           const searchOps = { _id: this.subscription._id }
           const insertOperation = {
-            $addToSet: { member_ids: { $each: [this.otherUserId] } },
+            $addToSet: { member_ids: this.otherUserId },
           }
           this.updateStub
             .calledWith(searchOps, insertOperation)
@@ -497,17 +520,86 @@ describe('SubscriptionUpdater', function () {
         }
       )
     })
+
+    it('should set the group plan code user property', function (done) {
+      this.SubscriptionUpdater.addUserToGroup(
+        this.subscription._id,
+        this.otherUserId,
+        () => {
+          sinon.assert.calledWith(
+            this.AnalyticsManager.setUserProperty,
+            this.otherUserId,
+            'group-subscription-plan-code',
+            'group_subscription'
+          )
+          done()
+        }
+      )
+    })
+
+    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()
+        .withArgs(this.otherUserId)
+        .returns([this.groupSubscription])
+      await this.SubscriptionUpdater.promises.addUserToGroup(
+        this.groupSubscription._id,
+        this.otherUserId
+      )
+      sinon.assert.calledWith(
+        this.AnalyticsManager.setUserProperty,
+        this.otherUserId,
+        'group-subscription-plan-code',
+        'group_subscription'
+      )
+    })
+
+    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()
+        .withArgs(this.otherUserId)
+        .returns([this.groupSubscription, this.betterGroupSubscription])
+      await this.SubscriptionUpdater.promises.addUserToGroup(
+        this.betterGroupSubscription._id,
+        this.otherUserId
+      )
+      sinon.assert.calledWith(
+        this.AnalyticsManager.setUserProperty,
+        this.otherUserId,
+        'group-subscription-plan-code',
+        'better_group_subscription'
+      )
+    })
+
+    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()
+        .withArgs(this.otherUserId)
+        .returns([this.betterGroupSubscription, this.groupSubscription])
+      await this.SubscriptionUpdater.promises.addUserToGroup(
+        this.betterGroupSubscription._id,
+        this.otherUserId
+      )
+      sinon.assert.calledWith(
+        this.AnalyticsManager.setUserProperty,
+        this.otherUserId,
+        'group-subscription-plan-code',
+        'better_group_subscription'
+      )
+    })
   })
 
   describe('removeUserFromGroups', function () {
     beforeEach(function () {
-      this.FeaturesUpdater.refreshFeatures = sinon.stub().callsArgWith(2)
+      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.fakeSubscriptions
       )
+      this.SubscriptionLocator.promises.getMemberSubscriptions.returns([])
     })
 
     it('should pull the users id from the group', function (done) {
@@ -515,16 +607,43 @@ describe('SubscriptionUpdater', function () {
         this.subscription._id,
         this.otherUserId,
         () => {
-          const searchOps = { _id: this.subscription._id }
           const removeOperation = { $pull: { member_ids: this.otherUserId } }
-          this.updateManyStub
-            .calledWith(searchOps, removeOperation)
+          this.updateStub
+            .calledWith({ _id: this.subscription._id }, removeOperation)
             .should.equal(true)
           done()
         }
       )
     })
 
+    it('should set the group plan code user property when removing user from group', function (done) {
+      this.SubscriptionUpdater.removeUserFromGroup(
+        this.subscription._id,
+        this.otherUserId,
+        () => {
+          sinon.assert.calledWith(
+            this.AnalyticsManager.setUserProperty,
+            this.otherUserId,
+            'group-subscription-plan-code',
+            null
+          )
+          done()
+        }
+      )
+    })
+
+    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.setUserProperty,
+          this.otherUserId,
+          'group-subscription-plan-code',
+          null
+        )
+        done()
+      })
+    })
+
     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'] }
@@ -583,40 +702,4 @@ describe('SubscriptionUpdater', function () {
       }
     })
   })
-
-  describe('_deleteAndReplaceSubscriptionFromRecurly', function () {
-    beforeEach(function (done) {
-      this.SubscriptionUpdater.deleteSubscription = sinon.stub().yields()
-      this.SubscriptionUpdater._createNewSubscription = sinon
-        .stub()
-        .yields(null, this.subscription)
-      this.SubscriptionUpdater._updateSubscriptionFromRecurly = sinon
-        .stub()
-        .yields()
-      this.SubscriptionUpdater._deleteAndReplaceSubscriptionFromRecurly(
-        this.recurlySubscription,
-        this.groupSubscription,
-        {},
-        done
-      )
-    })
-
-    it('should delete the old subscription', function () {
-      this.SubscriptionUpdater.deleteSubscription
-        .calledWithMatch(this.groupSubscription)
-        .should.equal(true)
-    })
-
-    it('should create a new subscription', function () {
-      this.SubscriptionUpdater._createNewSubscription
-        .calledWith(this.groupSubscription.admin_id)
-        .should.equal(true)
-    })
-
-    it('should update the new subscription', function () {
-      this.SubscriptionUpdater._updateSubscriptionFromRecurly
-        .calledWithMatch(this.recurlySubscription, this.subscription)
-        .should.equal(true)
-    })
-  })
 })