PushNotificationRecord model changes as per notification api response #3353

jaypanchal-13 · 2025-10-01T07:57:15Z

Product Description

Updated model as per /messaging/retrieve_notifications/ api changes

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

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-10-01T08:01:15Z

📝 Walkthrough

Walkthrough

The Pull Request updates PushNotificationRecord by converting multiple numeric fields to String, adjusting nullability and defaults, and renaming date_time to timeStamp with a new META_TIME_STAMP constant. It adds a Boolean readStatus. A companion object method fromJsonArray(JSONArray) is introduced to parse a JSON array into PushNotificationRecord instances, extracting nested “data” fields (connectMessageId, channel, action, opportunityId) and handling errors via Logger and RuntimeException. Necessary imports for Logger, JSONArray, and JSONException are added.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant Model as PushNotificationRecord
  participant JSON as JSONArray
  participant Item as JSONObject
  participant Data as data JSONObject

  Caller->>Model: fromJsonArray(JSONArray)
  activate Model
  Model->>JSON: iterate items
  loop For each item
    JSON-->>Model: JSONObject
    Model->>Item: read fields (notificationId, type, title, body,\nMETA_TIME_STAMP, confirmationStatus, paymentId, readStatus)
    alt Has nested "data"
      Item-->>Model: data JSONObject
      Model->>Data: read connectMessageId, channel, action, opportunityId
    else No "data"
      Model-->>Model: use defaults
    end
    Model-->>Caller: accumulate PushNotificationRecord
  end
  deactivate Model
  Note over Model: On parse error: log via Logger and throw RuntimeException

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Created table's for push notification history #3301 — Also modifies PushNotificationRecord; introduces persistable table/class while this PR changes field types, constants, and adds JSON parsing, indicating direct model-level linkage.

Suggested labels

cross requested

Suggested reviewers

Jignesh-dimagi
OrangeAndGreen
shubham1g5

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The description only fills the Product Description section and leaves the Technical Summary, Feature Flag, Safety Assurance (including safety story), Automated test coverage, and QA Plan entirely blank, so it does not adhere to the required template structure.	Please populate the Technical Summary with ticket links and design rationale, specify any applicable Feature Flag, detail your Safety story including data impact and testing confidence, add Automated test coverage information, outline the QA Plan, and confirm that labels and reviewer checklist items are addressed.
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.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title succinctly identifies the primary change—updating the PushNotificationRecord model to match the notification API response—and does so in a clear, single sentence without extraneous details.

✨ Finishing touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch pn-model-changes

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b6b0609 and d11bf74.

📒 Files selected for processing (1)

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

⏰ 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

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

Jignesh-dimagi · 2025-10-01T08:21:26Z

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

+                try {
+                    val obj = jsonArray.getJSONObject(i)
+                    val record = PushNotificationRecord().apply {
+                        notificationId = obj.optString(META_NOTIFICATION_ID,"")


@jaypanchal-13 Notification id is mandatory field and should raise exception if not available

Jignesh-dimagi · 2025-10-01T08:22:17Z

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

+                        title = obj.optString(META_TITLE,"")
+                        body = obj.optString(META_BODY,"")


@jaypanchal-13 title, body is mandatory field and should raise exception if not available

Jignesh-dimagi · 2025-10-01T08:22:29Z

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

+                        dataObj?.let {
+                            connectMessageId = it.optString(META_MESSAGE_ID,"")
+                            channel = it.optString(META_CHANNEL,"")
+                            action = it.optString(META_ACTION,"")


@jaypanchal-13 action is mandatory field and should raise exception if not available

Jignesh-dimagi · 2025-10-01T08:24:23Z

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

+                    Logger.exception("Error parsing notification at index $i", e)
+                    throw RuntimeException("Error parsing notification at index $i", e)


@jaypanchal-13 Only log Error parsing push notification history

OrangeAndGreen · 2025-10-01T18:53:46Z

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

+            return records
+        }
+
+        private fun getRequiredString(obj: JSONObject, key: String, index: Int): String {


I think this should be defined in JsonExtensions.kt instead so it can also be used by other parsers as we make more things officially required.

OrangeAndGreen · 2025-10-01T19:00:04Z

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

-    @MetaField(META_DATE_TIME)
-    var dateTime: String? = null
+    @MetaField(META_TIME_STAMP)
+    var timeStamp: String = ""


I think the name for this should be more descriptive. Thinking something like createdDate, sentDate, receivedDate, expirationDate. A name like "dateTime" or "timestamp" tells us the data type, but not it's function.

On that note though, shouldn't this field be a Date object instead of a String?

Yes changing it to Date

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

OrangeAndGreen · 2025-10-01T19:09:17Z

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

+                    records.add(record)
+                } catch (e: JSONException) {
+                    Logger.exception("Error parsing push notification", e)
+                    throw RuntimeException("Error parsing push notification history", e)


For @shubham1g5 and @Jignesh-dimagi to confirm:
I don't think we should throw a RuntimeException here, as we've discussed that one failed item in a list of received items should not cause the app to completely crash and block the user.

I think we should either ignore the corrupt item (aside from logging it), or have a "corrupt item" placeholder to show to the user (as we've implemented in other places).

think I would be fine either way on this given it's a non core workflow and users can arrive at the feature screens from other pathways.

I guess we should keep it as it will give us confirmation that nothing is wrong in newly build functionality and user are able to use it without any issues. Open to the suggestions

Maybe I'm mistaken, but won't the RuntimeException crash the app anytime we receive a corrupt notification item? It seems this call could be made somewhat frequently and without direct intention by the user (like to auto-refresh when the app starts or a page loads), and crashing due to one bad item could prevent the user from performing other work.

@OrangeAndGreen That's a good point. @jaypanchal-13 I think in that case, please handle it gracefully and report error to user.

…changes

shubham1g5 · 2025-10-06T08:54:56Z

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

+                } catch (e: JSONException) {
+                    val corruptedJson = obj?.toString() ?: "Unknown JSON"
+                    val errorMessage = "Corrupt pn at index $i: $corruptedJson"
+                    Logger.exception(errorMessage, e)
+                }


can we have this method throw JsonException as it is instead (no catch) and have callers decide whether they want to swallow vs re-throw. I don't think it's alright for us to swallow this mehtod always for example on notification UI screen where we should be just crashing the app instead.

shubham1g5 · 2025-10-06T08:56:37Z

app/src/org/commcare/utils/JsonExtensions.kt

+}
+
+fun JSONObject.getRequiredString(key: String, index: Int): String {
+    return optString(key, "").takeIf { it.isNotBlank() && it != "null" }


this should use optStringSafe ?

shubham1g5 · 2025-10-06T10:03:58Z

@OrangeAndGreen @Jignesh-dimagi blocked on your reviews and flagging that we want this to get in for 2.60 to avoid a DB migration.

shubham1g5 · 2025-10-06T10:04:38Z

@jaypanchal-13 Looks like code is failing to compile and build is failing because of that.

jaypanchal-13 · 2025-10-06T10:08:54Z

@jaypanchal-13 Looks like code is failing to compile and build is failing because of that.

@shubham1g5 yes once approved by both. Will intiate rebuild if still fails

shubham1g5 · 2025-10-06T10:12:39Z

yes once approved by both. Will intiate rebuild if still fails

@jaypanchal-13 Code must compile before a review

jaypanchal-13 · 2025-10-06T10:16:12Z

yes once approved by both. Will intiate rebuild if still fails

@jaypanchal-13 Code must compile before a review

@shubham1g5 Compiling successfully locally

shubham1g5 · 2025-10-06T10:20:10Z

@jaypanchal-13 Can you doube check ? I tried building the branch and it's failing straight away -

…successfully

jaypanchal-13 · 2025-10-06T10:26:46Z

@jaypanchal-13 Can you doube check ? I tried building the branch and it's failing straight away -

@shubham1g5 yes pushing that file too

Jignesh-dimagi · 2025-10-06T11:54:49Z

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

+                    notificationId = obj.getRequiredString(META_NOTIFICATION_ID, i)
+                    title = obj.getRequiredString(META_TITLE, i)
+                    body = obj.getRequiredString(META_BODY, i)


@jaypanchal-13 We don't want to crash the application if strings are not present as pointed out by @OrangeAndGreen. Let this method throw JSONException and calling function handle it depending upon the screen as @shubham1g5 mentioned earlier

@Jignesh-dimagi I think @OrangeAndGreen was referring for fields which are not mandadory for pn and notificationId title body are mandadory fields. So did not removed crash part for it cc: @shubham1g5

@jaypanchal-13 Let the decision for crashing / not crashing or handling the error be taken by calling screen. This is due to the fact that this API might be called at multiple places and if crashed, it will not allow user to work on any important task like opportunity. Yeah but when user on PN activity, app should crash exclusively.

Jignesh-dimagi · 2025-10-06T11:55:15Z

app/src/org/commcare/utils/JsonExtensions.kt

+fun JSONObject.getRequiredString(key: String, index: Int): String {
+    return this.optStringSafe(key)?.takeIf { it.isNotBlank() && it != "null" }
+        ?: throw RuntimeException("$key is missing at index $index")


@jaypanchal-13 We can remove it completely

shubham1g5 · 2025-10-06T13:47:03Z

app/src/org/commcare/connect/database/NotificationRecordDatabaseHelper.kt

+                    incoming.id = existing.id
+                }
+                storage.write(incoming)
+            } catch (e: SQLException) {


we should not be catching this as well

@shubham1g5 catching here with purpose of getting boolean true/false. Not throwing exception. What do you suggest

I will still opt for not catching it, app should crash in situations like this.

PushNotificationRecord model changes as per notification api response

d11bf74

jaypanchal-13 assigned OrangeAndGreen, shubham1g5 and Jignesh-dimagi Oct 1, 2025

jaypanchal-13 requested a review from shubham1g5 October 1, 2025 07:57

jaypanchal-13 unassigned OrangeAndGreen, shubham1g5 and Jignesh-dimagi Oct 1, 2025

jaypanchal-13 requested review from Jignesh-dimagi and OrangeAndGreen October 1, 2025 07:57

Merge branch 'master' into pn-model-changes

f5ee1c7

Jignesh-dimagi requested changes Oct 1, 2025

View reviewed changes

comment resolve

509ea65

jaypanchal-13 requested a review from Jignesh-dimagi October 1, 2025 10:36

Merge branch 'master' into pn-model-changes

8432aad

OrangeAndGreen requested changes Oct 1, 2025

View reviewed changes

Merge remote-tracking branch 'origin/master' into pn-model-changes

37b7eb3

shubham1g5 added this to the 2.60 milestone Oct 6, 2025

jaypanchal-13 added 4 commits October 6, 2025 14:17

PushNotificationRecord model changes as per notification api response

c66dcd0

comment resolve

aabaeae

Merge remote-tracking branch 'origin/pn-model-changes' into pn-model-…

f69345b

…changes

comments changes

78a593f

jaypanchal-13 requested a review from OrangeAndGreen October 6, 2025 08:52

shubham1g5 requested changes Oct 6, 2025

View reviewed changes

throw exception changes

730d9f5

jaypanchal-13 requested a review from shubham1g5 October 6, 2025 09:47

shubham1g5 previously approved these changes Oct 6, 2025

View reviewed changes

pushing NotificationRecordDatabaseHelper changes so PR can compiling …

d73df76

…successfully

jaypanchal-13 dismissed shubham1g5’s stale review via d73df76 October 6, 2025 10:25

jaypanchal-13 requested a review from shubham1g5 October 6, 2025 10:37

Jignesh-dimagi requested changes Oct 6, 2025

View reviewed changes

removed jsonexception from mandadory fields

481c45e

jaypanchal-13 requested a review from Jignesh-dimagi October 6, 2025 13:30

shubham1g5 reviewed Oct 6, 2025

View reviewed changes

OrangeAndGreen previously approved these changes Oct 6, 2025

View reviewed changes

jaypanchal-13 requested a review from shubham1g5 October 6, 2025 14:09

jaypanchal-13 added 2 commits October 6, 2025 19:40

Merge branch 'master' into pn-model-changes

eb2cbe9

removed exception

ca5b2e2

jaypanchal-13 dismissed OrangeAndGreen’s stale review via ca5b2e2 October 6, 2025 14:31

shubham1g5 approved these changes Oct 6, 2025

View reviewed changes

shubham1g5 added the skip-integration-tests Skip android tests. label Oct 6, 2025

shubham1g5 merged commit 03da90a into master Oct 6, 2025
4 of 7 checks passed

shubham1g5 deleted the pn-model-changes branch October 6, 2025 17:10

coderabbitai bot mentioned this pull request Oct 21, 2025

Jignesh/feat/ccct 1718 #3373

Merged

coderabbitai bot mentioned this pull request Nov 6, 2025

New response as per CCCT-1893 #3407

Merged

4 tasks

coderabbitai bot mentioned this pull request Dec 8, 2025

Notifications Parser refactor and tests #3455

Merged

4 tasks

		title = obj.optString(META_TITLE,"")
		body = obj.optString(META_BODY,"")

		Logger.exception("Error parsing notification at index $i", e)
		throw RuntimeException("Error parsing notification at index $i", e)

Uh oh!

PushNotificationRecord model changes as per notification api response #3353

PushNotificationRecord model changes as per notification api response #3353

Uh oh!

Conversation

jaypanchal-13 commented Oct 1, 2025

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Labels and Review

Uh oh!

coderabbitai bot commented Oct 1, 2025

Walkthrough

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jignesh-dimagi Oct 3, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubham1g5 Oct 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!

Choose a reason for hiding this comment

Uh oh!

shubham1g5 commented Oct 6, 2025

Uh oh!

shubham1g5 commented Oct 6, 2025

Uh oh!

jaypanchal-13 commented Oct 6, 2025

Uh oh!

shubham1g5 commented Oct 6, 2025

Uh oh!

jaypanchal-13 commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shubham1g5 commented Oct 6, 2025

Uh oh!

jaypanchal-13 commented Oct 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaypanchal-13 Oct 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 Oct 3, 2025 •

edited

Loading

shubham1g5 Oct 6, 2025 •

edited

Loading

jaypanchal-13 commented Oct 6, 2025 •

edited

Loading

jaypanchal-13 Oct 6, 2025 •

edited

Loading

jaypanchal-13 Oct 6, 2025 •

edited

Loading