CCCT-2447 Add Firebase Analytics For Add-Email Flows#3745
Conversation
Instrument the PersonalID add-email flows (signup / recovery / existing-user) with Firebase analytics: - Email OTP request/verify events via reportOtpEvent, with new OTP_EVENT_TYPE_REQUEST_EMAIL / VERIFY_EMAIL event types, an OTP_METHOD_EMAIL method, and new OtpOp enum values. - Request/verify reporting centralized in EmailHelper so both the email-entry and OTP-verification fragments are covered without duplication. - Continue/skip events via reportPersonalIDContinueClicked for the existing-user email offer prompt and the proceed-without-email path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Suggested Review Order
|
Jignesh-dimagi
requested review from
OrangeAndGreen,
conroy-ricketts and
shubham1g5
and removed request for
OrangeAndGreen and
shubham1g5
📝 WalkthroughWalkthroughThis PR adds analytics instrumentation to email OTP flows in CommCare. It introduces new analytics constants for email OTP methods and event types, extends the OtpAnalyticsMapper enum with REQUEST_EMAIL and VERIFY_EMAIL variants, and implements a reportEmailOtpEvent helper in FirebaseAnalyticsUtil. EmailHelper now reports success/failure analytics for both email OTP request and verification operations, capturing outcome, failure reason, and failed attempt counts. EmailOfferHelper reports button click events for the email offer dialog. PersonalIdEmailVerificationFragment wires the failedAttempts counter through the verification flow and reports a "proceed without email" analytics event. Tests are updated to accommodate the new failedAttempts parameter and verify the new email event type mappings. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3745 +/- ##
============================================
+ Coverage 25.87% 25.94% +0.06%
- Complexity 4389 4405 +16
============================================
Files 950 951 +1
Lines 57186 57328 +142
Branches 6810 6821 +11
============================================
+ Hits 14799 14875 +76
- Misses 40558 40623 +65
- Partials 1829 1830 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| activity.getString(R.string.personalid_email_offer_message), | ||
| ) | ||
| dialog.setPositiveButton(activity.getString(R.string.personalid_email_offer_yes)) { _, _ -> | ||
| FirebaseAnalyticsUtil.reportPersonalIDContinueClicked(EMAIL_OFFER_SCREEN, null) |
There was a problem hiding this comment.
reportPersonalIDContinueClicked should only be used for Continue buttons on the Personal ID Sign up pages, I am afraid this will interfere with the existing funnel setup otherwise.
I would suggest logging this into a new event instead - email_offer_dialog and use it for both the email offer dialog and having trouble with email dialog.
There was a problem hiding this comment.
To stay within Firebase event limits, we reuse a single "continue button clicked" event. Depending on the user's action, it logs the following payload:
- Current Screen Name as SCREEN_NAME and null as PERSONAL_ID_CONTINUE_CLICKED_INFO when the user taps continue.
- Current Screen Name as SCREEN_NAME and 'skip' as PERSONAL_ID_CONTINUE_CLICKED_INFO when the user taps skip.
- 'EmailOfferDialog' as SCREEN_NAME and null as PERSONAL_ID_CONTINUE_CLICKED_INFO when an existing user chooses to add an email from the prompt.
- 'EmailOfferDialog' as SCREEN_NAME and 'skip' as PERSONAL_ID_CONTINUE_CLICKED_INFO when an existing user chooses to skip adding an email from the prompt.
- Current Screen Name as SCREEN_NAME and 'proceed_without_email' as PERSONAL_ID_CONTINUE_CLICKED_INFO when the user taps Proceed without email on the dialog.
Your thoughts?
There was a problem hiding this comment.
it definitely come across as weird to me to use "Continue" button event to log "non" continue like events. Also this event is dedicated to track the Personal ID signup flow at the moment, I would highly encourage using a different event here on dialog confirmation CTAs otherwise it might be a mess to write a query tracking Personal ID funnel (for eg. when a continue button event appears in mid of a non-signup workflow.
Curious if @OrangeAndGreen have thoughts on this from writing a query perspective ?
Otherwise I would say that we should just add another generalised event here to track the email feature and log it with that.
There was a problem hiding this comment.
As far as querying goes, specifically related to the PersonalID funnel, I don't think this would cause issues. We aren't relying on this event in the funnel right now, and even if we start to we would pair it with the screen_name to know what page the user clicked Continue on, so the proposed new events could be easily filtered out since they wouldn't come from a page in the configuration workflow.
All in all I'm a bit torn. On the one side I can see Shubham's point that the usage of the event seems odd here, particularly in the "skip" case. But on the other hand I know we want to limit our usage of event_names in order to avoid the Firebase limit, so as long as we can reliably separate the data in queries then the proposed approach makes sense.
In the end, I think we're better off using a new event_name for these events. We have plenty of capacity for additional event_names, so while generally we should avoid creating new ones I think in this case it would be okay. When I think about it in terms of "Will we ever want to track these events together?" the answer seems to be no, i.e. the events for tracking Continue presses in the configuration workflow vs. other parts of the app would never be needed at the same time. So if the events are distinct that way, I think it makes sense for them to have different names.
There was a problem hiding this comment.
Thank you for your inputs @OrangeAndGreen @shubham1g5
I have added a new event, user_prompt. It is currently used in two places: email verification failure retries and existing user email reminders. However, because it is generic, it can also be used wherever a user's consent or decision is required in a prompt. One upcoming use case is in the profile management section, where a prompt will ask to send an OTP to an email address.
Changes done here
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
Looks great! Just had a few things to add
| /** | ||
| * Sends an email OTP request | ||
| */ | ||
| fun sendEmailOtp( |
There was a problem hiding this comment.
I know this function was created in a different PR, but can we rename this to requestEmailOtp()
There was a problem hiding this comment.
All API handlers follow a naming convention that directs the server to take action. For example, "send" is named from the server's perspective—meaning the server sends the OTP to the user's email. Client methods simply pass the endpoint name up through every layer unchanged. Your thoughts?
There was a problem hiding this comment.
Good point although think that might be true for PersonalIdApiHandler methods but not for it's callers.
There was a problem hiding this comment.
Yeah that is a good point
To play devil's advocate, the caller of PersonalIdApiHandler.sendPhoneOtp() uses the terminology "requestOtp" here, and that terminology is then reused going up through the OtpManager here, so there is an inconsistency there
But, definitely downgrading this point to a nit, especially because the Kdoc comment would help clear any potential confusion anyways
I'm cool with wherever you take this 👍🏾
…and failed attempts.
There was a problem hiding this comment.
This seems like a bit of over-engineering for a couple of counters. Thoughts on moving the counter variables to EmailHelper directly?
There was a problem hiding this comment.
Since 'EmailHelper' is stateless, it cannot hold that data. The only remaining option was to provide the tracker from the source consuming these APIs. Now, it will work across all workflows: personal ID configuration, email offer prompt, and manage profile.
There was a problem hiding this comment.
That makes sense to me. I'm cool with keeping this as-is
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Changing my review to non-blocking.
Jignesh-dimagi
dismissed stale reviews from conroy-ricketts and shubham1g5
via
cf8f6ab
| * {@link OtpAnalyticsMapper#workflowParam}. Logged to | ||
| * {@link CCAnalyticsParam#EMAIL_OTP_WORKFLOW}. | ||
| */ | ||
| public static void reportEmailOtpEvent(OtpAnalyticsMapper.OtpOp op, |
There was a problem hiding this comment.
This function along with reportPhoneOtpEvent feel like they could use some cleanup:
- Both usages rely on OtpAnalyticsMapper.getEventType to convert the OtpOp to a string, but the phone version relies on the fragment to call the helper (passing a string to analytics) while the email version passes the OtpOp to analytics and lets it call the helper. I think it makes sense for both version to pass the enum in and have analytics always be the one to call the helper function.
- Generally (and after making the change above), I see a lot of overlap between the two functions, with the only real difference being that one logs a couple extra params (failed attempts count and email workflow). I'm thinking these could share a common base function: reportOtpEvent
- Also to consider: Should the phone OTP workflow also log failed attempts? Maybe that's something we just didn't consider when implementing the phone version, and could be a nice improvement
- Similarly, I think we could change EMAIL_OTP_WORKFLOW to just OTP_WORKFLOW, and for now hard-code REGISTRATION for phone-related calls since that's the only time a user does the phone verification. If/when we support allowing users to change their phone number, this code will be ready to support the analytics for the new workflow.
There was a problem hiding this comment.
Alright, I have updated the phone OTP logging to match the email OTP logging, which tracks the number of times the OTP is sent and the number of times verification fails. Additionally, I have created a common function to report these metrics.
Change done here
| reportEvent(CCAnalyticsEvent.PERSONAL_ID_CONTINUE_CLICKED, params); | ||
| } | ||
|
|
||
| public static void reportUserPromptEvent(String type, String action, @Nullable String info) { |
There was a problem hiding this comment.
Nit: the info can be non-nullable (no one ever passes null in)
| getString(R.string.personalid_email_otp_failed_message), | ||
| ) | ||
| dialog.setPositiveButton(getString(R.string.personalid_email_otp_failed_retry)) { _, _ -> | ||
| FirebaseAnalyticsUtil.reportUserPromptEvent( |
There was a problem hiding this comment.
Nit: Looking at these calls to reportUserPrompt, the ordering of the inputs feels odd. Seems like the "info" parameter would make more sense coming before the action, since it's kind of a sub-category of the workflow.
Like:
- Email workflow, retrying after failure, user chooses to try again
- Email workflow, retrying after failure, user chooses to proceed without email
| public static final String EMAIL_OTP_WORKFLOW = "workflow"; | ||
|
|
||
| // Param keys for user prompt analytics | ||
| static final String USER_PROMPT_TYPE = "type"; |
There was a problem hiding this comment.
Can we make this string "event_type"? I'll start a convo with the team about this, but there's some benefit to re-using strings for similar types of fields across different events. We already have several similar parameters that map to "event_type", so making this new one do the same would be nice.
| <argument | ||
| android:name="emailOtpRequestCount" | ||
| app:argType="integer" | ||
| android:defaultValue="0" /> |
There was a problem hiding this comment.
Curious why this argument is optional? I see it means we have to set it separately in PersonalIdEmailFragment (rather than including it with the other two args), and it doesn't seem like there's ever a case where we wouldn't pass the arg in
There was a problem hiding this comment.
I was initially thinking of keeping it decoupled, but I agree with you that the user should have tried at least once before coming to this screen
Change done here
| * - [requestCount]: total email OTP send/request calls this session. | ||
| * - [failedAttempts]: absolute number of failed email OTP verifications this session. | ||
| */ | ||
| class EmailOtpAttemptTracker( |
There was a problem hiding this comment.
Nit: This could just be AttemptTracker, it doesn't need to be OTP and/or email specific.
4 participants
CCCT-2447
Technical Summary
Instruments the PersonalID add-email flows (signup / recovery / existing-user) with Firebase analytics. OTP request/verify reporting is centralized in
EmailHelper.sendEmailOtp/verifyEmailOtp; theattemptscount is threaded intoverifyEmailOtpas a parameter. New email-specific OTP event types/method (OTP_EVENT_TYPE_REQUEST_EMAIL/VERIFY_EMAIL,OTP_METHOD_EMAIL) andOtpOpenum values reuse the existingreportOtpEventpath. Continue/skip events usereportPersonalIDContinueClickedfor the existing-user offer prompt and the proceed-without-email path.Safety Assurance
Safety story
What gives me confidence:
OtpAnalyticsMappercases for the new email event types and the newEmailHelperemission tests (success/failure for both request and verify) all pass.EmailHelper), reducing the chance of missed or duplicated events.Automated test coverage
OtpAnalyticsMapperTest: new cases assertingREQUEST_EMAIL→request_emailandVERIFY_EMAIL→verify_email.EmailHelperTest: new cases that capture the API callback and assertreportEmailOtpEventis emitted with the correct op/outcome/reason/attempts on success and failure for bothsendEmailOtpandverifyEmailOtp(including the threadedfailedAttemptsvalue).