Skip to content
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

TIMOB-5994 - Facebook #722

Merged
merged 30 commits into from Nov 21, 2011
Merged

TIMOB-5994 - Facebook #722

merged 30 commits into from Nov 21, 2011

Conversation

nebrius
Copy link
Contributor

@nebrius nebrius commented Nov 17, 2011

No description provided.


// Sanity check
if (_appid == null) {
console.debug('App ID not set. Facebook authorization cancelled.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this should be logged as an error and probably an exception thrown since it's a programmer error, not a runtime error.

} else if (response.error) {
callback({'success':false,'error':response.error,'path':path,'source':Titanium.Facebook});
} else {
callback({'success':true,'result':response,'path':path,'source':Titanium.Facebook});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these callback() calls fail because "path" is undefined. Is path supposed to be action?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy paste error

api.requestWithGraphPath = function(){
console.debug('Method "Titanium.Facebook.requestWithGraphPath" is not implemented yet.');
api.requestWithGraphPath = function(path,params,httpMethod,callback){
if (!_loggedIn) return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not logged in, there's no way to know if the request failed. You should fire callback with success false.

if (!_loggedIn) {
callback({
'success' : false,
'error' : 'must be logged in to call Titanium.Facebook.dialog',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message is overly verbose. "Not logged in" would be sufficient.

@nebrius
Copy link
Contributor Author

nebrius commented Nov 20, 2011

Ok, now we have the old behavior (auto-login), but now it's done in a safe way that should avoid the issue of appID being set late.

@nebrius
Copy link
Contributor Author

nebrius commented Nov 20, 2011

Wow, wonder why I did that? Oh well, it's fixed.

@nebrius
Copy link
Contributor Author

nebrius commented Nov 20, 2011

Oh snap. Missed that in my testing. It's just a theoretical case where app_id is set before the div loads, so it's really hard to test in practice.

@cb1kenobi
Copy link
Contributor

Reviewed and tested. Request accepted!

@donthorp
Copy link
Contributor

Reviewed. Request accepted.

donthorp added a commit that referenced this pull request Nov 21, 2011
@donthorp donthorp merged commit febc5a7 into tidev:master Nov 21, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants