[Draft][Do Not Merge] skip biometric page for automated testing #3491
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 pull request modifies the PersonalId feature's navigation flow by redirecting users to an OTP page after successful phone number configuration instead of proceeding to biometric setup. The changes include adding two new navigation actions in the navigation XML graph and updating the PersonalIdPhoneFragment to invoke the new OTP navigation path upon successful configuration. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 Fix all issues with AI agents
In `@app/res/navigation/nav_graph_personalid.xml`:
- Around line 126-128: Remove the now-dead navigation action and its unused
caller: delete the <action> element with id
action_personalid_phone_fragment_to_personalid_biometric_config from the
nav_graph_personalid.xml and remove the unused method navigateToBiometricSetup()
from PersonalIdPhoneFragment (and any private helper it relies on) so there is
no unreachable navigation path; ensure only the active
action_personalid_phone_fragment_to_personalid_otp remains and update
imports/usages in PersonalIdPhoneFragment to eliminate any leftover references
or warnings.
In `@app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java`:
- Around line 511-514: The onConfigurationSuccess method currently always
navigates to navigateToOtp, bypassing biometric setup for all users; change it
to conditionally bypass only for automated/testing environments by checking a
test flag (e.g., BuildConfig.DEBUG && isAutomatedTest() or a dedicated
feature/test flag) and otherwise navigate to navigateToBiometricSetup; add or
reuse an isAutomatedTest() helper or configuration flag and update
onConfigurationSuccess to choose between navigateToOtp() and
navigateToBiometricSetup() based on that condition.
🧹 Nitpick comments (1)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
533-539: Dead code:navigateToBiometricSetup()is no longer called.The
navigateToBiometricSetup()method (lines 533-535) is now unused. Along with the commented-out line at 512, this creates dead code.For a draft PR, consider adding a TODO comment to clarify the intent:
private void onConfigurationSuccess() { -// Navigation.findNavController(binding.personalidPhoneContinueButton).navigate(navigateToBiometricSetup()); + // TODO: Restore biometric flow once we finalize the automated testing approach + // Navigation.findNavController(binding.personalidPhoneContinueButton).navigate(navigateToBiometricSetup()); Navigation.findNavController(binding.personalidPhoneContinueButton).navigate(navigateToOtp()); }When the final approach is decided, either remove
navigateToBiometricSetup()entirely or restore the conditional flow.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/res/navigation/nav_graph_personalid.xmlapp/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
🧰 Additional context used
🧠 Learnings (2)
📓 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.
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.
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 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.
📚 Learning: 2025-05-09T10:57:41.073Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 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.
Applied to files:
app/res/navigation/nav_graph_personalid.xmlapp/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
126
to
128
| <action | ||
| android:id="@+id/action_personalid_phone_fragment_to_personalid_otp" | ||
| app:destination="@id/personalid_otp_page" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the biometric action from phone fragment is still used anywhere
rg -n "actionPersonalidPhoneFragmentToPersonalidBiometricConfig"Repository: dimagi/commcare-android
Length of output: 250
🏁 Script executed:
#!/bin/bash
# Get context around the usage at line 534
sed -n '520,550p' app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.javaRepository: dimagi/commcare-android
Length of output: 1363
🏁 Script executed:
#!/bin/bash
# Check where navigateToBiometricSetup and navigateToOtp are actually called
echo "=== Calls to navigateToBiometricSetup() ==="
rg -n "navigateToBiometricSetup()"
echo ""
echo "=== Calls to navigateToOtp() ==="
rg -n "navigateToOtp()"Repository: dimagi/commcare-android
Length of output: 1075
🏁 Script executed:
#!/bin/bash
# Check if personalid_biometric_config is referenced elsewhere in the navigation graph
rg -n "personalid_biometric_config" app/res/navigation/nav_graph_personalid.xmlRepository: dimagi/commcare-android
Length of output: 967
Remove the dead navigation action from phone fragment.
The action action_personalid_phone_fragment_to_personalid_biometric_config (lines 124-125) is now unreachable—the method navigateToBiometricSetup() that calls it is commented out in PersonalIdPhoneFragment.java:512. The active flow navigates directly to OTP via the new action. Delete the dead action and the now-unused navigateToBiometricSetup() method.
🤖 Prompt for AI Agents
In `@app/res/navigation/nav_graph_personalid.xml` around lines 126 - 128, Remove
the now-dead navigation action and its unused caller: delete the <action>
element with id action_personalid_phone_fragment_to_personalid_biometric_config
from the nav_graph_personalid.xml and remove the unused method
navigateToBiometricSetup() from PersonalIdPhoneFragment (and any private helper
it relies on) so there is no unreachable navigation path; ensure only the active
action_personalid_phone_fragment_to_personalid_otp remains and update
imports/usages in PersonalIdPhoneFragment to eliminate any leftover references
or warnings.
Comment on lines
511
to
516
| private void onConfigurationSuccess() { | ||
| Navigation.findNavController(binding.personalidPhoneContinueButton).navigate(navigateToBiometricSetup()); | ||
| // Navigation.findNavController(binding.personalidPhoneContinueButton).navigate(navigateToBiometricSetup()); | ||
| Navigation.findNavController(binding.personalidPhoneContinueButton).navigate(navigateToOtp()); | ||
| } |
There was a problem hiding this comment.
Hardcoded biometric bypass affects all users, not just automated tests.
This change completely bypasses biometric setup for all users. Per the commit message, this is intentional and temporary. However, if merged as-is:
- Security regression: Biometric authentication is a security feature that will be skipped for production users
- No conditional logic: There's no flag/check to enable this only for automated testing (e.g.,
BuildConfig.DEBUG, test flag, or environment variable)
Consider implementing a conditional approach before finalizing:
private void onConfigurationSuccess() {
if (BuildConfig.DEBUG && isAutomatedTest()) {
Navigation.findNavController(binding.personalidPhoneContinueButton).navigate(navigateToOtp());
} else {
Navigation.findNavController(binding.personalidPhoneContinueButton).navigate(navigateToBiometricSetup());
}
}🤖 Prompt for AI Agents
In `@app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java`
around lines 511 - 514, The onConfigurationSuccess method currently always
navigates to navigateToOtp, bypassing biometric setup for all users; change it
to conditionally bypass only for automated/testing environments by checking a
test flag (e.g., BuildConfig.DEBUG && isAutomatedTest() or a dedicated
feature/test flag) and otherwise navigate to navigateToBiometricSetup; add or
reuse an isAutomatedTest() helper or configuration flag and update
onConfigurationSuccess to choose between navigateToOtp() and
navigateToBiometricSetup() based on that condition.
… until we decide on the aproach
shubham1g5
force-pushed
the
qa_automation
branch
from
61d1276 to
47789d7
Compare
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.
Product Description
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review