#5307 Adds ForceLoginFallback feature flag to Login and Registration #5325

ericdecanini · 2022-02-23T16:16:35Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Adds a new feature flag forceLoginFallback used in FtueAuthVariant's registration and login flows to force web registration / login to be used

Motivation and context

Addresses Issue #5307

I also took this as an opportunity to do some light refactoring within FtueAuthVariant with the only real change being the addition of said feature flag. It might look more functions have been added / removed in the diff when in reality they've only been moved around (and some functions have been split out for better abstraction and readability).

I limited this refactoring to only the flows directly related to where the feature flag was used, otherwise this PR would be far larger than it needs to be.

I suggest looking through the commits when reviewing as I've separated out the behavioural changes and the refactoral changes

Screenshots / GIFs

Feature Flag

Usage

Registration	Login

Tests

Run the build on a fresh install (or clear your data)
On the first FTUE screen go to developer options > private settings and enable Force login and registration fallback
Attempt a sign in & registration and see that they both force you to fallback to following the web flow

Tested devices

Physical
Emulator
OS version(s): Android 10

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

…ignin

…tration-feature-flag # Conflicts: # vector/src/debug/java/im/vector/app/features/debug/features/DebugFeaturesStateFactory.kt # vector/src/debug/java/im/vector/app/features/debug/features/DebugVectorFeatures.kt # vector/src/main/java/im/vector/app/features/VectorFeatures.kt

ericdecanini · 2022-02-23T16:37:53Z

library/core-utils/src/main/java/im/vector/lib/core/utils/extensions/Global.kt

+package im.vector.lib.core.utils.extensions
+
+@Suppress("UNUSED_PARAMETER")
+fun doNothing(reason: String = "") = Unit


Especially useful in single line lambdas (e.g. when statements, example below) as a cleaner alternative to { /* do nothing */ }

Can't we just write { Unit } ?

ericdecanini · 2022-02-23T16:38:39Z

vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt

@@ -228,6 +224,25 @@ class FtueAuthVariant(
        }.exhaustive
    }

+    private fun registrationShouldFallback(registrationFlowResult: OnboardingViewEvents.RegistrationFlowResult) =
+            vectorFeatures.isForceLoginFallbackEnabled() || registrationFlowResult.containsUnsupportedRegistrationFlow()


Real change no. 1

ericdecanini · 2022-02-23T16:39:28Z

vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt

                .setNegativeButton(R.string.no, null)
                .show()
    }

+    private fun handleSignInWithMatrixId(state: OnboardingViewState) {
+        if (vectorFeatures.isForceLoginFallbackEnabled())
+            onLoginModeNotSupported(state.loginModeSupportedTypes)


Real change no. 3

ericdecanini · 2022-02-23T16:39:45Z

vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt

-                ?.let { onboardingViewModel.handle(OnboardingAction.LoginWithToken(it)) }
+    private fun handleSignInSelected(state: OnboardingViewState) {
+        if (vectorFeatures.isForceLoginFallbackEnabled())
+            onLoginModeNotSupported(state.loginModeSupportedTypes)


Real change no. 2

github-actions · 2022-02-23T17:04:22Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
passed="25" failures="17" errors="0" skipped="2"
[org.matrix.android.sdk.account]
passed="1" failures="1" errors="0" skipped="0"
[org.matrix.android.sdk.internal]
passed="158" failures="12" errors="0" skipped="38"
[org.matrix.android.sdk.ordering]
passed="1" failures="1" errors="0" skipped="0"
[org.matrix.android.sdk.PermalinkParserTest]
passed="1" failures="1" errors="0" skipped="0"

github-actions · 2022-02-23T17:26:34Z

Unit Test Results

  84 files ±0   84 suites ±0 1m 2s ⏱️ -2s
157 tests ±0 157 ✔️ ±0 0 💤 ±0 0 ❌ ±0
504 runs ±0 504 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 65242df. ± Comparison against base commit 9e9dc1d.

♻️ This comment has been updated with latest results.

bmarty

Thanks for the PR. Some remarks.

bmarty · 2022-02-23T18:14:20Z

library/core-utils/src/main/java/im/vector/lib/core/utils/extensions/Global.kt

+package im.vector.lib.core.utils.extensions
+
+@Suppress("UNUSED_PARAMETER")
+fun doNothing(reason: String = "") = Unit


Can't we just write { Unit } ?

bmarty · 2022-02-23T18:15:44Z

vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt

@@ -110,7 +111,7 @@ class FtueAuthVariant(
    }

    override fun setIsLoading(isLoading: Boolean) {
-        // do nothing
+        doNothing()


I would prefer a comment, or write override fun setIsLoading(isLoading: Boolean) = Unit. I do not know what others think.

Fair enough. This is purely my personal preference, but Unit definitely works too

bmarty · 2022-02-23T18:17:50Z

vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt

+        is LoginMode.SsoAndPassword,
+        LoginMode.Password    -> openAuthLoginFragmentWithTag(FRAGMENT_LOGIN_TAG)
+        LoginMode.Unsupported -> onLoginModeNotSupported(state.loginModeSupportedTypes)
+    }.exhaustive


Here .exhaustive is not necessary anymore, since when statement is "assigned" to the fun

Good catch!

bmarty · 2022-02-23T18:21:47Z

vector/src/debug/java/im/vector/app/features/debug/features/DebugVectorFeatures.kt

@@ -106,4 +108,5 @@ object DebugFeatureKeys {
    val onboardingSplashCarousel = booleanPreferencesKey("onboarding-splash-carousel")
    val onboardingUseCase = booleanPreferencesKey("onbboarding-splash-carousel")
    val onboardingPersonalize = booleanPreferencesKey("onbboarding-personalize")
+    val forceLoginFallback = booleanPreferencesKey("force-login-fallback")


Maybe better to add this simple boolean to the screen DebugPrivateSettingsFragment as suggested by @ouchadam , since this is really something for internal test and not a feature we can disable or enable.

that was my thinking ^^^ I don't see us wanting to promoting this setting for production/all users

ouchadam · 2022-02-24T16:19:05Z

vector/src/main/java/im/vector/app/features/settings/VectorDataStore.kt

@@ -59,4 +59,16 @@ class VectorDataStore @Inject constructor(
            settings[forceDialPadDisplay] = force
        }
    }
+
+    private val forceLoginFallback = booleanPreferencesKey("force_login_fallback")


I would have expected this to be beside the dial pad option within the VectorDataStore (along with the store read/write access below) 🤔

I have an upcoming PR to move the override options out of the VectorDataStore and into a VectorOverrides abstraction

Soz for the conflict 😅 For visibility sake, because they're pretty minor, we decided that you'd resolve them on your pr

ouchadam

Looking good from my side! thanks for adding the screenshots 💯

bmarty

Thanks for the update!

bmarty · 2022-02-28T13:19:45Z

vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt

@@ -75,6 +75,8 @@ class FtueAuthVariant(
    private val popEnterAnim = R.anim.no_anim
    private val popExitAnim = R.anim.exit_fade_out

+    private var isForceLoginFallbackEnabled = false


Ok for this private var, as a cache, because the boolean is also in the state.

ericdecanini added 5 commits February 23, 2022 13:02

adds forceLoginFallback debug feature

507ddc2

adds feature flag usage to registration flow result

7132488

refactors FtueAuthVariant with new feature flag on registration and s…

f18808b

…ignin

adds changelog file

137d804

ericdecanini commented Feb 23, 2022

View reviewed changes

adds missing brackets on multiline if statements

c89f28f

bmarty reviewed Feb 23, 2022

View reviewed changes

bmarty requested a review from ouchadam February 23, 2022 18:22

ericdecanini added 9 commits February 24, 2022 14:45

Reverts adding force login fallback flag to debug features

4ebaa34

Adds forceLoginFallback private setting

2917d4e

Adds forceLoginFallback private setting view

092761c

Changes copy value of forceLoginFallback to it

981393f

Adds private setting usage to FtueAuthVariant

92c6d59

Reorders some functions within FtueAuthVariant

3d57d72

Removes Global doNothing function

8d0410d

Removes unnecessary exhaustive on when statement

9f0cef7

Adds missing invalidation step to forceLoginFallback checkbox

65242df

ouchadam reviewed Feb 24, 2022

View reviewed changes

ericdecanini requested review from bmarty and ouchadam February 24, 2022 16:27

ouchadam approved these changes Feb 24, 2022

View reviewed changes

bmarty approved these changes Feb 28, 2022

View reviewed changes

bmarty reviewed Feb 28, 2022

View reviewed changes

bmarty merged commit 2790506 into develop Feb 28, 2022

bmarty deleted the feature/eric/registration-feature-flag branch February 28, 2022 13:20

ouchadam mentioned this pull request Feb 28, 2022

Lifting debug overrides to their own abstraction #5361

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#5307 Adds ForceLoginFallback feature flag to Login and Registration #5325

#5307 Adds ForceLoginFallback feature flag to Login and Registration #5325

ericdecanini commented Feb 23, 2022 •

edited

Loading

ericdecanini Feb 23, 2022

bmarty Feb 23, 2022

ericdecanini Feb 23, 2022

ericdecanini Feb 23, 2022

ericdecanini Feb 23, 2022

github-actions bot commented Feb 23, 2022 •

edited

Loading

github-actions bot commented Feb 23, 2022 •

edited

Loading

bmarty left a comment

bmarty Feb 23, 2022

bmarty Feb 23, 2022

ericdecanini Feb 24, 2022

bmarty Feb 23, 2022

ericdecanini Feb 24, 2022

bmarty Feb 23, 2022

ouchadam Feb 23, 2022

ouchadam Feb 24, 2022 •

edited

Loading

ericdecanini Feb 24, 2022

ouchadam left a comment

bmarty left a comment

bmarty Feb 28, 2022

#5307 Adds ForceLoginFallback feature flag to Login and Registration #5325

#5307 Adds ForceLoginFallback feature flag to Login and Registration #5325

Conversation

ericdecanini commented Feb 23, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 23, 2022 • edited Loading

Matrix SDK

Integration Tests Results:

github-actions bot commented Feb 23, 2022 • edited Loading

Unit Test Results

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam left a comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericdecanini commented Feb 23, 2022 •

edited

Loading

github-actions bot commented Feb 23, 2022 •

edited

Loading

github-actions bot commented Feb 23, 2022 •

edited

Loading

ouchadam Feb 24, 2022 •

edited

Loading