Yandex & Mail.ru services #301
Conversation
mche
force-pushed
the
development
branch
3 times, most recently
from
ea5da36 to
7feb3d7
Compare
mche
force-pushed
the
development
branch
2 times, most recently
from
605d981 to
4d5ee0e
Compare
|
Just to confirm, since I am not familiar with this service. You've tested it and it works great with your PR? Once you confirm it is working, I'll merge it. Referencing #299 |
|
Sorry for PR #299 there is incorrect response_type=code. Now works response_type=token. Im recheck api and had tests works fine for this PR. |
| if($cordovaOauthUtility.isInAppBrowserInstalled()) { | ||
| if(options === undefined) { options = {}; } | ||
| if(appScope === undefined) { appScope = []; } | ||
| if(! options.hasOwnProperty("redirect_uri")) { |
There was a problem hiding this comment.
[MINOR] remove the space after !
| //~ deferred.reject({error: "You must set option redirect_uri that is an any url within YOUR DOMAIN! The redirect_uri:'http://localhost/callback' doesnt works."}); | ||
| //~ return deferred.promise; | ||
| } | ||
| if(! options.hasOwnProperty("display")) { |
| if(! options.hasOwnProperty("display")) { | ||
| options.display = "mobile"; | ||
| } | ||
| if(! options.hasOwnProperty("browserWindow")) { |
| } | ||
| var flowUrl = "https://connect.mail.ru/oauth/authorize?response_type=token&client_id=" + clientId + "&redirect_uri=" + options.redirect_uri + '&display=' + options.display + '&scope=' + appScope.join(" "); | ||
|
|
||
| var browserRef = window.cordova.InAppBrowser.open(flowUrl, '_blank', options.browserWindow);//,clearsessioncache=yes,clearcache=yes); |
There was a problem hiding this comment.
You can remove the comment since you are not using the code inside
| browserRef.addEventListener('loadstart', function(event) { | ||
| if((event.url).indexOf(options.redirect_uri) === 0) { | ||
| browserRef.removeEventListener("exit",function(event){}); | ||
| browserRef.close(); |
There was a problem hiding this comment.
I think the indentation of this block of code is wrong.
| var callbackResponse = (event.url).split("#")[1]; | ||
| var responseParameters = (callbackResponse).split("&"); | ||
| var parameterMap = {}; | ||
| for(var i = 0; i < responseParameters.length; i++) { |
There was a problem hiding this comment.
You can replace the for loop with a forEach and use some vars to let the code easier to understand. (Please test first cause I don't remember right now if the Array forEach is ES6 or ES5)
responseParameters.forEach(function(param) {
var splittedParam = param.split('=');
var mapKey = splittedParam[0];
var mapValue = splittedParam[1];
parameterMap[mapKey] = mapValue;
});| if(parameterMap.access_token !== undefined && parameterMap.access_token !== null) { | ||
| deferred.resolve(parameterMap); | ||
| } else { | ||
| //~ deferred.reject("Problem authenticating"); |
| } | ||
| var flowUrl = "https://oauth.yandex.ru/authorize?response_type=token&client_id=" + clientId + "&redirect_uri=" + options.redirect_uri + "&force_confirm=" + options.force_confirm; | ||
|
|
||
| var browserRef = window.cordova.InAppBrowser.open(flowUrl, '_blank', options.browserWindow);//,clearsessioncache=yes,clearcache=yes |
| var callbackResponse = (event.url).split("#")[1]; | ||
| var responseParameters = (callbackResponse).split("&"); | ||
| var parameterMap = {}; | ||
| for(var i = 0; i < responseParameters.length; i++) { |
| if(parameterMap.access_token !== undefined && parameterMap.access_token !== null) { | ||
| deferred.resolve(parameterMap); | ||
| } else { | ||
| //~ deferred.reject("Problem authenticating"); |
|
Thanks @matheusrocha89 |
|
Ok, I had inspected the recomendations. |
|
We ready to merge? |
|
Does my comments in README.md are clean? Or may be remove them? |
|
The comments for me are fine. I think you can remove the comments on the code and let them there on readme |
|
@matheusrocha89 Im sorry but comments in code does needs like for me. |
Hi!
Push big russian Yandex and Mail.ru services.