-
Notifications
You must be signed in to change notification settings - Fork 739
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
Storing and tracking the onboarding messaging use case #5009
Conversation
marked as draft whilst confirming if the identity payload is coming through |
Matrix SDKIntegration Tests Results:
|
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, some remarks
} | ||
} | ||
|
||
fun FtueUseCase.toTrackingValue(): Identity.FtueUseCaseSelection { |
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 put the extensions about analytics in the file https://github.com/vector-im/element-android/blob/a8c29f55f5a65cbf404dfe710cd9a6a3e976b6da/vector/src/main/java/im/vector/app/features/analytics/extensions/JoinedRoomExt.kt#L24, but not merge, it's part of #4734.
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.
will do the same (in a separate FtueUseCaseExt.kt
file) 👍
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.
import kotlinx.coroutines.flow.first | ||
import javax.inject.Inject | ||
|
||
private val Context.dataStore: DataStore<Preferences> by preferencesDataStore(name = "vector_onboarding") |
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 wonder how we will manage mutli accounts, if we manage it one day.
We should probably have scoped pref: global / per session.
Just thinking out loud.
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.
that's a good point, delaying the storing until after sign up makes sense to me, then we can associate to a session/account
I'm happy to try converting this to a realm entity in the global database (by user)
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.
assuming we're happy to avoid using a database for the time being~ I've updated the onboarding store to be by user id efbdd70
/** | ||
* Update user specific properties | ||
*/ | ||
fun updateUserProperties(identity: Identity) |
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.
@bmarty I'll also move this to the AnalyticsTracker
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.
0222a33
to
efbdd70
Compare
class OnboardingStore @Inject constructor( | ||
private val context: Context | ||
) { | ||
suspend fun readUseCase(userId: String) = context.dataStore.data.first().let { preferences -> |
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.
Q: is it possible to have a OnBoardingStore scoped to the session to avoid passing the userId
to each of its fun?
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 could do it for the MainActivity
injection site (for cleaning up the local storage) but not the OnboardingViewModel
as the session doesn't exist yet
alternatively we could have a provider/factory as a session extension instead of injecting the dependency, what do you think?
Session.onboardingStore(context: Context) = OnboardingStore(context, myUserId)
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 looks neat!
ff109c0
to
bcb065a
Compare
} | ||
|
||
private fun String.toUseCaseKey() = stringPreferencesKey("$this-use_case") | ||
} |
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 class could be simplified with:
class OnboardingStore(
private val context: Context,
myUserId: String
) {
private val useCasePreferences = stringPreferencesKey("$myUserId-use_case")
suspend fun readUseCase() = context.dataStore.data.first().let { preferences ->
preferences[useCasePreferences]?.let { FtueUseCase.from(it) }
}
suspend fun setUseCase(useCase: FtueUseCase) {
context.dataStore.edit { settings ->
settings[useCasePreferences] = useCase.persistableValue
}
}
suspend fun resetUseCase() {
context.dataStore.edit { settings ->
settings.remove(useCasePreferences)
}
}
suspend fun clear() {
resetUseCase()
}
}
and removeKeysWithPrefix
is not necessary since we know have a new per userId store object.
I am also wondering if we could make it more generic, so its goal will be to store any data app side related to the session. I have in mind the storage of a client secret for matrix-org/sygnal#70.
The class name could be something like SessionDataStore
(open for better ideas!)
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.
sounds good to me 👍
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.
went with VectorSessionStore
64c18ac
42f5ba8
to
64c18ac
Compare
…or the smaller tracking subset
- this ensures we have a unique session or account id to store the selection against in case we support multi account in the future
- will enable multi account support in the future
- avoids needing to know about the user id for each read/write
64c18ac
to
bc37391
Compare
) { | ||
|
||
private val Context.dataStore: DataStore<Preferences> by preferencesDataStore(name = "vector_session_store_$myUserId") |
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 am wondering if directly using the userId
may lead to some trouble: special chars - low risk, length - higher risk. Maybe using a hash of it would be better.
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.
very good point, I keep forgetting that user-id is actually a matrix id rather than a uuid, will change
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.
Yes. "Matrix Id" is not really a specific concept, but it is sometimes used for user ids. https://spec.matrix.org/v1.1/appendices/#identifier-grammar is an interesting reading on the subject
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, thanks for all the updates!
Part of #4585
Adds storing and tracking of the selected messaging use case as part of the onboarding journey (this will eventually be used to better tailor the first time experience)
Adds
VectorAnalytics.updateUserProperties
to allow for updating the identity/user properties outside of the analytics id/consent flow, eg locally capturing the onboarding selection and only sending after the user has consented.Posthog.Identify()
accumulates a local cache which is only sent once a user has consentedPosthog.OptOut(false)