Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Procfile.dev
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
web: env RUBY_DEBUG_OPEN=true bin/rails-sandbox server
watch: bin/rails-sandbox g solidus_stripe:install --force --watch
stripe: stripe listen --forward-to http://localhost:3000/solidus_stripe/test/webhooks --events charge.refunded,payment_intent.succeeded,payment_intent.payment_failed,payment_intent.canceled
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ need to append it to a generic webhook endpoint to get the URL for that payment
method. For example:

```ruby
SolidusStripe::PaymentMethod.last.webhook_endpoint_slug
SolidusStripe::PaymentMethod.last.slug
# "365a8435cd11300e87de864c149516e0"
```

Expand Down
4 changes: 3 additions & 1 deletion app/controllers/solidus_stripe/intents_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ def after_confirmation

def load_payment_method
@payment_method = current_order(create_order_if_necessary: true)
.available_payment_methods.find(params[:payment_method_id])
.available_payment_methods
.merge(SolidusStripe::PaymentMethod.with_slug(params[:slug]))
.first!
end
end
27 changes: 12 additions & 15 deletions app/models/solidus_stripe/payment_method.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,11 @@ class PaymentMethod < ::Spree::PaymentMethod
validates :available_to_admin, inclusion: { in: [false] }
validates :preferred_setup_future_usage, inclusion: { in: ['', 'on_session', 'off_session'] }

has_one :webhook_endpoint,
class_name: 'SolidusStripe::WebhookEndpoint',
inverse_of: :payment_method,
dependent: :destroy
has_one :slug_entry, class_name: 'SolidusStripe::SlugEntry', inverse_of: :payment_method, dependent: :destroy

after_create :assign_webhook_endpoint
after_create :assign_slug

# @!attribute [r] webhook_endpoint_slug
# @return [String] The slug of the webhook endpoint for this payment method.
delegate :slug,
to: :webhook_endpoint,
prefix: true
delegate :slug, to: :slug_entry

def partial_name
"stripe"
Expand All @@ -52,6 +45,10 @@ def payment_profiles_supported?
false
end

def self.with_slug(slug)
where(id: SlugEntry.where(slug: slug).select(:payment_method_id))
end

# TODO: re-evaluate the need for this and think of ways to always go throught the intent classes.
def self.intent_id_for_payment(payment)
return unless payment
Expand All @@ -70,12 +67,12 @@ def stripe_dashboard_url(intent_id)
end
end

private
def assign_slug

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think responsibilities were fairer when that method was part of the SlugEntry class, especially considering that the payment method already deals with a lot of stuff (via inheritance from upstream Solidus).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason was (I think) having access to preferred_test_mode (see the other comment), personally I'm fine with both arrangements, with a preference for SlugEntry not needing to know too much about the payment method.

# If there's only one payment method, we can use a default slug.
slug = preferred_test_mode ? 'test' : 'live' if self.class.count == 1
slug = SecureRandom.hex(16) while SlugEntry.exists?(slug: slug) || slug.nil?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure opinionated, but IMHO using recursion made it simpler (for instance, we didn't need to check for slug.nil?) 🙂

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was originally moved in order to check the .count and .preferred_test_mode on the payment method, but could be simplified for sure, e.g. the following is probably equivalent:

  return if slug_entry.present?

  slug = preferred_test_mode ? 'test' : 'live'
  slug = SecureRandom.hex(16) while SlugEntry.exists?(slug: slug)

Although given how rare it is I agree we should do it for readability (vs. performance).


def assign_webhook_endpoint
create_webhook_endpoint!(
slug: WebhookEndpoint.generate_slug
)
create_slug_entry!(slug: slug)
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -14,24 +14,7 @@ module SolidusStripe
#
# We use a slug instead of the payment method ID to be resilient to
# database changes and to avoid guessing about valid endpoint URLs.
class WebhookEndpoint < ::Spree::Base
belongs_to :payment_method,
class_name: 'SolidusStripe::PaymentMethod'

# @api private
def self.generate_slug
SecureRandom.hex(16).then do |slug|
exists?(slug: slug) ? generate_slug : slug
end
end

# Finds the payment method associated with the given slug.
#
# @param slug [String]
# @raise [ActiveRecord::RecordNotFound] if no payment method is found
# @return [SolidusStripe::PaymentMethod]
def self.payment_method(slug)
find_by!(slug: slug).payment_method
end
class SlugEntry < ::Spree::Base
belongs_to :payment_method, class_name: 'SolidusStripe::PaymentMethod'
Comment thread
rainerdema marked this conversation as resolved.
end
end
6 changes: 3 additions & 3 deletions config/routes.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
# frozen_string_literal: true

SolidusStripe::Engine.routes.draw do
scope ':payment_method_id' do
get :after_confirmation, controller: :intents
scope ':slug' do
get :after_confirmation, to: 'intents#after_confirmation'
post :webhooks, to: 'webhooks#create', format: false
end
post '/webhooks/:slug', format: false, to: 'webhooks#create'
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class RenameWebhookEndpointToPaymentMethodSlugEntries < ActiveRecord::Migration[7.0]
def change
rename_table :solidus_stripe_webhook_endpoints, :solidus_stripe_slug_entries
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
data-controller="solidus-stripe-confirm"
data-solidus-stripe-confirm-publishable-key-value="<%= payment.payment_method.preferred_publishable_key %>"
data-solidus-stripe-confirm-client-secret-value="<%= stripe_intent.client_secret %>"
data-solidus-stripe-confirm-return-url-value="<%= solidus_stripe.after_confirmation_url(payment.payment_method) %>"
data-solidus-stripe-confirm-error-base-url-value="<%= solidus_stripe.after_confirmation_url(payment.payment_method, payment_intent: stripe_intent.id) %>"
data-solidus-stripe-confirm-return-url-value="<%= solidus_stripe.after_confirmation_url(payment.payment_method.slug) %>"
data-solidus-stripe-confirm-error-base-url-value="<%= solidus_stripe.after_confirmation_url(payment.payment_method.slug, payment_intent: stripe_intent.id) %>"
data-action="submit@window->solidus-stripe-confirm#confirm"
>
<% end %>
2 changes: 1 addition & 1 deletion lib/solidus_stripe/testing_support/factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
stripe_intent_id { "pm_#{SecureRandom.uuid.delete('-')}" }
end

factory :stripe_webhook_endpoint, class: 'SolidusStripe::WebhookEndpoint' do
factory :stripe_slug_entry, class: 'SolidusStripe::SlugEntry' do
association :payment_method, factory: :stripe_payment_method
slug { SecureRandom.hex(16) }
end
Expand Down
2 changes: 1 addition & 1 deletion lib/solidus_stripe/webhook/event.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class Event
# @api private
class << self
def from_request(payload:, signature_header:, slug:, tolerance: default_tolerance)
payment_method = SolidusStripe::WebhookEndpoint.payment_method(slug)
payment_method = SolidusStripe::PaymentMethod.with_slug(slug).first!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure to get why we unwrapped the procedure to get the payment method. IMHO, that opens gates to duplication and copy-paste programming.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@waiting-for-dev that's a valid concern. The reason it was moved to a slightly lower-level construct like a scope is that it's more versatile and can be mixed and matched with other scopes without generating extra queries more easily, e.g. in this case

@payment_method = current_order(create_order_if_necessary: true)
.available_payment_methods
.merge(SolidusStripe::PaymentMethod.with_slug(params[:slug]))
.first!

stripe_event = Stripe::Webhook.construct_event(
payload,
signature_header,
Expand Down
45 changes: 44 additions & 1 deletion spec/models/solidus_stripe/payment_method_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
it 'creates a webhook endpoint' do
payment_method = create(:stripe_payment_method)

expect(payment_method.webhook_endpoint).to be_present
expect(payment_method.slug_entry).to be_present
end
end
end
Expand Down Expand Up @@ -73,4 +73,47 @@
expect(payment_method.stripe_dashboard_url(123)).to eq(nil)
end
end

describe '.with_slug' do
it 'selects by slug' do
payment_method = create(:stripe_payment_method)

expect(described_class.with_slug(payment_method.slug)).to eq([payment_method])
expect(described_class.with_slug('bad-slug')).to eq([])
end
end

describe '.assign_slug' do
it 'generates a "test" slug for the first payment method in test mode' do
payment_method = build(:stripe_payment_method)

payment_method.save!

expect(payment_method.slug).to eq('test')
end

it 'generates a "live" slug for the first payment method in live mode' do
payment_method = build(:stripe_payment_method, preferred_test_mode: false)

payment_method.save!

expect(payment_method.slug).to eq('live')
end

it 'generates a random hex string' do
_existing_payment_method = create(:stripe_payment_method)
payment_method = create(:stripe_payment_method)

expect(payment_method.slug).to match(/^[0-9a-f]{32}$/)
end

it 'generates a unique slug' do
slug = SecureRandom.hex(16)

create(:stripe_slug_entry, slug: slug)
allow(SecureRandom).to receive(:hex).and_return(slug).and_call_original

expect(create(:stripe_payment_method).slug).not_to eq(slug)
end
end
end
32 changes: 0 additions & 32 deletions spec/models/solidus_stripe/webhook_endpoint_spec.rb

This file was deleted.

4 changes: 2 additions & 2 deletions spec/requests/solidus_stripe/intents_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
order = create(:order_ready_to_complete)
sign_in order.user

get "/solidus_stripe/#{payment_method.id}/after_confirmation"
get "/solidus_stripe/#{payment_method.slug}/after_confirmation"

expect(response.status).to eq(422)
end
Expand All @@ -20,7 +20,7 @@
order = create(:order)
sign_in order.user

get "/solidus_stripe/#{payment_method.id}/after_confirmation?payment_intent=pi_123"
get "/solidus_stripe/#{payment_method.slug}/after_confirmation?payment_intent=pi_123"

expect(response).to redirect_to('/checkout/cart')
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def signature
end

def slug
@payment_method.webhook_endpoint_slug
@payment_method.slug
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion spec/support/solidus_stripe/webhook/request_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ module RequestHelper
# @param slug [String] It allows to override the slug in the payment
# method to simulate an invalid request.
def webhook_request(context, timestamp: context.timestamp)
post "/solidus_stripe/webhooks/#{context.slug}",
post "/solidus_stripe/#{context.slug}/webhooks",
params: context.json,
headers: { webhook_signature_header_key => webhook_signature_header(context, timestamp: timestamp) }
end
Expand Down