Added error text for biometric failure #3458
Conversation
jaypanchal-13
requested review from
Jignesh-dimagi,
OrangeAndGreen,
conroy-ricketts and
shubham1g5
📝 WalkthroughWalkthroughThis pull request adds structured error reporting for biometric authentication failures in the PersonalID verification flow. It introduces an error display TextView to the layout, adds localized error message strings for eight different biometric error scenarios across nine language variants (English, Spanish, French, Hindi, Lithuanian, Norwegian, Portuguese, Swahili, and Tigrinya), and implements error-to-message mapping via a new helper method that displays errors in the UI instead of relying solely on Toast notifications. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
![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 (2)
app/res/layout/screen_personalid_verify.xml (1)
100-109: Consider adding contentDescription for accessibility.The errorTextView is properly configured for displaying biometric errors (hidden by default, red color, centered). However, consider adding an
android:contentDescriptionattribute or managing it programmatically to ensure accessibility for users relying on TalkBack or other assistive technologies.Additionally, the text size is hardcoded to
14sp. While this works, consider using a dimension resource for consistency and easier maintenance.Example enhancement:
<TextView android:id="@+id/errorTextView" android:textStyle="bold" android:layout_width="wrap_content" android:layout_height="wrap_content" android:layout_margin="@dimen/activity_horizontal_margin" android:layout_gravity="center_horizontal" android:textColor="@android:color/holo_red_light" android:visibility="gone" android:textSize="@dimen/text_small" android:contentDescription="@string/error_message_description" />app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (1)
113-114: Inconsistent error display approach between error types.The error handling is inconsistent:
onAuthenticationErrordisplays errors inerrorTextView(lines 99-100), butonAuthenticationFailedhides theerrorTextViewand still uses a Toast. This creates a mixed user experience where some errors appear inline and others as toasts.Consider one of these approaches:
- Recommended: Display the authentication failure message in
errorTextViewinstead of Toast for consistency- Keep both if there's a specific UX reason to distinguish between error types
If you choose option 1, apply this diff:
@Override public void onAuthenticationFailed() { super.onAuthenticationFailed(); - binding.errorTextView.setVisibility(View.GONE); - Toast.makeText(requireActivity(), getString(R.string.personalid_authentication_failed), Toast.LENGTH_SHORT).show(); + binding.errorTextView.setVisibility(View.VISIBLE); + binding.errorTextView.setText(getString(R.string.personalid_authentication_failed)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/res/layout/screen_personalid_verify.xml(1 hunks)app/res/values-es/strings.xml(1 hunks)app/res/values-fr/strings.xml(1 hunks)app/res/values-hi/strings.xml(1 hunks)app/res/values-lt/strings.xml(1 hunks)app/res/values-no/strings.xml(1 hunks)app/res/values-pt/strings.xml(1 hunks)app/res/values-sw/strings.xml(1 hunks)app/res/values-ti/strings.xml(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java(1 hunks)app/src/org/commcare/utils/BiometricsHelper.java(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2949
File: app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java:95-131
Timestamp: 2025-02-04T21:38:11.970Z
Learning: Biometric authentication security improvements were considered but skipped in PR #2949 as per user's request. The implementation remained with basic biometric authentication without additional security layers.
📚 Learning: 2025-05-22T14:32:53.133Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 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.
Applied to files:
app/res/values-es/strings.xmlapp/res/values-pt/strings.xml
📚 Learning: 2025-02-04T21:29:29.594Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 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.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java
📚 Learning: 2025-02-04T21:38:11.970Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2949
File: app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java:95-131
Timestamp: 2025-02-04T21:38:11.970Z
Learning: Biometric authentication security improvements were considered but skipped in PR #2949 as per user's request. The implementation remained with basic biometric authentication without additional security layers.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.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 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.
Applied to files:
app/res/layout/screen_personalid_verify.xml
🧬 Code graph analysis (1)
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (1)
app/src/org/commcare/utils/BiometricsHelper.java (1)
BiometricsHelper(33-278)
⏰ 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 (11)
app/res/values-es/strings.xml (1)
530-537: LGTM - Biometric error localization added for Spanish.The addition of these 8 Spanish string resources for PersonalID biometric errors is appropriate and addresses the translation gap noted in previous reviews. The strings are consistently named and properly formatted.
Based on learnings, PersonalID features were previously not translated to Spanish, so this represents important progress in localization coverage.
app/res/values-hi/strings.xml (1)
528-535: LGTM - Hindi localization for biometric errors.The Hindi translations for PersonalID biometric error messages are properly formatted and consistent with the localization pattern established across other language files in this PR.
app/res/values-sw/strings.xml (1)
538-545: LGTM - Swahili localization complete.The Swahili translations for biometric error messages are properly added and maintain consistency with the other locale additions in this PR.
app/res/values-ti/strings.xml (1)
521-528: LGTM - Tigrinya localization added.The Tigrinya translations for PersonalID biometric errors are correctly formatted and consistent with other locales.
app/res/values-lt/strings.xml (1)
137-144: LGTM - Lithuanian localization addresses known gap.The Lithuanian translations for biometric errors are well-formed and consistent. This addresses the previously noted gap where PersonalID features lacked Lithuanian translations.
Based on learnings, Lithuanian translations were not available for PersonalID features, so this is an important localization improvement.
app/res/values-no/strings.xml (1)
137-144: LGTM - Norwegian localization completes translation coverage.The Norwegian translations for PersonalID biometric errors are properly implemented and address the known translation gap. This completes an important piece of localization infrastructure.
Based on learnings, Norwegian translations were not available for PersonalID features, making this a valuable addition.
app/src/org/commcare/utils/BiometricsHelper.java (1)
257-275: Verify error code coverage and add documentation.The
getBiometricErrormethod provides a clean mapping from error codes to user-facing messages. However, there are a few points to verify:
Error code coverage: The method handles common BiometricPrompt error codes but several are unmapped (e.g.,
ERROR_USER_CANCELED,ERROR_NO_SPACE,ERROR_UNABLE_TO_PROCESS,ERROR_VENDOR,ERROR_NO_DEVICE_CREDENTIAL,ERROR_SECURITY_UPDATE_REQUIRED). These will fall through to the default case showing "cannot_configure" which may not be semantically correct. Please verify this is acceptable or consider adding explicit cases for critical unmapped codes.Grouping clarification:
ERROR_NEGATIVE_BUTTONandERROR_CANCELEDboth map to "cancelled" - is this intentional? If so, consider adding a comment explaining the rationale.Null safety: The method doesn't check if
activityis null before callinggetString(). While the caller should ensure a valid activity, defensive programming suggests adding a null check or documenting the precondition.Documentation: Add javadoc to document the method's purpose, parameters, return value, and any preconditions.
Consider adding:
/** * Maps biometric error codes to user-facing error messages. * * @param errorCode The BiometricPrompt error code * @param activity The activity context for retrieving string resources (must not be null) * @return A localized error message string */ public static String getBiometricError(int errorCode, Activity activity) { // Implementation }app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (1)
107-107: LGTM: Error view properly hidden on success.The errorTextView is correctly hidden when authentication succeeds.
app/res/values/strings.xml (1)
703-710: LGTM: Clear and comprehensive error messages.The eight new biometric error strings are well-written, user-friendly, and provide clear guidance. The naming convention is consistent and the messages appropriately cover the error scenarios handled in
BiometricsHelper.getBiometricError().app/res/values-pt/strings.xml (1)
538-545: Verify Portuguese translations with a native speaker.The eight Portuguese biometric error strings are structurally correct and match the English keys. However, ensure these translations have been reviewed by a native Portuguese speaker or professional translator for accuracy and appropriate tone.
If you have a Portuguese-speaking team member or translator available, please have them verify these messages are natural and accurate.
app/res/values-fr/strings.xml (1)
533-540: Verify French translations with a native speaker.The eight French biometric error strings are structurally correct and match the English keys. However, ensure these translations have been reviewed by a native French speaker or professional translator for accuracy and appropriate tone.
If you have a French-speaking team member or translator available, please have them verify these messages are natural and accurate.
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java
Outdated
Show resolved
Hide resolved
|
@jaypanchal-13 For point 2 and 3, is that you are not able to reproduce or its solved in another ticket? If so please link here. |
|
I think point 2 was happening for some Android specific version, you can connect with Nitin to get more context. |
|
| Toast.makeText(context, getString(R.string.connect_verify_configuration_failed, errString), | ||
| Toast.LENGTH_SHORT).show(); | ||
| binding.errorTextView.setVisibility(View.VISIBLE); | ||
| binding.errorTextView.setText(BiometricsHelper.getBiometricError(errorCode,requireActivity())); |
There was a problem hiding this comment.
Any blocking errors here should use bottom sheet vs any where we want use to retry should be an inline error view.
There was a problem hiding this comment.
@shubham1g5 As per spec inline error should be enough. Still we should add bottom sheet?
There was a problem hiding this comment.
yeah that spec assumed that the error here would be temporary but give the error codes you have outlined it seems like user can get into blocking failures here as well
There was a problem hiding this comment.
checking with expectations here from @OrangeAndGreen as well
There was a problem hiding this comment.
Hmm, agreed that any permanent blocking failures should be displayed in a bottom sheet and then the user should be forced to exit the workflow. But also, I think we already handle those cases properly via other code paths, such that they should never occur here in the callback (it's only used once the code calls authenticatePin or authenticateFingerprint). We only try to authenticate if the relevant status is "Configured" and otherwise we show a bottom sheet if appropriate, i.e. here:
There was a problem hiding this comment.
I see we log a non-fatal exception in the onAuthenticationError callback and include the error code, so maybe it's worth checking Firebase to see which (and how many) of these error codes we're seeing. Otherwise it seems unnecessary to add strings and handling for error conditions that we'll never actually encounter.
There was a problem hiding this comment.
Hmm, agreed that any permanent blocking failures should be displayed in a bottom sheet and then the user should be forced to exit the workflow. But also, I think we already handle those cases properly via other code paths, such that they should never occur here in the callback (it's only used once the code calls authenticatePin or authenticateFingerprint). We only try to authenticate if the relevant status is "Configured" and otherwise we show a bottom sheet if appropriate, i.e. here:
@OrangeAndGreen Makes sense. Given the current flow, I think we can keep this as an inline error for now.
This callback is only reached after we’ve confirmed biometrics are configured and started authentication, and the permanent blocking cases are already handled earlier via other code paths.
Errors here should mostly be transient and retryable, so inline feedback seems appropriate. Since we’re logging the error codes as non-fatals, we can always revisit and add a blocking bottom sheet if we see unexpected permanent failures in practice. What do you suggest @shubham1g5
There was a problem hiding this comment.
Are you able to outline all error codes in this thread and specify what errors we should not expect to happen here because of earlier checks and what amongst those we expect to happen, user should be able to retry. Also for anything that's already being checked and we don't expect to happen here, we should be using the existing error strings for those instead of creating new ones.
There was a problem hiding this comment.
These are already used so removing it ERROR_HW_NOT_PRESENT, ERROR_HW_UNAVAILABLE, ERROR_NO_BIOMETRICS and keeping ERROR_NEGATIVE_BUTTON, ERROR_CANCELED, ERROR_LOCKOUT, ERROR_LOCKOUT_PERMANENT ERROR_TIMEOUT
| public static String getBiometricError(int errorCode, Activity activity) { | ||
| return switch (errorCode) { | ||
| case BiometricPrompt.ERROR_NEGATIVE_BUTTON, BiometricPrompt.ERROR_CANCELED -> | ||
| activity.getString(R.string.personalid_biometric_error_cancelled); | ||
| case BiometricPrompt.ERROR_LOCKOUT -> | ||
| activity.getString(R.string.personalid_biometric_error_lockout); | ||
| case BiometricPrompt.ERROR_LOCKOUT_PERMANENT -> | ||
| activity.getString(R.string.personalid_biometric_error_lockout_permanent); | ||
| case BiometricPrompt.ERROR_HW_NOT_PRESENT -> | ||
| activity.getString(R.string.personalid_biometric_error_hw_not_present); | ||
| case BiometricPrompt.ERROR_HW_UNAVAILABLE -> | ||
| activity.getString(R.string.personalid_biometric_error_hw_unavailable); | ||
| case BiometricPrompt.ERROR_NO_BIOMETRICS -> | ||
| activity.getString(R.string.personalid_biometric_error_no_biometric); | ||
| case BiometricPrompt.ERROR_TIMEOUT -> | ||
| activity.getString(R.string.personalid_biometric_error_timeout); | ||
| default -> activity.getString(R.string.personalid_biometric_error_cannot_configure); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Can we have more context on why we are adding new strings in this PR? It seems that the ticket was to inline the current error message and, if needed, new strings would be added in some other ticket?
There was a problem hiding this comment.
I actually led Jay into doing that as showing the error code to the user on a static error UI felt like a bad UX to implement here.
| android:text="@string/connect_verify_configure_pin" /> | ||
|
|
||
| <TextView | ||
| android:id="@+id/errorTextView" |
There was a problem hiding this comment.
Convention has been to use lowercase with underscores instead of camel-case for UI element IDs
| return activity.getString(R.string.personalid_configuration_process_biometric_needs_update_message); | ||
| } | ||
|
|
||
| public static String getBiometricError(int errorCode, Activity activity) { |
There was a problem hiding this comment.
Ideally it should be Context and not Activity
| activity.getString(R.string.personalid_biometric_error_cancelled); | ||
| case BiometricPrompt.ERROR_LOCKOUT -> | ||
| activity.getString(R.string.personalid_biometric_error_lockout); | ||
| case BiometricPrompt.ERROR_LOCKOUT_PERMANENT -> |
There was a problem hiding this comment.
@jaypanchal-13 ERROR_LOCKOUT_PERMANENT should be on bottom sheet as someone trying to access phone and also default seems blocking? @shubham1g5 @OrangeAndGreen
There was a problem hiding this comment.
From code ERROR_LOCKOUT_PERMANENT it seems correct but this code is seen when too many wrong attempt is tried and so user will be suggested to unlock with using pin or pattern. So I think there is no need of bottom sheet for it
There was a problem hiding this comment.
I was wondering as there are 2 separate errors ERROR_LOCKOUT_PERMANENT and ERROR_LOCKOUT so thinking for bottom sheet for first one.
There was a problem hiding this comment.
ERROR_LOCKOUT is seen when 5 failed attempted and it lasts for 30 seconds, after that user can retry. So no need of bottom sheet in this as well
There was a problem hiding this comment.
Ok got it and ERROR_LOCKOUT_PERMANENT is non-recoverable?
There was a problem hiding this comment.
It is recoverable. Once user unlock with using pin or pattern.
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
LGTM pending the recent comments that the team has posted
app/res/values/strings.xml
Outdated
| <string name="personalid_biometric_error_lockout">Too many attempts. Please wait and try again.</string> | ||
| <string name="personalid_biometric_error_lockout_permanent">Too many failed attempts. Please unlock your device and try again.</string> | ||
| <string name="personalid_biometric_error_timeout">Biometric operation timed out. Please try again.</string> | ||
| <string name="personalid_biometric_error_cannot_configure">We couldn’t configure biometrics on your device. Please try again.</string> |
There was a problem hiding this comment.
Given we are not sure here that the issue will resolve once the user retries, instead of saying "Please try again" we should just say that "Please contact customer support if the problem persists"
|
@shubham1g5 blocked for your approval |
Product Description
Ticket -> https://dimagi.atlassian.net/browse/CCCT-1913
Technical Summary
Point 1 is added in this PR. point 2 and 3 are already done
Screen shot ->
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review