New response as per CCCT-1893 #3407

Jignesh-dimagi · 2025-11-06T11:06:58Z

Technical Summary

https://dimagi.atlassian.net/browse/CCCT-1893

Labels and Review

Do we need to enhance the manual QA test coverage ? If yes, the "QA Note" label is set correctly
Does the PR introduce any major changes worth communicating ? If yes, the "Release Note" label is set and a "Release Note" is specified in PR description.
Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change

coderabbitai · 2025-11-06T11:09:17Z

📝 Walkthrough

Walkthrough

The pull request modifies PushNotificationRecord.kt to add a new Boolean field acknowledged with persistence annotations and refactors the JSON parsing logic in fromJsonArray. Previously, the method extracted field values from a nested "data" JSON object; now it reads connectMessageId, channel, action, and opportunityId directly from the top-level JSON object. The class declaration is also reflowed for formatting.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Verify that the new acknowledged field is correctly annotated with @Persisting(13) and matches the schema versioning
Confirm the JSON parsing change from nested dataObj extraction to direct top-level field access aligns with the current API response structure and does not break existing functionality
Ensure backward compatibility if older API responses still use the nested structure

Possibly related PRs

PushNotificationRecord model changes as per notification api response #3353: Modifies the same PushNotificationRecord.kt file and adds a similar Boolean status field alongside JSON parsing updates for connectMessageId, channel, and opportunityId.

Suggested reviewers

shubham1g5

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The pull request description is largely incomplete, missing critical sections like Product Description, Safety Story, Automated Test Coverage, and QA Plan required by the template.	Complete the PR description by adding Product Description (user-facing effects), Safety Story (testing and safety rationale), Automated Test Coverage, and QA Plan sections.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Title check	❓ Inconclusive	The title 'New response as per CCCT-1893' is vague and does not clearly describe the actual changes made to the codebase. While it references a ticket, it provides no meaningful information about what was changed.	Revise the title to be more descriptive of the actual changes, such as 'Add acknowledged field to PushNotificationRecord' or 'Update PushNotificationRecord JSON parsing logic'.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch jignesh/feat/ccct-1893

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 33d73bb and e4468ad.

📒 Files selected for processing (1)

app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt (3 hunks)

🧰 Additional context used

🧠 Learnings (5)

📓 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: 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: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.

📚 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/android/database/connect/models/PushNotificationRecord.kt

📚 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/PushNotificationRecord.kt

📚 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/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.kt

coderabbitai · 2025-11-06T11:09:21Z

app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt

+                val record =
+                    PushNotificationRecord().apply {
+                        notificationId = obj.optString(META_NOTIFICATION_ID, "")
+                        title = obj.optString(META_TITLE, "")
+                        body = obj.optString(META_BODY, "")
+                        notificationType = obj.optString(META_NOTIFICATION_TYPE, "")
+                        confirmationStatus = obj.optString(META_CONFIRMATION_STATUS, "")
+                        paymentId = obj.optString(META_PAYMENT_ID, "")
+                        readStatus = obj.optBoolean(META_READ_STATUS, false)
+                        val dateString: String = obj.getString(META_TIME_STAMP)
+                        createdDate = DateUtils.parseDateTime(dateString)
+                        connectMessageId = obj.optString(META_MESSAGE_ID, "")
+                        channel = obj.optString(META_CHANNEL, "")
+                        action = obj.optString(META_ACTION, "")
+                        opportunityId = obj.optString(META_OPPORTUNITY_ID, "")
                    }


⚠️ Potential issue | 🟠 Major

Populate acknowledged state from JSON
We introduced META_ACKNOWLEDGED, but fromJsonArray never assigns the new acknowledged field. Every record will therefore persist false, even if the server marks it as acknowledged. Please read the value from the payload so we don’t lose that state.

Apply this diff inside the apply { ... } block:

- opportunityId = obj.optString(META_OPPORTUNITY_ID, "") + opportunityId = obj.optString(META_OPPORTUNITY_ID, "") + acknowledged = obj.optBoolean(META_ACKNOWLEDGED, false)

📝 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.

Suggested change

val record =

PushNotificationRecord().apply {

notificationId = obj.optString(META_NOTIFICATION_ID, "")

title = obj.optString(META_TITLE, "")

body = obj.optString(META_BODY, "")

notificationType = obj.optString(META_NOTIFICATION_TYPE, "")

confirmationStatus = obj.optString(META_CONFIRMATION_STATUS, "")

paymentId = obj.optString(META_PAYMENT_ID, "")

readStatus = obj.optBoolean(META_READ_STATUS, false)

val dateString: String = obj.getString(META_TIME_STAMP)

createdDate = DateUtils.parseDateTime(dateString)

connectMessageId = obj.optString(META_MESSAGE_ID, "")

channel = obj.optString(META_CHANNEL, "")

action = obj.optString(META_ACTION, "")

opportunityId = obj.optString(META_OPPORTUNITY_ID, "")

}

val record =

PushNotificationRecord().apply {

notificationId = obj.optString(META_NOTIFICATION_ID, "")

title = obj.optString(META_TITLE, "")

body = obj.optString(META_BODY, "")

notificationType = obj.optString(META_NOTIFICATION_TYPE, "")

confirmationStatus = obj.optString(META_CONFIRMATION_STATUS, "")

paymentId = obj.optString(META_PAYMENT_ID, "")

readStatus = obj.optBoolean(META_READ_STATUS, false)

val dateString: String = obj.getString(META_TIME_STAMP)

createdDate = DateUtils.parseDateTime(dateString)

connectMessageId = obj.optString(META_MESSAGE_ID, "")

channel = obj.optString(META_CHANNEL, "")

action = obj.optString(META_ACTION, "")

opportunityId = obj.optString(META_OPPORTUNITY_ID, "")

acknowledged = obj.optBoolean(META_ACKNOWLEDGED, false)

}

🤖 Prompt for AI Agents

In app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt around lines 92 to 107, the newly added META_ACKNOWLEDGED value is not read when constructing PushNotificationRecord, so acknowledged remains default false; update the apply { ... } block to read obj.optBoolean(META_ACKNOWLEDGED, false) and assign it to the record's acknowledged property (e.g., acknowledged = obj.optBoolean(META_ACKNOWLEDGED, false)) so the persisted record reflects the payload's acknowledged state.

META_ACKNOWLEDGED is not coming from server so no need to read here

@Jignesh-dimagi, thank you for the clarification! I understand now that acknowledged is a client-side field managed within the app and not part of the server's JSON response for push notifications. That makes sense—the field will be updated locally when the user acknowledges a notification.

✏️ Learnings added

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.923Z 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.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used

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: 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.

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.

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: 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.

shubham1g5 · 2025-11-06T11:12:05Z

app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt

-                        opportunityId = it.optString(META_OPPORTUNITY_ID, "")
+                val record =
+                    PushNotificationRecord().apply {
+                        notificationId = obj.optString(META_NOTIFICATION_ID, "")


can we use getString for anything required ? Or are we doing payload validation at some other place ?

We are not raising the exception to grab the non-corrupt ones and ignoring the corrupt one.

New response as per CCCT-1893

e4468ad

Jignesh-dimagi requested review from OrangeAndGreen, conroy-ricketts and shubham1g5 and removed request for shubham1g5 November 6, 2025 11:07

Jignesh-dimagi added the skip-integration-tests Skip android tests. label Nov 6, 2025

coderabbitai bot reviewed Nov 6, 2025

View reviewed changes

shubham1g5 reviewed Nov 6, 2025

View reviewed changes

shubham1g5 approved these changes Nov 6, 2025

View reviewed changes

conroy-ricketts approved these changes Nov 6, 2025

View reviewed changes

Jignesh-dimagi merged commit 360f163 into master Nov 7, 2025
7 checks passed

Jignesh-dimagi deleted the jignesh/feat/ccct-1893 branch November 7, 2025 05:15

coderabbitai bot mentioned this pull request Nov 7, 2025

Bug solved for retrieve notification API for storing connect messaging #3410

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

New response as per CCCT-1893 #3407

New response as per CCCT-1893 #3407

Uh oh!

Jignesh-dimagi commented Nov 6, 2025

Uh oh!

coderabbitai bot commented Nov 6, 2025

Walkthrough

Estimated code review effort

Possibly related PRs

Suggested reviewers

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Nov 6, 2025 •

edited

Loading

Uh oh!

Jignesh-dimagi Nov 6, 2025

Uh oh!

coderabbitai bot Nov 6, 2025

Uh oh!

shubham1g5 Nov 6, 2025

Uh oh!

Jignesh-dimagi Nov 6, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

New response as per CCCT-1893 #3407

New response as per CCCT-1893 #3407

Uh oh!

Conversation

Jignesh-dimagi commented Nov 6, 2025

Technical Summary

Labels and Review

Uh oh!

coderabbitai bot commented Nov 6, 2025

Walkthrough

Estimated code review effort

Possibly related PRs

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jignesh-dimagi Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

shubham1g5 Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Jignesh-dimagi Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coderabbitai bot Nov 6, 2025 •

edited

Loading

Jignesh-dimagi Nov 6, 2025 •

edited

Loading