diff --git a/services/web/.gitignore b/services/web/.gitignore index fc0d0935d6..b86e3cce72 100644 --- a/services/web/.gitignore +++ b/services/web/.gitignore @@ -74,3 +74,4 @@ modules/**/Makefile # Intellij .idea +.run diff --git a/services/web/app/src/Features/Analytics/AnalyticsManager.js b/services/web/app/src/Features/Analytics/AnalyticsManager.js index 5a3e33051c..a0ac8f1b4f 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsManager.js +++ b/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', diff --git a/services/web/app/src/Features/Subscription/FeaturesUpdater.js b/services/web/app/src/Features/Subscription/FeaturesUpdater.js index 7952200c7a..263715836a 100644 --- a/services/web/app/src/Features/Subscription/FeaturesUpdater.js +++ b/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 {} } diff --git a/services/web/app/src/Features/Subscription/SubscriptionUpdater.js b/services/web/app/src/Features/Subscription/SubscriptionUpdater.js index 7acb236360..050dd9a0d6 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionUpdater.js +++ b/services/web/app/src/Features/Subscription/SubscriptionUpdater.js @@ -1,353 +1,401 @@ 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 - ) - } - ) - }, - - addUsersToGroupWithoutFeaturesRefresh(subscriptionId, memberIds, callback) { - const searchOps = { _id: subscriptionId } - const insertOperation = { $addToSet: { member_ids: { $each: memberIds } } } - - Subscription.updateOne(searchOps, insertOperation, 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 addUserToGroup(subscriptionId, userId, callback) { + 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) + }) + } + ) +} + +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(error) + } UserGetter.getUser(userId, function (error, user) { if (error) { return callback(error) } 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 - ) - } - ) - }, - - _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, - } - 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)) - }, - - _deleteAndReplaceSubscriptionFromRecurly( - recurlySubscription, - subscription, - requesterData, - callback - ) { - const adminUserId = subscription.admin_id - SubscriptionUpdater.deleteSubscription(subscription, requesterData, err => { - 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, - callback - ) - } - const updatedPlanCode = recurlySubscription.plan.plan_code - const plan = PlansLocator.findLocalPlanInSettings(updatedPlanCode) - - 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 SubscriptionUpdater._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 - - // 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) { +function removeUserFromAllGroups(userId, callback) { + SubscriptionLocator.getMemberSubscriptions( + userId, + function (error, subscriptions) { if (error) { return callback(error) } - SubscriptionUpdater._refreshUsersFeatures(subscription, callback) - }) - }, + 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 + ) + } + ) + }) + } + ) + } + ) } -SubscriptionUpdater.promises = promisifyAll(SubscriptionUpdater) -module.exports = SubscriptionUpdater +function deleteWithV1Id(v1TeamId, callback) { + Subscription.deleteOne({ 'overleaf.id': v1TeamId }, callback) +} + +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 + ) +} + +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 + ) + } + ) +} + +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) + } + _createNewSubscription(adminUserId, (err, newSubscription) => { + if (err) { + return callback(err) + } + updateSubscriptionFromRecurly( + recurlySubscription, + newSubscription, + requesterData, + callback + ) + }) + }) +} + +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 == 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 + + // 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 && + FeaturesUpdater.isFeatureSetBetter(plan.features, bestFeatures) + ) { + bestPlanCode = plan.planCode + bestFeatures = plan.features + } + } + 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}` + ) + } +} + +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), + }, +} diff --git a/services/web/app/src/util/promises.js b/services/web/app/src/util/promises.js index 295b22747c..c1a68784ea 100644 --- a/services/web/app/src/util/promises.js +++ b/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, diff --git a/services/web/scripts/recurly/resync_subscriptions.js b/services/web/scripts/recurly/resync_subscriptions.js index d1803adf1f..a0b081030d 100644 --- a/services/web/scripts/recurly/resync_subscriptions.js +++ b/services/web/scripts/recurly/resync_subscriptions.js @@ -84,7 +84,7 @@ const syncSubscription = (subscription, callback) => { return callback() } - SubscriptionUpdater._updateSubscriptionFromRecurly( + SubscriptionUpdater.updateSubscriptionFromRecurly( recurlySubscription, subscription, {}, diff --git a/services/web/test/acceptance/src/SubscriptionDeletionTests.js b/services/web/test/acceptance/src/SubscriptionDeletionTests.js index ecccd8b7ee..7dd7b61e4a 100644 --- a/services/web/test/acceptance/src/SubscriptionDeletionTests.js +++ b/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 => { diff --git a/services/web/test/acceptance/src/helpers/Subscription.js b/services/web/test/acceptance/src/helpers/Subscription.js index b1b7fd86d9..27793f811e 100644 --- a/services/web/test/acceptance/src/helpers/Subscription.js +++ b/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) { diff --git a/services/web/test/unit/src/Subscription/FeaturesUpdaterTests.js b/services/web/test/unit/src/Subscription/FeaturesUpdaterTests.js index deb9793bdf..41184a6249 100644 --- a/services/web/test/unit/src/Subscription/FeaturesUpdaterTests.js +++ b/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) + }) + }) }) diff --git a/services/web/test/unit/src/Subscription/SubscriptionUpdaterTests.js b/services/web/test/unit/src/Subscription/SubscriptionUpdaterTests.js index d947014bf1..9c3f2337f9 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionUpdaterTests.js +++ b/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) - }) - }) })