From 5b01290057aa218c1e9d8cb16cc03d43a371afd7 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Tue, 19 Apr 2022 11:41:17 +0100 Subject: [PATCH 1/5] extracting registration flow handling to its own function - flattening nested ifs - formatting --- .../onboarding/ftueauth/FtueAuthVariant.kt | 99 +++++++++++-------- 1 file changed, 58 insertions(+), 41 deletions(-) diff --git a/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt b/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt index d5816762df..abf42f8329 100644 --- a/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt +++ b/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt @@ -131,24 +131,7 @@ class FtueAuthVariant( private fun handleOnboardingViewEvents(viewEvents: OnboardingViewEvents) { when (viewEvents) { is OnboardingViewEvents.RegistrationFlowResult -> { - if (registrationShouldFallback(viewEvents)) { - // Display a popup to propose use web fallback - onRegistrationStageNotSupported() - } else { - if (viewEvents.isRegistrationStarted) { - // Go on with registration flow - handleRegistrationNavigation(viewEvents.flowResult) - } else { - if (vectorFeatures.isOnboardingCombinedRegisterEnabled()) { - openCombinedRegister() - } else { - // First ask for login and password - // I add a tag to indicate that this fragment is a registration stage. - // This way it will be automatically popped in when starting the next registration stage - openAuthLoginFragmentWithTag(FRAGMENT_REGISTRATION_STAGE_TAG) - } - } - } + onRegistrationFlow(viewEvents) } is OnboardingViewEvents.OutdatedHomeserver -> { MaterialAlertDialogBuilder(activity) @@ -176,25 +159,33 @@ class FtueAuthVariant( is OnboardingViewEvents.OnServerSelectionDone -> onServerSelectionDone(viewEvents) is OnboardingViewEvents.OnSignModeSelected -> onSignModeSelected(viewEvents) is OnboardingViewEvents.OnLoginFlowRetrieved -> - activity.addFragmentToBackstack(views.loginFragmentContainer, + activity.addFragmentToBackstack( + views.loginFragmentContainer, FtueAuthSignUpSignInSelectionFragment::class.java, - option = commonOption) + option = commonOption + ) is OnboardingViewEvents.OnWebLoginError -> onWebLoginError(viewEvents) is OnboardingViewEvents.OnForgetPasswordClicked -> - activity.addFragmentToBackstack(views.loginFragmentContainer, + activity.addFragmentToBackstack( + views.loginFragmentContainer, FtueAuthResetPasswordFragment::class.java, - option = commonOption) + option = commonOption + ) is OnboardingViewEvents.OnResetPasswordSendThreePidDone -> { supportFragmentManager.popBackStack(FRAGMENT_LOGIN_TAG, POP_BACK_STACK_EXCLUSIVE) - activity.addFragmentToBackstack(views.loginFragmentContainer, + activity.addFragmentToBackstack( + views.loginFragmentContainer, FtueAuthResetPasswordMailConfirmationFragment::class.java, - option = commonOption) + option = commonOption + ) } is OnboardingViewEvents.OnResetPasswordMailConfirmationSuccess -> { supportFragmentManager.popBackStack(FRAGMENT_LOGIN_TAG, POP_BACK_STACK_EXCLUSIVE) - activity.addFragmentToBackstack(views.loginFragmentContainer, + activity.addFragmentToBackstack( + views.loginFragmentContainer, FtueAuthResetPasswordSuccessFragment::class.java, - option = commonOption) + option = commonOption + ) } is OnboardingViewEvents.OnResetPasswordMailConfirmationSuccessDone -> { // Go back to the login fragment @@ -221,11 +212,13 @@ class FtueAuthVariant( // This is handled by the Fragments Unit OnboardingViewEvents.OpenUseCaseSelection -> { - activity.addFragmentToBackstack(views.loginFragmentContainer, + activity.addFragmentToBackstack( + views.loginFragmentContainer, FtueAuthUseCaseFragment::class.java, - option = commonOption) + option = commonOption + ) } - OnboardingViewEvents.OpenCombinedRegister -> openCombinedRegister() + OnboardingViewEvents.OpenCombinedRegister -> openStartCombinedRegister() is OnboardingViewEvents.OnAccountCreated -> onAccountCreated() OnboardingViewEvents.OnAccountSignedIn -> onAccountSignedIn() OnboardingViewEvents.OnChooseDisplayName -> onChooseDisplayName() @@ -244,7 +237,21 @@ class FtueAuthVariant( } } - private fun openCombinedRegister() { + private fun onRegistrationFlow(viewEvents: OnboardingViewEvents.RegistrationFlowResult) { + when { + registrationShouldFallback(viewEvents) -> displayFallbackWebDialog() + viewEvents.isRegistrationStarted -> handleRegistrationNavigation(viewEvents.flowResult) + vectorFeatures.isOnboardingCombinedRegisterEnabled() -> openStartCombinedRegister() + else -> { + // First ask for login and password + // I add a tag to indicate that this fragment is a registration stage. + // This way it will be automatically popped in when starting the next registration stage + openAuthLoginFragmentWithTag(FRAGMENT_REGISTRATION_STAGE_TAG) + } + } + } + + private fun openStartCombinedRegister() { addRegistrationStageFragmentToBackstack(FtueAuthCombinedRegisterFragment::class.java) } @@ -254,14 +261,16 @@ class FtueAuthVariant( private fun OnboardingViewEvents.RegistrationFlowResult.containsUnsupportedRegistrationFlow() = flowResult.missingStages.any { !it.isSupported() } - private fun onRegistrationStageNotSupported() { + private fun displayFallbackWebDialog() { MaterialAlertDialogBuilder(activity) .setTitle(R.string.app_name) .setMessage(activity.getString(R.string.login_registration_not_supported)) .setPositiveButton(R.string.yes) { _, _ -> - activity.addFragmentToBackstack(views.loginFragmentContainer, + activity.addFragmentToBackstack( + views.loginFragmentContainer, FtueAuthWebFragment::class.java, - option = commonOption) + option = commonOption + ) } .setNegativeButton(R.string.no, null) .show() @@ -283,9 +292,11 @@ class FtueAuthVariant( when (OnboardingViewEvents.serverType) { ServerType.MatrixOrg -> Unit // In this case, we wait for the login flow ServerType.EMS, - ServerType.Other -> activity.addFragmentToBackstack(views.loginFragmentContainer, + ServerType.Other -> activity.addFragmentToBackstack( + views.loginFragmentContainer, FtueAuthServerUrlFormFragment::class.java, - option = commonOption) + option = commonOption + ) ServerType.Unknown -> Unit /* Should not happen */ } } @@ -317,10 +328,12 @@ class FtueAuthVariant( } private fun openAuthLoginFragmentWithTag(tag: String) { - activity.addFragmentToBackstack(views.loginFragmentContainer, + activity.addFragmentToBackstack( + views.loginFragmentContainer, FtueAuthLoginFragment::class.java, tag = tag, - option = commonOption) + option = commonOption + ) } private fun onLoginModeNotSupported(supportedTypes: List) { @@ -341,9 +354,11 @@ class FtueAuthVariant( } private fun openAuthWebFragment() { - activity.addFragmentToBackstack(views.loginFragmentContainer, + activity.addFragmentToBackstack( + views.loginFragmentContainer, FtueAuthWebFragment::class.java, - option = commonOption) + option = commonOption + ) } /** @@ -437,14 +452,16 @@ class FtueAuthVariant( } private fun onChooseDisplayName() { - activity.addFragmentToBackstack(views.loginFragmentContainer, + activity.addFragmentToBackstack( + views.loginFragmentContainer, FtueAuthChooseDisplayNameFragment::class.java, option = commonOption ) } private fun onChooseProfilePicture() { - activity.addFragmentToBackstack(views.loginFragmentContainer, + activity.addFragmentToBackstack( + views.loginFragmentContainer, FtueAuthChooseProfilePictureFragment::class.java, option = commonOption ) From 1a76b4d6806bafb903a9be0ebd6f1653fe88e47c Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Tue, 19 Apr 2022 11:56:13 +0100 Subject: [PATCH 2/5] ordering the ftue onboarding steps to match the design flow - only applied when the combined register flag is enabled --- .../onboarding/ftueauth/FtueAuthVariant.kt | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt b/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt index abf42f8329..00e96295be 100644 --- a/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt +++ b/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt @@ -239,10 +239,10 @@ class FtueAuthVariant( private fun onRegistrationFlow(viewEvents: OnboardingViewEvents.RegistrationFlowResult) { when { - registrationShouldFallback(viewEvents) -> displayFallbackWebDialog() - viewEvents.isRegistrationStarted -> handleRegistrationNavigation(viewEvents.flowResult) + registrationShouldFallback(viewEvents) -> displayFallbackWebDialog() + viewEvents.isRegistrationStarted -> handleRegistrationNavigation(viewEvents.flowResult.orderedStages()) vectorFeatures.isOnboardingCombinedRegisterEnabled() -> openStartCombinedRegister() - else -> { + else -> { // First ask for login and password // I add a tag to indicate that this fragment is a registration stage. // This way it will be automatically popped in when starting the next registration stage @@ -251,6 +251,20 @@ class FtueAuthVariant( } } + private fun FlowResult.orderedStages() = when { + vectorFeatures.isOnboardingCombinedRegisterEnabled() -> missingStages.sortedBy { + when (it) { + is Stage.Email -> 0 + is Stage.Msisdn -> 1 + is Stage.Terms -> 2 + is Stage.ReCaptcha -> 3 + is Stage.Other -> 4 + is Stage.Dummy -> 5 + } + } + else -> missingStages + } + private fun openStartCombinedRegister() { addRegistrationStageFragmentToBackstack(FtueAuthCombinedRegisterFragment::class.java) } @@ -370,15 +384,15 @@ class FtueAuthVariant( ?.let { onboardingViewModel.handle(OnboardingAction.LoginWithToken(it)) } } - private fun handleRegistrationNavigation(flowResult: FlowResult) { + private fun handleRegistrationNavigation(remainingStages: List) { // Complete all mandatory stages first - val mandatoryStage = flowResult.missingStages.firstOrNull { it.mandatory } + val mandatoryStage = remainingStages.firstOrNull { it.mandatory } if (mandatoryStage != null) { doStage(mandatoryStage) } else { // Consider optional stages - val optionalStage = flowResult.missingStages.firstOrNull { !it.mandatory && it !is Stage.Dummy } + val optionalStage = remainingStages.firstOrNull { !it.mandatory && it !is Stage.Dummy } if (optionalStage == null) { // Should not happen... } else { From 2d7b71f70d94970aa53eccb20704f9856245c866 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Tue, 19 Apr 2022 12:30:33 +0100 Subject: [PATCH 3/5] extracting stage ordering to its own class with test --- .../onboarding/ftueauth/FtueAuthVariant.kt | 11 +--- ...FtueMissingRegistrationStagesComparator.kt | 35 +++++++++++++ ...MissingRegistrationStagesComparatorTest.kt | 52 +++++++++++++++++++ .../vector/app/test/fixtures/StageFixtures.kt | 26 ++++++++++ 4 files changed, 114 insertions(+), 10 deletions(-) create mode 100644 vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueMissingRegistrationStagesComparator.kt create mode 100644 vector/src/test/java/im/vector/app/features/onboarding/ftueauth/FtueMissingRegistrationStagesComparatorTest.kt create mode 100644 vector/src/test/java/im/vector/app/test/fixtures/StageFixtures.kt diff --git a/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt b/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt index 00e96295be..b73d0c7997 100644 --- a/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt +++ b/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt @@ -252,16 +252,7 @@ class FtueAuthVariant( } private fun FlowResult.orderedStages() = when { - vectorFeatures.isOnboardingCombinedRegisterEnabled() -> missingStages.sortedBy { - when (it) { - is Stage.Email -> 0 - is Stage.Msisdn -> 1 - is Stage.Terms -> 2 - is Stage.ReCaptcha -> 3 - is Stage.Other -> 4 - is Stage.Dummy -> 5 - } - } + vectorFeatures.isOnboardingCombinedRegisterEnabled() -> missingStages.sortedWith(FtueMissingRegistrationStagesComparator()) else -> missingStages } diff --git a/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueMissingRegistrationStagesComparator.kt b/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueMissingRegistrationStagesComparator.kt new file mode 100644 index 0000000000..6a6326625e --- /dev/null +++ b/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueMissingRegistrationStagesComparator.kt @@ -0,0 +1,35 @@ +/* + * Copyright (c) 2022 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package im.vector.app.features.onboarding.ftueauth + +import org.matrix.android.sdk.api.auth.registration.Stage + +class FtueMissingRegistrationStagesComparator : Comparator { + + override fun compare(a: Stage?, b: Stage?): Int { + return (a?.toPriority() ?: 0) - (b?.toPriority() ?: 0) + } + + private fun Stage.toPriority() = when (this) { + is Stage.Email -> 0 + is Stage.Msisdn -> 1 + is Stage.Terms -> 2 + is Stage.ReCaptcha -> 3 + is Stage.Other -> 4 + is Stage.Dummy -> 5 + } +} diff --git a/vector/src/test/java/im/vector/app/features/onboarding/ftueauth/FtueMissingRegistrationStagesComparatorTest.kt b/vector/src/test/java/im/vector/app/features/onboarding/ftueauth/FtueMissingRegistrationStagesComparatorTest.kt new file mode 100644 index 0000000000..010cf5de60 --- /dev/null +++ b/vector/src/test/java/im/vector/app/features/onboarding/ftueauth/FtueMissingRegistrationStagesComparatorTest.kt @@ -0,0 +1,52 @@ +/* + * Copyright (c) 2022 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package im.vector.app.features.onboarding.ftueauth + +import im.vector.app.test.fixtures.aDummyStage +import im.vector.app.test.fixtures.aMsisdnStage +import im.vector.app.test.fixtures.aRecaptchaStage +import im.vector.app.test.fixtures.aTermsStage +import im.vector.app.test.fixtures.anEmailStage +import im.vector.app.test.fixtures.anOtherStage +import org.amshove.kluent.shouldBeEqualTo +import org.junit.Test + +class FtueMissingRegistrationStagesComparatorTest { + + @Test + fun `when ordering stages, then prioritizes email`() { + val input = listOf( + aDummyStage(), + anOtherStage(), + aMsisdnStage(), + anEmailStage(), + aRecaptchaStage(), + aTermsStage() + ) + + val result = input.sortedWith(FtueMissingRegistrationStagesComparator()) + + result shouldBeEqualTo listOf( + anEmailStage(), + aMsisdnStage(), + aTermsStage(), + aRecaptchaStage(), + anOtherStage(), + aDummyStage() + ) + } +} diff --git a/vector/src/test/java/im/vector/app/test/fixtures/StageFixtures.kt b/vector/src/test/java/im/vector/app/test/fixtures/StageFixtures.kt new file mode 100644 index 0000000000..e06993523c --- /dev/null +++ b/vector/src/test/java/im/vector/app/test/fixtures/StageFixtures.kt @@ -0,0 +1,26 @@ +/* + * Copyright (c) 2022 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package im.vector.app.test.fixtures + +import org.matrix.android.sdk.api.auth.registration.Stage + +fun aDummyStage() = Stage.Dummy(mandatory = true) +fun anEmailStage() = Stage.Email(mandatory = true) +fun aMsisdnStage() = Stage.Msisdn(mandatory = true) +fun aTermsStage() = Stage.Terms(mandatory = true, policies = emptyMap()) +fun aRecaptchaStage() = Stage.ReCaptcha(mandatory = true, publicKey = "any-key") +fun anOtherStage() = Stage.Other(mandatory = true, type = "raw-type", params = emptyMap()) From e736bc73f476bcd2f7ed7323bfc3bb63a24e921d Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Tue, 19 Apr 2022 12:41:13 +0100 Subject: [PATCH 4/5] adding changelog entry --- changelog.d/5783.wip | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/5783.wip diff --git a/changelog.d/5783.wip b/changelog.d/5783.wip new file mode 100644 index 0000000000..e306c0f217 --- /dev/null +++ b/changelog.d/5783.wip @@ -0,0 +1 @@ +Reorders the registration steps to prioritise email, then terms for the FTUE onboarding From 636f7fdc302bd29b4a8e7c7b8677b4635469a42a Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Tue, 26 Apr 2022 15:14:58 +0100 Subject: [PATCH 5/5] removing comment re-explains what the code is already doing --- .../app/features/onboarding/ftueauth/FtueAuthVariant.kt | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt b/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt index b73d0c7997..655b24de91 100644 --- a/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt +++ b/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt @@ -242,12 +242,7 @@ class FtueAuthVariant( registrationShouldFallback(viewEvents) -> displayFallbackWebDialog() viewEvents.isRegistrationStarted -> handleRegistrationNavigation(viewEvents.flowResult.orderedStages()) vectorFeatures.isOnboardingCombinedRegisterEnabled() -> openStartCombinedRegister() - else -> { - // First ask for login and password - // I add a tag to indicate that this fragment is a registration stage. - // This way it will be automatically popped in when starting the next registration stage - openAuthLoginFragmentWithTag(FRAGMENT_REGISTRATION_STAGE_TAG) - } + else -> openAuthLoginFragmentWithTag(FRAGMENT_REGISTRATION_STAGE_TAG) } }