Skip to content

fixed wrong backup code bug #3159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shubham1g5 merged 1 commit into from
Jun 6, 2025
Merged

fixed wrong backup code bug #3159

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,15 @@

import org.commcare.activities.SettingsHelper;
import org.commcare.activities.connect.PersonalIdActivity;
import org.commcare.activities.connect.viewmodel.PersonalIdSessionDataViewModel;
import org.commcare.android.database.connect.models.PersonalIdSessionData;
import org.commcare.connect.ConnectConstants;
import org.commcare.connect.PersonalIdManager;
import org.commcare.connect.database.ConnectDatabaseHelper;
import org.commcare.dalvik.databinding.ScreenPersonalidMessageBinding;

import androidx.annotation.NonNull;
import androidx.lifecycle.ViewModelProvider;
import androidx.navigation.NavDirections;
import androidx.navigation.fragment.NavHostFragment;

Expand All @@ -35,13 +38,17 @@ public class PersonalIdMessageFragment extends BottomSheetDialogFragment {
private static final String KEY_CALLING_CLASS = "calling_class";
private static final String KEY_IS_CANCELLABLE = "is_cancellable";
private ScreenPersonalidMessageBinding binding;
private PersonalIdSessionData personalIdSessionData;


@Override
public View onCreateView(@NonNull LayoutInflater inflater, ViewGroup container,
Bundle savedInstanceState) {
binding = ScreenPersonalidMessageBinding.inflate(inflater, container, false);
View view = binding.getRoot();
loadSavedState(savedInstanceState);
personalIdSessionData = new ViewModelProvider(requireActivity()).get(
PersonalIdSessionDataViewModel.class).getPersonalIdSessionData();
Comment on lines +50 to +51
Copy link

@coderabbitai coderabbitai bot Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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.

binding.connectMessageButton.setOnClickListener(v -> handleContinueButtonPress());
binding.connectMessageButton2.setOnClickListener(v -> handleContinueButtonPress());
loadArguments();
Expand Down Expand Up @@ -133,6 +140,10 @@ private void finish() {
case ConnectConstants.PERSONALID_DEVICE_CONFIGURATION_FAILED:
directions = navigateToPhoneFragment();
break;
case ConnectConstants.PERSONALID_RECOVERY_ACCOUNT_ORPHANED:
personalIdSessionData.setAccountExists(false);
directions = navigateToBackupCode();
Copy link
Contributor

@shubham1g5 shubham1g5 Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can user still try more back up codes after this happens ?

Copy link
Contributor Author

@pm-dimagi pm-dimagi Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no cannot it will landup to set up new code only

Copy link
Contributor

@shubham1g5 shubham1g5 Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks

break;
Comment on lines +143 to +146
Copy link

@coderabbitai coderabbitai bot Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  1. Sets accountExists to false when account is orphaned
  2. 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 java

Length 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.java

Length 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.java

Length 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 java

Length 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.


}
if (directions != null) {
Expand Down