search

integrallis / stripe_event

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

Fixed nil error for @event instance variable #2

Closed barancw closed 11 years ago

barancw commented 11 years ago

I noticed that the code that you had put into your application_controller.rb file was never executed in my application. This is where the @event variable was set. After moving this method into your webhook_controller.rb, everything worked fine.

I'm not sure if there was a reason why this ended up in the application_controller in the first place? I'm not that familiar with overriding the application controller.

The other issue that I ran into was the "WARNING: Can't verify CSRF token authenticity." This will throw a 500 error on it's own. Stripe will never send this correctly. I added in a workaround for this that I found for this so that it will be ignored in this controller.

Last thing I did was bump the version number for you by .1 so you can re-create the gem.

I left the application_controller.rb file in the repository for you to check out, but I think it's now safe to remove. Let me know if anything I did here doesn't make sense.

Cheers, Chris

travisbot commented 11 years ago

This pull request passes (merged 82f6e899 into e2eb92cb).

travisbot commented 11 years ago

This pull request passes (merged 0208bbeb into e2eb92cb).

travisbot commented 11 years ago

This pull request passes (merged 250f051f into e2eb92cb).

travisbot commented 11 years ago

This pull request passes (merged c1dfdd1c into e2eb92cb).

travisbot commented 11 years ago

This pull request passes (merged e9fea12a into e2eb92cb).

invisiblefunnel commented 11 years ago

@barancw Thank you for the work you put into this pull request. I really appreciate it. I have a few concerns and ideas which I will address as soon as I'm able.

barancw commented 11 years ago

No problem @invisiblefunnel,

Keep me in the loop. I'm using the code from this pull in my app now. I'm interested in making sure this gets rolled forward so I can use future versions of this gem. I think this is a great effort you've made here and it makes dealing with the hooks very simple.

invisiblefunnel commented 11 years ago

@barancw,

I noticed that the code that you had put into your application_controller.rb file was never executed in my application. This is where the @event variable was set. After moving this method into your webhook_controller.rb, everything worked fine.

I haven't been able to recreate this behavior. It is working properly in the specs and in the apps I've deployed.

I'm not sure if there was a reason why this ended up in the application_controller in the first place? I'm not that familiar with overriding the application controller.

It was just a convenient place to put it when I was sketching out the gem. As your pull request demonstrates, it could instead be in the webhook controller. More likely I'll move it into the event action, or push it down into lib.

The other issue that I ran into was the "WARNING: Can't verify CSRF token authenticity." This will throw a 500 error on it's own. Stripe will never send this correctly. I added in a workaround for this that I found for this so that it will be ignored in this controller.

See my comment on #1 about this. I don't know why you were seeing this warning. In your pull request, you are subclassing the parent app's ApplicationController and not the engine's, which is why you're needing to skip_before_filter :verify_authenticity_token.

I really like your additions to the README about the test webhooks button and using the dashboard to test manually. Would you create a separate pull request for those? I don't think the screenshot is necessary. I'd like the README to match the most recent release (0.3.0) so there's no need to update it to include the configure method. I'll take care of bumping the version.

I'll need more information or a failing test case to know whether the behavior you saw was a bug in stripe_event. Thanks again for using the gem and for your feedback/contributions. I'll leave this pull request open, in case you want to investigate some more.

Best, Danny Whalen @invisiblefunnel

bousquet commented 11 years ago

I'm running into similar errors as @barancw mentioned. Specifically, I'm getting:

NoMethodError (undefined method `type' for nil:NilClass):
  stripe_event (0.3.0) lib/stripe_event.rb:12:in `publish'

and the CSRF warning as well. Full Stacktrace I'll test this patch next and report my results.

bousquet commented 11 years ago

This patch fixed the application issues I described. +1

invisiblefunnel commented 11 years ago

@bousquet Gotcha. I will reopen #1 and investigate this further. /cc @barancw

barancw commented 11 years ago

@invisiblefunnel - I can separate out the pull requests if you'll do me a favor and create a branch for the 0.3.x line. I'm not sure what commit you made the gem out of. I'm not sure if you made other changes past the keyword "registration" to "configure." If you make that branch, I'll pull that in a make a separate branch for a pull request with the correct doc update.

Sorry for lumping everything together. I'm a little green when it comes to github. I thought it would freeze the commit that I made the pull request out of. I can see that it tries to lump every commit I make. I understand now why they suggest a separate branch per pull request.

I'm going to see if I can dig a bit further. Overriding the application controller from your gem seems to sometimes work and sometimes not work. I'm guessing that they are making updates to rails that is causing errors with this behavior. It makes sense to me that a gem would not be allowed to modify my app's application controller.

I think what you may have been thinking is that the gem itself has a local version of it's own application controller. Right now, all of this is unconfirmed but I'll keep digging and come up with a good answer.

barancw commented 11 years ago

Just found out that it is Bundler's job to include the gems. This means that we may be on the same rails version but if we have different versions of Bundler running this may be the source of the different behavior. I'm running Bundler version 1.1.5. I had to upgrade rather recently because of some issues with capistrano calling a bundle install.

I'm watching through the railscasts on initialization: http://railscasts.com/episodes/299-rails-initialization-walkthrough. It is a pro episode though so if you don't have the subscription, you can't view it.

I'm going to keep digging and see if I can get a better answer on what bundler will do with an application controller override.

bousquet commented 11 years ago

I'm using Rails 3.2.8, Bundler 1.1.5 on ruby-1.9.3-p194.

invisiblefunnel commented 11 years ago

@barancw @bousquet I will take a closer look at this tonight. I pushed a branch which might fix the issue. Please try it out if you have a chance.

bousquet commented 11 years ago

@barancw, I tested your branch and it works fine. However, I took a stab at an even simpler approach and submitted a separate pull request. I also used a feature branch and ran the tests to make sure it's all clean. If accepted, you can bump the version on your own. See https://github.com/integrallis/stripe_event/pull/3

invisiblefunnel commented 11 years ago

@barancw and @bousquet thanks for all your help. v0.3.1 is out.

@barancw You can submit pull requests against master now that everything is squared away. Cheers to a successful bug hunt.