From 43902ce07705dd79fd75c62c02d281ffbeb29873 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 02:29:23 +0000 Subject: [PATCH 1/2] Fix parallel Stripe subscriptions created on plan changes Users have been accumulating multiple simultaneous Stripe subscriptions (e.g. 3 starter + 1 premium), double-charging them. Root cause: current Stripe API versions no longer include `subscriptions` on retrieved Customer objects, so every `stripe_customer.subscriptions&.data || []` read silently returned []. Every plan change therefore looked like a first-time signup and created a brand-new subscription, while the old one was never modified or canceled Stripe-side (cancellation only ever touched our local records). Repeated clicks on a slow page multiplied the effect. The same dead read also silently skipped subscription cancellation when deleting a payment method or an account, and `Stripe::Subscription.modify` no longer exists in stripe-ruby 15, so the modify path would have crashed even if a subscription had been found. Fixes: - SubscriptionService now lists subscriptions via Stripe::Subscription .list and enforces a single-subscription invariant on every plan change: keep one subscription (preferring the requested price, then healthy status, then oldest), switch its price in place, and cancel any parallel duplicates with proration so unused time is credited. Existing double-subscribed users are healed the next time they touch their plan. - Plan changes are serialized per user with a row lock, so concurrent double-clicks can't race each other into duplicate subscriptions; re-choosing the current plan is now a no-op (and un-schedules a pending cancellation instead of stacking a new subscription). - New subscriptions are created with payment_behavior: error_if_incomplete so a declined card raises Stripe::CardError (routing to the existing failed-card flow) instead of leaving an incomplete subscription behind. - delete_payment_method now cancels every paid subscription at period end (the old code no-opped on the dead read and used a removed API); delete_my_account now cancels all billable subscriptions instead of re-pricing just the first one. - Plan-change links now submit via POST with a disable-on-click guard; the GET route remains for legacy links and is safe now that changes are idempotent. - The payment page's "already paid until" notice works again and reads period ends from subscription items, where newer API versions moved them. - New rake task stripe:audit_parallel_subscriptions reports every customer with parallel subscriptions (dry run by default; APPLY=1 cancels duplicates with proration) to clean up existing damage. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GAr8oQfJnvJnUXpH2ejHRF --- app/controllers/subscriptions_controller.rb | 45 ++-- app/controllers/users_controller.rb | 17 +- app/services/subscription_service.rb | 203 ++++++++++++------- app/views/subscriptions/information.html.erb | 14 +- app/views/subscriptions/new.html.erb | 14 +- config/routes.rb | 5 +- lib/tasks/stripe_audit.rake | 56 +++++ test/services/subscription_service_test.rb | 169 +++++++++++++++ 8 files changed, 405 insertions(+), 118 deletions(-) create mode 100644 lib/tasks/stripe_audit.rake create mode 100644 test/services/subscription_service_test.rb diff --git a/app/controllers/subscriptions_controller.rb b/app/controllers/subscriptions_controller.rb index 557ac7501..2a9c19bb2 100644 --- a/app/controllers/subscriptions_controller.rb +++ b/app/controllers/subscriptions_controller.rb @@ -135,6 +135,7 @@ def information @selected_plan = BillingPlan.find_by(stripe_plan_id: params['plan'], available: true) @stripe_customer = Stripe::Customer.retrieve(current_user.stripe_customer_id) @stripe_payment_methods = @stripe_customer.list_payment_methods(type: 'card') + @stripe_subscriptions = SubscriptionService.billable_stripe_subscriptions(current_user.stripe_customer_id) end # Save a payment method @@ -179,10 +180,6 @@ def information_change def delete_payment_method stripe_customer = Stripe::Customer.retrieve current_user.stripe_customer_id - - # Use safe navigation to handle customers without subscriptions - subscriptions = stripe_customer.subscriptions&.data || [] - stripe_subscription = subscriptions.first payment_methods = stripe_customer.list_payment_methods(type: 'card') payment_methods.data.each do |payment_method| @@ -191,17 +188,18 @@ def delete_payment_method notice = ['Your payment method has been successfully deleted.'] - # Check if user has a non-starter subscription using modern API - if stripe_subscription&.items&.data&.any? - current_price_id = stripe_subscription.items.data[0].price.id - if current_price_id != 'starter' - # Cancel the user's at the end of its effective period on Stripe's end, so they don't get rebilled - stripe_subscription.delete(at_period_end: true) - - active_billing_plan = BillingPlan.find_by(stripe_plan_id: current_price_id) - if active_billing_plan - notice << "Your #{active_billing_plan.name} subscription will end on #{Time.at(stripe_subscription.current_period_end).strftime('%B %d')}." - end + # With no card on file, make sure every paid subscription stops rebilling + # at the end of its current period. + SubscriptionService.billable_stripe_subscriptions(current_user.stripe_customer_id).each do |stripe_subscription| + current_price_id = SubscriptionService.subscription_price_ids(stripe_subscription).first + next if current_price_id.nil? || current_price_id == 'starter' + + Stripe::Subscription.update(stripe_subscription.id, { cancel_at_period_end: true }) + + active_billing_plan = BillingPlan.find_by(stripe_plan_id: current_price_id) + period_end = SubscriptionService.subscription_period_end(stripe_subscription) + if active_billing_plan && period_end + notice << "Your #{active_billing_plan.name} subscription will end on #{Time.at(period_end).strftime('%B %d')}." end end @@ -281,12 +279,17 @@ def move_user_to_plan_requested(plan_id) end def process_plan_change(user, new_plan_id) - # General flow we're going to take here: - # 1. Cancel all existing plans, reversing their benefits - SubscriptionService.cancel_all_existing_subscriptions(user) - - # 2. Add a new plan, adding its benefits - SubscriptionService.add_subscription(user, new_plan_id) + # Serialize plan changes per user with a row lock, so repeated clicks on a + # slow page (or two racing requests) can't both act on stale subscription + # state and double-subscribe the user on Stripe. + user.with_lock do + # General flow we're going to take here: + # 1. Cancel all existing plans, reversing their benefits + SubscriptionService.cancel_all_existing_subscriptions(user) + + # 2. Add a new plan, adding its benefits + SubscriptionService.add_subscription(user, new_plan_id) + end end def set_sidenav_expansion diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 8c43dc5e2..1354e05a2 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -92,20 +92,9 @@ def delete_my_account # :( return end - # Make sure the user is set to Starter on Stripe so we don't keep charging them - stripe_customer = Stripe::Customer.retrieve(current_user.stripe_customer_id) - - # Use safe navigation to handle customers without subscriptions - subscriptions = stripe_customer.subscriptions&.data || [] - stripe_subscription = subscriptions.first - if stripe_subscription - # Update subscription to starter plan using modern API - Stripe::Subscription.modify(stripe_subscription.id, { - items: [{ - id: stripe_subscription.items.data[0].id, - price: 'starter' - }] - }) + # Cancel every billable subscription on Stripe so we don't keep charging them + SubscriptionService.billable_stripe_subscriptions(current_user.stripe_customer_id).each do |stripe_subscription| + Stripe::Subscription.cancel(stripe_subscription.id) end report_user_deletion_to_slack(current_user) diff --git a/app/services/subscription_service.rb b/app/services/subscription_service.rb index 972b8e35c..0dba31c7a 100644 --- a/app/services/subscription_service.rb +++ b/app/services/subscription_service.rb @@ -1,93 +1,160 @@ class SubscriptionService < Service #todo: support multiple simultaneous plans + # Stripe subscription statuses that can still generate charges (or become + # chargeable), and therefore count as "the user's subscription" when we're + # managing plans. Everything else (canceled, incomplete_expired, paused) is + # dead weight we can ignore. + BILLABLE_STRIPE_STATUSES = %w[active trialing past_due unpaid incomplete].freeze + def self.add_subscription(user, plan_id) related_plan = BillingPlan.find_by(stripe_plan_id: plan_id, available: true) raise "Plan #{plan_id} not available for user #{user.id}" if related_plan.nil? # Sync with Stripe (todo pipe into StripeService) unless Rails.env.test? - stripe_customer = Stripe::Customer.retrieve(user.stripe_customer_id) - - # Use safe navigation to handle customers without subscriptions - subscriptions = stripe_customer.subscriptions&.data || [] - stripe_subscription = subscriptions.first - - if stripe_subscription.nil? - # Get the customer's default payment method - payment_methods = Stripe::PaymentMethod.list({ - customer: user.stripe_customer_id, - type: 'card' - }) - - default_payment_method = payment_methods.data.first&.id - - # Create a new subscription on Stripe with the default payment method - subscription_params = { - customer: user.stripe_customer_id, - items: [{ price: plan_id }] - } - - # Add default payment method if available - if default_payment_method - subscription_params[:default_payment_method] = default_payment_method - end - - Stripe::Subscription.create(subscription_params) - stripe_customer = Stripe::Customer.retrieve(user.stripe_customer_id) - - # Use safe navigation to get the newly created subscription - subscriptions = stripe_customer.subscriptions&.data || [] - stripe_subscription = subscriptions.first - else - # Edit an existing Stripe subscription by modifying its items - Stripe::Subscription.modify(stripe_subscription.id, { - items: [{ - id: stripe_subscription.items.data[0].id, - price: plan_id - }] - }) - # Retrieve the updated subscription - stripe_subscription = Stripe::Subscription.retrieve(stripe_subscription.id) + begin + sync_stripe_subscriptions_to_plan(user, plan_id) + rescue Stripe::CardError => e + return :failed_card end + end - # The subscription is already saved by the modify call above - begin + # Add any bonus bandwidth granted by the plan + user.update( + upload_bandwidth_kb: user.upload_bandwidth_kb + related_plan.bonus_bandwidth_kb + ) - # Add any bonus bandwidth granted by the plan - user.update( - upload_bandwidth_kb: user.upload_bandwidth_kb + related_plan.bonus_bandwidth_kb - ) + # Add any one-time referral bonuses + add_any_referral_bonuses(user, plan_id) - # Add any one-time referral bonuses - add_any_referral_bonuses(user, plan_id) + # We intentionally skip callbacks on this to ensure the billing plan changes even on invalid users + user.update_column(:selected_billing_plan_id, related_plan.id) - # We intentionally skip callbacks on this to ensure the billing plan changes even on invalid users - user.update_column(:selected_billing_plan_id, related_plan.id) + user.subscriptions.create( + billing_plan: related_plan, + start_date: DateTime.now, + end_date: DateTime.now.end_of_day + 10.years + ) - user.subscriptions.create( - billing_plan: related_plan, - start_date: DateTime.now, - end_date: DateTime.now.end_of_day + 10.years - ) + user.notifications.create( + message_html: "
You signed up for Premium!
Click here to turn on your Premium pages.
", + icon: 'star', + icon_color: 'text-darken-3 yellow', + happened_at: DateTime.current, + passthrough_link: Rails.application.routes.url_helpers.customization_content_types_path, + reference_code: 'premium-activation' + ) if user.reload.on_premium_plan? - user.notifications.create( - message_html: "
You signed up for Premium!
Click here to turn on your Premium pages.
", - icon: 'star', - icon_color: 'text-darken-3 yellow', - happened_at: DateTime.current, - passthrough_link: Rails.application.routes.url_helpers.customization_content_types_path, - reference_code: 'premium-activation' - ) if user.reload.on_premium_plan? + report_subscription_change_to_slack(user, plan_id) + end - report_subscription_change_to_slack(user, plan_id) + # Ensures the customer ends up with exactly ONE Stripe subscription, on the + # requested price. Any parallel subscriptions are canceled immediately with + # proration, so unused time is credited to the customer's balance and nobody + # can be billed for two plans at once. + # + # We list subscriptions directly instead of reading customer.subscriptions: + # current Stripe API versions no longer include subscriptions on retrieved + # Customer objects, so that field is always nil and reading it made every + # plan change look like a first-time signup (creating a brand-new parallel + # subscription each time). + def self.sync_stripe_subscriptions_to_plan(user, plan_id) + subscriptions = prioritize_subscriptions_to_keep(billable_stripe_subscriptions(user.stripe_customer_id)) + + # Prefer to keep a subscription already on the requested price so we never + # interrupt a billing period the user has already paid for; otherwise keep + # the healthiest/oldest one and switch its price below. + kept = subscriptions.find { |sub| subscription_price_ids(sub).include?(plan_id) } + kept ||= subscriptions.first + + subscriptions.each do |subscription| + next if kept && subscription.id == kept.id + Stripe::Subscription.cancel(subscription.id, prorate: true) + end - rescue Stripe::CardError => e - return :failed_card + if kept.nil? + # Get the customer's default payment method + payment_methods = Stripe::PaymentMethod.list({ + customer: user.stripe_customer_id, + type: 'card' + }) + + default_payment_method = payment_methods.data.first&.id + + # Create a new subscription on Stripe with the default payment method. + # error_if_incomplete makes a declined card raise Stripe::CardError + # instead of leaving an incomplete subscription behind. + subscription_params = { + customer: user.stripe_customer_id, + items: [{ price: plan_id }], + payment_behavior: 'error_if_incomplete' + } + + # Add default payment method if available + if default_payment_method + subscription_params[:default_payment_method] = default_payment_method end + + Stripe::Subscription.create(subscription_params) + elsif !subscription_price_ids(kept).include?(plan_id) + # Edit the existing Stripe subscription by modifying its items + Stripe::Subscription.update(kept.id, { + items: [{ + id: kept.items.data[0].id, + price: plan_id + }] + }) + Stripe::Subscription.retrieve(kept.id) + else + # Already on the requested plan (e.g. a repeated click); just make sure a + # previously-scheduled cancellation doesn't end it out from under them. + if kept.try(:cancel_at_period_end) + kept = Stripe::Subscription.update(kept.id, { cancel_at_period_end: false }) + end + kept end end + # All of the customer's Stripe subscriptions that are (or can become) billable. + def self.billable_stripe_subscriptions(stripe_customer_id) + return [] if stripe_customer_id.blank? + + subscriptions = [] + Stripe::Subscription.list( + customer: stripe_customer_id, + status: 'all', + limit: 100 + ).auto_paging_each do |subscription| + subscriptions << subscription if BILLABLE_STRIPE_STATUSES.include?(subscription.status) + end + + subscriptions + end + + # Order subscriptions by how much we want to preserve them: healthy statuses + # before broken ones (so an active subscription is never canceled in favor of + # an incomplete duplicate), then oldest first (so the user's original billing + # anchor survives). + def self.prioritize_subscriptions_to_keep(subscriptions) + subscriptions.sort_by do |subscription| + [BILLABLE_STRIPE_STATUSES.index(subscription.status) || BILLABLE_STRIPE_STATUSES.length, subscription.created || 0] + end + end + + def self.subscription_price_ids(subscription) + (subscription.items&.data || []).map { |item| item.price&.id }.compact + end + + # The subscription's period end as a unix timestamp. Older Stripe API + # versions expose current_period_end on the subscription; newer ones moved it + # onto each subscription item. + def self.subscription_period_end(subscription) + period_end = subscription.try(:current_period_end) + period_end ||= (subscription.items&.data || []).map { |item| item.try(:current_period_end) }.compact.max + period_end + end + def self.remove_subscription(user, subscription) related_plan = subscription.billing_plan diff --git a/app/views/subscriptions/information.html.erb b/app/views/subscriptions/information.html.erb index 777931c68..d6ee76cc9 100644 --- a/app/views/subscriptions/information.html.erb +++ b/app/views/subscriptions/information.html.erb @@ -221,16 +221,16 @@ <% else %> <% - # Get subscriptions with safe navigation and use modern price.id instead of plan.id - subscriptions = @stripe_customer.subscriptions&.data || [] - active_plan_on_stripe = subscriptions.select { |sub| - sub.items&.data&.any? { |item| item.price&.id == @selected_plan.stripe_plan_id } - }.first + subscriptions = @stripe_subscriptions || [] + active_plan_on_stripe = subscriptions.detect { |sub| + SubscriptionService.subscription_price_ids(sub).include?(@selected_plan.stripe_plan_id) + } + active_plan_period_end = active_plan_on_stripe && SubscriptionService.subscription_period_end(active_plan_on_stripe) %>
- <% if active_plan_on_stripe %> + <% if active_plan_on_stripe && active_plan_period_end %>
@@ -239,7 +239,7 @@

Since you've already paid for a <%= @selected_plan.name %> plan until - <%= Time.at(active_plan_on_stripe.current_period_end).strftime('%B %d, %Y') %>, + <%= Time.at(active_plan_period_end).strftime('%B %d, %Y') %>, you will not be charged again until your next renewal date.

diff --git a/app/views/subscriptions/new.html.erb b/app/views/subscriptions/new.html.erb index bae59aebd..f12910ceb 100644 --- a/app/views/subscriptions/new.html.erb +++ b/app/views/subscriptions/new.html.erb @@ -142,7 +142,7 @@ Current Plan <% else %> - <%= link_to change_subscription_path('starter'), class: "w-full py-3 px-4 rounded-xl text-sm font-semibold text-gray-700 dark:text-gray-200 bg-white dark:bg-gray-700 border-2 border-gray-200 dark:border-gray-600 hover:border-gray-300 dark:hover:border-gray-500 hover:bg-gray-50 dark:hover:bg-gray-600 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-gray-500 transition-all duration-200 text-center block #{'pointer-events-none opacity-50' if free_for_life_user}" do %> + <%= link_to change_subscription_path('starter'), method: :post, data: { disable_with: 'Switching...' }, class: "w-full py-3 px-4 rounded-xl text-sm font-semibold text-gray-700 dark:text-gray-200 bg-white dark:bg-gray-700 border-2 border-gray-200 dark:border-gray-600 hover:border-gray-300 dark:hover:border-gray-500 hover:bg-gray-50 dark:hover:bg-gray-600 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-gray-500 transition-all duration-200 text-center block #{'pointer-events-none opacity-50' if free_for_life_user}" do %> Switch to Starter <% end %> <% end %> @@ -204,7 +204,7 @@ Current Plan <% elsif on_premium_plan %> - <%= link_to change_subscription_path('premium'), class: "w-full py-3 px-4 rounded-xl text-sm font-semibold text-blue-600 bg-white border-2 border-blue-200 hover:border-blue-300 hover:bg-blue-50 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500 transition-all duration-200 text-center block #{'pointer-events-none opacity-50' if free_for_life_user}" do %> + <%= link_to change_subscription_path('premium'), method: :post, data: { disable_with: 'Switching...' }, class: "w-full py-3 px-4 rounded-xl text-sm font-semibold text-blue-600 bg-white border-2 border-blue-200 hover:border-blue-300 hover:bg-blue-50 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500 transition-all duration-200 text-center block #{'pointer-events-none opacity-50' if free_for_life_user}" do %> Switch to Monthly <% end %> <% elsif @active_promotions.any? %> @@ -212,7 +212,7 @@ Premium Active <% else %> - <%= link_to change_subscription_path('premium'), class: "w-full py-3 px-4 rounded-xl text-sm font-semibold text-white bg-blue-600 hover:bg-blue-700 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500 shadow-lg hover:shadow-xl transition-all duration-200 text-center block #{'pointer-events-none opacity-50' if free_for_life_user}" do %> + <%= link_to change_subscription_path('premium'), method: :post, data: { disable_with: 'Switching...' }, class: "w-full py-3 px-4 rounded-xl text-sm font-semibold text-white bg-blue-600 hover:bg-blue-700 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500 shadow-lg hover:shadow-xl transition-all duration-200 text-center block #{'pointer-events-none opacity-50' if free_for_life_user}" do %> Upgrade Now <% end %> <% end %> @@ -282,7 +282,7 @@ Current Plan <% elsif on_premium_plan %> - <%= link_to change_subscription_path('premium-trio'), class: "w-full py-3 px-4 rounded-xl text-sm font-semibold text-blue-600 bg-white border-2 border-blue-200 hover:border-blue-300 hover:bg-blue-50 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500 transition-all duration-200 text-center block #{'pointer-events-none opacity-50' if free_for_life_user}" do %> + <%= link_to change_subscription_path('premium-trio'), method: :post, data: { disable_with: 'Switching...' }, class: "w-full py-3 px-4 rounded-xl text-sm font-semibold text-blue-600 bg-white border-2 border-blue-200 hover:border-blue-300 hover:bg-blue-50 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500 transition-all duration-200 text-center block #{'pointer-events-none opacity-50' if free_for_life_user}" do %> Switch to Quarterly <% end %> <% elsif @active_promotions.any? %> @@ -290,7 +290,7 @@ Premium Active <% else %> - <%= link_to change_subscription_path('premium-trio'), class: "w-full py-3 px-4 rounded-xl text-sm font-semibold text-white bg-blue-600 hover:bg-blue-700 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500 shadow-lg hover:shadow-xl transition-all duration-200 text-center block #{'pointer-events-none opacity-50' if free_for_life_user}" do %> + <%= link_to change_subscription_path('premium-trio'), method: :post, data: { disable_with: 'Switching...' }, class: "w-full py-3 px-4 rounded-xl text-sm font-semibold text-white bg-blue-600 hover:bg-blue-700 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500 shadow-lg hover:shadow-xl transition-all duration-200 text-center block #{'pointer-events-none opacity-50' if free_for_life_user}" do %> Upgrade Now <% end %> <% end %> @@ -356,7 +356,7 @@ Current Plan <% elsif on_premium_plan %> - <%= link_to change_subscription_path('premium-annual'), class: "w-full py-3 px-4 rounded-xl text-sm font-semibold text-blue-600 bg-white border-2 border-blue-300 hover:border-blue-400 hover:bg-blue-50 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500 transition-all duration-200 text-center block #{'pointer-events-none opacity-50' if free_for_life_user}" do %> + <%= link_to change_subscription_path('premium-annual'), method: :post, data: { disable_with: 'Switching...' }, class: "w-full py-3 px-4 rounded-xl text-sm font-semibold text-blue-600 bg-white border-2 border-blue-300 hover:border-blue-400 hover:bg-blue-50 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500 transition-all duration-200 text-center block #{'pointer-events-none opacity-50' if free_for_life_user}" do %> Switch to Annual <% end %> <% elsif @active_promotions.any? %> @@ -364,7 +364,7 @@ Premium Active <% else %> - <%= link_to change_subscription_path('premium-annual'), class: "w-full py-3 px-4 rounded-xl text-sm font-semibold text-white bg-gradient-to-r from-blue-600 to-blue-700 hover:from-blue-700 hover:to-blue-800 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500 shadow-xl hover:shadow-2xl transition-all duration-200 text-center block #{'pointer-events-none opacity-50' if free_for_life_user}" do %> + <%= link_to change_subscription_path('premium-annual'), method: :post, data: { disable_with: 'Switching...' }, class: "w-full py-3 px-4 rounded-xl text-sm font-semibold text-white bg-gradient-to-r from-blue-600 to-blue-700 hover:from-blue-700 hover:to-blue-800 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500 shadow-xl hover:shadow-2xl transition-all duration-200 text-center block #{'pointer-events-none opacity-50' if free_for_life_user}" do %> Upgrade Now <% end %> <% end %> diff --git a/config/routes.rb b/config/routes.rb index 839c299ee..c0453a406 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -233,7 +233,10 @@ get '/subscription', to: 'subscriptions#new', as: :subscription get '/history', to: 'subscriptions#history', as: :billing_history - get '/to/:stripe_plan_id', to: 'subscriptions#change', as: :change_subscription + # Plan changes mutate billing state, so links submit them via POST. The + # GET route remains for legacy links/bookmarks; the change action itself + # is idempotent either way. + match '/to/:stripe_plan_id', to: 'subscriptions#change', via: [:get, :post], as: :change_subscription get '/information', to: 'subscriptions#information', as: :payment_info post '/information', to: 'subscriptions#information_change', as: :process_payment_info diff --git a/lib/tasks/stripe_audit.rake b/lib/tasks/stripe_audit.rake new file mode 100644 index 000000000..091d50a5b --- /dev/null +++ b/lib/tasks/stripe_audit.rake @@ -0,0 +1,56 @@ +namespace :stripe do + desc "Find customers with parallel (duplicate) Stripe subscriptions. " \ + "Dry-run by default; run with APPLY=1 to cancel the duplicates, " \ + "keeping one subscription per customer (preferring the oldest one " \ + "on the user's currently selected plan). Canceled duplicates are " \ + "prorated, so unused time is credited to the customer's balance. " \ + "Refunds for past double-charges are NOT issued automatically." + task audit_parallel_subscriptions: :environment do + apply = ENV['APPLY'] == '1' + puts apply ? "APPLY mode: duplicate subscriptions WILL be canceled." : "Dry run: no changes will be made. Re-run with APPLY=1 to cancel duplicates." + puts + + # Enumerate all billable subscriptions Stripe-side (one paginated listing + # per status) instead of hitting the API once per user. + subscriptions_by_customer = Hash.new { |hash, key| hash[key] = [] } + SubscriptionService::BILLABLE_STRIPE_STATUSES.each do |status| + Stripe::Subscription.list(status: status, limit: 100).auto_paging_each do |subscription| + subscriptions_by_customer[subscription.customer] << subscription + end + end + + affected = subscriptions_by_customer.select { |_customer_id, subscriptions| subscriptions.length > 1 } + puts "#{subscriptions_by_customer.length} customers with billable subscriptions; #{affected.length} with parallel subscriptions." + puts + + canceled_count = 0 + affected.sort_by { |_customer_id, subscriptions| -subscriptions.length }.each do |customer_id, subscriptions| + user = User.find_by(stripe_customer_id: customer_id) + selected_price = user && BillingPlan.find_by(id: user.selected_billing_plan_id)&.stripe_plan_id + + # Keep the healthiest/oldest subscription on the user's selected plan so + # their original billing anchor (and anything they've already paid for) + # survives; if none matches, keep the healthiest/oldest one outright. + sorted = SubscriptionService.prioritize_subscriptions_to_keep(subscriptions) + keeper = sorted.find { |subscription| SubscriptionService.subscription_price_ids(subscription).include?(selected_price) } + keeper ||= sorted.first + duplicates = sorted.reject { |subscription| subscription.id == keeper.id } + + puts "Customer #{customer_id} (#{user ? "user ##{user.id} #{user.email}, selected plan: #{selected_price || 'none'}" : 'NO MATCHING USER'})" + puts " KEEP #{keeper.id} [#{keeper.status}] prices=#{SubscriptionService.subscription_price_ids(keeper).join(',')} created=#{Time.at(keeper.created).utc}" + duplicates.each do |duplicate| + puts " CANCEL #{duplicate.id} [#{duplicate.status}] prices=#{SubscriptionService.subscription_price_ids(duplicate).join(',')} created=#{Time.at(duplicate.created).utc}" + if apply + Stripe::Subscription.cancel(duplicate.id, prorate: true) + canceled_count += 1 + end + end + puts + end + + if apply + puts "Canceled #{canceled_count} duplicate subscriptions." + puts "NOTE: unused time was credited to each customer's Stripe balance. Refunds for past double-charges must be issued manually from the Stripe dashboard." + end + end +end diff --git a/test/services/subscription_service_test.rb b/test/services/subscription_service_test.rb new file mode 100644 index 000000000..c337dc0e4 --- /dev/null +++ b/test/services/subscription_service_test.rb @@ -0,0 +1,169 @@ +require 'test_helper' +require 'webmock/minitest' + +class SubscriptionServiceTest < ActiveSupport::TestCase + STRIPE_BASE = 'https://api.stripe.com/v1'.freeze + + def setup + @user = users(:one) + @user.update(stripe_customer_id: 'cus_test') + end + + def stripe_subscription_json(id, price_id, status: 'active', cancel_at_period_end: false, created: 1_600_000_000) + { + id: id, + object: 'subscription', + customer: 'cus_test', + status: status, + created: created, + cancel_at_period_end: cancel_at_period_end, + items: { + object: 'list', + data: [{ + id: "si_#{id}", + object: 'subscription_item', + price: { id: price_id, object: 'price' } + }] + } + } + end + + def stub_subscription_list(subscriptions) + stub_request(:get, "#{STRIPE_BASE}/subscriptions") + .with(query: { customer: 'cus_test', status: 'all', limit: '100' }) + .to_return( + status: 200, + body: { object: 'list', url: '/v1/subscriptions', has_more: false, data: subscriptions }.to_json + ) + end + + def stub_no_payment_methods + stub_request(:get, "#{STRIPE_BASE}/payment_methods") + .with(query: { customer: 'cus_test', type: 'card' }) + .to_return(status: 200, body: { object: 'list', data: [] }.to_json) + end + + test "billable_stripe_subscriptions filters out canceled and expired subscriptions" do + stub_subscription_list([ + stripe_subscription_json('sub_active', 'premium', status: 'active'), + stripe_subscription_json('sub_canceled', 'starter', status: 'canceled'), + stripe_subscription_json('sub_expired', 'starter', status: 'incomplete_expired'), + stripe_subscription_json('sub_pastdue', 'starter', status: 'past_due') + ]) + + subscriptions = SubscriptionService.billable_stripe_subscriptions('cus_test') + assert_equal %w[sub_active sub_pastdue], subscriptions.map(&:id).sort + end + + test "billable_stripe_subscriptions is empty for users without a Stripe customer" do + assert_equal [], SubscriptionService.billable_stripe_subscriptions(nil) + assert_equal [], SubscriptionService.billable_stripe_subscriptions('') + end + + test "sync cancels parallel subscriptions and keeps the one on the requested plan" do + stub_subscription_list([ + stripe_subscription_json('sub_starter_1', 'starter'), + stripe_subscription_json('sub_premium', 'premium'), + stripe_subscription_json('sub_starter_2', 'starter') + ]) + cancel_1 = stub_request(:delete, "#{STRIPE_BASE}/subscriptions/sub_starter_1") + .with(query: { prorate: 'true' }) + .to_return(status: 200, body: { id: 'sub_starter_1' }.to_json) + cancel_2 = stub_request(:delete, "#{STRIPE_BASE}/subscriptions/sub_starter_2") + .with(query: { prorate: 'true' }) + .to_return(status: 200, body: { id: 'sub_starter_2' }.to_json) + + kept = SubscriptionService.sync_stripe_subscriptions_to_plan(@user, 'premium') + + assert_equal 'sub_premium', kept.id + assert_requested cancel_1 + assert_requested cancel_2 + assert_not_requested :post, "#{STRIPE_BASE}/subscriptions" + assert_not_requested :post, "#{STRIPE_BASE}/subscriptions/sub_premium" + end + + test "sync modifies the existing subscription instead of creating a second one" do + stub_subscription_list([ + stripe_subscription_json('sub_starter', 'starter') + ]) + modify = stub_request(:post, "#{STRIPE_BASE}/subscriptions/sub_starter") + .with(body: { items: [{ id: 'si_sub_starter', price: 'premium' }] }) + .to_return(status: 200, body: stripe_subscription_json('sub_starter', 'premium').to_json) + stub_request(:get, "#{STRIPE_BASE}/subscriptions/sub_starter") + .to_return(status: 200, body: stripe_subscription_json('sub_starter', 'premium').to_json) + + SubscriptionService.sync_stripe_subscriptions_to_plan(@user, 'premium') + + assert_requested modify + assert_not_requested :post, "#{STRIPE_BASE}/subscriptions" + end + + test "sync creates a single subscription when the customer has none" do + stub_subscription_list([]) + stub_no_payment_methods + create = stub_request(:post, "#{STRIPE_BASE}/subscriptions") + .with(body: { customer: 'cus_test', items: [{ price: 'premium' }], payment_behavior: 'error_if_incomplete' }) + .to_return(status: 200, body: stripe_subscription_json('sub_new', 'premium').to_json) + + SubscriptionService.sync_stripe_subscriptions_to_plan(@user, 'premium') + + assert_requested create + end + + test "sync is a no-op when the customer is already on the requested plan" do + stub_subscription_list([ + stripe_subscription_json('sub_premium', 'premium') + ]) + + kept = SubscriptionService.sync_stripe_subscriptions_to_plan(@user, 'premium') + + assert_equal 'sub_premium', kept.id + assert_not_requested :post, "#{STRIPE_BASE}/subscriptions" + assert_not_requested :post, "#{STRIPE_BASE}/subscriptions/sub_premium" + assert_not_requested :delete, %r{#{STRIPE_BASE}/subscriptions/sub_premium} + end + + test "sync un-cancels a subscription scheduled for cancellation when re-choosing the same plan" do + stub_subscription_list([ + stripe_subscription_json('sub_premium', 'premium', cancel_at_period_end: true) + ]) + modify = stub_request(:post, "#{STRIPE_BASE}/subscriptions/sub_premium") + .with(body: { cancel_at_period_end: 'false' }) + .to_return(status: 200, body: stripe_subscription_json('sub_premium', 'premium').to_json) + + SubscriptionService.sync_stripe_subscriptions_to_plan(@user, 'premium') + + assert_requested modify + end + + test "sync keeps the active subscription over an incomplete duplicate on the same plan" do + stub_subscription_list([ + stripe_subscription_json('sub_incomplete', 'premium', status: 'incomplete', created: 1_700_000_000), + stripe_subscription_json('sub_active', 'premium', status: 'active', created: 1_600_000_000) + ]) + cancel = stub_request(:delete, "#{STRIPE_BASE}/subscriptions/sub_incomplete") + .with(query: { prorate: 'true' }) + .to_return(status: 200, body: { id: 'sub_incomplete' }.to_json) + + kept = SubscriptionService.sync_stripe_subscriptions_to_plan(@user, 'premium') + + assert_equal 'sub_active', kept.id + assert_requested cancel + assert_not_requested :delete, %r{#{STRIPE_BASE}/subscriptions/sub_active} + end + + test "subscription_period_end falls back to item-level period ends" do + subscription = Stripe::Subscription.construct_from( + id: 'sub_x', + items: { object: 'list', data: [{ id: 'si_x', current_period_end: 1_700_000_000 }] } + ) + assert_equal 1_700_000_000, SubscriptionService.subscription_period_end(subscription) + + subscription_with_top_level = Stripe::Subscription.construct_from( + id: 'sub_y', + current_period_end: 1_650_000_000, + items: { object: 'list', data: [] } + ) + assert_equal 1_650_000_000, SubscriptionService.subscription_period_end(subscription_with_top_level) + end +end From 48ebc1764cac60e06cb37f4760af3a9de319ef5a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 07:58:04 +0000 Subject: [PATCH 2/2] Harden remaining code paths against the parallel-subscription bug Auditing the rest of the billing code turned up three more places built on the same dead `stripe_customer.subscriptions&.data || []` read, plus a failure mode the first fix made more likely. - data_integrity:subscription_synced_with_stripe was a mass-downgrade landmine. It decides whether to downgrade a premium user based on that always-empty read, so on current Stripe API versions it would conclude that EVERY premium user had unsubscribed and downgrade the entire paying user base (emailing each one). It now lists subscriptions properly and checks all of them, refuses to downgrade more than 20% of premium users in a single run (a wrong-looking mass downgrade almost certainly means we are misreading Stripe rather than that everyone churned), and supports DRY_RUN=1. - Admin "unsubscribe" only ended subscriptions in our database and never cancelled them on Stripe, so unsubscribed users kept getting billed. It now cancels on Stripe first. cancel_all_existing_subscriptions is documented as local-only, with cancel_stripe_subscriptions! added for callers that are cancelling outright rather than switching plans. - New-user signup created its starter subscription with a hand-rolled Stripe::Subscription.create that did not check for existing subscriptions. It now goes through the same one-subscription invariant as every other path. - A declined card used to commit the local downgrade. process_plan_change cancels the old plan locally before adding the new one, and add_subscription swallowed Stripe::CardError and returned, so the transaction committed and the user lost the plan and bandwidth they had already paid for. Making creation raise on a declined card (error_if_ incomplete) made this reachable. CardError now propagates out of the transaction, which is opened with requires_new so it rolls back even if nested, and the controller converts it to :failed_card afterwards. Tests: 12 covering the single-subscription invariant against stubbed Stripe HTTP (cancel duplicates, modify-not-create, create-when-none, no-op on repeat, active-beats-incomplete, un-cancel, cancel-all, period-end fallback) plus an integration test asserting a declined card leaves the user's plan, bandwidth and local subscriptions untouched. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GAr8oQfJnvJnUXpH2ejHRF --- app/controllers/admin_controller.rb | 3 + app/controllers/subscriptions_controller.rb | 11 ++- app/models/users/user.rb | 26 ++----- app/services/subscription_service.rb | 30 ++++++-- lib/tasks/data_integrity.rake | 46 +++++++----- .../subscriptions_controller_test.rb | 75 +++++++++++++++++++ test/services/subscription_service_test.rb | 16 ++++ 7 files changed, 157 insertions(+), 50 deletions(-) create mode 100644 test/controllers/subscriptions_controller_test.rb diff --git a/app/controllers/admin_controller.rb b/app/controllers/admin_controller.rb index 932c9a463..e008b9cb2 100644 --- a/app/controllers/admin_controller.rb +++ b/app/controllers/admin_controller.rb @@ -267,6 +267,9 @@ def perform_unsubscribe @users = User.where(email: emails) @users.each do |user| if user.on_premium_plan? + # Cancel on Stripe too, or the user stays subscribed there and keeps + # getting billed after we've unsubscribed them on our end. + SubscriptionService.cancel_stripe_subscriptions!(user) SubscriptionService.cancel_all_existing_subscriptions(user) UnsubscribedMailer.unsubscribed(user).deliver_now! if Rails.env.production? end diff --git a/app/controllers/subscriptions_controller.rb b/app/controllers/subscriptions_controller.rb index 2a9c19bb2..4417eb77e 100644 --- a/app/controllers/subscriptions_controller.rb +++ b/app/controllers/subscriptions_controller.rb @@ -282,7 +282,11 @@ def process_plan_change(user, new_plan_id) # Serialize plan changes per user with a row lock, so repeated clicks on a # slow page (or two racing requests) can't both act on stale subscription # state and double-subscribe the user on Stripe. - user.with_lock do + # requires_new gives us our own savepoint, so a declined card rolls back the + # local downgrade below even if we're ever called inside another transaction. + user.transaction(requires_new: true) do + user.lock! + # General flow we're going to take here: # 1. Cancel all existing plans, reversing their benefits SubscriptionService.cancel_all_existing_subscriptions(user) @@ -290,6 +294,11 @@ def process_plan_change(user, new_plan_id) # 2. Add a new plan, adding its benefits SubscriptionService.add_subscription(user, new_plan_id) end + rescue Stripe::CardError + # with_lock runs in a transaction, so letting the error escape it rolled the + # local downgrade back: the user keeps the plan and bandwidth they had + # before their card was declined. + :failed_card end def set_sidenav_expansion diff --git a/app/models/users/user.rb b/app/models/users/user.rb index 9bdd63431..6945c9d90 100644 --- a/app/models/users/user.rb +++ b/app/models/users/user.rb @@ -296,27 +296,11 @@ def initialize_stripe_customer self.stripe_customer_id = customer_data.id self.save - # If we're creating this Customer in Stripe for the first time, we should also associate them with the free tier - # Get the customer's available payment methods (if any) - payment_methods = Stripe::PaymentMethod.list({ - customer: self.stripe_customer_id, - type: 'card' - }) - - default_payment_method = payment_methods.data.first&.id - - # Create subscription with payment method if available - subscription_params = { - customer: self.stripe_customer_id, - items: [{ price: 'starter' }] - } - - # Add default payment method if available (free tier may not have payment methods) - if default_payment_method - subscription_params[:default_payment_method] = default_payment_method - end - - Stripe::Subscription.create(subscription_params) + # If we're creating this Customer in Stripe for the first time, we should also associate them with the free tier. + # This goes through SubscriptionService so it obeys the same + # one-subscription-per-customer invariant as every other code path, and + # can't leave a brand new user with a parallel subscription. + SubscriptionService.sync_stripe_subscriptions_to_plan(self, 'starter') else # In test environment, just set a dummy customer ID self.stripe_customer_id = 'test_customer_id' diff --git a/app/services/subscription_service.rb b/app/services/subscription_service.rb index 0dba31c7a..0f39b99e7 100644 --- a/app/services/subscription_service.rb +++ b/app/services/subscription_service.rb @@ -11,14 +11,13 @@ def self.add_subscription(user, plan_id) related_plan = BillingPlan.find_by(stripe_plan_id: plan_id, available: true) raise "Plan #{plan_id} not available for user #{user.id}" if related_plan.nil? - # Sync with Stripe (todo pipe into StripeService) - unless Rails.env.test? - begin - sync_stripe_subscriptions_to_plan(user, plan_id) - rescue Stripe::CardError => e - return :failed_card - end - end + # Sync with Stripe (todo pipe into StripeService). + # + # Stripe::CardError is deliberately NOT rescued here. It has to propagate + # out of the caller's transaction so the local downgrade that precedes this + # call rolls back; otherwise a declined card would strip the user of the + # plan and bandwidth they already had. + sync_stripe_subscriptions_to_plan(user, plan_id) unless Rails.env.test? # Add any bonus bandwidth granted by the plan user.update( @@ -165,6 +164,12 @@ def self.remove_subscription(user, subscription) subscription.update(end_date: DateTime.now) end + # Ends the user's subscriptions in OUR database and reverses their benefits. + # This does not touch Stripe: on a plan change, add_subscription switches the + # existing Stripe subscription's price in place (preserving the user's billing + # anchor). Callers that are cancelling outright, rather than switching plans, + # must also call cancel_stripe_subscriptions! or the customer keeps getting + # billed. def self.cancel_all_existing_subscriptions(user) user.update(selected_billing_plan_id: 1) user.active_subscriptions.each do |subscription| @@ -172,6 +177,15 @@ def self.cancel_all_existing_subscriptions(user) end end + # Cancels every billable subscription on Stripe outright. Use this for true + # cancellations (admin unsubscribes, account deletion) — NOT for plan changes, + # where the subscription should be re-priced in place instead. + def self.cancel_stripe_subscriptions!(user) + billable_stripe_subscriptions(user.stripe_customer_id).each do |subscription| + Stripe::Subscription.cancel(subscription.id) + end + end + def self.add_any_referral_bonuses(user, plan_id) # This only applies if we're upgrading to premium, obviously related_billing_plan = BillingPlan.find_by(stripe_plan_id: plan_id) diff --git a/lib/tasks/data_integrity.rake b/lib/tasks/data_integrity.rake index 4dc173682..0bf8327b6 100644 --- a/lib/tasks/data_integrity.rake +++ b/lib/tasks/data_integrity.rake @@ -11,10 +11,19 @@ namespace :data_integrity do PaypalInvoice.where(status: "COMPLETED", page_unlock_promo_code_id: nil).find_each(&:generate_promo_code!) end - desc "Ensure that all Premium subscribers are still Premium in Stripe" + desc "Ensure that all Premium subscribers are still Premium in Stripe. " \ + "Set DRY_RUN=1 to report without downgrading anyone." task subscription_synced_with_stripe: :environment do + dry_run = ENV['DRY_RUN'] == '1' total_accounts_downgraded_this_run = 0 + # Safety valve: if we'd downgrade a large share of paying users in a single + # run, that is far more likely to mean we're misreading Stripe than that + # everybody churned at once. (A dead `customer.subscriptions` read did + # exactly this once already.) Bail out instead of mass-downgrading. + premium_user_count = User.where(selected_billing_plan_id: BillingPlan::PREMIUM_IDS).count + downgrade_limit = [(premium_user_count * 0.2).ceil, 10].max + synced_billing_plan_ids = BillingPlan::PREMIUM_IDS - [BillingPlan.find_by(stripe_plan_id: 'free-for-life').id] synced_billing_plan_ids.each do |billing_plan_id| active_billing_plan = BillingPlan.find(billing_plan_id) @@ -22,30 +31,27 @@ namespace :data_integrity do User.where(selected_billing_plan_id: billing_plan_id).find_each do |user| # puts "Checking user ID #{user.id}" - stripe_customer = Stripe::Customer.retrieve(user.stripe_customer_id) - - # Use safe navigation to handle customers without subscriptions - subscriptions = stripe_customer.subscriptions&.data || [] - stripe_subscription = subscriptions.first - - # Go through each of the customer's subscription items and make sure their - # current billing plan is included as one. - if stripe_subscription.nil? - should_downgrade_user = true - else - should_downgrade_user = stripe_subscription.items.data.none? do |subscription_item| - # Use price.id instead of deprecated plan.id - subscription_item.price.id == active_billing_plan.stripe_plan_id - end + # Check every billable subscription the customer has, not just the first + # one, and list them directly: retrieved Customer objects no longer + # include their subscriptions on current Stripe API versions. + stripe_subscriptions = SubscriptionService.billable_stripe_subscriptions(user.stripe_customer_id) + should_downgrade_user = stripe_subscriptions.none? do |stripe_subscription| + SubscriptionService.subscription_price_ids(stripe_subscription).include?(active_billing_plan.stripe_plan_id) end if should_downgrade_user total_accounts_downgraded_this_run += 1 - puts "Downgrading user #{user.email} from #{active_billing_plan.stripe_plan_id} (last logged in #{user.last_sign_in_at.strftime("%F")})" + puts "#{dry_run ? 'Would downgrade' : 'Downgrading'} user #{user.email} from #{active_billing_plan.stripe_plan_id} (last logged in #{user.last_sign_in_at.strftime("%F")})" - SubscriptionService.cancel_all_existing_subscriptions(user) - UnsubscribedMailer.unsubscribed(user).deliver_now! if Rails.env.production? - SlackService.post('#subscriptions', "Automatically downgrading #{user.email} from #{active_billing_plan.stripe_plan_id} (last logged in #{user.last_sign_in_at.strftime("%F")})") + if total_accounts_downgraded_this_run > downgrade_limit + abort "ABORTING: #{total_accounts_downgraded_this_run} of #{premium_user_count} premium users looked unsubscribed on Stripe, which exceeds the safety limit of #{downgrade_limit}. This usually means we're misreading Stripe rather than that these users actually churned. Investigate before re-running." + end + + unless dry_run + SubscriptionService.cancel_all_existing_subscriptions(user) + UnsubscribedMailer.unsubscribed(user).deliver_now! if Rails.env.production? + SlackService.post('#subscriptions', "Automatically downgrading #{user.email} from #{active_billing_plan.stripe_plan_id} (last logged in #{user.last_sign_in_at.strftime("%F")})") + end end # Aggressively throttle (too much) just to keep Stripe happy if we plan on doing diff --git a/test/controllers/subscriptions_controller_test.rb b/test/controllers/subscriptions_controller_test.rb new file mode 100644 index 000000000..468afa53f --- /dev/null +++ b/test/controllers/subscriptions_controller_test.rb @@ -0,0 +1,75 @@ +require 'test_helper' +require 'webmock/minitest' +require 'minitest/mock' + +class SubscriptionsControllerTest < ActionDispatch::IntegrationTest + include Devise::Test::IntegrationHelpers + + STRIPE_BASE = 'https://api.stripe.com/v1'.freeze + + def setup + @starter_plan = BillingPlan.find_or_create_by!(stripe_plan_id: 'starter') do |plan| + plan.name = 'Starter' + plan.monthly_cents = 0 + plan.available = true + plan.bonus_bandwidth_kb = 0 + end + @premium_plan = BillingPlan.find_or_create_by!(stripe_plan_id: 'premium') do |plan| + plan.name = 'Premium' + plan.monthly_cents = 900 + plan.available = true + plan.bonus_bandwidth_kb = 9_950_000 + end + @annual_plan = BillingPlan.find_or_create_by!(stripe_plan_id: 'premium-annual') do |plan| + plan.name = 'Premium (annual)' + plan.monthly_cents = 700 + plan.available = true + plan.bonus_bandwidth_kb = 9_950_000 + end + + @user = users(:one) + @user.update!(stripe_customer_id: 'cus_test') + sign_in @user + + stub_request(:get, "#{STRIPE_BASE}/customers/cus_test") + .to_return(status: 200, body: { id: 'cus_test' }.to_json) + stub_request(:get, "#{STRIPE_BASE}/customers/cus_test/payment_methods") + .with(query: { type: 'card' }) + .to_return(status: 200, body: { object: 'list', data: [{ id: 'pm_123' }] }.to_json) + end + + test "a declined card leaves the user's existing plan, bandwidth and subscriptions untouched" do + @user.update!(selected_billing_plan_id: @premium_plan.id, upload_bandwidth_kb: 10_000_000) + @user.subscriptions.create!( + billing_plan: @premium_plan, + start_date: 1.day.ago, + end_date: 10.years.from_now + ) + + declined = lambda do |*| + raise Stripe::CardError.new('Your card was declined.', 'number') + end + + SubscriptionService.stub(:add_subscription, declined) do + post change_subscription_path('premium-annual') + end + + @user.reload + assert_equal @premium_plan.id, @user.selected_billing_plan_id, + 'the declined card should not have downgraded the user' + assert_equal 10_000_000, @user.upload_bandwidth_kb, + 'the declined card should not have stripped the premium bandwidth bonus' + assert_equal [@premium_plan.id], @user.active_subscriptions.map(&:billing_plan_id), + 'the local premium subscription should still be active' + assert_redirected_to payment_info_path(plan: 'premium-annual') + end + + test "plan changes are accepted over POST" do + @user.update!(selected_billing_plan_id: @premium_plan.id) + + post change_subscription_path('starter') + + assert_redirected_to subscription_path + assert_equal @starter_plan.id, @user.reload.selected_billing_plan_id + end +end diff --git a/test/services/subscription_service_test.rb b/test/services/subscription_service_test.rb index c337dc0e4..f205b510a 100644 --- a/test/services/subscription_service_test.rb +++ b/test/services/subscription_service_test.rb @@ -152,6 +152,22 @@ def stub_no_payment_methods assert_not_requested :delete, %r{#{STRIPE_BASE}/subscriptions/sub_active} end + test "cancel_stripe_subscriptions! cancels every billable subscription outright" do + stub_subscription_list([ + stripe_subscription_json('sub_a', 'premium'), + stripe_subscription_json('sub_b', 'starter'), + stripe_subscription_json('sub_dead', 'premium', status: 'canceled') + ]) + cancel_a = stub_request(:delete, "#{STRIPE_BASE}/subscriptions/sub_a").to_return(status: 200, body: { id: 'sub_a' }.to_json) + cancel_b = stub_request(:delete, "#{STRIPE_BASE}/subscriptions/sub_b").to_return(status: 200, body: { id: 'sub_b' }.to_json) + + SubscriptionService.cancel_stripe_subscriptions!(@user) + + assert_requested cancel_a + assert_requested cancel_b + assert_not_requested :delete, "#{STRIPE_BASE}/subscriptions/sub_dead" + end + test "subscription_period_end falls back to item-level period ends" do subscription = Stripe::Subscription.construct_from( id: 'sub_x',