Jignesh/feat/ccct 1718 #3373
Jignesh/feat/ccct 1718 #3373
Conversation
📝 WalkthroughWalkthroughThis PR extends push notification handling by introducing conversion utilities and new constructors for working with notification payloads. Seven new constants are added to ConnectConstants for notification data fields. Helper methods convertPNRecordsToPayload and convertPNRecordToPayload are introduced to transform PushNotificationRecord objects into payload maps. PNApiSyncWorkerManager gains two new constructor overloads accepting notification payloads and records. FirebaseMessagingUtil gains a new getIntentForPNClick method that checks login status, converts PushNotificationRecord objects to payloads, and returns corresponding intents. PushNotificationViewModel initializes PNApiSyncWorkerManager after fetching notifications. PushNotificationActivity is updated to use getIntentForPNClick when handling notification clicks. The activity layout receives minor adjustments with RecyclerView padding and margin. Sequence DiagramsequenceDiagram
participant User
participant PushNotificationActivity
participant FirebaseMessagingUtil
participant AuthCheck as Login Check
participant PayloadConversion as Payload Conversion
participant IntentHandler
User->>PushNotificationActivity: Click notification in list
PushNotificationActivity->>FirebaseMessagingUtil: getIntentForPNClick(context, record)
FirebaseMessagingUtil->>AuthCheck: cccCheckPassed(context)
alt Login passed
AuthCheck-->>FirebaseMessagingUtil: true
FirebaseMessagingUtil->>PayloadConversion: convertPNRecordToPayload(record)
PayloadConversion-->>FirebaseMessagingUtil: payload map
FirebaseMessagingUtil->>IntentHandler: handleNotification(payload, false)
IntentHandler-->>FirebaseMessagingUtil: intent
FirebaseMessagingUtil-->>PushNotificationActivity: intent
PushNotificationActivity->>User: Start activity
else Login failed
AuthCheck-->>FirebaseMessagingUtil: false
FirebaseMessagingUtil-->>PushNotificationActivity: null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes span multiple files with mixed concerns including new utility methods for payload conversion, constructor overloads, and integration points in the view model and activity. While individual changes are straightforward, the interconnected nature of the notification flow and payload transformation requires careful review of the conversion logic and the interaction between components. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ 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: 1
🧹 Nitpick comments (4)
app/src/org/commcare/activities/PushNotificationActivity.kt (1)
70-74: Minor style: Add space afterifkeyword.The logic correctly checks for null before starting the activity, which is appropriate given that
getIntentForPNClickreturns null when the CCC login check fails.Apply this diff for consistent Kotlin style:
- if(activityIntent!=null) { + if (activityIntent != null) {app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
502-504: Consider breaking the ternary for readability.The method logic is sound—it checks CCC login status, converts the notification record to a payload, and delegates to
handleNotification. However, the ternary operator is quite long (exceeds typical line length limits).Apply this diff to improve readability:
public static Intent getIntentForPNClick(Context context, PushNotificationRecord pushNotificationRecord){ - return cccCheckPassed(context) ? handleNotification(context,PushNotificationApiHelper.INSTANCE.convertPNRecordToPayload(pushNotificationRecord),null ,false): null; + if (!cccCheckPassed(context)) { + return null; + } + return handleNotification(context, + PushNotificationApiHelper.INSTANCE.convertPNRecordToPayload(pushNotificationRecord), + null, + false); }app/src/org/commcare/utils/PushNotificationApiHelper.kt (2)
84-92: Simplify list mapping with idiomatic Kotlin.The logic is correct, but the mapping can be more concise.
Apply this diff:
fun convertPNRecordsToPayload(pnsRecords:List<PushNotificationRecord>?): ArrayList<Map<String,String>>{ - val pns = ArrayList<Map<String,String>>() - pnsRecords?.let { - it.map {pnRecord -> - pns.add(convertPNRecordToPayload(pnRecord)) - } - } - return pns + return pnsRecords?.mapTo(ArrayList()) { convertPNRecordToPayload(it) } + ?: ArrayList() }
94-107: Consider using string interpolation and explicit null handling.The conversion logic covers all required fields, but there are several areas for improvement:
- String concatenation: Use
.toString()or string templates instead of"" + value- Date formatting:
Date.toString()produces a locale-dependent format that may not be suitable for API payloads or persistence- Empty string handling: String concatenation with empty strings like
""+pnRecord.opportunityIdwill produce "null" string literals when the field is nullConsider this refactor:
fun convertPNRecordToPayload(pnRecord: PushNotificationRecord): HashMap<String,String> { val pn = HashMap<String,String>() pn.put(REDIRECT_ACTION,pnRecord.action) pn.put(NOTIFICATION_TITLE,pnRecord.title) pn.put(NOTIFICATION_BODY,pnRecord.body) - pn.put(NOTIFICATION_ID,""+pnRecord.notificationId) - pn.put(NOTIFICATION_TIME_STAMP,pnRecord.createdDate.toString()) + pn.put(NOTIFICATION_ID, pnRecord.notificationId) + pn.put(NOTIFICATION_TIME_STAMP, pnRecord.createdDate.time.toString()) // Use epoch millis pn.put(NOTIFICATION_STATUS,pnRecord.confirmationStatus) - pn.put(NOTIFICATION_MESSAGE_ID,""+pnRecord.connectMessageId) - pn.put(NOTIFICATION_CHANNEL_ID,""+pnRecord.channel) - pn.put(OPPORTUNITY_ID,""+pnRecord.opportunityId) - pn.put(PAYMENT_ID,""+pnRecord.paymentId) + pn.put(NOTIFICATION_MESSAGE_ID, pnRecord.connectMessageId) + pn.put(NOTIFICATION_CHANNEL_ID, pnRecord.channel) + pn.put(OPPORTUNITY_ID, pnRecord.opportunityId) + pn.put(PAYMENT_ID, pnRecord.paymentId) return pn }If these string fields can actually be null, add explicit null handling:
pn.put(OPPORTUNITY_ID, pnRecord.opportunityId ?: "")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/res/layout/activity_push_notification.xml(1 hunks)app/src/org/commcare/activities/PushNotificationActivity.kt(2 hunks)app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt(2 hunks)app/src/org/commcare/connect/ConnectConstants.java(1 hunks)app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt(3 hunks)app/src/org/commcare/utils/FirebaseMessagingUtil.java(2 hunks)app/src/org/commcare/utils/PushNotificationApiHelper.kt(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/org/commcare/utils/FirebaseMessagingUtil.java (3)
app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt (1)
PushNotificationRecord(12-117)app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt (1)
context(31-178)app/src/org/commcare/utils/PushNotificationApiHelper.kt (1)
convertPNRecordToPayload(94-107)
app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt (1)
app/src/org/commcare/utils/PushNotificationApiHelper.kt (1)
convertPNRecordsToPayload(84-92)
⏰ 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 (3)
app/res/layout/activity_push_notification.xml (1)
15-21: LGTM! Appropriate UI spacing adjustments.The addition of
paddingTop="?attr/actionBarSize"ensures the RecyclerView content clears the action bar, and the bottom margin provides appropriate spacing. These are sensible UI refinements.app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt (1)
60-63: LGTM! Constructor correctly delegates to conversion helper.The new constructor properly converts
PushNotificationRecordobjects to the internal payload format usingconvertPNRecordsToPayload. The helper handles null input gracefully by returning an empty list, which will result in no work requests being created (acceptable behavior).app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt (1)
42-46: No issues found. The implementation correctly uses Android WorkManager architecture.The code properly follows the fire-and-forget pattern for background sync:
Error handling is present: The
PNApiSyncWorkerhandles failures with built-in retry logic (MAX_RETRIES = 3) and explicit Result states (Result.retry(),Result.failure()).Async design is intentional: Using WorkManager to enqueue background jobs is the correct Android pattern. The sync should not block the UI.
UI updates are appropriate: Updating the UI immediately after enqueueing the sync (lines 47-49) is the expected behavior—the UI reflects notification state without waiting for the background sync to complete.
The return value (whether signaling work was queued) is ignored at the call site, which is acceptable in this architecture since WorkManager handles execution and failure states independently.
| public final static String NOTIFICATION_TITLE = "title"; | ||
| public final static String NOTIFICATION_BODY = "body"; | ||
| public final static String NOTIFICATION_ID = "notification_id"; | ||
| public final static String NOTIFICATION_TIME_STAMP = "timestamp"; | ||
| public final static String NOTIFICATION_STATUS = "status"; | ||
| public final static String NOTIFICATION_MESSAGE_ID = "message_id"; | ||
| public final static String NOTIFICATION_CHANNEL_ID = "channel"; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify no conflicts with existing NOTIFICATION_TITLE and NOTIFICATION_BODY.
The new constants are well-named and appropriately defined. However, note that NOTIFICATION_TITLE and NOTIFICATION_BODY are also defined in FCMMessageData (as seen in the imports). Ensure there are no conflicts or ambiguities when both are in scope.
🏁 Script executed:
#!/bin/bash
# Search for other definitions of NOTIFICATION_TITLE and NOTIFICATION_BODY
rg -nP --type=java --type=kt 'NOTIFICATION_TITLE\s*=' -C2
rg -nP --type=java --type=kt 'NOTIFICATION_BODY\s*=' -C2
# Check if both constants are used in the same files
rg -nP --type=java --type=kt -l 'ConnectConstants\.NOTIFICATION_(TITLE|BODY)' | xargs -I {} sh -c 'echo "File: {}"; rg -n "FCMMessageData\.NOTIFICATION_(TITLE|BODY)" {}'Length of output: 80
I'll adjust the search strategy to work with the available tools. Let me run improved shell scripts without the unsupported Kotlin file type:
🏁 Script executed:
#!/bin/bash
# Search for NOTIFICATION_TITLE and NOTIFICATION_BODY definitions in Java files only
echo "=== Searching for NOTIFICATION_TITLE definitions ==="
rg -nP --type=java 'NOTIFICATION_TITLE\s*=' -C2
echo ""
echo "=== Searching for NOTIFICATION_BODY definitions ==="
rg -nP --type=java 'NOTIFICATION_BODY\s*=' -C2Length of output: 2097
🏁 Script executed:
#!/bin/bash
# Look for FCMMessageData class and its constant definitions
echo "=== Searching for FCMMessageData class ==="
rg -nP --type=java 'class FCMMessageData' -C5
echo ""
echo "=== Searching for all NOTIFICATION constants in FCMMessageData ==="
rg -nP --type=java 'FCMMessageData' -C3 | head -50Length of output: 5982
🏁 Script executed:
#!/bin/bash
# Search for any usage of ConnectConstants.NOTIFICATION_TITLE or ConnectConstants.NOTIFICATION_BODY
echo "=== Usage of ConnectConstants.NOTIFICATION_TITLE ==="
rg -n 'ConnectConstants\.NOTIFICATION_TITLE' --type=java
echo ""
echo "=== Usage of ConnectConstants.NOTIFICATION_BODY ==="
rg -n 'ConnectConstants\.NOTIFICATION_BODY' --type=java
echo ""
echo "=== Checking for FCMMessageData usage patterns ==="
rg -n 'FCMMessageData\.NOTIFICATION' --type=javaLength of output: 412
Remove duplicate constants or consolidate with FCMMessageData—identical NOTIFICATION_TITLE and NOTIFICATION_BODY already exist.
Both NOTIFICATION_TITLE and NOTIFICATION_BODY are already defined in FCMMessageData with identical values ("title" and "body"). The codebase currently imports and uses these from FCMMessageData (as seen in FirebaseMessagingUtil.java lines 60-61), not from ConnectConstants. Adding duplicates violates DRY principles and risks inconsistency. Either remove these constants from ConnectConstants or consolidate all notification constants into a single location.
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/ConnectConstants.java around lines 49 to 55,
remove the duplicate NOTIFICATION_TITLE and NOTIFICATION_BODY constants (they
are already defined in FCMMessageData) or consolidate these notification keys
into a single constants class: delete or comment out NOTIFICATION_TITLE and
NOTIFICATION_BODY from ConnectConstants and update any references to import and
use FCMMessageData.* instead, leaving the remaining unique constants in
ConnectConstants intact to avoid duplication and maintain a single source of
truth.
There was a problem hiding this comment.
@Jignesh-dimagi given they are the same payload sent in 2 different places, should we be defining constraints only once as well ?
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!
There was a problem hiding this comment.
+1, changed to ConnectConstants
| } | ||
|
|
||
| public static Intent getIntentForPNClick(Context context, PushNotificationRecord pushNotificationRecord){ | ||
| return cccCheckPassed(context) ? handleNotification(context,PushNotificationApiHelper.INSTANCE.convertPNRecordToPayload(pushNotificationRecord),null ,false): null; |
There was a problem hiding this comment.
nit: rename handleNotification to getIntentForNotification to make it more clear that it's returning an intent that should be handled by caller.
There was a problem hiding this comment.
handleNotification is used by FCM for handling the notification depending upon the payload. It is also used when user clicks on PN either from tray or item of notification list. May be we can name as handleAndGetIntentForNotification ?
There was a problem hiding this comment.
think fine to leave it as it is in that case.
Product Description
https://dimagi.atlassian.net/browse/CCCT-1629
https://dimagi.atlassian.net/browse/CCCT-1718