Merge improvements from anonymous auth PR (#309) #808
Conversation
Signed-off-by: Alex Saveau <[email protected]>
There was a problem hiding this comment.
@SUPERCILEX thanks for doing this! Excited about the Twitter improvements. Just a few questions.
Also we should target 2.1.1-dev not master.
| if ((providerId.equalsIgnoreCase(GoogleAuthProvider.PROVIDER_ID) | ||
| || providerId.equalsIgnoreCase(FacebookAuthProvider.PROVIDER_ID) | ||
| || providerId.equalsIgnoreCase(TwitterAuthProvider.PROVIDER_ID) | ||
| || providerId.equalsIgnoreCase(GithubAuthProvider.PROVIDER_ID)) |
There was a problem hiding this comment.
What's github doing here?
There was a problem hiding this comment.
Lol, must have been a merge mistake from a long time ago.
|
|
||
| try { | ||
| email = object.getString("email"); | ||
| } catch (JSONException ignored) { } |
There was a problem hiding this comment.
Shouldn't we at least log a warning if we can't get email?
| TwitterCore.getInstance() | ||
| .getApiClient() | ||
| .getAccountService() | ||
| .verifyCredentials(false, false, true) |
There was a problem hiding this comment.
Can you add a comment documenting the false, false, true magic sequence?
There was a problem hiding this comment.
TBH, I just copied that from StackOverflow 😆. I did a bit of debugging and figured some of it out (in order): includeEntities will populate the User#entities field but we don't need it, skipStatus I have no idea what that does, includeEmail includes the email (duh! 😄).
Do you want me to document the parameter names? Intellij shows them, but it could be useful for GitHub:
There was a problem hiding this comment.
That's alright, thanks for clarifying!
| } | ||
|
|
||
| private IdpResponse( | ||
| String providerId, | ||
| String email, | ||
| User user, | ||
| String phoneNumber, |
There was a problem hiding this comment.
any reason why email was merged into User but not phoneNumber?
Signed-off-by: Alex Saveau <[email protected]>
|
@SUPERCILEX last thing left is to target the |
|
@samtstern Oops, I've updated the base now. |
@samtstern While working on automatically merging user profiles for #309, I made several improvements worth merging back into master:
IdpResponseuser data is unified and uses ourUserobject under the hood now