Skip to content

Conversation

@pm-dimagi
Copy link
Contributor

Product Description

This pr is just about to make api call for the confirm backup code by the new syntax we are following for the api call
Note: navigation flow, photo and set backupcode will be in separate pr

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 pm-dimagi added the skip-integration-tests Skip android tests. label May 28, 2025
@coderabbitai
Copy link

coderabbitai bot commented May 28, 2025

📝 Walkthrough

Walkthrough

This change refactors the backup code confirmation flow in the CommCare Connect application. The method for confirming a backup code is renamed and its parameters are updated to use token-based authentication instead of phone and secret. A new response parser class is introduced to handle the API response, extracting relevant session data. The API endpoint constant and service interface methods are renamed for clarity and to reflect their new purpose. The fragment responsible for user interaction now uses a higher-level API handler abstraction, simplifying error handling and response parsing.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant PersonalIdBackupCodeFragment
    participant PersonalIdApiHandler
    participant ApiPersonalId
    participant ApiService
    participant Server

    User->>PersonalIdBackupCodeFragment: Enter backup code
    PersonalIdBackupCodeFragment->>PersonalIdApiHandler: confirmBackupCode(activity, backupCode, sessionData)
    PersonalIdApiHandler->>ApiPersonalId: confirmBackupCode(context, pin, token, callback)
    ApiPersonalId->>ApiService: confirmPin("Authorization: Bearer <token>", {recovery_pin: pin})
    ApiService->>Server: POST /users/set_recovery_pin
    Server-->>ApiService: JSON response
    ApiService-->>ApiPersonalId: ResponseBody
    ApiPersonalId-->>PersonalIdApiHandler: callback.onSuccess/onFailure
    PersonalIdApiHandler-->>PersonalIdBackupCodeFragment: onSuccess/onFailure
    PersonalIdBackupCodeFragment-->>User: Display result
Loading

Suggested reviewers

  • OrangeAndGreen
  • shubham1g5
✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (1)
app/src/org/commcare/connect/network/ApiPersonalId.java (1)

262-263: Fix formatting and add parameter validation.

There are two issues with the method signature:

  1. Formatting: Missing space between parameters pin,String token should be pin, String token
  2. Parameter validation: The token parameter should be validated to prevent potential issues

Apply this diff to fix the formatting and add validation:

-    public static void confirmBackupCode(Context context,
-                                String pin,String token ,IApiCallback callback) {
+    public static void confirmBackupCode(Context context,
+                                String pin, String token, IApiCallback callback) {
+        if (token == null || token.trim().isEmpty()) {
+            callback.processFailure(400); // or appropriate error handling
+            return;
+        }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 055914f and 27ee74a.

📒 Files selected for processing (6)
  • app/src/org/commcare/connect/network/ApiPersonalId.java (1 hunks)
  • app/src/org/commcare/connect/network/PersonalIdApiHandler.java (2 hunks)
  • app/src/org/commcare/connect/network/connectId/ApiEndPoints.java (1 hunks)
  • app/src/org/commcare/connect/network/connectId/ApiService.java (1 hunks)
  • app/src/org/commcare/connect/network/parser/ConfirmBackupCodeResponseParser.java (1 hunks)
  • app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/src/org/commcare/connect/network/parser/ConfirmBackupCodeResponseParser.java (1)
app/src/org/commcare/utils/JsonExtensions.kt (1)
  • optStringSafe (7-10)
app/src/org/commcare/connect/network/connectId/ApiService.java (1)
app/src/org/commcare/connect/network/connectId/ApiEndPoints.java (1)
  • ApiEndPoints (3-36)
app/src/org/commcare/connect/network/ApiPersonalId.java (3)
app/src/org/commcare/connect/workers/ConnectHeartbeatWorker.kt (1)
  • context (12-28)
app/src/org/commcare/network/HttpUtils.java (1)
  • HttpUtils (27-100)
app/src/org/commcare/connect/network/connectId/ApiClient.java (1)
  • ApiClient (16-63)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Lint Code Base
🔇 Additional comments (9)
app/src/org/commcare/connect/network/connectId/ApiEndPoints.java (1)

23-23: LGTM! Good refactoring for clarity.

Renaming the constant from setPIN to setBackupCode improves code readability and aligns with the backup code terminology being adopted throughout the codebase, while maintaining the actual endpoint URL.

app/src/org/commcare/connect/network/PersonalIdApiHandler.java (2)

7-7: LGTM! Proper import for the new parser.

The import for ConfirmBackupCodeResponseParser is correctly added to support the new backup code confirmation functionality.


96-101: LGTM! Well-structured API handler method.

The confirmBackupCode method follows the established pattern in this class:

  • Properly extracts the session token from sessionData
  • Uses the appropriate parser (ConfirmBackupCodeResponseParser)
  • Handles errors consistently with the defined error code
  • Integrates seamlessly with the existing callback infrastructure
app/src/org/commcare/connect/network/parser/ConfirmBackupCodeResponseParser.java (1)

1-26: LGTM! Excellent parser implementation.

The ConfirmBackupCodeResponseParser class is well-implemented with several strengths:

  • Safe JSON extraction: Uses JsonExtensions.optStringSafe to handle null/missing values gracefully
  • Proper error handling: Uses optBoolean with sensible default for the boolean field
  • Clear documentation: Well-documented class and method purposes
  • Consistent pattern: Follows the established parser interface pattern

The parser correctly extracts all expected fields (username, db_key, account_orphaned, password) from the API response and populates the session data appropriately.

app/src/org/commcare/connect/network/ApiPersonalId.java (2)

268-270: LGTM! Proper token-based authentication implementation.

The authentication mechanism has been correctly updated:

  • Uses AuthInfo.TokenAuth for secure token-based authentication
  • Properly generates the authorization header using HttpUtils.getCredential
  • Improves security by replacing phone/secret authentication

This change aligns well with modern authentication best practices.


264-274:

Details

✅ Verification successful

Verify the service method name consistency.

The method confirmBackupCode calls apiService.confirmPin(tokenAuth, params) on line 272. While this might be intentional if the service interface uses different naming, it could be confusing for maintainability.

Please verify that the service method naming is consistent across the codebase:


🏁 Script executed:

#!/bin/bash
# Description: Check for consistency between method names in ApiService interface
# Expected: Either all methods use 'confirmPin' or they should be updated to 'confirmBackupCode'

# Search for confirmPin method definition in ApiService
rg -A 3 "confirmPin.*\(" --type java

# Search for any other backup code related method names
rg -A 3 "(confirmBackupCode|confirm.*backup|backup.*confirm)" --type java

Length of output: 3894


Service method naming is consistent

The ApiService interface defines a single confirmPin(...) method (mapped to setBackupCode), and all calls to confirm a backup code go through the confirmBackupCode wrapper in ApiPersonalId. There is no conflicting or missing method in ApiService, and the wrapper naming aligns with other handlers. No changes required.

app/src/org/commcare/connect/network/connectId/ApiService.java (1)

52-53: Good naming convention improvement.

The method name change from confirmPIN to confirmPin follows proper camelCase convention, and renaming the parameter from confirmPINRequest to confirmBackupRequest is more descriptive and aligns with the backup code confirmation functionality.

app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (2)

177-177: Good method name improvement.

The method name change from confirmBackupPin() to confirmBackupCode() better reflects the functionality and aligns with the backup code terminology used throughout the codebase.


239-241: Good centralization of error handling.

The new navigateFailure() method properly delegates error handling to the centralized PersonalIdApiErrorHandler, which improves maintainability and consistency across the application.

@Body Map<String, String> confirmBackupRequest);

@POST(ApiEndPoints.setPIN)
@POST(ApiEndPoints.setBackupCode)
Copy link

@coderabbitai coderabbitai bot May 28, 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 endpoint mapping aligns with method purpose.

The endpoint annotation was changed from @POST(ApiEndPoints.setPIN) to @POST(ApiEndPoints.setBackupCode). Based on the relevant code snippets, setBackupCode maps to "/users/set_recovery_pin" which appears correct for setting a backup code. However, ensure this change aligns with the actual backend API expectations.


🏁 Script executed:

#!/bin/bash
# Description: Verify the endpoint constants and their usage across the codebase
# Expected: Confirm setBackupCode endpoint is used consistently for backup code operations

# Search for setBackupCode endpoint usage
rg -A 2 -B 2 "setBackupCode"

# Search for setPIN endpoint usage to see if it's still referenced elsewhere
rg -A 2 -B 2 "setPIN"

Length of output: 2477


Align APIService method signature with backup code endpoint

In app/src/org/commcare/connect/network/connectId/ApiService.java (at the @POST(ApiEndPoints.setBackupCode) annotation), the method and its parameter are still named for PIN changes, which is misleading. Rename them to reflect the backup-code operation:

Suggested diff:

-    @POST(ApiEndPoints.setBackupCode)
-    Call<ResponseBody> changePIN(@Header("Authorization") String token,
-                                 @Body Map<String, String> changePINRequest);
+    @POST(ApiEndPoints.setBackupCode)
+    Call<ResponseBody> setBackupCode(@Header("Authorization") String token,
+                                     @Body Map<String, String> backupCodeRequest);

This keeps the endpoint mapping clear and consistent with its purpose.

📝 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
@POST(ApiEndPoints.setBackupCode)
@POST(ApiEndPoints.setBackupCode)
Call<ResponseBody> setBackupCode(@Header("Authorization") String token,
@Body Map<String, String> backupCodeRequest);
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/network/connectId/ApiService.java at line 55,
the method annotated with @POST(ApiEndPoints.setBackupCode) still uses names
related to PIN changes, which is misleading. Rename the method and its
parameters to clearly indicate they handle backup code operations, ensuring the
method signature aligns with the setBackupCode endpoint and reflects its purpose
accurately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the format Is used keep the function name as same as end point. of the api... here it is users/set_recovery_pin

Copy link

Choose a reason for hiding this comment

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

@pm-dimagi Thank you for clarifying your naming convention! I understand now that you maintain consistency between the API service method names and the actual endpoint names. Since the endpoint is users/set_recovery_pin, keeping the method name aligned with that endpoint structure makes perfect sense for maintainability and creates a clear mapping between the code and API specification.

I'll keep this convention in mind for future reviews.

ヘ(^◡^)ノ


✏️ Learnings added
Learnt from: pm-dimagi
PR: dimagi/commcare-android#3133
File: app/src/org/commcare/connect/network/connectId/ApiService.java:55-55
Timestamp: 2025-05-28T11:30:37.986Z
Learning: In the CommCare Android codebase, API service method names in ApiService.java should match the format of the actual API endpoint names rather than using semantically meaningful names. For example, if the endpoint is "users/set_recovery_pin", the method name should follow that endpoint structure for consistency and maintainability.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

@pm-dimagi Thank you for the commit! I can see you've addressed the method naming to align with your established convention of matching API endpoint structure. This maintains consistency in your codebase.


🧠 Learnings used
Learnt from: pm-dimagi
PR: dimagi/commcare-android#3133
File: app/src/org/commcare/connect/network/connectId/ApiService.java:55-55
Timestamp: 2025-05-28T11:30:37.986Z
Learning: In the CommCare Android codebase, API service method names in ApiService.java should match the format of the actual API endpoint names rather than using semantically meaningful names. For example, if the endpoint is "users/set_recovery_pin", the method name should follow that endpoint structure for consistency and maintainability.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

Comment on lines 220 to 237
private void confirmBackupCode() {
String backupCode = binding.connectPinInput.getText().toString();

ApiPersonalId.checkPin(getActivity(), name, secret, pin, new IApiCallback() {
new PersonalIdApiHandler() {
@Override
public void processSuccess(int responseCode, InputStream responseData) {
try {
JSONObject json = new JSONObject(new String(StreamsUtil.inputStreamToByteArray(responseData)));
handleSuccessfulRecovery(json, pin);
} catch (IOException | JSONException e) {
Logger.exception("Error parsing recovery response", e);
showRecoveryFailure();
}
protected void onSuccess(PersonalIdSessionData sessionData) {
}

@Override
public void processFailure(int responseCode) {
handleFailedPinAttempt();
protected void onFailure(PersonalIdApiErrorCodes failureCode) {
navigateFailure(failureCode);
}
}.confirmBackupCode(
requireActivity(),
backupCode,
personalIdSessionDataViewModel.getPersonalIdSessionData());

@Override
public void processNetworkFailure() {
ConnectNetworkHelper.showNetworkError(getActivity());
}

@Override
public void processTokenUnavailableError() {
ConnectNetworkHelper.handleTokenUnavailableException(requireActivity());
}

@Override
public void processTokenRequestDeniedError() {
ConnectNetworkHelper.handleTokenDeniedException(requireActivity());
}
}
Copy link

@coderabbitai coderabbitai bot May 28, 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

Excellent architectural improvement, but verify missing functionality.

The refactoring to use PersonalIdApiHandler improves separation of concerns and centralizes error handling. However, there are concerns about missing functionality:

  1. The onSuccess callback is empty (line 225-226), but the old implementation had significant logic for handling successful recovery including JSON parsing and navigation.
  2. The old implementation called handleSuccessfulRecovery() which performed database operations and user management.

Please verify that the missing success handling logic is now handled elsewhere, such as within the PersonalIdApiHandler.confirmBackupCode() method itself. Run the following script to check:


🏁 Script executed:

#!/bin/bash
# Description: Search for the missing success handling logic in PersonalIdApiHandler
# Expected: Find where handleSuccessfulRecovery or similar logic is now implemented

# Search for PersonalIdApiHandler implementation
ast-grep --pattern 'class PersonalIdApiHandler {
  $$$
  confirmBackupCode($$$) {
    $$$
  }
  $$$
}'

# Search for handleSuccessfulRecovery logic
rg -A 10 "handleSuccessfulRecovery\|username.*json\|ConnectConstants.CONNECT_KEY_USERNAME"

Length of output: 197


🏁 Script executed:

#!/bin/bash
set -e

echo "1. Locating PersonalIdApiHandler file(s):"
rg -l "class PersonalIdApiHandler"

echo
echo "2. Displaying class definition and surrounding context:"
file_path=$(rg -l "class PersonalIdApiHandler")
rg -n "class PersonalIdApiHandler" -A 50 "$file_path"

echo
echo "3. Inspecting confirmBackupCode(...) implementation in that file:"
rg -n "confirmBackupCode" -A 50 "$file_path"

echo
echo "4. Searching for legacy handleSuccessfulRecovery references:"
rg -n "handleSuccessfulRecovery" -A 10

Length of output: 5144


🏁 Script executed:

#!/bin/bash
set -e

# 1. Locate ConfirmBackupCodeResponseParser file
echo "Locating ConfirmBackupCodeResponseParser implementation..."
parser_file=$(rg -l "class ConfirmBackupCodeResponseParser" -g "app/src/**/*.java")
echo "Found: $parser_file"

# 2. Display the full class for analysis
echo
echo "Displaying ConfirmBackupCodeResponseParser contents:"
rg -n "class ConfirmBackupCodeResponseParser" -A 200 "$parser_file"

Length of output: 1544


Missing success handling in confirmBackupCode callback
The refactor now parses the response into PersonalIdSessionData via ConfirmBackupCodeResponseParser, but never invokes the legacy recovery logic (DB passphrase, user record setup, PIN reset, navigation). You need to re-introduce that workflow in the anonymous onSuccess(...) override.

Locations to update:

  • app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java, inside confirmBackupCode() around lines 220–237.

Suggested diff:

     new PersonalIdApiHandler() {
-        @Override
-        protected void onSuccess(PersonalIdSessionData sessionData) {
-        }
+        @Override
+        protected void onSuccess(PersonalIdSessionData sessionData) {
+            // restore DB passphrase
+            ConnectDatabaseHelper.handleReceivedDbPassphrase(
+                    requireActivity(), sessionData.getDbKey());
+
+            // recreate user and reset PIN
+            ConnectUserRecord user = new ConnectUserRecord(
+                    sessionData.getName(),
+                    sessionData.getUsername(),
+                    "", sessionData.getName(), "");
+            user.setPin(backupCode);
+            user.setLastPinDate(new Date());
+            resetUserPassword(user);
+
+            // proceed to next screen
+            navigateNext();
+        }
📝 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
private void confirmBackupCode() {
String backupCode = binding.connectPinInput.getText().toString();
ApiPersonalId.checkPin(getActivity(), name, secret, pin, new IApiCallback() {
new PersonalIdApiHandler() {
@Override
public void processSuccess(int responseCode, InputStream responseData) {
try {
JSONObject json = new JSONObject(new String(StreamsUtil.inputStreamToByteArray(responseData)));
handleSuccessfulRecovery(json, pin);
} catch (IOException | JSONException e) {
Logger.exception("Error parsing recovery response", e);
showRecoveryFailure();
}
protected void onSuccess(PersonalIdSessionData sessionData) {
}
@Override
public void processFailure(int responseCode) {
handleFailedPinAttempt();
protected void onFailure(PersonalIdApiErrorCodes failureCode) {
navigateFailure(failureCode);
}
}.confirmBackupCode(
requireActivity(),
backupCode,
personalIdSessionDataViewModel.getPersonalIdSessionData());
@Override
public void processNetworkFailure() {
ConnectNetworkHelper.showNetworkError(getActivity());
}
@Override
public void processTokenUnavailableError() {
ConnectNetworkHelper.handleTokenUnavailableException(requireActivity());
}
@Override
public void processTokenRequestDeniedError() {
ConnectNetworkHelper.handleTokenDeniedException(requireActivity());
}
}
private void confirmBackupCode() {
String backupCode = binding.connectPinInput.getText().toString();
new PersonalIdApiHandler() {
@Override
protected void onSuccess(PersonalIdSessionData sessionData) {
// restore DB passphrase
ConnectDatabaseHelper.handleReceivedDbPassphrase(
requireActivity(), sessionData.getDbKey());
// recreate user and reset PIN
ConnectUserRecord user = new ConnectUserRecord(
sessionData.getName(),
sessionData.getUsername(),
"", sessionData.getName(), "");
user.setPin(backupCode);
user.setLastPinDate(new Date());
resetUserPassword(user);
// proceed to next screen
navigateNext();
}
@Override
protected void onFailure(PersonalIdApiErrorCodes failureCode) {
navigateFailure(failureCode);
}
}.confirmBackupCode(
requireActivity(),
backupCode,
personalIdSessionDataViewModel.getPersonalIdSessionData());
}
🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java
around lines 220 to 237, the onSuccess callback of
PersonalIdApiHandler.confirmBackupCode is currently empty, missing the legacy
success handling logic that manages database passphrase setup, user record
initialization, PIN reset, and navigation. To fix this, reintroduce the legacy
recovery workflow by invoking the appropriate methods or logic inside the
onSuccess override, ensuring the successful recovery process is fully handled as
before.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link

Choose a reason for hiding this comment

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

@shubham1g5 Thanks for confirming! The missing success handling logic definitely needs to be restored to maintain the complete recovery workflow.

If you'd like me to help implement this fix or need assistance with any other aspects of the backup code confirmation functionality, just let me know!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will be done in the next subsequent pr just to keep the logic separate i will create anothe rpr for that @shubham1g5

Copy link

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Comment on lines 220 to 237
private void confirmBackupCode() {
String backupCode = binding.connectPinInput.getText().toString();

ApiPersonalId.checkPin(getActivity(), name, secret, pin, new IApiCallback() {
new PersonalIdApiHandler() {
@Override
public void processSuccess(int responseCode, InputStream responseData) {
try {
JSONObject json = new JSONObject(new String(StreamsUtil.inputStreamToByteArray(responseData)));
handleSuccessfulRecovery(json, pin);
} catch (IOException | JSONException e) {
Logger.exception("Error parsing recovery response", e);
showRecoveryFailure();
}
protected void onSuccess(PersonalIdSessionData sessionData) {
}

@Override
public void processFailure(int responseCode) {
handleFailedPinAttempt();
protected void onFailure(PersonalIdApiErrorCodes failureCode) {
navigateFailure(failureCode);
}
}.confirmBackupCode(
requireActivity(),
backupCode,
personalIdSessionDataViewModel.getPersonalIdSessionData());

@Override
public void processNetworkFailure() {
ConnectNetworkHelper.showNetworkError(getActivity());
}

@Override
public void processTokenUnavailableError() {
ConnectNetworkHelper.handleTokenUnavailableException(requireActivity());
}

@Override
public void processTokenRequestDeniedError() {
ConnectNetworkHelper.handleTokenDeniedException(requireActivity());
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@pm-dimagi pm-dimagi requested a review from shubham1g5 May 28, 2025 10:21
clearPinFields();

clearBackupCodeFields();
personalIdSessionDataViewModel = new ViewModelProvider(this).get(PersonalIdSessionDataViewModel.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

@pm-dimagi This has wrong owner, it should be activity and not fragment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 55 to 56
private static final String KEY_NAME = "name";
private static final String KEY_RECOVERY = "is_recovery";
Copy link
Contributor

Choose a reason for hiding this comment

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

@pm-dimagi What is the reason that we are saving the name and is_recovery in saved instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the old flow code we will refactor this in the later pr

Copy link
Contributor

Choose a reason for hiding this comment

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

@pm-dimagi Ok. This is effecting the initArguments method so I think we should remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes sure will remove it in next pr which is about the same things

user.setPin(pin);
user.setPin(backupCode);
user.setLastPinDate(new Date());
resetUserPassword(user);
Copy link
Contributor

Choose a reason for hiding this comment

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

@pm-dimagi Just asking: Are we allowing to reset the password? Do we have password thing in connect id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is about the user password we create internally now i will confirm this dave that is needed now or not in new flow... but this is not about the user password which user sets manully earlier

ConnectUserRecord user = ConnectUserDatabaseUtil.getUser(getActivity());

ApiPersonalId.setBackupCode(getActivity(), user.getUserId(), user.getPassword(), pin, new IApiCallback() {
ApiPersonalId.setBackupCode(getActivity(), user.getUserId(), user.getPassword(), backupCode, new IApiCallback() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@pm-dimagi When are we setting user and password during registration flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above creply

if (commaIndex != -1) {
base64Image = base64Image.substring(commaIndex + 1);
} else {
throw (new IllegalStateException("Not a valid base64 string"));
Copy link
Contributor

Choose a reason for hiding this comment

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

@pm-dimagi IllegalArgumentException instead of IllegalStateException?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shubham has asked for illegalStateException

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree it should be IllegalArgumentException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pm-dimagi pm-dimagi merged commit 1b7a885 into master May 28, 2025
4 of 7 checks passed
This was referenced May 28, 2025
@coderabbitai coderabbitai bot mentioned this pull request Jun 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-integration-tests Skip android tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants