Periodic Notifications Sync #3384
Conversation
…g multiple notiifcations fetches
shubham1g5
requested review from
Jignesh-dimagi,
OrangeAndGreen and
conroy-ricketts
shubham1g5
changed the title
| private const val SYNC_BACKOFF_DELAY_IN_MILLIS: Long = 3 * 60 * 1000L // min 3 minutes | ||
| private const val NOTIFICATION_RETRIEVAL_PERIODIC_WORKER_TAG = "notification_retrieval_periodic_worker" | ||
| private const val PERIODICITY_FOR_NOTIFICATION_RETRIEVAL_IN_HOURS = 1L | ||
| private const val NOTIFICATION_RETRIEVAL_BACKOFF_DELAY_FOR_RETRIEVAL_RETRY = 10L |
There was a problem hiding this comment.
Nit: Specify IN_MINUTES?
There was a problem hiding this comment.
It's a long name already, so maybe just as a comment instead...
|
|
||
|
|
||
| companion object { | ||
| private const val SYNC_BACKOFF_DELAY_IN_MILLIS: Long = 3 * 60 * 1000L // min 3 minutes |
There was a problem hiding this comment.
Nit: Could just make it minutes here (like the other backoff below) to be more consistent as well as save some math
| private fun startWorkRequest(notificationPayload: Map<String, String>, action: SyncAction, uniqueWorkName: String) { | ||
| startWorkRequest(notificationPayload, action, uniqueWorkName, syncType) | ||
| } |
There was a problem hiding this comment.
@shubham1g5 This method seems redundant and not required, can directly call second startWorkRequest
There was a problem hiding this comment.
Used Kotlin default params instead - 2dfd31b
| if (!isNotificationSyncScheduled) { | ||
| // we want to get info on pending notifications irrespective of whether there are notification related FCMs or not | ||
| startPersonalIdNotificationsWorker(emptyMap()) | ||
| } |
There was a problem hiding this comment.
@shubham1g5 We should check signaling=true also as we want to sync only if push notification is related to connect and ignore for other types of notifications i.e. commcare
There was a problem hiding this comment.
@Jignesh-dimagi there is no push notification payload to show for this sync (it's not coming from fcm but more of a post-fcm sync task)
There was a problem hiding this comment.
@shubham1g5 I mean if application receives some commcare notification, its not required to call startPersonalIdNotificationsWorker so check for signaling=true which ensure that connect notification being received.
Also, this will again call the sync notification API from here
| notificationPayload, | ||
| SyncAction.SYNC_PERSONALID_NOTIFICATIONS, | ||
| SyncAction.SYNC_PERSONALID_NOTIFICATIONS.toString(), | ||
| SyncType.OTHER |
There was a problem hiding this comment.
@shubham1g5 In this case, it will never show the connect message push notification on tray. This should use the SyncType of the this class only.
|
@Jignesh-dimagi I am going to merge it in order to move ahead with this work but happy to take the conversation going and resolve it in the base PR. |
Product Description
https://dimagi.atlassian.net/browse/CCCT-1805
Alternate approach to #3376 to consolidate common code and address the concern raised here
Review commit by commit for ease (Diff is quite bad as a whole due to renaming)
Technical Summary
Instead of having 2 different worker managers and workers for notifications sync, I am now using the existing framework to sync notifications. This also removes earlier code to sync messages as messages are now synced through the notifications worker itself.
Safety Assurance
Safety story
QA Plan
Part of https://dimagi.atlassian.net/browse/QA-8180
Labels and Review