- Restricted demo user from sending firebase otp #3124
Conversation
jaypanchal-13
requested review from
Jignesh-dimagi,
OrangeAndGreen,
pm-dimagi and
shubham1g5
📝 WalkthroughWalkthroughThe changes introduce support for a "demo user" flow in the Personal ID feature. A boolean argument Sequence Diagram(s)sequenceDiagram
participant User
participant PersonalIdPhoneFragment
participant PersonalIdBiometricConfigFragment
participant AppUtils
participant NameScreen
User->>PersonalIdPhoneFragment: Continue with phone
PersonalIdPhoneFragment->>PersonalIdBiometricConfigFragment: Navigate (with isDemoUser flag)
PersonalIdBiometricConfigFragment->>PersonalIdBiometricConfigFragment: onCreate (reads isDemoUser)
PersonalIdBiometricConfigFragment->>PersonalIdBiometricConfigFragment: navigateForward()
alt isDemoUser == false
PersonalIdBiometricConfigFragment->>OTPFragment: Navigate to OTP screen
else isDemoUser == true
PersonalIdBiometricConfigFragment->>AppUtils: showSnackBarWithOk()
User->>AppUtils: Tap "OK"
AppUtils->>PersonalIdBiometricConfigFragment: Callback
PersonalIdBiometricConfigFragment->>NameScreen: Navigate to Name screen
end
Suggested labels
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ 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: 1
🧹 Nitpick comments (4)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (2)
8-8: Unused import addedThe
android.util.Logimport is added but never used in the code.-import android.util.Log;
219-221: Inconsistency: Unused navigation methodThe
navigateToBiometricSetup()method is no longer used since the navigation logic was updated inonConfigurationSuccess().- private NavDirections navigateToBiometricSetup() { - return PersonalIdPhoneFragmentDirections.actionPersonalidPhoneFragmentToPersonalidBiometricConfig(); - }app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (2)
250-254: Improve code formatting in conditional navigation logic.The conditional navigation logic is functionally correct, but the formatting could be improved for better readability.
- if (!isDemoUser){ + if (!isDemoUser) { NavHostFragment.findNavController(this).navigate(navigateToOtpScreen()); - }else { - showSnackBarWithOk(getView(), getString(R.string.connect_verify_skip_phone_number), v -> NavHostFragment.findNavController(this).navigate(navigateToNameScreen())); + } else { + showSnackBarWithOk( + getView(), + getString(R.string.connect_verify_skip_phone_number), + v -> NavHostFragment.findNavController(this).navigate(navigateToNameScreen()) + ); }
250-254: Check for potential view nullability.The call to
getView()assumes the view is non-null, which should be the case during normal execution flow. However, it might be worth adding a null check for robustness.} else { - showSnackBarWithOk(getView(), getString(R.string.connect_verify_skip_phone_number), v -> NavHostFragment.findNavController(this).navigate(navigateToNameScreen())); + View view = getView(); + if (view != null) { + showSnackBarWithOk(view, getString(R.string.connect_verify_skip_phone_number), + v -> NavHostFragment.findNavController(this).navigate(navigateToNameScreen())); + } else { + // Fallback if view is null + NavHostFragment.findNavController(this).navigate(navigateToNameScreen()); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/res/navigation/nav_graph_personalid.xml(3 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/AppUtils.java(2 hunks)app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java(6 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (16)
app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java (1)
137-139: Good addition of a getter for the isDemo field.The implementation correctly exposes the existing
isDemofield for use by other components that will check for demo user status.app/res/values/strings.xml (1)
531-531: Well-placed string resource for demo user flow.This string resource is appropriately positioned among other phone verification strings and provides a clear message to demo users about skipping the phone verification process.
app/src/org/commcare/AppUtils.java (1)
208-220: Good utility method for persistent notification with acknowledgment.The
showSnackBarWithOkmethod is well-documented and provides a reusable way to display important messages that require user acknowledgment. This is a good approach for the demo user flow where you want to ensure the user is aware of skipped verification steps.app/res/values-sw/strings.xml (1)
580-580: Appropriate Swahili translation for demo user message.The Swahili translation is consistent with the English version and properly communicates that phone verification will be skipped for demo users.
app/res/values-ti/strings.xml (1)
437-437: String resource for demo user flow looks goodThis new string resource adds support for skipping phone verification for demo users in the Tigrinya language.
app/res/navigation/nav_graph_personalid.xml (3)
13-16: LGTM - Added demo user flag parameterAdding this boolean parameter to the biometric config fragment allows properly determining when a user is in demo mode.
25-28: LGTM - Added navigation to name screenThis new navigation action supports the demo user flow by allowing direct navigation from biometric config to the name screen, bypassing phone verification.
151-151: Code cleanup - removed trailing spaceMinor formatting improvement by removing an unnecessary trailing space.
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (2)
172-172: LGTM - Passing demo user flag to configuration success handlerGood implementation - passing the demo user flag from the session data to the success handler allows proper routing of demo users.
186-189: Improved navigation with Safe ArgsThe updated navigation implementation properly uses the Navigation Component's Safe Args pattern, which is more type-safe than the previous approach. The demo user flag is correctly passed to the destination fragment.
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (6)
3-5: Appropriate use of static imports.The imports are correctly added to support the new functionality, with
showSnackBarWithOkbeing statically imported fromAppUtilsfor cleaner code.
19-19: Well-handled navigation import.The addition of
NavHostFragmentimport supports the updated navigation logic in the fragment.
44-44: Good initialization of demo user flag.The boolean field is properly initialized with a default value of
false, which aligns with the expected behavior where most users are not demo users.
63-63: Correct argument extraction.The code properly extracts the
isDemoUserflag from the fragment arguments using the generatedPersonalIdBiometricConfigFragmentArgsclass, ensuring the demo user state is maintained throughout the navigation flow.
272-274: Well-structured navigation method.The new
navigateToNameScreen()method follows the same pattern as other navigation methods in the class, maintaining code consistency.
250-254:Details
✅ Verification successful
Verify the snackbar message for demo users.
The implementation correctly skips phone verification for demo users, but we should verify that the message string
R.string.connect_verify_skip_phone_numberclearly communicates this to users.
🏁 Script executed:
#!/bin/bash # Check the actual text of the string resource for the skip phone number message rg "connect_verify_skip_phone_number" --type xmlLength of output: 671
Snackbar message clarity verified
The string resource reads “I see you’re a demo user so we’ll skip the phone check,” which clearly informs demo users that phone verification is skipped. No changes needed.
app/res/values-pt/strings.xml
Outdated
| <string name="connect_verify_phone_resend">Não recebeu o seu código? Reenviar</string> | ||
| <string name="connect_verify_phone_error">Erro ao verificar o código SMS</string> | ||
| <string name="connect_verify_phone_resend_code">Reenviar código</string> | ||
| <string name="connect_verify_skip_phone_number">Veo que eres un usuario de demostración, así que omitiremos la verificación del teléfono</string> |
There was a problem hiding this comment.
Incorrect language in Portuguese string resource file
The added string resource is in Spanish ("Veo que eres un usuario de demostración...") instead of Portuguese, despite being in the Portuguese (pt) strings file.
This needs to be replaced with the proper Portuguese translation for consistency with the rest of the file.
🤖 Prompt for AI Agents
In app/res/values-pt/strings.xml at line 449, the string resource is incorrectly
written in Spanish instead of Portuguese. Replace the Spanish text with the
correct Portuguese translation to maintain language consistency in this
Portuguese strings file.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@jaypanchal-13 can we add a small video in the PR depicting the flow you have build just to get an overall feel of UX here.
@shubham1g5 right now code is not testable as some api work is pending before this flow starts. So will not be able to take video of this flow
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java
Outdated
Show resolved
Hide resolved
wil you not be able to mimick |
app/src/org/commcare/AppUtils.java
Outdated
| import android.view.View; | ||
|
|
||
| import com.google.android.material.snackbar.Snackbar; |
|
@shubham1g5 here is the video |
|
thanks @jaypanchal-13 for the video! It looks good to me and you can proceed with the merge on this. |
…mo-user # Conflicts: # app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java # app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-1210
Feature Flag
Improvements
Labels and Review