Feature/google authentication #6
Conversation
…y/mentors into feature/facebook_signup
|
Couldn't reopen pull request for the previous branch, so had to create a new one. |
controller/googleOauthHandler.js
Outdated
| if (await dbQuery.idExists(userInfo)) { | ||
| return { | ||
| message: token, | ||
| statusCode: 200 |
There was a problem hiding this comment.
don't hardcode status codes
controller/googleOauthHandler.js
Outdated
| } | ||
| } catch(error) { | ||
| console.log(error); | ||
| return { message:"Error connecting database", statusCode: 500} |
There was a problem hiding this comment.
don't hardcode messages
controller/googleOauthHandler.js
Outdated
| const token = await tokenGenerator.token(userInfo.email); | ||
| const refresh_token = await tokenGenerator.refresh_token(userInfo.email); | ||
|
|
||
| try { |
There was a problem hiding this comment.
can't we use single try catch?
controller/googleOauthHandler.js
Outdated
| idExists: async (userInfo) => { | ||
| try { | ||
| const googleId = await User.findOne({ google_id: userInfo.id }); | ||
| if (googleId === null) { |
There was a problem hiding this comment.
do we need to check googleId === null?
dosen't it return true or false?
There was a problem hiding this comment.
@vipul- coordinate with the @AwaleRohin for success, failure response and status code and messages.
controller/googleOauthHandler.js
Outdated
| //If the google id already exists in the db, store nothing to the db | ||
| if (await dbQuery.idExists(userInfo)) { | ||
| return { | ||
| message: token, |
There was a problem hiding this comment.
@vipul- follow the feedback dropped to the @AwaleRohin for response and constants.
controller/googleOauthHandler.js
Outdated
|
|
||
| try { | ||
| //If the google id already exists in the db, store nothing to the db | ||
| if (await dbQuery.idExists(userInfo)) { |
There was a problem hiding this comment.
@vipul- you have named the method idExists but you have passed the whole information of userInfo. Pass only user identifier.
controller/accessTokenGenerator.js
Outdated
| @@ -0,0 +1,22 @@ | |||
| const jwt = require('jsonwebtoken'); | |||
There was a problem hiding this comment.
@vipul- AuthTokenGenerator will be the best name for this file.
controller/googleOauthHandler.js
Outdated
| if (await dbQuery.idExists(userInfo)) { | ||
| return { | ||
| message: token, | ||
| statusCode: 200 |
There was a problem hiding this comment.
@vipul- did you sent the user data on a response?
routes/socialAuth.js
Outdated
| //Config | ||
| const app = express(); | ||
| app.use(cors()); | ||
| app.use(bodyParser.json({ type: 'application/*+json' })); |
There was a problem hiding this comment.
@vipul- why are you duplicating the things of index.js class?
routes/socialAuth.js
Outdated
| let response = await googleOauthHandler.getMessage(googleAccessToken); | ||
| res.status(response.statusCode).send(response.message); | ||
| } catch(error) { | ||
| res.status(500).send("Internal Server Error"); |
There was a problem hiding this comment.
@vipul- collaborate with @AwaleRohin regarding success and failure response.
controller/googleOauthHandler.js
Outdated
| idExists: async (userInfo) => { | ||
| try { | ||
| const googleId = await User.findOne({ google_id: userInfo.id }); | ||
| if (googleId === null) { |
There was a problem hiding this comment.
@vipul- directly return return googleId === null its a expression it returns true or false by itself. You don't need to return true or false.
controller/googleOauthHandler.js
Outdated
| refresh_token | ||
| } | ||
| try { | ||
| User.create(user); |
There was a problem hiding this comment.
@vipul- don't you need to use await syntax for this?
controller/googleOauthHandler.js
Outdated
| message: token, | ||
| statusCode: 200 | ||
| }; | ||
| } else { |
There was a problem hiding this comment.
@vipul- you can use else if
|
@khadkarajesh Made changes suggested in the review. |
index.js
Outdated
| @@ -24,4 +27,5 @@ app.get('/', (req, res) => { | |||
| app.use(bodyParser.json()) | |||
| app.use(cors()) | |||
| app.use(morgan('combined')) | |||
| app.use('/mentors/', signup) | |||
| app.use('v1/mentors/', signup) | |||
There was a problem hiding this comment.
@vipul- is it correct resource name for mentor signup?
No description provided.