Skip to content
This repository was archived by the owner on Aug 21, 2025. It is now read-only.

Bind privacy settings and userInfo with the TracksTracker#118

Merged
AdamGrzybkowski merged 6 commits intotrunkfrom
adam/GRA-762
Aug 21, 2025
Merged

Bind privacy settings and userInfo with the TracksTracker#118
AdamGrzybkowski merged 6 commits intotrunkfrom
adam/GRA-762

Conversation

@AdamGrzybkowski
Copy link
Contributor

Description

This PR binds the PrivacySettings and the userId with the TracksTracker.

To not create a dependency on :userComponent in the :analytics module, the :analytics module exposes a public interface TrackerSetupDataProvider that is implemented and provided in the :app module.

TracksTracker uses that provider to reactively update the state.

Testing Steps

  1. Enable tracking
  2. Go through the app
  3. Confirm the events are sent (in the Tracks UI or via network requests)
  4. Disable the tracking
  5. Go through the app
  6. Confirm the events aren't sent (in the Tracks UI or via network requests)

@AdamGrzybkowski AdamGrzybkowski added the enhancement New feature or request label Aug 20, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR integrates privacy settings and user information with the TracksTracker by implementing a reactive data provider pattern. It introduces a TrackerSetupDataProvider interface in the analytics module that is implemented by AppTrackerSetupDataProvider in the app module to avoid direct dependencies.

  • Removes direct userId setting from Tracker abstract class and implements reactive privacy/user state binding
  • Creates TrackerSetupDataProvider interface and implementation to provide tracking state and user ID
  • Adds comprehensive test coverage for the new data provider functionality

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
analytics/src/main/kotlin/com/gravatar/analytics/TrackerSetupDataProvider.kt Defines interface for providing tracker setup data
analytics/src/main/kotlin/com/gravatar/analytics/TrackerSetupData.kt Data models for tracking state and user information
analytics/src/main/kotlin/com/gravatar/analytics/Tracker.kt Removes abstract userId property
analytics/src/main/kotlin/com/gravatar/analytics/tracks/TracksTracker.kt Implements reactive binding to privacy settings and user data
app/src/main/java/com/gravatar/app/AppTrackerSetupDataProvider.kt Concrete implementation combining privacy settings and user repository
app/src/main/java/com/gravatar/app/di/AppModule.kt Dependency injection setup for the data provider
app/src/test/kotlin/com/gravatar/app/AppTrackerSetupDataProviderTest.kt Comprehensive test coverage for the data provider
app/build.gradle.kts Adds test dependencies
analytics/build.gradle.kts Adds coroutines dependency
app/src/main/java/com/gravatar/app/analytics/AppEvent.kt Removes unused event class

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

avatarAltText = "John Doe's Gravatar"
pronouns = "he/him"
pronunciation = "John Doe"
verifiedAccounts = emptyList()
Copy link

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

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

The verifiedAccounts property is set twice in this profile builder (lines 139 and 145). Remove the duplicate assignment.

Suggested change
verifiedAccounts = emptyList()

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,11 @@
package com.gravatar.analytics

data class TrackerSetupData(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about the name. Any ideas?

Choose a reason for hiding this comment

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

What about TrackingContext or TrackerConfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm not sure - do you have a strong preference for any of those?

Choose a reason for hiding this comment

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

I like TrackingContext, but I don't have a strong opinion. Feel free to keep TrackerSetupData if you want.

Copy link

@hamorillo hamorillo left a comment

Choose a reason for hiding this comment

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

🚀 Looks great!

}

@Test
fun `emits ENABLED when analytics enabled with non-null user id`() = runTest {

Choose a reason for hiding this comment

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

⛏️ Nitpicking: I think the test name can lead to some confusion, as it looks like analytics will be only enabled with a non-null user ID when there is no relation between them.

@@ -0,0 +1,11 @@
package com.gravatar.analytics

data class TrackerSetupData(

Choose a reason for hiding this comment

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

What about TrackingContext or TrackerConfig?

private val userRepository: UserRepository,
) : TrackerSetupDataProvider {
override fun getTrackerSetupData(): Flow<TrackerSetupData> {
val userIdFlow: Flow<String?> = userRepository.getProfile().map { it?.userId?.toString() }.distinctUntilChanged()

Choose a reason for hiding this comment

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

🛑 We need to use the WPCOM userName for tracks instead of the userId.

Suggested change
val userIdFlow: Flow<String?> = userRepository.getProfile().map { it?.userId?.toString() }.distinctUntilChanged()
val userIdFlow: Flow<String?> = userRepository.getProfile().map { it?.userLogin }.distinctUntilChanged()

That suggestion is not enough, as we need to persist it in the DB.

@AdamGrzybkowski AdamGrzybkowski merged commit 642ce32 into trunk Aug 21, 2025
10 checks passed
@AdamGrzybkowski AdamGrzybkowski deleted the adam/GRA-762 branch August 21, 2025 08:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants