Complete profile refactor

[pm-dimagi]

Product Description

Refactored the complete profile page on the following

• API call as per the common way
• removed connect userecord take the data from session
• created the data base after success from the server

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Labels and Review

• Do we need to enhance the manual QA test coverage ? If yes, the "QA Note" label is set correctly
• Does the PR introduce any major changes worth communicating ? If yes, the "Release Note" label is set and a "Release Note" is specified in PR description.
• Risk label is set correctly
• The set of people pinged as reviewers is appropriate for the level of risk of the change

[pm-dimagi]

@coderabbitai full review please

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

I'll help you with a full review of PR #3139. Let me gather the necessary information about the pull request first.

---

🏁 Script executed:

#!/bin/bash

# Get PR details including title, description, and metadata
echo "=== PR Details ==="
gh pr view 3139 --json title,body,state,author,reviewDecision,files,additions,deletions,changedFiles

echo -e "\n=== Files Changed ==="
gh pr view 3139 --json files | jq -r '.files[].path'

echo -e "\n=== PR Diff ==="
gh pr diff 3139

Length of output: 14148

---

Now let me examine the specific files and conduct a deeper analysis of the codebase to understand the full context and potential impacts.

---

🏁 Script executed:

#!/bin/bash

# Search for any other usages of the old setPhotoAndCompleteProfile method signature
echo "=== Searching for other usages of setPhotoAndCompleteProfile ==="
rg "setPhotoAndCompleteProfile" -A 3 -B 1

echo -e "\n=== Checking for CompleteProfileResponseParser ==="
fd "CompleteProfileResponseParser" --type f

echo -e "\n=== Searching for any direct usage of ConnectUserRecord in PersonalId related files ==="
rg "ConnectUserRecord" app/src/org/commcare/fragments/personalId/ -A 2 -B 1

echo -e "\n=== Checking PersonalIdManager usage ==="
rg "PersonalIdManager.*getUser" -A 2 -B 1

echo -e "\n=== Examining the PersonalIdSessionDataViewModel ==="
fd "PersonalIdSessionDataViewModel" --type f -x cat {}

Length of output: 7594

---

🏁 Script executed:

#!/bin/bash

# Check for CompleteProfileResponseParser and related response parsers
echo "=== Searching for response parsers in the codebase ==="
rg "ResponseParser" app/src/org/commcare/connect/network/parser/ -l

echo -e "\n=== Examining existing response parsers ==="
fd "ResponseParser" app/src/org/commcare/connect/network/parser/ --type f -x basename {}

echo -e "\n=== Checking PersonalIdSessionData structure ==="
fd "PersonalIdSessionData" --type f -x cat {} | head -50

echo -e "\n=== Checking if there are any references to the old method signature ==="
rg "setPhotoAndCompleteProfile.*userId.*password" -A 2 -B 1

echo -e "\n=== Examining TokenAuth usage ==="
rg "TokenAuth" -A 2 -B 1

echo -e "\n=== Checking imports in PersonalIdPhotoCaptureFragment to understand what was removed ==="
git show HEAD:app/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.java | head -30

Length of output: 21237

---

@pm-dimagi I'll provide a comprehensive review of this profile refactor PR. Let me examine the changes and identify potential issues.

🔍 Code Review Summary

This is a significant architectural refactor that moves from a database-first to session-first approach for profile completion, with improved authentication patterns. Here are my findings:

⚠️ Critical Issues

1. Missing CompleteProfileResponseParser

// PersonalIdApiHandler.java:8
import org.commcare.connect.network.parser.CompleteProfileResponseParser;

Issue: This class is imported and used but doesn't exist in the codebase.

Impact: Compilation failure

Recommendation: Create the missing parser class or use an existing one.

2. Variable Name Inconsistency

// ApiPersonalId.java:329-330
AuthInfo authInfo = new AuthInfo.TokenAuth(token);
String tokenAuth = HttpUtils.getCredential(authInfo);
Objects.requireNonNull(token); // Should be tokenAuth

Issue: The null check is on the wrong variable - should validate tokenAuth instead of token.

🎯 Architectural Improvements

✅ Positive Changes

1. Better Separation of Concerns: Moving from PersonalIdManager.getInstance().getUser() to PersonalIdSessionDataViewModel improves testability
2. Improved Authentication: Token-based auth is more secure than passing userId/password
3. Structured Error Handling: Using enum-based error codes is much better than raw callbacks
4. Session-based Data Flow: More predictable state management

🔧 Security Considerations

The token authentication refactor is good, but ensure:

// Consider adding token validation
Objects.requireNonNull(tokenAuth, "Token authentication failed");
if (tokenAuth.isEmpty()) {
    throw new IllegalArgumentException("Invalid authentication token");
}

📋 Detailed File Analysis

ApiPersonalId.java

• ✅ Good: Simplified method signature
• ⚠️ Issue: Null check on wrong variable (line 330)
• ✅ Good: Consistent TokenAuth usage

PersonalIdApiHandler.java

• ❌ Critical: Missing CompleteProfileResponseParser class
• ✅ Good: Consistent with existing patterns in the class

PersonalIdPhotoCaptureFragment.java

• ✅ Excellent: Much cleaner error handling with enum codes
• ✅ Good: Proper ViewModel usage for session data
• ✅ Good: Database operations moved to success callback
• ⚠️ Consider: Add validation for session data completeness

🚀 Suggestions for Improvement

1. Add Session Data Validation

private void validateSessionData(PersonalIdSessionData sessionData) {
    if (sessionData == null || sessionData.getPersonalId() == null || 
        sessionData.getToken() == null) {
        throw new IllegalStateException("Invalid session data for profile completion");
    }
}

2. Improve Error Context

// Consider adding more context to error messages
case INVALID_RESPONSE_ERROR:
    onPhotoUploadFailure(
        requireContext().getString(R.string.connectid_photo_upload_failure) + 
        " (Response validation failed)", true);
    break;

🧪 Testing Considerations

This refactor affects critical user flows. Ensure testing covers:

• Token authentication success/failure scenarios
• Session data persistence across app states
• Database consistency after profile completion
• Error handling for network failures
• Backward compatibility with existing user records

📝 Minor Issues

1. Formatting: Missing space in method signature (line 325 in ApiPersonalId.java)

// Current
String photoAsBase64, String backupCode, String token,IApiCallback callback) {
// Should be  
String photoAsBase64, String backupCode, String token, IApiCallback callback) {

✨ Overall Assessment

This is a well-architected refactor that improves the codebase structure significantly. The main blocker is the missing CompleteProfileResponseParser class. Once that's resolved and the minor issues are addressed, this will be a solid improvement to the profile completion flow.

Recommendation: Fix the missing parser class and variable validation issues, then this is ready to merge.

---

🏁 Script executed:

# Check what response parsers exist that could be used as a template
echo "=== Examining existing response parser structure ==="
cat app/src/org/commcare/connect/network/parser/ConfirmBackupCodeResponseParser.java

Length of output: 1302

---

Based on the existing response parser pattern, here's the missing CompleteProfileResponseParser class that needs to be created:

🔧 Critical Fix Required

Create the missing CompleteProfileResponseParser.java:

package org.commcare.connect.network.parser;

import org.commcare.android.database.connect.models.PersonalIdSessionData;
import org.commcare.utils.JsonExtensions;
import org.json.JSONException;
import org.json.JSONObject;

/**
 * Parses a JSON response from the complete profile API call
 * and populates a PersonalIdSessionData instance.
 */
public class CompleteProfileResponseParser implements PersonalIdApiResponseParser {
    /**
     * Parses and sets values on the given PersonalIdSessionData instance.
     *
     * @param sessionData the instance to populate
     * @throws JSONException if a parsing error occurs
     */
    @Override
    public void parse(JSONObject json, PersonalIdSessionData sessionData) throws JSONException {
        // Parse common profile completion response fields
        sessionData.setPersonalId(JsonExtensions.optStringSafe(json, "username", sessionData.getPersonalId()));
        sessionData.setDbKey(JsonExtensions.optStringSafe(json, "db_key", sessionData.getDbKey()));
        sessionData.setAccountOrphaned(json.optBoolean("account_orphaned", false));
        sessionData.setOauthPassword(JsonExtensions.optStringSafe(json, "password", sessionData.getOauthPassword()));
        
        // Profile completion specific fields (adjust based on actual API response)
        sessionData.setPhotoBase64(JsonExtensions.optStringSafe(json, "photo", sessionData.getPhotoBase64()));
        
        // Any other fields returned by the complete profile endpoint
        // Add more fields as needed based on the actual API response structure
    }
}

🔧 Additional Critical Fixes

Fix Variable Validation in ApiPersonalId.java:

public static void setPhotoAndCompleteProfile(Context context, String userName,
                                              String photoAsBase64, String backupCode, String token, IApiCallback callback) {
    Objects.requireNonNull(photoAsBase64);
    Objects.requireNonNull(userName);
    AuthInfo authInfo = new AuthInfo.TokenAuth(token);
    String tokenAuth = HttpUtils.getCredential(authInfo);
    Objects.requireNonNull(tokenAuth); // Fixed: validate tokenAuth instead of token

    HashMap<String, String> params = new HashMap<>();
    params.put("photo", photoAsBase64);
    params.put("username", userName);
    params.put("recovery_pin", backupCode);

    ApiService apiService = ApiClient.getClientApi();
    Call<ResponseBody> call = apiService.completeProfile(tokenAuth, params);
    callApi(context, call, callback);
}

Fix Spacing in Method Signature:

// Line 324-325 in ApiPersonalId.java
public static void setPhotoAndCompleteProfile(Context context, String userName,
                                              String photoAsBase64, String backupCode, String token, IApiCallback callback) {
//                                                                                    ^ Add space here

🎯 Updated Assessment

Once these critical fixes are implemented:

1. ✅ Create CompleteProfileResponseParser.java with the structure above
2. ✅ Fix the null validation in ApiPersonalId.java
3. ✅ Fix the spacing in method signature

The refactor will be solid and ready for merge. The architectural improvements in this PR are excellent and will significantly improve the codebase maintainability.

---

🏁 Script executed:

# Check what fields might be returned from the complete profile API
echo "=== Checking for any existing complete profile API documentation or similar patterns ==="
rg "completeProfile" -A 5 -B 5

echo -e "\n=== Looking for any other profile-related response handling ==="
rg "profile.*response\|profile.*json" -i -A 3 -B 1

Length of output: 7113

---

Based on the available information and the pattern from other response parsers, here's the complete solution:

🔧 Required Files and Fixes

1. Create Missing CompleteProfileResponseParser

package org.commcare.connect.network.parser;

import org.commcare.android.database.connect.models.PersonalIdSessionData;
import org.commcare.utils.JsonExtensions;
import org.json.JSONException;
import org.json.JSONObject;

/**
 * Parses a JSON response from the complete profile API call
 * and populates a PersonalIdSessionData instance.
 */
public class CompleteProfileResponseParser implements PersonalIdApiResponseParser {
    /**
     * Parses and sets values on the given PersonalIdSessionData instance.
     *
     * @param sessionData the instance to populate  
     * @throws JSONException if a parsing error occurs
     */
    @Override
    public void parse(JSONObject json, PersonalIdSessionData sessionData) throws JSONException {
        // Based on other parsers, typical response fields include:
        sessionData.setPersonalId(JsonExtensions.optStringSafe(json, "username", sessionData.getPersonalId()));
        sessionData.setDbKey(JsonExtensions.optStringSafe(json, "db_key", sessionData.getDbKey()));
        sessionData.setAccountOrphaned(json.optBoolean("account_orphaned", false));
        sessionData.setOauthPassword(JsonExtensions.optStringSafe(json, "password", sessionData.getOauthPassword()));
        
        // Profile completion may return success confirmation or updated fields
        sessionData.setPhotoBase64(JsonExtensions.optStringSafe(json, "photo", sessionData.getPhotoBase64()));
        
        // Additional fields that might be returned after profile completion
        // Note: Adjust these based on actual API response documentation
    }
}

2. Fix Critical Issues in ApiPersonalId.java

public static void setPhotoAndCompleteProfile(Context context, String userName,
                                              String photoAsBase64, String backupCode, String token, IApiCallback callback) {
    Objects.requireNonNull(photoAsBase64);
    Objects.requireNonNull(userName);
    Objects.requireNonNull(token); // Add this validation
    
    AuthInfo authInfo = new AuthInfo.TokenAuth(token);
    String tokenAuth = HttpUtils.getCredential(authInfo);
    Objects.requireNonNull(tokenAuth); // Fixed: validate tokenAuth instead of token

    HashMap<String, String> params = new HashMap<>();
    params.put("photo", photoAsBase64);
    params.put("username", userName);  // Note: API uses "username" not "name"
    params.put("recovery_pin", backupCode);

    ApiService apiService = ApiClient.getClientApi();
    Call<ResponseBody> call = apiService.completeProfile(tokenAuth, params);
    callApi(context, call, callback);
}

🚨 Additional Issue Found

Looking at the API parameters, there's an inconsistency:

In ApiPersonalId.java (line 336):

params.put("name", userName);  // ❌ Wrong parameter name

Should be:

params.put("username", userName);  // ✅ Correct parameter name

This matches the pattern from other API calls in the codebase where "username" is the standard parameter name.

📋 Final Checklist

To make this PR ready for merge:

1. ✅ Create app/src/org/commcare/connect/network/parser/CompleteProfileResponseParser.java
2. ✅ Fix null validation in ApiPersonalId.java (line 330)
3. ✅ Fix parameter name from "name" to "username" (line 336)
4. ✅ Fix spacing in method signature (line 325)
5. ✅ Add token validation before creating TokenAuth

🎯 Updated Overall Assessment

This refactor represents excellent architectural improvements:

• ✅ Better separation of concerns with ViewModel pattern
• ✅ Improved error handling with structured error codes
• ✅ More secure token-based authentication
• ✅ Cleaner data flow through session management

Once the missing parser class is created and the critical fixes are applied, this will be a solid enhancement that improves code maintainability and user experience.

The changes align well with modern Android development patterns and will make the codebase more testable and maintainable.

---

🏁 Script executed:

# Look at the updateProfile API to understand response patterns
echo "=== Checking updateProfile and similar API responses ==="
rg "updateProfile" -A 10 -B 5

echo -e "\n=== Check for any API response documentation or examples ==="
rg "username.*password.*photo\|db_key.*username\|account_orphaned" -A 3 -B 3

echo -e "\n=== Look at how other profile-related parsers work ==="
fd "*Profile*Parser" --type f -x cat {}

Length of output: 8639

---

Based on my comprehensive analysis, here's my final review summary:

🎯 FINAL REVIEW SUMMARY

This is an excellent architectural refactor that modernizes the profile completion flow. However, there are 4 critical fixes needed before merge.

❌ BLOCKING ISSUES - Must Fix

1. Missing CompleteProfileResponseParser.java

Location: app/src/org/commcare/connect/network/parser/
Solution: Create the missing class:

package org.commcare.connect.network.parser;

import org.commcare.android.database.connect.models.PersonalIdSessionData;
import org.commcare.utils.JsonExtensions;
import org.json.JSONException;
import org.json.JSONObject;

/**
 * Parses a JSON response from the complete profile API call
 * and populates a PersonalIdSessionData instance.
 */
public class CompleteProfileResponseParser implements PersonalIdApiResponseParser {
    @Override
    public void parse(JSONObject json, PersonalIdSessionData sessionData) throws JSONException {
        // Standard response fields based on other parsers
        sessionData.setPersonalId(JsonExtensions.optStringSafe(json, "username", sessionData.getPersonalId()));
        sessionData.setDbKey(JsonExtensions.optStringSafe(json, "db_key", sessionData.getDbKey()));
        sessionData.setAccountOrphaned(json.optBoolean("account_orphaned", false));
        sessionData.setOauthPassword(JsonExtensions.optStringSafe(json, "password", sessionData.getOauthPassword()));
    }
}

2. Wrong Null Validation

File: ApiPersonalId.java:330

// ❌ Current (wrong)
Objects.requireNonNull(token);

// ✅ Should be
Objects.requireNonNull(tokenAuth);

3. Wrong Parameter Name

File: ApiPersonalId.java:336

// ❌ Current (inconsistent with API)
params.put("name", userName);

// ✅ Should be (matches updateProfile pattern)
params.put("username", userName);

4. Missing Space in Method Signature

File: ApiPersonalId.java:325

// ❌ Current
String token,IApiCallback callback) {

// ✅ Should be  
String token, IApiCallback callback) {

✅ ARCHITECTURAL WINS

1. Session Management: Moving from PersonalIdManager.getUser() to PersonalIdSessionDataViewModel ⭐
2. Authentication: Token-based auth instead of user/password ⭐
3. Error Handling: Structured error codes vs raw callbacks ⭐
4. Data Flow: Session-first approach with database sync on success ⭐

🚀 RECOMMENDATIONS

Security Enhancement:

// Add token validation
Objects.requireNonNull(token, "Authentication token cannot be null");
if (token.trim().isEmpty()) {
    throw new IllegalArgumentException("Authentication token cannot be empty");
}

Session Validation:

// In PersonalIdPhotoCaptureFragment
private void validateSessionData() {
    if (personalIdSessionData == null || 
        personalIdSessionData.getToken() == null ||
        personalIdSessionData.getPersonalId() == null) {
        throw new IllegalStateException("Invalid session state for profile completion");
    }
}

📊 RISK ASSESSMENT

• 🟢 LOW RISK: Architectural changes are well-structured
• 🟡 MEDIUM RISK: Authentication method change (ensure thorough testing)
• 🔴 HIGH RISK: Missing parser will cause compilation failure

✅ FINAL VERDICT

APPROVE with required fixes. This refactor significantly improves the codebase architecture and maintainability. Once the 4 blocking issues are resolved, this will be a valuable enhancement.

The move to session-based data management and structured error handling aligns perfectly with modern Android development patterns. Great work on this refactor! 🎉

---

🏁 Script executed:

# Fix the search pattern for profile parsers
echo "=== Looking for profile-related parsers ==="
fd -g "*Profile*Parser*" --type f -x cat {}

echo -e "\n=== Check parameter patterns in updateProfile ==="
rg "params\.put.*name" -A 1 -B 1

echo -e "\n=== Look at all existing parsers to understand the pattern ==="
ls app/src/org/commcare/connect/network/parser/

echo -e "\n=== Check one more parser for pattern ==="
cat app/src/org/commcare/connect/network/parser/StartConfigurationResponseParser.java

Length of output: 3567

---

Perfect! Now I have the complete picture. Based on the parameter patterns and parser structure, let me provide the definitive solution:

🎯 FINAL COMPREHENSIVE SOLUTION

✅ Parameter Name is Actually Correct

Looking at the parameter patterns:

• "username" → Used for authentication/login APIs
• "name" → Used for profile/display name APIs (✅ correct for completeProfile)

So the current params.put("name", userName) is actually correct for a profile completion API.

🔧 Complete CompleteProfileResponseParser.java

Based on the StartConfigurationResponseParser pattern:

package org.commcare.connect.network.parser;

import org.commcare.android.database.connect.models.PersonalIdSessionData;
import org.commcare.utils.JsonExtensions;
import org.json.JSONException;
import org.json.JSONObject;

/**
 * Parses a JSON response from the complete profile API call
 * and populates a PersonalIdSessionData instance.
 */
public class CompleteProfileResponseParser implements PersonalIdApiResponseParser {

    /**
     * Parses and sets values on the given PersonalIdSessionData instance.
     *
     * @param sessionData the instance to populate
     * @throws JSONException if a parsing error occurs
     */
    @Override
    public void parse(JSONObject json, PersonalIdSessionData sessionData) throws JSONException {
        // Parse typical profile completion response fields
        sessionData.setPersonalId(JsonExtensions.optStringSafe(json, "username", sessionData.getPersonalId()));
        sessionData.setDbKey(JsonExtensions.optStringSafe(json, "db_key", sessionData.getDbKey()));
        sessionData.setAccountOrphaned(json.optBoolean("account_orphaned", false));
        sessionData.setOauthPassword(JsonExtensions.optStringSafe(json, "password", sessionData.getOauthPassword()));
        sessionData.setToken(JsonExtensions.optStringSafe(json, "token", sessionData.getToken()));
        
        // Profile-specific fields that may be returned after completion
        sessionData.setPhotoBase64(JsonExtensions.optStringSafe(json, "photo", sessionData.getPhotoBase64()));
        sessionData.setUserName(JsonExtensions.optStringSafe(json, "name", sessionData.getUserName()));
    }
}

🔧 CORRECTED ISSUES LIST

1. ❌ Critical: Missing Parser Class

Create CompleteProfileResponseParser.java (shown above)

2. ❌ Critical: Wrong Null Validation

File: ApiPersonalId.java:330

// Current (wrong)
Objects.requireNonNull(token);

// Should be
Objects.requireNonNull(tokenAuth);

3. ❌ Minor: Missing Space

File: ApiPersonalId.java:325

// Current
String token,IApiCallback callback) {

// Should be  
String token, IApiCallback callback) {

4. ✅ Parameter Name is Correct

The params.put("name", userName) is actually correct for profile APIs.

🎯 UPDATED FINAL ASSESSMENT

APPROVE with 3 required fixes (down from 4).

This is an excellent refactor that:

• ✅ Modernizes authentication from user/password to token-based
• ✅ Improves architecture with session-first data management
• ✅ Enhances error handling with structured error codes
• ✅ Follows Android development best practices

Once the missing parser class is created and the validation bug is fixed, this will be a high-quality enhancement to the codebase! 🚀

🧠

 ∩---∩
(  • ‿ •  )  This refactor shows thoughtful architectural improvements!
 \_______/   Great work on modernizing the profile completion flow.

[pm-dimagi]

reviewers Here the file CompleteProfileParser and BackupCodeParser is almost same i still kept it different for code clearnce and if there is any change in the future in fields

[OrangeAndGreen]

Nit: Can be a local variable

[pm-dimagi]

d33374e

[OrangeAndGreen]

This should go to ConnectNetworkHelper.handleTokenDeniedException, which will crash the app, then delete the DB and tell the user to configure PersonalID again when they restart the app.

[shubham1g5]

these keys must be mandatory now right ?

[pm-dimagi]

yes it is
d33374e

[shubham1g5]

think using optStringSafe is fine there but we should do a separate invalidation later on each of these paramertes to be non null and throw a different exception than Json Exception, maybe IllegalStateException . This is because we still want to check for "null" strings when validating this, the handling for which is only built in optStringSafe and not getString

[pm-dimagi]

cd77c9a

[shubham1g5]

nit: call it createAndSaveConnectUser

[pm-dimagi]

d33374e

[shubham1g5]

can we use Objects.requireNonNull( inside response parser instead after we have parsed these.

[pm-dimagi]

63b9399