adamthedeveloper / wepay-rails

Collect payments from wepay in your rails application.
MIT License
32 stars 24 forks source link

callback_uri - is it possible to make it dynamic? #20

Closed thinkmorebetter closed 12 years ago

thinkmorebetter commented 12 years ago

Hi,

I am doing testing of my app on a number of servers, including localhost, so it'd be nice if there was a way to have the callback_uri reflect that, instead of having to hardcode it in the yaml file. Does anyone know how I could go about doing that?

Thanks, Alex

SteveAquino commented 12 years ago

You can pass any of those options in as part of your params has in any api call. For example:

checkout_params = {
  :amount => cart.grand_total,
  :short_description => cart.short_description,
  :long_description => cart.long_description,
  :callback_uri => my_wepay_callback_path # your path as defined in config/routes.rb
}
init_checkout_and_send_user_to_wepay(checkout_params)

That's just one example, but as I said, any API call can override the default options.

adamthedeveloper commented 12 years ago

You can also create a wepay.yml.erb file in your config folder which allows you to make your yml values dynamic. Am I right Steve?

SteveAquino commented 12 years ago

Yes that's true, allowing you to use ENV variables or some other method to dynamically set any of those values.

On 06/27/2012 08:04 PM, Adam Medeiros wrote:

You can also create a wepay.yml.erb file in your config folder which allows you to make your yml values dynamic. Am I right Steve?


Reply to this email directly or view it on GitHub: https://github.com/adamthedeveloper/wepay-rails/issues/20#issuecomment-6620916

thinkmorebetter commented 12 years ago

Unfortunately, I couldn't find any way to extract the "localhost" hostname from any ENV variables. The best option was Socket.gethostname, but that gave me my machine's name rather than localhost.

I ended up passing in my own redirect_uri to the params for the checkout, but I had to make a small change to the gem to add the security_token to them before sending them to Wepay. Here's the change:

https://github.com/ChromeBurner/wepay-rails/commit/59c2568eeeba25aa855b6af42572d22bac4dd1bc

Also, I changed the way routes are setup for the gem's controllers. The way it was done before wasn't working for me because I have a match "*a" rule at the end of my routes.rb which superseded all the routes added by wepay-rails. Instead, I have the gem add its routes at the top of the file.

https://github.com/ChromeBurner/wepay-rails/commit/33c93dfea37be2a0d43812ba849f0060ff55a1c3

Thanks for your help guys.

adamthedeveloper commented 12 years ago

I am liking these changes. Can you add some tests around these and send me a pull request? I'd love to add them to the gem.

SteveAquino commented 12 years ago

Yes I agree, the security token thing was puzzling me as well, great solution!

thinkmorebetter commented 12 years ago

What kind of tests are you looking for? I'm not too familiar with the testing framework, to be honest.

SteveAquino commented 12 years ago

Currently the gem is using the default Rails testing framework, which is basically and extension of Test::Unit if I'm not mistaken. Testing is pretty simple, just check out the Ruby on Rails guides, they have a guide for tests that will cover pretty much all you'd need to know.
Beyond that, the major weakness with the current tests is that they all call the Wepay API in each test, they should really be stubbed API calls to improve performance and reliability of the tests. In all honesty I meant to work on that this weekend, but I've just got too much going on and haven't gotten to it yet.

On 06/30/2012 04:52 PM, Alex Perelman wrote:

What kind of tests are you looking for? I'm not too familiar with the testing framework, to be honest.


Reply to this email directly or view it on GitHub: https://github.com/adamthedeveloper/wepay-rails/issues/20#issuecomment-6689589

SteveAquino commented 12 years ago

Hey ChromeBurner, just wondered if you were able to figure out some tests or if you needed some help. Also, the changes made relate to the gem's config/routes.rb priority for writing custom routes, as well as apply the security token to the callback and redirect uri params. I'm just wondering if the issues you were having were related to the routing priority or if there was something else going on.

thinkmorebetter commented 12 years ago

Well, I'm just not quite sure about the kinds of tests you're looking for or how to implement them.

The routes issue was certainly due to the priority because all my /wepay/ calls were being caught by my `match "*a" rule at the end that I use to catch all bad requests.

SteveAquino commented 12 years ago

Well, like I said, the only parts of the code you changed were to update the way custom routes are written, and to apply the security token to the custom uris in the params hash when creating a checkout. In theory, those are the only two things you need to test for. For example, one test would simply make sure all the custom routes are both written and properly stored as routes (example: assert_equal "wepay/checkout", wepay_checkout_path or something along those lines), and another test would create a new checkout object, passing custom callback and redirect uri paramters, and then make sure the response has a security token applied.

These could be further extended to assure proper response from the gem's default controller actions as well, but since we're talking about custom uris, that doesn't exactly apply to this particular update.

If you're still not sure I can write up the tests, just give me an hour or so to finish what I'm working on right now.

thinkmorebetter commented 12 years ago

Is there some special way I need to run these tests? I'm getting the following error with your example when I run it via ruby -Itest test/test_wepay_rails_routes.rb

    ERROR should have correct checkout routes (0.81s) 
          NameError: undefined local variable or method `wepay_checkout_path' for #<TestWepayRailsRoutes:0x007fadaab40578>
          test/test_wepay_rails_routes.rb:11:in `block in <class:TestWepayRailsRoutes>'

Or maybe I need to include the url helpers somehow?

SteveAquino commented 12 years ago

Well it looks like you need to actually load an instance of a Rails app to get the routing that way... Other gems (such as Devise) actually do have a test app within the test directory. As in rails new inside the gem's test directory, then adding a few lines to our test helper.

In short, it's a little more complicated. I'd be happy to work on this, just give me a little bit.

SteveAquino commented 12 years ago

... OK so I guess that to test both of these changes we're going to need to create a mini test app within the test directory to properly test for everything. I've pulled the changes ChromeBurner made into my own fork, and finished the changes that I've made. I added some new tests as well, but this doesn't properly test any of the changes on this issue thread. To be sure, I pushed everything to my own fork and made a fresh rails app, installed our gem, and ran the generators, and everything is working properly. So I'm wondering if I should just go ahead and push these recent changes since they contain some important updates, specifically handling timeouts and applying the security token properly.

My changes basically involve replacing HTTParty with Tophoeus to deal with timeout issues. It seems that certain "invalid" requests are simply ignored by WePay specifically when they're updating the API, so this change addresses that by setting a default timeout at 30 seconds, where it raises a custom error from within the gem. You can also override the default timeout by passing an optional 3rd value to call_api that is an integer in milliseconds. In the future this could additionally be an option in the wepay.yml file that gets loaded on initialization, but also could still be overridden as a method parameter. This has the advantage of always raising the same error on time out and not being dependent on the server environment.

SteveAquino commented 12 years ago

It's been over a week and I still haven't gotten a response if I should push out the above changes so we can continue working on the gem. If I do this, any one who updates should be fine with the exception that there's a new error that's raised on timeout to avoid cross platform issues when WePay blocks our API calls. If I don't get a response i'll probably just push it out and then, as I said, we can move forward with proper testing by creating a mini app to initalize and make calls to/from.

thinkmorebetter commented 12 years ago

That sounds good. I'm happy to help make the tests once you get that mini app up and running. You may also want to grab my very latest change. I just made the same access_token change for the ipn_controller as I had earlier for the checkout_controller.

adamthedeveloper commented 12 years ago

Hey Guys, HTTparty doesn't handle timeouts in a good way? Sorry about coming late to the party but what are we trying to solve? Before we go this route, I'd like to suggest having a look at incorporating the wepay ruby sdk: https://github.com/wepay/Ruby-SDK

If this change is just so we can run some tests when wepay makes some changes, I think we should rethink it. I don't want to railroad the developers on this project though - so let's do what's best for the project - if that means swapping out HTTParty for Typh(I can't spell it and I am too lazy to copy, paste) - then I am for it.

SteveAquino commented 12 years ago

I too had thoughts on just integrating the ruby-sdk gem, and just adding the specific features for Rails. That seems logical, and perhaps if they needed help with their API would could collaborate instead of rewriting the wheel. I haven't looked into that gem yet, but it's certainly seems logical instead of require various other gems.

On 07/11/2012 01:28 PM, Adam Medeiros wrote:

Hey Guys, HTTparty doesn't handle timeouts in a good way? Sorry about coming late to the party but what are we trying to solve? Before we go this route, I'd like to suggest having a look at incorporating the wepay ruby sdk: https://github.com/wepay/Ruby-SDK

If this change is just so we can run some tests when wepay makes some changes, I think we should rethink it. I don't want to railroad the developers on this project though - so let's do what's best for the project - if that means swapping out HTTParty for Typh(I can't spell it and I am too lazy to copy, paste) - then I am for it.


Reply to this email directly or view it on GitHub: https://github.com/adamthedeveloper/wepay-rails/issues/20#issuecomment-6918362

adamthedeveloper commented 12 years ago

Yeah - makes perfect sense we should use theirs. It's nice and light too! Hopefully it can handle what you were trying to solve though.

SteveAquino commented 12 years ago

Yeah it is, it's just two files! Changing our custom api calls would trivial.

SteveAquino commented 12 years ago

I don't think the problem here is actually as the title describes, because what really happened was that WePay suddenly required a User-Agent string in the header of all api calls. The updates chromburner have done address that, the changes I made were because, from what I could tell, HTTParty simply doesn't handle timeouts, which is fine, except that if I run a test, run development, and run on heroku, all the timeout errors are different, which makes it impossible to determine if WePay is kicking us out or if there's some restriction with the server. You would have been able to pass in a custom timeout integer representing in milliseconds how long a request could take, I think it defaults to 30 seconds.

Besides that Typheous just seemed a little more robust, but this could all be overkill for all intents and purposes of the gem. The reality is we can't get tests to consistently pass on Travis when WePay updates their stuff, making development difficult (or altogether impossible, arg!). Personal frustrations with WePay aside, the gem is authored by someone who works at WePay, and I think honoring the simplicity of their one page declaration will save us headache and frustration. They use HTTP::Net, so I don't know how timeouts are handled there, but then again, like I say, this may be overkill.

adamthedeveloper commented 12 years ago

I just opened an issue on them to write real tests on their gem that talks to their API and makes sure all is ok. Since they control both sides, it would be ideal AND it would save us from writing tests that interact with their API. Let's see what they come back and say.

Thanks for putting the thought into making the gem better! I am all for that.

SteveAquino commented 12 years ago

Any word from the wepay/Ruby-SDK team?

adamthedeveloper commented 12 years ago

None at all.

On Sat, Aug 4, 2012 at 2:55 PM, Steve Aquino reply@reply.github.com wrote:

Any word from the wepay/Ruby-SDK team?


Reply to this email directly or view it on GitHub: https://github.com/adamthedeveloper/wepay-rails/issues/20#issuecomment-7505799

A.R. Medeiros

SteveAquino commented 12 years ago

I'd like to take a stab at this, but from what I can tell their SDK is not a gem, just a script. Is there a way require this with bundler or would we basically be copying their code into our gem?

SteveAquino commented 12 years ago

OK, I removed Typhoeus and reintegrated HTTParty, added some error handling for timeouts, and stubbed the API calls in the tests. The only major difference people will notice is that you'll now get a consistent error (specifically WepayRails::Exceptions::WepayApiError) instead of various errors dependent on the server. To be safe I'm not pushing this yet until I know things on my own project are working, but feel free to clone from here: https://github.com/DragonStarWebDesign/wepay-rails

Assuming everything works right I'll go ahead and push this update to the main branch shortly.

adamthedeveloper commented 12 years ago

That's great Steve. I am sorry for going dark on this project - I started up with a new company and getting into the swing of things has taken a lot of my time. I'll get back to wepay-rails soon. There are some new features that need to be folded in. Thanks again.

SteveAquino commented 12 years ago

OK just pushed updates, everything works great, and we can now move onto adding some of the new features.