From e2d38e684b71701eb498f52e57353ff03dcb2a5d Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Fri, 27 Oct 2023 22:39:31 +0530 Subject: [PATCH 01/18] Refactor photo task fragment (#2019) --- .../remote/firebase/FirebaseStorageManager.kt | 1 - .../ground/repository/UserMediaRepository.kt | 4 +- .../tasks/AbstractTaskViewModel.kt | 2 +- .../datacollection/tasks/photo/PhotoResult.kt | 19 ++-- .../tasks/photo/PhotoTaskFragment.kt | 35 ++---- .../tasks/photo/PhotoTaskViewModel.kt | 102 +++--------------- .../ui/home/mapcontainer/cards/LoiCardUtil.kt | 3 +- .../google/android/ground/util/StringExt.kt | 18 ++++ .../res/drawable/ic_baseline_delete_24.xml | 28 ----- ground/src/main/res/layout/photo_task.xml | 9 -- .../src/main/res/layout/photo_task_frag.xml | 18 ++-- .../tasks/photo/PhotoTaskFragmentTest.kt | 7 ++ 12 files changed, 74 insertions(+), 172 deletions(-) create mode 100644 ground/src/main/java/com/google/android/ground/util/StringExt.kt delete mode 100644 ground/src/main/res/drawable/ic_baseline_delete_24.xml diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirebaseStorageManager.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirebaseStorageManager.kt index d8609630b8..2f6e6577a6 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirebaseStorageManager.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirebaseStorageManager.kt @@ -56,7 +56,6 @@ class FirebaseStorageManager @Inject constructor() : RemoteStorageManager { * user-media/surveys/{survey_id}/submissions/{field_id-uuid.jpg} */ // TODO: Refactor this into MediaStorageRepository. - @JvmStatic fun getRemoteMediaPath(surveyId: String, filename: String): String = StringJoiner(File.separator) .add(MEDIA_ROOT_DIR) diff --git a/ground/src/main/java/com/google/android/ground/repository/UserMediaRepository.kt b/ground/src/main/java/com/google/android/ground/repository/UserMediaRepository.kt index 03ff219af9..0f99c6db70 100644 --- a/ground/src/main/java/com/google/android/ground/repository/UserMediaRepository.kt +++ b/ground/src/main/java/com/google/android/ground/repository/UserMediaRepository.kt @@ -77,8 +77,8 @@ constructor( * * @param path Final destination path of the uploaded file relative to Firestore */ - suspend fun getDownloadUrl(path: String): Uri = - if (path.isEmpty()) Uri.EMPTY else getFileUriFromRemotePath(path) + suspend fun getDownloadUrl(path: String?): Uri = + if (path.isNullOrEmpty()) Uri.EMPTY else getFileUriFromRemotePath(path) private suspend fun getFileUriFromRemotePath(destinationPath: String): Uri { val file = getLocalFileFromRemotePath(destinationPath) diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/AbstractTaskViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/AbstractTaskViewModel.kt index 32c2309d46..8fafeff660 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/AbstractTaskViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/AbstractTaskViewModel.kt @@ -71,7 +71,7 @@ open class AbstractTaskViewModel internal constructor(private val resources: Res setResponse(taskData) } - protected fun detailsTextFlowable(): @Cold(stateful = true, terminates = false) Flowable = + private fun detailsTextFlowable(): @Cold(stateful = true, terminates = false) Flowable = taskDataSubject.distinctUntilChanged().map { taskDataOptional: Optional -> taskDataOptional.map { it.getDetailsText() }.orElse("") } diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoResult.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoResult.kt index a271c0e8ad..943e504144 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoResult.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoResult.kt @@ -16,15 +16,16 @@ package com.google.android.ground.ui.datacollection.tasks.photo import android.graphics.Bitmap +import com.google.android.ground.util.isNotNullOrEmpty /** Contains the bitmap or path to the photo a user captured or selected. */ -data class PhotoResult -@JvmOverloads -constructor( - val taskId: String, - val bitmap: Bitmap? = null, - val path: String? = null, - val isHandled: Boolean = false -) { - fun isEmpty(): Boolean = bitmap == null && path == null +data class PhotoResult constructor(val taskId: String, val bitmap: Bitmap?, val path: String?) { + + init { + check(bitmap != null || path.isNotNullOrEmpty()) { + "At least one of bitmap or path should be non-null, found " + + "bitmap=${bitmap != null}, " + + "path=${path.isNotNullOrEmpty()}" + } + } } diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragment.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragment.kt index f0b815a274..cca085dbee 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragment.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragment.kt @@ -27,7 +27,6 @@ import com.google.android.ground.BuildConfig import com.google.android.ground.coroutines.ApplicationScope import com.google.android.ground.databinding.PhotoTaskFragBinding import com.google.android.ground.repository.UserMediaRepository -import com.google.android.ground.rx.RxAutoDispose.autoDisposable import com.google.android.ground.system.PermissionDeniedException import com.google.android.ground.system.PermissionsManager import com.google.android.ground.ui.datacollection.components.TaskView @@ -58,6 +57,7 @@ class PhotoTaskFragment : Hilt_PhotoTaskFragment() { override fun onCreateTaskBody(inflater: LayoutInflater): View { val taskBinding = PhotoTaskFragBinding.inflate(inflater) taskBinding.lifecycleOwner = this + taskBinding.fragment = this taskBinding.dataCollectionViewModel = dataCollectionViewModel taskBinding.viewModel = viewModel return taskBinding.root @@ -79,13 +79,9 @@ class PhotoTaskFragment : Hilt_PhotoTaskFragment() { viewModel.onCapturePhotoResult(result) } - viewModel.setEditable(true) - viewModel.setSurveyId(dataCollectionViewModel.surveyId) - viewModel.setTaskWaitingForPhoto(taskWaitingForPhoto) - viewModel.setCapturedPhotoPath(capturedPhotoPath) - - observeSelectPhotoClicks() - observePhotoResults() + viewModel.surveyId = dataCollectionViewModel.surveyId + viewModel.taskWaitingForPhoto = taskWaitingForPhoto + viewModel.capturedPhotoPath = capturedPhotoPath } override fun onCreateActionButtons() { @@ -105,21 +101,8 @@ class PhotoTaskFragment : Hilt_PhotoTaskFragment() { override fun onSaveInstanceState(outState: Bundle) { super.onSaveInstanceState(outState) - outState.putString(TASK_WAITING_FOR_PHOTO, viewModel.getTaskWaitingForPhoto()) - outState.putString(CAPTURED_PHOTO_PATH, viewModel.getCapturedPhotoPath()) - } - - private fun observeSelectPhotoClicks() { - viewModel.getTakePhotoClicks().`as`(autoDisposable(viewLifecycleOwner)).subscribe { - onTakePhoto() - } - } - - private fun observePhotoResults() { - viewModel - .getLastPhotoResult() - .`as`(autoDisposable(viewLifecycleOwner)) - .subscribe { photoResult -> viewModel.onPhotoResult(photoResult) } + outState.putString(TASK_WAITING_FOR_PHOTO, viewModel.taskWaitingForPhoto) + outState.putString(CAPTURED_PHOTO_PATH, viewModel.capturedPhotoPath) } private fun obtainCapturePhotoPermissions(onPermissionsGranted: () -> Unit = {}) { @@ -135,7 +118,7 @@ class PhotoTaskFragment : Hilt_PhotoTaskFragment() { } } - private fun onTakePhoto() { + fun onTakePhoto() { // TODO(#1600): Launch intent is not invoked if the permission is not granted by default. obtainCapturePhotoPermissions { launchPhotoCapture(viewModel.task.id) } } @@ -143,8 +126,8 @@ class PhotoTaskFragment : Hilt_PhotoTaskFragment() { private fun launchPhotoCapture(taskId: String) { val photoFile = userMediaRepository.createImageFile(taskId) val uri = FileProvider.getUriForFile(requireContext(), BuildConfig.APPLICATION_ID, photoFile) - viewModel.setTaskWaitingForPhoto(taskId) - viewModel.setCapturedPhotoPath(photoFile.absolutePath) + viewModel.taskWaitingForPhoto = taskId + viewModel.capturedPhotoPath = photoFile.absolutePath capturePhotoLauncher.launch(uri) Timber.d("Capture photo intent sent") } diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskViewModel.kt index 7505fbc31e..4eda78c50e 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskViewModel.kt @@ -18,23 +18,17 @@ package com.google.android.ground.ui.datacollection.tasks.photo import android.content.res.Resources import android.net.Uri import androidx.lifecycle.LiveData -import androidx.lifecycle.MutableLiveData -import androidx.lifecycle.toLiveData +import androidx.lifecycle.asLiveData import com.google.android.ground.model.submission.TextTaskData.Companion.fromString -import com.google.android.ground.model.task.Task +import com.google.android.ground.model.submission.isNotNullOrEmpty import com.google.android.ground.persistence.remote.firebase.FirebaseStorageManager.Companion.getRemoteMediaPath import com.google.android.ground.repository.UserMediaRepository -import com.google.android.ground.rx.annotations.Hot import com.google.android.ground.ui.datacollection.tasks.AbstractTaskViewModel import com.google.android.ground.ui.util.BitmapUtil -import io.reactivex.Observable -import io.reactivex.subjects.BehaviorSubject -import io.reactivex.subjects.PublishSubject -import io.reactivex.subjects.Subject import java.io.File import java.io.IOException import javax.inject.Inject -import kotlinx.coroutines.rx2.rxSingle +import kotlinx.coroutines.flow.map import timber.log.Timber class PhotoTaskViewModel @@ -45,18 +39,11 @@ constructor( resources: Resources ) : AbstractTaskViewModel(resources) { - /** - * Emits the last photo task id updated and either its photo result, or empty if removed. The last - * value is emitted on each subscription because {@see #onPhotoResult} is called before - * subscribers are created. - */ - private val lastPhotoResult: Subject = BehaviorSubject.create() - /** * Task id waiting for a photo taskData. As only 1 photo result is returned at a time, we can * directly map it 1:1 with the task waiting for a photo taskData. */ - private var taskWaitingForPhoto: String? = null + var taskWaitingForPhoto: String? = null /** * Full path of the captured photo in local storage. In case of selecting a photo from storage, @@ -64,59 +51,22 @@ constructor( * result returns true/false based on whether the operation passed or not. As only 1 photo result * is returned at a time, we can directly map it 1:1 with the path of the captured photo. */ - private var capturedPhotoPath: String? = null - - val uri: LiveData = - detailsTextFlowable() - .switchMapSingle { rxSingle { userMediaRepository.getDownloadUrl(it) } } - .toLiveData() - - val isPhotoPresent: LiveData = detailsTextFlowable().map { it.isNotEmpty() }.toLiveData() - - private var surveyId: String? = null - - private val takePhotoClicks: @Hot Subject = PublishSubject.create() - private val editable: @Hot(replays = true) MutableLiveData = MutableLiveData(false) + var capturedPhotoPath: String? = null - fun onTakePhotoClick() { - takePhotoClicks.onNext(task) - } - - fun getTakePhotoClicks(): @Hot Observable = takePhotoClicks - - fun setEditable(enabled: Boolean) { - editable.postValue(enabled) - } + lateinit var surveyId: String - fun isEditable(): LiveData = editable - - fun updateResponse(value: String) { - setResponse(fromString(value)) - } + val uri: LiveData = + taskDataValue.map { userMediaRepository.getDownloadUrl(it?.getDetailsText()) }.asLiveData() - fun setSurveyId(surveyId: String?) { - this.surveyId = surveyId - } + val isPhotoPresent: LiveData = taskDataValue.map { it.isNotNullOrEmpty() }.asLiveData() - fun onPhotoResult(photoResult: PhotoResult) { - if (photoResult.isHandled) { - return - } - if (surveyId == null) { - Timber.e("surveyId not set") - return - } + private fun onPhotoResult(photoResult: PhotoResult) { if (photoResult.taskId != task.id) { // Update belongs to another task. return } - if (photoResult.isEmpty()) { - clearResponse() - Timber.v("Photo cleared") - return - } try { - val imageFile = getFileFromResult(photoResult.copy(isHandled = true)) + val imageFile = getFileFromResult(photoResult) val filename = imageFile.name val path = imageFile.absolutePath @@ -124,28 +74,13 @@ constructor( userMediaRepository.addImageToGallery(path, filename) // Update taskData. - val remoteDestinationPath = getRemoteMediaPath(surveyId!!, filename) - updateResponse(remoteDestinationPath) + val remoteDestinationPath = getRemoteMediaPath(surveyId, filename) + setResponse(fromString(remoteDestinationPath)) } catch (e: IOException) { - // TODO: Report error. Timber.e(e, "Failed to save photo") } } - fun getTaskWaitingForPhoto(): String? = taskWaitingForPhoto - - fun setTaskWaitingForPhoto(taskWaitingForPhoto: String?) { - this.taskWaitingForPhoto = taskWaitingForPhoto - } - - fun getCapturedPhotoPath(): String? = capturedPhotoPath - - fun setCapturedPhotoPath(photoUri: String?) { - this.capturedPhotoPath = photoUri - } - - fun getLastPhotoResult(): Observable = lastPhotoResult - fun onSelectPhotoResult(uri: Uri?) { if (uri == null) { Timber.v("Select photo failed or canceled") @@ -157,7 +92,7 @@ constructor( return } try { - onPhotoProvided(PhotoResult(currentTask, bitmapUtil.fromUri(uri))) + onPhotoProvided(PhotoResult(currentTask, bitmapUtil.fromUri(uri), null)) Timber.v("Select photo result returned") } catch (e: IOException) { Timber.e(e, "Error getting photo selected from storage") @@ -179,21 +114,16 @@ constructor( Timber.e("Photo captured but no path available to read the result") return } - onPhotoProvided(PhotoResult(currentTask, /* bitmap=*/ null, capturedPhotoPath)) + onPhotoProvided(PhotoResult(currentTask, null, capturedPhotoPath)) Timber.v("Photo capture result returned") } private fun onPhotoProvided(result: PhotoResult) { capturedPhotoPath = null taskWaitingForPhoto = null - lastPhotoResult.onNext(result) - } - - fun clearPhoto(taskId: String) { - lastPhotoResult.onNext(PhotoResult(taskId)) + onPhotoResult(result) } - @Throws(IOException::class) private fun getFileFromResult(result: PhotoResult): File { if (result.bitmap != null) { return userMediaRepository.savePhoto(result.bitmap, result.taskId) diff --git a/ground/src/main/java/com/google/android/ground/ui/home/mapcontainer/cards/LoiCardUtil.kt b/ground/src/main/java/com/google/android/ground/ui/home/mapcontainer/cards/LoiCardUtil.kt index 0084ad80a8..20184da5d1 100644 --- a/ground/src/main/java/com/google/android/ground/ui/home/mapcontainer/cards/LoiCardUtil.kt +++ b/ground/src/main/java/com/google/android/ground/ui/home/mapcontainer/cards/LoiCardUtil.kt @@ -22,6 +22,7 @@ import com.google.android.ground.model.geometry.MultiPolygon import com.google.android.ground.model.geometry.Point import com.google.android.ground.model.geometry.Polygon import com.google.android.ground.model.locationofinterest.LocationOfInterest +import com.google.android.ground.util.isNotNullOrEmpty /** Helper class for creating user-visible text. */ object LoiCardUtil { @@ -67,6 +68,4 @@ object LoiCardUtil { is MultiPolygon -> context.getString(R.string.unnamed_area) else -> throw IllegalArgumentException("Unsupported geometry type $this") } - - private fun String?.isNotNullOrEmpty(): Boolean = !this.isNullOrEmpty() } diff --git a/ground/src/main/java/com/google/android/ground/util/StringExt.kt b/ground/src/main/java/com/google/android/ground/util/StringExt.kt new file mode 100644 index 0000000000..98c79be313 --- /dev/null +++ b/ground/src/main/java/com/google/android/ground/util/StringExt.kt @@ -0,0 +1,18 @@ +/* + * Copyright 2023 Google LLC + * + * 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.android.ground.util + +fun String?.isNotNullOrEmpty(): Boolean = !this.isNullOrEmpty() diff --git a/ground/src/main/res/drawable/ic_baseline_delete_24.xml b/ground/src/main/res/drawable/ic_baseline_delete_24.xml deleted file mode 100644 index 18a8a49291..0000000000 --- a/ground/src/main/res/drawable/ic_baseline_delete_24.xml +++ /dev/null @@ -1,28 +0,0 @@ - - - - - - - diff --git a/ground/src/main/res/layout/photo_task.xml b/ground/src/main/res/layout/photo_task.xml index b2e6da71c4..8c1f1b1e51 100644 --- a/ground/src/main/res/layout/photo_task.xml +++ b/ground/src/main/res/layout/photo_task.xml @@ -43,14 +43,5 @@ android:scaleType="centerCrop" app:imageUri="@{viewModel.uri}" tools:src="@drawable/splash_background" /> - \ No newline at end of file diff --git a/ground/src/main/res/layout/photo_task_frag.xml b/ground/src/main/res/layout/photo_task_frag.xml index 5090bb297c..7de606c4e4 100644 --- a/ground/src/main/res/layout/photo_task_frag.xml +++ b/ground/src/main/res/layout/photo_task_frag.xml @@ -20,10 +20,12 @@ xmlns:app="http://schemas.android.com/apk/res-auto"> - + @@ -32,19 +34,19 @@ + android:paddingEnd="8dp"> + app:icon="@drawable/outline_photo_camera" + app:iconTint="?attr/colorOnSurface" /> - \ No newline at end of file + diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragmentTest.kt index a8e203989b..60aac0e7c3 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragmentTest.kt @@ -28,6 +28,7 @@ import javax.inject.Inject import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mock +import org.mockito.kotlin.whenever import org.robolectric.RobolectricTestRunner @HiltAndroidTest @@ -47,6 +48,12 @@ class PhotoTaskFragmentTest : BaseTaskFragmentTest(job, task) From d1d66c7834a4b74122523a6dbc347a1a54e45c98 Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Fri, 27 Oct 2023 22:39:53 +0530 Subject: [PATCH 02/18] Use coroutine in Navigator (#2020) * Replace PublishSubject with StateFlow in Navigator * Update unit tests * Make flow non-null * Listen to flow after navhostfragment is initialized to prevent race condition if app is resumed from background * Fix import order --- .../com/google/android/ground/MainActivity.kt | 25 ++++------------- .../android/ground/ui/common/Navigator.kt | 28 +++++++++---------- .../android/ground/MainViewModelTest.kt | 22 +++++++-------- .../ground/ui/home/HomeScreenFragmentTest.kt | 12 ++++---- .../HomeScreenMapContainerFragmentTest.kt | 15 ++++++---- .../ui/tos/TermsOfServiceFragmentTest.kt | 13 +++++---- 6 files changed, 54 insertions(+), 61 deletions(-) diff --git a/ground/src/main/java/com/google/android/ground/MainActivity.kt b/ground/src/main/java/com/google/android/ground/MainActivity.kt index 7dd5b7b1b0..72a27fec54 100644 --- a/ground/src/main/java/com/google/android/ground/MainActivity.kt +++ b/ground/src/main/java/com/google/android/ground/MainActivity.kt @@ -20,18 +20,19 @@ import android.app.ProgressDialog import android.content.Intent import android.os.Bundle import androidx.core.view.WindowInsetsCompat +import androidx.lifecycle.lifecycleScope import androidx.navigation.NavDirections import androidx.navigation.fragment.NavHostFragment import com.google.android.ground.databinding.MainActBinding import com.google.android.ground.repository.UserRepository import com.google.android.ground.rx.RxAutoDispose.autoDisposable -import com.google.android.ground.rx.Schedulers import com.google.android.ground.system.ActivityStreams import com.google.android.ground.system.SettingsManager import com.google.android.ground.ui.common.* import dagger.hilt.android.AndroidEntryPoint import java8.util.function.Consumer import javax.inject.Inject +import kotlinx.coroutines.launch import timber.log.Timber /** @@ -51,8 +52,6 @@ class MainActivity : Hilt_MainActivity() { @Inject lateinit var popups: EphemeralPopups - @Inject lateinit var schedulers: Schedulers - private lateinit var viewModel: MainViewModel private lateinit var navHostFragment: NavHostFragment @@ -69,22 +68,6 @@ class MainActivity : Hilt_MainActivity() { callback.accept(this) } - navigator - .getNavigateRequests() - .observeOn(schedulers.ui()) - .`as`(autoDisposable(this)) - .subscribe { navDirections: NavDirections -> onNavigate(navDirections) } - - navigator - .getNavigateUpRequests() - .observeOn(schedulers.ui()) - .`as`(autoDisposable(this)) - .subscribe { navigateUp() } - - navigator.getFinishRequests().observeOn(schedulers.ui()).`as`(autoDisposable(this)).subscribe { - finish() - } - val binding = MainActBinding.inflate(layoutInflater) setContentView(binding.root) @@ -95,6 +78,10 @@ class MainActivity : Hilt_MainActivity() { viewModel.signInProgressDialogVisibility.observe(this) { visible: Boolean -> onSignInProgress(visible) } + + lifecycleScope.launch { navigator.getNavigateRequests().collect { onNavigate(it) } } + lifecycleScope.launch { navigator.getNavigateUpRequests().collect { navigateUp() } } + lifecycleScope.launch { navigator.getFinishRequests().collect { finish() } } } override fun onWindowInsetChanged(insets: WindowInsetsCompat) { diff --git a/ground/src/main/java/com/google/android/ground/ui/common/Navigator.kt b/ground/src/main/java/com/google/android/ground/ui/common/Navigator.kt index f730d20fa9..74bb5ee390 100644 --- a/ground/src/main/java/com/google/android/ground/ui/common/Navigator.kt +++ b/ground/src/main/java/com/google/android/ground/ui/common/Navigator.kt @@ -16,44 +16,42 @@ package com.google.android.ground.ui.common import androidx.navigation.NavDirections -import com.google.android.ground.rx.annotations.Hot -import io.reactivex.Observable -import io.reactivex.subjects.PublishSubject -import io.reactivex.subjects.Subject import javax.inject.Inject import javax.inject.Singleton import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers.Main +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.filterNotNull import kotlinx.coroutines.launch /** * Responsible for abstracting navigation from fragment to fragment. Exposes various actions to - * ViewModels that cause a NavDirections to be emitted to the observer (in this case, the [ ], which - * is expected to pass it to the current [ ]. + * ViewModels that cause a NavDirections to be emitted to the flow. */ @Singleton class Navigator @Inject constructor() { - private val navigateRequests: @Hot Subject = PublishSubject.create() - private val navigateUpRequests: @Hot Subject = PublishSubject.create() - private val finishRequests: @Hot Subject = PublishSubject.create() + private val navigateRequests: MutableStateFlow = MutableStateFlow(null) + private val navigateUpRequests: MutableStateFlow = MutableStateFlow(null) + private val finishRequests: MutableStateFlow = MutableStateFlow(null) /** Stream of navigation requests for fulfillment by the view layer. */ - fun getNavigateRequests(): Observable = navigateRequests + fun getNavigateRequests(): Flow = navigateRequests.filterNotNull() - fun getNavigateUpRequests(): Observable = navigateUpRequests + fun getNavigateUpRequests(): Flow = navigateUpRequests.filterNotNull() - fun getFinishRequests(): Observable = finishRequests + fun getFinishRequests(): Flow = finishRequests.filterNotNull() /** Navigates up one level on the back stack. */ fun navigateUp() { - navigateUpRequests.onNext(Any()) + CoroutineScope(Main).launch { navigateUpRequests.emit(Any()) } } fun navigate(directions: NavDirections) { - CoroutineScope(Main).launch { navigateRequests.onNext(directions) } + CoroutineScope(Main).launch { navigateRequests.emit(directions) } } fun finishApp() { - finishRequests.onNext(Any()) + CoroutineScope(Main).launch { finishRequests.emit(Any()) } } } diff --git a/ground/src/test/java/com/google/android/ground/MainViewModelTest.kt b/ground/src/test/java/com/google/android/ground/MainViewModelTest.kt index ec0094aff9..0597c249ec 100644 --- a/ground/src/test/java/com/google/android/ground/MainViewModelTest.kt +++ b/ground/src/test/java/com/google/android/ground/MainViewModelTest.kt @@ -18,6 +18,7 @@ package com.google.android.ground import android.content.SharedPreferences import android.os.Looper import androidx.navigation.NavDirections +import app.cash.turbine.test import com.google.android.ground.persistence.local.room.LocalDataStoreException import com.google.android.ground.repository.TermsOfServiceRepository import com.google.android.ground.repository.UserRepository @@ -32,7 +33,6 @@ import com.sharedtest.TestObservers.observeUntilFirstChange import com.sharedtest.persistence.remote.FakeRemoteDataStore import com.sharedtest.system.auth.FakeAuthenticationManager import dagger.hilt.android.testing.HiltAndroidTest -import io.reactivex.observers.TestObserver import javax.inject.Inject import kotlin.test.assertFailsWith import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -56,16 +56,11 @@ class MainViewModelTest : BaseHiltTest() { @Inject lateinit var tosRepository: TermsOfServiceRepository @Inject lateinit var userRepository: UserRepository - private lateinit var navDirectionsTestObserver: TestObserver - @Before override fun setUp() { super.setUp() fakeAuthenticationManager.setUser(FakeData.USER) - - // Subscribe to navigation requests - navDirectionsTestObserver = navigator.getNavigateRequests().test() } private fun setupUserPreferences() { @@ -89,11 +84,16 @@ class MainViewModelTest : BaseHiltTest() { assertThat(viewModel.signInProgressDialogVisibility.value).isEqualTo(visible) } - private fun verifyNavigationRequested(vararg navDirections: NavDirections) { - navDirectionsTestObserver.assertNoErrors() - navDirectionsTestObserver.assertNotComplete() - navDirectionsTestObserver.assertValues(*navDirections) - } + private fun verifyNavigationRequested(navDirections: NavDirections? = null) = + runWithTestDispatcher { + navigator.getNavigateRequests().test { + if (navDirections == null) { + expectNoEvents() + } else { + assertThat(expectMostRecentItem()).isEqualTo(navDirections) + } + } + } @Test fun testSignInStateChanged_onSignedOut() { diff --git a/ground/src/test/java/com/google/android/ground/ui/home/HomeScreenFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/home/HomeScreenFragmentTest.kt index 6664c35f05..f3f95cf488 100644 --- a/ground/src/test/java/com/google/android/ground/ui/home/HomeScreenFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/home/HomeScreenFragmentTest.kt @@ -27,6 +27,7 @@ import androidx.test.espresso.matcher.ViewMatchers import androidx.test.espresso.matcher.ViewMatchers.isEnabled import androidx.test.espresso.matcher.ViewMatchers.withId import androidx.test.espresso.matcher.ViewMatchers.withText +import app.cash.turbine.test import com.google.android.ground.BaseHiltTest import com.google.android.ground.R import com.google.android.ground.launchFragmentInHiltContainer @@ -34,6 +35,7 @@ import com.google.android.ground.model.Survey import com.google.android.ground.model.imagery.TileSource import com.google.android.ground.repository.SurveyRepository import com.google.android.ground.ui.common.Navigator +import com.google.common.truth.Truth.assertThat import com.sharedtest.FakeData import com.squareup.picasso.Picasso import dagger.hilt.android.testing.HiltAndroidTest @@ -41,7 +43,6 @@ import javax.inject.Inject import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.advanceUntilIdle import org.hamcrest.CoreMatchers.not -import org.junit.Assert.* import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -153,6 +154,7 @@ class HomeScreenFragmentTest : AbstractHomeScreenFragmentTest() { } } +@OptIn(ExperimentalCoroutinesApi::class) @HiltAndroidTest @RunWith(ParameterizedRobolectricTestRunner::class) class NavigationDrawerItemClickTest( @@ -163,13 +165,13 @@ class NavigationDrawerItemClickTest( @Inject lateinit var navigator: Navigator @Test - fun clickDrawerMenuItem() { - val navDirectionsTestObserver = navigator.getNavigateRequests().test() - + fun clickDrawerMenuItem() = runWithTestDispatcher { openDrawer() onView(withText(menuItemLabel)).check(matches(isEnabled())).perform(click()) - navDirectionsTestObserver.assertValue(expectedNavDirection) + navigator.getNavigateRequests().test { + assertThat(expectMostRecentItem()).isEqualTo(expectedNavDirection) + } verifyDrawerClosed() } diff --git a/ground/src/test/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerFragmentTest.kt index 97192bedf2..b4407e4350 100644 --- a/ground/src/test/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerFragmentTest.kt @@ -20,18 +20,21 @@ import androidx.test.espresso.action.ViewActions.click import androidx.test.espresso.assertion.ViewAssertions.matches import androidx.test.espresso.matcher.ViewMatchers.isEnabled import androidx.test.espresso.matcher.ViewMatchers.withId +import app.cash.turbine.test import com.google.android.ground.* import com.google.android.ground.NavGraphDirections.ShowMapTypeDialogFragment import com.google.android.ground.ui.common.Navigator import com.google.android.ground.ui.map.MapType +import com.google.common.truth.Truth.assertThat import dagger.hilt.android.testing.HiltAndroidTest import javax.inject.Inject -import org.junit.Assert.* +import kotlinx.coroutines.ExperimentalCoroutinesApi import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner +@OptIn(ExperimentalCoroutinesApi::class) @HiltAndroidTest @RunWith(RobolectricTestRunner::class) class HomeScreenMapContainerFragmentTest : BaseHiltTest() { @@ -45,13 +48,13 @@ class HomeScreenMapContainerFragmentTest : BaseHiltTest() { } @Test - fun clickMapType_launchesMapTypeDialogFragment() { - val navDirectionsTestObserver = navigator.getNavigateRequests().test() - + fun clickMapType_launchesMapTypeDialogFragment() = runWithTestDispatcher { onView(withId(R.id.map_type_btn)).perform(click()).check(matches(isEnabled())) - navDirectionsTestObserver.assertValue { - it is ShowMapTypeDialogFragment && it.mapTypes.contentEquals(MapType.values()) + navigator.getNavigateRequests().test { + val result = expectMostRecentItem() + assertThat(result).isInstanceOf(ShowMapTypeDialogFragment::class.java) + assertThat((result as ShowMapTypeDialogFragment).mapTypes).isEqualTo(MapType.values()) } } } diff --git a/ground/src/test/java/com/google/android/ground/ui/tos/TermsOfServiceFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/tos/TermsOfServiceFragmentTest.kt index 323ded52e7..7008a4cef2 100644 --- a/ground/src/test/java/com/google/android/ground/ui/tos/TermsOfServiceFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/tos/TermsOfServiceFragmentTest.kt @@ -20,6 +20,7 @@ import androidx.test.espresso.Espresso.onView import androidx.test.espresso.action.ViewActions.click import androidx.test.espresso.assertion.ViewAssertions.matches import androidx.test.espresso.matcher.ViewMatchers.* +import app.cash.turbine.test import com.google.android.ground.BaseHiltTest import com.google.android.ground.R import com.google.android.ground.launchFragmentInHiltContainer @@ -29,11 +30,13 @@ import com.google.android.ground.ui.surveyselector.SurveySelectorFragmentDirecti import com.google.common.truth.Truth.assertThat import dagger.hilt.android.testing.HiltAndroidTest import javax.inject.Inject +import kotlinx.coroutines.ExperimentalCoroutinesApi import org.hamcrest.Matchers.not import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner +@OptIn(ExperimentalCoroutinesApi::class) @HiltAndroidTest @RunWith(RobolectricTestRunner::class) class TermsOfServiceFragmentTest : BaseHiltTest() { @@ -69,19 +72,19 @@ class TermsOfServiceFragmentTest : BaseHiltTest() { } @Test - fun agreeButton_whenPressed_shouldUpdatePrefAndNavigate() { + fun agreeButton_whenPressed_shouldUpdatePrefAndNavigate() = runWithTestDispatcher { launchFragmentInHiltContainer(bundleOf()) assertThat(termsOfServiceRepository.isTermsOfServiceAccepted).isFalse() - val navDirectionsTestObserver = navigator.getNavigateRequests().test() onView(withId(R.id.agreeCheckBox)).perform(click()) onView(withId(R.id.agreeButton)).perform(click()) assertThat(termsOfServiceRepository.isTermsOfServiceAccepted).isTrue() - navDirectionsTestObserver.assertValue( - SurveySelectorFragmentDirections.showSurveySelectorScreen(true) - ) + navigator.getNavigateRequests().test { + assertThat(expectMostRecentItem()) + .isEqualTo(SurveySelectorFragmentDirections.showSurveySelectorScreen(true)) + } } @Test From 8977bd02734367b599c658e65991662168921e8f Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Sat, 28 Oct 2023 18:06:54 +0530 Subject: [PATCH 03/18] [Rx -> Coroutine] Migrate Offline Area Data layer (#2021) * Use coroutine to load offline areas * Replace another usage when removing offline areas from device * Replace last remaining usage and tests * Cleanup usages of flowable --- .../local/room/dao/OfflineAreaDao.kt | 4 +-- .../local/room/stores/RoomOfflineAreaStore.kt | 14 +++++------ .../local/stores/LocalOfflineAreaStore.kt | 5 ++-- .../repository/OfflineAreaRepository.kt | 25 ++++++++----------- .../ui/offlineareas/OfflineAreasViewModel.kt | 22 ++++++---------- .../persistence/local/LocalDataStoreTests.kt | 5 +++- 6 files changed, 32 insertions(+), 43 deletions(-) diff --git a/ground/src/main/java/com/google/android/ground/persistence/local/room/dao/OfflineAreaDao.kt b/ground/src/main/java/com/google/android/ground/persistence/local/room/dao/OfflineAreaDao.kt index 4bba578acb..9034715d9d 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/local/room/dao/OfflineAreaDao.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/local/room/dao/OfflineAreaDao.kt @@ -18,12 +18,12 @@ package com.google.android.ground.persistence.local.room.dao import androidx.room.Dao import androidx.room.Query import com.google.android.ground.persistence.local.room.entity.OfflineAreaEntity -import io.reactivex.Flowable +import kotlinx.coroutines.flow.Flow /** Provides read/write operations for writing [OfflineAreaEntity] to the local db. */ @Dao interface OfflineAreaDao : BaseDao { - @Query("SELECT * FROM offline_area") fun findAllOnceAndStream(): Flowable> + @Query("SELECT * FROM offline_area") fun findAll(): Flow> @Query("SELECT * FROM offline_area WHERE id = :id") suspend fun findById(id: String): OfflineAreaEntity? diff --git a/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomOfflineAreaStore.kt b/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomOfflineAreaStore.kt index 712fa057f4..d9ea72338a 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomOfflineAreaStore.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomOfflineAreaStore.kt @@ -22,24 +22,22 @@ import com.google.android.ground.persistence.local.room.dao.OfflineAreaDao import com.google.android.ground.persistence.local.room.dao.insertOrUpdate import com.google.android.ground.persistence.local.room.entity.OfflineAreaEntity import com.google.android.ground.persistence.local.stores.LocalOfflineAreaStore -import com.google.android.ground.rx.Schedulers -import io.reactivex.Flowable import javax.inject.Inject import javax.inject.Singleton +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.map @Singleton class RoomOfflineAreaStore @Inject internal constructor() : LocalOfflineAreaStore { @Inject lateinit var offlineAreaDao: OfflineAreaDao - @Inject lateinit var schedulers: Schedulers override suspend fun insertOrUpdate(area: OfflineArea) = offlineAreaDao.insertOrUpdate(area.toOfflineAreaEntity()) - override fun offlineAreasOnceAndStream(): Flowable> = - offlineAreaDao - .findAllOnceAndStream() - .map { areas: List -> areas.map { it.toModelObject() } } - .subscribeOn(schedulers.io()) + override fun offlineAreas(): Flow> = + offlineAreaDao.findAll().map { areas: List -> + areas.map { it.toModelObject() } + } override suspend fun getOfflineAreaById(id: String): OfflineArea? = offlineAreaDao.findById(id)?.toModelObject() diff --git a/ground/src/main/java/com/google/android/ground/persistence/local/stores/LocalOfflineAreaStore.kt b/ground/src/main/java/com/google/android/ground/persistence/local/stores/LocalOfflineAreaStore.kt index 3cc120a815..be1108d783 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/local/stores/LocalOfflineAreaStore.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/local/stores/LocalOfflineAreaStore.kt @@ -16,8 +16,7 @@ package com.google.android.ground.persistence.local.stores import com.google.android.ground.model.imagery.OfflineArea -import com.google.android.ground.rx.annotations.Cold -import io.reactivex.Flowable +import kotlinx.coroutines.flow.Flow interface LocalOfflineAreaStore { /** @@ -27,7 +26,7 @@ interface LocalOfflineAreaStore { suspend fun insertOrUpdate(area: OfflineArea) /** Returns all queued, failed, and completed offline areas from the local data store. */ - fun offlineAreasOnceAndStream(): @Cold(terminates = false) Flowable> + fun offlineAreas(): Flow> /** Delete an offline area and any associated tiles that are no longer needed. */ suspend fun deleteOfflineArea(offlineAreaId: String) diff --git a/ground/src/main/java/com/google/android/ground/repository/OfflineAreaRepository.kt b/ground/src/main/java/com/google/android/ground/repository/OfflineAreaRepository.kt index 37b7d2fad8..1574cd0dad 100644 --- a/ground/src/main/java/com/google/android/ground/repository/OfflineAreaRepository.kt +++ b/ground/src/main/java/com/google/android/ground/repository/OfflineAreaRepository.kt @@ -20,7 +20,6 @@ import com.google.android.ground.model.imagery.OfflineArea import com.google.android.ground.model.imagery.TileSource import com.google.android.ground.persistence.local.stores.LocalOfflineAreaStore import com.google.android.ground.persistence.uuid.OfflineUuidGenerator -import com.google.android.ground.rx.annotations.Cold import com.google.android.ground.system.GeocodingManager import com.google.android.ground.ui.map.Bounds import com.google.android.ground.ui.map.gms.mog.MogClient @@ -33,15 +32,14 @@ import com.google.android.ground.ui.util.FileUtil import com.google.android.ground.util.ByteCount import com.google.android.ground.util.deleteIfEmpty import com.google.android.ground.util.rangeOf -import io.reactivex.Flowable import java.io.File import javax.inject.Inject import javax.inject.Singleton import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flow -import kotlinx.coroutines.reactive.asFlow -import kotlinx.coroutines.reactive.awaitFirst +import kotlinx.coroutines.flow.map import timber.log.Timber /** @@ -76,10 +74,9 @@ constructor( /** * Retrieves all offline areas from the local store and continually streams the list as the local - * store is updated. Triggers `onError` only if there is a problem accessing the local store. + * store is updated. */ - fun offlineAreasOnceAndStream(): @Cold(terminates = false) Flowable> = - localOfflineAreaStore.offlineAreasOnceAndStream() + fun offlineAreas(): Flow> = localOfflineAreaStore.offlineAreas() /** Fetches a single offline area by ID. */ suspend fun getOfflineArea(offlineAreaId: String): OfflineArea? = @@ -109,11 +106,9 @@ constructor( private suspend fun getLocalTileSourcePath(): String = File(fileUtil.getFilesDir(), "tiles").path fun getOfflineTileSourcesFlow() = - surveyRepository.activeSurveyFlow - // TODO(#1593): Use Room DAO's Flow once we figure out why it never emits a value. - .combine(getOfflineAreaBounds().asFlow()) { survey, bounds -> - applyBounds(survey?.tileSources, bounds) - } + surveyRepository.activeSurveyFlow.combine(getOfflineAreaBounds()) { survey, bounds -> + applyBounds(survey?.tileSources, bounds) + } private suspend fun applyBounds( tileSources: List?, @@ -133,8 +128,8 @@ constructor( ) } - private fun getOfflineAreaBounds(): Flowable> = - localOfflineAreaStore.offlineAreasOnceAndStream().map { list -> list.map { it.bounds } } + private fun getOfflineAreaBounds(): Flow> = + localOfflineAreaStore.offlineAreas().map { list -> list.map { it.bounds } } /** * Uses the first tile source URL of the currently active survey and returns a [MogClient], or @@ -180,7 +175,7 @@ constructor( val tilesInSelectedArea = offlineArea.tiles if (tilesInSelectedArea.isEmpty()) Timber.w("No tiles associate with offline area $offlineArea") localOfflineAreaStore.deleteOfflineArea(offlineArea.id) - val remainingAreas = localOfflineAreaStore.offlineAreasOnceAndStream().awaitFirst() + val remainingAreas = localOfflineAreaStore.offlineAreas().first() val remainingTiles = remainingAreas.flatMap { it.tiles }.toSet() val tilesToRemove = tilesInSelectedArea - remainingTiles val tileSourcePath = getLocalTileSourcePath() diff --git a/ground/src/main/java/com/google/android/ground/ui/offlineareas/OfflineAreasViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/offlineareas/OfflineAreasViewModel.kt index 444c1855b8..7a8fafbab7 100644 --- a/ground/src/main/java/com/google/android/ground/ui/offlineareas/OfflineAreasViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/offlineareas/OfflineAreasViewModel.kt @@ -16,7 +16,7 @@ package com.google.android.ground.ui.offlineareas import androidx.lifecycle.LiveData -import androidx.lifecycle.toLiveData +import androidx.lifecycle.asLiveData import com.google.android.ground.model.imagery.OfflineArea import com.google.android.ground.repository.OfflineAreaRepository import com.google.android.ground.ui.common.AbstractViewModel @@ -24,8 +24,8 @@ import com.google.android.ground.ui.common.Navigator import com.google.android.ground.util.toMb import com.google.android.ground.util.toMbString import javax.inject.Inject -import kotlinx.coroutines.rx2.rxSingle -import timber.log.Timber +import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.onStart /** * View model for the offline area manager fragment. Handles the current list of downloaded areas. @@ -49,17 +49,11 @@ internal constructor( val showProgressSpinner: LiveData init { - val offlineAreas = - offlineAreaRepository - .offlineAreasOnceAndStream() - .switchMapSingle { rxSingle { toViewModel(it) } } - .doOnError { Timber.e(it, "Unexpected error loading offline areas from the local db") } - .onErrorReturnItem(listOf()) - - this.offlineAreas = offlineAreas.toLiveData() - showProgressSpinner = offlineAreas.map { false }.startWith(true).toLiveData() - showNoAreasMessage = offlineAreas.map { it.isEmpty() }.startWith(false).toLiveData() - showList = offlineAreas.map { it.isNotEmpty() }.startWith(false).toLiveData() + val offlineAreas = offlineAreaRepository.offlineAreas().map { toViewModel(it) } + this.offlineAreas = offlineAreas.asLiveData() + showProgressSpinner = offlineAreas.map { false }.onStart { emit(true) }.asLiveData() + showNoAreasMessage = offlineAreas.map { it.isEmpty() }.onStart { emit(false) }.asLiveData() + showList = offlineAreas.map { it.isNotEmpty() }.onStart { emit(false) }.asLiveData() } private suspend fun toViewModel( diff --git a/ground/src/test/java/com/google/android/ground/persistence/local/LocalDataStoreTests.kt b/ground/src/test/java/com/google/android/ground/persistence/local/LocalDataStoreTests.kt index 71b54b4226..8a9df40834 100644 --- a/ground/src/test/java/com/google/android/ground/persistence/local/LocalDataStoreTests.kt +++ b/ground/src/test/java/com/google/android/ground/persistence/local/LocalDataStoreTests.kt @@ -15,6 +15,7 @@ */ package com.google.android.ground.persistence.local +import app.cash.turbine.test import com.google.android.ground.BaseHiltTest import com.google.android.ground.model.Survey import com.google.android.ground.model.User @@ -345,7 +346,9 @@ class LocalDataStoreTests : BaseHiltTest() { @Test fun testGetOfflineAreas() = runWithTestDispatcher { localOfflineAreaStore.insertOrUpdate(TEST_OFFLINE_AREA) - localOfflineAreaStore.offlineAreasOnceAndStream().test().assertValue(listOf(TEST_OFFLINE_AREA)) + localOfflineAreaStore.offlineAreas().test { + assertThat(expectMostRecentItem()).isEqualTo(listOf(TEST_OFFLINE_AREA)) + } } @Test From 712bf499ac244aaffd30ce2b05f9e11776d3b7b9 Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Mon, 30 Oct 2023 22:45:16 +0530 Subject: [PATCH 04/18] [Test] Update tests to also check for button order in data collection fragments (#2024) --- .../ui/datacollection/tasks/AbstractTaskFragment.kt | 4 ++++ .../ui/datacollection/tasks/BaseTaskFragmentTest.kt | 9 +++++++-- .../ui/datacollection/tasks/date/DateTaskFragmentTest.kt | 2 +- .../tasks/location/CaptureLocationTaskFragmentTest.kt | 6 +++--- .../multiplechoice/MultipleChoiceTaskFragmentTest.kt | 2 +- .../tasks/number/NumberTaskFragmentTest.kt | 2 +- .../datacollection/tasks/photo/PhotoTaskFragmentTest.kt | 2 +- .../tasks/point/DropAPinTaskFragmentTest.kt | 7 ++++++- .../tasks/polygon/PolygonDrawingTaskFragmentTest.kt | 4 ++-- .../ui/datacollection/tasks/text/TextTaskFragmentTest.kt | 2 +- .../ui/datacollection/tasks/time/TimeTaskFragmentTest.kt | 2 +- 11 files changed, 28 insertions(+), 14 deletions(-) diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/AbstractTaskFragment.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/AbstractTaskFragment.kt index 6014744cb7..8dc2c90ae2 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/AbstractTaskFragment.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/AbstractTaskFragment.kt @@ -43,6 +43,7 @@ abstract class AbstractTaskFragment : AbstractFragmen hiltNavGraphViewModels(R.id.data_collection) private val buttons: EnumMap = EnumMap(ButtonAction::class.java) + private val buttonsIndex: MutableMap = mutableMapOf() private lateinit var taskView: TaskView protected lateinit var viewModel: T @@ -151,12 +152,15 @@ abstract class AbstractTaskFragment : AbstractFragmen taskView.actionButtonsContainer, layoutInflater ) + buttonsIndex[buttons.size] = action buttons[action] = button return button } @TestOnly fun getButtons() = buttons + @TestOnly fun getButtonsIndex() = buttonsIndex + protected fun getButton(action: ButtonAction): TaskButton { check(buttons.contains(action)) { "Expected key $action in $buttons" } return buttons[action]!! diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/BaseTaskFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/BaseTaskFragmentTest.kt index dfd7b0f8d5..2e2d007e1f 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/BaseTaskFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/BaseTaskFragmentTest.kt @@ -35,6 +35,7 @@ import com.google.android.ground.ui.common.ViewModelFactory import com.google.android.ground.ui.datacollection.DataCollectionViewModel import com.google.android.ground.ui.datacollection.components.ButtonAction import com.google.common.truth.Truth.assertThat +import com.google.common.truth.Truth.assertWithMessage import org.hamcrest.core.IsNot.not import org.mockito.kotlin.whenever @@ -72,9 +73,13 @@ abstract class BaseTaskFragmentTest, VM : AbstractT viewModel.taskDataFlow.test { assertThat(expectMostRecentItem()).isEqualTo(taskData) } } - protected fun hasButtons(vararg buttonActions: ButtonAction) { - // TODO: Also check for order of action buttons. + /** Asserts that the task fragment has the given list of buttons in the exact same order. */ + protected fun assertFragmentHasButtons(vararg buttonActions: ButtonAction) { assertThat(fragment.getButtons().keys).containsExactlyElementsIn(buttonActions) + buttonActions.withIndex().forEach { (index, expected) -> + val actual = fragment.getButtonsIndex()[index] + assertWithMessage("Incorrect button order").that(actual).isEqualTo(expected) + } } protected fun getButton(buttonAction: ButtonAction): View = diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/date/DateTaskFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/date/DateTaskFragmentTest.kt index c763ae938c..88d29e0d40 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/date/DateTaskFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/date/DateTaskFragmentTest.kt @@ -85,7 +85,7 @@ class DateTaskFragmentTest : BaseTaskFragmentTest(job, task) - hasButtons(ButtonAction.NEXT, ButtonAction.SKIP) + assertFragmentHasButtons(ButtonAction.SKIP, ButtonAction.NEXT) } @Test diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/location/CaptureLocationTaskFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/location/CaptureLocationTaskFragmentTest.kt index 4297e48b6a..5355cc8fbf 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/location/CaptureLocationTaskFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/location/CaptureLocationTaskFragmentTest.kt @@ -104,11 +104,11 @@ class CaptureLocationTaskFragmentTest : fun testActionButtons() { setupTaskFragment(job, task) - hasButtons( - ButtonAction.NEXT, + assertFragmentHasButtons( ButtonAction.SKIP, ButtonAction.UNDO, - ButtonAction.CAPTURE_LOCATION + ButtonAction.CAPTURE_LOCATION, + ButtonAction.NEXT ) } diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskFragmentTest.kt index 35bcf263ba..30b601a9fa 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskFragmentTest.kt @@ -143,7 +143,7 @@ class MultipleChoiceTaskFragmentTest : fun testActionButtons() { setupTaskFragment(job, task) - hasButtons(ButtonAction.NEXT, ButtonAction.SKIP) + assertFragmentHasButtons(ButtonAction.SKIP, ButtonAction.NEXT) } @Test diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/number/NumberTaskFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/number/NumberTaskFragmentTest.kt index 7c6a7ffc07..75039a4656 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/number/NumberTaskFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/number/NumberTaskFragmentTest.kt @@ -96,7 +96,7 @@ class NumberTaskFragmentTest : BaseTaskFragmentTest(job, task) - hasButtons(ButtonAction.NEXT, ButtonAction.SKIP) + assertFragmentHasButtons(ButtonAction.SKIP, ButtonAction.NEXT) } @Test diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragmentTest.kt index 60aac0e7c3..fd65dfb608 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragmentTest.kt @@ -65,7 +65,7 @@ class PhotoTaskFragmentTest : BaseTaskFragmentTest(job, task) - hasButtons(ButtonAction.NEXT, ButtonAction.SKIP, ButtonAction.UNDO) + assertFragmentHasButtons(ButtonAction.UNDO, ButtonAction.SKIP, ButtonAction.NEXT) } @Test diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/point/DropAPinTaskFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/point/DropAPinTaskFragmentTest.kt index 8f6ce18d0c..bc42a1030f 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/point/DropAPinTaskFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/point/DropAPinTaskFragmentTest.kt @@ -105,7 +105,12 @@ class DropAPinTaskFragmentTest : fun testActionButtons() { setupTaskFragment(job, task) - hasButtons(ButtonAction.NEXT, ButtonAction.SKIP, ButtonAction.UNDO, ButtonAction.DROP_PIN) + assertFragmentHasButtons( + ButtonAction.SKIP, + ButtonAction.UNDO, + ButtonAction.DROP_PIN, + ButtonAction.NEXT + ) } @Test diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/polygon/PolygonDrawingTaskFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/polygon/PolygonDrawingTaskFragmentTest.kt index 36433de04f..0b7a249aba 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/polygon/PolygonDrawingTaskFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/polygon/PolygonDrawingTaskFragmentTest.kt @@ -76,10 +76,10 @@ class PolygonDrawingTaskFragmentTest : fun testActionButtons() { setupTaskFragment(job, task) - hasButtons( - ButtonAction.NEXT, + assertFragmentHasButtons( ButtonAction.SKIP, ButtonAction.UNDO, + ButtonAction.NEXT, ButtonAction.ADD_POINT, ButtonAction.COMPLETE ) diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/text/TextTaskFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/text/TextTaskFragmentTest.kt index d0bf89fdd9..a8bf394aff 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/text/TextTaskFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/text/TextTaskFragmentTest.kt @@ -90,7 +90,7 @@ class TextTaskFragmentTest : BaseTaskFragmentTest(job, task) - hasButtons(ButtonAction.NEXT, ButtonAction.SKIP) + assertFragmentHasButtons(ButtonAction.SKIP, ButtonAction.NEXT) } @Test diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/time/TimeTaskFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/time/TimeTaskFragmentTest.kt index 5b230dfe62..dd8047da3b 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/time/TimeTaskFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/time/TimeTaskFragmentTest.kt @@ -82,7 +82,7 @@ class TimeTaskFragmentTest : BaseTaskFragmentTest(job, task) - hasButtons(ButtonAction.NEXT, ButtonAction.SKIP) + assertFragmentHasButtons(ButtonAction.SKIP, ButtonAction.NEXT) } @Test From c849c5a1ddf289267eca7da57acbf3c1d0369470 Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Mon, 30 Oct 2023 23:16:35 +0530 Subject: [PATCH 05/18] [Data Collection] Update "Next" to "Done" when on last task (#2023) --- .../datacollection/DataCollectionFragment.kt | 2 +- .../datacollection/DataCollectionViewModel.kt | 37 ++++++++++++------- .../datacollection/components/ButtonAction.kt | 1 + .../tasks/AbstractTaskFragment.kt | 15 ++++++-- .../location/CaptureLocationTaskFragment.kt | 2 +- .../tasks/point/DropAPinTaskFragment.kt | 2 +- .../DataCollectionFragmentTest.kt | 4 +- .../MultipleChoiceTaskFragmentTest.kt | 10 +++++ 8 files changed, 51 insertions(+), 22 deletions(-) diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionFragment.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionFragment.kt index 8e69cb4f5c..64c2319e0a 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionFragment.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionFragment.kt @@ -129,7 +129,7 @@ class DataCollectionFragment : Hilt_DataCollectionFragment(), BackPressListener false } else { // Otherwise, select the previous step. - viewModel.setCurrentPosition(viewModel.currentPosition.value!! - 1) + viewModel.updateCurrentPosition(viewModel.getVisibleTaskPosition() - 1) true } diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionViewModel.kt index b0ac5a769a..76c9fa250f 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionViewModel.kt @@ -116,10 +116,9 @@ internal constructor( private val responses: MutableMap = LinkedHashMap() - private val currentPositionKey = "currentPosition" - // Tracks the user's current position in the list of tasks for the current Job + // Tracks the task's current position in the list of tasks for the current job var currentPosition: @Hot(replays = true) MutableLiveData = - savedStateHandle.getLiveData(currentPositionKey, 0) + savedStateHandle.getLiveData(TASK_POSITION_KEY, 0) var currentTaskData: TaskData? = null @@ -162,7 +161,7 @@ internal constructor( * Validates the user's input and displays an error if the user input was invalid. Progresses to * the next Data Collection screen if the user input was valid. */ - fun onNextClicked() { + fun onNextClicked(position: Int) { val currentTask = currentTaskViewModel ?: return val validationError = currentTask.validate() @@ -171,16 +170,10 @@ internal constructor( return } - val currentTaskPosition = currentPosition.value!! - val finalTaskPosition = tasks.size - 1 - - assert(finalTaskPosition >= 0) - assert(currentTaskPosition in 0..finalTaskPosition) - responses[currentTask.task] = currentTaskData - if (currentTaskPosition != finalTaskPosition) { - setCurrentPosition(currentPosition.value!! + 1) + if (!isLastPosition(position)) { + updateCurrentPosition(position + 1) } else { val taskDataDeltas = responses.map { (task, taskData) -> TaskDataDelta(task.id, task.type, taskData) } @@ -202,14 +195,30 @@ internal constructor( } } - fun setCurrentPosition(position: Int) { - savedStateHandle[currentPositionKey] = position + /** Returns the position of the task fragment visible to the user. */ + fun getVisibleTaskPosition() = currentPosition.value!! + + /** Displays the task at the given position to the user. */ + fun updateCurrentPosition(position: Int) { + savedStateHandle[TASK_POSITION_KEY] = position + } + + /** Returns true if the given task position is last. */ + fun isLastPosition(taskPosition: Int): Boolean { + val finalTaskPosition = tasks.size - 1 + + assert(finalTaskPosition >= 0) + assert(taskPosition in 0..finalTaskPosition) + + return taskPosition == finalTaskPosition } private fun createSuggestLoiTask(taskType: Task.Type): Task = Task(id = "-1", index = -1, taskType, resources.getString(R.string.new_site), isRequired = true) companion object { + private const val TASK_POSITION_KEY = "currentPosition" + fun getViewModelClass(taskType: Task.Type): Class = when (taskType) { Task.Type.TEXT -> TextTaskViewModel::class.java diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/components/ButtonAction.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/components/ButtonAction.kt index 0b113d5240..a95bf60da7 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/components/ButtonAction.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/components/ButtonAction.kt @@ -28,6 +28,7 @@ enum class ButtonAction( ) { // All tasks + DONE(Type.TEXT, Theme.DARK_GREEN, textId = R.string.done), NEXT(Type.TEXT, Theme.DARK_GREEN, textId = R.string.next), SKIP(Type.TEXT, Theme.TRANSPARENT, textId = R.string.skip), UNDO(Type.ICON, Theme.LIGHT_GREEN, drawableId = R.drawable.ic_undo_black), diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/AbstractTaskFragment.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/AbstractTaskFragment.kt index 8dc2c90ae2..4adcc01e76 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/AbstractTaskFragment.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/AbstractTaskFragment.kt @@ -120,7 +120,7 @@ abstract class AbstractTaskFragment : AbstractFragmen protected fun addNextButton() = addButton(ButtonAction.NEXT) - .setOnClickListener { dataCollectionViewModel.onNextClicked() } + .setOnClickListener { moveToNext() } .setOnTaskUpdated { button, taskData -> button.enableIfTrue(taskData.isNotNullOrEmpty()) } .disable() @@ -135,7 +135,11 @@ abstract class AbstractTaskFragment : AbstractFragmen private fun onSkip() { check(viewModel.hasNoData()) { "User should not be able to skip a task with data." } - dataCollectionViewModel.onNextClicked() + moveToNext() + } + + fun moveToNext() { + dataCollectionViewModel.onNextClicked(position) } fun addUndoButton() = @@ -144,7 +148,8 @@ abstract class AbstractTaskFragment : AbstractFragmen .setOnTaskUpdated { button, taskData -> button.showIfTrue(taskData.isNotNullOrEmpty()) } .hide() - protected fun addButton(action: ButtonAction): TaskButton { + protected fun addButton(buttonAction: ButtonAction): TaskButton { + val action = if (buttonAction.shouldReplaceWithDoneButton()) ButtonAction.DONE else buttonAction check(!buttons.contains(action)) { "Button $action already bound" } val button = TaskButtonFactory.createAndAttachButton( @@ -157,6 +162,10 @@ abstract class AbstractTaskFragment : AbstractFragmen return button } + /** Returns true if the given [ButtonAction] should be replace with "Done" button. */ + private fun ButtonAction.shouldReplaceWithDoneButton() = + this == ButtonAction.NEXT && dataCollectionViewModel.isLastPosition(position) + @TestOnly fun getButtons() = buttons @TestOnly fun getButtonsIndex() = buttonsIndex diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/location/CaptureLocationTaskFragment.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/location/CaptureLocationTaskFragment.kt index 6c31f728cc..94c61cbc35 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/location/CaptureLocationTaskFragment.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/location/CaptureLocationTaskFragment.kt @@ -58,7 +58,7 @@ class CaptureLocationTaskFragment : .setOnClickListener { viewModel.updateResponse() } .setOnTaskUpdated { button, taskData -> button.showIfTrue(taskData.isNullOrEmpty()) } addButton(ButtonAction.NEXT) - .setOnClickListener { dataCollectionViewModel.onNextClicked() } + .setOnClickListener { moveToNext() } .setOnTaskUpdated { button, taskData -> button.showIfTrue(taskData.isNotNullOrEmpty()) } .hide() } diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/point/DropAPinTaskFragment.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/point/DropAPinTaskFragment.kt index e441822cd1..913edadaf4 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/point/DropAPinTaskFragment.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/point/DropAPinTaskFragment.kt @@ -55,7 +55,7 @@ class DropAPinTaskFragment : Hilt_DropAPinTaskFragment() .setOnClickListener { viewModel.dropPin() } .setOnTaskUpdated { button, taskData -> button.showIfTrue(taskData.isNullOrEmpty()) } addButton(ButtonAction.NEXT) - .setOnClickListener { dataCollectionViewModel.onNextClicked() } + .setOnClickListener { moveToNext() } .setOnTaskUpdated { button, taskData -> button.showIfTrue(taskData.isNotNullOrEmpty()) } .hide() } diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/DataCollectionFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/DataCollectionFragmentTest.kt index d00fb5cb7e..13fa77e8b5 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/DataCollectionFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/DataCollectionFragmentTest.kt @@ -146,9 +146,9 @@ class DataCollectionFragmentTest : BaseHiltTest() { onView(withText(TASK_1_NAME)).check(matches(not(isDisplayed()))) onView(withText(TASK_2_NAME)).check(matches(isDisplayed())) - // Click "next" on final task + // Click "done" on final task onView(allOf(withId(R.id.user_response_text), isDisplayed())).perform(typeText(task2Response)) - onView(allOf(withText("Next"), isDisplayed())).perform(click()) + onView(allOf(withText("Done"), isDisplayed())).perform(click()) advanceUntilIdle() verify(submissionRepository) diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskFragmentTest.kt index 30b601a9fa..cba9a61d02 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskFragmentTest.kt @@ -45,6 +45,8 @@ import org.junit.Assert.assertThrows import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mock +import org.mockito.kotlin.any +import org.mockito.kotlin.whenever import org.robolectric.RobolectricTestRunner import org.robolectric.shadows.ShadowAlertDialog @@ -146,6 +148,14 @@ class MultipleChoiceTaskFragmentTest : assertFragmentHasButtons(ButtonAction.SKIP, ButtonAction.NEXT) } + @Test + fun testActionButtons_whenLastTask() { + whenever(dataCollectionViewModel.isLastPosition(any())).thenReturn(true) + setupTaskFragment(job, task) + + hasButtons(ButtonAction.SKIP, ButtonAction.DONE) + } + @Test fun testActionButtons_whenTaskIsOptional() { setupTaskFragment(job, task.copy(isRequired = false)) From 8bc3f9a041783e56d5a0290f60e54dad29948c25 Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Mon, 30 Oct 2023 23:17:57 +0530 Subject: [PATCH 06/18] [Rx -> Coroutine] Use coroutine when fetching LOIs from local db (#2022) --- .../stores/RoomLocationOfInterestStore.kt | 8 --- .../stores/LocalLocationOfInterestStore.kt | 8 --- .../LocationOfInterestRepository.kt | 25 ++++---- .../HomeScreenMapContainerViewModel.kt | 5 +- .../persistence/local/LocalDataStoreTests.kt | 19 +++--- .../LocationOfInterestRepositoryTest.kt | 58 ++++++++++--------- 6 files changed, 57 insertions(+), 66 deletions(-) diff --git a/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomLocationOfInterestStore.kt b/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomLocationOfInterestStore.kt index 2820002545..5b31be7d81 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomLocationOfInterestStore.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomLocationOfInterestStore.kt @@ -52,14 +52,6 @@ class RoomLocationOfInterestStore @Inject internal constructor() : LocalLocation * local database and returns a [Flowable] that continually emits the complete set anew any time * the underlying table changes (insertions, deletions, updates). */ - override fun getLocationsOfInterestOnceAndStream( - survey: Survey - ): Flowable> = - locationOfInterestDao - .findOnceAndStream(survey.id, EntityState.DEFAULT) - .map { toLocationsOfInterest(survey, it) } - .subscribeOn(schedulers.io()) - override fun findLocationsOfInterest(survey: Survey) = locationOfInterestDao.findByState(survey.id, EntityState.DEFAULT).map { toLocationsOfInterest(survey, it) diff --git a/ground/src/main/java/com/google/android/ground/persistence/local/stores/LocalLocationOfInterestStore.kt b/ground/src/main/java/com/google/android/ground/persistence/local/stores/LocalLocationOfInterestStore.kt index 3050b4e46a..dc15dde52d 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/local/stores/LocalLocationOfInterestStore.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/local/stores/LocalLocationOfInterestStore.kt @@ -27,14 +27,6 @@ import kotlinx.coroutines.flow.Flow interface LocalLocationOfInterestStore : LocalMutationStore { - /** - * Returns a long-lived stream that emits the full set of LOIs for a survey on subscribe, and - * continues to return the full set each time a LOI is added/changed/removed. - */ - fun getLocationsOfInterestOnceAndStream( - survey: Survey - ): @Cold(terminates = false) Flowable> - /** * Returns a main-safe flow that emits the full set of LOIs for a survey on subscribe, and * continues to return the full set each time a LOI is added/changed/removed. diff --git a/ground/src/main/java/com/google/android/ground/repository/LocationOfInterestRepository.kt b/ground/src/main/java/com/google/android/ground/repository/LocationOfInterestRepository.kt index de12950aae..d8c506b341 100644 --- a/ground/src/main/java/com/google/android/ground/repository/LocationOfInterestRepository.kt +++ b/ground/src/main/java/com/google/android/ground/repository/LocationOfInterestRepository.kt @@ -37,7 +37,10 @@ import io.reactivex.Flowable import io.reactivex.Single import javax.inject.Inject import javax.inject.Singleton -import kotlinx.coroutines.reactive.awaitFirst +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.map /** * Coordinates persistence and retrieval of [LocationOfInterest] instances from remote, local, and @@ -122,23 +125,17 @@ constructor( MutationEntitySyncStatus.FAILED ) - /** Returns a flowable of all [LocationOfInterest] for the given [Survey]. */ - private fun getLocationsOfInterestOnceAndStream( - survey: Survey - ): Flowable> = localLoiStore.getLocationsOfInterestOnceAndStream(survey) - - fun getLocationsOfInterest(survey: Survey) = localLoiStore.findLocationsOfInterest(survey) + /** Returns a flow of all [LocationOfInterest] associated with the given [Survey]. */ + fun getLocationsOfInterests(survey: Survey): Flow> = + localLoiStore.findLocationsOfInterest(survey) /** Returns a list of geometries associated with the given [Survey]. */ suspend fun getAllGeometries(survey: Survey): List = - getLocationsOfInterestOnceAndStream(survey).awaitFirst().map { it.geometry } + getLocationsOfInterests(survey).first().map { it.geometry } - /** Returns a flowable of all [LocationOfInterest] within the map bounds (viewport). */ - fun getWithinBoundsOnceAndStream( - survey: Survey, - bounds: Bounds - ): Flowable> = - getLocationsOfInterestOnceAndStream(survey) + /** Returns a flow of all [LocationOfInterest] within the map bounds (viewport). */ + fun getWithinBounds(survey: Survey, bounds: Bounds): Flow> = + getLocationsOfInterests(survey) .map { lois -> lois.filter { bounds.contains(it.geometry) } } .distinctUntilChanged() } diff --git a/ground/src/main/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerViewModel.kt index fa0ad7670c..9ae2e674a0 100644 --- a/ground/src/main/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerViewModel.kt @@ -56,7 +56,6 @@ import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.mapNotNull import kotlinx.coroutines.flow.stateIn -import kotlinx.coroutines.reactive.asFlow import timber.log.Timber @OptIn(ExperimentalCoroutinesApi::class) @@ -128,7 +127,7 @@ internal constructor( .flatMapLatest { (survey, isZoomedIn) -> val bounds = currentCameraPosition.value?.bounds if (bounds == null || survey == null || !isZoomedIn) flowOf(listOf()) - else loiRepository.getWithinBoundsOnceAndStream(survey, bounds).asFlow() + else loiRepository.getWithinBounds(survey, bounds) } .stateIn(viewModelScope, SharingStarted.Lazily, listOf()) @@ -179,7 +178,7 @@ internal constructor( fun getZoomThresholdCrossed(): Observable = zoomThresholdCrossed private fun getLocationOfInterestFeatures(survey: Survey): Flow> = - loiRepository.getLocationsOfInterest(survey).map { + loiRepository.getLocationsOfInterests(survey).map { it.map { loi -> loi.toFeature() }.toPersistentSet() } diff --git a/ground/src/test/java/com/google/android/ground/persistence/local/LocalDataStoreTests.kt b/ground/src/test/java/com/google/android/ground/persistence/local/LocalDataStoreTests.kt index 8a9df40834..8cd86c98da 100644 --- a/ground/src/test/java/com/google/android/ground/persistence/local/LocalDataStoreTests.kt +++ b/ground/src/test/java/com/google/android/ground/persistence/local/LocalDataStoreTests.kt @@ -173,14 +173,16 @@ class LocalDataStoreTests : BaseHiltTest() { } @Test - fun testGetLoisOnceAndStream() = runWithTestDispatcher { + fun testFindLocationsOfInterest() = runWithTestDispatcher { localUserStore.insertOrUpdateUser(TEST_USER) localSurveyStore.insertOrUpdateSurvey(TEST_SURVEY) - val subscriber = localLoiStore.getLocationsOfInterestOnceAndStream(TEST_SURVEY).test() - subscriber.assertValue(setOf()) localLoiStore.applyAndEnqueue(TEST_LOI_MUTATION) + val loi = localLoiStore.getLocationOfInterest(TEST_SURVEY, "loi id").blockingGet() - subscriber.assertValueSet(setOf(setOf(), setOf(loi))) + + localLoiStore.findLocationsOfInterest(TEST_SURVEY).test { + assertThat(expectMostRecentItem()).isEqualTo(setOf(loi)) + } } @Test @@ -312,11 +314,12 @@ class LocalDataStoreTests : BaseHiltTest() { localSurveyStore.insertOrUpdateSurvey(TEST_SURVEY) localLoiStore.applyAndEnqueue(TEST_LOI_MUTATION) localSubmissionStore.applyAndEnqueue(TEST_SUBMISSION_MUTATION) - val subscriber = localLoiStore.getLocationsOfInterestOnceAndStream(TEST_SURVEY).test() // Assert that one LOI is streamed. val loi = localLoiStore.getLocationOfInterest(TEST_SURVEY, "loi id").blockingGet() - subscriber.assertValueAt(0, setOf(loi)) + localLoiStore.findLocationsOfInterest(TEST_SURVEY).test { + assertThat(expectMostRecentItem()).isEqualTo(setOf(loi)) + } val mutation = TEST_LOI_MUTATION.copy(id = null, type = Mutation.Type.DELETE) // Calling applyAndEnqueue marks the local LOI as deleted. @@ -329,7 +332,9 @@ class LocalDataStoreTests : BaseHiltTest() { } // Verify that the local LOI is now removed from the latest LOI stream. - subscriber.assertValueAt(1, setOf()) + localLoiStore.findLocationsOfInterest(TEST_SURVEY).test { + assertThat(expectMostRecentItem()).isEmpty() + } // After successful remote sync, delete LOI is called by LocalMutationSyncWorker. localLoiStore.deleteLocationOfInterest("loi id") diff --git a/ground/src/test/java/com/google/android/ground/repository/LocationOfInterestRepositoryTest.kt b/ground/src/test/java/com/google/android/ground/repository/LocationOfInterestRepositoryTest.kt index 6c43cb9a63..c9822babeb 100644 --- a/ground/src/test/java/com/google/android/ground/repository/LocationOfInterestRepositoryTest.kt +++ b/ground/src/test/java/com/google/android/ground/repository/LocationOfInterestRepositoryTest.kt @@ -15,18 +15,22 @@ */ package com.google.android.ground.repository +import app.cash.turbine.test import com.google.android.ground.BaseHiltTest import com.google.android.ground.domain.usecases.survey.ActivateSurveyUseCase -import com.google.android.ground.model.geometry.* +import com.google.android.ground.model.geometry.Coordinates +import com.google.android.ground.model.geometry.LinearRing +import com.google.android.ground.model.geometry.Point +import com.google.android.ground.model.geometry.Polygon import com.google.android.ground.model.mutation.Mutation.Type.CREATE import com.google.android.ground.persistence.sync.MutationSyncWorkManager import com.google.android.ground.ui.map.Bounds +import com.google.common.truth.Truth.assertThat import com.sharedtest.FakeData import com.sharedtest.persistence.remote.FakeRemoteDataStore import com.sharedtest.system.auth.FakeAuthenticationManager import dagger.hilt.android.testing.BindValue import dagger.hilt.android.testing.HiltAndroidTest -import java.util.* import javax.inject.Inject import kotlin.test.assertFailsWith import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -34,8 +38,11 @@ import kotlinx.coroutines.test.advanceUntilIdle import org.junit.Before import org.junit.Test import org.junit.runner.RunWith -import org.mockito.* -import org.mockito.Mockito.* +import org.mockito.ArgumentMatchers.anyString +import org.mockito.Mock +import org.mockito.Mockito.`when` +import org.mockito.kotlin.times +import org.mockito.kotlin.verify import org.robolectric.RobolectricTestRunner @HiltAndroidTest @@ -123,44 +130,43 @@ class LocationOfInterestRepositoryTest : BaseHiltTest() { // TODO(#1373): Add tests for getLocationsOfInterest once new LOI sync implemented. @Test - fun testLoiWithinBounds_whenOutOfBounds_returnsEmptyList() { + fun testLoiWithinBounds_whenOutOfBounds_returnsEmptyList() = runWithTestDispatcher { val southwest = Coordinates(-60.0, -60.0) val northeast = Coordinates(-50.0, -50.0) - locationOfInterestRepository - .getWithinBoundsOnceAndStream(TEST_SURVEY, Bounds(southwest, northeast)) - .test() - .assertValues(listOf()) + locationOfInterestRepository.getWithinBounds(TEST_SURVEY, Bounds(southwest, northeast)).test { + assertThat(expectMostRecentItem()).isEmpty() + } } @Test - fun testLoiWithinBounds_whenSomeLOIsInsideBounds_returnsPartialList() { + fun testLoiWithinBounds_whenSomeLOIsInsideBounds_returnsPartialList() = runWithTestDispatcher { val southwest = Coordinates(-20.0, -20.0) val northeast = Coordinates(-10.0, -10.0) - locationOfInterestRepository - .getWithinBoundsOnceAndStream(TEST_SURVEY, Bounds(southwest, northeast)) - .test() - .assertValues(listOf(TEST_POINT_OF_INTEREST_1, TEST_AREA_OF_INTEREST_1)) + locationOfInterestRepository.getWithinBounds(TEST_SURVEY, Bounds(southwest, northeast)).test { + assertThat(expectMostRecentItem()) + .isEqualTo(listOf(TEST_POINT_OF_INTEREST_1, TEST_AREA_OF_INTEREST_1)) + } } @Test - fun testLoiWithinBounds_whenAllLOIsInsideBounds_returnsCompleteList() { + fun testLoiWithinBounds_whenAllLOIsInsideBounds_returnsCompleteList() = runWithTestDispatcher { val southwest = Coordinates(-20.0, -20.0) val northeast = Coordinates(20.0, 20.0) - locationOfInterestRepository - .getWithinBoundsOnceAndStream(TEST_SURVEY, Bounds(southwest, northeast)) - .test() - .assertValues( - listOf( - TEST_POINT_OF_INTEREST_1, - TEST_POINT_OF_INTEREST_2, - TEST_POINT_OF_INTEREST_3, - TEST_AREA_OF_INTEREST_1, - TEST_AREA_OF_INTEREST_2 + locationOfInterestRepository.getWithinBounds(TEST_SURVEY, Bounds(southwest, northeast)).test { + assertThat(expectMostRecentItem()) + .isEqualTo( + listOf( + TEST_POINT_OF_INTEREST_1, + TEST_POINT_OF_INTEREST_2, + TEST_POINT_OF_INTEREST_3, + TEST_AREA_OF_INTEREST_1, + TEST_AREA_OF_INTEREST_2 + ) ) - ) + } } companion object { From bbbb4da5d0f9ea43434cd6268895592868936d6b Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Mon, 30 Oct 2023 23:29:22 +0530 Subject: [PATCH 07/18] Remove unused code (#2025) --- .../android/ground/persistence/local/room/dao/BaseDao.kt | 8 +------- .../local/room/stores/RoomLocationOfInterestStore.kt | 2 +- .../persistence/local/room/stores/RoomSubmissionStore.kt | 2 +- .../persistence/local/room/stores/RoomSurveyStore.kt | 2 +- 4 files changed, 4 insertions(+), 10 deletions(-) diff --git a/ground/src/main/java/com/google/android/ground/persistence/local/room/dao/BaseDao.kt b/ground/src/main/java/com/google/android/ground/persistence/local/room/dao/BaseDao.kt index 93d83c22d8..645be5e37e 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/local/room/dao/BaseDao.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/local/room/dao/BaseDao.kt @@ -18,7 +18,6 @@ package com.google.android.ground.persistence.local.room.dao import androidx.room.Delete import androidx.room.Insert import androidx.room.Update -import io.reactivex.Completable /** * Base interface for DAOs that implement operations on a specific entity type. @@ -27,18 +26,13 @@ import io.reactivex.Completable */ interface BaseDao { - /** Insert entity into local db. Main-safe. */ @Insert suspend fun insert(entity: E) - /** Update entity in local db. Main-safe. */ @Update suspend fun update(entity: E): Int @Update suspend fun updateAll(entities: List) - @Deprecated("Replace usage with deleteSuspend") @Delete fun delete(entity: E): Completable - - // TODO(#1581): Rename to delete once all existing usages are migrated to coroutine. - @Delete suspend fun deleteSuspend(entity: E) + @Delete suspend fun delete(entity: E) } /** Try to update the specified entity, and if it doesn't yet exist, create it. Main-safe. */ diff --git a/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomLocationOfInterestStore.kt b/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomLocationOfInterestStore.kt index 5b31be7d81..ca157c423f 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomLocationOfInterestStore.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomLocationOfInterestStore.kt @@ -121,7 +121,7 @@ class RoomLocationOfInterestStore @Inject internal constructor() : LocalLocation override suspend fun deleteLocationOfInterest(locationOfInterestId: String) { Timber.d("Deleting local location of interest : $locationOfInterestId") locationOfInterestDao.findByIdSuspend(locationOfInterestId)?.let { - locationOfInterestDao.deleteSuspend(it) + locationOfInterestDao.delete(it) } } diff --git a/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomSubmissionStore.kt b/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomSubmissionStore.kt index 1c17bdae55..b960db642e 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomSubmissionStore.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomSubmissionStore.kt @@ -192,7 +192,7 @@ class RoomSubmissionStore @Inject internal constructor() : LocalSubmissionStore } override suspend fun deleteSubmission(submissionId: String) { - submissionDao.findByIdSuspend(submissionId)?.let { submissionDao.deleteSuspend(it) } + submissionDao.findByIdSuspend(submissionId)?.let { submissionDao.delete(it) } } override fun getSubmissionMutationsByLocationOfInterestIdOnceAndStream( diff --git a/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomSurveyStore.kt b/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomSurveyStore.kt index 6346d3d43f..73298e1f1c 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomSurveyStore.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/local/room/stores/RoomSurveyStore.kt @@ -79,7 +79,7 @@ class RoomSurveyStore @Inject internal constructor() : LocalSurveyStore { /** Deletes the provided [Survey] from the local database, if it exists in the database. */ override suspend fun deleteSurvey(survey: Survey) = - surveyDao.deleteSuspend(survey.toLocalDataStoreObject()) + surveyDao.delete(survey.toLocalDataStoreObject()) private suspend fun insertOrUpdateOption(taskId: String, option: Option) = optionDao.insertOrUpdate(option.toLocalDataStoreObject(taskId)) From b154f5e77cddd8dd8c0b226468e62a632822603a Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Mon, 30 Oct 2023 23:58:30 +0530 Subject: [PATCH 08/18] Prevent app crash when checking for current user's permissions (#2026) --- .../com/google/android/ground/model/User.kt | 1 - .../ground/repository/UserRepository.kt | 7 +++- .../ground/repository/UserRepositoryTest.kt | 32 +++++++++++++++++++ .../MultipleChoiceTaskFragmentTest.kt | 2 +- 4 files changed, 39 insertions(+), 3 deletions(-) diff --git a/ground/src/main/java/com/google/android/ground/model/User.kt b/ground/src/main/java/com/google/android/ground/model/User.kt index fab5dc5a04..df2b086688 100644 --- a/ground/src/main/java/com/google/android/ground/model/User.kt +++ b/ground/src/main/java/com/google/android/ground/model/User.kt @@ -17,7 +17,6 @@ package com.google.android.ground.model /** Represents a single application user. */ data class User -@JvmOverloads constructor( val id: String, val email: String, diff --git a/ground/src/main/java/com/google/android/ground/repository/UserRepository.kt b/ground/src/main/java/com/google/android/ground/repository/UserRepository.kt index 9942b7fdd9..8e0bd82dd9 100644 --- a/ground/src/main/java/com/google/android/ground/repository/UserRepository.kt +++ b/ground/src/main/java/com/google/android/ground/repository/UserRepository.kt @@ -69,5 +69,10 @@ constructor( * survey. If no survey is active at the moment, then it returns false. */ fun canUserSubmitData(): Boolean = - surveyRepository.activeSurvey?.getRole(currentUser.email) != Role.VIEWER + try { + surveyRepository.activeSurvey?.getRole(currentUser.email) != Role.VIEWER + } catch (e: IllegalStateException) { + Timber.e(e, "Error getting role for user $currentUser") + false + } } diff --git a/ground/src/test/java/com/google/android/ground/repository/UserRepositoryTest.kt b/ground/src/test/java/com/google/android/ground/repository/UserRepositoryTest.kt index cb0a26b57d..cfdbaab6f2 100644 --- a/ground/src/test/java/com/google/android/ground/repository/UserRepositoryTest.kt +++ b/ground/src/test/java/com/google/android/ground/repository/UserRepositoryTest.kt @@ -16,6 +16,7 @@ package com.google.android.ground.repository import com.google.android.ground.BaseHiltTest +import com.google.android.ground.model.Role import com.google.android.ground.persistence.local.LocalValueStore import com.google.android.ground.persistence.local.stores.LocalUserStore import com.google.android.ground.system.NetworkManager @@ -40,6 +41,7 @@ class UserRepositoryTest : BaseHiltTest() { @Inject lateinit var fakeAuthenticationManager: FakeAuthenticationManager @Inject lateinit var localUserStore: LocalUserStore @Inject lateinit var localValueStore: LocalValueStore + @Inject lateinit var surveyRepository: SurveyRepository @Inject lateinit var userRepository: UserRepository @Inject lateinit var fakeRemoteDataStore: FakeRemoteDataStore @@ -91,4 +93,34 @@ class UserRepositoryTest : BaseHiltTest() { assertThat(localValueStore.lastActiveSurveyId).isEmpty() } + + @Test + fun `canUserSubmitData() when user has permissions returns true`() { + val user = FakeData.USER + val survey = FakeData.SURVEY.copy(acl = mapOf(Pair(user.email, Role.OWNER.toString()))) + fakeAuthenticationManager.setUser(user) + surveyRepository.activeSurvey = survey + + assertThat(userRepository.canUserSubmitData()).isTrue() + } + + @Test + fun `canUserSubmitData() when user doesn't have permissions returns false`() { + val user = FakeData.USER + val survey = FakeData.SURVEY.copy(acl = mapOf()) + fakeAuthenticationManager.setUser(user) + surveyRepository.activeSurvey = survey + + assertThat(userRepository.canUserSubmitData()).isFalse() + } + + @Test + fun `canUserSubmitData() when user email is empty returns false`() { + val user = FakeData.USER.copy(email = "") + val survey = FakeData.SURVEY.copy(acl = mapOf(Pair("user@gmail.com", Role.OWNER.toString()))) + fakeAuthenticationManager.setUser(user) + surveyRepository.activeSurvey = survey + + assertThat(userRepository.canUserSubmitData()).isFalse() + } } diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskFragmentTest.kt index cba9a61d02..470328b4f8 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskFragmentTest.kt @@ -153,7 +153,7 @@ class MultipleChoiceTaskFragmentTest : whenever(dataCollectionViewModel.isLastPosition(any())).thenReturn(true) setupTaskFragment(job, task) - hasButtons(ButtonAction.SKIP, ButtonAction.DONE) + assertFragmentHasButtons(ButtonAction.SKIP, ButtonAction.DONE) } @Test From 436bb8793bac4bef56ea5bef205ab0449a5ea5fd Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Mon, 30 Oct 2023 23:59:51 +0530 Subject: [PATCH 09/18] Revert "Use coroutine in Navigator (#2020)" (#2028) This reverts commit d1d66c7834a4b74122523a6dbc347a1a54e45c98. --- .../com/google/android/ground/MainActivity.kt | 25 +++++++++++++---- .../android/ground/ui/common/Navigator.kt | 28 ++++++++++--------- .../android/ground/MainViewModelTest.kt | 22 +++++++-------- .../ground/ui/home/HomeScreenFragmentTest.kt | 12 ++++---- .../HomeScreenMapContainerFragmentTest.kt | 15 ++++------ .../ui/tos/TermsOfServiceFragmentTest.kt | 13 ++++----- 6 files changed, 61 insertions(+), 54 deletions(-) diff --git a/ground/src/main/java/com/google/android/ground/MainActivity.kt b/ground/src/main/java/com/google/android/ground/MainActivity.kt index 72a27fec54..7dd5b7b1b0 100644 --- a/ground/src/main/java/com/google/android/ground/MainActivity.kt +++ b/ground/src/main/java/com/google/android/ground/MainActivity.kt @@ -20,19 +20,18 @@ import android.app.ProgressDialog import android.content.Intent import android.os.Bundle import androidx.core.view.WindowInsetsCompat -import androidx.lifecycle.lifecycleScope import androidx.navigation.NavDirections import androidx.navigation.fragment.NavHostFragment import com.google.android.ground.databinding.MainActBinding import com.google.android.ground.repository.UserRepository import com.google.android.ground.rx.RxAutoDispose.autoDisposable +import com.google.android.ground.rx.Schedulers import com.google.android.ground.system.ActivityStreams import com.google.android.ground.system.SettingsManager import com.google.android.ground.ui.common.* import dagger.hilt.android.AndroidEntryPoint import java8.util.function.Consumer import javax.inject.Inject -import kotlinx.coroutines.launch import timber.log.Timber /** @@ -52,6 +51,8 @@ class MainActivity : Hilt_MainActivity() { @Inject lateinit var popups: EphemeralPopups + @Inject lateinit var schedulers: Schedulers + private lateinit var viewModel: MainViewModel private lateinit var navHostFragment: NavHostFragment @@ -68,6 +69,22 @@ class MainActivity : Hilt_MainActivity() { callback.accept(this) } + navigator + .getNavigateRequests() + .observeOn(schedulers.ui()) + .`as`(autoDisposable(this)) + .subscribe { navDirections: NavDirections -> onNavigate(navDirections) } + + navigator + .getNavigateUpRequests() + .observeOn(schedulers.ui()) + .`as`(autoDisposable(this)) + .subscribe { navigateUp() } + + navigator.getFinishRequests().observeOn(schedulers.ui()).`as`(autoDisposable(this)).subscribe { + finish() + } + val binding = MainActBinding.inflate(layoutInflater) setContentView(binding.root) @@ -78,10 +95,6 @@ class MainActivity : Hilt_MainActivity() { viewModel.signInProgressDialogVisibility.observe(this) { visible: Boolean -> onSignInProgress(visible) } - - lifecycleScope.launch { navigator.getNavigateRequests().collect { onNavigate(it) } } - lifecycleScope.launch { navigator.getNavigateUpRequests().collect { navigateUp() } } - lifecycleScope.launch { navigator.getFinishRequests().collect { finish() } } } override fun onWindowInsetChanged(insets: WindowInsetsCompat) { diff --git a/ground/src/main/java/com/google/android/ground/ui/common/Navigator.kt b/ground/src/main/java/com/google/android/ground/ui/common/Navigator.kt index 74bb5ee390..f730d20fa9 100644 --- a/ground/src/main/java/com/google/android/ground/ui/common/Navigator.kt +++ b/ground/src/main/java/com/google/android/ground/ui/common/Navigator.kt @@ -16,42 +16,44 @@ package com.google.android.ground.ui.common import androidx.navigation.NavDirections +import com.google.android.ground.rx.annotations.Hot +import io.reactivex.Observable +import io.reactivex.subjects.PublishSubject +import io.reactivex.subjects.Subject import javax.inject.Inject import javax.inject.Singleton import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers.Main -import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.flow.MutableStateFlow -import kotlinx.coroutines.flow.filterNotNull import kotlinx.coroutines.launch /** * Responsible for abstracting navigation from fragment to fragment. Exposes various actions to - * ViewModels that cause a NavDirections to be emitted to the flow. + * ViewModels that cause a NavDirections to be emitted to the observer (in this case, the [ ], which + * is expected to pass it to the current [ ]. */ @Singleton class Navigator @Inject constructor() { - private val navigateRequests: MutableStateFlow = MutableStateFlow(null) - private val navigateUpRequests: MutableStateFlow = MutableStateFlow(null) - private val finishRequests: MutableStateFlow = MutableStateFlow(null) + private val navigateRequests: @Hot Subject = PublishSubject.create() + private val navigateUpRequests: @Hot Subject = PublishSubject.create() + private val finishRequests: @Hot Subject = PublishSubject.create() /** Stream of navigation requests for fulfillment by the view layer. */ - fun getNavigateRequests(): Flow = navigateRequests.filterNotNull() + fun getNavigateRequests(): Observable = navigateRequests - fun getNavigateUpRequests(): Flow = navigateUpRequests.filterNotNull() + fun getNavigateUpRequests(): Observable = navigateUpRequests - fun getFinishRequests(): Flow = finishRequests.filterNotNull() + fun getFinishRequests(): Observable = finishRequests /** Navigates up one level on the back stack. */ fun navigateUp() { - CoroutineScope(Main).launch { navigateUpRequests.emit(Any()) } + navigateUpRequests.onNext(Any()) } fun navigate(directions: NavDirections) { - CoroutineScope(Main).launch { navigateRequests.emit(directions) } + CoroutineScope(Main).launch { navigateRequests.onNext(directions) } } fun finishApp() { - CoroutineScope(Main).launch { finishRequests.emit(Any()) } + finishRequests.onNext(Any()) } } diff --git a/ground/src/test/java/com/google/android/ground/MainViewModelTest.kt b/ground/src/test/java/com/google/android/ground/MainViewModelTest.kt index 0597c249ec..ec0094aff9 100644 --- a/ground/src/test/java/com/google/android/ground/MainViewModelTest.kt +++ b/ground/src/test/java/com/google/android/ground/MainViewModelTest.kt @@ -18,7 +18,6 @@ package com.google.android.ground import android.content.SharedPreferences import android.os.Looper import androidx.navigation.NavDirections -import app.cash.turbine.test import com.google.android.ground.persistence.local.room.LocalDataStoreException import com.google.android.ground.repository.TermsOfServiceRepository import com.google.android.ground.repository.UserRepository @@ -33,6 +32,7 @@ import com.sharedtest.TestObservers.observeUntilFirstChange import com.sharedtest.persistence.remote.FakeRemoteDataStore import com.sharedtest.system.auth.FakeAuthenticationManager import dagger.hilt.android.testing.HiltAndroidTest +import io.reactivex.observers.TestObserver import javax.inject.Inject import kotlin.test.assertFailsWith import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -56,11 +56,16 @@ class MainViewModelTest : BaseHiltTest() { @Inject lateinit var tosRepository: TermsOfServiceRepository @Inject lateinit var userRepository: UserRepository + private lateinit var navDirectionsTestObserver: TestObserver + @Before override fun setUp() { super.setUp() fakeAuthenticationManager.setUser(FakeData.USER) + + // Subscribe to navigation requests + navDirectionsTestObserver = navigator.getNavigateRequests().test() } private fun setupUserPreferences() { @@ -84,16 +89,11 @@ class MainViewModelTest : BaseHiltTest() { assertThat(viewModel.signInProgressDialogVisibility.value).isEqualTo(visible) } - private fun verifyNavigationRequested(navDirections: NavDirections? = null) = - runWithTestDispatcher { - navigator.getNavigateRequests().test { - if (navDirections == null) { - expectNoEvents() - } else { - assertThat(expectMostRecentItem()).isEqualTo(navDirections) - } - } - } + private fun verifyNavigationRequested(vararg navDirections: NavDirections) { + navDirectionsTestObserver.assertNoErrors() + navDirectionsTestObserver.assertNotComplete() + navDirectionsTestObserver.assertValues(*navDirections) + } @Test fun testSignInStateChanged_onSignedOut() { diff --git a/ground/src/test/java/com/google/android/ground/ui/home/HomeScreenFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/home/HomeScreenFragmentTest.kt index f3f95cf488..6664c35f05 100644 --- a/ground/src/test/java/com/google/android/ground/ui/home/HomeScreenFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/home/HomeScreenFragmentTest.kt @@ -27,7 +27,6 @@ import androidx.test.espresso.matcher.ViewMatchers import androidx.test.espresso.matcher.ViewMatchers.isEnabled import androidx.test.espresso.matcher.ViewMatchers.withId import androidx.test.espresso.matcher.ViewMatchers.withText -import app.cash.turbine.test import com.google.android.ground.BaseHiltTest import com.google.android.ground.R import com.google.android.ground.launchFragmentInHiltContainer @@ -35,7 +34,6 @@ import com.google.android.ground.model.Survey import com.google.android.ground.model.imagery.TileSource import com.google.android.ground.repository.SurveyRepository import com.google.android.ground.ui.common.Navigator -import com.google.common.truth.Truth.assertThat import com.sharedtest.FakeData import com.squareup.picasso.Picasso import dagger.hilt.android.testing.HiltAndroidTest @@ -43,6 +41,7 @@ import javax.inject.Inject import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.advanceUntilIdle import org.hamcrest.CoreMatchers.not +import org.junit.Assert.* import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -154,7 +153,6 @@ class HomeScreenFragmentTest : AbstractHomeScreenFragmentTest() { } } -@OptIn(ExperimentalCoroutinesApi::class) @HiltAndroidTest @RunWith(ParameterizedRobolectricTestRunner::class) class NavigationDrawerItemClickTest( @@ -165,13 +163,13 @@ class NavigationDrawerItemClickTest( @Inject lateinit var navigator: Navigator @Test - fun clickDrawerMenuItem() = runWithTestDispatcher { + fun clickDrawerMenuItem() { + val navDirectionsTestObserver = navigator.getNavigateRequests().test() + openDrawer() onView(withText(menuItemLabel)).check(matches(isEnabled())).perform(click()) - navigator.getNavigateRequests().test { - assertThat(expectMostRecentItem()).isEqualTo(expectedNavDirection) - } + navDirectionsTestObserver.assertValue(expectedNavDirection) verifyDrawerClosed() } diff --git a/ground/src/test/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerFragmentTest.kt index b4407e4350..97192bedf2 100644 --- a/ground/src/test/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerFragmentTest.kt @@ -20,21 +20,18 @@ import androidx.test.espresso.action.ViewActions.click import androidx.test.espresso.assertion.ViewAssertions.matches import androidx.test.espresso.matcher.ViewMatchers.isEnabled import androidx.test.espresso.matcher.ViewMatchers.withId -import app.cash.turbine.test import com.google.android.ground.* import com.google.android.ground.NavGraphDirections.ShowMapTypeDialogFragment import com.google.android.ground.ui.common.Navigator import com.google.android.ground.ui.map.MapType -import com.google.common.truth.Truth.assertThat import dagger.hilt.android.testing.HiltAndroidTest import javax.inject.Inject -import kotlinx.coroutines.ExperimentalCoroutinesApi +import org.junit.Assert.* import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner -@OptIn(ExperimentalCoroutinesApi::class) @HiltAndroidTest @RunWith(RobolectricTestRunner::class) class HomeScreenMapContainerFragmentTest : BaseHiltTest() { @@ -48,13 +45,13 @@ class HomeScreenMapContainerFragmentTest : BaseHiltTest() { } @Test - fun clickMapType_launchesMapTypeDialogFragment() = runWithTestDispatcher { + fun clickMapType_launchesMapTypeDialogFragment() { + val navDirectionsTestObserver = navigator.getNavigateRequests().test() + onView(withId(R.id.map_type_btn)).perform(click()).check(matches(isEnabled())) - navigator.getNavigateRequests().test { - val result = expectMostRecentItem() - assertThat(result).isInstanceOf(ShowMapTypeDialogFragment::class.java) - assertThat((result as ShowMapTypeDialogFragment).mapTypes).isEqualTo(MapType.values()) + navDirectionsTestObserver.assertValue { + it is ShowMapTypeDialogFragment && it.mapTypes.contentEquals(MapType.values()) } } } diff --git a/ground/src/test/java/com/google/android/ground/ui/tos/TermsOfServiceFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/tos/TermsOfServiceFragmentTest.kt index 7008a4cef2..323ded52e7 100644 --- a/ground/src/test/java/com/google/android/ground/ui/tos/TermsOfServiceFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/tos/TermsOfServiceFragmentTest.kt @@ -20,7 +20,6 @@ import androidx.test.espresso.Espresso.onView import androidx.test.espresso.action.ViewActions.click import androidx.test.espresso.assertion.ViewAssertions.matches import androidx.test.espresso.matcher.ViewMatchers.* -import app.cash.turbine.test import com.google.android.ground.BaseHiltTest import com.google.android.ground.R import com.google.android.ground.launchFragmentInHiltContainer @@ -30,13 +29,11 @@ import com.google.android.ground.ui.surveyselector.SurveySelectorFragmentDirecti import com.google.common.truth.Truth.assertThat import dagger.hilt.android.testing.HiltAndroidTest import javax.inject.Inject -import kotlinx.coroutines.ExperimentalCoroutinesApi import org.hamcrest.Matchers.not import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner -@OptIn(ExperimentalCoroutinesApi::class) @HiltAndroidTest @RunWith(RobolectricTestRunner::class) class TermsOfServiceFragmentTest : BaseHiltTest() { @@ -72,19 +69,19 @@ class TermsOfServiceFragmentTest : BaseHiltTest() { } @Test - fun agreeButton_whenPressed_shouldUpdatePrefAndNavigate() = runWithTestDispatcher { + fun agreeButton_whenPressed_shouldUpdatePrefAndNavigate() { launchFragmentInHiltContainer(bundleOf()) assertThat(termsOfServiceRepository.isTermsOfServiceAccepted).isFalse() + val navDirectionsTestObserver = navigator.getNavigateRequests().test() onView(withId(R.id.agreeCheckBox)).perform(click()) onView(withId(R.id.agreeButton)).perform(click()) assertThat(termsOfServiceRepository.isTermsOfServiceAccepted).isTrue() - navigator.getNavigateRequests().test { - assertThat(expectMostRecentItem()) - .isEqualTo(SurveySelectorFragmentDirections.showSurveySelectorScreen(true)) - } + navDirectionsTestObserver.assertValue( + SurveySelectorFragmentDirections.showSurveySelectorScreen(true) + ) } @Test From 923eaac73f781d6356de0b834f1bd850b7a5f713 Mon Sep 17 00:00:00 2001 From: Gino Miceli <228050+gino-m@users.noreply.github.com> Date: Tue, 31 Oct 2023 19:55:17 +0200 Subject: [PATCH 10/18] Make survey list reactive to local and remote changes (#2030) * Upgrade Firestore deps * Draft remote / local survey list * Remove dead comment * Refactor SurveyListItem * Cleanup and refactor * Fix test * Fix test * Fix tests --- ground/build.gradle | 4 +- .../SurveyItem.kt => model/SurveyListItem.kt} | 15 +++-- .../persistence/remote/RemoteDataStore.kt | 4 +- .../remote/firebase/FirestoreDataStore.kt | 16 ++++- .../schema/SurveysCollectionReference.kt | 11 ++-- .../ground/repository/SurveyRepository.kt | 42 ++++++++----- .../android/ground/system/NetworkManager.kt | 34 +++++++++++ .../ui/surveyselector/SurveyListAdapter.kt | 13 ++-- .../surveyselector/SurveySelectorViewModel.kt | 42 +++---------- .../src/main/res/layout/survey_card_item.xml | 14 ++--- .../ground/repository/SurveyRepositoryTest.kt | 7 ++- .../SurveySelectorFragmentTest.kt | 61 ++++++++++--------- sharedTest/build.gradle | 3 +- .../persistence/remote/FakeRemoteDataStore.kt | 7 ++- 14 files changed, 162 insertions(+), 111 deletions(-) rename ground/src/main/java/com/google/android/ground/{ui/surveyselector/SurveyItem.kt => model/SurveyListItem.kt} (67%) diff --git a/ground/build.gradle b/ground/build.gradle index 33a49a0f8e..d4e14fced2 100644 --- a/ground/build.gradle +++ b/ground/build.gradle @@ -193,9 +193,9 @@ dependencies { testImplementation 'org.json:json:20180813' // Firebase and related libraries. - implementation platform('com.google.firebase:firebase-bom:32.2.3') + implementation platform('com.google.firebase:firebase-bom:32.4.1') implementation 'com.google.firebase:firebase-analytics' - implementation 'com.google.firebase:firebase-firestore-ktx' + implementation 'com.google.firebase:firebase-firestore' implementation 'com.google.firebase:firebase-functions-ktx' implementation 'com.google.firebase:firebase-auth' implementation 'com.google.firebase:firebase-perf' diff --git a/ground/src/main/java/com/google/android/ground/ui/surveyselector/SurveyItem.kt b/ground/src/main/java/com/google/android/ground/model/SurveyListItem.kt similarity index 67% rename from ground/src/main/java/com/google/android/ground/ui/surveyselector/SurveyItem.kt rename to ground/src/main/java/com/google/android/ground/model/SurveyListItem.kt index d5b2f4379b..1b51fdef8d 100644 --- a/ground/src/main/java/com/google/android/ground/ui/surveyselector/SurveyItem.kt +++ b/ground/src/main/java/com/google/android/ground/model/SurveyListItem.kt @@ -14,11 +14,14 @@ * limitations under the License. */ -package com.google.android.ground.ui.surveyselector +package com.google.android.ground.model -data class SurveyItem( - val surveyId: String, - val surveyTitle: String, - val surveyDescription: String, - val isAvailableOffline: Boolean +data class SurveyListItem( + val id: String, + val title: String, + val description: String, + val availableOffline: Boolean ) + +fun Survey.toListItem(availableOffline: Boolean): SurveyListItem = + SurveyListItem(id, title, description, availableOffline) diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/RemoteDataStore.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/RemoteDataStore.kt index 7713c691ab..79399e671e 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/RemoteDataStore.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/RemoteDataStore.kt @@ -16,18 +16,20 @@ package com.google.android.ground.persistence.remote import com.google.android.ground.model.Survey +import com.google.android.ground.model.SurveyListItem import com.google.android.ground.model.TermsOfService import com.google.android.ground.model.User import com.google.android.ground.model.locationofinterest.LocationOfInterest import com.google.android.ground.model.mutation.Mutation import com.google.android.ground.model.submission.Submission +import kotlinx.coroutines.flow.Flow /** * Defines API for accessing data in a remote data store. Implementations must ensure all * subscriptions are run in a background thread (i.e., not the Android main thread). */ interface RemoteDataStore { - suspend fun loadSurveySummaries(user: User): List + fun getSurveyList(user: User): Flow> /** * Loads the survey with the specified id from the remote data store. Returns `null` if the survey diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirestoreDataStore.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirestoreDataStore.kt index 5a7b5d01bd..7912985dee 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirestoreDataStore.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirestoreDataStore.kt @@ -17,6 +17,7 @@ package com.google.android.ground.persistence.remote.firebase import com.google.android.ground.coroutines.IoDispatcher import com.google.android.ground.model.Survey +import com.google.android.ground.model.SurveyListItem import com.google.android.ground.model.TermsOfService import com.google.android.ground.model.User import com.google.android.ground.model.locationofinterest.LocationOfInterest @@ -24,6 +25,7 @@ import com.google.android.ground.model.mutation.LocationOfInterestMutation import com.google.android.ground.model.mutation.Mutation import com.google.android.ground.model.mutation.SubmissionMutation import com.google.android.ground.model.submission.Submission +import com.google.android.ground.model.toListItem import com.google.android.ground.persistence.remote.RemoteDataStore import com.google.firebase.firestore.WriteBatch import com.google.firebase.functions.FirebaseFunctions @@ -32,6 +34,10 @@ import com.google.firebase.messaging.ktx.messaging import javax.inject.Inject import javax.inject.Singleton import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.emitAll +import kotlinx.coroutines.flow.flow +import kotlinx.coroutines.flow.map import kotlinx.coroutines.tasks.await import kotlinx.coroutines.withContext import timber.log.Timber @@ -64,8 +70,14 @@ internal constructor( override suspend fun loadTermsOfService(): TermsOfService? = withContext(ioDispatcher) { db().termsOfService().terms().get() } - override suspend fun loadSurveySummaries(user: User): List = - withContext(ioDispatcher) { db().surveys().getReadable(user) } + override fun getSurveyList(user: User): Flow> = flow { + emitAll( + db().surveys().getReadable(user).map { list -> + // TODO(#2031): Return SurveyListItem from getReadable(), only fetch required fields. + list.map { it.toListItem(false) } + } + ) + } override suspend fun loadLocationsOfInterest(survey: Survey) = db().surveys().survey(survey.id).lois().locationsOfInterest(survey) diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/SurveysCollectionReference.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/SurveysCollectionReference.kt index 58be8b0688..1a67982c5e 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/SurveysCollectionReference.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/SurveysCollectionReference.kt @@ -21,8 +21,10 @@ import com.google.android.ground.model.Survey import com.google.android.ground.model.User import com.google.android.ground.persistence.remote.firebase.base.FluentCollectionReference import com.google.firebase.firestore.CollectionReference -import com.google.firebase.firestore.DocumentSnapshot import com.google.firebase.firestore.FieldPath +import com.google.firebase.firestore.snapshots +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.map private const val ACL_FIELD = "acl" @@ -31,9 +33,8 @@ class SurveysCollectionReference internal constructor(ref: CollectionReference) fun survey(id: String) = SurveyDocumentReference(reference().document(id)) - suspend fun getReadable(user: User): List = - runQuery(reference().whereIn(FieldPath.of(ACL_FIELD, user.email), Role.valueStrings())) { - doc: DocumentSnapshot -> - SurveyConverter.toSurvey(doc) + fun getReadable(user: User): Flow> = + reference().whereIn(FieldPath.of(ACL_FIELD, user.email), Role.valueStrings()).snapshots().map { + it.documents.map { doc -> SurveyConverter.toSurvey(doc) } } } diff --git a/ground/src/main/java/com/google/android/ground/repository/SurveyRepository.kt b/ground/src/main/java/com/google/android/ground/repository/SurveyRepository.kt index 433e6b6bf3..bf6a1d262a 100644 --- a/ground/src/main/java/com/google/android/ground/repository/SurveyRepository.kt +++ b/ground/src/main/java/com/google/android/ground/repository/SurveyRepository.kt @@ -17,30 +17,34 @@ package com.google.android.ground.repository import com.google.android.ground.coroutines.ApplicationScope import com.google.android.ground.model.Survey +import com.google.android.ground.model.SurveyListItem import com.google.android.ground.model.User +import com.google.android.ground.model.toListItem 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 com.google.android.ground.rx.annotations.Cold +import com.google.android.ground.system.NetworkManager +import com.google.android.ground.system.NetworkStatus import io.reactivex.Flowable import java8.util.Optional import javax.inject.Inject import javax.inject.Singleton import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharedFlow import kotlinx.coroutines.flow.SharingStarted -import kotlinx.coroutines.flow.asFlow +import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.shareIn import kotlinx.coroutines.rx2.asFlowable -import kotlinx.coroutines.withTimeout import kotlinx.coroutines.withTimeoutOrNull import timber.log.Timber private const val LOAD_REMOTE_SURVEY_TIMEOUT_MILLS: Long = 15 * 1000 -private const val LOAD_REMOTE_SURVEY_SUMMARIES_TIMEOUT_MILLIS: Long = 30 * 1000 /** * Coordinates persistence and retrieval of [Survey] instances from remote, local, and in memory @@ -54,6 +58,7 @@ constructor( private val localSurveyStore: LocalSurveyStore, private val remoteDataStore: RemoteDataStore, private val localValueStore: LocalValueStore, + private val networkManager: NetworkManager, @ApplicationScope private val externalScope: CoroutineScope ) { private val _activeSurvey = MutableStateFlow(null) @@ -82,8 +87,8 @@ constructor( val activeSurveyFlowable: @Cold Flowable> = activeSurveyFlow.map { if (it == null) Optional.empty() else Optional.of(it) }.asFlowable() - val offlineSurveys: Flow> - get() = localSurveyStore.surveys + val localSurveyListFlow: Flow> + get() = localSurveyStore.surveys.map { list -> list.map { it.toListItem(true) } } var lastActiveSurveyId: String by localValueStore::lastActiveSurveyId internal set @@ -115,17 +120,22 @@ constructor( activeSurvey = null } - suspend fun getSurveySummaries(user: User): Flow> = - try { - val surveys = - withTimeout(LOAD_REMOTE_SURVEY_SUMMARIES_TIMEOUT_MILLIS) { - Timber.d("Loading survey list from remote") - remoteDataStore.loadSurveySummaries(user) - } - listOf(surveys).asFlow() - } catch (e: Throwable) { - Timber.d(e, "Failed to load survey list from remote") - offlineSurveys + fun getSurveyList(user: User): Flow> = + @OptIn(ExperimentalCoroutinesApi::class) + networkManager.networkStatusFlow.flatMapLatest { networkStatus -> + if (networkStatus == NetworkStatus.AVAILABLE) { + getRemoteSurveyList(user) + } else { + localSurveyListFlow + } + } + + private fun getRemoteSurveyList(user: User): Flow> = + remoteDataStore.getSurveyList(user).combine(localSurveyListFlow) { remoteSurveys, localSurveys + -> + remoteSurveys.map { remoteSurvey -> + remoteSurvey.copy(availableOffline = localSurveys.any { it.id == remoteSurvey.id }) + } } /** Attempts to remove the locally synced survey. Doesn't throw an error if it doesn't exist. */ diff --git a/ground/src/main/java/com/google/android/ground/system/NetworkManager.kt b/ground/src/main/java/com/google/android/ground/system/NetworkManager.kt index 9ed369f9df..44020769c9 100644 --- a/ground/src/main/java/com/google/android/ground/system/NetworkManager.kt +++ b/ground/src/main/java/com/google/android/ground/system/NetworkManager.kt @@ -17,15 +17,49 @@ package com.google.android.ground.system import android.content.Context import android.net.ConnectivityManager +import android.net.Network +import android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET +import android.net.NetworkRequest import androidx.annotation.RequiresPermission import dagger.hilt.android.qualifiers.ApplicationContext import java.net.ConnectException import javax.inject.Inject import javax.inject.Singleton +import kotlinx.coroutines.channels.awaitClose +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.callbackFlow +enum class NetworkStatus { + AVAILABLE, + UNAVAILABLE +} /** Abstracts access to network state. */ @Singleton class NetworkManager @Inject constructor(@ApplicationContext private val context: Context) { + val networkStatusFlow = initNetworkStatusFlow() + + fun initNetworkStatusFlow(): Flow { + val connectivityManager = context.getSystemService(ConnectivityManager::class.java) + return callbackFlow { + val callback = + object : ConnectivityManager.NetworkCallback() { + override fun onAvailable(network: Network) { + trySend(NetworkStatus.AVAILABLE) + } + + override fun onLost(network: Network) { + trySend(NetworkStatus.UNAVAILABLE) + } + } + + val request = NetworkRequest.Builder().addCapability(NET_CAPABILITY_INTERNET).build() + // Emit initial state. + trySend(if (isNetworkConnected()) NetworkStatus.AVAILABLE else NetworkStatus.UNAVAILABLE) + connectivityManager.registerNetworkCallback(request, callback) + + awaitClose { connectivityManager.unregisterNetworkCallback(callback) } + } + } /** Returns true iff the device has internet connectivity, false otherwise. */ @RequiresPermission("android.permission.ACCESS_NETWORK_STATE") diff --git a/ground/src/main/java/com/google/android/ground/ui/surveyselector/SurveyListAdapter.kt b/ground/src/main/java/com/google/android/ground/ui/surveyselector/SurveyListAdapter.kt index d3513895de..5508da90b4 100644 --- a/ground/src/main/java/com/google/android/ground/ui/surveyselector/SurveyListAdapter.kt +++ b/ground/src/main/java/com/google/android/ground/ui/surveyselector/SurveyListAdapter.kt @@ -20,10 +20,11 @@ import android.view.LayoutInflater import android.view.ViewGroup import androidx.recyclerview.widget.RecyclerView import com.google.android.ground.databinding.SurveyCardItemBinding +import com.google.android.ground.model.SurveyListItem import com.google.android.ground.ui.surveyselector.SurveyListAdapter.ViewHolder /** - * An implementation of [RecyclerView.Adapter] that associates [SurveyItem] data with the + * An implementation of [RecyclerView.Adapter] that associates [SurveyListItem] data with the * [ViewHolder] views. */ class SurveyListAdapter( @@ -31,7 +32,7 @@ class SurveyListAdapter( private val fragment: SurveySelectorFragment ) : RecyclerView.Adapter() { - private val surveys: MutableList = mutableListOf() + private val surveys: MutableList = mutableListOf() /** Creates a new [ViewHolder] item without any data. */ override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): ViewHolder { @@ -39,9 +40,9 @@ class SurveyListAdapter( return ViewHolder(binding) } - /** Binds [SurveyItem] data to [ViewHolder]. */ + /** Binds [SurveyListItem] data to [ViewHolder]. */ override fun onBindViewHolder(holder: ViewHolder, position: Int) { - val item: SurveyItem = surveys[position] + val item: SurveyListItem = surveys[position] holder.binding.item = item holder.binding.viewModel = viewModel holder.binding.fragment = fragment @@ -51,13 +52,13 @@ class SurveyListAdapter( override fun getItemCount() = surveys.size /** Overwrites existing cards. */ - fun updateData(newItemsList: List) { + fun updateData(newItemsList: List) { surveys.clear() surveys.addAll(newItemsList) notifyDataSetChanged() } - /** View item representing the [SurveyItem] data in the list. */ + /** View item representing the [SurveyListItem] data in the list. */ class ViewHolder(internal val binding: SurveyCardItemBinding) : RecyclerView.ViewHolder(binding.root) } diff --git a/ground/src/main/java/com/google/android/ground/ui/surveyselector/SurveySelectorViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/surveyselector/SurveySelectorViewModel.kt index 9aadeb41a3..4a26f457ae 100644 --- a/ground/src/main/java/com/google/android/ground/ui/surveyselector/SurveySelectorViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/surveyselector/SurveySelectorViewModel.kt @@ -19,7 +19,7 @@ import androidx.lifecycle.viewModelScope import com.google.android.ground.coroutines.ApplicationScope import com.google.android.ground.coroutines.IoDispatcher import com.google.android.ground.domain.usecases.survey.ActivateSurveyUseCase -import com.google.android.ground.model.Survey +import com.google.android.ground.model.SurveyListItem import com.google.android.ground.repository.SurveyRepository import com.google.android.ground.repository.UserRepository import com.google.android.ground.system.auth.AuthenticationManager @@ -29,16 +29,14 @@ import com.google.android.ground.ui.home.HomeScreenFragmentDirections import javax.inject.Inject import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.FlowPreview import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow -import kotlinx.coroutines.flow.flatMapMerge import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.flow.onStart import kotlinx.coroutines.launch /** Represents view state and behaviors of the survey selector dialog. */ -@OptIn(FlowPreview::class) class SurveySelectorViewModel @Inject internal constructor( @@ -52,11 +50,11 @@ internal constructor( ) : AbstractViewModel() { val surveyListState: MutableStateFlow = MutableStateFlow(null) - val surveySummaries: Flow> + val surveySummaries: Flow> init { surveySummaries = - createSurveySummaries().onEach { + getSurveyList().onEach { if (it.isEmpty()) { setNotFound() } else { @@ -65,32 +63,12 @@ internal constructor( } } - /** Returns a flow of locally stored surveys. */ - private fun offlineSurveys(): Flow> = - surveyRepository.offlineSurveys.onEach { setLoading() } - - /** Returns a flow of remotely stored surveys. */ - private suspend fun allSurveys(): Flow> = - surveyRepository.getSurveySummaries(authManager.currentUser).onEach { setLoading() } - - /** Returns a flow of [SurveyItem] to be displayed to the user. */ - private fun createSurveySummaries(): Flow> = - offlineSurveys().flatMapMerge { offlineSurveys: List -> - allSurveys().map { allSurveys: List -> - allSurveys - .map { createSurveyItem(it, offlineSurveys) } - .sortedBy { it.surveyTitle } - .sortedByDescending { it.isAvailableOffline } - } - } - - private fun createSurveyItem(survey: Survey, localSurveys: List): SurveyItem = - SurveyItem( - surveyId = survey.id, - surveyTitle = survey.title, - surveyDescription = survey.description, - isAvailableOffline = localSurveys.any { it.id == survey.id } - ) + /** Returns a flow of [SurveyListItem] to be displayed to the user. */ + private fun getSurveyList(): Flow> = + surveyRepository + .getSurveyList(authManager.currentUser) + .onStart { setLoading() } + .map { surveys -> surveys.sortedBy { it.title }.sortedByDescending { it.availableOffline } } /** Triggers the specified survey to be loaded and activated. */ fun activateSurvey(surveyId: String) = diff --git a/ground/src/main/res/layout/survey_card_item.xml b/ground/src/main/res/layout/survey_card_item.xml index 55ed54ed13..2d52b0af65 100644 --- a/ground/src/main/res/layout/survey_card_item.xml +++ b/ground/src/main/res/layout/survey_card_item.xml @@ -23,7 +23,7 @@ + type="com.google.android.ground.model.SurveyListItem" /> @@ -42,7 +42,7 @@ style="@style/Widget.App.CardView.SurfaceContainerLowest" android:clickable="true" android:focusable="true" - android:onClick="@{() -> viewModel.activateSurvey(item.surveyId)}" + android:onClick="@{() -> viewModel.activateSurvey(item.id)}" android:orientation="vertical"> + app:visible="@{item.availableOffline}" />