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

facebook picture field added to get profile picture #347

Merged
merged 2 commits into from Mar 6, 2018

Conversation

@satnam-sandhu
Copy link
Contributor

satnam-sandhu commented Mar 2, 2018

Updated test Case for facebook but when running tests it shows error in other files.
This is my error:

Coverage: 92.13% (124/1575)
lib/oauth.js missing coverage on line(s): 312, 313, 316, 346
lib/providers/arcgisonline.js missing coverage on line(s): 21, 24, 26, 28
lib/providers/auth0.js missing coverage on line(s): 29, 30
lib/providers/azuread.js missing coverage on line(s): 24, 26, 29
lib/providers/bitbucket.js missing coverage on line(s): 19, 21
lib/providers/digitalocean.js missing coverage on line(s): 23, 24, 26
lib/providers/discord.js missing coverage on line(s): 20, 22
lib/providers/dropbox-v2.js missing coverage on line(s): 21, 22
lib/providers/dropbox.js missing coverage on line(s): 20, 21
lib/providers/fitbit.js missing coverage on line(s): 21, 23
lib/providers/foursquare.js missing coverage on line(s): 20, 23, 25, 26, 28
lib/providers/github.js missing coverage on line(s): 30, 32
lib/providers/gitlab.js missing coverage on line(s): 24, 25
lib/providers/google.js missing coverage on line(s): 21, 23
lib/providers/googleplus.js missing coverage on line(s): 21, 23
lib/providers/instagram.js missing coverage on line(s): 24, 29, 31, 32, 35, 36
lib/providers/linkedin.js missing coverage on line(s): 30
lib/providers/live.js missing coverage on line(s): 21, 23, 31
lib/providers/medium.js missing coverage on line(s): 23, 25
lib/providers/meetup.js missing coverage on line(s): 24, 25
lib/providers/mixer.js missing coverage on line(s): 22, 24, 26, 27
lib/providers/office365.js missing coverage on line(s): 21, 23
lib/providers/okta.js missing coverage on line(s): 29, 31
lib/providers/phabricator.js missing coverage on line(s): 30, 32, 34, 36
lib/providers/pingfed.js missing coverage on line(s): 26, 28
lib/providers/pinterest.js missing coverage on line(s): 23, 25, 27, 29
lib/providers/reddit.js missing coverage on line(s): 25, 28, 30, 31
lib/providers/salesforce.js missing coverage on line(s): 40, 41, 42, 44, 45
lib/providers/slack.js missing coverage on line(s): 22, 25, 27, 28, 31, 35, 36, 37, 38
lib/providers/spotify.js missing coverage on line(s): 28, 29
lib/providers/stripe.js missing coverage on line(s): 24, 26, 27
lib/providers/trakt.js missing coverage on line(s): 31, 33, 35, 36
lib/providers/tumblr.js missing coverage on line(s): 21, 22
lib/providers/twitch.js missing coverage on line(s): 22, 24, 26, 27
lib/providers/twitter.js missing coverage on line(s): 35, 36, 43
lib/providers/vk.js missing coverage on line(s): 20, 23, 25, 26, 28
lib/providers/wordpress.js missing coverage on line(s): 25, 29, 31, 33, 34, 35, 36, 37
lib/providers/yahoo.js missing coverage on line(s): 13, 21, 23, 33
Code coverage below threshold: 92.13 < 100
@satnam-sandhu

This comment has been minimized.

Copy link
Contributor Author

satnam-sandhu commented Mar 2, 2018

@AdriVanHoudt I closed my last PR and sent a fresh one but these tests!! Sorry for disturbing but i really want to learn more about testing and CI.

@AdriVanHoudt AdriVanHoudt reopened this Mar 2, 2018
@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Mar 2, 2018

Hi @satnam-sandhu The first commit's build failed due to Travis I think, I can't get the logs so seems like an issue on their end.

Am I correct to assume that the final implementation asks for the picture but you will get it from the raw profile?

@satnam-sandhu

This comment has been minimized.

Copy link
Contributor Author

satnam-sandhu commented Mar 2, 2018

@AdriVanHoudt Yes that seemed like the only way possible for me at the moment. But i think that there should be picture field in the main profile object. But not possible for me to implement it right now. Will learn how to deal with Travis tests and would try to update it soon.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Mar 6, 2018

It looks like all your tests run fine (there was a small Travis hiccup).
I will merge this already since it is good to go and then you can take a jab at adding it to the profile object.
It should definitely be possible since (I think) Discord already does something similar.

@AdriVanHoudt AdriVanHoudt self-assigned this Mar 6, 2018
@AdriVanHoudt AdriVanHoudt added the feature label Mar 6, 2018
@AdriVanHoudt AdriVanHoudt merged commit 535c85f into hapijs:master Mar 6, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hueniverse hueniverse added this to the 9.1.1 milestone Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.