Managed read/unread pn status and its icon #3383
Conversation
jaypanchal-13
requested review from
Jignesh-dimagi,
OrangeAndGreen,
conroy-ricketts and
shubham1g5
jaypanchal-13
changed the title
📝 WalkthroughWalkthroughThis PR implements notification read/unread status tracking for push notifications. It introduces a new Sequence DiagramsequenceDiagram
actor User
participant DrawerUI as Drawer UI
participant NavHelper as ConnectNavHelper
participant PNActivity as PushNotificationActivity
participant ViewModel as PushNotificationViewModel
participant APIHelper as PushNotificationApiHelper
participant Prefs as NotificationPrefs
participant Broadcaster as LocalBroadcastManager
participant DrawerActivity as BaseDrawerActivity
User->>DrawerUI: Receives unread notification
Note over DrawerUI: Icon shows ic_new_notification_bell
User->>DrawerUI: Taps notification
DrawerUI->>NavHelper: goToNotification()
NavHelper->>Prefs: setNotificationAsRead(context)
NavHelper->>PNActivity: Launch PushNotificationActivity
PNActivity->>ViewModel: Refresh notifications
ViewModel->>APIHelper: retrieveLatestPushNotifications(context, true)
rect rgb(200, 230, 255)
Note over APIHelper: isFromPNActivity=true
APIHelper->>APIHelper: Fetch notifications
APIHelper-->>ViewModel: Return results (no side effects)
end
rect rgb(230, 200, 255)
Note over APIHelper: Background refresh or ViewModel refresh
APIHelper->>Prefs: setNotificationAsUnread(context)
APIHelper->>Broadcaster: Broadcast ACTION_NEW_NOTIFICATIONS
end
Broadcaster->>DrawerActivity: notificationUpdateReceiver receives broadcast
DrawerActivity->>DrawerActivity: refreshDrawer()
DrawerUI->>Prefs: getNotificationReadStatus()
Note over DrawerUI: Icon shows ic_bell (read status)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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: 0
🧹 Nitpick comments (2)
app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (1)
23-27: Consider consolidating receiver logic.The
notificationUpdateReceiverandmessagingUpdateReceiver(lines 55-59) both perform the same action: refreshing the drawer. While keeping them separate provides clarity about their distinct triggers, consolidating them into a single receiver handling multiple intent filters could reduce duplication.Example:
-private val notificationUpdateReceiver: BroadcastReceiver = object : BroadcastReceiver() { - override fun onReceive(context: Context, intent: Intent) { - refreshDrawer() - } -} - -private val messagingUpdateReceiver: BroadcastReceiver = object : BroadcastReceiver() { - override fun onReceive(context: Context, intent: Intent) { - drawerController?.refreshDrawerContent() - } -} +private val updateReceiver: BroadcastReceiver = object : BroadcastReceiver() { + override fun onReceive(context: Context, intent: Intent) { + refreshDrawer() + } +}Then register once with both filters.
app/src/org/commcare/utils/PushNotificationApiHelper.kt (1)
45-51: Clarify the dual update handling pattern.The conditional
if (result.isNotEmpty() && !isFromPNActivity)creates two distinct handling paths:
- Background fetch (
isFromPNActivity=false): Stores notifications, acknowledges to server, marks as unread, and broadcasts.- PN activity fetch (
isFromPNActivity=true): Returns data only;PushNotificationViewModeldelegates storage/acknowledgment toPNApiSyncWorkerManager.This split responsibility across different layers (API helper vs. ViewModel's worker manager) isn't immediately obvious and could complicate maintenance. Consider:
- Adding a comment explaining why updates are skipped when
isFromPNActivity=true.- Evaluating whether both code paths could be unified, or if the separation serves a specific performance/UX goal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt(1 hunks)app/src/org/commcare/connect/ConnectNavHelper.kt(2 hunks)app/src/org/commcare/navdrawer/BaseDrawerActivity.kt(2 hunks)app/src/org/commcare/navdrawer/BaseDrawerController.kt(2 hunks)app/src/org/commcare/preferences/NotificationPrefs.kt(1 hunks)app/src/org/commcare/utils/PushNotificationApiHelper.kt(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt (1)
app/src/org/commcare/utils/PushNotificationApiHelper.kt (1)
retrieveLatestPushNotifications(32-35)
⏰ 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 (8)
app/src/org/commcare/preferences/NotificationPrefs.kt (2)
10-13: LGTM!Clean implementation using Kotlin's
editextension for SharedPreferences.
20-23: Verify the default value semantics.The default value of
truemeans notifications start in a "read" state on fresh installs. This approach prevents showing a notification badge when there are no actual notifications, which seems reasonable. However, ensure this aligns with the intended UX—particularly that the badge only appears after the first actual notification is received and marked unread.app/src/org/commcare/connect/ConnectNavHelper.kt (1)
42-42: LGTM!Correctly marks notifications as read before launching the notification activity, ensuring the icon state updates appropriately.
app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt (1)
39-39: LGTM!Correctly passes
isFromPNActivity = trueto distinguish user-initiated refreshes from background fetches, enabling the API helper to skip redundant broadcasts and status updates.app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)
136-139: LGTM!Clean conditional logic that dynamically selects the notification icon based on read status, integrated appropriately within the drawer refresh flow.
app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (1)
39-42: Note: LocalBroadcastManager is deprecated.While
LocalBroadcastManageris deprecated as of AndroidX, it remains functional. For future work, consider migrating to alternatives like LiveData, SharedFlow, or a custom event bus. For now, this implementation is acceptable and consistent with existing patterns in the codebase.Also applies to: 48-48
app/src/org/commcare/utils/PushNotificationApiHelper.kt (2)
30-30: LGTM!Clear constant definition for the notification broadcast action.
49-50: Note: LocalBroadcastManager is deprecated.
LocalBroadcastManageris deprecated as of AndroidX. While it remains functional, consider migrating to alternatives (e.g., LiveData, SharedFlow, or a custom event bus) in future iterations. For now, this is acceptable given its usage throughout the codebase.
| LocalBroadcastManager.getInstance(this).registerReceiver( | ||
| notificationUpdateReceiver, | ||
| IntentFilter(PushNotificationApiHelper.ACTION_NEW_NOTIFICATIONS) | ||
| ) |
There was a problem hiding this comment.
@jaypanchal-13 onResume would be more ideal as it will restart listening when coming back
There was a problem hiding this comment.
@Jignesh-dimagi It is already added in onResume
| import android.os.Bundle | ||
| import org.commcare.utils.PushNotificationApiHelper | ||
|
|
||
| abstract class BaseDrawerActivity<T> : CommCareActivity<T>() { |
There was a problem hiding this comment.
@jaypanchal-13 Need to listen in ConnectActivity also to update the bell icon
|
|
||
| suspend fun retrieveLatestPushNotifications(context: Context): Result<List<PushNotificationRecord>>{ | ||
| val pushNotificationListResult = callPushNotificationApi(context) | ||
| suspend fun retrieveLatestPushNotifications(context: Context,isFromPNActivity: Boolean = false): Result<List<PushNotificationRecord>>{ |
There was a problem hiding this comment.
@jaypanchal-13 Let's not complicate the logic here. Let it throw the broadcast when new PNs but there will be no listeners, also in PushNotificationViewModel.kt change status to read after receiving the new PNs.
There was a problem hiding this comment.
@Jignesh-dimagi status will be updated to unread after receiveing new PN's. Managed status in PNActivity
There was a problem hiding this comment.
@jaypanchal-13 Whenver pushNotificationViewModel receives the latest notifications, it should set the icon to read and update the broadcast.
There was a problem hiding this comment.
Ok seen that you have taken care in Push Notification Activity.
| import androidx.lifecycle.LifecycleOwner | ||
| import androidx.localbroadcastmanager.content.LocalBroadcastManager | ||
| object NotificationBroadcastHelper { | ||
| const val ACTION_NEW_NOTIFICATIONS = "com.dimagi.NEW_NOTIFICATIONS_RECEIVED" |
There was a problem hiding this comment.
can we namespace this to the package instead org.commcare.dalvik.action.NEW_NOTIFICATION
|
|
||
| override fun onDestroy() { | ||
| super.onDestroy() | ||
| NotificationPrefs.setNotificationAsRead(this) |
There was a problem hiding this comment.
We should be setting this only when we set the notifications list to adapter on UI as that's an indication that user has successfully seen the notification page. setNotificationAsRead should also be removed from all the other places in the code as just clicking the bell icon or menu item is not enough indication of success (if code fails after click but before showing notifications page, the status should remain un-read)
| override fun onCreate(savedInstanceState: Bundle?) { | ||
| super.onCreate(savedInstanceState) | ||
| checkForDrawerSetUp() | ||
| NotificationBroadcastHelper.registerForNotifications(this, this) { |
There was a problem hiding this comment.
should only happen if drawerController is not null
|
|
||
|
|
||
| val isNotificationRead = NotificationPrefs.getNotificationReadStatus(activity) | ||
| val notificationIconRes = |
There was a problem hiding this comment.
Have a single method getNotificationIcon that checks for the unread status and give you the right icon and use it at both places - In drawer and on the action bar item.
|
|
||
| object NotificationPrefs { | ||
| private const val PREF_NAME = "notification_prefs" | ||
| private const val KEY_PN_READ_STATUS = "pn_read_status" |
There was a problem hiding this comment.
replace pn -> notification
| when { | ||
| notifications.isNotEmpty() -> { | ||
| pushNotificationAdapter.submitList(notifications) | ||
| NotificationPrefs.setNotificationAsRead(this) |
There was a problem hiding this comment.
nit: probably no functional side-effect but this should not ideally depend on notifications.isNotEmpty() but happen always
There was a problem hiding this comment.
@shubham1g5 I still think onDestroy is better place for setNotificationAsRead. Because if user is in notification screen and new notification is fetched by clicking sync button on notification screen. It will not update pn icon as read. What are your views?
There was a problem hiding this comment.
would not this observer get fired again when you click sync ?
There was a problem hiding this comment.
Yes isNotEmpty() if list is not empty and else if no item is fetched
There was a problem hiding this comment.
so that's fine right ? Maybe I am not really understanding the issue here.
There was a problem hiding this comment.
Yes but In PushNotificationApiHelperon success of api call I updated status to unread. When sync btn is clicked and api is called again so it makes status unread even if I make it status read on this. So When placed setNotificationAsRead in onDestroy it updates status correctly
There was a problem hiding this comment.
When sync btn is clicked and api is called again so it makes status unread even if I make it status read on this
We should be marking it read after the API call and my sense is that your observer gets triggered after the API call otherwise user will never see the updated data from the API. if it's not happening through observer, can you point out how are we handling data refresh when the network fetch results in new notifications ?
There was a problem hiding this comment.
Yes it is behaving same. Just an error of sequence was there update api is also called on success of get pn api and after calling update api again process for get api was continued because continuation.resumeWith is added in update pn api.
| snackbar.show(); | ||
| } | ||
|
|
||
| public static int getNotificationIcon(Context context) { |
There was a problem hiding this comment.
don't think this is the right class for this to go, think NotificationUtil would be a better choice here.
|
@jaypanchal-13 Can you also try to satisfy Kotlin lint errors on this PR (you can ignore the java ones) |
|
|
@jaypanchal-13 right, you can see the errors here |
|
@damagatchi retest this please |
Product Description
Ticket -> https://dimagi.atlassian.net/browse/CCCT-1839
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review