-
Notifications
You must be signed in to change notification settings - Fork 675
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 - Sign up analytics #6042
FTUE - Sign up analytics #6042
Conversation
5391402
to
0999306
Compare
48f0303
to
86c9e60
Compare
aea4d5e
to
9878747
Compare
- the viewmodel is now responsible for inferring connectivity errors and providing a retry action
…n up after the user has consented to tracking
62d9f72
to
d002ab6
Compare
@@ -113,9 +121,31 @@ class HomeActivityViewModel @AssistedInject constructor( | |||
} | |||
} | |||
.launchIn(viewModelScope) | |||
|
|||
initialState.authenticationDescription?.let { recentAuthentication -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the view model handles the account sign up tracking (and potentially sign in in the future), if a authenticationDescription
has been passed to the HomeActivity
@@ -202,11 +203,11 @@ class LoginFragment @Inject constructor() : AbstractSSOLoginFragment<FragmentLog | |||
views.loginSocialLoginContainer.isVisible = true | |||
views.loginSocialLoginButtons.ssoIdentityProviders = state.loginMode.ssoIdentityProviders?.sorted() | |||
views.loginSocialLoginButtons.listener = object : SocialLoginButtonsView.InteractionListener { | |||
override fun onProviderSelected(id: String?) { | |||
override fun onProviderSelected(provider: SsoIdentityProvider?) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to know the SsoIdentityProvider
in order to provide a tracking value (only used by the FTUE classes)
@@ -745,8 +754,12 @@ class OnboardingViewModel @AssistedInject constructor( | |||
return loginConfig?.homeServerUrl | |||
} | |||
|
|||
fun getSsoUrl(redirectUrl: String, deviceId: String?, providerId: String?): String? { | |||
return authenticationService.getSsoUrl(redirectUrl, deviceId, providerId) | |||
fun getSsoUrl(redirectUrl: String, deviceId: String?, provider: SsoIdentityProvider?): String? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure about adding a side effect to store the current SSO Provider
here but the activity results and web redirects lose the information about the currently selected SSO Provider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly I think changing the name fetchSsoUrl
would justify the side effect, since fetch communicates that it's more than just a simple getter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed! db3cb42
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few feedback.
@@ -113,9 +121,31 @@ class HomeActivityViewModel @AssistedInject constructor( | |||
} | |||
} | |||
.launchIn(viewModelScope) | |||
|
|||
initialState.authenticationDescription?.let { recentAuthentication -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe remove this line and add null -> Unit
case in the when
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -395,7 +425,7 @@ class HomeActivityViewModel @AssistedInject constructor( | |||
HomeActivityViewActions.PushPromptHasBeenReviewed -> { | |||
vectorPreferences.setDidAskUserToEnableSessionPush() | |||
} | |||
HomeActivityViewActions.ViewStarted -> { | |||
is HomeActivityViewActions.ViewStarted -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not necessary I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch! 6fcbd24 (originally the authentication description was held in the ViewStarted
but develop
moved the state to the initialState
)
this, | ||
accountCreation = loginViewState.signMode == SignMode.SignUp | ||
) | ||
val intent = HomeActivity.newIntent(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure to understand, why don't we pass a value for authenticationDescription
param here?
Applicable to all HomeActivity.newIntent
invocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's because the sign up tracking has only been implemented for the FTUE flow, the previous accountCreation
was unused , although now I can see that develop has changed make use of the flag for secure backup/cross signing
will look to reintroduce
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
AuthenticationDescription.AuthenticationType.Github -> Signup.AuthenticationType.GitHub | ||
AuthenticationDescription.AuthenticationType.Gitlab -> Signup.AuthenticationType.GitLab |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super optional nitpick
AuthenticationDescription.AuthenticationType.Github -> Signup.AuthenticationType.GitHub | |
AuthenticationDescription.AuthenticationType.Gitlab -> Signup.AuthenticationType.GitLab | |
AuthenticationDescription.AuthenticationType.GitHub -> Signup.AuthenticationType.GitHub | |
AuthenticationDescription.AuthenticationType.GitLab -> Signup.AuthenticationType.GitLab |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -745,8 +754,12 @@ class OnboardingViewModel @AssistedInject constructor( | |||
return loginConfig?.homeServerUrl | |||
} | |||
|
|||
fun getSsoUrl(redirectUrl: String, deviceId: String?, providerId: String?): String? { | |||
return authenticationService.getSsoUrl(redirectUrl, deviceId, providerId) | |||
fun getSsoUrl(redirectUrl: String, deviceId: String?, provider: SsoIdentityProvider?): String? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly I think changing the name fetchSsoUrl
would justify the side effect, since fetch communicates that it's more than just a simple getter
…tains any data - it's part of the initialstate instead
…uth description from the sign mode - the type is set to other as the legacy viewmodel doesn't support tracking the sso provider
Matrix SDKIntegration Tests Results:
|
Type of change
Content
Fixes #5285
Adds
SignUp
event tracking after the user creates an account AND consents to tracking.AuthenticationDescription
abstraction and also sets the ground work for tracking sign in in the future.AuthenticationDescription
through to theHomeActivity
in order to be trackedMotivation and context
To enable the collection of sign up metrics (and sign up type)
Screenshots / GIFs
No UI changes
Tests
Tested devices