Instagram authentication #8

Merged
merged 9 commits into from Jan 3, 2017

Projects

None yet

4 participants

@iamraphson
Contributor

Add the driver to handle Instagram OAuth2, also with some tests which works like other tests unit
Also add an example that work well.

I'd also would like to take this pull request to thanks you for Adonis, great framework! One Love 👍👌🏽✌️

@thetutlage
Member

Tests are failing due to linting issues. Mind fixes them?

@iamraphson
Contributor

I have been able to fix the linting issues, i had earlier. Kindly review and merge..

@Aeryax
Aeryax commented Dec 27, 2016

Hi @iamraphson,

I think you should remove your client_id and client_secret from 'examples/setup/config.js' pushed with your latest commit.

@iamraphson
Contributor
iamraphson commented Dec 27, 2016 edited

Hi @Aeryax

Thanks for that.i have removed it..

@RomainLanz

Also don't forget to reset them!

@iamraphson
Contributor

Hi @RomainLanz

Reset them in terms of what please??

@iamraphson iamraphson closed this Dec 27, 2016
@iamraphson iamraphson reopened this Dec 27, 2016
@RomainLanz

Reset your secret, cause everyone can see them and use them.

@iamraphson
Contributor

@RomainLanz oh, thanks

@iamraphson
Contributor

@thetutlage

This PR is about fixing a bug in twitter driver and wrote test for twitter driver.

@iamraphson iamraphson changed the title from Instagram authentication to Instagram / Foursquare authentication Jan 1, 2017
@iamraphson
Contributor

Hi @thetutlage

Add the driver to handle foursquare OAuth2 as part of my new year gift to adonisJS framework😆😆.
Add an example that works fine.

I'd also would like to take this pull request to thanks you for Adonis, great framework! One Love 👍👌🏽✌️

@thetutlage
Member

@iamraphson It will be great if you can have a different PR for FourSquare, moving everything to a single PR is little dirty.
Simplest way will be to cherry-pick the commit ids to a new branch and create a PR from that

Also I will make sure to review the code today itself and will merge it if all is good 👍

@iamraphson iamraphson changed the title from Instagram / Foursquare authentication to Instagram authentication Jan 2, 2017
@iamraphson
Contributor

Hello @thetutlage

Thanks so much for your reply. I will create different PR for the FourSquare.you can review and merge instagram authentication.. Any issues, let me know...

src/Drivers/Instagram.js
+ * @private
+ */
+ * _getUserProfile (accessToken) {
+ // fields = _.size(fields) ? fields : this._fields
@thetutlage
thetutlage Jan 3, 2017 Member

Just remove the comment

src/Drivers/Instagram.js
+ * @return {String}
+ */
+ parseRedirectError (queryParams) {
+ return queryParams.error_message || 'Oauth failed during redirect'
@thetutlage
thetutlage Jan 3, 2017 Member

Instagram returns error_description instead of error_message. Docs here https://www.instagram.com/developer/authentication/

Below should be good

return queryParams.error_description || queryParams.error || 'Oauth failed during redirect'
src/Drivers/Instagram.js
+ .setFields(
+ userProfile.data.id,
+ userProfile.data.full_name,
+ userProfile.data.email || null,
@thetutlage
thetutlage Jan 3, 2017 Member

Since Instagram never returns email, this can be just null explicitly.

src/Drivers/Instagram.js
+ accessTokenResponse.accessToken,
+ accessTokenResponse.refreshToken,
+ null,
+ Number(_.get(accessTokenResponse, 'result.expires'))
@thetutlage
thetutlage Jan 3, 2017 Member

Instagram tokens never expire, to this can be null explicitly

@thetutlage
Member
thetutlage commented Jan 3, 2017 edited

I have left some comments for the changes, apart from that all looks fine to me. Also there are some formatting issue, that I will take care of.

Thanks for taking out time 😄

@thetutlage thetutlage merged commit 3d5ca8f into adonisjs:develop Jan 3, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@iamraphson
Contributor

oh, thanks @thetutlage ..you can review and revert the foursquare authentication...

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