From c6210a4476383952c00eaf83efbb51ced0abb77f Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma Date: Mon, 15 Jun 2026 15:51:36 +0530 Subject: [PATCH] fix(billing): harden currency switch idempotency, usd location reset, and default-plan resolution --- .../enterprise/billing/plan_configuration.rb | 4 +- .../stripe_currency_switch_executor.rb | 21 ++++-- .../billing/switch_currency_service.rb | 11 +-- .../billing/switch_currency_service_spec.rb | 70 +++++++++++++++++++ 4 files changed, 92 insertions(+), 14 deletions(-) diff --git a/enterprise/app/services/enterprise/billing/plan_configuration.rb b/enterprise/app/services/enterprise/billing/plan_configuration.rb index 9334b9e14..dd6ebc607 100644 --- a/enterprise/app/services/enterprise/billing/plan_configuration.rb +++ b/enterprise/app/services/enterprise/billing/plan_configuration.rb @@ -36,8 +36,8 @@ module Enterprise::Billing::PlanConfiguration price_ids_by_currency(plan).values.flatten.compact.include?(price_id) end - def default_price?(price_id) - plan_contains_price_id?(default_plan, price_id) + def default_plan?(plan) + plan.present? && plan['name'] == default_plan&.dig('name') end # [plan, currency] for a price id, else [nil, nil]. diff --git a/enterprise/app/services/enterprise/billing/stripe_currency_switch_executor.rb b/enterprise/app/services/enterprise/billing/stripe_currency_switch_executor.rb index ff3bad9f7..fd09730f0 100644 --- a/enterprise/app/services/enterprise/billing/stripe_currency_switch_executor.rb +++ b/enterprise/app/services/enterprise/billing/stripe_currency_switch_executor.rb @@ -10,7 +10,7 @@ class Enterprise::Billing::StripeCurrencySwitchExecutor # Returns the newly-created Stripe subscription. def execute(subscription:, change:) - validate_payment_method! unless Enterprise::Billing::PlanConfiguration.default_price?(subscription['plan']['id']) + validate_payment_method! unless change[:default_plan] previous_currency = account.billing_currency sync_customer_location(target_currency) @@ -55,7 +55,13 @@ class Enterprise::Billing::StripeCurrencySwitchExecutor params = { customer: stripe_customer_id, items: [{ price: price_id, quantity: change[:quantity] }] } # trial_end preserves the already-paid time so switching mid-cycle doesn't double-charge. params[:trial_end] = change[:paid_through] if change[:paid_through].present? && change[:paid_through] > Time.current.to_i - Stripe::Subscription.create(params, { idempotency_key: "switch-#{account.id}-#{change[:key]}" }) + Stripe::Subscription.create(params, { idempotency_key: idempotency_key }) + end + + # Fresh per switch attempt: a retry after a rolled-back (cancelled) create must create a new + # subscription, not replay Stripe's stored response for the now-cancelled one. + def idempotency_key + @idempotency_key ||= "switch-#{account.id}-#{SecureRandom.uuid}" end def validate_payment_method! @@ -68,16 +74,17 @@ class Enterprise::Billing::StripeCurrencySwitchExecutor Stripe::Customer.update(stripe_customer_id, invoice_settings: { default_payment_method: payment_methods.data.first.id }) end - # Only currencies that need a country override (e.g. BRL/PIX) push an address/locale to Stripe; - # usd keeps Stripe's defaults, matching how the customer is first created. + # Currencies that need a country override (e.g. BRL/PIX) push it to Stripe; for currencies without + # one (usd) we clear any prior override so the customer matches how a usd customer is first created + # — otherwise switching away from BRL would leave a stale BR/pt-BR address on the customer. def sync_customer_location(currency_code) country = Enterprise::Billing::Currencies.country_for(currency_code) - return if country.blank? + locale = Enterprise::Billing::Currencies.preferred_locale_for(currency_code) Stripe::Customer.update( stripe_customer_id, - address: { country: country }, - preferred_locales: [Enterprise::Billing::Currencies.preferred_locale_for(currency_code)] + address: { country: country.presence || '' }, + preferred_locales: locale.present? ? [locale] : [] ) end diff --git a/enterprise/app/services/enterprise/billing/switch_currency_service.rb b/enterprise/app/services/enterprise/billing/switch_currency_service.rb index 23fb7d607..17e78ccbf 100644 --- a/enterprise/app/services/enterprise/billing/switch_currency_service.rb +++ b/enterprise/app/services/enterprise/billing/switch_currency_service.rb @@ -21,12 +21,13 @@ class Enterprise::Billing::SwitchCurrencyService def perform subscription = eligibility.subscription! resolver = Enterprise::Billing::PlanPriceResolver.new(subscription: subscription, target_currency: target_currency) - change = change_for(subscription, resolver.target_price_id) + plan = resolver.plan + change = change_for(subscription, resolver.target_price_id, default_plan: Enterprise::Billing::PlanConfiguration.default_plan?(plan)) mark_pending new_subscription = executor.execute(subscription: subscription, change: change) - persist_currency(build_custom_attributes(new_subscription, resolver.plan)) + persist_currency(build_custom_attributes(new_subscription, plan)) Enterprise::Billing::ReconcilePlanFeaturesService.new(account: account).perform rescue Enterprise::Billing::CurrencySwitchEligibility::Error, Enterprise::Billing::PlanPriceResolver::Error, @@ -50,14 +51,14 @@ class Enterprise::Billing::SwitchCurrencyService @target_currency ||= Enterprise::Billing::Currencies.normalize(currency) end - def change_for(subscription, new_price_id) + def change_for(subscription, new_price_id, default_plan:) { new_price_id: new_price_id, quantity: subscription['quantity'], # Paid plans preserve paid-through (new sub trials until then); the free default plan switches # immediately to an active sub, so a default-plan account can switch again any time. - paid_through: Enterprise::Billing::PlanConfiguration.default_price?(subscription['plan']['id']) ? nil : subscription_period_end(subscription), - key: subscription.id + paid_through: default_plan ? nil : subscription_period_end(subscription), + default_plan: default_plan } end diff --git a/spec/enterprise/services/enterprise/billing/switch_currency_service_spec.rb b/spec/enterprise/services/enterprise/billing/switch_currency_service_spec.rb index d0cc5aec4..21d64e99c 100644 --- a/spec/enterprise/services/enterprise/billing/switch_currency_service_spec.rb +++ b/spec/enterprise/services/enterprise/billing/switch_currency_service_spec.rb @@ -57,6 +57,14 @@ describe Enterprise::Billing::SwitchCurrencyService do expect(Stripe::Subscription).to have_received(:cancel).with('sub_usd', { prorate: false }).ordered end + it 'uses a per-attempt idempotency key not derived from the subscription id' do + service.perform + + expect(Stripe::Subscription).to have_received(:create).with( + anything, hash_including(idempotency_key: a_string_matching(/\Aswitch-#{account.id}-[0-9a-f-]{36}\z/)) + ) + end + it 'persists the new currency and clears the pending marker' do service.perform @@ -146,5 +154,67 @@ describe Enterprise::Billing::SwitchCurrencyService do expect(attributes).not_to have_key('billing_currency_switch_pending') end end + + context 'when a free default-plan subscription has a stale price id' do + # Price id no longer in CHATWOOT_CLOUD_PLANS, but product_id still maps to the default (Hacker) plan. + let(:active_subscription) do + Stripe::Subscription.construct_from( + id: 'sub_hacker', status: 'active', quantity: 1, current_period_end: period_end, + plan: { id: 'price_hacker_legacy_usd', product: 'prod_hacker' }, metadata: {} + ) + end + let(:new_subscription) do + Stripe::Subscription.construct_from( + id: 'sub_hacker_brl', status: 'active', quantity: 1, current_period_end: period_end, + plan: { id: 'price_hacker_brl', product: 'prod_hacker' }, metadata: {} + ) + end + + before do + account.update!(custom_attributes: { plan_name: 'Hacker', stripe_customer_id: stripe_customer_id, billing_currency: 'usd' }) + # No payment method available — a default-plan switch must not require one. + allow(Stripe::Customer).to receive(:retrieve).and_return( + Struct.new(:invoice_settings, :default_source).new(Struct.new(:default_payment_method).new(nil), nil) + ) + allow(Stripe::PaymentMethod).to receive(:list).and_return(Struct.new(:data).new([])) + end + + it 'switches without requiring a payment method or a paid-through trial' do + service.perform + + expect(Stripe::Customer).not_to have_received(:retrieve) + expect(Stripe::Subscription).to have_received(:create).with(hash_not_including(:trial_end), anything) + expect(account.reload.custom_attributes['billing_currency']).to eq('brl') + end + end + + context 'when switching from brl to usd' do + let(:target_currency) { 'usd' } + let(:active_subscription) do + Stripe::Subscription.construct_from( + id: 'sub_brl', status: 'active', quantity: 2, current_period_end: period_end, + plan: { id: 'price_business_brl', product: 'prod_business' }, metadata: {} + ) + end + let(:new_subscription) do + Stripe::Subscription.construct_from( + id: 'sub_usd', status: 'trialing', quantity: 2, current_period_end: period_end, + plan: { id: 'price_business_usd', product: 'prod_business' }, metadata: {} + ) + end + + before do + account.update!(custom_attributes: { plan_name: 'Business', stripe_customer_id: stripe_customer_id, billing_currency: 'brl' }) + end + + it 'clears the stripe billing country and locale override' do + service.perform + + expect(Stripe::Customer).to have_received(:update).with( + stripe_customer_id, hash_including(address: { country: '' }, preferred_locales: []) + ) + expect(account.reload.custom_attributes['billing_currency']).to eq('usd') + end + end end end