Merge pull request #1028 from sharelatex/spd-no-github-for-new-users

retry: Don't add old v1 features for new accounts

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

@@ -52,8 +52,14 @@ module.exports = FeaturesUpdater =
 			callback err, (subs or []).map FeaturesUpdater._subscriptionToFeatures
 
 	_getV1Features: (user_id, callback = (error, features = {}) ->) ->
-		V1SubscriptionManager.getPlanCodeFromV1 user_id, (err, planCode) ->
+		V1SubscriptionManager.getPlanCodeFromV1 user_id, (err, planCode, v1Id) ->
-			callback err, FeaturesUpdater._planCodeToFeatures(planCode)
+			if err?
+				return callback(err)
+
+			callback(err, FeaturesUpdater._mergeFeatures(
+				V1SubscriptionManager.getGrandfatheredFeaturesForV1User(v1Id) or {},
+				FeaturesUpdater._planCodeToFeatures(planCode)
+			))
 
 	_mergeFeatures: (featuresA, featuresB) ->
 		features = Object.assign({}, featuresA)

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

@@ -11,12 +11,12 @@ module.exports = V1SubscriptionManager =
 	#   - 'v1_pro_plus'
 	#   - 'v1_student'
 	#   - 'v1_free'
-	getPlanCodeFromV1: (userId, callback=(err, planCode)->) ->
+	getPlanCodeFromV1: (userId, callback=(err, planCode, v1Id)->) ->
 		logger.log {userId}, "[V1SubscriptionManager] fetching v1 plan for user"
 		V1SubscriptionManager._v1Request userId, {
 			method: 'GET',
 			url: (v1Id) -> "/api/v1/sharelatex/users/#{v1Id}/plan_code"
-		}, (error, body) ->
+		}, (error, body, v1Id) ->
 			return callback(error) if error?
 			planName = body?.plan_name
 			logger.log {userId, planName, body}, "[V1SubscriptionManager] fetched v1 plan for user"
@@ -25,7 +25,7 @@ module.exports = V1SubscriptionManager =
 			else
 				# Throw away 'anonymous', etc as being equivalent to null
 				planName = null
-			return callback(null, planName)
+			return callback(null, planName, v1Id)
 
 	notifyV1OfFeaturesChange: (userId, callback = (error) ->) ->
 		V1SubscriptionManager._v1Request userId, {
@@ -33,21 +33,40 @@ module.exports = V1SubscriptionManager =
 			url: (v1Id) -> "/api/v1/sharelatex/users/#{v1Id}/sync"
 		}, callback
 
-	getSubscriptionsFromV1: (userId, callback=(err, subscriptions) ->) ->
+	getSubscriptionsFromV1: (userId, callback=(err, subscriptions, v1Id) ->) ->
 		V1SubscriptionManager._v1Request userId, {
 			method: 'GET',
 			url: (v1Id) -> "/api/v1/sharelatex/users/#{v1Id}/subscriptions"
 		}, callback
 
-	_v1Request: (userId, options, callback=(err, body)->) ->
+	v1IdForUser: (userId, callback=(err, v1Id) ->) ->
-		if !settings?.apis?.v1
-			return callback null, null
 		UserGetter.getUser userId, {'overleaf.id': 1}, (err, user) ->
 			return callback(err) if err?
 			v1Id = user?.overleaf?.id
 			if !v1Id?
 				logger.log {userId}, "[V1SubscriptionManager] no v1 id found for user"
-				return callback(null, null)
+
+			callback(null, v1Id)
+
+	# v1 accounts created before migration to v2 had github and mendeley for free
+	# but these are now paid-for features for new accounts (v1id > cutoff)
+	getGrandfatheredFeaturesForV1User: (v1Id) ->
+		cutoff = settings.v1GrandfatheredFeaturesUidCutoff
+		return {} if !cutoff?
+		return {} if !v1Id?
+
+		if (v1Id < cutoff)
+			return settings.v1GrandfatheredFeatures or {}
+		else
+			return {}
+
+	_v1Request: (userId, options, callback=(err, body, v1Id)->) ->
+		if !settings?.apis?.v1
+			return callback null, null
+
+		V1SubscriptionManager.v1IdForUser userId, (err, v1Id) ->
+			return callback(err) if err?
+			return callback(null, null, null) if !v1Id?
 			request {
 				baseUrl: settings.apis.v1.url
 				url: options.url(v1Id)
@@ -64,7 +83,6 @@ module.exports = V1SubscriptionManager =
 					error = new V1ConnectionError('No V1 connection') if error.code == 'ECONNREFUSED'
 					return callback(error)
 				if 200 <= response.statusCode < 300
-					return callback null, body
+					return callback null, body, v1Id
 				else
 					return callback new Error("non-success code from v1: #{response.statusCode}")
-

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

@@ -15,10 +15,14 @@ describe 'V1SubscriptionManager', ->
 				log: sinon.stub()
 				err: sinon.stub()
 				warn: sinon.stub()
-			"settings-sharelatex":
+			"settings-sharelatex": @Settings =
 				apis:
 					v1:
 						host: @host = "http://overleaf.example.com"
+				v1GrandfatheredFeaturesUidCutoff: 10
+				v1GrandfatheredFeatures:
+					github: true
+					mendeley: true
 			"request": @request = sinon.stub()
 		@userId = 'abcd'
 		@v1UserId = 42
@@ -51,6 +55,11 @@ describe 'V1SubscriptionManager', ->
 					).to.equal true
 					done()
 
+			it 'should return the v1 user id', (done) ->
+				@call (err, planCode, v1Id) ->
+					expect(v1Id).to.equal @v1UserId
+					done()
+
 			it 'should produce a plan-code without error', (done) ->
 				@call (err, planCode) =>
 					expect(err).to.not.exist
@@ -67,14 +76,27 @@ describe 'V1SubscriptionManager', ->
 						expect(planCode).to.equal null
 						done()
 
+	describe 'getGrandfatheredFeaturesForV1User', ->
+		describe 'when the user ID is greater than the cutoff', ->
+			it 'should return an empty feature set', (done) ->
+				expect(@V1SubscriptionManager.getGrandfatheredFeaturesForV1User 100).to.eql {}
+				done()
+
+		describe 'when the user ID is less than the cutoff', ->
+			it 'should return a feature set with grandfathered properties for github and mendeley', (done) ->
+				expect(@V1SubscriptionManager.getGrandfatheredFeaturesForV1User 1).to.eql
+					github: true
+					mendeley: true
+				done()
+
 	describe '_v1Request', ->
 		beforeEach ->
 			@UserGetter.getUser = sinon.stub()
 				.yields(null, @user)
 
-		describe 'when getUser produces an error', ->
+		describe 'when v1IdForUser produces an error', ->
 			beforeEach ->
-				@UserGetter.getUser = sinon.stub()
+				@V1SubscriptionManager.v1IdForUser = sinon.stub()
 					.yields(new Error('woops'))
 				@call = (cb) =>
 					@V1SubscriptionManager._v1Request @user_id, { url: () -> '/foo' }, cb
@@ -91,9 +113,9 @@ describe 'V1SubscriptionManager', ->
 					expect(err).to.exist
 					done()
 
-		describe 'when getUser does not find a user', ->
+		describe 'when v1IdForUser does not find a user', ->
 			beforeEach ->
-				@UserGetter.getUser = sinon.stub()
+				@V1SubscriptionManager.v1IdForUser = sinon.stub()
 					.yields(null, null)
 				@call = (cb) =>
 					@V1SubscriptionManager._v1Request @user_id, { url: () -> '/foo' }, cb
@@ -120,3 +142,83 @@ describe 'V1SubscriptionManager', ->
 				@call (err) =>
 					expect(err).to.exist
 					done()
+
+		describe 'when the call succeeds', ->
+			beforeEach ->
+				@V1SubscriptionManager.v1IdForUser = sinon.stub()
+					.yields(null, @v1UserId)
+				@request.yields(null, { statusCode: 200 }, "{}")
+				@call = (cb) =>
+					@V1SubscriptionManager._v1Request @user_id, { url: () -> '/foo' }, cb
+
+			it 'should not produce an error', (done) ->
+				@call (err, body, v1Id) =>
+					expect(err).not.to.exist
+					done()
+
+			it 'should return the v1 user id', (done) ->
+				@call (err, body, v1Id) =>
+					expect(v1Id).to.equal @v1UserId
+					done()
+
+			it 'should return the http response body', (done) ->
+				@call (err, body, v1Id) =>
+					expect(body).to.equal "{}"
+					done()
+
+		describe 'when the call returns an http error status code', ->
+			beforeEach ->
+				@V1SubscriptionManager.v1IdForUser = sinon.stub()
+					.yields(null, @v1UserId)
+				@request.yields(null, { statusCode: 500 }, "{}")
+				@call = (cb) =>
+					@V1SubscriptionManager._v1Request @user_id, { url: () -> '/foo' }, cb
+
+			it 'should produce an error', (done) ->
+				@call (err, body, v1Id) =>
+					expect(err).to.exist
+					done()
+
+	describe 'v1IdForUser', ->
+		beforeEach ->
+			@UserGetter.getUser = sinon.stub()
+				.yields(null, @user)
+
+		describe 'when getUser produces an error', ->
+			beforeEach ->
+				@UserGetter.getUser = sinon.stub()
+					.yields(new Error('woops'))
+				@call = (cb) =>
+					@V1SubscriptionManager.v1IdForUser @user_id, cb
+
+			it 'should produce an error', (done) ->
+				@call (err) =>
+					expect(err).to.exist
+					done()
+
+		describe 'when getUser does not find a user', ->
+			beforeEach ->
+				@UserGetter.getUser = sinon.stub()
+					.yields(null, null)
+				@call = (cb) =>
+					@V1SubscriptionManager.v1IdForUser @user_id, cb
+
+			it 'should not error', (done) ->
+				@call (err, user_id) =>
+					expect(err).to.not.exist
+					done()
+
+		describe 'when it works', ->
+			beforeEach ->
+				@call = (cb) =>
+					@V1SubscriptionManager.v1IdForUser @user_id, cb
+
+			it 'should not error', (done) ->
+				@call (err, user_id) =>
+					expect(err).to.not.exist
+					done()
+
+			it 'should return the v1 user id', (done) ->
+				@call (err, user_id) =>
+					expect(user_id).to.eql 42
+					done()