FTUE - Capability based personalisation flow #5375

ouchadam · 2022-02-28T11:26:41Z

Type of change

WIP Feature
Bugfix
Technical
Other :

Content

Draft PR, relies on #5361 #5323

Dynamically shows/hides the personalisation onboarding screen based on the homeservers capabilities.

when display name and profile picture are not supported

Account created only offers the Take me home button

when only changing display name is supported

Account created personalisation is enabled but only shows the display name step

when only changing profile picture is supported

Account created personalisation is enabled but only change profile picture step, removing the toolbar back to the match the display name flow

Also introduces debug overrides for the homeserver capabilities to make testing the different options easier.

Motivation and context

To only show personalisation options to users on homeservers which support them. Part of #5200

Screenshots / GIFs

ALL OPTIONS DISABLED	ONLY DISPLAY NAME	ONLY PROFILE PICTURE	ALL ENABLED

Tests

With a fresh install
Enable the Personalisation feature flag
Enable or disable forced homeserver capabilities within the private settings

Tested devices

Physical
Emulator
OS version(s): 29

github-actions · 2022-02-28T12:09:07Z

Unit Test Results

  98 files   98 suites 1m 22s ⏱️
178 tests 178 ✔️ 0 💤 0 ❌
584 runs 584 ✔️ 0 💤 0 ❌

Results for commit c06d3ff.

♻️ This comment has been updated with latest results.

github-actions · 2022-02-28T13:51:37Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
=passed=6 failures=17 errors=0 skipped=2
[org.matrix.android.sdk.account]
=passed=0 failures=1 errors=0 skipped=0
[org.matrix.android.sdk.internal]
=passed=0 failures=1 errors=0 skipped=0
[org.matrix.android.sdk.ordering]
=passed=0 failures=1 errors=0 skipped=0
[org.matrix.android.sdk.PermalinkParserTest]
=passed=2 failures=0 errors=0 skipped=0

ouchadam · 2022-03-10T09:16:21Z

vector/src/debug/java/im/vector/app/features/debug/settings/OverrideDropdownView.kt

+import androidx.appcompat.widget.AppCompatSpinner
+import im.vector.app.R
+
+class OverrideDropdownView @JvmOverloads constructor(


a reusable dropdown view for the Debug menu -> Private settings (in the debug sourceset)

Nice! 👍 I am a big fan of custom views!

bmarty

(I did not review the PR, just add 2 small comments 🙈 !)

changelog.d/5375.wip

bmarty · 2022-03-10T12:32:22Z

vector/src/main/res/layout/fragment_ftue_account_created.xml

+
+    <Button
+        android:id="@+id/accountCreatedTakeMeHomeCta"
+        style="@style/Widget.Vector.Button.Login"


We have a handy Widget.Vector.Button.CallToAction but it's used much (it's quite new). I am wondering if we should at some point use this style a bit more.

I think it makes sense to use the CTA style, especially when it's defined in the components library https://www.figma.com/file/X4XTH9iS2KGJ2wFKDqkyed/Compound?node-id=1970%3A25160

I'm tempted to update all the usages in one go in a separate PR rather than just this layout, what do you think?

Sounds like a plan! (that was my original intention to make you think that 👿 )

mnaturel

Nice! I have only done static code review. Only small remarks and questions.
Is there a doc I can find somewhere to learn how to test account creation with a local homeserver?

mnaturel · 2022-03-10T13:34:36Z

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

 import kotlinx.coroutines.flow.map
 import org.matrix.android.sdk.api.extensions.orFalse

 private val Context.dataStore: DataStore<Preferences> by preferencesDataStore(name = "vector_overrides")
 private val keyForceDialPadDisplay = booleanPreferencesKey("force_dial_pad_display")
 private val keyForceLoginFallback = booleanPreferencesKey("force_login_fallback")
+private val forceCanChangeDisplayName = booleanPreferencesKey("force_can_change_display_name")


I have never used DataStore API but I see this so much cleaner than the SharedPreferences API. 😄

the main "downside" is that the api forces us to use suspend functions or flows (maybe a good thing from a performance standpoint 😄 )

mnaturel · 2022-03-10T13:41:52Z

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

@@ -51,4 +62,18 @@ class DebugVectorOverrides(private val context: Context) : VectorOverrides {
            settings[keyForceLoginFallback] = force
        }
    }
+
+    suspend fun updateHomeserverCapabilities(block: HomeserverCapabilitiesOverride.() -> HomeserverCapabilitiesOverride) {


Maybe we could rename it to setHomeserverCapabilities to keep the same convention as the other methods?

good point, will do 👍

mnaturel · 2022-03-10T13:53:33Z

vector/src/debug/java/im/vector/app/features/debug/settings/DebugPrivateSettingsViewActions.kt

+sealed interface DebugPrivateSettingsViewActions : VectorViewModelAction {
+    data class SetDialPadVisibility(val force: Boolean) : DebugPrivateSettingsViewActions
+    data class SetForceLoginFallbackEnabled(val force: Boolean) : DebugPrivateSettingsViewActions
+    data class SetDisplayNameCapabilityOverride(val option: BooleanHomeserverCapabilitiesOverride?) : DebugPrivateSettingsViewActions


I am just wondering why is the option BooleanHomeserverCapabilitiesOverride a nullable? Is this a case we should handle? I thought it would be either enabled or disabled.

it's because the override allows the option to be both forced to disabled or enabled, null means don't override

we could replace the 3 way dropdown with a checkbox wrapped with another checkbox but I felt using nullable would be less boilerplate~

Okay it makes sense. I forgot it is an override setting.

mnaturel · 2022-03-10T14:09:48Z

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

@@ -51,4 +62,18 @@ class DebugVectorOverrides(private val context: Context) : VectorOverrides {
            settings[keyForceLoginFallback] = force
        }
    }
+
+    suspend fun updateHomeserverCapabilities(block: HomeserverCapabilitiesOverride.() -> HomeserverCapabilitiesOverride) {


I am wondering if we should switch to Dispatchers.io for write operations? Right now it uses implicitly the Dispatcher.main.immediate from the viewModelScope

ah good point, I was under the assumption the datastore api would internally switch, I'll double check!

can confirm we're okay https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:datastore/datastore-core/src/main/java/androidx/datastore/core/SingleProcessDataStore.kt;l=88

under the hood there's an IO scope created

mnaturel · 2022-03-10T14:15:20Z

vector/src/debug/java/im/vector/app/features/debug/settings/OverrideDropdownView.kt

+    init {
+        orientation = HORIZONTAL
+        gravity = Gravity.CENTER_VERTICAL
+        inflate(context, R.layout.view_boolean_dropdown, this)


If you want you could migrate to ViewBinding like in this custom View.

will do, thanks for the example 👍

mnaturel · 2022-03-10T14:25:35Z

vector/src/main/java/im/vector/app/features/DefaultVectorOverrides.kt

+        val canChangeDisplayName: Boolean?,
+        val canChangeAvatar: Boolean?
+)
+
 class DefaultVectorOverrides : VectorOverrides {


I am a little confused about the purpose of DefaultVectorOverrides class. I see it is only used for testing purpose. Am I right? If it is the case, we may move it to the FakeVectorOverrides which is currently not used. What do you think?

the missing piece of the puzzle here is that the default implementations are injected by the release variant of the app release module whereas the debug module allows for providing the runtime overridable version

Okay thanks for explaining.

mnaturel · 2022-03-10T14:36:46Z

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

            is OnboardingAction.UpdateDisplayName          -> updateDisplayName(action.displayName)
-            OnboardingAction.UpdateDisplayNameSkipped      -> _viewEvents.post(OnboardingViewEvents.OnDisplayNameSkipped)
+            OnboardingAction.UpdateDisplayNameSkipped      -> handleDisplayNameStepComplete()
            OnboardingAction.UpdateProfilePictureSkipped   -> _viewEvents.post(OnboardingViewEvents.OnPersonalizationComplete)


I guess we could create a method onPersonalizationComplete to be used each time we need to post the event OnboardingViewEvents.OnPersonalizationComplete. What do you think?

happy to extract 👍 the reason I left it inlined was because there was no additional logic needed

mnaturel · 2022-03-10T14:51:34Z

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

@@ -42,8 +44,15 @@ class FtueAuthAccountCreatedFragment @Inject constructor(

    private fun setupViews() {
        views.accountCreatedSubtitle.text = getString(R.string.ftue_account_created_subtitle, activeSessionHolder.getActiveSession().myUserId)
-        views.accountCreatedPersonalize.debouncedClicks { viewModel.handle(OnboardingAction.PostViewEvent(OnboardingViewEvents.OnPersonalizeProfile)) }
+        views.accountCreatedPersonalize.debouncedClicks { viewModel.handle(OnboardingAction.PersonalizeProfile) }
        views.accountCreatedTakeMeHome.debouncedClicks { viewModel.handle(OnboardingAction.PostViewEvent(OnboardingViewEvents.OnTakeMeHome)) }


Why are there 2 buttons accountCreatedTakeMeHome and accountCreatedTakeMeHomeCta? Is it to handle the 2 cases: can personalize and cannot?

yes exactly ^^^ the screen is reused for both cases with slightly different button styling

PERSONALISE ENABLED PERSONALISE DISABLED

I could reuse the PERSONALIZE PROFILE button instead of creating a separate view, what do you think?

(I would vote to have separate views, it's easier to apply different style in the layout, and also the action are not the same so could be more confusing if we update the text and the action depending on the state)

Okay for me as well to have 2 different views.

mnaturel · 2022-03-10T14:57:14Z

vector/src/test/java/im/vector/app/test/fakes/FakeVectorOverrides.kt

+import im.vector.app.features.DefaultVectorOverrides
+import im.vector.app.features.VectorOverrides
+
+class FakeVectorOverrides : VectorOverrides by DefaultVectorOverrides()


This class is not used, is it normal?

🤦 nope, it's meant to be used here https://github.com/vector-im/element-android/pull/5375/files#diff-9bd5641dbabf55509a3dea8218d6d7d326781e4a23bf46867ec249eb640de34aR267

functionally the same but the abstraction enables reuse, will update 👍

This class is not used, is it normal?

good catch!

mnaturel · 2022-03-10T16:11:15Z

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

-        } else {
-            navigateToHome(createdAccount = true)
-        }
+        activity.supportFragmentManager.popBackStack(null, FragmentManager.POP_BACK_STACK_INCLUSIVE)


Just an idea, it would be cool if we could encapsulate all navigation code into a dedicated interface. It abstracts the navigation implementation and reduce the component responsability. Of course I don't suggest to change it in this PR 😄 But maybe later?

we have one for activities but not fragments, I think it's great idea and would help towards making this class testable

I've tracked it as an issue #5508

ouchadam · 2022-03-11T12:23:46Z

Nice! I have only done static code review. Only small remarks and questions. Is there a doc I can find somewhere to learn how to test account creation with a local homeserver?

there's nothing specific needed for testing account creation from the synapse standpoint, the easiest way to get setup is to run the demo script https://github.com/matrix-org/synapse#quick-start

mnaturel

I didn't achieve to run the Synapse demo server on local (some strange config errors when starting server) to test the code. But I approve the PR since I don't see anything wrong in static.

ouchadam · 2022-03-14T11:54:16Z

@mnaturel thanks for the review 👍 (and trying to run synapse locally, I'm happy to help out if you want to give it another go)

- cases for the personalisation and display name actions

…er avatar

- moves the personalisation state to a dedicated model to allow for back and forth state restoration

…ilities

…erver supports personalisation

…ser home if the display name personalisation is not supported

…f the homeserver - when avatars can't be changed we complete the personlisation flow

…'t supported but pictures are when personalising the profile

… if personalization is supported - also removes a legacy loading workaround for the account creation step, we're navigating to a new screen AccountCreated so we have to stop the loading

Co-authored-by: Benoit Marty <[email protected]>

ouchadam mentioned this pull request Feb 28, 2022

Mobile FTUE: Profile Personalisation screens (Android) #5200

Closed

10 tasks

ouchadam mentioned this pull request Mar 1, 2022

FTUE - Personalisation complete #5389

Merged

6 tasks

ouchadam added the Z-FTUE Issue is relevant to the first time use project or experience label Mar 3, 2022

ouchadam force-pushed the feature/adm/display-personalisation-based-on-capabilities branch from c3c193d to 147de6e Compare March 7, 2022 16:43

ouchadam changed the base branch from feature/adm/tmp-base to develop March 7, 2022 16:43

ouchadam marked this pull request as ready for review March 7, 2022 16:44

ouchadam requested review from a team and mnaturel and removed request for a team March 10, 2022 09:15

ouchadam commented Mar 10, 2022

View reviewed changes

bmarty reviewed Mar 10, 2022

View reviewed changes

mnaturel reviewed Mar 10, 2022

View reviewed changes

ouchadam mentioned this pull request Mar 10, 2022

Extract onboarding navigation #5508

Open

ouchadam requested a review from mnaturel March 11, 2022 12:17

ouchadam mentioned this pull request Mar 11, 2022

Make use of the CTA design library component in the onboarding #5515

Open

mnaturel approved these changes Mar 14, 2022

View reviewed changes

ouchadam added 10 commits March 14, 2022 11:54

adding base choose name fragment with UI

4a1bf11

add click handling for the display name actions

074cde4

adding tests around the onboarding view model

1c80914

- cases for the personalisation and display name actions

adding barebones profile picture fragment with ability to select a us…

232524d

…er avatar

extracting method for the handling of the profile picture selection

567fd9a

adding dedicated camera icon for choosing profile picture

6c4381f

add click handling for the display name actions

0685b57

add click handling for the display name actions

7ded900

updating upstream avatar on profile picture save and continue step

10e4fd1

- moves the personalisation state to a dedicated model to allow for back and forth state restoration

extracting method for the handling of the profile picture selection

50740b1

ouchadam and others added 15 commits March 14, 2022 11:54

adding entry points for injecting and overriding the homeserver capab…

3df4f1e

…ilities

dynamically changing the account created layout based on if the homes…

537c2f5

…erver supports personalisation

adding test around account creation via dummy

a033243

hiding the toolbar back button and handling system back as take the u…

46be481

…ser home if the display name personalisation is not supported

dynamically switching the onboarding flow based on the capabilities o…

716928d

…f the homeserver - when avatars can't be changed we complete the personlisation flow

formatting

b5778bd

forwarding to the profile picture flow when display name changing isn…

4b9b177

…'t supported but pictures are when personalising the profile

using correct label for the avatar capability debug override

ab9e440

adding changelog entry

7e79d7e

taking the personalization feature flag into account when calculating…

bdedffb

… if personalization is supported - also removes a legacy loading workaround for the account creation step, we're navigating to a new screen AccountCreated so we have to stop the loading

using consistent method naming for setting the capabilities override

edee6ab

making use of binding api instead of manual findviewbyid

d89cc71

extracting the personalization complete emitting to a dedicated function

c2fe669

making use of the fake overrides for testing

c84ce5a

Updating changelog copy

c06d3ff

Co-authored-by: Benoit Marty <[email protected]>

ouchadam force-pushed the feature/adm/display-personalisation-based-on-capabilities branch from 21222c1 to c06d3ff Compare March 14, 2022 12:58

mnaturel approved these changes Mar 14, 2022

View reviewed changes

ouchadam enabled auto-merge March 14, 2022 13:04

ouchadam merged commit 82e1afd into develop Mar 14, 2022

ouchadam deleted the feature/adm/display-personalisation-based-on-capabilities branch March 14, 2022 13:34

FTUE - Capability based personalisation flow #5375

FTUE - Capability based personalisation flow #5375

Conversation

ouchadam commented Feb 28, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

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

Unit Test Results

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

Matrix SDK

Integration Tests Results:

ouchadam Mar 10, 2022 • edited Loading

Choose a reason for hiding this 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel 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

ouchadam Mar 10, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam commented Mar 11, 2022 • edited Loading

mnaturel left a comment

Choose a reason for hiding this comment

ouchadam commented Mar 14, 2022

ouchadam commented Feb 28, 2022 •

edited

Loading

github-actions bot commented Feb 28, 2022 •

edited

Loading

github-actions bot commented Feb 28, 2022 •

edited

Loading

ouchadam Mar 10, 2022 •

edited

Loading

ouchadam Mar 10, 2022 •

edited

Loading

ouchadam commented Mar 11, 2022 •

edited

Loading