From a868e6b838ddd151a93203a5a0d782931c8f20a0 Mon Sep 17 00:00:00 2001 From: Justin DiRose Date: Wed, 19 Aug 2020 14:37:47 -0500 Subject: [PATCH] FEATURE: Cancel payments at end of subscription vs immediately Previously, when a user canceled a subscription, the access would revoke immediately on Discourse vs. at the end of the billing period. This commit changes the behavior to remove membership at the end of the billing period using Stripe's `cancel_at_period_end` attribute on the Subscription object. This commit now requires the setup of webhooks for subscription processing to occur correctly. --- .../hooks_controller.rb | 45 ++++-- .../user/subscriptions_controller.rb | 27 +--- .../routes/user-billing-subscriptions.js.es6 | 1 + .../templates/user/billing/subscriptions.hbs | 2 +- config/locales/client.en.yml | 1 + config/routes.rb | 2 +- .../user/subscriptions_controller_spec.rb | 147 ------------------ 7 files changed, 38 insertions(+), 187 deletions(-) diff --git a/app/controllers/discourse_subscriptions/hooks_controller.rb b/app/controllers/discourse_subscriptions/hooks_controller.rb index 1c64768..487e807 100644 --- a/app/controllers/discourse_subscriptions/hooks_controller.rb +++ b/app/controllers/discourse_subscriptions/hooks_controller.rb @@ -3,6 +3,10 @@ module DiscourseSubscriptions class HooksController < ::ApplicationController include DiscourseSubscriptions::Group + include DiscourseSubscriptions::Stripe + layout false + skip_before_action :check_xhr + skip_before_action :redirect_to_login_if_required skip_before_action :verify_authenticity_token, only: [:create] def create @@ -15,7 +19,7 @@ module DiscourseSubscriptions rescue JSON::ParserError => e render_json_error e.message return - rescue Stripe::SignatureVerificationError => e + rescue ::Stripe::SignatureVerificationError => e render_json_error e.message return end @@ -34,19 +38,7 @@ module DiscourseSubscriptions end when 'customer.subscription.deleted' - - customer = Customer.find_by( - customer_id: event[:data][:object][:customer], - product_id: event[:data][:object][:plan][:product] - ) - - if customer - customer.delete - - user = ::User.find(customer.user_id) - group = plan_group(event[:data][:object][:plan]) - group.remove(user) if group - end + delete_subscription(event) end head 200 @@ -59,11 +51,32 @@ module DiscourseSubscriptions end def subscription_complete?(event) - event.dig(:data, :object, :status) == 'complete' + event && event[:data] && event[:data][:object] && event[:data][:object][:status] && event[:data][:object][:status] == 'complete' end def previously_incomplete?(event) - event.dig(:data, :previous_attributes, :status) == 'incomplete' + event && event[:data] && event[:data][:previous_attributes] && event[:data][:previous_attributes][:status] && event[:data][:previous_attributes][:status] == 'incomplete' + end + + def delete_subscription(event) + customer = Customer.find_by( + customer_id: event[:data][:object][:customer], + product_id: event[:data][:object][:plan][:product] + ) + + if customer + sub_model = Subscription.find_by( + customer_id: customer.id, + external_id: [:id] + ) + + sub_model.delete if sub_model + + user = ::User.find(customer.user_id) + customer.delete + group = plan_group(event[:data][:object][:plan]) + group.remove(user) if group + end end end end diff --git a/app/controllers/discourse_subscriptions/user/subscriptions_controller.rb b/app/controllers/discourse_subscriptions/user/subscriptions_controller.rb index 5c129b6..7155db3 100644 --- a/app/controllers/discourse_subscriptions/user/subscriptions_controller.rb +++ b/app/controllers/discourse_subscriptions/user/subscriptions_controller.rb @@ -48,30 +48,13 @@ module DiscourseSubscriptions end def destroy + # we cancel but don't remove until the end of the period + # full removal is done via webhooks begin - subscription = ::Stripe::Subscription.retrieve(params[:id]) + subscription = ::Stripe::Subscription.update(params[:id], { cancel_at_period_end: true, } ) - customer = Customer.find_by( - user_id: current_user.id, - customer_id: subscription[:customer], - product_id: subscription[:plan][:product] - ) - - if customer.present? - sub_model = Subscription.find_by( - customer_id: customer.id, - external_id: params[:id] - ) - - deleted = ::Stripe::Subscription.delete(params[:id]) - customer.delete - - sub_model.delete if sub_model - - group = plan_group(subscription[:plan]) - group.remove(current_user) if group - - render_json_dump deleted + if subscription + render_json_dump subscription else render_json_error I18n.t('discourse_subscriptions.customer_not_found') end diff --git a/assets/javascripts/discourse/routes/user-billing-subscriptions.js.es6 b/assets/javascripts/discourse/routes/user-billing-subscriptions.js.es6 index 264c5a1..161478e 100644 --- a/assets/javascripts/discourse/routes/user-billing-subscriptions.js.es6 +++ b/assets/javascripts/discourse/routes/user-billing-subscriptions.js.es6 @@ -29,6 +29,7 @@ export default Route.extend({ ) .finally(() => { subscription.set("loading", false); + this.refresh(); }); } } diff --git a/assets/javascripts/discourse/templates/user/billing/subscriptions.hbs b/assets/javascripts/discourse/templates/user/billing/subscriptions.hbs index 52868b0..72d8ef8 100644 --- a/assets/javascripts/discourse/templates/user/billing/subscriptions.hbs +++ b/assets/javascripts/discourse/templates/user/billing/subscriptions.hbs @@ -19,7 +19,7 @@ {{#if subscription.loading}} {{loading-spinner size="small"}} {{else}} - {{d-button disabled=subscription.canceled label="cancel" action=(route-action "cancelSubscription" subscription) icon="times"}} + {{d-button disabled=subscription.canceled_at label="discourse_subscriptions.user.subscriptions.cancelled" action=(route-action "cancelSubscription" subscription) icon="times"}} {{/if}} diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 3235b67..f19c04e 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -51,6 +51,7 @@ en: id: Subscription ID status: Status created_at: Created + cancelled: cancelled operations: destroy: confirm: Are you sure you want to cancel this subscription? diff --git a/config/routes.rb b/config/routes.rb index 493afce..758b702 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -19,13 +19,13 @@ DiscourseSubscriptions::Engine.routes.draw do end resources :customers, only: [:create] - resources :hooks, only: [:create] resources :plans, only: [:index], constraints: SubscriptionsUserConstraint.new resources :products, only: [:index, :show] resources :subscriptions, only: [:create] post '/subscriptions/finalize' => 'subscriptions#finalize' + post '/hooks' => 'hooks#create' get '/' => 'subscriptions#index', constraints: SubscriptionsUserConstraint.new get '/:id' => 'subscriptions#index', constraints: SubscriptionsUserConstraint.new end diff --git a/spec/requests/user/subscriptions_controller_spec.rb b/spec/requests/user/subscriptions_controller_spec.rb index a8a9953..8105713 100644 --- a/spec/requests/user/subscriptions_controller_spec.rb +++ b/spec/requests/user/subscriptions_controller_spec.rb @@ -82,153 +82,6 @@ module DiscourseSubscriptions ) end end - - describe "delete" do - let(:group) { Fabricate(:group, name: 'subscribers') } - - before do - # Users can have more than one customer id - Customer.create(user_id: user.id, customer_id: 'customer_id_1', product_id: 'p_1') - Customer.create(user_id: user.id, customer_id: 'customer_id_1', product_id: 'p_2') - Customer.create(user_id: user.id, customer_id: 'customer_id_2', product_id: 'p_2') - - group.add(user) - end - - it "does not delete a subscription when the customer is wrong" do - ::Stripe::Subscription - .expects(:retrieve) - .with('sub_12345') - .returns( - plan: { product: 'p_1' }, - customer: 'wrong_id' - ) - - ::Stripe::Subscription - .expects(:delete) - .never - - expect { - delete "/s/user/subscriptions/sub_12345.json" - }.not_to change { DiscourseSubscriptions::Customer.count } - - expect(response.status).to eq 422 - end - - it "does not deletes the subscription when the product is wrong" do - ::Stripe::Subscription - .expects(:retrieve) - .with('sub_12345') - .returns( - plan: { product: 'p_wrong' }, - customer: 'customer_id_2' - ) - - ::Stripe::Subscription - .expects(:delete) - .never - - expect { - delete "/s/user/subscriptions/sub_12345.json" - }.not_to change { DiscourseSubscriptions::Customer.count } - - expect(response.status).to eq 422 - end - - it "removes the user from the group" do - ::Stripe::Subscription - .expects(:retrieve) - .with('sub_12345') - .returns( - plan: { product: 'p_1', metadata: { group_name: 'subscribers' } }, - customer: 'customer_id_1' - ) - - ::Stripe::Subscription - .expects(:delete) - - expect { - delete "/s/user/subscriptions/sub_12345.json" - }.to change { user.groups.count }.by(-1) - end - - it "does not remove the user from the group" do - ::Stripe::Subscription - .expects(:retrieve) - .with('sub_12345') - .returns( - plan: { product: 'p_1', metadata: { group_name: 'does_not_exist' } }, - customer: 'customer_id_1' - ) - - ::Stripe::Subscription - .expects(:delete) - - expect { - delete "/s/user/subscriptions/sub_12345.json" - }.not_to change { user.groups.count } - end - - it "deletes the first subscription product 1" do - ::Stripe::Subscription - .expects(:retrieve) - .with('sub_12345') - .returns( - plan: { product: 'p_1', metadata: {} }, - customer: 'customer_id_1' - ) - - ::Stripe::Subscription - .expects(:delete) - .with('sub_12345') - - expect { - delete "/s/user/subscriptions/sub_12345.json" - }.to change { DiscourseSubscriptions::Customer.count }.by(-1) - - expect(response.status).to eq 200 - end - - it "deletes the first subscription product 2" do - ::Stripe::Subscription - .expects(:retrieve) - .with('sub_12345') - .returns( - plan: { product: 'p_2', metadata: {} }, - customer: 'customer_id_1' - ) - - ::Stripe::Subscription - .expects(:delete) - .with('sub_12345') - - expect { - delete "/s/user/subscriptions/sub_12345.json" - }.to change { DiscourseSubscriptions::Customer.count }.by(-1) - - expect(response.status).to eq 200 - end - - it "deletes the second subscription" do - ::Stripe::Subscription - .expects(:retrieve) - .with('sub_12345') - .returns( - plan: { product: 'p_2', metadata: {} }, - customer: 'customer_id_2' - ) - - ::Stripe::Subscription - .expects(:delete) - .with('sub_12345') - - expect { - delete "/s/user/subscriptions/sub_12345.json" - }.to change { DiscourseSubscriptions::Customer.count }.by(-1) - - expect(response.status).to eq 200 - end - end end end end