Skip to content

Notifications Parser refactor and tests #3455

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shubham1g5 merged 12 commits into from
Dec 10, 2025
Merged

Notifications Parser refactor and tests #3455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6039427
refactor notifications parser to maintain separation of concerns
shubham1g5 Dec 7, 2025
c80f698
Further optimize code to parse notifications and messages in a single…
shubham1g5 Dec 8, 2025
cb09195
tests for notification parsing(written with claude)
shubham1g5 Dec 8, 2025
a047d89
pass message notifications Ids for acknowledgement, clean up un-ncess…
shubham1g5 Dec 8, 2025
1996e7f
Only return notifications saved into DB
shubham1g5 Dec 8, 2025
844e842
adds a test with complete message encryption testing
shubham1g5 Dec 10, 2025
de419b2
export test data constructors to a util class
shubham1g5 Dec 10, 2025
3c0c345
use assertFalse
shubham1g5 Dec 10, 2025
b0f7303
remove service as it's just another helper
shubham1g5 Dec 10, 2025
ef9714b
ktlint
shubham1g5 Dec 10, 2025
c1b156b
ktlint
shubham1g5 Dec 10, 2025
78932ff
ktlint
shubham1g5 Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion app/src/org/commcare/android/database/connect/models/ConnectMessagingMessageRecord.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,15 @@ public ConnectMessagingMessageRecord() {
@MetaField(META_MESSAGE_USER_VIEWED)
private boolean userViewed;

/**
* Creates a decrypted message record from encrypted JSON payload by using the channel key
*
* @param json JSON data to parse
* @param channels List of channels from the Database with keys for decryption
* @return ConnectMessagingMessageRecord or null if decryption fails or channel not found
* @throws JSONException
* @throws ParseException
*/
public static ConnectMessagingMessageRecord fromJson(JSONObject json, List<ConnectMessagingChannelRecord> channels) throws JSONException, ParseException{
ConnectMessagingMessageRecord connectMessagingMessageRecord = new ConnectMessagingMessageRecord();

Expand Down Expand Up @@ -236,4 +245,4 @@ public boolean getUserViewed() {
public void setUserViewed(boolean userViewed) {
this.userViewed = userViewed;
}
}
}
42 changes: 16 additions & 26 deletions app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import org.commcare.models.framework.Persisting
import org.commcare.modern.database.Table
import org.commcare.modern.models.MetaField
import org.javarosa.core.model.utils.DateUtils
import org.json.JSONArray
import org.json.JSONObject
import java.io.Serializable
import java.util.Date

Expand Down Expand Up @@ -84,31 +84,21 @@ class PushNotificationRecord :

const val META_TIME_STAMP = "timestamp"

fun fromJsonArray(jsonArray: JSONArray): List<PushNotificationRecord> {
val records = mutableListOf<PushNotificationRecord>()

for (i in 0 until jsonArray.length()) {
val obj = jsonArray.getJSONObject(i)
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, "")
}
records.add(record)
fun fromJson(obj: JSONObject): PushNotificationRecord =
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)
Comment on lines +96 to +97
Copy link

@coderabbitai coderabbitai bot Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.

connectMessageId = obj.optString(META_MESSAGE_ID, "")
channel = obj.optString(META_CHANNEL, "")
action = obj.optString(META_ACTION, "")
opportunityId = obj.optString(META_OPPORTUNITY_ID, "")
}

return records
}
}
}
9 changes: 4 additions & 5 deletions app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package org.commcare.connect.network.connectId;

import static org.commcare.connect.network.NetworkUtils.getErrorCodes;

import android.app.Activity;
import android.content.Context;

Expand All @@ -10,6 +12,7 @@
import org.commcare.connect.network.NoParsingResponseParser;
import org.commcare.connect.network.base.BaseApiCallback;
import org.commcare.connect.network.base.BaseApiHandler;
import org.commcare.connect.network.base.BaseApiResponseParser;
import org.commcare.connect.network.connectId.parser.AddOrVerifyNameParser;
import org.commcare.connect.network.connectId.parser.CompleteProfileResponseParser;
import org.commcare.connect.network.connectId.parser.ConfirmBackupCodeResponseParser;
Expand All @@ -33,8 +36,6 @@

import kotlin.Pair;

import static org.commcare.connect.network.NetworkUtils.getErrorCodes;

public abstract class PersonalIdApiHandler<T> extends BaseApiHandler<T> {


Expand Down Expand Up @@ -299,14 +300,12 @@ public void heartbeatRequest(Context context, ConnectUserRecord user) {
);
}


public void retrieveNotifications(Context context, ConnectUserRecord user) {
ApiPersonalId.retrieveNotifications(
context,
user.getUserId(),
user.getPassword(),
createCallback(new RetrieveNotificationsResponseParser<>(context), null)
);
createCallback((BaseApiResponseParser<T>) new RetrieveNotificationsResponseParser(context), null));
Copy link
Contributor

@Jignesh-dimagi Jignesh-dimagi Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shubham1g5 Why data type casting is required here?

Copy link
Contributor Author

@shubham1g5 shubham1g5 Dec 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's required due to the change in RetrieveNotificationsResponseParser definition to extend from BaseApiResponseParser<NotificationParseResult> instead of BaseApiResponseParser<T>

Copy link
Contributor

@Jignesh-dimagi Jignesh-dimagi Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can still keep BaseApiResponseParser<T> and return NotificationParseResult as T?

Copy link
Contributor Author

@shubham1g5 shubham1g5 Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong thoughts on my end, Do you have a sense of why one is better than other ?

Copy link
Contributor Author

@shubham1g5 shubham1g5 Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we need to cast over here or as done in this PR.

}

public void updateNotifications(
Expand Down
16 changes: 16 additions & 0 deletions app/src/org/commcare/connect/network/connectId/parser/NotificationParseResult.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package org.commcare.connect.network.connectId.parser

import org.commcare.android.database.connect.models.ConnectMessagingChannelRecord
import org.commcare.android.database.connect.models.ConnectMessagingMessageRecord
import org.commcare.android.database.connect.models.PushNotificationRecord

/**
* Data class to hold the result of parsing notification response
* Contains three separate, exclusive entities: notifications, channels, and messages
*/
data class NotificationParseResult(
val nonMessagingNotifications: List<PushNotificationRecord>,
val channels: List<ConnectMessagingChannelRecord>,
val messages: List<ConnectMessagingMessageRecord>,
val messagingNotificationIds: List<String>,
)
110 changes: 53 additions & 57 deletions app/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParser.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,109 +4,105 @@ import android.content.Context
import org.commcare.android.database.connect.models.ConnectMessagingChannelRecord
import org.commcare.android.database.connect.models.ConnectMessagingMessageRecord
import org.commcare.android.database.connect.models.PushNotificationRecord
import org.commcare.android.database.connect.models.PushNotificationRecord.Companion.META_NOTIFICATION_ID
import org.commcare.connect.database.ConnectMessagingDatabaseHelper
import org.commcare.connect.network.base.BaseApiResponseParser
import org.json.JSONArray
import org.json.JSONObject
import java.io.InputStream

/**
* Parser for retrieving notification response
* Parser for retrieve_notification API endpoint
* Parses JSON response into separate notifications, channels, and messages
*/
class RetrieveNotificationsResponseParser<T>(
val context: Context,
) : BaseApiResponseParser<T> {
val channels: MutableList<ConnectMessagingChannelRecord> = ArrayList()
val messages: MutableList<ConnectMessagingMessageRecord?> = ArrayList()
var notificationsJsonArray: JSONArray? = null
class RetrieveNotificationsResponseParser(
private val context: Context,
) : BaseApiResponseParser<NotificationParseResult> {
private var notificationsJsonArray: JSONArray = JSONArray()

override fun parse(
responseCode: Int,
responseData: InputStream,
anyInputObject: Any?,
): T {
): NotificationParseResult {
val jsonText = responseData.bufferedReader().use { it.readText() }
val responseJsonObject = JSONObject(jsonText)
parseNotifications(responseJsonObject)
parseChannel(responseJsonObject)
parseMessages()
return PushNotificationRecord.fromJsonArray(notificationsJsonArray ?: JSONArray()) as T
}

private fun parseNotifications(responseJsonObject: JSONObject) {
if (responseJsonObject.has("notifications")) {
notificationsJsonArray = responseJsonObject.getJSONArray("notifications")
}

val channels = parseChannels(responseJsonObject)
val (nonMessageNotifications, messages, messagesNotificationsIds) = parseAndSeparateNotifications()

return NotificationParseResult(
nonMessageNotifications,
channels,
messages,
messagesNotificationsIds,
)
}

private fun parseChannel(responseJsonObject: JSONObject) {
private fun parseChannels(responseJsonObject: JSONObject): MutableList<ConnectMessagingChannelRecord> {
val channels: MutableList<ConnectMessagingChannelRecord> = ArrayList()
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)
}
ConnectMessagingDatabaseHelper.storeMessagingChannels(context, channels, true)
for (channel in channels) {
// if there is no key for channel, remove that messages for PN so that it can be retrieved back again by calling API
if (channel.getConsented() && channel.getKey().length == 0) {
excludeMessagesForChannel(channel.channelId)
}
}
}
return channels
}

private fun parseMessages() {
notificationsJsonArray?.let {
/**
* Parses and separates notifications into two categories in a single pass:
* - Non-messaging notifications as PushNotificationRecord
* - Messaging notifications as ConnectMessagingMessageRecord
* This avoids double parsing and eliminates filtering overhead
*/
private fun parseAndSeparateNotifications(): Triple<
MutableList<PushNotificationRecord>,
MutableList<ConnectMessagingMessageRecord>,
MutableList<String>,
> {
val nonMessageNotifications = mutableListOf<PushNotificationRecord>()
val messages = mutableListOf<ConnectMessagingMessageRecord>()
val messagesNotificationsIds = mutableListOf<String>()

notificationsJsonArray.let { jsonArray ->
// Get existing channels from database - these have the encryption keys required for message decryption
val existingChannels = ConnectMessagingDatabaseHelper.getMessagingChannels(context)
for (notificationJsonIndex in 0 until notificationsJsonArray!!.length()) {
val notificationJsonObject =
notificationsJsonArray!!.getJSONObject(notificationJsonIndex)
for (notificationIndex in 0 until jsonArray.length()) {
val notificationJsonObject = jsonArray.getJSONObject(notificationIndex)

if (isNotificationMessageType(notificationJsonObject)) {
// Handle messaging notifications - parse as ConnectMessagingMessageRecord
val message =
ConnectMessagingMessageRecord.fromJson(
notificationJsonObject,
existingChannels,
)
if (message != null) {
messages.add(message)
messagesNotificationsIds.add(notificationJsonObject.getString(META_NOTIFICATION_ID))
}
} else {
// Handle non-messaging notifications
val notification = PushNotificationRecord.fromJson(notificationJsonObject)
nonMessageNotifications.add(notification)
}
}
ConnectMessagingDatabaseHelper.storeMessagingMessages(context, messages, false)
}
}

private fun excludeMessagesForChannel(channelId: String) {
val newNotificationsJsonArray = JSONArray()
notificationsJsonArray?.let {
for (notificationJsonIndex in 0 until it.length()) {
val notificationJsonObject =
notificationsJsonArray!!.getJSONObject(notificationJsonIndex)
if (!shouldRemoveChannel(notificationJsonObject, channelId)) {
newNotificationsJsonArray.put(notificationJsonObject)
}
}
}
notificationsJsonArray = newNotificationsJsonArray
return Triple(nonMessageNotifications, messages, messagesNotificationsIds)
}

private fun shouldRemoveChannel(
notificationJsonObject: JSONObject,
channelId: String,
): Boolean =
isNotificationMessageType(notificationJsonObject) && notificationJsonObject
.get("channel") != null &&
channelId.equals(
notificationJsonObject.get("channel"),
)

private fun isNotificationMessageType(notificationJsonObject: JSONObject) =
/**
* Checks if a notification JSON object is of messaging type
*/
private fun isNotificationMessageType(notificationJsonObject: JSONObject): Boolean =
notificationJsonObject.has("notification_type") &&
"MESSAGING".equals(
notificationJsonObject.get(
"notification_type",
),
)
"MESSAGING" == notificationJsonObject.getString("notification_type")
}
Loading