adding 'include_email' parameter

[EvanBrightside]

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.

[ebihara99999]

@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?

[EvanBrightside]

@ebihara99999 yep, it will returns nil in this case.

[ebihara99999]

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_email forces nil email in all applications
• So it would be better to make include_email optional

What do you think @Ch4s3?

[ebihara99999]

@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.

[Ch4s3]

@EvanBrightside let us know if you're still working on this

[EvanBrightside]

Gents @ebihara99999 @Ch4s3 I've added some specs!

[Ch4s3]

I’ll take a look tonight

[EvanBrightside]

@Ch4s3 mate, what about this PR? )

[joshbuker]

👍

[joshbuker]

Including the email is good, but iirc it requires additional permissions on the app config side. I'm worried that this could potentially break existing applications, and this should probably be a optional config in-of-itself. Unfortunately though, I don't have the time currently to investigate myself.

@ebihara99999 or @EvanBrightside, can you look into how this interacts with existing apps that don't have the email permission checked? If we can confirm that it doesn't break existing apps, and/or update this to be configurable from initializer.rb, then I'd feel more comfortable merging. Thanks!

[ebihara99999]

Thank you for the review. As I said I think the parameter should be optional too because it returns nil, email is nil, if the app doesn’t have granted by Twitter. It breaks not-null constraint, which is imposed almost all cases.

[ebihara99999]

@EvanBrightside

Would you change codes into configurable in initializer?

See this PR
https://github.com/Sorcery/sorcery/pull/109/files

And the initializer is lib/generators/sorcery/templates/initializer.rb

[ebihara99999]

If it’s somehow difficult or you don’t understand very well, mention to me. Just typing with a mobile, the explanation may be too short.

[joshbuker]

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.