Stripe Prices fixes

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

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

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)

app/views/alaveteli_pro/plans/show.html.erb

@@ -65,7 +65,7 @@
         </div>
 
         <div class="settings__section">
-          <%= 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' %>
           <p id="card-errors"></p>

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

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

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

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]