Replace GIDGoogleUser with SocialService.User in case .google#764
Conversation
72f663a to
26c8d44
Compare
26c8d44 to
a78ed0d
Compare
| public var user: User { | ||
| switch self { | ||
| case .google(let user): return user | ||
| case .apple(let user): return user | ||
| } | ||
| } | ||
|
|
||
| /// Google's Signup Linked Account | ||
| /// | ||
| case google(user: GIDGoogleUser) | ||
| case google(user: User) |
There was a problem hiding this comment.
apple and google both have a User associated type now.
This kind of makes it backward to have an enum with associated type, we should have a type with an enum property
struct SocialUser {
let fullName: String
let email: String
let source: SocialUser.Source // .apple, .google, ...
}I've come across other bits of information in some places that could go into this type, such as the token.
I'm skeptical of working on "such a big" rewrite, though. We should be making changes to the auth layer here carefully. Also, I'd like to first remove all the Google-SDK cruft first. The downside is that we'll have to go through at least two major versions. I think it's worth it, though.
| socialService: SocialServiceName? = nil, | ||
| socialServiceIDToken: String? = nil, | ||
| googleUser: GIDGoogleUser? = nil, | ||
| googleUser: SocialService.User? = nil, | ||
| appleUser: SocialService.User? = nil) { |
There was a problem hiding this comment.
I wrote it elsewhere already, but it seems to me like these parameters are ripe for consolidation into a single type (SocialUser?).
I'm not keen on introducing such a change right now because I'd like to stay focused on removing the Google SDK only. The tradeoff is that we'll have to cycle through two major version changes, but that seems acceptable. It's just a number after all.
There was a problem hiding this comment.
I wrote it elsewhere
Elsewhere = the file after this, 14 hours ago 🤦♂️ 🙃
But hey, at least I'm consistent in the naming suggestion I'm leaving...
| ) | ||
| } | ||
|
|
||
| func updateLoginFields(email: String, username: String, fullName: String, googleToken: String) { |
There was a problem hiding this comment.
This username argument is not used, should we get rid of it? Also, any reason adding another updateLoginFields function here, rather than do the assignments in the one above?
There was a problem hiding this comment.
What I should have done with that username parameter was using it in the method and then passing .email at callsite. I guess I was trying to cover my basis.
But your observation made me realize the version with the GIDGoogleUser input is unused, and this new one is only used by it. So, I removed both of them.
Closes #759.
As described in #759, the idea with this PR is to remove the Google SDK-dependent type from the
SocialServicegooglecaseassociated value and use the pure SwiftSocialService.Userinstead. The new type was introduced in #763.To test
The code changes here are consumed via unit tests alone. The demo app doesn't read the
loginFieldsmetadata whereSocialServicelives.The "real" test for this new code is in the Jetpack/WordPress iOS that uses it wordpress-mobile/WordPress-iOS#20128
CHANGELOG.mdif necessary.