Update unit tests and refactor to make more testable

services/web/app/coffee/Features/Referal/ReferalAllocator.coffee

@@ -1,8 +1,8 @@
 _ = require("underscore")
 logger = require('logger-sharelatex')
 User = require('../../models/User').User
-SubscriptionLocator = require "../Subscription/SubscriptionLocator"
 Settings = require "settings-sharelatex"
+SubscriptionUpdater = require "../Subscription/SubscriptionUpdater"
 
 module.exports = ReferalAllocator =
 	allocate: (referal_id, new_user_id, referal_source, referal_medium, callback = ->)->
@@ -25,48 +25,6 @@ module.exports = ReferalAllocator =
 					if err?
 						logger.err err:err, referal_id:referal_id, new_user_id:new_user_id, "something went wrong allocating referal"
 						return callback(err)
-					ReferalAllocator.assignBonus user._id, callback
+					SubscriptionUpdater.refreshFeatures user._id, callback
 			else
 				callback()
-
-
-
-	getBonusFeatures: (user_id, callback = (error) ->) ->
-		query = _id: user_id
-		User.findOne query, (error, user) ->
-			return callback(error) if error
-			return callback(new Error("user not found #{user_id} for assignBonus")) if !user?
-			logger.log user_id: user_id, refered_user_count: user.refered_user_count, "assigning bonus"
-			if user.refered_user_count? and user.refered_user_count > 0
-				newFeatures = ReferalAllocator._calculateFeatures(user)
-				callback null, newFeatures
-			else
-				callback()
-
-	_calculateFeatures : (user)->
-		bonusLevel = ReferalAllocator._getBonusLevel(user)
-		currentFeatures = _.clone(user.features) #need to clone because we exend with underscore later
-		betterBonusFeatures = {}
-		_.each Settings.bonus_features["#{bonusLevel}"], (bonusLevel, key)->
-			currentLevel = user?.features?[key]
-			if _.isBoolean(currentLevel) and currentLevel == false
-				betterBonusFeatures[key] = bonusLevel
-
-			if _.isNumber(currentLevel)
-				if currentLevel == -1
-					return
-				bonusIsGreaterThanCurrent = currentLevel < bonusLevel
-				if bonusIsGreaterThanCurrent or bonusLevel == -1
-					betterBonusFeatures[key] = bonusLevel
-		newFeatures = _.extend(currentFeatures, betterBonusFeatures)
-		return newFeatures
-
-
-	_getBonusLevel: (user)->
-		highestBonusLevel = 0
-		_.each _.keys(Settings.bonus_features), (level)->
-			levelIsLessThanUser = level <= user.refered_user_count
-			levelIsMoreThanCurrentHighest = level >= highestBonusLevel
-			if levelIsLessThanUser and levelIsMoreThanCurrentHighest
-				highestBonusLevel = level
-		return highestBonusLevel

services/web/app/coffee/Features/Referal/ReferalFeatures.coffee

@@ -0,0 +1,44 @@
+_ = require("underscore")
+logger = require('logger-sharelatex')
+User = require('../../models/User').User
+Settings = require "settings-sharelatex"
+
+module.exports = ReferalFeatures =
+	getBonusFeatures: (user_id, callback = (error) ->) ->
+		query = _id: user_id
+		User.findOne query, (error, user) ->
+			return callback(error) if error
+			return callback(new Error("user not found #{user_id} for assignBonus")) if !user?
+			logger.log user_id: user_id, refered_user_count: user.refered_user_count, "assigning bonus"
+			if user.refered_user_count? and user.refered_user_count > 0
+				newFeatures = ReferalFeatures._calculateFeatures(user)
+				callback null, newFeatures
+			else
+				callback null, {}
+
+	_calculateFeatures : (user)->
+		bonusLevel = ReferalFeatures._getBonusLevel(user)
+		currentFeatures = _.clone(user.features) #need to clone because we exend with underscore later
+		betterBonusFeatures = {}
+		_.each Settings.bonus_features["#{bonusLevel}"], (bonusLevel, key)->
+			currentLevel = user?.features?[key]
+			if _.isBoolean(currentLevel) and currentLevel == false
+				betterBonusFeatures[key] = bonusLevel
+
+			if _.isNumber(currentLevel)
+				if currentLevel == -1
+					return
+				bonusIsGreaterThanCurrent = currentLevel < bonusLevel
+				if bonusIsGreaterThanCurrent or bonusLevel == -1
+					betterBonusFeatures[key] = bonusLevel
+		newFeatures = _.extend(currentFeatures, betterBonusFeatures)
+		return newFeatures
+
+	_getBonusLevel: (user)->
+		highestBonusLevel = 0
+		_.each _.keys(Settings.bonus_features), (level)->
+			levelIsLessThanUser = level <= user.refered_user_count
+			levelIsMoreThanCurrentHighest = level >= highestBonusLevel
+			if levelIsLessThanUser and levelIsMoreThanCurrentHighest
+				highestBonusLevel = level
+		return highestBonusLevel

services/web/app/coffee/Features/Subscription/SubscriptionController.coffee

@@ -239,8 +239,8 @@ module.exports = SubscriptionController =
 				req.body = body
 				next()
 
-	refreshUserSubscription: (req, res, next) ->
+	refreshUserFeatures: (req, res, next) ->
 		{user_id} = req.params
-		SubscriptionUpdater.refreshSubscription user_id, (error) ->
+		SubscriptionUpdater.refreshFeatures user_id, (error) ->
 			return next(error) if error?
 			res.sendStatus 200

services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee

@@ -47,5 +47,5 @@ module.exports =
 		webRouter.post "/user/subscription/upgrade-annual",  AuthenticationController.requireLogin(), SubscriptionController.processUpgradeToAnnualPlan
 
 		# Currently used in acceptance tests only, as a way to trigger the syncing logic
-		publicApiRouter.post "/user/:user_id/subscription/sync", AuthenticationController.httpAuth, SubscriptionController.refreshUserSubscription
+		publicApiRouter.post "/user/:user_id/features/sync", AuthenticationController.httpAuth, SubscriptionController.refreshUserFeatures
 

services/web/app/coffee/Features/Subscription/SubscriptionUpdater.coffee

@@ -7,7 +7,7 @@ PlansLocator = require("./PlansLocator")
 Settings = require("settings-sharelatex")
 logger = require("logger-sharelatex")
 ObjectId = require('mongoose').Types.ObjectId	
-ReferalAllocator = require("../Referal/ReferalAllocator")
+ReferalFeatures = require("../Referal/ReferalFeatures")
 V1SubscriptionManager = require("./V1SubscriptionManager")
 
 oneMonthInSeconds = 60 * 60 * 24 * 30
@@ -54,7 +54,7 @@ module.exports = SubscriptionUpdater =
 			if err?
 				logger.err err:err, searchOps:searchOps, removeOperation:removeOperation, "error removing user from group"
 				return callback(err)
-			SubscriptionUpdater._setUsersMinimumFeatures user_id, callback
+			SubscriptionUpdater.refreshFeatures user_id, callback
 
 	removeEmailInviteFromGroup: (adminUser_id, email, callback)->
 		Subscription.update {
@@ -63,9 +63,6 @@ module.exports = SubscriptionUpdater =
 			invited_emails: email
 		}, callback
 
-	refreshSubscription: (user_id, callback=(err)->) ->
-		SubscriptionUpdater._setUsersMinimumFeatures user_id, callback
-
 	deleteSubscription: (subscription_id, callback = (error) ->) ->
 		SubscriptionLocator.getSubscription subscription_id, (err, subscription) ->
 			return callback(err) if err?
@@ -73,7 +70,7 @@ module.exports = SubscriptionUpdater =
 			logger.log {subscription_id, affected_user_ids}, "deleting subscription and downgrading users"
 			Subscription.remove {_id: ObjectId(subscription_id)}, (err) ->
 				return callback(err) if err?
-				async.mapSeries affected_user_ids, SubscriptionUpdater._setUsersMinimumFeatures, callback
+				async.mapSeries affected_user_ids, SubscriptionUpdater.refreshFeatures, callback
 
 	_createNewSubscription: (adminUser_id, callback)->
 		logger.log adminUser_id:adminUser_id, "creating new subscription"
@@ -101,36 +98,41 @@ module.exports = SubscriptionUpdater =
 			allIds = _.union subscription.member_ids, [subscription.admin_id]
 			jobs = allIds.map (user_id)->
 				return (cb)->
-					SubscriptionUpdater._setUsersMinimumFeatures user_id, cb
+					SubscriptionUpdater.refreshFeatures user_id, cb
 			async.series jobs, callback
 
-	_setUsersMinimumFeatures: (user_id, callback)->
+	refreshFeatures: (user_id, callback)->
 		jobs =
-			individualFeatures: (cb)->
+			individualFeatures: (cb) -> SubscriptionUpdater._getIndividualFeatures user_id, cb
-				SubscriptionLocator.getUsersSubscription user_id, (err, sub)->
+			groupFeatureSets:   (cb) -> SubscriptionUpdater._getGroupFeatureSets user_id, cb
-					cb err, SubscriptionUpdater._subscriptionToFeatures(sub)
+			v1Features:         (cb) -> SubscriptionUpdater._getV1Features user_id, cb
-			groupFeatures: (cb) ->
+			bonusFeatures:      (cb) -> ReferalFeatures.getBonusFeatures user_id, cb
-				SubscriptionLocator.getGroupSubscriptionsMemberOf user_id, (err, subs) ->
-					cb err, (subs or []).map SubscriptionUpdater._subscriptionToFeatures
-			v1Features: (cb) ->
-				V1SubscriptionManager.getPlanCodeFromV1 user_id, (err, planCode) ->
-					cb err, SubscriptionUpdater._planCodeToFeatures(planCode)
-			bonusFeatures: (cb) ->
-				ReferalAllocator.getBonusFeatures user_id, cb
 		async.series jobs, (err, results)->
 			if err?
 				logger.err err:err, user_id:user_id,
-					"error getting subscription or group for _setUsersMinimumFeatures"
+					"error getting subscription or group for refreshFeatures"
 				return callback(err)
 
-			{individualFeatures, groupFeatures, v1Features, bonusFeatures} = results
+			{individualFeatures, groupFeatureSets, v1Features, bonusFeatures} = results
-			logger.log {user_id, individualFeatures, groupFeatures, v1Features, bonusFeatures}, 'merging user features'
+			logger.log {user_id, individualFeatures, groupFeatureSets, v1Features, bonusFeatures}, 'merging user features'
-			featureSets = groupFeatures.concat [individualFeatures, v1Features, bonusFeatures]
+			featureSets = groupFeatureSets.concat [individualFeatures, v1Features, bonusFeatures]
 			features = _.reduce(featureSets, SubscriptionUpdater._mergeFeatures, Settings.defaultFeatures)
 
 			logger.log {user_id, features}, 'updating user features'
 			UserFeaturesUpdater.updateFeatures user_id, features, callback
 
+	_getIndividualFeatures: (user_id, callback = (error, features = {}) ->) ->
+		SubscriptionLocator.getUsersSubscription user_id, (err, sub)->
+			callback err, SubscriptionUpdater._subscriptionToFeatures(sub)
+
+	_getGroupFeatureSets: (user_id, callback = (error, featureSets = []) ->) ->
+		SubscriptionLocator.getGroupSubscriptionsMemberOf user_id, (err, subs) ->
+			callback err, (subs or []).map SubscriptionUpdater._subscriptionToFeatures
+
+	_getV1Features: (user_id, callback = (error, features = {}) ->) ->
+		V1SubscriptionManager.getPlanCodeFromV1 user_id, (err, planCode) ->
+			callback err, SubscriptionUpdater._planCodeToFeatures(planCode)
+
 	_mergeFeatures: (featuresA, featuresB) ->
 		features = Object.assign({}, featuresA)
 		for key, value of featuresB

services/web/app/coffee/Features/Subscription/UserFeaturesUpdater.coffee

@@ -1,9 +1,7 @@
 logger = require("logger-sharelatex")
 User = require('../../models/User').User
-PlansLocator = require("./PlansLocator")
 
 module.exports =
-	
 	updateFeatures: (user_id, features, callback = (err, features)->)->
 		conditions = _id:user_id
 		update = {}

services/web/test/acceptance/coffee/SubscriptionTests.coffee

@@ -12,7 +12,7 @@ MockV1Api = require "./helpers/MockV1Api"
 syncUserAndGetFeatures = (user, callback = (error, features) ->) ->
 	request {
 		method: 'POST',
-		url: "/user/#{user._id}/subscription/sync",
+		url: "/user/#{user._id}/features/sync",
 		auth:
 			user: 'sharelatex'
 			pass: 'password'

services/web/test/unit/coffee/Referal/ReferalAllocatorTests.coffee

@@ -4,12 +4,12 @@ require('chai').should()
 sinon = require('sinon')
 modulePath = require('path').join __dirname, '../../../../app/js/Features/Referal/ReferalAllocator.js'
 
-describe 'Referalallocator', ->
+describe 'ReferalAllocator', ->
 
 	beforeEach ->
 		@ReferalAllocator = SandboxedModule.require modulePath, requires:
 			'../../models/User': User: @User = {}
-			"../Subscription/SubscriptionLocator": @SubscriptionLocator = {}
+			"../Subscription/SubscriptionUpdater": @SubscriptionUpdater = {}
 			"settings-sharelatex": @Settings = {}
 			'logger-sharelatex':
 				log:->
@@ -26,7 +26,7 @@ describe 'Referalallocator', ->
 				@referal_source = "bonus"
 				@User.update = sinon.stub().callsArgWith 3, null
 				@User.findOne = sinon.stub().callsArgWith 1, null, { _id: @user_id }
-				@ReferalAllocator.assignBonus = sinon.stub().callsArg 1
+				@SubscriptionUpdater.refreshFeatures = sinon.stub().yields()
 				@ReferalAllocator.allocate @referal_id, @new_user_id, @referal_source, @referal_medium, @callback
 
 			it 'should update the referring user with the refered users id', ->
@@ -44,8 +44,8 @@ describe 'Referalallocator', ->
 					.calledWith( referal_id: @referal_id )
 					.should.equal true
 			
-			it "shoudl assign the user their bonus", ->
+			it "should refresh the user's subscription", ->
-				@ReferalAllocator.assignBonus
+				@SubscriptionUpdater.refreshFeatures
 					.calledWith(@user_id)
 					.should.equal true
 
@@ -57,7 +57,7 @@ describe 'Referalallocator', ->
 				@referal_source = "public_share"
 				@User.update = sinon.stub().callsArgWith 3, null
 				@User.findOne = sinon.stub().callsArgWith 1, null, { _id: @user_id }
-				@ReferalAllocator.assignBonus = sinon.stub().callsArg 1
+				@SubscriptionUpdater.refreshFeatures = sinon.stub().yields()
 				@ReferalAllocator.allocate @referal_id, @new_user_id, @referal_source, @referal_medium, @callback
 
 			it 'should not update the referring user with the refered users id', ->
@@ -69,118 +69,7 @@ describe 'Referalallocator', ->
 					.should.equal true
 			
 			it "should not assign the user a bonus", ->
-				@ReferalAllocator.assignBonus.called.should.equal false
+				@SubscriptionUpdater.refreshFeatures.called.should.equal false
 
 			it "should call the callback", ->
 				@callback.called.should.equal true
-
-	describe "assignBonus", ->
-			beforeEach ->
-				@refered_user_count = 3
-				@Settings.bonus_features =
-					"3":
-						collaborators: 3
-						dropbox: false
-						versioning: false
-				stubbedUser = { 
-					refered_user_count: @refered_user_count, 
-					features:{collaborators:1, dropbox:false, versioning:false} 
-				}
-
-				@User.findOne = sinon.stub().callsArgWith 1, null, stubbedUser
-				@User.update = sinon.stub().callsArgWith 2, null
-				@ReferalAllocator.assignBonus @user_id, @callback
-
-			it "should get the users number of refered user", ->
-				@User.findOne
-					.calledWith(_id: @user_id)
-					.should.equal true
-
-			it "should update the user to bonus features", ->
-				@User.update
-					.calledWith({
-						_id: @user_id
-					}, {
-						$set:
-							features:
-								@Settings.bonus_features[@refered_user_count.toString()]
-					})
-					.should.equal true
-
-			it "should call the callback", ->
-				@callback.called.should.equal true
-	
-		describe "when there is nothing to assign", ->
-
-			beforeEach ->
-				@ReferalAllocator._calculateBonuses = sinon.stub().returns({})
-				@stubbedUser =
-					refered_user_count:4
-					features:{collaborators:3, versioning:true, dropbox:false}
-				@Settings.bonus_features =
-					"4":
-						collaborators:3
-						versioning:true
-						dropbox:false
-				@User.findOne = sinon.stub().callsArgWith 1, null, @stubbedUser
-				@User.update = sinon.stub().callsArgWith 2, null
-
-			it "should not call update if there are no bonuses to apply", (done)->
-				@ReferalAllocator.assignBonus @user_id, (err)=>
-					@User.update.called.should.equal false
-					done()
-
-		describe "when the user has better features already", ->
-			
-			beforeEach ->
-				@refered_user_count = 3
-				@stubbedUser =
-					refered_user_count:4
-					features:
-						collaborators:3
-						dropbox:false
-						versioning:false
-				@Settings.bonus_features =
-					"4":
-						collaborators: 10
-						dropbox: true
-						versioning: false
-
-				@User.findOne = sinon.stub().callsArgWith 1, null, @stubbedUser
-				@User.update = sinon.stub().callsArgWith 2, null
-
-			it "should not set in in mongo when the feature is better", (done)->
-				@ReferalAllocator.assignBonus @user_id, =>
-					@User.update.calledWith({_id: @user_id }, {$set: features:{dropbox:true, versioning:false, collaborators:10} }).should.equal true
-					done()
-
-			it "should not overright if the user has -1 users", (done)->
-				@stubbedUser.features.collaborators = -1
-				@ReferalAllocator.assignBonus @user_id, =>
-					@User.update.calledWith({_id: @user_id }, {$set: features:{dropbox:true, versioning:false, collaborators:-1} }).should.equal true
-					done()
-
-		describe "when the user is not at a bonus level", ->
-			beforeEach ->
-				@refered_user_count = 0
-				@Settings.bonus_features =
-					"1":
-						collaborators: 3
-						dropbox: false
-						versioning: false
-				@User.findOne = sinon.stub().callsArgWith 1, null, { refered_user_count: @refered_user_count }
-				@User.update = sinon.stub().callsArgWith 2, null
-				@ReferalAllocator.assignBonus @user_id, @callback
-
-			it "should get the users number of refered user", ->
-				@User.findOne
-					.calledWith(_id: @user_id)
-					.should.equal true
-
-			it "should not update the user to bonus features", ->
-				@User.update.called.should.equal false
-
-			it "should call the callback", ->
-				@callback.called.should.equal true
-
-

services/web/test/unit/coffee/Referal/ReferalFeaturesTests.coffee

@@ -0,0 +1,65 @@
+SandboxedModule = require('sandboxed-module')
+assert = require('assert')
+require('chai').should()
+sinon = require('sinon')
+modulePath = require('path').join __dirname, '../../../../app/js/Features/Referal/ReferalFeatures.js'
+
+describe 'ReferalFeatures', ->
+
+	beforeEach ->
+		@ReferalFeatures = SandboxedModule.require modulePath, requires:
+			'../../models/User': User: @User = {}
+			"settings-sharelatex": @Settings = {}
+			'logger-sharelatex':
+				log:->
+				err:->
+		@callback = sinon.stub()
+		@referal_id = "referal-id-123"
+		@referal_medium = "twitter"
+		@user_id = "user-id-123"
+		@new_user_id = "new-user-id-123"
+
+	describe "getBonusFeatures", ->
+			beforeEach ->
+				@refered_user_count = 3
+				@Settings.bonus_features =
+					"3":
+						collaborators: 3
+						dropbox: false
+						versioning: false
+				stubbedUser = { 
+					refered_user_count: @refered_user_count, 
+					features:{collaborators:1, dropbox:false, versioning:false} 
+				}
+
+				@User.findOne = sinon.stub().callsArgWith 1, null, stubbedUser
+				@ReferalFeatures.getBonusFeatures @user_id, @callback
+
+			it "should get the users number of refered user", ->
+				@User.findOne
+					.calledWith(_id: @user_id)
+					.should.equal true
+
+			it "should call the callback with the features", ->
+				@callback.calledWith(null, @Settings.bonus_features[3]).should.equal true
+
+		describe "when the user is not at a bonus level", ->
+			beforeEach ->
+				@refered_user_count = 0
+				@Settings.bonus_features =
+					"1":
+						collaborators: 3
+						dropbox: false
+						versioning: false
+				@User.findOne = sinon.stub().callsArgWith 1, null, { refered_user_count: @refered_user_count }
+				@ReferalFeatures.getBonusFeatures @user_id, @callback
+
+			it "should get the users number of refered user", ->
+				@User.findOne
+					.calledWith(_id: @user_id)
+					.should.equal true
+
+			it "should call the callback with no features", ->
+				@callback.calledWith(null, {}).should.equal true
+
+

services/web/test/unit/coffee/Subscription/SubscriptionControllerTests.coffee

@@ -75,6 +75,7 @@ describe "SubscriptionController", ->
 			"./SubscriptionDomainHandler":@SubscriptionDomainHandler
 			"../User/UserGetter": @UserGetter
 			"./RecurlyWrapper": @RecurlyWrapper = {}
+			"./SubscriptionUpdater": @SubscriptionUpdater = {}
 
 
 		@res = new MockResponse()

services/web/test/unit/coffee/Subscription/SubscriptionUpdaterTests.coffee

@@ -1,5 +1,6 @@
 SandboxedModule = require('sandboxed-module')
 should = require('chai').should()
+expect = require('chai').expect
 sinon = require 'sinon'
 modulePath = "../../../../app/js/Features/Subscription/SubscriptionUpdater"
 assert = require("chai").assert
@@ -22,6 +23,7 @@ describe "SubscriptionUpdater", ->
 			save: sinon.stub().callsArgWith(0)
 			freeTrial:{}
 			planCode:"student_or_something"
+		@user_id = @adminuser_id
 
 		@groupSubscription =
 			admin_id: @adminUser._id
@@ -48,15 +50,15 @@ describe "SubscriptionUpdater", ->
 		@Settings = 
 			freeTrialPlanCode: "collaborator"
 			defaultPlanCode: "personal"
+			defaultFeatures: { "default": "features" }
 
 		@UserFeaturesUpdater =
-			updateFeatures : sinon.stub().callsArgWith(2)
+			updateFeatures : sinon.stub().yields()
 
 		@PlansLocator =
 			findLocalPlanInSettings: sinon.stub().returns({})
 
-		@ReferalAllocator = assignBonus:sinon.stub().callsArgWith(1)
+		@ReferalFeatures = getBonusFeatures: sinon.stub().callsArgWith(1)
-		@ReferalAllocator.cock = true
 		@Modules = {hooks: {fire: sinon.stub().callsArgWith(2, null, null)}}
 		@SubscriptionUpdater = SandboxedModule.require modulePath, requires:
 			'../../models/Subscription': Subscription:@SubscriptionModel
@@ -65,8 +67,9 @@ describe "SubscriptionUpdater", ->
 			'./PlansLocator': @PlansLocator
 			"logger-sharelatex": log:->
 			'settings-sharelatex': @Settings
-			"../Referal/ReferalAllocator" : @ReferalAllocator
+			"../Referal/ReferalFeatures" : @ReferalFeatures
 			'../../infrastructure/Modules': @Modules
+			"./V1SubscriptionManager": @V1SubscriptionManager = {}
 
 
 	describe "syncSubscription", ->
@@ -97,7 +100,7 @@ describe "SubscriptionUpdater", ->
 
 	describe "_updateSubscriptionFromRecurly", ->
 		beforeEach ->
-			@SubscriptionUpdater._setUsersMinimumFeatures = sinon.stub().callsArgWith(1)
+			@SubscriptionUpdater.refreshFeatures = sinon.stub().callsArgWith(1)
 			
 		it "should update the subscription with token etc when not expired", (done)->
 			@SubscriptionUpdater._updateSubscriptionFromRecurly @recurlySubscription, @subscription, (err)=>
@@ -108,7 +111,7 @@ describe "SubscriptionUpdater", ->
 				assert.equal(@subscription.freeTrial.expiresAt, undefined)
 				assert.equal(@subscription.freeTrial.planCode, undefined)
 				@subscription.save.called.should.equal true
-				@SubscriptionUpdater._setUsersMinimumFeatures.calledWith(@adminUser._id).should.equal true
+				@SubscriptionUpdater.refreshFeatures.calledWith(@adminUser._id).should.equal true
 				done()
 
 		it "should remove the recurlySubscription_id when expired", (done)->
@@ -117,15 +120,15 @@ describe "SubscriptionUpdater", ->
 			@SubscriptionUpdater._updateSubscriptionFromRecurly @recurlySubscription, @subscription, (err)=>
 				assert.equal(@subscription.recurlySubscription_id, undefined)
 				@subscription.save.called.should.equal true
-				@SubscriptionUpdater._setUsersMinimumFeatures.calledWith(@adminUser._id).should.equal true
+				@SubscriptionUpdater.refreshFeatures.calledWith(@adminUser._id).should.equal true
 				done()
 
 		it "should update all the users features", (done)->
 			@SubscriptionUpdater._updateSubscriptionFromRecurly @recurlySubscription, @subscription, (err)=>
-				@SubscriptionUpdater._setUsersMinimumFeatures.calledWith(@adminUser._id).should.equal true
+				@SubscriptionUpdater.refreshFeatures.calledWith(@adminUser._id).should.equal true
-				@SubscriptionUpdater._setUsersMinimumFeatures.calledWith(@allUserIds[0]).should.equal true
+				@SubscriptionUpdater.refreshFeatures.calledWith(@allUserIds[0]).should.equal true
-				@SubscriptionUpdater._setUsersMinimumFeatures.calledWith(@allUserIds[1]).should.equal true
+				@SubscriptionUpdater.refreshFeatures.calledWith(@allUserIds[1]).should.equal true
-				@SubscriptionUpdater._setUsersMinimumFeatures.calledWith(@allUserIds[2]).should.equal true
+				@SubscriptionUpdater.refreshFeatures.calledWith(@allUserIds[2]).should.equal true
 				done()
 
 		it "should set group to true and save how many members can be added to group", (done)->
@@ -168,7 +171,7 @@ describe "SubscriptionUpdater", ->
 
 	describe "removeUserFromGroup", ->
 		beforeEach ->
-			@SubscriptionUpdater._setUsersMinimumFeatures = sinon.stub().callsArgWith(1)
+			@SubscriptionUpdater.refreshFeatures = sinon.stub().callsArgWith(1)
 
 		it "should pull the users id from the group", (done)->
 			@SubscriptionUpdater.removeUserFromGroup @adminUser._id, @otherUserId, =>
@@ -181,70 +184,159 @@ describe "SubscriptionUpdater", ->
 
 		it "should update the users features", (done)->
 			@SubscriptionUpdater.removeUserFromGroup @adminUser._id, @otherUserId, =>
-				@SubscriptionUpdater._setUsersMinimumFeatures.calledWith(@otherUserId).should.equal true
+				@SubscriptionUpdater.refreshFeatures.calledWith(@otherUserId).should.equal true
 				done()
 
-	describe "_setUsersMinimumFeatures", ->
+	describe "refreshFeatures", ->
+		beforeEach ->
+			@SubscriptionUpdater._getIndividualFeatures = sinon.stub().yields(null, { 'individual': 'features' })
+			@SubscriptionUpdater._getGroupFeatureSets = sinon.stub().yields(null, [{ 'group': 'features' }, { 'group': 'features2' }])
+			@SubscriptionUpdater._getV1Features = sinon.stub().yields(null, { 'v1': 'features' })
+			@ReferalFeatures.getBonusFeatures = sinon.stub().yields(null, { 'bonus': 'features' })
+			@SubscriptionUpdater._mergeFeatures = sinon.stub().returns({'merged': 'features'})
+			@callback = sinon.stub()
+			@SubscriptionUpdater.refreshFeatures @user_id, @callback
 
-		it "should call updateFeatures with the subscription if set", (done)->
+		it "should get the individual features", ->
-			@SubscriptionLocator.getUsersSubscription.callsArgWith(1, null, @subscription)
+			@SubscriptionUpdater._getIndividualFeatures
-			@SubscriptionLocator.getGroupSubscriptionMemberOf.callsArgWith(1, null)
+				.calledWith(@user_id)
+				.should.equal true
 
-			@SubscriptionUpdater._setUsersMinimumFeatures @adminUser._id, (err)=>
+		it "should get the group features", ->
-				args = @UserFeaturesUpdater.updateFeatures.args[0]
+			@SubscriptionUpdater._getGroupFeatureSets
-				assert.equal args[0], @adminUser._id
+				.calledWith(@user_id)
-				assert.equal args[1], @subscription.planCode
+				.should.equal true
-				done()
 
-		it "should call updateFeatures with the  group subscription if set", (done)->
+		it "should get the v1 features", ->
-			@SubscriptionLocator.getUsersSubscription.callsArgWith(1, null)
+			@SubscriptionUpdater._getV1Features
-			@SubscriptionLocator.getGroupSubscriptionMemberOf.callsArgWith(1, null, @groupSubscription)
+				.calledWith(@user_id)
+				.should.equal true
 
-			@SubscriptionUpdater._setUsersMinimumFeatures @adminUser._id, (err)=>
+		it "should get the bonus features", ->
-				args = @UserFeaturesUpdater.updateFeatures.args[0]
+			@ReferalFeatures.getBonusFeatures
-				assert.equal args[0], @adminUser._id
+				.calledWith(@user_id)
-				assert.equal args[1], @groupSubscription.planCode
+				.should.equal true
-				done()
 
-		it "should call updateFeatures with the overleaf subscription if set", (done)->
+		it "should merge from the default features", ->
-			@SubscriptionLocator.getUsersSubscription.callsArgWith(1, null)
+			@SubscriptionUpdater._mergeFeatures.calledWith(@Settings.defaultFeatures).should.equal true
-			@SubscriptionLocator.getGroupSubscriptionMemberOf.callsArgWith(1, null, null)
-			@Modules.hooks.fire = sinon.stub().callsArgWith(2, null, ['ol_pro'])
 
-			@SubscriptionUpdater._setUsersMinimumFeatures @adminUser._id, (err)=>
+		it "should merge the individual features", ->
-				args = @UserFeaturesUpdater.updateFeatures.args[0]
+			@SubscriptionUpdater._mergeFeatures.calledWith(sinon.match.any, { 'individual': 'features' }).should.equal true
-				assert.equal args[0], @adminUser._id
-				assert.equal args[1], 'ol_pro'
-				done()
 
-		it "should call not call updateFeatures  with users subscription if the subscription plan code is the default one (downgraded)", (done)->
+		it "should merge the group features", ->
-			@subscription.planCode = @Settings.defaultPlanCode
+			@SubscriptionUpdater._mergeFeatures.calledWith(sinon.match.any, { 'group': 'features' }).should.equal true
-			@SubscriptionLocator.getUsersSubscription.callsArgWith(1, null, @subscription)
+			@SubscriptionUpdater._mergeFeatures.calledWith(sinon.match.any, { 'group': 'features2' }).should.equal true
-			@SubscriptionLocator.getGroupSubscriptionMemberOf.callsArgWith(1, null, @groupSubscription)
-			@Modules.hooks.fire = sinon.stub().callsArgWith(2, null, null)
-			@SubscriptionUpdater._setUsersMinimumFeatures @adminuser_id, (err)=>
-				args = @UserFeaturesUpdater.updateFeatures.args[0]
-				assert.equal args[0], @adminUser._id
-				assert.equal args[1], @groupSubscription.planCode
-				done()
 
+		it "should merge the v1 features", ->
+			@SubscriptionUpdater._mergeFeatures.calledWith(sinon.match.any, { 'v1': 'features' }).should.equal true
 
-		it "should call updateFeatures with default if there are no subscriptions for user", (done)->
+		it "should merge the bonus features", ->
-			@SubscriptionLocator.getUsersSubscription.callsArgWith(1, null)
+			@SubscriptionUpdater._mergeFeatures.calledWith(sinon.match.any, { 'bonus': 'features' }).should.equal true
-			@SubscriptionLocator.getGroupSubscriptionMemberOf.callsArgWith(1, null)
-			@Modules.hooks.fire = sinon.stub().callsArgWith(2, null, null)
-			@SubscriptionUpdater._setUsersMinimumFeatures @adminuser_id, (err)=>
-				args = @UserFeaturesUpdater.updateFeatures.args[0]
-				assert.equal args[0], @adminUser._id
-				assert.equal args[1], @Settings.defaultPlanCode
-				done()
 
-		it "should call assignBonus", (done)->
+		it "should update the user with the merged features", ->
-			@SubscriptionLocator.getUsersSubscription.callsArgWith(1, null)
+			@UserFeaturesUpdater.updateFeatures
-			@SubscriptionLocator.getGroupSubscriptionMemberOf.callsArgWith(1, null)
+				.calledWith(@user_id, {'merged': 'features'})
-			@SubscriptionUpdater._setUsersMinimumFeatures @adminuser_id, (err)=>
+				.should.equal true
-				@ReferalAllocator.assignBonus.calledWith(@adminuser_id).should.equal true
+
-				done()
+	describe "_mergeFeatures", ->
+		it "should prefer priority over standard for compileGroup", ->
+			expect(@SubscriptionUpdater._mergeFeatures({
+				compileGroup: 'priority'
+			}, {
+				compileGroup: 'standard'
+			})).to.deep.equal({
+				compileGroup: 'priority'
+			})
+			expect(@SubscriptionUpdater._mergeFeatures({
+				compileGroup: 'standard'
+			}, {
+				compileGroup: 'priority'
+			})).to.deep.equal({
+				compileGroup: 'priority'
+			})
+			expect(@SubscriptionUpdater._mergeFeatures({
+				compileGroup: 'priority'
+			}, {
+				compileGroup: 'priority'
+			})).to.deep.equal({
+				compileGroup: 'priority'
+			})
+			expect(@SubscriptionUpdater._mergeFeatures({
+				compileGroup: 'standard'
+			}, {
+				compileGroup: 'standard'
+			})).to.deep.equal({
+				compileGroup: 'standard'
+			})
+
+		it "should prefer -1 over any other for collaborators", ->
+			expect(@SubscriptionUpdater._mergeFeatures({
+				collaborators: -1
+			}, {
+				collaborators: 10
+			})).to.deep.equal({
+				collaborators: -1
+			})
+			expect(@SubscriptionUpdater._mergeFeatures({
+				collaborators: 10
+			}, {
+				collaborators: -1
+			})).to.deep.equal({
+				collaborators: -1
+			})
+			expect(@SubscriptionUpdater._mergeFeatures({
+				collaborators: 4
+			}, {
+				collaborators: 10
+			})).to.deep.equal({
+				collaborators: 10
+			})
+
+		it "should prefer the higher of compileTimeout", ->
+			expect(@SubscriptionUpdater._mergeFeatures({
+				compileTimeout: 20
+			}, {
+				compileTimeout: 10
+			})).to.deep.equal({
+				compileTimeout: 20
+			})
+			expect(@SubscriptionUpdater._mergeFeatures({
+				compileTimeout: 10
+			}, {
+				compileTimeout: 20
+			})).to.deep.equal({
+				compileTimeout: 20
+			})
+
+		it "should prefer the true over false for other keys", ->
+			expect(@SubscriptionUpdater._mergeFeatures({
+				github: true
+			}, {
+				github: false
+			})).to.deep.equal({
+				github: true
+			})
+			expect(@SubscriptionUpdater._mergeFeatures({
+				github: false
+			}, {
+				github: true
+			})).to.deep.equal({
+				github: true
+			})
+			expect(@SubscriptionUpdater._mergeFeatures({
+				github: true
+			}, {
+				github: true
+			})).to.deep.equal({
+				github: true
+			})
+			expect(@SubscriptionUpdater._mergeFeatures({
+				github: false
+			}, {
+				github: false
+			})).to.deep.equal({
+				github: false
+			})
 
 	describe "deleteSubscription", ->
 		beforeEach (done) ->
@@ -255,7 +347,7 @@ describe "SubscriptionUpdater", ->
 				member_ids: [ ObjectId(), ObjectId(), ObjectId() ]
 			}
 			@SubscriptionLocator.getSubscription = sinon.stub().yields(null, @subscription)
-			@SubscriptionUpdater._setUsersMinimumFeatures = sinon.stub().yields()
+			@SubscriptionUpdater.refreshFeatures = sinon.stub().yields()
 			@SubscriptionUpdater.deleteSubscription @subscription_id, done
 			
 		it "should look up the subscription", ->
@@ -269,22 +361,12 @@ describe "SubscriptionUpdater", ->
 				.should.equal true
 		
 		it "should downgrade the admin_id", ->
-			@SubscriptionUpdater._setUsersMinimumFeatures
+			@SubscriptionUpdater.refreshFeatures
 				.calledWith(@subscription.admin_id)
 				.should.equal true
 		
 		it "should downgrade all of the members", ->
 			for user_id in @subscription.member_ids
-				@SubscriptionUpdater._setUsersMinimumFeatures
+				@SubscriptionUpdater.refreshFeatures
 					.calledWith(user_id)
 					.should.equal true
-
-	describe 'refreshSubscription', ->
-		beforeEach ->
-			@SubscriptionUpdater._setUsersMinimumFeatures = sinon.stub()
-				.callsArgWith(1, null)
-
-		it 'should call to _setUsersMinimumFeatures', ->
-			@SubscriptionUpdater.refreshSubscription(@adminUser._id, ()->)
-			@SubscriptionUpdater._setUsersMinimumFeatures.callCount.should.equal 1
-			@SubscriptionUpdater._setUsersMinimumFeatures.calledWith(@adminUser._id).should.equal true

services/web/test/unit/coffee/Subscription/UserFeaturesUpdaterTests.coffee

@@ -4,31 +4,20 @@ sinon = require 'sinon'
 modulePath = "../../../../app/js/Features/Subscription/UserFeaturesUpdater"
 assert = require("chai").assert
 
-
 describe "UserFeaturesUpdater", ->
-
 	beforeEach ->
-
 		@User =
 			update: sinon.stub().callsArgWith(2)
-
-		@PlansLocator = 
-			findLocalPlanInSettings : sinon.stub()
-
 		@UserFeaturesUpdater = SandboxedModule.require modulePath, requires:
 			'../../models/User': User:@User
 			"logger-sharelatex": log:->
-			'./PlansLocator': @PlansLocator
 
 	describe "updateFeatures", ->
-
 		it "should send the users features", (done)->
 			user_id = "5208dd34438842e2db000005"
-			plan_code = "student"
+			@features = {versioning:true, collaborators:10}
-			@features = features:{versioning:true, collaborators:10}
+			@UserFeaturesUpdater.updateFeatures user_id, @features, (err, features)=>
-			@PlansLocator.findLocalPlanInSettings = sinon.stub().returns(@features)
-			@UserFeaturesUpdater.updateFeatures user_id, plan_code, (err, features)=>
 				update = {"features.versioning":true, "features.collaborators":10}
 				@User.update.calledWith({"_id":user_id}, update).should.equal true
-				features.should.deep.equal @features.features
+				features.should.deep.equal @features
 				done()

services/web/test/unit/coffee/Subscription/V1SusbcriptionManagerTests.coffee

@@ -0,0 +1,128 @@
+should = require('chai').should()
+SandboxedModule = require('sandboxed-module')
+assert = require('assert')
+path = require('path')
+modulePath = path.join __dirname, '../../../../app/js/Features/Subscription/V1SubscriptionManager'
+sinon = require("sinon")
+expect = require("chai").expect
+
+
+describe 'V1SubscriptionManager', ->
+	beforeEach ->
+		@V1SubscriptionManager = SandboxedModule.require modulePath, requires:
+			"../User/UserGetter": @UserGetter = {}
+			"logger-sharelatex":
+				log: sinon.stub()
+				err: sinon.stub()
+				warn: sinon.stub()
+			"settings-sharelatex":
+				overleaf:
+					host: @host = "http://overleaf.example.com"
+			"request": @request = sinon.stub()
+		@V1SubscriptionManager._v1PlanRequest = sinon.stub()
+		@userId = 'abcd'
+		@v1UserId = 42
+		@user =
+			_id: @userId
+			email: 'user@example.com'
+			overleaf:
+				id: @v1UserId
+
+	describe 'getPlanCodeFromV1', ->
+		beforeEach ->
+			@responseBody =
+				id: 32,
+				plan_name: 'pro'
+			@UserGetter.getUser = sinon.stub()
+				.yields(null, @user)
+			@V1SubscriptionManager._v1PlanRequest = sinon.stub()
+				.yields(null, @responseBody)
+			@call = (cb) =>
+				@V1SubscriptionManager.getPlanCodeFromV1 @userId, cb
+
+		describe 'when all goes well', ->
+
+			it 'should call getUser', (done) ->
+				@call (err, planCode) =>
+					expect(
+						@UserGetter.getUser.callCount
+					).to.equal 1
+					expect(
+						@UserGetter.getUser.calledWith(@userId)
+					).to.equal true
+					done()
+
+			it 'should call _v1PlanRequest', (done) ->
+				@call (err, planCode) =>
+					expect(
+						@V1SubscriptionManager._v1PlanRequest.callCount
+					).to.equal 1
+					expect(
+						@V1SubscriptionManager._v1PlanRequest.calledWith(
+							@v1UserId
+						)
+					).to.equal true
+					done()
+
+			it 'should produce a plan-code without error', (done) ->
+				@call (err, planCode) =>
+					expect(err).to.not.exist
+					expect(planCode).to.equal 'v1_pro'
+					done()
+
+			describe 'when the plan_name from v1 is null', ->
+				beforeEach ->
+					@responseBody.plan_name = null
+
+				it 'should produce a null plan-code without error', (done) ->
+					@call (err, planCode) =>
+						expect(err).to.not.exist
+						expect(planCode).to.equal null
+						done()
+
+		describe 'when getUser produces an error', ->
+			beforeEach ->
+				@UserGetter.getUser = sinon.stub()
+					.yields(new Error('woops'))
+
+			it 'should not call _v1PlanRequest', (done) ->
+				@call (err, planCode) =>
+					expect(
+						@V1SubscriptionManager._v1PlanRequest.callCount
+					).to.equal 0
+					done()
+
+			it 'should produce an error', (done) ->
+				@call (err, planCode) =>
+					expect(err).to.exist
+					expect(planCode).to.not.exist
+					done()
+
+		describe 'when getUser does not find a user', ->
+			beforeEach ->
+				@UserGetter.getUser = sinon.stub()
+					.yields(null, null)
+
+			it 'should not call _v1PlanRequest', (done) ->
+				@call (err, planCode) =>
+					expect(
+						@V1SubscriptionManager._v1PlanRequest.callCount
+					).to.equal 0
+					done()
+
+			it 'should produce a null plan-code, without error', (done) ->
+				@call (err, planCode) =>
+					expect(err).to.not.exist
+					expect(planCode).to.not.exist
+					done()
+
+		describe 'when the request to v1 fails', ->
+			beforeEach ->
+				@V1SubscriptionManager._v1PlanRequest = sinon.stub()
+					.yields(new Error('woops'))
+
+			it 'should produce an error', (done) ->
+				@call (err, planCode) =>
+					expect(err).to.exist
+					expect(planCode).to.not.exist
+					done()