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,13 +47,16 @@ import org.junit.Rule
 import org.junit.Test
 import org.junit.rules.TemporaryFolder
 import kotlin.test.assertEquals
+import kotlin.test.assertTrue
 
 class OfflineFirstNewsRepositoryTest {
 
     private val testScope = TestScope(UnconfinedTestDispatcher())
 
     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/main/java/com/google/samples/apps/nowinandroid/sync/di/SyncModule.kt → 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() {