sorcery
sorcery copied to clipboard
adding 'include_email' parameter
With this parameter user can get email address from Twitter REST API, it will be returned in the user objects as a string. If the user does not have an email address on their account, or if the email address is not verified, null will be returned.
@EvanBrightside
Thanks for the PR!
Looking over the document, requesting an email needs to be granted by Twitter.
Wondering if the PR works on the application that is not granted (or even not apply the email access to Twitter). Would you tell me what returns if the not granted application requests an email? It returns nil?
@ebihara99999 yep, it will returns nil in this case.
This PR would be greater if include_email is optional by configration or documentation the following reasons:
- In most applications, email is required
- The request with the parameter
include_emailforces nil email in all applications - So it would be better to make
include_emailoptional
What do you think @Ch4s3?
@EvanBrightside And adding tests are preferable around the shared_example.
The test will be like this
expect do
User.create_from_provider('facebook', '123', username: 'Noam Ben Ari', email: nil)
end.to change { User.count }.by(1)
See this PR, which fixes the inconsistency that required email and the lack of email by the twitter api specification.
@EvanBrightside let us know if you're still working on this
Gents @ebihara99999 @Ch4s3 I've added some specs!
I’ll take a look tonight
@Ch4s3 mate, what about this PR? )
This will be implemented in v1, ideally with some tests for how to handle when permission does get denied. Ideally an app should be able to let the user know that it failed due to them not granting email permissions, then let them try again.