Refactor out single use logic from SurveyRepository to existing use c…

…ases (#3017)

* Simplify tests for MakeSurveyAvailableOfflineUseCase

* Cleanup tests for removeOfflineSurvey()

This is in preparation for moving loadAndSyncSurveyWithRemote() out of SurveyRepository

* Move logic for fetching and saving survey to SyncSurveyUseCase

* Inline subscribeToSurveyUpdates in MakeSurveyAvailableOfflineUseCase

* Fix detekt warnings

* Merge survey active check with ReactivateLastSurveyUseCase

* Create methods for activating survey instead of directly accessing internal variable

* Automatically added GitHub issue links to TODOs

app/src/main/java/com/google/android/ground/MainViewModel.kt

@@ -115,7 +115,7 @@ constructor(
       userRepository.saveUserDetails(user)
       if (!isTosAccepted()) {
         MainUiState.TosNotAccepted
-      } else if (!attemptToReactiveLastActiveSurvey()) {
+      } else if (!reactivateLastSurvey()) {
         MainUiState.NoActiveSurvey
       } else {
         // Everything is fine, show the home screen
@@ -127,10 +127,4 @@ constructor(
 
   /** Returns true if the user has already accepted the Terms of Service. */
   private fun isTosAccepted(): Boolean = termsOfServiceRepository.isTermsOfServiceAccepted
-
-  /** Returns true if the last survey was successfully reactivated, if any. */
-  private suspend fun attemptToReactiveLastActiveSurvey(): Boolean {
-    reactivateLastSurvey()
-    return surveyRepository.selectedSurveyId != null
-  }
 }

app/src/main/java/com/google/android/ground/domain/usecases/survey/ActivateSurveyUseCase.kt

@@ -16,32 +16,36 @@
 
 package com.google.android.ground.domain.usecases.survey
 
+import com.google.android.ground.persistence.local.stores.LocalSurveyStore
+import com.google.android.ground.persistence.sync.SurveySyncWorker
 import com.google.android.ground.repository.SurveyRepository
 import javax.inject.Inject
 
+/**
+ * Sets the survey with the specified ID as the currently active.
+ *
+ * First attempts to load the survey from the local db. If not present, fetches from remote and
+ * activates offline sync. Throws an error if the survey isn't found or cannot be made available
+ * offline. Activating a survey which is already available offline doesn't force a re-sync, since
+ * this is handled by [SurveySyncWorker].
+ */
 class ActivateSurveyUseCase
 @Inject
 constructor(
-  private val surveyRepository: SurveyRepository,
+  private val localSurveyStore: LocalSurveyStore,
   private val makeSurveyAvailableOffline: MakeSurveyAvailableOfflineUseCase,
+  private val surveyRepository: SurveyRepository,
 ) {
-  /**
-   * Sets the survey with the specified ID as the currently active. First attempts to load the
-   * survey from the local db, and if not present, fetches from remote and activates offline sync.
-   * Throws an error if the survey isn't found or cannot be made available offlien. Activating a
-   * survey which is already available offline doesn't force a resync, since this is handled by
-   * [com.google.android.ground.persistence.sync.SurveySyncWorker].
-   */
   suspend operator fun invoke(surveyId: String) {
-    // Do nothing if specified survey is already active.
-    if (surveyId == surveyRepository.activeSurvey?.id) {
+    if (surveyRepository.isSurveyActive(surveyId)) {
+      // Do nothing if survey is already active.
       return
     }
 
-    surveyRepository.getOfflineSurvey(surveyId)
+    localSurveyStore.getSurveyById(surveyId)
       ?: makeSurveyAvailableOffline(surveyId)
       ?: error("Survey $surveyId not found in remote db")
 
-    surveyRepository.selectedSurveyId = surveyId
+    surveyRepository.activateSurvey(surveyId)
   }
 }

app/src/main/java/com/google/android/ground/domain/usecases/survey/MakeSurveyAvailableOfflineUseCase.kt

@@ -17,23 +17,20 @@
 package com.google.android.ground.domain.usecases.survey
 
 import com.google.android.ground.model.Survey
-import com.google.android.ground.repository.SurveyRepository
+import com.google.android.ground.persistence.remote.RemoteDataStore
 import javax.inject.Inject
 
+/**
+ * Makes the survey with the specified ID and related LOIs available offline. Subscribes to updates
+ * from the remote server so that they may be re-fetched on change. Throws an error if the survey
+ * cannot be retrieved, or `null` if not found in the remote db.
+ */
 class MakeSurveyAvailableOfflineUseCase
 @Inject
 constructor(
-  private val surveyRepository: SurveyRepository,
+  private val remoteDataStore: RemoteDataStore,
   private val syncSurvey: SyncSurveyUseCase,
 ) {
-  /**
-   * Makes the survey with the specified ID and related LOIs available offline. Subscribes to
-   * updates from the remote server so that they may be refetched on change. Throws an error if the
-   * survey cannot be retrieved, or `null` if not found in the remote db.
-   */
-  suspend operator fun invoke(surveyId: String): Survey? {
-    val survey = syncSurvey(surveyId) ?: return null
-    surveyRepository.subscribeToSurveyUpdates(surveyId)
-    return survey
-  }
+  suspend operator fun invoke(surveyId: String): Survey? =
+    syncSurvey(surveyId)?.also { remoteDataStore.subscribeToSurveyUpdates(surveyId) }
 }

app/src/main/java/com/google/android/ground/domain/usecases/survey/ReactivateLastSurveyUseCase.kt

@@ -16,24 +16,28 @@
 
 package com.google.android.ground.domain.usecases.survey
 
+import com.google.android.ground.persistence.local.LocalValueStore
 import com.google.android.ground.repository.SurveyRepository
 import javax.inject.Inject
 
+/** Attempts to reactivate the last survey. If survey is already active, does nothing. */
 class ReactivateLastSurveyUseCase
 @Inject
 constructor(
-  private val surveyRepository: SurveyRepository,
   private val activateSurvey: ActivateSurveyUseCase,
+  private val localValueStore: LocalValueStore,
+  private val surveyRepository: SurveyRepository,
 ) {
-  suspend operator fun invoke() {
-    // Do nothing if never activated.
-    if (surveyRepository.lastActiveSurveyId.isEmpty()) {
-      return
-    }
-    // Do nothing if survey is already active.
-    if (surveyRepository.activeSurvey != null) {
-      return
+
+  suspend operator fun invoke(): Boolean =
+    if (getLastActiveSurveyId().isEmpty()) {
+      false
+    } else if (surveyRepository.hasActiveSurvey()) {
+      true
+    } else {
+      activateSurvey(getLastActiveSurveyId())
+      surveyRepository.hasActiveSurvey()
     }
-    activateSurvey(surveyRepository.lastActiveSurveyId)
-  }
+
+  private fun getLastActiveSurveyId(): String = localValueStore.lastActiveSurveyId
 }

app/src/main/java/com/google/android/ground/domain/usecases/survey/SyncSurveyUseCase.kt

@@ -17,26 +17,43 @@
 package com.google.android.ground.domain.usecases.survey
 
 import com.google.android.ground.model.Survey
+import com.google.android.ground.persistence.local.stores.LocalSurveyStore
+import com.google.android.ground.persistence.remote.RemoteDataStore
 import com.google.android.ground.repository.LocationOfInterestRepository
-import com.google.android.ground.repository.SurveyRepository
 import javax.inject.Inject
+import kotlinx.coroutines.withTimeoutOrNull
+import timber.log.Timber
 
+private const val LOAD_REMOTE_SURVEY_TIMEOUT_MILLS: Long = 15 * 1000
+
+/**
+ * Loads the survey with the specified id and related LOIs from remote and writes to local db.
+ *
+ * If the survey isn't found or operation times out, then we return null. Otherwise returns the
+ * updated [Survey].
+ *
+ * @throws error if the remote query fails.
+ */
 class SyncSurveyUseCase
 @Inject
 constructor(
-  private val surveyRepository: SurveyRepository,
+  private val localSurveyStore: LocalSurveyStore,
   private val loiRepository: LocationOfInterestRepository,
+  private val remoteDataStore: RemoteDataStore,
 ) {
-  /**
-   * Downloads the survey with the specified ID and related LOIs from remote and inserts and/or
-   * updates them on the local device. Returns the updated [Survey], or `null` if the survey could
-   * not be found.
-   */
-  suspend operator fun invoke(surveyId: String): Survey? {
-    val survey = surveyRepository.loadAndSyncSurveyWithRemote(surveyId) ?: return null
 
-    loiRepository.syncLocationsOfInterest(survey)
+  suspend operator fun invoke(surveyId: String): Survey? =
+    fetchSurvey(surveyId)?.also { syncSurvey(it) }
+
+  private suspend fun fetchSurvey(surveyId: String): Survey? =
+    withTimeoutOrNull(LOAD_REMOTE_SURVEY_TIMEOUT_MILLS) {
+      Timber.d("Loading survey $surveyId")
+      remoteDataStore.loadSurvey(surveyId)
+    }
 
-    return survey
+  private suspend fun syncSurvey(survey: Survey) {
+    localSurveyStore.insertOrUpdateSurvey(survey)
+    loiRepository.syncLocationsOfInterest(survey)
+    Timber.d("Synced survey ${survey.id}")
   }
 }

app/src/main/java/com/google/android/ground/repository/SurveyRepository.kt

@@ -20,7 +20,6 @@ import com.google.android.ground.coroutines.ApplicationScope
 import com.google.android.ground.model.Survey
 import com.google.android.ground.persistence.local.LocalValueStore
 import com.google.android.ground.persistence.local.stores.LocalSurveyStore
-import com.google.android.ground.persistence.remote.RemoteDataStore
 import javax.inject.Inject
 import javax.inject.Singleton
 import kotlinx.coroutines.CoroutineScope
@@ -36,10 +35,6 @@ import kotlinx.coroutines.flow.launchIn
 import kotlinx.coroutines.flow.onEach
 import kotlinx.coroutines.flow.stateIn
 import kotlinx.coroutines.flow.update
-import kotlinx.coroutines.withTimeoutOrNull
-import timber.log.Timber
-
-private const val LOAD_REMOTE_SURVEY_TIMEOUT_MILLS: Long = 15 * 1000
 
 /**
  * Coordinates persistence and retrieval of [Survey] instances from remote, local, and in memory
@@ -53,69 +48,56 @@ class SurveyRepository
 constructor(
   private val firebaseCrashLogger: FirebaseCrashLogger,
   private val localSurveyStore: LocalSurveyStore,
-  private val remoteDataStore: RemoteDataStore,
   private val localValueStore: LocalValueStore,
   @ApplicationScope private val externalScope: CoroutineScope,
 ) {
-  private val _selectedSurveyIdFlow = MutableStateFlow<String?>(null)
-  var selectedSurveyId: String?
-    get() = _selectedSurveyIdFlow.value
-    set(value) {
-      _selectedSurveyIdFlow.update { value }
-      firebaseCrashLogger.setSelectedSurveyId(value)
-    }
+  private val _selectedSurveyId = MutableStateFlow<String?>(null)
 
   val activeSurveyFlow: StateFlow<Survey?> =
-    _selectedSurveyIdFlow
-      .flatMapLatest { id -> offlineSurvey(id) }
+    _selectedSurveyId
+      .flatMapLatest { id -> getOfflineSurveyFlow(id) }
       .stateIn(externalScope, SharingStarted.Lazily, null)
 
   /** The currently active survey, or `null` if no survey is active. */
   val activeSurvey: Survey?
     get() = activeSurveyFlow.value
 
   /** The id of the last activated survey. */
-  var lastActiveSurveyId: String by localValueStore::lastActiveSurveyId
-    internal set
+  private var lastActiveSurveyId: String by localValueStore::lastActiveSurveyId
 
   init {
     activeSurveyFlow.filterNotNull().onEach { lastActiveSurveyId = it.id }.launchIn(externalScope)
   }
 
-  /** Listens for remote changes to the survey with the specified id. */
-  suspend fun subscribeToSurveyUpdates(surveyId: String) =
-    remoteDataStore.subscribeToSurveyUpdates(surveyId)
-
   /**
    * Returns the survey with the specified id from the local db, or `null` if not available offline.
    */
   suspend fun getOfflineSurvey(surveyId: String): Survey? = localSurveyStore.getSurveyById(surveyId)
 
-  private fun offlineSurvey(id: String?): Flow<Survey?> =
-    if (id == null) flowOf(null) else localSurveyStore.survey(id)
+  private fun getOfflineSurveyFlow(id: String?): Flow<Survey?> =
+    if (id.isNullOrBlank()) flowOf(null) else localSurveyStore.survey(id)
 
-  /**
-   * Loads the survey with the specified id from remote and writes to local db. If the survey isn't
-   * found or operation times out, then we return null and don't fetch the survey from local db.
-   *
-   * @throws error if the remote query fails.
-   */
-  suspend fun loadAndSyncSurveyWithRemote(id: String): Survey? =
-    withTimeoutOrNull(LOAD_REMOTE_SURVEY_TIMEOUT_MILLS) {
-        Timber.d("Loading survey $id")
-        remoteDataStore.loadSurvey(id)
-      }
-      ?.apply { localSurveyStore.insertOrUpdateSurvey(this) }
+  fun activateSurvey(surveyId: String) {
+    _selectedSurveyId.update { surveyId }
+    firebaseCrashLogger.setSelectedSurveyId(surveyId)
+  }
 
   fun clearActiveSurvey() {
-    selectedSurveyId = null
+    activateSurvey("")
   }
 
+  // TODO: Use activeSurvey instead of selectedSurveyId as it is possible to have no active survey.
+  // Issue URL: https://github.com/google/ground-android/issues/3020
+  fun hasActiveSurvey(): Boolean = _selectedSurveyId.value?.isNotBlank() ?: false
+
+  fun isSurveyActive(surveyId: String): Boolean =
+    surveyId.isNotBlank() && activeSurvey?.id == surveyId
+
   /** Attempts to remove the locally synced survey. Doesn't throw an error if it doesn't exist. */
   suspend fun removeOfflineSurvey(surveyId: String) {
     val survey = localSurveyStore.getSurveyById(surveyId)
     survey?.let { localSurveyStore.deleteSurvey(survey) }
-    if (activeSurvey?.id == surveyId) {
+    if (isSurveyActive(surveyId)) {
       clearActiveSurvey()
     }
   }

app/src/test/java/com/google/android/ground/domain/usecase/MakeSurveyAvailableOfflineUseCaseTest.kt

@@ -18,56 +18,52 @@ package com.google.android.ground.domain.usecase
 
 import com.google.android.ground.BaseHiltTest
 import com.google.android.ground.domain.usecases.survey.MakeSurveyAvailableOfflineUseCase
-import com.google.android.ground.repository.SurveyRepository
+import com.google.android.ground.domain.usecases.survey.SyncSurveyUseCase
+import com.google.common.truth.Truth.assertThat
 import com.sharedtest.FakeData.SURVEY
+import com.sharedtest.persistence.remote.FakeRemoteDataStore
 import dagger.hilt.android.testing.BindValue
 import dagger.hilt.android.testing.HiltAndroidTest
 import javax.inject.Inject
-import kotlin.test.assertEquals
-import kotlin.test.assertFails
-import kotlin.test.assertNull
 import kotlinx.coroutines.runBlocking
+import org.junit.Assert.assertThrows
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.mockito.Mock
-import org.mockito.Mockito.verify
-import org.mockito.Mockito.`when`
+import org.mockito.kotlin.whenever
 import org.robolectric.RobolectricTestRunner
 
 @HiltAndroidTest
 @RunWith(RobolectricTestRunner::class)
 class MakeSurveyAvailableOfflineUseCaseTest : BaseHiltTest() {
+  @BindValue @Mock lateinit var syncSurveyUseCase: SyncSurveyUseCase
+
   @Inject lateinit var makeSurveyAvailableOffline: MakeSurveyAvailableOfflineUseCase
-  @BindValue @Mock lateinit var surveyRepository: SurveyRepository
+  @Inject lateinit var fakeRemoteDataStore: FakeRemoteDataStore
 
   @Test
-  fun `Returns null when survey doesn't exist`() = runWithTestDispatcher {
-    `when`(surveyRepository.loadAndSyncSurveyWithRemote(SURVEY.id)).thenReturn(null)
+  fun `when survey sync returns null, should return null`() = runWithTestDispatcher {
+    whenever(syncSurveyUseCase(SURVEY.id)).thenReturn(null)
 
-    assertNull(makeSurveyAvailableOffline(SURVEY.id))
-  }
+    val result = makeSurveyAvailableOffline(SURVEY.id)
 
-  @Test
-  fun `Throws error when survey can't be loaded`() {
-    runBlocking {
-      `when`(surveyRepository.loadAndSyncSurveyWithRemote(SURVEY.id)).thenThrow(Error::class.java)
-
-      assertFails { makeSurveyAvailableOffline(SURVEY.id) }
-    }
+    assertThat(result).isNull()
   }
 
   @Test
-  fun `Returns survey on success`() = runWithTestDispatcher {
-    `when`(surveyRepository.loadAndSyncSurveyWithRemote(SURVEY.id)).thenReturn(SURVEY)
+  fun `when survey sync throws error, should throw error`() = runWithTestDispatcher {
+    whenever(syncSurveyUseCase(SURVEY.id)).thenThrow(Error::class.java)
 
-    assertEquals(SURVEY, makeSurveyAvailableOffline(SURVEY.id))
+    assertThrows(Error::class.java) { runBlocking { makeSurveyAvailableOffline(SURVEY.id) } }
   }
 
   @Test
-  fun `Subscribes to updates on success`() = runWithTestDispatcher {
-    `when`(surveyRepository.loadAndSyncSurveyWithRemote(SURVEY.id)).thenReturn(SURVEY)
+  fun `when survey sync succeeds, should subscribe to updates`() = runWithTestDispatcher {
+    whenever(syncSurveyUseCase(SURVEY.id)).thenReturn(SURVEY)
+
+    val result = makeSurveyAvailableOffline(SURVEY.id)
 
-    makeSurveyAvailableOffline(SURVEY.id)
-    verify(surveyRepository).subscribeToSurveyUpdates(SURVEY.id)
+    assertThat(result).isEqualTo(SURVEY)
+    assertThat(fakeRemoteDataStore.isSubscribedToSurveyUpdates(SURVEY.id)).isTrue()
   }
 }