diff --git a/app/controllers/alaveteli_pro/stripe_webhooks_controller.rb b/app/controllers/alaveteli_pro/stripe_webhooks_controller.rb index 924e1b1347..95d693d475 100644 --- a/app/controllers/alaveteli_pro/stripe_webhooks_controller.rb +++ b/app/controllers/alaveteli_pro/stripe_webhooks_controller.rb @@ -53,26 +53,26 @@ def customer_subscription_deleted def invoice_payment_succeeded charge_id = @stripe_event.data.object.charge - if charge_id - charge = Stripe::Charge.retrieve(charge_id) - - subscription_id = @stripe_event.data.object.subscription - subscription = Stripe::Subscription.retrieve( - id: subscription_id, expand: ['plan.product'] - ) - plan_name = subscription.plan.product.name - - Stripe::Charge.update( - charge.id, description: "#{pro_site_name}: #{plan_name}" - ) - end + return unless charge_id + + charge = Stripe::Charge.retrieve(charge_id) + + subscription_id = @stripe_event.data.object.subscription + subscription = Stripe::Subscription.retrieve( + id: subscription_id, expand: ['plan.product'] + ) + plan_name = subscription.plan.product.name + + Stripe::Charge.update( + charge.id, description: "#{pro_site_name}: #{plan_name}" + ) end def invoice_payment_failed account = pro_account_from_stripe_event(@stripe_event) - if account - AlaveteliPro::SubscriptionMailer.payment_failed(account.user).deliver_now - end + return unless account&.subscription? + + AlaveteliPro::SubscriptionMailer.payment_failed(account.user).deliver_now end def store_unhandled_webhook @@ -96,10 +96,10 @@ def pro_account_from_stripe_event(event) end def check_for_event_type - unless @stripe_event.respond_to?(:type) - msg = "undefined method `type' for #{ @stripe_event.inspect }" - raise MissingTypeStripeWebhookError, msg - end + return if @stripe_event.respond_to?(:type) + + msg = "undefined method `type' for #{ @stripe_event.inspect }" + raise MissingTypeStripeWebhookError, msg end def notify_exception(error) @@ -108,7 +108,6 @@ def notify_exception(error) ExceptionNotifier.notify_exception(error, env: request.env) end - # ignore any that don't match our plan namespace def filter_hooks plans = [] case @stripe_event.data.object.object @@ -118,17 +117,14 @@ def filter_hooks plans = plan_ids(@stripe_event.data.object.lines) end - # ignore any plans that don't start with our namespace - plans.delete_if { |plan| !plan_matches_namespace?(plan) } + # ignore any prices that aren't configured + plans.delete_if do |price_id| + !AlaveteliConfiguration.stripe_prices.key?(price_id) + end raise UnknownPlanStripeWebhookError if plans.empty? end - def plan_matches_namespace?(plan_id) - (AlaveteliConfiguration.stripe_namespace == '' || - plan_id =~ /^#{AlaveteliConfiguration.stripe_namespace}/) - end - def plan_ids(items) items.map { |item| item.plan.id if item.plan }.compact.uniq end diff --git a/app/models/alaveteli_pro/price.rb b/app/models/alaveteli_pro/price.rb index 6bec30061f..8195c19593 100644 --- a/app/models/alaveteli_pro/price.rb +++ b/app/models/alaveteli_pro/price.rb @@ -4,6 +4,8 @@ class AlaveteliPro::Price < SimpleDelegator include Taxable + UnknownPrice = Class.new(StandardError) + tax :unit_amount def self.list @@ -12,15 +14,15 @@ def self.list end end - def self.retrieve(id) - key = AlaveteliConfiguration.stripe_prices.key(id) - new(Stripe::Price.retrieve(key)) + def self.retrieve(param) + id = AlaveteliConfiguration.stripe_prices.key(param) + new(Stripe::Price.retrieve(id)) rescue Stripe::InvalidRequestError nil end def to_param - AlaveteliConfiguration.stripe_prices[id] || id + AlaveteliConfiguration.stripe_prices[id] || raise(UnknownPrice) end # product diff --git a/app/models/alaveteli_pro/subscription.rb b/app/models/alaveteli_pro/subscription.rb index 7bfc742cc4..6042959bb9 100644 --- a/app/models/alaveteli_pro/subscription.rb +++ b/app/models/alaveteli_pro/subscription.rb @@ -34,9 +34,7 @@ def payment_intent end def require_authorisation? - invoice_open? && %w[ - requires_source_action require_action - ].include?(payment_intent.status) + invoice_open? && payment_intent.status == 'requires_action' end def update(attributes) diff --git a/app/views/alaveteli_pro/plans/show.html.erb b/app/views/alaveteli_pro/plans/show.html.erb index 5b4be17931..634de6aab1 100644 --- a/app/views/alaveteli_pro/plans/show.html.erb +++ b/app/views/alaveteli_pro/plans/show.html.erb @@ -65,7 +65,7 @@
- <%= hidden_field_tag 'price_id', @price.id %> + <%= hidden_field_tag 'price_id', @price.to_param %> <%= submit_tag _('Subscribe'), id: 'js-stripe-submit', disabled: true, data: { disable_with: 'Processing...' } %> <%= link_to _('Cancel'), pro_plans_path, class: 'settings__cancel-button' %>

diff --git a/spec/controllers/alaveteli_pro/stripe_webhooks_controller_spec.rb b/spec/controllers/alaveteli_pro/stripe_webhooks_controller_spec.rb index 41a38e282d..0528987ca4 100644 --- a/spec/controllers/alaveteli_pro/stripe_webhooks_controller_spec.rb +++ b/spec/controllers/alaveteli_pro/stripe_webhooks_controller_spec.rb @@ -16,7 +16,7 @@ let(:stripe_plan) do stripe_helper.create_plan( - id: 'test', product: product.id, + id: 'plan_123', product: product.id, amount: 10, currency: 'gbp' ) end @@ -26,8 +26,8 @@ plan: stripe_plan.id) end - let(:paid_invoice) do - invoice = Stripe::Invoice.create( + let(:invoice) do + Stripe::Invoice.create( lines: [ { data: { @@ -42,13 +42,16 @@ ], subscription: stripe_subscription.id ) - invoice.pay end + let(:paid_invoice) { invoice.pay } + let(:charge) { Stripe::Charge.retrieve(paid_invoice.charge) } let(:stripe_event) do - StripeMock.mock_webhook_event('customer.subscription.deleted') + StripeMock.mock_webhook_event( + 'customer.subscription.deleted', items: stripe_subscription.items + ) end let(:payload) do @@ -63,8 +66,8 @@ def send_request end before do - allow(AlaveteliConfiguration).to receive(:stripe_namespace). - and_return('') + allow(AlaveteliConfiguration).to receive(:stripe_prices). + and_return('plan_123' => 'pro') allow(AlaveteliConfiguration).to receive(:stripe_webhook_secret). and_return(config_secret) StripeMock.start @@ -183,8 +186,8 @@ def send_request context 'when using namespaced plans' do before do - allow(AlaveteliConfiguration).to receive(:stripe_namespace). - and_return('WDTK') + allow(AlaveteliConfiguration).to receive(:stripe_prices). + and_return('WDTK-test' => 'pro') end context 'the webhook does not reference our plan namespace' do @@ -236,7 +239,9 @@ def send_request context 'the webhook data does not have namespaced plans' do let(:stripe_event) do - StripeMock.mock_webhook_event('invoice.payment_succeeded') + StripeMock.mock_webhook_event( + 'invoice.payment_succeeded', lines: paid_invoice.lines + ) end it 'does not raise an error when trying to filter on plan name' do @@ -252,34 +257,57 @@ def send_request describe 'a payment fails' do let(:stripe_event) do - StripeMock.mock_webhook_event('invoice.payment_failed') + StripeMock.mock_webhook_event( + 'invoice.payment_failed', lines: invoice.lines + ) end - let!(:user) do - _user = FactoryBot.create(:pro_user) - _user.pro_account.stripe_customer_id = stripe_event.data.object.customer - _user.pro_account.save! - _user + let(:customer_id) { stripe_event.data.object.customer } + + let(:pro_account) do + FactoryBot.create(:pro_account, stripe_customer_id: customer_id) end before do - send_request + allow(ProAccount).to receive(:find_by). + with(stripe_customer_id: customer_id).and_return(pro_account) end it 'handles the event' do expect(response.status).to eq(200) end - it 'notifies the user that their payment failed' do - mail = ActionMailer::Base.deliveries.first - expect(mail.subject).to match(/Payment failed/) - expect(mail.to).to include(user.email) + context 'the user has a subscription' do + before do + allow(pro_account).to receive(:subscription?).and_return(true) + send_request + end + + it 'notifies the user that their payment failed' do + mail = ActionMailer::Base.deliveries.first + expect(mail.subject).to match(/Payment failed/) + expect(mail.to).to include(pro_account.user.email) + end + end + + context 'the user does not have a subscription' do + before do + allow(pro_account).to receive(:subscription?).and_return(false) + send_request + end + + it 'does not notify the user that their payment failed' do + mail = ActionMailer::Base.deliveries.first + expect(mail).to be_nil + end end end describe 'a customer moves to a new billing period' do let(:stripe_event) do - StripeMock.mock_webhook_event('subscription-renewed') + StripeMock.mock_webhook_event( + 'subscription-renewed', items: stripe_subscription.items + ) end it 'handles the event' do @@ -294,7 +322,9 @@ def send_request describe 'a trial ends' do let(:stripe_event) do - StripeMock.mock_webhook_event('trial-ended-first-payment-failed') + StripeMock.mock_webhook_event( + 'trial-ended-first-payment-failed', items: stripe_subscription.items + ) end it 'handles the event' do @@ -309,7 +339,9 @@ def send_request describe 'a customer cancels' do let(:stripe_event) do - StripeMock.mock_webhook_event('subscription-cancelled') + StripeMock.mock_webhook_event( + 'subscription-cancelled', items: stripe_subscription.items + ) end it 'handles the event' do @@ -324,10 +356,10 @@ def send_request describe 'a cancelled subscription is deleted at the end of the billing period' do let!(:user) do - _user = FactoryBot.create(:pro_user) - _user.pro_account.stripe_customer_id = stripe_event.data.object.customer - _user.pro_account.save! - _user + user = FactoryBot.create(:pro_user) + user.pro_account.stripe_customer_id = stripe_event.data.object.customer + user.pro_account.save! + user end it 'removes the pro role from the associated user' do @@ -352,9 +384,12 @@ def send_request context 'when there is a charge for an invoice' do let(:stripe_event) do - StripeMock.mock_webhook_event('invoice.payment_succeeded', - charge: paid_invoice.charge, - subscription: stripe_subscription.id) + StripeMock.mock_webhook_event( + 'invoice.payment_succeeded', + lines: paid_invoice.lines, + charge: paid_invoice.charge, + subscription: stripe_subscription.id + ) end it 'updates the charge description with the site and plan name' do @@ -365,8 +400,9 @@ def send_request context 'when there is no charge for an invoice' do let(:stripe_event) do - StripeMock.mock_webhook_event('invoice.payment_succeeded', - charge: nil) + StripeMock.mock_webhook_event( + 'invoice.payment_succeeded', lines: paid_invoice.lines, charge: nil + ) end it 'does not attempt to update the nil charge' do diff --git a/spec/models/alaveteli_pro/price_spec.rb b/spec/models/alaveteli_pro/price_spec.rb index d018dc711b..18f3d9e5ca 100644 --- a/spec/models/alaveteli_pro/price_spec.rb +++ b/spec/models/alaveteli_pro/price_spec.rb @@ -26,7 +26,7 @@ end describe '.retrieve' do - it 'retrieves a price from Stripe' do + it 'retrieves a price for a given param from Stripe' do stripe_price = double('stripe_price') allow(Stripe::Price).to receive(:retrieve).with('pro'). and_return(stripe_price) @@ -44,6 +44,11 @@ expect(price.to_param).to eq('pro') end + + it 'raises UnknownPrice error if price is not defined' do + price = described_class.new(double('stripe_price', id: 'unknown')) + expect { price.to_param }.to raise_error(described_class::UnknownPrice) + end end describe '#product' do diff --git a/spec/models/alaveteli_pro/subscription_spec.rb b/spec/models/alaveteli_pro/subscription_spec.rb index 4a8ab82b69..09306ffc66 100644 --- a/spec/models/alaveteli_pro/subscription_spec.rb +++ b/spec/models/alaveteli_pro/subscription_spec.rb @@ -104,16 +104,9 @@ allow(subscription).to receive(:invoice_open?).and_return(true) end - it 'return true if payment intent status is requires_source_action' do + it 'return true if payment intent status is requires_action' do allow(subscription).to receive(:payment_intent).and_return( - double('Stripe::PaymentIntent', status: 'requires_source_action') - ) - is_expected.to eq true - end - - it 'return true if payment intent status is require_action' do - allow(subscription).to receive(:payment_intent).and_return( - double('Stripe::PaymentIntent', status: 'require_action') + double('Stripe::PaymentIntent', status: 'requires_action') ) is_expected.to eq true end diff --git a/spec/views/alaveteli_pro/plans/index.html.erb_spec.rb b/spec/views/alaveteli_pro/plans/index.html.erb_spec.rb index eaba2f75c1..36f953ed42 100644 --- a/spec/views/alaveteli_pro/plans/index.html.erb_spec.rb +++ b/spec/views/alaveteli_pro/plans/index.html.erb_spec.rb @@ -17,6 +17,8 @@ end before do + allow(AlaveteliConfiguration).to receive(:stripe_prices). + and_return('price_123' => 'pro') allow(AlaveteliConfiguration).to receive(:iso_currency_code). and_return('GBP') assign :prices, [price]