Backup Code page redirection #3137
Conversation
…nto pm_ccct_1223_2 # Conflicts: # app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java
📝 WalkthroughWalkthroughThis set of changes refactors the backup code recovery flow in the personal ID feature. It updates navigation arguments by adding a new Sequence Diagram(s)sequenceDiagram
participant User
participant PersonalIdPhoneFragment
participant PersonalIdNameFragment
participant PersonalIdBackupCodeFragment
participant PersonalIdSessionDataViewModel
participant PersonalIdApiHandler
participant Navigation
User->>PersonalIdPhoneFragment: Enter phone number
PersonalIdPhoneFragment->>PersonalIdSessionDataViewModel: Store phone number
PersonalIdPhoneFragment->>Navigation: Navigate to Name Fragment
User->>PersonalIdNameFragment: Enter name
PersonalIdNameFragment->>PersonalIdSessionDataViewModel: Store user name
PersonalIdNameFragment->>Navigation: Navigate to Backup Code Fragment
User->>PersonalIdBackupCodeFragment: Enter backup code
PersonalIdBackupCodeFragment->>PersonalIdSessionDataViewModel: Access session data
PersonalIdBackupCodeFragment->>PersonalIdApiHandler: Confirm backup code
PersonalIdApiHandler-->>PersonalIdBackupCodeFragment: Success/Failure
alt Success
PersonalIdBackupCodeFragment->>Navigation: Navigate to Photo Capture Fragment or Success Message
else Failure
PersonalIdBackupCodeFragment->>Navigation: Navigate to Recovery Failed Message
end
Possibly related PRs
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:
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: 0
🔭 Outside diff range comments (2)
app/src/org/commcare/connect/network/PersonalIdApiHandler.java (1)
97-102: 💡 Verification agent🧩 Analysis chain
Good improvement to error specificity.
Changing the default error code from
INVALID_RESPONSE_ERRORtoWRONG_BACKUP_CODEfor theconfirmBackupCodemethod provides more specific error handling, allowing the UI to display appropriate messages to users.Verify that the error handler properly handles this new error code:
🏁 Script executed:
#!/bin/bash # Description: Verify WRONG_BACKUP_CODE is handled in PersonalIdApiErrorHandler # Check if PersonalIdApiErrorHandler handles the new error code ast-grep --pattern 'case WRONG_BACKUP_CODE:' # Also check for any switch statements on PersonalIdApiErrorCodes rg -A 10 'switch.*PersonalIdApiErrorCodes|case.*WRONG_BACKUP_CODE'Length of output: 115
🏁 Script executed:
#!/bin/bash # Locate the PersonalIdApiErrorHandler source file handler_file=$(rg --files-with-matches "class PersonalIdApiErrorHandler") echo "Found handler at: $handler_file" echo # Search the handler for explicit handling of WRONG_BACKUP_CODE echo "Checking for WRONG_BACKUP_CODE occurrences:" rg -n "WRONG_BACKUP_CODE" "$handler_file" || echo "No matches for WRONG_BACKUP_CODE" echo # List switch/case blocks on PersonalIdApiErrorCodes echo "Listing switch statements on PersonalIdApiErrorCodes:" rg -n -A5 "switch.*PersonalIdApiErrorCodes" "$handler_file" || echo "No switch on PersonalIdApiErrorCodes found"Length of output: 932
Handle new WRONG_BACKUP_CODE in the error handler
The
PersonalIdApiErrorHandlernever checks for the newly introducedWRONG_BACKUP_CODE, so callers will fall through to a generic error. Please add an explicit branch for this code:• File:
app/src/org/commcare/connect/network/PersonalIdApiErrorHandler.java
– Locate the switch (or if/else) onPersonalIdApiErrorCodes
– Add something like:case WRONG_BACKUP_CODE: // Map to a user‐friendly message or specific UI state return new PersonalIdApiError( PersonalIdApiErrorCodes.WRONG_BACKUP_CODE, "The backup code you entered is incorrect.");That way, the UI can present the precise “wrong backup code” message instead of a generic failure.
app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java (1)
80-104: 💡 Verification agent🧩 Analysis chain
Potential data loss with alternatePhone handling.
The constructor and
getUserFromIntentno longer handlealternatePhone, butputUserInIntentstill includes it (line 103). This creates an asymmetry where:
- New records created via constructor will have null
alternatePhone- Records retrieved from intent won't preserve the alternate phone
- But when putting user in intent, a null value will be stored
Verify if this is intentional or if the alternatePhone handling should be completely removed:
🏁 Script executed:
#!/bin/bash # Description: Check usage of alternatePhone field and ALT_PHONE constant # Check if alternatePhone is still being used elsewhere rg -A 3 'setAlternatePhone|getAlternatePhone|alternatePhone' # Check usage of ALT_PHONE constant rg -A 3 'ALT_PHONE' # Check if any code still tries to extract ALT_PHONE from intents ast-grep --pattern 'getStringExtra(ConnectConstants.ALT_PHONE)'Length of output: 6231
Fix asymmetric alternatePhone handling in getUserFromIntent
The
getUserFromIntentmethod never reads backALT_PHONE, so any alternate phone put into the intent will be lost. Update it to pullALT_PHONEand set it on the record:In app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:
public static ConnectUserRecord getUserFromIntent(Intent intent) { - return new ConnectUserRecord( + ConnectUserRecord user = new ConnectUserRecord( intent.getStringExtra(ConnectConstants.PHONE), intent.getStringExtra(ConnectConstants.USERNAME), intent.getStringExtra(ConnectConstants.PASSWORD), intent.getStringExtra(ConnectConstants.NAME)); + user.setAlternatePhone(intent.getStringExtra(ConnectConstants.ALT_PHONE)); + return user; }• Ensures symmetry with
putUserInIntent
• Prevents losing the alternatePhone value when rehydrating from an Intent
🧹 Nitpick comments (1)
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (1)
127-142: Consider improving error message readability.The validation logic is correct, but the nested ternary operator for error messages could be more readable.
Consider extracting the error message logic for better readability:
- String errorText = !isValid && code1.length() != BACKUP_CODE_LENGTH - ? getString(R.string.connect_pin_length, BACKUP_CODE_LENGTH) - : (!isRecovery && !code1.equals(code2)) - ? getString(R.string.connect_pin_mismatch) - : ""; + String errorText = ""; + if (!isValid) { + if (code1.length() != BACKUP_CODE_LENGTH) { + errorText = getString(R.string.connect_pin_length, BACKUP_CODE_LENGTH); + } else if (!isRecovery && !code1.equals(code2)) { + errorText = getString(R.string.connect_pin_mismatch); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/res/navigation/nav_graph_personalid.xml(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java(2 hunks)app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt(1 hunks)app/src/org/commcare/connect/network/PersonalIdApiHandler.java(2 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java(5 hunks)app/src/org/commcare/fragments/personalId/PersonalIdNameFragment.java(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java (1)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-63)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
app/src/org/commcare/utils/PhoneNumberHelper.java (1)
PhoneNumberHelper(35-180)
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (6)
app/src/org/commcare/utils/KeyboardHelper.java (1)
KeyboardHelper(14-39)app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-63)app/src/org/commcare/connect/network/PersonalIdApiErrorHandler.java (1)
PersonalIdApiErrorHandler(22-52)app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (1)
ConnectDatabaseHelper(27-117)app/src/org/commcare/connect/PersonalIdManager.java (1)
PersonalIdManager(63-664)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
FirebaseAnalyticsUtil(38-524)
🔇 Additional comments (15)
app/res/values/strings.xml (1)
927-928: LGTM! Clear and appropriate string resources.The new string resources
recovery_failedandrecovery_failed_messageprovide clear messaging for recovery failure scenarios and integrate well with the Personal ID backup code recovery flow improvements.app/src/org/commcare/fragments/personalId/PersonalIdNameFragment.java (1)
80-80: LGTM! Proper session data management.The addition of
sessionData.setUserName()correctly stores the trimmed user name in the shared session data, aligning with the refactoring to centralize user information management inPersonalIdSessionData.app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt (1)
17-22:Details
✅ Verification successful
Verify property renaming doesn't break existing code.
The renaming of
usernametouserIdand addition of new properties looks good for enhanced session management. However, ensure that all existing references to the oldusernameproperty have been updated throughout the codebase.Run the following script to verify no existing code still references the old
usernameproperty:
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the old 'username' property in PersonalIdSessionData rg -A 3 -B 3 "\.username\b" --type java --type ktLength of output: 78
🏁 Script executed:
#!/bin/bash # Re-run search for any references to the old 'username' property in Kotlin and Java files rg -A 3 -B 3 "\.username\b" --glob "*.kt" --glob "*.java"Length of output: 11183
🏁 Script executed:
#!/bin/bash # Search for any references to the PersonalIdSessionData class to verify usage and property access rg -n "PersonalIdSessionData" --glob "*.kt" --glob "*.java"Length of output: 9957
#!/bin/bash # Check for any direct or getter-based references to the old 'username' property on PersonalIdSessionData rg -n "getUsername" --glob "*.java" --glob "*.kt" rg -n "sessionData.username" --glob "*.kt"
Rename Safe to Merge: no remaining
usernamereferences on PersonalIdSessionData
- Ran grep across
.ktand.javafiles; found no calls tosessionData.usernameorgetUsername()onPersonalIdSessionData.- The new nullable fields
userNameandphoneNumberare correctly scoped.All clear to merge.
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (4)
50-50: LGTM! Good refactoring to use instance variable.The addition of the
phoneinstance variable eliminates code duplication and provides better state management within the fragment.
132-135: LGTM! Consistent phone number formatting.The use of
PhoneNumberHelper.buildPhoneNumber()ensures consistent phone number formatting by removing special characters and combining country code with phone number.
173-176: LGTM! Consistent implementation pattern.The phone number construction follows the same pattern as in
updateContinueButtonState(), maintaining consistency throughout the fragment.
197-197: LGTM! Proper session data update.Setting the phone number in
PersonalIdSessionDataaligns with the centralized session management pattern and ensures the phone number is available for subsequent fragments in the flow.app/res/navigation/nav_graph_personalid.xml (1)
167-172:Details
✅ Verification successful
Verify null safety for navigation arguments.
The removal of the default value for
userNameand the addition ofbackupCodewithout nullability specification could lead to runtime crashes if these arguments are not provided during navigation.Run the following script to verify all navigation calls to this destination include the required arguments:
🏁 Script executed:
#!/bin/bash # Description: Verify all navigation calls to personalid_photo_capture include required arguments # Search for navigation actions to personalid_photo_capture ast-grep --pattern 'actionPersonalidPinToPersonalidPhotoCapture($$$)' # Search for any navigation to personalid_photo_capture using NavController rg -A 5 'personalid_photo_capture|action.*PersonalidPhotoCapture'Length of output: 3762
Navigation argument usage verified
All calls to
personalid_photo_captureexplicitly pass bothuserNameandbackupCode. Removing the default foruserNamewon’t introduce any missing-argument crashes, and the newbackupCodeargument is correctly supplied inPersonalIdBackupCodeFragment. No further changes needed.app/src/org/commcare/connect/network/PersonalIdApiHandler.java (1)
27-34: Consider retry behavior for WRONG_BACKUP_CODE.The new
WRONG_BACKUP_CODEerror is not included in theshouldAllowRetry()method. This means users won't be able to retry automatically if they enter a wrong backup code, which seems intentional for security reasons.app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (6)
12-42: Good architectural improvement with ViewModel usage.The refactoring to use
PersonalIdSessionDataViewModelfor session data management is a significant improvement, providing better separation of concerns and data persistence across configuration changes.
43-64: Clean implementation of lifecycle methods.Good use of the
KeyboardHelperutility and proper initialization of the ViewModel. The derivation ofisRecoveryfrompersonalIdSessionData.getAccountExists()is logical.
88-93: Proper use of session data for UI updates.The method correctly uses session data to populate the user name and photo, with appropriate null checking for the photo.
107-114: Correct placement of validation logic.Moving the validation call from
afterTextChangedtoonTextChangedprovides more immediate feedback to users as they type, improving the user experience.
144-150: Good separation of concerns.Removing the direct API call for backup code registration and using navigation instead properly separates the fragment's responsibilities.
205-210:Details
✅ Verification successful
Verify secure handling of backup code in navigation.
The backup code is being passed as a navigation argument. Ensure this sensitive data is not logged or exposed in the navigation backstack.
🏁 Script executed:
#!/bin/bash # Description: Check if backup code is properly handled in the destination fragment # Check how PersonalIdPhotoCaptureFragment handles the backup code argument ast-grep --pattern 'class PersonalIdPhotoCaptureFragment { $$$ }' # Look for any logging of navigation arguments rg -A 5 'NavArgs|navigation.*argument.*log|Log.*nav'Length of output: 70120
🏁 Script executed:
#!/bin/bash # Locate the destination fragment and inspect how the backup code argument is handled # Find the fragment declaration rg -n "class PersonalIdPhotoCaptureFragment" -C3 app/src # Find SafeArgs usage for this fragment rg -n "PersonalIdPhotoCaptureFragmentArgs" -C3 app/src # Check where arguments are unpacked rg -n "fromBundle" -C3 app/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.java # Ensure no logging of the pin or userName rg -n "Log\." -C3 app/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.javaLength of output: 2165
🏁 Script executed:
#!/bin/bash # Verify how the backup code (pin) argument is accessed and used in the destination fragment # Search for getPin in the fragment rg -n "getPin" -C3 app/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.java # Inspect any storage or logging of the pin rg -n "pin" -C5 app/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.javaLength of output: 568
Secure handling of backup code verified
No
Logstatements reference the PIN inPersonalIdPhotoCaptureFragment. The backup code is passed via SafeArgs and only stored in the fragment’s argument bundle, which isn’t logged or printed by default.• No logging of the PIN in destination fragment
• SafeArgs-based nav args aren’t exposed in logs
• (Optional) If you need to prevent the PIN from remaining in the backstack, clear or consume the argument after use or manage it via ViewModel
pm-dimagi
assigned
pm-dimagi and unassigned
OrangeAndGreen,
shubham1g5 and
Jignesh-dimagi
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Seeing a couple issues in the backup code fragment, hopefully nothing too difficult to address.
|
|
||
| String backupcode1 = String.valueOf(binding.connectPinInput.getText()); | ||
| String code2 = String.valueOf(binding.connectPinRepeatInput.getText()); | ||
| boolean isValid = false; |
There was a problem hiding this comment.
Something's wrong here. isValid starts false, then we check for !isValid a couple lines later (always true).
Also, the value is never changed after initialization, so the Pin button will always be disabled. Looks like the else case is missing (i.e. where we used to set isButtonEnabled = true).
| new PersonalIdApiHandler() { | ||
| @Override | ||
| protected void onSuccess(PersonalIdSessionData sessionData) { | ||
| if (Boolean.FALSE.equals(sessionData.getAccountOrphaned())) { |
There was a problem hiding this comment.
Seems like this might not handle the null case correctly? Like if the user successfully enters their backup code the first time, I think getAccountOrphaned() will return null, and this code will incorrectly go to the error case
There was a problem hiding this comment.
Here we have passed the fallback value as false whenever there is null it will be considered as false
| @Override | ||
| protected void onFailure(PersonalIdApiErrorCodes failureCode) { | ||
| navigateFailure(failureCode); | ||
| if (failureCode == PersonalIdApiErrorCodes.WRONG_BACKUP_CODE) { |
There was a problem hiding this comment.
This seems wrong to me, I think the call will still succeed (i.e. HTTP 200) when they enter the wrong code but will send the account_orphaned flag to indicate the failure and whether the user can try again.
So after the call, if the session data contains the final fields (CID, DB passphrase, password) we know the call succeeded. Otherwise we'll have gotten the account_orphaned flag, and we can check that to determine whether to give the user another chance.
There was a problem hiding this comment.
I Think when there is wrong code input then it will be the failure case not 200 , 200 will be the case when account is orphoned or success.. this is i am saying on the basis of the previous api calls
There was a problem hiding this comment.
can we verify this with server impl, currently handleFailedBackupCodeAttempt is in both onSuccess and onFailure so seems like we are not really sure of the expected behaviour here.
There was a problem hiding this comment.
I will do that with charl today
|
|
||
| private void handleFailedBackupCodeAttempt() { | ||
| PersonalIdManager idManager = PersonalIdManager.getInstance(); | ||
| idManager.setFailureAttempt(idManager.getFailureAttempt() + 1); |
There was a problem hiding this comment.
Hopefully we'll get the change for the server to tell us this value, since we can't accurately track it over time. In other words, if the user restarts the workflow or reinstalls the app, the server will still know they only have 2 tries left but the app won't.
| ConnectUserDatabaseUtil.storeUser(getActivity(), user); | ||
| } | ||
| } | ||
| private void navigateWithMessage(int titleRes, int msgRes, int phase) { |
There was a problem hiding this comment.
Nit: Seems like sometimes we return NavDirections for the caller to navigate on, and sometimes (as in this function) we actually do the navigation as well. I'm in favor of this version, where the caller can assume the navigation is performed when they call the navigate... function. It's hard to imagine a scenario where we would create NavDirections but not immediately navigate to them, and returning them runs the risk of the caller forgetting to handle the navigation (a bug I encountered in different code yesterday)
|
Also note there's currently a compile error in ConfirmBackupCodeResponseParser due to the changed name for setUsername in PersonalIdSessionData |
| public void onResume() { | ||
| super.onResume(); | ||
| requestInputFocus(); | ||
| KeyboardHelper.showKeyboardOnInput(activity, binding.connectPinInput); |
There was a problem hiding this comment.
i will refactor all the code from pin to backup in seprate pr
app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| protected void onFailure(PersonalIdApiErrorCodes failureCode) { | ||
| navigateFailure(failureCode); | ||
| if (failureCode == PersonalIdApiErrorCodes.WRONG_BACKUP_CODE) { |
There was a problem hiding this comment.
can we verify this with server impl, currently handleFailedBackupCodeAttempt is in both onSuccess and onFailure so seems like we are not really sure of the expected behaviour here.
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java
Outdated
Show resolved
Hide resolved
…tabase on success, refactored the api call as common standard
|
@damagatchi retest this please |
…nto pm_ccct_1223_2
…ndroid into pm_ccct_1223_3 # Conflicts: # app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java
|
@damagatchi retest this please |
…nto pm_ccct_1223_2
…ndroid into pm_ccct_1223_3
Complete profile refactor
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Mostly nits but also a couple changes that need to be made
app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java
Outdated
Show resolved
Hide resolved
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Just one small nit, but not a big deal so approved.
| activity = requireActivity(); | ||
|
|
||
| initArguments(savedInstanceState); | ||
| personalIdSessionDataViewModel = new ViewModelProvider(requireActivity()).get( |
There was a problem hiding this comment.
Nit: Don't need personalIdSessionDataViewModel at all, can just do it like here
|
@damagatchi retest this please |
Product Description
Handled Backup code redirection in case of recovery and set pin
Refactored code removed unused stuffs
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review