Handling failed biometric config properly #3174
Conversation
…message to user) instead of succeeding.
📝 WalkthroughWalkthroughThis set of changes includes several updates across the Android application. The AndroidManifest.xml was modified to downgrade the version name and enforce portrait orientation for a specific activity. Multiple localization files (French, Portuguese, Swahili, Tigrinya, and default) were updated to replace the term "ConnectID" with "PersonalId," reflecting a branding change. A device compatibility check was added to the PersonalIdManager, and this check is now used to control the visibility of certain menu items in activities. UI logic for connect-related buttons has been updated to restrict or disable their visibility and functionality. Various fragments related to the PersonalId feature have been refactored to improve UI state handling, result processing, and phone number parsing. Sequence Diagram(s)sequenceDiagram
participant User
participant LoginActivity
participant PersonalIdManager
User->>LoginActivity: Open menu
LoginActivity->>PersonalIdManager: isloggedIn()
LoginActivity->>PersonalIdManager: checkDeviceCompability()
alt Not logged in AND device compatible
LoginActivity->>User: Show "Connect Sign In" menu item
else
LoginActivity->>User: Hide "Connect Sign In" menu item
end
sequenceDiagram
participant User
participant PersonalIdPhoneFragment
participant PhoneNumberHelper
User->>PersonalIdPhoneFragment: Provide phone number
PersonalIdPhoneFragment->>PhoneNumberHelper: getCountryCode(phone)
PersonalIdPhoneFragment->>PhoneNumberHelper: getNationalNumber(phone)
alt Valid country code and national number
PersonalIdPhoneFragment->>User: Set phone and country code fields
else
PersonalIdPhoneFragment->>User: Do not update fields
end
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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: 3
🔭 Outside diff range comments (1)
app/src/org/commcare/fragments/SelectInstallModeFragment.java (1)
213-224: 💡 Verification agent🧩 Analysis chain
Temporary disabling of connect button functionality.
The connect button is now unconditionally disabled as indicated by the TODO comment referencing "phase 4 merge". While this appears to be intentional based on the broader changes to PersonalId functionality, ensure this temporary change is properly tracked.
Consider creating a tracking issue for re-enabling this functionality when phase 4 is ready:
🏁 Script executed:
#!/bin/bash # Description: Search for other TODO comments related to phase 4 or connect functionality # Expected: Find any other temporary disabling of connect features rg -i "phase 4|TODO.*connect" --type javaLength of output: 891
Establish a Phase 4 Connect Re-enablement Tracking Issue
We’ve identified multiple temporary TODOs that disable or defer PersonalId “connect” functionality across the codebase:
• app/src/org/commcare/fragments/SelectInstallModeFragment.java
– ///TODO: connect uncomment after phase 4 merge
• app/src/org/commcare/connect/PersonalIdManager.java
– ///TODO update the code with connect code
– ///TODO uncomment with connect pahse pr
• app/src/org/commcare/activities/LoginActivityUIController.java
– ///TODO: connect set to true after phase 4 merge
• app/src/org/commcare/activities/LoginActivity.java
– ///TODO: connect uncomment with connect merge
• app/src/org/commcare/tasks/templates/CommCareTask.java
– //TODO: Maybe notify the old thing that we're disconnecting?
• app/src/org/commcare/models/AndroidSessionWrapper.java
– // TODO: This is really a join situation. Need a way to outline connections between tables to enable joiningPlease create a single tracking issue (e.g., PHASE4-123) that:
- Consolidates all of these TODOs under one ticket
- References the ticket ID in each TODO comment (e.g.,
// TODO PHASE4-123: connect…)- Ensures this issue is resolved once Phase 4 is complete
🧹 Nitpick comments (4)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
5-5: Remove unused import.The
android.os.Buildimport is not used anywhere in this file and should be removed to keep the code clean.-import android.os.Build;app/res/values/strings.xml (1)
917-917: Fix typo in error message string.
The word “tying” should be corrected to “trying” in thefailed_to_login_with_connectidmessage.app/res/values-pt/strings.xml (1)
534-534: Refine verb choice in unlink message.
Consider replacing “cortar” with “desvincular” for a more natural translation of “sever this login.”app/src/org/commcare/activities/LoginActivityUIController.java (1)
567-568: Temporary functionality disabling - ensure phase 4 completion.The connect button is being temporarily hidden even for logged-in users, which may impact user experience. The TODO comment indicates this should be reverted after a "phase 4" merge. Please ensure this temporary change is tracked and restored appropriately once the referenced development phase is complete.
Consider adding a more specific TODO with a tracking ticket reference:
- ///TODO: connect set to true after phase 4 merge + // TODO: QA-7836 - restore connect button visibility after phase 4 merge
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/AndroidManifest.xml(2 hunks)app/res/values-fr/strings.xml(5 hunks)app/res/values-pt/strings.xml(5 hunks)app/res/values-sw/strings.xml(5 hunks)app/res/values-ti/strings.xml(5 hunks)app/res/values/strings.xml(2 hunks)app/src/org/commcare/activities/CommCareSetupActivity.java(2 hunks)app/src/org/commcare/activities/GeoPointMapActivity.java(1 hunks)app/src/org/commcare/activities/LoginActivity.java(1 hunks)app/src/org/commcare/activities/LoginActivityUIController.java(1 hunks)app/src/org/commcare/connect/PersonalIdManager.java(2 hunks)app/src/org/commcare/fragments/SelectInstallModeFragment.java(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java(2 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java(3 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(3 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java(2 hunks)app/src/org/commcare/utils/PhoneNumberHelper.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/src/org/commcare/connect/PersonalIdManager.java (1)
app/src/org/commcare/android/security/AesKeyStoreHandler.kt (1)
Build(15-54)
app/src/org/commcare/activities/CommCareSetupActivity.java (1)
app/src/org/commcare/connect/PersonalIdManager.java (1)
PersonalIdManager(65-694)
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (1)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-54)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (41)
app/src/org/commcare/activities/GeoPointMapActivity.java (1)
71-72: Validate window feature request ordering.Moving
requestWindowFeature(Window.FEATURE_NO_TITLE)beforesuper.onCreate(savedInstanceState)ensures the feature is requested prior to content creation, preventing potentialAndroidRuntimeException. This aligns with Android’s window feature guidelines.app/AndroidManifest.xml (2)
5-5: Confirm versionName adjustment
The downgrade from "2.58" to "2.57" should align with your release strategy. Ensure this change is intentional, matches the versionCode, and is reflected in your release notes.
583-585: Enforce portrait orientation for MicroImageActivity
Addingandroid:screenOrientation="portrait"here is consistent with other connect‐related activities. Please verify on target devices that the UI displays correctly and that no rotation issues arise.app/src/org/commcare/connect/PersonalIdManager.java (1)
6-6: LGTM: Build import added appropriately.The import is correctly added to support the new device compatibility check.
app/src/org/commcare/utils/PhoneNumberHelper.java (1)
95-108: LGTM: Well-implemented method following established patterns.The
getNationalNumber()method correctly follows the same pattern as the existinggetCountryCode()method, with appropriate error handling and consistent return behavior (-1 for invalid input).app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (3)
183-184: LGTM: Improved lifecycle management for timer and SMS receiver.Moving
startResendTimer()andregisterSmsBroadcastReceiver()toonResume()ensures they only run when the fragment is in the foreground, which is more appropriate than the previous lifecycle hooks.
191-197: LGTM: Proper cleanup in onPause.Moving
stopResendTimer()toonPause()ensures timely cleanup when the fragment goes to background, preventing potential memory leaks and unnecessary background operations.
254-254: LGTM: Prevent unnecessary timestamp updates.Only setting
otpRequestTimewhen a valid phone number exists prevents unnecessary timestamp updates and improves logic clarity.app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (2)
77-77: LGTM: Enable link handling for check text.Adding
LinkMovementMethod.getInstance()properly enables clickable links in thecheckTextview, improving user interaction.
150-162: LGTM: Well-implemented phone number display logic.The
displayPhoneNumber()method correctly utilizes the newgetNationalNumber()helper method to parse and display phone numbers. The null checks and validation logic are appropriate.app/src/org/commcare/activities/LoginActivity.java (1)
555-556: Inconsistency with PR objectives and method name typoThis change adds device compatibility checking to menu visibility, which doesn't align with the stated PR objective of "handling failed biometric config properly." The actual change appears to be about filtering menu visibility based on device capabilities.
Additionally, there's a typo in the method name:
checkDeviceCompability()should becheckDeviceCompatibility().Let me verify the method name and confirm if this change is related to the intended biometric configuration handling:
#!/bin/bash # Search for the checkDeviceCompability method definition and usage rg -A 10 "checkDeviceCompability|checkDeviceCompatibility"Likely an incorrect or invalid review comment.
app/res/values-ti/strings.xml (1)
343-522: String rebranding changes inconsistent with PR objectivesThese changes consistently update the terminology from "ConnectID" to "PersonalId" across the Tigrinya localization strings. While the rebranding is executed correctly and consistently, these changes don't relate to the stated PR objective of "handling failed biometric config properly."
The string updates themselves are well-executed and maintain consistency across the localization.
Likely an incorrect or invalid review comment.
app/res/values/strings.xml (1)
938-938: Add missing translations for new string.
The newdevice_incompatible_version_errorentry is only present in the default locale; please add corresponding translations in all supportedvalues-<locale>folders.app/res/values-pt/strings.xml (16)
356-356: Approve translation for PersonalId login hint.
“Via PersonalId (ou pressione aqui)” accurately reflects the English source.
358-358: Approve translation for secondary phone confirmation.
The Portuguese text matches the intended meaning and placeholder usage.
367-367: Approve translation forconnect_recover_no_account.
The string “Não tem PersonalId?” correctly covers “Don’t have PersonalID?”.
393-393: Approve translation for PersonalId backup code title.
“Código de recuperação PersonalId” aligns with the English “PersonalID Backup Code.”
419-419: Approve translation for recovery success message.
The Portuguese sentence correctly conveys the success of account recovery.
422-422: Approve translation for unlock title.
“Desbloquear PersonalId” matches the English source “Unlock PersonalID.”
436-436: Approve translation for registration success message.
The message accurately reflects the successful account creation.
439-439: Approve translation for verify instructions.
The guidance to configure a method for unlocking both device and account is clear.
441-441: Approve translation for fingerprint configured message.
The sentence correctly informs users about the fingerprint unlock setup.
444-444: Approve translation for PIN configured message.
The Portuguese text clearly communicates the PIN unlock configuration.
530-530: Approve translation for login button text.
“Iniciar sessão com o PersonalId” accurately corresponds to “Login with PersonalId.”
531-531: Approve translation for link to PersonalId title.
“Link para PersonalId?” matches the English prompt.
532-532: Approve translation for link message.
The phrasing properly conveys the option to link login to the PersonalId account.
533-533: Approve translation for unlink title.
“Desvincular o PersonalId?” aligns with “Unlink PersonalId?”
537-537: Approve translation for sign-up menu option.
“Inscreva-se no PersonalId” correctly mirrors “Sign up for PersonalID.”
538-538: Approve translation for forget user menu option.
“Esqueça o usuário do PersonalId” accurately conveys “Forget PersonalID user.”app/res/values-sw/strings.xml (1)
354-577: LGTM: Systematic PersonalId rebranding completed.The systematic replacement of "ConnectID" with "PersonalId" throughout the Swahili localization strings is consistent and comprehensive. This aligns with the documented rebranding effort mentioned in the PR objectives and maintains consistency across all user-facing text related to authentication and account management.
app/src/org/commcare/activities/CommCareSetupActivity.java (2)
8-8: LGTM: Required import for device compatibility check.The addition of the
android.os.Buildimport is necessary to support the device compatibility check functionality being added to the menu visibility logic.
510-511: LGTM: Proper device compatibility gating added.The enhanced menu visibility condition correctly gates the PersonalId sign-in option behind device compatibility. This ensures users on devices running below Android Nougat (API 24) won't see menu options for features that aren't supported on their devices, preventing potential runtime issues.
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (2)
70-70: Good refactoring: Centralize UI configuration logic.Moving the
isRecoveryassignment to the start ofconfigureUiByModemethod centralizes the UI configuration logic and ensures the flag is set before any UI elements depend on it.
146-146: Improved error message visibility handling.Explicitly setting the visibility before updating the error text ensures the UI state is properly managed. This prevents potential issues where the error message might not be visible when needed.
app/res/values-fr/strings.xml (1)
353-353: Systematic branding update from ConnectID to PersonalId.The changes consistently replace "ConnectID" with "PersonalId" throughout the French localization strings, aligning with the broader rebranding effort mentioned in the PR objectives.
Also applies to: 354-354, 356-356, 362-362, 365-365, 366-366, 410-410, 416-416, 419-419, 432-432, 435-435, 437-437, 440-440, 513-513, 514-514, 515-515, 516-516, 517-517, 520-520, 521-521, 576-576
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (2)
226-226: Improved result code handling for PIN activity.Now properly propagates the actual
resultCodefrom the PIN configuration activity instead of always returningRESULT_OK. This aligns with the PR objective of handling failed biometric configuration properly.
230-230: Enhanced error handling for biometric configuration.The method now correctly passes whether the biometric configuration failed (
resultCode != RESULT_OK) tonavigateForward, enabling proper error handling flow instead of always assuming success. This directly addresses the PR objective of properly handling failed biometric configuration.app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java (4)
13-14: LGTM - Clean ViewModel integration.The import statements correctly support the ViewModel pattern integration for session data management.
Also applies to: 21-21
41-41: LGTM - Appropriate field declaration.The field declaration follows proper naming conventions and is correctly scoped as private.
50-51: LGTM - Proper ViewModel initialization.The ViewModel is correctly scoped to the activity and follows Android best practices for accessing shared data across fragments.
143-146:Details
✅ Verification successful
Verify null safety for personalIdSessionData access.
The new case handling for
PERSONALID_RECOVERY_ACCOUNT_ORPHANEDlooks logical - setting account existence to false and navigating to backup code. However, ensure thatpersonalIdSessionDatais never null whenfinish()is called.
🏁 Script executed:
#!/bin/bash # Description: Check if finish() method can be called before onCreateView in the fragment lifecycle # Expected: No direct calls to finish() before onCreateView setup echo "Searching for direct calls to finish() method in PersonalIdMessageFragment..." ast-grep --pattern 'finish()' echo "Searching for any activity finish calls that might bypass onCreateView..." rg -A 3 -B 3 "\.finish\(\)" --type javaLength of output: 39508
🏁 Script executed:
#!/bin/bash echo "Inspecting declaration and initialization of personalIdSessionData in PersonalIdMessageFragment.java" rg -n "personalIdSessionData" -C3 app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.javaLength of output: 1461
PersonalIdSessionData is always initialized before use
personalIdSessionDatais set vianew ViewModelProvider(requireActivity())…getPersonalIdSessionData()in onCreateView (lines 50–51), before any calls to finish() or its use in thePERSONALID_RECOVERY_ACCOUNT_ORPHANEDbranch. No additional null checks are necessary here.
| public boolean checkDeviceCompability() { | ||
| return Build.VERSION.SDK_INT >= Build.VERSION_CODES.N; | ||
| } |
There was a problem hiding this comment.
Fix the method name typo and add documentation.
The method has a spelling error in its name and lacks documentation explaining its purpose.
+ /**
+ * Checks if the device meets the minimum Android version requirement for PersonalId features.
+ * PersonalId requires Android N (API 24) or higher for biometric functionality.
+ *
+ * @return true if device SDK version is >= Android N (API 24), false otherwise
+ */
- public boolean checkDeviceCompability() {
+ public boolean checkDeviceCompatibility() {
return Build.VERSION.SDK_INT >= Build.VERSION_CODES.N;
}📝 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.
| public boolean checkDeviceCompability() { | |
| return Build.VERSION.SDK_INT >= Build.VERSION_CODES.N; | |
| } | |
| /** | |
| * Checks if the device meets the minimum Android version requirement for PersonalId features. | |
| * PersonalId requires Android N (API 24) or higher for biometric functionality. | |
| * | |
| * @return true if device SDK version is >= Android N (API 24), false otherwise | |
| */ | |
| public boolean checkDeviceCompatibility() { | |
| return Build.VERSION.SDK_INT >= Build.VERSION_CODES.N; | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/PersonalIdManager.java around lines 665 to 667,
the method name checkDeviceCompability contains a typo and should be renamed to
checkDeviceCompatibility. Additionally, add a Javadoc comment above the method
to clearly describe its purpose, which is to verify if the device's Android
version is at least Nougat (API level N).
| <string name="connect_recover">Recuperar</string> | ||
| <string name="connect_registration_have_account">Já tem\nConnectID? Não tem\numa conta?</string> | ||
| <string name="connect_recover_no_account">Não tem ConnectID?</string> | ||
| <string name="connect_registration_have_account">Já tem\nPersonalId? Não tem\numa conta?</string> |
There was a problem hiding this comment.
Incorrect merging of prompts in connect_registration_have_account.
This string combines two distinct prompts into one; it should only ask “Already have an account?” and not duplicate the recovery prompt.
🤖 Prompt for AI Agents
In app/res/values-pt/strings.xml at line 366, the string resource
"connect_registration_have_account" incorrectly merges two prompts into one.
Update the string to only ask "Já tem uma conta?" (Already have an account?)
without including the recovery prompt or newline characters. Remove the
duplicated or unrelated text to keep the prompt clear and singular.
| <string name="connect_backup_code_verify_forgot">Esqueci o código</string> | ||
| <string name="connect_recovery_title">Configuração do ConnectID</string> | ||
| <string name="connect_recovery_title">Configuração do PersonalId</string> | ||
| <string name="connect_recovery_alt_title">Telefone alternativo</string> |
There was a problem hiding this comment.
Incorrect translation for recovery title.
“Configuração do PersonalId” implies setup; the English string is “PersonalID Recovery.” Please use “Recuperação do PersonalId” or similar.
🤖 Prompt for AI Agents
In app/res/values-pt/strings.xml at line 414, the string
"connect_recovery_alt_title" is incorrectly translated as "Telefone
alternativo," which does not match the English meaning "PersonalID Recovery."
Update the translation to "Recuperação do PersonalId" or a similar phrase that
accurately conveys the recovery context instead of setup or alternative phone.
https://dimagi.atlassian.net/browse/QA-7836
Product Description
User will see an error message if biometric config fails or is aborted (instead of incorrectly proceeding to the next step in the device configuration workflow).
Technical Summary
We were ignoring the result code that came back from the biometric config activity.
Passing the result code through so it can be handled properly now.
Feature Flag
PersonalID
Safety Assurance
Safety story
Tested the change out and I see the on-screen error message now.
Automated test coverage
None
QA Plan
This is in the QA plan already, the bug came from QA testing