Analytics for user funneling #3328
Conversation
jaypanchal-13
requested review from
Jignesh-dimagi,
OrangeAndGreen and
shubham1g5
📝 WalkthroughWalkthroughThis PR introduces new analytics events and adjusts when they are logged across several user flows (continue button, messaging, personal ID linking, learning and delivery progress). It adds three event constants and corresponding helper methods in FirebaseAnalyticsUtil. ConnectJobHelper changes conditionally log learning completion and delivery progress, including use of a new ConnectJobRecord.getDeliveryProgressPercentage() method. PersonalIdManager now logs an event upon successful link, ConnectMessageFragment logs on successful message send, and PersonalIdPhoneFragment logs on continue button click. Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as PersonalIdPhoneFragment
participant Analytics as FirebaseAnalyticsUtil
User->>UI: Tap Continue
note over UI: Start processing
UI->>Analytics: continueButtonClicks()
note over Analytics: Log CONTINUE_BUTTON_CLICKED
sequenceDiagram
autonumber
actor User
participant Manager as PersonalIdManager
participant App as LinkedApp
participant Analytics as FirebaseAnalyticsUtil
User->>Manager: Link Personal ID (password)
Manager->>App: linkToPersonalId(password)
App-->>Manager: Success/Failure
alt Success
Manager->>Analytics: personalIdLinked()
note over Analytics: Log PERSONAL_ID_LINKED
else Failure
note over Manager: No analytics change
end
sequenceDiagram
autonumber
actor User
participant UI as ConnectMessageFragment
participant Backend as Messaging API
participant Analytics as FirebaseAnalyticsUtil
User->>UI: Send message
UI->>Backend: sendMessage()
Backend-->>UI: Success
UI->>UI: Update read status, scroll
UI->>Analytics: sentMessage()
note over Analytics: Log SENT_MESSAGE
sequenceDiagram
autonumber
participant Helper as ConnectJobHelper
participant Job as ConnectJobRecord
participant Analytics as FirebaseAnalyticsUtil
Helper->>Job: getDeliveryProgressPercentage()
alt maxVisits > 0
Job-->>Helper: floor(100 * completed / max)
else maxVisits == 0
Job-->>Helper: 100
end
alt percentage == 100
Helper->>Analytics: Log delivery completion
else percentage < 100
note over Helper: No delivery completion event
end
alt Learning
Helper->>Helper: Check job.learningPercentComplete == 100 or job.passedAssessment()
alt condition true
Helper->>Analytics: Log learning completion
else
note over Helper: Skip analytics
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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: 2
🧹 Nitpick comments (5)
app/src/org/commcare/connect/PersonalIdManager.java (1)
359-360: Log placement ACK.Firing PERSONAL_ID_LINKED immediately after linkToPersonalId(password) makes sense for the funnel step. If the link can fail silently, consider moving this after storeApp() returns successfully or adding a complementary “link_failed/declined” event for coverage.
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java (1)
30-31: Track failures too for a complete funnel.You log SENT_MESSAGE on success. Consider also logging a failure event (or the same event with a result param) in the !success branch to monitor reliability and compute success rates.
Also applies to: 173-181
app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java (1)
490-494: Clamp delivery percentage to [0,100].Protect against bad data (completed > total or negatives) while preserving current zero-total semantics.
Apply:
- public int getDeliveryProgressPercentage() { - int completed = getCompletedVisits(); - int total = getMaxVisits(); - return total > 0 ? (100 * completed / total) : 100; - } + public int getDeliveryProgressPercentage() { + int completed = Math.max(0, getCompletedVisits()); + int total = getMaxVisits(); + int pct = total > 0 ? (100 * completed / total) : 100; + return Math.min(100, Math.max(0, pct)); + }app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (2)
515-517: Name consistency: prefer the existing "report" verb for analytics methods*Most methods here use the
report*prefix (e.g.,reportLoginClicks,reportNavDrawerOpen). Consider aligning this one for API consistency.Proposed minimal tweak (keeps current method for back-compat and adds a canonical name):
- public static void continueButtonClicks() { - reportEvent(CCAnalyticsEvent.CONTINUE_BUTTON_CLICKED); - } + // Kept for back-compat; prefer reportContinueButtonClick() + public static void continueButtonClicks() { + reportContinueButtonClick(); + } + public static void reportContinueButtonClick() { + reportEvent(CCAnalyticsEvent.CONTINUE_BUTTON_CLICKED); + }
518-520: sentMessage() only called on success; optional messageType overload recommended
- Confirmed
FirebaseAnalyticsUtil.sentMessage()is invoked solely in the success branch ofMessageManager.sendMessage(...)(ConnectMessageFragment.java:180).- Optional: add an overload accepting
String messageTypeto pass viaFirebaseAnalytics.Param.ITEM_CATEGORYfor richer analytics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java(1 hunks)app/src/org/commcare/connect/ConnectJobHelper.kt(2 hunks)app/src/org/commcare/connect/PersonalIdManager.java(1 hunks)app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java(2 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(1 hunks)app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java(1 hunks)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: shubham1g5
PR: dimagi/commcare-android#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.
📚 Learning: 2025-02-19T15:15:01.935Z
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2956
File: app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java:58-60
Timestamp: 2025-02-19T15:15:01.935Z
Learning: Error handling for message retrieval in ConnectMessageChannelListFragment's retrieveMessages callback is not required as per user preference.
Applied to files:
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
📚 Learning: 2025-07-29T14:14:07.954Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3248
File: app/src/org/commcare/adapters/ConnectMessageAdapter.java:0-0
Timestamp: 2025-07-29T14:14:07.954Z
Learning: In the CommCare Android Connect messaging system (ConnectMessageAdapter.java), the team prefers to let getMessage() potentially return null and crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately during development.
Applied to files:
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
📚 Learning: 2025-05-08T11:08:18.530Z
Learnt from: shubham1g5
PR: dimagi/commcare-android#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/connect/ConnectJobHelper.kt
🧬 Code graph analysis (4)
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java (1)
CCAnalyticsEvent(7-62)
app/src/org/commcare/connect/PersonalIdManager.java (1)
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
FirebaseAnalyticsUtil(41-579)
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java (1)
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
FirebaseAnalyticsUtil(41-579)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
FirebaseAnalyticsUtil(41-579)
⏰ 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 (4)
app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java (1)
58-60: Events look good.Names are concise, Firebase-safe, and consistent with existing conventions.
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
239-243: Good: click logged at entry, then button disabled.This should minimize duplicate logs from fast double taps.
app/src/org/commcare/connect/ConnectJobHelper.kt (2)
65-67: Learning analytics gating: 👍Logging only at 100% or after a passed assessment is reasonable.
91-98: Drop stale-data concern—fields are updated before gating. DeliveryAppProgressResponseParser assignsjob.deliveriesandjob.paymentsfrom the response prior to ConnectJobHelper’s checks, so in-memory data is current.Likely an incorrect or invalid review comment.
| if (deliveryAppProgressResponseModel.updatedJob) { | ||
| FirebaseAnalyticsUtil.reportCccApiDeliveryProgress(true) | ||
| ConnectJobUtils.upsertJob(context, job) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Deduplicate delivery analytics; avoid triple-logging on one response.
You now log in updatedJob, optionally in hasDeliveries/hasPayment, and again unconditionally at Line 105. Collapse to a single, gated emission to prevent skew.
Apply:
override fun onSuccess(deliveryAppProgressResponseModel: DeliveryAppProgressResponseModel) {
- if (deliveryAppProgressResponseModel.updatedJob) {
- FirebaseAnalyticsUtil.reportCccApiDeliveryProgress(true)
- ConnectJobUtils.upsertJob(context, job)
- }
+ var shouldLogDelivery = false
+ if (deliveryAppProgressResponseModel.updatedJob) {
+ shouldLogDelivery = true
+ ConnectJobUtils.upsertJob(context, job)
+ }
@@
- if (deliveryAppProgressResponseModel.hasDeliveries) {
- if (job.getDeliveryProgressPercentage() == 100) {
- FirebaseAnalyticsUtil.reportCccApiDeliveryProgress(true)
- }
- ConnectJobUtils.storeDeliveries(context, job.deliveries, job.jobId, true)
- }
+ if (deliveryAppProgressResponseModel.hasDeliveries) {
+ if (job.deliveryProgressPercentage == 100) {
+ shouldLogDelivery = true
+ }
+ ConnectJobUtils.storeDeliveries(context, job.deliveries, job.jobId, true)
+ }
@@
- if (deliveryAppProgressResponseModel.hasPayment) {
- if (job.payments.isNotEmpty()){
- FirebaseAnalyticsUtil.reportCccApiDeliveryProgress(true)
- }
- ConnectJobUtils.storePayments(context, job.payments, job.jobId, true)
- }
+ if (deliveryAppProgressResponseModel.hasPayment) {
+ if (job.payments.isNotEmpty()){
+ shouldLogDelivery = true
+ }
+ ConnectJobUtils.storePayments(context, job.payments, job.jobId, true)
+ }
@@
- FirebaseAnalyticsUtil.reportCccApiDeliveryProgress(true)
+ if (shouldLogDelivery) {
+ FirebaseAnalyticsUtil.reportCccApiDeliveryProgress(true)
+ }
listener.connectActivityComplete(true)Also applies to: 91-106
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/ConnectJobHelper.kt around lines 86-106, you
currently call FirebaseAnalyticsUtil.reportCccApiDeliveryProgress multiple times
(inside updatedJob, inside hasDeliveries/hasPayment branches, and again
unconditionally at line 105), causing triple-logging; replace these scattered
calls with a single gated emission: compute a single boolean (e.g., shouldReport
= deliveryAppProgressResponseModel.updatedJob ||
deliveryAppProgressResponseModel.hasDeliveries ||
deliveryAppProgressResponseModel.hasPayment) and call
FirebaseAnalyticsUtil.reportCccApiDeliveryProgress(shouldReport) exactly once
after the response handling, keep ConnectJobUtils.upsertJob(context, job) only
when updatedJob is true, and remove the other analytics calls in the earlier
branches so logging happens exactly once per response.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| public static void personalIdLinked() { | ||
| reportEvent(CCAnalyticsEvent.PERSONAL_ID_LINKED); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider including linking method/source; align name with "report" convention*
To analyze funnel sources (QR, SMS, manual), consider an overload that records the method. Also, for naming consistency, a reportPersonalIdLinked() alias helps.
Run to confirm it’s called only after a successful, persisted link:
Proposed overload + alias:
- public static void personalIdLinked() {
- reportEvent(CCAnalyticsEvent.PERSONAL_ID_LINKED);
- }
+ // Kept as simple wrapper
+ public static void personalIdLinked() {
+ reportPersonalIdLinked(null);
+ }
+ public static void reportPersonalIdLinked(String method) {
+ if (method == null) {
+ reportEvent(CCAnalyticsEvent.PERSONAL_ID_LINKED);
+ } else {
+ reportEvent(CCAnalyticsEvent.PERSONAL_ID_LINKED, FirebaseAnalytics.Param.METHOD, method);
+ }
+ }🏁 Script executed:
#!/bin/bash
# Check all call sites and surrounding context for success conditions
rg -nP -C3 '\bFirebaseAnalyticsUtil\.personalIdLinked\s*\(' app/ | sed -n '1,200p'Length of output: 657
Ensure analytics reported after persistence
- In
app/src/org/commcare/connect/PersonalIdManager.java(line 359), moveFirebaseAnalyticsUtil.personalIdLinked()to immediately afterConnectAppDatabaseUtil.storeApp(activity, linkedApp)so the event is sent only once the link is successfully saved. - Optional: in
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java, addto record the linking method and align with thepublic static void reportPersonalIdLinked(String method) { … } public static void personalIdLinked() { reportPersonalIdLinked(null); }
report*naming convention.
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/PersonalIdManager.java around line 359, move the
call to FirebaseAnalyticsUtil.personalIdLinked() so it is invoked immediately
after ConnectAppDatabaseUtil.storeApp(activity, linkedApp) completes
successfully (i.e., after confirming the app was persisted) to ensure the
analytics event is only reported after persistence; in
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java around
lines 522–524 add a new reportPersonalIdLinked(String method) method that does
the actual event reporting and change personalIdLinked() to delegate to
reportPersonalIdLinked(null) so you can record an optional linking method and
follow the report* naming convention.
This reverts commit 8ec5dc2.
…g' into CCCT-1671-analytic-user-funneling
| android:layout_height="wrap_content" | ||
| android:layout_marginTop="10dp" | ||
| android:background="@color/personal_id_credentials_earned" | ||
| android:drawableEnd="@drawable/ic_close_yellow" |
There was a problem hiding this comment.
these changes should be removed from the PR
| return numLearning > 0 ? (100 * getCompletedLearningModules() / numLearning) : 100; | ||
| } | ||
|
|
||
| public int getDeliveryProgressPercentage() { |
There was a problem hiding this comment.
Confirmed, that's consistent with the logic that we use on the delivery progress page for total progress.
There was a problem hiding this comment.
@jaypanchal-13 seems like we are maintaining this same logic in 2 places, can we also use this method from the other place Dave linked to maintain consistency of logic.
| if (job.learningPercentComplete == 100 || job.passedAssessment()){ | ||
| FirebaseAnalyticsUtil.reportCccApiLearnProgress(true) | ||
| } |
There was a problem hiding this comment.
think we always want to log this and not just for 100
| if (deliveryAppProgressResponseModel.updatedJob) { | ||
| FirebaseAnalyticsUtil.reportCccApiDeliveryProgress(true) | ||
| ConnectJobUtils.upsertJob(context, job) | ||
| } | ||
|
|
| public static void continueButtonClicks() { | ||
| reportEvent(CCAnalyticsEvent.CONTINUE_BUTTON_CLICKED); | ||
| } | ||
| public static void sentMessage() { | ||
| reportEvent(CCAnalyticsEvent.SENT_MESSAGE); | ||
| } | ||
|
|
||
| public static void personalIdLinked() { | ||
| reportEvent(CCAnalyticsEvent.PERSONAL_ID_LINKED); | ||
| } |
There was a problem hiding this comment.
can we use the existing method naming convention in this class reportX
| static final String PERSONAL_ID_CONFIGURATION_FAILURE = "personal_id_configuration_failure"; | ||
| static final String NAV_DRAWER_OPEN = "nav_drawer_open"; | ||
| static final String NAV_DRAWER_ITEM_SELECTED = "nav_drawer_item_selected"; | ||
| static final String CONTINUE_BUTTON_CLICKED = "continue_button_click"; |
There was a problem hiding this comment.
rename to
PERSONAL_ID_CONTINUE_CLICKEDPERSONAL_ID_MESSAGE_SENT
| reportEvent(CCAnalyticsEvent.SENT_MESSAGE); | ||
| } | ||
|
|
||
| public static void personalIdLinked() { |
There was a problem hiding this comment.
- we should send the app id we are linking here as a custom parameter.
- Think we should also log if user denies the linking or linking fails due to any reason as part of this event
| public static void continueButtonClicks() { | ||
| reportEvent(CCAnalyticsEvent.CONTINUE_BUTTON_CLICKED); | ||
| } |
There was a problem hiding this comment.
@OrangeAndGreen curious about your thoughts on logging this on every page of sign up flow with a custom param to signify screen from where it's logged ?
There was a problem hiding this comment.
I like that idea. In many cases we may already have a similar event that essentially tracks that action (like an API call or page transition), but this would make it more consistent across the pages rather than having to track different events. Essentially, for each page this would allow us to track that they tried to proceed, while screen_view events indicate when they succeeded.
There was a problem hiding this comment.
@jaypanchal-13 FYI to change this implementation as specified.
| public static void continueButtonClicks() { | ||
| reportEvent(CCAnalyticsEvent.CONTINUE_BUTTON_CLICKED); | ||
| } | ||
| public static void sentMessage() { |
There was a problem hiding this comment.
@OrangeAndGreen do you know what we are expecting to get out of this event ?
There was a problem hiding this comment.
I think the goal is to measure usage of the messaging feature as part of user funneling. Specifically row 17 of this spreadsheet which seeks to track "Has ever sent a message using in-app messaging".
| static final String PERSONAL_ID_CONTINUE_CLICKED = "continue_button_click"; | ||
| static final String PERSONAL_ID_MESSAGE_SENT = "message_sent"; | ||
| static final String PERSONAL_ID_LINKED = "personal_id_linked"; | ||
| static final String PERSONAL_ID_LINKING_FAILED = "personal_id_linking_failed"; |
There was a problem hiding this comment.
@jaypanchal-13 Please change values also to their name e.g. PERSONAL_ID_CONTINUE_CLICKED = "PERSONAL_ID_CONTINUE_CLICKED" as Firebase will have these values.
| if (shouldLogDelivery) { | ||
| FirebaseAnalyticsUtil.reportCccApiDeliveryProgress(true) | ||
| } |
There was a problem hiding this comment.
@jaypanchal-13 How does reporting CCC API for delivery progress depends upon the changed values? Isn't it just reporting that API has succeeded?
There was a problem hiding this comment.
@Jignesh-dimagi Yes you are correct. I was also wondering it. @OrangeAndGreen Can you address it?
There was a problem hiding this comment.
@jaypanchal-13 @Jignesh-dimagi think the confusion is caused by trying to utilise the same event to also give us info on the state of the job. Think we need to introduce an additional parameter here like - reportCccApiDeliveryProgress(apiSuccess, param2) where param2 reports anything additional we want to report which I am confused at this point what exactly we are trying to report. @jaypanchal-13 can you give more details on that.
There was a problem hiding this comment.
@shubham1g5 Yes you are suggesting correct to add additional parameter. Check row 25,26,27 for more details here
| public static void continueButtonClicks() { | ||
| reportEvent(CCAnalyticsEvent.CONTINUE_BUTTON_CLICKED); | ||
| } |
There was a problem hiding this comment.
@jaypanchal-13 FYI to change this implementation as specified.
| static final String PERSONAL_ID_LINKED = "PERSONAL_ID_LINKED"; | ||
| static final String PERSONAL_ID_LINKING_FAILED = "PERSONAL_ID_LINKING_FAILED"; |
There was a problem hiding this comment.
both of these should be same events with a custom parameter success to denote failure vs success.
| reportEvent(CCAnalyticsEvent.LOGIN_CLICK); | ||
| } | ||
|
|
||
| public static void reportContinueButtonClicks() { |
There was a problem hiding this comment.
can we change these methods to say reportX where X is the exact name of the event. For eg. reportPersonalIDContinueClicked and so on.
| } | ||
|
|
||
| private void onContinueClicked() { | ||
| FirebaseAnalyticsUtil.reportContinueButtonClicks(); |
There was a problem hiding this comment.
we also need to log this from other Personal ID sign up pages.
There was a problem hiding this comment.
we also need to log this from other Personal ID sign up pages.
@shubham1g5 Can you suggest where other places do I need to add this log?.
There was a problem hiding this comment.
From all the Pages that have a continue button and are part of nav graph in PersonalIdActivity
| if (shouldLogDelivery) { | ||
| FirebaseAnalyticsUtil.reportCccApiDeliveryProgress(true) | ||
| } |
There was a problem hiding this comment.
@jaypanchal-13 @Jignesh-dimagi think the confusion is caused by trying to utilise the same event to also give us info on the state of the job. Think we need to introduce an additional parameter here like - reportCccApiDeliveryProgress(apiSuccess, param2) where param2 reports anything additional we want to report which I am confused at this point what exactly we are trying to report. @jaypanchal-13 can you give more details on that.
| object : ConnectApiHandler<DeliveryAppProgressResponseModel>() { | ||
| override fun onSuccess(deliveryAppProgressResponseModel: DeliveryAppProgressResponseModel) { | ||
| if (deliveryAppProgressResponseModel.updatedJob) { | ||
| FirebaseAnalyticsUtil.reportCccApiDeliveryProgress(true,"Started a delivery job") |
There was a problem hiding this comment.
All event parameters should be underscored constant codes rather than a free text (start_delivery, end_delivery etc)
| dialog.setNegativeButton(activity.getString(R.string.login_link_connectid_no), (d, w) -> { | ||
| activity.dismissAlertDialog(); | ||
| ConnectAppDatabaseUtil.storeApp(activity, linkedApp); | ||
| FirebaseAnalyticsUtil.reportPersonalIDLinking(linkedApp.getAppId(),false); |
There was a problem hiding this comment.
we should also send the info on whether the user pushed negative button to cause a failure here or it was a connect unlock failure, As such I suggest that we send a string valued parameter with these values instead of a boolean - success, failure_user_denied, failure_unlock_failed
| } | ||
|
|
||
| private void handleBackupCodeSubmission() { | ||
| FirebaseAnalyticsUtil.reportPersonalIDContinueClicked(SCREEN_PERSONAL_ID_BACKUP_CODE_FRAGMENT,""); |
There was a problem hiding this comment.
lets be consistent and pass null instead when the extra info is not required
| } | ||
|
|
||
| private void onFingerprintButtonClicked() { | ||
| FirebaseAnalyticsUtil.reportPersonalIDContinueClicked("PersonalIdBiometricConfigFragment","fingerprint clicked"); |
There was a problem hiding this comment.
parameters should not be freetexts
| public static void reportPersonalIDContinueClicked(String screenName,@Nullable String info) { | ||
| Bundle params = new Bundle(); | ||
| params.putString(FirebaseAnalytics.Param.SCREEN_NAME, screenName); | ||
| params.putString(CCAnalyticsParam.PERSONAL_ID_CONTINUE_CLICKED_INFO, Objects.toString(info, "")); |
There was a problem hiding this comment.
should only log this if info is not null.
| } | ||
|
|
||
| private void onContinueClicked() { | ||
| FirebaseAnalyticsUtil.reportPersonalIDContinueClicked("PersonalIdPhoneFragment",null); |
There was a problem hiding this comment.
we should not hard code class names here but should get them from Java Class APIs
…671-analytic-user-funneling
| } | ||
|
|
||
| FirebaseAnalyticsUtil.reportCccApiDeliveryProgress(true) | ||
| FirebaseAnalyticsUtil.reportCccApiDeliveryProgress(true,null) |
There was a problem hiding this comment.
this would cause duplicate logs if any of the other reportCccApiDeliveryProgress methods above have already got fired.
| dialog.setNegativeButton(activity.getString(R.string.login_link_connectid_no), (d, w) -> { | ||
| activity.dismissAlertDialog(); | ||
| ConnectAppDatabaseUtil.storeApp(activity, linkedApp); | ||
| FirebaseAnalyticsUtil.reportPersonalIDLinking(linkedApp.getAppId(),"failure_user_denied"); |
There was a problem hiding this comment.
can we use constants for the parameter values (look at AnalyticsParamValue) , that helps us generate easy AI docs on analytics logging
| } | ||
|
|
||
| private void onFingerprintButtonClicked() { | ||
| FirebaseAnalyticsUtil.reportPersonalIDContinueClicked(PersonalIdBiometricConfigFragment.class.getSimpleName(),"continue_with_fingerprint"); |
There was a problem hiding this comment.
use constants for param values
| static final String PERSONAL_ID_CONTINUE_CLICKED = "PERSONAL_ID_CONTINUE_CLICKED"; | ||
| static final String PERSONAL_ID_MESSAGE_SENT = "PERSONAL_ID_MESSAGE_SENT"; | ||
| static final String PERSONAL_ID_LINKING = "PERSONAL_ID_LINKING"; |
There was a problem hiding this comment.
values should be small case.
There was a problem hiding this comment.
@shubham1g5 I have changed it as per jignesh comment here. Do you want me to make it small case?
| } | ||
|
|
||
| private void onContinueClicked() { | ||
| FirebaseAnalyticsUtil.reportPersonalIDContinueClicked(PersonalIdPhoneFragment.class.getSimpleName(),null); |
There was a problem hiding this comment.
you should instead use this.getClass().getSimpleName() so that we make sure that we are referencing the current class only.
…g' into CCCT-1671-analytic-user-funneling
| } | ||
|
|
||
| FirebaseAnalyticsUtil.reportCccApiDeliveryProgress(true) | ||
| if (events.isEmpty()) events.add(null) |
|
@jaypanchal-13 PR looks ok to me, only a minor change requested |
|
@damagatchi retest this please |
Product Description
Ticket -> https://dimagi.atlassian.net/browse/CCCT-1671
Added GA for user
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review