From 9d8426346374af43c59ee2ff51c7e7d6a7d56f77 Mon Sep 17 00:00:00 2001 From: James Allen Date: Mon, 4 Jun 2018 17:55:02 +0100 Subject: [PATCH 1/8] Fetch subscription details from v1 and display to user --- .../Subscription/LimitationsManager.coffee | 17 ++++++- .../SubscriptionController.coffee | 5 +- .../SubscriptionViewModelBuilder.coffee | 50 ++++++++++--------- .../Subscription/V1SubscriptionManager.coffee | 6 +++ .../web/app/views/subscriptions/dashboard.pug | 21 +++++++- 5 files changed, 70 insertions(+), 29 deletions(-) diff --git a/services/web/app/coffee/Features/Subscription/LimitationsManager.coffee b/services/web/app/coffee/Features/Subscription/LimitationsManager.coffee index d7144b0ba0..9f2847904b 100644 --- a/services/web/app/coffee/Features/Subscription/LimitationsManager.coffee +++ b/services/web/app/coffee/Features/Subscription/LimitationsManager.coffee @@ -5,6 +5,7 @@ SubscriptionLocator = require("./SubscriptionLocator") Settings = require("settings-sharelatex") CollaboratorsHandler = require("../Collaborators/CollaboratorsHandler") CollaboratorsInvitesHandler = require("../Collaborators/CollaboratorsInviteHandler") +V1SubscriptionManager = require("./V1SubscriptionManager") module.exports = LimitationsManager = allowedNumberOfCollaboratorsInProject: (project_id, callback) -> @@ -37,8 +38,10 @@ module.exports = LimitationsManager = return callback(err) if err? @userIsMemberOfGroupSubscription user, (err, isMember)=> return callback(err) if err? - logger.log user_id:user._id, isMember:isMember, hasSubscription:hasSubscription, "checking if user has subscription or is group member" - callback err, isMember or hasSubscription, subscription + @userHasV1SubscriptionOrTeam 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 userHasSubscription: (user, callback = (err, hasSubscription, subscription)->) -> logger.log user_id:user._id, "checking if user has subscription" @@ -55,6 +58,16 @@ module.exports = LimitationsManager = return callback(err) if err? callback err, subscriptions.length > 0, subscriptions + 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 c89545ac68..a01c855f14 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee @@ -101,9 +101,9 @@ module.exports = SubscriptionController = logger.log user: user, "redirecting to plans" res.redirect "/user/subscription/plans" else - SubscriptionViewModelBuilder.buildUsersSubscriptionViewModel user, (error, subscription, groupSubscriptions, billingDetailsLink) -> + SubscriptionViewModelBuilder.buildUsersSubscriptionViewModel user, (error, subscription, groupSubscriptions, billingDetailsLink, v1Subscription) -> return next(error) if error? - logger.log {user, subscription, hasSubOrIsGroupMember, groupSubscriptions, billingDetailsLink}, "showing subscription dashboard" + logger.log {user, subscription, hasSubOrIsGroupMember, groupSubscriptions, billingDetailsLink, v1Subscription}, "showing subscription dashboard" plans = SubscriptionViewModelBuilder.buildViewModel() res.render "subscriptions/dashboard", title: "your_subscription" @@ -116,6 +116,7 @@ module.exports = SubscriptionController = user:user saved_billing_details: req.query.saved_billing_details? billingDetailsLink: billingDetailsLink + v1Subscription: v1Subscription userCustomSubscriptionPage: (req, res, next)-> user = AuthenticationController.getSessionUser(req) diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionViewModelBuilder.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionViewModelBuilder.coffee index e03a954b36..eee2c33f6b 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionViewModelBuilder.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionViewModelBuilder.coffee @@ -4,6 +4,7 @@ PlansLocator = require("./PlansLocator") SubscriptionFormatters = require("./SubscriptionFormatters") LimitationsManager = require("./LimitationsManager") SubscriptionLocator = require("./SubscriptionLocator") +V1SubscriptionManager = require("./V1SubscriptionManager") logger = require('logger-sharelatex') _ = require("underscore") @@ -27,34 +28,37 @@ module.exports = SubscriptionLocator.getMemberSubscriptions user, (err, memberSubscriptions = []) -> return callback(err) if err? - if subscription? - return callback(error) if error? + V1SubscriptionManager.getSubscriptionsFromV1 user._id, (err, v1Subscriptions) -> + return callback(err) if err? - plan = PlansLocator.findLocalPlanInSettings(subscription.planCode) + if subscription? + return callback(error) if error? - if !plan? - err = new Error("No plan found for planCode '#{subscription.planCode}'") - logger.error {user_id: user._id, err}, "error getting subscription plan for user" - return callback(err) + plan = PlansLocator.findLocalPlanInSettings(subscription.planCode) - RecurlyWrapper.getSubscription subscription.recurlySubscription_id, includeAccount: true, (err, recurlySubscription)-> - tax = recurlySubscription?.tax_in_cents || 0 + if !plan? + err = new Error("No plan found for planCode '#{subscription.planCode}'") + logger.error {user_id: user._id, err}, "error getting subscription plan for user" + return callback(err) - callback null, { - admin_id:subscription.admin_id - name: plan.name - nextPaymentDueAt: SubscriptionFormatters.formatDate(recurlySubscription?.current_period_ends_at) - state: recurlySubscription?.state - price: SubscriptionFormatters.formatPrice (recurlySubscription?.unit_amount_in_cents + tax), recurlySubscription?.currency - planCode: subscription.planCode - currency:recurlySubscription?.currency - taxRate:parseFloat(recurlySubscription?.tax_rate?._) - groupPlan: subscription.groupPlan - trial_ends_at:recurlySubscription?.trial_ends_at - }, memberSubscriptions, buildBillingDetails(recurlySubscription) + RecurlyWrapper.getSubscription subscription.recurlySubscription_id, includeAccount: true, (err, recurlySubscription)-> + tax = recurlySubscription?.tax_in_cents || 0 - else - callback null, null, memberSubscriptions, null + callback null, { + admin_id:subscription.admin_id + name: plan.name + nextPaymentDueAt: SubscriptionFormatters.formatDate(recurlySubscription?.current_period_ends_at) + state: recurlySubscription?.state + price: SubscriptionFormatters.formatPrice (recurlySubscription?.unit_amount_in_cents + tax), recurlySubscription?.currency + planCode: subscription.planCode + currency:recurlySubscription?.currency + taxRate:parseFloat(recurlySubscription?.tax_rate?._) + groupPlan: subscription.groupPlan + trial_ends_at:recurlySubscription?.trial_ends_at + }, memberSubscriptions, buildBillingDetails(recurlySubscription), v1Subscriptions + + else + callback null, null, memberSubscriptions, null, v1Subscriptions buildViewModel : -> plans = Settings.plans diff --git a/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee b/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee index e920b94f6c..dcf23a3453 100644 --- a/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee +++ b/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee @@ -32,6 +32,12 @@ module.exports = V1SubscriptionManager = url: (v1Id) -> "/api/v1/sharelatex/users/#{v1Id}/sync" }, callback + getSubscriptionsFromV1: (userId, callback=(err, subscriptions) ->) -> + 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 diff --git a/services/web/app/views/subscriptions/dashboard.pug b/services/web/app/views/subscriptions/dashboard.pug index 0db4c05423..e6de2cee5c 100644 --- a/services/web/app/views/subscriptions/dashboard.pug +++ b/services/web/app/views/subscriptions/dashboard.pug @@ -103,18 +103,35 @@ block content +printPlans(plans.studentAccounts) +printPlans(plans.individualMonthlyPlans) +printPlans(plans.individualAnnualPlans) - + hr each groupSubscription in groupSubscriptions - if (user._id+'' != groupSubscription.admin_id._id+'') div p !{translate("member_of_group_subscription", {admin_email: "" + groupSubscription.admin_id.email + ""})} span - button.btn.btn-danger(ng-click="removeSelfFromGroup('"+groupSubscription.admin_id._id+"')") #{translate("leave_group")} + button.btn.btn-danger.text-capitalise(ng-click="removeSelfFromGroup('"+groupSubscription.admin_id._id+"')") #{translate("leave_group")} + hr -if(subscription.groupPlan && user._id+'' == subscription.admin_id+'') div a(href="/subscription/group").btn.btn-primary !{translate("manage_group")} + hr + + if v1Subscription.has_subscription + p + | You are subscribed to Overleaf through Overleaf v1 + p + a.btn.btn-primary(href=settings.overleaf.host+"/users/edit#status") Manage v1 Subscription + hr + + if v1Subscription.teams && v1Subscription.teams.length > 0 + for team in v1Subscription.teams + p + | You are a member of the Overleaf v1 team: #{team.name} + p + a.btn.btn-primary(href=settings.overleaf.host+"/teams") Manage v1 Team Membership + hr .card(ng-if="view == 'cancelation'") .page-header From ce60e6941cc0210b7928d02e3e3549d76aa7c9e8 Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 5 Jun 2018 11:57:25 +0100 Subject: [PATCH 2/8] Check with v1 before allowing a new subscription --- .../Subscription/LimitationsManager.coffee | 18 ++++++++++++-- .../SubscriptionController.coffee | 24 ++++++++++++------- .../Subscription/SubscriptionHandler.coffee | 6 ++--- 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/services/web/app/coffee/Features/Subscription/LimitationsManager.coffee b/services/web/app/coffee/Features/Subscription/LimitationsManager.coffee index 9f2847904b..a23372903b 100644 --- a/services/web/app/coffee/Features/Subscription/LimitationsManager.coffee +++ b/services/web/app/coffee/Features/Subscription/LimitationsManager.coffee @@ -34,7 +34,7 @@ module.exports = LimitationsManager = callback null, false userHasSubscriptionOrIsGroupMember: (user, callback = (err, hasSubscriptionOrIsMember)->) -> - @userHasSubscription user, (err, hasSubscription, subscription)=> + @userHasV2Subscription user, (err, hasSubscription, subscription)=> return callback(err) if err? @userIsMemberOfGroupSubscription user, (err, isMember)=> return callback(err) if err? @@ -43,7 +43,7 @@ module.exports = LimitationsManager = 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 - userHasSubscription: (user, callback = (err, hasSubscription, subscription)->) -> + userHasV2Subscription: (user, callback = (err, hasSubscription, subscription)->) -> logger.log user_id:user._id, "checking if user has subscription" SubscriptionLocator.getUsersSubscription user._id, (err, subscription)-> if err? @@ -52,12 +52,26 @@ module.exports = LimitationsManager = logger.log user:user, hasValidSubscription:hasValidSubscription, subscription:subscription, "checking if user has subscription" callback err, hasValidSubscription, subscription + userHasV1OrV2Subscription: (user, callback = (err, hasSubscription) ->) -> + @userHasV2Subscription user, (err, hasV2Subscription) => + return callback(err) if err? + return callback null, true if hasV2Subscription + @userHasV1Subscription user, (err, hasV1Subscription) => + return callback(err) if err? + return callback null, true if hasV1Subscription + return callback null, false + userIsMemberOfGroupSubscription: (user, callback = (error, isMember, subscriptions) ->) -> logger.log user_id: user._id, "checking is user is member of subscription groups" SubscriptionLocator.getMemberSubscriptions user._id, (err, subscriptions = []) -> return callback(err) if err? callback err, subscriptions.length > 0, subscriptions + userHasV1Subscription: (user, callback = (error, hasV1Subscription) ->) -> + V1SubscriptionManager.getSubscriptionsFromV1 user._id, (err, v1Subscription) -> + 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? diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee index a01c855f14..a553d0303f 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee @@ -45,12 +45,12 @@ module.exports = SubscriptionController = paymentPage: (req, res, next) -> user = AuthenticationController.getSessionUser(req) plan = PlansLocator.findLocalPlanInSettings(req.query.planCode) - LimitationsManager.userHasSubscription user, (err, hasSubscription)-> + LimitationsManager.userHasV1OrV2Subscription user, (err, hasSubscription)-> return next(err) if err? if hasSubscription or !plan? res.redirect "/user/subscription" else - # LimitationsManager.userHasSubscription only checks Mongo. Double check with + # LimitationsManager.userHasV2Subscription only checks Mongo. Double check with # Recurly as well at this point (we don't do this most places for speed). SubscriptionHandler.validateNoSubscriptionInRecurly user._id, (error, valid) -> return next(error) if error? @@ -135,11 +135,17 @@ module.exports = SubscriptionController = recurly_token_id = req.body.recurly_token_id subscriptionDetails = req.body.subscriptionDetails logger.log recurly_token_id: recurly_token_id, user_id:user._id, subscriptionDetails:subscriptionDetails, "creating subscription" - SubscriptionHandler.createSubscription user, subscriptionDetails, recurly_token_id, (err)-> - if err? - logger.err err:err, user_id:user._id, "something went wrong creating subscription" - return res.sendStatus 500 - res.sendStatus 201 + + LimitationsManager.userHasV1OrV2Subscription user, (err, hasSubscription)-> + return next(err) if err? + if hasSubscription + logger.warn {user_id: user._id}, 'user already has subscription' + res.sendStatus 409 # conflict + SubscriptionHandler.createSubscription user, subscriptionDetails, recurly_token_id, (err)-> + if err? + logger.err err:err, user_id:user._id, "something went wrong creating subscription" + return next(err) + res.sendStatus 201 successful_subscription: (req, res, next)-> user = AuthenticationController.getSessionUser(req) @@ -195,7 +201,7 @@ module.exports = SubscriptionController = renderUpgradeToAnnualPlanPage: (req, res, next)-> user = AuthenticationController.getSessionUser(req) - LimitationsManager.userHasSubscription user, (err, hasSubscription, subscription)-> + LimitationsManager.userHasV2Subscription user, (err, hasSubscription, subscription)-> return next(err) if err? planCode = subscription?.planCode.toLowerCase() if planCode?.indexOf("annual") != -1 @@ -225,7 +231,7 @@ module.exports = SubscriptionController = extendTrial: (req, res, next)-> user = AuthenticationController.getSessionUser(req) - LimitationsManager.userHasSubscription user, (err, hasSubscription, subscription)-> + LimitationsManager.userHasV2Subscription user, (err, hasSubscription, subscription)-> return next(err) if err? SubscriptionHandler.extendTrial subscription, 14, (err)-> if err? diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionHandler.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionHandler.coffee index 99da3270a8..9bbbf62ba5 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionHandler.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionHandler.coffee @@ -36,7 +36,7 @@ module.exports = updateSubscription: (user, plan_code, coupon_code, callback)-> logger.log user:user, plan_code:plan_code, coupon_code:coupon_code, "updating subscription" - LimitationsManager.userHasSubscription user, (err, hasSubscription, subscription)-> + LimitationsManager.userHasV2Subscription user, (err, hasSubscription, subscription)-> if !hasSubscription return callback() else @@ -56,7 +56,7 @@ module.exports = cancelSubscription: (user, callback) -> - LimitationsManager.userHasSubscription user, (err, hasSubscription, subscription)-> + LimitationsManager.userHasV2Subscription user, (err, hasSubscription, subscription)-> if hasSubscription RecurlyWrapper.cancelSubscription subscription.recurlySubscription_id, (error) -> return callback(error) if error? @@ -73,7 +73,7 @@ module.exports = callback() reactivateSubscription: (user, callback) -> - LimitationsManager.userHasSubscription user, (err, hasSubscription, subscription)-> + LimitationsManager.userHasV2Subscription user, (err, hasSubscription, subscription)-> if hasSubscription RecurlyWrapper.reactivateSubscription subscription.recurlySubscription_id, (error) -> return callback(error) if error? From 034e50760e8ef0905434694e4e1740aa923034b9 Mon Sep 17 00:00:00 2001 From: James Allen Date: Wed, 6 Jun 2018 15:41:13 +0100 Subject: [PATCH 3/8] Add flag that can disable plans page --- .../app/coffee/Features/Subscription/SubscriptionRouter.coffee | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee index e512a1c3dd..a8c715d466 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee @@ -9,7 +9,8 @@ module.exports = apply: (webRouter, privateApiRouter, publicApiRouter) -> return unless Settings.enableSubscriptions - webRouter.get '/user/subscription/plans', SubscriptionController.plansPage + if !Settings.disablePlansPage + webRouter.get '/user/subscription/plans', SubscriptionController.plansPage webRouter.get '/user/subscription', AuthenticationController.requireLogin(), SubscriptionController.userSubscriptionPage From 606419476fe2580dd218135d3621c4bc4082632b Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 7 Jun 2018 13:26:05 +0100 Subject: [PATCH 4/8] Update LimitationsManager unit tests --- .../LimitationsManagerTests.coffee | 113 ++++++++++++++---- 1 file changed, 92 insertions(+), 21 deletions(-) diff --git a/services/web/test/unit/coffee/Subscription/LimitationsManagerTests.coffee b/services/web/test/unit/coffee/Subscription/LimitationsManagerTests.coffee index 63af90f62d..4b47250d4a 100644 --- a/services/web/test/unit/coffee/Subscription/LimitationsManagerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/LimitationsManagerTests.coffee @@ -31,6 +31,7 @@ describe "LimitationsManager", -> 'settings-sharelatex' : @Settings = {} "../Collaborators/CollaboratorsHandler": @CollaboratorsHandler = {} "../Collaborators/CollaboratorsInviteHandler": @CollaboratorsInviteHandler = {} + "./V1SubscriptionManager": @V1SubscriptionManager = {} 'logger-sharelatex':log:-> describe "allowedNumberOfCollaboratorsInProject", -> @@ -183,33 +184,33 @@ describe "LimitationsManager", -> it "should return false", -> @callback.calledWith(null, false).should.equal true - describe "userHasSubscription", -> + describe "userHasV2Subscription", -> beforeEach -> @SubscriptionLocator.getUsersSubscription = sinon.stub() it "should return true if the recurly token is set", (done)-> @SubscriptionLocator.getUsersSubscription.callsArgWith(1, null, recurlySubscription_id : "1234") - @LimitationsManager.userHasSubscription @user, (err, hasSubscription)-> + @LimitationsManager.userHasV2Subscription @user, (err, hasSubscription)-> hasSubscription.should.equal true done() it "should return false if the recurly token is not set", (done)-> @SubscriptionLocator.getUsersSubscription.callsArgWith(1, null, {}) @subscription = {} - @LimitationsManager.userHasSubscription @user, (err, hasSubscription)-> + @LimitationsManager.userHasV2Subscription @user, (err, hasSubscription)-> hasSubscription.should.equal false done() it "should return false if the subscription is undefined", (done)-> @SubscriptionLocator.getUsersSubscription.callsArgWith(1) - @LimitationsManager.userHasSubscription @user, (err, hasSubscription)-> + @LimitationsManager.userHasV2Subscription @user, (err, hasSubscription)-> hasSubscription.should.equal false done() it "should return the subscription", (done)-> stubbedSubscription = {freeTrial:{}, token:""} @SubscriptionLocator.getUsersSubscription.callsArgWith(1, null, stubbedSubscription) - @LimitationsManager.userHasSubscription @user, (err, hasSubOrIsGroupMember, subscription)-> + @LimitationsManager.userHasV2Subscription @user, (err, hasSubOrIsGroupMember, subscription)-> subscription.should.deep.equal stubbedSubscription done() @@ -220,12 +221,12 @@ describe "LimitationsManager", -> @SubscriptionLocator.getUsersSubscription.callsArgWith(1, null, @fakeSubscription) it 'should return true', (done) -> - @LimitationsManager.userHasSubscription @user, (err, hasSubscription, subscription)-> + @LimitationsManager.userHasV2Subscription @user, (err, hasSubscription, subscription)-> hasSubscription.should.equal true done() it 'should return the subscription', (done) -> - @LimitationsManager.userHasSubscription @user, (err, hasSubscription, subscription)=> + @LimitationsManager.userHasV2Subscription @user, (err, hasSubscription, subscription)=> subscription.should.deep.equal @fakeSubscription done() @@ -248,37 +249,55 @@ describe "LimitationsManager", -> describe "userHasSubscriptionOrIsGroupMember", -> beforeEach -> - @LimitationsManager.userIsMemberOfGroupSubscription = sinon.stub() - @LimitationsManager.userHasSubscription = sinon.stub() + @LimitationsManager.userIsMemberOfGroupSubscription = sinon.stub().yields(null, false) + @LimitationsManager.userHasV2Subscription = sinon.stub().yields(null, false) + @LimitationsManager.userHasV1SubscriptionOrTeam = sinon.stub().yields(null, false) - it "should return true if both are true", (done)-> - @LimitationsManager.userIsMemberOfGroupSubscription.callsArgWith(1, null, true) - @LimitationsManager.userHasSubscription.callsArgWith(1, null, true) + it "should return true if userIsMemberOfGroupSubscription", (done)-> + @LimitationsManager.userIsMemberOfGroupSubscription = sinon.stub().yields(null, true) @LimitationsManager.userHasSubscriptionOrIsGroupMember @user, (err, hasSubOrIsGroupMember)-> hasSubOrIsGroupMember.should.equal true done() - it "should return true if one is true", (done)-> - @LimitationsManager.userIsMemberOfGroupSubscription.callsArgWith(1, null, true) - @LimitationsManager.userHasSubscription.callsArgWith(1, null, false) + it "should return true if userHasV2Subscription", (done)-> + @LimitationsManager.userHasV2Subscription = sinon.stub().yields(null, true) @LimitationsManager.userHasSubscriptionOrIsGroupMember @user, (err, hasSubOrIsGroupMember)-> hasSubOrIsGroupMember.should.equal true done() - it "should return true if other is true", (done)-> - @LimitationsManager.userIsMemberOfGroupSubscription.callsArgWith(1, null, false) - @LimitationsManager.userHasSubscription.callsArgWith(1, null, true) + it "should return true if userHasV1SubscriptionOrTeam", (done)-> + @LimitationsManager.userHasV1SubscriptionOrTeam = sinon.stub().yields(null, true) @LimitationsManager.userHasSubscriptionOrIsGroupMember @user, (err, hasSubOrIsGroupMember)-> hasSubOrIsGroupMember.should.equal true done() - it "should return false if both are false", (done)-> - @LimitationsManager.userIsMemberOfGroupSubscription.callsArgWith(1, null, false) - @LimitationsManager.userHasSubscription.callsArgWith(1, null, false) + it "should return false if none are true", (done)-> @LimitationsManager.userHasSubscriptionOrIsGroupMember @user, (err, hasSubOrIsGroupMember)-> hasSubOrIsGroupMember.should.equal false done() + describe "userHasV1OrV2Subscription", -> + beforeEach -> + @LimitationsManager.userHasV2Subscription = sinon.stub().yields(null, false) + @LimitationsManager.userHasV1Subscription = sinon.stub().yields(null, false) + + it "should return true if userHasV2Subscription", (done)-> + @LimitationsManager.userHasV2Subscription = sinon.stub().yields(null, true) + @LimitationsManager.userHasV1OrV2Subscription @user, (err, hasSub)-> + hasSub.should.equal true + done() + + it "should return true if userHasV1Subscription", (done)-> + @LimitationsManager.userHasV1Subscription = sinon.stub().yields(null, true) + @LimitationsManager.userHasV1OrV2Subscription @user, (err, hasSub)-> + hasSub.should.equal true + done() + + it "should return false if none are true", (done)-> + @LimitationsManager.userHasV1OrV2Subscription @user, (err, hasSub)-> + hasSub.should.equal false + done() + describe "hasGroupMembersLimitReached", -> beforeEach -> @@ -309,3 +328,55 @@ describe "LimitationsManager", -> @LimitationsManager.hasGroupMembersLimitReached @user_id, (err, limitReached)-> limitReached.should.equal true done() + + describe 'userHasV1Subscription', -> + it 'should return true if v1 returns has_subscription = true', (done) -> + @V1SubscriptionManager.getSubscriptionsFromV1 = sinon.stub().yields(null, { has_subscription: true }) + @LimitationsManager.userHasV1Subscription @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', (done) -> + @V1SubscriptionManager.getSubscriptionsFromV1 = sinon.stub().yields(null, { has_subscription: false }) + @LimitationsManager.userHasV1Subscription @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.userHasV1Subscription @user, (error, result) => + @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() + From 28f51cd4b4bf11ff001b3da5727682b20a8dffaa Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 7 Jun 2018 13:32:15 +0100 Subject: [PATCH 5/8] Update SubscriptionController unit tests --- .../SubscriptionControllerTests.coffee | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/services/web/test/unit/coffee/Subscription/SubscriptionControllerTests.coffee b/services/web/test/unit/coffee/Subscription/SubscriptionControllerTests.coffee index 0d1929d46a..0fb9344108 100644 --- a/services/web/test/unit/coffee/Subscription/SubscriptionControllerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/SubscriptionControllerTests.coffee @@ -40,7 +40,8 @@ describe "SubscriptionController", -> @LimitationsManager = userHasSubscriptionOrIsGroupMember: sinon.stub() - userHasSubscription : sinon.stub() + userHasV1OrV2Subscription : sinon.stub() + userHasV2Subscription: sinon.stub() @SubscriptionViewModelBuilder = buildUsersSubscriptionViewModel:sinon.stub().callsArgWith(1, null, @activeRecurlySubscription) @@ -163,7 +164,7 @@ describe "SubscriptionController", -> describe "with a user without a subscription", -> beforeEach -> - @LimitationsManager.userHasSubscription.callsArgWith(1, null, false) + @LimitationsManager.userHasV1OrV2Subscription.callsArgWith(1, null, false) @PlansLocator.findLocalPlanInSettings.returns({}) describe "with a valid plan code", -> @@ -176,7 +177,7 @@ describe "SubscriptionController", -> describe "with a user with subscription", -> it "should redirect to the subscription dashboard", (done)-> - @LimitationsManager.userHasSubscription.callsArgWith(1, null, true) + @LimitationsManager.userHasV1OrV2Subscription.callsArgWith(1, null, true) @res.redirect = (url)=> url.should.equal "/user/subscription" done() @@ -184,7 +185,7 @@ describe "SubscriptionController", -> describe "with an invalid plan code", -> it "should redirect to the subscription dashboard", (done)-> - @LimitationsManager.userHasSubscription.callsArgWith(1, null, false) + @LimitationsManager.userHasV1OrV2Subscription.callsArgWith(1, null, false) @PlansLocator.findLocalPlanInSettings.returns(null) @res.redirect = (url)=> url.should.equal "/user/subscription" @@ -193,7 +194,7 @@ describe "SubscriptionController", -> describe "which currency to use", -> beforeEach -> - @LimitationsManager.userHasSubscription.callsArgWith(1, null, false) + @LimitationsManager.userHasV1OrV2Subscription.callsArgWith(1, null, false) @PlansLocator.findLocalPlanInSettings.returns({}) it "should use the set currency from the query string", (done)-> @@ -221,7 +222,7 @@ describe "SubscriptionController", -> describe "with a recurly subscription already", -> it "should redirect to the subscription dashboard", (done)-> - @LimitationsManager.userHasSubscription.callsArgWith(1, null, false) + @LimitationsManager.userHasV1OrV2Subscription.callsArgWith(1, null, false) @SubscriptionHandler.validateNoSubscriptionInRecurly = sinon.stub().yields(null, false) @res.redirect = (url)=> url.should.equal "/user/subscription" @@ -334,6 +335,7 @@ describe "SubscriptionController", -> cvv:"123" @req.body.recurly_token_id = "1234" @req.body.subscriptionDetails = @subscriptionDetails + @LimitationsManager.userHasV1OrV2Subscription.yields(null, false) @SubscriptionController.createSubscription @req, @res it "should send the user and subscriptionId to the handler", (done)-> @@ -442,7 +444,7 @@ describe "SubscriptionController", -> it "should redirect to the plans page if the user does not have a subscription", (done)-> - @LimitationsManager.userHasSubscription.callsArgWith(1, null, false) + @LimitationsManager.userHasV2Subscription.callsArgWith(1, null, false) @res.redirect = (url)-> url.should.equal "/user/subscription/plans" done() @@ -451,7 +453,7 @@ describe "SubscriptionController", -> it "should pass the plan code to the view - student", (done)-> - @LimitationsManager.userHasSubscription.callsArgWith(1, null, true, {planCode:"Student free trial 14 days"}) + @LimitationsManager.userHasV2Subscription.callsArgWith(1, null, true, {planCode:"Student free trial 14 days"}) @res.render = (view, opts)-> view.should.equal "subscriptions/upgradeToAnnual" opts.planName.should.equal "student" @@ -460,7 +462,7 @@ describe "SubscriptionController", -> it "should pass the plan code to the view - collaborator", (done)-> - @LimitationsManager.userHasSubscription.callsArgWith(1, null, true, {planCode:"free trial for Collaborator free trial 14 days"}) + @LimitationsManager.userHasV2Subscription.callsArgWith(1, null, true, {planCode:"free trial for Collaborator free trial 14 days"}) @res.render = (view, opts)-> opts.planName.should.equal "collaborator" done() @@ -468,7 +470,7 @@ describe "SubscriptionController", -> it "should pass annual as the plan name if the user is already on an annual plan", (done)-> - @LimitationsManager.userHasSubscription.callsArgWith(1, null, true, {planCode:"student annual with free trial"}) + @LimitationsManager.userHasV2Subscription.callsArgWith(1, null, true, {planCode:"student annual with free trial"}) @res.render = (view, opts)-> opts.planName.should.equal "annual" done() From 6e2ec2f982e4bde69ef0955bb09947d044a897cd Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 7 Jun 2018 17:16:21 +0100 Subject: [PATCH 6/8] Update remaining unit tests --- .../SubscriptionHandlerTests.coffee | 17 ++++++++--------- .../SubscriptionViewModelBuilderTests.coffee | 6 +++++- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/services/web/test/unit/coffee/Subscription/SubscriptionHandlerTests.coffee b/services/web/test/unit/coffee/Subscription/SubscriptionHandlerTests.coffee index 1e2cec4bfa..bc3488f1d8 100644 --- a/services/web/test/unit/coffee/Subscription/SubscriptionHandlerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/SubscriptionHandlerTests.coffee @@ -52,8 +52,7 @@ describe "SubscriptionHandler", -> startFreeTrial: sinon.stub().callsArgWith(1) @LimitationsManager = - userHasSubscription: sinon.stub() - userHasSubscriptionOrFreeTrial: sinon.stub() + userHasV2Subscription: sinon.stub() @EmailHandler = sendEmail:sinon.stub() @@ -110,7 +109,7 @@ describe "SubscriptionHandler", -> describe "with a valid plan code", -> beforeEach (done) -> @plan_code = "collaborator" - @LimitationsManager.userHasSubscription.callsArgWith(1, null, true, @subscription) + @LimitationsManager.userHasV2Subscription.callsArgWith(1, null, true, @subscription) @SubscriptionHandler.updateSubscription(@user, @plan_code, null, done) it "should update the subscription", -> @@ -129,7 +128,7 @@ describe "SubscriptionHandler", -> describe "with a user without a subscription", -> beforeEach (done) -> - @LimitationsManager.userHasSubscription.callsArgWith(1, null, false) + @LimitationsManager.userHasV2Subscription.callsArgWith(1, null, false) @SubscriptionHandler.updateSubscription(@user, @plan_code, null, done) it "should redirect to the subscription dashboard", -> @@ -140,7 +139,7 @@ describe "SubscriptionHandler", -> beforeEach (done) -> @plan_code = "collaborator" @coupon_code = "1231312" - @LimitationsManager.userHasSubscription.callsArgWith(1, null, true, @subscription) + @LimitationsManager.userHasV2Subscription.callsArgWith(1, null, true, @subscription) @SubscriptionHandler.updateSubscription(@user, @plan_code, @coupon_code, done) it "should get the users account", -> @@ -158,7 +157,7 @@ describe "SubscriptionHandler", -> describe "cancelSubscription", -> describe "with a user without a subscription", -> beforeEach (done) -> - @LimitationsManager.userHasSubscription.callsArgWith(1, null, false, @subscription) + @LimitationsManager.userHasV2Subscription.callsArgWith(1, null, false, @subscription) @SubscriptionHandler.cancelSubscription @user, done @@ -167,7 +166,7 @@ describe "SubscriptionHandler", -> describe "with a user with a subscription", -> beforeEach (done) -> - @LimitationsManager.userHasSubscription.callsArgWith(1, null, true, @subscription) + @LimitationsManager.userHasV2Subscription.callsArgWith(1, null, true, @subscription) @SubscriptionHandler.cancelSubscription @user, done it "should cancel the subscription", -> @@ -180,7 +179,7 @@ describe "SubscriptionHandler", -> describe "reactiveRecurlySubscription", -> describe "with a user without a subscription", -> beforeEach (done) -> - @LimitationsManager.userHasSubscription.callsArgWith(1, null, false, @subscription) + @LimitationsManager.userHasV2Subscription.callsArgWith(1, null, false, @subscription) @SubscriptionHandler.reactivateSubscription @user, done it "should redirect to the subscription dashboard", -> @@ -188,7 +187,7 @@ describe "SubscriptionHandler", -> describe "with a user with a subscription", -> beforeEach (done) -> - @LimitationsManager.userHasSubscription.callsArgWith(1, null, true, @subscription) + @LimitationsManager.userHasV2Subscription.callsArgWith(1, null, true, @subscription) @SubscriptionHandler.reactivateSubscription @user, done it "should reactivate the subscription", -> diff --git a/services/web/test/unit/coffee/Subscription/SubscriptionViewModelBuilderTests.coffee b/services/web/test/unit/coffee/Subscription/SubscriptionViewModelBuilderTests.coffee index 1f2c3f8808..51ed2dbebd 100644 --- a/services/web/test/unit/coffee/Subscription/SubscriptionViewModelBuilderTests.coffee +++ b/services/web/test/unit/coffee/Subscription/SubscriptionViewModelBuilderTests.coffee @@ -42,6 +42,7 @@ describe 'SubscriptionViewModelBuilder', -> "./SubscriptionLocator": @SubscriptionLocator = {} "./SubscriptionFormatters": @SubscriptionFormatters "./LimitationsManager": {} + "./V1SubscriptionManager": @V1SubscriptionManager = {} "logger-sharelatex": log:-> warn:-> @@ -50,13 +51,15 @@ describe 'SubscriptionViewModelBuilder', -> @PlansLocator.findLocalPlanInSettings = sinon.stub().returns(@plan) @SubscriptionLocator.getUsersSubscription = sinon.stub().callsArgWith(1, null, mockSubscription) @SubscriptionLocator.getMemberSubscriptions = sinon.stub().callsArgWith(1, null, null) + @V1SubscriptionManager.getSubscriptionsFromV1 = sinon.stub().yields(null, @mockV1Sub = ['mock-v1-subs']) it 'builds the user view model', -> - callback = (error, subscription, memberSubscriptions, billingDetailsLink) => + callback = (error, subscription, memberSubscriptions, billingDetailsLink, v1Sub) => @error = error @subscription = subscription @memberSubscriptions = memberSubscriptions @billingDetailsLink = billingDetailsLink + @v1Sub = v1Sub @builder.buildUsersSubscriptionViewModel(@user, callback) @@ -64,3 +67,4 @@ describe 'SubscriptionViewModelBuilder', -> @subscription.nextPaymentDueAt.should.eq 'Formatted date' @subscription.price.should.eq 'Formatted price' @billingDetailsLink.should.eq "https://example.com.recurly.com/account/billing_info/edit?ht=hosted_login_token" + @v1Sub.should.deep.equal @mockV1Sub \ No newline at end of file From a417dc8ff4e7d7aa173ec2479fb5f45845bcec13 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 7 Jun 2018 17:18:21 +0100 Subject: [PATCH 7/8] v1Subscription -> v1Subscriptions --- .../Features/Subscription/SubscriptionController.coffee | 6 +++--- services/web/app/views/subscriptions/dashboard.pug | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee index a553d0303f..25086ffcf7 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee @@ -101,9 +101,9 @@ module.exports = SubscriptionController = logger.log user: user, "redirecting to plans" res.redirect "/user/subscription/plans" else - SubscriptionViewModelBuilder.buildUsersSubscriptionViewModel user, (error, subscription, groupSubscriptions, billingDetailsLink, v1Subscription) -> + SubscriptionViewModelBuilder.buildUsersSubscriptionViewModel user, (error, subscription, groupSubscriptions, billingDetailsLink, v1Subscriptions) -> return next(error) if error? - logger.log {user, subscription, hasSubOrIsGroupMember, groupSubscriptions, billingDetailsLink, v1Subscription}, "showing subscription dashboard" + logger.log {user, subscription, hasSubOrIsGroupMember, groupSubscriptions, billingDetailsLink, v1Subscriptions}, "showing subscription dashboard" plans = SubscriptionViewModelBuilder.buildViewModel() res.render "subscriptions/dashboard", title: "your_subscription" @@ -116,7 +116,7 @@ module.exports = SubscriptionController = user:user saved_billing_details: req.query.saved_billing_details? billingDetailsLink: billingDetailsLink - v1Subscription: v1Subscription + v1Subscriptions: v1Subscriptions userCustomSubscriptionPage: (req, res, next)-> user = AuthenticationController.getSessionUser(req) diff --git a/services/web/app/views/subscriptions/dashboard.pug b/services/web/app/views/subscriptions/dashboard.pug index e6de2cee5c..cc07e2845b 100644 --- a/services/web/app/views/subscriptions/dashboard.pug +++ b/services/web/app/views/subscriptions/dashboard.pug @@ -118,15 +118,15 @@ block content a(href="/subscription/group").btn.btn-primary !{translate("manage_group")} hr - if v1Subscription.has_subscription + if v1Subscriptions.has_subscription p | You are subscribed to Overleaf through Overleaf v1 p a.btn.btn-primary(href=settings.overleaf.host+"/users/edit#status") Manage v1 Subscription hr - if v1Subscription.teams && v1Subscription.teams.length > 0 - for team in v1Subscription.teams + if v1Subscriptions.teams && v1Subscriptions.teams.length > 0 + for team in v1Subscriptions.teams p | You are a member of the Overleaf v1 team: #{team.name} p From b00a79ff1c483da0d065eb6baf95b0470f7e6825 Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 12 Jun 2018 13:51:42 +0100 Subject: [PATCH 8/8] Redirect to v1 plans page from v2 --- .../coffee/Features/Subscription/SubscriptionRouter.coffee | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee index a8c715d466..9e9cc702f5 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee @@ -9,7 +9,10 @@ module.exports = apply: (webRouter, privateApiRouter, publicApiRouter) -> return unless Settings.enableSubscriptions - if !Settings.disablePlansPage + if Settings.overleaf? + webRouter.get '/user/subscription/plans', (req, res) -> + res.redirect "#{Settings.overleaf.host}/plans" + else webRouter.get '/user/subscription/plans', SubscriptionController.plansPage webRouter.get '/user/subscription', AuthenticationController.requireLogin(), SubscriptionController.userSubscriptionPage