Connect message review -1 #3145
Conversation
Made changes to handle notification data when no channel in db
…CareFirebaseMessagingService and launcher activity
📝 WalkthroughWalkthroughThis set of changes refactors the handling of Firebase Cloud Messaging (FCM) notifications and CommCare Connect messaging within the application. The Sequence Diagram(s)sequenceDiagram
participant FCM as Firebase Cloud Messaging
participant Service as CommCareFirebaseMessagingService
participant Util as FirebaseMessagingUtil
participant UI as App UI / Activities
FCM->>Service: onMessageReceived(RemoteMessage)
Service->>Util: handleNotification(context, payload, showNotification)
alt CCC Action
Util->>Util: handleCCCActionPushNotification(...)
alt Message Channel
Util->>Util: handleCCCMessageChannelPushNotification(...)
Util->>UI: Broadcast update or show notification
else Payment
Util->>Util: handleCCCPaymentPushNotification(...)
Util->>UI: Show notification with actions
else Generic Connect
Util->>Util: handleCCCConnectPushNotification(...)
Util->>UI: Show connect notification
end
else General App Notification
Util->>Util: handleGeneralApplicationPushNotification(...)
Util->>UI: Show app notification
else Sync Action
Util->>Util: FirebaseMessagingDataSyncer.sync(...)
end
sequenceDiagram
participant User as User
participant ConnectMessagingActivity as ConnectMessagingActivity
participant DB as Local Database
participant Server as Server
User->>ConnectMessagingActivity: Launch with intent (action, channelId)
ConnectMessagingActivity->>ConnectMessagingActivity: handleRedirect(action)
alt ChannelId empty
ConnectMessagingActivity->>ConnectMessagingActivity: show failure message
else ChannelId present
ConnectMessagingActivity->>DB: Query channel by channelId
alt Channel not found
ConnectMessagingActivity->>Server: Retrieve channels/messages
Server-->>ConnectMessagingActivity: Response
ConnectMessagingActivity->>DB: Retry query
else Channel found
alt Consent missing
ConnectMessagingActivity->>ConnectMessagingActivity: Show consent prompt
else Consent given
alt Encryption key missing
ConnectMessagingActivity->>Server: Retrieve encryption key (async)
Server-->>ConnectMessagingActivity: Key received
end
ConnectMessagingActivity->>ConnectMessagingActivity: Navigate to message fragment
end
end
end
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
📝 WalkthroughWalkthroughThis set of changes refactors the notification and messaging handling for the Connect messaging feature in the application. The notification processing logic is moved from Sequence Diagram(s)sequenceDiagram
participant FCM as Firebase Cloud Messaging
participant Service as CommCareFirebaseMessagingService
participant Util as FirebaseMessagingUtil
participant Activity as ConnectMessagingActivity
participant Fragment as ConnectMessageChannelListFragment
FCM->>Service: Delivers FCM message
Service->>Util: handleNotification(context, payload, showNotification)
Util->>Util: Parse payload, determine action type
alt CCC message action
Util->>Activity: Launch or update ConnectMessagingActivity with channel ID
Activity->>Activity: Validate channel ID and consent
Activity->>Activity: If missing, fetch channel/encryption key asynchronously
Activity->>Fragment: Navigate to appropriate fragment (consent, message, or list)
else Payment action
Util->>Activity: Launch ConnectMessagingActivity with payment info
else General notification
Util->>Activity: Launch main app activity
end
Util->>Service: Optionally show notification or broadcast update
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (14)
🧰 Additional context used🧠 Learnings (3)📓 Common learningsapp/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java (1)app/src/org/commcare/activities/connect/ConnectMessagingActivity.java (1)🧬 Code Graph Analysis (4)app/src/org/commcare/activities/connect/ConnectActivity.java (1)
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java (1)
app/src/org/commcare/services/PaymentAcknowledgeReceiver.java (1)
app/src/org/commcare/utils/FirebaseMessagingUtil.java (9)
🔇 Additional comments (19)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 5
🔭 Outside diff range comments (1)
app/src/org/commcare/services/FCMMessageData.java (1)
138-143:⚠️ Potential issueFix potential NullPointerException in writeExternal.
The
actionfield could be null if not present in the payload, causing a NullPointerException when callingtoString().Apply this diff to fix the issue:
@Override public void writeExternal(DataOutputStream out) throws IOException { - ExtUtil.writeString(out, action.toString()); + ExtUtil.writeString(out, actionType.toString()); ExtUtil.writeString(out, username); ExtUtil.writeString(out, domain); ExtUtil.writeNumeric(out, creationTime.getMillis()); }
🧹 Nitpick comments (4)
app/AndroidManifest.xml (1)
148-150: Validate the necessity and scope ofandroid:exported="true"
Explicitly exportingConnectMessagingActivityopens it to external apps, which may introduce unintended security exposure if not required. Confirm that external invocation (e.g., via push notifications or deep links) justifies this change.If you don’t need external apps to launch this activity, consider reverting to
exported="false"or gating it behind a permission:- <activity - android:name="org.commcare.activities.connect.ConnectMessagingActivity" - android:exported="true"> + <activity + android:name="org.commcare.activities.connect.ConnectMessagingActivity" + android:exported="false">app/res/values/strings.xml (1)
557-558: Ensure translation coverage and naming consistency
The new stringconnect_messaging_pn_wrong_channelis user-facing. Please:
- Verify it’s included in all locale files for translation.
- Consider renaming the key to align with existing channel-related patterns (e.g.,
connect_messaging_channel_wrong_fallback) for clarity.app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
36-37: Remove unused import.The
CCC_MESSAGEconstant import appears to be unused in this file after the refactoring. Consider removing it to keep the imports clean.-import static org.commcare.connect.ConnectConstants.CCC_MESSAGE; -app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
275-277: Track the TODO for backend changes.The TODO comment indicates temporary code that should be removed once the backend starts sending channel names in new channel push notifications.
Would you like me to create a GitHub issue to track this technical debt and ensure it's addressed when the backend changes are implemented?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/AndroidManifest.xml(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/activities/connect/ConnectActivity.java(2 hunks)app/src/org/commcare/activities/connect/ConnectMessagingActivity.java(3 hunks)app/src/org/commcare/adapters/ChannelAdapter.java(2 hunks)app/src/org/commcare/connect/ConnectConstants.java(1 hunks)app/src/org/commcare/connect/MessageManager.java(1 hunks)app/src/org/commcare/connect/network/ApiPersonalId.java(1 hunks)app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java(5 hunks)app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java(2 hunks)app/src/org/commcare/services/CommCareFirebaseMessagingService.java(2 hunks)app/src/org/commcare/services/FCMMessageData.java(4 hunks)app/src/org/commcare/services/PaymentAcknowledgeReceiver.java(2 hunks)app/src/org/commcare/utils/FirebaseMessagingUtil.java(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java (1)
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3093
File: app/res/navigation/nav_graph_connect_messaging.xml:41-45
Timestamp: 2025-05-09T10:57:41.073Z
Learning: In the CommCare Android codebase, the navigation graph for Connect messaging (`nav_graph_connect_messaging.xml`) intentionally uses `channel_id` as the argument name in the connectMessageFragment, despite using `channelId` in other parts of the same navigation graph. This naming difference is by design in the refactored code.
app/src/org/commcare/activities/connect/ConnectMessagingActivity.java (2)
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3093
File: app/res/navigation/nav_graph_connect_messaging.xml:41-45
Timestamp: 2025-05-09T10:57:41.073Z
Learning: In the CommCare Android codebase, the navigation graph for Connect messaging (`nav_graph_connect_messaging.xml`) intentionally uses `channel_id` as the argument name in the connectMessageFragment, despite using `channelId` in other parts of the same navigation graph. This naming difference is by design in the refactored code.
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.
🧬 Code Graph Analysis (5)
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java (1)
app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
FirebaseMessagingUtil(45-387)
app/src/org/commcare/services/PaymentAcknowledgeReceiver.java (1)
app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
FirebaseMessagingUtil(45-387)
app/src/org/commcare/activities/connect/ConnectActivity.java (1)
app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
FirebaseMessagingUtil(45-387)
app/src/org/commcare/connect/network/ApiPersonalId.java (1)
app/src/org/commcare/connect/workers/ConnectHeartbeatWorker.kt (1)
context(12-28)
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (2)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-61)app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
FirebaseMessagingUtil(45-387)
🔇 Additional comments (18)
app/src/org/commcare/connect/ConnectConstants.java (1)
30-32: Approve newCCC_MESSAGEconstant
AddingCCC_MESSAGEcentralizes the action type for CommCare Connect messaging notifications, matching the refactored flow inFirebaseMessagingUtil.app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java (2)
29-31: Approve import ofFirebaseMessagingUtil
Replacing the old service import withFirebaseMessagingUtilaligns this fragment with the new centralized messaging utility.
76-77: Approve use of centralized broadcast constant
Switching toFirebaseMessagingUtil.MESSAGING_UPDATE_BROADCASTensures the fragment listens to the unified messaging update broadcast.app/src/org/commcare/activities/connect/ConnectActivity.java (2)
34-34: Approve import ofFirebaseMessagingUtil
IntroducingFirebaseMessagingUtilhere harmonizes this activity with the refactored broadcast constant.
110-112: Approve broadcast registration update
Updating theIntentFilterto useFirebaseMessagingUtil.MESSAGING_UPDATE_BROADCASTcorrectly subscribes the activity to messaging updates from the new utility.app/src/org/commcare/services/PaymentAcknowledgeReceiver.java (1)
11-11: LGTM! Proper constant migration.The constants have been correctly migrated from
CommCareFirebaseMessagingServicetoFirebaseMessagingUtil, which aligns with the broader refactoring effort to centralize notification handling. The functionality remains unchanged.Also applies to: 27-29
app/src/org/commcare/adapters/ChannelAdapter.java (1)
47-47: Good refactor! Removed redundant parameter.The
ItemChannelBindingparameter has been correctly removed from thebindmethod since theChannelViewHolderalready has access to the binding via itsbindingfield. This improves code clarity and eliminates unnecessary parameter passing.Also applies to: 64-64
app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java (3)
75-83: Proper lifecycle handling improvement.Moving the channel ID argument processing from
onCreateViewtoonViewCreatedis the correct approach, as the view hierarchy is fully constructed at this point. Using the staticCHANNEL_IDconstant also improves consistency.
91-91: Correct broadcast receiver update.The broadcast intent filter has been properly updated to use
FirebaseMessagingUtil.MESSAGING_UPDATE_BROADCAST, which aligns with the centralization of messaging constants in theFirebaseMessagingUtilclass.
118-118: Safer navigation approach.Using
requireView()instead ofgetView()is a good improvement as it ensures the view is available and throws a clear exception if it's not, preventing potential null pointer issues.app/src/org/commcare/connect/MessageManager.java (1)
185-185: Good API clarity improvement.The method call has been correctly updated to use
handleReceivedChannelEncryptionKey, which is more specific and clear about the type of encryption key being handled compared to the previous generichandleReceivedEncryptionKey.app/src/org/commcare/connect/network/ApiPersonalId.java (1)
458-458: LGTM! Method rename improves clarity.The rename from
handleReceivedEncryptionKeytohandleReceivedChannelEncryptionKeybetter reflects the method's specific purpose of handling channel encryption keys, which improves code readability and maintainability.app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
55-59: Excellent refactoring to centralize notification handling!The delegation to
FirebaseMessagingUtil.handleNotification()properly separates concerns and improves maintainability by centralizing all notification processing logic in a single utility class.app/src/org/commcare/activities/connect/ConnectMessagingActivity.java (3)
63-187: Excellent refactoring with improved modularity and error handling!The refactoring successfully:
- Modularizes channel validation logic into focused helper methods
- Implements proper asynchronous encryption key retrieval
- Provides clear user feedback through Toast messages
- Maintains consistent navigation patterns with back stack management
The code is now more maintainable and follows single responsibility principles.
24-24:Details
✅ Verification successful
Verify navigation argument consistency.
The constant
CHANNEL_ID = "channel_id"should match the navigation argument name used in the navigation graph. According to the retrieved learnings, the navigation graph useschannel_idforconnectMessageFragment.Run the following script to verify the navigation argument name consistency:
🏁 Script executed:
#!/bin/bash # Description: Verify that the CHANNEL_ID constant matches the navigation argument in the navigation graph # Search for channel_id usage in navigation XML files fd -e xml -x grep -l "channel_id\|channelId" {} \; | xargs grep -A 2 -B 2 'argument.*channel'Length of output: 98
The previous search didn’t yield any matches—let’s try a more targeted lookup in the Connect messaging nav graph:
🏁 Script executed:
#!/bin/bash # Locate the connect messaging navigation graph and list its <argument> entries for channel_id/channelId file=$(fd -e xml 'nav_graph_connect_messaging.xml') echo "Inspecting $file" grep -n '<argument' "$file" | grep -E 'channel_id|channelId'Length of output: 341
Let’s explicitly look for the attribute on its own line:
🏁 Script executed:
#!/bin/bash file="app/res/navigation/nav_graph_connect_messaging.xml" echo "Searching for literal “channel_id” in $file" grep -R '\"channel_id\"' -n "$file" || echo "No channel_id attribute found" echo echo "Searching for literal “channelId” in $file" grep -R '\"channelId\"' -n "$file" || echo "No channelId attribute found"Length of output: 679
No action needed on CHANNEL_ID constant.
Confirmed thatCHANNEL_ID = "channel_id"matches theandroid:name="channel_id"entry inapp/res/navigation/nav_graph_connect_messaging.xml(line 42). The separatechannelIdargument at line 13 serves a different destination by design.
101-109:Details
❓ Verification inconclusive
Consider potential UI thread blocking.
The
MessageManager.retrieveMessages()call might perform network operations. Ensure this method handles threading internally to avoid ANR issues.Run the following script to check if MessageManager.retrieveMessages handles threading:
🏁 Script executed:
#!/bin/bash # Description: Check if MessageManager.retrieveMessages handles threading internally # Search for the retrieveMessages implementation ast-grep --pattern 'retrieveMessages($_, $_) { $$$ }' # Also check for async/thread handling patterns rg -A 10 "retrieveMessages.*Context.*RetrieveCallback" app/src/org/commcare/connect/MessageManager.javaLength of output: 163
🏁 Script executed:
#!/bin/bash # Locate MessageManager implementation file rg "class MessageManager" -n . # Search for the retrieveMessages definition and usages across the repo rg "retrieveMessages" -n .Length of output: 1442
🏁 Script executed:
#!/bin/bash # Display the implementation of retrieveMessages in MessageManager sed -n '30,80p' app/src/org/commcare/connect/MessageManager.java # Display the implementation of retrieveMessages in ApiPersonalId sed -n '380,440p' app/src/org/commcare/connect/network/ApiPersonalId.javaLength of output: 5909
Verify asynchronous execution in
retrieveMessagesIt looks like
MessageManager.retrieveMessages(…)builds its callback and then delegates toApiPersonalId.retrieveMessages(…), which in turn callsConnectNetworkHelper.get(…). Before merging, please:
- Confirm that
ConnectNetworkHelper.get(…)enqueues its HTTP request on a background thread (e.g., via OkHttp’senqueueor an AsyncTask) rather than blocking the caller.- If it’s a synchronous/blocking call, wrap the network invocation in a background executor or AsyncTask to avoid ANRs.
- Ensure all heavy JSON parsing and database writes in the callback occur off the UI thread.
app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
83-89: Good improvement to token error handling!The updated logic correctly handles token retrieval failures by only updating the token on success and logging exceptions otherwise. This prevents storing invalid tokens.
app/src/org/commcare/services/FCMMessageData.java (1)
27-38: Well-structured expansion of the notification data model!The addition of new fields and their corresponding getters/setters provides a comprehensive data model for handling various types of notifications. The default values and initialization logic are appropriate.
Also applies to: 145-263
| private void showChannelListFragmentWithFailureMessage(String failureMessage){ | ||
| Toast.makeText(this, failureMessage, Toast.LENGTH_LONG).show(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Navigate to channel list after showing error.
The method shows an error toast but doesn't navigate to the channel list fragment. Consider navigating to the channel list to provide a better user experience.
private void showChannelListFragmentWithFailureMessage(String failureMessage){
Toast.makeText(this, failureMessage, Toast.LENGTH_LONG).show();
+ navController.navigate(R.id.channelListFragment, null, getNavOptions());
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/org/commcare/activities/connect/ConnectMessagingActivity.java around
lines 154 to 156, the method showChannelListFragmentWithFailureMessage only
shows a toast with the failure message but does not navigate to the channel list
fragment. After displaying the toast, add code to navigate to the channel list
fragment to improve user experience by directing users to the relevant screen
after an error.
| public static boolean hasCccAction(String action) { | ||
| return action != null && action.contains("ccc_"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use startsWith() for more accurate prefix checking.
The current implementation using contains() could match "ccc_" anywhere in the action string, potentially causing false positives.
public static boolean hasCccAction(String action) {
- return action != null && action.contains("ccc_");
+ return action != null && action.startsWith("ccc_");
}📝 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.
| public static boolean hasCccAction(String action) { | |
| return action != null && action.contains("ccc_"); | |
| } | |
| public static boolean hasCccAction(String action) { | |
| return action != null && action.startsWith("ccc_"); | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseMessagingUtil.java at lines 379 to 381,
replace the use of action.contains("ccc_") with action.startsWith("ccc_") in the
hasCccAction method to ensure the check only matches when the action string
begins with "ccc_", preventing false positives from matches elsewhere in the
string.
| private static FirebaseMessagingDataSyncer dataSyncer; | ||
| private static FirebaseMessagingDataSyncer getDataSyncer(Context context){ | ||
| if(dataSyncer==null){ | ||
| dataSyncer = new FirebaseMessagingDataSyncer(context); | ||
| } | ||
| return dataSyncer; | ||
| } |
There was a problem hiding this comment.
Add thread safety to singleton implementation.
The current lazy initialization pattern is not thread-safe. Multiple threads could create multiple instances of FirebaseMessagingDataSyncer if they access getDataSyncer simultaneously.
Apply this diff to implement thread-safe singleton using double-checked locking:
private static FirebaseMessagingDataSyncer dataSyncer;
private static FirebaseMessagingDataSyncer getDataSyncer(Context context){
- if(dataSyncer==null){
- dataSyncer = new FirebaseMessagingDataSyncer(context);
+ if(dataSyncer == null){
+ synchronized (FirebaseMessagingUtil.class) {
+ if(dataSyncer == null){
+ dataSyncer = new FirebaseMessagingDataSyncer(context);
+ }
+ }
}
return dataSyncer;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseMessagingUtil.java around lines 121 to
127, the lazy initialization of the static dataSyncer instance is not
thread-safe, which can lead to multiple instances being created in concurrent
scenarios. To fix this, implement double-checked locking by synchronizing on the
class object inside the getDataSyncer method: first check if dataSyncer is null,
then synchronize, check again, and if still null, create the instance. This
ensures only one instance is created even with concurrent access.
| PendingIntent yesPendingIntent = PendingIntent.getBroadcast(context, 1, | ||
| yesIntent, flags); | ||
|
|
||
| // No button intent with payment_id from payload | ||
| Intent noIntent = new Intent(context, PaymentAcknowledgeReceiver.class); | ||
| noIntent.putExtra(OPPORTUNITY_ID, fcmMessageData.getPayloadData().get(OPPORTUNITY_ID)); | ||
| noIntent.putExtra(PAYMENT_ID, fcmMessageData.getPayloadData().get(PAYMENT_ID)); | ||
| noIntent.putExtra(PAYMENT_STATUS, false); | ||
| PendingIntent noPendingIntent = PendingIntent.getBroadcast(context, 2, | ||
| noIntent, flags); |
There was a problem hiding this comment.
Use dynamic request codes for PendingIntents to avoid conflicts.
The hardcoded request codes (1 and 2) could cause issues if multiple payment notifications are shown simultaneously. Later notifications might override earlier ones due to PendingIntent reuse.
Consider using dynamic request codes based on payment ID:
+ // Generate unique request codes based on payment ID to avoid conflicts
+ int yesRequestCode = fcmMessageData.getPayloadData().get(PAYMENT_ID).hashCode();
+ int noRequestCode = yesRequestCode + 1;
+
// Yes button intent with payment_id from payload
Intent yesIntent = new Intent(context, PaymentAcknowledgeReceiver.class);
yesIntent.putExtra(OPPORTUNITY_ID, fcmMessageData.getPayloadData().get(OPPORTUNITY_ID));
yesIntent.putExtra(PAYMENT_ID, fcmMessageData.getPayloadData().get(PAYMENT_ID));
yesIntent.putExtra(PAYMENT_STATUS, true);
- PendingIntent yesPendingIntent = PendingIntent.getBroadcast(context, 1,
+ PendingIntent yesPendingIntent = PendingIntent.getBroadcast(context, yesRequestCode,
yesIntent, flags);
// No button intent with payment_id from payload
Intent noIntent = new Intent(context, PaymentAcknowledgeReceiver.class);
noIntent.putExtra(OPPORTUNITY_ID, fcmMessageData.getPayloadData().get(OPPORTUNITY_ID));
noIntent.putExtra(PAYMENT_ID, fcmMessageData.getPayloadData().get(PAYMENT_ID));
noIntent.putExtra(PAYMENT_STATUS, false);
- PendingIntent noPendingIntent = PendingIntent.getBroadcast(context, 2,
+ PendingIntent noPendingIntent = PendingIntent.getBroadcast(context, noRequestCode,
noIntent, flags);📝 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.
| PendingIntent yesPendingIntent = PendingIntent.getBroadcast(context, 1, | |
| yesIntent, flags); | |
| // No button intent with payment_id from payload | |
| Intent noIntent = new Intent(context, PaymentAcknowledgeReceiver.class); | |
| noIntent.putExtra(OPPORTUNITY_ID, fcmMessageData.getPayloadData().get(OPPORTUNITY_ID)); | |
| noIntent.putExtra(PAYMENT_ID, fcmMessageData.getPayloadData().get(PAYMENT_ID)); | |
| noIntent.putExtra(PAYMENT_STATUS, false); | |
| PendingIntent noPendingIntent = PendingIntent.getBroadcast(context, 2, | |
| noIntent, flags); | |
| // Generate unique request codes based on payment ID to avoid conflicts | |
| int yesRequestCode = fcmMessageData.getPayloadData().get(PAYMENT_ID).hashCode(); | |
| int noRequestCode = yesRequestCode + 1; | |
| // Yes button intent with payment_id from payload | |
| Intent yesIntent = new Intent(context, PaymentAcknowledgeReceiver.class); | |
| yesIntent.putExtra(OPPORTUNITY_ID, fcmMessageData.getPayloadData().get(OPPORTUNITY_ID)); | |
| yesIntent.putExtra(PAYMENT_ID, fcmMessageData.getPayloadData().get(PAYMENT_ID)); | |
| yesIntent.putExtra(PAYMENT_STATUS, true); | |
| PendingIntent yesPendingIntent = PendingIntent.getBroadcast( | |
| context, | |
| yesRequestCode, | |
| yesIntent, | |
| flags | |
| ); | |
| // No button intent with payment_id from payload | |
| Intent noIntent = new Intent(context, PaymentAcknowledgeReceiver.class); | |
| noIntent.putExtra(OPPORTUNITY_ID, fcmMessageData.getPayloadData().get(OPPORTUNITY_ID)); | |
| noIntent.putExtra(PAYMENT_ID, fcmMessageData.getPayloadData().get(PAYMENT_ID)); | |
| noIntent.putExtra(PAYMENT_STATUS, false); | |
| PendingIntent noPendingIntent = PendingIntent.getBroadcast( | |
| context, | |
| noRequestCode, | |
| noIntent, | |
| flags | |
| ); |
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseMessagingUtil.java around lines 214 to
223, the PendingIntent request codes are hardcoded as 1 and 2, which can cause
conflicts when multiple payment notifications are active. To fix this, generate
dynamic request codes using a unique identifier such as the payment ID from the
payload data. Replace the fixed integers with a hash or integer derived from the
payment ID to ensure each PendingIntent is unique and does not override others.
| public FCMMessageData(Map<String, String> payloadData){ | ||
| this.action = getActionType(payloadData.get("action")); | ||
| this.username = payloadData.get("username"); | ||
| this.domain = payloadData.get("domain"); | ||
| this.creationTime = convertISO8601ToDateTime(payloadData.get("created_at")); | ||
| this.payloadData = payloadData; | ||
| actionType = getActionType(payloadData.get("action")); | ||
| username = payloadData.get("username"); | ||
| domain = payloadData.get("domain"); | ||
| creationTime = convertISO8601ToDateTime(payloadData.get("created_at")); | ||
| notificationTitle = payloadData.get("title"); | ||
| notificationText = payloadData.get("body"); | ||
| action = payloadData.get("action"); | ||
| priority = NotificationCompat.PRIORITY_HIGH; | ||
| notificationChannel = CommCareNoficationManager.NOTIFICATION_CHANNEL_PUSH_NOTIFICATIONS_ID; | ||
| smallIcon = R.drawable.commcare_actionbar_logo; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for payloadData parameter.
The constructor should handle the case where payloadData is null to prevent NullPointerException.
Add a null check at the beginning of the constructor:
public FCMMessageData(Map<String, String> payloadData){
if (payloadData == null) {
payloadData = new HashMap<>();
}
this.payloadData = payloadData;
// ... rest of the initialization
}🤖 Prompt for AI Agents
In app/src/org/commcare/services/FCMMessageData.java around lines 44 to 56, the
constructor does not check if the payloadData parameter is null, which can cause
a NullPointerException. Add a null check at the start of the constructor to
initialize payloadData to an empty HashMap if it is null before proceeding with
the rest of the initialization.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
app/src/org/commcare/services/PaymentAcknowledgeReceiver.java (1)
34-34: 💡 Verification agent🧩 Analysis chain
Verify if this static method call should also be updated.
Line 34 still calls
CommCareFirebaseMessagingService.clearNotification(context). Should this method also be moved toFirebaseMessagingUtilfor consistency with the centralization effort?
🏁 Script executed:
#!/bin/bash # Description: Check if clearNotification method exists in FirebaseMessagingUtil or if it's still in CommCareFirebaseMessagingService # Search for clearNotification method definition ast-grep --pattern 'clearNotification($$$) { $$$ }' # Also check with ripgrep for any clearNotification references rg -A 3 "clearNotification" --type javaLength of output: 6657
I’d like to confirm whether FirebaseMessagingUtil exists and if it already contains a clearNotification method.
🏁 Script executed:
#!/bin/bash # Search for the FirebaseMessagingUtil class rg -l "class FirebaseMessagingUtil" --type java # If found, show its definition and methods rg -A 5 "class FirebaseMessagingUtil" --type javaLength of output: 870
Continuing verification to see if
clearNotificationis defined inFirebaseMessagingUtil:
🏁 Script executed:
#!/bin/bash # Check for clearNotification method in FirebaseMessagingUtil rg -n "clearNotification" app/src/org/commcare/utils/FirebaseMessagingUtil.javaLength of output: 79
Add
clearNotificationtoFirebaseMessagingUtiland update callsI didn’t find a
clearNotification(Context)method inFirebaseMessagingUtil—it still lives only inCommCareFirebaseMessagingService. To complete the centralization effort, please:
- Move
public static void clearNotification(Context)fromCommCareFirebaseMessagingServiceintoFirebaseMessagingUtil.- Update all callers to use
FirebaseMessagingUtil.clearNotification(context)instead ofCommCareFirebaseMessagingService.clearNotification(context).Affected location:
- app/src/org/commcare/services/PaymentAcknowledgeReceiver.java:34 — replace
CommCareFirebaseMessagingService.clearNotification(context)withFirebaseMessagingUtil.clearNotification(context).app/src/org/commcare/activities/connect/ConnectMessagingActivity.java (1)
23-188: 💡 Verification agent🧩 Analysis chain
Verify security implications of exported activity.
Since this activity is marked as exported in the AndroidManifest.xml (according to the AI summary), ensure that the intent handling in
handleRedirect()properly validates all incoming data to prevent security vulnerabilities.
🏁 Script executed:
#!/bin/bash # Check if the activity is properly exported and secured in AndroidManifest.xml rg -A 5 -B 5 "ConnectMessagingActivity" app/AndroidManifest.xmlLength of output: 556
Secure exported ConnectMessagingActivity
The
ConnectMessagingActivityis marked asandroid:exported="true"without any permission guard, allowing any app to launch it by class name. To prevent unauthorized invocation and ensure only trusted callers can triggerhandleRedirect(), please:• In AndroidManifest.xml, restrict the activity with a signature‐level or custom permission (or remove
exportedif external launches aren’t needed):<manifest …> + <permission + android:name="org.commcare.permission.MANAGE_CONNECT_MESSAGES" + android:protectionLevel="signature"/> … <activity android:name="org.commcare.activities.connect.ConnectMessagingActivity" - android:exported="true"> + android:exported="true" + android:permission="org.commcare.permission.MANAGE_CONNECT_MESSAGES"> </activity> </manifest>• In ConnectMessagingActivity.java, tighten intent handling in
handleRedirect():
– UsegetIntent().getAction()(not a generic extra) and only proceed when it exactly matches your defined constant (e.g.,CCC_MESSAGE).
– Sanitize and whitelist any extras (like the channel ID) before querying the database.These changes will ensure only authorized, in-app or signed callers can invoke this activity and reduce risk of malicious intents.
app/src/org/commcare/services/FCMMessageData.java (1)
139-139:⚠️ Potential issueFix serialization bug - wrong field being serialized.
The method is serializing
action(String) instead ofactionType(enum), which will cause deserialization issues.- ExtUtil.writeString(out, action.toString()); + ExtUtil.writeString(out, actionType.toString());
🧹 Nitpick comments (6)
app/src/org/commcare/activities/connect/ConnectActivity.java (1)
32-32: Remove unused import of CommCareFirebaseMessagingService
The import at line 32 is no longer referenced and should be cleaned up to avoid confusion.app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
58-58: Clean delegation pattern implemented.The refactoring to delegate all notification handling to
FirebaseMessagingUtilis a good architectural decision that centralizes the notification logic.Consider documenting what the boolean parameter
truerepresents for better code clarity.app/src/org/commcare/activities/connect/ConnectMessagingActivity.java (1)
102-102: Fix spacing for consistency.- ConnectMessagingChannelRecord connectMessagingChannelRecord =ConnectMessagingDatabaseHelper.getMessagingChannel(this, channelId); + ConnectMessagingChannelRecord connectMessagingChannelRecord = ConnectMessagingDatabaseHelper.getMessagingChannel(this, channelId);app/src/org/commcare/services/FCMMessageData.java (1)
146-263: Consider enhancing javadoc comments for better clarity.The getter/setter javadoc comments are generic. Consider adding more descriptive comments that explain the purpose and expected values for each field.
For example:
- /** - * Getter for notification title - * @return - */ + /** + * Gets the notification title displayed in the notification bar + * @return The notification title text, or null if not set + */app/src/org/commcare/utils/FirebaseMessagingUtil.java (2)
276-277: Track the TODO for backend channel name support.This TODO indicates a temporary workaround that should be removed once the backend starts sending channel names in push notifications.
Would you like me to create an issue to track this backend dependency?
342-344: Consider using warning level for empty notifications.Logging empty notifications as exceptions might be too aggressive and could clutter error logs. Consider using warning level instead.
- Logger.exception("Empty push notification", - new Throwable(String.format("Empty notification for action '%s'", fcmMessageData.getAction()))); + Logger.log("empty-notification", + String.format("Empty notification received for action '%s'", fcmMessageData.getAction()));
🛑 Comments failed to post (4)
app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java (1)
74-83: 🛠️ Refactor suggestion
Consider the implications of removing the argument from the bundle.
While moving the channel ID processing to
onViewCreatedis good practice, removing the argument at line 79 could cause issues if the fragment is recreated (e.g., after a configuration change). The channel ID would be lost and the automatic navigation wouldn't occur on recreation.Consider keeping the argument in the bundle or storing the channel ID in a member variable to handle fragment recreation properly.
🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java around lines 74 to 83, avoid removing the CHANNEL_ID argument from the bundle as it causes loss of data on fragment recreation. Instead, keep the argument intact and store the channel ID in a member variable during onViewCreated. Use this member variable to handle channel selection, ensuring the channel ID persists across configuration changes and fragment lifecycle events.app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
36-36:
⚠️ Potential issueRemove unused import.
The static import of
CCC_MESSAGEis not used in this file and should be removed.Apply this diff to remove the unused import:
-import static org.commcare.connect.ConnectConstants.CCC_MESSAGE; -📝 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.🤖 Prompt for AI Agents
In app/src/org/commcare/services/CommCareFirebaseMessagingService.java at line 36, remove the unused static import of CCC_MESSAGE since it is not referenced anywhere in the file. Simply delete the line importing CCC_MESSAGE to clean up the imports.app/src/org/commcare/utils/FirebaseMessagingUtil.java (2)
121-127: 🛠️ Refactor suggestion
Consider thread safety for the singleton data syncer.
The lazy initialization of the data syncer singleton is not thread-safe. Multiple threads could create multiple instances.
- private static FirebaseMessagingDataSyncer dataSyncer; - private static FirebaseMessagingDataSyncer getDataSyncer(Context context){ - if(dataSyncer==null){ - dataSyncer = new FirebaseMessagingDataSyncer(context); - } - return dataSyncer; - } + private static volatile FirebaseMessagingDataSyncer dataSyncer; + private static FirebaseMessagingDataSyncer getDataSyncer(Context context){ + if(dataSyncer == null){ + synchronized(FirebaseMessagingUtil.class) { + if(dataSyncer == null) { + dataSyncer = new FirebaseMessagingDataSyncer(context); + } + } + } + return dataSyncer; + }📝 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.private static volatile FirebaseMessagingDataSyncer dataSyncer; private static FirebaseMessagingDataSyncer getDataSyncer(Context context) { if (dataSyncer == null) { synchronized (FirebaseMessagingUtil.class) { if (dataSyncer == null) { dataSyncer = new FirebaseMessagingDataSyncer(context); } } } return dataSyncer; }🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseMessagingUtil.java around lines 121 to 127, the lazy initialization of the static dataSyncer singleton is not thread-safe, which can lead to multiple instances being created in concurrent scenarios. To fix this, synchronize the method or use a synchronized block to ensure only one thread can initialize dataSyncer at a time, or implement double-checked locking to maintain thread safety while minimizing synchronization overhead.
290-291: 💡 Verification agent
🧩 Analysis chain
Verify reliability of static UI state checks.
Using static fields (
ConnectMessageChannelListFragment.isActiveandConnectMessageFragment.activeChannel) to determine UI state could be unreliable due to Android lifecycle complexities.
🏁 Script executed:
#!/bin/bash # Check how these static fields are managed in the fragments echo "Checking ConnectMessageChannelListFragment.isActive management:" ast-grep --pattern 'class ConnectMessageChannelListFragment { $$$ static $_$ isActive $$$ $$$ }' echo -e "\nChecking lifecycle methods that update isActive:" rg -A 3 -B 3 "isActive\s*=" app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.javaLength of output: 943
Ensure robust UI state checks
The
isActiveflag inConnectMessageChannelListFragmentis indeed toggled in its lifecycle (onResume/onPause), but relying on static UI state can still introduce timing or memory‐leak issues when messages arrive outside the fragment’s active window.• app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java
– onResume():isActive = true
– onPause():isActive = false
• app/src/org/commcare/utils/FirebaseMessagingUtil.java (lines ~290–291)if (!ConnectMessageChannelListFragment.isActive && !channelId.equals(ConnectMessageFragment.activeChannel)) { … }Recommendation:
– Replace static flags with a lifecycle-aware mechanism (e.g., LiveData or a shared ViewModel) so messaging updates automatically respect fragment attachment state.
– This will eliminate potential race conditions or stale state when fragments are recreated or multiple instances exist.🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseMessagingUtil.java around lines 290-291, the code uses static fields ConnectMessageChannelListFragment.isActive and ConnectMessageFragment.activeChannel to check UI state, which is unreliable due to Android lifecycle complexities. Refactor this by removing static state checks and instead implement a lifecycle-aware approach such as using LiveData or a shared ViewModel to track fragment active state and current channel. Update the messaging logic to observe this lifecycle-aware state so it only processes messages when the fragment is truly active, avoiding race conditions and stale state issues.
|
|
||
| Bundle bundle = new Bundle(); | ||
| bundle.putString("channel_id", channelId); | ||
| /** |
There was a problem hiding this comment.
Nit: I think we can skip the javadocs for private methods, and the ones here aren't adding much info anyway
| } | ||
|
|
||
| //If we still don't have a key, this will return null and we'll ignore the message | ||
| message = ConnectMessagingMessageRecord.fromMessagePayload(payloadData, channel.getKey()); |
There was a problem hiding this comment.
I see ConnectMessagingMessageRecord.fromMessagePayload won't be called from anywhere after this change, is that intentional? If so, we can delete the obsolete method
There was a problem hiding this comment.
@OrangeAndGreen Yeah correct. Previously, whenever app was receiving the notification, it start connecting the connect message DB and there are few instances where connect message DB is not present, due to this app was failing to show even the notification. Now, backend is sending entire information required to show the notification like channel id, channel name etc. This has enabled app to show the notification even without connecting DB.
When user clicks on the notification, app is handling the encryption key, consent etc on MessageActivity. So messages get display without any error.
Agree with you, I will delete this method.
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Looking good overall, let some minor comments.
app/src/org/commcare/activities/connect/ConnectMessagingActivity.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/activities/connect/ConnectMessagingActivity.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/activities/connect/ConnectMessagingActivity.java
Outdated
Show resolved
Hide resolved
| */ | ||
| public static Intent handleNotification(Context context, Map<String, String>notificationPayload, boolean showNotification){ | ||
| FCMMessageData fcmMessageData = new FCMMessageData(notificationPayload); | ||
| if (fcmMessageData.getPayloadData() == null || fcmMessageData.getPayloadData().isEmpty()) { |
There was a problem hiding this comment.
probably just better to init payloadData as empty map to avoid doing any null checks on it.
There was a problem hiding this comment.
Its handling whatever is coming directly from data payload.
There was a problem hiding this comment.
Although would not new FCMMessageData(notificationPayload); crash already if notificationPayload is null ? As such It seems confusing to me that we are doing null check in one place but not in another.
There was a problem hiding this comment.
Yeah so it will not crash but as it doesn't have title and body, it will throw exception here and logged the exception.
There was a problem hiding this comment.
I meant the line above this null check would crash itself with a NPE if fcmMessageData.getPayloadData() == null and user will never reach here in that case.
There was a problem hiding this comment.
I am not really seeing this addressed in the commit above.
There was a problem hiding this comment.
@Jignesh-dimagi Code looks good to me, can you take a pass on docstrings added in the PR and remove anything that's quite obvious from the code as well.
Also would you be able to reply on comments in the parent phase 4 PR that this PR resolves.
|
I will review the docstrings and I guess I have removed code which is unused but will reverify it. Sorry I didn't get you for phase 4, can you please send the PR here so that I can look into it.
|
|
The PR where I added my review on for messaging - #3108 |
|
@shubham1g5 Many of points are addressed in this PR but your list is long so ideally we can break it in parts. We can merge this PR first and take the rest? |
That's ok. But if you can reply to the individual threads over there with a link to this PR , it would help us track what is resolved vs what is not. |
|
@shubham1g5 commented in individual threads.
|
…yload was missed, now added
|
|
||
| if (dataPayload == null || dataPayload.isEmpty()) { | ||
| if (!showNotificationFromNotificationPayload(context,notificationPayload)) { | ||
| Logger.exception("Empty push notification", new Throwable(String.format("Empty notification without payload"))); |
There was a problem hiding this comment.
Probably better to modify the text of exception now to reflect inability to show notification using payloadNotification
There was a problem hiding this comment.
Actually, at this point both data and notification payloads are empty so kept this message
| * @param fcmMessageData | ||
| * @return | ||
| */ | ||
| private static Intent handleResumeLearningOrDeliveryJobPushNotification(Boolean isLearning, Context context, FCMMessageData fcmMessageData){ |
There was a problem hiding this comment.
code formatting is off in all the new notification handling methods.
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-1058
Labels and Review