Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix regressions found in the login with OAuth flow #1976

Merged
merged 4 commits into from
Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ public final class LoginToutViewController: UIViewController, MFMailComposeViewC
|> \.translatesAutoresizingMaskIntoConstraints .~ false
}()

private lazy var signupOrLoginWithOAuthButton = { UIButton(type: .custom)
|> \.translatesAutoresizingMaskIntoConstraints .~ false
}()

private lazy var loginContextStackView = { UIStackView() }()
private lazy var logoImageView = { UIImageView(frame: .zero) }()
internal var processingView: ProcessingView? = ProcessingView(frame: .zero)
Expand Down Expand Up @@ -138,15 +142,12 @@ public final class LoginToutViewController: UIViewController, MFMailComposeViewC
}
|> UILabel.lens.text %~ { _ in Strings.Get_notified_when_your_friends_back_and_launch_projects() }

if self.viewModel.outputs.loginWithOAuthEnabled {
// TODO: Add and translate a new version of this string for this page.
_ = self.loginButton |> greenButtonStyle
self.loginButton
.setTitle(Strings.login_tout_generic_intent_traditional_signup_or_login_button(), for: .normal)
} else {
_ = self.loginButton |> greyButtonStyle
self.loginButton.setTitle(Strings.login_tout_back_intent_traditional_login_button(), for: .normal)
}
_ = self.loginButton |> greyButtonStyle
self.loginButton.setTitle(Strings.login_tout_back_intent_traditional_login_button(), for: .normal)

_ = self.signupOrLoginWithOAuthButton |> greenButtonStyle
self.signupOrLoginWithOAuthButton
.setTitle(Strings.login_tout_generic_intent_traditional_signup_or_login_button(), for: .normal)

_ = self.loginContextStackView
|> UIStackView.lens.spacing .~ Styles.gridHalf(1)
Expand Down Expand Up @@ -190,6 +191,12 @@ public final class LoginToutViewController: UIViewController, MFMailComposeViewC
self?.pushSignupViewController()
}

self.viewModel.outputs.startOAuthSignupOrLogin
.observeForControllerAction()
.observeValues { [weak self] _ in
self?.pushOAuthFlow()
}

self.viewModel.outputs.logIntoEnvironmentWithApple
.observeValues { [weak self] accessTokenEnv in
AppEnvironment.login(accessTokenEnv)
Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either show the new, combined button, or the two old buttons.

self.signupButton.rac.hidden = self.viewModel.outputs.showLoginWithOAuth
self.signupOrLoginWithOAuthButton.rac.hidden = self.viewModel.outputs.showLoginWithOAuth.signal.negate()

self.viewModel.outputs.headlineLabelHidden
.observeForUI()
.observeValues { [weak self] isHidden in
Expand Down Expand Up @@ -357,12 +368,9 @@ public final class LoginToutViewController: UIViewController, MFMailComposeViewC
_ = ([self.appleLoginButton, self.fbLoginButton, self.getNotifiedLabel], self.fbLoginStackView)
|> ksr_addArrangedSubviewsToStackView()

if self.viewModel.outputs.loginWithOAuthEnabled {
self.emailLoginStackView.addArrangedSubview(self.loginButton)
} else {
self.emailLoginStackView.addArrangedSubview(self.signupButton)
self.emailLoginStackView.addArrangedSubview(self.loginButton)
}
self.emailLoginStackView.addArrangedSubview(self.signupOrLoginWithOAuthButton)
self.emailLoginStackView.addArrangedSubview(self.signupButton)
self.emailLoginStackView.addArrangedSubview(self.loginButton)
}

private func setupConstraints() {
Expand Down Expand Up @@ -391,6 +399,8 @@ public final class LoginToutViewController: UIViewController, MFMailComposeViewC
)
self.loginButton.addTarget(self, action: #selector(self.loginButtonPressed(_:)), for: .touchUpInside)
self.signupButton.addTarget(self, action: #selector(self.signupButtonPressed), for: .touchUpInside)
self.signupOrLoginWithOAuthButton
.addTarget(self, action: #selector(self.signupOrLoginWithOAuthButtonPressed), for: .touchUpInside)
}

private func attemptAppleLogin() {
Expand All @@ -410,13 +420,17 @@ public final class LoginToutViewController: UIViewController, MFMailComposeViewC
}

fileprivate func pushLoginViewController() {
if self.viewModel.outputs.loginWithOAuthEnabled, let session = createAuthorizationSession() {
session.presentationContextProvider = self
session.start()
} else {
self.navigationController?.pushViewController(LoginViewController.instantiate(), animated: true)
self.navigationItem.backBarButtonItem = UIBarButtonItem.back(nil, selector: nil)
self.navigationController?.pushViewController(LoginViewController.instantiate(), animated: true)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now only called by the traditional login button.

self.navigationItem.backBarButtonItem = UIBarButtonItem.back(nil, selector: nil)
}

fileprivate func pushOAuthFlow() {
guard featureLoginWithOAuthEnabled(), let session = createAuthorizationSession() else {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only called by the new login button.

return
}

session.presentationContextProvider = self
session.start()
}

fileprivate func createAuthorizationSession() -> ASWebAuthenticationSession? {
Expand Down Expand Up @@ -548,6 +562,10 @@ public final class LoginToutViewController: UIViewController, MFMailComposeViewC
@objc private func signupButtonPressed() {
self.viewModel.inputs.signupButtonPressed()
}

@objc private func signupOrLoginWithOAuthButtonPressed() {
self.viewModel.inputs.signupOrLoginWithOAuthButtonPressed()
}
}

extension LoginToutViewController: TabBarControllerScrollable {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,16 @@ internal final class LoginToutViewControllerTests: TestCase {
}

func testLoginToutView() {
let mockConfigClient = MockRemoteConfigClient()
mockConfigClient.features = [
RemoteConfigFeature.loginWithOAuthEnabled.rawValue: false
]

let devices = [Device.phone4_7inch, Device.phone5_8inch, Device.pad]
let intents = [LoginIntent.generic, .starProject, .messageCreator, .backProject]

orthogonalCombos(Language.allLanguages, devices, intents).forEach { language, device, intent in
withEnvironment(language: language) {
withEnvironment(language: language, remoteConfigClient: mockConfigClient) {
let controller = LoginToutViewController.configuredWith(loginIntent: intent)
let (parent, _) = traitControllers(device: device, orientation: .portrait, child: controller)

Expand Down
14 changes: 8 additions & 6 deletions Library/RemoteConfig/RemoteConfigFeature+Helpers.swift
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
/// Return remote config values either a value from the cloud, if it found one, or a default value based on the provided key
private func featureEnabled(feature: RemoteConfigFeature, defaultValue: Bool = false) -> Bool {
let valueFromDefaults = AppEnvironment.current.userDefaults
.remoteConfigFeatureFlags[feature.rawValue]

private func featureEnabled(feature: RemoteConfigFeature) -> Bool {
return AppEnvironment.current.userDefaults
.remoteConfigFeatureFlags[feature.rawValue] ??
(AppEnvironment.current.remoteConfigClient?
.isFeatureEnabled(featureKey: feature) ?? false)
let valueFromRemoteConfig = AppEnvironment.current.remoteConfigClient?
.isFeatureEnabled(featureKey: feature)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point - will fix then merge.


return valueFromDefaults ?? valueFromRemoteConfig ?? defaultValue
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight refactoring to support new default values, and to make this easier to read and debug.

}

public func featureBlockUsersEnabled() -> Bool {
Expand Down Expand Up @@ -32,7 +34,7 @@ public func featureReportThisProjectEnabled() -> Bool {
}

public func featureLoginWithOAuthEnabled() -> Bool {
featureEnabled(feature: .loginWithOAuthEnabled)
featureEnabled(feature: .loginWithOAuthEnabled, defaultValue: true)
}

public func featureUseKeychainForOAuthTokenEnabled() -> Bool {
Expand Down
21 changes: 18 additions & 3 deletions Library/ViewModels/LoginToutViewModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ public protocol LoginToutViewModelInputs {
/// Call when sign up button is pressed
func signupButtonPressed()

/// Call with login with OAuth button is pressed
func signupOrLoginWithOAuthButtonPressed()

/// Call when a user session starts.
func userSessionStarted()

Expand Down Expand Up @@ -92,6 +95,9 @@ public protocol LoginToutViewModelOutputs {
/// Emits when Signup view should be shown
var startSignup: Signal<(), Never> { get }

/// Emits when OAuth flow should be shown
var startOAuthSignupOrLogin: Signal<(), Never> { get }

/// Emits a Facebook user and access token when Facebook login has occurred
var startFacebookConfirmation: Signal<(ErrorEnvelope.FacebookUser?, String), Never> { get }

Expand All @@ -100,7 +106,7 @@ public protocol LoginToutViewModelOutputs {

/// True if the feature flag for OAuth login is true.
/// Note that this is not a signal, because we don't want it to ever change after the screen is loaded.
var loginWithOAuthEnabled: Bool { get }
var showLoginWithOAuth: Signal<Bool, Never> { get }
}

public protocol LoginToutViewModelType {
Expand All @@ -125,6 +131,7 @@ public final class LoginToutViewModel: LoginToutViewModelType, LoginToutViewMode
self.isLoading = isLoading.signal.skipRepeats()
self.startLogin = self.loginButtonPressedProperty.signal
self.startSignup = self.signupButtonPressedProperty.signal
self.startOAuthSignupOrLogin = self.signupOrLoginWithOAuthButtonPressedProperty.signal
self.attemptFacebookLogin = self.facebookLoginButtonPressedProperty.signal
self.attemptAppleLogin = self.appleLoginButtonPressedProperty.signal.ignoreValues()

Expand Down Expand Up @@ -269,7 +276,9 @@ public final class LoginToutViewModel: LoginToutViewModelType, LoginToutViewMode

self.logIntoEnvironmentWithApple = logIntoEnvironmentWithApple.signal
self.logIntoEnvironmentWithFacebook = logIntoEnvironmentWithFacebook.signal
self.loginWithOAuthEnabled = featureLoginWithOAuthEnabled()
self.showLoginWithOAuth = self.viewWillAppearProperty.signal.map { _ in
featureLoginWithOAuthEnabled()
}
}

public var inputs: LoginToutViewModelInputs { return self }
Expand Down Expand Up @@ -329,6 +338,11 @@ public final class LoginToutViewModel: LoginToutViewModelType, LoginToutViewMode
self.signupButtonPressedProperty.value = ()
}

fileprivate let signupOrLoginWithOAuthButtonPressedProperty = MutableProperty(())
public func signupOrLoginWithOAuthButtonPressed() {
self.signupOrLoginWithOAuthButtonPressedProperty.value = ()
}

fileprivate let userSessionStartedProperty = MutableProperty(())
public func userSessionStarted() {
self.userSessionStartedProperty.value = ()
Expand Down Expand Up @@ -356,10 +370,11 @@ public final class LoginToutViewModel: LoginToutViewModelType, LoginToutViewMode
public let startFacebookConfirmation: Signal<(ErrorEnvelope.FacebookUser?, String), Never>
public let startLogin: Signal<(), Never>
public let startSignup: Signal<(), Never>
public let startOAuthSignupOrLogin: Signal<(), Never>
public let startTwoFactorChallenge: Signal<String, Never>
public let showAppleErrorAlert: Signal<String, Never>
public let showFacebookErrorAlert: Signal<AlertError, Never>
public let loginWithOAuthEnabled: Bool
public let showLoginWithOAuth: Signal<Bool, Never>
}

private func statusString(_ forStatus: LoginIntent) -> String {
Expand Down
41 changes: 41 additions & 0 deletions Library/ViewModels/LoginToutViewModelTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ final class LoginToutViewModelTests: TestCase {
fileprivate let startFacebookConfirmation = TestObserver<String, Never>()
fileprivate let startLogin = TestObserver<(), Never>()
fileprivate let startSignup = TestObserver<(), Never>()
fileprivate let startOAuthSignupOrLogin = TestObserver<(), Never>()
fileprivate let startTwoFactorChallenge = TestObserver<String, Never>()
fileprivate let showLoginWithOAuth = TestObserver<Bool, Never>()

override func setUp() {
super.setUp()
Expand All @@ -47,7 +49,9 @@ final class LoginToutViewModelTests: TestCase {
.observe(self.startFacebookConfirmation.observer)
self.vm.outputs.startLogin.observe(self.startLogin.observer)
self.vm.outputs.startSignup.observe(self.startSignup.observer)
self.vm.outputs.startOAuthSignupOrLogin.observe(self.startOAuthSignupOrLogin.observer)
self.vm.outputs.startTwoFactorChallenge.observe(self.startTwoFactorChallenge.observer)
self.vm.outputs.showLoginWithOAuth.observe(self.showLoginWithOAuth.observer)
}

func testLoginIntent_Pledge() {
Expand Down Expand Up @@ -117,6 +121,43 @@ final class LoginToutViewModelTests: TestCase {
)
}

func testStartSignupOrLoginWithOAuth() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some simple/boilerplate tests, just for sanity.

self.vm.inputs.viewWillAppear()
self.vm.inputs.signupOrLoginWithOAuthButtonPressed()
self.startOAuthSignupOrLogin.assertValueCount(1, "OAuth signup/loginlogin emitted")
}

func testShowSignupOrLoginWithOAuth_featureFlagOn() {
let mockConfigClient = MockRemoteConfigClient()
mockConfigClient.features = [
RemoteConfigFeature.loginWithOAuthEnabled.rawValue: true
]

withEnvironment(remoteConfigClient: mockConfigClient) {
self.vm.inputs.viewWillAppear()
self.showLoginWithOAuth.assertValues([true])
}
}

func testShowSignupOrLoginWithOAuth_featureFlagOff() {
let mockConfigClient = MockRemoteConfigClient()
mockConfigClient.features = [
RemoteConfigFeature.loginWithOAuthEnabled.rawValue: false
]

withEnvironment(remoteConfigClient: mockConfigClient) {
self.vm.inputs.viewWillAppear()
self.showLoginWithOAuth.assertValues([false])
}
}

func testShowSignupOrLoginWithOAuth_featureFlagUnset() {
withEnvironment(remoteConfigClient: nil) {
self.vm.inputs.viewWillAppear()
self.showLoginWithOAuth.assertValues([true])
}
}

func testHeadlineLabelHidden() {
self.vm.inputs.configureWith(.starProject, project: nil, reward: nil)
self.vm.inputs.viewWillAppear()
Expand Down