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

Remove legacy checks for read/write permissions. #34

Conversation

djanowski
Copy link

Tested with latest version of the plugin, everything works smoothly.

@noahcooper noahcooper changed the base branch from master to develop February 9, 2021 02:56
@noahcooper
Copy link

noahcooper commented Feb 9, 2021

Thanks for this PR! Do you happen to have some reference to a page on the Facebook Developer site, perhaps an entry in the changelog, that indicates that apps can now request both read and publish permissions at the same time? https://developers.facebook.com/docs/facebook-login/ios/permissions/ seems to suggest that these permissions must still be separate.

@djanowski
Copy link
Author

@noahcooper Thanks for looking at this so quickly and for your pointer! You're right, it seems this is still the case (however the link to learn more on that section is broken.) It's not clear to me whether this is a policy by Apple or Facebook, or whether Facebook would error out if requesting read and publish permissions together. If this is a policy by Facebook, and Facebook errors out if requesting both read and publish permissions, then I don't think the plugin needs to duplicate this logic? The developer would see the error presented by Facebook immediately. It's late over here but I can do some tests tomorrow.

In any case, after I upgraded to this version of the plugin, Facebook started to complain about me requesting manage_pages. This is correct -- I also upgraded to the new, granular permissions, and everything works.

The new permissions I need are all prefixed with pages_. The plugin isn't catching those as "publish permissions", and everything works correctly, no complaint from Facebook.

@noahcooper
Copy link

@djanowski you make a valid point -- I'll test further just to confirm that no uncaught exception is thrown by the Facebook SDK if both read and publish permissions are requested at once, if not, I don't see any reason to duplicate this logic in the plugin. (To your point, the check to determine if a given permission is a publish permission is not even up-to-date because the syntax of Facebook permission names has long since changed, and in fact, some of the permissions referenced as publish permissions were deprecated years ago.

Would you mind updating your PR to make the same change on Android?

@noahcooper noahcooper merged commit 2ad3b18 into cordova-plugin-facebook-connect:develop Feb 19, 2021
@noahcooper
Copy link

@djanowski I've merged your changes for iOS and updated Android as well. In my testing, everything works as expected.

@djanowski
Copy link
Author

Thank you @noahcooper! 🙏 ✨

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

2 participants