Merge pull request #649 from android/tj/backend-requested-sync

Wire up backend requested sync

core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepository.kt

@@ -27,6 +27,7 @@ import com.google.samples.apps.nowinandroid.core.database.model.PopulatedNewsRes
 import com.google.samples.apps.nowinandroid.core.database.model.TopicEntity
 import com.google.samples.apps.nowinandroid.core.database.model.asExternalModel
 import com.google.samples.apps.nowinandroid.core.datastore.ChangeListVersions
+import com.google.samples.apps.nowinandroid.core.datastore.NiaPreferencesDataSource
 import com.google.samples.apps.nowinandroid.core.model.data.NewsResource
 import com.google.samples.apps.nowinandroid.core.network.NiaNetworkDataSource
 import com.google.samples.apps.nowinandroid.core.network.model.NetworkNewsResource
@@ -45,6 +46,7 @@ private const val SYNC_BATCH_SIZE = 40
  * Reads are exclusively from local storage to support offline access.
  */
 class OfflineFirstNewsRepository @Inject constructor(
+    private val niaPreferencesDataSource: NiaPreferencesDataSource,
     private val newsResourceDao: NewsResourceDao,
     private val topicDao: TopicDao,
     private val network: NiaNetworkDataSource,
@@ -72,16 +74,27 @@ class OfflineFirstNewsRepository @Inject constructor(
             },
             modelDeleter = newsResourceDao::deleteNewsResources,
             modelUpdater = { changedIds ->
+                val userData = niaPreferencesDataSource.userData.first()
+                val hasOnboarded = userData.shouldHideOnboarding
+                val followedTopicIds = userData.followedTopics
+
                 // TODO: Make this more efficient, there is no need to retrieve populated
                 //  news resources when all that's needed are the ids
-                val existingNewsResourceIds = newsResourceDao.getNewsResources(
-                    useFilterNewsIds = true,
-                    filterNewsIds = changedIds.toSet(),
-                )
-                    .first()
-                    .map { it.entity.id }
-                    .toSet()
+                val existingNewsResourceIdsThatHaveChanged = when {
+                    hasOnboarded -> newsResourceDao.getNewsResources(
+                        useFilterTopicIds = true,
+                        filterTopicIds = followedTopicIds,
+                        useFilterNewsIds = true,
+                        filterNewsIds = changedIds.toSet(),
+                    )
+                        .first()
+                        .map { it.entity.id }
+                        .toSet()
+                    // No need to retrieve anything if notifications won't be sent
+                    else -> emptySet()
+                }
 
+                // Obtain the news resources which have changed from the network and upsert them locally
                 changedIds.chunked(SYNC_BATCH_SIZE).forEach { chunkedIds ->
                     val networkNewsResources = network.getNewsResources(ids = chunkedIds)
 
@@ -106,19 +119,18 @@ class OfflineFirstNewsRepository @Inject constructor(
                     )
                 }
 
-                val addedNewsResources = newsResourceDao.getNewsResources(
-                    useFilterNewsIds = true,
-                    filterNewsIds = changedIds.toSet(),
-                )
-                    .first()
-                    .filter { !existingNewsResourceIds.contains(it.entity.id) }
-                    .map(PopulatedNewsResource::asExternalModel)
+                if (hasOnboarded) {
+                    val addedNewsResources = newsResourceDao.getNewsResources(
+                        useFilterTopicIds = true,
+                        filterTopicIds = followedTopicIds,
+                        useFilterNewsIds = true,
+                        filterNewsIds = changedIds.toSet() - existingNewsResourceIdsThatHaveChanged,
+                    )
+                        .first()
+                        .map(PopulatedNewsResource::asExternalModel)
 
-                // TODO: Define business logic for notifications on first time sync.
-                //  we probably do not want to send notifications on first install.
-                //  We can easily check if the change list version is 0 and not send notifications
-                // if it is.
-                if (addedNewsResources.isNotEmpty()) notifier.onNewsAdded(addedNewsResources)
+                    if (addedNewsResources.isNotEmpty()) notifier.onNewsAdded(addedNewsResources)
+                }
             },
         )
 }

core/data/src/test/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepositoryTest.kt

@@ -34,6 +34,7 @@ import com.google.samples.apps.nowinandroid.core.database.model.asExternalModel
 import com.google.samples.apps.nowinandroid.core.datastore.NiaPreferencesDataSource
 import com.google.samples.apps.nowinandroid.core.datastore.test.testUserPreferencesDataStore
 import com.google.samples.apps.nowinandroid.core.model.data.NewsResource
+import com.google.samples.apps.nowinandroid.core.model.data.Topic
 import com.google.samples.apps.nowinandroid.core.network.model.NetworkChangeList
 import com.google.samples.apps.nowinandroid.core.network.model.NetworkNewsResource
 import com.google.samples.apps.nowinandroid.core.testing.notifications.TestNotifier
@@ -46,6 +47,7 @@ import org.junit.Rule
 import org.junit.Test
 import org.junit.rules.TemporaryFolder
 import kotlin.test.assertEquals
+import kotlin.test.assertTrue
 
 class OfflineFirstNewsRepositoryTest {
 
@@ -53,6 +55,8 @@ class OfflineFirstNewsRepositoryTest {
 
     private lateinit var subject: OfflineFirstNewsRepository
 
+    private lateinit var niaPreferencesDataSource: NiaPreferencesDataSource
+
     private lateinit var newsResourceDao: TestNewsResourceDao
 
     private lateinit var topicDao: TestTopicDao
@@ -68,17 +72,19 @@ class OfflineFirstNewsRepositoryTest {
 
     @Before
     fun setup() {
+        niaPreferencesDataSource = NiaPreferencesDataSource(
+            tmpFolder.testUserPreferencesDataStore(testScope),
+        )
         newsResourceDao = TestNewsResourceDao()
         topicDao = TestTopicDao()
         network = TestNiaNetworkDataSource()
         notifier = TestNotifier()
         synchronizer = TestSynchronizer(
-            NiaPreferencesDataSource(
-                tmpFolder.testUserPreferencesDataStore(testScope),
-            ),
+            niaPreferencesDataSource,
         )
 
         subject = OfflineFirstNewsRepository(
+            niaPreferencesDataSource = niaPreferencesDataSource,
             newsResourceDao = newsResourceDao,
             topicDao = topicDao,
             network = network,
@@ -130,6 +136,8 @@ class OfflineFirstNewsRepositoryTest {
     @Test
     fun offlineFirstNewsRepository_sync_pulls_from_network() =
         testScope.runTest {
+            // User has not onboarded
+            niaPreferencesDataSource.setShouldHideOnboarding(false)
             subject.syncWith(synchronizer)
 
             val newsResourcesFromNetwork = network.getNewsResources()
@@ -151,16 +159,16 @@ class OfflineFirstNewsRepositoryTest {
                 actual = synchronizer.getChangeListVersions().newsResourceVersion,
             )
 
-            // Notifier should have been called with new news resources
-            assertEquals(
-                expected = newsResourcesFromDb.map(NewsResource::id).sorted(),
-                actual = notifier.addedNewsResources.first().map(NewsResource::id).sorted(),
-            )
+            // Notifier should not have been called
+            assertTrue(notifier.addedNewsResources.isEmpty())
         }
 
     @Test
     fun offlineFirstNewsRepository_sync_deletes_items_marked_deleted_on_network() =
         testScope.runTest {
+            // User has not onboarded
+            niaPreferencesDataSource.setShouldHideOnboarding(false)
+
             val newsResourcesFromNetwork = network.getNewsResources()
                 .map(NetworkNewsResource::asEntity)
                 .map(NewsResourceEntity::asExternalModel)
@@ -198,17 +206,16 @@ class OfflineFirstNewsRepositoryTest {
                 actual = synchronizer.getChangeListVersions().newsResourceVersion,
             )
 
-            // Notifier should have been called with news resources from network that are not
-            // deleted
-            assertEquals(
-                expected = (newsResourcesFromNetwork.map(NewsResource::id) - deletedItems).sorted(),
-                actual = notifier.addedNewsResources.first().map(NewsResource::id).sorted(),
-            )
+            // Notifier should not have been called
+            assertTrue(notifier.addedNewsResources.isEmpty())
         }
 
     @Test
     fun offlineFirstNewsRepository_incremental_sync_pulls_from_network() =
         testScope.runTest {
+            // User has not onboarded
+            niaPreferencesDataSource.setShouldHideOnboarding(false)
+
             // Set news version to 7
             synchronizer.updateChangeListVersions {
                 copy(newsResourceVersion = 7)
@@ -244,11 +251,8 @@ class OfflineFirstNewsRepositoryTest {
                 actual = synchronizer.getChangeListVersions().newsResourceVersion,
             )
 
-            // Notifier should have been called with only added news resources from network
-            assertEquals(
-                expected = newsResourcesFromNetwork.map(NewsResource::id).sorted(),
-                actual = notifier.addedNewsResources.first().map(NewsResource::id).sorted(),
-            )
+            // Notifier should not have been called
+            assertTrue(notifier.addedNewsResources.isEmpty())
         }
 
     @Test
@@ -283,4 +287,70 @@ class OfflineFirstNewsRepositoryTest {
                     .sortedBy(NewsResourceTopicCrossRef::toString),
             )
         }
+
+    @Test
+    fun offlineFirstNewsRepository_sends_notifications_for_newly_synced_news_that_is_followed() =
+        testScope.runTest {
+            // User has onboarded
+            niaPreferencesDataSource.setShouldHideOnboarding(true)
+
+            val networkNewsResources = network.getNewsResources()
+
+            // Follow roughly half the topics
+            val followedTopicIds = networkNewsResources
+                .flatMap(NetworkNewsResource::topicEntityShells)
+                .mapNotNull { topic ->
+                    when (topic.id.chars().sum() % 2) {
+                        0 -> topic.id
+                        else -> null
+                    }
+                }
+                .toSet()
+
+            // Set followed topics
+            niaPreferencesDataSource.setFollowedTopicIds(followedTopicIds)
+
+            subject.syncWith(synchronizer)
+
+            val followedNewsResourceIdsFromNetwork = networkNewsResources
+                .filter { (it.topics intersect followedTopicIds).isNotEmpty() }
+                .map(NetworkNewsResource::id)
+                .sorted()
+
+            // Notifier should have been called with only news resources that have topics
+            // that the user follows
+            assertEquals(
+                expected = followedNewsResourceIdsFromNetwork,
+                actual = notifier.addedNewsResources.first().map(NewsResource::id).sorted(),
+            )
+        }
+
+    @Test
+    fun offlineFirstNewsRepository_does_not_send_notifications_for_existing_news_resources() =
+        testScope.runTest {
+            // User has onboarded
+            niaPreferencesDataSource.setShouldHideOnboarding(true)
+
+            val networkNewsResources = network.getNewsResources()
+                .map(NetworkNewsResource::asEntity)
+
+            val newsResources = networkNewsResources
+                .map(NewsResourceEntity::asExternalModel)
+
+            // Prepopulate dao with news resources
+            newsResourceDao.upsertNewsResources(networkNewsResources)
+
+            val followedTopicIds = newsResources
+                .flatMap(NewsResource::topics)
+                .map(Topic::id)
+                .toSet()
+
+            // Follow all topics
+            niaPreferencesDataSource.setFollowedTopicIds(followedTopicIds)
+
+            subject.syncWith(synchronizer)
+
+            // Notifier should not have been called bc all news resources existed previously
+            assertTrue(notifier.addedNewsResources.isEmpty())
+        }
 }

core/data/src/test/java/com/google/samples/apps/nowinandroid/core/data/testdoubles/TestNewsResourceDao.kt

@@ -47,7 +47,11 @@ class TestNewsResourceDao : NewsResourceDao {
         filterNewsIds: Set<String>,
     ): Flow<List<PopulatedNewsResource>> =
         entitiesStateFlow
-            .map { it.map(NewsResourceEntity::asPopulatedNewsResource) }
+            .map { newsResourceEntities ->
+                newsResourceEntities.map { entity ->
+                    entity.asPopulatedNewsResource(topicCrossReferences)
+                }
+            }
             .map { resources ->
                 var result = resources
                 if (useFilterTopicIds) {
@@ -78,10 +82,6 @@ class TestNewsResourceDao : NewsResourceDao {
         return entities.map { it.id.toLong() }
     }
 
-    override suspend fun updateNewsResources(entities: List<NewsResourceEntity>) {
-        throw NotImplementedError("Unused in tests")
-    }
-
     override suspend fun upsertNewsResources(newsResourceEntities: List<NewsResourceEntity>) {
         entitiesStateFlow.update { oldValues ->
             // New values come first so they overwrite old values
@@ -109,16 +109,20 @@ class TestNewsResourceDao : NewsResourceDao {
     }
 }
 
-private fun NewsResourceEntity.asPopulatedNewsResource() = PopulatedNewsResource(
+private fun NewsResourceEntity.asPopulatedNewsResource(
+    topicCrossReferences: List<NewsResourceTopicCrossRef>,
+) = PopulatedNewsResource(
     entity = this,
-    topics = listOf(
-        TopicEntity(
-            id = filteredInterestsIds.random(),
-            name = "name",
-            shortDescription = "short description",
-            longDescription = "long description",
-            url = "URL",
-            imageUrl = "image URL",
-        ),
-    ),
+    topics = topicCrossReferences
+        .filter { it.newsResourceId == id }
+        .map { newsResourceTopicCrossRef ->
+            TopicEntity(
+                id = newsResourceTopicCrossRef.topicId,
+                name = "name",
+                shortDescription = "short description",
+                longDescription = "long description",
+                url = "URL",
+                imageUrl = "image URL",
+            )
+        },
 )

core/database/src/main/java/com/google/samples/apps/nowinandroid/core/database/dao/NewsResourceDao.kt

@@ -21,7 +21,6 @@ import androidx.room.Insert
 import androidx.room.OnConflictStrategy
 import androidx.room.Query
 import androidx.room.Transaction
-import androidx.room.Update
 import androidx.room.Upsert
 import com.google.samples.apps.nowinandroid.core.database.model.NewsResourceEntity
 import com.google.samples.apps.nowinandroid.core.database.model.NewsResourceTopicCrossRef
@@ -72,12 +71,6 @@ interface NewsResourceDao {
     @Insert(onConflict = OnConflictStrategy.IGNORE)
     suspend fun insertOrIgnoreNewsResources(entities: List<NewsResourceEntity>): List<Long>
 
-    /**
-     * Updates [entities] in the db that match the primary key, and no-ops if they don't
-     */
-    @Update
-    suspend fun updateNewsResources(entities: List<NewsResourceEntity>)
-
     /**
      * Inserts or updates [newsResourceEntities] in the db under the specified primary keys
      */

sync/work/src/demo/java/com/google/samples/apps/nowinandroid/sync/di/SyncModule.kt

@@ -17,6 +17,8 @@
 package com.google.samples.apps.nowinandroid.sync.di
 
 import com.google.samples.apps.nowinandroid.core.data.util.SyncManager
+import com.google.samples.apps.nowinandroid.sync.status.StubSyncSubscriber
+import com.google.samples.apps.nowinandroid.sync.status.SyncSubscriber
 import com.google.samples.apps.nowinandroid.sync.status.WorkManagerSyncManager
 import dagger.Binds
 import dagger.Module
@@ -30,4 +32,9 @@ interface SyncModule {
     fun bindsSyncStatusMonitor(
         syncStatusMonitor: WorkManagerSyncManager,
     ): SyncManager
+
+    @Binds
+    fun bindsSyncSubscriber(
+        syncSubscriber: StubSyncSubscriber,
+    ): SyncSubscriber
 }

sync/work/src/main/java/com/google/samples/apps/nowinandroid/sync/initializers/SyncWorkHelpers.kt

@@ -27,6 +27,7 @@ import androidx.work.ForegroundInfo
 import androidx.work.NetworkType
 import com.google.samples.apps.nowinandroid.sync.R
 
+const val SYNC_TOPIC = "sync"
 private const val SyncNotificationId = 0
 private const val SyncNotificationChannelID = "SyncNotificationChannel"
 

sync/work/src/main/java/com/google/samples/apps/nowinandroid/sync/services/SyncNotificationsService.kt

@@ -19,11 +19,10 @@ package com.google.samples.apps.nowinandroid.sync.services
 import com.google.firebase.messaging.FirebaseMessagingService
 import com.google.firebase.messaging.RemoteMessage
 import com.google.samples.apps.nowinandroid.core.data.util.SyncManager
+import com.google.samples.apps.nowinandroid.sync.initializers.SYNC_TOPIC
 import dagger.hilt.android.AndroidEntryPoint
 import javax.inject.Inject
 
-private const val SYNC_TOPIC = "sync"
-
 @AndroidEntryPoint
 class SyncNotificationsService : FirebaseMessagingService() {
 

sync/work/src/main/java/com/google/samples/apps/nowinandroid/sync/status/StubSyncSubscriber.kt

@@ -0,0 +1,31 @@
+/*
+ * Copyright 2023 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.samples.apps.nowinandroid.sync.status
+
+import android.util.Log
+import javax.inject.Inject
+
+private const val TAG = "StubSyncSubscriber"
+
+/**
+ * Stub implementation of [SyncSubscriber]
+ */
+class StubSyncSubscriber @Inject constructor() : SyncSubscriber {
+    override suspend fun subscribe() {
+        Log.d(TAG, "Subscribing to sync")
+    }
+}

sync/work/src/main/java/com/google/samples/apps/nowinandroid/sync/status/SyncSubscriber.kt

@@ -0,0 +1,24 @@
+/*
+ * Copyright 2023 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.samples.apps.nowinandroid.sync.status
+
+/**
+ * Subscribes to backend requested synchronization
+ */
+interface SyncSubscriber {
+    suspend fun subscribe()
+}

sync/work/src/main/java/com/google/samples/apps/nowinandroid/sync/workers/SyncWorker.kt

@@ -34,6 +34,7 @@ import com.google.samples.apps.nowinandroid.core.network.Dispatcher
 import com.google.samples.apps.nowinandroid.core.network.NiaDispatchers.IO
 import com.google.samples.apps.nowinandroid.sync.initializers.SyncConstraints
 import com.google.samples.apps.nowinandroid.sync.initializers.syncForegroundInfo
+import com.google.samples.apps.nowinandroid.sync.status.SyncSubscriber
 import dagger.assisted.Assisted
 import dagger.assisted.AssistedInject
 import kotlinx.coroutines.CoroutineDispatcher
@@ -54,6 +55,7 @@ class SyncWorker @AssistedInject constructor(
     private val newsRepository: NewsRepository,
     @Dispatcher(IO) private val ioDispatcher: CoroutineDispatcher,
     private val analyticsHelper: AnalyticsHelper,
+    private val syncSubscriber: SyncSubscriber,
 ) : CoroutineWorker(appContext, workerParams), Synchronizer {
 
     override suspend fun getForegroundInfo(): ForegroundInfo =
@@ -63,6 +65,8 @@ class SyncWorker @AssistedInject constructor(
         traceAsync("Sync", 0) {
             analyticsHelper.logSyncStarted()
 
+            syncSubscriber.subscribe()
+
             // First sync the repositories in parallel
             val syncedSuccessfully = awaitAll(
                 async { topicRepository.sync() },

sync/work/src/prod/java/com/google/samples/apps/nowinandroid/sync/di/SyncModule.kt

@@ -0,0 +1,51 @@
+/*
+ * Copyright 2022 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.samples.apps.nowinandroid.sync.di
+
+import com.google.firebase.ktx.Firebase
+import com.google.firebase.messaging.FirebaseMessaging
+import com.google.firebase.messaging.ktx.messaging
+import com.google.samples.apps.nowinandroid.core.data.util.SyncManager
+import com.google.samples.apps.nowinandroid.sync.status.FirebaseSyncSubscriber
+import com.google.samples.apps.nowinandroid.sync.status.SyncSubscriber
+import com.google.samples.apps.nowinandroid.sync.status.WorkManagerSyncManager
+import dagger.Binds
+import dagger.Module
+import dagger.Provides
+import dagger.hilt.InstallIn
+import dagger.hilt.components.SingletonComponent
+import javax.inject.Singleton
+
+@Module
+@InstallIn(SingletonComponent::class)
+interface SyncModule {
+    @Binds
+    fun bindsSyncStatusMonitor(
+        syncStatusMonitor: WorkManagerSyncManager,
+    ): SyncManager
+
+    @Binds
+    fun bindsSyncSubscriber(
+        syncSubscriber: FirebaseSyncSubscriber,
+    ): SyncSubscriber
+
+    companion object {
+        @Provides
+        @Singleton
+        fun provideFirebaseMessaging(): FirebaseMessaging = Firebase.messaging
+    }
+}

sync/work/src/prod/java/com/google/samples/apps/nowinandroid/sync/status/FirebaseSyncSubscriber.kt

@@ -0,0 +1,35 @@
+/*
+ * Copyright 2023 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.samples.apps.nowinandroid.sync.status
+
+import com.google.firebase.messaging.FirebaseMessaging
+import com.google.samples.apps.nowinandroid.sync.initializers.SYNC_TOPIC
+import kotlinx.coroutines.tasks.await
+import javax.inject.Inject
+
+/**
+ * Implementation of [SyncSubscriber] that subscribes to the FCM [SYNC_TOPIC]
+ */
+class FirebaseSyncSubscriber @Inject constructor(
+    private val firebaseMessaging: FirebaseMessaging,
+) : SyncSubscriber {
+    override suspend fun subscribe() {
+        firebaseMessaging
+            .subscribeToTopic(SYNC_TOPIC)
+            .await()
+    }
+}