integrallis / stripe_event

Stripe webhook integration for Rails applications.
https://rubygems.org/gems/stripe_event
MIT License
841 stars 107 forks source link

Use Stripe::Event.construct_from for event_retriever #24

Closed adamonduty closed 10 years ago

adamonduty commented 10 years ago

This commit along with the proper event_retriever allows stripe_event to accept test webhooks directly from stripe.

I learned some interesting things with this commit. First, Stripe::Event.construct_from can be used to convert json into a Stripe::Event. That is really useful because it ensures all the behaviors are the same rather than using json directly as the event in the subscribe block.

But this wasn't working at first. Turns out that construct_from is sensitive to symbol keys in the hash. If string keys are used, event[:type] returns nil. Here's an example:

1.9.3p429 :046 > e = Stripe::Event.construct_from(JSON.parse("{\"id\":1}"))
 => #<Stripe::Event:0x2df1be4 id=1> JSON: {"id":1} 
1.9.3p429 :047 > e[:id]
 => nil 
1.9.3p429 :048 > e['id']
 => nil 
1.9.3p429 :049 > e.id
 => 1 
1.9.3p429 :050 > e = Stripe::Event.construct_from(JSON.parse("{\"id\":1}").symbolize_keys)
 => #<Stripe::Event:0x38651d4> JSON: {"id":1} 
1.9.3p429 :051 > e[:id]
 => 1 
1.9.3p429 :052 > e['id']
 => 1 
1.9.3p429 :053 > e.id
 => 1 

I didn't really dig into why this happens, but the supporting test calls the .type method in publish if event[:type] is nil. Here's the event_retriever as it could be implemented:

if Rails.env.development? || Rails.env.test?
  StripeEvent.event_retriever = lambda {|params| Stripe::Event.construct_from(params) }
end

This works nicely for me during testing. I hope you'll consider merging it.

ajsharp commented 10 years ago

Any movement on this? I'd love to see this merged as well.

adamonduty commented 10 years ago

I'm using this event_retriever for now on version 0.6.1:

StripeEvent.event_retriever = lambda {|params| Stripe::Event.construct_from(params.symbolize_keys) }

Decided that just calling symbolize_keys wasn't so bad in development, but wouldn't want to do that in production.

invisiblefunnel commented 10 years ago

The solution proposed by @adamonduty (Stripe::Event.construct_from(params.symbolize_keys)) is the best solution if you want to construct full Stripe::Event instances from the request params provided by the test webhooks (evt_0000...) from Stripe. The implementation of StripeObject.construct_from expects a hash with symbol keys and falling back to the accessor method as a workaround feels incomplete. This is definitely food for thought w.r.t. the event_retriever implementation though, so I'm still open to changes. Thank you for the pull request and sorry for letting it sit for so long.

ajsharp commented 10 years ago

Cool, thanks for the explanation.

octocall.com https://www.octocall.com @ajsharp https://twitter.com/ajsharp

On Thu, Dec 5, 2013 at 9:00 AM, Danny Whalen notifications@github.comwrote:

Closed #24 https://github.com/integrallis/stripe_event/pull/24.

— Reply to this email directly or view it on GitHubhttps://github.com/integrallis/stripe_event/pull/24 .

adamonduty commented 10 years ago

Thanks for feedback. I agree the behavior tested in the pull is a bit obscure.

With this event retriever, a little stripe-ruby-mock, and some other minor changes I've written a set of unit tests. I don't know about you guys, but billing code without unit tests scares the crap out of me. Are there any samples out there for testing our hooks?

invisiblefunnel commented 10 years ago

Here is an example app showing how I recommend testing webhooks: https://github.com/invisiblefunnel/test-hooks. The key part is to stub the HTTP request to Stripe in event_retriever and return a known response.