Improved PersonalID API Error Handling #3167
Conversation
Crashing on JSONExceptions or unknown errors by default No generic toast message about not being able to reach server.
…nto dv/personalid_api_error_handling
…ents can display in UI (instead of toast messages).
📝 WalkthroughWalkthroughThis change refactors error handling and display logic across several fragments and related classes in the Personal ID feature. It updates error handler method signatures to accept a Throwable parameter, returns error messages as strings instead of directly displaying UI elements, and centralizes error message display in fragment-level methods. The PersonalIdApiHandler and its error code enum are updated to distinguish error types more precisely, including adding a FORBIDDEN_ERROR. Several UI fragments now clear previous error messages before new API calls and display error messages using new helper methods. Some method and resource names are corrected or clarified. Sequence Diagram(s)sequenceDiagram
participant User
participant Fragment
participant PersonalIdApiHandler
participant ApiPersonalId
participant PersonalIdApiErrorHandler
User->>Fragment: Initiate API action (e.g., confirm backup code)
Fragment->>Fragment: clearError()
Fragment->>PersonalIdApiHandler: make API call
PersonalIdApiHandler->>ApiPersonalId: callApi()
ApiPersonalId-->>PersonalIdApiHandler: Callback (success or failure)
alt Success
PersonalIdApiHandler-->>Fragment: onSuccess()
else Failure
PersonalIdApiHandler-->>Fragment: onFailure(errorCode, throwable)
Fragment->>PersonalIdApiErrorHandler: handle(errorCode, throwable)
PersonalIdApiErrorHandler-->>Fragment: return error message
Fragment->>Fragment: showError(error message)
end
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (1)
122-134: 🛠️ Refactor suggestionConsider adding error clearing before API call for consistency.
Unlike other fragments in this refactor, there's no
clearOtpError()call before thevalidateFirebaseIdToken()API call. This could leave stale error messages visible if a previous error occurred.Add error clearing at the start of the method:
private void validateFirebaseIdToken(String firebaseIdToken) { + clearOtpError(); new PersonalIdApiHandler() {
🧹 Nitpick comments (1)
app/src/org/commcare/connect/network/PersonalIdApiHandler.java (1)
63-87: Consider removing the toast message for consistency.The PR objectives mention replacing toast messages with UI error displays. However, line 74-75 still shows a toast for server error messages.
Consider removing the toast to maintain consistency with the new error handling approach, or document why this specific toast should remain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/res/layout/screen_personalid_phoneno.xml(1 hunks)app/res/values/strings.xml(0 hunks)app/src/org/commcare/connect/PersonalIdManager.java(1 hunks)app/src/org/commcare/connect/network/ApiPersonalId.java(1 hunks)app/src/org/commcare/connect/network/PersonalIdApiErrorHandler.java(1 hunks)app/src/org/commcare/connect/network/PersonalIdApiHandler.java(2 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java(3 hunks)app/src/org/commcare/fragments/personalId/PersonalIdNameFragment.java(3 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(4 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java(2 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.java(2 hunks)
💤 Files with no reviewable changes (1)
- app/res/values/strings.xml
🧰 Additional context used
🧬 Code Graph Analysis (5)
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (1)
app/src/org/commcare/connect/network/PersonalIdApiErrorHandler.java (1)
PersonalIdApiErrorHandler(21-47)
app/src/org/commcare/fragments/personalId/PersonalIdNameFragment.java (1)
app/src/org/commcare/connect/network/PersonalIdApiErrorHandler.java (1)
PersonalIdApiErrorHandler(21-47)
app/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.java (1)
app/src/org/commcare/connect/network/PersonalIdApiErrorHandler.java (1)
PersonalIdApiErrorHandler(21-47)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
app/src/org/commcare/connect/network/PersonalIdApiErrorHandler.java (1)
PersonalIdApiErrorHandler(21-47)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (1)
app/src/org/commcare/connect/network/PersonalIdApiErrorHandler.java (1)
PersonalIdApiErrorHandler(21-47)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (27)
app/res/layout/screen_personalid_phoneno.xml (1)
119-119: LGTM! Improved ID naming consistency.The TextView ID change from
personalid_name_errortopersonalid_phone_errorcorrectly aligns the element identifier with its purpose in the phone number input screen.app/src/org/commcare/connect/PersonalIdManager.java (1)
500-502: Excellent resource management improvement.The try-with-resources statement ensures the InputStream is properly closed after use, preventing potential resource leaks. This is a best practice that improves the robustness of the code.
app/src/org/commcare/connect/network/ApiPersonalId.java (1)
244-244: Good addition of null-safety annotations.The
@NonNullannotations on theonFailuremethod parameters improve type safety and make the non-null contract explicit, helping prevent potential null pointer exceptions.app/src/org/commcare/connect/network/PersonalIdApiErrorHandler.java (3)
31-32: Good architectural improvement with method signature change.The refactored method signature that returns error message strings and accepts a
Throwableparameter is a significant improvement that separates error message generation from UI handling, enabling better error context and more flexible error display.
35-42: Well-implemented error message generation.Returning localized error message strings instead of directly triggering UI side effects is a good architectural pattern that gives callers more control over error presentation.
40-40:Details
✅ Verification successful
Verify null return handling for TOKEN_DENIED_ERROR.
The method returns
nullforTOKEN_DENIED_ERRORcase. Ensure that all calling code properly handles this null return value to avoid potential null pointer exceptions.
🏁 Script executed:
#!/bin/bash # Search for usages of PersonalIdApiErrorHandler.handle to verify null handling rg -A 5 -B 2 "PersonalIdApiErrorHandler\.handle"Length of output: 5023
🏁 Script executed:
#!/bin/bash # Show the handle method(s) in PersonalIdApiErrorHandler to inspect return types and null handling rg -n "handle(" -C5 app/src/org/commcare/connect/network/PersonalIdApiErrorHandler.javaLength of output: 154
🏁 Script executed:
#!/bin/bash # Extract the handle method implementation to verify return values, including null rg -n 'public static .*handle\(.*' -C20 app/src/org/commcare/connect/network/PersonalIdApiErrorHandler.javaLength of output: 1863
🏁 Script executed:
#!/bin/bash # Locate definitions of showError and displayOtpError to verify null handling rg -n "void showError" -C5 app/src rg -n "void displayOtpError" -C5 app/srcLength of output: 7483
No changes needed: null return is handled safely
The onlynullreturn occurs onTOKEN_DENIED_ERROR, and all call sites pass thatnullinto UI methods (showError/displayOtpError), which simply doTextView.setText(null). Android’ssetText(null)is a no-op (renders an empty string) and does not throw NPEs.app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (2)
130-132: LGTM: Error handling signature and delegation updated correctly.The
onFailuremethod signature is properly updated to accept theThrowableparameter and correctly delegates tohandleFailurewith both the error code and throwable context.
309-311: LGTM: Centralized error handling implementation.The
handleFailuremethod correctly uses the updatedPersonalIdApiErrorHandler.handle()method that returns a localized error message string, then displays it using the existingdisplayOtpError()method.app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (3)
171-171: LGTM: Error clearing added before API call.The
clearError()call at the start ofconfirmBackupCode()ensures any previous error messages are cleared before initiating the new API request, following the consistent pattern established in this refactor.
190-196: LGTM: Updated error handling with Throwable context.The
onFailuremethod signature is correctly updated to accept theThrowableparameter, and the error is properly displayed using the centralizedPersonalIdApiErrorHandler.handle()method that returns a localized error message.
213-221: LGTM: Consistent error UI management methods.The
clearError()andshowError()methods follow the same pattern as other fragments in this refactor, properly managing the visibility and content of the error message UI element.app/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.java (3)
85-85: LGTM: Error clearing added before API call.The
clearError()call ensures any previous error messages are cleared before starting the profile completion process, maintaining consistency with the error handling refactor.
95-97: LGTM: Updated failure callback signature.The
onFailuremethod signature is correctly updated to accept theThrowableparameter and properly delegates to theonCompleteProfileFailuremethod with both the error code and throwable context.
103-110: LGTM: Enhanced error handling with retry logic.The
onCompleteProfileFailuremethod correctly uses the updatedPersonalIdApiErrorHandler.handle()method to get a localized error message and displays it using the existingshowError()method. The retry logic appropriately re-enables buttons when the error code allows retry.app/src/org/commcare/fragments/personalId/PersonalIdNameFragment.java (4)
78-78: LGTM: Error clearing added before API call.The
clearError()call at the start ofverifyOrAddName()ensures any previous error messages are cleared before initiating the name verification process, following the consistent pattern from this refactor.
87-89: LGTM: Updated failure callback signature and delegation.The
onFailuremethod signature is correctly updated to accept theThrowableparameter and properly delegates tonavigateFailurewith both the error code and throwable context.
97-103: LGTM: Enhanced error handling with retry logic.The
navigateFailuremethod correctly uses the updatedPersonalIdApiErrorHandler.handle()method to get a localized error message and displays it usingshowError(). The retry logic appropriately re-enables the continue button when the error code allows retry.
105-113: LGTM: Consistent error UI management methods.The
clearError()andshowError()methods follow the same pattern as other fragments in this refactor, properly managing the visibility and content of the error message UI element.app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (4)
179-179: Good practice to clear previous errors.Clearing error messages before starting a new request ensures users don't see stale error messages from previous attempts.
209-209: Typo fix improves code consistency.
219-224: Improved error handling with specific treatment for FORBIDDEN_ERROR.The differentiated handling for FORBIDDEN_ERROR during configuration aligns well with the PR objectives.
248-256: Well-structured error display methods.The centralized error handling methods improve code maintainability and consistency.
app/src/org/commcare/connect/network/PersonalIdApiHandler.java (5)
30-37: Appropriate error categorization and retry logic.The addition of
FORBIDDEN_ERRORand the refined retry logic correctly distinguish between transient errors (network issues) and permanent errors (parsing/validation failures).
42-42: Simplified callback creation improves clarity.
50-56: Enhanced error handling with specific exception types.Separating JSON parsing errors from IO errors provides better debugging context.
92-108: Consistent error handling across all failure scenarios.
149-149: Enhanced error context propagation.Adding the Throwable parameter enables better debugging and more informative error messages.
| default: | ||
| throw new RuntimeException(t); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider safer default error handling.
The default case throws a RuntimeException which will crash the app. Consider returning a generic error message string instead to maintain app stability while still logging the unexpected error.
-default:
- throw new RuntimeException(t);
+default:
+ if (t != null) {
+ Logger.exception("Unhandled API error", t);
+ }
+ return activity.getString(R.string.generic_api_error);📝 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.
| default: | |
| throw new RuntimeException(t); | |
| } | |
| default: | |
| if (t != null) { | |
| Logger.exception("Unhandled API error", t); | |
| } | |
| return activity.getString(R.string.generic_api_error); | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/network/PersonalIdApiErrorHandler.java around
lines 43 to 45, the default case throws a RuntimeException which can crash the
app. Modify this to catch the unexpected error safely by logging the error and
returning a generic error message string instead, ensuring the app remains
stable without abrupt termination.
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Good set of changes overall, left a few more suggestions.
app/src/org/commcare/connect/network/PersonalIdApiErrorHandler.java
Outdated
Show resolved
Hide resolved
| } catch (JSONException e) { | ||
| Logger.exception("JSON error parsing API error response", e); | ||
| onFailure(PersonalIdApiErrorCodes.JSON_PARSING_ERROR, e); | ||
| return; | ||
| } catch (IOException e) { | ||
| Logger.exception("Error parsing API error response", e); | ||
| onFailure(defaultFailureCode); | ||
| onFailure(PersonalIdApiErrorCodes.NETWORK_ERROR, e); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Error in response handling of error body should not propogate same as error in response hadling of successful response body, think it's fine to eat these errors instead and have the onFailure be called with an "unknown error" message instead.
There was a problem hiding this comment.
I'm not sure I understand. Are you saying to create an UNKNOWN_ERROR enum with a different message, and get rid of the code that attempts to retrieve the "error" from the response?
There was a problem hiding this comment.
and get rid of the code that attempts to retrieve the "error" from the response
Not really, we should have that code but any failures in that code shuld just return as "UNKNOWN ERROR" (enum should be fine) instead of JSON_PARSING_ERROR as that json parsing error is not the real reason this request failed.
| if(responseCode == 403) { | ||
| onFailure(PersonalIdApiErrorCodes.FORBIDDEN_ERROR, null); | ||
| return; | ||
| } |
There was a problem hiding this comment.
we should incorporate all critical reponse codes here -
401 - Auth failure
500 - Server Error
503/429 -> Rate Limited error / Too many requests
There was a problem hiding this comment.
9868847
I wasn't sure whether we also want unique error messages to be displayed to the user in these cases? For now I kept them all reporting "There was a network issue, please try again."
There was a problem hiding this comment.
I do think we should have different messaging here on the lines of -
401 - You are not authorized to make this request
500 - There was an error processing your request.
503-429 -> Too many requests attemptes, try again after an hour.
|
@OrangeAndGreen I am merging this for now but think we must try and iterate on the couple outsatanding threads and get it in the release as it would be quite valuable to resolve any issues coming from users. |
Cleaned up some of the error handling code around API errors from PersonalID calls.