From 72af966c9c725660fc925fa56afd93a19b23ca45 Mon Sep 17 00:00:00 2001 From: Thomas Date: Tue, 27 Apr 2021 16:17:39 +0200 Subject: [PATCH] Schedule subscription downgrades to occur at the current term end (#3801) * Schedule subscription downgrades to occur at the current term end. If the plan is a downgrade, schedule the subscription change for term end. Use Recurly v3 API subscription change event instead of v2 update subscription. * Add ability for user to revert a pending subscription change In the case where a user has downgraded, but has since decided they'd rather stay on their current plan, we need a way to let them revert. It isn't enough to re-use a subscription change, because Recurly sees it as an attempt to make a change from the current plan to itself. Instead, we use a new dialog and call a new endpoint that has the specific intent of reverting the pending plan change, by calling the removeSubscriptionChange recurly client method. * Add message prompting users to contact support for immediate changes We're showing this in the confirmation modal for a plan change that would occur in the future, and and on the subscription page if a pending change is due. Most users shouldn't need this, but it should help them out if they find an edge case like moving from eg. Student (Annual) to Professional (Monthly) and were expecting to be "upgraded" immediately. GitOrigin-RevId: c5be0efbeb8568ed9caa941aadcef6f6db65c420 --- services/web/app.js | 4 + .../Features/Subscription/GroupPlansData.js | 1 + .../src/Features/Subscription/PlansLocator.js | 36 ++-- .../Features/Subscription/RecurlyClient.js | 61 ++++-- .../Subscription/SubscriptionController.js | 26 ++- .../Subscription/SubscriptionHandler.js | 69 ++++++- .../Subscription/SubscriptionHelper.js | 11 ++ .../Subscription/SubscriptionRouter.js | 5 + .../SubscriptionViewModelBuilder.js | 58 +++++- .../web/app/views/subscriptions/dashboard.pug | 1 + .../dashboard/_change_plans_mixins.pug | 9 +- .../_personal_subscription_recurly.pug | 25 +++ .../js/main/subscription-dashboard.js | 39 ++++ services/web/locales/en.json | 7 + .../src/Subscription/PlansLocatorTests.js | 51 +++++ .../src/Subscription/RecurlyClientTests.js | 146 ++++++++++++++ .../Subscription/SubscriptionHandlerTests.js | 182 ++++++++++++++---- .../Subscription/SubscriptionHelperTests.js | 50 +++++ 18 files changed, 695 insertions(+), 86 deletions(-) create mode 100644 services/web/app/src/Features/Subscription/SubscriptionHelper.js create mode 100644 services/web/test/unit/src/Subscription/PlansLocatorTests.js create mode 100644 services/web/test/unit/src/Subscription/SubscriptionHelperTests.js diff --git a/services/web/app.js b/services/web/app.js index f1ee079cd3..f6ccd2cb17 100644 --- a/services/web/app.js +++ b/services/web/app.js @@ -13,6 +13,7 @@ const metrics = require('@overleaf/metrics') metrics.initialize(process.env.METRICS_APP_NAME || 'web') const Settings = require('settings-sharelatex') const logger = require('logger-sharelatex') +const PlansLocator = require('./app/src/Features/Subscription/PlansLocator') logger.initialize(process.env.METRICS_APP_NAME || 'web') logger.logger.serializers.user = require('./app/src/infrastructure/LoggerSerializers').user logger.logger.serializers.docs = require('./app/src/infrastructure/LoggerSerializers').docs @@ -43,6 +44,9 @@ if (!module.parent) { if (!process.env.WEB_API_USER || !process.env.WEB_API_PASSWORD) { throw new Error('No API user and password provided') } + + PlansLocator.ensurePlansAreSetupCorrectly() + Promise.all([mongodb.waitForDb(), mongoose.connectionPromise]) .then(() => { Server.server.listen(port, host, function () { diff --git a/services/web/app/src/Features/Subscription/GroupPlansData.js b/services/web/app/src/Features/Subscription/GroupPlansData.js index c1c4e82d44..5814e9286c 100644 --- a/services/web/app/src/Features/Subscription/GroupPlansData.js +++ b/services/web/app/src/Features/Subscription/GroupPlansData.js @@ -40,6 +40,7 @@ for (const [usage, planData] of Object.entries(groups)) { planCode )} - Group Account (${size} licenses) - ${capitalize(usage)}`, hideFromUsers: true, + price: groups[usage][planCode].USD[size], annual: true, features: Settings.features[planCode], groupPlan: true, diff --git a/services/web/app/src/Features/Subscription/PlansLocator.js b/services/web/app/src/Features/Subscription/PlansLocator.js index ee1e0625af..86e1f1e390 100644 --- a/services/web/app/src/Features/Subscription/PlansLocator.js +++ b/services/web/app/src/Features/Subscription/PlansLocator.js @@ -1,19 +1,25 @@ -// TODO: This file was created by bulk-decaffeinate. -// Sanity-check the conversion and remove this comment. -/* - * decaffeinate suggestions: - * DS101: Remove unnecessary use of Array.from - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const Settings = require('settings-sharelatex') +const logger = require('logger-sharelatex') + +function ensurePlansAreSetupCorrectly() { + Settings.plans.forEach(plan => { + if (typeof plan.price !== 'number') { + logger.fatal({ plan }, 'missing price on plan') + process.exit(1) + } + }) +} + +function findLocalPlanInSettings(planCode) { + for (let plan of Settings.plans) { + if (plan.planCode === planCode) { + return plan + } + } + return null +} module.exports = { - findLocalPlanInSettings(planCode) { - for (let plan of Array.from(Settings.plans)) { - if (plan.planCode === planCode) { - return plan - } - } - return null - }, + ensurePlansAreSetupCorrectly, + findLocalPlanInSettings, } diff --git a/services/web/app/src/Features/Subscription/RecurlyClient.js b/services/web/app/src/Features/Subscription/RecurlyClient.js index cee41dbef0..bf10dbb2c4 100644 --- a/services/web/app/src/Features/Subscription/RecurlyClient.js +++ b/services/web/app/src/Features/Subscription/RecurlyClient.js @@ -9,18 +9,6 @@ const recurlyApiKey = recurlySettings ? recurlySettings.apiKey : undefined const client = new recurly.Client(recurlyApiKey) -module.exports = { - errors: recurly.errors, - - getAccountForUserId: callbackify(getAccountForUserId), - createAccountForUserId: callbackify(createAccountForUserId), - - promises: { - getAccountForUserId, - createAccountForUserId, - }, -} - async function getAccountForUserId(userId) { try { return await client.getAccount(`code-${userId}`) @@ -51,3 +39,52 @@ async function createAccountForUserId(userId) { logger.log({ userId, account }, 'created recurly account') return account } + +async function getSubscription(subscriptionId) { + return await client.getSubscription(subscriptionId) +} + +async function changeSubscription(subscriptionId, body) { + const change = await client.createSubscriptionChange(subscriptionId, body) + logger.log( + { subscriptionId, changeId: change.id }, + 'created subscription change' + ) + return change +} + +async function changeSubscriptionByUuid(subscriptionUuid, ...args) { + return await changeSubscription('uuid-' + subscriptionUuid, ...args) +} + +async function removeSubscriptionChange(subscriptionId) { + const removed = await client.removeSubscriptionChange(subscriptionId) + logger.log({ subscriptionId }, 'removed pending subscription change') + return removed +} + +async function removeSubscriptionChangeByUuid(subscriptionUuid) { + return await removeSubscriptionChange('uuid-' + subscriptionUuid) +} + +module.exports = { + errors: recurly.errors, + + getAccountForUserId: callbackify(getAccountForUserId), + createAccountForUserId: callbackify(createAccountForUserId), + getSubscription: callbackify(getSubscription), + changeSubscription: callbackify(changeSubscription), + changeSubscriptionByUuid: callbackify(changeSubscriptionByUuid), + removeSubscriptionChange: callbackify(removeSubscriptionChange), + removeSubscriptionChangeByUuid: callbackify(removeSubscriptionChangeByUuid), + + promises: { + getSubscription, + getAccountForUserId, + createAccountForUserId, + changeSubscription, + changeSubscriptionByUuid, + removeSubscriptionChange, + removeSubscriptionChangeByUuid, + }, +} diff --git a/services/web/app/src/Features/Subscription/SubscriptionController.js b/services/web/app/src/Features/Subscription/SubscriptionController.js index b0d3c7ae56..175ecda17c 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionController.js +++ b/services/web/app/src/Features/Subscription/SubscriptionController.js @@ -163,7 +163,9 @@ module.exports = SubscriptionController = { return next(error) } const fromPlansPage = req.query.hasSubscription - const plans = SubscriptionViewModelBuilder.buildPlansList() + const plans = SubscriptionViewModelBuilder.buildPlansList( + personalSubscription ? personalSubscription.plan : undefined + ) const data = { title: 'your_subscription', plans, @@ -323,6 +325,24 @@ module.exports = SubscriptionController = { ) }, + cancelPendingSubscriptionChange(req, res, next) { + const user = AuthenticationController.getSessionUser(req) + logger.log({ user_id: user._id }, 'canceling pending subscription change') + SubscriptionHandler.cancelPendingSubscriptionChange(user, function (err) { + if (err != null) { + OError.tag( + err, + 'something went wrong canceling pending subscription change', + { + user_id: user._id, + } + ) + return next(err) + } + res.redirect('/user/subscription') + }) + }, + updateAccountEmailAddress(req, res, next) { const user = AuthenticationController.getSessionUser(req) RecurlyWrapper.updateAccountEmailAddress( @@ -340,14 +360,14 @@ module.exports = SubscriptionController = { reactivateSubscription(req, res, next) { const user = AuthenticationController.getSessionUser(req) logger.log({ user_id: user._id }, 'reactivating subscription') - return SubscriptionHandler.reactivateSubscription(user, function (err) { + SubscriptionHandler.reactivateSubscription(user, function (err) { if (err != null) { OError.tag(err, 'something went wrong reactivating subscription', { user_id: user._id, }) return next(err) } - return res.redirect('/user/subscription') + res.redirect('/user/subscription') }) }, diff --git a/services/web/app/src/Features/Subscription/SubscriptionHandler.js b/services/web/app/src/Features/Subscription/SubscriptionHandler.js index 6459fc9788..7550de9e77 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionHandler.js +++ b/services/web/app/src/Features/Subscription/SubscriptionHandler.js @@ -1,5 +1,6 @@ const async = require('async') const RecurlyWrapper = require('./RecurlyWrapper') +const RecurlyClient = require('./RecurlyClient') const { User } = require('../../models/User') const { promisifyAll } = require('../../util/promises') const logger = require('logger-sharelatex') @@ -7,6 +8,8 @@ const SubscriptionUpdater = require('./SubscriptionUpdater') const LimitationsManager = require('./LimitationsManager') const EmailHandler = require('../Email/EmailHandler') const Analytics = require('../Analytics/AnalyticsManager') +const PlansLocator = require('./PlansLocator') +const SubscriptionHelper = require('./SubscriptionHelper') const SubscriptionHandler = { validateNoSubscriptionInRecurly(userId, callback) { @@ -98,7 +101,7 @@ const SubscriptionHandler = { { includeAccount: true }, function (err, usersSubscription) { if (err != null) { - return callback(err) + return cb(err) } RecurlyWrapper.redeemCoupon( usersSubscription.account.account_code, @@ -108,21 +111,45 @@ const SubscriptionHandler = { } ) }, - cb => - RecurlyWrapper.updateSubscription( + function (cb) { + let changeAtTermEnd + const currentPlan = PlansLocator.findLocalPlanInSettings( + subscription.planCode + ) + const newPlan = PlansLocator.findLocalPlanInSettings(planCode) + if (currentPlan && newPlan) { + changeAtTermEnd = SubscriptionHelper.shouldPlanChangeAtTermEnd( + currentPlan, + newPlan + ) + } else { + logger.error( + { currentPlan: subscription.planCode, newPlan: planCode }, + 'unable to locate both plans in settings' + ) + return cb( + new Error('unable to locate both plans in settings') + ) + } + const timeframe = changeAtTermEnd ? 'term_end' : 'now' + RecurlyClient.changeSubscriptionByUuid( subscription.recurlySubscription_id, - { plan_code: planCode, timeframe: 'now' }, - function (error, recurlySubscription) { + { planCode: planCode, timeframe: timeframe }, + function (error, subscriptionChange) { if (error != null) { - return callback(error) + return cb(error) } - SubscriptionUpdater.syncSubscription( - recurlySubscription, + // v2 recurly API wants a UUID, but UUID isn't included in the subscription change response + // we got the UUID from the DB using userHasV2Subscription() - it is the only property + // we need to be able to build a 'recurlySubscription' object for syncSubscription() + SubscriptionHandler.syncSubscription( + { uuid: subscription.recurlySubscription_id }, user._id, cb ) } - ), + ) + }, ], callback ) @@ -131,6 +158,30 @@ const SubscriptionHandler = { ) }, + cancelPendingSubscriptionChange(user, callback) { + LimitationsManager.userHasV2Subscription( + user, + function (err, hasSubscription, subscription) { + if (err) { + return callback(err) + } + if (hasSubscription) { + RecurlyClient.removeSubscriptionChangeByUuid( + subscription.recurlySubscription_id, + function (error) { + if (error != null) { + return callback(error) + } + callback() + } + ) + } else { + callback() + } + } + ) + }, + cancelSubscription(user, callback) { LimitationsManager.userHasV2Subscription( user, diff --git a/services/web/app/src/Features/Subscription/SubscriptionHelper.js b/services/web/app/src/Features/Subscription/SubscriptionHelper.js new file mode 100644 index 0000000000..f3135f06b8 --- /dev/null +++ b/services/web/app/src/Features/Subscription/SubscriptionHelper.js @@ -0,0 +1,11 @@ +/** + * If the user changes to a less expensive plan, we shouldn't apply the change immediately. + * This is to avoid unintended/artifical credits on users Recurly accounts. + */ +function shouldPlanChangeAtTermEnd(oldPlan, newPlan) { + return oldPlan.price > newPlan.price +} + +module.exports = { + shouldPlanChangeAtTermEnd, +} diff --git a/services/web/app/src/Features/Subscription/SubscriptionRouter.js b/services/web/app/src/Features/Subscription/SubscriptionRouter.js index b70ebd265e..89a251ddad 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionRouter.js +++ b/services/web/app/src/Features/Subscription/SubscriptionRouter.js @@ -88,6 +88,11 @@ module.exports = { AuthenticationController.requireLogin(), SubscriptionController.updateSubscription ) + webRouter.post( + '/user/subscription/cancel-pending', + AuthenticationController.requireLogin(), + SubscriptionController.cancelPendingSubscriptionChange + ) webRouter.post( '/user/subscription/cancel', AuthenticationController.requireLogin(), diff --git a/services/web/app/src/Features/Subscription/SubscriptionViewModelBuilder.js b/services/web/app/src/Features/Subscription/SubscriptionViewModelBuilder.js index 3993e78e02..25c919435e 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionViewModelBuilder.js +++ b/services/web/app/src/Features/Subscription/SubscriptionViewModelBuilder.js @@ -9,6 +9,7 @@ const PublishersGetter = require('../Publishers/PublishersGetter') const sanitizeHtml = require('sanitize-html') const _ = require('underscore') const async = require('async') +const SubscriptionHelper = require('./SubscriptionHelper') function buildHostedLink(recurlySubscription, type) { const recurlySubdomain = Settings.apis.recurly.subdomain @@ -176,18 +177,14 @@ module.exports = { const totalLicenses = (plan.membersLimit || 0) + additionalLicenses personalSubscription.recurly = { tax, - taxRate: parseFloat( - recurlySubscription.tax_rate && recurlySubscription.tax_rate._ - ), + taxRate: recurlySubscription.tax_rate + ? parseFloat(recurlySubscription.tax_rate._) + : 0, billingDetailsLink: buildHostedLink( recurlySubscription, 'billingDetails' ), accountManagementLink: buildHostedLink(recurlySubscription), - price: SubscriptionFormatters.formatPrice( - recurlySubscription.unit_amount_in_cents + addOnPrice + tax, - recurlySubscription.currency - ), additionalLicenses, totalLicenses, nextPaymentDueAt: SubscriptionFormatters.formatDate( @@ -202,6 +199,33 @@ module.exports = { activeCoupons: recurlyCoupons, account: recurlySubscription.account, } + if (recurlySubscription.pending_subscription) { + const pendingSubscriptionTax = + personalSubscription.recurly.taxRate * + recurlySubscription.pending_subscription.unit_amount_in_cents + personalSubscription.recurly.price = SubscriptionFormatters.formatPrice( + recurlySubscription.pending_subscription.unit_amount_in_cents + + addOnPrice + + pendingSubscriptionTax, + recurlySubscription.currency + ) + const pendingPlan = PlansLocator.findLocalPlanInSettings( + recurlySubscription.pending_subscription.plan.plan_code + ) + if (pendingPlan == null) { + return callback( + new Error( + `No plan found for planCode '${personalSubscription.planCode}'` + ) + ) + } + personalSubscription.pendingPlan = pendingPlan + } else { + personalSubscription.recurly.price = SubscriptionFormatters.formatPrice( + recurlySubscription.unit_amount_in_cents + addOnPrice + tax, + recurlySubscription.currency + ) + } } for (const memberGroupSubscription of memberGroupSubscriptions) { @@ -225,14 +249,30 @@ module.exports = { ) }, - buildPlansList() { + buildPlansList(currentPlan) { const { plans } = Settings const allPlans = {} - plans.forEach(plan => (allPlans[plan.planCode] = plan)) + plans.forEach(plan => { + allPlans[plan.planCode] = plan + }) const result = { allPlans } + if (currentPlan) { + result.planCodesChangingAtTermEnd = _.pluck( + _.filter(plans, plan => { + if (!plan.hideFromUsers) { + return SubscriptionHelper.shouldPlanChangeAtTermEnd( + currentPlan, + plan + ) + } + }), + 'planCode' + ) + } + result.studentAccounts = _.filter( plans, plan => plan.planCode.indexOf('student') !== -1 diff --git a/services/web/app/views/subscriptions/dashboard.pug b/services/web/app/views/subscriptions/dashboard.pug index 2c0e8af038..649d91911d 100644 --- a/services/web/app/views/subscriptions/dashboard.pug +++ b/services/web/app/views/subscriptions/dashboard.pug @@ -7,6 +7,7 @@ block head-scripts block append meta meta(name="ol-managedInstitutions", data-type="json", content=managedInstitutions) + meta(name="ol-planCodesChangingAtTermEnd", data-type="json", content=plans.planCodesChangingAtTermEnd) if (personalSubscription && personalSubscription.recurly) meta(name="ol-recurlyApiKey" content=settings.apis.recurly.publicKey) meta(name="ol-subscription" data-type="json" content=personalSubscription) diff --git a/services/web/app/views/subscriptions/dashboard/_change_plans_mixins.pug b/services/web/app/views/subscriptions/dashboard/_change_plans_mixins.pug index 7e03adef16..c0078ccac0 100644 --- a/services/web/app/views/subscriptions/dashboard/_change_plans_mixins.pug +++ b/services/web/app/views/subscriptions/dashboard/_change_plans_mixins.pug @@ -10,7 +10,14 @@ mixin printPlan(plan) | {{price}} / #{translate("month")} td if (typeof(personalSubscription.planCode) != "undefined" && plan.planCode == personalSubscription.planCode.split("_")[0]) - button.btn.disabled #{translate("your_plan")} + if (personalSubscription.pendingPlan) + form + input(type="hidden", ng-model="plan_code", name="plan_code", value=plan.planCode) + input(type="submit", ng-click="cancelPendingPlanChange()", value=translate("keep_current_plan")).btn.btn-success + else + button.btn.disabled #{translate("your_plan")} + else if (personalSubscription.pendingPlan && typeof(personalSubscription.pendingPlan.planCode) != "undefined" && plan.planCode == personalSubscription.pendingPlan.planCode.split("_")[0]) + button.btn.disabled #{translate("your_new_plan")} else form input(type="hidden", ng-model="plan_code", name="plan_code", value=plan.planCode) diff --git a/services/web/app/views/subscriptions/dashboard/_personal_subscription_recurly.pug b/services/web/app/views/subscriptions/dashboard/_personal_subscription_recurly.pug index 1f9fe4ce9f..bfaeab2ceb 100644 --- a/services/web/app/views/subscriptions/dashboard/_personal_subscription_recurly.pug +++ b/services/web/app/views/subscriptions/dashboard/_personal_subscription_recurly.pug @@ -7,11 +7,16 @@ div(ng-controller="RecurlySubscriptionController") case personalSubscription.recurly.state when "active" p !{translate("currently_subscribed_to_plan", {planName: personalSubscription.plan.name}, ['strong'])} + if (personalSubscription.pendingPlan) + | + | !{translate("your_plan_is_changing_at_term_end", {pendingPlanName: personalSubscription.pendingPlan.name}, ['strong'])} if (personalSubscription.recurly.additionalLicenses > 0) | | !{translate("additional_licenses", {additionalLicenses: personalSubscription.recurly.additionalLicenses, totalLicenses: personalSubscription.recurly.totalLicenses}, ['strong', 'strong'])} |   a(href, ng-click="switchToChangePlanView()", ng-if="showChangePlanButton") !{translate("change_plan")}. + if (personalSubscription.pendingPlan) + p #{translate("want_change_to_apply_before_plan_end")} if (personalSubscription.recurly.trialEndsAtFormatted && personalSubscription.recurly.trial_ends_at > Date.now()) p You're on a free trial which ends on #{personalSubscription.recurly.trialEndsAtFormatted} p !{translate("next_payment_of_x_collectected_on_y", {paymentAmmount: personalSubscription.recurly.price, collectionDate: personalSubscription.recurly.nextPaymentDueAt}, ['strong', 'strong'])} @@ -86,6 +91,9 @@ script(type='text/ng-template', id='confirmChangePlanModalTemplate') h3 #{translate("change_plan")} .modal-body p !{translate("sure_you_want_to_change_plan", {planName: '{{plan.name}}'}, ['strong'])} + div(ng-show="planChangesAtTermEnd") + p #{translate("existing_plan_active_until_term_end")} + p #{translate("want_change_to_apply_before_plan_end")} .modal-footer button.btn.btn-default( ng-disabled="inflight" @@ -97,3 +105,20 @@ script(type='text/ng-template', id='confirmChangePlanModalTemplate') ) span(ng-hide="inflight") #{translate("change_plan")} span(ng-show="inflight") #{translate("processing")}… + +script(type='text/ng-template', id='cancelPendingPlanChangeModalTemplate') + .modal-header + h3 #{translate("change_plan")} + .modal-body + p !{translate("sure_you_want_to_cancel_plan_change", {planName: '{{plan.name}}'}, ['strong'])} + .modal-footer + button.btn.btn-default( + ng-disabled="inflight" + ng-click="cancel()" + ) #{translate("cancel")} + button.btn.btn-success( + ng-disabled="state.inflight" + ng-click="confirmCancelPendingPlanChange()" + ) + span(ng-hide="inflight") #{translate("revert_pending_plan_change")} + span(ng-show="inflight") #{translate("processing")}… \ No newline at end of file diff --git a/services/web/frontend/js/main/subscription-dashboard.js b/services/web/frontend/js/main/subscription-dashboard.js index 623187f593..a6f585edf0 100644 --- a/services/web/frontend/js/main/subscription-dashboard.js +++ b/services/web/frontend/js/main/subscription-dashboard.js @@ -103,8 +103,25 @@ App.controller( scope: $scope, }) + $scope.cancelPendingPlanChange = () => + $modal.open({ + templateUrl: 'cancelPendingPlanChangeModalTemplate', + controller: 'CancelPendingPlanChangeController', + scope: $scope, + }) + $scope.$watch('plan', function (plan) { if (!plan) return + const planCodesChangingAtTermEnd = getMeta( + 'ol-planCodesChangingAtTermEnd' + ) + $scope.planChangesAtTermEnd = false + if ( + planCodesChangingAtTermEnd && + planCodesChangingAtTermEnd.indexOf(plan.planCode) > -1 + ) { + $scope.planChangesAtTermEnd = true + } const planCode = plan.planCode const subscription = getMeta('ol-subscription') const { currency, taxRate } = subscription.recurly @@ -139,6 +156,28 @@ App.controller( } ) +App.controller( + 'CancelPendingPlanChangeController', + function ($scope, $modalInstance, $http) { + $scope.confirmCancelPendingPlanChange = function () { + const body = { + _csrf: window.csrfToken, + } + + $scope.inflight = true + + return $http + .post('/user/subscription/cancel-pending', body) + .then(() => location.reload()) + .catch(() => + console.log('something went wrong reverting pending plan change') + ) + } + + return ($scope.cancel = () => $modalInstance.dismiss('cancel')) + } +) + App.controller( 'LeaveGroupModalController', function ($scope, $modalInstance, $http) { diff --git a/services/web/locales/en.json b/services/web/locales/en.json index 40d4ff8fe9..b8a805bb0b 100644 --- a/services/web/locales/en.json +++ b/services/web/locales/en.json @@ -1009,10 +1009,13 @@ "month": "month", "subscribe_to_this_plan": "Subscribe to this plan", "your_plan": "Your plan", + "your_new_plan": "Your new plan", "your_subscription": "Your Subscription", "on_free_trial_expiring_at": "You are currently using a free trial which expires on __expiresAt__.", "choose_a_plan_below": "Choose a plan below to subscribe to.", "currently_subscribed_to_plan": "You are currently subscribed to the <0>__planName__ plan.", + "your_plan_is_changing_at_term_end": "Your plan is changing to <0>__pendingPlanName__ at the end of the current billing period.", + "want_change_to_apply_before_plan_end": "If you wish this change to apply before the end of your current billing period, please contact us.", "change_plan": "Change plan", "next_payment_of_x_collectected_on_y": "The next payment of <0>__paymentAmmount__ will be collected on <1>__collectionDate__.", "additional_licenses": "Your subscription includes <0>__additionalLicenses__ additional license(s) for a total of <1>__totalLicenses__ licenses.", @@ -1164,8 +1167,12 @@ "privacy": "Privacy", "contact": "Contact", "change_to_this_plan": "Change to this plan", + "keep_current_plan": "Keep my current plan", "processing": "processing", "sure_you_want_to_change_plan": "Are you sure you want to change plan to <0>__planName__?", + "sure_you_want_to_cancel_plan_change": "Are you sure you want to revert your scheduled plan change? You will remain subscribed to the <0>__planName__ plan.", + "revert_pending_plan_change": "Revert scheduled plan change", + "existing_plan_active_until_term_end": "Your existing plan and its features will remain active until the end of the current billing period.", "move_to_annual_billing": "Move to Annual Billing", "annual_billing_enabled": "Annual billing enabled", "move_to_annual_billing_now": "Move to annual billing now", diff --git a/services/web/test/unit/src/Subscription/PlansLocatorTests.js b/services/web/test/unit/src/Subscription/PlansLocatorTests.js new file mode 100644 index 0000000000..b2c829c103 --- /dev/null +++ b/services/web/test/unit/src/Subscription/PlansLocatorTests.js @@ -0,0 +1,51 @@ +const SandboxedModule = require('sandboxed-module') +const { expect } = require('chai') +const modulePath = '../../../../app/src/Features/Subscription/PlansLocator' + +const plans = [ + { + planCode: 'first', + name: '1st', + price: 800, + features: {}, + featureDescription: {}, + }, + { + planCode: 'second', + name: '2nd', + price: 1500, + features: {}, + featureDescription: {}, + }, + { + planCode: 'third', + name: '3rd', + price: 3000, + features: {}, + featureDescription: {}, + }, +] + +describe('PlansLocator', function () { + beforeEach(function () { + this.settings = { plans } + + this.PlansLocator = SandboxedModule.require(modulePath, { + requires: { + 'settings-sharelatex': this.settings, + }, + }) + }) + + describe('findLocalPlanInSettings', function () { + it('should return the found plan', function () { + const plan = this.PlansLocator.findLocalPlanInSettings('second') + expect(plan).to.have.property('name', '2nd') + expect(plan).to.have.property('price', 1500) + }) + it('should return null if no matching plan is found', function () { + const plan = this.PlansLocator.findLocalPlanInSettings('gibberish') + expect(plan).to.be.a('null') + }) + }) +}) diff --git a/services/web/test/unit/src/Subscription/RecurlyClientTests.js b/services/web/test/unit/src/Subscription/RecurlyClientTests.js index a78c0763e1..1391feb3ff 100644 --- a/services/web/test/unit/src/Subscription/RecurlyClientTests.js +++ b/services/web/test/unit/src/Subscription/RecurlyClientTests.js @@ -21,9 +21,21 @@ describe('RecurlyClient', function () { } this.user = { _id: '123456', email: 'joe@example.com', first_name: 'Joe' } + this.subscription = { + id: 'subscription-123', + uuid: 'subscription-uuid-123', + } + this.subscriptionChange = { id: 'subscription-change-123' } + this.recurlyAccount = new recurly.Account() Object.assign(this.recurlyAccount, { code: this.user._id }) + this.recurlySubscription = new recurly.Subscription() + Object.assign(this.recurlySubscription, this.subscription) + + this.recurlySubscriptionChange = new recurly.SubscriptionChange() + Object.assign(this.recurlySubscriptionChange, this.subscriptionChange) + this.UserGetter = { promises: { getUser: sinon.stub().callsFake(userId => { @@ -118,4 +130,138 @@ describe('RecurlyClient', function () { ).to.eventually.be.rejectedWith(Error) }) }) + + describe('getSubscription', function () { + it('should return the subscription found by recurly', async function () { + this.client.getSubscription = sinon + .stub() + .resolves(this.recurlySubscription) + await expect( + this.RecurlyClient.promises.getSubscription(this.subscription.id) + ) + .to.eventually.be.an.instanceOf(recurly.Subscription) + .that.has.property('id', this.subscription.id) + }) + + it('should throw any API errors', async function () { + this.client.getSubscription = sinon.stub().throws() + await expect( + this.RecurlyClient.promises.getSubscription(this.user._id) + ).to.eventually.be.rejectedWith(Error) + }) + }) + + describe('changeSubscription', function () { + beforeEach(function () { + this.client.createSubscriptionChange = sinon + .stub() + .resolves(this.recurlySubscriptionChange) + }) + + it('should attempt to create a subscription change', async function () { + this.RecurlyClient.promises.changeSubscription(this.subscription.id, {}) + expect(this.client.createSubscriptionChange).to.be.calledWith( + this.subscription.id + ) + }) + + it('should return the subscription change event', async function () { + await expect( + this.RecurlyClient.promises.changeSubscription( + this.subscriptionChange.id, + {} + ) + ) + .to.eventually.be.an.instanceOf(recurly.SubscriptionChange) + .that.has.property('id', this.subscriptionChange.id) + }) + + it('should throw any API errors', async function () { + this.client.createSubscriptionChange = sinon.stub().throws() + await expect( + this.RecurlyClient.promises.changeSubscription(this.subscription.id, {}) + ).to.eventually.be.rejectedWith(Error) + }) + + describe('changeSubscriptionByUuid', function () { + it('should attempt to create a subscription change', async function () { + this.RecurlyClient.promises.changeSubscriptionByUuid( + this.subscription.uuid, + {} + ) + expect(this.client.createSubscriptionChange).to.be.calledWith( + 'uuid-' + this.subscription.uuid + ) + }) + + it('should return the subscription change event', async function () { + await expect( + this.RecurlyClient.promises.changeSubscriptionByUuid( + this.subscriptionChange.id, + {} + ) + ) + .to.eventually.be.an.instanceOf(recurly.SubscriptionChange) + .that.has.property('id', this.subscriptionChange.id) + }) + + it('should throw any API errors', async function () { + this.client.createSubscriptionChange = sinon.stub().throws() + await expect( + this.RecurlyClient.promises.changeSubscriptionByUuid( + this.subscription.id, + {} + ) + ).to.eventually.be.rejectedWith(Error) + }) + }) + }) + + describe('removeSubscriptionChange', function () { + beforeEach(function () { + this.client.removeSubscriptionChange = sinon.stub().resolves() + }) + + it('should attempt to remove a pending subscription change', async function () { + this.RecurlyClient.promises.removeSubscriptionChange( + this.subscription.id, + {} + ) + expect(this.client.removeSubscriptionChange).to.be.calledWith( + this.subscription.id + ) + }) + + it('should throw any API errors', async function () { + this.client.removeSubscriptionChange = sinon.stub().throws() + await expect( + this.RecurlyClient.promises.removeSubscriptionChange( + this.subscription.id, + {} + ) + ).to.eventually.be.rejectedWith(Error) + }) + + describe('removeSubscriptionChangeByUuid', function () { + it('should attempt to remove a pending subscription change', async function () { + this.RecurlyClient.promises.removeSubscriptionChangeByUuid( + this.subscription.uuid, + {} + ) + expect(this.client.removeSubscriptionChange).to.be.calledWith( + 'uuid-' + this.subscription.uuid + ) + }) + + it('should throw any API errors', async function () { + this.client.removeSubscriptionChange = sinon.stub().throws() + await expect( + this.RecurlyClient.promises.removeSubscriptionChangeByUuid( + this.subscription.id, + {} + ) + ).to.eventually.be.rejectedWith(Error) + }) + }) + }) }) diff --git a/services/web/test/unit/src/Subscription/SubscriptionHandlerTests.js b/services/web/test/unit/src/Subscription/SubscriptionHandlerTests.js index f0119abeea..72bd4b8c8b 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionHandlerTests.js +++ b/services/web/test/unit/src/Subscription/SubscriptionHandlerTests.js @@ -1,5 +1,7 @@ const SandboxedModule = require('sandboxed-module') const sinon = require('sinon') +const chai = require('chai') +const { expect } = chai const modulePath = '../../../../app/src/Features/Subscription/SubscriptionHandler' @@ -19,6 +21,30 @@ const mockRecurlySubscriptions = { }, } +const mockRecurlyClientSubscriptions = { + 'subscription-123-active': { + id: 'subscription-123-recurly-id', + uuid: 'subscription-123-active', + plan: { + name: 'Gold', + code: 'gold', + }, + currentPeriodEndsAt: new Date(), + state: 'active', + unitAmount: 10, + account: { + code: 'user-123', + }, + }, +} + +const mockSubscriptionChanges = { + 'subscription-123-active': { + id: 'subscription-change-id', + subscriptionId: 'subscription-123-recurly-id', // not the UUID + }, +} + describe('SubscriptionHandler', function () { beforeEach(function () { this.Settings = { @@ -39,6 +65,10 @@ describe('SubscriptionHandler', function () { } this.activeRecurlySubscription = mockRecurlySubscriptions['subscription-123-active'] + this.activeRecurlyClientSubscription = + mockRecurlyClientSubscriptions['subscription-123-active'] + this.activeRecurlySubscriptionChange = + mockSubscriptionChanges['subscription-123-active'] this.User = {} this.user = { _id: (this.user_id = 'user_id_here_') } this.subscription = { @@ -48,9 +78,6 @@ describe('SubscriptionHandler', function () { getSubscription: sinon .stub() .callsArgWith(2, null, this.activeRecurlySubscription), - updateSubscription: sinon - .stub() - .callsArgWith(2, null, this.activeRecurlySubscription), cancelSubscription: sinon.stub().callsArgWith(1), reactivateSubscription: sinon.stub().callsArgWith(1), redeemCoupon: sinon.stub().callsArgWith(2), @@ -61,6 +88,14 @@ describe('SubscriptionHandler', function () { getAccountPastDueInvoices: sinon.stub().yields(), attemptInvoiceCollection: sinon.stub().yields(), } + this.RecurlyClient = { + changeSubscriptionByUuid: sinon + .stub() + .yields(null, this.activeRecurlySubscriptionChange), + getSubscription: sinon + .stub() + .yields(null, this.activeRecurlyClientSubscription), + } this.SubscriptionUpdater = { syncSubscription: sinon.stub().yields(), @@ -73,9 +108,18 @@ describe('SubscriptionHandler', function () { this.AnalyticsManager = { recordEvent: sinon.stub() } + this.PlansLocator = { + findLocalPlanInSettings: sinon.stub().returns({ planCode: 'plan' }), + } + + this.SubscriptionHelper = { + shouldPlanChangeAtTermEnd: sinon.stub(), + } + this.SubscriptionHandler = SandboxedModule.require(modulePath, { requires: { './RecurlyWrapper': this.RecurlyWrapper, + './RecurlyClient': this.RecurlyClient, 'settings-sharelatex': this.Settings, '../../models/User': { User: this.User, @@ -84,6 +128,8 @@ describe('SubscriptionHandler', function () { './LimitationsManager': this.LimitationsManager, '../Email/EmailHandler': this.EmailHandler, '../Analytics/AnalyticsManager': this.AnalyticsManager, + './PlansLocator': this.PlansLocator, + './SubscriptionHelper': this.SubscriptionHelper, }, }) @@ -153,11 +199,43 @@ describe('SubscriptionHandler', function () { }) }) - describe('updateSubscription', function () { - describe('with a user with a subscription', function () { - describe('with a valid plan code', function () { + function shouldUpdateSubscription() { + it('should update the subscription', function () { + expect( + this.RecurlyClient.changeSubscriptionByUuid + ).to.have.been.calledWith(this.subscription.recurlySubscription_id) + const updateOptions = this.RecurlyClient.changeSubscriptionByUuid + .args[0][1] + updateOptions.planCode.should.equal(this.plan_code) + }) + } + + function shouldSyncSubscription() { + it('should sync the new subscription to the user', function () { + expect(this.SubscriptionUpdater.syncSubscription).to.have.been.called + this.SubscriptionUpdater.syncSubscription.args[0][0].should.deep.equal( + this.activeRecurlySubscription + ) + this.SubscriptionUpdater.syncSubscription.args[0][1].should.deep.equal( + this.user._id + ) + }) + } + + function testUserWithASubscription(shouldPlanChangeAtTermEnd, timeframe) { + describe( + 'when change should happen with timeframe ' + timeframe, + function () { beforeEach(function (done) { + this.user.id = this.activeRecurlySubscription.account.account_code + this.User.findById = (userId, projection, callback) => { + userId.should.equal(this.user.id) + callback(null, this.user) + } this.plan_code = 'collaborator' + this.SubscriptionHelper.shouldPlanChangeAtTermEnd = sinon + .stub() + .returns(shouldPlanChangeAtTermEnd) this.LimitationsManager.userHasV2Subscription.callsArgWith( 1, null, @@ -172,31 +250,55 @@ describe('SubscriptionHandler', function () { ) }) - it('should update the subscription', function () { - this.RecurlyWrapper.updateSubscription - .calledWith(this.subscription.recurlySubscription_id) + shouldUpdateSubscription() + shouldSyncSubscription() + + it('should update with timeframe ' + timeframe, function () { + const updateOptions = this.RecurlyClient.changeSubscriptionByUuid + .args[0][1] + updateOptions.timeframe.should.equal(timeframe) + }) + } + ) + } + + describe('updateSubscription', function () { + describe('with a user with a subscription', function () { + testUserWithASubscription(false, 'now') + testUserWithASubscription(true, 'term_end') + + describe('when plan(s) could not be located in settings', function () { + beforeEach(function () { + this.user.id = this.activeRecurlySubscription.account.account_code + this.User.findById = (userId, projection, callback) => { + userId.should.equal(this.user.id) + callback(null, this.user) + } + this.plan_code = 'collaborator' + this.PlansLocator.findLocalPlanInSettings = sinon.stub().returns(null) + this.LimitationsManager.userHasV2Subscription.callsArgWith( + 1, + null, + true, + this.subscription + ) + this.callback = sinon.stub() + this.SubscriptionHandler.updateSubscription( + this.user, + this.plan_code, + null, + this.callback + ) + }) + + it('should not update the subscription', function () { + this.RecurlyClient.changeSubscriptionByUuid.called.should.equal(false) + }) + + it('should return an error to the callback', function () { + this.callback + .calledWith(sinon.match.instanceOf(Error)) .should.equal(true) - const updateOptions = this.RecurlyWrapper.updateSubscription - .args[0][1] - updateOptions.plan_code.should.equal(this.plan_code) - }) - - it('should update immediately', function () { - const updateOptions = this.RecurlyWrapper.updateSubscription - .args[0][1] - updateOptions.timeframe.should.equal('now') - }) - - it('should sync the new subscription to the user', function () { - this.SubscriptionUpdater.syncSubscription.calledOnce.should.equal( - true - ) - this.SubscriptionUpdater.syncSubscription.args[0][0].should.deep.equal( - this.activeRecurlySubscription - ) - this.SubscriptionUpdater.syncSubscription.args[0][1].should.deep.equal( - this.user._id - ) }) }) }) @@ -217,7 +319,7 @@ describe('SubscriptionHandler', function () { }) it('should redirect to the subscription dashboard', function () { - this.RecurlyWrapper.updateSubscription.called.should.equal(false) + this.RecurlyClient.changeSubscriptionByUuid.called.should.equal(false) this.SubscriptionHandler.syncSubscriptionToUser.called.should.equal( false ) @@ -226,6 +328,11 @@ describe('SubscriptionHandler', function () { describe('with a coupon code', function () { beforeEach(function (done) { + this.user.id = this.activeRecurlySubscription.account.account_code + this.User.findById = (userId, projection, callback) => { + userId.should.equal(this.user.id) + callback(null, this.user) + } this.plan_code = 'collaborator' this.coupon_code = '1231312' this.LimitationsManager.userHasV2Subscription.callsArgWith( @@ -248,7 +355,7 @@ describe('SubscriptionHandler', function () { .should.equal(true) }) - it('should redeme the coupon', function (done) { + it('should redeem the coupon', function (done) { this.RecurlyWrapper.redeemCoupon .calledWith( this.activeRecurlySubscription.account.account_code, @@ -259,11 +366,12 @@ describe('SubscriptionHandler', function () { }) it('should update the subscription', function () { - this.RecurlyWrapper.updateSubscription - .calledWith(this.subscription.recurlySubscription_id) - .should.equal(true) - const updateOptions = this.RecurlyWrapper.updateSubscription.args[0][1] - updateOptions.plan_code.should.equal(this.plan_code) + expect(this.RecurlyClient.changeSubscriptionByUuid).to.be.calledWith( + this.subscription.recurlySubscription_id + ) + const updateOptions = this.RecurlyClient.changeSubscriptionByUuid + .args[0][1] + updateOptions.planCode.should.equal(this.plan_code) }) }) }) diff --git a/services/web/test/unit/src/Subscription/SubscriptionHelperTests.js b/services/web/test/unit/src/Subscription/SubscriptionHelperTests.js new file mode 100644 index 0000000000..2465682d31 --- /dev/null +++ b/services/web/test/unit/src/Subscription/SubscriptionHelperTests.js @@ -0,0 +1,50 @@ +const SandboxedModule = require('sandboxed-module') +const { expect } = require('chai') +const modulePath = + '../../../../app/src/Features/Subscription/SubscriptionHelper' + +const plans = { + expensive: { + planCode: 'expensive', + price: 15, + }, + cheaper: { + planCode: 'cheaper', + price: 5, + }, + alsoCheap: { + plancode: 'also-cheap', + price: 5, + }, + bad: {}, +} + +describe('SubscriptionHelper', function () { + beforeEach(function () { + this.SubscriptionHelper = SandboxedModule.require(modulePath) + }) + + describe('shouldPlanChangeAtTermEnd', function () { + it('should return true if the new plan is less expensive', function () { + const changeAtTermEnd = this.SubscriptionHelper.shouldPlanChangeAtTermEnd( + plans.expensive, + plans.cheaper + ) + expect(changeAtTermEnd).to.be.true + }) + it('should return false if the new plan is more exepensive', function () { + const changeAtTermEnd = this.SubscriptionHelper.shouldPlanChangeAtTermEnd( + plans.cheaper, + plans.expensive + ) + expect(changeAtTermEnd).to.be.false + }) + it('should return false if the new plan is the same price', function () { + const changeAtTermEnd = this.SubscriptionHelper.shouldPlanChangeAtTermEnd( + plans.cheaper, + plans.alsoCheap + ) + expect(changeAtTermEnd).to.be.false + }) + }) +})