Update how the app decides whether to accept a push notification

WooCommerce/src/main/kotlin/com/woocommerce/android/notifications/push/NotificationMessageHandler.kt

@@ -19,6 +19,7 @@ import com.woocommerce.android.util.NotificationsParser
 import com.woocommerce.android.util.WooLog
 import com.woocommerce.android.util.WooLog.T.NOTIFICATIONS
 import com.woocommerce.android.viewmodel.ResourceProvider
+import kotlinx.coroutines.runBlocking
 import org.greenrobot.eventbus.EventBus
 import org.wordpress.android.fluxc.Dispatcher
 import org.wordpress.android.fluxc.generated.NotificationActionBuilder
@@ -34,6 +35,7 @@ class NotificationMessageHandler @Inject constructor(
     private val analyticsTracker: NotificationAnalyticsTracker,
     private val notificationsParser: NotificationsParser,
     private val accountStore: AccountStore,
+    private val registrationStatus: PushNotificationRegistrationStatus,
     private val wooLog: WooLog,
     private val dispatcher: Dispatcher,
     private val resourceProvider: ResourceProvider,
@@ -80,24 +82,33 @@ class NotificationMessageHandler @Inject constructor(
             return
         }
 
-        val pushUserId = messageData[PUSH_ARG_USER]
-        // pushUserId is always set server side, but better to double check it here.
-        if (accountStore.account.userId.toString() != pushUserId) {
-            wooLog.e(NOTIFICATIONS, "WP.com userId found in the app doesn't match with the ID in the PN. Aborting.")
-            return
-        }
-
         val notificationModel = notificationsParser.buildNotificationModelFromPayloadMap(messageData)
         if (notificationModel == null) {
             wooLog.e(NOTIFICATIONS, "Notification data is empty!")
             return
         }
 
         val notification = notificationModel.toAppModel(resourceProvider)
-        if (notification.remoteNoteId == 0L) {
+        val registrationStatusResult = runBlocking { registrationStatus(notification.remoteSiteId) }
+        val isRegisteredForWooPush =
+            registrationStatusResult == PushNotificationRegistrationStatus.Status.REGISTERED_WOO_ONLY ||
+                registrationStatusResult == PushNotificationRegistrationStatus.Status.REGISTERED_BOTH
+        val pushUserId = messageData[PUSH_ARG_USER]
+
+        // We need to filter out duplicate notifications from the WPCOM system
+        if (isRegisteredForWooPush) {
+            if (notification.remoteNoteId > 0L) {
+                wooLog.d(NOTIFICATIONS, "Skipping WPCOM notification, already registered with Woo Core")
+                return
+            }
+        } else if (notification.remoteNoteId == 0L) {
             // At this point 'note_id' is always available in the notification bundle.
             wooLog.e(NOTIFICATIONS, "Push notification received without a valid note_id in the payload!")
             return
+        } else if (accountStore.account.userId.toString() != pushUserId) {
+            // At this point, pushUserId is always set server side, but better to double check it here.
+            wooLog.e(NOTIFICATIONS, "WP.com userId found in the app doesn't match with the ID in the PN. Aborting.")
+            return
         }
 
         dispatchBackgroundEvents(notificationModel)
@@ -179,7 +190,7 @@ class NotificationMessageHandler @Inject constructor(
             // We always generate new id for NewOrder.
             activeNotifications
                 .firstOrNull {
-                    it.remoteNoteId == noteId &&
+                    noteId != 0L && it.remoteNoteId == noteId &&
                         it.noteTypeTrackingValue != WooNotificationType.NewOrder.trackingValue
                 }
                 ?.let { return it.id }

WooCommerce/src/main/kotlin/com/woocommerce/android/notifications/push/PushNotificationRegistrationStatus.kt

@@ -15,17 +15,17 @@ class PushNotificationRegistrationStatus @Inject constructor(
         val isWooRegistered = siteId != null && pushNotificationRepository.isWooPushTokenRegisteredForSite(siteId)
         val isWpComRegistered = wpComPushServerId.isNotNullOrEmpty()
         return when {
-            isWooRegistered && isWpComRegistered -> Status.REGISTERED_IN_BOTH
-            isWooRegistered -> Status.WOO_REGISTERED
-            isWpComRegistered -> Status.WPCOM_REGISTERED
+            isWooRegistered && isWpComRegistered -> Status.REGISTERED_BOTH
+            isWooRegistered -> Status.REGISTERED_WOO_ONLY
+            isWpComRegistered -> Status.REGISTERED_WPCOM_ONLY
             else -> Status.UNREGISTERED
         }
     }
 
     enum class Status {
-        WOO_REGISTERED,
-        WPCOM_REGISTERED,
-        REGISTERED_IN_BOTH, // Registered in both WP.com and Woo Core PN systems
+        REGISTERED_WOO_ONLY,
+        REGISTERED_WPCOM_ONLY,
+        REGISTERED_BOTH, // Registered in both WP.com and Woo Core PN systems
         UNREGISTERED
     }
 }

WooCommerce/src/main/kotlin/com/woocommerce/android/notifications/push/RegisterDevice.kt

@@ -24,12 +24,12 @@ class RegisterDevice @Inject constructor(
             IF_NEEDED -> {
                 when (pushRegistrationStatus) {
                     Status.UNREGISTERED -> sendToken()
-                    Status.WPCOM_REGISTERED -> if (FeatureFlag.WOO_PUSH_NOTIFICATIONS_SYSTEM.isEnabled()) {
+                    Status.REGISTERED_WPCOM_ONLY -> if (FeatureFlag.WOO_PUSH_NOTIFICATIONS_SYSTEM.isEnabled()) {
                         sendToken()
                     }
 
-                    Status.WOO_REGISTERED,
-                    Status.REGISTERED_IN_BOTH -> {
+                    Status.REGISTERED_WOO_ONLY,
+                    Status.REGISTERED_BOTH -> {
                     }
                 }
             }

WooCommerce/src/test/kotlin/com/woocommerce/android/notifications/push/NotificationMessageHandlerTest.kt

@@ -52,6 +52,7 @@ class NotificationMessageHandlerTest {
     private val accountStore: AccountStore = mock {
         on { account } doReturn accountModel
     }
+    private val registrationStatus: PushNotificationRegistrationStatus = mock()
     private val dispatcher: Dispatcher = mock()
     private val actionCaptor: KArgumentCaptor<Action<*>> = argumentCaptor()
     private val wooLog: WooLog = mock()
@@ -91,13 +92,14 @@ class NotificationMessageHandlerTest {
     @Before
     fun setUp() {
         notificationMessageHandler = NotificationMessageHandler(
+            notificationBuilder = notificationBuilder,
+            analyticsTracker = notificationAnalyticsTracker,
+            notificationsParser = notificationsParser,
+            registrationStatus = registrationStatus,
             accountStore = accountStore,
             wooLog = wooLog,
             dispatcher = dispatcher,
             resourceProvider = resourceProvider,
-            notificationBuilder = notificationBuilder,
-            analyticsTracker = notificationAnalyticsTracker,
-            notificationsParser = notificationsParser,
             selectedSite = selectedSite,
             workManagerScheduler = workManagerScheduler,
         )
@@ -138,10 +140,13 @@ class NotificationMessageHandlerTest {
     }
 
     @Test
-    fun `when the user id does not match, then do not process the notification`() {
-        notificationMessageHandler.onNewMessageReceived(mapOf("type" to "new_order", "user" to "67890"))
+    fun `when the user id does not match, then do not process the notification`() = runTest {
+        whenever(registrationStatus.invoke(any())).thenReturn(PushNotificationRegistrationStatus.Status.UNREGISTERED)
+        val payload = NotificationTestUtils.generateTestNewOrderNotificationPayload(userId = 67890)
+
+        notificationMessageHandler.onNewMessageReceived(payload)
         verify(accountStore, atLeastOnce()).hasAccessToken()
-        verify(wooLog, only()).e(
+        verify(wooLog).e(
             eq(WooLog.T.NOTIFICATIONS),
             eq("WP.com userId found in the app doesn't match with the ID in the PN. Aborting.")
         )
@@ -159,6 +164,21 @@ class NotificationMessageHandlerTest {
         verify(wooLog, only()).e(eq(WooLog.T.NOTIFICATIONS), eq("Notification data is empty!"))
     }
 
+    @Test
+    fun `given woo push registered, when wpcom notification received, then skip notification`() = runTest {
+        whenever(registrationStatus.invoke(any()))
+            .thenReturn(PushNotificationRegistrationStatus.Status.REGISTERED_WOO_ONLY)
+
+        // WPCOM notifications have remoteNoteId > 0
+        notificationMessageHandler.onNewMessageReceived(orderNotificationPayload)
+
+        verify(wooLog).d(
+            eq(WooLog.T.NOTIFICATIONS),
+            eq("Skipping WPCOM notification, already registered with Woo Core")
+        )
+        verifyNoInteractions(dispatcher)
+    }
+
     @Test
     fun `when an incoming notification is received, then we should update that notification to local cache`() {
         notificationMessageHandler.onNewMessageReceived(orderNotificationPayload)

WooCommerce/src/test/kotlin/com/woocommerce/android/notifications/push/PushNotificationRegistrationStatusTest.kt

@@ -37,7 +37,7 @@ class PushNotificationRegistrationStatusTest : BaseUnitTest() {
 
         val result = sut(TEST_SITE_ID)
 
-        assertThat(result).isEqualTo(Status.REGISTERED_IN_BOTH)
+        assertThat(result).isEqualTo(Status.REGISTERED_BOTH)
     }
 
     @Test
@@ -47,7 +47,7 @@ class PushNotificationRegistrationStatusTest : BaseUnitTest() {
 
         val result = sut(TEST_SITE_ID)
 
-        assertThat(result).isEqualTo(Status.WOO_REGISTERED)
+        assertThat(result).isEqualTo(Status.REGISTERED_WOO_ONLY)
     }
 
     @Test
@@ -57,7 +57,7 @@ class PushNotificationRegistrationStatusTest : BaseUnitTest() {
 
         val result = sut(TEST_SITE_ID)
 
-        assertThat(result).isEqualTo(Status.WPCOM_REGISTERED)
+        assertThat(result).isEqualTo(Status.REGISTERED_WPCOM_ONLY)
     }
 
     @Test
@@ -76,7 +76,7 @@ class PushNotificationRegistrationStatusTest : BaseUnitTest() {
 
         val result = sut(null)
 
-        assertThat(result).isEqualTo(Status.WPCOM_REGISTERED)
+        assertThat(result).isEqualTo(Status.REGISTERED_WPCOM_ONLY)
     }
 
     @Test

WooCommerce/src/test/kotlin/com/woocommerce/android/notifications/push/RegisterDeviceTest.kt

@@ -95,7 +95,7 @@ class RegisterDeviceTest : BaseUnitTest() {
 
     @Test
     fun `given user logged in and WOO_REGISTERED, when invoked forcefully, then registers in WPCom`() = testBlocking {
-        setupStatus(Status.WOO_REGISTERED)
+        setupStatus(Status.REGISTERED_WOO_ONLY)
 
         sut(FORCEFULLY)
 
@@ -104,7 +104,7 @@ class RegisterDeviceTest : BaseUnitTest() {
 
     @Test
     fun `given user logged in and WPCOM_REGISTERED, when invoked forcefully, then registers in WPCom`() = testBlocking {
-        setupStatus(Status.WPCOM_REGISTERED)
+        setupStatus(Status.REGISTERED_WPCOM_ONLY)
 
         sut(FORCEFULLY)
 
@@ -113,7 +113,7 @@ class RegisterDeviceTest : BaseUnitTest() {
 
     @Test
     fun `given REGISTERED_IN_BOTH, when invoked forcefully, then registers in WPCom`() = testBlocking {
-        setupStatus(Status.REGISTERED_IN_BOTH)
+        setupStatus(Status.REGISTERED_BOTH)
 
         sut(FORCEFULLY)
 
@@ -131,7 +131,7 @@ class RegisterDeviceTest : BaseUnitTest() {
 
     @Test
     fun `given WPCOM_REGISTERED, when invoked if needed, then does not register`() = testBlocking {
-        setupStatus(Status.WPCOM_REGISTERED)
+        setupStatus(Status.REGISTERED_WPCOM_ONLY)
 
         sut(IF_NEEDED)
 
@@ -141,7 +141,7 @@ class RegisterDeviceTest : BaseUnitTest() {
 
     @Test
     fun `given WOO_REGISTERED, when invoked if needed, then does not register`() = testBlocking {
-        setupStatus(Status.WOO_REGISTERED)
+        setupStatus(Status.REGISTERED_WOO_ONLY)
 
         sut(IF_NEEDED)
 
@@ -151,7 +151,7 @@ class RegisterDeviceTest : BaseUnitTest() {
 
     @Test
     fun `given REGISTERED_IN_BOTH, when invoked if needed, then does not register`() = testBlocking {
-        setupStatus(Status.REGISTERED_IN_BOTH)
+        setupStatus(Status.REGISTERED_BOTH)
 
         sut(IF_NEEDED)
 

WooCommerce/src/test/kotlin/com/woocommerce/android/ui/dashboard/DashboardViewModelTest.kt

@@ -59,7 +59,7 @@ class DashboardViewModelTest : BaseUnitTest() {
     }
 
     private val pushNotificationRegistrationStatus: PushNotificationRegistrationStatus = mock {
-        onBlocking { invoke(any()) } doReturn Status.WPCOM_REGISTERED
+        onBlocking { invoke(any()) } doReturn Status.REGISTERED_WPCOM_ONLY
     }
     private val feedbackPrefs: FeedbackPrefs = mock {
         onBlocking { userFeedbackIsDueObservable } doReturn flowOf(false)
@@ -351,7 +351,7 @@ class DashboardViewModelTest : BaseUnitTest() {
                         }
                     )
                 )
-                whenever(pushNotificationRegistrationStatus.invoke(any())).thenReturn(Status.WPCOM_REGISTERED)
+                whenever(pushNotificationRegistrationStatus.invoke(any())).thenReturn(Status.REGISTERED_WPCOM_ONLY)
             }
 
             val jetpackBenefitsBanner = viewModel.jetpackBenefitsBannerState.getOrAwaitValue()