CCCT-2163 - Configure Fingerprint or PIN false error message#3574
CCCT-2163 - Configure Fingerprint or PIN false error message#3574Jignesh-dimagi wants to merge 2 commits intomasterfrom
Conversation
Jignesh-dimagi
requested review from
OrangeAndGreen,
conroy-ricketts and
shubham1g5
📝 WalkthroughWalkthroughThe changes modify the biometric enrollment workflow in two fragments. In PersonalIdBiometricConfigFragment, the handleFinishedPinActivity method now evaluates whether any biometric method (fingerprint or PIN) has been configured and only navigates forward if nothing is configured. In PersonalIdMessageFragment, onResume now includes logic to detect enrollment failure and automatically navigate up if biometric authentication is subsequently configured. Comprehensive unit tests are added to verify the navigation behavior across various biometric configuration scenarios. Sequence Diagram(s)sequenceDiagram
participant User
participant BiometricConfigFrag as PersonalIdBiometricConfigFragment
participant BiometricManager
participant Navigation
participant MessageFrag as PersonalIdMessageFragment
User->>BiometricConfigFrag: completes PIN/biometric enrollment
activate BiometricConfigFrag
BiometricConfigFrag->>BiometricManager: check if fingerprint/PIN configured
activate BiometricManager
BiometricManager-->>BiometricConfigFrag: configuration status
deactivate BiometricManager
alt Any biometric method configured
BiometricConfigFrag->>BiometricConfigFrag: no navigation action
else Nothing configured
BiometricConfigFrag->>Navigation: navigateForward(true)
activate Navigation
Navigation-->>User: proceed to next screen
deactivate Navigation
end
deactivate BiometricConfigFrag
User->>MessageFrag: returns to message screen (onResume)
activate MessageFrag
alt Enrollment failed AND biometric now configured
MessageFrag->>BiometricManager: verify configuration state
activate BiometricManager
BiometricManager-->>MessageFrag: biometric status confirmed
deactivate BiometricManager
MessageFrag->>Navigation: navigate up
activate Navigation
Navigation-->>User: dismiss screen
deactivate Navigation
else Enrollment state unchanged
MessageFrag->>MessageFrag: remain on screen
end
deactivate MessageFrag
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java (1)
3-39:⚠️ Potential issue | 🟡 MinorRemove duplicate imports.
The following imports appear twice in the file:
android.app.Activity.RESULT_OK(lines 3 and 39)androidx.annotation.NonNull(lines 11 and 32)androidx.lifecycle.ViewModelProvider(lines 12 and 33)androidx.navigation.NavDirections(lines 13 and 34)androidx.navigation.fragment.NavHostFragment(lines 14 and 35)🧹 Proposed fix
package org.commcare.fragments.personalId; import static android.app.Activity.RESULT_OK; import android.app.Activity; import android.os.Bundle; import android.view.LayoutInflater; import android.view.View; import android.view.ViewGroup; import androidx.annotation.NonNull; import androidx.lifecycle.ViewModelProvider; import androidx.navigation.NavDirections; import androidx.navigation.fragment.NavHostFragment; import com.google.android.material.bottomsheet.BottomSheetDialogFragment; import org.commcare.activities.SettingsHelper; import org.commcare.activities.connect.viewmodel.PersonalIdSessionDataViewModel; import org.commcare.android.database.connect.models.ConnectReleaseToggleRecord; import org.commcare.android.database.connect.models.PersonalIdSessionData; import androidx.biometric.BiometricManager; import org.commcare.connect.ConnectConstants; import org.commcare.connect.PersonalIdManager; import org.commcare.utils.BiometricsHelper; import org.commcare.connect.database.ConnectAppDatabaseUtil; import org.commcare.connect.database.ConnectDatabaseHelper; import org.commcare.dalvik.databinding.ScreenPersonalidMessageBinding; import org.commcare.utils.GeoUtils; -import androidx.annotation.NonNull; -import androidx.lifecycle.ViewModelProvider; -import androidx.navigation.NavDirections; -import androidx.navigation.fragment.NavHostFragment; import java.util.List; -import static android.app.Activity.RESULT_OK;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java` around lines 3 - 39, The file contains duplicate import statements—remove the redundant imports so each type is imported only once; specifically delete the repeated copies of android.app.Activity.RESULT_OK, androidx.annotation.NonNull, androidx.lifecycle.ViewModelProvider, androidx.navigation.NavDirections, and androidx.navigation.fragment.NavHostFragment, leaving a single import line for each to clean up the top-of-file import list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java`:
- Around line 3-39: The file contains duplicate import statements—remove the
redundant imports so each type is imported only once; specifically delete the
repeated copies of android.app.Activity.RESULT_OK, androidx.annotation.NonNull,
androidx.lifecycle.ViewModelProvider, androidx.navigation.NavDirections, and
androidx.navigation.fragment.NavHostFragment, leaving a single import line for
each to clean up the top-of-file import list.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.javaapp/unit-tests/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragmentTest.kt
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
left some design concerns on this to simplify code state flow
| binding.connectMessageMessage.setText(message); | ||
| binding.connectMessageButton.setText(buttonText); | ||
| setButton2Text(button2Text); | ||
| // When this screen is shown due to a biometric enrollment failure, the user may tap |
There was a problem hiding this comment.
think a better pattern to follow here would be to dismiss the bottom sheet as soon as we re-direct to settings screen and have the checks happen as part of the biometric fragment itself i.e. we don't want to load the message fragment with additional duplicate functionality like this and it's always nice to delegate this to the original fragment for a re-check.
There was a problem hiding this comment.
While testing, I found the issue with this approach. If the user directly adds the fingerprint from the settings screen by minimising our app, it keeps on showing this error message.
There was a problem hiding this comment.
Got it, Do you have any proposals on how best to manage it so that we don't need to add these checks in message fragment ? I don't think this is sustainable approach and may potentially result in a bunch of duplicated checks added in this fragment to re-validate error state on resuming app.
| boolean anyBiometricConfigured = | ||
| BiometricsHelper.isFingerprintConfigured(getActivity(), biometricManager) | ||
| || BiometricsHelper.isPinConfigured(getActivity(), biometricManager); | ||
| if (!anyBiometricConfigured) { | ||
| // Nothing configured — navigate to error message display | ||
| navigateForward(true); | ||
| } |
There was a problem hiding this comment.
think we should have a single method in this class that does all the required checks again after an expected config/state change from user and just call that method every time we need to reload state based on updated configuration, maybe that method is updateUiBasedOnMinSecurityRequired today.
This will keep the code state flow conscise here without us needing to check for configuration at bunch of different places.
There was a problem hiding this comment.
Here, the case is somewhat different, it's required to take the decision after returning from the setting screen's activity. So I'm not sure if that function fits because the app needs to show an error message if nothing is configured at onActivityResult.
There was a problem hiding this comment.
Would retriggering the initial checks that we do on loading the biometric screen not trigger this error ? Or in other words when does the original errror to configure fingerprint or pin gets triggered on this screen
2 participants
Product Description
The application was showing the false error message even when the fingerprint or PIN was configured successfully. Now, this issue is solved.
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-2163
RCA:
The bug is a false assumption about resultCode. The code assumes that if the system settings biometric enrolment activity does not return RESULT_ OK, enrolment has failed. This is incorrect. For configuring a fingerprint or PIN, the app opens a system activity, and this activity always returns RESULT_CANCELED (= 0) as the user has to navigate back a few screens after configuring the fingerprint or PIN. So that was wrong.
Safety Assurance
Safety story
Tested the application for configuring the PIN and fingerprint separately, and it's working fine.
Labels and Review