Skip to content

FTUE - Onboarding registration steps unit tests #5408

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.

Sign up for GitHub

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

Merged
ouchadam merged 20 commits into from
Mar 18, 2022
Merged

FTUE - Onboarding registration steps unit tests #5408

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
c15e908
converting onboarding action to sealed interface
ouchadam Mar 1, 2022
4225f62
adding test helper for asserting states whilst combining previous upd…
ouchadam Mar 2, 2022
3fa4150
extracting registration steps to separate handler to make testing the…
ouchadam Mar 2, 2022
434ee67
ensure the pid add/resend methods do not trigger the next registratio…
ouchadam Mar 2, 2022
b2a1aa1
adding commas to separate the test name sections
ouchadam Mar 2, 2022
75cbb72
cleaning up test names and bodies to be clearer
ouchadam Mar 2, 2022
804513c
adding case for result ignoring register actions
ouchadam Mar 2, 2022
390ae43
allowing test withPrevious to be supplied a list
ouchadam Mar 2, 2022
694016f
adding test case for the non loading registration steps
ouchadam Mar 2, 2022
fc5c057
adding changelog entry
ouchadam Mar 2, 2022
fe206fe
fixing wrong action for starting the sign up
ouchadam Mar 4, 2022
3d20d46
enabling the personalize step for the unit tests preemptively for the…
ouchadam Mar 7, 2022
d77061b
removing fully qualified import
ouchadam Mar 17, 2022
5df2ae9
updating with previous state helper and including javadoc to help exp…
ouchadam Mar 17, 2022
d514751
avoiding shadowed lambda parameters
ouchadam Mar 17, 2022
ba76aac
removing unused fake helper methods
ouchadam Mar 17, 2022
192d1c4
converting open class to sealed interface for extra type safety
ouchadam Mar 17, 2022
abf62af
extracting named function out for cancelling the email validation job…
ouchadam Mar 17, 2022
7f943d3
explicitly declaring the fake registrationb wizard as not relaxed and…
ouchadam Mar 18, 2022
ce2c309
including verification to ensure no other methods are being called
ouchadam Mar 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/5408.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Improved onboarding registration unit test coverage
68 changes: 27 additions & 41 deletions vector/src/main/java/im/vector/app/features/onboarding/OnboardingAction.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,45 +22,31 @@ import im.vector.app.features.login.LoginConfig
import im.vector.app.features.login.ServerType
import im.vector.app.features.login.SignMode
import org.matrix.android.sdk.api.auth.data.Credentials
import org.matrix.android.sdk.api.auth.registration.RegisterThreePid
import org.matrix.android.sdk.internal.network.ssl.Fingerprint

sealed class OnboardingAction : VectorViewModelAction {
data class OnGetStarted(val resetLoginConfig: Boolean, val onboardingFlow: OnboardingFlow) : OnboardingAction()
data class OnIAlreadyHaveAnAccount(val resetLoginConfig: Boolean, val onboardingFlow: OnboardingFlow) : OnboardingAction()

data class UpdateServerType(val serverType: ServerType) : OnboardingAction()
data class UpdateHomeServer(val homeServerUrl: String) : OnboardingAction()
data class UpdateUseCase(val useCase: FtueUseCase) : OnboardingAction()
object ResetUseCase : OnboardingAction()
data class UpdateSignMode(val signMode: SignMode) : OnboardingAction()
data class LoginWithToken(val loginToken: String) : OnboardingAction()
data class WebLoginSuccess(val credentials: Credentials) : OnboardingAction()
data class InitWith(val loginConfig: LoginConfig?) : OnboardingAction()
data class ResetPassword(val email: String, val newPassword: String) : OnboardingAction()
object ResetPasswordMailConfirmed : OnboardingAction()
sealed interface OnboardingAction : VectorViewModelAction {
data class OnGetStarted(val resetLoginConfig: Boolean, val onboardingFlow: OnboardingFlow) : OnboardingAction
data class OnIAlreadyHaveAnAccount(val resetLoginConfig: Boolean, val onboardingFlow: OnboardingFlow) : OnboardingAction

data class UpdateServerType(val serverType: ServerType) : OnboardingAction
data class UpdateHomeServer(val homeServerUrl: String) : OnboardingAction
data class UpdateUseCase(val useCase: FtueUseCase) : OnboardingAction
object ResetUseCase : OnboardingAction
data class UpdateSignMode(val signMode: SignMode) : OnboardingAction
data class LoginWithToken(val loginToken: String) : OnboardingAction
data class WebLoginSuccess(val credentials: Credentials) : OnboardingAction
data class InitWith(val loginConfig: LoginConfig?) : OnboardingAction
data class ResetPassword(val email: String, val newPassword: String) : OnboardingAction
object ResetPasswordMailConfirmed : OnboardingAction

// Login or Register, depending on the signMode
data class LoginOrRegister(val username: String, val password: String, val initialDeviceName: String) : OnboardingAction()

// Register actions
open class RegisterAction : OnboardingAction()

data class AddThreePid(val threePid: RegisterThreePid) : RegisterAction()
object SendAgainThreePid : RegisterAction()

// TODO Confirm Email (from link in the email, open in the phone, intercepted by the app)
data class ValidateThreePid(val code: String) : RegisterAction()

data class CheckIfEmailHasBeenValidated(val delayMillis: Long) : RegisterAction()
object StopEmailValidationCheck : RegisterAction()
data class LoginOrRegister(val username: String, val password: String, val initialDeviceName: String) : OnboardingAction
object StopEmailValidationCheck : OnboardingAction

data class CaptchaDone(val captchaResponse: String) : RegisterAction()
object AcceptTerms : RegisterAction()
object RegisterDummy : RegisterAction()
data class PostRegisterAction(val registerAction: RegisterAction) : OnboardingAction

// Reset actions
open class ResetAction : OnboardingAction()
open class ResetAction : OnboardingAction

@mnaturel mnaturel Mar 17, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we try to use sealed interface as well for ResetAction? Like this:

sealed interface ResetAction : OnboardingAction
object ResetHomeServerType : ResetAction
object ResetHomeServerUrl : ResetAction
object ResetSignMode : ResetAction
object ResetLogin : ResetAction
object ResetResetPassword : ResetAction

@ouchadam ouchadam Mar 17, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah! makes sense since I'm in the area 👍


object ResetHomeServerType : ResetAction()
object ResetHomeServerUrl : ResetAction()
Expand All @@ -69,16 +55,16 @@ sealed class OnboardingAction : VectorViewModelAction {
object ResetResetPassword : ResetAction()

// Homeserver history
object ClearHomeServerHistory : OnboardingAction()
object ClearHomeServerHistory : OnboardingAction

data class PostViewEvent(val viewEvent: OnboardingViewEvents) : OnboardingAction()
data class PostViewEvent(val viewEvent: OnboardingViewEvents) : OnboardingAction

data class UserAcceptCertificate(val fingerprint: Fingerprint) : OnboardingAction()
data class UserAcceptCertificate(val fingerprint: Fingerprint) : OnboardingAction

object PersonalizeProfile : OnboardingAction()
data class UpdateDisplayName(val displayName: String) : OnboardingAction()
object UpdateDisplayNameSkipped : OnboardingAction()
data class ProfilePictureSelected(val uri: Uri) : OnboardingAction()
object SaveSelectedProfilePicture : OnboardingAction()
object UpdateProfilePictureSkipped : OnboardingAction()
object PersonalizeProfile : OnboardingAction
data class UpdateDisplayName(val displayName: String) : OnboardingAction
object UpdateDisplayNameSkipped : OnboardingAction
data class ProfilePictureSelected(val uri: Uri) : OnboardingAction
object SaveSelectedProfilePicture : OnboardingAction
object UpdateProfilePictureSkipped : OnboardingAction
}
177 changes: 43 additions & 134 deletions vector/src/main/java/im/vector/app/features/onboarding/OnboardingViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ class OnboardingViewModel @AssistedInject constructor(
private val vectorFeatures: VectorFeatures,
private val analyticsTracker: AnalyticsTracker,
private val uriFilenameResolver: UriFilenameResolver,
private val registrationActionHandler: RegistrationActionHandler,
private val vectorOverrides: VectorOverrides
) : VectorViewModel<OnboardingViewState, OnboardingAction, OnboardingViewEvents>(initialState) {

Expand Down Expand Up @@ -116,16 +117,16 @@ class OnboardingViewModel @AssistedInject constructor(

private val matrixOrgUrl = stringProvider.getString(R.string.matrix_org_server_url).ensureTrailingSlash()

private val registrationWizard: RegistrationWizard
get() = authenticationService.getRegistrationWizard()

val currentThreePid: String?
get() = registrationWizard?.currentThreePid
get() = registrationWizard.currentThreePid

// True when login and password has been sent with success to the homeserver
val isRegistrationStarted: Boolean
get() = authenticationService.isRegistrationStarted

private val registrationWizard: RegistrationWizard?
get() = authenticationService.getRegistrationWizard()

private val loginWizard: LoginWizard?
get() = authenticationService.getLoginWizard()

Expand Down Expand Up @@ -153,7 +154,7 @@ class OnboardingViewModel @AssistedInject constructor(
is OnboardingAction.WebLoginSuccess -> handleWebLoginSuccess(action)
is OnboardingAction.ResetPassword -> handleResetPassword(action)
is OnboardingAction.ResetPasswordMailConfirmed -> handleResetPasswordMailConfirmed()
is OnboardingAction.RegisterAction -> handleRegisterAction(action)
is OnboardingAction.PostRegisterAction -> handleRegisterAction(action.registerAction)
is OnboardingAction.ResetAction -> handleResetAction(action)
is OnboardingAction.UserAcceptCertificate -> handleUserAcceptCertificate(action)
OnboardingAction.ClearHomeServerHistory -> handleClearHomeServerHistory()
Expand All @@ -164,6 +165,7 @@ class OnboardingViewModel @AssistedInject constructor(
is OnboardingAction.ProfilePictureSelected -> handleProfilePictureSelected(action)
OnboardingAction.SaveSelectedProfilePicture -> updateProfilePicture()
is OnboardingAction.PostViewEvent -> _viewEvents.post(action.viewEvent)
OnboardingAction.StopEmailValidationCheck -> currentJob = null

@mnaturel mnaturel Mar 17, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you think we could add a comment to explain what type of Job we are cancelling here. I find it hard to understand what process we are cancelling here: there are several assignments for currentJob in the ViewModel.

@ouchadam ouchadam Mar 17, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

as part of the registration flow it's cancelling the wait for email verification step (when we send an email and wait for the user to click the verification link in their email client), however like you mentioned the viewmodel reuses the same currentJob instance for all of its tasks

for some extra context

  • the currentJob uses a property setter to cancel any previous jobs on update
private var currentJob: Job? = null
    set(value) {
        // Cancel any previous Job
        field?.cancel()
        field = value
    }

The email validation waiting is tied to the pause/resume lifecycle

override fun onResume() {
    viewModel.handle(OnboardingAction.PostRegisterAction(RegisterAction.CheckIfEmailHasBeenValidated(0)))
}

override fun onPause() {
    viewModel.handle(OnboardingAction.StopEmailValidationCheck)
}

I would prefer to avoid adding a comment if a suitable method could be extracted, how do you feel about...

OnboardingAction.StopEmailValidationCheck      -> cancelWaitForEmailValidation()

fun cancelWaitForEmailValidation() {
  currentJob = null
}

@mnaturel mnaturel Mar 18, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes okay it is clear with a new private method, no need for comments in that case.

}.exhaustive
}

Expand Down Expand Up @@ -266,131 +268,41 @@ class OnboardingViewModel @AssistedInject constructor(
}
}

private fun handleRegisterAction(action: OnboardingAction.RegisterAction) {
when (action) {
is OnboardingAction.CaptchaDone -> handleCaptchaDone(action)
is OnboardingAction.AcceptTerms -> handleAcceptTerms()
is OnboardingAction.RegisterDummy -> handleRegisterDummy()
is OnboardingAction.AddThreePid -> handleAddThreePid(action)
is OnboardingAction.SendAgainThreePid -> handleSendAgainThreePid()
is OnboardingAction.ValidateThreePid -> handleValidateThreePid(action)
is OnboardingAction.CheckIfEmailHasBeenValidated -> handleCheckIfEmailHasBeenValidated(action)
is OnboardingAction.StopEmailValidationCheck -> handleStopEmailValidationCheck()
}
}

private fun handleCheckIfEmailHasBeenValidated(action: OnboardingAction.CheckIfEmailHasBeenValidated) {
// We do not want the common progress bar to be displayed, so we do not change asyncRegistration value in the state
currentJob = executeRegistrationStep(withLoading = false) {
it.checkIfEmailHasBeenValidated(action.delayMillis)
}
}

private fun handleStopEmailValidationCheck() {
currentJob = null
}

private fun handleValidateThreePid(action: OnboardingAction.ValidateThreePid) {
currentJob = executeRegistrationStep {
it.handleValidateThreePid(action.code)
}
}

private fun executeRegistrationStep(withLoading: Boolean = true,
block: suspend (RegistrationWizard) -> RegistrationResult): Job {
if (withLoading) {
setState { copy(asyncRegistration = Loading()) }
}
return viewModelScope.launch {
try {
registrationWizard?.let { block(it) }
/*
// Simulate registration disabled
throw Failure.ServerError(MatrixError(
code = MatrixError.FORBIDDEN,
message = "Registration is disabled"
), 403))
*/
} catch (failure: Throwable) {
if (failure !is CancellationException) {
_viewEvents.post(OnboardingViewEvents.Failure(failure))
}
null
}
?.let { data ->
when (data) {
is RegistrationResult.Success -> onSessionCreated(data.session, isAccountCreated = true)
is RegistrationResult.FlowResponse -> onFlowResponse(data.flowResult)
}
}

setState {
copy(
asyncRegistration = Uninitialized
)
}
}
}

private fun handleAddThreePid(action: OnboardingAction.AddThreePid) {
setState { copy(asyncRegistration = Loading()) }
currentJob = viewModelScope.launch {
try {
registrationWizard?.addThreePid(action.threePid)
} catch (failure: Throwable) {
_viewEvents.post(OnboardingViewEvents.Failure(failure))
}
setState {
copy(
asyncRegistration = Uninitialized
)
}
}
}

private fun handleSendAgainThreePid() {
setState { copy(asyncRegistration = Loading()) }
private fun handleRegisterAction(action: RegisterAction) {
currentJob = viewModelScope.launch {
try {
registrationWizard?.sendAgainThreePid()
} catch (failure: Throwable) {
_viewEvents.post(OnboardingViewEvents.Failure(failure))
}
setState {
copy(
asyncRegistration = Uninitialized
)
}
}
}

private fun handleAcceptTerms() {
currentJob = executeRegistrationStep {
it.acceptTerms()
}
}

private fun handleRegisterDummy() {
currentJob = executeRegistrationStep {
it.dummy()
if (action.hasLoadingState()) {
setState { copy(asyncRegistration = Loading()) }
}
kotlin.runCatching { registrationActionHandler.handleRegisterAction(registrationWizard, action) }

@mnaturel mnaturel Mar 17, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we can remove the kotlin. prefix here?

@ouchadam ouchadam Mar 17, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch, will do 👍

.fold(
onSuccess = {
when {
action.ignoresResult() -> {
// do nothing
}
else -> when (it) {
is RegistrationResult.Success -> onSessionCreated(it.session, isAccountCreated = true)
is RegistrationResult.FlowResponse -> onFlowResponse(it.flowResult)
}
}
},
onFailure = {
if (it !is CancellationException) {
_viewEvents.post(OnboardingViewEvents.Failure(it))
}
}
)
setState { copy(asyncRegistration = Uninitialized) }
}
}

private fun handleRegisterWith(action: OnboardingAction.LoginOrRegister) {
reAuthHelper.data = action.password
currentJob = executeRegistrationStep {
it.createAccount(
action.username,
action.password,
action.initialDeviceName
)
}
}

private fun handleCaptchaDone(action: OnboardingAction.CaptchaDone) {
currentJob = executeRegistrationStep {
it.performReCaptcha(action.captchaResponse)
}
handleRegisterAction(RegisterAction.CreateAccount(
action.username,
action.password,
action.initialDeviceName
))
}

private fun handleResetAction(action: OnboardingAction.ResetAction) {
Expand Down Expand Up @@ -461,7 +373,7 @@ class OnboardingViewModel @AssistedInject constructor(
}

when (action.signMode) {
SignMode.SignUp -> startRegistrationFlow()
SignMode.SignUp -> handleRegisterAction(RegisterAction.StartRegistration)
SignMode.SignIn -> startAuthenticationFlow()
SignMode.SignInWithMatrixId -> _viewEvents.post(OnboardingViewEvents.OnSignModeSelected(SignMode.SignInWithMatrixId))
SignMode.Unknown -> Unit
Expand Down Expand Up @@ -499,7 +411,7 @@ class OnboardingViewModel @AssistedInject constructor(

// If there is a pending email validation continue on this step
try {
if (registrationWizard?.isRegistrationStarted == true) {
if (registrationWizard.isRegistrationStarted) {
currentThreePid?.let {
handle(OnboardingAction.PostViewEvent(OnboardingViewEvents.OnSendEmailSuccess(it)))
}
Expand Down Expand Up @@ -730,12 +642,6 @@ class OnboardingViewModel @AssistedInject constructor(
}
}

private fun startRegistrationFlow() {
currentJob = executeRegistrationStep {
it.getRegistrationFlow()
}
}

private fun startAuthenticationFlow() {
// Ensure Wizard is ready
loginWizard
Expand All @@ -745,15 +651,18 @@ class OnboardingViewModel @AssistedInject constructor(

private fun onFlowResponse(flowResult: FlowResult) {
// If dummy stage is mandatory, and password is already sent, do the dummy stage now
if (isRegistrationStarted &&
flowResult.missingStages.any { it is Stage.Dummy && it.mandatory }) {
if (isRegistrationStarted && flowResult.missingStages.any { it is Stage.Dummy && it.mandatory }) {
handleRegisterDummy()
} else {
// Notify the user
_viewEvents.post(OnboardingViewEvents.RegistrationFlowResult(flowResult, isRegistrationStarted))
}
}

private fun handleRegisterDummy() {
handleRegisterAction(RegisterAction.RegisterDummy)
}

private suspend fun onSessionCreated(session: Session, isAccountCreated: Boolean) {
val state = awaitState()
state.useCase?.let { useCase ->
Expand Down
Loading