From c3fc2eb9369a93f00a4b90206646b428dc68625d Mon Sep 17 00:00:00 2001
From: Graeme Porteous <graeme@rgbp.co.uk>
Date: Wed, 27 Nov 2024 09:54:51 +0000
Subject: [PATCH 1/6] Minor cleanup

- Add guard clauses
- Remove brackets around conditional
- Remove leading underscore on variables names
---
 .../stripe_webhooks_controller.rb             | 44 +++++++++----------
 .../stripe_webhooks_controller_spec.rb        | 16 +++----
 2 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/app/controllers/alaveteli_pro/stripe_webhooks_controller.rb b/app/controllers/alaveteli_pro/stripe_webhooks_controller.rb
index 924e1b1347..dcd37f74a3 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
+
+    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)
@@ -125,8 +125,8 @@ def filter_hooks
   end
 
   def plan_matches_namespace?(plan_id)
-    (AlaveteliConfiguration.stripe_namespace == '' ||
-     plan_id =~ /^#{AlaveteliConfiguration.stripe_namespace}/)
+    AlaveteliConfiguration.stripe_namespace == '' ||
+      plan_id =~ /^#{AlaveteliConfiguration.stripe_namespace}/
   end
 
   def plan_ids(items)
diff --git a/spec/controllers/alaveteli_pro/stripe_webhooks_controller_spec.rb b/spec/controllers/alaveteli_pro/stripe_webhooks_controller_spec.rb
index 41a38e282d..f79b62c672 100644
--- a/spec/controllers/alaveteli_pro/stripe_webhooks_controller_spec.rb
+++ b/spec/controllers/alaveteli_pro/stripe_webhooks_controller_spec.rb
@@ -256,10 +256,10 @@ def send_request
       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
+        user = FactoryBot.create(:pro_user)
+        user.pro_account.stripe_customer_id = stripe_event.data.object.customer
+        user.pro_account.save!
+        user
       end
 
       before do
@@ -324,10 +324,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

From 894795868d2196a43c3719ac18fe484f97afd54a Mon Sep 17 00:00:00 2001
From: Graeme Porteous <graeme@rgbp.co.uk>
Date: Wed, 27 Nov 2024 09:52:38 +0000
Subject: [PATCH 2/6] Fix subscription checkouts

Need to pass in the subscription param instead of ID otherwise Pro
Price#retrieve method won't find the Stripe Price instance.

This commit also clears up the variables in Pro Price to make it clearer
what is expected and ensures prices are defined in config/general.yml.
---
 app/models/alaveteli_pro/price.rb                     | 10 ++++++----
 app/views/alaveteli_pro/plans/show.html.erb           |  2 +-
 spec/models/alaveteli_pro/price_spec.rb               |  7 ++++++-
 spec/views/alaveteli_pro/plans/index.html.erb_spec.rb |  2 ++
 4 files changed, 15 insertions(+), 6 deletions(-)

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/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 @@
         </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>
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/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]

From ce41455115a31fcb0e1e26f8b92c8476767c3e44 Mon Sep 17 00:00:00 2001
From: Graeme Porteous <graeme@rgbp.co.uk>
Date: Wed, 27 Nov 2024 10:21:34 +0000
Subject: [PATCH 3/6] Fix Pro Subscription#require_authorisation?

Fixing invalid Stripe payment intent status.

Should be "requires_action" this would result in payments attempts being
left in an incomplete state and the user being shown the generic "try
again" error message when they are signing up.

See: https://docs.stripe.com/payments/paymentintents/lifecycle#intent-statuses
---
 app/models/alaveteli_pro/subscription.rb       | 2 +-
 spec/models/alaveteli_pro/subscription_spec.rb | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/models/alaveteli_pro/subscription.rb b/app/models/alaveteli_pro/subscription.rb
index 7bfc742cc4..f0715d30d1 100644
--- a/app/models/alaveteli_pro/subscription.rb
+++ b/app/models/alaveteli_pro/subscription.rb
@@ -35,7 +35,7 @@ def payment_intent
 
     def require_authorisation?
       invoice_open? && %w[
-        requires_source_action require_action
+        requires_source_action requires_action
       ].include?(payment_intent.status)
     end
 
diff --git a/spec/models/alaveteli_pro/subscription_spec.rb b/spec/models/alaveteli_pro/subscription_spec.rb
index 4a8ab82b69..45bd60128c 100644
--- a/spec/models/alaveteli_pro/subscription_spec.rb
+++ b/spec/models/alaveteli_pro/subscription_spec.rb
@@ -111,9 +111,9 @@
         is_expected.to eq true
       end
 
-      it 'return true if payment intent status is require_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: 'require_action')
+          double('Stripe::PaymentIntent', status: 'requires_action')
         )
         is_expected.to eq true
       end

From dbdda0469a80b520d35f7a2a7f1badcc74a875e8 Mon Sep 17 00:00:00 2001
From: Graeme Porteous <graeme@rgbp.co.uk>
Date: Wed, 27 Nov 2024 10:27:10 +0000
Subject: [PATCH 4/6] Update Pro Subscription#require_authorisation?

Change payment intents with the  "requires_source_action" status from
returning true.

According to payment intent lifecycle this state is for when processing
fails and we should display an error to the user. This change will do
just that.

See: https://docs.stripe.com/payments/paymentintents/lifecycle#intent-statuses
---
 app/models/alaveteli_pro/subscription.rb       | 4 +---
 spec/models/alaveteli_pro/subscription_spec.rb | 7 -------
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/app/models/alaveteli_pro/subscription.rb b/app/models/alaveteli_pro/subscription.rb
index f0715d30d1..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 requires_action
-      ].include?(payment_intent.status)
+      invoice_open? && payment_intent.status == 'requires_action'
     end
 
     def update(attributes)
diff --git a/spec/models/alaveteli_pro/subscription_spec.rb b/spec/models/alaveteli_pro/subscription_spec.rb
index 45bd60128c..09306ffc66 100644
--- a/spec/models/alaveteli_pro/subscription_spec.rb
+++ b/spec/models/alaveteli_pro/subscription_spec.rb
@@ -104,13 +104,6 @@
         allow(subscription).to receive(:invoice_open?).and_return(true)
       end
 
-      it 'return true if payment intent status is requires_source_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 requires_action' do
         allow(subscription).to receive(:payment_intent).and_return(
           double('Stripe::PaymentIntent', status: 'requires_action')

From 8fbf59f721115c06dd45ddf0b2624a8036afc483 Mon Sep 17 00:00:00 2001
From: Graeme Porteous <graeme@rgbp.co.uk>
Date: Wed, 27 Nov 2024 10:54:34 +0000
Subject: [PATCH 5/6] Update failed payment notification

Don't send the notification when the user has no subscriptions which are
past due.
---
 .../stripe_webhooks_controller.rb             |  2 +-
 .../stripe_webhooks_controller_spec.rb        | 39 ++++++++++++++-----
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/app/controllers/alaveteli_pro/stripe_webhooks_controller.rb b/app/controllers/alaveteli_pro/stripe_webhooks_controller.rb
index dcd37f74a3..46ed5bfb9b 100644
--- a/app/controllers/alaveteli_pro/stripe_webhooks_controller.rb
+++ b/app/controllers/alaveteli_pro/stripe_webhooks_controller.rb
@@ -70,7 +70,7 @@ def invoice_payment_succeeded
 
   def invoice_payment_failed
     account = pro_account_from_stripe_event(@stripe_event)
-    return unless account
+    return unless account&.subscription?
 
     AlaveteliPro::SubscriptionMailer.payment_failed(account.user).deliver_now
   end
diff --git a/spec/controllers/alaveteli_pro/stripe_webhooks_controller_spec.rb b/spec/controllers/alaveteli_pro/stripe_webhooks_controller_spec.rb
index f79b62c672..ebb3c2cf0b 100644
--- a/spec/controllers/alaveteli_pro/stripe_webhooks_controller_spec.rb
+++ b/spec/controllers/alaveteli_pro/stripe_webhooks_controller_spec.rb
@@ -255,25 +255,44 @@ def send_request
         StripeMock.mock_webhook_event('invoice.payment_failed')
       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
 

From 05977680b1518b43720295a38b8fd25b63694cb1 Mon Sep 17 00:00:00 2001
From: Graeme Porteous <graeme@rgbp.co.uk>
Date: Wed, 27 Nov 2024 13:49:15 +0000
Subject: [PATCH 6/6] Update Stripe Webhook handling

Before this change, webhooks would be ignored if the main object
plan/price ID didn't match the namespace.

This change means we now look up the price ID against those configured
in config/general.yml.

Note: Since only Stripe plans are returned in the webhooks generated
from the stripe-ruby-mock gem the specs still rely on setting up plans
are can't be migrated to prices.
---
 .../stripe_webhooks_controller.rb             | 12 ++--
 .../stripe_webhooks_controller_spec.rb        | 55 ++++++++++++-------
 2 files changed, 40 insertions(+), 27 deletions(-)

diff --git a/app/controllers/alaveteli_pro/stripe_webhooks_controller.rb b/app/controllers/alaveteli_pro/stripe_webhooks_controller.rb
index 46ed5bfb9b..95d693d475 100644
--- a/app/controllers/alaveteli_pro/stripe_webhooks_controller.rb
+++ b/app/controllers/alaveteli_pro/stripe_webhooks_controller.rb
@@ -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/spec/controllers/alaveteli_pro/stripe_webhooks_controller_spec.rb b/spec/controllers/alaveteli_pro/stripe_webhooks_controller_spec.rb
index ebb3c2cf0b..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,7 +257,9 @@ 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(:customer_id) { stripe_event.data.object.customer }
@@ -298,7 +305,9 @@ def send_request
 
     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
@@ -313,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
@@ -328,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
@@ -371,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
@@ -384,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