New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update OAuth1Binding to allow post and get to receive params #539

Merged
merged 2 commits into from Dec 12, 2012

Conversation

Projects
None yet
3 participants
@timhaines
Contributor

timhaines commented Dec 11, 2012

Adjusted OAuth1Binding

  • Added a 'post' convenience method
  • adjusted get and call to accept params
  • adjusted _getSignature to properly handle the params.

This allows OAuth1Binding to be used to call Twitter API endpoints that require parameters.

@avital

This comment has been minimized.

Contributor

avital commented Dec 12, 2012

Looks good to me but I want to make sure -- did you verify that the change to _getSignature indeed works? Is there a simple way I can test it?

@timhaines

This comment has been minimized.

Contributor

timhaines commented Dec 12, 2012

@avital Yes, I tested the changes work. I have an app under development that is utilizing this code. The change to _getSignature works for gets and posts.

@avital

This comment has been minimized.

Contributor

avital commented Dec 12, 2012

Ok. I would have loved to have a test for this but this file is completely untested so I won't ask that of you. But test coverage around this area should probably be improved.

I'll merge this in, but I need to first ask you to sign our CLA: http://contribute.meteor.com/

@timhaines

This comment has been minimized.

Contributor

timhaines commented Dec 12, 2012

@avital I signed the CLA.

I agree - tests would be awesome. When I'm more familiar with everything and have more experience with stack I might circle back and add them, but for now it would be great if these changes could be merged in.

@avital

This comment has been minimized.

Contributor

avital commented Dec 12, 2012

Agreed.

avital added a commit that referenced this pull request Dec 12, 2012

Merge pull request #539 from timhaines/oauth1binding-to-accept-post-p…
…arams

Update OAuth1Binding to allow post and get to receive params

@avital avital merged commit f247840 into meteor:devel Dec 12, 2012

@avital

This comment has been minimized.

Contributor

avital commented Dec 12, 2012

Worth pointing out that there is a breaking change here -- the return value to OAuth1Binding.call and get. I think this is probably fine, because this is a semi-internal API, and it's probable that you are the first person using this externally (judging by this Pull Request).

I'll make sure, though, that this change is appropriately documented in History.md

@zealoushacker

This comment has been minimized.

Contributor

zealoushacker commented Dec 22, 2012

@timhaines Since you've been looking at the oauth1 adapter, do you think my bug in #530 might have had something to do with not passing/taking correct params?

@timhaines

This comment has been minimized.

Contributor

timhaines commented Dec 26, 2012

@zealoushacker sorry for the delay. I see you got your issues resolved. What was the cause?

@zealoushacker

This comment has been minimized.

Contributor

zealoushacker commented Dec 26, 2012

@timhaines That's quite alright. I haven't gotten the linkedin issue resolved yet. First, I wanted to get #572 finished. But, it seemed like there might be a problem with some params that are being passed around between meteor and linkedin, which causes the bug error that I had documented above. In other words, I don't think it's on linkedin's end. It's me, passing or attempting to parse out incorrect parameters, in the request or the response, respectively.

@timhaines

This comment has been minimized.

Contributor

timhaines commented Dec 26, 2012

@zealoushacker It's quite possibly this: #570 (comment) I'll be submitting a pull request for it shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment