Notifications Parser refactor and tests #3455
Conversation
Parser → Service → API Handler flow Parser -> purely focused on parsing JSON data NotificationService -> Takes the parsed models and store into DB and supply modified result to the Api handler PushNotificationApiHelper -> Post hooks for UI logic + acknowledgment to server
shubham1g5
requested review from
Jignesh-dimagi,
conroy-ricketts and
orangejenny
📝 WalkthroughWalkthroughThis PR refactors the notification parsing and processing workflow into a multi-stage pipeline. It introduces a new factory method on ConnectMessagingMessageRecord for JSON decryption, changes PushNotificationRecord's parsing from batch to single-object, and restructures RetrieveNotificationsResponseParser from a generic parser to a concrete parser that returns NotificationParseResult. A new NotificationService processes parsed data into the database, separating messaging and push notifications into distinct paths. PersonalIdApiHandler, PushNotificationApiHelper, and related components are updated to use the new ProcessedNotificationResult container, replacing direct list handling with structured result objects. Sequence DiagramsequenceDiagram
participant API as PersonalIdApiHandler
participant Parser as RetrieveNotificationsResponseParser
participant Service as NotificationService
participant DB as Database
participant Result as ProcessedNotificationResult
API->>Parser: parse(responseStream)
Parser->>Parser: parseChannels()
Parser->>Parser: parseAndSeparateNotifications()
Note over Parser: Classify notifications into<br/>Push and Messaging types
Parser-->>API: NotificationParseResult
API->>Service: processNotificationData(context, parseResult)
Service->>Service: Filter notifications by type
Service->>DB: Save messaging channels
Service->>DB: Save messaging messages
Service->>DB: Save push notifications
Service-->>Result: ProcessedNotificationResult<br/>(savedNotifications,<br/>messagingIds,<br/>savedIds)
API->>API: Handle processed results<br/>and broadcast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 3
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/android/database/connect/models/ConnectMessagingMessageRecord.java (1)
90-91: Potential NPE ifDateUtils.parseDateTimereturns null.Similar to
PushNotificationRecord,DateUtils.parseDateTimecan returnnullon parse failure, which would settimeStamptonull. Consider adding a null check or default:String dateString = json.getString(META_MESSAGE_TIMESTAMP); -connectMessagingMessageRecord.timeStamp = DateUtils.parseDateTime(dateString); +Date parsedDate = DateUtils.parseDateTime(dateString); +if (parsedDate == null) { + return null; +} +connectMessagingMessageRecord.timeStamp = parsedDate;
🧹 Nitpick comments (3)
app/unit-tests/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParserTest.kt (1)
212-242: Consider adding a test for successful message decryption.The current test at lines 223-226 mocks
getMessagingChannelsto return an empty list, so messaging notifications are never decrypted (as noted in the comment at line 233). Consider adding a test that provides a channel with a valid encryption key to verify the message decryption path works correctly.app/src/org/commcare/connect/network/connectId/parser/NotificationParseResult.kt (1)
7-10: Clarify "exclusive" in the documentation.The term "exclusive" suggests the three entity types are mutually exclusive (only one can be present at a time), but the parser can populate all three lists simultaneously from a single response. Consider rewording to "separate" or "distinct" to avoid confusion.
Apply this diff to clarify the documentation:
/** * Data class to hold the result of parsing notification response - * Contains three separate, exclusive entities: notifications, channels, and messages + * Contains three separate entities: notifications, channels, and messages */app/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParser.kt (1)
45-56: Consider more idiomatic Kotlin.The channel parsing logic is correct but could be simplified using Kotlin's collection operations for better readability.
Apply this diff for a more idiomatic approach:
- private fun parseChannels(responseJsonObject: JSONObject): MutableList<ConnectMessagingChannelRecord> { - val channels: MutableList<ConnectMessagingChannelRecord> = ArrayList() + private fun parseChannels(responseJsonObject: JSONObject): List<ConnectMessagingChannelRecord> { if (responseJsonObject.has("channels")) { val channelsJson: JSONArray = responseJsonObject.getJSONArray("channels") - for (i in 0 until channelsJson.length()) { - val obj = channelsJson.get(i) as JSONObject - val channel = ConnectMessagingChannelRecord.fromJson(obj) - channels.add(channel) - } + return (0 until channelsJson.length()) + .map { channelsJson.getJSONObject(it) } + .map { ConnectMessagingChannelRecord.fromJson(it) } } - return channels + return emptyList() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/src/org/commcare/android/database/connect/models/ConnectMessagingMessageRecord.java(2 hunks)app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt(2 hunks)app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java(2 hunks)app/src/org/commcare/connect/network/connectId/parser/NotificationParseResult.kt(1 hunks)app/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParser.kt(1 hunks)app/src/org/commcare/connect/services/NotificationService.kt(1 hunks)app/src/org/commcare/connect/services/ProcessedNotificationResult.kt(1 hunks)app/src/org/commcare/utils/PushNotificationApiHelper.kt(2 hunks)app/unit-tests/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParserTest.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 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: 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: 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.
📚 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/RetrieveNotificationsResponseParser.ktapp/src/org/commcare/connect/network/connectId/parser/NotificationParseResult.ktapp/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java
📚 Learning: 2025-02-04T21:22:56.537Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2949
File: app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java:244-275
Timestamp: 2025-02-04T21:22:56.537Z
Learning: Direct JSONObject parsing is acceptable for handling user data responses in ConnectIdPinFragment, as decided by the team. No need to enforce ConnectUserResponseParser usage in this context.
Applied to files:
app/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParser.ktapp/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java
📚 Learning: 2025-11-06T11:44:23.934Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 3407
File: app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt:92-107
Timestamp: 2025-11-06T11:44:23.934Z
Learning: In PushNotificationRecord, the `acknowledged` field (META_ACKNOWLEDGED) is a client-side field that is not included in the server's JSON response, so it should not be read in the fromJsonArray method.
Applied to files:
app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt
📚 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/android/database/connect/models/PushNotificationRecord.ktapp/src/org/commcare/android/database/connect/models/ConnectMessagingMessageRecord.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/android/database/connect/models/ConnectMessagingMessageRecord.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/android/database/connect/models/ConnectMessagingMessageRecord.java
📚 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/android/database/connect/models/ConnectMessagingMessageRecord.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/android/database/connect/models/ConnectMessagingMessageRecord.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/android/database/connect/models/ConnectMessagingMessageRecord.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/android/database/connect/models/ConnectMessagingMessageRecord.javaapp/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.javaapp/src/org/commcare/utils/PushNotificationApiHelper.kt
📚 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/android/database/connect/models/ConnectMessagingMessageRecord.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/android/database/connect/models/ConnectMessagingMessageRecord.java
📚 Learning: 2025-01-27T09:08:32.722Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2847
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecord.java:86-93
Timestamp: 2025-01-27T09:08:32.722Z
Learning: The JSON fields (status, unitName, slug, entityId, entityName, reason) in ConnectJobDeliveryRecord's fromJson method are guaranteed to be non-null through the application's data contract, except for the 'id' field which is explicitly checked.
Applied to files:
app/src/org/commcare/android/database/connect/models/ConnectMessagingMessageRecord.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/network/connectId/PersonalIdApiHandler.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (6)
app/src/org/commcare/connect/services/ProcessedNotificationResult.kt (1)
8-12: LGTM!Clean immutable data class with clear separation between saved notifications and their IDs. Good documentation.
app/unit-tests/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParserTest.kt (1)
31-42: LGTM!Comprehensive test suite with good coverage of parsing scenarios including empty arrays, push notifications, channels, complete responses with all types, optional fields, and error conditions.
app/src/org/commcare/utils/PushNotificationApiHelper.kt (1)
68-106: LGTM!Clean refactor to use
ProcessedNotificationResult. The logic correctly:
- Sets notification as unread only when there are saved notification IDs
- Broadcasts when there are either saved notifications or messaging notifications
- Acknowledges both saved and messaging notification IDs
- Returns only non-messaging notifications for backward compatibility
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (1)
313-330: The review comment is incorrect. ThestopLoadingAndInformErrormethod inBaseApiHandler.kt(line 156) callsonStop()internally before invokingonFailure(), so the error path properly terminates the loading state. No issue exists here.Likely an incorrect or invalid review comment.
app/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParser.kt (2)
17-43: LGTM! Clean refactoring of the parser flow.The new structure clearly separates channel parsing, notification parsing, and result construction. The concrete return type (NotificationParseResult) improves type safety and aligns well with the refactor objectives.
98-101: LGTM!The notification type detection logic is clear and correct.
| val dateString: String = obj.getString(META_TIME_STAMP) | ||
| createdDate = DateUtils.parseDateTime(dateString) |
There was a problem hiding this comment.
Potential NPE if DateUtils.parseDateTime returns null.
DateUtils.parseDateTime(dateString) can return null if the date string cannot be parsed. Assigning null to createdDate (which is non-null Date) will cause issues. Consider adding null-safety:
val dateString: String = obj.getString(META_TIME_STAMP)
-createdDate = DateUtils.parseDateTime(dateString)
+createdDate = DateUtils.parseDateTime(dateString) ?: Date()📝 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.
| val dateString: String = obj.getString(META_TIME_STAMP) | |
| createdDate = DateUtils.parseDateTime(dateString) | |
| val dateString: String = obj.getString(META_TIME_STAMP) | |
| createdDate = DateUtils.parseDateTime(dateString) ?: Date() |
🤖 Prompt for AI Agents
In
app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt
around lines 96-97, createdDate is assigned directly from
DateUtils.parseDateTime(dateString) which can return null; update the code to
safely handle a null return by calling parseDateTime into a nullable variable,
then either (a) provide a non-null fallback (e.g., Date() or Date(0)) and log a
warning, or (b) throw/propagate a descriptive exception if a missing date should
be considered fatal; ensure createdDate remains non-null after this check and
that any logging uses the original dateString for context.
app/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParser.kt
Outdated
Show resolved
Hide resolved
shubham1g5
requested review from
OrangeAndGreen
and removed request for
orangejenny
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
Looks great to me! I'd like another set of eyes on this before we merge though
...src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParserTest.kt
Outdated
Show resolved
Hide resolved
| val nonMessagingNotifications: List<PushNotificationRecord>, | ||
| val channels: List<ConnectMessagingChannelRecord>, | ||
| val messages: List<ConnectMessagingMessageRecord>, | ||
| val messagingNotificationIds: List<String> |
There was a problem hiding this comment.
@shubham1g5 We can follow the same procedure to get the list of messagingNotificationIds as done for nonMessagingNotifications. We don't have to pass the messagingNotificationIds here but in fact this and this should return the list of message Ids which are store successfully. So this way its consistent with both non message and message local DB storing
There was a problem hiding this comment.
@Jignesh-dimagi messagingNotificationIds are not the message ids here but the ids of the notifications associated with the notifications which is what we need to acknowledge to the server.
There was a problem hiding this comment.
@Jignesh-dimagi
messagingNotificationIdsare not the message ids here but the ids of the notifications associated with the notifications which is what we need to acknowledge to the server.
Yeah agree. I was trying to see if we can only acknowledge message notifications which are stored successfully. I also see your point that we don't have right now a way to do that.
|
|
||
| // ========== Test Data Builders ========== | ||
|
|
||
| private fun createPushNotificationJson( |
There was a problem hiding this comment.
@shubham1g5 Just a thought. We can create a util NotificationsTestUtil and add all create methods in it to be used by other tests also in future.
| private IApiCallback createNotificationCallback(Context context) { | ||
| onStart(); | ||
| return new BaseApiCallback<T>(this) { | ||
| @Override | ||
| public void processSuccess(int responseCode, InputStream responseData) { | ||
| try { | ||
| RetrieveNotificationsResponseParser parser = new RetrieveNotificationsResponseParser(context); | ||
| NotificationParseResult parseResult = parser.parse(responseCode, responseData, null); | ||
| ProcessedNotificationResult processedResult = | ||
| NotificationService.INSTANCE.processNotificationData(context, parseResult); | ||
| onSuccess((T) processedResult); | ||
| onStop(); | ||
| } catch (Exception e) { | ||
| Logger.exception("Error processing notification data", e); | ||
| stopLoadingAndInformError(PersonalIdOrConnectApiErrorCodes.JSON_PARSING_ERROR, e); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
@shubham1g5 This class doesn't have implementation for BaseApiCallback for particular API. I am seeing that you can directly return ProcessedNotificationResult from here and let PushNotificationApiHelper handle the NotificationService part. This will maintain the uniformity of this class for all API calls.
| user.getPassword(), | ||
| createCallback(new RetrieveNotificationsResponseParser<>(context), null) | ||
| ); | ||
| createCallback((BaseApiResponseParser<T>) new RetrieveNotificationsResponseParser(context), null)); |
There was a problem hiding this comment.
@shubham1g5 Why data type casting is required here?
There was a problem hiding this comment.
It's required due to the change in RetrieveNotificationsResponseParser definition to extend from BaseApiResponseParser<NotificationParseResult> instead of BaseApiResponseParser<T>
There was a problem hiding this comment.
We can still keep BaseApiResponseParser<T> and return NotificationParseResult as T?
There was a problem hiding this comment.
No strong thoughts on my end, Do you have a sense of why one is better than other ?
There was a problem hiding this comment.
Seems like we need to cast over here or as done in this PR.
Product Description
https://dimagi.atlassian.net/browse/CCCT-1940
Technical Summary
Refactors the code to separate out DB operations from parser to be able to uniformly test parser without mocking a lot of DB calls.
API Response → Parser (Focuses on pure json parsing) → PushNotificationApiHelper (Stores data into DB, UI state change and Acknowledment to server)
Safety Assurance
Safety story
This should not cause any changes in the existing functionality, I have verified it with tests but given the code has changed a bunch, I think we will need to have a QA cycle on notifications before releasing this (already part of QA plan)
QA Plan
Should be QA'ed as part of mobile release
Labels and Review