Skip to content
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

Add fields to the query #115

Merged
merged 3 commits into from Jul 29, 2015
Merged

Add fields to the query #115

merged 3 commits into from Jul 29, 2015

Conversation

@edimoldovan
Copy link
Contributor

edimoldovan commented Jul 18, 2015

Added the fields which are then by default in the profile normalisation.

Added the fields which are then by default in the profile normalisation.
@edimoldovan

This comment has been minimized.

Copy link
Contributor Author

edimoldovan commented Jul 18, 2015

Hi,

Can I get a little help with how to update the tests for this PR to pass the Travis build?

Thanks,
Eduárd

@@ -20,7 +20,8 @@ exports = module.exports = function (options) {
profile: function (credentials, params, get, callback) {

var query = {
appsecret_proof: Crypto.createHmac('sha256', this.clientSecret).update(credentials.token).digest('hex')
appsecret_proof: Crypto.createHmac('sha256', this.clientSecret).update(credentials.token).digest('hex'),
fields: 'id,name,email,first_name,last_name,middle_name'

This comment has been minimized.

Copy link
@edimoldovan

edimoldovan Jul 18, 2015

Author Contributor

The graph query seams to return the fields only if they are explicitly provided.

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Jul 21, 2015

I am only getting the behavior you describe when using the v2.4 API but we're using v2.3 right now.

Either way, your PR looks good and works with 2.3.

For the tests, you need to change two lines in test/oauth.js:

expect(uri).to.equal('https://graph.facebook.com/v2.3/me?appsecret_proof=d32b1d35fd115c4a496e06fd8df67eed8057688b17140a2cef365cb235817102');

Change it to include the fields parameter
&fields=id,name,email,... and it should work.

I have to say that Lab/Expect are doing a really bad job at telling us what's wrong.

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Jul 27, 2015

@edimoldovan Is there anything more you need?

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Jul 27, 2015

profileParams - an object of key-value pairs that specify additional URL query parameters to send with the profile request to the provider. The built-in facebook provider, for example, could have fields specified to determine the fields returned from the user's graph, which would then be available to you in the auth.credentials.profile.raw object.

If you still have problems before this is fixed, you can use profileParams as documented to get around it. Though I think those fields should definitely be included, especially as we upgrade to v2.4 API.

@edimoldovan

This comment has been minimized.

Copy link
Contributor Author

edimoldovan commented Jul 27, 2015

Hi @ldesplat
Sorry, I missed your original reply, just noticed it now.

I tried the quick fix, doesn’t seam to work.

How do I run tests locally?

Thanks,
Eduárd

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Jul 27, 2015

run npm install and then npm test

If you have issues and you don't know where it comes from you can also do npm run test-cov-html

@edimoldovan

This comment has been minimized.

Copy link
Contributor Author

edimoldovan commented Jul 28, 2015

@ldesplat If code/tests are fine with you, they’re ready to go from my side.
And thanks for helping!

@ldesplat ldesplat added this to the 5.0.1 milestone Jul 28, 2015
ldesplat added a commit that referenced this pull request Jul 29, 2015
Explicitly specify fields for facebook provider.
@ldesplat ldesplat merged commit 6d51f27 into hapijs:master Jul 29, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Jul 29, 2015

@edimoldovan Thank you! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.