[feat] change_language 이벤트 설계 (앱 내 언어 설정이 바뀐 경우)#528
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements analytics tracking for language changes by introducing a ChangeLanguageEvent and integrating it into the LanguageSelectorScreen. It also adds a unit test to verify the event payload. The review feedback suggests optimizing the language selection logic to prevent redundant operations when the same language is selected and recommends renaming event properties to follow Kotlin naming conventions and maintain consistency with existing analytics events.
| onLanguageSelected = { language -> | ||
| if (selectedLanguage != language) { | ||
| analyticsTracker.track( | ||
| ChangeLanguageEvent( | ||
| lang_from = selectedLanguage.code, | ||
| lang_to = language.code, | ||
| ), | ||
| ) | ||
| } | ||
| viewModel.selectLanguage(language) | ||
| }, |
There was a problem hiding this comment.
To avoid redundant network calls and unnecessary Activity recreations (triggered by AppCompatDelegate.setApplicationLocales), the call to viewModel.selectLanguage(language) should be moved inside the if (selectedLanguage != language) block. This ensures that processing only occurs when a different language is actually selected.
| onLanguageSelected = { language -> | |
| if (selectedLanguage != language) { | |
| analyticsTracker.track( | |
| ChangeLanguageEvent( | |
| lang_from = selectedLanguage.code, | |
| lang_to = language.code, | |
| ), | |
| ) | |
| } | |
| viewModel.selectLanguage(language) | |
| }, | |
| onLanguageSelected = { language -> | |
| if (selectedLanguage != language) { | |
| analyticsTracker.track( | |
| ChangeLanguageEvent( | |
| languageBefore = selectedLanguage.code, | |
| languageAfter = language.code, | |
| ), | |
| ) | |
| viewModel.selectLanguage(language) | |
| } | |
| }, |
| data class ChangeLanguageEvent( | ||
| val lang_from: String, | ||
| val lang_to: String, | ||
| ) : AppAnalyticsEvent { | ||
| override val eventName = "change_language" | ||
| override val properties = buildMap { | ||
| put("lang_from", lang_from) | ||
| put("lang_to", lang_to) | ||
| } | ||
| } |
There was a problem hiding this comment.
Kotlin property names should follow the camelCase convention (e.g., languageBefore instead of lang_from). Additionally, for consistency with other transition-based events in this repository (like WidgetAnalyticsEvent.Changed), it is recommended to use language_before and language_after as the property keys in the analytics payload.
| data class ChangeLanguageEvent( | |
| val lang_from: String, | |
| val lang_to: String, | |
| ) : AppAnalyticsEvent { | |
| override val eventName = "change_language" | |
| override val properties = buildMap { | |
| put("lang_from", lang_from) | |
| put("lang_to", lang_to) | |
| } | |
| } | |
| data class ChangeLanguageEvent( | |
| val languageBefore: String, | |
| val languageAfter: String, | |
| ) : AppAnalyticsEvent { | |
| override val eventName = "change_language" | |
| override val properties = buildMap { | |
| put("language_before", languageBefore) | |
| put("language_after", languageAfter) | |
| } | |
| } |
| fun `change language payload keeps previous and next locale codes`() { | ||
| val payload = ChangeLanguageEvent( | ||
| lang_from = "ko", | ||
| lang_to = "en", | ||
| ).toPayload() | ||
|
|
||
| assertEquals("change_language", payload.eventName) | ||
| assertEquals( | ||
| mapOf( | ||
| "lang_from" to "ko", | ||
| "lang_to" to "en", | ||
| ), | ||
| payload.properties, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Updating the test case to reflect the suggested naming improvements in ChangeLanguageEvent for consistency and adherence to Kotlin naming conventions.
| fun `change language payload keeps previous and next locale codes`() { | |
| val payload = ChangeLanguageEvent( | |
| lang_from = "ko", | |
| lang_to = "en", | |
| ).toPayload() | |
| assertEquals("change_language", payload.eventName) | |
| assertEquals( | |
| mapOf( | |
| "lang_from" to "ko", | |
| "lang_to" to "en", | |
| ), | |
| payload.properties, | |
| ) | |
| } | |
| @Test | |
| fun `change language payload keeps previous and next locale codes`() { | |
| val payload = ChangeLanguageEvent( | |
| languageBefore = "ko", | |
| languageAfter = "en", | |
| ).toPayload() | |
| assertEquals("change_language", payload.eventName) | |
| assertEquals( | |
| mapOf( | |
| "language_before" to "ko", | |
| "language_after" to "en", | |
| ), | |
| payload.properties, | |
| ) | |
| } |
Summary
PostHog 마지막 요구사항인 change_langauge 이벤트 설계를 완료했습니다.
Describe your changes
Issue
To reviewers