Fix regressions found in the login with OAuth flow #1976
Conversation
| @@ -37,6 +37,10 @@ public final class LoginToutViewController: UIViewController, MFMailComposeViewC | |||
| |> \.translatesAutoresizingMaskIntoConstraints .~ false | |||
| }() | |||
|
|
|||
| private lazy var loginOrSignUpWithOAuthButton = { UIButton(type: .custom) | |||
There was a problem hiding this comment.
Most of this boiler-plate code is splitting out the "Sign up or Log in" button from the "Log In" button, where it was stuffed earlier.
| } else { | ||
| self.navigationController?.pushViewController(LoginViewController.instantiate(), animated: true) | ||
| self.navigationItem.backBarButtonItem = UIBarButtonItem.back(nil, selector: nil) | ||
| self.navigationController?.pushViewController(LoginViewController.instantiate(), animated: true) |
There was a problem hiding this comment.
This is now only called by the traditional login button.
| } | ||
|
|
||
| fileprivate func pushOAuthFlow() { | ||
| guard featureLoginWithOAuthEnabled(), let session = createAuthorizationSession() else { |
There was a problem hiding this comment.
This is only called by the new login button.
This way, config flag changes that occur shortly after launch will update the UI appropriately.
amy-at-kickstarter
force-pushed
the
feat/adyer/reactive-login-logout-buttons
branch
from
6c2d6f7
to
ad0b58a
Compare
amy-at-kickstarter
changed the title
| @@ -298,6 +305,10 @@ public final class LoginToutViewController: UIViewController, MFMailComposeViewC | |||
| self.contextLabel.rac.text = self.viewModel.outputs.logInContextText | |||
| self.bringCreativeProjectsToLifeLabel.rac.hidden = self.viewModel.outputs.headlineLabelHidden | |||
|
|
|||
| self.loginButton.rac.hidden = self.viewModel.outputs.showLoginWithOAuth | |||
There was a problem hiding this comment.
Either show the new, combined button, or the two old buttons.
| let valueFromRemoteConfig = AppEnvironment.current.remoteConfigClient? | ||
| .isFeatureEnabled(featureKey: feature) | ||
|
|
||
| return valueFromDefaults ?? valueFromRemoteConfig ?? defaultValue |
There was a problem hiding this comment.
Slight refactoring to support new default values, and to make this easier to read and debug.
| @@ -117,6 +121,43 @@ final class LoginToutViewModelTests: TestCase { | |||
| ) | |||
| } | |||
|
|
|||
| func testStartSignupOrLoginWithOAuth() { | |||
There was a problem hiding this comment.
Some simple/boilerplate tests, just for sanity.
| (AppEnvironment.current.remoteConfigClient? | ||
| .isFeatureEnabled(featureKey: feature) ?? false) | ||
| let valueFromRemoteConfig = AppEnvironment.current.remoteConfigClient? | ||
| .isFeatureEnabled(featureKey: feature) |
There was a problem hiding this comment.
nit: Unlike before, valueFromRemoteConfig will now be calculated each time. I wouldn't expect that to be a problem (it's probably cached in the remote config client and thus not an expensive operation) but just in case it is actually expensive or always does a network call or something, I think we should stick with early returns. Ex use if let blocks with returns instead of just having one return statement at the end.
There was a problem hiding this comment.
Ah, good point - will fix then merge.
* Make the display of login/logout/oauth buttons reactive. This way, config flag changes that occur shortly after launch will update the UI appropriately. * Add tests for OAuth login button in LoginToutViewModelTests * Make featureLoginWithOAuthEnabled default to true, if no remote or local config is set * Change per review #1976
📲 What
featureLoginWithOAuthEnabledtrue.🤔 Why
featureLoginWithOAuthEnabled(). This means it was never updating, even after new values of the OAuth feature flag loaded. Fixing this bug ensures we can correctly use the feature flag for OAuth, in case we need to ramp down.