Add Apple specific parameters to social service connect #182
Conversation
frosty
left a comment
frosty
left a comment
There was a problem hiding this comment.
Just a query about the parameters for the social method!
| public func connectToSocialService(_ service: SocialServiceName, serviceIDToken token: String, oAuthClientID: String, oAuthClientSecret: String, success:@escaping (() -> Void), failure:@escaping ((NSError) -> Void)) { | ||
| public func connectToSocialService(_ service: SocialServiceName, | ||
| serviceIDToken token: String, | ||
| appleEmail: String? = nil, |
There was a problem hiding this comment.
I feel like these apple parameters don't really belong in here 🤔 The method can be used for any service, so having apple specific parameters there if you're using Google doesn't feel quite right to me (even though you can ignore them because of the defaults).
How about if we just accept a parameters dictionary, which we can append to our own params? We could add a separate method to generate it, like appleSignInParameters(email:fullName:), so the client doesn't need to know the correct keys for those items.
There was a problem hiding this comment.
Or we could just pass in userEmail and userFullName, and the services can do with them what they want (or ignore them)
…eter to connectToSocialService.
|
Hey @frosty . Changes made and ready for another review please. |
frosty
left a comment
frosty
left a comment
There was a problem hiding this comment.
Hey, one more suggestion for a tweak to make the method less Apple centric.
| serviceIDToken token: String, | ||
| appleEmail: String? = nil, | ||
| appleFullName: String? = nil, | ||
| appleConnectParameters: [String:AnyObject]? = nil, |
There was a problem hiding this comment.
So could we just name this connectParameters? Perhaps the doc comment could say "currently only used for the Apple service".
| if service == .apple { | ||
| params["user_email"] = appleEmail as AnyObject | ||
| params["user_name"] = appleFullName as AnyObject | ||
| if service == .apple, let appleConnectParameters = appleConnectParameters { |
There was a problem hiding this comment.
We could just always append the parameters regardless of the service, then we don't need special conditional code for Apple?
There was a problem hiding this comment.
I figured it's better not to append them just in case, but ok.
I'm not trying to make it Apple centric. I'm trying to point out that these params are for Apple only. But ok, I'll generalize everything and just have a comment. |
|
OK @frosty . One more time (🤞 ) please kind sir. |
frosty
left a comment
frosty
left a comment
There was a problem hiding this comment.
Thank you for putting up with my suggestions! Looks good! 👍
Description
Fixes #n/a
This does two things:
appleservice name. This was needed for the associated WPAuth change.social-login/connectendpoint. This wasn't needed yet, but hey, while we're here...Testing Details
This can be tested with WPiOS PR: wordpress-mobile/WordPress-iOS#12360
Ref WPAuth PR: wordpress-mobile/WordPressAuthenticator-iOS#117