Skip to content

Conversation

@ouchadam
Copy link
Contributor

@ouchadam ouchadam commented May 12, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

  • Adds missing deeplink handling to the new FTUE combined login/registration screens
  • Fixes offline errors when attempting to sign in being treated as an invalid homeserver selection
    • Introduces a helper for inferring if the device has an active connection, if not we can further breakdown unknown host errors and infer that the failure is caused by being offline rather than the homeserver being unavailable
  • Lifts the invalid homeserver error dialog to the Activity(/FtueVariant) with a retry action passed from the ViewModel

Motivation and context

Fixes #6023

Screenshots / GIFs

Legacy flows

WITH FTUE DISABLED GET STARTED WITH FTUE DISABLED LEGACY RESETTING DEEPLINK
after-deeplink-legacy-sign-in after-deeplink-legacy-get-started after-deeplink-legacy-error

FTUE flows

SIGN UP SIGN IN
after-deeplink-sign-up-error after-deeplink-new-sign-in-error
SIGN UP SIGN IN
deeplink-reg-disabled after-sign-in-valid

Offline deeplink

BEFORE AFTER
Screenshot_20220512_144406 after-offline-sign-in

Tests

Tested devices

  • Physical
  • Emulator
  • OS version(s): 28

@ouchadam ouchadam added the Z-FTUE Issue is relevant to the first time use project or experience label May 12, 2022

@Provides
@Singleton
fun providesBuildMeta() = BuildMeta()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introduces a build meta abstraction to allow for providing Build version overrides for unit tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome idea!

}
}

private fun LoginConfig?.toHomeserverConfig(): HomeServerConnectionConfig? {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no longer needed as the SelectHomeserver will read from the deeplink or default to matrix.org

onHomeServerSelected(config, serverTypeOverride, authResult)
}
is OnboardingAction.HomeServerChange.EditHomeServer -> {
when (awaitState().onboardingFlow) {
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 when clauses of when (trigger) have been extracted to their own functions

}
OnboardingFlow.SignIn -> {
updateServerSelection(config, serverTypeOverride, authResult)
_viewEvents.post(OnboardingViewEvents.OnLoginFlowRetrieved)
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 case was a legacy flow which has been removed by providing stronger typing to onAuthenticationStartedSuccess( trigger: OnboardingAction.HomeServerChange)instead of the baseOnboardingAction`

// Nothing to do
}

override fun onError(throwable: Throwable) {
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 is now handled by the activity(/FtueVariant)

import org.matrix.android.sdk.api.network.ssl.Fingerprint

sealed interface OnboardingAction : VectorViewModelAction {
data class OnGetStarted(val resetLoginConfig: Boolean, val onboardingFlow: OnboardingFlow) : OnboardingAction
Copy link
Contributor Author

Choose a reason for hiding this comment

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

resetLoginConfig has become a dedicated reset action ResetDeeplinkConfig

@github-actions
Copy link

github-actions bot commented May 12, 2022

Unit Test Results

124 files  ±0  124 suites  ±0   2m 17s ⏱️ ±0s
221 tests +1  221 ✔️ +1  0 💤 ±0  0 ±0 
730 runs  +4  730 ✔️ +4  0 💤 ±0  0 ±0 

Results for commit 096db6c. ± Comparison against base commit 0675b7c.

♻️ This comment has been updated with latest results.

@ouchadam ouchadam mentioned this pull request May 13, 2022
6 tasks
@ouchadam ouchadam force-pushed the feature/adm/ftue-sign-in branch 2 times, most recently from a0fcef6 to 8c44c98 Compare May 25, 2022 09:30
@ouchadam ouchadam force-pushed the feature/adm/ftue-deeplinks branch from 48f0303 to 86c9e60 Compare May 25, 2022 12:36
Base automatically changed from feature/adm/ftue-sign-in to develop May 25, 2022 16:30
@ouchadam ouchadam marked this pull request as ready for review May 25, 2022 16:30
// Nothing to do
}

override fun onError(throwable: Throwable) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

handled by the FtueVariant -> OnboardingViewEvents observer

@ouchadam ouchadam requested review from a team, ericdecanini and onurays and removed request for a team May 25, 2022 16:34
Copy link
Contributor

@ericdecanini ericdecanini left a comment

Choose a reason for hiding this comment

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

LGTM


@Provides
@Singleton
fun providesBuildMeta() = BuildMeta()
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome idea!

*/
@Suppress("deprecation")
@SuppressLint("NewApi") // false positive
fun Context.inferNoConnectivity(buildMeta: BuildMeta): Boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a description about buildMeta parameter?

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 add some java doc 👍

networkCapabilities?.hasTransport(NetworkCapabilities.TRANSPORT_CELLULAR) == true -> false
networkCapabilities?.hasTransport(NetworkCapabilities.TRANSPORT_WIFI) == true -> false
networkCapabilities?.hasTransport(NetworkCapabilities.TRANSPORT_VPN) == true -> false
else -> true
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a silly question/use case but should we check the internet connection over TRANSPORT_ETHERNET and TRANSPORT_BLUETOOTH?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! I will double check if hasTransport considers bluetooth as an internet provider or simply if a bluetooth connection is present

Copy link
Contributor

@onurays onurays left a comment

Choose a reason for hiding this comment

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

LGTM, just one remark.

@ouchadam ouchadam merged commit 7fdf138 into develop May 31, 2022
@ouchadam ouchadam deleted the feature/adm/ftue-deeplinks branch May 31, 2022 08:30
@ouchadam
Copy link
Contributor Author

ouchadam commented May 31, 2022

ahh sorry @onurays I merged too soon, I will address your comments in another PR

@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=20 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

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.

FTUE - Deeplinks

4 participants