Fix ESLint console warnings #1337
Conversation
# Conflicts: # app/models/user.server.model.js # package.json
|
Those Travis failures seem to be unrelated to this PR, although they should be checked, specially #3 ( |
|
Reran build, I think that was just a temporary thing. |
|
Should not affect code coverage... |
| @@ -116,7 +116,7 @@ exports.oauthCallback = function (strategy) { | |||
| return res.redirect('/authentication/signin'); | |||
| } | |||
|
|
|||
| return res.redirect(redirectURL || sessionRedirectURL || '/'); | |||
There was a problem hiding this comment.
seems can not simply update:
return res.redirect(redirectURL || sessionRedirectURL || '/');
to:
return res.redirect(sessionRedirectURL || '/');
because before oauthCallback function called, there will be another function saveOAuthUserProfile execute before it and have the following code snippet to pass value to redirectURL:
user.save(function (err) { return done(err, user, '/settings/accounts'); });
so, if pass value to redirectURL, the value will be right, if do not pass anything, the value will become an object.
for your reference, thanks.
There was a problem hiding this comment.
@fauria can you please refer to this comment by @OneOfTheWorld ?
There was a problem hiding this comment.
I get @OneOfTheWorld point, the thing is, oauthCallback is actually called before saveOAuthUserProfile so even if we do:
user.save(function (err) {
req.session.redirect_to = '/settings/accounts';
return done(err, user);
});
the redirection will still not be applied.
There was a problem hiding this comment.
@fauria I already merged your commit with @OneOfTheWorld change.
If it's still not solving the problem, do you want to issue a PR that does?
There was a problem hiding this comment.
A PR would be fine, I think it would be a good idea to review the authentication process and rework the redirections.
There was a problem hiding this comment.
We're always open for improvements. Go ahead and open a PR and let's see what's on the table.
We already had a big refactoring to the authentication and authorization code one time.
|
@fauria so where does this PR stand? |
|
@lirantal Yeah, I know. I thought they were going to be merged faster, so I didn't think on multiple branches... |
|
So right now I'm not sure on the state of this PR - is everything really taken care of? or do you want to create 3 new PRs for us to review and assess them each on it's own? |
|
They are quite trivial, doesn't look like they need there own PR... |
|
@fauria I ended up fixing the issue mentioned by @OneOfTheWorld in another PR: #1388 @fauria thanks for all your help! Hope to see you again with another PR :) |
fix (config): ESLint console warnings
Adds
'no-console': 1option to.eslintrc.Fixes #1336
Update:
fix redirect to
'[object Object]'after login with social accounts.Fixes #1284
Update 2:
fix
TypeError: Cannot read property 'additionalProvidersData' of undefined at SocialAccountsController.hasConnectedAdditionalSocialAccounts. Usevminstead of$scope.Fixes #1347