Created table's for push notification history #3301
Conversation
jaypanchal-13
requested review from
Jignesh-dimagi,
OrangeAndGreen and
shubham1g5
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
participant App
participant DBHelper as DatabaseConnectOpenHelper
participant DB as Connect DB
App->>DBHelper: onCreate(db)
DBHelper->>DB: create table PersonalIdCredential
DBHelper->>DB: create table PushNotificationRecord
DBHelper->>DB: create table ConnectChannel
DBHelper->>DB: create table ConnectMessage
DBHelper->>DB: create numbers table
sequenceDiagram
participant App
participant Upgrader as ConnectDatabaseUpgrader
participant DB as Connect DB
App->>Upgrader: onUpgrade(db, old=16, new=17)
Upgrader->>DB: addTableForNewModel PushNotificationRecord
Upgrader->>DB: addTableForNewModel ConnectChannel
Upgrader->>DB: addTableForNewModel ConnectMessage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
app/src/org/commcare/android/database/connect/models/ConnectChannel.kt (1)
9-26: Add KDoc for fields and class to document intent and formatsConsistent with other Connect models, add brief documentation describing field purpose and constraints (e.g., channelId origin, uniqueness, channelName expectations). This improves maintainability and aids migrations.
Example:
@Table(ConnectChannel.STORAGE_KEY) -class ConnectChannel : Persisted(), Serializable { +/** + * Persisted Connect channel metadata. + * + * - channelId: Server-assigned channel identifier (stable, non-negative). + * - channelName: Human-readable name shown in UI. + */ +class ConnectChannel : Persisted(), Serializable { @Persisting(1) @MetaField(META_CHANNEL_ID) - var channelId: Int = 0 + /** Server-assigned unique identifier for this channel. */ + var channelId: Int = 0 @Persisting(2) @MetaField(META_CHANNEL_NAME) - var channelName: String = "" + /** Display name for the channel. */ + var channelName: String = ""app/src/org/commcare/android/database/connect/models/ConnectMessage.kt (1)
9-56: Document expected formats (timestamp, status) and relationshipsAdd KDoc to clarify:
- timestamp format (e.g., ISO-8601 UTC string) and timezone expectations.
- status allowed values (enum-like string?).
- channel vs channelName relationship and whether channel is a ConnectChannel.id.
This aligns with team practices on documenting Connect model fields.Example:
@Table(ConnectMessage.STORAGE_KEY) -class ConnectMessage : Persisted(), Serializable { +/** + * Persisted Connect message payload and metadata. + * + * - messageId: Server-assigned message identifier. + * - status: Delivery/processing status (document allowed values). + * - ciphertext/nonce: Encrypted payload fields (define encoding). + * - timestamp: ISO-8601 UTC string (e.g., 2025-08-18T14:06:37Z). + * - channel: Foreign key to ConnectChannel.channelId (no FK enforced). + * - channelName: Convenience copy of channel's name at time of write. + */ +class ConnectMessage : Persisted(), Serializable { @Persisting(5) @MetaField(META_TIMESTAMP) - var timestamp: String = "" + /** ISO-8601 UTC timestamp string when the message was produced/received. */ + var timestamp: String = ""app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt (4)
12-47: Add KDoc per Connect persistence conventionsPer prior Connect models, please document the class and each persisted field (purpose, format, relationships). This aids future migrations and API consumers.
Apply illustrative KDoc (fill in specifics as appropriate):
class PushNotificationRecord : Persisted(), Serializable { + /** + * Unique identifier of the push notification as received from the platform (e.g., FCM). + * Not the storage record ID. + */ @Persisting(1) @MetaField(META_NOTIFICATION_ID) var notificationId: Int = 0 + /** High-level type of the push (e.g., "message", "reminder", "status"). */ @Persisting(2) @MetaField(META_ACTION) var action: String = "" + /** Notification title as shown to the user. */ @Persisting(3) @MetaField(META_TITLE) var title: String = "" + /** Notification body as shown to the user. */ @Persisting(4) @MetaField(META_BODY) var body: String = "" + /** Timestamp when the notification was received/rendered. Specify the format in companion docs. */ @Persisting(5) @MetaField(META_DATE_TIME) var dateTime: String = "" + /** Delivery/interaction state (e.g., "delivered", "opened", "dismissed"). */ @Persisting(6) @MetaField(META_CONFIRMATION_STATUS) var confirmationStatus: String = "" + /** Optional linkage to an Opportunity entity, if applicable. */ @Persisting(7) @MetaField(META_OPPORTUNITY_ID) var opportunityId: Int = 0 + /** Foreign key to ConnectMessage (if this notification mirrors a Connect message). */ @Persisting(8) @MetaField(META_CONNECT_MESSAGE_ID) var connectMessageId: Int = 0 + /** Foreign key to ConnectChannel where the message originated. */ @Persisting(9) @MetaField(META_CONNECT_CHANNEL_ID) var connectChannelId: Int = 0
28-31: Prefer numeric timestamps over String for efficient queriesStoring time as a
Long(epoch millis) makes sorting/range queries and migrations safer and more performant than a free-form String. If this table hasn’t shipped yet, consider switching now.Minimal change:
- @MetaField(META_DATE_TIME) - var dateTime: String = "" + @MetaField(META_DATE_TIME) + var dateTime: Long = 0LIf you need human-readable display, keep a derived/formatting layer rather than persisting the formatted value.
40-46: Align foreign key column names with referenced tablesUse shared column names for FKs to reduce confusion and enable generic helpers. Other models use
message_idandchannel_id; this class currently usesconnect_message_id/connect_channel_id.Keep field names if you prefer, but align the Meta column names:
@Persisting(8) - @MetaField(META_CONNECT_MESSAGE_ID) + @MetaField(META_CONNECT_MESSAGE_ID) var connectMessageId: Int = 0 @Persisting(9) - @MetaField(META_CONNECT_CHANNEL_ID) + @MetaField(META_CONNECT_CHANNEL_ID) var connectChannelId: Int = 0 @@ - const val META_CONNECT_MESSAGE_ID = "connect_message_id" - const val META_CONNECT_CHANNEL_ID = "connect_channel_id" + const val META_CONNECT_MESSAGE_ID = "message_id" + const val META_CONNECT_CHANNEL_ID = "channel_id"Note: This is a forward-compatible tweak if the table hasn’t been released yet. Otherwise, it requires a migration.
Also applies to: 58-60
48-60: Optional: define serialVersionUIDIf this class might ever be passed via Java serialization, declare a
serialVersionUIDfor stability. If not, feel free to skip.companion object { const val STORAGE_KEY = "push_notification_history" + // For Java serialization compatibility (optional) + private const val serialVersionUID: Long = 1L + const val META_NOTIFICATION_ID = "notification_id"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
app/src/org/commcare/android/database/connect/models/ConnectChannel.kt(1 hunks)app/src/org/commcare/android/database/connect/models/ConnectMessage.kt(1 hunks)app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt(1 hunks)app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java(5 hunks)app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java(4 hunks)app/unit-tests/src/org/commcare/android/tests/processing/FormStorageTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: shubham1g5
PR: dimagi/commcare-android#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.
📚 Learning: 2025-04-18T20:13:29.655Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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/models/database/connect/DatabaseConnectOpenHelper.javaapp/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java
📚 Learning: 2025-05-08T11:08:18.530Z
Learnt from: shubham1g5
PR: dimagi/commcare-android#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/models/database/connect/DatabaseConnectOpenHelper.javaapp/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java
📚 Learning: 2025-06-06T19:54:26.428Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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/models/database/connect/DatabaseConnectOpenHelper.javaapp/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java
📚 Learning: 2025-06-06T20:15:21.134Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3108
File: app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java:65-71
Timestamp: 2025-06-06T20:15:21.134Z
Learning: In the CommCare Android Connect module, job.getLearnAppInfo() and getLearnModules() should never return null according to the system design. The team prefers to let the code crash if these values are unexpectedly null rather than adding defensive null checks, following a fail-fast philosophy to catch bugs early rather than masking them.
Applied to files:
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java
📚 Learning: 2025-06-06T19:52:53.173Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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/models/database/connect/DatabaseConnectOpenHelper.java
📚 Learning: 2025-01-28T08:41:32.380Z
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/android/database/connect/models/ConnectLinkedAppRecordV8.java:107-107
Timestamp: 2025-01-28T08:41:32.380Z
Learning: When upgrading from ConnectLinkedAppRecordV3 to ConnectLinkedAppRecordV8, the connectIdLinked field is intentionally set to true as part of the database migration strategy since this field was not available from the server in V3 and is introduced in V8.
Applied to files:
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.javaapp/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java
📚 Learning: 2025-01-21T17:28:09.007Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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.ktapp/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.javaapp/src/org/commcare/android/database/connect/models/ConnectMessage.kt
📚 Learning: 2025-01-23T21:38:24.443Z
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/android/database/connect/models/ConnectLinkedAppRecordV3.java:44-68
Timestamp: 2025-01-23T21:38:24.443Z
Learning: The ConnectLinkedAppRecord classes are intentionally designed with simple getters/setters without additional validation or defensive copies.
Applied to files:
app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java
📚 Learning: 2025-01-23T21:38:17.427Z
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/android/database/connect/models/ConnectLinkedAppRecordV3.java:1-68
Timestamp: 2025-01-23T21:38:17.427Z
Learning: The versioned database model classes (e.g., ConnectLinkedAppRecordV3) intentionally maintain their original design patterns, including their security implementation, to preserve backward compatibility.
Applied to files:
app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java
📚 Learning: 2025-07-29T14:14:07.954Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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/models/database/connect/ConnectDatabaseUpgrader.java
🧬 Code Graph Analysis (2)
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java (3)
app/src/org/commcare/android/database/connect/models/ConnectChannel.kt (1)
ConnectChannel(9-26)app/src/org/commcare/android/database/connect/models/ConnectMessage.kt (1)
ConnectMessage(9-56)app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt (1)
PushNotificationRecord(9-61)
app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java (3)
app/src/org/commcare/android/database/connect/models/ConnectChannel.kt (1)
ConnectChannel(9-26)app/src/org/commcare/android/database/connect/models/ConnectMessage.kt (1)
ConnectMessage(9-56)app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt (1)
PushNotificationRecord(9-61)
🔇 Additional comments (5)
app/unit-tests/src/org/commcare/android/tests/processing/FormStorageTest.java (1)
401-404: Good addition: new externalizable classes are listedThe three new classes are correctly appended to the complete history list.
app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java (2)
124-127: Upgrade step 16 -> 17 added — make sure it can runThe new 16→17 step looks correct but will only execute if CONNECT_DB_VERSION is bumped to 17 in DatabaseConnectOpenHelper. See my comment there.
603-607: Table creations for v17 look correctaddTableForNewModel calls for PushNotificationRecord, ConnectChannel, and ConnectMessage are consistent with prior patterns.
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java (1)
147-155: onCreate additions for new tables look goodAll three new tables are created during initial DB creation in the right place before DbUtil.createNumbersTable.
app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt (1)
9-11: LGTM: Table mapping and base class usage look correct
@Table(PushNotificationRecord.STORAGE_KEY)and extendingPersisted()/Serializablealign with existing Connect persistence patterns.
| package org.commcare.android.database.connect.models | ||
|
|
||
| import org.commcare.android.storage.framework.Persisted | ||
| import org.commcare.models.framework.Persisting | ||
| import org.commcare.modern.database.Table | ||
| import org.commcare.modern.models.MetaField | ||
| import java.io.Serializable | ||
|
|
||
| @Table(PushNotificationRecord.STORAGE_KEY) | ||
| class PushNotificationRecord : Persisted(), Serializable { | ||
|
|
||
| @Persisting(1) | ||
| @MetaField(META_NOTIFICATION_ID) | ||
| var notificationId: Int = 0 | ||
|
|
||
| @Persisting(2) | ||
| @MetaField(META_ACTION) | ||
| var action: String = "" | ||
|
|
||
| @Persisting(3) | ||
| @MetaField(META_TITLE) | ||
| var title: String = "" | ||
|
|
||
| @Persisting(4) | ||
| @MetaField(META_BODY) | ||
| var body: String = "" | ||
|
|
||
| @Persisting(5) | ||
| @MetaField(META_DATE_TIME) | ||
| var dateTime: String = "" | ||
|
|
||
| @Persisting(6) | ||
| @MetaField(META_CONFIRMATION_STATUS) | ||
| var confirmationStatus: String = "" | ||
|
|
||
| @Persisting(7) | ||
| @MetaField(META_OPPORTUNITY_ID) | ||
| var opportunityId: Int = 0 | ||
|
|
||
| @Persisting(8) | ||
| @MetaField(META_CONNECT_MESSAGE_ID) | ||
| var connectMessageId: Int = 0 | ||
|
|
||
| @Persisting(9) | ||
| @MetaField(META_CONNECT_CHANNEL_ID) | ||
| var connectChannelId: Int = 0 | ||
|
|
||
| companion object { | ||
| const val STORAGE_KEY = "push_notification_history" | ||
|
|
||
| const val META_NOTIFICATION_ID = "notification_id" | ||
| const val META_ACTION = "action" | ||
| const val META_TITLE = "title" | ||
| const val META_BODY = "body" | ||
| const val META_DATE_TIME = "date_time" | ||
| const val META_CONFIRMATION_STATUS = "confirmation_status" | ||
| const val META_OPPORTUNITY_ID = "opportunity_id" | ||
| const val META_CONNECT_MESSAGE_ID = "connect_message_id" | ||
| const val META_CONNECT_CHANNEL_ID = "connect_channel_id" | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify table creation and upgrade wiring (onCreate, 16→17) and check for typos
Double-check that:
- DatabaseConnectOpenHelper.onCreate creates this table using
push_notification_history. - ConnectDatabaseUpgrader handles 16→17 creating this table.
- CONNECT_DB_VERSION is bumped accordingly.
- No stray "connect_messaget" typos exist anywhere.
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find storage key usages (createTable calls, queries) =="
rg -n -C3 -P '\bpush_notification_history\b'
echo
echo "== Check DB version constant(s) =="
rg -n -C2 -P 'CONNECT_DB_VERSION\s*=\s*\d+'
echo
echo "== Inspect upgrade path mentioning 17 and table creation =="
rg -n -C3 -P '(upgrade|onUpgrade|onCreate).*17|version\s*17|createTable' app | sed -n '1,200p'
echo
echo "== Typos: search for 'connect_messaget' =="
rg -n 'connect_messaget'Length of output: 3783
Add DB migration for push_notification_history (v16→17)
Please update DatabaseConnectOpenHelper.java to wire up the new table:
- Bump the version constant from 16 to 17:
private static final int CONNECT_DB_VERSION = 17;
- In onCreate(...), add a call to create the
push_notification_historytable (and similarly forconnect_messageandconnect_channel):TableBuilder.createTable(db, PushNotificationRecord.class);
- In onUpgrade(...), between case 16 and the fall-through, invoke the same
createTablecall so that upgrades from v16→v17 pick up the new tables. - Verified: no stray “connect_messaget” typos remain.
🤖 Prompt for AI Agents
In
app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt
lines 1-61: add a DB migration in DatabaseConnectOpenHelper.java to register the
new push_notification_history table by bumping CONNECT_DB_VERSION from 16 to 17,
adding a call to TableBuilder.createTable(db, PushNotificationRecord.class) in
onCreate(...), and invoking the same createTable call in onUpgrade(...) in the
branch that handles upgrades from version 16 so v16→v17 creates the new tables
(also add similar createTable calls for connect_message and connect_channel),
ensuring no typos like “connect_messaget”.
| * V.17 - Added push_notification_history,connect_channel,connect_message table | ||
| */ | ||
| private static final int CONNECT_DB_VERSION = 16; | ||
|
|
There was a problem hiding this comment.
DB version mismatch blocks upgrade to new tables (bump to 17)
You documented V.17 additions and added an upgrader for 16→17, but CONNECT_DB_VERSION remains 16. This prevents existing installs at 16 from upgrading and creating the new tables, causing missing-table crashes when those models are accessed. New installs will get the tables via onCreate, but upgrades will not.
Please bump CONNECT_DB_VERSION to 17.
Apply:
- * V.17 - Added push_notification_history,connect_channel,connect_message table
+ * V.17 - Added push_notification_history, connect_channel, and connect_message tables
- private static final int CONNECT_DB_VERSION = 16;
+ private static final int CONNECT_DB_VERSION = 17;📝 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.
| * V.17 - Added push_notification_history,connect_channel,connect_message table | |
| */ | |
| private static final int CONNECT_DB_VERSION = 16; | |
| * V.17 - Added push_notification_history, connect_channel, and connect_message tables | |
| */ | |
| private static final int CONNECT_DB_VERSION = 17; |
🤖 Prompt for AI Agents
In app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java
around lines 63 to 66, the constant CONNECT_DB_VERSION is left at 16 while you
added V.17 schema changes and an upgrader for 16→17; update CONNECT_DB_VERSION
to 17 so existing installs will run the onUpgrade path and create the new
tables. Change the static final int CONNECT_DB_VERSION value from 16 to 17 and
run tests/upgrade flow to verify the 16→17 migration executes and the new tables
are present.
There was a problem hiding this comment.
@jaypanchal-13 Oops! Look like a good catch from coderabbit, this value needs to be changed to 17
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| ,"org.commcare.android.database.connect.models.PushNotificationRecord" | ||
| ,"org.commcare.android.database.connect.models.ConnectChannel" | ||
| ,"org.commcare.android.database.connect.models.ConnectMessage" | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify PrototypeFactory registration of new classes
Ensure these classes are actually registered in AndroidPrototypeFactory so the prototype factory can resolve them at runtime; otherwise the test will fail or, worse, runtime serialization could break.
Run this to confirm registration points:
Additionally, consider annotating these with a comment indicating the app version they were added in (consistent with the rest of this list) to aid future migrations.
🏁 Script executed:
#!/bin/bash
# Find AndroidPrototypeFactory and check for registrations of new classes
fd -a 'AndroidPrototypeFactory.java' -t f
rg -n -C3 'ConnectMessage|ConnectChannel|PushNotificationRecord' --type=java --type=kt
# Also list any factory helpers that assemble the prototype factory used in tests
rg -n -C3 'PrototypeFactory|register|addClass' app/src --type=java --type=kt | sed -n '1,200p'Length of output: 355
🏁 Script executed:
#!/bin/bash
# Search for all .register( calls in AndroidPrototypeFactory.java
rg -nP '\.register\(' app/src/org/commcare/models/AndroidPrototypeFactory.java
# Specifically check for registrations of the new model classes
rg -n -C3 'PushNotificationRecord.class|ConnectChannel.class|ConnectMessage.class' app/src/org/commcare/models/AndroidPrototypeFactory.javaLength of output: 222
Missing PrototypeFactory registrations for new storage models
I didn’t find any calls to register PushNotificationRecord, ConnectChannel, or ConnectMessage in AndroidPrototypeFactory.java—without these, both your unit tests and runtime serialization will break. Please add registrations for each class in:
- app/src/org/commcare/models/AndroidPrototypeFactory.java
and, following existing conventions, annotate each with the app version they were introduced to aid future migrations.
🤖 Prompt for AI Agents
In app/unit-tests/src/org/commcare/android/tests/processing/FormStorageTest.java
around lines 401-404, the test adds
"org.commcare.android.database.connect.models.PushNotificationRecord",
"org.commcare.android.database.connect.models.ConnectChannel", and
"org.commcare.android.database.connect.models.ConnectMessage" but these classes
are not registered in app/src/org/commcare/models/AndroidPrototypeFactory.java;
open that file and add PrototypeFactory.register entries for each
fully-qualified class name following the existing pattern, placing them with the
correct grouping and adding the same @IntroducedInAppVersion (or equivalent)
annotation used elsewhere with the appropriate app version when these models
were added, then run tests to ensure serialization/deserialization works.
|
@jaypanchal-13 I think we will need relational database this time with Foreign key in PushNotificationRecord for ConnectMessage and ConnectChannelId. Push notification response will be list of JSON only with all key-value but its required to separate out in app and store separately in tables (ConnectMessage and ConnectChannelId) along with main PushNotificationRecord. I am not sure if we can directly store all key-value in same PushNotificationRecord without the need for ConnectMessage and ConnectChannelId, but this will be too cumbersome with large number of data. |
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Left some comments here but I'll also add comments to the design doc, I think we may need to add more to this model to support things like lifetime/expiration and other persistence.
| import java.io.Serializable | ||
|
|
||
| @Table(ConnectChannel.STORAGE_KEY) | ||
| class ConnectChannel : Persisted(), Serializable { |
There was a problem hiding this comment.
I don't think we need this new table, we already have storage for ConnectMessagingChannelRecord for defining messaging channels.
| import java.io.Serializable | ||
|
|
||
| @Table(ConnectMessage.STORAGE_KEY) | ||
| class ConnectMessage : Persisted(), Serializable { |
There was a problem hiding this comment.
I don't think we need this new table, we already have storage for ConnectMessagingMessageRecord for storing messages.
| * V.17 - Added push_notification_history,connect_channel,connect_message table | ||
| */ | ||
| private static final int CONNECT_DB_VERSION = 16; | ||
|
|
There was a problem hiding this comment.
@jaypanchal-13 Oops! Look like a good catch from coderabbit, this value needs to be changed to 17
|
|
||
| @Persisting(7) | ||
| @MetaField(META_OPPORTUNITY_ID) | ||
| var opportunityId: Int = 0 |
There was a problem hiding this comment.
This will need to be nullable so we can handle notifications that aren't specific to an opportunity
|
|
||
| @Persisting(8) | ||
| @MetaField(META_CONNECT_MESSAGE_ID) | ||
| var connectMessageId: Int = 0 |
There was a problem hiding this comment.
I think we should invert this relationship, so sub-classes (like messages, channels) foreign-key to the base class (push notification). There should be fewer null links that way, and we can create more sub-classes in the future without having to add more and more nullable links to the base class.
There was a problem hiding this comment.
@jaypanchal-13 ConnectMessageId can be null depending upon the notification type, we can make it nullable
|
@jaypanchal-13 We will need notification_id in |
| companion object { | ||
| const val STORAGE_KEY = "push_notification_history" | ||
|
|
||
| const val META_NOTIFICATION_ID = "notification_id" | ||
| const val META_ACTION = "action" | ||
| const val META_TITLE = "title" | ||
| const val META_BODY = "body" | ||
| const val META_DATE_TIME = "date_time" | ||
| const val META_CONFIRMATION_STATUS = "confirmation_status" | ||
| const val META_OPPORTUNITY_ID = "opportunity_id" | ||
| const val META_CONNECT_MESSAGE_ID = "connect_message_id" | ||
| const val META_CONNECT_CHANNEL_ID = "connect_channel_id" | ||
| } |
There was a problem hiding this comment.
@jaypanchal-13 Are we going to use these variables outside of this class, if no, it can be simply const val and no need for companion object.
There was a problem hiding this comment.
@Jignesh-dimagi As of now we are using STORAGE_KEY in ConnectDatabaseUpgrader while updating db. For other fields we might need in other classes
|
|
||
| @Persisting(8) | ||
| @MetaField(META_CONNECT_MESSAGE_ID) | ||
| var connectMessageId: Int = 0 |
There was a problem hiding this comment.
@jaypanchal-13 ConnectMessageId can be null depending upon the notification type, we can make it nullable
…626-pn-history-db # Conflicts: # app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java # app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java # app/unit-tests/src/org/commcare/android/tests/processing/FormStorageTest.java
…626-pn-history-db
…626-pn-history-db
…626-pn-history-db # Conflicts: # app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java
…626-pn-history-db
Managed foreign keys Migrated db
…y-db # Conflicts: # app/unit-tests/src/org/commcare/android/tests/processing/FormStorageTest.java
This reverts commit f2977be
…on icon visibility
| import android.content.Context | ||
| import org.commcare.android.database.connect.models.PushNotificationRecord | ||
|
|
||
| class NotificationRecordDatabaseHelper { |
There was a problem hiding this comment.
@jaypanchal-13 please create below methods also
- To fetch the notification from notification_id
- Update the read status for a notification using notification_id
- To append the notification(s) to DB basically, write storage for new notification(s)
| } | ||
|
|
||
| private void upgradeEighteenNineteen(IDatabase db) { | ||
| addTableForNewModel(db, PushNotificationRecord.STORAGE_KEY, new PushNotificationRecord()); |
There was a problem hiding this comment.
Add db.setTransactionSuccessful();
There was a problem hiding this comment.
@Jignesh-dimagi I don't think here it is needed to add table
There was a problem hiding this comment.
@jaypanchal-13 I think it has, latest hotfix was about it only.
There was a problem hiding this comment.
@jaypanchal-13 @Jignesh-dimagi db.setTransactionSuccessful() is already happening inside addTableForNewModel here and is not really needed but wll introduce a bug if we do so as introducing another db.setTransactionSuccessful(); will pre-maturely close any internal SQL transactions that we might be inside. In general if we are calling db.setTransactionSuccessful() we must also be adding relevant db.beginTransaction(); and db.endTransaction(); methods and the hotfix issue was due to us having begin and end but never having the call to setTransactionSuccessful() which resulted into any applied changes being rolled back.
There was a problem hiding this comment.
Oh, my bad, I thought it already have db.beginTransaction(); like in method upgradeSeventeenEighteen and upgradeSixteenSeventeen.
| fun getAllNotifications(context: Context): List<PushNotificationRecord> { | ||
| return getStorage(context).getRecordsForValues(arrayOf(), arrayOf()) |
There was a problem hiding this comment.
@jaypanchal-13 Return type should be List<PushNotificationRecord>?
|
|
||
| @Persisting(1) | ||
| @MetaField(META_NOTIFICATION_ID) | ||
| var notificationId: Int? = null |
There was a problem hiding this comment.
@jaypanchal-13 NotificationId cannot be null, so please make non null
|
|
||
| @Persisting(3) | ||
| @MetaField(META_ACTION) | ||
| var action: String? = null |
There was a problem hiding this comment.
@jaypanchal-13 Please make action also non null
| } | ||
|
|
||
| private void upgradeEighteenNineteen(IDatabase db) { | ||
| addTableForNewModel(db, PushNotificationRecord.STORAGE_KEY, new PushNotificationRecord()); |
There was a problem hiding this comment.
@jaypanchal-13 @Jignesh-dimagi db.setTransactionSuccessful() is already happening inside addTableForNewModel here and is not really needed but wll introduce a bug if we do so as introducing another db.setTransactionSuccessful(); will pre-maturely close any internal SQL transactions that we might be inside. In general if we are calling db.setTransactionSuccessful() we must also be adding relevant db.beginTransaction(); and db.endTransaction(); methods and the hotfix issue was due to us having begin and end but never having the call to setTransactionSuccessful() which resulted into any applied changes being rolled back.
| /** | ||
| * 1. Fetch a notification from notification_id | ||
| */ |
There was a problem hiding this comment.
we should remove the numbering from comments
|
@OrangeAndGreen Waiting for your review |
|
@damagatchi retest this please |
|
@damagatchi retest this please |
jaypanchal-13
dismissed stale reviews from shubham1g5, Jignesh-dimagi, and OrangeAndGreen
via
98aa9cb
Product Description
Ticket -> https://dimagi.atlassian.net/browse/CCCT-1626
Created push_notification_history table and managed new notification icon visibility
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review