ULS: Unify Google auth flows #295
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also, show HUD for login flow as well as signup.
3 tasks
mindgraffiti
approved these changes
Jun 3, 2020
Gio2018
suggested changes
Jun 3, 2020
| @@ -47,6 +84,7 @@ class GoogleAuthenticator: NSObject { | |||
| private override init() {} | |||
| var loginDelegate: GoogleAuthenticatorLoginDelegate? | |||
| var signupDelegate: GoogleAuthenticatorSignupDelegate? | |||
| var delegate: GoogleAuthenticatorDelegate? | |||
There was a problem hiding this comment.
This property should be weak (and the related protocol, class bound), otherwise it looks like an instance of GoogleAuthViewController is retained after the login completes. Here's a screenshot of the memory graph showing that.
Note that when I tweaked that var to weak, this did not happen. It maybe worth looking in the two existing delegates as well
There was a problem hiding this comment.
Dang it! I always forget to make them weak. Thanks @Gio2018 ! I'll address that in my next PR!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ref: #282
Can be tested with WPiOS PR: wordpress-mobile/WordPress-iOS#14235
To accommodate unified Google auth flow:
GoogleAuthenticatorDelegatethat encompasses all delegate methods needed for the unified flow.GoogleAuthViewController(the VC for the unified flow).GoogleAuthenticatorLoginDelegateandGoogleAuthenticatorSignupDelegateremain to support the old separate flows (LoginPrologueViewController,LoginEmailViewController,SignupGoogleViewController) .Processing AccountHUD is now shown in the login flow (previously it was only shown in signup).With this change, the Google flows are unified. Since
GoogleAuthViewControlleris presented from both login and signup, they both use the same flow. 🎉There is one path that is not unified yet.
After a Google account is selected,
GoogleAuthenticatorwill first attempt to login. If a WP account does not exist, it does not redirect to signup as there is no logic currently to do so. (That's the point of this project after all, innit? 😄 ) Instead, this view is displayed. This will be addressed in a future PR. Stay tuned!