Pre Merge QA Build #2972
Pre Merge QA Build #2972
Conversation
…rity, separate notification channel
Drawing a red circle on the icon when there are unread messages.
…ssage is received for that channel and the key hasn't been retrieved yet.
…e-android into dv/pre_merge_qa
…are-android into dv/pre_merge_qa
📝 WalkthroughWalkthroughThis pull request removes two existing vector drawable resources and replaces them with new icons for messaging. The menu items have been updated to reference the new drawables. New localized string resources have been added for the messaging notification channel, including updates that add bell emojis to existing notification titles. A dedicated notification channel for messaging is defined in the notification manager with maximum importance. Several model files had unused imports removed, and new public methods have been introduced to allow clearing tokens in both linked app and user records. Enhancements in messaging functionality include the introduction of encryption key retrieval via new methods in the API helper class, along with refined error handling in network and request generator classes. A new utility method converts vector drawables into bitmaps for notifications, and adjustments have been made to correctly discard invalid tokens during authentication failures. Sequence Diagram(s)sequenceDiagram
participant MM as MessageManager
participant ACI as ApiConnectId
participant CNH as ConnectNetworkHelper
participant DB as Database
MM->>ACI: Check if encryption key exists
alt Key missing
ACI->>CNH: PostSync request with channel ID
CNH-->>ACI: Return response stream
ACI->>ACI: Handle response via handleReceivedEncryptionKey
ACI->>DB: Update channel record with encryption key
else Key exists
MM-->>MM: Process message normally
end
sequenceDiagram
participant CRG as CommcareRequestGenerator
participant Resp as API Response
participant CSH as ConnectSsoHelper
participant CLR as Client Records
CRG->>Resp: Execute network request
Resp-->>CRG: Return response (e.g., 401 error)
CRG->>CRG: Check for token error (checkForTokenError)
alt 401 Error with TokenAuth
CRG->>CSH: Invoke discardTokens method
CSH->>CLR: Clear tokens in linked app and user records
else
CRG-->>CRG: Continue normal processing
end
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 0
🔭 Outside diff range comments (1)
app/res/values/strings.xml (1)
347-356: 💡 Verification agent🧩 Analysis chain
Verify localization of notification strings
Let's ensure that the notification channel strings are properly localized in all supported languages to maintain a consistent user experience across different locales.
🏁 Script executed:
#!/bin/bash # Check if the messaging notification channel strings are localized in all supported languages # List all language-specific values directories fd -t d "values-" app/res # Check for the messaging notification channel strings in each language for lang_dir in $(fd -t d "values-" app/res); do echo "Checking $lang_dir" rg "notification_channel_messaging_title" "$lang_dir/strings.xml" || echo "Missing in $lang_dir" rg "notification_channel_messaging_description" "$lang_dir/strings.xml" || echo "Missing in $lang_dir" doneLength of output: 12843
Localization Check: Missing Messaging Notification Channel Strings
It appears that while the default language file (in app/res/values/strings.xml) includes the messaging notification channel strings, only a limited number of language-specific resource folders (namely values-pt and values-ti) provide localized entries. In several other folders intended for language translations (e.g., values-am, values-es, values-fr, values-lt, values-ne, values-no, and possibly values-sw), these keys are missing.
- Confirm that these language-specific resource folders are meant to have explicit translations rather than relying on the default strings.
- If explicit localization is required, please update the corresponding strings.xml files in these locales.
- For folders that serve as layout or qualifier overrides (e.g., values-large, values-sw320dp, etc.), no action is needed unless they are intended to provide translations.
🧹 Nitpick comments (5)
app/src/org/commcare/connect/network/ApiConnectId.java (5)
5-5: Unused import addedThe
android.util.Logimport has been added but is not used in this file, which creates unnecessary clutter.-import android.util.Log;
517-530: Add error logging for failed encryption key retrievalWhile the method correctly handles successful responses, it lacks error logging when the auth token is null or when the response is not in the 2xx range. Also, the method doesn't close the response stream if the response code is outside the success range, potentially causing a resource leak.
public static void retrieveChannelEncryptionKeySync(Context context, ConnectMessagingChannelRecord channel) { AuthInfo.TokenAuth auth = ApiConnectId.retrieveConnectIdTokenSync(context); if(auth != null) { HashMap<String, Object> params = new HashMap<>(); params.put("channel_id", channel.getChannelId()); ConnectNetworkHelper.PostResult result = ConnectNetworkHelper.postSync(context, channel.getKeyUrl(), null, auth, params, true, true); if(result.responseCode >= 200 && result.responseCode < 300) { handleReceivedEncryptionKey(context, result.responseStream, channel); + } else { + Logger.log(LogTypes.TYPE_MAINTENANCE, "Failed to retrieve encryption key for channel: " + channel.getChannelId() + ", response code: " + result.responseCode); + try { + if (result.responseStream != null) { + result.responseStream.close(); + } + } catch (IOException e) { + Logger.exception("Closing encryption key response stream", e); + } } + } else { + Logger.log(LogTypes.TYPE_MAINTENANCE, "Failed to retrieve Connect ID token for encryption key retrieval"); } }
543-556: Improve exception handling for encryption key processingThe current implementation catches all exceptions, which could mask specific issues. It would be better to catch specific exception types and validate the JSON response more thoroughly.
public static void handleReceivedEncryptionKey(Context context, InputStream stream, ConnectMessagingChannelRecord channel) { try { String responseAsString = new String( StreamsUtil.inputStreamToByteArray(stream)); if(responseAsString.length() > 0) { JSONObject json = new JSONObject(responseAsString); + if (!json.has("key")) { + Logger.log(LogTypes.TYPE_MAINTENANCE, "Missing key in encryption key response"); + return; + } channel.setKey(json.getString("key")); ConnectDatabaseHelper.storeMessagingChannel(context, channel); } - } catch(Exception e) { + } catch(IOException e) { + Logger.exception("Error reading encryption key response stream", e); + } catch(JSONException e) { Logger.exception("Messaging channel encryption key", e); } }
548-551: Add validation for the encryption keyThe code doesn't validate the encryption key beyond checking if the response is not empty. Consider adding validation for the key format or length to ensure it meets security requirements.
if(responseAsString.length() > 0) { JSONObject json = new JSONObject(responseAsString); + String key = json.getString("key"); + if (key == null || key.trim().isEmpty()) { + Logger.log(LogTypes.TYPE_MAINTENANCE, "Empty encryption key received for channel: " + channel.getChannelId()); + return; + } - channel.setKey(json.getString("key")); + channel.setKey(key); ConnectDatabaseHelper.storeMessagingChannel(context, channel); }
517-517: Make encryption key retrieval methods return statusBoth methods currently return void, making it difficult for callers to know if the operation succeeded. Consider returning a boolean to indicate success or failure.
-public static void retrieveChannelEncryptionKeySync(Context context, ConnectMessagingChannelRecord channel) { +public static boolean retrieveChannelEncryptionKeySync(Context context, ConnectMessagingChannelRecord channel) { AuthInfo.TokenAuth auth = ApiConnectId.retrieveConnectIdTokenSync(context); if(auth != null) { HashMap<String, Object> params = new HashMap<>(); params.put("channel_id", channel.getChannelId()); ConnectNetworkHelper.PostResult result = ConnectNetworkHelper.postSync(context, channel.getKeyUrl(), null, auth, params, true, true); if(result.responseCode >= 200 && result.responseCode < 300) { - handleReceivedEncryptionKey(context, result.responseStream, channel); + return handleReceivedEncryptionKey(context, result.responseStream, channel); } } + return false; } -public static void handleReceivedEncryptionKey(Context context, InputStream stream, ConnectMessagingChannelRecord channel) { +public static boolean handleReceivedEncryptionKey(Context context, InputStream stream, ConnectMessagingChannelRecord channel) { try { String responseAsString = new String( StreamsUtil.inputStreamToByteArray(stream)); if(responseAsString.length() > 0) { JSONObject json = new JSONObject(responseAsString); channel.setKey(json.getString("key")); ConnectDatabaseHelper.storeMessagingChannel(context, channel); + return true; } } catch(Exception e) { Logger.exception("Messaging channel encryption key", e); } + return false; }Also applies to: 543-543
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
app/res/drawable/ic_connect_menu_notification.xml(0 hunks)app/res/drawable/ic_connect_menu_notification_none.xml(0 hunks)app/res/drawable/ic_connect_messaging_base.xml(1 hunks)app/res/drawable/ic_connect_messaging_unread.xml(1 hunks)app/res/menu/menu_connect.xml(1 hunks)app/res/menu/menu_connect_messaging.xml(1 hunks)app/res/values-pt/strings.xml(1 hunks)app/res/values-ti/strings.xml(1 hunks)app/res/values/strings.xml(2 hunks)app/src/org/commcare/CommCareNoficationManager.java(2 hunks)app/src/org/commcare/activities/connect/ConnectActivity.java(1 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobAssessmentRecord.java(0 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecord.java(0 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobLearningRecord.java(0 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobPaymentRecord.java(0 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobPaymentRecordV3.java(0 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java(0 hunks)app/src/org/commcare/android/database/connect/models/ConnectLinkedAppRecord.java(1 hunks)app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java(1 hunks)app/src/org/commcare/connect/ConnectManager.java(0 hunks)app/src/org/commcare/connect/MessageManager.java(3 hunks)app/src/org/commcare/connect/network/ApiConnectId.java(5 hunks)app/src/org/commcare/connect/network/ConnectNetworkHelper.java(3 hunks)app/src/org/commcare/connect/network/ConnectSsoHelper.java(2 hunks)app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java(0 hunks)app/src/org/commcare/network/CommcareRequestGenerator.java(6 hunks)app/src/org/commcare/services/CommCareFirebaseMessagingService.java(4 hunks)
💤 Files with no reviewable changes (10)
- app/src/org/commcare/android/database/connect/models/ConnectJobLearningRecord.java
- app/src/org/commcare/android/database/connect/models/ConnectJobPaymentRecord.java
- app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java
- app/src/org/commcare/android/database/connect/models/ConnectJobPaymentRecordV3.java
- app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecord.java
- app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java
- app/res/drawable/ic_connect_menu_notification_none.xml
- app/res/drawable/ic_connect_menu_notification.xml
- app/src/org/commcare/android/database/connect/models/ConnectJobAssessmentRecord.java
- app/src/org/commcare/connect/ConnectManager.java
✅ Files skipped from review due to trivial changes (4)
- app/res/menu/menu_connect.xml
- app/res/menu/menu_connect_messaging.xml
- app/res/drawable/ic_connect_messaging_base.xml
- app/res/drawable/ic_connect_messaging_unread.xml
🧰 Additional context used
🧠 Learnings (1)
app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java (1)
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecordV5.java:72-76
Timestamp: 2025-01-26T14:32:09.266Z
Learning: In ConnectUserRecordV5, initializing lastPasswordDate and connectTokenExpiration to new Date() in the constructor is intentional.
🔇 Additional comments (36)
app/src/org/commcare/android/database/connect/models/ConnectLinkedAppRecord.java (1)
120-123:Details
✅ Verification successful
Good addition for handling invalid token scenarios.
This new method complements the existing
updateHqTokenmethod by providing a way to clear token information when it's determined to be invalid. This supports the PR's goal of deleting tokens when invalid tokens are detected.Let's check if similar token clearance methods are implemented consistently across related classes:
🏁 Script executed:
#!/bin/bash # Description: Verify if token clearance is implemented consistently # Find token-related methods/fields in the codebase rg -A 3 "Token\s*=\s*null" --glob "*.java" rg -A 3 "clearToken|clearHqToken|clearConnectToken" --glob "*.java"Length of output: 3192
Token clearance consistency verified
The implementation of
clearHqTokeninConnectLinkedAppRecord.javais consistent with the token clearance methods in related classes, such asclearConnectTokeninConnectUserRecord.java. Both methods correctly set the token tonulland update the expiration date with a new instance ofDate(). Additionally, the calls to these methods fromConnectSsoHelper.javaconfirm that they're used uniformly across the codebase.app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java (1)
197-200: Well-designed token removal method.This method mirrors the
clearHqToken()added toConnectLinkedAppRecord, providing a consistent way to invalidate tokens. The implementation correctly nullifies the token and resets the expiration date to the current time.The pattern matches the existing
updateConnectTokenmethod, maintaining code consistency.app/src/org/commcare/activities/connect/ConnectActivity.java (1)
165-168:Details
✅ Verification successful
Good improvement to the messaging icon nomenclature.
The icon resources have been renamed from generic notification icons to clearly messaging-specific icons, making their purpose more obvious. The naming is also more descriptive - "base" for no unread messages and "unread" for when there are unviewed messages.
Let's confirm that the new icon resources are properly defined and that menu layouts have been updated accordingly:
🏁 Script executed:
#!/bin/bash # Description: Verify that the new icon resources exist and menu files are updated # Check for the new icon resource definitions echo "Looking for new messaging icon resources..." find app/res -name "ic_connect_messaging_*.xml" -type f # Check if menu layouts reference the new icons echo "Checking menu layout files for icon references..." rg "ic_connect_messaging_base" app/res/menu/Length of output: 592
Messaging icon resources and menu references verified
The new icon resource files (
app/res/drawable/ic_connect_messaging_base.xmlandapp/res/drawable/ic_connect_messaging_unread.xml) have been confirmed to exist, and the menu layout files (app/res/menu/menu_connect.xmlandapp/res/menu/menu_connect_messaging.xml) correctly reference the new icons. This confirms that the improvements to the messaging icon nomenclature are properly implemented.app/res/values-ti/strings.xml (1)
286-287:Details
✅ Verification successful
Good localization of messaging notification strings.
The addition of these localized strings for the messaging notification channel in Tigrinya ensures that users of this locale will see properly translated notification channel information. This is a good internationalization practice.
Let's check if these strings have been consistently added to other language resource files:
🏁 Script executed:
#!/bin/bash # Description: Verify if messaging notification strings are present in other locales # Look for notification_channel_messaging strings in other language resource files echo "Checking other language resources for messaging strings..." rg "notification_channel_messaging_title" app/res/values*/strings.xml rg "notification_channel_messaging_description" app/res/values*/strings.xmlLength of output: 1928
Localization Check Complete: Consistent Messaging Strings
Excellent work on adding the Tigrinya localized messaging notification strings in app/res/values-ti/strings.xml (lines 286-287). The verification shows that these strings are consistently implemented across the other locale resource files (e.g., default and Portuguese), confirming that users will receive properly translated messaging notifications.
app/src/org/commcare/CommCareNoficationManager.java (2)
43-43: Well-defined constant for messaging notifications.The new constant follows the existing naming pattern and will be used to identify the messaging notification channel.
200-203: High-priority notification channel for messaging appropriately configured.Setting the messaging notification channel to
IMPORTANCE_MAXis appropriate for high-priority messaging notifications that require immediate user attention. This ensures users won't miss important messages.app/res/values-pt/strings.xml (1)
298-299: Properly localized message notification strings.The Portuguese translation for the messaging notification channel title and description are appropriate and consistent with the application's localization strategy.
app/src/org/commcare/connect/network/ConnectSsoHelper.java (3)
7-8: New imports for token management functionality.The imports for
ConnectUserRecordand logging utilities are necessary for the new token discarding functionality.
12-13: Appropriate logging imports added.The imports for
LogTypesandLoggerare necessary for proper logging in the new method.
102-120: Well-implemented token cleanup mechanism.The
discardTokensmethod provides a clean way to clear invalid tokens from both the app record and user record. This is important for security and authentication integrity when tokens are detected as invalid.Some specific strengths of this implementation:
- It properly handles the case when username is null
- It includes appropriate logging
- It updates the database records after clearing the tokens
app/src/org/commcare/connect/MessageManager.java (3)
7-8: Good use of utility library.Adding the Google Guava
Stringsutility class provides a clean way to check for null or empty strings, improving code readability.
46-52: Auto-retrieval of encryption key enhances security.This change adds a fallback mechanism to retrieve missing encryption keys before processing messages. The null/empty check with
Strings.isNullOrEmpty()is clear and the comment on line 51 explains the behavior well.
215-219: Improved encryption key handling.The refactoring to use
ApiConnectId.handleReceivedEncryptionKey()centralizes the encryption key handling logic and simplifies this method. This is a good architectural improvement that likely reduces code duplication.app/src/org/commcare/network/CommcareRequestGenerator.java (6)
153-154: Good refactoring to store auth info in a variableThis change improves code readability and consistency by storing the authentication information in a variable before passing it to the requester.
162-164: Good enhancement for token error handlingStoring the response and checking for token errors before returning ensures that invalid tokens are properly handled. This follows the fail-fast principle and improves security by immediately discarding invalid tokens.
215-220: Well-implemented token error detectionThe new method for checking token errors is well-implemented with:
- Clear conditions checking for 401 (unauthorized) status code
- Appropriate type checking for token authentication
- Proper logging of the exception
- Immediate token invalidation via ConnectSsoHelper
This implements the "Deleting tokens when invalid token detected" objective from the PR.
233-244: Consistent implementation of token error handling across methodsThe same pattern is appropriately applied to the
makeKeyFetchRequestmethod, which maintains consistency throughout the codebase.
287-302: Consistent implementation in postMultipart methodThe auth variable pattern and token error checking are consistently applied to the
postMultipartmethod as well.
315-325:Details
✅ Verification successful
Verify token error handling implementation is consistent across all methods
The pattern is also applied to the
simpleGetmethod, which ensures consistent error handling throughout the class.
🏁 Script executed:
#!/bin/bash # Verify that all network request methods implement token error checking # Check if all network methods in CommcareRequestGenerator have token error checking rg "checkForTokenError\(response, auth\)" app/src/org/commcare/network/CommcareRequestGenerator.java # Verify if there are any methods making network requests that are not utilizing the token error check rg "makeRequest\(\)" app/src/org/commcare/network/CommcareRequestGenerator.java | grep -v "response = requester.makeRequest"Length of output: 409
Token Error Handling Implementation Verified
The token error handling is applied consistently across all relevant methods in the CommcareRequestGenerator class. The verification confirms that every network request using
makeRequest()is followed by the appropriatecheckForTokenError(response, auth)call, including in thesimpleGetmethod. No deviations were found.app/src/org/commcare/connect/network/ConnectNetworkHelper.java (5)
201-202: Good enhancement to pass token authentication statusPassing the token authentication status (
authInfo instanceof AuthInfo.TokenAuth) to the response processor enables more targeted error handling based on the authentication type.
328-329: Consistent implementation across methodsThe same pattern is applied to the
getInternalmethod, ensuring consistent error handling throughout the class.
335-336: Method signature updated to support token-specific error handlingThe
getResponseProcessormethod signature is appropriately updated to include theusingTokenAuthparameter, which will be used for conditional error handling.
356-360: Enhanced error handling for token authenticationThis conditional block provides specialized error handling for token authentication failures:
- It checks for HTTP 401 (unauthorized) status
- It verifies that token authentication is being used
- It logs the exception and discards the tokens
This implements the "Deleting tokens when invalid token detected" objective from the PR and ensures consistent behavior across the application.
344-363:Details
✅ Verification successful
Verify consistent token invalidation across the codebase
The token invalidation is implemented in both
CommcareRequestGeneratorandConnectNetworkHelper. Let's verify thatConnectSsoHelper.discardTokensis working consistently across both implementations.
🏁 Script executed:
#!/bin/bash # Verify the implementation of discardTokens method in ConnectSsoHelper # Check for the definition of discardTokens rg "discardTokens" app/src/org/commcare/connect/network/ConnectSsoHelper.java # Check for all usages of discardTokens across codebase rg "ConnectSsoHelper\.discardTokens"Length of output: 588
Consistency Verified: Token Invalidation Across Implementations
The verification confirms that the
ConnectSsoHelper.discardTokensmethod is consistently used across the codebase:
- In ConnectNetworkHelper the call is made with
context, null.- In ApiConnectId and CommcareRequestGenerator the same method is invoked with context and the respective username parameters.
No discrepancies were found in the usage or implementation across these locations, so the token invalidation behavior is consistent.
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (5)
102-111: Good utility method for bitmap conversionThe new
getBitmapFromVectorDrawablemethod is well implemented:
- It handles the conversion from vector drawable to bitmap properly
- It sets appropriate dimensions based on the intrinsic size of the drawable
- It draws the drawable on the canvas correctly
This supports the "Improvements to messaging in-app icon" objective from the PR.
123-125: Good practice to define variables for notification parametersDefining variables for notification channel, priority, and large icon makes the code more maintainable and allows for conditional customization based on the notification type.
131-134: Enhanced notification configuration for messagingThis implementation provides specialized notification handling for messaging:
- Uses a dedicated messaging notification channel
- Sets maximum priority for messaging notifications
- Uses a custom large icon for better visual recognition
These enhancements improve user experience by making messaging notifications more prominent and visually distinct.
215-222: Consistent use of notification parametersThe notification builder correctly uses the dynamically determined parameters for channel, large icon, and priority, ensuring consistent notification appearance based on the notification type.
7-14:Details
✅ Verification successful
Verify resource availability
Let's ensure that the drawable resource
R.drawable.commcare_actionbar_logois available and that the notification channelCommCareNoficationManager.NOTIFICATION_CHANNEL_MESSAGING_IDis properly defined.
🏁 Script executed:
#!/bin/bash # Verify resource availability for messaging notifications # Check for the drawable resource fd "commcare_actionbar_logo" app/res/drawable # Check for the notification channel definition rg "NOTIFICATION_CHANNEL_MESSAGING_ID" app/src/org/commcare/CommCareNoficationManager.java # Check for the creation of the messaging notification channel rg "createNotificationChannel.*NOTIFICATION_CHANNEL_MESSAGING_ID" app/src/org/commcare/CommCareNoficationManager.javaLength of output: 593
Resource Availability Verified
- The drawable resource
R.drawable.commcare_actionbar_logois found atapp/res/drawable/commcare_actionbar_logo.xml.- The notification channel constant
CommCareNoficationManager.NOTIFICATION_CHANNEL_MESSAGING_IDis properly defined and used, with the creation of the channel clearly present inCommCareNoficationManager.java.app/res/values/strings.xml (3)
354-355: Good addition of messaging notification channel stringsThe new string resources for the messaging notification channel are well-defined:
- Clear title that indicates the purpose of the channel ("Messages from CommCare")
- Descriptive text that explains what kinds of notifications will come through this channel
These resources support the implementation of the dedicated messaging notification channel.
555-555: Enhanced visual notification title with emojisAdding bell emojis (🔔) to the message notification title makes it more visually distinctive and increases its prominence in the notification drawer. This enhances user experience by making messaging notifications more noticeable.
557-557: Consistent styling for notification titlesThe same bell emoji styling is applied to the channel notification title, maintaining visual consistency across different types of messaging notifications.
app/src/org/commcare/connect/network/ApiConnectId.java (4)
103-103: Improved HTTP status code handlingThe code now correctly handles all success HTTP status codes (2xx) rather than just 200 (OK). This makes the code more robust by properly handling other success codes like 201 (Created), 202 (Accepted), etc.
127-129: Good error handling for invalid tokensAdding specific handling for 401 (Unauthorized) responses is excellent. The code now properly logs the authentication failure and discards invalid tokens, preventing future failed requests with the same token.
517-530:Details
❓ Verification inconclusive
Verify impact on UI thread
This is a synchronous method that performs network operations. If called from the UI thread, it could cause ANR (Application Not Responding) issues. Consider adding documentation to warn callers or refactor to use an asynchronous approach consistent with other methods in the class.
🏁 Script executed:
#!/bin/bash # Description: Check where retrieveChannelEncryptionKeySync is called to verify thread safety # Find where the method is being called rg "retrieveChannelEncryptionKeySync" --type javaLength of output: 348
Potential UI Thread Block Warning
The synchronous method
retrieveChannelEncryptionKeySyncinvokes network operations and is called fromMessageManager.java. This may lead to UI thread blocking if not handled properly. Please verify that its callers (especially inMessageManager.java) ensure execution on a background thread. If there's any chance the method might be invoked from the UI thread, consider adding documentation to warn callers or refactoring it to adopt an asynchronous approach consistent with the rest of the class.
526-528:Details
❓ Verification inconclusive
Verify stream closure after processing
The stream is passed to
handleReceivedEncryptionKeybut it's not clear if it gets closed. To prevent resource leaks, ensure the stream is always closed, even in error cases.
🏁 Script executed:
#!/bin/bash # Description: Check if the response stream is properly closed in the codebase # Search for stream closing in methods related to encryption key handling rg -A 5 -B 5 "handleReceivedEncryptionKey|retrieveChannelEncryptionKeySync" | rg "close\(\)" # Check for similar network methods to see how they handle stream closure rg -A 5 "postSync" | rg "responseStream.*close\(\)"Length of output: 252
Action Required: Verify Stream Closure in Encryption Key Handling
- When
result.responseCodeis between 200 and 299, the response stream is passed tohandleReceivedEncryptionKey.- In other parts of the code (e.g., in the
postSyncbranch), the stream is explicitly closed viapostResult.responseStream.close().- Please confirm that
handleReceivedEncryptionKeyproperly closes the stream—even in error scenarios. If it doesn’t, refactor the code (for example, by using a try-with-resources block or a final cleanup) to ensure no resource leaks occur.
Using this PR to create a special build for QA testing that includes these PRs that haven't been merged yet:
Encryption key auto-retrieval for messaging
Improvements to messaging in-app icon
Deleting tokens when invalid token detected
cross-request: dimagi/commcare-core#1455