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

FTUE - Choose a display name #5211

Merged
merged 13 commits into from
Mar 2, 2022
Merged

Conversation

ouchadam
Copy link
Contributor

@ouchadam ouchadam commented Feb 11, 2022

Depends on #5158 Part of #5200

Type of change

  • WIP Feature
  • Bugfix
  • Technical
  • Other :

Content

Adds a Choose a display name screen to the onboarding personalisation flow, sits behind the FTUE personailzation feature flag.

Motivation and context

To allow the user to personalise their account during the account creation process

Screenshots / GIFs

*the gif animations are odd because the emulator has them turned off

EMPTY WITH CONTENT WITH CONTENT AND KEYBOARD
Screenshot_20220211_154053 Screenshot_20220211_154358 Screenshot_20220211_154117
GIF
after-update-name

Tests

  • Adds unit tests around the OnboardingViewModel (along with the fakes needed to setup the class) to ensure the display name is updated and errors are handled

Tested devices

  • Physical
  • Emulator
  • OS version(s): 32 / Sv2

@@ -897,6 +899,21 @@ class OnboardingViewModel @AssistedInject constructor(
fun getFallbackUrl(forSignIn: Boolean, deviceId: String?): String? {
return authenticationService.getFallbackUrl(forSignIn, deviceId)
}

private fun updateDisplayName(displayName: String) {
setState { copy(asyncDisplayName = Loading()) }
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 uses the same async state mechanism as the other parts of the onboarding state (sign in/register), the login V2 converts this to a simple isLoading: boolean but I've avoided doing the same change here to keep the diff smaller

}

private fun onDisplayNameUpdated() {
// TODO go to the real profile picture fragment
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 will be the next PR

@@ -102,10 +102,16 @@ class SharedSecureStorageViewModelTest {
viewModel.handle(SharedSecureStorageAction.UseKey)

test
.assertState(aViewState(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

assertState was removed in favour of a helper to assert all the states (including the initial state)

@github-actions
Copy link

github-actions bot commented Feb 11, 2022

Unit Test Results

  88 files  +  4    88 suites  +4   1m 10s ⏱️ +8s
160 tests +  3  160 ✔️ +  3  0 💤 ±0  0 ±0 
516 runs  +12  516 ✔️ +12  0 💤 ±0  0 ±0 

Results for commit 16424f7. ± Comparison against base commit b8a0aa7.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Feb 11, 2022

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    passed="25" failures="17" errors="0" skipped="2"
  • [org.matrix.android.sdk.account]
    passed="5" failures="0" errors="0" skipped="2"
  • [org.matrix.android.sdk.internal]
    passed="23" failures="1" errors="0" skipped="3"
  • [org.matrix.android.sdk.ordering]
    passed="16" failures="0" errors="0" skipped="0"
  • [org.matrix.android.sdk.PermalinkParserTest]
    passed="2" failures="0" errors="0" skipped="0"

@ouchadam ouchadam force-pushed the feature/adm/choose-display-name branch from cdd121c to 98f1085 Compare February 18, 2022 11:54
Base automatically changed from feature/adm/ftue-post-account-creation to develop February 23, 2022 10:54
@ouchadam ouchadam force-pushed the feature/adm/choose-display-name branch from 98f1085 to 4a01193 Compare February 23, 2022 12:50
@ouchadam ouchadam marked this pull request as ready for review February 23, 2022 12:56
@ouchadam ouchadam mentioned this pull request Feb 23, 2022
6 tasks
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Nice PR, thanks.
Just a quick remark about error management.


override fun onBackPressed(toolbarButton: Boolean): Boolean {
viewModel.handle(OnboardingAction.PostViewEvent(OnboardingViewEvents.OnTakeMeHome))
return false
Copy link
Member

@bmarty bmarty Feb 23, 2022

Choose a reason for hiding this comment

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

return 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.

ah 🤦 thought I'd included updates from the previous PR comments, will fix!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated 8dec4fd

views.displayNameInput.editText?.addTextChangedListener(object : SimpleTextWatcher() {
override fun afterTextChanged(s: Editable) {
val newContent = s.toString()
views.displayNameSubmit.isEnabled = newContent.isNotEmpty()
Copy link
Member

Choose a reason for hiding this comment

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

Could be nice to reset the error here.
Because the error when setting display name could be displayed in the TextInputLayout. You can trigger an error for instance by setting a display name with @

Copy link
Contributor Author

@ouchadam ouchadam Feb 25, 2022

Choose a reason for hiding this comment

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

at the moment errors are coming back in the form of the generic login dialog, I'm not seeing any errors when using @ in the display name though 🤔

2022-02-25T12:09:55,986486346+00:00

Copy link
Member

Choose a reason for hiding this comment

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

I mixed between display name and userId, my bad.

Copy link
Member

Choose a reason for hiding this comment

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

Still it could be nice to display the error in the TextInputLayout. You can do it easily by overidding onError in FtueAuthChooseDisplayNameFragment.

For network error (in airplane mode for instance), it's maybe better if the error is displayed in a dialog (so just call super.onError in this case).

Long display name can be rejected by the homeserver, and other rules may apply, that's why it could be nice to inline the error in such case IMO, instead of

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the screenshot and explanation! I've checked in with product/design, we'd like to do a pass of all the onboarding errors and their copy

I'll create another subtask in #5200 to revisit the error handling here in case there's other types of errors we'd like to capture within the text field instead of dialog and I'll need to track down all the possible error types (and their copy)

Copy link
Member

Choose a reason for hiding this comment

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

Ack

<com.google.android.material.textfield.TextInputEditText
android:layout_width="match_parent"
android:layout_height="match_parent"
android:maxLines="1" />
Copy link
Member

Choose a reason for hiding this comment

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

Could be nice to set an inputType and android:imeOptions="actionDone"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great idea, will do 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added 6fb5d05

@ouchadam ouchadam force-pushed the feature/adm/choose-display-name branch from 6fb5d05 to 16424f7 Compare February 25, 2022 12:26
@ouchadam ouchadam requested a review from bmarty March 2, 2022 12:43
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for the update.
Note a blocker, but could be nice to handle #5211 (comment)

@ouchadam
Copy link
Contributor Author

ouchadam commented Mar 2, 2022

thanks for the review 💯 the scope of the errors has increased a bit, I'll merge this in favour of a separate task/PR for the error updates #5211 (comment)

@ouchadam ouchadam merged commit 99e5a8f into develop Mar 2, 2022
@ouchadam ouchadam deleted the feature/adm/choose-display-name branch March 2, 2022 17:59
@ouchadam ouchadam added the Z-FTUE Issue is relevant to the first time use project or experience label Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-FTUE Issue is relevant to the first time use project or experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants