feat: explore dash merchant search event tracking by ClaudeHangui · Pull Request #917 · dashpay/dash-wallet

ClaudeHangui · 2022-03-18T21:32:02Z

Use Firebase to track user interactions related to explore dash for merchants

Issue being fixed or feature implemented

Related PR's and Dependencies

Screenshots / Videos

How Has This Been Tested?

QA (Mobile Team)

Checklist:

I have performed a self-review of my own code and added comments where necessary
I have added or updated relevant unit/integration/functional/e2e tests

ClaudeHangui · 2022-03-18T21:45:47Z

                    )
                }
+
+                map.setOnCameraMoveStartedListener { reason ->


There are various reasons that can cause the camera to be in motion in a map: however our tracking only focuses on the Camera motion initiated in response to user gestures on the map including pinch to zoom and pan gestures. All other causes/reasons initiated or not by the user are to be ignored for the tracking

ClaudeHangui · 2022-03-18T21:53:51Z

+                            viewModel.hasZoomLevelChanged(previousZoomLevel, map.cameraPosition.zoom) -> {
+                                viewModel.zoomLevelChangeCallback.call()
+                            }
+                            viewModel.hasCameraCenterChanged(


Check if a pan gesture was made: we rely on the coordinates (position) of the camera in the center of the map

ClaudeHangui · 2022-03-18T21:54:20Z

                    val bounds = map.projection.visibleRegion.latLngBounds
+                    if (cameraMovementReason == GoogleMap.OnCameraMoveStartedListener.REASON_GESTURE){
+                        when {
+                            viewModel.hasZoomLevelChanged(previousZoomLevel, map.cameraPosition.zoom) -> {


Check if the user zoomed (in/out) the map

This entire when can be moved to the ViewModel. You're calling a viewModel check and then calling a viewModel event if the check is satisfied. Just call a triggerEventIfSatisfied(param, param) method once.

This entire check is needed to trigger an event that is then observed from another fragment that logs the event. I don't quite get why can't you either

Log it from this fragment

Log it from the viewModel

… changed

Syn-McJ

@ClaudeHangui there are some problems with events being passed around from one fragment to another for no reason. This can be simplified greatly.

Syn-McJ · 2022-03-19T05:04:26Z

                    val bounds = map.projection.visibleRegion.latLngBounds
+                    if (cameraMovementReason == GoogleMap.OnCameraMoveStartedListener.REASON_GESTURE){
+                        when {
+                            viewModel.hasZoomLevelChanged(previousZoomLevel, map.cameraPosition.zoom) -> {


This entire when can be moved to the ViewModel. You're calling a viewModel check and then calling a viewModel event if the check is satisfied. Just call a triggerEventIfSatisfied(param, param) method once.

This entire check is needed to trigger an event that is then observed from another fragment that logs the event. I don't quite get why can't you either

Log it from this fragment

Log it from the viewModel

Syn-McJ · 2022-03-19T05:07:18Z

 import androidx.lifecycle.*
 import androidx.paging.*
 import androidx.paging.PagingData
+import com.google.android.gms.maps.model.LatLng


The viewModel should not reference LatLng since it's implementation-dependent and we're working with an interface in case of UserLocationState. If the implementation of UserLocationState changed, you'll have to change a lot of related code in the viewModel as well. Please use GeoBounds instead.

Syn-McJ · 2022-03-19T05:08:37Z

+                analyticsService.logEvent(AnalyticsConstants.ExploreDash.ZOOM_MERCHANT_MAP, bundleOf())
+            }
+        }
+        viewModel.cameraCenterChangeCallback.observe(viewLifecycleOwner){


SearchFragment doesn't have to know anything about the map, I don't get why this is here.

ClaudeHangui · 2022-03-19T10:03:08Z

@Syn-McJ there was actually some reason for me handling the tracking of some events in SearchFragment: I originally had those events in the ExploreMapFragment but I realised that we are to implement these same events for the ATM Search as well (in another story), and since the SearchFragment is aware of the ExploreTopic I thought it was ideal for it to handle all the map-related trackings. Guess it was the wrong choice. Thanks for the feedback !

Syn-McJ · 2022-03-19T10:09:34Z

@ClaudeHangui I see, well there is no reason not to log events from the ViewModel - in fact, I would prefer not doing injections into fragments at all. But not all user interactions go into the ViewModel and so it might not be always convenient.

- update ExploreViewModelTest.kt to handle injected dependency AnalyticsService

ClaudeHangui · 2022-03-20T01:08:14Z

-                        map.cameraPosition.zoom
-                    )
+                    viewModel.previousZoomLevel = map.cameraPosition.zoom
+                    viewModel.previousCameraGeoBounds = getGeoBounds()


using searchBounds in our triggerPanAndZoomEvents() gives us a NPE, I'm not sure I understand why; so I had to create another property to hold the current geobounds. Moreover, it appears this searchBounds var is no longer used in the ExploreMapFragment

@ClaudeHangui if you pass the map instance into getGeoBounds, you can make the return value non-nullable. Then you won't have to accept nullable GeoBounds into the triggerPanAndZoomEvents. Then you won't have to use the !! operator on it down the line.
previousCameraGeoBounds also either shouldn't be nullable or shouldn't be unpacked with the !! operator - it's better to make use of kotlins null-safety features when you can.

- initialize previousCameraGeoBounds var with non-nullable value

Syn-McJ

Looks great

HashEngineering · 2022-03-21T02:44:51Z

+        const val WHERE_TO_SPEND = "portal_where_to_spend"
+        const val PORTAL_ATM = "portal_atm"
+        const val LEARN_MORE = "info_learn_more"
+        const val CONTINUE = "info_continue"
+        const val ONLINE_MERCHANTS = "online_merchants"
+        const val NEARBY_MERCHANTS = "nearby_merchants"
+        const val ALL_MERCHANTS = "all_merchants"
+        const val FILTER_MERCHANTS_TOP = "filter_merchants_top"
+        const val FILTER_MERCHANTS_BOTTOM = "filter_merchants_bottom"
+        const val SELECT_MERCHANT_LOCATION = "select_merchant_location"
+        const val SELECT_MERCHANT_MARKER = "select_merchant_marker"
+        const val INFO_EXPLORE_MERCHANT = "info_search"
+        const val PAN_MERCHANT_MAP = "pan_merchant_map"
+        const val ZOOM_MERCHANT_MAP = "zoom_merchant_map"


Same comment as #915, there should be a prefix, otherwise within the list of events, some explore dash events are at A and others at Z.

HashEngineering

Add prefix to event names.

HashEngineering

excellent work

ClaudeHangui added 2 commits March 16, 2022 12:53

track firebase events from explore dash portal and info screens

0c69cc1

track firebase events for merchant explore

961e23d

ClaudeHangui requested a review from HashEngineering March 18, 2022 21:32

ClaudeHangui changed the title ~~feat: explore dash merchant search~~ feat: explore dash merchant search event tracking Mar 18, 2022

ClaudeHangui commented Mar 18, 2022

View reviewed changes

ClaudeHangui added 2 commits March 18, 2022 21:58

refactor funtion to determine if center position of map has changed

87b8af2

fix indentation on funtion to determine if center position of map has…

802cb09

… changed

Syn-McJ suggested changes Mar 19, 2022

View reviewed changes

- move map-related tracking to viewmodel

3d47624

- update ExploreViewModelTest.kt to handle injected dependency AnalyticsService

ClaudeHangui commented Mar 20, 2022

View reviewed changes

- refactor getGeoBounds() function -> set parameter of type GoogleMap

f022f12

- initialize previousCameraGeoBounds var with non-nullable value

Syn-McJ approved these changes Mar 20, 2022

View reviewed changes

HashEngineering reviewed Mar 21, 2022

View reviewed changes

HashEngineering requested changes Mar 21, 2022

View reviewed changes

add explore prefix to explore dash tracing event names

e849250

HashEngineering approved these changes Mar 23, 2022

View reviewed changes

Merge branch 'master' into feature-explore-dash-firebase-merchant-search

2483bb5

HashEngineering merged commit d55c432 into master Mar 23, 2022

HashEngineering deleted the feature-explore-dash-firebase-merchant-search branch July 20, 2022 23:19

Uh oh!

Conversation

ClaudeHangui commented Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

Related PR's and Dependencies

Screenshots / Videos

How Has This Been Tested?

Checklist:

Uh oh!

ClaudeHangui Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Syn-McJ left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ClaudeHangui commented Mar 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Syn-McJ commented Mar 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Syn-McJ left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HashEngineering left a comment

Choose a reason for hiding this comment

Uh oh!

HashEngineering left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ClaudeHangui commented Mar 18, 2022 •

edited

Loading

ClaudeHangui Mar 18, 2022 •

edited

Loading

ClaudeHangui commented Mar 19, 2022 •

edited

Loading