From caa65dddc19d4bc2061598a70a7b87d92efcaa9b Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Thu, 23 Nov 2017 16:16:13 +0000 Subject: [PATCH 1/4] Send user to hosted recurly billing-details page. And remove our update-billing-details form, the APIs for which will be deprecated soon. --- .../SubscriptionController.coffee | 36 +++++++++++-------- .../subscriptions/edit-billing-details.pug | 21 +++-------- .../SubscriptionControllerTests.coffee | 15 +++++--- 3 files changed, 38 insertions(+), 34 deletions(-) diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee index cc3013a42d..d37e8e02fe 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee @@ -127,24 +127,32 @@ module.exports = SubscriptionController = editBillingDetailsPage: (req, res, next) -> user = AuthenticationController.getSessionUser(req) - LimitationsManager.userHasSubscription user, (err, hasSubscription)-> + LimitationsManager.userHasSubscription user, (err, hasSubscription, subscription)-> return next(err) if err? if !hasSubscription res.redirect "/user/subscription" else - RecurlyWrapper.sign { - account_code: user._id - }, (error, signature) -> - return next(error) if error? - res.render "subscriptions/edit-billing-details", - title : "update_billing_details" - recurlyConfig: JSON.stringify - currency: "USD" - subdomain: Settings.apis.recurly.subdomain - signature : signature - successURL : "#{Settings.siteUrl}/user/subscription/billing-details/update" - user : - id : user._id + RecurlyWrapper.getSubscription subscription.recurlySubscription_id, + includeAccount: true, + (err, usersSubscription)-> + account = usersSubscription.account + hostedLoginToken = account.hosted_login_token + if !hostedLoginToken + err = new Error('no hosted_login_token on recurly account') + return next(err) + subdomain = Settings?.apis?.recurly?.subdomain + if !subdomain + err = new Error('recurly subdomain not configured') + return next(err) + link = "https://" + + subdomain + + ".recurly.com/account/billing_info/edit?ht=" + + hostedLoginToken + res.render "subscriptions/edit-billing-details", + title: "update_billing_details" + hostedBillingDetailsPageLink: link + user: + id: user._id updateBillingDetails: (req, res, next) -> res.redirect "/user/subscription?saved_billing_details=true" diff --git a/services/web/app/views/subscriptions/edit-billing-details.pug b/services/web/app/views/subscriptions/edit-billing-details.pug index cc41e0a4b9..0b40be7eb7 100644 --- a/services/web/app/views/subscriptions/edit-billing-details.pug +++ b/services/web/app/views/subscriptions/edit-billing-details.pug @@ -1,7 +1,5 @@ extends ../layout -block scripts - script(src=buildJsPath('libs/recurly.min.js', {fingerprint:false})) block content .content.content-alt @@ -11,17 +9,8 @@ block content .card .page-header h1.text-centered #{translate("update_your_billing_details")} - #billingDetailsForm #{translate("loading_billing_form")}... - - - script(type="text/javascript"). - Recurly.config(!{recurlyConfig}) - Recurly.buildBillingInfoUpdateForm({ - target : "#billingDetailsForm", - successURL : "#{successURL}?_csrf=#{csrfToken}&origin=editBillingDetails", - signature : "!{signature}", - accountCode : "#{user.id}" - }); - - - + div.text-center + a.btn.btn-primary.btn-lg( + href=hostedBillingDetailsPageLink + target="_blank" + ) #{translate('open_your_billing_details_page')} diff --git a/services/web/test/UnitTests/coffee/Subscription/SubscriptionControllerTests.coffee b/services/web/test/UnitTests/coffee/Subscription/SubscriptionControllerTests.coffee index b95fba1cdd..428ed58f7a 100644 --- a/services/web/test/UnitTests/coffee/Subscription/SubscriptionControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Subscription/SubscriptionControllerTests.coffee @@ -55,7 +55,7 @@ describe "SubscriptionController", -> collaborator:"COLLABORATORCODEHERE" apis: recurly: - subdomain:"sl.recurly.com" + subdomain:"sl" siteUrl: "http://de.sharelatex.dev:3000" gaExperiments:{} @GeoIpLookup = @@ -122,7 +122,12 @@ describe "SubscriptionController", -> describe "editBillingDetailsPage", -> describe "with a user with a subscription", -> beforeEach (done) -> - @LimitationsManager.userHasSubscription.callsArgWith(1, null, true) + @settings.apis.recurly.subdomain = 'test' + @userSub = {account: {hosted_login_token: 'abcd'}} + @LimitationsManager.userHasSubscription + .callsArgWith(1, null, true, @activeRecurlySubscription) + @RecurlyWrapper.getSubscription = sinon.stub() + .callsArgWith(2, null, @userSub) @user._id = @activeRecurlySubscription.account.account_code @res.callback = done @SubscriptionController.editBillingDetailsPage(@req, @res) @@ -132,8 +137,10 @@ describe "SubscriptionController", -> @res.renderedTemplate.should.equal "subscriptions/edit-billing-details" it "should set the correct variables for the template", -> - should.exist @res.renderedVariables.signature - @res.renderedVariables.successURL.should.equal "#{@settings.siteUrl}/user/subscription/billing-details/update" + should.exist @res.renderedVariables.hostedBillingDetailsPageLink + @res.renderedVariables.hostedBillingDetailsPageLink.should.equal( + "https://test.recurly.com/account/billing_info/edit?ht=abcd" + ) @res.renderedVariables.user.id.should.equal @user._id describe "with a user without subscription", -> From c121653dc45f593865812d128ced5d0e80834837 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Thu, 23 Nov 2017 16:22:57 +0000 Subject: [PATCH 2/4] Add error-handling to attempt to get user subscription --- .../coffee/Features/Subscription/SubscriptionController.coffee | 1 + 1 file changed, 1 insertion(+) diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee index d37e8e02fe..2a48e8e93c 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee @@ -135,6 +135,7 @@ module.exports = SubscriptionController = RecurlyWrapper.getSubscription subscription.recurlySubscription_id, includeAccount: true, (err, usersSubscription)-> + return next(err) if err? account = usersSubscription.account hostedLoginToken = account.hosted_login_token if !hostedLoginToken From 22c163c60ac31adfa275860a8c3efdff2867c7ab Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Tue, 28 Nov 2017 11:40:48 +0000 Subject: [PATCH 3/4] Move the recurly-billing-details link up to main subscription page --- .../SubscriptionController.coffee | 47 +++++++++++++------ .../web/app/views/subscriptions/dashboard.pug | 5 +- .../SubscriptionControllerTests.coffee | 12 +++-- 3 files changed, 46 insertions(+), 18 deletions(-) diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee index 2a48e8e93c..e75c123c2d 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee @@ -97,20 +97,39 @@ module.exports = SubscriptionController = logger.log user: user, "redirecting to plans" res.redirect "/user/subscription/plans" else - SubscriptionViewModelBuilder.buildUsersSubscriptionViewModel user, (error, subscription, groupSubscriptions) -> - return next(error) if error? - logger.log user: user, subscription:subscription, hasSubOrIsGroupMember:hasSubOrIsGroupMember, groupSubscriptions:groupSubscriptions, "showing subscription dashboard" - plans = SubscriptionViewModelBuilder.buildViewModel() - res.render "subscriptions/dashboard", - title: "your_subscription" - recomendedCurrency: subscription?.currency - taxRate:subscription?.taxRate - plans: plans - subscription: subscription || {} - groupSubscriptions: groupSubscriptions - subscriptionTabActive: true - user:user - saved_billing_details: req.query.saved_billing_details? + RecurlyWrapper.getSubscription subscription.recurlySubscription_id, + includeAccount: true, + (err, usersSubscription)-> + # always render the page, but skip the recurly link if + # we can't get it for some reason + if err? + logger.err {err, userId: user._id}, "error getting billing details link from recurly, proceeding" + hostedLoginToken = usersSubscription?.account?.hosted_login_token + recurlySubdomain = Settings?.apis?.recurly?.subdomain + if err? || !hostedLoginToken || !recurlySubdomain + billingDetailsLink = null + else + billingDetailsLink = [ + "https://", + recurlySubdomain, + ".recurly.com/account/billing_info/edit?ht=", + hostedLoginToken + ].join("") + SubscriptionViewModelBuilder.buildUsersSubscriptionViewModel user, (error, subscription, groupSubscriptions) -> + return next(error) if error? + logger.log {user, subscription, hasSubOrIsGroupMember, groupSubscriptions, billingDetailsLink}, "showing subscription dashboard" + plans = SubscriptionViewModelBuilder.buildViewModel() + res.render "subscriptions/dashboard", + title: "your_subscription" + recomendedCurrency: subscription?.currency + taxRate:subscription?.taxRate + plans: plans + subscription: subscription || {} + groupSubscriptions: groupSubscriptions + subscriptionTabActive: true + user:user + saved_billing_details: req.query.saved_billing_details? + billingDetailsLink: billingDetailsLink 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 fb4f834c34..0db4c05423 100644 --- a/services/web/app/views/subscriptions/dashboard.pug +++ b/services/web/app/views/subscriptions/dashboard.pug @@ -61,7 +61,10 @@ block content p !{translate("next_payment_of_x_collectected_on_y", {paymentAmmount:"" + subscription.price + "", collectionDate:"" + subscription.nextPaymentDueAt + ""})} p.pull-right p - a(href="/user/subscription/billing-details/edit").btn.btn-info #{translate("update_your_billing_details")} + if billingDetailsLink + a(href=billingDetailsLink, target="_blank").btn.btn-info #{translate("update_your_billing_details")} + else + a(href=billingDetailsLink, disabled).btn.btn-info.btn-disabled #{translate("update_your_billing_details")} |   a(href, ng-click="switchToCancelationView()").btn.btn-primary !{translate("cancel_your_subscription")} when "canceled" diff --git a/services/web/test/UnitTests/coffee/Subscription/SubscriptionControllerTests.coffee b/services/web/test/UnitTests/coffee/Subscription/SubscriptionControllerTests.coffee index 428ed58f7a..71fed6d8f0 100644 --- a/services/web/test/UnitTests/coffee/Subscription/SubscriptionControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Subscription/SubscriptionControllerTests.coffee @@ -44,6 +44,7 @@ describe "SubscriptionController", -> @RecurlyWrapper = sign: sinon.stub().callsArgWith(1, null, "somthing") + getSubscription: sinon.stub().callsArgWith(2, null, {}) @SubscriptionViewModelBuilder = buildUsersSubscriptionViewModel:sinon.stub().callsArgWith(1, null, @activeRecurlySubscription) @@ -262,7 +263,12 @@ describe "SubscriptionController", -> describe "with an existing subscription", -> beforeEach (done)-> @res.callback = done - @LimitationsManager.userHasSubscriptionOrIsGroupMember.callsArgWith(1, null, true) + @settings.apis.recurly.subdomain = 'test' + @userSub = {account: {hosted_login_token: 'abcd'}} + @RecurlyWrapper.getSubscription = sinon.stub() + .callsArgWith(2, null, @userSub) + @LimitationsManager.userHasSubscriptionOrIsGroupMember + .callsArgWith(1, null, true, {}) @SubscriptionController.userSubscriptionPage @req, @res @@ -273,7 +279,7 @@ describe "SubscriptionController", -> beforeEach (done) -> @res.callback = done @SubscriptionViewModelBuilder.buildUsersSubscriptionViewModel.callsArgWith(1, null, @activeRecurlySubscription) - @LimitationsManager.userHasSubscriptionOrIsGroupMember.callsArgWith(1, null, true) + @LimitationsManager.userHasSubscriptionOrIsGroupMember.callsArgWith(1, null, true, {}) @SubscriptionController.userSubscriptionPage @req, @res it "should render the dashboard", (done)-> @@ -288,7 +294,7 @@ describe "SubscriptionController", -> beforeEach (done) -> @res.callback = done @SubscriptionViewModelBuilder.buildUsersSubscriptionViewModel.callsArgWith(1, null, @activeRecurlySubscription) - @LimitationsManager.userHasSubscriptionOrIsGroupMember.callsArgWith(1, null, true) + @LimitationsManager.userHasSubscriptionOrIsGroupMember.callsArgWith(1, null, true, {}) @SubscriptionController.userSubscriptionPage @req, @res it "should render the dashboard", -> From cc9986cefd77946b1cb9cb1bf3952588b48e45ac Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Tue, 28 Nov 2017 11:52:14 +0000 Subject: [PATCH 4/4] Remove the obsolete update-billing-details page --- .../SubscriptionController.coffee | 34 ------------------ .../Subscription/SubscriptionRouter.coffee | 2 -- .../subscriptions/edit-billing-details.pug | 16 --------- .../SubscriptionControllerTests.coffee | 35 ------------------- 4 files changed, 87 deletions(-) delete mode 100644 services/web/app/views/subscriptions/edit-billing-details.pug diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee index e75c123c2d..e665aa517d 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee @@ -143,40 +143,6 @@ module.exports = SubscriptionController = title: "your_subscription" subscription: subscription - - editBillingDetailsPage: (req, res, next) -> - user = AuthenticationController.getSessionUser(req) - LimitationsManager.userHasSubscription user, (err, hasSubscription, subscription)-> - return next(err) if err? - if !hasSubscription - res.redirect "/user/subscription" - else - RecurlyWrapper.getSubscription subscription.recurlySubscription_id, - includeAccount: true, - (err, usersSubscription)-> - return next(err) if err? - account = usersSubscription.account - hostedLoginToken = account.hosted_login_token - if !hostedLoginToken - err = new Error('no hosted_login_token on recurly account') - return next(err) - subdomain = Settings?.apis?.recurly?.subdomain - if !subdomain - err = new Error('recurly subdomain not configured') - return next(err) - link = "https://" + - subdomain + - ".recurly.com/account/billing_info/edit?ht=" + - hostedLoginToken - res.render "subscriptions/edit-billing-details", - title: "update_billing_details" - hostedBillingDetailsPageLink: link - user: - id: user._id - - updateBillingDetails: (req, res, next) -> - res.redirect "/user/subscription?saved_billing_details=true" - createSubscription: (req, res, next)-> user = AuthenticationController.getSessionUser(req) recurly_token_id = req.body.recurly_token_id diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee index 9a3e34ce5a..d6d049f5bf 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee @@ -15,8 +15,6 @@ module.exports = webRouter.get '/user/subscription/new', AuthenticationController.requireLogin(), SubscriptionController.paymentPage - webRouter.get '/user/subscription/billing-details/edit', AuthenticationController.requireLogin(), SubscriptionController.editBillingDetailsPage - webRouter.post '/user/subscription/billing-details/update', AuthenticationController.requireLogin(), SubscriptionController.updateBillingDetails webRouter.get '/user/subscription/thank-you', AuthenticationController.requireLogin(), SubscriptionController.successful_subscription diff --git a/services/web/app/views/subscriptions/edit-billing-details.pug b/services/web/app/views/subscriptions/edit-billing-details.pug deleted file mode 100644 index 0b40be7eb7..0000000000 --- a/services/web/app/views/subscriptions/edit-billing-details.pug +++ /dev/null @@ -1,16 +0,0 @@ -extends ../layout - - -block content - .content.content-alt - .container - .row - .col-md-6.col-md-offset-3 - .card - .page-header - h1.text-centered #{translate("update_your_billing_details")} - div.text-center - a.btn.btn-primary.btn-lg( - href=hostedBillingDetailsPageLink - target="_blank" - ) #{translate('open_your_billing_details_page')} diff --git a/services/web/test/UnitTests/coffee/Subscription/SubscriptionControllerTests.coffee b/services/web/test/UnitTests/coffee/Subscription/SubscriptionControllerTests.coffee index 71fed6d8f0..30019fadfd 100644 --- a/services/web/test/UnitTests/coffee/Subscription/SubscriptionControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Subscription/SubscriptionControllerTests.coffee @@ -119,41 +119,6 @@ describe "SubscriptionController", -> @UserGetter.getUser.callCount.should.equal 0 done() - - describe "editBillingDetailsPage", -> - describe "with a user with a subscription", -> - beforeEach (done) -> - @settings.apis.recurly.subdomain = 'test' - @userSub = {account: {hosted_login_token: 'abcd'}} - @LimitationsManager.userHasSubscription - .callsArgWith(1, null, true, @activeRecurlySubscription) - @RecurlyWrapper.getSubscription = sinon.stub() - .callsArgWith(2, null, @userSub) - @user._id = @activeRecurlySubscription.account.account_code - @res.callback = done - @SubscriptionController.editBillingDetailsPage(@req, @res) - - it "should render the edit billing details page", -> - @res.rendered.should.equal true - @res.renderedTemplate.should.equal "subscriptions/edit-billing-details" - - it "should set the correct variables for the template", -> - should.exist @res.renderedVariables.hostedBillingDetailsPageLink - @res.renderedVariables.hostedBillingDetailsPageLink.should.equal( - "https://test.recurly.com/account/billing_info/edit?ht=abcd" - ) - @res.renderedVariables.user.id.should.equal @user._id - - describe "with a user without subscription", -> - beforeEach (done) -> - @res.callback = done - @LimitationsManager.userHasSubscription.callsArgWith(1, null, false) - @SubscriptionController.reactivateSubscription @req, @res - - it "should redirect to the subscription dashboard", -> - @res.redirected.should.equal true - @res.redirectedTo.should.equal "/user/subscription" - describe "paymentPage", -> beforeEach -> @req.headers = {}