From 6fc6c4460595d37680835812a0d784145b97af41 Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Thu, 11 Oct 2018 12:16:59 +0100 Subject: [PATCH 1/2] Don't add old v1 features for new accounts Introduces the notion of v1 'grandfathered features', specifically Gihub and Mendeley integration. This allows us to create new v1 accounts for new users without them automatically getting the new features. Requires a settings change in `settings.web.sl.coffee` to disable these features by default for v1 accounts. bug: overleaf/sharelatex#1014 --- .../Subscription/FeaturesUpdater.coffee | 10 ++- .../Subscription/V1SubscriptionManager.coffee | 38 +++++++--- .../V1SusbcriptionManagerTests.coffee | 71 +++++++++++++++++-- 3 files changed, 102 insertions(+), 17 deletions(-) diff --git a/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee b/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee index ef5a6c4bb2..b091ed9f30 100644 --- a/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee +++ b/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) -> - callback err, FeaturesUpdater._planCodeToFeatures(planCode) + V1SubscriptionManager.getPlanCodeFromV1 user_id, (err, planCode, v1Id) -> + if err? + return callback(err) + + callback(err, FeaturesUpdater._mergeFeatures( + V1SubscriptionManager.getGrandfatheredFeaturesForV1User(v1Id) or {}, + FeaturesUpdater._planCodeToFeatures(planCode) + )) _mergeFeatures: (featuresA, featuresB) -> features = Object.assign({}, featuresA) diff --git a/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee b/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee index 857881df4d..b4f538873f 100644 --- a/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee +++ b/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)->) -> - if !settings?.apis?.v1 - return callback null, null + v1IdForUser: (userId, callback=(err, v1Id) ->) -> 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) 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}") - diff --git a/services/web/test/unit/coffee/Subscription/V1SusbcriptionManagerTests.coffee b/services/web/test/unit/coffee/Subscription/V1SusbcriptionManagerTests.coffee index 9d85d81f84..7f207e93af 100644 --- a/services/web/test/unit/coffee/Subscription/V1SusbcriptionManagerTests.coffee +++ b/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 @@ -67,14 +71,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 +108,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 +137,47 @@ describe 'V1SubscriptionManager', -> @call (err) => 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() \ No newline at end of file From 0f54bc2c524e1f9bd345418a2e5c355a9daf76a7 Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Tue, 16 Oct 2018 10:15:42 +0100 Subject: [PATCH 2/2] Add additional tests for V1SubscriptionManager One call was not returning the v1Id correctly. These tests check for that case. Also added some more generic tests for the v1 API call. bug: overleaf/sharelatex#1014 --- .../Subscription/V1SubscriptionManager.coffee | 2 +- .../V1SusbcriptionManagerTests.coffee | 41 +++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee b/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee index b4f538873f..2d25c8b3e5 100644 --- a/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee +++ b/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee @@ -66,7 +66,7 @@ module.exports = V1SubscriptionManager = V1SubscriptionManager.v1IdForUser userId, (err, v1Id) -> return callback(err) if err? - return callback(null, null) if !v1Id? + return callback(null, null, null) if !v1Id? request { baseUrl: settings.apis.v1.url url: options.url(v1Id) diff --git a/services/web/test/unit/coffee/Subscription/V1SusbcriptionManagerTests.coffee b/services/web/test/unit/coffee/Subscription/V1SusbcriptionManagerTests.coffee index 7f207e93af..7666a418bb 100644 --- a/services/web/test/unit/coffee/Subscription/V1SusbcriptionManagerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/V1SusbcriptionManagerTests.coffee @@ -55,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 @@ -138,6 +143,42 @@ describe 'V1SubscriptionManager', -> 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()