Refactor API calls #3479
Refactor API calls #3479
Conversation
📝 WalkthroughWalkthroughThis PR refactors the Connect networking layer to standardize asynchronous API handling through a PersonalIdApiHandler abstraction. It removes hardcoded URL string resources, adds new Retrofit-based API endpoints for channel consent and messaging operations, migrates AsyncTask-based token retrieval to CompletableFuture in ConnectSsoHelper, and introduces parser classes for processing HQ token retrieval, channel encryption key, and HQ worker linking responses. Public method signatures are largely preserved; internal implementation shifts to callback-based handler patterns with centralized error handling. Sequence Diagram(s)sequenceDiagram
participant Client as Client/MessageManager
participant Handler as PersonalIdApiHandler
participant ApiPersonalId
participant Retrofit as Retrofit ApiService
participant Backend as Backend API
participant Callback as Response Callback/Parser
rect rgb(220, 245, 220)
note right of Client: Channel Consent Flow
Client->>Handler: updateChannelConsent(context, user, channel)
Handler->>ApiPersonalId: updateChannelConsent(context, user, channel)
ApiPersonalId->>Retrofit: POST /messaging/update_consent/
Retrofit->>Backend: Send request with token & headers
Backend-->>Retrofit: 200 Response
Retrofit-->>Callback: onResponse(Response)
Callback->>Handler: onSuccess(Boolean)
Handler->>Client: Notify listener
end
rect rgb(220, 245, 240)
note right of Client: Channel Encryption Key Retrieval
Client->>Handler: retrieveChannelEncryptionKey(context, user, channel)
Handler->>ApiPersonalId: retrieveChannelEncryptionKey(context, user, channel)
ApiPersonalId->>Retrofit: POST with channelUrl
Retrofit->>Backend: Send request
Backend-->>Retrofit: JSON with key
Retrofit-->>Callback: onResponse(InputStream)
Callback->>Callback: parse(JSON) → extract key
Callback->>Callback: storeMessagingChannel(channel)
Handler->>Client: onSuccess(Boolean)
end
rect rgb(245, 230, 220)
note right of Client: Send Messaging Flow
Client->>Handler: sendMessagingMessage(context, user, message, channel)
Handler->>ApiPersonalId: sendMessagingMessage(context, user, message, channel)
ApiPersonalId->>Retrofit: POST /messaging/send_message/
Retrofit->>Backend: Send with encryption key
Backend-->>Retrofit: 200 OK
Retrofit-->>Callback: onResponse(Response)
Callback->>Handler: onSuccess(Boolean)
Handler->>Client: Mark message confirmed & persist
end
rect rgb(240, 230, 245)
note right of Client: HQ Token Async Retrieval
Client->>Handler: retrieveHqTokenASync(context, hqUsername, connectToken)
Handler->>ApiPersonalId: retrieveHqTokenASync(context, hqUsername, connectToken)
ApiPersonalId->>Retrofit: GET /token endpoint
Retrofit->>Backend: Send request
Backend-->>Retrofit: JSON {token, expires}
Retrofit-->>Callback: onResponse(InputStream)
Callback->>Callback: parse(JSON) → TokenAuth
Callback->>Callback: storeHqToken(context, seatedAppId, hqUsername)
Handler->>Client: onSuccess(TokenAuth)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/org/commcare/connect/MessageManager.java (1)
83-106: Missing listener callback when channel is null or key is empty.When
channelis null or the key is empty (line 87), the method logs an error but never invokeslistener.connectActivityComplete(false). This leaves the caller waiting indefinitely without notification of failure.Proposed fix
} else { Logger.log("Messaging", "Tried to send message but no encryption key"); + listener.connectActivityComplete(false); }
🤖 Fix all issues with AI Agents
In @app/src/org/commcare/connect/MessageManager.java:
- Around line 54-69: Fix the typo in the log message inside
getChannelEncryptionKey: update the Logger.log call in the
PersonalIdApiHandler.onFailure override to use "retrieve" instead of "retreive"
so the message reads "Failed to retrieve encryption key: ..." (locate the
Logger.log in getChannelEncryptionKey / PersonalIdApiHandler.onFailure to change
the string).
In
@app/src/org/commcare/connect/network/connectId/parser/RetrieveChannelEncryptionKeyResponseParser.kt:
- Around line 26-34: The parser currently treats an empty HTTP body as a
successful response and only catches JSONException; update retrieve logic in
RetrieveChannelEncryptionKeyResponseParser (the block that reads
responseAsString, parses JSONObject, calls channel.setKey(...) and
ConnectMessagingDatabaseHelper.storeMessagingChannel(...)) so that an empty
response is treated as an error (e.g., throw a RuntimeException or return a
failure rather than returning true) and ensure
StreamsUtil.inputStreamToByteArray() IOExceptions are caught and handled the
same way as the JSONException so missing/empty bodies or IO failures don’t
silently succeed.
In
@app/src/org/commcare/connect/network/connectId/parser/RetrieveHqTokenResponseParser.kt:
- Line 41: CommCareApplication.instance().getCurrentApp() can be null, so before
calling getUniqueId() in RetrieveHqTokenResponseParser (where you create
seatedAppId and then call SsoToken and ConnectDatabaseHelper.storeHqToken),
fetch currentApp into a local variable, check it for null, and only call
currentApp.getUniqueId() and store the token when non-null; in the else branch
handle the no-app-seated case (e.g., log an error via the parser's logger and
return/skip storing) so you avoid an NPE.
In @app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java:
- Around line 348-362: Rename the misspelled parameter `messsage` to `message`
in the method signature of sendMessagingMessage and update all references inside
the method body (e.g., the argument passed to
ApiPersonalId.sendMessagingMessage) to use the corrected name; ensure the
parameter type remains ConnectMessagingMessageRecord and no other behavior
changes are introduced.
In @app/src/org/commcare/connect/network/ConnectSsoHelper.java:
- Around line 261-266: Fix the typo "workder" to "worker" in the failure logging
inside the onFailure callback in ConnectSsoHelper: update both Logger.exception
calls (the one logging the Throwable t and the one constructing a new Throwable)
so the message reads "Failed to link HQ worker" and keep existing Throwable
usage and context intact.
🧹 Nitpick comments (5)
app/src/org/commcare/connect/network/ApiEndPoints.java (1)
25-27: Minor formatting inconsistency.Line 26 is missing a space after the
=sign, unlike line 25 and other constants in this file.🔎 Suggested fix
public static final String CONNECT_MESSAGE_CHANNEL_CONSENT_URL = "/messaging/update_consent/"; - public static final String CONNECT_MESSAGE_SEND_URL ="/messaging/send_message/"; + public static final String CONNECT_MESSAGE_SEND_URL = "/messaging/send_message/";app/src/org/commcare/connect/network/connectId/parser/LinkHqWorkerResponseParser.kt (1)
16-20: Consider null-safety check foranyInputObject.The cast on line 17 will throw a
ClassCastExceptionifanyInputObjectis null or not aConnectLinkedAppRecord. Based on the fail-fast guidance for Kotlin, an explicit check with a clear error message would help debugging.🔎 Suggested improvement
): T { - val appRecord: ConnectLinkedAppRecord = anyInputObject as ConnectLinkedAppRecord + val appRecord: ConnectLinkedAppRecord = anyInputObject as? ConnectLinkedAppRecord + ?: throw IllegalArgumentException("Expected ConnectLinkedAppRecord but got ${anyInputObject?.javaClass?.name}") appRecord.setWorkerLinked(true) ConnectAppDatabaseUtil.storeApp(context, appRecord) return true as T }app/src/org/commcare/connect/network/ConnectSsoHelper.java (1)
161-169:InterruptedExceptionhandling discards thread interrupt status.When catching
InterruptedException, the thread's interrupt status is lost. Best practice is to restore it by callingThread.currentThread().interrupt(). Also consider adding a timeout tocompletableFuture.get()to prevent indefinite blocking.🔎 Suggested improvement
+import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; ... try { - return completableFuture.get(); + return completableFuture.get(30, TimeUnit.SECONDS); } catch (InterruptedException | ExecutionException e) { + if (e instanceof InterruptedException) { + Thread.currentThread().interrupt(); + } if(e.getCause() instanceof TokenDeniedException){ throw new TokenDeniedException(); }else{ throw new TokenUnavailableException(); } + } catch (TimeoutException e) { + throw new TokenUnavailableException(); }app/src/org/commcare/connect/network/connectId/parser/RetrieveHqTokenResponseParser.kt (1)
25-32: Consider adding null-safety and IOException handling.The cast on line 25 will throw if
anyInputObjectis null. Additionally,StreamsUtil.inputStreamToByteArray()can throwIOExceptionwhich is not caught.🔎 Suggested improvement
+import java.io.IOException + override fun parse( responseCode: Int, responseData: InputStream, anyInputObject: Any?, ): T { - val hqUsername: String = anyInputObject as String + val hqUsername: String = anyInputObject as? String + ?: throw IllegalArgumentException("Expected String (hqUsername) but got ${anyInputObject?.javaClass?.name}") try { val responseAsString = String( StreamsUtil.inputStreamToByteArray( responseData, ), ) // ... rest of parsing - } catch (e: JSONException) { + } catch (e: Exception) { + when (e) { + is JSONException, is IOException -> throw RuntimeException(e) + else -> throw e + } - throw RuntimeException(e) } }app/src/org/commcare/connect/network/ApiPersonalId.java (1)
167-189: Consider extracting HQ OAuth client ID to a constant.The client ID
"4eHlQad1oasGZF0lPiycZIjyL0SY1zx7ZblA6SCV"is duplicated here (line 169) and inretrieveHqTokenSync(line 194). Extracting it to a named constant would improve maintainability and prevent drift.Proposed refactor
public class ApiPersonalId { private static final String API_VERSION_NONE = null; private static final String CONNECT_CLIENT_ID = "zqFUtAAMrxmjnC1Ji74KAa6ZpY1mZly0J0PlalIa"; + private static final String HQ_OAUTH_CLIENT_ID = "4eHlQad1oasGZF0lPiycZIjyL0SY1zx7ZblA6SCV";Then use
HQ_OAUTH_CLIENT_IDin both methods.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/res/values/strings.xmlapp/src/org/commcare/connect/MessageManager.javaapp/src/org/commcare/connect/network/ApiEndPoints.javaapp/src/org/commcare/connect/network/ApiPersonalId.javaapp/src/org/commcare/connect/network/ApiService.javaapp/src/org/commcare/connect/network/ConnectSsoHelper.javaapp/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.javaapp/src/org/commcare/connect/network/connectId/parser/LinkHqWorkerResponseParser.ktapp/src/org/commcare/connect/network/connectId/parser/RetrieveChannelEncryptionKeyResponseParser.ktapp/src/org/commcare/connect/network/connectId/parser/RetrieveHqTokenResponseParser.kt
💤 Files with no reviewable changes (1)
- app/res/values/strings.xml
🧰 Additional context used
🧠 Learnings (24)
📓 Common learnings
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T13:40:19.645Z
Learning: PR #3048 introduces a comprehensive messaging system in the Connect feature, implementing secure encryption using AES-GCM for message content, proper channel management with consent flows, and a well-designed UI separation between sent and received messages with real-time notification integration.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/connect/MessageManager.java:0-0
Timestamp: 2025-07-29T14:54:47.940Z
Learning: User OrangeAndGreen organizes messaging-related changes into separate PRs rather than mixing them with other Connect work, which aligns with their preference for handling code issues in separate PRs when they are not directly related to the main changes.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/connect/network/ApiPersonalId.java:0-0
Timestamp: 2025-07-29T14:56:40.681Z
Learning: User OrangeAndGreen prefers to defer ApiPersonalId refactoring (specifically InputStream resource management improvements) to separate PRs rather than mixing them with Connect feature work, even when the code is already in master.
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2847
File: app/src/org/commcare/connect/network/connectId/ApiService.java:8-63
Timestamp: 2025-01-28T09:38:59.882Z
Learning: The ConnectID API service methods should use Map<String, String> for request bodies and ResponseBody for responses, as per team preference.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:206-213
Timestamp: 2025-01-21T18:20:43.883Z
Learning: In the Connect ID feature, server responses containing user data should be parsed using ConnectUserResponseParser to ensure consistent handling of payment information (owner_name and phone_number) and secondary phone verification across all authentication flows.
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2949
File: app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java:244-275
Timestamp: 2025-02-04T21:22:56.537Z
Learning: Direct JSONObject parsing is acceptable for handling user data responses in ConnectIdPinFragment, as decided by the team. No need to enforce ConnectUserResponseParser usage in this context.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/connect/MessageManager.java:0-0
Timestamp: 2025-07-29T14:45:13.470Z
Learning: In the CommCare Android Connect messaging system (MessageManager.java), the error handling logic in updateChannelConsent's processFailure callback intentionally attempts to retrieve the channel encryption key even after consent update failure. This behavior was recommended by the server team and the worst case scenario is silently failing to retrieve the encryption key, which is considered acceptable.
📚 Learning: 2025-05-08T13:40:19.645Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T13:40:19.645Z
Learning: PR #3048 introduces a comprehensive messaging system in the Connect feature, implementing secure encryption using AES-GCM for message content, proper channel management with consent flows, and a well-designed UI separation between sent and received messages with real-time notification integration.
Applied to files:
app/src/org/commcare/connect/network/ApiEndPoints.javaapp/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.javaapp/src/org/commcare/connect/MessageManager.javaapp/src/org/commcare/connect/network/ApiPersonalId.java
📚 Learning: 2025-07-29T14:45:13.470Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/connect/MessageManager.java:0-0
Timestamp: 2025-07-29T14:45:13.470Z
Learning: In the CommCare Android Connect messaging system (MessageManager.java), the error handling logic in updateChannelConsent's processFailure callback intentionally attempts to retrieve the channel encryption key even after consent update failure. This behavior was recommended by the server team and the worst case scenario is silently failing to retrieve the encryption key, which is considered acceptable.
Applied to files:
app/src/org/commcare/connect/network/ApiEndPoints.javaapp/src/org/commcare/connect/network/connectId/parser/RetrieveChannelEncryptionKeyResponseParser.ktapp/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.javaapp/src/org/commcare/connect/network/ApiService.javaapp/src/org/commcare/connect/MessageManager.javaapp/src/org/commcare/connect/network/ApiPersonalId.java
📚 Learning: 2025-05-09T10:57:41.073Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 3093
File: app/res/navigation/nav_graph_connect_messaging.xml:41-45
Timestamp: 2025-05-09T10:57:41.073Z
Learning: In the CommCare Android codebase, the navigation graph for Connect messaging (`nav_graph_connect_messaging.xml`) intentionally uses `channel_id` as the argument name in the connectMessageFragment, despite using `channelId` in other parts of the same navigation graph. This naming difference is by design in the refactored code.
Applied to files:
app/src/org/commcare/connect/network/ApiEndPoints.javaapp/src/org/commcare/connect/MessageManager.java
📚 Learning: 2025-05-08T11:08:18.530Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
Applied to files:
app/src/org/commcare/connect/network/ApiEndPoints.javaapp/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.javaapp/src/org/commcare/connect/MessageManager.javaapp/src/org/commcare/connect/network/ConnectSsoHelper.javaapp/src/org/commcare/connect/network/ApiPersonalId.java
📚 Learning: 2025-05-28T11:30:37.998Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 3133
File: app/src/org/commcare/connect/network/connectId/ApiService.java:55-55
Timestamp: 2025-05-28T11:30:37.998Z
Learning: In the CommCare Android codebase, API service method names in ApiService.java should match the format of the actual API endpoint names rather than using semantically meaningful names. For example, if the endpoint is "users/set_recovery_pin", the method name should follow that endpoint structure for consistency and maintainability.
Applied to files:
app/src/org/commcare/connect/network/ApiEndPoints.javaapp/src/org/commcare/connect/network/ApiService.javaapp/src/org/commcare/connect/network/ApiPersonalId.java
📚 Learning: 2025-01-21T18:20:43.883Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:206-213
Timestamp: 2025-01-21T18:20:43.883Z
Learning: In the Connect ID feature, server responses containing user data should be parsed using ConnectUserResponseParser to ensure consistent handling of payment information (owner_name and phone_number) and secondary phone verification across all authentication flows.
Applied to files:
app/src/org/commcare/connect/network/connectId/parser/LinkHqWorkerResponseParser.ktapp/src/org/commcare/connect/network/connectId/parser/RetrieveChannelEncryptionKeyResponseParser.ktapp/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.javaapp/src/org/commcare/connect/MessageManager.javaapp/src/org/commcare/connect/network/connectId/parser/RetrieveHqTokenResponseParser.kt
📚 Learning: 2025-02-04T21:22:56.537Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2949
File: app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java:244-275
Timestamp: 2025-02-04T21:22:56.537Z
Learning: Direct JSONObject parsing is acceptable for handling user data responses in ConnectIdPinFragment, as decided by the team. No need to enforce ConnectUserResponseParser usage in this context.
Applied to files:
app/src/org/commcare/connect/network/connectId/parser/LinkHqWorkerResponseParser.ktapp/src/org/commcare/connect/network/connectId/parser/RetrieveChannelEncryptionKeyResponseParser.ktapp/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.javaapp/src/org/commcare/connect/network/connectId/parser/RetrieveHqTokenResponseParser.kt
📚 Learning: 2025-12-10T16:39:05.007Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3448
File: app/src/org/commcare/gis/EntityMapUtils.kt:214-234
Timestamp: 2025-12-10T16:39:05.007Z
Learning: In Kotlin files (here: app/src/org/commcare/gis/EntityMapUtils.kt), follow fail-fast guidance: treat conditions that indicate a programmer error or broken invariant as exceptions rather than defensive handling. If a condition represents a bug (e.g., internal API assumptions like array/list size mismatches), crash early to alert developers instead of attempting to recover. Use clear runtime exceptions (e.g., IllegalStateException, IllegalArgumentException) or assertions where appropriate, and avoid swallowing or masking bugs. Ensure invariants are checked and failures are surfaced with actionable messages for quick debugging.
Applied to files:
app/src/org/commcare/connect/network/connectId/parser/LinkHqWorkerResponseParser.ktapp/src/org/commcare/connect/network/connectId/parser/RetrieveChannelEncryptionKeyResponseParser.ktapp/src/org/commcare/connect/network/connectId/parser/RetrieveHqTokenResponseParser.kt
📚 Learning: 2025-12-08T12:01:48.545Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 3455
File: app/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParser.kt:64-93
Timestamp: 2025-12-08T12:01:48.545Z
Learning: In the CommCare Android Connect notification system (RetrieveNotificationsResponseParser.kt), messages are only decrypted using channels that already exist in the database with their encryption keys. When new channels arrive without keys, a separate network flow fetches the channel keys, and then the notification API is re-called to decrypt any messages for those channels. This explains why parseAndSeparateNotifications() correctly uses existingChannels from the database rather than newly parsed channels.
Applied to files:
app/src/org/commcare/connect/network/connectId/parser/RetrieveChannelEncryptionKeyResponseParser.ktapp/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.javaapp/src/org/commcare/connect/MessageManager.javaapp/src/org/commcare/connect/network/connectId/parser/RetrieveHqTokenResponseParser.ktapp/src/org/commcare/connect/network/ApiPersonalId.java
📚 Learning: 2025-04-22T17:04:26.780Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3043
File: app/src/org/commcare/android/database/connect/models/ConnectMessagingChannelRecord.java:0-0
Timestamp: 2025-04-22T17:04:26.780Z
Learning: In the Connect API contract, the server doesn't include the fields `created`, `answered_consent`, and `key` in JSON responses for messaging channels, which is why the `fromJson` method in `ConnectMessagingChannelRecord` explicitly initializes these values rather than parsing them from JSON.
Applied to files:
app/src/org/commcare/connect/network/connectId/parser/RetrieveChannelEncryptionKeyResponseParser.ktapp/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.javaapp/src/org/commcare/connect/MessageManager.javaapp/src/org/commcare/connect/network/ApiPersonalId.java
📚 Learning: 2025-04-22T17:04:26.780Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3043
File: app/src/org/commcare/android/database/connect/models/ConnectMessagingChannelRecord.java:0-0
Timestamp: 2025-04-22T17:04:26.780Z
Learning: In the Connect API contract, the server doesn't include the fields `created`, `answered_consent`, and `key` in JSON responses for messaging channels, which is why the `fromJson` method in `ConnectMessagingChannelRecord` explicitly initializes these values rather than parsing them.
Applied to files:
app/src/org/commcare/connect/network/connectId/parser/RetrieveChannelEncryptionKeyResponseParser.ktapp/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.javaapp/src/org/commcare/connect/network/ApiPersonalId.java
📚 Learning: 2025-01-21T17:28:09.007Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:66-71
Timestamp: 2025-01-21T17:28:09.007Z
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.
Applied to files:
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.javaapp/src/org/commcare/connect/MessageManager.java
📚 Learning: 2025-06-06T19:54:26.428Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java:74-78
Timestamp: 2025-06-06T19:54:26.428Z
Learning: In ConnectDownloadingFragment.java and similar Connect-related code, the team prefers to let "should never happen" scenarios like null app records crash rather than add defensive null checks, following a fail-fast philosophy to catch programming errors during development.
Applied to files:
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.javaapp/src/org/commcare/connect/MessageManager.java
📚 Learning: 2025-04-22T16:44:26.867Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3043
File: app/src/org/commcare/android/database/connect/models/ConnectMessagingChannelRecord.java:86-99
Timestamp: 2025-04-22T16:44:26.867Z
Learning: In ConnectMessagingChannelRecord, fields like META_CHANNEL_ID, META_CONSENT, META_CHANNEL_NAME, and META_KEY_URL are required fields, and the code is intentionally designed to crash if they're missing to raise awareness of data integrity issues rather than handling them silently.
Applied to files:
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.javaapp/src/org/commcare/connect/MessageManager.javaapp/src/org/commcare/connect/network/ApiPersonalId.java
📚 Learning: 2025-01-28T09:38:59.882Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2847
File: app/src/org/commcare/connect/network/connectId/ApiService.java:8-63
Timestamp: 2025-01-28T09:38:59.882Z
Learning: The ConnectID API service methods should use Map<String, String> for request bodies and ResponseBody for responses, as per team preference.
Applied to files:
app/src/org/commcare/connect/network/ApiService.java
📚 Learning: 2025-07-29T14:14:07.954Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/adapters/ConnectMessageAdapter.java:0-0
Timestamp: 2025-07-29T14:14:07.954Z
Learning: In the CommCare Android Connect messaging system (ConnectMessageAdapter.java), the team prefers to let getMessage() potentially return null and crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately during development.
Applied to files:
app/src/org/commcare/connect/MessageManager.java
📚 Learning: 2025-07-29T14:09:49.805Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java:0-0
Timestamp: 2025-07-29T14:09:49.805Z
Learning: In the CommCare Android Connect messaging system, message retry logic is implemented at the MessageManager level via MessageManager.sendUnsentMessages(). This method is called automatically when the channel list loads (ConnectMessageChannelListFragment), and it attempts to resend any outgoing messages that are not yet confirmed. Individual message fragments do not need to implement their own retry logic.
Applied to files:
app/src/org/commcare/connect/MessageManager.java
📚 Learning: 2025-02-19T15:15:01.935Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2956
File: app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java:58-60
Timestamp: 2025-02-19T15:15:01.935Z
Learning: Error handling for message retrieval in ConnectMessageChannelListFragment's retrieveMessages callback is not required as per user preference.
Applied to files:
app/src/org/commcare/connect/MessageManager.javaapp/src/org/commcare/connect/network/ApiPersonalId.java
📚 Learning: 2025-04-18T20:13:29.655Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3040
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java:39-55
Timestamp: 2025-04-18T20:13:29.655Z
Learning: In the CommCare Android Connect feature, the JSON object passed to `ConnectJobDeliveryFlagRecord.fromJson()` method should never be null based on the implementation design.
Applied to files:
app/src/org/commcare/connect/MessageManager.java
📚 Learning: 2025-06-06T19:52:53.173Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java:163-180
Timestamp: 2025-06-06T19:52:53.173Z
Learning: In the CommCare Android Connect feature, database operations like ConnectJobUtils.upsertJob should be allowed to crash rather than being wrapped in try-catch blocks. The team prefers fail-fast behavior for database errors instead of graceful error handling.
Applied to files:
app/src/org/commcare/connect/MessageManager.java
📚 Learning: 2025-01-21T18:19:05.799Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:214-215
Timestamp: 2025-01-21T18:19:05.799Z
Learning: In ConnectIdPasswordVerificationFragment, when creating a ConnectUserRecord, it's acceptable for payment information (paymentName and paymentPhone) to be empty strings if the server response doesn't include payment info in the CONNECT_PAYMENT_INFO field.
Applied to files:
app/src/org/commcare/connect/MessageManager.java
📚 Learning: 2025-04-22T17:05:39.842Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3043
File: app/src/org/commcare/android/database/connect/models/ConnectMessagingMessageRecord.java:0-0
Timestamp: 2025-04-22T17:05:39.842Z
Learning: In ConnectMessagingMessageRecord, decryption failures are expected in some scenarios and are handled by logging the exception with Logger.exception() but continuing execution by returning null, allowing the application to gracefully handle the failure.
Applied to files:
app/src/org/commcare/connect/MessageManager.javaapp/src/org/commcare/connect/network/ApiPersonalId.java
📚 Learning: 2025-02-04T19:06:14.568Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2927
File: app/AndroidManifest.xml:151-165
Timestamp: 2025-02-04T19:06:14.568Z
Learning: The HQUserInviteActivity should validate all deep link parameters (callback_url, hq_username, invite_code, hq_domain, connect_username) before processing the invitation. The callback URL should be restricted to trusted domains (commcare.dimagi.com).
Applied to files:
app/src/org/commcare/connect/network/ConnectSsoHelper.javaapp/src/org/commcare/connect/network/ApiPersonalId.java
🧬 Code graph analysis (4)
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (6)
app/src/org/commcare/connect/network/connectId/parser/LinkHqWorkerResponseParser.kt (1)
context(9-22)app/src/org/commcare/connect/network/connectId/parser/RetrieveChannelEncryptionKeyResponseParser.kt (1)
context(12-36)app/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParser.kt (1)
context(18-108)app/src/org/commcare/connect/network/connectId/parser/RetrieveWorkHistoryResponseParser.kt (1)
context(13-23)app/src/org/commcare/connect/network/ApiPersonalId.java (1)
ApiPersonalId(52-474)app/src/org/commcare/connect/network/base/BaseApiHandler.kt (1)
createCallback(113-138)
app/src/org/commcare/connect/network/ApiService.java (2)
app/src/org/commcare/connect/network/ApiEndPoints.java (1)
ApiEndPoints(3-28)app/unit-tests/src/org/commcare/connect/network/connectId/parser/ConnectTokenResponseParserTest.kt (1)
token(20-24)
app/src/org/commcare/connect/network/ConnectSsoHelper.java (4)
app/src/org/commcare/connect/network/base/BaseApiHandler.kt (1)
onFailure(19-22)app/src/org/commcare/connect/network/connectId/parser/LinkHqWorkerResponseParser.kt (1)
context(9-22)app/src/org/commcare/connect/network/connectId/parser/RetrieveHqTokenResponseParser.kt (1)
context(17-50)app/src/org/commcare/connect/network/ApiPersonalId.java (1)
ApiPersonalId(52-474)
app/src/org/commcare/connect/network/ApiPersonalId.java (7)
app/src/org/commcare/connect/network/connectId/parser/LinkHqWorkerResponseParser.kt (1)
context(9-22)app/src/org/commcare/connect/network/connectId/parser/RetrieveChannelEncryptionKeyResponseParser.kt (1)
context(12-36)app/src/org/commcare/connect/network/connectId/parser/RetrieveHqTokenResponseParser.kt (1)
context(17-50)app/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParser.kt (1)
context(18-108)app/src/org/commcare/connect/network/connectId/PersonalIdApiClient.java (1)
PersonalIdApiClient(10-30)app/src/org/commcare/connect/network/base/BaseApi.kt (2)
callApi(20-89)callApi(23-67)app/src/org/commcare/connect/network/ApiEndPoints.java (1)
ApiEndPoints(3-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (11)
app/src/org/commcare/connect/network/ConnectSsoHelper.java (1)
96-100:linkHqWorkeris fire-and-forget; failure won't block token retrieval.The call to
linkHqWorkeron line 99 does not wait for completion before proceeding to retrieve the HQ SSO token. If linking fails, the subsequent token retrieval may succeed but the user won't be marked as linked. Verify this is the intended behavior.app/src/org/commcare/connect/network/ApiService.java (1)
102-110: LGTM!The new
updateChannelConsentandsendMessagingMessagemethods follow the established patterns in this interface, using the team's preferred approach withMap<String, String>for headers andResponseBodyfor responses.app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (2)
332-346: LGTM!The
updateChannelConsentmethod follows the established patterns in this handler class and correctly delegates toApiPersonalId.updateChannelConsentwith appropriate parameters.
365-401: LGTM!The new methods
retrieveChannelEncryptionKey,linkHqWorker, andretrieveHqTokenASyncfollow consistent patterns with appropriate parser delegation. The integration with the new response parsers (RetrieveChannelEncryptionKeyResponseParser,LinkHqWorkerResponseParser,RetrieveHqTokenResponseParser) aligns with the broader refactoring in this PR.app/src/org/commcare/connect/MessageManager.java (2)
12-12: LGTM on import update.The import change from
ApiPersonalIdandIApiCallbacktoPersonalIdApiHandleraligns with the refactoring to the handler-based pattern.
32-52: LGTM onupdateChannelConsentrefactoring.The migration to
PersonalIdApiHandler<Boolean>is clean. The success path correctly stores the channel and conditionally fetches the encryption key based on consent status. Based on learnings, the behavior of fetching the encryption key even in some failure scenarios was intentional per server team guidance—this updated flow simplifies by only fetching on success, which seems like an intentional design change.app/src/org/commcare/connect/network/ApiPersonalId.java (5)
148-165: LGTM onlinkHqWorkerasync implementation.The refactoring to Retrofit-based calls with
makePostRequestis well-structured. TheObjects.requireNonNull(tokenAuth)on line 158 provides fail-fast behavior if credentials are invalid.
175-180: WrappingMalformedURLExceptionasRuntimeExceptionis acceptable here.Since
ServerUrls.getKeyServer()returns a configured URL, a malformed URL indicates a serious configuration error. Failing fast with a RuntimeException is appropriate.
381-396: LGTM onupdateChannelConsentmigration to Retrofit.The refactoring to use
ApiService.updateChannelConsentwith proper header construction and callback-based handling is clean and consistent with the other async methods.
417-441: LGTM onretrieveChannelEncryptionKeyRetrofit migration.The token callback pattern is preserved while the internal implementation now uses the Retrofit-based
makePostRequest. Token unavailability and denial are properly propagated via the callback interface.
443-473: LGTM onsendMessagingMessageRetrofit migration.The method correctly builds the encrypted message payload and delegates to the Retrofit-based
sendMessagingMessageendpoint. The error handling for encryption failures (lines 460-462) logs the exception appropriately.
| if (responseAsString.isNotEmpty()) { | ||
| val json = JSONObject(responseAsString) | ||
| channel.setKey(json.getString("key")) | ||
| ConnectMessagingDatabaseHelper.storeMessagingChannel(context, channel) | ||
| } | ||
| } catch (e: JSONException) { | ||
| throw RuntimeException(e) | ||
| } | ||
| return true as T |
There was a problem hiding this comment.
Empty response silently succeeds without updating the channel key.
When responseAsString.isEmpty(), the method returns true without setting the key or persisting the channel. This could mask a server issue returning an empty body. Consider whether this should be treated as an error.
Additionally, StreamsUtil.inputStreamToByteArray() can throw IOException which is not caught here, unlike JSONException.
🔎 Suggested improvement
+import java.io.IOException
+
class RetrieveChannelEncryptionKeyResponseParser<T>(
val context: Context,
) : BaseApiResponseParser<T> {
override fun parse(
responseCode: Int,
responseData: InputStream,
anyInputObject: Any?,
): T {
try {
val channel = anyInputObject as ConnectMessagingChannelRecord
val responseAsString =
String(
StreamsUtil.inputStreamToByteArray(responseData),
)
if (responseAsString.isNotEmpty()) {
val json = JSONObject(responseAsString)
channel.setKey(json.getString("key"))
ConnectMessagingDatabaseHelper.storeMessagingChannel(context, channel)
+ } else {
+ throw RuntimeException("Empty response received for channel encryption key")
}
- } catch (e: JSONException) {
+ } catch (e: Exception) {
+ when (e) {
+ is JSONException, is IOException -> throw RuntimeException(e)
+ else -> throw e
+ }
- throw RuntimeException(e)
}
return true as T
}
}🤖 Prompt for AI Agents
In
@app/src/org/commcare/connect/network/connectId/parser/RetrieveChannelEncryptionKeyResponseParser.kt
around lines 26 - 34, The parser currently treats an empty HTTP body as a
successful response and only catches JSONException; update retrieve logic in
RetrieveChannelEncryptionKeyResponseParser (the block that reads
responseAsString, parses JSONObject, calls channel.setKey(...) and
ConnectMessagingDatabaseHelper.storeMessagingChannel(...)) so that an empty
response is treated as an error (e.g., throw a RuntimeException or return a
failure rather than returning true) and ensure
StreamsUtil.inputStreamToByteArray() IOExceptions are caught and handled the
same way as the JSONException so missing/empty bodies or IO failures don’t
silently succeed.
There was a problem hiding this comment.
This seems worth logging (or even crashing) if encountered as it should never happen
There was a problem hiding this comment.
@OrangeAndGreen, agreed! Since an empty response represents a data contract violation (the server should always return a key field), it should fail-fast with a RuntimeException rather than silently succeeding.
Based on the established error-handling pattern in Connect code (per the learning from ConnectUnlockFragment), I'd suggest:
- Empty response: Throw
RuntimeExceptionwith a descriptive message (data contract violation) - JSONException: Continue throwing
RuntimeException(already does this - data contract violation) - IOException from
StreamsUtil.inputStreamToByteArray(): Log the error and either return a failure indicator or throw, depending on whether you consider stream reading failures as recoverable
Would you like me to plan the code changes for this fix? If so, should IOException be handled gracefully (logged but not crashed), or should it also crash since it prevents us from reading the encryption key?
🧠 Learnings used
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 3455
File: app/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParser.kt:64-93
Timestamp: 2025-12-08T12:01:48.545Z
Learning: In the CommCare Android Connect notification system (RetrieveNotificationsResponseParser.kt), messages are only decrypted using channels that already exist in the database with their encryption keys. When new channels arrive without keys, a separate network flow fetches the channel keys, and then the notification API is re-called to decrypt any messages for those channels. This explains why parseAndSeparateNotifications() correctly uses existingChannels from the database rather than newly parsed channels.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/connect/MessageManager.java:0-0
Timestamp: 2025-07-29T14:45:13.470Z
Learning: In the CommCare Android Connect messaging system (MessageManager.java), the error handling logic in updateChannelConsent's processFailure callback intentionally attempts to retrieve the channel encryption key even after consent update failure. This behavior was recommended by the server team and the worst case scenario is silently failing to retrieve the encryption key, which is considered acceptable.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:206-213
Timestamp: 2025-01-21T18:20:43.883Z
Learning: In the Connect ID feature, server responses containing user data should be parsed using ConnectUserResponseParser to ensure consistent handling of payment information (owner_name and phone_number) and secondary phone verification across all authentication flows.
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2949
File: app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java:244-275
Timestamp: 2025-02-04T21:22:56.537Z
Learning: Direct JSONObject parsing is acceptable for handling user data responses in ConnectIdPinFragment, as decided by the team. No need to enforce ConnectUserResponseParser usage in this context.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3043
File: app/src/org/commcare/android/database/connect/models/ConnectMessagingChannelRecord.java:0-0
Timestamp: 2025-04-22T17:04:26.780Z
Learning: In the Connect API contract, the server doesn't include the fields `created`, `answered_consent`, and `key` in JSON responses for messaging channels, which is why the `fromJson` method in `ConnectMessagingChannelRecord` explicitly initializes these values rather than parsing them from JSON.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3043
File: app/src/org/commcare/android/database/connect/models/ConnectMessagingChannelRecord.java:0-0
Timestamp: 2025-04-22T17:04:26.780Z
Learning: In the Connect API contract, the server doesn't include the fields `created`, `answered_consent`, and `key` in JSON responses for messaging channels, which is why the `fromJson` method in `ConnectMessagingChannelRecord` explicitly initializes these values rather than parsing them.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3043
File: app/src/org/commcare/android/database/connect/models/ConnectMessagingChannelRecord.java:86-99
Timestamp: 2025-04-22T16:44:26.867Z
Learning: In ConnectMessagingChannelRecord, fields like META_CHANNEL_ID, META_CONSENT, META_CHANNEL_NAME, and META_KEY_URL are required fields, and the code is intentionally designed to crash if they're missing to raise awareness of data integrity issues rather than handling them silently.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:0-0
Timestamp: 2025-07-29T14:08:55.466Z
Learning: In ConnectUnlockFragment.java and similar Connect-related code, the team has agreed that JSONExceptions should crash the app by throwing RuntimeException (fail-fast approach for data contract violations), while IOExceptions should be logged and allow processing to continue (graceful handling for network/stream issues). This intentional inconsistency in error handling reflects different treatment for different types of failures.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3043
File: app/src/org/commcare/android/database/connect/models/ConnectMessagingMessageRecord.java:0-0
Timestamp: 2025-04-22T17:05:39.842Z
Learning: In ConnectMessagingMessageRecord, decryption failures are expected in some scenarios and are handled by logging the exception with Logger.exception() but continuing execution by returning null, allowing the application to gracefully handle the failure.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/activities/StandardHomeActivityUIController.java:0-0
Timestamp: 2025-06-20T13:21:20.908Z
Learning: User OrangeAndGreen prefers to handle code issues in separate PRs rather than immediately fixing them in the current PR when they are not directly related to the main changes.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/connect/network/ApiPersonalId.java:0-0
Timestamp: 2025-07-29T14:56:40.681Z
Learning: User OrangeAndGreen prefers to defer ApiPersonalId refactoring (specifically InputStream resource management improvements) to separate PRs rather than mixing them with Connect feature work, even when the code is already in master.
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T13:40:19.645Z
Learning: PR #3048 introduces a comprehensive messaging system in the Connect feature, implementing secure encryption using AES-GCM for message content, proper channel management with consent flows, and a well-designed UI separation between sent and received messages with real-time notification integration.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/connect/MessageManager.java:0-0
Timestamp: 2025-07-29T14:54:47.940Z
Learning: User OrangeAndGreen organizes messaging-related changes into separate PRs rather than mixing them with other Connect work, which aligns with their preference for handling code issues in separate PRs when they are not directly related to the main changes.
| val seconds = if (json.has(key)) json.getInt(key) else 0 | ||
| expiration.setTime(expiration.getTime() + (seconds.toLong() * 1000)) | ||
|
|
||
| val seatedAppId = CommCareApplication.instance().getCurrentApp().getUniqueId() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check how getCurrentApp() is used in the codebase
rg -n "getCurrentApp\(\)" --type kotlin --type java -C2 | head -100Repository: dimagi/commcare-android
Length of output: 11601
🏁 Script executed:
#!/bin/bash
# Find the CommCareApplication class to check getCurrentApp() signature
fd -t f "CommCareApplication\." --type f | head -10
rg -n "fun getCurrentApp" --type kotlin --type java -B2 -A5Repository: dimagi/commcare-android
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Check the specific file mentioned in the review
cat -n app/src/org/commcare/connect/network/connectId/parser/RetrieveHqTokenResponseParser.kt | sed -n '35,50p'Repository: dimagi/commcare-android
Length of output: 819
🏁 Script executed:
#!/bin/bash
# Find getCurrentApp() method definition
rg -n "fun getCurrentApp|public.*getCurrentApp" --type kotlin --type java -A3Repository: dimagi/commcare-android
Length of output: 1389
🏁 Script executed:
#!/bin/bash
# Also check CommCareApplication.java for getCurrentApp()
rg -n "getCurrentApp" app/src/org/commcare/CommCareApplication.java -B2 -A5Repository: dimagi/commcare-android
Length of output: 4113
Add null check before calling getUniqueId().
CommCareApplication.instance().getCurrentApp() can return null (see defensive null checks in TestUtils), but line 41 calls .getUniqueId() directly without a null check, risking an NPE if no app is currently seated.
Example fix
val currentApp = CommCareApplication.instance().getCurrentApp()
if (currentApp != null) {
val seatedAppId = currentApp.getUniqueId()
val ssoToken = SsoToken(token, expiration)
ConnectDatabaseHelper.storeHqToken(context, seatedAppId, hqUsername, ssoToken)
} else {
// Handle case when no app is seated
}🤖 Prompt for AI Agents
In
@app/src/org/commcare/connect/network/connectId/parser/RetrieveHqTokenResponseParser.kt
at line 41, CommCareApplication.instance().getCurrentApp() can be null, so
before calling getUniqueId() in RetrieveHqTokenResponseParser (where you create
seatedAppId and then call SsoToken and ConnectDatabaseHelper.storeHqToken),
fetch currentApp into a local variable, check it for null, and only call
currentApp.getUniqueId() and store the token when non-null; in the else branch
handle the no-app-seated case (e.g., log an error via the parser's logger and
return/skip storing) so you avoid an NPE.
| public void sendMessagingMessage( | ||
| Context context, | ||
| ConnectUserRecord user, | ||
| ConnectMessagingMessageRecord messsage, | ||
| ConnectMessagingChannelRecord channel | ||
| ) { | ||
| ApiPersonalId.sendMessagingMessage( | ||
| context, | ||
| user.getUserId(), | ||
| user.getPassword(), | ||
| messsage, | ||
| channel.getKey(), | ||
| createCallback(new NoParsingResponseParser<>(), null) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Typo in parameter name: messsage should be message.
🔎 Suggested fix
public void sendMessagingMessage(
Context context,
ConnectUserRecord user,
- ConnectMessagingMessageRecord messsage,
+ ConnectMessagingMessageRecord message,
ConnectMessagingChannelRecord channel
) {
ApiPersonalId.sendMessagingMessage(
context,
user.getUserId(),
user.getPassword(),
- messsage,
+ message,
channel.getKey(),
createCallback(new NoParsingResponseParser<>(), null)
);
}📝 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.
| public void sendMessagingMessage( | |
| Context context, | |
| ConnectUserRecord user, | |
| ConnectMessagingMessageRecord messsage, | |
| ConnectMessagingChannelRecord channel | |
| ) { | |
| ApiPersonalId.sendMessagingMessage( | |
| context, | |
| user.getUserId(), | |
| user.getPassword(), | |
| messsage, | |
| channel.getKey(), | |
| createCallback(new NoParsingResponseParser<>(), null) | |
| ); | |
| } | |
| public void sendMessagingMessage( | |
| Context context, | |
| ConnectUserRecord user, | |
| ConnectMessagingMessageRecord message, | |
| ConnectMessagingChannelRecord channel | |
| ) { | |
| ApiPersonalId.sendMessagingMessage( | |
| context, | |
| user.getUserId(), | |
| user.getPassword(), | |
| message, | |
| channel.getKey(), | |
| createCallback(new NoParsingResponseParser<>(), null) | |
| ); | |
| } |
🤖 Prompt for AI Agents
In @app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java
around lines 348 - 362, Rename the misspelled parameter `messsage` to `message`
in the method signature of sendMessagingMessage and update all references inside
the method body (e.g., the argument passed to
ApiPersonalId.sendMessagingMessage) to use the corrected name; ensure the
parameter type remains ConnectMessagingMessageRecord and no other behavior
changes are introduced.
| public void onFailure(@NonNull PersonalIdOrConnectApiErrorCodes errorCode, @Nullable Throwable t) { | ||
| if(t!=null){ | ||
| Logger.exception("Failed to link HQ workder ", t); | ||
| }else{ | ||
| Logger.exception("Failed to link HQ workder", new Throwable("Failed to link HQ workder")); | ||
| } |
There was a problem hiding this comment.
Typo in log messages: "workder" should be "worker".
🔎 Suggested fix
@Override
public void onFailure(@NonNull PersonalIdOrConnectApiErrorCodes errorCode, @Nullable Throwable t) {
if(t!=null){
- Logger.exception("Failed to link HQ workder ", t);
+ Logger.exception("Failed to link HQ worker", t);
}else{
- Logger.exception("Failed to link HQ workder", new Throwable("Failed to link HQ workder"));
+ Logger.exception("Failed to link HQ worker", new Throwable("Failed to link HQ worker"));
}
}📝 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.
| public void onFailure(@NonNull PersonalIdOrConnectApiErrorCodes errorCode, @Nullable Throwable t) { | |
| if(t!=null){ | |
| Logger.exception("Failed to link HQ workder ", t); | |
| }else{ | |
| Logger.exception("Failed to link HQ workder", new Throwable("Failed to link HQ workder")); | |
| } | |
| public void onFailure(@NonNull PersonalIdOrConnectApiErrorCodes errorCode, @Nullable Throwable t) { | |
| if(t!=null){ | |
| Logger.exception("Failed to link HQ worker", t); | |
| }else{ | |
| Logger.exception("Failed to link HQ worker", new Throwable("Failed to link HQ worker")); | |
| } |
🤖 Prompt for AI Agents
In @app/src/org/commcare/connect/network/ConnectSsoHelper.java around lines 261
- 266, Fix the typo "workder" to "worker" in the failure logging inside the
onFailure callback in ConnectSsoHelper: update both Logger.exception calls (the
one logging the Throwable t and the one constructing a new Throwable) so the
message reads "Failed to link HQ worker" and keep existing Throwable usage and
context intact.
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
Looking good, just had minor feedback. Think we should also at least address the typos that Coderabbit pointed out
app/src/org/commcare/connect/network/connectId/parser/RetrieveHqTokenResponseParser.kt
Outdated
Show resolved
Hide resolved
|
|
||
| public static void retrieveHqTokenASync(Context context, String hqUsername, String connectToken,IApiCallback callback) { | ||
| HashMap<String, Object> params = new HashMap<>(); | ||
| params.put("client_id", "4eHlQad1oasGZF0lPiycZIjyL0SY1zx7ZblA6SCV"); |
There was a problem hiding this comment.
Can we move the client ID to a constant variable
There was a problem hiding this comment.
Also, just to confirm, should this ID always be the same?
There was a problem hiding this comment.
This is used in this HQ token API only, so not putting in constant.
There was a problem hiding this comment.
Understandable, I was thinking that having it as a constant like CONNECT_CLIENT_ID currently is at the top of this class would make for easy visibility. But I'm fine with this being optional
|
Also, should we include unit tests with these new parser classes? @shubham1g5 As a broader discussion, should the team be required to write new unit tests when applicable for new changes? Can't remember if we had already discussed this. I know I've personally been slacking a bit when it comes to this |
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
The formatting on new code is off at several places and lots of un-needed spacing between lines which should be corrected before merge.
| // This is created if needs to be used in future like above post method | ||
| @GET | ||
| Call<ResponseBody> makeGetRequest( |
There was a problem hiding this comment.
we should not be creating any unused methods that we think might get used in future, in general PRs should not have any unused code unless you soon plan to do another PR as a followup to this using the corresponding block of code.
| HashMap<String, String> headers = new HashMap<>(); | ||
| String token = HttpUtils.getCredential(tokenAuth); | ||
| RequestBody requestBody = ConnectNetworkHelper.buildPostFormHeaders(params, true, PersonalIdApiClient.API_VERSION, headers); | ||
| ApiService apiService = PersonalIdApiClient.getClientApi(); | ||
| Call<ResponseBody> call = apiService.makePostRequest(channelUrl,token, headers,requestBody); |
There was a problem hiding this comment.
looks like we are repating this code at various places in this PR and should be abstracted.
There was a problem hiding this comment.
Yeah, as AuthInfo is different for some URLs, I kept separate in their corresponding methods only.
There was a problem hiding this comment.
as AuthInfo is different for some URLs
Would you be able to share more on this ? I am not sure I get why that will be an issue here given it has a common superclass.
There was a problem hiding this comment.
@shubham1g5 Sorry, I initially mis-understand it, changed now here.
| } | ||
| @Override | ||
| public void onFailure(PersonalIdOrConnectApiErrorCodes failureCode, Throwable t) { | ||
| Logger.log("Messaging", "Failed to retreive encryption key: " + (t!=null ? t.getMessage():"")); |
There was a problem hiding this comment.
typo as flagged by code rabbit below
| ConnectMessagingChannelRecord channel = ConnectMessagingDatabaseHelper.getMessagingChannel(context, message.getChannelId()); | ||
|
|
||
| if (channel.getKey().length() > 0) { | ||
| if (channel!=null && !channel.getKey().isEmpty()) { |
There was a problem hiding this comment.
how can channel be null here ?
| if (linkHqUser || appRecord.getWorkerLinked()) { | ||
|
|
||
| // first get the connect token | ||
| new PersonalIdApiHandler<AuthInfo.TokenAuth>() { |
There was a problem hiding this comment.
can we abstract this into another method, the method seems to run super long already with the top level network call.
| public void onSuccess(AuthInfo.TokenAuth hqToken) { | ||
| callback.tokenRetrieved(hqToken); | ||
| } | ||
| }.retrieveHqTokenASync(context, hqUsername,connectIdToken.bearerToken); |
There was a problem hiding this comment.
is there a reason we are adding ASync suffix here when we are not doing so with other similar network methods in PersonalIdApiHandler. I am confused is there is something different about these Async methods in comparison to other ApiPersonalId calls
There was a problem hiding this comment.
No different than other ApiPersonalId calls so removed here
|
|
||
|
|
||
| } | ||
| }.connectToken(context, user); |
There was a problem hiding this comment.
nit: can rename to retrievePersonalIdToken to be consistent with underlying ApiPersonalId call
| if(hqTokenAuth != null) { | ||
| return hqTokenAuth; | ||
| } | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { |
There was a problem hiding this comment.
we already require Android N for signing up for Personal ID, as such think it's safe to remove these checks and delete anything connect/personal ID related that is there to support devices < Android N
There was a problem hiding this comment.
@shubham1g5 This is really excellent point. I will remove all such checking for Android N. Thanks for flagging this.
@conroy-ricketts Good callout here, I don't think we have concrete expectations there yet, think I would expect tests for components that already have tests like parsers for example although think we should be calling this out in Jira tickets explicitly as these tests will tend to increase scope of the tickets. I don't think we did it for this ticket, so would leave it to @Jignesh-dimagi based on the capacity he have in the Sprint to tackle tests on it, otherwise think we should create a separate ticket for the new parsers that we introduced in this PR. |
@shubham1g5 @conroy-ricketts I was also with view that parsers should have unit tests and was going to implement the same but this time my sprint is not having good bandwidth to attempt it as this ticket has taken some extra time to clean up so lean towards separate ticket for the same if it's okay to you guys. |
There was a problem hiding this comment.
I noticed that the JIRA ticket mentioned moving the few remaining helper methods out of ConnectNetworkHelper making it effectively obsolete (and I assume we should be ale to delete this class entirely). Is this something we still want to do here?
There was a problem hiding this comment.
ConnectNetworkHelper's network call methods are obsolete and removed, now it has only abstract methods to build the request.
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Can we specify Safety Story for PR based on dev testing you have done for the PR. think it's critical for us to test the major SSO related flows before merging this in, specially things like "Syncing forms inside the HQ app with a Personal ID login"
| } | ||
|
|
||
|
|
||
| public static void retrieveConnectIdTokenAsync(Context context, @NonNull ConnectUserRecord user, TokenCallback callback) { |
There was a problem hiding this comment.
should rename to retrievePersonalIdToken as well
I have already added points in |
Yes we will need QA to verify as mentioned in description that with my account I am getting the same error with master branch also. |
Yes it's working from my side. |
Are you able to dig into why that's happening, we are regularly deploying test betas from |
Ok sure. Should I create a separate ticket for the same as this is the same without this PR changes also. |
I don't think there is a platform bug here, and it might be related to just your account/dev setup. Are you able to spend a couple hours looking into it so that we have more info on this before creating a ticket on to why it's happening. |
Yeah, It tried debugging before raising this PR but as server is responding with 500, no more other information. I guess my account is already linked so giving this error and that's why I was proposing QA here to find out more. |
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Might be worth running the code formatter to address spacing nits. Otherwise just one noteworthy concern about async behavior when linking PersonalID-HQ
| if (responseAsString.isNotEmpty()) { | ||
| val json = JSONObject(responseAsString) | ||
| channel.setKey(json.getString("key")) | ||
| ConnectMessagingDatabaseHelper.storeMessagingChannel(context, channel) | ||
| } | ||
| } catch (e: JSONException) { | ||
| throw RuntimeException(e) | ||
| } | ||
| return true as T |
There was a problem hiding this comment.
This seems worth logging (or even crashing) if encountered as it should never happen
| } | ||
|
|
||
| public static void linkHqWorker(Context context, String hqUsername, ConnectLinkedAppRecord appRecord, String connectToken) { | ||
| public static void linkHqWorker(Context context, String hqUsername, ConnectLinkedAppRecord appRecord, String connectToken,IApiCallback callback) { |
There was a problem hiding this comment.
Nit: Missing space "connectToken,IApiCallback"
|
|
||
| public static AuthInfo.TokenAuth retrieveHqTokenSync(Context context, String hqUsername, String connectToken) | ||
| throws TokenUnavailableException { | ||
| public static void retrieveHqTokenASync(Context context, String hqUsername, String connectToken,IApiCallback callback) { |
There was a problem hiding this comment.
We can get rid of the "ASync" in function name
|
|
||
| public static AuthInfo.TokenAuth retrieveHqTokenSync(Context context, String hqUsername, String connectToken) | ||
| throws TokenUnavailableException { | ||
| public static void retrieveHqTokenASync(Context context, String hqUsername, String connectToken,IApiCallback callback) { |
There was a problem hiding this comment.
Nit: Missing space "connectToken,IApiCallback"
There was a problem hiding this comment.
This can be deleted now, no longer used
| IApiCallback callback){ | ||
| RequestBody requestBody = ConnectNetworkHelper.buildPostFormHeaders(params, useFormEncoding, PersonalIdApiClient.API_VERSION, headers); | ||
| ApiService apiService = PersonalIdApiClient.getClientApi(); | ||
| Call<ResponseBody> call = apiService.makePostRequest(channelUrl,token, headers,requestBody); |
| RequestBody requestBody = ConnectNetworkHelper.buildPostFormHeaders(params, false, PersonalIdApiClient.API_VERSION, headers); | ||
| ApiService apiService = PersonalIdApiClient.getClientApi(); | ||
| Call<ResponseBody> call = apiService.sendMessagingMessage(tokenAuth, headers,requestBody); | ||
| BaseApi.Companion.callApi(context, call, callback,ApiEndPoints.CONNECT_MESSAGE_SEND_URL); |
| } | ||
| @Override | ||
| public void onSuccess(AuthInfo.TokenAuth connectIdToken) { | ||
| linkHqUser(context,appRecord,hqUsername,connectIdToken); |
There was a problem hiding this comment.
I think this may not work correctly since linkHqWorker is asynchronous and retrieveHqToken requires the link to be configured in order to succeed (we shouldn't try to retrieve a token until performing the link succeeds)
There was a problem hiding this comment.
@OrangeAndGreen Yeah thats good catch. Thanks.
Previous code was not checking if linking HQ worker was successful or not. So with this new change, we should try to retrieve token irrespective of linking HQ worker success/failure as server might return error if already linked previously, your thoughts?
There was a problem hiding this comment.
Hmm, that's a good point. I think the behavior you described (server returning an error if user is already linked) used to be the case but that was changed at some point. Now the server should simply succeed the call (without making any changes) when the user is already linked.
But if we want to be safe and maintain the existing behavior (trying to proceed to token retrieval even when the linking call fails) that should be fine. If the linking call fails for good reason the subsequent call to retrieve token will fail anyway, so the end result for the user is the same.
The important thing is that we give the linking call a chance to succeed before we make the token call.
| if (!appRecord.getWorkerLinked()) { | ||
| //Link user if necessary | ||
| ApiPersonalId.linkHqWorker(context, hqUsername, appRecord, connectIdToken.bearerToken); | ||
| @RequiresApi(api = Build.VERSION_CODES.N) |
There was a problem hiding this comment.
Noting that this API requirement creates a compiler warning/error in PersonalIdManager.getHqTokenIfLinked
There was a problem hiding this comment.
Yeah, it was assumed that PersonalId / Connect is restricted to Android N only but I have changed now to remove that that OS specific requirements. Done here
| //Link user if necessary | ||
| ApiPersonalId.linkHqWorker(context, hqUsername, appRecord, connectIdToken.bearerToken); | ||
| @RequiresApi(api = Build.VERSION_CODES.N) | ||
| public static AuthInfo.TokenAuth retrieveHqSsoTokenSync(Context context, @NonNull ConnectUserRecord user, @NonNull ConnectLinkedAppRecord appRecord, String hqUsername, boolean performLink) throws |
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Left a couple questions to better understand the exception handling.
Also heads up that it looks like the formatter should be run one more time, at least for ConnectSsoHelper
| } | ||
|
|
||
| override fun tokenUnavailable() { | ||
| // Cannot raise exception as not handled here |
There was a problem hiding this comment.
It looks like the exception is expected to rise through CommCareRequestGenerator.buildAuth (note that method declares that it throws the two token exceptions). So it seems the relevant exceptions should be thrown from here?
There was a problem hiding this comment.
I was thinking of not raising the exception but handling token = null here. But as this CommCareRequestGenerator.buildAuth is also raising these exceptions, I agree with your point and added back the exception again.
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
LGTM pending Dave's latest comments
| appRecord: ConnectLinkedAppRecord, | ||
| hqUsername: String?, | ||
| performLink: Boolean, | ||
| ): AuthInfo.TokenAuth? = |
There was a problem hiding this comment.
Nit: "AuthInfo." not necessary here, TokenAuth is imported directly
| appRecord: ConnectLinkedAppRecord, | ||
| hqUsername: String?, | ||
| performLink: Boolean, | ||
| ): AuthInfo.TokenAuth? = |
There was a problem hiding this comment.
Nit: "AuthInfo." not necessary here, TokenAuth is imported directly
|
@shubham1g5 It looks like the review is done. As you have offered help here for testing, can you please do that? @OrangeAndGreen @conroy-ricketts I have already done one round from my side, as mentioned in the ticket description, but if anyone has bandwidth to just test it once, as it's a very crucial part change |
@Jignesh-dimagi Yeah sure thing, I will try as soon as I can (hopefully later today) and let you know |
|
@Jignesh-dimagi Just noticed there are some conflicts to resolve here. |
|
@Jignesh-dimagi I was able to test the SSO auth with HQ here and submit a form successfully with Personal ID login. |
Thanks @shubham1g5 for testing |
Jignesh-dimagi
dismissed stale reviews from OrangeAndGreen and conroy-ricketts
via
6b4e6be
@OrangeAndGreen updated the base branch, thanks |
Product Description
There are no visible changes in this PR
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-1888
API refactored to convert for async using retrofit
Connect Messaging APIs:
OAuth token calls:
Other API:
Safety Story
QA Plan
QA should test following:
Labels and Review