integrallis / stripe_event

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

Subscribe to `account.application.deauthorized` events #32

Closed rmm5t closed 10 years ago

rmm5t commented 10 years ago

When a Stripe Connect application receives an account.application.deauthorized event, the tokens have already been decommissioned, so the event_retriever is pretty much guaranteed to throw a Stripe::AuthenticationError, and no subsequent event subscriptions fire.

Instead, if we receive a Stripe::AuthenticationError from a known account and the type is account.application.deauthorized, we can be confident that this was an authorized webhook from Stripe. Under this scenario, construct a Stripe::Event from the passed params instead of retrieving the Event from Stripe's API and fire the subscribed hooks directly.

Fixes #15

rmm5t commented 10 years ago

It's worth noting that event.user_id will be set on this type of event which can be used to determine which account was deauthorized.

Also, I think this scenario requires a customized event_retriever, but the logic for catching the unauthorized exception is outside of it, so the event_retriever can stay naïve to the intricate details of the account.application.deauthorized event type behavior.

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling f1ced39d1bac76b18fe2dd22299e0cfebd521576 on rmm5t:account-application-deauthorized into b1645eb0b200523447fd7cea46871214b2d63165 on integrallis:master.

invisiblefunnel commented 10 years ago

@rmm5t thank you! This pull request made my day. I will merge and cut a new release ASAP. If we're trying to follow semver, is 1.2.0 the proper release version?

rmm5t commented 10 years ago

Yes, I think 1.2.0 is probably prudent. It's not a backwards incompatible change, but it is a new feature and one that could affect someone who has an unusually custom event_retriever implementation.

I haven't yet had a chance to fully test this in a live Stripe Connect application though, but I have verified that Stripe's API returns the error that we're rescuing. I'll be able to fully test this in a Stripe Connect application by tomorrow morning. If you want to wait until I get you that verification, I understand.

invisiblefunnel commented 10 years ago

Sounds reasonable. I'll wait for your confirmation - there's no rush on my end.

rmm5t commented 10 years ago

@invisiblefunnel I just tested this in a live application and found a mild problem. To summarize from my commit message:

The Stripe api expects params to be passed into their StripeObject's with symbolized keys, but the params that we pass through from a account.application.deauthorized webhook are a HashWithIndifferentAccess (keys stored as strings always).

I had to ensure that the hash of params passed to the Stripe::Event.construct_from used symbols as keys.

This is now fixed and ready to be pulled in. I've tested the branch successfully against a live Stripe Connect application, and I added a new test to ensure we have coverage of this seemingly unusual scenario.

A new 1.2.0 release would be much appreciated.

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling cf3f4ac22113eb22b8aeb69935f3130cdf99e0cd on rmm5t:account-application-deauthorized into b1645eb0b200523447fd7cea46871214b2d63165 on integrallis:master.

invisiblefunnel commented 10 years ago

@rmm5t thank you for this pull request and for testing the change in a live environment. I've published version 1.2.0 to Rubygems.