Fixed biometric config options when fingerprint not available #3369
Conversation
…ig page. Fixed issue where fingerprint option could be shown when device doesn't have a fingerprint scanner.
📝 WalkthroughWalkthroughThis PR refactors the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The refactoring involves reorganized control flow with interdependent logic across button visibility determination, title/message assignment, and error handling within a single method. Understanding the interaction between these deferred operations and verifying edge cases around biometric hardware availability and PIN configuration requires careful analysis of the complete method behavior. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (2)
141-149: Gate "Configure fingerprint" to NotConfigured; avoid showing it for NeedsUpdate/NotAvailable.Currently any status except NoHardware shows the configure button, which can mislead when the state is NeedsUpdate/NotAvailable. Prefer restricting to NotConfigured.
Apply this diff:
- if(hasFingerprintHardware) { - fingerprintButton = getString(R.string.connect_verify_configure_fingerprint); - } + if (fingerprintStatus == BiometricsHelper.ConfigurationStatus.NotConfigured) { + fingerprintButton = getString(R.string.connect_verify_configure_fingerprint); + }Based on relevant code snippets (status enums).
145-149: Minor: cache requiredLock locally to avoid repeated lookups.Read requiredLock once into a local variable at method start and reuse.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (2)
app/src/org/commcare/utils/BiometricsHelper.java (1)
BiometricsHelper(33-258)app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt (1)
PIN(33-35)
🔇 Additional comments (2)
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (2)
137-149: Confirm intent: hide PIN option when fingerprint is configured even if PIN is required.With this logic, when requiredLock == PIN and fingerprint is Configured, the PIN button is never shown. Is this a product decision? If not, consider also showing PIN when it’s configured/available.
141-142: Verify copy for PIN-only scenarios.When only PIN is visible, message uses connect_verify_message (generic). Ensure this string doesn’t reference fingerprint. If it does, set a PIN-specific message when fingerprintButton is null.
Also applies to: 169-171
| if(fingerprintButton != null && pinButton != null) { | ||
| title = getString(R.string.connect_verify_title); | ||
| } else if (fingerprintButton != null) { | ||
| title = getString(R.string.connect_verify_use_fingerprint_long); | ||
| } else if (pinButton != null) { | ||
| title = getString(R.string.connect_verify_use_pin_long); | ||
| } else { | ||
| showNoBiometricHardwareError(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Route PIN-specific errors when no options are available (wrong error today).
If requiredLock is PIN and neither button is shown, current code surfaces "No biometric hardware" which is incorrect. Show PIN-specific errors based on pinStatus instead.
Apply this diff:
- } else {
- showNoBiometricHardwareError();
- return;
- }
+ } else {
+ String requiredLock = personalIdSessionDataViewModel.getPersonalIdSessionData().getRequiredLock();
+ if (PIN.equals(requiredLock)) {
+ if (pinStatus == BiometricsHelper.ConfigurationStatus.NeedsUpdate) {
+ showPinNeedsUpdateError();
+ } else {
+ showPinNotAvailableError();
+ }
+ } else {
+ showNoBiometricHardwareError();
+ }
+ return;
+ }Based on relevant code snippets (BiometricsHelper.checkStatus mappings).
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
I think we're okay with the error message being as is in this special case
There was a problem hiding this comment.
@OrangeAndGreen, understood! I'll defer to your judgment on this. The current error message is acceptable for this edge case.
| // Need at least Fingerprint configuration for BIOMETRIC_TYPE | ||
| } else { | ||
| message = getString(R.string.connect_verify_message); | ||
| if(hasFingerprintHardware) { |
There was a problem hiding this comment.
hasFingerprintHardware should check for other scenario also i.e. for NotAvailable and NeedsUpdate?
There was a problem hiding this comment.
I think the idea is to show the option as long as the hardware exists on the device, and then when the user clicks the button for fingerprint we'll show an error message if there's an issue (see switch around line 194).
The key here is that if the device doesn't physically have a fingerprint sensor, we don't even want to show it as an option.
There was a problem hiding this comment.
That's good flow but what I have observed is that even user has PIN as required lock, app is trying to configure the malformed finger print and at the end its giving error but actually user should have been able to move ahead by PIN only.
There was a problem hiding this comment.
@Jignesh-dimagi Do you mean user gets blocked on using pin if fingerprint errors while doing a setup step ? If so, I agree that's not the behaviour we would want.
There was a problem hiding this comment.
@Jignesh-dimagi I don't see how this PR is changing that behaviour though, is it something you think already happens and we should change OR is it something you think is introduced by this PR ?
There was a problem hiding this comment.
@shubham1g5 Not with PR exactly but seen configuring the malfunctioning fingerprint is blocking the users from moving ahead. I am fine if we want to take it separately.
There was a problem hiding this comment.
got it, lets take it separately in that case so that we don't block the release. Curious if you had seen it as part of an interrupt ticket or just on your phone ? It might be cruicial to know the exact failure mode here for us to take actions.
There was a problem hiding this comment.
@shubham1g5 I am not able to reproduce it but seen in some interrupt tickets where there was error saying update the play service and another with message that not able to use your hardware or unavailable at this moment. I am not able to find that ticket now, will search for it.
There was a problem hiding this comment.
got it, no worries if you can't. I am sure it will surface again someway if it's not fixed :)
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java
Outdated
Show resolved
Hide resolved
… to user based on whether fingerprint and/or PIN are options.
|
@OrangeAndGreen seems like build is not compiling here. |
https://dimagi.atlassian.net/browse/QA-8145
Product Description
Fingerprint option will never be displayed on the Biometric Config page if the device doesn't have a fingerprint scanner.
Technical Summary
Rearranged logic for showing fingerprint/PIN option on biometric config page.
Fixed issue where fingerprint option could be shown when device doesn't have a fingerprint scanner.
Feature Flag
PersonalID
Safety Assurance
Safety story
Tested manually by dev
Automated test coverage
None
QA Plan
Test the Biometric Config page under various scenarios of fingerprint/PIN being available and/or configured and Connect vs. non-Connect user. Make sure the appropriate options are shown to the user in every case.