Beta hotfix backmerge #3218
Merged
Beta hotfix backmerge #3218
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughThis update introduces new localized string resources in English, French, Portuguese, Swahili, and Tigrinya for handling PersonalID configuration errors and recovery lockout scenarios. It enhances error handling in the PersonalID flow by mapping technical exceptions to user-friendly messages and updating callback interfaces for integrity token retrieval. The flow for confirming backup codes is modified to explicitly handle account lockout states, including new navigation and messaging logic. Additional changes include logging improvements, removal of unused code, and the introduction of a new constant for account lockout. No existing public APIs are removed, but several method signatures are updated for improved error reporting. Sequence Diagram(s)sequenceDiagram
participant User
participant PersonalIdPhoneFragment
participant IntegrityTokenApiRequestHelper
participant IntegrityTokenViewModel
participant Server
User->>PersonalIdPhoneFragment: Initiate PersonalID configuration
PersonalIdPhoneFragment->>IntegrityTokenApiRequestHelper: withIntegrityToken(requestBody, callback)
IntegrityTokenApiRequestHelper->>IntegrityTokenViewModel: requestIntegrityToken(requestHash, hasRetried, callback)
IntegrityTokenViewModel->>IntegrityTokenApiRequestHelper: onTokenReceived(token, requestHash) or onTokenFailure(exception)
alt Token received
IntegrityTokenApiRequestHelper->>PersonalIdPhoneFragment: onTokenReceived(token, requestHash)
PersonalIdPhoneFragment->>Server: makeStartConfigurationCall(token, requestHash, body)
Server-->>PersonalIdPhoneFragment: Success or Failure response
alt Failure with error code
PersonalIdPhoneFragment->>PersonalIdPhoneFragment: onConfigurationFailure(failureCause, failureMessage)
PersonalIdPhoneFragment->>UI: Show localized error message
end
else Token failure
IntegrityTokenApiRequestHelper->>PersonalIdPhoneFragment: onTokenFailure(exception)
PersonalIdPhoneFragment->>UI: Show mapped error message
end
sequenceDiagram
participant User
participant PersonalIdBackupCodeFragment
participant Server
User->>PersonalIdBackupCodeFragment: Enter backup code
PersonalIdBackupCodeFragment->>Server: Confirm backup code
Server-->>PersonalIdBackupCodeFragment: Response with error_code (e.g., LOCKED_ACCOUNT)
alt error_code == LOCKED_ACCOUNT
PersonalIdBackupCodeFragment->>PersonalIdBackupCodeFragment: handleAccountLockout()
PersonalIdBackupCodeFragment->>UI: Show lockout title and message
else error_code != LOCKED_ACCOUNT
alt Success
PersonalIdBackupCodeFragment->>UI: Proceed to next step
else Failure with attempts left
PersonalIdBackupCodeFragment->>UI: Show error, allow retry
end
end
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
app/res/values-pt/strings.xml (2)
451-454: Consistency with existing terminologyExisting strings almost always qualify the app store as “Google Play Store”.
Consider adding the “Google” qualifier here for consistency and to avoid confusion on devices that ship with alternative stores.-<string name="personalid_configuration_process_failed_play_services">Certifique-se de que a Play Store está disponível no seu dispositivo e está atualizada.</string> +<string name="personalid_configuration_process_failed_play_services">Certifique-se de que a Google Play Store está disponível no seu dispositivo e está atualizada.</string>
465-466: Maintain a consistent voice (“Você …”)Most of the Portuguese UI copy addresses the user explicitly with “Você …”.
Pre-pending the same here keeps the tone uniform.-<string name="personalid_recovery_lockout_message">Introduziu o Código de Backup errado muitas vezes e a sua conta foi bloqueada.</string> +<string name="personalid_recovery_lockout_message">Você introduziu o código de backup errado muitas vezes e a sua conta foi bloqueada.</string>app/res/values-sw/strings.xml (2)
451-454: Prefer localised store nameSwahili strings elsewhere use “Duka la Google Play”. Aligning the terminology avoids user confusion.
-<string name="personalid_configuration_process_failed_play_services">Tafadhali hakikisha kuwa Play Store inapatikana kwenye kifaa chako na imesasishwa.</string> +<string name="personalid_configuration_process_failed_play_services">Tafadhali hakikisha kuwa Duka la Google Play linapatikana kwenye kifaa chako na limesasishwa.</string>
465-466: Remove duplicated noun“Nambari ya Hifadhi Nambari” repeats the word Nambari. Use Nakala (backup) instead.
-<string name="personalid_recovery_lockout_message">Umeweka Nambari ya Hifadhi Nambari isiyo sahihi mara nyingi sana na akaunti yako imefungwa.</string> +<string name="personalid_recovery_lockout_message">Umeweka Nambari ya Hifadhi Nakala isiyo sahihi mara nyingi sana na akaunti yako imefungwa.</string>app/res/values-ti/strings.xml (3)
437-440: Minor wording / typo fixes
- “ጸገኒ መረብ” → “ጸገይ መረብ”
- Use a semicolon or “እና” between the two clauses for readability.
-<string name="personalid_configuration_process_failed_network_error">ጸገኒ መረብ ኣብ ምርግጋን ብትኽክል መሳርሒ እንተሃለወ። እባኮም ዝሓለፈ መርበብ ክርከቡ እንተኾነ ኣረጋግጹ እና ደጊሞ ፈትኑ።</string> +<string name="personalid_configuration_process_failed_network_error">ጸገይ መረብ ኣብ ምርግጋን ብትኽክል መሳርሒ እንተሃለወ፤ እባኮም መርበብኩም ዝሓለፈ እንተሎ ኣረጋግጹ እና ደጊሞ ፈትኑ።</string>
449-451: Clarify backup-code wordingUsing the original English phrase “Backup Code” inside the Tigrinya sentence can be jarring.
If possible, substitute with a fully localised term (e.g., “ናይ ምሕዋይ ኮድ”) which is already used elsewhere.-<string name="personalid_recovery_lockout_title">ኣካውንት ተዓጽዩ።</string> +<string name="personalid_recovery_lockout_title">ኣካውንት ተዓጽዩ</string>(Full-stop removed to match the rest of the titles.)
451-451: Full-stop duplicationThe sentence ends with “።” but most messages omit terminal punctuation.
Dropping it keeps the style consistent.-<string name="personalid_recovery_lockout_message">ጌጋ Backup Code ብዙሕ ግዜ ኣእቲኻ ኣካውንትካ ተዓጽዩ ኣሎ።</string> +<string name="personalid_recovery_lockout_message">ጌጋ Backup Code ብዙሕ ግዜ ኣእቲኻ፣ ኣካውንትካ ተዓጽዩ ኣሎ</string>app/src/org/commcare/android/integrity/IntegrityTokenApiRequestHelper.kt (1)
78-114: Consider makinggetErrorForExceptiona static method.Since this method doesn't access any instance fields, it could be made static for better reusability and testability.
- fun getErrorForException(context: Context, exception: Exception): String { + companion object { + @JvmStatic + fun getErrorForException(context: Context, exception: Exception): String {The error mapping logic is comprehensive and the crash-fast approach for configuration errors aligns with the codebase conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/res/values-fr/strings.xml(2 hunks)app/res/values-pt/strings.xml(2 hunks)app/res/values-sw/strings.xml(2 hunks)app/res/values-ti/strings.xml(2 hunks)app/res/values/strings.xml(2 hunks)app/src/org/commcare/android/integrity/IntegrityTokenApiRequestHelper.kt(5 hunks)app/src/org/commcare/android/integrity/IntegrityTokenViewModel.kt(2 hunks)app/src/org/commcare/connect/ConnectConstants.java(1 hunks)app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java(2 hunks)app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(8 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java(0 hunks)app/src/org/commcare/utils/FirebaseAuthService.java(2 hunks)
💤 Files with no reviewable changes (1)
- app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java
🧰 Additional context used
🧠 Learnings (14)
📓 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.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/res/values-ti/strings.xml:350-350
Timestamp: 2025-05-22T14:32:53.133Z
Learning: PersonalID and Connect features haven't been translated to Spanish, Lithuanian, or Norwegian yet, so users with those language settings see the English strings by default.
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#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.
app/src/org/commcare/connect/ConnectConstants.java (3)
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.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:66-71
Timestamp: 2025-01-21T17:28:09.007Z
Learning: The ConnectUserRecord class in CommCare Android uses @Persisting annotations with sequential indices for field persistence, and @MetaField annotations for fields that have corresponding META constants. Fields should be documented with Javadoc comments explaining their purpose, format requirements, and relationships with other fields.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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.
app/src/org/commcare/utils/FirebaseAuthService.java (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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.
app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java (3)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:206-213
Timestamp: 2025-01-21T18:20:43.883Z
Learning: In the Connect ID feature, server responses containing user data should be parsed using ConnectUserResponseParser to ensure consistent handling of payment information (owner_name and phone_number) and secondary phone verification across all authentication flows.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java:244-275
Timestamp: 2025-02-04T21:22:56.537Z
Learning: Direct JSONObject parsing is acceptable for handling user data responses in ConnectIdPinFragment, as decided by the team. No need to enforce ConnectUserResponseParser usage in this context.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/connect/network/SsoToken.java:0-0
Timestamp: 2025-01-23T21:36:19.292Z
Learning: When handling SSO tokens in Java:
1. Always specify UTF-8 encoding when converting bytes to String
2. Handle potential integer overflow in expiration time calculations
3. Validate token format (e.g., JWT structure)
4. Ensure proper resource cleanup using try-finally blocks
app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java (12)
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3093
File: app/res/navigation/nav_graph_connect_messaging.xml:41-45
Timestamp: 2025-05-09T10:57:41.073Z
Learning: In the CommCare Android codebase, the navigation graph for Connect messaging (`nav_graph_connect_messaging.xml`) intentionally uses `channel_id` as the argument name in the connectMessageFragment, despite using `channelId` in other parts of the same navigation graph. This naming difference is by design in the refactored code.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/src/org/commcare/activities/CommCareSetupActivity.java:360-364
Timestamp: 2025-05-22T14:28:35.959Z
Learning: In CommCareSetupActivity.java, the call to installFragment.showConnectErrorMessage() after fragment transactions is intentionally unguarded with null checks. This follows the app's design pattern where critical error paths prefer immediate crashes over silent failures, making potential issues immediately visible during development rather than hiding them with defensive programming.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java:235-236
Timestamp: 2025-02-04T21:29:29.594Z
Learning: The empty performPasswordUnlock method in ConnectIdBiometricConfigFragment is intentionally left empty and should not be flagged in reviews.
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.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3108
File: app/src/org/commcare/models/connect/ConnectLoginJobListModel.java:79-92
Timestamp: 2025-06-20T15:51:14.157Z
Learning: The ConnectLoginJobListModel class in app/src/org/commcare/models/connect/ConnectLoginJobListModel.java does not need to implement Parcelable interface as it is not passed between Android activities or fragments.
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.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/src/org/commcare/fragments/SelectInstallModeFragment.java:201-205
Timestamp: 2025-05-22T14:26:41.341Z
Learning: In SelectInstallModeFragment.java, the showConnectErrorMessage method intentionally omits null checks because it's called at a specific point in the startup flow where UI is guaranteed to be loaded. It's designed to crash if activity or view is null to make potential issues immediately visible rather than hiding them with defensive programming.
Learnt from: shubham1g5
PR: dimagi/commcare-android#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.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3108
File: app/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java:74-78
Timestamp: 2025-06-06T19:54:26.428Z
Learning: In ConnectDownloadingFragment.java and similar Connect-related code, the team prefers to let "should never happen" scenarios like null app records crash rather than add defensive null checks, following a fail-fast philosophy to catch programming errors during development.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java:244-275
Timestamp: 2025-02-04T21:22:56.537Z
Learning: Direct JSONObject parsing is acceptable for handling user data responses in ConnectIdPinFragment, as decided by the team. No need to enforce ConnectUserResponseParser usage in this context.
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (5)
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java:235-236
Timestamp: 2025-02-04T21:29:29.594Z
Learning: The empty performPasswordUnlock method in ConnectIdBiometricConfigFragment is intentionally left empty and should not be flagged in reviews.
Learnt from: shubham1g5
PR: dimagi/commcare-android#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.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/src/org/commcare/activities/CommCareSetupActivity.java:360-364
Timestamp: 2025-05-22T14:28:35.959Z
Learning: In CommCareSetupActivity.java, the call to installFragment.showConnectErrorMessage() after fragment transactions is intentionally unguarded with null checks. This follows the app's design pattern where critical error paths prefer immediate crashes over silent failures, making potential issues immediately visible during development rather than hiding them with defensive programming.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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.
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.
app/res/values-pt/strings.xml (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/res/values-ti/strings.xml:350-350
Timestamp: 2025-05-22T14:32:53.133Z
Learning: PersonalID and Connect features haven't been translated to Spanish, Lithuanian, or Norwegian yet, so users with those language settings see the English strings by default.
app/res/values/strings.xml (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/res/values-ti/strings.xml:350-350
Timestamp: 2025-05-22T14:32:53.133Z
Learning: PersonalID and Connect features haven't been translated to Spanish, Lithuanian, or Norwegian yet, so users with those language settings see the English strings by default.
app/res/values-sw/strings.xml (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/res/values-ti/strings.xml:350-350
Timestamp: 2025-05-22T14:32:53.133Z
Learning: PersonalID and Connect features haven't been translated to Spanish, Lithuanian, or Norwegian yet, so users with those language settings see the English strings by default.
app/res/values-ti/strings.xml (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/res/values-ti/strings.xml:350-350
Timestamp: 2025-05-22T14:32:53.133Z
Learning: PersonalID and Connect features haven't been translated to Spanish, Lithuanian, or Norwegian yet, so users with those language settings see the English strings by default.
app/res/values-fr/strings.xml (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/res/values-ti/strings.xml:350-350
Timestamp: 2025-05-22T14:32:53.133Z
Learning: PersonalID and Connect features haven't been translated to Spanish, Lithuanian, or Norwegian yet, so users with those language settings see the English strings by default.
app/src/org/commcare/android/integrity/IntegrityTokenApiRequestHelper.kt (3)
Learnt from: shubham1g5
PR: dimagi/commcare-android#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.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:81-89
Timestamp: 2025-01-21T17:27:58.754Z
Learning: In the CommCare Android codebase, use org.jetbrains.annotations.NotNull for null-safety annotations. Empty strings are acceptable for unspecified values, but null values should be prevented using @NotNull.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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.
app/src/org/commcare/android/integrity/IntegrityTokenViewModel.kt (1)
Learnt from: shubham1g5
PR: dimagi/commcare-android#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.
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (21)
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3093
File: app/res/navigation/nav_graph_connect_messaging.xml:41-45
Timestamp: 2025-05-09T10:57:41.073Z
Learning: In the CommCare Android codebase, the navigation graph for Connect messaging (`nav_graph_connect_messaging.xml`) intentionally uses `channel_id` as the argument name in the connectMessageFragment, despite using `channelId` in other parts of the same navigation graph. This naming difference is by design in the refactored code.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/src/org/commcare/activities/CommCareSetupActivity.java:360-364
Timestamp: 2025-05-22T14:28:35.959Z
Learning: In CommCareSetupActivity.java, the call to installFragment.showConnectErrorMessage() after fragment transactions is intentionally unguarded with null checks. This follows the app's design pattern where critical error paths prefer immediate crashes over silent failures, making potential issues immediately visible during development rather than hiding them with defensive programming.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java:235-236
Timestamp: 2025-02-04T21:29:29.594Z
Learning: The empty performPasswordUnlock method in ConnectIdBiometricConfigFragment is intentionally left empty and should not be flagged in reviews.
Learnt from: shubham1g5
PR: dimagi/commcare-android#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.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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.
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.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/src/org/commcare/fragments/SelectInstallModeFragment.java:201-205
Timestamp: 2025-05-22T14:26:41.341Z
Learning: In SelectInstallModeFragment.java, the showConnectErrorMessage method intentionally omits null checks because it's called at a specific point in the startup flow where UI is guaranteed to be loaded. It's designed to crash if activity or view is null to make potential issues immediately visible rather than hiding them with defensive programming.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java:244-275
Timestamp: 2025-02-04T21:22:56.537Z
Learning: Direct JSONObject parsing is acceptable for handling user data responses in ConnectIdPinFragment, as decided by the team. No need to enforce ConnectUserResponseParser usage in this context.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:214-215
Timestamp: 2025-01-21T18:19:05.799Z
Learning: In ConnectIdPasswordVerificationFragment, when creating a ConnectUserRecord, it's acceptable for payment information (paymentName and paymentPhone) to be empty strings if the server response doesn't include payment info in the CONNECT_PAYMENT_INFO field.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2927
File: app/src/org/commcare/activities/connect/HQUserInviteActivity.java:0-0
Timestamp: 2025-02-04T15:29:12.993Z
Learning: In HQUserInviteActivity, parameters from Intent data (domain, inviteCode, username, etc.) are required and the code is designed to fail immediately if they're missing, rather than adding null checks. This helps catch integration issues early.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:81-89
Timestamp: 2025-01-21T17:27:58.754Z
Learning: In the CommCare Android codebase, use org.jetbrains.annotations.NotNull for null-safety annotations. Empty strings are acceptable for unspecified values, but null values should be prevented using @NotNull.
Learnt from: shubham1g5
PR: dimagi/commcare-android#2955
File: app/src/org/commcare/models/database/user/models/CommCareEntityStorageCache.java:135-143
Timestamp: 2025-02-18T17:42:31.516Z
Learning: In CommCare Android, record IDs are expected to be numeric and should fail fast (crash) if a non-numeric ID is encountered, rather than handling the NumberFormatException.
Learnt from: shubham1g5
PR: dimagi/commcare-android#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.
Learnt from: shubham1g5
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java:58-59
Timestamp: 2025-03-10T08:16:59.436Z
Learning: All fragments using view binding should implement proper cleanup in onDestroyView() by setting binding to null to prevent memory leaks.
Learnt from: shubham1g5
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java:58-59
Timestamp: 2025-03-10T08:16:59.436Z
Learning: All fragments using view binding should implement proper cleanup in onDestroyView() by setting binding to null to prevent memory leaks.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3037
File: app/src/org/commcare/connect/ConnectIDManager.java:233-243
Timestamp: 2025-04-22T15:48:29.346Z
Learning: Never instantiate Android Activity classes directly with 'new' as it bypasses the Android component lifecycle. Activities should only be created by the system through Intents. Non-static instance fields don't need manual resetting as they'll be initialized with their default values when a new activity instance is properly created.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3037
File: app/src/org/commcare/connect/ConnectIDManager.java:233-243
Timestamp: 2025-04-22T15:48:29.346Z
Learning: Never instantiate Android Activity classes directly with 'new'. Activities should only be created through the Android framework using Intents.
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.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/connect/network/connectId/ApiService.java:8-63
Timestamp: 2025-01-28T09:38:59.882Z
Learning: The ConnectID API service methods should use Map<String, String> for request bodies and ResponseBody for responses, as per team preference.
🧬 Code Graph Analysis (1)
app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java (1)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-55)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (18)
app/src/org/commcare/utils/FirebaseAuthService.java (2)
20-20: LGTM!The import addition is necessary for the logging functionality and follows proper Java import conventions.
101-103: Excellent improvement for error diagnostics.The exception logging addition is well-implemented:
- Preserves the existing error handling flow by logging after callback invocation
- Includes appropriate null check to prevent potential NPE
- Uses descriptive log message for debugging purposes
- Aligns with the broader effort to improve error handling and diagnostics across authentication flows
This change enhances the application's ability to diagnose Firebase OTP verification issues without disrupting the user experience.
app/res/values/strings.xml (2)
602-605: LGTM! Well-structured error messages for PersonalID configuration failures.The new error strings provide clear, user-friendly messages for different device eligibility failure scenarios. The messages appropriately guide users on next steps (check Play Store, retry after network improvement, contact support).
615-615: Good addition of lockout messaging for account security.The lockout title and message strings appropriately communicate the account lockout state due to multiple incorrect backup code attempts, enhancing the user experience during security scenarios.
Also applies to: 617-617
app/src/org/commcare/connect/ConnectConstants.java (1)
54-54: LGTM! Properly follows the established constant pattern.The new
PERSONALID_RECOVERY_ACCOUNT_LOCKEDconstant correctly follows the sequential numbering pattern using thePERSONAL_ID_TASK_ID_OFFSETand maintains uniqueness.app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java (1)
26-28: LGTM! Proper JSON field parsing with null safety.The addition correctly checks for field existence using
has()before parsing the error code, following established JSON parsing patterns in the codebase.app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java (1)
138-141: LGTM! Appropriate case handling for account lockout scenario.The addition of the
PERSONALID_RECOVERY_ACCOUNT_LOCKEDcase correctly groups it withPERSONALID_DEVICE_CONFIGURATION_FAILED, both appropriately callingactivity.finish()to terminate the flow.app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (2)
205-207: LGTM! Robust lockout detection with case-insensitive comparison.The condition properly checks for the "LOCKED_ACCOUNT" error code using case-insensitive comparison, ensuring reliable detection of the lockout state from server responses.
255-261: LGTM! Well-structured lockout handling method.The
handleAccountLockout()method follows the established pattern of other error handlers:
- Logs the recovery failure for analytics
- Clears input fields for security
- Navigates to appropriate message screen using the new constant and localized strings
This provides a consistent user experience during account lockout scenarios.
app/res/values-fr/strings.xml (2)
445-448: LGTM! French translations for PersonalID configuration errors are clear and appropriate.The error messages provide good user guidance for different failure scenarios and are consistent with the enhanced error handling mentioned in the PR summary.
457-457: LGTM! Account lockout messages are clear and informative.The French translations effectively communicate the lockout state and reason to users.
Also applies to: 459-459
app/src/org/commcare/android/integrity/IntegrityTokenApiRequestHelper.kt (2)
22-22: Good refactoring to use explicit callback interface.Using
IntegrityTokenCallbackinstead of a generic function type improves type safety and enables proper error propagation through theonTokenFailuremethod.Also applies to: 48-48, 58-58
26-26: Good improvement to propagate actual failure exceptions.Storing and passing the actual exception instead of null enables better error reporting and debugging.
Also applies to: 38-38, 56-56, 74-74
app/src/org/commcare/android/integrity/IntegrityTokenViewModel.kt (2)
48-51: Good addition of exception logging for provider preparation failures.Logging the exception helps with debugging provider initialization issues.
117-120: Well-designed callback interface with explicit error handling.The addition of
onTokenFailureand includingrequestHashinonTokenReceivedimproves error handling and request tracking capabilities.app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (3)
187-199: Good improvement to error handling with user-friendly messages.The explicit callback implementation with proper error mapping provides better user feedback for integrity token failures.
202-202: Appropriate null safety annotations.Removing nullable annotations from
makeStartConfigurationCallparameters is correct as they're never null. TheonFailureparameters remain nullable which is appropriate.Also applies to: 222-223
224-227: Good security-conscious handling of FORBIDDEN_ERROR.Providing a generic message for forbidden errors is appropriate to avoid exposing security implementation details to users.
Comment on lines
+86
to
+97
| _providerState.observeForever(object : Observer<TokenProviderState> { | ||
| override fun onChanged(state: TokenProviderState) { | ||
| if (state is TokenProviderState.Success) { | ||
| _providerState.removeObserver(this) | ||
| requestIntegrityToken(requestHash, true, callback) | ||
| } else if (state is TokenProviderState.Failure) { | ||
| _providerState.removeObserver(this) | ||
| Logger.log("Error re-preparing token provider after failure", state.exception.message ) | ||
| callback.onTokenFailure(state.exception) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
Potential memory leak with observeForever in retry logic.
Using observeForever could leak the observer if the ViewModel is cleared before the state changes. Consider using a timeout or ensuring the observer is removed in all cases.
Alternative approaches:
- Store the observer reference and remove it in
onCleared() - Use a coroutine with timeout
- Use
observeOnceextension if available
+ private val retryObservers = mutableSetOf<Observer<TokenProviderState>>()
+
+ override fun onCleared() {
+ super.onCleared()
+ retryObservers.forEach { _providerState.removeObserver(it) }
+ retryObservers.clear()
+ }
private fun handleRequestFailureAndRetry(...) {
if (exception is StandardIntegrityException && shouldRetryForIntegrityError(exception) && !hasRetried) {
Logger.log(LogTypes.TYPE_MAINTENANCE, "Integrity provider is invalid or outdated, re-preparing and retrying...")
prepareTokenProvider()
// Observe the new preparation and retry once
- _providerState.observeForever(object : Observer<TokenProviderState> {
+ val observer = object : Observer<TokenProviderState> {
override fun onChanged(state: TokenProviderState) {
if (state is TokenProviderState.Success) {
_providerState.removeObserver(this)
+ retryObservers.remove(this)
requestIntegrityToken(requestHash, true, callback)
} else if (state is TokenProviderState.Failure) {
_providerState.removeObserver(this)
+ retryObservers.remove(this)
Logger.log("Error re-preparing token provider after failure", state.exception.message )
callback.onTokenFailure(state.exception)
}
}
- })
+ }
+ retryObservers.add(observer)
+ _providerState.observeForever(observer)📝 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.
Suggested change
| _providerState.observeForever(object : Observer<TokenProviderState> { | |
| override fun onChanged(state: TokenProviderState) { | |
| if (state is TokenProviderState.Success) { | |
| _providerState.removeObserver(this) | |
| requestIntegrityToken(requestHash, true, callback) | |
| } else if (state is TokenProviderState.Failure) { | |
| _providerState.removeObserver(this) | |
| Logger.log("Error re-preparing token provider after failure", state.exception.message ) | |
| callback.onTokenFailure(state.exception) | |
| } | |
| } | |
| }) | |
| val observer = object : Observer<TokenProviderState> { | |
| override fun onChanged(state: TokenProviderState) { | |
| if (state is TokenProviderState.Success) { | |
| _providerState.removeObserver(this) | |
| retryObservers.remove(this) | |
| requestIntegrityToken(requestHash, true, callback) | |
| } else if (state is TokenProviderState.Failure) { | |
| _providerState.removeObserver(this) | |
| retryObservers.remove(this) | |
| Logger.log("Error re-preparing token provider after failure", state.exception.message ) | |
| callback.onTokenFailure(state.exception) | |
| } | |
| } | |
| } | |
| retryObservers.add(observer) | |
| _providerState.observeForever(observer) |
🤖 Prompt for AI Agents
In app/src/org/commcare/android/integrity/IntegrityTokenViewModel.kt around
lines 86 to 97, the use of observeForever without guaranteed removal risks a
memory leak if the ViewModel is cleared before state changes. Fix this by
storing the observer reference and removing it in the ViewModel's onCleared()
method, or replace observeForever with a coroutine-based approach that includes
a timeout to automatically cancel observation, or use an observeOnce extension
if available to ensure the observer is removed after one update.
OrangeAndGreen
approved these changes
Jun 27, 2025
This was referenced Sep 9, 2025
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backmerge for #3212