fixed wrong backup code bug #3159
Conversation
📝 WalkthroughWalkthroughThe changes update the Sequence Diagram(s)sequenceDiagram
participant User
participant PersonalIdMessageFragment
participant PersonalIdSessionDataViewModel
participant PersonalIdSessionData
participant NavigationController
User->>PersonalIdMessageFragment: onCreateView()
PersonalIdMessageFragment->>PersonalIdSessionDataViewModel: getInstance(activity)
PersonalIdSessionDataViewModel-->>PersonalIdMessageFragment: PersonalIdSessionData
User->>PersonalIdMessageFragment: Triggers finish() with result
alt result == PERSONALID_RECOVERY_ACCOUNT_ORPHANED
PersonalIdMessageFragment->>PersonalIdSessionData: set accountExists = false
PersonalIdMessageFragment->>NavigationController: navigate to backup code screen
else other results
PersonalIdMessageFragment->>NavigationController: handle as before
end
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java(3 hunks)
🔇 Additional comments (2)
app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java (2)
13-14: LGTM: Import statements are correct.The new imports for
PersonalIdSessionDataViewModel,PersonalIdSessionData, andViewModelProviderare necessary and correctly added for the functionality being implemented.Also applies to: 21-21
41-41: LGTM: Field declaration is appropriate.The private field declaration for
PersonalIdSessionDatafollows proper encapsulation and naming conventions.
| personalIdSessionData = new ViewModelProvider(requireActivity()).get( | ||
| PersonalIdSessionDataViewModel.class).getPersonalIdSessionData(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null safety check for ViewModel data.
The PersonalIdSessionData is retrieved from the ViewModel without null checking. This could cause issues if the ViewModel doesn't have the data initialized or returns null.
Consider adding a null check:
- personalIdSessionData = new ViewModelProvider(requireActivity()).get(
- PersonalIdSessionDataViewModel.class).getPersonalIdSessionData();
+ PersonalIdSessionDataViewModel viewModel = new ViewModelProvider(requireActivity())
+ .get(PersonalIdSessionDataViewModel.class);
+ personalIdSessionData = viewModel.getPersonalIdSessionData();
+ if (personalIdSessionData == null) {
+ // Handle the case where session data is not available
+ throw new IllegalStateException("PersonalIdSessionData not available");
+ }📝 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.
| personalIdSessionData = new ViewModelProvider(requireActivity()).get( | |
| PersonalIdSessionDataViewModel.class).getPersonalIdSessionData(); | |
| PersonalIdSessionDataViewModel viewModel = new ViewModelProvider(requireActivity()) | |
| .get(PersonalIdSessionDataViewModel.class); | |
| personalIdSessionData = viewModel.getPersonalIdSessionData(); | |
| if (personalIdSessionData == null) { | |
| // Handle the case where session data is not available | |
| throw new IllegalStateException("PersonalIdSessionData not available"); | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java at
lines 50-51, the code retrieves PersonalIdSessionData from the ViewModel without
checking for null, which may cause null pointer exceptions. Add a null check
after getting the data from the ViewModel to verify it is not null before using
it. If it is null, handle the case appropriately, such as showing an error
message or initializing default data.
| case ConnectConstants.PERSONALID_RECOVERY_ACCOUNT_ORPHANED: | ||
| personalIdSessionData.setAccountExists(false); | ||
| directions = navigateToBackupCode(); | ||
| break; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify null safety and logic correctness for the new case.
The new case handling for PERSONALID_RECOVERY_ACCOUNT_ORPHANED looks correct for the bug fix, but there's a potential null pointer risk since personalIdSessionData is used without null checking.
The logic correctly:
- Sets
accountExiststofalsewhen account is orphaned - Navigates to backup code screen for recovery
However, ensure personalIdSessionData is never null when this case is reached. Consider adding a null check or ensure the ViewModel initialization in onCreateView handles null cases properly.
Run this script to verify all usages of personalIdSessionData in the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Check for null safety patterns around PersonalIdSessionData usage
# Expected: Find patterns of null checking or initialization
# Search for PersonalIdSessionData usage patterns
rg -A 3 -B 3 "personalIdSessionData\." --type java
# Search for null checks on personalIdSessionData
rg "personalIdSessionData.*null" --type java
# Search for PersonalIdSessionDataViewModel usage patterns
rg -A 5 "PersonalIdSessionDataViewModel" --type javaLength of output: 25193
I’d like to inspect how personalIdSessionData is initialized and used in PersonalIdMessageFragment to confirm it can’t be null:
🏁 Script executed:
#!/bin/bash
# Inspect class declaration and key methods in PersonalIdMessageFragment
rg "class PersonalIdMessageFragment" -n app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java
rg "onCreateView" -n -C5 app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java
rg "loadArguments" -n -C5 app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java
rg "handleContinueButtonPress" -n -C5 app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.javaLength of output: 3235
I need to inspect the finish() method in PersonalIdMessageFragment to see exactly how personalIdSessionData is used there:
🏁 Script executed:
#!/bin/bash
# Show the finish() implementation around line 120
rg -n -A30 -B5 "private void finish" --context-separator="-" app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.javaLength of output: 1797
I need to confirm how the PersonalIdSessionDataViewModel initializes its data and where setPersonalIdSessionData is invoked:
🏁 Script executed:
#!/bin/bash
# 1. Dump the ViewModel implementation to see default initialization
sed -n '1,200p' app/src/org/commcare/activities/connect/viewmodel/PersonalIdSessionDataViewModel.java
# 2. Find all call sites for setting the session data
rg -n "setPersonalIdSessionData" --type javaLength of output: 1236
Add null-check for personalIdSessionData to avoid NPE
In PersonalIdMessageFragment.finish() (lines 143–146) you call
personalIdSessionData.setAccountExists(false);but getPersonalIdSessionData() can return null if setPersonalIdSessionData(...) wasn’t called. Add a non-null assertion or guard immediately after you retrieve it in onCreateView, for example:
@@ app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java:47
- personalIdSessionData = new ViewModelProvider(requireActivity())
- .get(PersonalIdSessionDataViewModel.class).getPersonalIdSessionData();
+ personalIdSessionData = new ViewModelProvider(requireActivity())
+ .get(PersonalIdSessionDataViewModel.class).getPersonalIdSessionData();
+ Objects.requireNonNull(personalIdSessionData,
+ "PersonalIdSessionData must be initialized before showing this fragment");Or guard its use in finish():
@@ app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java:143
- case ConnectConstants.PERSONALID_RECOVERY_ACCOUNT_ORPHANED:
- personalIdSessionData.setAccountExists(false);
- directions = navigateToBackupCode();
- break;
+ case ConnectConstants.PERSONALID_RECOVERY_ACCOUNT_ORPHANED:
+ if (personalIdSessionData == null) {
+ throw new IllegalStateException("Session data unavailable on orphaned recovery");
+ }
+ personalIdSessionData.setAccountExists(false);
+ directions = navigateToBackupCode();
+ break;– File: app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java
– Location: onCreateView() initialization or finish() switch branch
This will prevent a potential NullPointerException when the ViewModel hasn’t been populated.
🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java
around lines 143 to 146, the code uses personalIdSessionData without checking
for null, which risks a NullPointerException if the ViewModel data is not
initialized. To fix this, add a null check for personalIdSessionData either
immediately after retrieving it in onCreateView or before using it in the
finish() method's switch case. If null, handle gracefully or assert non-null to
ensure safe usage and prevent crashes.
| break; | ||
| case ConnectConstants.PERSONALID_RECOVERY_ACCOUNT_ORPHANED: | ||
| personalIdSessionData.setAccountExists(false); | ||
| directions = navigateToBackupCode(); |
There was a problem hiding this comment.
can user still try more back up codes after this happens ?
There was a problem hiding this comment.
no cannot it will landup to set up new code only
Product Description
https://dimagi.atlassian.net/browse/QA-7822
fixed when user types 3 time wrong backup code and navigate issue
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review