Graceful error handling for SSO token retrieval #2996
Conversation
Cleaned up call chains around the SSO calls to enforce the same flow. Preparing to better handle the token exceptions.
… into dv/token_exceptions Resolved conflicts in ApiConnectId and ConnectSsoHelper.
… into dv/token_exceptions
Handling thrown token exceptions throughout the code and reporting to UI where appropriate.
OrangeAndGreen
requested review from
Jignesh-dimagi,
avazirna,
pm-dimagi and
shubham1g5
📝 WalkthroughWalkthroughThis change set updates the application's handling of token management and related error scenarios. New notification messages have been added to inform users when the SSO token is either unavailable or denied, with corresponding strings updated in both Android and XML resource files. The changes extend to various components including activities, network helpers, and fragments where error handling is now more granular. Key updates include modifications to token retrieval methods in both synchronous and asynchronous flows, the introduction of a new callback interface with additional error methods, and the removal of unnecessary exception parameters to simplify processing. Additionally, API request methods and heartbeat workers have been revised to incorporate these token-related checks, ensuring that errors are handled immediately and user guidance is provided in case of token retrieval failures. Sequence Diagram(s)sequenceDiagram
participant UI as "User Interface"
participant SSO as "ConnectSsoHelper"
participant API as "API Service"
participant Network as "ConnectNetworkHelper"
UI->>SSO: Initiate token retrieval (Async/Sync)
SSO->>SSO: Attempt to retrieve token
alt Token Retrieved Successfully
SSO->>UI: Call tokenRetrieved(token)
UI->>API: Proceed with API operation using token
else Token Unavailable
SSO->>Network: Call tokenUnavailable()
Network->>UI: Display "Token Unavailable" notification
else Token Request Denied
SSO->>Network: Call tokenRequestDenied()
Network->>UI: Display "Token Request Denied" notification
end
Possibly related PRs
✨ 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:
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: 3
🧹 Nitpick comments (14)
app/src/org/commcare/connect/network/TokenUnavailableException.kt (1)
12-13: Empty constructor could be removedThe empty constructor doesn't provide any additional functionality beyond what's already available from the superclass.
-constructor() : super() { -} +constructor() : super()🧰 Tools
🪛 detekt (1.23.7)
[warning] 12-13: This empty block of code can be removed.
(detekt.empty-blocks.EmptySecondaryConstructor)
app/src/org/commcare/connect/network/ApiConnect.java (1)
23-45: Consider improving null-token handling and reducing repetition.Each callback checks for a
nulltoken and simply returns without notifying users or logging. It might be helpful to surface an error or fallback flow to provide feedback. Additionally, the sequence of retrieving the token and subsequently making the network call is repeated in multiple methods. Consolidating this logic into a shared helper can make the code more concise and easier to maintain.app/src/org/commcare/connect/network/ConnectNetworkHelper.java (3)
360-360: Revised client error handling.Now calls
handler.processFailure(responseCode)without an exception parameter. Ensure that any downstream code that relied on the old exception handling is updated or replaced with new token-related callbacks if needed.
379-380: Use ofLogger.exceptionfor I/O errors.Logging the exception is good, but also calling
handler.processFailure(-1)lumps multiple I/O errors under a generic code. If needed, consider customizing the error for more granular user feedback or logging strategies.
436-443: New handling for token unavailability and denial.The toast notifications for token unavailability and denial are straightforward. If these errors occur frequently, consider alternative user experience strategies (e.g., dialogues or retry prompts) to enhance clarity.
app/src/org/commcare/connect/ConnectManager.java (1)
504-525: Handlenulltokens more visibly.When retrieving the HQ SSO token asynchronously, if
token == null, the flow simply returns. Providing a clearer error path or user notification can reduce confusion, especially during link attempts.app/src/org/commcare/connect/network/ConnectSsoHelper.java (5)
31-31: Minor nitpick on the comment.
Consider converting this inline comment into a proper Javadoc or a brief method-level comment if needed, so it appears in auto-generated documentation.
37-37: Validate captured exception type.
Storing a genericExceptionis fine, but if you can narrow this down to a more specific type (e.g.,IOException,RuntimeException), it may improve clarity.
63-72: Log context check prior to callback invocation.
While logging is done for the exception, consider adding context or additional details (e.g., data fromhqUsernameor environment) to aid in troubleshooting.
89-101: Proper centralization of Connect ID token retrieval logic.
The method correctly defers toConnectManager.getConnectToken()when one is already stored. Just confirm you’re handling the situation when the token is near expiration but still valid—some advanced flows might use a refresh approach rather than letting it fully expire.
128-128: Token retrieval final step.
Be sure to handle or log any exception fromApiConnectId.retrieveHqTokenSync()beyondTokenUnavailableExceptionif the underlying call might throw something else. This helps debugging unforeseen network or JSON issues.app/src/org/commcare/connect/network/ApiConnectId.java (3)
74-88: Centralized Connect ID token retrieval.
This new method effectively handles token creation and updating the user record. Keep an eye on concurrency: if multiple threads call this method at once, you might end up with multiple writes to the user record.
143-145: Reuse existing constants.
The client ID string"4eHlQad1oasGZF0lPiycZIjyL0SY1zx7ZblA6SCV"is repeated here. Consider referencing the same constant from line 55 (HQ_CLIENT_ID) or storing it in a single place to avoid inconsistencies.
526-547: Asynchronous channel key retrieval with new callbacks.
AdoptingtokenUnavailable()andtokenRequestDenied()fosters better error handling. Ensure the callback logic is robust if the user moves away from the screen mid-request—otherwise the callback references may become invalid.Would you like help writing a small test or snippet to confirm successful cleanup when the user navigates away?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
app/assets/locales/android_translatable_strings.txt(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/activities/LoginActivity.java(1 hunks)app/src/org/commcare/connect/ConnectManager.java(8 hunks)app/src/org/commcare/connect/MessageManager.java(10 hunks)app/src/org/commcare/connect/network/ApiConnect.java(7 hunks)app/src/org/commcare/connect/network/ApiConnectId.java(5 hunks)app/src/org/commcare/connect/network/ConnectNetworkHelper.java(6 hunks)app/src/org/commcare/connect/network/ConnectSsoHelper.java(4 hunks)app/src/org/commcare/connect/network/IApiCallback.java(1 hunks)app/src/org/commcare/connect/network/TokenRequestDeniedException.kt(1 hunks)app/src/org/commcare/connect/network/TokenUnavailableException.kt(1 hunks)app/src/org/commcare/connect/workers/ConnectHeartbeatWorker.kt(2 hunks)app/src/org/commcare/engine/references/JavaHttpReference.java(1 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java(3 hunks)app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java(2 hunks)app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java(2 hunks)app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java(7 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java(2 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPhoneFragment.java(4 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragmnet.java(5 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java(3 hunks)app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java(5 hunks)app/src/org/commcare/network/CommcareRequestGenerator.java(4 hunks)app/src/org/commcare/network/HttpCalloutTask.java(4 hunks)app/src/org/commcare/tasks/ManageKeyRecordTask.java(3 hunks)app/src/org/commcare/views/notifications/NotificationMessageFactory.java(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:206-213
Timestamp: 2025-03-20T09:59:10.779Z
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.
app/src/org/commcare/connect/MessageManager.java (2)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:81-89
Timestamp: 2025-03-20T09:59:10.779Z
Learning: In the CommCare Android codebase, use org.jetbrains.annotations.NotNull for null-safety annotations. Empty strings are acceptable for unspecified values, but null values should be prevented using @NotNull.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:66-71
Timestamp: 2025-03-20T09:59:10.779Z
Learning: The ConnectUserRecord class in CommCare Android uses @Persisting annotations with sequential indices for field persistence, and @MetaField annotations for fields that have corresponding META constants. Fields should be documented with Javadoc comments explaining their purpose, format requirements, and relationships with other fields.
app/src/org/commcare/connect/network/ConnectSsoHelper.java (2)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:81-89
Timestamp: 2025-03-20T09:59:10.779Z
Learning: In the CommCare Android codebase, use org.jetbrains.annotations.NotNull for null-safety annotations. Empty strings are acceptable for unspecified values, but null values should be prevented using @NotNull.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/android/database/connect/models/ConnectLinkedAppRecordV3.java:44-68
Timestamp: 2025-03-20T09:59:10.779Z
Learning: The ConnectLinkedAppRecord classes are intentionally designed with simple getters/setters without additional validation or defensive copies.
🧬 Code Definitions (12)
app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java (1)
app/src/org/commcare/connect/network/ConnectNetworkHelper.java (1) (1)
ConnectNetworkHelper(49-469)
app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java (1)
app/src/org/commcare/connect/network/ConnectNetworkHelper.java (1) (1)
ConnectNetworkHelper(49-469)
app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java (1)
app/src/org/commcare/connect/network/ConnectNetworkHelper.java (1) (1)
ConnectNetworkHelper(49-469)
app/src/org/commcare/fragments/connectId/ConnectIdPhoneFragment.java (1)
app/src/org/commcare/connect/network/ConnectNetworkHelper.java (1) (1)
ConnectNetworkHelper(49-469)
app/src/org/commcare/activities/LoginActivity.java (1)
app/src/org/commcare/connect/ConnectManager.java (1) (1)
ConnectManager(86-1127)
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (2)
app/src/org/commcare/connect/database/ConnectJobUtils.java (1) (1)
ConnectJobUtils(23-420)app/src/org/commcare/connect/network/ConnectNetworkHelper.java (1) (1)
ConnectNetworkHelper(49-469)
app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java (1)
app/src/org/commcare/connect/network/ConnectNetworkHelper.java (1) (1)
ConnectNetworkHelper(49-469)
app/src/org/commcare/connect/MessageManager.java (4)
app/src/org/commcare/connect/network/ConnectNetworkHelper.java (1) (1)
ConnectNetworkHelper(49-469)app/src/org/commcare/connect/network/ConnectSsoHelper.java (1) (1)
ConnectSsoHelper(24-154)app/src/org/commcare/connect/workers/ConnectHeartbeatWorker.kt (1) (1)
context(14-35)app/src/org/commcare/connect/network/ApiConnectId.java (1) (1)
ApiConnectId(51-603)
app/src/org/commcare/network/CommcareRequestGenerator.java (3)
app/src/org/commcare/connect/network/ConnectSsoHelper.java (1) (1)
ConnectSsoHelper(24-154)app/src/org/commcare/CommCareApplication.java (1) (1)
CommCareApplication(157-1282)app/src/org/commcare/connect/ConnectManager.java (1) (1)
ConnectManager(86-1127)
app/src/org/commcare/connect/ConnectManager.java (2)
app/src/org/commcare/connect/network/ConnectSsoHelper.java (1) (1)
ConnectSsoHelper(24-154)app/src/org/commcare/connect/network/ConnectNetworkHelper.java (1) (1)
ConnectNetworkHelper(49-469)
app/src/org/commcare/connect/network/ConnectSsoHelper.java (3)
app/src/org/commcare/connect/workers/ConnectHeartbeatWorker.kt (1) (1)
context(14-35)app/src/org/commcare/connect/ConnectManager.java (1) (1)
ConnectManager(86-1127)app/src/org/commcare/connect/network/ApiConnectId.java (1) (1)
ApiConnectId(51-603)
app/src/org/commcare/connect/network/ApiConnectId.java (4)
app/src/org/commcare/connect/workers/ConnectHeartbeatWorker.kt (1) (1)
context(14-35)app/src/org/commcare/connect/network/ConnectNetworkHelper.java (1) (1)
ConnectNetworkHelper(49-469)app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java (1) (1)
ConnectUserDatabaseUtil(11-75)app/src/org/commcare/connect/network/ConnectSsoHelper.java (1) (1)
ConnectSsoHelper(24-154)
🪛 detekt (1.23.7)
app/src/org/commcare/connect/network/TokenUnavailableException.kt
[warning] 12-13: This empty block of code can be removed.
(detekt.empty-blocks.EmptySecondaryConstructor)
🔇 Additional comments (88)
app/src/org/commcare/connect/network/TokenRequestDeniedException.kt (1)
1-8: Well-structured exception class for token request denialsThis new exception class is a good addition that provides a clear, specific error type for token request denial scenarios. This aligns well with the PR objective of enhancing error messaging related to SSO tokens.
app/src/org/commcare/engine/references/JavaHttpReference.java (1)
76-76: Minor formatting improvement adding space before conditional checkThis change improves code readability by adding a blank line before the response status check, which helps separate the network request from its error handling.
app/src/org/commcare/connect/network/TokenUnavailableException.kt (2)
5-11: Good implementation of token unavailability exceptionThis exception class properly extends IOException and provides a clean way to represent token unavailability scenarios. The message property clearly communicates the nature of the error.
15-21: Well-structured secondary constructorsThe additional constructors properly handle both exception-caused failures and HTTP error code scenarios, providing good flexibility in error handling.
app/res/values/strings.xml (1)
331-332: Clear user-facing error messages for token issuesThese new strings provide appropriate user guidance for token-related errors, consistent with the PR's objective to enhance error messaging for SSO tokens. The messages are concise and actionable, directing users to either retry or recover their account depending on the error condition.
app/src/org/commcare/activities/LoginActivity.java (1)
560-564: Simplified app selection logicThe changes simplify the app selector control flow by removing the conditional check for
selectorIndex > 0. The code now directly retrieves the selected app ID and handles empty lists with a default empty string. This improvement makes the logic more straightforward and handles edge cases better.app/src/org/commcare/connect/workers/ConnectHeartbeatWorker.kt (2)
10-12: Added required imports for token authenticationThe new imports support the token-based authentication mechanism being implemented.
30-32: Enhanced heartbeat request with token-based authenticationThe code now uses proper token authentication by first retrieving a ConnectID token before making the heartbeat request. This change aligns with the PR objective of gracefully handling SSO token retrieval.
app/src/org/commcare/views/notifications/NotificationMessageFactory.java (1)
169-179: Added new notification message types for token errorsAdded two new notification types to handle specific SSO token error cases:
TokenUnavailable- For cases when a token cannot be retrieved temporarilyTokenRequestDenied- For cases when a token request is explicitly deniedThese additions directly support the PR's goal of enhancing error messaging for SSO token issues.
app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java (2)
146-150: Simplified callback failure method signatureThe method signature has been simplified by removing the IOException parameter, making the interface more concise while maintaining the necessary error handling functionality.
158-169: Added token-specific error handling methodsImplemented two new callback methods to handle specific token error scenarios:
processTokenUnavailableError()- Handles temporary token unavailabilityprocessTokenRequestDeniedError()- Handles denied token requestsThese additions provide targeted error handling for SSO token issues and display appropriate user-facing messages.
app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java (3)
120-123: Signature updated to remove exception parameterThe
processFailuremethod signature has been updated to remove theIOException eparameter, aligning with the broader changes to streamline error handling across the codebase. This change makes the error handling more consistent with the new token-specific error methods.
131-135: Token unavailable error handling addedNew method implementation for handling token unavailability scenarios. The method correctly delegates to
ConnectNetworkHelper.handleTokenUnavailableException()and reports the API call as failed, ensuring users receive appropriate feedback when tokens cannot be retrieved.
137-141: Token request denied error handling addedNew method implementation for handling token request denial scenarios. The method delegates to
ConnectNetworkHelper.handleTokenRequestDeniedException()and properly reports the API call as failed, providing clear user feedback when token requests are denied.app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java (3)
109-112: Signature updated to remove exception parameterThe
processFailuremethod signature has been updated to remove theIOException eparameter, aligning with the changes made to theIApiCallbackinterface. This simplifies error handling by focusing on HTTP response codes rather than handling exceptions directly.
120-124: Token unavailable error handling addedThis new method implementation properly handles token unavailability scenarios by calling
ConnectNetworkHelper.handleTokenUnavailableException()and reporting the API call as failed, ensuring users are informed of token retrieval issues.
126-130: Token request denied error handling addedNew method implementation that correctly handles token request denial scenarios by invoking
ConnectNetworkHelper.handleTokenRequestDeniedException()and reporting the API call as failed, providing appropriate user feedback for token denial cases.app/src/org/commcare/connect/network/IApiCallback.java (2)
10-10: Updated failure callback signatureThe
processFailuremethod signature has been simplified by removing theIOException eparameter. This change streamlines error handling across implementations by focusing on HTTP response codes rather than exceptions.
13-14: Added new token-related error callback methodsTwo new callback methods have been added to specifically handle token-related errors:
processTokenUnavailableError(): For handling cases where a token cannot be retrieved temporarilyprocessTokenRequestDeniedError(): For handling cases where the server has denied token requests for a userThese additions allow for more granular error handling and align with the PR objective of enhancing error messaging for SSO token issues.
app/assets/locales/android_translatable_strings.txt (2)
559-561: Added user-friendly notification for token unavailabilityAdded clear and user-actionable notification strings for token unavailability scenarios:
- Title: "Couldn't connect to the server."
- Detail: "CommCare had an issue retrieving the SSO token for communication."
- Action: "Check your device's time and date to make sure they are correct, then try again."
These strings provide appropriate guidance for users encountering token retrieval issues.
563-565: Added user-friendly notification for token request denialAdded clear notification strings for token request denial scenarios:
- Title: "Couldn't connect to the server."
- Detail: "CommCare can no longer communicate with the ConnectID server."
- Action: "Please recover your ConnectID account, then try again."
These strings effectively communicate the account-level issue to users and provide clear next steps for resolution.
app/src/org/commcare/tasks/ManageKeyRecordTask.java (3)
11-12: New exception imports for token handling.These imports are appropriately added to support the new token-related exception handling introduced in this PR to improve error messaging for SSO tokens.
216-223: Good addition of token-related error cases.The implementation correctly handles two new error scenarios:
TokenUnavailable- When a token cannot be retrieved but may succeed on retryTokenRequestDenied- When the server has stopped issuing tokens for a specific userEach case properly logs the error and raises an appropriate notification message to inform the user.
351-351: Method signature updated to declare token exceptions.The
doHttpRequestmethod signature has been appropriately updated to declare the new checked exceptionsTokenRequestDeniedExceptionandTokenUnavailableException, which ensures that callers are aware of these potential failure modes.app/src/org/commcare/network/CommcareRequestGenerator.java (4)
13-14: New exception imports for token handling.These imports are appropriately added to support the new token-related exception handling.
177-200: Improved token retrieval with better error handling.The
buildAuth()method has been refactored to handle token retrieval more gracefully. The implementation now:
- Directly attempts to retrieve an HQ SSO token for the current username
- Returns the token if available (with proper logging)
- Falls back to session-based authentication if token isn't available
- Properly handles exceptions during this process
This approach is more robust and provides clearer error handling paths.
214-214: Updated params variable type to use Multimap.The type declaration for
paramshas been changed from a standard Map to a Multimap, which is more appropriate for HTTP parameters that can have multiple values for the same key.
228-228: Updated HashMap initialization to use diamond operator.This is a small but good modernization of the code style, using the diamond operator (
<>) instead of explicitly repeating the type parameters.app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java (2)
227-227: Simplified processFailure method signature.The
IOException eparameter has been removed from the method signature, which simplifies the interface. This is appropriate since the exception isn't being used in the implementation of this method.
237-244: Added token error handling methods.Two new methods have been added to handle specific token-related errors:
processTokenUnavailableError()- Shows a message when token is temporarily unavailableprocessTokenRequestDeniedError()- Shows a message when token request is deniedBoth methods appropriately delegate to the helper methods in
ConnectNetworkHelperto maintain consistent error handling across the application.app/src/org/commcare/network/HttpCalloutTask.java (4)
5-6: New exception imports for token handling.These imports are appropriately added to support the token-related exception handling introduced in this PR.
49-51: Added enum values for token error states.Two new enum values have been added to
HttpCalloutOutcomesto represent token-related errors:
TokenUnavailable- Indicates a temporary failure to retrieve a tokenTokenRequestDenied- Indicates a permanent rejection of token requestsThese additions provide more specific error states that can be handled appropriately by callers.
105-110: Added exception handlers for token-related errors.Two new catch blocks have been added to handle the token-related exceptions:
TokenUnavailableException- Maps to theTokenUnavailableoutcomeTokenRequestDeniedException- Maps to theTokenRequestDeniedoutcomeThis implementation ensures that token-related errors are properly caught and converted to appropriate outcomes that can be handled by the application.
149-149: Updated method signature to declare token exceptions.The
doHttpRequestmethod signature has been appropriately updated to declare the new checked exceptions, which ensures that implementing classes are aware of these potential failure modes and must handle them.app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java (6)
18-19: Added imports for new exception typesThe imports for
TokenRequestDeniedExceptionandTokenUnavailableExceptionsupport the implementation of enhanced error handling for SSO token-related issues, which aligns well with the PR objectives.
235-237: Simplified processFailure method signatureRemoving the
IOException eparameter simplifies the error handling by focusing on the response code. The code now provides a clear, generic error message without needing to handle exception-specific logic.
249-254: Added token unavailability error handlingGood addition that implements the token-related error handling mentioned in the PR objectives. The handler sets an appropriate error message and resets the SMS request time to allow immediate retry.
256-259: Added token request denied error handlingThis implementation correctly handles the case where token requests are denied by displaying an appropriate error message. Note that unlike the token unavailability handler, this doesn't reset the SMS time since retrying immediately wouldn't help in this case.
292-294: Simplified processFailure in second callbackConsistent with the earlier change, this simplifies the method signature for the second callback implementation.
302-310: Added token error handling to second callbackThese handlers maintain consistency with the first callback implementation, ensuring that token-related errors are properly communicated to users during SMS code verification.
app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java (5)
231-233: Simplified processFailure signatureThe method now focuses solely on the response code, consistent with the pattern used in other fragments, providing cleaner and more focused error handling.
236-238: Using requireActivity() instead of getActivity()Good improvement. Using
requireActivity()instead ofgetActivity()ensures the context is non-null, preventing potential NullPointerExceptions.
240-248: Added token-related error handlersThese new methods properly handle token-related errors by delegating to the specialized methods in ConnectNetworkHelper, ensuring consistent error handling across the application.
312-314: Consistent pattern in second callbackThe implementation maintains the same pattern, using
requireActivity()for safer context handling.
316-374: Comprehensive error handling for resetPassword callbackThe addition of token error handlers to all callbacks ensures that token-related errors are properly handled throughout all network operations in this fragment, which aligns well with the PR objectives of improving SSO token error messaging.
app/src/org/commcare/fragments/connectId/ConnectIdPhoneFragment.java (4)
260-264: Simplified processFailure method in phone verificationThe signature change removes the IOException parameter, consistent with the approach in other callbacks throughout the codebase.
267-270: Improved context safety in network failure handlerUsing
requireContext()instead ofgetContext()ensures that a valid context is available, preventing potential NullPointerExceptions.
272-282: Added token error handlers to phone verificationThese new handlers properly reset the phone number check flag and display appropriate error messages using the helper methods from ConnectNetworkHelper.
356-366: Added token error handlers to phone availability checkThe implementation completes the token error handling by adding it to the phone availability check process, ensuring comprehensive error handling throughout the phone verification flow.
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (1)
167-172: Simplified processFailure by removing IOException parameterConsistent with other changes in the PR, this simplifies the error handling to focus on the response code rather than handling specific exceptions.
app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java (3)
104-108: Method signature simplified for better error handlingThe
processFailuremethod signature has been updated to remove theIOException eparameter, which streamlines the error handling process. This change is part of a broader effort to improve token-related error handling across the application.
111-114: Improved network error handling using centralized utilityThe implementation now delegates network error handling to the
ConnectNetworkHelper.showNetworkError()method instead of directly creating a Toast message. This centralizes error handling logic and ensures consistent error messaging throughout the application.
116-126: Added token-specific error handling methodsTwo new methods have been added to handle specific token error scenarios:
processTokenUnavailableError: Handles cases where a token is temporarily unavailableprocessTokenRequestDeniedError: Handles cases where token requests are denied by the serverBoth methods properly call
setFragmentRedirection()and use the appropriate helper methods fromConnectNetworkHelperto display user-friendly error messages.app/src/org/commcare/connect/MessageManager.java (7)
14-19: Added new imports for token-related error handlingThe imports have been updated to include the necessary classes for the enhanced token error handling functionality, specifically
ConnectNetworkHelper,ConnectSsoHelper, and the new token exceptions.
54-60: Added robust token retrieval error handlingThe code now properly handles token retrieval exceptions when attempting to get encryption keys for messaging channels. This try-catch block ensures that token-related errors are caught and logged appropriately, preventing potential crashes in the message handling flow.
132-133: Simplified method signature for improved error handlingThe
processFailuremethod signature has been updated to remove theIOException eparameter, which simplifies the error handling process and makes it consistent with other implementations in the codebase.
141-149: Added token-specific error handling methodsNew callback methods have been added to handle specific token error scenarios:
processTokenUnavailableError: Handles cases where a token is temporarily unavailableprocessTokenRequestDeniedError: Handles cases where token requests are deniedBoth methods properly call
listener.connectActivityComplete(false)to signal the failure to the parent activity.
225-267: Enhanced error handling in getChannelEncryptionKeyThe implementation of the
getChannelEncryptionKeymethod has been updated with robust error handling for various scenarios including network failures and token-related errors. Each callback method follows a consistent pattern of checking if the listener is null before callingconnectActivityComplete().
304-312: Added token error handling for message confirmationToken-specific error handling has been added to the message confirmation process, ensuring that any token-related issues during this operation are properly communicated to the user.
358-366: Added token error handling for message sendingToken-specific error handling has been added to the message sending process, ensuring that any token-related issues during this operation are properly communicated to the user and the application can respond appropriately.
app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java (4)
285-291: Improved error handling with centralized showError methodAdded a private
showErrormethod to centralize error display logic. This improves code maintainability by:
- Reducing code duplication
- Providing a consistent way to display error messages
- Making the error display logic more readable and easier to update
This refactoring aligns with good software engineering practices.
307-308: Simplified method signature for better error handlingThe
processFailuremethod signature has been updated to remove theIOException eparameter, which streamlines the error handling process and makes it consistent with other implementations in the codebase.
322-334: Enhanced error handling with specific token error methodsThe implementation now uses the centralized
showErrormethod for displaying different types of errors and adds new methods for token-specific error scenarios:
processTokenUnavailableError: Shows a message when a token is temporarily unavailableprocessTokenRequestDeniedError: Shows a message when token requests are deniedThis enhances the user experience by providing more specific error messages.
414-422: Added token-specific error handling to registration flowToken-specific error handling has been added to the account creation process, utilizing the
ConnectNetworkHelperutility methods to display appropriate error messages to the user when token-related issues occur during registration.app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragmnet.java (5)
326-328: Simplified method signature and error messageThe
processFailuremethod signature has been updated to remove theIOException eparameter, and the error message has been simplified to a static message. This makes the code more maintainable and provides a clearer error message to the user.
340-348: Added token-specific error handling methodsNew callback methods have been added to handle specific token error scenarios:
processTokenUnavailableError: Displays a message when a token is temporarily unavailableprocessTokenRequestDeniedError: Displays a message when token requests are deniedThese methods use string resources to provide user-friendly error messages.
437-441: Improved error handling in verifySmsCodeThe
processFailuremethod has been updated to remove theIOException eparameter and use a simpler, more user-friendly error message from string resources. This enhances the user experience during SMS verification.
448-456: Added token-specific error handling to SMS verificationToken-specific error handling has been added to the SMS verification process, providing clear error messages to users when token-related issues occur during this critical step of the verification flow.
501-513: Enhanced error handling in resetPasswordThe implementation now uses the
ConnectNetworkHelperutility methods for displaying different types of errors including network issues and token-related problems. This provides a more consistent error handling approach across the application.app/src/org/commcare/connect/network/ApiConnect.java (6)
8-8: No issues with the newAuthInfoimport.
55-78: Same token-retrieval pattern as lines 23-45.
88-110: Same token-retrieval pattern as lines 23-45.
120-142: Same token-retrieval pattern as lines 23-45.
152-174: Same token-retrieval pattern as lines 23-45.
184-209: Same token-retrieval pattern as lines 23-45.app/src/org/commcare/connect/network/ConnectNetworkHelper.java (2)
343-346: Ensure 401 handling is appropriate.Currently, a 401 with token auth triggers
discardTokens(...)(existing code) and callshandler.processTokenUnavailableError(). Verify that this path adequately differentiates between a token that’s genuinely expired/unavailable versus unauthorized resource access.
370-370: Revised server error handling.Similarly,
handler.processFailure(responseCode)is invoked after a 5xx error. Confirm that higher-level layers provide users with enough feedback or retry logic on server-side issues.app/src/org/commcare/connect/ConnectManager.java (3)
807-826: Confirm fallback logic when DB passphrase retrieval fails.Token or network errors now invoke
Logger.logor user-facing methods likehandleTokenRequestDeniedException. Ensure that subsequent calls (e.g., retries, offline usage) are handled gracefully after these errors.
907-919: Refined token error flow inupdateLearningProgress.The new methods
processTokenUnavailableError()andprocessTokenRequestDeniedError()provide targeted error handling. If usage repeats in multiple places, consider a shared approach for uniform logging or analytics submission.
1029-1041: Refined token error flow inupdateDeliveryProgress.This block mirrors the approach in
updateLearningProgress. The consistent handling ensures clarity for end-users. Good job maintaining uniform patterns throughout.app/src/org/commcare/connect/network/ConnectSsoHelper.java (4)
27-28: Ensure consistent naming and usage with existing callbacks.
The addition oftokenUnavailable()andtokenRequestDenied()clearly distinguishes error states. Just confirm these new methods are consistently invoked wherever these error conditions arise.
47-58: Good handling of the new exceptions.
Catching bothTokenUnavailableExceptionandTokenRequestDeniedExceptionand storing them for post-execution handling is a solid approach. Just ensure all non-token-related exceptions are also properly handled or surfaced to the user if they might occur here.
77-81: Asynchronous Connect ID token retrieval is properly encapsulated.
The usage of anAsyncTaskhere is consistent. Ensure that thecallbackis thread-safe if it interacts with UI components.Do you want to verify all callback invocations on the main thread to prevent potential UI threading issues?
102-119: HQ SSO token retrieval with optional linking logic.
The reorganized workflow (check ifperformLinkor ifworkerLinkedis set) is a good approach. Ensure that user linking can’t interfere with the successful retrieval of the HQ token in partial network outages or other edge cases.Could you confirm the test coverage for partial retrieval failures or linking failures?
app/src/org/commcare/connect/network/ApiConnectId.java (4)
61-72: Refined heartbeat request logic.
Returning a dummyPostResultwith code-1when there is no FCM token helps signify an unusable result. Just be sure that the caller side understands and properly interprets that uninitialized code.
169-179: Effective handling of retrieved HQ token.
Storing the token and updating its expiration date is aligned with the changes for token unavailability/denial. Keep in mind that if the server changes the JSON schema, an extra safety check forjson.has(key)beforegetString(key)might prevent unexpected crashes.
187-187: ThrowingTokenUnavailableException.
Great for clarity to differentiate from a general exception. Ensure the calling methods properly handle or propagate this to the UI.
511-511: Synchronous encryption key retrieval.
Check if blocking the UI thread is acceptable here, especially if this call can be time-consuming. Consider deferring to a background thread or ensure the worker thread usage is enforced.
| @Override | ||
| public void processTokenRequestDeniedError() { | ||
| setJobListData(ConnectJobUtils.getCompositeJobs(getActivity(), -1, null)); | ||
| ConnectNetworkHelper.showOutdatedApiError(getContext()); | ||
| reportApiCall(false, 0, 0); | ||
| refreshUi(); | ||
| } |
There was a problem hiding this comment.
Incorrect error message in token request denied handler
Similar to the previous issue, this method is using the wrong error handler. It should use the token-specific handler instead of the outdated API error message.
Apply this fix:
public void processTokenRequestDeniedError() {
setJobListData(ConnectJobUtils.getCompositeJobs(getActivity(), -1, null));
- ConnectNetworkHelper.showOutdatedApiError(getContext());
+ ConnectNetworkHelper.handleTokenRequestDeniedException(getContext());
reportApiCall(false, 0, 0);
refreshUi();
}📝 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.
| @Override | |
| public void processTokenRequestDeniedError() { | |
| setJobListData(ConnectJobUtils.getCompositeJobs(getActivity(), -1, null)); | |
| ConnectNetworkHelper.showOutdatedApiError(getContext()); | |
| reportApiCall(false, 0, 0); | |
| refreshUi(); | |
| } | |
| @Override | |
| public void processTokenRequestDeniedError() { | |
| setJobListData(ConnectJobUtils.getCompositeJobs(getActivity(), -1, null)); | |
| ConnectNetworkHelper.handleTokenRequestDeniedException(getContext()); | |
| reportApiCall(false, 0, 0); | |
| refreshUi(); | |
| } |
| @Override | ||
| public void processTokenUnavailableError() { | ||
| setJobListData(ConnectJobUtils.getCompositeJobs(getActivity(), -1, null)); | ||
| ConnectNetworkHelper.showOutdatedApiError(getContext()); | ||
| reportApiCall(false, 0, 0); | ||
| refreshUi(); | ||
| } |
There was a problem hiding this comment.
Incorrect error message in token unavailability handler
The method is calling showOutdatedApiError() instead of the token-specific error handler handleTokenUnavailableException(). This will show users an incorrect error message when token unavailability is the actual issue.
Apply this fix:
public void processTokenUnavailableError() {
setJobListData(ConnectJobUtils.getCompositeJobs(getActivity(), -1, null));
- ConnectNetworkHelper.showOutdatedApiError(getContext());
+ ConnectNetworkHelper.handleTokenUnavailableException(getContext());
reportApiCall(false, 0, 0);
refreshUi();
}📝 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.
| @Override | |
| public void processTokenUnavailableError() { | |
| setJobListData(ConnectJobUtils.getCompositeJobs(getActivity(), -1, null)); | |
| ConnectNetworkHelper.showOutdatedApiError(getContext()); | |
| reportApiCall(false, 0, 0); | |
| refreshUi(); | |
| } | |
| @Override | |
| public void processTokenUnavailableError() { | |
| setJobListData(ConnectJobUtils.getCompositeJobs(getActivity(), -1, null)); | |
| ConnectNetworkHelper.handleTokenUnavailableException(getContext()); | |
| reportApiCall(false, 0, 0); | |
| refreshUi(); | |
| } |
| @Override | ||
| public void processTokenUnavailableError() { | ||
| ConnectNetworkHelper.handleTokenUnavailableException(context); | ||
| reportApiCall(false); | ||
| listener.connectActivityComplete(false); | ||
| } | ||
|
|
||
| @Override | ||
| public void processTokenRequestDeniedError() { | ||
| ConnectNetworkHelper.handleTokenRequestDeniedException(context); | ||
| reportApiCall(false); | ||
| listener.connectActivityComplete(false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle payment confirmation failure more robustly.
In addition to showing a toast for token unavailability or a token request denial, consider offering a retry option or queueing the payment confirmation for automatic retry once the token becomes valid again.
|
@damagatchi retest this please |
OrangeAndGreen
changed the title
|
@damagatchi retest this please |
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
Outdated
Show resolved
Hide resolved
…the relevant message for each scenario.
… into dv/token_exceptions
… into dv/token_exceptions
|
@OrangeAndGreen It seems that this branch is not compiling. Might be problem in locating |
|
@damagatchi retest this please |
1 similar comment
|
@damagatchi retest this please |
|
Build fixed, the cross-reference to a core PR was missing, and then to the wrong PR. |
Removed extra data in TokenUnavailableException (it was never used since logging happens earlier in the code).
| /** | ||
| * Exception thrown when an SSO token is unavailable. | ||
| * This can occur when the API call to retrieve an SSO token fails, i.e. due to bad network connectivity | ||
| * It can also happen if one SSO token (ConnectID) is being used as auth to acquire another SSO token (HQ), | ||
| * and the first token is rejected by the second OAuth server (i.e. due to expired token) | ||
| * The best course of action is usually to try again. | ||
| */ |
There was a problem hiding this comment.
nit: formatting seems off here
… into dv/token_exceptions
https://dimagi.atlassian.net/browse/CCCT-666
cross-request: dimagi/commcare-core#1455
Product Description
Better error messaging when there are issues with the SSO tokens (both ConnectID and HQ).
Technical Summary
Introduces two new exceptions related to token errors:
TokenUnavailableException = Unable to get token right now but will probably succeed on retry
TokenRequestDeniedException = Server no longer issuing tokens to this user. Specific to ConnectID
Feature Flag
ConnectID
Safety Assurance
Safety story
Requesting QA to do some regression testing.
These changes should only improve messaging when there are token issues.
But the changes grew large enough that I want to be sure nothing got broken.
Automated test coverage
No automated tests for ConnectID yet.
QA Plan
Testing for this generally involves testing the various API calls and verifying that they continue to work as expected.
Two specific tests to test the new exceptions:
Labels and Review