Pm 7615 #3011
Pm 7615 #3011
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new navigation action in the navigation graph ( Sequence Diagram(s)sequenceDiagram
participant User
participant Activity as ConnectIdActivity
participant SignupFrag as ConnectIDSignupFragment
participant SecondaryFrag as ConnectIDSecondaryPhoneNumber
participant Manager as ConnectManager
participant PinFrag as ConnectIdPinFragment
participant DB as ConnectDatabaseHelper
User->>Activity: Initiate Registration (Alternate Phone)
Activity->>SignupFrag: Navigate using action_connectid_signup_fragment_to_connectid_secondary_phone_fragment
SignupFrag->>SecondaryFrag: Load Secondary Phone Screen
SecondaryFrag->>Manager: Retrieve Primary Phone Number
SecondaryFrag->>SecondaryFrag: Validate secondary phone != primary phone
alt Validation Fails
SecondaryFrag-->>User: Display error message
else Validation Succeeds
SecondaryFrag->>PinFrag: Proceed with Registration
PinFrag->>DB: setRegistrationPhase(CONNECT_REGISTRATION_ALTERNATE_PHONE)
PinFrag->>User: Complete Registration Process
end
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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:
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/res/navigation/nav_graph_connectid.xml(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/activities/connect/ConnectIdActivity.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
app/src/org/commcare/activities/connect/ConnectIdActivity.java (1)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-79)
app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java (1)
app/src/org/commcare/connect/ConnectManager.java (1)
ConnectManager(86-1064)
🔇 Additional comments (4)
app/res/values/strings.xml (1)
793-793: New string resource for validation logicThis string resource is added to support the new validation feature that prevents users from using the same number for both primary and alternate phone numbers. Consider a slight grammatical improvement: "Primary and alternate phone numbers are the same".
app/res/navigation/nav_graph_connectid.xml (1)
290-292: Navigation path added for secondary phone registrationGood addition of a navigation action from the signup fragment to the secondary phone fragment. This supports the bug fix mentioned in the PR objectives by enabling proper flow between these screens during the registration process.
app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java (1)
402-402: Reordering setRegistrationPhase helps with app interruption scenarios.Moving the registration phase setting before the navigation setup ensures the state is saved immediately upon a successful operation. This is a critical fix for the bug where the registration process resumed incorrectly if the app was closed during navigation.
app/src/org/commcare/activities/connect/ConnectIdActivity.java (1)
103-106: Proper navigation to secondary phone fragment implemented.The implementation now uses the Navigation component to navigate directly to the secondary phone fragment instead of setting a fragment ID. This approach ensures that when resuming from the alternate phone registration phase, the user is directed to the correct screen with proper parameters.
| String userPrimaryNumber = ConnectManager.getUser(requireActivity()).getPrimaryPhone(); | ||
| if (userPrimaryNumber.equals(phone)) { | ||
| binding.errorTextView.setVisibility(View.VISIBLE); | ||
| binding.errorTextView.setText(R.string.primary_and_alternate_phone_number); | ||
| valid = false; | ||
| }else{ | ||
| binding.errorTextView.setVisibility(View.GONE); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fixed validation to prevent duplicate phone numbers
This change correctly implements validation to ensure the alternate phone number differs from the primary phone number, addressing the bug mentioned in the PR objectives. The error message display and button disabling logic is well implemented.
However, consider adding a null check before accessing the primary phone number to prevent potential NullPointerException if the user record is not available.
- String userPrimaryNumber = ConnectManager.getUser(requireActivity()).getPrimaryPhone();
- if (userPrimaryNumber.equals(phone)) {
+ ConnectUserRecord user = ConnectManager.getUser(requireActivity());
+ String userPrimaryNumber = user != null ? user.getPrimaryPhone() : null;
+ if (userPrimaryNumber != null && userPrimaryNumber.equals(phone)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String userPrimaryNumber = ConnectManager.getUser(requireActivity()).getPrimaryPhone(); | |
| if (userPrimaryNumber.equals(phone)) { | |
| binding.errorTextView.setVisibility(View.VISIBLE); | |
| binding.errorTextView.setText(R.string.primary_and_alternate_phone_number); | |
| valid = false; | |
| }else{ | |
| binding.errorTextView.setVisibility(View.GONE); | |
| } | |
| ConnectUserRecord user = ConnectManager.getUser(requireActivity()); | |
| String userPrimaryNumber = user != null ? user.getPrimaryPhone() : null; | |
| if (userPrimaryNumber != null && userPrimaryNumber.equals(phone)) { | |
| binding.errorTextView.setVisibility(View.VISIBLE); | |
| binding.errorTextView.setText(R.string.primary_and_alternate_phone_number); | |
| valid = false; | |
| } else { | |
| binding.errorTextView.setVisibility(View.GONE); | |
| } |
Product Description
Fixed bugs for alternate phone number is same as primary phone number
FIxed bug when user kill or close the app on secondary phone number registration starts from there
Technical Summary
https://dimagi.atlassian.net/browse/QA-7615
cross-request: dimagi/commcare-core#1455
Feature Flag
Safety Assurance
Safety story
Tested the changes at dev end
Automated test coverage
QA Plan
Labels and Review