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] - Email input and verification #5868

Merged
merged 27 commits into from
May 23, 2022

Conversation

ouchadam
Copy link
Contributor

Type of change

  • WIP Feature
  • Bugfix
  • Technical
  • Other :

Content

Introduces new dedicated email entry and waiting for verification fragments to the FTUE onboarding flow.

  • Lifts the 401 verification polling error handling logic out of the fragment and into the RegisterActionHandler, adds tests around the polling.
  • A loading spinner is shown only when returning to the verification fragment rather than on the initial navigation
  • A dedicated resend email button has been added

Motivation and context

Fixes #5278

Screenshots / GIFs

Flow

after-signup-e2e

Phones

INPUT INPUT WITH CONTENT WAITING WAITING RETURN TO SCREEN
Screenshot_20220428_151111 Screenshot_20220428_151127 Screenshot_20220428_151135 Screenshot_20220428_151145

Small Phone

INPUT WAITING
Screenshot_20220428_151300 Screenshot_20220428_151312

Tablet

INPUT WAITING
Screenshot_20220428_151415 Screenshot_20220428_151426
Screenshot_20220428_151409 Screenshot_20220428_151433

Tests

  • Enable the combined register feature flag
  • Create an account of matrix.org
  • Notice the new email verification screens

Tested devices

  • Physical
  • Emulator
  • OS version(s): 28, 29, 31

@ouchadam ouchadam added the Z-FTUE Issue is relevant to the first time use project or experience label Apr 28, 2022
onSuccess = { it.toRegistrationResult() },
onFailure = {
when {
action.threePid is RegisterThreePid.Email && it.is401() -> RegistrationResult.SendEmailSuccess(action.threePid.email)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the 401 == successful pid sent logic is no longer handled at the UI/fragment layer but instead at our business logic layer

This logic feels like something that should be part of the SDK

Copy link
Member

Choose a reason for hiding this comment

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

I agree!

)
}

private tailrec suspend fun handleCheckIfEmailIsValidated(registrationWizard: RegistrationWizard, delayMillis: Long): RegistrationResult {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

uses tailrec to provide recursion with the safety of avoiding stackoverflows, the kotlin compiler will optimise this to an imperative loop

else -> RegistrationResult.Error(it)
}
}
) ?: handleCheckIfEmailIsValidated(registrationWizard, 10_000)
Copy link
Contributor Author

@ouchadam ouchadam Apr 28, 2022

Choose a reason for hiding this comment

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

previously we were providing the verification polling through the entire fragment -> viewmodel -> sdk stack, now it only happens here, the start/stop api remains the same

@github-actions
Copy link

github-actions bot commented Apr 28, 2022

Unit Test Results

124 files  ±  0  124 suites  ±0   2m 1s ⏱️ -9s
220 tests +  3  220 ✔️ +  3  0 💤 ±0  0 ±0 
726 runs  +12  726 ✔️ +12  0 💤 ±0  0 ±0 

Results for commit c71f9c8. ± Comparison against base commit 4094a66.

♻️ This comment has been updated with latest results.

@ouchadam ouchadam force-pushed the feature/adm/ftue-email-verification branch from 985178a to 4c114e4 Compare April 28, 2022 15:27
@ouchadam ouchadam changed the title FTUE - Email input and verification [FTUE] - Email input and verification Apr 28, 2022
@ouchadam ouchadam added the PR-Large PR with more than 500 updated lines label Apr 28, 2022
@ouchadam ouchadam force-pushed the feature/adm/ftue-email-verification branch from b9ff015 to 1a68211 Compare April 28, 2022 16:19
@ouchadam ouchadam requested review from a team and jmartinesp and removed request for a team April 29, 2022 07:55
@ouchadam ouchadam marked this pull request as ready for review April 29, 2022 08:12
@ouchadam ouchadam requested review from a team, bmarty and ganfra and removed request for a team and jmartinesp April 29, 2022 08:16
Copy link
Member

@ganfra ganfra left a comment

Choose a reason for hiding this comment

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

Some minor remarks, otherwise good work!

import javax.inject.Inject
import org.matrix.android.sdk.api.auth.registration.RegistrationResult as SdkRegistrationResult
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename SdkRegistrationResult by MatrixRegistrationResult?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

views.emailEntryInput.error = null
views.emailEntrySubmit.isEnabled = it.isEmail()
}
.launchIn(lifecycleScope)
Copy link
Member

Choose a reason for hiding this comment

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

It's safer to use viewLifecycleOwner.lifecycleScope

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 catch, 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.

b2e3dd1 updated other usages as well

viewModel.handle(OnboardingAction.PostRegisterAction(RegisterAction.CheckIfEmailHasBeenValidated(0)))
}

private fun showLoadingIfReturningToScreen() {
when (inferHasLeftAndReturnedToScreen) {
true -> views.emailVerificationWaiting.isVisible = true
Copy link
Member

Choose a reason for hiding this comment

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

Why are we not always showing the loader by the way?

Copy link
Contributor Author

@ouchadam ouchadam May 5, 2022

Choose a reason for hiding this comment

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

the idea was to avoid the impression that app was busy/loading when we're waiting for the user to click the link in the email verification

the middle ground was to show the spinner if we detect the user has left and returned to app as we're inferring that they've clicked the link and the app is now waiting for the homeserver to respond

Copy link
Member

Choose a reason for hiding this comment

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

I agree, this is a great UX improvement, thanks!

@ouchadam ouchadam force-pushed the feature/adm/ftue-email-verification branch from d037dde to 0c556a2 Compare May 5, 2022 13:02
@ouchadam ouchadam added the X-Needs-Design May require input from the design team label May 5, 2022
@ouchadam ouchadam requested a review from amshakal May 5, 2022 13:53
@ouchadam ouchadam force-pushed the feature/adm/ftue-email-verification branch from 6ae54a5 to e56a459 Compare May 9, 2022 08:36
@ouchadam ouchadam mentioned this pull request May 9, 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.

Great work!
Some remarks on the form.
Also, out of interest, how will be managed the case where a homeserver requires either an email or a phone number to let the user create an account?
Not sure if covered by the previous question, but is the optional email case correctly handled?

@Binds
@IntoMap
@FragmentKey(FtueAuthEmailEntryFragment::class)
fun bindFtueAuthEmailEntryFragment(fragment: FtueAuthEmailEntryFragment): Fragment
Copy link
Member

Choose a reason for hiding this comment

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

Maybe one day #5420 will be implemented...

viewModel.handle(OnboardingAction.PostRegisterAction(RegisterAction.CheckIfEmailHasBeenValidated(0)))
}

private fun showLoadingIfReturningToScreen() {
when (inferHasLeftAndReturnedToScreen) {
true -> views.emailVerificationWaiting.isVisible = true
Copy link
Member

Choose a reason for hiding this comment

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

I agree, this is a great UX improvement, thanks!

super.onError(throwable)
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for moving this out of the View!

app:layout_constraintBottom_toTopOf="@id/emailVerificationSpace5"
app:layout_constraintEnd_toEndOf="@id/ftueAuthGutterEnd"
app:layout_constraintStart_toStartOf="@id/ftueAuthGutterStart"
app:layout_constraintTop_toBottomOf="@id/emailVerificationFooter" />
Copy link
Member

Choose a reason for hiding this comment

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

Maybe show this Button and the TextView above with the same strategy than the loader, i.e. when we detect that the user is coming back after checking their email? Or after a delay of, let's say, 30s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will delegate this to @daniellekirkwood

I would be weary of hiding the button based on them leaving and returning as some users may use a different device to verify the email

@ouchadam
Copy link
Contributor Author

Great work! Some remarks on the form. Also, out of interest, how will be managed the case where a homeserver requires either an email or a phone number to let the user create an account? Not sure if covered by the previous question, but is the optional email case correctly handled?

I might be missing something, this PR is for the homeserver requiring an email, or is it possible to require email without also verifying it?

The phone number input case will be handled by another ticket #6043

Optional email wasn't something we were aware of, will raise with product

@ouchadam ouchadam requested a review from bmarty May 18, 2022 14:29
@ouchadam ouchadam force-pushed the feature/adm/ftue-email-verification branch from 0422b74 to 65832ef Compare May 19, 2022 09:50
ouchadam added 23 commits May 20, 2022 12:11
…lick logic for resending verification emails
…tching designs

- renames drawable which redirects to the attribute colorBackground
- fixes crash where the scheduled callbacks can attempt to trigger after the view has been destroyed
@ouchadam ouchadam force-pushed the feature/adm/ftue-email-verification branch from 65832ef to 98a421a Compare May 20, 2022 11:29
…it to be cancelled when resetting the auth flow

- extracts an auto cancelling job delegate
@ouchadam ouchadam force-pushed the feature/adm/ftue-email-verification branch from 98a421a to c71f9c8 Compare May 20, 2022 12:10
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.

LGTM (did not make a full review)

@ouchadam ouchadam merged commit 98999c7 into develop May 23, 2022
@ouchadam ouchadam deleted the feature/adm/ftue-email-verification branch May 23, 2022 10:14
@github-actions
Copy link

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    = passed=20 failures=0 errors=0 skipped=3
  • [org.matrix.android.sdk.account]
    = passed=3 failures=0 errors=0 skipped=2
  • [org.matrix.android.sdk.internal]
    = passed=28 failures=0 errors=0 skipped=0
  • [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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Large PR with more than 500 updated lines X-Needs-Design May require input from the design team 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.

Mobile FTUE: Email collect and verify screens (Android)
4 participants