feat: Add Analytics events #258

omerhabib26 · 2024-03-12T20:40:00Z

Description

Add analytics for the following modules
AppReview, Profile, WhatsNew, Course, Discussion, Logistration, MainDashboard, Discovery, Core
Restructure the analytics implementation to module level
Update the event names accordingly
Update the test cases accordingly

fix: LEARNER-9876

k1rill

Thank you for the very accurate job! Just minor nits

app/src/main/java/org/openedx/app/AppViewModel.kt

auth/src/main/java/org/openedx/auth/presentation/logistration/LogistrationViewModel.kt

auth/src/main/java/org/openedx/auth/presentation/restore/RestorePasswordViewModel.kt

auth/src/main/java/org/openedx/auth/presentation/signin/SignInViewModel.kt

profile/src/main/java/org/openedx/profile/presentation/profile/ProfileViewModel.kt

k1rill

LGTM

HamzaIsrar12

Some things to look into:

Discovery Events are implemented for Native experience only.
CalendarSynnc Action Events is triggering for all cases.
Please follow a similar convention in Analytics Screen & Keys enum names.

app/src/main/java/org/openedx/app/AnalyticsManager.kt

app/src/main/java/org/openedx/app/di/AppModule.kt

auth/src/main/java/org/openedx/auth/presentation/AuthAnalytics.kt

auth/src/main/java/org/openedx/auth/presentation/logistration/LogistrationViewModel.kt

course/src/main/java/org/openedx/course/presentation/CourseAnalytics.kt

core/src/main/java/org/openedx/core/module/download/BaseDownloadViewModel.kt

core/src/main/java/org/openedx/core/presentation/CoreAnalytics.kt

course/src/main/java/org/openedx/course/presentation/outline/CourseOutlineViewModel.kt

HamzaIsrar12 · 2024-03-21T04:09:33Z

course/src/main/java/org/openedx/course/presentation/unit/video/BaseVideoViewModel.kt

The currentVideoTime is always returning 0 in evens.

we can create a bug ticket to tackle currentVideoTime as according to current implementation it is hard to trace why its not working

Okay but Please create and attach the bug ticket here. 👍🏻

HamzaIsrar12

Discovery Events are implemented for Native experience only.
WhatsNew:Close event is not implemented

app/src/main/java/org/openedx/app/AnalyticsManager.kt

HamzaIsrar12 · 2024-03-25T08:42:26Z

course/src/main/java/org/openedx/course/presentation/unit/video/BaseVideoViewModel.kt

Okay but Please create and attach the bug ticket here. 👍🏻

core/src/main/java/org/openedx/core/module/download/BaseDownloadViewModel.kt

course/src/main/java/org/openedx/course/presentation/container/CourseContainerViewModel.kt

course/src/main/java/org/openedx/course/presentation/CourseAnalytics.kt

course/src/main/java/org/openedx/course/presentation/dates/CourseDatesViewModel.kt

app/src/main/java/org/openedx/app/MainFragment.kt

course/src/main/java/org/openedx/course/presentation/container/CourseContainerFragment.kt

core/src/main/java/org/openedx/core/presentation/dialog/alert/ActionDialogFragment.kt

- Add analytics for following modules - AppReview, Profile, WhatsNew, Course, Discussion, Logistration, MainDashboard, Discovery - Restructure the analytics implementation to module level - Update the event names accordingly fix: LEARNER-9876

fix: LEARNER-9876

HamzaIsrar12

Thank you, Moin, great work !! 🏎️

volodymyr-chekyrta · 2024-03-28T08:59:22Z

auth/src/main/java/org/openedx/auth/presentation/AuthAnalytics.kt

-    fun userLoginEvent(method: String)
-    fun signUpClickedEvent()
-    fun createAccountClickedEvent(provider: String)
-    fun registrationSuccessEvent(provider: String)
-    fun forgotPasswordClickedEvent()
-    fun resetPasswordClickedEvent(success: Boolean)
    fun setUserIdForSession(userId: Long)
-}
+    fun logEvent(event: String, params: Map<String, Any?>)


What is the purpose of removing these methods?
I see a similar approach in a few other interfaces, but that leads to inconsistency with CourseAnalytics, DashboardAnalytics, DiscoveryAnalytics, DiscussionAnalytics, etc.
We should select one approach and follow it for all modules.
My +1 for keeping methods in interfaces: They help quickly understand what this interface does, same to table of contents in books;
abstract method logEvent cannot provide this information.

What is the purpose of removing these methods?

We're moving methods to ViewModels for better organization and maintenance. Each ViewModel handles its own analytics, making future changes easier without affecting other code. This promotes code reuse and reduces the risk of errors.

I see a similar approach in a few other interfaces, but that leads to inconsistency with CourseAnalytics, DashboardAnalytics, DiscoveryAnalytics, DiscussionAnalytics, etc.

Some methods may still exist from a previous approach, but lack proper event details. These can be addressed in an improvement ticket, ensuring that any remaining methods are properly handled and documented for clarity and future maintenance.

We should select one approach and follow it for all modules. My +1 for keeping methods in interfaces: They help quickly understand what this interface does, same to table of contents in books;
abstract method logEvent cannot provide this information.

In some cases, we use default logEvent instead custom method which can be improved in a separate ticket for better understanding and readability.

If you have any alternative solutions in mind, we're open to considering them as well.

volodymyr-chekyrta · 2024-03-28T09:00:59Z

course/src/test/java/org/openedx/course/presentation/container/CourseContainerViewModelTest.kt

        viewModel.preloadCourseStructure()
        advanceUntilIdle()

        coVerify(exactly = 1) { interactor.preloadCourseStructure(any()) }
+        verify(exactly = 1) { analytics.logEvent(any(), any()) }


Should check specific value, not any

Hey @volodymyr-chekyrta,
As per the current implementation, we are not verifying the method with a specific value.
i.e, coVerify(exactly = 1) { interactor.preloadCourseStructure(any()) }

In the case of interactor.preloadCourseStructure(any()), it's not important what value is passed as an argument, but in the case of analytics.logEvent, we need to check what event we logged.

I have create a new PR for these changes Please have a look

volodymyr-chekyrta · 2024-03-28T09:01:11Z

course/src/test/java/org/openedx/course/presentation/container/CourseContainerViewModelTest.kt

        viewModel.preloadCourseStructure()
        advanceUntilIdle()

        coVerify(exactly = 1) { interactor.preloadCourseStructure(any()) }
+        verify(exactly = 1) { analytics.logEvent(any(), any()) }


Should check specific value, not any

volodymyr-chekyrta · 2024-03-28T09:01:32Z

course/src/test/java/org/openedx/course/presentation/detail/CourseDetailsViewModelTest.kt

        advanceUntilIdle()

        coVerify(exactly = 1) { interactor.enrollInACourse(any()) }
-        verify(exactly = 1) { analytics.courseEnrollClickedEvent(any(), any()) }
-        verify(exactly = 0) { analytics.courseEnrollSuccessEvent(any(), any()) }
+        verify(exactly = 1) { analytics.logEvent(any(), any()) }


Should check specific value, not any

volodymyr-chekyrta · 2024-03-28T09:01:58Z

course/src/test/java/org/openedx/course/presentation/container/CourseContainerViewModelTest.kt

        viewModel.preloadCourseStructure()
        advanceUntilIdle()

        coVerify(exactly = 1) { interactor.preloadCourseStructure(any()) }
+        verify(exactly = 1) { analytics.logEvent(any(), any()) }


Should check specific value, not any

volodymyr-chekyrta · 2024-03-28T09:02:36Z

course/src/test/java/org/openedx/course/presentation/outline/CourseOutlineViewModelTest.kt

            downloadDao,
            workerController
        )

        viewModel.saveDownloadModels("", "")
        advanceUntilIdle()
+        verify(exactly = 1) { coreAnalytics.logEvent(any(), any()) }


Should check specific value, not any

volodymyr-chekyrta · 2024-03-28T09:07:44Z

course/src/test/java/org/openedx/course/presentation/unit/video/VideoUnitViewModelTest.kt

@@ -70,6 +76,7 @@ class VideoUnitViewModelTest {
                any()
            )
        }
+        verify(exactly = 1) { courseAnalytics.logEvent(any(), any()) }


Should check specific value, not any

volodymyr-chekyrta · 2024-03-28T09:11:17Z

course/src/test/java/org/openedx/course/presentation/unit/video/VideoUnitViewModelTest.kt

@@ -96,6 +105,7 @@ class VideoUnitViewModelTest {
                any()
            )
        }
+        verify(exactly = 1) { courseAnalytics.logEvent(any(), any()) }


Should check specific value, not any

volodymyr-chekyrta · 2024-03-28T09:11:31Z

course/src/test/java/org/openedx/course/presentation/unit/video/VideoViewModelTest.kt

@@ -67,18 +72,21 @@ class VideoViewModelTest {
                any()
            )
        }
+        verify(exactly = 1) { courseAnalytics.logEvent(any(), any()) }


Should check specific value, not any

volodymyr-chekyrta · 2024-03-28T09:11:36Z

course/src/test/java/org/openedx/course/presentation/unit/video/VideoViewModelTest.kt

@@ -87,6 +95,7 @@ class VideoViewModelTest {
                any()
            )
        }
+        verify(exactly = 1) { courseAnalytics.logEvent(any(), any()) }


Should check specific value, not any

omerhabib26 requested review from k1rill, HamzaIsrar12, farhan-arshad-dev and volodymyr-chekyrta March 12, 2024 20:40

omerhabib26 changed the title ~~feat: Add Analytics events~~ [WIP]feat: Add Analytics events Mar 13, 2024

omerhabib26 changed the title ~~[WIP]feat: Add Analytics events~~ feat: Add Analytics events Mar 15, 2024

k1rill reviewed Mar 18, 2024

View reviewed changes

omerhabib26 force-pushed the omer/LEARNER-9876 branch from 751a468 to 5ae94f0 Compare March 18, 2024 17:52

k1rill previously approved these changes Mar 18, 2024

View reviewed changes

HamzaIsrar12 requested changes Mar 21, 2024

View reviewed changes

omerhabib26 dismissed k1rill’s stale review via 0dfd4fd March 21, 2024 18:02

omerhabib26 force-pushed the omer/LEARNER-9876 branch from 0dfd4fd to 4a4135b Compare March 21, 2024 18:15

HamzaIsrar12 requested changes Mar 25, 2024

View reviewed changes

omerhabib26 added 14 commits March 27, 2024 20:12

feat: Add Analytics events

1350347

- Add analytics for following modules - AppReview, Profile, WhatsNew, Course, Discussion, Logistration, MainDashboard, Discovery - Restructure the analytics implementation to module level - Update the event names accordingly fix: LEARNER-9876

fix: Updated the failed test cases

a3c0010

fix: LEARNER-9876

fix: Updated the analytics with iOS parity

563e933

fix: LEARNER-9876

fix: Updated the analytics with iOS parity - 2

a3e0e94

fix: LEARNER-9876

fix: Address PR comments by Kirill

ed1f4fd

fix: LEARNER-9876

fix: minor code fixes

4a835b1

fix: LEARNER-9876

fix: Address PR comments

b7156d3

fix: LEARNER-9876

fix: fix test cases

55ab226

fix: LEARNER-9876

fix: Address PR comments by Hamza - 2

67b36e5

fix: refactoring code

62d70c7

fix: crash fix on ThankYouDialogFragment

4eda427

fix: resolve warnings

4cdaf99

fix: added webView analytics

28c6679

fix: test cases

89a1e83

omerhabib26 force-pushed the omer/LEARNER-9876 branch from 7b31d64 to 89a1e83 Compare March 27, 2024 15:14

HamzaIsrar12 approved these changes Mar 27, 2024

View reviewed changes

omerhabib26 merged commit 2da3666 into openedx:develop Mar 27, 2024
3 checks passed

omerhabib26 deleted the omer/LEARNER-9876 branch March 27, 2024 18:45

volodymyr-chekyrta reviewed Mar 28, 2024

View reviewed changes

omerhabib26 restored the omer/LEARNER-9876 branch March 28, 2024 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Analytics events #258

feat: Add Analytics events #258

omerhabib26 commented Mar 12, 2024 •

edited

Loading

k1rill left a comment

k1rill left a comment

HamzaIsrar12 left a comment

HamzaIsrar12 Mar 21, 2024

omerhabib26 Mar 21, 2024

HamzaIsrar12 Mar 25, 2024

HamzaIsrar12 left a comment •

edited

Loading

HamzaIsrar12 Mar 25, 2024

HamzaIsrar12 left a comment

volodymyr-chekyrta Mar 28, 2024

omerhabib26 Mar 29, 2024

volodymyr-chekyrta Mar 28, 2024

omerhabib26 Mar 29, 2024

volodymyr-chekyrta Mar 29, 2024

omerhabib26 Mar 29, 2024

volodymyr-chekyrta Mar 28, 2024

volodymyr-chekyrta Mar 28, 2024

volodymyr-chekyrta Mar 28, 2024

volodymyr-chekyrta Mar 28, 2024

volodymyr-chekyrta Mar 28, 2024

volodymyr-chekyrta Mar 28, 2024

volodymyr-chekyrta Mar 28, 2024

volodymyr-chekyrta Mar 28, 2024

feat: Add Analytics events #258

feat: Add Analytics events #258

Conversation

omerhabib26 commented Mar 12, 2024 • edited Loading

Description

k1rill left a comment

Choose a reason for hiding this comment

k1rill left a comment

Choose a reason for hiding this comment

HamzaIsrar12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamzaIsrar12 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamzaIsrar12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omerhabib26 commented Mar 12, 2024 •

edited

Loading

HamzaIsrar12 left a comment •

edited

Loading