Feature/7675 social login service call - #7872
Conversation
…nto feature/7675-social-login-service-call
…nto feature/7675-social-login-service-call
…ordpress-mobile/WordPress-iOS into feature/7675-social-login-service-call
| let email = user.profile.email else { | ||
| // The Google SignIn for may have been canceled. | ||
| //TODO: Add analytis | ||
| return; |
There was a problem hiding this comment.
Trailing Semicolon Violation: Lines should not have trailing semicolons. (trailing_semicolon)
…nto feature/7675-social-login-service-call
nheagy
left a comment
nheagy
left a comment
There was a problem hiding this comment.
Seems to be running well 👍
I've got some minor code changes requested, but nothing huge. Nice work 😀
| - (void)loginToWordPressDotComWithGoogleIDToken:(NSString *)googleIDToken | ||
| { | ||
| if ([self.delegate respondsToSelector:@selector(displayLoginMessage:)]) { | ||
| [self.delegate displayLoginMessage:NSLocalizedString(@"Connecting to WordPress.com", nil)]; |
There was a problem hiding this comment.
I know we already do this in -signInToWordpressDotCom but I don't really love having UI-surfaced strings in the networking code. If instead we told the delegate the state had changed, the VC could decide how to present it: string, icon, or otherwise. Think that's worth changing?
There was a problem hiding this comment.
I'm not too much of a fan either. I still have the dream of nuking the facade completely, but until then I think we should be consistent.
| @param success | ||
| @param failure | ||
| */ | ||
| - (void)connectToSocialService:(NSString *)service |
There was a problem hiding this comment.
Could this be an enum or at least a typedef instead of a string?
There was a problem hiding this comment.
A string enum would be perfect but... objc. :/ Maybe a typedef constant tho.
| "id_token" : token, | ||
| ] as [String : Any] | ||
|
|
||
| // Passes an empty string for the |
There was a problem hiding this comment.
This comment is incomplete. Passes an empty string for the…
THE SUSPENSE 😂
| import Foundation | ||
|
|
||
| @objc | ||
| public class SocialLogin2FANonceInfo: NSObject { |
There was a problem hiding this comment.
I wish this could be a struct. shakes fist at Objective-C
|
|
||
| // Make sure we received expected data. | ||
| guard let responseDictionary = responseObject as? [String: AnyObject], | ||
| let data = responseDictionary["data"] as? [String: AnyObject] else { |
There was a problem hiding this comment.
data isn't very descriptive. responseData might be better?
| @IBOutlet var bottomContentConstraint: NSLayoutConstraint? | ||
| @IBOutlet var verticalCenterConstraint: NSLayoutConstraint? | ||
| var onePasswordButton: UIButton! | ||
| var googleLoginButon: UIButton? |
There was a problem hiding this comment.
Minor thing, but this should have two ts in Buton 😉
|
Heya @nheagy. Thanks for the review. I've made some tweaks. For the typedef suggestion I've refactored the new methods into a Swift category so a string enum can be used. Ready for another peek! |
| /// - token The service's OpenID Connect (JWT) ID token for the user. | ||
| /// - success | ||
| /// - failure | ||
| func connectToSocialService(_ service: SocialServiceName, serviceIDToken token:String, success:@escaping (() -> Void), failure:@escaping ((NSError) -> Void)) { |
There was a problem hiding this comment.
Colon Violation: Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. (colon)
| import Foundation | ||
| import WordPressComKit | ||
|
|
||
| extension AccountService { |
| } | ||
|
|
||
|
|
||
| extension AccountServiceRemoteREST { |
|
Thanks @nheagy !! |
3 participants
Refs #7675
This PR adds the service and networking layer to support social login. It also wires up the google sign in button to test the simplest login scenario.
This PR does not address cases where:
These cases will be addressed in subsequent PRs.
To test:
Use the Google sign in button to login to a WPcom account that has already been connected to a Google profile. Confirm you can login successfully in this scenario.
Confirm that the google button is correctly positioned on the iPad.
Needs review: @nheagy