Push notification functionality #3360
Conversation
…625-ui-api-pn-history
…625-ui-api-pn-history
…nto CCCT-1625-ui-api-pn-history
…-ui-api-pn-history
…625-ui-api-pn-history
jaypanchal-13
requested review from
Jignesh-dimagi,
OrangeAndGreen and
shubham1g5
📝 WalkthroughWalkthroughAdds push notifications UI and data flow: new activity layout and item layout, adapter for RecyclerView, icons, color, and “no notifications” strings in multiple locales. Introduces PushNotificationViewModel to load and observe notifications. Integrates database helper changes (return saved IDs, bulk update function), new network endpoints and service methods to retrieve and update notifications, and a parser for retrieval responses. Updates PushNotificationActivity to initialize ViewModel, bind adapter, observe LiveData, and trigger fetch/refresh. Adds a new V19 record class, extends existing record with an acknowledged field, and implements a database version upgrade (19→20) to migrate data. Updates tests to include the new externalizable class. Sequence Diagram(s)sequenceDiagram
autonumber
participant UA as User/Activity
participant VM as PushNotificationViewModel
participant DB as NotificationRecordDatabaseHelper
participant API as PersonalIdApiHandler
participant NET as ApiService
participant PR as RetrieveNotificationsParser
UA->>VM: loadNotifications()
activate VM
VM->>DB: load cached notifications
DB-->>VM: cached list
VM-->>UA: allNotifications LiveData update
VM->>API: retrieveNotifications(userId, password)
activate API
API->>NET: GET RETRIEVE_NOTIFICATIONS (Authorization)
NET-->>API: HTTP 200 + body
API-->>VM: InputStream
deactivate API
VM->>PR: parse(InputStream)
PR-->>VM: List<PushNotificationRecord>
VM->>DB: storeNotifications(records)
DB-->>VM: saved IDs
VM-->>UA: allNotifications LiveData update
deactivate VM
rect rgb(245,245,245)
note over UA,NET: On error
API-->>VM: error code/exception
VM-->>UA: fetchApiError LiveData
end
sequenceDiagram
autonumber
participant UA as User/Activity
participant VM as PushNotificationViewModel
participant DB as NotificationRecordDatabaseHelper
participant API as PersonalIdApiHandler
participant NET as ApiService
UA->>VM: updateNotifications(userId, pwd, notificationIds)
VM->>API: updateNotifications(...)
API->>NET: POST UPDATE_NOTIFICATIONS (Authorization, Form)
NET-->>API: HTTP 200
API-->>VM: success
VM->>DB: updateColumnForNotifications(ids){ it.acknowledged = true }
DB-->>VM: updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 14
🧹 Nitpick comments (13)
app/res/layout/activity_push_notification.xml (1)
22-35: Consider using a dimension resource for text size.The empty state TextView is properly configured with center alignment and initially hidden. However, the text size is hardcoded as
20sp. For consistency and easier maintenance, consider extracting this to a dimension resource (e.g.,@dimen/empty_state_text_size).app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java (1)
664-687: RenamefromV18tofromV19
In PushNotificationRecord.kt, the static migration methodfromV18(PushNotificationRecordV19)should be renamed tofromV19to align its suffix with the input record class version.app/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParser.kt (2)
11-11: Remove unnecessary empty constructor parentheses.In Kotlin, empty parentheses
()on a class declaration are redundant when the class has no parameters.Apply this diff:
-class RetrieveNotificationsResponseParser<T>() : BaseApiResponseParser<T> { +class RetrieveNotificationsResponseParser<T> : BaseApiResponseParser<T> {
14-15: Consider memory safety when reading large responses.The implementation reads the entire response into memory via
bufferedReader().use { it.readText() }. If the server returns a very large notification list, this could cause out-of-memory errors.If unbounded response sizes are possible, consider streaming or pagination. Otherwise, document the expected response size limit.
app/src/org/commcare/activities/PushNotificationActivity.kt (1)
50-66: Empty click listener implementation.The
onNotificationClickmethod is currently a no-op (Lines 61-63). If this is intentional placeholder for future functionality, consider adding a TODO comment. Otherwise, implement the navigation or action logic.Do you want me to open a new issue to track implementing the click handler, or is this intentionally deferred?
app/src/org/commcare/connect/database/NotificationRecordDatabaseHelper.kt (2)
44-65: Re-fetching after write is inefficient.After writing each notification (Line 55), the code immediately re-fetches the record from storage (Lines 57-61) to verify persistence. This doubles the I/O operations. Consider whether
storage.write()already provides guarantees or returns a success indicator, and rely on that instead.If the storage layer does not provide write confirmation, consider adding that capability rather than re-reading.
67-86: Consider optimizing bulk updates.The method iterates over notification IDs and performs individual read-modify-write operations (Lines 79-84). For large lists, this could be slow. If the underlying storage supports batch updates or transactions, consider leveraging that for better performance.
app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt (5)
48-54: Remove or uncomment the commented code.Lines 48-54 contain commented-out code that triggers automatic notification updates. If this functionality is not needed, remove it. If it's intended for future use, add a TODO comment explaining why it's disabled and when it should be enabled.
Consider either:
- Removing the commented code if not needed
- Adding a TODO/FIXME comment explaining the intent
Example:
- /*if (savedNotificationIds.isNotEmpty()) { - updateNotifications( - user.userId, - user.password, - savedNotificationIds - ) - }*/ + // TODO: Enable automatic acknowledgment after testing + // if (savedNotificationIds.isNotEmpty()) { + // updateNotifications(user.userId, user.password, savedNotificationIds) + // }
27-27: Consider making savedNotificationIds private or read-only.The
savedNotificationIdsproperty is public and mutable, allowing external modification. This breaks encapsulation and could lead to unexpected behavior if modified outside the ViewModel.Apply this diff to make it read-only externally:
- var savedNotificationIds: List<String> = emptyList() + private(set) var savedNotificationIds: List<String> = emptyList()
48-54: Remove or implement the commented code.The commented code suggests an automatic update flow after storing notifications. Either implement this feature or remove the dead code to maintain clarity.
If the feature is not ready, consider adding a TODO comment explaining why it's deferred:
- /*if (savedNotificationIds.isNotEmpty()) { - updateNotifications( - user.userId, - user.password, - savedNotificationIds - ) - }*/ + // TODO: Implement automatic acknowledgment flow after requirements are finalized
27-27: Consider making savedNotificationIds private or read-only.The
savedNotificationIdsproperty is declared asvarwith public visibility, allowing external mutation. If it's only meant to be written by this ViewModel, consider making itprivateor exposing it as a read-only property.- var savedNotificationIds: List<String> = emptyList() + private var savedNotificationIds: List<String> = emptyList()
48-54: Commented-out code indicates incomplete implementation.The commented-out automatic update trigger suggests this feature is incomplete or deferred. Consider either implementing this functionality or removing the commented code to maintain clarity.
Do you want me to open a new issue to track implementing automatic notification updates after fetching, or would you prefer to remove this commented code?
app/src/org/commcare/android/database/connect/models/PushNotificationRecordV19.kt (1)
10-11: Add serialVersionUID for Serializable class.The class implements
Serializablebut doesn't declare aserialVersionUID. While not strictly required, it's a best practice to explicitly define it to prevent deserialization issues across different versions.@Table(PushNotificationRecordV19.STORAGE_KEY) class PushNotificationRecordV19 : Persisted(), Serializable { + companion object { + private const val serialVersionUID = 1L + const val STORAGE_KEY = "push_notification_history" + // ... rest of constants + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
app/res/drawable/ic_delivery_pn.xml(1 hunks)app/res/drawable/ic_dollar_payment_pn.xml(1 hunks)app/res/layout/activity_push_notification.xml(1 hunks)app/res/layout/item_push_notification.xml(1 hunks)app/res/values-es/strings.xml(1 hunks)app/res/values-fr/strings.xml(1 hunks)app/res/values-hi/strings.xml(1 hunks)app/res/values-pt/strings.xml(1 hunks)app/res/values-sw/strings.xml(1 hunks)app/res/values-ti/strings.xml(1 hunks)app/res/values/colors.xml(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/activities/PushNotificationActivity.kt(2 hunks)app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt(1 hunks)app/src/org/commcare/adapters/PushNotificationAdapter.kt(1 hunks)app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt(3 hunks)app/src/org/commcare/android/database/connect/models/PushNotificationRecordV19.kt(1 hunks)app/src/org/commcare/connect/database/NotificationRecordDatabaseHelper.kt(1 hunks)app/src/org/commcare/connect/network/ApiEndPoints.java(1 hunks)app/src/org/commcare/connect/network/ApiPersonalId.java(1 hunks)app/src/org/commcare/connect/network/ApiService.java(1 hunks)app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java(3 hunks)app/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParser.kt(1 hunks)app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java(3 hunks)app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java(1 hunks)app/unit-tests/src/org/commcare/android/tests/processing/FormStorageTest.java(1 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt
- app/src/org/commcare/android/database/connect/models/PushNotificationRecordV19.kt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-21T18:20:43.883Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:206-213
Timestamp: 2025-01-21T18:20:43.883Z
Learning: In the Connect ID feature, server responses containing user data should be parsed using ConnectUserResponseParser to ensure consistent handling of payment information (owner_name and phone_number) and secondary phone verification across all authentication flows.
Applied to files:
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java
🧬 Code graph analysis (4)
app/src/org/commcare/connect/network/ApiPersonalId.java (4)
app/src/org/commcare/network/HttpUtils.java (1)
HttpUtils(27-100)app/src/org/commcare/connect/network/connectId/PersonalIdApiClient.java (1)
PersonalIdApiClient(10-30)app/src/org/commcare/connect/network/ApiEndPoints.java (1)
ApiEndPoints(3-25)app/src/org/commcare/connect/network/ConnectNetworkHelper.java (1)
ConnectNetworkHelper(46-464)
app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java (2)
app/src/org/commcare/android/database/connect/models/PushNotificationRecordV19.kt (1)
PushNotificationRecordV19(10-77)app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt (2)
PushNotificationRecord(12-136)fromV18(117-134)
app/src/org/commcare/connect/network/ApiService.java (2)
app/src/org/commcare/connect/network/ApiEndPoints.java (1)
ApiEndPoints(3-25)app/src/org/commcare/connect/database/NotificationRecordDatabaseHelper.kt (1)
getAllNotifications(16-18)
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (3)
app/src/org/commcare/connect/network/ApiPersonalId.java (1)
ApiPersonalId(52-483)app/src/org/commcare/connect/network/base/BaseApiHandler.kt (1)
createCallback(50-68)app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt (1)
updateNotifications(69-88)
⏰ 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 (42)
app/res/values-pt/strings.xml (1)
497-497: Portuguese copy looks goodThe fallback message reads naturally and matches the English source. Thanks for keeping the locale in sync.
app/res/values/colors.xml (1)
223-223: New color aligns with design palette
pn_item_grey_coloris a sensible neutral tone for separators and fits the existing palette. 👍app/res/values/strings.xml (1)
676-676: Base string addition is spot onGlad to see the default locale updated—the new key will keep the empty state consistent across languages.
app/res/values-sw/strings.xml (1)
497-497: Swahili translation reads wellThe Swahili copy communicates the empty-state intent clearly and mirrors the base string.
app/res/values-es/strings.xml (1)
489-489: Spanish translation approved“Notificaciones” phrasing matches the English message and keeps terminology consistent with prior strings.
app/res/drawable/ic_dollar_payment_pn.xml (1)
1-17: Vector asset looks production-readyViewport/viewport scaling and path data are clean; no performance or rendering concerns.
app/res/values-ti/strings.xml (1)
481-481: Tigrinya copy aligns with sourceThanks for covering this locale as well; the phrasing conveys the “no notifications” state clearly.
app/res/drawable/ic_delivery_pn.xml (1)
1-24: Delivery icon SVG is well-structuredFill and stroke choices match the new notification styling, and the clip-path keeps the artwork tight.
app/res/values-hi/strings.xml (1)
486-486: LGTM!The Hindi localization string for the empty notification state is correctly added and properly positioned in the file.
app/res/values-fr/strings.xml (1)
491-491: LGTM!The French localization string for the empty notification state is correctly added and properly positioned in the file.
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java (1)
65-67: LGTM!The database version bump and changelog entry are consistent with the PR objectives. The change is properly documented.
app/unit-tests/src/org/commcare/android/tests/processing/FormStorageTest.java (1)
406-406: LGTM!The addition of
PushNotificationRecordV19to the externalizable class list ensures proper migration test coverage. This aligns with the database migration path introduced in this PR.app/res/layout/activity_push_notification.xml (2)
2-8: LGTM!The root element change to
ConstraintLayoutwith the app namespace is appropriate for the new constraint-based layout containing the RecyclerView and empty state view.
10-20: LGTM!The RecyclerView configuration is correct:
- Uses
0dpfor width/height with proper constraints (match_constraint pattern)- Specifies
LinearLayoutManagerfor vertical list layout- Includes
tools:listitemfor design-time previewapp/src/org/commcare/connect/network/ApiEndPoints.java (1)
23-24: LGTM!The new notification endpoint constants follow the established naming conventions and REST path patterns. The paths are descriptive and properly formatted.
app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java (2)
33-33: LGTM!The import is necessary for the database migration from version 19 to 20.
135-138: LGTM!The upgrade path from version 19 to 20 follows the established pattern and correctly increments the version after migration.
app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt (2)
62-64: LGTM!The
acknowledgedfield is properly added with:
- Sequential
@Persisting(13)annotation- Correct
@MetaFieldreference to the constant- Safe default value of
falseThis aligns with the PR objective to add acknowledgment tracking for push notifications.
81-81: LGTM!The constant
META_ACKNOWLEDGEDfollows the established naming convention and is properly defined.app/src/org/commcare/connect/network/ApiService.java (1)
94-100: LGTM!The new Retrofit endpoint declarations follow the established patterns in this file and are consistent with the API design.
app/res/layout/item_push_notification.xml (4)
10-17: LGTM!The icon constraints and fixed size work correctly. The
tools:srcattribute is helpful for layout preview.
19-31: LGTM!The text view is properly constrained to span between the icon and the forward arrow, with appropriate vertical alignment.
33-41: LGTM!The forward arrow image is correctly aligned to the end and vertically centered with the text.
43-50: LGTM!The divider view is appropriately sized and positioned below the notification text.
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (1)
166-174: LGTM!The new delegation methods follow the established pattern in this handler class and correctly use the appropriate parsers for retrieve and update operations.
app/src/org/commcare/activities/PushNotificationActivity.kt (4)
27-29: LGTM!The initialization sequence is well-structured: views are initialized, observers are set up, and data is fetched.
32-48: LGTM!The observers correctly update UI visibility based on notification list state and handle errors gracefully with user-facing messages.
68-70: LGTM!The fetch method correctly delegates to the ViewModel's
loadNotificationsmethod.
84-86: LGTM!The menu item handler correctly triggers a fresh fetch of notifications.
app/src/org/commcare/connect/database/NotificationRecordDatabaseHelper.kt (1)
67-86: No error handling for updateAction.If the caller-supplied
updateActionlambda throws an exception, the current implementation will propagate it and halt the loop. Consider whether partial updates should be allowed or whether the entire operation should be transactional.Confirm the intended behavior: should one failing update abort the entire batch, or continue with remaining records?
app/src/org/commcare/connect/network/ApiPersonalId.java (2)
318-325: LGTM!The
retrieveNotificationsmethod follows the established authentication and API call pattern in this file.
327-337: LGTM!The
updateNotificationsmethod correctly builds a JSON request body (withuseFormEncoding=falseon Line 334) for the list of notification IDs and follows the standard flow.app/src/org/commcare/adapters/PushNotificationAdapter.kt (3)
19-34: LGTM!The adapter setup with ListAdapter and ViewHolder binding is correct.
36-61: LGTM!The binding logic correctly maps notification properties to views, applies action-specific icons, and wires up click listeners.
63-65: LGTM!The click listener interface is well-defined.
app/src/org/commcare/android/database/connect/models/PushNotificationRecordV19.kt (5)
10-60: LGTM! Well-structured persistence model.The field annotations, indices, and default values are correctly defined. The sequential
@Persistingindices (1-12) are properly ordered, and the metadata keys follow a consistent naming convention.Note: The
createdDatefield defaults toDate(), which creates the timestamp at object instantiation. This is appropriate for database deserialization but ensure the actual creation timestamp is set by the caller when creating new records programmatically.
61-77: LGTM! Constants are consistent with field annotations.All
META_*constants correctly match the@MetaFieldannotations in the field declarations. TheSTORAGE_KEYis properly used in the@Tableannotation, maintaining consistency throughout the class.
10-77: LGTM! The class structure follows persistence framework conventions.The class is properly structured:
- Sequential and unique
@Persistingindices (1-12)- Consistent
@MetaFieldannotations matching the companion object constants- Appropriate default values for all fields
- Correct
@Tableannotation and interface implementationsThe V19 designation and the AI summary indicate this class supports database migration to a newer version (V20) that includes an
acknowledgedfield.
61-76: LGTM!The companion object constants are well-structured and follow a consistent naming convention. The storage key and metadata field constants provide clear documentation for the database schema.
33-35: Avoid usingDate()as the default forcreatedDate
Date()runs at instantiation, not on deserialization—new columns or null values will fall back to the creation timestamp rather than a stored or intended default. Manually verify your migration and field‐mapping logic populate this column correctly; otherwise make it nullable and assign defaults explicitly.app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt (2)
26-26: Handle possible null from getUser. Confirm whetherConnectUserDatabaseUtil.getUser(application)can return null; if it can, guard or assert non-null before accessinguser.userIdoruser.password.
40-42: Clarify the data merge strategy.The current approach appends API results to cached data and then deduplicates using
distinctBy, which retains the last occurrence. This means:
- If a notification exists in both cache and API response with different data, the API version wins.
- If the cache contains notifications that are not in the API response, they remain in the UI.
This could lead to stale or duplicate data. Consider whether you want to:
- Replace cached data entirely with API results
- Merge based on timestamps or other criteria
- Filter out notifications not present in the API response
Verify the intended behavior with the product team and clarify the merge strategy.
| private val _fetchApiError = | ||
| MutableLiveData<Pair<BaseApiHandler.PersonalIdOrConnectApiErrorCodes, Throwable?>>() | ||
| val fetchApiError: LiveData<Pair<BaseApiHandler.PersonalIdOrConnectApiErrorCodes, Throwable?>> = | ||
| _fetchApiError |
There was a problem hiding this comment.
Dead LiveData: fetchApiError is never populated.
The fetchApiError LiveData is declared but never assigned in any of the onFailure handlers (lines 57-61, 81-85), making it effectively dead code.
Apply this diff to populate the error LiveData in both failure handlers:
override fun onFailure(
failureCode: PersonalIdOrConnectApiErrorCodes, t: Throwable?
) {
-
+ _fetchApiError.postValue(Pair(failureCode, t))
}And similarly for the updateNotifications failure handler:
override fun onFailure(
failureCode: PersonalIdOrConnectApiErrorCodes, t: Throwable?
) {
-
+ _fetchApiError.postValue(Pair(failureCode, t))
}🤖 Prompt for AI Agents
In
app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt
around lines 17-20, the MutableLiveData _fetchApiError is declared but never
updated; update the onFailure handlers (around lines 57-61, 81-85 and the
updateNotifications failure handler) to set _fetchApiError.value with a Pair
containing the appropriate BaseApiHandler.PersonalIdOrConnectApiErrorCodes value
and the Throwable received (or null if unavailable). Ensure you choose the
matching enum error code per each failure context, pass the caught throwable
from the callback, and reuse the same pattern for the updateNotifications
failure branch so the LiveData is populated whenever an API call fails.
| val allNotifications: LiveData<List<PushNotificationRecord>> = _allNotifications | ||
|
|
||
| private val notificationRecordDbHelper = NotificationRecordDatabaseHelper() | ||
| private val user = ConnectUserDatabaseUtil.getUser(application) |
There was a problem hiding this comment.
User data won't refresh if the user changes.
The user property is initialized once at ViewModel construction. If the user logs out and back in or switches accounts during the app lifecycle, stale user credentials will continue to be used.
Consider fetching the user at the point of use in loadNotifications() rather than caching it at construction time:
class PushNotificationViewModel(application: Application) : AndroidViewModel(application) {
private val _fetchApiError =
MutableLiveData<Pair<BaseApiHandler.PersonalIdOrConnectApiErrorCodes, Throwable?>>()
val fetchApiError: LiveData<Pair<BaseApiHandler.PersonalIdOrConnectApiErrorCodes, Throwable?>> =
_fetchApiError
private val _allNotifications = MutableLiveData<List<PushNotificationRecord>>()
val allNotifications: LiveData<List<PushNotificationRecord>> = _allNotifications
private val notificationRecordDbHelper = NotificationRecordDatabaseHelper()
- private val user = ConnectUserDatabaseUtil.getUser(application)
var savedNotificationIds: List<String> = emptyList()
fun loadNotifications() {
viewModelScope.launch(Dispatchers.IO) {
+ val user = ConnectUserDatabaseUtil.getUser(getApplication())
// Load from DB first
val cachedNotifications =
notificationRecordDbHelper.getAllNotifications(getApplication()).orEmpty()🤖 Prompt for AI Agents
In
app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt
around line 26, the ViewModel caches the user with a private val user =
ConnectUserDatabaseUtil.getUser(application), causing stale credentials if the
user changes; remove this cached property and instead call
ConnectUserDatabaseUtil.getUser(application) inside loadNotifications() (and any
other methods that currently reference the cached user) so the current user is
fetched at time of use; update null checks/early returns accordingly and ensure
no remaining references to the removed property remain in the class.
|
|
||
| private val notificationRecordDbHelper = NotificationRecordDatabaseHelper() | ||
| private val user = ConnectUserDatabaseUtil.getUser(application) | ||
| var savedNotificationIds: List<String> = emptyList() |
There was a problem hiding this comment.
Thread safety concern with public mutable property.
savedNotificationIds is a public mutable property that is written from coroutines (IO dispatcher) and could be read from the main thread or other coroutines. This creates a potential race condition.
Consider one of these solutions:
- Make it private and expose it through a LiveData or StateFlow
- Use an atomic reference or synchronized access
- Use a thread-safe collection
Apply this diff to make it private and expose via LiveData:
- var savedNotificationIds: List<String> = emptyList()
+ private val _savedNotificationIds = MutableLiveData<List<String>>(emptyList())
+ val savedNotificationIds: LiveData<List<String>> = _savedNotificationIdsThen update line 44-47 to:
- savedNotificationIds =
- notificationRecordDbHelper.storeNotifications(
- getApplication(), result
- )
+ _savedNotificationIds.postValue(
+ notificationRecordDbHelper.storeNotifications(
+ getApplication(), result
+ )
+ )| object : PersonalIdApiHandler<List<PushNotificationRecord>>() { | ||
| override fun onSuccess(result: List<PushNotificationRecord>) { | ||
| // append new results to UI | ||
| val currentNotifications = _allNotifications.value.orEmpty() | ||
| val updatedNotifications = currentNotifications + result | ||
| _allNotifications.postValue(updatedNotifications.distinctBy { it.notificationId }) | ||
| // Save API result into DB and get notification IDs for stored notifications | ||
| savedNotificationIds = | ||
| notificationRecordDbHelper.storeNotifications( | ||
| getApplication(), result | ||
| ) | ||
| /*if (savedNotificationIds.isNotEmpty()) { | ||
| updateNotifications( | ||
| user.userId, | ||
| user.password, | ||
| savedNotificationIds | ||
| ) | ||
| }*/ | ||
| } |
There was a problem hiding this comment.
Incomplete error handling in API callback.
The onFailure handler at line 60 is empty, which means API errors are silently ignored and not propagated to the UI through _fetchApiError. Users won't know if the notification fetch failed.
Apply this diff to propagate errors to the UI:
override fun onFailure(
failureCode: PersonalIdOrConnectApiErrorCodes, t: Throwable?
) {
-
+ _fetchApiError.postValue(Pair(failureCode, t))
}📝 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.
| object : PersonalIdApiHandler<List<PushNotificationRecord>>() { | |
| override fun onSuccess(result: List<PushNotificationRecord>) { | |
| // append new results to UI | |
| val currentNotifications = _allNotifications.value.orEmpty() | |
| val updatedNotifications = currentNotifications + result | |
| _allNotifications.postValue(updatedNotifications.distinctBy { it.notificationId }) | |
| // Save API result into DB and get notification IDs for stored notifications | |
| savedNotificationIds = | |
| notificationRecordDbHelper.storeNotifications( | |
| getApplication(), result | |
| ) | |
| /*if (savedNotificationIds.isNotEmpty()) { | |
| updateNotifications( | |
| user.userId, | |
| user.password, | |
| savedNotificationIds | |
| ) | |
| }*/ | |
| } | |
| object : PersonalIdApiHandler<List<PushNotificationRecord>>() { | |
| override fun onSuccess(result: List<PushNotificationRecord>) { | |
| // append new results to UI | |
| val currentNotifications = _allNotifications.value.orEmpty() | |
| val updatedNotifications = currentNotifications + result | |
| _allNotifications.postValue(updatedNotifications.distinctBy { it.notificationId }) | |
| // Save API result into DB and get notification IDs for stored notifications | |
| savedNotificationIds = | |
| notificationRecordDbHelper.storeNotifications( | |
| getApplication(), result | |
| ) | |
| /*if (savedNotificationIds.isNotEmpty()) { | |
| updateNotifications( | |
| user.userId, | |
| user.password, | |
| savedNotificationIds | |
| ) | |
| }*/ | |
| } | |
| override fun onFailure( | |
| failureCode: PersonalIdOrConnectApiErrorCodes, t: Throwable? | |
| ) { | |
| _fetchApiError.postValue(Pair(failureCode, t)) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt
around lines 37 to 55 (and the empty onFailure at ~line 60), the API failure is
ignored; update the PersonalIdApiHandler.onFailure implementation to propagate
the error to the UI by posting to the existing _fetchApiError LiveData (use
postValue for background thread safety), and also log the error for diagnostics;
ensure you don't swallow the exception and that any UI-visible error state is
set consistently so the UI can react.
| val currentNotifications = _allNotifications.value.orEmpty() | ||
| val updatedNotifications = currentNotifications + result | ||
| _allNotifications.postValue(updatedNotifications.distinctBy { it.notificationId }) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify deduplication logic doesn't discard newer notifications.
Using distinctBy { it.notificationId } after appending keeps the first occurrence of each notification ID. If the server returns updated versions of existing cached notifications, those updates will be discarded.
Consider whether the deduplication should prefer newer notifications or merge updates:
- val updatedNotifications = currentNotifications + result
- _allNotifications.postValue(updatedNotifications.distinctBy { it.notificationId })
+ // Build a map with notificationId as key, preferring newer entries
+ val notificationMap = (currentNotifications + result)
+ .groupBy { it.notificationId }
+ .mapValues { it.value.last() } // or implement proper merge logic
+ _allNotifications.postValue(notificationMap.values.toList())🏁 Script executed:
#!/bin/bash
# Search for how PushNotificationRecord equality/updates are handled elsewhere
rg -nP --type=kt -C3 'notificationId.*equals|PushNotificationRecord.*equals|update.*notification'Length of output: 26
🏁 Script executed:
#!/bin/bash
# Find the definition of PushNotificationRecord
rg -n 'data class PushNotificationRecord' -g '*.kt' -C3
rg -n 'class PushNotificationRecord' -g '*.kt' -C3
# Show other deduplication patterns
rg -n 'distinctBy' -g '*.kt' -C3
rg -n 'groupBy.*notificationId' -g '*.kt' -C3Length of output: 2633
Prefer newer notifications when deduplicating (PushNotificationViewModel.kt:40-42)
distinctBy retains the first occurrence, so any updated records in result are dropped. Invert before deduplication to keep the newest entry:
- val updatedNotifications = currentNotifications + result
- _allNotifications.postValue(updatedNotifications.distinctBy { it.notificationId })
+ val updated = (currentNotifications + result)
+ .asReversed()
+ .distinctBy { it.notificationId }
+ .asReversed()
+ _allNotifications.postValue(updated)📝 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.
| val currentNotifications = _allNotifications.value.orEmpty() | |
| val updatedNotifications = currentNotifications + result | |
| _allNotifications.postValue(updatedNotifications.distinctBy { it.notificationId }) | |
| val currentNotifications = _allNotifications.value.orEmpty() | |
| val updated = (currentNotifications + result) | |
| .asReversed() | |
| .distinctBy { it.notificationId } | |
| .asReversed() | |
| _allNotifications.postValue(updated) |
🤖 Prompt for AI Agents
In
app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt
around lines 40 to 42, deduplication currently uses distinctBy on the
concatenation of currentNotifications + result which keeps the first occurrence
and therefore drops newer entries from result; change the merge so newer
notifications win by prepending result before deduplication (or reverse the
combined list, call distinctBy on notificationId, then restore order) so that
the latest items from result are retained.
There was a problem hiding this comment.
@coderabbitai what do you think on it
val updatedNotifications = (result + currentNotifications).distinctBy { it.notificationId }
There was a problem hiding this comment.
@jaypanchal-13, excellent solution! 🎯
Your approach val updatedNotifications = (result + currentNotifications).distinctBy { it.notificationId } is much cleaner and achieves exactly what we need:
✅ Correct behavior: By placing result first, distinctBy will keep the newer server data when there are duplicate notification IDs, discarding the older cached versions.
✅ Simpler: Your one-liner is more readable and maintainable than my asReversed().distinctBy().asReversed() suggestion.
✅ Edge cases handled:
- New notifications from the server are added
- Existing cached notifications are preserved if not in the server response
- Duplicates correctly favor the fresher data
This is a better solution than what I originally proposed. Go ahead with this approach! 👍
🐰✨
| fun updateNotifications(userId: String, password: String, notificationIds: List<String>) { | ||
| viewModelScope.launch(Dispatchers.IO) { | ||
| object : PersonalIdApiHandler<Unit>() { | ||
| override fun onSuccess(result: Unit) { | ||
| notificationRecordDbHelper.updateColumnForNotifications( | ||
| getApplication(), | ||
| savedNotificationIds | ||
| ) { record -> | ||
| record.acknowledged = true | ||
| } |
There was a problem hiding this comment.
Fix: Use the passed parameter instead of the class property.
Line 75 uses savedNotificationIds (the class property) instead of the notificationIds parameter passed to the method. This means the parameter is completely ignored, and the method will always operate on the last saved IDs rather than the specified IDs. This is a logic error that could lead to incorrect notifications being marked as acknowledged.
Apply this diff to fix the issue:
fun updateNotifications(userId: String, password: String, notificationIds: List<String>) {
viewModelScope.launch(Dispatchers.IO) {
object : PersonalIdApiHandler<Unit>() {
override fun onSuccess(result: Unit) {
notificationRecordDbHelper.updateColumnForNotifications(
getApplication(),
- savedNotificationIds
+ notificationIds
) { record ->
record.acknowledged = true
}
}🤖 Prompt for AI Agents
In
app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt
around lines 69 to 78, the method updateNotifications ignores its
notificationIds parameter and uses the class property savedNotificationIds
instead; replace savedNotificationIds with the notificationIds parameter when
calling notificationRecordDbHelper.updateColumnForNotifications so the passed-in
list is used to update/acknowledge the correct records.
| override fun onFailure( | ||
| failureCode: PersonalIdOrConnectApiErrorCodes, t: Throwable? | ||
| ) { | ||
|
|
||
| } |
There was a problem hiding this comment.
Incomplete error handling in update callback.
The onFailure handler is empty, which means update errors are silently ignored. Users won't know if marking notifications as acknowledged failed, potentially leading to duplicate notifications or confusion.
Apply this diff to propagate errors:
override fun onFailure(
failureCode: PersonalIdOrConnectApiErrorCodes, t: Throwable?
) {
-
+ _fetchApiError.postValue(Pair(failureCode, t))
}Note: If update errors need separate handling from fetch errors, consider adding a separate _updateApiError LiveData.
📝 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.
| override fun onFailure( | |
| failureCode: PersonalIdOrConnectApiErrorCodes, t: Throwable? | |
| ) { | |
| } | |
| override fun onFailure( | |
| failureCode: PersonalIdOrConnectApiErrorCodes, t: Throwable? | |
| ) { | |
| _fetchApiError.postValue(Pair(failureCode, t)) | |
| } |
🤖 Prompt for AI Agents
In
app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt
around lines 81-85, the onFailure callback is empty so update errors are
silently ignored; propagate the error to the ViewModel by posting the failure
(failureCode and Throwable) to the existing LiveData error holder (or create and
post to a new _updateApiError LiveData if update errors should be distinct),
ensure any loading state is cleared and relevant UI-visible error message/state
is set, and log the error for diagnostics so failures to mark notifications
acknowledged are visible and can be handled by the UI.
Handle API failures properly.
Similar to loadNotifications(), the empty onFailure handler means update failures will be silently ignored. Users won't know if their acknowledgment failed.
Apply this diff to report errors:
override fun onFailure(
failureCode: PersonalIdOrConnectApiErrorCodes, t: Throwable?
) {
-
+ _fetchApiError.postValue(Pair(failureCode, t))
}📝 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.
| override fun onFailure( | |
| failureCode: PersonalIdOrConnectApiErrorCodes, t: Throwable? | |
| ) { | |
| } | |
| override fun onFailure( | |
| failureCode: PersonalIdOrConnectApiErrorCodes, t: Throwable? | |
| ) { | |
| _fetchApiError.postValue(Pair(failureCode, t)) | |
| } |
🤖 Prompt for AI Agents
In
app/src/org/commcare/activities/connect/viewmodel/PushNotificationViewModel.kt
around lines 81 to 85, the onFailure callback is empty so update failures are
ignored; implement it to log the failureCode and throwable, update the
appropriate LiveData/state to surface an error to the UI (e.g., set an error
LiveData or emit a Result/Error state) and revert any optimistic changes made
when the update was attempted so the UI stays consistent; also ensure any
user-facing message or retry option is triggered (e.g., Toast or error event)
similar to how loadNotifications() reports failures.
| override fun areContentsTheSame( | ||
| oldItem: PushNotificationRecord, | ||
| newItem: PushNotificationRecord | ||
| ): Boolean { | ||
| return oldItem.notificationId == newItem.notificationId | ||
| } |
There was a problem hiding this comment.
DiffCallback may miss content changes.
areContentsTheSame only compares notificationId, which is the same as areItemsTheSame (Lines 68-73). This means changes to other fields like title, readStatus, or acknowledged won't trigger item rebinding, potentially causing stale UI.
Apply this diff to properly detect content changes:
override fun areContentsTheSame(
oldItem: PushNotificationRecord,
newItem: PushNotificationRecord
): Boolean {
- return oldItem.notificationId == newItem.notificationId
+ return oldItem == newItem
}This assumes PushNotificationRecord has a proper equals() implementation. If not, compare relevant fields explicitly:
override fun areContentsTheSame(
oldItem: PushNotificationRecord,
newItem: PushNotificationRecord
): Boolean {
- return oldItem.notificationId == newItem.notificationId
+ return oldItem.title == newItem.title &&
+ oldItem.action == newItem.action &&
+ oldItem.readStatus == newItem.readStatus &&
+ oldItem.acknowledged == newItem.acknowledged
}🤖 Prompt for AI Agents
In app/src/org/commcare/adapters/PushNotificationAdapter.kt around lines 75 to
80, areContentsTheSame currently only compares notificationId (same as
areItemsTheSame) which will miss changes to title, readStatus, acknowledged,
etc.; update it to return either oldItem == newItem (if PushNotificationRecord
implements equals/hashCode correctly) or explicitly compare the relevant fields
(e.g., title, body, readStatus, acknowledged, timestamp) so any content changes
trigger proper rebinding.
There was a problem hiding this comment.
@coderabbitai only comparing ids it will differntiate items no need to compare other items
There was a problem hiding this comment.
@jaypanchal-13, I understand your point, but I'd like to clarify the distinction between these two methods:
areItemsTheSame: Identifies whether two items represent the same entity (by ID). This is correct to compare onlynotificationId.areContentsTheSame: Determines whether the contents of those items have changed, which tells DiffUtil if the item needs to be rebound/updated in the UI.
When both methods only compare IDs, DiffUtil can't detect content changes. Here's a concrete scenario:
- A notification is displayed with
acknowledged = false - User acknowledges it, and the field updates to
acknowledged = true - The list is refreshed with updated data
- Since both methods only compare
notificationId, DiffUtil thinks nothing changed and doesn't rebind the item - The UI still shows the old state
The same issue applies to changes in title, readStatus, or any other field. The current implementation means the RecyclerView won't update when notification contents change—only when items are added or removed.
If the ID truly changes whenever any content changes (making it immutable), then your approach would work. But based on the data model and the new acknowledged field added in this PR, it appears the same notification ID can have changing content.
That said, if you're certain the current behavior meets your requirements, I'll defer to your judgment on the implementation.
| @JvmStatic | ||
| fun fromV18(old: PushNotificationRecordV19): PushNotificationRecord { | ||
| return PushNotificationRecord().apply { | ||
| notificationId = old.notificationId | ||
| notificationType = old.notificationType | ||
| action = old.action | ||
| title = old.title | ||
| body = old.body | ||
| createdDate = old.createdDate | ||
| confirmationStatus = old.confirmationStatus | ||
| opportunityId = old.opportunityId | ||
| connectMessageId = old.connectMessageId | ||
| channel = old.channel | ||
| paymentId = old.paymentId | ||
| readStatus = old.readStatus | ||
| acknowledged = false | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Migration method is correct; verify naming convention.
The fromV18 method properly migrates data from PushNotificationRecordV19:
- Copies all existing fields
- Sets
acknowledgedtofalse(safe default for existing notifications) - Uses
@JvmStaticfor Java interoperability
However, the method name fromV18 appears inconsistent with the parameter type PushNotificationRecordV19. Verify whether this follows an internal convention (e.g., referring to database schema version) or if it should be renamed to fromV19 for clarity.
🏁 Script executed:
#!/bin/bash
# Search for similar fromVX naming patterns across the codebase to understand the convention
rg -n "fromV\d+.*Record" --type kt --type java -B 2 -A 5Length of output: 26
🏁 Script executed:
#!/bin/bash
# Search across Kotlin and Java files for migration methods named fromVX
rg -nP "fun fromV\\d+" -g "*.kt" -g "*.java" -C3Length of output: 848
Rename fromV18 to fromV19
The migration method takes a PushNotificationRecordV19; its name should be fromV19 to match the parameter version.
🤖 Prompt for AI Agents
In
app/src/org/commcare/android/database/connect/models/PushNotificationRecord.kt
around lines 117–134, the static factory is misnamed fromV18 even though it
accepts a PushNotificationRecordV19; rename the function to fromV19 and update
all references/usages to call PushNotificationRecord.fromV19(...); keep the
parameter type, body, and @JvmStatic annotation unchanged so binary
compatibility is preserved, and run project-wide search to update any tests or
migration registration that referenced fromV18.
| override fun parse(responseCode: Int, responseData: InputStream, anyInputObject: Any?): T { | ||
| val jsonText = responseData.bufferedReader().use { it.readText() } | ||
| val jsonArray = JSONArray(jsonText) | ||
| val result = PushNotificationRecord.fromJsonArray(jsonArray) | ||
| return result as T | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify type safety of the unchecked cast.
The method performs an unchecked cast result as T on Line 17. Since fromJsonArray returns a List<PushNotificationRecord>, callers must ensure T matches this type, otherwise a ClassCastException will occur at runtime.
Confirm that all call sites consistently use RetrieveNotificationsResponseParser<List<PushNotificationRecord>>() (or a compatible supertype) so the cast is safe. Run the following script to find usages:
🏁 Script executed:
#!/bin/bash
# Find all instantiations of RetrieveNotificationsResponseParser
rg -nP --type=java --type=kotlin -C3 '\bRetrieveNotificationsResponseParser\s*<'Length of output: 1881
🏁 Script executed:
#!/usr/bin/env bash
file="app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java"
echo "=== Imports ==="
sed -n '1,50p' "$file"
echo -e "\n=== retrieveNotifications snippet ==="
sed -n '150,200p' "$file"
echo -e "\n=== createCallback references ==="
rg -n 'createCallback' -C3Length of output: 19097
🏁 Script executed:
#!/usr/bin/env bash
echo "=== BaseApiResponseParser.kt ==="
rg -n 'interface BaseApiResponseParser' -C3
echo -e "\n=== PersonalIdApiResponseParser ==="
rg -n 'interface PersonalIdApiResponseParser' -C3Length of output: 1541
🏁 Script executed:
#!/usr/bin/env bash
echo "=== Subclasses of PersonalIdApiHandler ==="
rg -n 'class .* extends PersonalIdApiHandler' -C3
echo -e "\n=== Instantiations of PersonalIdApiHandler ==="
rg -n 'new PersonalIdApiHandler' -C3Length of output: 4831
Broken cast in RetrieveNotificationsResponseParser.parse()
fromJsonArray returns a List<PushNotificationRecord>, but the code does an unchecked result as T, yet all existing handlers use T=PersonalIdSessionData (or other types), leading to a guaranteed ClassCastException. Refactor this parser to return a concrete List<PushNotificationRecord> or require callers to instantiate the handler with T=List<PushNotificationRecord>.
🤖 Prompt for AI Agents
In
app/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParser.kt
around lines 13 to 18, the parser currently performs an unsafe cast result as T
which causes a guaranteed ClassCastException because
PushNotificationRecord.fromJsonArray returns List<PushNotificationRecord>. Fix
by making the parser return a concrete List<PushNotificationRecord> (change the
method signature to override fun parse(...): List<PushNotificationRecord>) and
update any callers or handler registration to expect
List<PushNotificationRecord>, or alternatively make the parser's generic type
explicitly List<PushNotificationRecord> so callers instantiate the handler with
T = List<PushNotificationRecord>; remove the unchecked cast and return the
result directly.
|
|
||
| R.id.notification_cloud_sync -> { | ||
| //api call to sync notification | ||
| fetchAllNotifications() |
There was a problem hiding this comment.
@jaypanchal-13 This should only fetch from API and append items received in it, instead of loading again entire list from local DB too.
| override fun onFailure( | ||
| failureCode: PersonalIdOrConnectApiErrorCodes, t: Throwable? | ||
| ) { | ||
| _fetchApiError.postValue(failureCode to t) | ||
| } | ||
| }.updateNotifications(getApplication(), userId, password, savedNotificationIds) |
There was a problem hiding this comment.
@jaypanchal-13 If there is failure in updating the notification, we should not show any error message to user.
| override fun onNotificationClick(notificationRecord: PushNotificationRecord) { | ||
|
|
||
| } |
There was a problem hiding this comment.
You can start marking the pn read status as true
| getNotificationById(context, incoming.notificationId)?.let { existing -> | ||
| incoming.id = existing.id | ||
| } | ||
| storage.write(incoming) |
There was a problem hiding this comment.
No need to store again if already exists in local db
| fun onNotificationClick(notificationRecord: PushNotificationRecord) | ||
| } | ||
|
|
||
| companion object DiffCallback : DiffUtil.ItemCallback<PushNotificationRecord>() { |
There was a problem hiding this comment.
Rename to NotificationDiffCallback
| override fun onBindViewHolder(holder: PushNotificationViewHolder, position: Int) { | ||
| val item = getItem(position) | ||
| with(holder.binding) { | ||
| tvNotification.text = item.title |
There was a problem hiding this comment.
I think we need to use the body instead of title. Title may be Payment received or New Connect Message so user cannot distinguish really the PN. But body has the description like You have received a payment ofUSD 100.00 for Sample 9. so user can distinguish each PN
| val cachedNotifications = | ||
| notificationRecordDbHelper.getAllNotifications(getApplication()).orEmpty() | ||
| _allNotifications.postValue(cachedNotifications) |
There was a problem hiding this comment.
App needs to show the latest notification first and I think this will show reverse.
| /** | ||
| * Update notifications for a list of notification IDs | ||
| */ | ||
| fun updateNotifications(userId: String, password: String, savedNotificationIds: List<String>) { |
There was a problem hiding this comment.
No need to pass userId and password, this function can directly use from user object
|
@damagatchi retest this please |
| val savedNotificationIds = | ||
| notificationRecordDbHelper.storeNotifications( | ||
| getApplication(), result | ||
| ) | ||
| /*if (savedNotificationIds.isNotEmpty()) { | ||
| updateNotifications( | ||
| user.userId, | ||
| user.password, | ||
| savedNotificationIds | ||
| ) | ||
| }*/ |
There was a problem hiding this comment.
@jaypanchal-13 Thinking to move the logic for storing and updating the notification in RetrieveNotificationParser only as this retrieve_notification is required to be called from different places, and we can avoid repeating this code.
There was a problem hiding this comment.
@Jignesh-dimagi Yes, but there are some logics we need to perform on list with current list(adding, appending), also we need to call update and get(onRefresh also) both api of pn as well. So I am thinking PushNotificationViewModel is better place to handle all these operations
There was a problem hiding this comment.
@jaypanchal-13 We will need some way to wrap this in single method so that whenever retrieve_notification is called, we are not worried about storing and updating the notification. Also, talk is going to store the connect messages from retrieve_notification only so that can also automate if done at single place.
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
had a very rough look and things look good in general. Leaving a DB migration comment as I think it can be simplified negating the need of maintaining a versioned notification model class.
| addTableForNewModel(db, PushNotificationRecord.STORAGE_KEY, new PushNotificationRecord()); | ||
| } | ||
|
|
||
| private void upgradeNineteenTwenty(IDatabase db) { |
There was a problem hiding this comment.
this should just delete the old table and create a new one similar to upgradeSeventeenEighteen
…gi/commcare-android into CCCT-1625-ui-api-pn-history
|
New Changes suggested
| val notifications = NotificationRecordDatabaseHelper.getAllNotifications(activity) | ||
| val hasUnreadNotification = notifications!!.any { !it.acknowledged } |
There was a problem hiding this comment.
@jaypanchal-13 For new notification, API is required to be called, not from local DB, and implement following logic
- If any new notification fetched from API, consider it has unread notification and update the icon accordingly
- if no new notification fetched from API, don't do anything i.e. don't update icon or anything
- When user clicks on notification icon from side nav bar, change the icon to read
There was a problem hiding this comment.
@Jignesh-dimagi I think we are taking care of these read/unread icon in another ticket?
There was a problem hiding this comment.
@Jignesh-dimagi can we put down a ticket here if we are keeping it for another ticket ?
There was a problem hiding this comment.
that doesn't seem to be about managing icon state but just scheduling the notification sync
There was a problem hiding this comment.
Is the code to mark notifications as read when user clicks them already implemented ?
There was a problem hiding this comment.
We need to implement it.
There was a problem hiding this comment.
ok, can we add a new ticket in that case as there are definitely parts of it that doesn't fit into the sync ticket, to me sync ticket is more about just scheduling the worker thread at various places in the code.
| } | ||
| } | ||
|
|
||
| @SuppressLint("SimpleDateFormat") |
There was a problem hiding this comment.
I think we should not suppress as SimpleDateFormat constructor used may not support all locales.
solved last comment removed read/unread logic as we need to manage in another ticket
Product Description
Ticket's-> https://dimagi.atlassian.net/browse/CCCT-1625
https://dimagi.atlassian.net/browse/CCCT-1627
https://dimagi.atlassian.net/browse/CCCT-1628
Note-> update api is not tested yet and commented so that it doesn't get hit
Screenshot
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review