FTUE - Only override sign up steps for `matrix.org` #6065

ouchadam · 2022-05-16T11:39:48Z

Type of change

WIP Feature
Bugfix
Technical
Other :

Content

Fixes #5783 (First iteration #5785)

Only overrides the order of the sign up flow if the selected server is matrix.org

Motivation and context

Ultimately we want the homeserver to control the sign up flow however until this capability easily exists, we'll override only matrix.org's order from the client side and gather feedback.

Screenshots / GIFs

Before	After

Tests

Create a new account of matrix.org
Notice email is asked first
Create an account on another server which has mandatory email (eg 0wnz.at)
Notice recpatcha is asked first

Tested devices

Physical
Emulator
OS version(s): 28

….org is selected - lifts the logic up to the view model

github-actions · 2022-05-16T12:17:06Z

Unit Test Results

122 files ±0 122 suites ±0 2m 14s ⏱️ -5s
205 tests ±0 205 ✔️ ±0 0 💤 ±0 0 ❌ ±0
690 runs ±0 690 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 5f2cb67. ± Comparison against base commit 955f366.

This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.

im.vector.app.features.onboarding.ftueauth.FtueMissingRegistrationStagesComparatorTest ‑ when ordering stages, then prioritizes email

im.vector.app.features.onboarding.ftueauth.MatrixOrgRegistrationStagesComparatorTest ‑ when ordering stages, then prioritizes email

♻️ This comment has been updated with latest results.

bmarty

LGTM, one small remark.

Also having a first time look on FtueMissingRegistrationStagesComparator, I am wondering if we could use compareTo instead of doing a subtraction, as per https://github.com/vector-im/element-android/blob/main/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/auth/data/SsoIdentityProvider.kt#L63.

Warning, it could reverse the order (I never know :) )

bmarty · 2022-05-16T15:50:29Z

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

    }

+    private fun OnboardingViewState.hasSelectedMatrixOrg() = selectedHomeserver.userFacingUrl == matrixOrgUrl
+
+    private fun FlowResult.overrideOrder() = copy(missingStages = missingStages.sortedWith(FtueMissingRegistrationStagesComparator()))


It's strange that this extension is actually doing a copyof the state. I would keep the copy at the call site.

will inline 👍

ouchadam · 2022-05-16T16:16:08Z

Also having a first time look on FtueMissingRegistrationStagesComparator, I am wondering if we could use compareTo instead of doing a subtraction, as per https://github.com/vector-im/element-android/blob/main/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/auth/data/SsoIdentityProvider.kt#L63.

Warning, it could reverse the order (I never know :) )

can do 👍 (and the test confirms it's the same order https://github.com/vector-im/element-android/blob/develop/vector/src/test/java/im/vector/app/features/onboarding/ftueauth/FtueMissingRegistrationStagesComparatorTest.kt#L31)

bmarty

Thanks for the update! One final non blocking remark.

bmarty · 2022-05-17T07:41:15Z

...a/im/vector/app/features/onboarding/ftueauth/MatrixOrgMissingRegistrationStagesComparator.kt

@@ -18,10 +18,10 @@ package im.vector.app.features.onboarding.ftueauth

 import org.matrix.android.sdk.api.auth.registration.Stage

-class FtueMissingRegistrationStagesComparator : Comparator<Stage> {
+class MatrixOrgMissingRegistrationStagesComparator : Comparator<Stage> {


Nit: thinking again about the name of this class, I think the word Missing could be removed.

bmarty · 2022-05-17T07:41:40Z

.../vector/app/features/onboarding/ftueauth/MatrixOrgMissingRegistrationStagesComparatorTest.kt

@@ -25,7 +25,7 @@ import im.vector.app.test.fixtures.anOtherStage
 import org.amshove.kluent.shouldBeEqualTo
 import org.junit.Test

-class FtueMissingRegistrationStagesComparatorTest {
+class MatrixOrgMissingRegistrationStagesComparatorTest {


Same remark about the name here then

bmarty

Perfect, thanks!

ganfra

All good for me, thanks!

github-actions · 2022-05-23T21:10:10Z

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=49 failures=1 errors=0 skipped=1
[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

applying the manual reordering of the sign up stages only when matrix…

a893f5a

….org is selected - lifts the logic up to the view model

ouchadam added Z-FTUE Issue is relevant to the first time use project or experience PR-Small PR with less than 20 updated lines labels May 16, 2022

ouchadam requested review from a team, yostyle and bmarty and removed request for a team May 16, 2022 13:09

ouchadam changed the title ~~FTYUE - Only override sign up steps for matrix.org~~ FTUE - Only override sign up steps for matrix.org May 16, 2022

bmarty reviewed May 16, 2022

View reviewed changes

ouchadam added 3 commits May 16, 2022 17:16

using compareTo instead of direct subtraction

6c3150e

renaming comparator to give more context to its usage

51ffe26

inlining single use extension

4449803

bmarty approved these changes May 17, 2022

View reviewed changes

ouchadam added 2 commits May 17, 2022 10:31

renaming comparator to only specify the matrix.org domain

1ec99ee

making comparator override non null as kotlin guards against this

5f2cb67

ouchadam mentioned this pull request May 18, 2022

FTUE - Stage selection and Registration action testing #6091

Merged

6 tasks

bmarty approved these changes May 20, 2022

View reviewed changes

ouchadam requested review from a team and ganfra and removed request for yostyle and a team May 20, 2022 14:31

ganfra approved these changes May 23, 2022

View reviewed changes

ouchadam merged commit a59b8bf into develop May 23, 2022

ouchadam deleted the feature/adm/matrix-org-ordering branch May 23, 2022 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FTUE - Only override sign up steps for `matrix.org` #6065

FTUE - Only override sign up steps for `matrix.org` #6065

ouchadam commented May 16, 2022

github-actions bot commented May 16, 2022 •

edited

Loading

bmarty left a comment

bmarty May 16, 2022

ouchadam May 16, 2022

ouchadam commented May 16, 2022

bmarty left a comment

bmarty May 17, 2022

bmarty May 17, 2022

ouchadam May 17, 2022

bmarty left a comment

ganfra left a comment

github-actions bot commented May 23, 2022

FTUE - Only override sign up steps for matrix.org #6065

FTUE - Only override sign up steps for matrix.org #6065

Conversation

ouchadam commented May 16, 2022

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

github-actions bot commented May 16, 2022 • edited Loading

Unit Test Results

bmarty left a comment

Choose a reason for hiding this comment

bmarty May 16, 2022

Choose a reason for hiding this comment

ouchadam May 16, 2022

Choose a reason for hiding this comment

ouchadam commented May 16, 2022

bmarty left a comment

Choose a reason for hiding this comment

bmarty May 17, 2022

Choose a reason for hiding this comment

bmarty May 17, 2022

Choose a reason for hiding this comment

ouchadam May 17, 2022

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

ganfra left a comment

Choose a reason for hiding this comment

github-actions bot commented May 23, 2022

Matrix SDK

Integration Tests Results:

FTUE - Only override sign up steps for `matrix.org` #6065

FTUE - Only override sign up steps for `matrix.org` #6065

github-actions bot commented May 16, 2022 •

edited

Loading