FTUE - Server selection #5691

ouchadam · 2022-04-04T16:34:36Z

Type of change

WIP Feature
Bugfix
Technical
Other :

Content

Fixes #2396 - Is behind the combined registration feature flag #5648

Adds a new server selection screen
Moves the homeserver selection related state to a dedicated state class
Extracts the authentication state logic to a use case with its own set of tests
URLs which use http:// do not have their prefix removed when editing the server url (as a way to remind the user they're insecure)

Motivation and context

Introduces a new server selection screen where the user can enter a homeserver url, the aim is to centralise the flow avoiding branching paths (simplifying the overall experience)

Screenshots / GIFs

Note - some screenshots are missing the EMS hyperlink

Interactions

DISMISS	3RD PARTY	MATRIX.ORG	HTTP	CREATE ACCOUNT

EMS GET IN TOUCH	EMS HYPERLINK

Theming

LIGHT	DARK

Devices


SMALL
PIXEL 4 XL		N/A
TABLET

Tests

Enabled the combined registration flag
Create start the create new account flow
Tap edit in the register screen

Tested devices

Physical
Emulator
OS version(s): 29, 31Sv2

github-actions · 2022-04-04T17:11:53Z

Unit Test Results

114 files +  4 114 suites +4 1m 29s ⏱️ +8s
201 tests +  6 201 ✔️ +  6 0 💤 ±0 0 ❌ ±0
674 runs +24 674 ✔️ +24 0 💤 ±0 0 ❌ ±0

Results for commit 5f9d3e1. ± Comparison against base commit 15cf065.

♻️ This comment has been updated with latest results.

ouchadam · 2022-04-06T08:04:21Z

vector/src/main/java/im/vector/app/features/onboarding/OnboardingViewModel.kt

@@ -139,7 +140,8 @@ class OnboardingViewModel @AssistedInject constructor(
            is OnboardingAction.UpdateServerType           -> handleUpdateServerType(action)
            is OnboardingAction.UpdateSignMode             -> handleUpdateSignMode(action)
            is OnboardingAction.InitWith                   -> handleInitWith(action)
-            is OnboardingAction.UpdateHomeServer           -> handleUpdateHomeserver(action).also { lastAction = action }
+            is OnboardingAction.SelectHomeServer           -> run { lastAction = action }.also { handleHomeserverChange(action.homeServerUrl) }


actions are set straight away as setting them afterwards implicitly relies on asynchronous coroutines (which isn't the case for tests!)

ouchadam · 2022-04-06T08:10:25Z

vector/src/main/java/im/vector/app/features/onboarding/OnboardingViewModel.kt

-                                homeServerUrlFromUser = null,
-                                homeServerUrl = null,
-                                loginMode = LoginMode.Unknown,
-                                serverType = ServerType.Unknown,


resetting the server url shouldn't also reset the server type as the server type is actually the user selection from the "Matrix, EMS, Other" page

ouchadam · 2022-04-06T08:16:14Z

vector/src/main/java/im/vector/app/features/onboarding/OnboardingViewState.kt

@@ -70,6 +61,15 @@ enum class OnboardingFlow {
    SignInSignUp
 }

+@Parcelize
+data class SelectedHomeserverState(


the individual homeserver state has been bundled into a single SelectedHomeserverState, this object is created/updated when starting the authentication flow (by selecting or editing a homeserver url) through a single entry point StartAuthenticationFlowUseCase.kt

amshakal

Thank you! 💯

ariskotsomitopoulos

Thanks for the large PR, good job for adding tests, I have some remarks!

ariskotsomitopoulos · 2022-04-06T09:36:58Z

vector/src/main/java/im/vector/app/core/extensions/Collections.kt

@@ -18,3 +18,5 @@ package im.vector.app.core.extensions

 inline fun <reified T> List<T>.nextOrNull(index: Int) = getOrNull(index + 1)
 inline fun <reified T> List<T>.prevOrNull(index: Int) = getOrNull(index - 1)
+
+fun <T> List<T>.containsAll(vararg items: T) = this.containsAll(items.toList())


I would avoid extension functions with the exact name as the original, I don't have a strong point though

I was imagining this as an overload for containsAll but I agree it can be tricky to discover the extension, will rename

ariskotsomitopoulos · 2022-04-06T09:45:35Z

vector/src/main/java/im/vector/app/features/onboarding/OnboardingViewModel.kt

@@ -139,7 +140,8 @@ class OnboardingViewModel @AssistedInject constructor(
            is OnboardingAction.UpdateServerType           -> handleUpdateServerType(action)
            is OnboardingAction.UpdateSignMode             -> handleUpdateSignMode(action)
            is OnboardingAction.InitWith                   -> handleInitWith(action)
-            is OnboardingAction.UpdateHomeServer           -> handleUpdateHomeserver(action).also { lastAction = action }
+            is OnboardingAction.SelectHomeServer           -> run { lastAction = action }.also { handleHomeserverChange(action.homeServerUrl) }


Maybe its worth create a function to wrap duplicate lines:

run { lastAction = action }.also { handleHomeserverChange(action.homeServerUrl) } run { lastAction = action }.also { handleHomeserverChange(action.homeServerUrl) }

great idea! I can extract the run { lastAction = action } but the handleHomeserverChange(action.homeServerUrl) is a bit trickier as we need to pull the server url from two different action types SelectHomeServer.homeServerUrl & EditHomeServer.homeServerUrl

could be a good case for a sealed interface with a common property, will have a play around!

Nice approach with the sealed interface, I like it

ariskotsomitopoulos · 2022-04-06T10:05:44Z

vector/src/main/java/im/vector/app/features/onboarding/OnboardingViewModel.kt

-                data.supportedLoginTypes.contains(LoginFlowTypes.PASSWORD)         -> LoginMode.Password
-                else                                                               -> LoginMode.Unsupported
-            }
+            runCatching { startAuthenticationFlowUseCase.execute(homeServerConnectionConfig) }.fold(


Using withContext(Dispatchers.IO) to startAuthenticationFlowUseCase will reduce the work on main thread. Or maybe using viewModelScope.launch(Dispatchers.IO) and exclude state updates to run on Main dispatcher

I wasn't sure about this, the authentication service has a bunch of internal non thread safe state, we can rely on the lower layers (retrofit/awaitTransaction) for doing the dispatcher switching for us, at least whilst the other parts aren't thread safe

but it does mean there's some thread jumping happening~

Yes, I was talking mainly for the logic that is running on the main thread, except retrofit calls etc that always run on another thread. But if we have thread safety issues, there are not much we can do!

ariskotsomitopoulos · 2022-04-06T10:13:14Z

...n/java/im/vector/app/features/onboarding/ftueauth/FtueAuthCombinedServerSelectionFragment.kt

+        return FragmentFtueServerSelectionCombinedBinding.inflate(inflater, container, false)
+    }
+
+    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {


Minor, I would have created some more methods instead of having all the login in onViewCreated

ariskotsomitopoulos · 2022-04-06T10:16:28Z

...ain/java/im/vector/app/features/onboarding/ftueauth/FtueAuthSignUpSignInSelectionFragment.kt

                views.loginSignupSigninText.text = getString(R.string.login_server_matrix_org_text)
            }
            ServerType.EMS       -> {
                views.loginSignupSigninServerIcon.setImageResource(R.drawable.ic_logo_element_matrix_services)
                views.loginSignupSigninServerIcon.isVisible = true
                views.loginSignupSigninTitle.text = getString(R.string.login_connect_to_modular)
-                views.loginSignupSigninText.text = state.homeServerUrlFromUser.toReducedUrl()
+                views.loginSignupSigninText.text = state.selectedHomeserver.userFacingUrl.toReducedUrl()
            }
            ServerType.Other     -> {
                views.loginSignupSigninServerIcon.isVisible = false
                views.loginSignupSigninTitle.text = getString(R.string.login_server_other_title)


We don't need setting the icon in ServerType.Other case?
views.loginSignupSigninServerIcon.setImageResource(R.drawable.ic_logo_element_matrix_services) or maybe set it to null?

for the other case we're hiding the icon with views.loginSignupSigninServerIcon.isVisible = false, this icon is the one under the element logo

OTHER MATRIX.ORG

also worth mentioning these screens are part of the legacy flow and will eventually be removed when the FTUE becomes the only flow 🤞

Thanks, for explaining

ariskotsomitopoulos · 2022-04-06T10:21:39Z

...ain/java/im/vector/app/features/onboarding/ftueauth/FtueAuthSignUpSignInSelectionFragment.kt

@@ -60,27 +60,27 @@ class FtueAuthSignUpSignInSelectionFragment @Inject constructor() : AbstractSSOF
            ServerType.MatrixOrg -> {


Extremely minor, maybe its worth creating a function like:

private fun update(@DrawableRes drawableId: Int, isServerIconVisible: Boolean, signInTitle: String, signInText: String ){ views.loginSignupSigninServerIcon.setImageResource(drawableId) views.loginSignupSigninServerIcon.isVisible = isServerIconVisible views.loginSignupSigninTitle.text = signInTitle views.loginSignupSigninText.text = signInText }

to reduce code duplication

ariskotsomitopoulos · 2022-04-06T10:39:01Z

vector/src/main/res/values/donottranslate.xml

+    <string name="ftue_auth_choose_server_entry_footer">You can only connect to a server that has already been set up</string>
+    <string name="ftue_auth_choose_server_ems_title">Want to host your own server?</string>
+
+    <string name="ftue_ems_url">https://element.io/ems</string>


Maybe we should add this to vector-config urls.xml ?

ariskotsomitopoulos · 2022-04-06T10:41:51Z

vector/src/test/java/im/vector/app/features/onboarding/OnboardingViewModelTest.kt

@@ -58,6 +62,9 @@ private val A_HOMESERVER_CAPABILITIES = aHomeServerCapabilities(canChangeDisplay
 private val AN_IGNORED_FLOW_RESULT = FlowResult(missingStages = emptyList(), completedStages = emptyList())
 private val ANY_CONTINUING_REGISTRATION_RESULT = RegistrationResult.FlowResponse(AN_IGNORED_FLOW_RESULT)
 private val A_LOGIN_OR_REGISTER_ACTION = OnboardingAction.LoginOrRegister("@a-user:id.org", "a-password", "a-device-name")
+private const val A_HOMESERVER_URL = "https://edited-homeserver.org"
+private val A_HOMESERVER_CONFIG = HomeServerConnectionConfig(FakeUri().instance)
+private val SELECTED_HOMESERVER_STATE = SelectedHomeserverState(preferredLoginMode = LoginMode.Password)

 class OnboardingViewModelTest {


+1 for adding Tests!

- replaces previous separately url strings with state usage - makes use of the state for updating the sign up and server selection fields

- helps to highlight a non secture connection, for https we strip the prefix

- moves some of the homeserver specific state to the selectServer model

- should better reflect this steps mvoes the onboarding process forwards

…e the cases differently

…count, fixes flaky initial value setting

…lection submit

…void duplication in the model

follows other ftue fragment conventions

…e duplication and better enforce consistency

ouchadam changed the title ~~Feature/adm/ftue server selection~~ FTUE - Server selection Apr 4, 2022

ouchadam force-pushed the feature/adm/ftue-server-selection branch 2 times, most recently from 9ed5786 to 3886c42 Compare April 5, 2022 11:58

ouchadam added the X-Needs-Design May require input from the design team label Apr 5, 2022

ouchadam requested a review from amshakal April 5, 2022 12:43

ouchadam marked this pull request as ready for review April 5, 2022 12:43

ouchadam added the PR-Large PR with more than 500 updated lines label Apr 6, 2022

ouchadam commented Apr 6, 2022

View reviewed changes

ouchadam requested review from a team and ariskotsomitopoulos and removed request for a team April 6, 2022 08:12

ouchadam commented Apr 6, 2022

View reviewed changes

amshakal approved these changes Apr 6, 2022

View reviewed changes

ariskotsomitopoulos approved these changes Apr 6, 2022

View reviewed changes

ouchadam added 14 commits April 8, 2022 13:16

adding barebones server selection UI

c3cf221

adding dedicated server selection state to onboarding state

7f90dda

- replaces previous separately url strings with state usage - makes use of the state for updating the sign up and server selection fields

keeping the http:// schema in the server selection input field

985dbfe

- helps to highlight a non secture connection, for https we strip the prefix

launching the edit selection from the ftue variant entry point

51c294a

extracting the authentication start logic to a dedicated use case

f34df39

- moves some of the homeserver specific state to the selectServer model

renaming update homeserver to select homeserver

1e52012

- should better reflect this steps mvoes the onboarding process forwards

splitting the homeserver edit from the selection so that we can handl…

e9f5003

…e the cases differently

adding tests around the editing of the homeserver url

8b2e2a1

checking the input field content for emptyness rather than its child …

c022a38

…count, fixes flaky initial value setting

adding missing IME handling for the register next focus and server se…

89f182a

…lection submit

launching the ems site when tapping get in touch

1d92b42

addressing line length

2ba3bd3

adding link to ems copy

0ce2012

renaming homeserver url properties to better define their content

21102a2

ouchadam added 6 commits April 8, 2022 13:16

adding changelog entry

db50225

renaming extension to help with discoverability

f70d613

creating a common type for the edit/selecting of homeserver urls to a…

6304d5d

…void duplication in the model

extracting view setup to its own method -

05a7d40

follows other ftue fragment conventions

extracting server information rendering to a reusable method to reduc…

a2bcbfb

…e duplication and better enforce consistency

moving ftue ems url to the urls config file

5f9d3e1

ouchadam force-pushed the feature/adm/ftue-server-selection branch from 05bc538 to 5f9d3e1 Compare April 8, 2022 12:18

ouchadam removed the X-Needs-Design May require input from the design team label Apr 8, 2022

ouchadam merged commit bd3e980 into develop Apr 8, 2022

ouchadam deleted the feature/adm/ftue-server-selection branch April 8, 2022 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FTUE - Server selection #5691

FTUE - Server selection #5691

ouchadam commented Apr 4, 2022 •

edited

Loading

github-actions bot commented Apr 4, 2022 •

edited

Loading

ouchadam Apr 6, 2022

ouchadam Apr 6, 2022

ouchadam Apr 6, 2022

amshakal left a comment

ariskotsomitopoulos left a comment

ariskotsomitopoulos Apr 6, 2022

ouchadam Apr 8, 2022

ouchadam Apr 8, 2022

ariskotsomitopoulos Apr 8, 2022

ariskotsomitopoulos Apr 6, 2022

ouchadam Apr 8, 2022

ouchadam Apr 8, 2022

ariskotsomitopoulos Apr 8, 2022

ariskotsomitopoulos Apr 6, 2022

ouchadam Apr 8, 2022

ariskotsomitopoulos Apr 8, 2022

ariskotsomitopoulos Apr 6, 2022

ouchadam Apr 8, 2022

ariskotsomitopoulos Apr 8, 2022

ariskotsomitopoulos Apr 6, 2022

ouchadam Apr 8, 2022 •

edited

Loading

ariskotsomitopoulos Apr 8, 2022

ariskotsomitopoulos Apr 6, 2022

ouchadam Apr 8, 2022

ariskotsomitopoulos Apr 8, 2022

ariskotsomitopoulos Apr 6, 2022

ouchadam Apr 8, 2022

ariskotsomitopoulos Apr 8, 2022

ariskotsomitopoulos Apr 6, 2022

		@@ -60,27 +60,27 @@ class FtueAuthSignUpSignInSelectionFragment @Inject constructor() : AbstractSSOF
		ServerType.MatrixOrg -> {

FTUE - Server selection #5691

FTUE - Server selection #5691

Conversation

ouchadam commented Apr 4, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Interactions

Theming

Devices

Tests

Tested devices

github-actions bot commented Apr 4, 2022 • edited Loading

Unit Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amshakal left a comment

Choose a reason for hiding this comment

ariskotsomitopoulos 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

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 Apr 8, 2022 • edited Loading

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

Choose a reason for hiding this comment

ouchadam commented Apr 4, 2022 •

edited

Loading

github-actions bot commented Apr 4, 2022 •

edited

Loading

ouchadam Apr 8, 2022 •

edited

Loading