ConnectID Messaging in app home #3038
Conversation
Added messaging to app home menu. Moved some messaging code for better reuse.
… into dv/messaging_in_app_home
… into dv/messaging_in_app_home
OrangeAndGreen
requested review from
Jignesh-dimagi,
avazirna,
pm-dimagi and
shubham1g5
📝 WalkthroughWalkthroughThis set of changes introduces a new XML menu resource ( Sequence Diagram(s)sequenceDiagram
participant User
participant StandardHomeActivity
participant MessageManager
participant BroadcastReceiver
participant ConnectManager
User->>StandardHomeActivity: Launches activity
StandardHomeActivity->>StandardHomeActivity: onCreateOptionsMenu inflates menu_app_home.xml
StandardHomeActivity->>MessageManager: updateMessagingIcon(context, messagingMenuItem)
MessageManager-->>StandardHomeActivity: Sets icon based on unread messages
Note over StandardHomeActivity: Registers BroadcastReceiver in onResume
BroadcastReceiver->>StandardHomeActivity: Receives MESSAGING_UPDATE_BROADCAST
StandardHomeActivity->>MessageManager: updateMessagingIcon(context, messagingMenuItem)
User->>StandardHomeActivity: Selects action_messaging menu item
StandardHomeActivity->>ConnectManager: goToMessaging(activity)
Possibly related PRs
Suggested reviewers
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ 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:
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: 1
🔭 Outside diff range comments (1)
app/src/org/commcare/connect/ConnectManager.java (1)
429-433: 💡 Verification agent🧩 Analysis chain
Method looks correct for launching the messaging activity
The
goToMessagingmethod provides proper navigation to the ConnectMessagingActivity.Verify that the calling conditions for this method check whether the user is signed in via ConnectID, as per PR requirements that messaging should only be visible to users signed in via ConnectID.
🏁 Script executed:
#!/bin/bash # Search for calls to goToMessaging and check if they're properly guarded rg -p "goToMessaging" --glob "*.java"Length of output: 370
🏁 Script executed:
#!/bin/bash # Show context around calls to goToMessaging to verify ConnectID guard rg -C5 "ConnectManager.goToMessaging" app/src/org/commcare/activities/connect/ConnectActivity.java rg -C5 "ConnectManager.goToMessaging" app/src/org/commcare/activities/StandardHomeActivity.javaLength of output: 840
Ensure messaging is only accessible to authenticated ConnectID users
The
goToMessagingmethod itself correctly launchesConnectMessagingActivity, but its invocations in both activities aren’t guarded by a ConnectID sign‐in check. As a result, users who aren’t signed in can still tap “Messaging.” You should either:
- Wrap the calls in
onOptionsItemSelectedwith a check against your ConnectID auth state (e.g.,if (!ConnectManager.isSignedIn()) { /* show prompt */ return true; }) before callinggoToMessaging(this);, or- Conditionally inflate or hide the action_messaging menu item in
onCreateOptionsMenuunlessConnectManager.isSignedIn()is true.Locations needing fixes:
- app/src/org/commcare/activities/connect/ConnectActivity.java →
onOptionsItemSelectedhandling ofR.id.action_messaging(≈line 169)- app/src/org/commcare/activities/StandardHomeActivity.java →
onOptionsItemSelectedhandling ofR.id.action_messaging(≈line 254)
🧹 Nitpick comments (6)
app/res/menu/menu_app_home.xml (1)
1-49: Menu resource structure looks goodThis new XML menu resource for the app home provides a clean structure with proper organization of menu items. The messaging item is set to always show in the action bar, while other items are in the overflow menu.
A couple of minor suggestions:
- Consider using string resources for all menu item titles for better localization support
- Some menu items are missing icons (like action_set_pin)
For improved localization support, replace hardcoded titles with string resources:
- android:title="Update" + android:title="@string/update" - android:title="Saved Forms" + android:title="@string/saved_forms" - android:title="Change Language" + android:title="@string/change_language" - android:title="About" + android:title="@string/about" - android:title="Advanced" + android:title="@string/advanced" - android:title="Preferences" + android:title="@string/preferences" - android:title="Set PIN" + android:title="@string/set_pin" - android:title="Update CommCare" + android:title="@string/update_commcare"app/src/org/commcare/activities/connect/ConnectActivity.java (2)
115-132: Minor: guard icon refresh until menu is inflated
messagingMenuItemisnullon the very firstonResume()(executed beforeonCreateOptionsMenu()during initial launch).
MessageManager.updateMessagingIcon()handles thenull, so there is no crash, but the broadcast receiver may fire before the menu exists, losing the update.A tiny improvement is to early‑return when
messagingMenuItem == nullinstead of hitting the DB:protected void onResume() { super.onResume(); - MessageManager.updateMessagingIcon(this, messagingMenuItem); + if (messagingMenuItem != null) { + MessageManager.updateMessagingIcon(this, messagingMenuItem); + }Purely an optimisation—feel free to skip if you prefer to keep the code compact.
153-156: Analytics mapping missing for new menu id
action_messagingis handled inonOptionsItemSelected, but it is not recorded in the analytics map increateMenuItemToAnalyticsParamMapping()(still only lives inStandardHomeActivity).
If you want to track taps consistently, add an entry such as:menuIdToAnalyticsEvent.put(R.id.action_messaging, AnalyticsParamValue.ITEM_MESSAGING);(assuming a corresponding
AnalyticsParamValueexists).app/src/org/commcare/activities/StandardHomeActivity.java (3)
133-147: Avoid triggering a full network sync on everyonResume()
MessageManager.retrieveMessages()performs a REST call, DB writes, and potentially crypto, every time the home activity comes to the foreground.
Typical user behaviour (quick app switching or permission dialogs) can make this run dozens of times in a short period.Consider throttling:
- Cache the last‑sync timestamp and skip if called within N minutes.
- Kick off a WorkManager periodic job instead of coupling it to UI lifecycle.
- Perform an exponential back‑off if the last request failed.
This will save battery and bandwidth without sacrificing freshness.
169-181: Centralise menu‑title localisation to reduce duplicationSeven consecutive lines set titles manually. That’s easy to forget when adding a new item. Extract to a helper:
private static void localise(Menu menu, int itemId, @StringRes int stringId) { menu.findItem(itemId).setTitle(Localization.get(getString(stringId))); }Call it in a small loop. Pure readability but keeps this method short.
228-256: Switch‑case body is ripe for refactorThe long
else ifladder duplicates the pattern used elsewhere. Aswitchor aMap<Integer,Runnable>would trim ~40 lines. Not blocking, but pays back quickly as new actions are added.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/res/menu/menu_app_home.xml(1 hunks)app/res/menu/menu_connect.xml(1 hunks)app/res/menu/menu_connect_messaging.xml(0 hunks)app/src/org/commcare/activities/StandardHomeActivity.java(5 hunks)app/src/org/commcare/activities/connect/ConnectActivity.java(4 hunks)app/src/org/commcare/activities/connect/ConnectMessagingActivity.java(0 hunks)app/src/org/commcare/connect/ConnectManager.java(3 hunks)app/src/org/commcare/connect/MessageManager.java(2 hunks)app/unit-tests/src/org/commcare/android/tests/DemoUserRestoreTest.java(2 hunks)
💤 Files with no reviewable changes (2)
- app/res/menu/menu_connect_messaging.xml
- app/src/org/commcare/activities/connect/ConnectMessagingActivity.java
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/src/org/commcare/connect/ConnectManager.java (1)
app/src/org/commcare/connect/database/ConnectMessagingDatabaseHelper.java (1)
ConnectMessagingDatabaseHelper(20-193)
app/src/org/commcare/connect/MessageManager.java (1)
app/src/org/commcare/connect/database/ConnectMessagingDatabaseHelper.java (1)
ConnectMessagingDatabaseHelper(20-193)
app/src/org/commcare/activities/connect/ConnectActivity.java (1)
app/src/org/commcare/connect/MessageManager.java (1)
MessageManager(33-378)
🔇 Additional comments (6)
app/src/org/commcare/connect/ConnectManager.java (1)
6-6: Added imports for the messaging feature supportThe added imports will support the new messaging functionality. These imports are necessary for:
MenuItem- To update the messaging menu itemResourcesCompat- For resource handling likely related to icon updatesConnectMessagingDatabaseHelper- To access messaging data for the featureAlso applies to: 15-15, 43-43
app/res/menu/menu_connect.xml (2)
5-5: Improved menu item identification and localizationGood changes:
- Standardizing on
action_messagingID across the app- Using string resources instead of hardcoded titles for proper localization
Also applies to: 7-7
13-13: Proper use of string resource for sync titleUsing
@string/syncimproves localization support for the sync menu item.app/unit-tests/src/org/commcare/android/tests/DemoUserRestoreTest.java (2)
18-18: Added import for resource accessAdding the import for
org.commcare.dalvik.Ris necessary to reference the menu resource IDs.
152-159: Updated test to use resource IDs instead of constantsThis is a good update to match the code changes. The test now properly references menu items by their resource IDs rather than static constants.
app/src/org/commcare/activities/StandardHomeActivity.java (1)
263-277: Add analytics entry for the new Messaging action
action_messagingis absent here, so taps on the icon won’t be logged. Mirror the pattern used for other items:menuIdToAnalyticsEvent.put(R.id.action_messaging, AnalyticsParamValue.ITEM_MESSAGING);(Define
ITEM_MESSAGINGif missing.)
| public static void updateMessagingIcon(Context context, MenuItem messagingMenuItem) { | ||
| if(messagingMenuItem != null) { | ||
| int icon = R.drawable.ic_connect_messaging_base; | ||
| if(ConnectMessagingDatabaseHelper.getUnviewedMessages(context).size() > 0) { | ||
| icon = R.drawable.ic_connect_messaging_unread; | ||
| } | ||
| messagingMenuItem.setIcon(ResourcesCompat.getDrawable(context.getResources(), icon, null)); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid loading full message objects just to test the “unread” state
getUnviewedMessages(context) deserialises every unread message into a Java object and returns it in a List, only for us to call .size(). On even a modest message backlog this allocates many objects and hits the DB on the UI thread every time the icon is refreshed (on onResume, in a BroadcastReceiver, in onCreateOptionsMenu, etc.).
Introduce a lightweight query that only returns a count/boolean and move the DB‑work off the UI thread:
-if(ConnectMessagingDatabaseHelper.getUnviewedMessages(context).size() > 0) {
+if (ConnectMessagingDatabaseHelper.hasUnviewedMessages(context)) {Where hasUnviewedMessages could be implemented in the helper with:
public static boolean hasUnviewedMessages(Context c) {
SQLiteDatabase db = getDb(c);
try (Cursor cur = db.rawQuery(
"SELECT 1 FROM messaging WHERE " + META_MESSAGE_USER_VIEWED + " = 0 LIMIT 1", null)) {
return cur.moveToFirst();
}
}Besides the performance win, doing the DB call inside a coroutine/AsyncTask (or at least wrapping it in AsyncTask.THREAD_POOL_EXECUTOR) would prevent a Strict‑Mode violation on newer Android versions.
🏁 Script executed:
#!/bin/bash
# Locate all references to getUnviewedMessages
rg --color=never "getUnviewedMessages" -n
# Show the definition of getUnviewedMessages with surrounding context
rg --color=never "public static .*getUnviewedMessages" -n -A5 -B2
# Find where updateMessagingIcon is invoked in the codebase
rg --color=never "updateMessagingIcon" -nLength of output: 2623
Implement a lightweight “unread” check and move DB calls off the UI thread
Loading full message records just to test for any unread messages blocks the UI and allocates many objects. Instead:
• In app/src/org/commcare/connect/database/ConnectMessagingDatabaseHelper.java (add after line 145):
public static boolean hasUnviewedMessages(Context c) {
SQLiteDatabase db = getDb(c);
try (Cursor cur = db.rawQuery(
"SELECT 1 FROM messaging WHERE " + META_MESSAGE_USER_VIEWED + " = 0 LIMIT 1",
null)) {
return cur.moveToFirst();
}
}• In app/src/org/commcare/connect/MessageManager.java (replace at line 73):
- if (ConnectMessagingDatabaseHelper.getUnviewedMessages(context).size() > 0) {
+ if (ConnectMessagingDatabaseHelper.hasUnviewedMessages(context)) {• Wrap calls to updateMessagingIcon(…) in a background thread (e.g. AsyncTask.THREAD_POOL_EXECUTOR or a coroutine on Dispatchers.IO), then post the icon update back to the UI thread to avoid Strict‑Mode violations.
|
is it possible to PR the menu refactor to |
|
Created a PR to master here |
|
thanks @OrangeAndGreen , think we should rebase |
…re-android into dv/messaging_in_app_home
https://dimagi.atlassian.net/browse/CCCT-707
cross-request: dimagi/commcare-core#1455
Product Description
ConnectID Messaging is now accessible from the app home page when signed in to an app via ConnectID.
Technical Summary
Converted menu in app home to an XML resource.
Added messaging to app home menu (as a toolbar action).
Only showing messaging when user is signed in to an app via ConnectID.
Retrieving messages when app home resumes and updating messaging icon.
Feature Flag
ConnectID Messaging
Safety Assurance
Safety story
Tested locally, see specific tests cases in QA Plan
Automated test coverage
None for ConnectID yet
QA Plan
-Verify that the existing menu functionality on the app home page still works
-Log into a traditional app via user/pass and verify that the messaging icon does not appear
-Log into a traditional app managed by ConnectID and verify that the messaging icon appears
-Log into a Connect app and verify that the messaging icon appears
-Send a ConnectID message to the user, then login to an app via ConnectID on the device and verify that the red dot appears on the messaging icon