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
11893 oauth auto login #12029
11893 oauth auto login #12029
Conversation
The build seems to have failed because CircleCI has problems accessing npm (see status page). |
Rebase onto master to get ride of the codespell errors. circle ci seem to have problems with npm right now. |
I rebased the code on master. CircleCI seems to be fine again, too. |
$location.path('/redirect'); | ||
window.location.href = url; | ||
}; | ||
if ($scope.oauth.google) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure the redirect should have handled in the frontend. Seems easier to redirect the user in the backend. Any specific reason for doing it in the frontend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are absolutely right, redirecting in the backend is cleaner. I will update the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is ready for review.
Do you want me to change anything else? |
Sorry! No maintainer had time to review this. We are quite a bit behind but we will get to this. It's a good feature, we just want to review the implementation before merging it. Please be patient :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Could you please rebase this onto master? Id be happy to merge it once that's done :)
Great! I rebased onto master. |
I rebased onto master again to include the new changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I will merge this once the scss change have been reverted since its not related to this feature.
Redirect in backend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you for contributing! |
See issue #11893 for the relevant proposal.
Changes: