CCCT-1840 Resend OTP With Fallback

[conroy-ricketts]

CCCT-1840

Technical Summary

I implemented a fallback to Twilio (via Personal ID) when the user reattempts to send the OTP code the first time. See the ticket linked above for more details. There are no user-visible changes for this ticket.

I feel like I'm still getting familiar with this area of the code, so let me know if I should approach this ticket from a different angle and I'd be happy to make that adjustment!

Safety Assurance

Safety story

I verified that my changes work by following these testing steps:

1. Going through the Personal ID signup flow, verified that the very first OTP code sent to my phone number was from Firebase and that the OTP code was successfully verified in the app.
2. Going through the Personal ID signup flow, verified that the second OTP code sent to my phone number (after tapping on the "Resend Code" button exactly once) was from Twilio and that the OTP code was successfully verified in the app.
3. Going through the Personal ID signup flow, verified that the third OTP code sent to my phone number (after tapping on the "Resend Code" button exactly twice) was from Firebase and that the OTP code was successfully verified in the app.
4. Went over to BigQuery and verified that the new otp_requested_event Firebase analytics event was being logged properly and appropriately for my test device's ID.

QA Plan

QA should probably follow the same steps I took but skip the last step. So here is the test plan for QA:

1. Going through the Personal ID signup flow, verify that the very first OTP code sent to your phone number was from Firebase and that the OTP code was successfully verified in the app. (you'll know it's from Firebase because the SMS message has different wording compared to Twilio!)
2. Going through the Personal ID signup flow, verify that the second OTP code sent to your phone number (after tapping on the "Resend Code" button exactly once) was from Twilio and that the OTP code was successfully verified in the app.
3. Going through the Personal ID signup flow, verify that the third OTP code sent to your phone number (after tapping on the "Resend Code" button exactly twice) was from Firebase and that the OTP code was successfully verified in the app.

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request adds OTP reattempt tracking and analytics reporting to the CommCare Android Personal ID verification flow. The changes include adding an otpReattempts property to PersonalIdSessionData to track resend attempts, implementing logic in PersonalIdPhoneVerificationFragment to switch SMS method providers based on reattempt count (defaulting to Twilio initially, then switching to PersonalID on first reattempt), and introducing new analytics constants and a reporting method to track OTP request events with reattempt metadata.

Sequence Diagram

sequenceDiagram
    participant User
    participant Fragment as PersonalIdPhoneVerificationFragment
    participant SessionData as PersonalIdSessionData
    participant OtpManager
    participant Analytics as FirebaseAnalyticsUtil

    User->>Fragment: Request OTP (initial or resend)
    Fragment->>SessionData: Get current otpReattempts
    alt First attempt (reattempts = 0)
        Fragment->>OtpManager: Initialize with Twilio (default)
    else Reattempt (reattempts > 0)
        Fragment->>Fragment: setupOtpManagerForReattempt()
        Fragment->>OtpManager: Reinitialize with SMS_METHOD_PERSONAL_ID
    end
    Fragment->>OtpManager: Request OTP
    Fragment->>SessionData: Increment otpReattempts
    Fragment->>Analytics: reportOtpRequested(reattemptCount)
    Analytics->>Analytics: Log OTP_REQUESTED_EVENT with OTP_REATTEMPTS param

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• OtpManager switching logic in PersonalIdPhoneVerificationFragment: Verify that setupOtpManagerForReattempt() correctly transitions between Twilio and PersonalID SMS methods, and that manager reinitialization doesn't cause lifecycle issues.
• Reattempt counter consistency: Ensure otpReattempts is incremented at the correct points and persists across the session lifecycle.
• Analytics event integration: Confirm that OTP_REQUESTED_EVENT and OTP_REATTEMPTS parameter names are consistent with backend expectations and that the reporting method is called at appropriate points in the flow.
• @DeviceAuthType annotation placement: Verify that the annotation on requiredLock is correctly interpreted by the authentication validation logic.

Possibly related PRs

• Fallback to PersonalID from Firebase Otp auth in case of Errors #3334: Modifies PersonalIdPhoneVerificationFragment and PersonalIdSessionData to change OTP routing between Firebase and PersonalID based on error conditions; directly overlaps with the reattempt-based switching introduced here.
• Backmerge Commcare 2.57 #3177: Contains extensive refactors to PersonalId phone verification, OtpManager/FirebaseAuthService initialization, and OTP handling types that interact with the reattempt logic and SMS method switching.
• Ccct 1159 send request connectid verify firebase otp #3125: Main PR building on OTP handling in PersonalIdPhoneVerificationFragment and modifies OtpManager usage and Firebase OTP flow similar to this PR's resend flow updates.

Suggested reviewers

• OrangeAndGreen
• shubham1g5
• Jignesh-dimagi
• jaypanchal-13

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 5.26% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately reflects the main technical change: implementing OTP resend functionality with a fallback mechanism.
Description check	✅ Passed	The description includes most required template sections: Product Description (marked as no visible changes), Technical Summary with ticket link, Safety Assurance with detailed testing steps, and QA Plan. However, Feature Flag, Automated test coverage, and Labels/Review sections are missing.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch task/CCCT-1840-resend-OTP-with-fallback

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)

580-586: Consider improving plural formatting in analytics string.

The method correctly implements OTP request reporting and follows existing patterns. However, the format string "%d reattempt(s)" always uses "reattempt(s)" regardless of count (e.g., "1 reattempt(s)").

For better grammatical correctness, consider:

-                String.format("%d reattempt(s)", numberOfReattempts)
+                numberOfReattempts == 1 ? "1 reattempt" : String.format("%d reattempts", numberOfReattempts)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b900b75 and c47633c.

📒 Files selected for processing (5)

• app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt (1 hunks)
• app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (10 hunks)
• app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java (1 hunks)
• app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (1 hunks)
• app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (2 hunks)

🧰 Additional context used

🧠 Learnings (9)

📓 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.

📚 Learning: 2025-06-04T19:17:21.213Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:62-64
Timestamp: 2025-06-04T19:17:21.213Z
Learning: In ConnectUnlockFragment.java, the user prefers to let getArguments() potentially throw NullPointerException rather than adding null checks, as the arguments are required for proper navigation flow and their absence indicates a programming error that should fail fast.

Applied to files:

• app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java

📚 Learning: 2025-04-21T18:48:08.330Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3037
File: app/src/org/commcare/connect/ConnectConstants.java:11-15
Timestamp: 2025-04-21T18:48:08.330Z
Learning: Request codes used for startActivityForResult should be unique throughout the application, even if they're used in different activities. COMMCARE_SETUP_CONNECT_LAUNCH_REQUEST_CODE and STANDARD_HOME_CONNECT_LAUNCH_REQUEST_CODE should have different values.

Applied to files:

• app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java

📚 Learning: 2025-07-29T14:10:55.131Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:0-0
Timestamp: 2025-07-29T14:10:55.131Z
Learning: In ConnectUnlockFragment.java, opportunityId values are expected to always contain valid integer strings. Integer.parseInt() should be allowed to throw NumberFormatException if invalid data is encountered, following the fail-fast philosophy used throughout the Connect feature for data contract violations.

Applied to files:

• app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java

📚 Learning: 2025-01-21T17:29:58.014Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/fragments/connect/ConnectPaymentSetupFragment.java:61-66
Timestamp: 2025-01-21T17:29:58.014Z
Learning: In the CommCare Android app, for non-critical convenience features like phone number auto-population, exceptions should be logged but fail silently when there's a manual fallback available. This approach prevents app crashes while maintaining the ability to debug issues through logs.

Applied to files:

• app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java

📚 Learning: 2025-03-10T08:16:29.416Z

Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 2949
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:173-247
Timestamp: 2025-03-10T08:16:29.416Z
Learning: In the ConnectIdPasswordVerificationFragment, password comparisons should use MessageDigest.isEqual() rather than equals() to prevent timing attacks, and empty password validation should be implemented before verification attempts.

Applied to files:

• app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java

📚 Learning: 2025-04-21T15:02:17.492Z

Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 3042
File: app/src/org/commcare/fragments/BreadcrumbBarViewModel.java:50-55
Timestamp: 2025-04-21T15:02:17.492Z
Learning: ViewModels should not store View or Activity references as this can cause memory leaks. Unlike Fragments with setRetainInstance(true), ViewModels don't have automatic view detachment mechanisms. When migrating from Fragments to ViewModels, view references should be replaced with data-only state in the ViewModel.

Applied to files:

• app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java

📚 Learning: 2025-06-13T15:53:12.951Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/connect/network/PersonalIdApiHandler.java:51-56
Timestamp: 2025-06-13T15:53:12.951Z
Learning: For `PersonalIdApiHandler`, the team’s convention is to propagate `JSONException` as an unchecked `RuntimeException` so the app crashes, signalling a contract/implementation bug rather than attempting a graceful retry.

Applied to files:

• app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java

📚 Learning: 2025-05-22T14:26:41.341Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/fragments/SelectInstallModeFragment.java:201-205
Timestamp: 2025-05-22T14:26:41.341Z
Learning: In the Connect error handling flow of CommCare Android, error messages are shown once and then automatically cleared because the underlying error record gets deleted after being displayed. This is why there's no need for explicit methods to hide these messages.

Applied to files:

• app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java

🧬 Code graph analysis (2)

app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (2)

app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java (1)

• CCAnalyticsEvent (7-63)

app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (1)

• CCAnalyticsParam (7-48)

app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (5)

app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)

• FirebaseAnalyticsUtil (44-601)

app/src/org/commcare/utils/OtpManager.java (1)

• OtpManager (12-42)

app/src/org/commcare/utils/PersonalIDAuthService.kt (2)

• activity (10-44)
• requestOtp (16-26)

app/src/org/commcare/fragments/personalId/BasePersonalIdFragment.kt (3)

• handleCommonSignupFailures (10-47)
• handleCommonSignupFailures (12-27)
• navigateToMessageDisplay (40-46)

app/src/org/commcare/connect/network/connectId/PersonalIdApiErrorHandler.java (1)

• PersonalIdApiErrorHandler (26-70)

⏰ 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 (10)

app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java (1)

60-60: LGTM! Analytics event constant added correctly.

The new OTP_REQUESTED_EVENT constant follows existing naming conventions and is positioned appropriately among other Personal ID-related events.

app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt (2)

10-10: LGTM! Type-safety annotation added.

Adding the @DeviceAuthType annotation to requiredLock improves type safety by enforcing compile-time checks for valid authentication types.

---

27-27: LGTM! OTP reattempt tracking field added correctly.

The otpReattempts field appropriately tracks user-initiated resend attempts with a sensible default value of 0. The comment clearly explains its purpose.

app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)

9-11: LGTM! Required imports added.

These navigation-related imports are used by the existing getNavControllerPageChangeLoggingListener() method (lines 539-552) and should have been present.

app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (1)

43-43: LGTM! Analytics parameter constant added correctly.

The OTP_REATTEMPTS constant follows existing naming conventions and is appropriately used in the analytics reporting flow.

app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (5)

32-32: LGTM! Required imports added.

The imports for FirebaseAnalyticsUtil and SMS_METHOD_PERSONAL_ID are correctly added to support the new OTP analytics and fallback functionality.

Also applies to: 45-45

---

105-117: LGTM! Auto-switch logic correctly handles Firebase failures.

The automatic fallback to PersonalId SMS on non-recoverable Firebase errors is well-implemented. Correctly distinguishes between automatic failover (no reattempt increment) and user-initiated resends (tracked via otpReattempts).

---

183-186: LGTM! Resend flow correctly integrated with fallback logic.

The resend button now properly invokes setupOtpManagerForReattempt() before requesting a new OTP, enabling the fallback behavior described in the PR objectives.

---

296-301: LGTM! Analytics correctly integrated into OTP request flow.

The analytics call properly reports the current reattempt count after it's been incremented by setupOtpManagerForReattempt(), ensuring accurate tracking of user-initiated resend attempts.

---

367-387: LGTM! OTP fallback logic correctly implements the specified behavior.

The method correctly implements the fallback strategy described in the PR objectives:

• First resend (otpReattempts == 0): Switches to Twilio via Personal ID SMS method
• Subsequent resends: Returns to default method (Firebase from session data)

The implementation works by creating new OtpManager instances without persisting the SMS method change to personalIdSessionData, allowing subsequent resends to fall back to the original method.

[shubham1g5]

Left a suggestion but nothing blocking.

[shubham1g5]

think this can also be part of setupOtpManager to decide on allowedToFallback. The idea here is if the default method is already SMS_METHOD_PERSONAL_ID and then fall back logic is redundant as we are already using the fallback as default. Think we can now get rid of shouldAutoSwitchToPersonalIdAuth method and directly use errorType.isNonRecoverable() in it's place.

[conroy-ricketts]

I think this could work - I want to spend some more time testing this before I push changes for it