Add support for Stripe webhooks#157
Conversation
waiting-for-dev
force-pushed
the
waiting-for-dev/webhooks
branch
2 times, most recently
from
71a3090 to
a812d5d
Compare
kennyadsl
left a comment
kennyadsl
left a comment
There was a problem hiding this comment.
Left some comments but overall looks great!
| respond_to :json | ||
|
|
||
| def create | ||
| request.body.read |
There was a problem hiding this comment.
Thoughts on moving this to a service? I read your point that we are not ready with the infrastructure, and that makes sense. But when we will, and want to convert this, it will probably be easier because we can deprecate that class.
No strong preference here: in the @elia's PR there are several controllers not using services, so this is more a general discussion about the approach we want to take.
There was a problem hiding this comment.
Some of this could probably be kept inside SolidusStripe::Webhook::Event, something like:
def (SolidusStripe::Webhook::Event).from_request(request)
tolerance = SolidusStripe.configuration.webhook_signature_tolerance
secret = Rails.application.credentials.solidus_stripe[:webhook_endpoint_secret] # move to a configuration?
signature_header = request.headers[SIGNATURE_HEADER]
Stripe::Webhook.construct_event(payload, signature_header, secret, tolerance: tolerance)
endThere was a problem hiding this comment.
Moving things to services requires some decisions to be made, like the interface we create for a result object. I think it doesn't pay off to build a skeleton here that will need to be deprecated later. At most, as @elia suggested, we can try to move some things into collaborators. The from_request method makes sense, but I'd also move the creation of the SolidusStripe::Webhook::Event there. This way, an instance of the collaborator will be involved. I'd mark the method as @api private so we're free to remove it later. Thoughts?
There was a problem hiding this comment.
I think I need to see the code in order to understand fully the proposal, but I'm down with it from what I understand… and it's way easier to go from @api private to @api public than the other way around 🙌
The main goal is to have building block that are easy to reuse in case the target app wants (or needs) to customize or redo the controller.
There was a problem hiding this comment.
Please, take a look now. I moved the logic other than publishing the event to a class method in SolidusStripe::Webhook::Event.
| respond_to :json | ||
|
|
||
| def create | ||
| request.body.read |
There was a problem hiding this comment.
Some of this could probably be kept inside SolidusStripe::Webhook::Event, something like:
def (SolidusStripe::Webhook::Event).from_request(request)
tolerance = SolidusStripe.configuration.webhook_signature_tolerance
secret = Rails.application.credentials.solidus_stripe[:webhook_endpoint_secret] # move to a configuration?
signature_header = request.headers[SIGNATURE_HEADER]
Stripe::Webhook.construct_event(payload, signature_header, secret, tolerance: tolerance)
end
elia
force-pushed
the
elia/152-implement-the-standard-gateway-interfaces-using-the-stripe-gem
branch
from
a8bf830 to
d6a7c23
Compare
waiting-for-dev
changed the base branch from
elia/152-implement-the-standard-gateway-interfaces-using-the-stripe-gem
to
gsmendoza/sol-523-enable-stale-bot-on-all-solidusio
waiting-for-dev
changed the base branch from
gsmendoza/sol-523-enable-stale-bot-on-all-solidusio
to
elia/152-implement-the-standard-gateway-interfaces-using-the-stripe-gem
waiting-for-dev
force-pushed
the
waiting-for-dev/webhooks
branch
2 times, most recently
from
bc88704 to
0019b9a
Compare
|
I'm currently investigating the issue on CI on Ruby 2.7. When doing: .then { |stripe_event| SolidusStripe::Webhook::Event.new(stripe_event: stripe_event) }
.then { |event| Spree::Bus.publish(event) }The event yield on the second step is a legit |
|
@waiting-for-dev would probably be wiser to use |
@elia, the |
Yes, I meant that's the root cause in my opinion, combined with 2.7 handling of keyword-arguments splatting and potentially the |
When a method accepts both splatted positional & keyword parameters and
a single argument responding to `#to_hash` is given, the argument is
coerced to hash and assigned to the keyword arguments.
E.g.:
class Hashable
def to_hash
{ im: :a_hash }
end
end
def method_with_splat_on_the_positional_argument(*args, **kwargs) # method_with_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => []
puts kwargs.inspect # => {:im=>:a_hash}
end
However,
def method_without_splat_on_the_positional_argument(args, **kwargs) # method_without_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => #<Hashable:0x000055cfe8b072b0>
puts kwargs.inspect # => {}
end
We fix the problem on the Spree::Bus override by being explicit about
the expected parameters.
References solidusio/solidus_stripe#157 (comment)
Thanks, @elia, it turned out to be related to an override in Solidus itself. See solidusio/solidus#4875 |
When a method accepts both splatted positional & keyword parameters and
a single argument responding to `#to_hash` is given, the argument is
coerced to hash and assigned to the keyword arguments.
E.g.:
class Hashable
def to_hash
{ im: :a_hash }
end
end
def method_with_splat_on_the_positional_argument(*args, **kwargs) # method_with_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => []
puts kwargs.inspect # => {:im=>:a_hash}
end
However,
def method_without_splat_on_the_positional_argument(args, **kwargs) # method_without_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => #<Hashable:0x000055cfe8b072b0>
puts kwargs.inspect # => {}
end
We fix the problem on the Spree::Bus override by being explicit about
the expected parameters.
References solidusio/solidus_stripe#157 (comment)
When a method accepts both splatted positional & keyword parameters and
a single argument responding to `#to_hash` is given, the argument is
coerced to hash and assigned to the keyword arguments.
E.g.:
class Hashable
def to_hash
{ im: :a_hash }
end
end
def method_with_splat_on_the_positional_argument(*args, **kwargs) # method_with_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => []
puts kwargs.inspect # => {:im=>:a_hash}
end
However,
def method_without_splat_on_the_positional_argument(args, **kwargs) # method_without_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => #<Hashable:0x000055cfe8b072b0>
puts kwargs.inspect # => {}
end
We fix the problem on the Spree::Bus override by being explicit about
the expected parameters.
References solidusio/solidus_stripe#157 (comment)
When a method accepts both splatted positional & keyword parameters and
a single argument responding to `#to_hash` is given, the argument is
coerced to hash and assigned to the keyword arguments.
E.g.:
class Hashable
def to_hash
{ im: :a_hash }
end
end
def method_with_splat_on_the_positional_argument(*args, **kwargs) # method_with_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => []
puts kwargs.inspect # => {:im=>:a_hash}
end
However,
def method_without_splat_on_the_positional_argument(args, **kwargs) # method_without_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => #<Hashable:0x000055cfe8b072b0>
puts kwargs.inspect # => {}
end
We fix the problem on the Spree::Bus override by being explicit about
the expected parameters.
References solidusio/solidus_stripe#157 (comment)
When a method accepts both splatted positional & keyword parameters and
a single argument responding to `#to_hash` is given, the argument is
coerced to hash and assigned to the keyword arguments.
E.g.:
class Hashable
def to_hash
{ im: :a_hash }
end
end
def method_with_splat_on_the_positional_argument(*args, **kwargs) # method_with_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => []
puts kwargs.inspect # => {:im=>:a_hash}
end
However,
def method_without_splat_on_the_positional_argument(args, **kwargs) # method_without_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => #<Hashable:0x000055cfe8b072b0>
puts kwargs.inspect # => {}
end
We fix the problem on the Spree::Bus override by being explicit about
the expected parameters.
References solidusio/solidus_stripe#157 (comment)
waiting-for-dev
force-pushed
the
waiting-for-dev/webhooks
branch
from
0019b9a to
d6fc466
Compare
When a method accepts both splatted positional & keyword parameters and
a single argument responding to `#to_hash` is given, the argument is
coerced to hash and assigned to the keyword arguments.
E.g.:
class Hashable
def to_hash
{ im: :a_hash }
end
end
def method_with_splat_on_the_positional_argument(*args, **kwargs) # method_with_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => []
puts kwargs.inspect # => {:im=>:a_hash}
end
However,
def method_without_splat_on_the_positional_argument(args, **kwargs) # method_without_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => #<Hashable:0x000055cfe8b072b0>
puts kwargs.inspect # => {}
end
We fix the problem on the Spree::Bus override by being explicit about
the expected parameters.
References solidusio/solidus_stripe#157 (comment)
waiting-for-dev
force-pushed
the
waiting-for-dev/webhooks
branch
from
d6fc466 to
a598424
Compare
We set up the backbone to support Stripe webhook [1] events. There're two different use cases: - Users want to listen to certain webhook events. They'll need to add their names in a `webhook_events` setting. - This extension code makes internal use of some of them. That's something programmed, and when the time comes, we'll need to add the event names in the `SolidusStripe::Webhook::Event::CORE_EVENTS` constant. A new `POST /solidus_stripe/webhooks` endpoint exists to support the integration. Users must also configure a `solidus_stripe.webhook_endpoint_secret` Rails credential to validate the incoming events [2]. There's also a `webhook_signature_tolerance` setting to change the default number of seconds while a signature is valid [3]. When an incoming webhook arrives, a matching `Spree::Bus` event prepended with `stripe.` is published. Therefore, all the usual techniques in Solidus's pub/sub system [4] are allowed. [1] - https://stripe.com/docs/webhooks [2] - https://stripe.com/docs/webhooks/signatures [3] - https://stripe.com/docs/webhooks/signatures#replay-attacks [4] - https://guides.solidus.io/customization/subscribing-to-events
waiting-for-dev
force-pushed
the
waiting-for-dev/webhooks
branch
from
a598424 to
9779798
Compare
|
@elia, @kennyadsl, I have addressed your suggestions. Please, could you take another look? I also added the new settings to the initializer template (I had forgotten) and refactored the test fixture to easily share them. |
elia
left a comment
elia
left a comment
There was a problem hiding this comment.
Awesome!
I tried to understand what's going on with the sqlite failure in the CI but couldn't from the diff, any ideas @waiting-for-dev?
We already talked about it offline, but for context this is the issue I talked in #157 (comment) |
When a method accepts both splatted positional & keyword parameters and
a single argument responding to `#to_hash` is given, the argument is
coerced to hash and assigned to the keyword arguments.
E.g.:
class Hashable
def to_hash
{ im: :a_hash }
end
end
def method_with_splat_on_the_positional_argument(*args, **kwargs) # method_with_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => []
puts kwargs.inspect # => {:im=>:a_hash}
end
However,
def method_without_splat_on_the_positional_argument(args, **kwargs) # method_without_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => #<Hashable:0x000055cfe8b072b0>
puts kwargs.inspect # => {}
end
We fix the problem on the Spree::Bus override by being explicit about
the expected parameters.
References solidusio/solidus_stripe#157 (comment)
When a method accepts both splatted positional & keyword parameters and
a single argument responding to `#to_hash` is given, the argument is
coerced to hash and assigned to the keyword arguments.
E.g.:
class Hashable
def to_hash
{ im: :a_hash }
end
end
def method_with_splat_on_the_positional_argument(*args, **kwargs) # method_with_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => []
puts kwargs.inspect # => {:im=>:a_hash}
end
However,
def method_without_splat_on_the_positional_argument(args, **kwargs) # method_without_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => #<Hashable:0x000055cfe8b072b0>
puts kwargs.inspect # => {}
end
We fix the problem on the Spree::Bus override by being explicit about
the expected parameters.
References solidusio/solidus_stripe#157 (comment)
(cherry picked from commit d69f8d7)
When a method accepts both splatted positional & keyword parameters and
a single argument responding to `#to_hash` is given, the argument is
coerced to hash and assigned to the keyword arguments.
E.g.:
class Hashable
def to_hash
{ im: :a_hash }
end
end
def method_with_splat_on_the_positional_argument(*args, **kwargs) # method_with_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => []
puts kwargs.inspect # => {:im=>:a_hash}
end
However,
def method_without_splat_on_the_positional_argument(args, **kwargs) # method_without_splat_on_the_positional_argument(Hashable.new)
puts args.inspect # => #<Hashable:0x000055cfe8b072b0>
puts kwargs.inspect # => {}
end
We fix the problem on the Spree::Bus override by being explicit about
the expected parameters.
References solidusio/solidus_stripe#157 (comment)
(cherry picked from commit d69f8d7)
3 participants
Summary
We set up the backbone to support Stripe webhook events. There're two different use cases:
webhook_eventssetting.SolidusStripe::Webhook::Event::CORE_EVENTSconstant.A new
POST /solidus_stripe/webhooksendpoint exists to support the integration. Users must also configure asolidus_stripe.webhook_endpoint_secretRails credential to validate the incoming events. There's also awebhook_signature_tolerancesetting to change the default number of seconds while a signature is valid.When an incoming webhook arrives, a matching
Spree::Busevent prepended withstripe.is published. Therefore, all the usual techniques in Solidus's pub/sub system are allowed.I'd like to move the decoding of the event from a controller to a service, but as we don't have the infrastructure ready on Solidus is better to wait and not implement a custom solution here.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed (
cross them outif they are not):[ ] I have attached screenshots to demo visual changes.[ ] I have opened a PR to update the guides.