CCCT-1889 - Review Personal ID Error Codes

[conroy-ricketts]

CCCT-1889

Technical Summary

I reviewed how we are handling all the possible Personal ID error codes coming form server. Please see my notes for each one below:

• INVALID_TOKEN
  • Something interesting I noticed is that the way error codes are sent from server is not consistent. For example, here INVALID_TOKEN is being sent with the label "error", and here it is being sent with the label "error_code". This is just one example of the inconsistency. I added some changes to our code to account for that, but I wonder if this was an error on the server side?
    • Edit: I later found out that the inconsistencies I saw are due to legacy reasons. The server team started adding error_code instead of just error to get more consistency on the server side and let mobile decide how to handle different error codes instead of telling mobile what to do. In general, error might contain a string sentence like "Something went wrong" whereas error_code would be just a code like "INVALID_TOKEN" for example.
  • Anyhow, this seems like something we would want to crash the app for (please correct me if I am wrong). So I added some code to route a Throwable (created in the handleErrorCodeIfPresent() function in PersonalIdApiHandler) to a new Runtime Exception (via the handle() function in PersonalIdApiErrorHandler).
  • Edit: I tweaked this to not crash the app as per the comment here.
• INVALID_SECRET_KEY
  • It seems that this error code should only ever be thrown when calling the following API endpoints:
    • /users/recover/confirm_pin
    • /users/recover/initiate_deactivation
    • /users/recover/confirm_deactivation
  • Currently, our mobile client does not call any of these endpoints. So, there is no need for us to handle this error code.
• INCORRECT_OTP
  • We were already handling this error code (as a PersonalIdOrConnectApiErrorCodes.FAILED_AUTH_ERROR), but we did not formally specify the error code from server, so I added some code to directly use the PersonalIdOrConnectApiErrorCodes.FAILED_AUTH_ERROR when the error code is "INCORRECT_OTP". The behavior should remain the same.
  • Edit: I updated my changes to route this error code through PersonalIdOrConnectApiErrorCodes.INCORRECT_OTP_ERROR to a user-visible message that says "You have entered the wrong 6-digit passcode. Please try again."
• USER_DOES_NOT_EXIST
  • It seems that this error code should only ever be thrown when calling the following API endpoints:
    • /users/recover/initiate_deactivation
    • /users/recover/confirm_deactivation
  • Currently, our mobile client does not call any of these endpoints. So, there is no need for us to handle this error code.
• TOKEN_EXPIRED
  • It seems that this error code should only ever be thrown when calling this API endpoint:
    • /users/recover/confirm_deactivation
  • Currently, our mobile client does not call this endpoint. So, there is no need for us to handle this error code.
• NO_RECOVERY_PIN_SET
  • This seems to be an error we would get when the user tries to continue with a Personal ID backup code.
  • I can't think of a scenario where the user would confirm a backup code and it has not already been set unless there is a bug. So I think it would be good to let the app crash if that ever happened for some unlikely reason. So I added some code to route a Throwable (created in the handleErrorCodeIfPresent() function in PersonalIdApiHandler) to a new Runtime Exception (via the handle() function in PersonalIdApiErrorHandler).
• INCORRECT_CODE
  • It seems that this error code should only ever be thrown when calling this API endpoint:
    • /users/recover/confirm_pin
  • Currently, our mobile client does not call this endpoint. So, there is no need for us to handle this error code.
• INVALID_STEP
  • It seems that this error code should only ever be thrown when calling this API endpoint:
    • /users/recover/confirm_pin
  • Currently, our mobile client does not call this endpoint. So, there is no need for us to handle this error code.
• MISSING_RECOVERY_PIN
  • It seems that this error code should only ever be thrown when calling this API endpoint:
    • /users/set_recovery_pin
  • Currently, our mobile client does not call this endpoint. So, there is no need for us to handle this error code.
• FAILED_TO_UPLOAD
  • This error code is related to uploading user profile photos.
  • This is a status 500 server error which we are already handling via PersonalIdOrConnectApiErrorCodes.SERVER_ERROR. But, we did not formally specify the error code from server, so I added some code to directly use the PersonalIdOrConnectApiErrorCodes.SERVER_ERROR when the error code is "FAILED_TO_UPLOAD". The behavior should remain the same.
• FILE_TOO_LARGE
  • This error code is related to uploading user profile photos (the photo is too large).
  • This is a status 500 server error which we are already handling via PersonalIdOrConnectApiErrorCodes.SERVER_ERROR. But, we did not formally specify the error code from server, so I added some code to directly use the PersonalIdOrConnectApiErrorCodes.SERVER_ERROR when the error code is "FILE_TOO_LARGE". The behavior should remain the same.
  • This one is interesting though because I feel like we could benefit from adding some new UI to let the user know that the photo they are trying to use is too large. I'd love to hear everyone's thoughts on this. For now, I'm keeping the current behavior the same and routing this error as an internal server error. I think this would be worth discussing and creating a new ticket for if we want to though.
  • Edit: Since users actually don't have much control over their profile photo size, I tweaked the code to crash in that scenario so that it is brought to our attention that we are not compressing the photos down to a size small enough for server to handle.
• MISSING_DATA
  • This error code is used across a few different APIs whenever there is data missing from the mobile client side.
  • In this scenario we want the app to crash, so I added some code to route a Throwable (created in the handleErrorCodeIfPresent() function in PersonalIdApiHandler) to a new Runtime Exception (via the handle() function in PersonalIdApiErrorHandler).
• PHONE_MISMATCH
  • This is related to a phone number mismatch when validating the firebase ID token.
  • I think it would be good to let the app crash if this ever happened for some unlikely reason. So I added some code to route a Throwable (created in the handleErrorCodeIfPresent() function in PersonalIdApiHandler) to a new Runtime Exception (via the handle() function in PersonalIdApiErrorHandler).
• MISSING_TOKEN
  • This is related to the firebase ID token being missing when validating it.
  • I think it would be good to let the app crash if this ever happened for some unlikely reason. So I added some code to route a Throwable (created in the handleErrorCodeIfPresent() function in PersonalIdApiHandler) to a new Runtime Exception (via the handle() function in PersonalIdApiErrorHandler).
• FAILED_VALIDATING_TOKEN
  • This is related to there being an issue verifying the firebase ID token.
  • I think it would be good to let the app crash if this ever happened for some unlikely reason. So I added some code to route a Throwable (created in the handleErrorCodeIfPresent() function in PersonalIdApiHandler) to a new Runtime Exception (via the handle() function in PersonalIdApiErrorHandler).
• NAME_REQUIRED
  • This is related to user's name being missing from the mobile client side.
  • In this scenario we want the app to crash, so I added some code to route a Throwable (created in the handleErrorCodeIfPresent() function in PersonalIdApiHandler) to a new Runtime Exception (via the handle() function in PersonalIdApiErrorHandler).
• PHONE_NOT_VALIDATED
  • This can come from a few different API calls when the user's phone is not validated.
  • We were already handling this error code (as a PersonalIdOrConnectApiErrorCodes.FORBIDDEN_ERROR), but we did not formally specify the error code from server, so I added some code to directly use the PersonalIdOrConnectApiErrorCodes.FORBIDDEN_ERROR when the error code is "PHONE_NOT_VALIDATED". The behavior should remain the same.
• LOCKED_ACCOUNT
  • We were already handling this error code correctly, so no changes needed for this.
• UNSUPPORTED_COUNTRY
  • We were already handling this error code (as a PersonalIdOrConnectApiErrorCodes.FORBIDDEN_ERROR), but we did not formally specify the error code from server, so I added some code to directly use the PersonalIdOrConnectApiErrorCodes.FORBIDDEN_ERROR when the error code is "UNSUPPORTED_COUNTRY". The behavior should remain the same.
• SESSION_NOT_FOUND
  • It seems that this error code should only ever be thrown when calling this API endpoint:
    • /users/generate_manual_otp
  • Currently, our mobile client does not call this endpoint. So, there is no need for us to handle this error code.
• INVALID_CREDENTIALS
  • It seems that this error code should only ever be thrown when calling this API endpoint:
    • /users/add_credential
  • Currently, our mobile client does not call this endpoint. So, there is no need for us to handle this error code.
• INVALID_DATA
  • It seems that this error code is not currently used anywhere by server. So, there is no need for us to handle this error code.
• ACTIVE_USER_EXISTS
  • This is related to a user trying to create a new profile with a phone number that is already tied to an existing account.
  • I don't believe that this should be possible in the current Personal ID sign-in flow (correct me if I'm wrong), so I added some code to route a Throwable (created in the handleErrorCodeIfPresent() function in PersonalIdApiHandler) to a new Runtime Exception (via the handle() function in PersonalIdApiErrorHandler).
• NOT_ALLOWED
  • This is related to an uninvited user receiving an OTP.
  • We were already handling this error code (as a PersonalIdOrConnectApiErrorCodes.FORBIDDEN_ERROR), but we did not formally specify the error code from server, so I added some code to directly use the PersonalIdOrConnectApiErrorCodes.FORBIDDEN_ERROR when the error code is "NOT_ALLOWED". The behavior should remain the same.

Safety Assurance

Safety story

I tested some error codes by going through the Personal ID signup flow and triggering the errors while watching the device logs, and I tested others by forcing errors in the code.

I also made sure that a successful Personal ID signup still works/behaves as expected.

Edit: I verified that the app will crash when we see the appropriate error codes (i.e. the app will crash for the following error codes: NO_RECOVERY_PIN_SET, FILE_TOO_LARGE, MISSING_DATA, PHONE_MISMATCH, MISSING_TOKEN, FAILED_VALIDATING_TOKEN, NAME_REQUIRED, ACTIVE_USER_EXISTS). This is expected - if a crash occurs then it needs to be brought to our attention. I also verified that if server sends the INCORRECT_OTP code, then mobile will display a message that says "You have entered the wrong 6-digit passcode. Please try again."

QA Plan

It would be good for QA to go through the Personal ID signup flow and ensure that nothing unexpected happens, especially if an error occurs.

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request enhances error handling and response parsing across the API layer, particularly for Personal ID Connect operations. Changes include: improved JSON error parsing with fallback handling for an "error" field when "error_code" is empty; addition of eight new error codes to the PersonalIdOrConnectApiErrorCodes enum; expanded error case handling in PersonalIdApiHandler with a switch statement replacing conditionals; and refactored API call wrappers to use consistent multi-line formatting with appropriate parser instances passed to callbacks. Minor formatting and import reorganization adjustments are also included.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• PersonalIdApiHandler.java requires careful attention: the replacement of conditionals with an extensive switch statement over error codes introduces many new handling paths that need verification for correct message mapping and error code correspondence.
• New error code handlers in PersonalIdApiHandler must be validated to ensure each case correctly corresponds to its error message and intended behavior.
• Parser instantiation refactoring: verify that generic type conversions (e.g., RetrieveWorkHistoryResponseParser<>(context)) are correctly applied across all affected API call wrappers.
• Import reorganization: confirm that all newly used parsers and error codes are properly imported and no unused imports remain.

Possibly related PRs

• Improved PersonalID API Error Handling #3167: Modifies PersonalId API error handling with expanded error codes and changes to error-case handling in PersonalIdApiHandler and related callbacks.
• Issue solved for App crashes on entering wrong backup code #3418: Updates error parsing and response handling for ConfirmBackupCode, affecting parser instantiation and error code mapping similar to this PR's refactoring.
• CCCT1397 - integrity error checks #3233: Modifies PersonalIdOrConnectApiErrorCodes and related error-handling paths, touching the same retry and error-mapping logic.

Suggested labels

QA Note

Suggested reviewers

• OrangeAndGreen
• Jignesh-dimagi
• shubham1g5
• pm-dimagi

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 4.76% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically references the main change: a review of Personal ID error codes with the ticket identifier CCCT-1889, accurately summarizing the primary focus of the changeset.
Description check	✅ Passed	PR description is comprehensive, well-structured, and addresses all template sections with detailed technical rationale and safety testing notes.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch task/CCCT-1889-review-personal-id-error-codes

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

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

118-121: Consider a specific error code for FILE_TOO_LARGE.

Currently FILE_TOO_LARGE is mapped to SERVER_ERROR, which prevents the UI from displaying a specific "file too large" message to users. The PR description mentions a "UI consideration" for this case.

If user-facing clarity is desired, consider adding FILE_TOO_LARGE_ERROR to the enum and handling it specifically:

 case "FAILED_TO_UPLOAD", "FILE_TOO_LARGE":
-    // These error codes relate to uploading user profile photos.
-    onFailure(PersonalIdOrConnectApiErrorCodes.SERVER_ERROR, null);
+    // FILE_TOO_LARGE could have a more specific error code for better UI messaging
+    onFailure(
+        "FILE_TOO_LARGE".equals(errorCode) 
+            ? PersonalIdOrConnectApiErrorCodes.FILE_TOO_LARGE_ERROR 
+            : PersonalIdOrConnectApiErrorCodes.SERVER_ERROR, 
+        null
+    );
     return true;

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4c69c81 and 3f5de13.

📒 Files selected for processing (3)

• app/src/org/commcare/connect/network/NetworkUtils.kt (1 hunks)
• app/src/org/commcare/connect/network/base/BaseApiHandler.kt (1 hunks)
• app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (5 hunks)

🧰 Additional context used

🧠 Learnings (16)

📓 Common learnings

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:206-213
Timestamp: 2025-01-21T18:20:43.883Z
Learning: In the Connect ID feature, server responses containing user data should be parsed using ConnectUserResponseParser to ensure consistent handling of payment information (owner_name and phone_number) and secondary phone verification across all authentication flows.

📚 Learning: 2025-06-13T15:53:12.951Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/connect/network/PersonalIdApiHandler.java:51-56
Timestamp: 2025-06-13T15:53:12.951Z
Learning: For `PersonalIdApiHandler`, the team’s convention is to propagate `JSONException` as an unchecked `RuntimeException` so the app crashes, signalling a contract/implementation bug rather than attempting a graceful retry.

Applied to files:

• app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java

📚 Learning: 2025-05-08T11:08:18.530Z

Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.

Applied to files:

• app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java

📚 Learning: 2025-02-04T21:22:56.537Z

Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2949
File: app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java:244-275
Timestamp: 2025-02-04T21:22:56.537Z
Learning: Direct JSONObject parsing is acceptable for handling user data responses in ConnectIdPinFragment, as decided by the team. No need to enforce ConnectUserResponseParser usage in this context.

Applied to files:

• app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java

📚 Learning: 2025-01-21T18:20:43.883Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:206-213
Timestamp: 2025-01-21T18:20:43.883Z
Learning: In the Connect ID feature, server responses containing user data should be parsed using ConnectUserResponseParser to ensure consistent handling of payment information (owner_name and phone_number) and secondary phone verification across all authentication flows.

Applied to files:

• app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java

📚 Learning: 2025-05-22T14:26:41.341Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/fragments/SelectInstallModeFragment.java:201-205
Timestamp: 2025-05-22T14:26:41.341Z
Learning: In SelectInstallModeFragment.java, the showConnectErrorMessage method intentionally omits null checks because it's called at a specific point in the startup flow where UI is guaranteed to be loaded. It's designed to crash if activity or view is null to make potential issues immediately visible rather than hiding them with defensive programming.

Applied to files:

• app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java

📚 Learning: 2025-04-21T18:48:08.330Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3037
File: app/src/org/commcare/connect/ConnectConstants.java:11-15
Timestamp: 2025-04-21T18:48:08.330Z
Learning: Request codes used for startActivityForResult should be unique throughout the application, even if they're used in different activities. COMMCARE_SETUP_CONNECT_LAUNCH_REQUEST_CODE and STANDARD_HOME_CONNECT_LAUNCH_REQUEST_CODE should have different values.

Applied to files:

• app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java

📚 Learning: 2025-05-22T14:28:35.959Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/activities/CommCareSetupActivity.java:360-364
Timestamp: 2025-05-22T14:28:35.959Z
Learning: In CommCareSetupActivity.java, the call to installFragment.showConnectErrorMessage() after fragment transactions is intentionally unguarded with null checks. This follows the app's design pattern where critical error paths prefer immediate crashes over silent failures, making potential issues immediately visible during development rather than hiding them with defensive programming.

Applied to files:

• app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java

📚 Learning: 2025-07-29T14:10:55.131Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:0-0
Timestamp: 2025-07-29T14:10:55.131Z
Learning: In ConnectUnlockFragment.java, opportunityId values are expected to always contain valid integer strings. Integer.parseInt() should be allowed to throw NumberFormatException if invalid data is encountered, following the fail-fast philosophy used throughout the Connect feature for data contract violations.

Applied to files:

• app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java

📚 Learning: 2025-06-06T19:54:26.428Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java:74-78
Timestamp: 2025-06-06T19:54:26.428Z
Learning: In ConnectDownloadingFragment.java and similar Connect-related code, the team prefers to let "should never happen" scenarios like null app records crash rather than add defensive null checks, following a fail-fast philosophy to catch programming errors during development.

Applied to files:

• app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java

📚 Learning: 2025-07-29T14:14:07.954Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/adapters/ConnectMessageAdapter.java:0-0
Timestamp: 2025-07-29T14:14:07.954Z
Learning: In the CommCare Android Connect messaging system (ConnectMessageAdapter.java), the team prefers to let getMessage() potentially return null and crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately during development.

Applied to files:

• app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java

📚 Learning: 2025-07-29T14:10:58.243Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java:0-0
Timestamp: 2025-07-29T14:10:58.243Z
Learning: In ConnectJobsListsFragment.java, the team intentionally uses different error handling strategies: JSONException should throw RuntimeException to crash the app (fail-fast for data contract violations), while IOException should be logged and allow graceful continuation (for network/stream issues). This follows the established Connect module pattern of treating different exception types differently based on their nature.

Applied to files:

• app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java
• app/src/org/commcare/connect/network/NetworkUtils.kt

📚 Learning: 2025-07-29T14:11:36.386Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java:0-0
Timestamp: 2025-07-29T14:11:36.386Z
Learning: In ConnectJobsListsFragment.java error handling for JSON parsing, if the JSONObject obj is null when passed to handleCorruptJob(), the team prefers to let the code crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately rather than masking them.

Applied to files:

• app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java

📚 Learning: 2025-07-29T14:08:55.466Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:0-0
Timestamp: 2025-07-29T14:08:55.466Z
Learning: In ConnectUnlockFragment.java and similar Connect-related code, the team has agreed that JSONExceptions should crash the app by throwing RuntimeException (fail-fast approach for data contract violations), while IOExceptions should be logged and allow processing to continue (graceful handling for network/stream issues). This intentional inconsistency in error handling reflects different treatment for different types of failures.

Applied to files:

• app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java
• app/src/org/commcare/connect/network/NetworkUtils.kt

📚 Learning: 2025-01-28T09:38:59.882Z

Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2847
File: app/src/org/commcare/connect/network/connectId/ApiService.java:8-63
Timestamp: 2025-01-28T09:38:59.882Z
Learning: The ConnectID API service methods should use Map<String, String> for request bodies and ResponseBody for responses, as per team preference.

Applied to files:

• app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java

📚 Learning: 2025-04-18T20:13:29.655Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3040
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java:39-55
Timestamp: 2025-04-18T20:13:29.655Z
Learning: In the CommCare Android Connect feature, the JSON object passed to `ConnectJobDeliveryFlagRecord.fromJson()` method should never be null based on the implementation design.

Applied to files:

• app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java

🧬 Code graph analysis (1)

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

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

• ReportIntegrityResponseParser (19-28)

app/src/org/commcare/connect/network/NetworkUtils.kt (1)

• getErrorCodes (32-49)

app/src/org/commcare/connect/network/base/BaseApiHandler.kt (2)

• createCallback (62-87)
• processSuccess (68-85)

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

• ApiPersonalId (52-483)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Lint Code Base

🔇 Additional comments (6)

app/src/org/commcare/connect/network/NetworkUtils.kt (1)

38-46: LGTM! Good defensive handling of server inconsistency.

The fallback to check the "error" field when "error_code" is empty properly addresses the server-side inconsistency mentioned in the PR description.

app/src/org/commcare/connect/network/base/BaseApiHandler.kt (1)

40-55: LGTM! Enum additions support expanded error handling.

The new error codes are well-named, follow existing conventions, and align with the error handling cases added in PersonalIdApiHandler.java.

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

49-82: LGTM! Exception handling follows framework patterns.

The try-catch blocks properly distinguish between JSON parsing errors (data contract issues) and I/O errors (network issues), routing both to appropriate error codes via onFailure. This aligns with the error handling pattern established in BaseApiHandler.createCallback.

---

84-169: LGTM! Comprehensive error handling with appropriate fallback.

The switch statement thoroughly covers the error codes identified in the PR description. The default case correctly returns false to allow the base handler to process unrecognized error codes, and the exception handling around the switch provides good defensive coverage.

---

172-326: LGTM! Consistent callback pattern with appropriate parser selection.

The refactored API call methods now consistently use either the local createCallback (for flows requiring PersonalIdSessionData and custom error handling) or BaseApiHandler.createCallback (for simpler flows). The multi-line formatting improves readability, and parser selection is appropriate for each endpoint.

---

103-108: Verify crash behavior and error handling consistency.

The review comment's concern cannot be fully resolved because BaseApiHandler is an external dependency not available in the codebase. However, examination reveals two important issues requiring verification:

1. Crash behavior uncertain: All observable onFailure() implementations (in fragments and services) show graceful error handling rather than crashes. Without the BaseApiHandler implementation, the actual behavior when calling onFailure() with a Throwable cannot be confirmed.

2. Inconsistency with team conventions: Per learnings, the team's convention is to propagate JSONException as an unchecked RuntimeException to crash (fail-fast for data contract violations), while IOException should be logged gracefully. However, lines 63–69 catch both exception types and pass them to onFailure(), contradicting this convention.

Please manually verify that the error handling approach matches the intended crash/logging behavior and aligns with established team patterns.

[Jignesh-dimagi]

@conroy-ricketts +1 for the ticket description and your observations

[shubham1g5]

thanks for the great notes here!

[Jignesh-dimagi]

Right now, PersonalIdApiHandler has error handling but I was thinking of having this kind of parsing (error_code) in BaseApiCallback and it can be used through out the app for other API calls also. @shubham1g5 @conroy-ricketts your thoughts?

[conroy-ricketts]

Right now, PersonalIdApiHandler has error handling but I was thinking of having this kind of parsing (error_code) in BaseApiCallback and it can be used through out the app for other API calls also. @shubham1g5 @conroy-ricketts your thoughts?

@Jignesh-dimagi I'm open to the idea! My thinking was that it may be better to do the error handling in PersonalIdApiHandler because these error codes are specific to the Personal ID feature and PersonalIdApiHandler is already in the org.commcare.connect.network.connectId package so I think it makes sense from a separation of concerns standpoint. But I'm very curious what the rest of the team thinks about this

[shubham1g5]

Looks good, think it would be nice to confirm in PR safety story that we have tested any core functional changes in the PR (like - incorrect otp check) before we merge this in.

[conroy-ricketts]

@damagatchi retest this please