Notification Analytics #3393
Notification Analytics #3393
Conversation
shubham1g5
requested review from
Jignesh-dimagi,
OrangeAndGreen and
conroy-ricketts
📝 WalkthroughWalkthroughThis PR adds comprehensive analytics tracking for push notifications across the app's notification lifecycle. Analytics events are reported when notifications are received via FCM (CommCareFirebaseMessagingService) and when stored (NotificationRecordDatabaseHelper), as well as when users interact with notifications through multiple entry points: the notification tray (DispatchActivity, PushNotificationActivity), notification history (PushNotificationActivity), and the Connect menu (ConnectActivity). Supporting analytics constants are introduced in the analytics utilities layer. Additionally, PushNotificationApiHelper's public API is refactored to separate notification storage from acknowledgment by replacing Sequence DiagramsequenceDiagram
participant FCM as FCM Service
participant Svc as CommCareFirebaseMessagingService
participant Analytics as FirebaseAnalyticsUtil
participant DB as NotificationRecordDatabaseHelper
participant User as User
participant DispatchActivity
FCM->>Svc: onMessageReceived(remoteMessage)
Svc->>Analytics: reportNotificationReceived(FIREBASE, action, notificationId)
Svc->>DB: handleNotification()
Note over DB: Store notification record
DB->>Analytics: reportNotificationReceived(PERSONAL_ID_API, action, notificationId)
User->>DispatchActivity: Click notification (tray)
DispatchActivity->>Analytics: reportNotificationClicked(action, NOTIFICATION_TRAY)
DispatchActivity->>DispatchActivity: Start PN intent
User->>Svc: Click notification (via history)
Svc->>Analytics: reportNotificationClicked(action, NOTIFICATION_HISTORY)
Svc->>Svc: Handle intent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/src/org/commcare/connect/database/NotificationRecordDatabaseHelper.kt (1)
64-64: Consider breaking this line for readability.Line 64 exceeds typical line length limits. Consider formatting for better readability:
- FirebaseAnalyticsUtil.reportNotificationReceived(AnalyticsParamValue.REPORT_NOTIFICATION_METHOD_PERSONAL_ID_API, incoming.action, incoming.notificationId) + FirebaseAnalyticsUtil.reportNotificationReceived( + AnalyticsParamValue.REPORT_NOTIFICATION_METHOD_PERSONAL_ID_API, + incoming.action, + incoming.notificationId + )app/src/org/commcare/activities/connect/ConnectActivity.java (1)
189-196: Consider documenting the mapping or making it more maintainable.The mapping currently includes only two menu items. If additional menu items are added to
menu_connect.xmlin the future, they might be missed in the analytics tracking. Consider adding a comment noting that new menu items should be added here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/src/org/commcare/activities/DispatchActivity.java(3 hunks)app/src/org/commcare/activities/PushNotificationActivity.kt(2 hunks)app/src/org/commcare/activities/connect/ConnectActivity.java(5 hunks)app/src/org/commcare/connect/database/NotificationRecordDatabaseHelper.kt(3 hunks)app/src/org/commcare/google/services/analytics/AnalyticsParamValue.java(2 hunks)app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java(1 hunks)app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java(2 hunks)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java(1 hunks)app/src/org/commcare/services/CommCareFirebaseMessagingService.java(2 hunks)app/src/org/commcare/utils/PushNotificationApiHelper.kt(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T13:40:19.645Z
Learning: PR #3048 introduces a comprehensive messaging system in the Connect feature, implementing secure encryption using AES-GCM for message content, proper channel management with consent flows, and a well-designed UI separation between sent and received messages with real-time notification integration.
📚 Learning: 2025-05-08T11:08:18.530Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
Applied to files:
app/src/org/commcare/activities/DispatchActivity.javaapp/src/org/commcare/activities/connect/ConnectActivity.java
📚 Learning: 2025-06-06T20:15:21.134Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java:65-71
Timestamp: 2025-06-06T20:15:21.134Z
Learning: In the CommCare Android Connect module, job.getLearnAppInfo() and getLearnModules() should never return null according to the system design. The team prefers to let the code crash if these values are unexpectedly null rather than adding defensive null checks, following a fail-fast philosophy to catch bugs early rather than masking them.
Applied to files:
app/src/org/commcare/activities/DispatchActivity.java
📚 Learning: 2025-06-06T19:52:53.173Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java:163-180
Timestamp: 2025-06-06T19:52:53.173Z
Learning: In the CommCare Android Connect feature, database operations like ConnectJobUtils.upsertJob should be allowed to crash rather than being wrapped in try-catch blocks. The team prefers fail-fast behavior for database errors instead of graceful error handling.
Applied to files:
app/src/org/commcare/activities/DispatchActivity.java
📚 Learning: 2025-04-18T20:13:29.655Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3040
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java:39-55
Timestamp: 2025-04-18T20:13:29.655Z
Learning: In the CommCare Android Connect feature, the JSON object passed to `ConnectJobDeliveryFlagRecord.fromJson()` method should never be null based on the implementation design.
Applied to files:
app/src/org/commcare/activities/DispatchActivity.java
🧬 Code graph analysis (4)
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (2)
app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (1)
CCAnalyticsParam(7-50)app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java (1)
CCAnalyticsEvent(7-62)
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (3)
app/src/org/commcare/google/services/analytics/AnalyticsParamValue.java (1)
AnalyticsParamValue(7-202)app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (1)
CCAnalyticsParam(7-50)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
FirebaseAnalyticsUtil(42-602)
app/src/org/commcare/activities/DispatchActivity.java (3)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-56)app/src/org/commcare/google/services/analytics/AnalyticsParamValue.java (1)
AnalyticsParamValue(7-202)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
FirebaseAnalyticsUtil(42-602)
app/src/org/commcare/activities/connect/ConnectActivity.java (2)
app/src/org/commcare/google/services/analytics/AnalyticsParamValue.java (1)
AnalyticsParamValue(7-202)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
FirebaseAnalyticsUtil(42-602)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (11)
app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java (1)
60-61: LGTM!The new analytics event constants follow the established naming conventions and visibility patterns in this file.
app/src/org/commcare/utils/PushNotificationApiHelper.kt (2)
63-67: Clean separation of storage and acknowledgment.The refactoring to separate notification storage from acknowledgment improves clarity. Storing notifications first to obtain IDs, then acknowledging them, is a logical flow.
93-95: No remaining references to old method signature detected.The verification confirms that all references to
updatePushNotificationshave been removed and no orphaned call sites exist. The new methodacknowledgeNotificationsReceiptis properly defined and internally called within the file. Other usages ofPushNotificationApiHelperin the codebase reference different methods (retrieveLatestPushNotifications,convertPNRecordsToPayload, etc.) and remain unaffected by this change.app/src/org/commcare/activities/PushNotificationActivity.kt (1)
76-79: LGTM!Analytics tracking correctly placed before handling the notification click, using the appropriate method constant for notification history interactions.
app/src/org/commcare/activities/DispatchActivity.java (1)
170-174: LGTM!Analytics tracking correctly placed before handling the notification intent, using the appropriate method constant for notification tray interactions.
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
42-44: LGTM!Analytics tracking correctly placed at the point of FCM message receipt, using the appropriate delivery method constant.
app/src/org/commcare/connect/database/NotificationRecordDatabaseHelper.kt (1)
46-65: Analytics correctly placed after successful storage.The analytics call at line 64 properly tracks only new notifications (skipping duplicates at line 59) and reports after successful persistence, which is the right approach.
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (2)
582-592: LGTM!The
reportNotificationReceivedmethod correctly handles required and optional parameters, building the analytics bundle appropriately with conditional inclusion of nullable fields.
594-601: LGTM!The
reportNotificationClickedmethod correctly handles required and optional parameters, building the analytics bundle appropriately with conditional inclusion of the nullable action type.app/src/org/commcare/google/services/analytics/AnalyticsParamValue.java (2)
23-24: LGTM: Menu item constants are well-placed and follow conventions.The new constants for Connect sync and notifications bell menu items are appropriately placed in the options menu section and follow the established naming pattern.
197-201: LGTM: Notification analytics constants are well-organized and clearly named.The new constants for notification delivery methods and click sources follow clear naming conventions and are appropriately grouped. The distinction between
REPORT_NOTIFICATION_METHOD_*(for delivery tracking) andREPORT_NOTIFICATION_CLICK_*(for interaction tracking) provides good semantic clarity.
| FirebaseAnalyticsUtil.reportOptionsMenuItemClick(this.getClass(), | ||
| menuIdToAnalyticsParam.get(item.getItemId())); |
There was a problem hiding this comment.
Guard against null when menu item is not in the mapping.
If a menu item not present in menuIdToAnalyticsParam is clicked (e.g., android.R.id.home from the action bar), get() will return null, which is passed to reportOptionsMenuItemClick. This could cause issues since the itemLabel parameter is not marked @Nullable.
Consider adding a null check:
@Override
public boolean onOptionsItemSelected(MenuItem item) {
- FirebaseAnalyticsUtil.reportOptionsMenuItemClick(this.getClass(),
- menuIdToAnalyticsParam.get(item.getItemId()));
+ String analyticsParam = menuIdToAnalyticsParam.get(item.getItemId());
+ if (analyticsParam != null) {
+ FirebaseAnalyticsUtil.reportOptionsMenuItemClick(this.getClass(), analyticsParam);
+ }
if (item.getItemId() == R.id.action_bell) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FirebaseAnalyticsUtil.reportOptionsMenuItemClick(this.getClass(), | |
| menuIdToAnalyticsParam.get(item.getItemId())); | |
| @Override | |
| public boolean onOptionsItemSelected(MenuItem item) { | |
| String analyticsParam = menuIdToAnalyticsParam.get(item.getItemId()); | |
| if (analyticsParam != null) { | |
| FirebaseAnalyticsUtil.reportOptionsMenuItemClick(this.getClass(), analyticsParam); | |
| } | |
| if (item.getItemId() == R.id.action_bell) { |
🤖 Prompt for AI Agents
In app/src/org/commcare/activities/connect/ConnectActivity.java around lines 212
to 213, the call passes menuIdToAnalyticsParam.get(item.getItemId()) directly
which can return null for unmapped menu IDs (e.g., android.R.id.home); add a
null guard before calling FirebaseAnalyticsUtil.reportOptionsMenuItemClick:
retrieve the mapped value into a local variable, check for null, and only call
reportOptionsMenuItemClick when the value is non-null (optionally log or skip
analytics when null).
| static final String NOTIFICATION_DELIVERY_METHOD = "notification_delivery_method"; | ||
| static final String NOTIFICATION_ACTION_TYPE = "notification_action_type"; | ||
| static final String NOTIFICATION_ID= "notification_id"; |
There was a problem hiding this comment.
Fix formatting: add space before assignment operator.
Line 48 is missing a space before the = operator, which is inconsistent with the rest of the file's formatting style.
Apply this diff to fix the spacing:
- static final String NOTIFICATION_ID= "notification_id";
+ static final String NOTIFICATION_ID = "notification_id";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static final String NOTIFICATION_DELIVERY_METHOD = "notification_delivery_method"; | |
| static final String NOTIFICATION_ACTION_TYPE = "notification_action_type"; | |
| static final String NOTIFICATION_ID= "notification_id"; | |
| static final String NOTIFICATION_DELIVERY_METHOD = "notification_delivery_method"; | |
| static final String NOTIFICATION_ACTION_TYPE = "notification_action_type"; | |
| static final String NOTIFICATION_ID = "notification_id"; |
🤖 Prompt for AI Agents
In app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java around
lines 46 to 48, the third constant declaration is missing a space before the
equals sign; update the line so there's a single space before and after the '='
to match the file's formatting style (i.e., "static final String NOTIFICATION_ID
= \"notification_id\";"), then run the project's code formatter or checkstyle to
ensure consistent spacing across the file.
| static final String NOTIFICATION_DELIVERY_METHOD = "notification_delivery_method"; | ||
| static final String NOTIFICATION_ACTION_TYPE = "notification_action_type"; | ||
| static final String NOTIFICATION_ID= "notification_id"; | ||
| public static final String NOTIFICATION_CLICK_METHOD = "notification_click_method"; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the visibility inconsistency for notification constants.
Three notification constants (lines 46-48) are package-private while NOTIFICATION_CLICK_METHOD (line 49) is public. Ensure this visibility difference is intentional and that all four constants don't need uniform access control.
Run the following script to check where these constants are referenced:
🏁 Script executed:
#!/bin/bash
# Description: Verify usage of notification-related constants to confirm visibility requirements
# Search for usage of all four notification constants
echo "=== Usage of NOTIFICATION_DELIVERY_METHOD ==="
rg -n --type=java '\bNOTIFICATION_DELIVERY_METHOD\b'
echo -e "\n=== Usage of NOTIFICATION_ACTION_TYPE ==="
rg -n --type=java '\bNOTIFICATION_ACTION_TYPE\b'
echo -e "\n=== Usage of NOTIFICATION_ID ==="
rg -n --type=java '\bNOTIFICATION_ID\b'
echo -e "\n=== Usage of NOTIFICATION_CLICK_METHOD ==="
rg -n --type=java '\bNOTIFICATION_CLICK_METHOD\b'Length of output: 2866
Change NOTIFICATION_CLICK_METHOD to package-private for consistency.
All four notification constants are used exclusively within FirebaseAnalyticsUtil in the same package (org.commcare.google.services.analytics). The three package-private constants (lines 46-48) should match NOTIFICATION_CLICK_METHOD (line 49), which should be changed from public static final to static final.
🤖 Prompt for AI Agents
In app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java around
lines 46 to 49, change the visibility of NOTIFICATION_CLICK_METHOD from public
static final to package-private (static final) so it matches the other three
notification constants; update the declaration accordingly and run a quick
compile to ensure no external references require it to remain public.
|
@shubham1g5 I guess its important to log the events for signalling also? Interested to know how many were able to sync in background? |
| String actionType = pnIntent.getStringExtra(REDIRECT_ACTION); | ||
| FirebaseAnalyticsUtil.reportNotificationClicked( | ||
| actionType, | ||
| AnalyticsParamValue.REPORT_NOTIFICATION_CLICK_NOTIFICATION_TRAY | ||
| ); |
There was a problem hiding this comment.
don't they all go through dispatch ?
There was a problem hiding this comment.
Not when pending intents are set.
| String actionType = remoteMessage.getData().get("action"); | ||
| String notificationId = remoteMessage.getData().get("notification_id"); | ||
| FirebaseAnalyticsUtil.reportNotificationReceived(AnalyticsParamValue.REPORT_NOTIFICATION_METHOD_FIREBASE, actionType, notificationId); | ||
|
|
There was a problem hiding this comment.
action and notification_id might not present if not CCC notification. Do we want to log only CCC?
There was a problem hiding this comment.
think we should log them all and reportNotificationReceived does treat these fields as nullables
You mean notifications received for signalling ? |
Whenever app is able to sync in worker upon receiving FCM (only), should we log the event to see success rates? |
You mean like API success rate ? I am not sure if we want to do network analytics but think probably a good idea to log everytime we push a notification to tray, would that be sufficient here ? |
| static final String PERSONAL_ID_MESSAGE_SENT = "personal_id_message_sent"; | ||
| static final String PERSONAL_ID_LINKING = "personal_id_linking"; | ||
|
|
||
| static final String PERSONAL_ID_NOTIFICATION_RECEIVED = "personal_id_notification_received"; |
There was a problem hiding this comment.
We'd discussed limiting the number of unique events that we send to analytics since there's a lifetime limit. Given that, I wonder if we want a single personalid_notification event with parameters to specify additional info? Retrieving the additional info from BigQuery doesn't add much difficulty, and then we can continue to reuse this event for any additional notification-related events that we want to log in the future.
There was a problem hiding this comment.
Nice, although these values could be cleaned up now.
| new String[]{selectedItem}); | ||
| } | ||
|
|
||
| public static void reportNotificationReceived(String deliveryMethod, @Nullable String actionType,@Nullable String notificationId) { |
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
LGTM pending comments that others have posted
Yes, it should be good |
| static final String PERSONAL_ID_MESSAGE_SENT = "personal_id_message_sent"; | ||
| static final String PERSONAL_ID_LINKING = "personal_id_linking"; | ||
|
|
||
| static final String PERSONAL_ID_NOTIFICATION_RECEIVED = "personal_id_notification_received"; |
There was a problem hiding this comment.
Nice, although these values could be cleaned up now.
shubham1g5
dismissed stale reviews from conroy-ricketts, OrangeAndGreen, and Jignesh-dimagi
via
b355600
shubham1g5
requested review from
Jignesh-dimagi,
OrangeAndGreen and
conroy-ricketts
Product Description
https://dimagi.atlassian.net/browse/CCCT-1630
Technical Summary
Labels and Review