fix(users): fix redirect when signup or add provider #1573
Conversation
| @@ -85,6 +85,9 @@ exports.signout = function (req, res) { | |||
| */ | |||
| exports.oauthCall = function (strategy, scope) { | |||
| return function (req, res, next) { | |||
| if (req.query && req.query.redirect_to) | |||
| req.session.redirect_to = req.query.redirect_to | |||
| // OAuth provider request | ||
| function callOauthProvider(url) { | ||
| if ($state.previous && $state.previous.href) { | ||
| url += '?redirect_to=' + encodeURIComponent($state.$current.url.prefix); |
There was a problem hiding this comment.
I see that #_=_ is added to the end of redirect_to querystring parameter.
There was a problem hiding this comment.
$state.$current.url.prefix contains /settings/accounts, is that your question?
There was a problem hiding this comment.
I'm just wondering why you're using $state.$current.url.prefix, rather than $state.$current.url. Is there a specific reason? It should work without using .prefix.
Also, you don't need to have the conditional statement in the above line. It doesn't match up with the logic below it either. It checks for the existence of $state.previous but that's not what is used below. You can just set the url since we will always want them to redirect here anyways.
There was a problem hiding this comment.
perhaps you are confusing $state.current.url with $state.$current.url, the first one return the string:
"/accounts"and the last one return the object:
{
$paramNames:Array[0]
params:extend
prefix:"/settings/accounts"
regexp:/^\/settings\/accounts$/
segments:Array[1]
0:"/settings/accounts"
length:1
__proto__:Array[0]
source:"/settings/accounts"
sourcePath:"/settings/accounts"
sourceSearch:""
}so I choose prefix just because it contains the url that I want, but I also could use source and sourcePath without problems (I think)
There was a problem hiding this comment.
Using prefix does work, so I'm not questioning that. You're right though, that I was getting the two confused, and even so it wouldn't work to use $state.current.url for the exact reason you stated; it would return /accounts.
My confusion was reinforced by the fact that using $state.$current.url would work in this case due to the URI encoding converting the url property to a string, which returns /settings/accounts.
All that aside, you are correct & using prefix is ok. It just wasn't clear as to why it was used. The only thing I would request is to remove the conditional statement since it's incorrect & it's not needed.
There was a problem hiding this comment.
Just for clarification, I checked, and the .toString() prototype method on $state.$current.url is
UrlMatcher.prototype.toString = function () {
return this.source;
};
|
@itelo This is mostly good. Just left a review requesting a change, and asking for clarification on one thing. Also, please be in the habit of adding commit messages. It makes it easier to review in the commit history. |
|
LGTM. Thank you @itelo! We'll be merging this soon, to get it in for the 0.5.0 release. |
No description provided.