Migrate notification settings events to Event Horizon#5103
Open
MiSikora wants to merge 2 commits intomehow/task/event-horizon-settings-4from
Open
Migrate notification settings events to Event Horizon#5103MiSikora wants to merge 2 commits intomehow/task/event-horizon-settings-4from
MiSikora wants to merge 2 commits intomehow/task/event-horizon-settings-4from
Conversation
MiSikora
requested review from
Copilot and
sztomek
and removed request for
a team
Collaborator
Generated by 🚫 Danger |
Contributor
There was a problem hiding this comment.
Pull request overview
Migrates notification-related analytics from the legacy AnalyticsTracker/AnalyticsEvent system to the Event Horizon tracking API, aligning notification onboarding + settings events with the newer analytics pipeline.
Changes:
- Replaced
AnalyticsTrackercalls withEventHorizon.track(...)in notification permission prompt + notification settings flows. - Updated notification preference enums to carry Event Horizon value types (e.g., play-over and vibration types).
- Updated/added unit test wiring to use
EventHorizon(TestEventSink())and bumped theeventhorizondependency revision.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/support/Support.kt | Updates support output to print the new Event Horizon-backed play-over-notification value. |
| modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/notification/NotificationOpenReceiverActivity.kt | Migrates notification_opened tracking to NotificationOpenedEvent. |
| modules/services/preferences/src/main/java/au/com/shiftyjelly/pocketcasts/preferences/model/PlayOverNotificationSetting.kt | Replaces analyticsString with strongly-typed PlayOverNotificationType. |
| modules/services/preferences/src/main/java/au/com/shiftyjelly/pocketcasts/preferences/model/NotificationVibrateSetting.kt | Replaces analyticsString with strongly-typed NotificationVibrationType. |
| modules/features/settings/src/test/java/au/com/shiftyjelly/pocketcasts/settings/settings/NotificationsSettingsViewModelTest.kt | Switches analytics assertions to Event Horizon via TestEventSink. |
| modules/features/settings/src/main/java/au/com/shiftyjelly/pocketcasts/settings/notifications/NotificationsSettingsViewModel.kt | Migrates all notification settings events to Event Horizon event types. |
| modules/features/podcasts/src/test/java/au/com/shiftyjelly/pocketcasts/podcasts/viewmodel/notifications/EnableNotificationsPromptViewModelTest.kt | Removes legacy analytics dependency and uses Event Horizon in VM construction. |
| modules/features/podcasts/src/main/java/au/com/shiftyjelly/pocketcasts/podcasts/viewmodel/notifications/EnableNotificationsPromptViewModel.kt | Migrates notification permission/opt-in events to Event Horizon. |
| gradle/libs.versions.toml | Updates Event Horizon dependency revision. |
...au/com/shiftyjelly/pocketcasts/repositories/notification/NotificationOpenReceiverActivity.kt
Outdated
Show resolved
Hide resolved
Comment on lines
83
to
+92
| @Test | ||
| fun `GIVEN vm created WHEN onShow called THEN tracker invoked`() = runTest { | ||
| val viewModel = createViewModel() | ||
|
|
||
| viewModel.onShown() | ||
|
|
||
| verify(analytics).track(AnalyticsEvent.SETTINGS_NOTIFICATIONS_SHOWN) | ||
| assertEquals( | ||
| SettingsNotificationsShownEvent, | ||
| eventSink.pollEvent(), | ||
| ) |
There was a problem hiding this comment.
This test name still refers to a “tracker”, but the assertion now verifies an Event Horizon event. Renaming the test (and/or its description) to mention Event Horizon would keep the intent clear and avoid confusion when reading failures.
Comment on lines
65
to
103
| internal fun onPreferenceChanged(preference: NotificationPreferenceType) { | ||
| viewModelScope.launch { | ||
| when (preference) { | ||
| is NotificationPreferenceType.NotifyMeOnNewEpisodes -> { | ||
| preferenceRepository.setPreference(preference) | ||
| analyticsTracker.track( | ||
| AnalyticsEvent.SETTINGS_NOTIFICATIONS_NEW_EPISODES_TOGGLED, | ||
| mapOf("enabled" to preference.isEnabled), | ||
| eventHorizon.track( | ||
| SettingsNotificationsNewEpisodesToggledEvent( | ||
| enabled = preference.isEnabled, | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| is NotificationPreferenceType.HidePlaybackNotificationOnPause -> { | ||
| preferenceRepository.setPreference(preference) | ||
| analyticsTracker.track( | ||
| AnalyticsEvent.SETTINGS_NOTIFICATIONS_HIDE_PLAYBACK_NOTIFICATION_ON_PAUSE, | ||
| mapOf("enabled" to preference.isEnabled), | ||
| eventHorizon.track( | ||
| SettingsNotificationsHidePlaybackNotificationOnPauseEvent( | ||
| enabled = preference.isEnabled, | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| is NotificationPreferenceType.PlayOverNotifications -> { | ||
| preferenceRepository.setPreference(preference) | ||
| analyticsTracker.track( | ||
| AnalyticsEvent.SETTINGS_NOTIFICATIONS_PLAY_OVER_NOTIFICATIONS_TOGGLED, | ||
| mapOf( | ||
| "enabled" to (preference.value != PlayOverNotificationSetting.NEVER), | ||
| "value" to preference.value.analyticsString, | ||
| eventHorizon.track( | ||
| SettingsNotificationsPlayOverNotificationsToggledEvent( | ||
| enabled = preference.value != PlayOverNotificationSetting.NEVER, | ||
| value = preference.value.eventHorizonValue, | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| is NotificationPreferenceType.NotificationVibration -> { | ||
| preferenceRepository.setPreference(preference) | ||
| analyticsTracker.track( | ||
| AnalyticsEvent.SETTINGS_NOTIFICATIONS_VIBRATION_CHANGED, | ||
| mapOf("value" to preference.value.analyticsString), | ||
| eventHorizon.track( | ||
| SettingsNotificationsVibrationChangedEvent( | ||
| value = preference.value.eventHorizonValue, | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Event Horizon tracking for preference changes is now parameterized (e.g., enabled, value, action booleans), but the current unit tests only assert the onShown event. Adding tests that drive onPreferenceChanged(...) for a few representative preferences and assert the emitted events/fields would help catch accidental schema mismatches during this migration.
Comment on lines
47
to
102
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fun handleCtaClick() { | ||
| analyticsTracker.track(AnalyticsEvent.NOTIFICATIONS_PERMISSIONS_ALLOW_TAPPED) | ||
| eventHorizon.track(NotificationsPermissionsAllowTappedEvent) | ||
| when (val state = stateFlow.value) { | ||
| is UiState.PreNewOnboarding -> { | ||
| viewModelScope.launch { | ||
| _messagesFlow.emit(UiMessage.RequestPermission) | ||
| } | ||
| } | ||
|
|
||
| is UiState.NewOnboarding -> { | ||
| eventHorizon.track( | ||
| NewsletterOptInChangedEvent( | ||
| source = NewsletterSource.WelcomeNewAccount, | ||
| enabled = state.subscribedToNewsletter, | ||
| ), | ||
| ) | ||
| settings.marketingOptIn.set(state.subscribedToNewsletter, updateModifiedAt = true) | ||
| viewModelScope.launch { | ||
| _messagesFlow.emit( | ||
| if (state.notificationsEnabled) { | ||
| UiMessage.RequestPermission | ||
| } else { | ||
| UiMessage.Dismiss | ||
| }, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fun reportShown() { | ||
| analyticsTracker.track(AnalyticsEvent.NOTIFICATIONS_PERMISSIONS_SHOWN) | ||
| eventHorizon.track(NotificationsPermissionsShownEvent) | ||
| } | ||
|
|
||
| fun reportNotificationsOptInShown() { | ||
| analyticsTracker.track(AnalyticsEvent.NOTIFICATIONS_OPT_IN_SHOWN) | ||
| eventHorizon.track(NotificationsOptInShownEvent) | ||
| } | ||
|
|
||
| fun reportNotificationRequestResult(wasEnabled: Boolean) { | ||
| analyticsTracker.track(if (wasEnabled) AnalyticsEvent.NOTIFICATIONS_OPT_IN_ALLOWED else AnalyticsEvent.NOTIFICATIONS_OPT_IN_DENIED) | ||
| val event = if (wasEnabled) { | ||
| NotificationsOptInAllowedEvent | ||
| } else { | ||
| NotificationsOptInDeniedEvent | ||
| } | ||
| eventHorizon.track(event) | ||
| } | ||
|
|
||
| fun handleDismissedByUser() { | ||
| settings.notificationsPromptAcknowledged.set(value = true, updateModifiedAt = true) | ||
| analyticsTracker.track(AnalyticsEvent.NOTIFICATIONS_PERMISSIONS_DISMISSED) | ||
| eventHorizon.track(NotificationsPermissionsNotNowTappedEvent) | ||
| } |
There was a problem hiding this comment.
This migration switches several user actions from AnalyticsTracker to Event Horizon events (shown/allow/not-now/opt-in allowed/denied). There aren’t unit tests asserting that the correct Event Horizon events are emitted for these actions; adding small tests using TestEventSink for handleCtaClick(), reportShown(), reportNotificationsOptInShown(), and reportNotificationRequestResult(...) would reduce the risk of silent analytics regressions.
Collaborator
Project dependencies changeslist! Upgraded Dependencies
com.automattic:eventhorizon:pocket-casts-7ed2344c715d2a021b2452175ed89ef98b710746, (changed from pocket-casts-bc34f8fc2d09a6d6aa27b862f182a70ef453e7b2)tree +--- project :modules:features:account
| \--- project :modules:features:search
| \--- project :modules:services:analytics
-| +--- com.automattic:eventhorizon:pocket-casts-bc34f8fc2d09a6d6aa27b862f182a70ef453e7b2
+| +--- com.automattic:eventhorizon:pocket-casts-7ed2344c715d2a021b2452175ed89ef98b710746
| +--- project :modules:services:model
-| | +--- com.automattic:eventhorizon:pocket-casts-bc34f8fc2d09a6d6aa27b862f182a70ef453e7b2 (*)
+| | +--- com.automattic:eventhorizon:pocket-casts-7ed2344c715d2a021b2452175ed89ef98b710746 (*)
| | \--- project :modules:services:utils
| | \--- project :modules:services:payment
-| | \--- com.automattic:eventhorizon:pocket-casts-bc34f8fc2d09a6d6aa27b862f182a70ef453e7b2 (*)
+| | \--- com.automattic:eventhorizon:pocket-casts-7ed2344c715d2a021b2452175ed89ef98b710746 (*)
| \--- project :modules:services:preferences
-| \--- com.automattic:eventhorizon:pocket-casts-bc34f8fc2d09a6d6aa27b862f182a70ef453e7b2 (*)
+| \--- com.automattic:eventhorizon:pocket-casts-7ed2344c715d2a021b2452175ed89ef98b710746 (*)
\--- project :modules:features:discover
\--- project :modules:features:podcasts
\--- project :modules:features:player
\--- project :modules:features:transcripts
\--- project :modules:services:sharing
- \--- com.automattic:eventhorizon:pocket-casts-bc34f8fc2d09a6d6aa27b862f182a70ef453e7b2 (*)
+ \--- com.automattic:eventhorizon:pocket-casts-7ed2344c715d2a021b2452175ed89ef98b710746 (*) |
sztomek
approved these changes
Mar 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This migrates notification settings related events to Event Horizon.
Relates to PCDROID-419
Testing Instructions
I would mostly focus on the code review and smoke test some of these events as the migration process is rather a mechanical one.
notifications_opt_in_shownnotifications_opt_in_allowednotifications_opt_in_deniednotifications_permissions_shownnotifications_permissions_allow_tappednotifications_permissions_not_now_tappednotification_openedsettings_notifications_shownsettings_notifications_new_episodes_toggledsettings_notifications_podcasts_changedsettings_notifications_actions_changedsettings_notifications_advanced_settings_tappedsettings_notifications_play_over_notifications_toggledsettings_notifications_hide_playback_notification_on_pausesettings_notifications_sound_changedsettings_notifications_vibration_changedsettings_notifications_daily_reminders_togglesettings_notifications_trending_togglesettings_daily_reminders_advanced_settings_tappedsettings_trending_advanced_settings_tappedsettings_notifications_new_features_togglesettings_notifications_new_features_advanced_settings_tappedsettings_notifications_offers_togglesettings_notifications_offers_advanced_settings_tappednotifications_permissions_open_system_settingsScreenshots or Screencast
N/A
Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)modules/services/localization/src/main/res/values/strings.xmlI have tested any UI changes...