sorcery icon indicating copy to clipboard operation
sorcery copied to clipboard

adding 'include_email' parameter

Open EvanBrightside opened this issue 8 years ago • 9 comments
trafficstars

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 avatar Sep 11 '17 13:09 EvanBrightside

@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 avatar Sep 18 '17 16:09 ebihara99999

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

EvanBrightside avatar Sep 18 '17 16:09 EvanBrightside

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 avatar Sep 19 '17 14:09 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.

ebihara99999 avatar Sep 19 '17 14:09 ebihara99999

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

Ch4s3 avatar Nov 28 '17 03:11 Ch4s3

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

EvanBrightside avatar Jan 05 '18 09:01 EvanBrightside

I’ll take a look tonight

Ch4s3 avatar Jan 07 '18 16:01 Ch4s3

@Ch4s3 mate, what about this PR? )

EvanBrightside avatar Feb 13 '18 10:02 EvanBrightside

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.

joshbuker avatar Oct 29 '20 19:10 joshbuker