From 33247960860f9e8e27106685019db2c62fca11cd Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Mon, 3 Sep 2018 15:09:57 +0100 Subject: [PATCH] don't regard v1 teams as paid subscriptions - use `userHasV1Subscription` instead of `userHasV1SubscriptionOrTeam` in `LimitationsManager.userHasSubscriptionOrIsGroupMember ` - remove `userHasV1SubscriptionOrTeam` - rename `LimitationsManager.userHasSubscriptionOrIsGroupMember` to `LimitationsManager.hasPaidSubscription` - rename some variables for clarity --- .../Features/Project/ProjectController.coffee | 4 +- .../Subscription/LimitationsManager.coffee | 14 +----- .../SubscriptionController.coffee | 10 ++--- .../Project/ProjectControllerTests.coffee | 8 ++-- .../LimitationsManagerTests.coffee | 45 ++++--------------- .../SubscriptionControllerTests.coffee | 16 +++---- 6 files changed, 29 insertions(+), 68 deletions(-) diff --git a/services/web/app/coffee/Features/Project/ProjectController.coffee b/services/web/app/coffee/Features/Project/ProjectController.coffee index 039a193921..24dca35e96 100644 --- a/services/web/app/coffee/Features/Project/ProjectController.coffee +++ b/services/web/app/coffee/Features/Project/ProjectController.coffee @@ -189,10 +189,10 @@ module.exports = ProjectController = return cb(null, projects: [], tags: [], noConnection: true) return cb(error, projects[0]) # hooks.fire returns an array of results, only need first hasSubscription: (cb)-> - LimitationsManager.userHasSubscriptionOrIsGroupMember currentUser, (error, hasSub) -> + LimitationsManager.hasPaidSubscription currentUser, (error, hasPaidSubscription) -> if error? and error instanceof V1ConnectionError return cb(null, true) - return cb(error, hasSub) + return cb(error, hasPaidSubscription) user: (cb) -> User.findById user_id, "featureSwitches overleaf awareOfV2 features", cb }, (err, results)-> diff --git a/services/web/app/coffee/Features/Subscription/LimitationsManager.coffee b/services/web/app/coffee/Features/Subscription/LimitationsManager.coffee index 078676086b..1cac328d4b 100644 --- a/services/web/app/coffee/Features/Subscription/LimitationsManager.coffee +++ b/services/web/app/coffee/Features/Subscription/LimitationsManager.coffee @@ -33,12 +33,12 @@ module.exports = LimitationsManager = else callback null, false - userHasSubscriptionOrIsGroupMember: (user, callback = (err, hasSubscriptionOrIsMember)->) -> + hasPaidSubscription: (user, callback = (err, hasSubscriptionOrIsMember)->) -> @userHasV2Subscription user, (err, hasSubscription, subscription)=> return callback(err) if err? @userIsMemberOfGroupSubscription user, (err, isMember)=> return callback(err) if err? - @userHasV1SubscriptionOrTeam user, (err, hasV1Subscription)=> + @userHasV1Subscription user, (err, hasV1Subscription)=> return callback(err) if err? logger.log {user_id:user._id, isMember, hasSubscription, hasV1Subscription}, "checking if user has subscription or is group member" callback err, isMember or hasSubscription or hasV1Subscription, subscription @@ -72,16 +72,6 @@ module.exports = LimitationsManager = logger.log {user_id: user._id, v1Subscription}, '[userHasV1Subscription]' callback err, !!v1Subscription?.has_subscription - userHasV1SubscriptionOrTeam: (user, callback = (error, hasV1Subscription) ->) -> - V1SubscriptionManager.getSubscriptionsFromV1 user._id, (err, v1Subscription = {}) -> - return callback(err) if err? - hasV1Subscription = false - if v1Subscription.has_subscription - hasV1Subscription = true - if (v1Subscription.teams or []).length > 0 - hasV1Subscription = true - return callback null, hasV1Subscription - teamHasReachedMemberLimit: (subscription) -> currentTotal = (subscription.member_ids or []).length + (subscription.teamInvites or []).length + diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee index 2b7ca06ca8..f03a749cee 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee @@ -90,22 +90,22 @@ module.exports = SubscriptionController = userSubscriptionPage: (req, res, next) -> user = AuthenticationController.getSessionUser(req) - LimitationsManager.userHasSubscriptionOrIsGroupMember user, (err, hasSubOrIsGroupMember, subscription)-> + LimitationsManager.hasPaidSubscription user, (err, hasPaidSubscription, subscription)-> return next(err) if err? groupLicenceInviteUrl = SubscriptionDomainHandler.getDomainLicencePage(user) if subscription?.customAccount logger.log user: user, "redirecting to custom account page" res.redirect "/user/subscription/custom_account" - else if groupLicenceInviteUrl? and !hasSubOrIsGroupMember + else if groupLicenceInviteUrl? and !hasPaidSubscription logger.log user:user, "redirecting to group subscription invite page" res.redirect groupLicenceInviteUrl - else if !hasSubOrIsGroupMember + else if !hasPaidSubscription logger.log user: user, "redirecting to plans" res.redirect "/user/subscription/plans" else SubscriptionViewModelBuilder.buildUsersSubscriptionViewModel user, (error, subscription, groupSubscriptions, billingDetailsLink, v1Subscriptions) -> return next(error) if error? - logger.log {user, subscription, hasSubOrIsGroupMember, groupSubscriptions, billingDetailsLink, v1Subscriptions}, "showing subscription dashboard" + logger.log {user, subscription, hasPaidSubscription, groupSubscriptions, billingDetailsLink, v1Subscriptions}, "showing subscription dashboard" plans = SubscriptionViewModelBuilder.buildViewModel() res.render "subscriptions/dashboard", title: "your_subscription" @@ -122,7 +122,7 @@ module.exports = SubscriptionController = userCustomSubscriptionPage: (req, res, next)-> user = AuthenticationController.getSessionUser(req) - LimitationsManager.userHasSubscriptionOrIsGroupMember user, (err, hasSubOrIsGroupMember, subscription)-> + LimitationsManager.hasPaidSubscription user, (err, hasPaidSubscription, subscription)-> return next(err) if err? if !subscription? err = new Error("subscription null for custom account, user:#{user?._id}") diff --git a/services/web/test/unit/coffee/Project/ProjectControllerTests.coffee b/services/web/test/unit/coffee/Project/ProjectControllerTests.coffee index dc23107d66..b909376cca 100644 --- a/services/web/test/unit/coffee/Project/ProjectControllerTests.coffee +++ b/services/web/test/unit/coffee/Project/ProjectControllerTests.coffee @@ -36,7 +36,7 @@ describe "ProjectController", -> @SubscriptionLocator = getUsersSubscription: sinon.stub() @LimitationsManager = - userHasSubscriptionOrIsGroupMember: sinon.stub() + hasPaidSubscription: sinon.stub() @TagsHandler = getAllTags: sinon.stub() @NotificationsHandler = @@ -283,7 +283,7 @@ describe "ProjectController", -> @UserModel.findById = (id, fields, callback) => callback null, @users[id] - @LimitationsManager.userHasSubscriptionOrIsGroupMember.callsArgWith(1, null, false) + @LimitationsManager.hasPaidSubscription.callsArgWith(1, null, false) @TagsHandler.getAllTags.callsArgWith(1, null, @tags, {}) @NotificationsHandler.getUserNotifications = sinon.stub().callsArgWith(1, null, @notifications, {}) @ProjectGetter.findAllUsersProjects.callsArgWith(2, null, @allProjects) @@ -327,7 +327,7 @@ describe "ProjectController", -> @ProjectController.projectListPage @req, @res it 'should send hasSubscription == true when there is a subscription', (done) -> - @LimitationsManager.userHasSubscriptionOrIsGroupMember = sinon.stub().callsArgWith(1, null, true) + @LimitationsManager.hasPaidSubscription = sinon.stub().callsArgWith(1, null, true) @res.render = (pageName, opts)=> opts.hasSubscription.should.equal true done() @@ -447,7 +447,7 @@ describe "ProjectController", -> @UserModel.findById = (id, fields, callback) => callback null, @users[id] - @LimitationsManager.userHasSubscriptionOrIsGroupMember.callsArgWith(1, null, false) + @LimitationsManager.hasPaidSubscription.callsArgWith(1, null, false) @TagsHandler.getAllTags.callsArgWith(1, null, @tags, {}) @NotificationsHandler.getUserNotifications = sinon.stub().callsArgWith(1, null, @notifications, {}) @ProjectGetter.findAllUsersProjects.callsArgWith(2, null, @allProjects) diff --git a/services/web/test/unit/coffee/Subscription/LimitationsManagerTests.coffee b/services/web/test/unit/coffee/Subscription/LimitationsManagerTests.coffee index 9d87870121..57231d9f38 100644 --- a/services/web/test/unit/coffee/Subscription/LimitationsManagerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/LimitationsManagerTests.coffee @@ -248,32 +248,32 @@ describe "LimitationsManager", -> retSubscriptions.should.equal subscriptions done() - describe "userHasSubscriptionOrIsGroupMember", -> + describe "hasPaidSubscription", -> beforeEach -> @LimitationsManager.userIsMemberOfGroupSubscription = sinon.stub().yields(null, false) @LimitationsManager.userHasV2Subscription = sinon.stub().yields(null, false) - @LimitationsManager.userHasV1SubscriptionOrTeam = sinon.stub().yields(null, false) + @LimitationsManager.userHasV1Subscription = sinon.stub().yields(null, false) it "should return true if userIsMemberOfGroupSubscription", (done)-> @LimitationsManager.userIsMemberOfGroupSubscription = sinon.stub().yields(null, true) - @LimitationsManager.userHasSubscriptionOrIsGroupMember @user, (err, hasSubOrIsGroupMember)-> + @LimitationsManager.hasPaidSubscription @user, (err, hasSubOrIsGroupMember)-> hasSubOrIsGroupMember.should.equal true done() it "should return true if userHasV2Subscription", (done)-> @LimitationsManager.userHasV2Subscription = sinon.stub().yields(null, true) - @LimitationsManager.userHasSubscriptionOrIsGroupMember @user, (err, hasSubOrIsGroupMember)-> + @LimitationsManager.hasPaidSubscription @user, (err, hasSubOrIsGroupMember)-> hasSubOrIsGroupMember.should.equal true done() - it "should return true if userHasV1SubscriptionOrTeam", (done)-> - @LimitationsManager.userHasV1SubscriptionOrTeam = sinon.stub().yields(null, true) - @LimitationsManager.userHasSubscriptionOrIsGroupMember @user, (err, hasSubOrIsGroupMember)-> + it "should return true if userHasV1Subscription", (done)-> + @LimitationsManager.userHasV1Subscription= sinon.stub().yields(null, true) + @LimitationsManager.hasPaidSubscription @user, (err, hasSubOrIsGroupMember)-> hasSubOrIsGroupMember.should.equal true done() it "should return false if none are true", (done)-> - @LimitationsManager.userHasSubscriptionOrIsGroupMember @user, (err, hasSubOrIsGroupMember)-> + @LimitationsManager.hasPaidSubscription @user, (err, hasSubOrIsGroupMember)-> hasSubOrIsGroupMember.should.equal false done() @@ -351,32 +351,3 @@ describe "LimitationsManager", -> @V1SubscriptionManager.getSubscriptionsFromV1.calledWith(@user_id).should.equal true result.should.equal false done() - - describe 'userHasV1SubscriptionOrTeam', -> - it 'should return true if v1 returns has_subscription = true', (done) -> - @V1SubscriptionManager.getSubscriptionsFromV1 = sinon.stub().yields(null, { has_subscription: true }) - @LimitationsManager.userHasV1SubscriptionOrTeam @user, (error, result) => - @V1SubscriptionManager.getSubscriptionsFromV1.calledWith(@user_id).should.equal true - result.should.equal true - done() - - it 'should return true if v1 returns some teams', (done) -> - @V1SubscriptionManager.getSubscriptionsFromV1 = sinon.stub().yields(null, { teams: ['mock-team'] }) - @LimitationsManager.userHasV1SubscriptionOrTeam @user, (error, result) => - @V1SubscriptionManager.getSubscriptionsFromV1.calledWith(@user_id).should.equal true - result.should.equal true - done() - - it 'should return false if v1 returns has_subscription = false and no teams', (done) -> - @V1SubscriptionManager.getSubscriptionsFromV1 = sinon.stub().yields(null, { has_subscription: false, teams: [] }) - @LimitationsManager.userHasV1SubscriptionOrTeam @user, (error, result) => - @V1SubscriptionManager.getSubscriptionsFromV1.calledWith(@user_id).should.equal true - result.should.equal false - done() - - it 'should return false if v1 returns nothing', (done) -> - @V1SubscriptionManager.getSubscriptionsFromV1 = sinon.stub().yields(null, null) - @LimitationsManager.userHasV1SubscriptionOrTeam @user, (error, result) => - @V1SubscriptionManager.getSubscriptionsFromV1.calledWith(@user_id).should.equal true - result.should.equal false - done() diff --git a/services/web/test/unit/coffee/Subscription/SubscriptionControllerTests.coffee b/services/web/test/unit/coffee/Subscription/SubscriptionControllerTests.coffee index 5fa0e414f5..085241203d 100644 --- a/services/web/test/unit/coffee/Subscription/SubscriptionControllerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/SubscriptionControllerTests.coffee @@ -39,7 +39,7 @@ describe "SubscriptionController", -> findLocalPlanInSettings: sinon.stub() @LimitationsManager = - userHasSubscriptionOrIsGroupMember: sinon.stub() + hasPaidSubscription: sinon.stub() userHasV1OrV2Subscription : sinon.stub() userHasV2Subscription: sinon.stub() @@ -226,7 +226,7 @@ describe "SubscriptionController", -> describe "with a user without a subscription", -> beforeEach (done) -> @res.callback = done - @LimitationsManager.userHasSubscriptionOrIsGroupMember.callsArgWith(1, null, false) + @LimitationsManager.hasPaidSubscription.callsArgWith(1, null, false) @SubscriptionController.userSubscriptionPage @req, @res it "should redirect to the plans page", -> @@ -241,7 +241,7 @@ describe "SubscriptionController", -> describe "without an existing subscription", -> beforeEach (done)-> @res.callback = done - @LimitationsManager.userHasSubscriptionOrIsGroupMember.callsArgWith(1, null, false) + @LimitationsManager.hasPaidSubscription.callsArgWith(1, null, false) @SubscriptionController.userSubscriptionPage @req, @res it "should redirect to the group invite url", -> @@ -253,7 +253,7 @@ describe "SubscriptionController", -> @res.callback = done @settings.apis.recurly.subdomain = 'test' @userSub = {account: {hosted_login_token: 'abcd'}} - @LimitationsManager.userHasSubscriptionOrIsGroupMember + @LimitationsManager.hasPaidSubscription .callsArgWith(1, null, true, {}) @SubscriptionController.userSubscriptionPage @req, @res @@ -265,7 +265,7 @@ describe "SubscriptionController", -> beforeEach (done) -> @res.callback = done @SubscriptionViewModelBuilder.buildUsersSubscriptionViewModel.callsArgWith(1, null, @activeRecurlySubscription) - @LimitationsManager.userHasSubscriptionOrIsGroupMember.callsArgWith(1, null, true, {}) + @LimitationsManager.hasPaidSubscription.callsArgWith(1, null, true, {}) @SubscriptionController.userSubscriptionPage @req, @res it "should render the dashboard", (done)-> @@ -280,7 +280,7 @@ describe "SubscriptionController", -> beforeEach (done) -> @res.callback = done @SubscriptionViewModelBuilder.buildUsersSubscriptionViewModel.callsArgWith(1, null, @activeRecurlySubscription) - @LimitationsManager.userHasSubscriptionOrIsGroupMember.callsArgWith(1, null, true, {}) + @LimitationsManager.hasPaidSubscription.callsArgWith(1, null, true, {}) @SubscriptionController.userSubscriptionPage @req, @res it "should render the dashboard", -> @@ -292,7 +292,7 @@ describe "SubscriptionController", -> describe "when its a custom subscription which is non recurly", -> beforeEach ()-> - @LimitationsManager.userHasSubscriptionOrIsGroupMember.callsArgWith(1, null, true, {customAccount:true}) + @LimitationsManager.hasPaidSubscription.callsArgWith(1, null, true, {customAccount:true}) @SubscriptionController.userSubscriptionPage @req, @res it "should redirect to /user/subscription/custom_account", -> @@ -301,7 +301,7 @@ describe "SubscriptionController", -> describe "userCustomSubscriptionPage", -> beforeEach (done) -> @res.callback = done - @LimitationsManager.userHasSubscriptionOrIsGroupMember.callsArgWith(1, null, true, {}) + @LimitationsManager.hasPaidSubscription.callsArgWith(1, null, true, {}) @SubscriptionController.userCustomSubscriptionPage @req, @res it "should render the page", (done)->