Skip to content
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

Add support for UserProperties to analytics and capture FTUE use case selection. #5591

Merged
merged 3 commits into from
Feb 18, 2022

Conversation

pixlwave
Copy link
Member

@pixlwave pixlwave commented Feb 15, 2022

Additionally update taps to interactions following updates in the analytics event repo.

Post review steps

  • Update release/swift in the Analytics repo
  • Run pod update AnalyticsEvents

Fixes #5590.

… selection.

Additionally update taps to interactions following updates in the analytics event repo.
Copy link
Contributor

@Anderas Anderas left a comment

Choose a reason for hiding this comment

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

A few questions for my better understanding of the way we use analytics

Riot/Modules/Analytics/Analytics.swift Outdated Show resolved Hide resolved
/// - Parameter session: The session to gather any user properties from.
/// - Returns: The properties to be set.
private func makeUserProperties(for session: MXSession) -> AnalyticsEvent.UserProperties {
var useCaseSelection: AnalyticsEvent.UserProperties.FtueUseCaseSelection?
Copy link
Contributor

Choose a reason for hiding this comment

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

It took me a while to understand what useCase means in this context. It seems we use it both for architectural components (e.g. OnboardingUseCase) as well as a specific feature (i.e. use case = reason to use element). Is this correct?

Copy link
Member Author

@pixlwave pixlwave Feb 17, 2022

Choose a reason for hiding this comment

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

Ohhhh, thanks for pointing this out. I meant to rename OnboardingUseCase to OnboardingUseCaseScreen/OnboardingUseCasePicker/OnboardingUseCaseView after I created it. That's the screen where the useCase is selected.

It's annoying that the analytics schema has this as UseCaseSelection and not just UseCase. Do you think it would be clearer rename all occurrences of useCase to useCaseSelection for consistency?

Copy link
Contributor

@Anderas Anderas Feb 18, 2022

Choose a reason for hiding this comment

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

I think my confusion was broader, as the term "use case" itself (even as "use case selection"), refers to something quite generic wherease the FTUE means a more specific meaning, something like "what is your reason to use element". So to push it to an extreme, this would be FTUEUseCaseUseCase 😅 . Obviously I'm not suggesting we use that name, I was just trying to understand that distinction.

Copy link
Contributor

Choose a reason for hiding this comment

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

For instance looking at this type UserSessionProperties.UseCase on its own, I would not know what it refers to without a wider context

Copy link
Member Author

Choose a reason for hiding this comment

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

Understood, I'll have a think about this (Definition 2 in that link seems entirely new to me), but will go ahead and merge this PR 👍

Riot/Modules/Analytics/PostHogAnalyticsClient.swift Outdated Show resolved Hide resolved
@pixlwave pixlwave merged commit 4a0a67f into develop Feb 18, 2022
@pixlwave pixlwave deleted the doug/5590_ftue_analytics branch February 18, 2022 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for UserProperties analytics
2 participants