From 9625c2eb857b038f727525bde63b396d80e516b4 Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Mon, 2 Dec 2024 23:14:49 +0530 Subject: [PATCH 1/2] [Code health] Exclude preview functions from test coverage (#2888) * Add a new annotation for excluding functions from jacoco report * Annotate preview functions with generated annotation --- .../ExcludeFromJacocoGeneratedReport.kt | 21 +++++++++++++++++++ .../android/ground/PermissionDeniedDialog.kt | 2 ++ .../android/ground/ui/compose/HtmlText.kt | 2 ++ .../DataSubmissionConfirmationDialog.kt | 2 ++ .../location/LocationPermissionDialog.kt | 2 ++ .../android/ground/ui/home/AboutFragment.kt | 2 ++ .../ground/ui/home/DataSharingTermsDialog.kt | 5 +++++ .../ui/home/SignOutConfirmationDialog.kt | 2 ++ .../ground/ui/home/UserDetailsDialog.kt | 2 ++ .../ui/offlineareas/OfflineAreaListItem.kt | 2 ++ .../ground/ui/syncstatus/SyncListItem.kt | 2 ++ 11 files changed, 44 insertions(+) create mode 100644 ground/src/main/java/com/google/android/ground/ExcludeFromJacocoGeneratedReport.kt diff --git a/ground/src/main/java/com/google/android/ground/ExcludeFromJacocoGeneratedReport.kt b/ground/src/main/java/com/google/android/ground/ExcludeFromJacocoGeneratedReport.kt new file mode 100644 index 0000000000..ff94fc1c9b --- /dev/null +++ b/ground/src/main/java/com/google/android/ground/ExcludeFromJacocoGeneratedReport.kt @@ -0,0 +1,21 @@ +/* + * Copyright 2024 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 + +// https://stackoverflow.com/questions/47824761/how-would-i-add-an-annotation-to-exclude-a-method-from-a-jacoco-code-coverage-re +@Retention(AnnotationRetention.RUNTIME) +@Target(AnnotationTarget.FUNCTION) +annotation class ExcludeFromJacocoGeneratedReport diff --git a/ground/src/main/java/com/google/android/ground/PermissionDeniedDialog.kt b/ground/src/main/java/com/google/android/ground/PermissionDeniedDialog.kt index ac82e3f399..85c8e50126 100644 --- a/ground/src/main/java/com/google/android/ground/PermissionDeniedDialog.kt +++ b/ground/src/main/java/com/google/android/ground/PermissionDeniedDialog.kt @@ -108,6 +108,7 @@ private fun SignupLink(signupLink: String) { } @Preview +@ExcludeFromJacocoGeneratedReport @Composable fun PermissionDeniedDialogWithSignupLinkPreview() { AppTheme { @@ -116,6 +117,7 @@ fun PermissionDeniedDialogWithSignupLinkPreview() { } @Preview +@ExcludeFromJacocoGeneratedReport @Composable fun PermissionDeniedDialogWithoutSignupLinkPreview() { AppTheme { PermissionDeniedDialog(signupLink = "", onSignOut = {}, onCloseApp = {}) } diff --git a/ground/src/main/java/com/google/android/ground/ui/compose/HtmlText.kt b/ground/src/main/java/com/google/android/ground/ui/compose/HtmlText.kt index 8c0e21d93e..4fa9194cb6 100644 --- a/ground/src/main/java/com/google/android/ground/ui/compose/HtmlText.kt +++ b/ground/src/main/java/com/google/android/ground/ui/compose/HtmlText.kt @@ -23,6 +23,7 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.viewinterop.AndroidView import androidx.core.text.HtmlCompat +import com.google.android.ground.ExcludeFromJacocoGeneratedReport import com.google.android.ground.R import com.google.android.ground.ui.theme.AppTheme @@ -42,6 +43,7 @@ fun HtmlText(html: String, modifier: Modifier = Modifier) { @Composable @Preview +@ExcludeFromJacocoGeneratedReport fun PreviewHtmlText() { AppTheme { HtmlText(html = "

Hello World


This is a preview.") } } diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/DataSubmissionConfirmationDialog.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/DataSubmissionConfirmationDialog.kt index 3f9ad474d8..329556aa74 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/DataSubmissionConfirmationDialog.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/DataSubmissionConfirmationDialog.kt @@ -45,6 +45,7 @@ import androidx.compose.ui.text.style.TextAlign import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.dp import androidx.compose.ui.unit.sp +import com.google.android.ground.ExcludeFromJacocoGeneratedReport import com.google.android.ground.R import com.google.android.ground.ui.theme.AppTheme @@ -140,6 +141,7 @@ private fun CloseButton(modifier: Modifier = Modifier, onDismiss: () -> Unit) { @Composable @Preview(heightDp = 320, widthDp = 800) @Preview +@ExcludeFromJacocoGeneratedReport fun DataSubmissionConfirmationDialogPreview() { AppTheme { DataSubmissionConfirmationDialog {} } } diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/location/LocationPermissionDialog.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/location/LocationPermissionDialog.kt index ab67663c4a..37e60c18c4 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/location/LocationPermissionDialog.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/location/LocationPermissionDialog.kt @@ -26,6 +26,7 @@ import androidx.compose.runtime.Composable import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.res.stringResource import androidx.compose.ui.tooling.preview.Preview +import com.google.android.ground.ExcludeFromJacocoGeneratedReport import com.google.android.ground.R import com.google.android.ground.ui.theme.AppTheme @@ -68,6 +69,7 @@ fun LocationPermissionDialog(onDismiss: () -> Unit, onCancel: () -> Unit) { @Composable @Preview +@ExcludeFromJacocoGeneratedReport fun PreviewUserDetailsDialog() { AppTheme { LocationPermissionDialog({}, {}) } } diff --git a/ground/src/main/java/com/google/android/ground/ui/home/AboutFragment.kt b/ground/src/main/java/com/google/android/ground/ui/home/AboutFragment.kt index 7ac707bd76..b336a01ca9 100644 --- a/ground/src/main/java/com/google/android/ground/ui/home/AboutFragment.kt +++ b/ground/src/main/java/com/google/android/ground/ui/home/AboutFragment.kt @@ -41,6 +41,7 @@ import androidx.compose.ui.unit.dp import androidx.core.content.res.ResourcesCompat import androidx.navigation.fragment.findNavController import com.google.android.gms.oss.licenses.OssLicensesMenuActivity +import com.google.android.ground.ExcludeFromJacocoGeneratedReport import com.google.android.ground.R import com.google.android.ground.ui.common.AbstractFragment import com.google.android.ground.ui.compose.HyperlinkText @@ -58,6 +59,7 @@ class AboutFragment : AbstractFragment() { ): View = ComposeView(requireContext()).apply { setContent { AppTheme { CreateView() } } } @Preview + @ExcludeFromJacocoGeneratedReport @Composable private fun CreateView() { Scaffold( diff --git a/ground/src/main/java/com/google/android/ground/ui/home/DataSharingTermsDialog.kt b/ground/src/main/java/com/google/android/ground/ui/home/DataSharingTermsDialog.kt index 7f73b8fa1c..30d16872e5 100644 --- a/ground/src/main/java/com/google/android/ground/ui/home/DataSharingTermsDialog.kt +++ b/ground/src/main/java/com/google/android/ground/ui/home/DataSharingTermsDialog.kt @@ -29,6 +29,7 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.res.stringResource import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.dp +import com.google.android.ground.ExcludeFromJacocoGeneratedReport import com.google.android.ground.R import com.google.android.ground.proto.Survey import com.google.android.ground.ui.compose.HtmlText @@ -92,6 +93,7 @@ fun DataSharingTermsDialog( @Composable @Preview +@ExcludeFromJacocoGeneratedReport fun DataSharingTermsDialogPreviewWithPrivateType() { AppTheme { DataSharingTermsDialog( @@ -104,6 +106,7 @@ fun DataSharingTermsDialogPreviewWithPrivateType() { @Composable @Preview +@ExcludeFromJacocoGeneratedReport fun DataSharingTermsDialogPreviewWithPublicType() { AppTheme { DataSharingTermsDialog( @@ -118,6 +121,7 @@ fun DataSharingTermsDialogPreviewWithPublicType() { @Composable @Preview +@ExcludeFromJacocoGeneratedReport fun DataSharingTermsDialogPreviewWithCustomType() { AppTheme { DataSharingTermsDialog( @@ -133,6 +137,7 @@ fun DataSharingTermsDialogPreviewWithCustomType() { @Composable @Preview +@ExcludeFromJacocoGeneratedReport fun DataSharingTermsDialogPreviewWithNoType() { AppTheme { DataSharingTermsDialog( diff --git a/ground/src/main/java/com/google/android/ground/ui/home/SignOutConfirmationDialog.kt b/ground/src/main/java/com/google/android/ground/ui/home/SignOutConfirmationDialog.kt index 0be5ee1231..2a801195a2 100644 --- a/ground/src/main/java/com/google/android/ground/ui/home/SignOutConfirmationDialog.kt +++ b/ground/src/main/java/com/google/android/ground/ui/home/SignOutConfirmationDialog.kt @@ -21,6 +21,7 @@ import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable import androidx.compose.ui.res.stringResource import androidx.compose.ui.tooling.preview.Preview +import com.google.android.ground.ExcludeFromJacocoGeneratedReport import com.google.android.ground.R import com.google.android.ground.ui.theme.AppTheme @@ -48,6 +49,7 @@ fun SignOutConfirmationDialog(signOutCallback: () -> Unit, dismissCallback: () - @Composable @Preview +@ExcludeFromJacocoGeneratedReport fun PreviewSignOutConfirmationDialog() { AppTheme { SignOutConfirmationDialog(signOutCallback = {}, dismissCallback = {}) } } diff --git a/ground/src/main/java/com/google/android/ground/ui/home/UserDetailsDialog.kt b/ground/src/main/java/com/google/android/ground/ui/home/UserDetailsDialog.kt index 70f59f6638..590fb497d9 100644 --- a/ground/src/main/java/com/google/android/ground/ui/home/UserDetailsDialog.kt +++ b/ground/src/main/java/com/google/android/ground/ui/home/UserDetailsDialog.kt @@ -22,6 +22,7 @@ import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable import androidx.compose.ui.res.stringResource import androidx.compose.ui.tooling.preview.Preview +import com.google.android.ground.ExcludeFromJacocoGeneratedReport import com.google.android.ground.R import com.google.android.ground.model.User import com.google.android.ground.ui.theme.AppTheme @@ -48,6 +49,7 @@ fun UserDetailsDialog(user: User, signOutCallback: () -> Unit, dismissCallback: @Composable @Preview +@ExcludeFromJacocoGeneratedReport fun PreviewUserDetailsDialog() { AppTheme { UserDetailsDialog( diff --git a/ground/src/main/java/com/google/android/ground/ui/offlineareas/OfflineAreaListItem.kt b/ground/src/main/java/com/google/android/ground/ui/offlineareas/OfflineAreaListItem.kt index 3eb3ff8df1..5296ed2ba0 100644 --- a/ground/src/main/java/com/google/android/ground/ui/offlineareas/OfflineAreaListItem.kt +++ b/ground/src/main/java/com/google/android/ground/ui/offlineareas/OfflineAreaListItem.kt @@ -37,6 +37,7 @@ import androidx.compose.ui.text.font.FontFamily import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.dp import androidx.compose.ui.unit.sp +import com.google.android.ground.ExcludeFromJacocoGeneratedReport import com.google.android.ground.R import com.google.android.ground.ui.theme.AppTheme @@ -100,6 +101,7 @@ fun OfflineAreaListItem( @Composable @Preview(showBackground = true, showSystemUi = true) +@ExcludeFromJacocoGeneratedReport fun PreviewOfflineAreaListItem() { AppTheme { OfflineAreaListItem( diff --git a/ground/src/main/java/com/google/android/ground/ui/syncstatus/SyncListItem.kt b/ground/src/main/java/com/google/android/ground/ui/syncstatus/SyncListItem.kt index 6df7cbf04c..20096ad930 100644 --- a/ground/src/main/java/com/google/android/ground/ui/syncstatus/SyncListItem.kt +++ b/ground/src/main/java/com/google/android/ground/ui/syncstatus/SyncListItem.kt @@ -41,6 +41,7 @@ import androidx.compose.ui.text.font.FontWeight import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.dp import androidx.compose.ui.unit.sp +import com.google.android.ground.ExcludeFromJacocoGeneratedReport import com.google.android.ground.R import com.google.android.ground.model.job.Job import com.google.android.ground.model.mutation.Mutation @@ -137,6 +138,7 @@ private fun Mutation.SyncStatus.toIcon(): Int = @Composable @Preview(showBackground = true, showSystemUi = true) +@ExcludeFromJacocoGeneratedReport fun PreviewSyncListItem( detail: MutationDetail = MutationDetail( From 9ab91bc7bb3df8c1dccdc20f33792373383965b8 Mon Sep 17 00:00:00 2001 From: Gino Miceli <228050+gino-m@users.noreply.github.com> Date: Mon, 2 Dec 2024 15:20:20 -0500 Subject: [PATCH 2/2] [Data sync] Rewrite data and photo sync workers to be more robust and readable (#2867) * Add wrapper for related mutations * Repository accessor for upload queue * Add ktdoc * Update repo methods * Update ktdoc * Update ktdoc * Rewrite sync worker WIP * Stop syncing per LOI * Simplify upload queue API * Enqueue media workers (part 1) * Update ktdoc * MediaUploadWorker cleanup WIP * MediaUploadWorker cleanup WIP * MediaUploadWorker cleanup WIP * MediaUploadWorker cleanup WIP * MediaUploadWorker cleanup WIP * Minor refactors * Remove unused import * Media worker - process entire queue * Minor readability improvements * Clean up unused * Add TODO * Add media mutation class * Revert addition of MediaMutation * Refactor all the things * Make method private * Remove unused funs * Make method private * Remove obsolete TODO * Roll back fun rename * Update ktdoc * Lift ktdoc into interface * Make tests build * Make tests build * Format TODO * Remove obsolete tests * Increment retry on photo upload failure * Use static imports for constants * Fix compile errors in tests * Tweak return syntax * Add TODO * Rename val * Tweak logging * Fix tests * Remove unused fun * Remove unused member * Update ktdoc * Fix test * Only kick workers if items in queue * Filter user and log upstream * Log network issues as debug * Tweak workaround * Small refactor and TODO * Rename function * Tweak TODO * Tweak ktdoc * Alphabetize constructor args * Remove unused constant * Add explicit type --- .../java/com/google/android/ground/Config.kt | 3 - .../model/mutation/SubmissionMutation.kt | 7 - .../model/submission/UploadQueueEntry.kt | 36 +++++ .../sync/LocalMutationSyncWorker.kt | 88 ++++--------- .../sync/MediaUploadWorkManager.kt | 6 +- .../persistence/sync/MediaUploadWorker.kt | 114 +++++----------- .../sync/MutationSyncWorkManager.kt | 17 +-- .../LocationOfInterestRepository.kt | 2 +- .../ground/repository/MutationRepository.kt | 123 ++++++++++++------ .../ground/repository/SubmissionRepository.kt | 2 +- .../ground/ui/home/HomeScreenViewModel.kt | 12 +- .../com/google/android/ground/util/Debug.kt | 4 + .../sync/LocalMutationSyncWorkerTest.kt | 61 ++++----- .../persistence/sync/MediaUploadWorkerTest.kt | 111 ++++++++-------- .../LocationOfInterestRepositoryTest.kt | 7 +- 15 files changed, 271 insertions(+), 322 deletions(-) create mode 100644 ground/src/main/java/com/google/android/ground/model/submission/UploadQueueEntry.kt diff --git a/ground/src/main/java/com/google/android/ground/Config.kt b/ground/src/main/java/com/google/android/ground/Config.kt index 987d9fc09f..860be54621 100644 --- a/ground/src/main/java/com/google/android/ground/Config.kt +++ b/ground/src/main/java/com/google/android/ground/Config.kt @@ -44,9 +44,6 @@ object Config { */ const val CLUSTERING_ZOOM_THRESHOLD = 14f - /** Maximum number of attempts for retrying unsuccessful media uploads. */ - const val MAX_MEDIA_UPLOAD_RETRY_COUNT = 5 - // TODO(#1730): Make sub-paths configurable and stop hardcoding here. const val DEFAULT_MOG_TILE_LOCATION = "/offline-imagery/default" private const val DEFAULT_MOG_MIN_ZOOM = 8 diff --git a/ground/src/main/java/com/google/android/ground/model/mutation/SubmissionMutation.kt b/ground/src/main/java/com/google/android/ground/model/mutation/SubmissionMutation.kt index 6a83d51233..85a61629a8 100644 --- a/ground/src/main/java/com/google/android/ground/model/mutation/SubmissionMutation.kt +++ b/ground/src/main/java/com/google/android/ground/model/mutation/SubmissionMutation.kt @@ -38,15 +38,8 @@ data class SubmissionMutation( override fun toString(): String = super.toString() + "deltas= $deltas" - fun incrementRetryCount() = this.copy(retryCount = this.retryCount + 1) - fun updateSyncStatus(status: SyncStatus) = this.copy(syncStatus = status) - /** Returns true if this mutation is in a state in which it is ready for media upload. */ - fun mediaUploadPending() = - this.syncStatus == SyncStatus.MEDIA_UPLOAD_PENDING || - this.syncStatus == SyncStatus.MEDIA_UPLOAD_AWAITING_RETRY - fun getPhotoData(): List = deltas.map { it.newTaskData }.filterIsInstance().filter { !it.isEmpty() } } diff --git a/ground/src/main/java/com/google/android/ground/model/submission/UploadQueueEntry.kt b/ground/src/main/java/com/google/android/ground/model/submission/UploadQueueEntry.kt new file mode 100644 index 0000000000..b7fd97712b --- /dev/null +++ b/ground/src/main/java/com/google/android/ground/model/submission/UploadQueueEntry.kt @@ -0,0 +1,36 @@ +/* + * Copyright 2024 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.model.submission + +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 java.util.Date + +/** + * A set of changes to be applied to the remote datastore, initiated by the user by completing the + * data collection flow and clicking "Submit". + */ +data class UploadQueueEntry( + val userId: String, + val clientTimestamp: Date, + val uploadStatus: Mutation.SyncStatus, + val loiMutation: LocationOfInterestMutation?, + val submissionMutation: SubmissionMutation?, +) { + fun mutations(): List = listOfNotNull(loiMutation, submissionMutation) +} diff --git a/ground/src/main/java/com/google/android/ground/persistence/sync/LocalMutationSyncWorker.kt b/ground/src/main/java/com/google/android/ground/persistence/sync/LocalMutationSyncWorker.kt index 06a3c06e4a..ba795443d2 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/sync/LocalMutationSyncWorker.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/sync/LocalMutationSyncWorker.kt @@ -18,19 +18,14 @@ package com.google.android.ground.persistence.sync import android.content.Context import androidx.hilt.work.HiltWorker import androidx.work.CoroutineWorker -import androidx.work.Data import androidx.work.ListenableWorker.Result.retry import androidx.work.ListenableWorker.Result.success import androidx.work.WorkerParameters -import com.google.android.ground.model.User import com.google.android.ground.model.mutation.Mutation -import com.google.android.ground.persistence.local.room.fields.MutationEntitySyncStatus.FAILED -import com.google.android.ground.persistence.local.room.fields.MutationEntitySyncStatus.IN_PROGRESS -import com.google.android.ground.persistence.local.room.fields.MutationEntitySyncStatus.PENDING import com.google.android.ground.persistence.remote.RemoteDataStore -import com.google.android.ground.persistence.sync.LocalMutationSyncWorker.Companion.createInputData import com.google.android.ground.repository.MutationRepository import com.google.android.ground.repository.UserRepository +import com.google.android.ground.util.priority import dagger.assisted.Assisted import dagger.assisted.AssistedInject import kotlinx.coroutines.Dispatchers @@ -38,9 +33,8 @@ import kotlinx.coroutines.withContext import timber.log.Timber /** - * A worker that syncs local changes to the remote data store. Each instance handles mutations for a - * specific map location of interest, whose id is provided in the [Data] object built by - * [createInputData] and provided to the worker request while being enqueued. + * A worker that uploads all pending local changes to the remote data store. Larger uploads (photos) + * are then delegated to [MediaUploadWorkManager], which is enqueued and run in parallel. */ @HiltWorker class LocalMutationSyncWorker @@ -54,73 +48,39 @@ constructor( private val userRepository: UserRepository, ) : CoroutineWorker(context, params) { - private val locationOfInterestId: String = - params.inputData.getString(LOCATION_OF_INTEREST_ID_PARAM_KEY)!! - - override suspend fun doWork(): Result = withContext(Dispatchers.IO) { doWorkInternal() } - - private suspend fun doWorkInternal(): Result = - try { - val mutations = getIncompleteMutations() - Timber.d("Syncing ${mutations.size} changes for LOI $locationOfInterestId") - val result = processMutations(mutations) - mediaUploadWorkManager.enqueueSyncWorker(locationOfInterestId) - if (result) success() else retry() - } catch (t: Throwable) { - Timber.e(t, "Failed to sync changes for LOI $locationOfInterestId") - retry() + override suspend fun doWork(): Result = + withContext(Dispatchers.IO) { + val queue = mutationRepository.getIncompleteUploads() + Timber.d("Uploading ${queue.size} additions / changes") + val results = queue.map { processMutations(it.mutations()) } + if (results.any { it }) mediaUploadWorkManager.enqueueSyncWorker() + if (results.all { it }) success() else retry() } /** - * Attempts to fetch all mutations from the [MutationRepository] that are `PENDING`, `FAILED`, or - * `IN_PROGRESS` state. The latter should never occur since only on worker should be scheduled per - * LOI at a given time. - */ - private suspend fun getIncompleteMutations(): List = - mutationRepository.getMutations(locationOfInterestId, PENDING, FAILED, IN_PROGRESS) - - /** - * Applies mutations to remote data store. Once successful, removes them from the local db. + * Applies mutations to remote data store, updating their status in the queue accordingly. Catches + * and handles all exceptions. * - * @return `true` if the mutations were successfully synced with [RemoteDataStore]. + * @return `true` if all mutations were successfully synced with [RemoteDataStore], `false` if at + * least one failed. */ private suspend fun processMutations(mutations: List): Boolean { if (mutations.isEmpty()) return true - return try { + try { val user = userRepository.getAuthenticatedUser() - filterMutationsByUser(mutations, user) - .takeIf { it.isNotEmpty() } - ?.let { - mutationRepository.markAsInProgress(it) - remoteDataStore.applyMutations(it, user) - mutationRepository.finalizePendingMutationsForMediaUpload(it) - } - true + mutationRepository.markAsInProgress(mutations) + // TODO(https://github.com/google/ground-android/issues/2883): + // Apply mutations via repository layer rather than accessing data store directly. + remoteDataStore.applyMutations(mutations, user) + mutationRepository.finalizePendingMutationsForMediaUpload(mutations) + + return true } catch (t: Throwable) { // Mark all mutations as having failed since the remote datastore only commits when all // mutations have succeeded. mutationRepository.markAsFailed(mutations, t) - Timber.e(t, "Failed to sync survey ${mutations.first().surveyId}") - false - } - } - - private fun filterMutationsByUser(mutations: List, user: User): List { - val userIds = mutations.map { it.userId }.toSet() - if (userIds.size != 1) { - Timber.e("Expected exactly 1 user, but found ${userIds.size}") + Timber.log(t.priority(), t, "Failed to sync local data") + return false } - val (validMutations, invalidMutations) = mutations.partition { it.userId == user.id } - invalidMutations.forEach { Timber.e("Invalid mutation: $it") } - return validMutations - } - - companion object { - internal const val LOCATION_OF_INTEREST_ID_PARAM_KEY = "locationOfInterestId" - - /** Returns a new work [Data] object containing the specified location of interest id. */ - @JvmStatic - fun createInputData(locationOfInterestId: String): Data = - Data.Builder().putString(LOCATION_OF_INTEREST_ID_PARAM_KEY, locationOfInterestId).build() } } diff --git a/ground/src/main/java/com/google/android/ground/persistence/sync/MediaUploadWorkManager.kt b/ground/src/main/java/com/google/android/ground/persistence/sync/MediaUploadWorkManager.kt index e6b3f7af46..6db58c8e84 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/sync/MediaUploadWorkManager.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/sync/MediaUploadWorkManager.kt @@ -19,7 +19,6 @@ import androidx.work.ExistingWorkPolicy import androidx.work.NetworkType import androidx.work.WorkManager import com.google.android.ground.persistence.local.LocalValueStore -import com.google.android.ground.persistence.sync.MediaUploadWorker.Companion.createInputData import javax.inject.Inject /** Enqueues media upload work to be performed in the background. */ @@ -38,13 +37,12 @@ constructor(private val workManager: WorkManager, private val localValueStore: L * This method returns as soon as the worker is added to the work queue, not when the work * completes. */ - fun enqueueSyncWorker(locationOfInterestId: String) { - val workInputData = createInputData(locationOfInterestId) + fun enqueueSyncWorker() { val request = WorkRequestBuilder() .setWorkerClass(MediaUploadWorker::class.java) .setNetworkType(preferredNetworkType()) - .buildWorkerRequest(workInputData) + .buildWorkerRequest() workManager.enqueueUniqueWork( MediaUploadWorker::class.java.name, ExistingWorkPolicy.APPEND_OR_REPLACE, diff --git a/ground/src/main/java/com/google/android/ground/persistence/sync/MediaUploadWorker.kt b/ground/src/main/java/com/google/android/ground/persistence/sync/MediaUploadWorker.kt index e3f50bd5f8..1db783ffe0 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/sync/MediaUploadWorker.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/sync/MediaUploadWorker.kt @@ -18,17 +18,15 @@ package com.google.android.ground.persistence.sync import android.content.Context import androidx.hilt.work.HiltWorker import androidx.work.CoroutineWorker -import androidx.work.Data +import androidx.work.ListenableWorker.Result.retry +import androidx.work.ListenableWorker.Result.success import androidx.work.WorkerParameters -import com.google.android.ground.Config -import com.google.android.ground.model.mutation.Mutation import com.google.android.ground.model.mutation.SubmissionMutation import com.google.android.ground.model.submission.PhotoTaskData -import com.google.android.ground.persistence.local.room.fields.MutationEntitySyncStatus import com.google.android.ground.persistence.remote.RemoteStorageManager import com.google.android.ground.repository.MutationRepository import com.google.android.ground.repository.UserMediaRepository -import com.google.firebase.FirebaseNetworkException +import com.google.android.ground.util.priority import dagger.assisted.Assisted import dagger.assisted.AssistedInject import java.io.FileNotFoundException @@ -56,75 +54,38 @@ constructor( private val userMediaRepository: UserMediaRepository, ) : CoroutineWorker(context, workerParams) { - private val loiId: String = workerParams.inputData.getString(LOI_ID) ?: "" - - override suspend fun doWork(): Result = withContext(Dispatchers.IO) { doWorkInternal() } - - /** Performs media uploads for all eligible submission mutations associated with a given LOI. */ - private suspend fun doWorkInternal(): Result { - check(loiId.isNotEmpty()) { "work was queued for an empty location of interest ID" } - Timber.d("Starting media upload for LOI: $loiId") - - val mutations = - mutationRepository.getSubmissionMutations( - loiId, - MutationEntitySyncStatus.MEDIA_UPLOAD_PENDING, - MutationEntitySyncStatus.MEDIA_UPLOAD_IN_PROGRESS, - MutationEntitySyncStatus.MEDIA_UPLOAD_AWAITING_RETRY, - ) - val results = uploadMedia(mutations) - return if (results.any { it.mediaUploadPending() }) Result.retry() else Result.success() - } - - /** - * Attempts to upload associated media for a given list of [SubmissionMutation] and updates the - * status of each mutation based on whether uploads succeeded or failed. - */ - private suspend fun uploadMedia(mutations: List): List = - mutations - .map { mutation -> - mutation - .updateSyncStatus(Mutation.SyncStatus.MEDIA_UPLOAD_IN_PROGRESS) - .incrementRetryCount() - } - .also { mutationRepository.saveMutationsLocally(it) } - .map { mutation -> uploadMedia(mutation) } - .also { mutationRepository.saveMutationsLocally(it) } + override suspend fun doWork(): Result = + withContext(Dispatchers.IO) { + val mutations = mutationRepository.getIncompleteMediaMutations() + Timber.d("Uploading photos for ${mutations.size} submission mutations") + val results = mutations.map { uploadAllMedia(it) } + if (results.all { it }) success() else retry() + } /** - * Attempts to upload all media associated with a given submission. Updates the submission's sync - * status depending on whether or the uploads failed or succeeded. + * Upload all media associated with a given submission. Returns `true` if all uploads succeeds or + * if there is nothing to upload, `false` otherwise. */ - private suspend fun uploadMedia(mutation: SubmissionMutation): SubmissionMutation { + private suspend fun uploadAllMedia(mutation: SubmissionMutation): Boolean { + mutationRepository.markAsMediaUploadInProgress(listOf(mutation)) val photoTasks = mutation.getPhotoData() - return uploadPhotos(photoTasks) - .fold( - onSuccess = { mutation.updateSyncStatus(Mutation.SyncStatus.COMPLETED) }, - onFailure = { - return mutation.copy( - syncStatus = - if (it is FileNotFoundException || !mutation.canRetry()) { - Mutation.SyncStatus.FAILED - } else { - Mutation.SyncStatus.MEDIA_UPLOAD_AWAITING_RETRY - }, - lastError = it.message ?: "unknown upload error", - ) - }, + val results = photoTasks.map { uploadPhotoMedia(it) } + if (results.all { it.isSuccess }) { + mutationRepository.markAsComplete(listOf(mutation)) + return true + } else { + mutationRepository.markAsFailedMediaUpload( + listOf(mutation), + // TODO(https://github.com/google/ground-android/issues/2120): Replace this workaround with + // update of specific [MediaMutation], aggregate to [UploadQueueEntry] for display in UI. + results.firstNotNullOfOrNull { it.exceptionOrNull() } ?: UnknownError(), ) + return false + } } - private suspend fun uploadPhotos(photoTaskDataList: List): kotlin.Result = - // TODO(#2120): Retry uploads on a per-photo basis, instead of per-response. - photoTaskDataList - .map { uploadPhotoMedia(it) } - .fold(kotlin.Result.success(Unit)) { a, b -> if (a.isSuccess) b else a } - - /** - * Attempts to upload a single photo to remote storage. Returns an [Result] indicating whether the - * upload attempt failed or succeeded. - */ - private suspend fun uploadPhotoMedia(photoTaskData: PhotoTaskData): kotlin.Result = + /** Attempts to upload a single photo to remote storage. */ + private suspend fun uploadPhotoMedia(photoTaskData: PhotoTaskData): kotlin.Result { try { val path = photoTaskData.remoteFilename val photoFile = userMediaRepository.getLocalFileFromRemotePath(path) @@ -133,23 +94,10 @@ constructor( throw FileNotFoundException(photoFile.path) } remoteStorageManager.uploadMediaFromFile(photoFile, path) - kotlin.Result.success(Unit) - } catch (t: FirebaseNetworkException) { - Timber.d(t, "Can't connect to Firebase to upload photo") - kotlin.Result.failure(t) + return kotlin.Result.success(Unit) } catch (t: Throwable) { - Timber.e(t, "Failed to upload photo") - kotlin.Result.failure(t) + Timber.log(t.priority(), t, "Photo upload failed") + return kotlin.Result.failure(t) } - - companion object { - private const val LOI_ID = "locationOfInterestId" - - @JvmStatic - fun createInputData(locationOfInterestId: String): Data = - Data.Builder().putString(LOI_ID, locationOfInterestId).build() - - private fun SubmissionMutation.canRetry() = - this.retryCount < Config.MAX_MEDIA_UPLOAD_RETRY_COUNT } } diff --git a/ground/src/main/java/com/google/android/ground/persistence/sync/MutationSyncWorkManager.kt b/ground/src/main/java/com/google/android/ground/persistence/sync/MutationSyncWorkManager.kt index b36d9afeb0..ab429c0f75 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/sync/MutationSyncWorkManager.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/sync/MutationSyncWorkManager.kt @@ -17,7 +17,6 @@ package com.google.android.ground.persistence.sync import androidx.work.ExistingWorkPolicy import androidx.work.WorkManager -import com.google.android.ground.persistence.sync.LocalMutationSyncWorker.Companion.createInputData import javax.inject.Inject /** Enqueues data sync work to be done in the background. */ @@ -27,18 +26,14 @@ class MutationSyncWorkManager @Inject constructor(private val workManager: WorkM * Enqueues a worker that sends changes made locally to the remote data store once a network * connection is available. The returned `Completable` completes immediately as soon as the worker * is added to the work queue (not once the sync job completes). + * + * Rather than having the running worker monitor the queue for new mutations, we instead queue the + * worker again on each new mutation. This simplifies the worker implementation and avoids race + * conditions in the rare event the worker finishes just when new mutations are added to the db. */ - fun enqueueSyncWorker(locationOfInterestId: String) { - // Rather than having running workers monitor the queue for new mutations for their respective - // locationOfInterestId, we instead queue a new worker on each new mutation. This simplifies the - // worker - // implementation and avoids race conditions in the rare event the worker finishes just when new - // mutations are added to the db. - val inputData = createInputData(locationOfInterestId) + fun enqueueSyncWorker() { val request = - WorkRequestBuilder() - .setWorkerClass(LocalMutationSyncWorker::class.java) - .buildWorkerRequest(inputData) + WorkRequestBuilder().setWorkerClass(LocalMutationSyncWorker::class.java).buildWorkerRequest() workManager.enqueueUniqueWork( LocalMutationSyncWorker::class.java.name, ExistingWorkPolicy.APPEND, 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 9249fea434..f8bed98c5c 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 @@ -136,7 +136,7 @@ constructor( */ suspend fun applyAndEnqueue(mutation: LocationOfInterestMutation) { localLoiStore.applyAndEnqueue(mutation) - mutationSyncWorkManager.enqueueSyncWorker(mutation.locationOfInterestId) + mutationSyncWorkManager.enqueueSyncWorker() } /** Returns a flow of all valid (not deleted) [LocationOfInterest] in the given [Survey]. */ diff --git a/ground/src/main/java/com/google/android/ground/repository/MutationRepository.kt b/ground/src/main/java/com/google/android/ground/repository/MutationRepository.kt index 97050e97aa..117b95b749 100644 --- a/ground/src/main/java/com/google/android/ground/repository/MutationRepository.kt +++ b/ground/src/main/java/com/google/android/ground/repository/MutationRepository.kt @@ -17,24 +17,25 @@ package com.google.android.ground.repository import com.google.android.ground.model.Survey +import com.google.android.ground.model.User import com.google.android.ground.model.mutation.LocationOfInterestMutation import com.google.android.ground.model.mutation.Mutation import com.google.android.ground.model.mutation.Mutation.SyncStatus +import com.google.android.ground.model.mutation.Mutation.SyncStatus.* import com.google.android.ground.model.mutation.Mutation.SyncStatus.FAILED import com.google.android.ground.model.mutation.Mutation.SyncStatus.IN_PROGRESS import com.google.android.ground.model.mutation.Mutation.SyncStatus.MEDIA_UPLOAD_PENDING import com.google.android.ground.model.mutation.SubmissionMutation -import com.google.android.ground.persistence.local.room.converter.toModelObject -import com.google.android.ground.persistence.local.room.entity.LocationOfInterestMutationEntity -import com.google.android.ground.persistence.local.room.entity.SubmissionMutationEntity -import com.google.android.ground.persistence.local.room.fields.MutationEntitySyncStatus +import com.google.android.ground.model.submission.UploadQueueEntry import com.google.android.ground.persistence.local.stores.LocalLocationOfInterestStore import com.google.android.ground.persistence.local.stores.LocalSubmissionStore -import com.google.android.ground.persistence.local.stores.LocalSurveyStore +import com.google.android.ground.system.auth.AuthenticationManager import javax.inject.Inject import javax.inject.Singleton import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.first +import timber.log.Timber /** * Coordinates persistence of mutations across [LocationOfInterestMutation] and [SubmissionMutation] @@ -44,7 +45,7 @@ import kotlinx.coroutines.flow.combine class MutationRepository @Inject constructor( - private val localSurveyStore: LocalSurveyStore, + private val authenticationManager: AuthenticationManager, private val localLocationOfInterestStore: LocalLocationOfInterestStore, private val localSubmissionStore: LocalSubmissionStore, ) { @@ -54,59 +55,85 @@ constructor( */ fun getSurveyMutationsFlow(survey: Survey): Flow> { // TODO(https://github.com/google/ground-android/issues/2838): Show mutations for all surveys, - // not just current one. + // not just current one. + // TODO(https://github.com/google/ground-android/issues/2838): This method is also named + // incorrectly - it only returns one of LOI or submission mutations. We should delete this + // method in favor of [getUploadQueueFlow()]. val locationOfInterestMutations = localLocationOfInterestStore.getAllSurveyMutations(survey) val submissionMutations = localSubmissionStore.getAllSurveyMutationsFlow(survey) return locationOfInterestMutations.combine(submissionMutations, this::combineAndSortMutations) } - fun getAllMutationsFlow(): Flow> { - val locationOfInterestMutations = localLocationOfInterestStore.getAllMutationsFlow() - val submissionMutations = localSubmissionStore.getAllMutationsFlow() - - return locationOfInterestMutations.combine(submissionMutations, this::combineAndSortMutations) - } + /** + * Return the set of data upload queue entries not yet marked as completed sorted in chronological + * order (FIFO). Media/photo uploads are not included. + */ + suspend fun getIncompleteUploads(): List = + getUploadQueueFlow().first().filter { + setOf(PENDING, IN_PROGRESS, FAILED, UNKNOWN).contains(it.uploadStatus) + } /** - * Returns all local submission mutations associated with the the given LOI ID that have one of - * the provided sync statues. + * Return the set of photo/media upload queue entries not yet marked as completed, sorted in + * chronological order (FIFO). */ - suspend fun getSubmissionMutations( - loiId: String, - vararg entitySyncStatus: MutationEntitySyncStatus, - ) = getMutations(loiId, *entitySyncStatus).filterIsInstance() + suspend fun getIncompleteMediaMutations(): List = + getUploadQueueFlow() + .first() + .filter { + setOf(MEDIA_UPLOAD_PENDING, MEDIA_UPLOAD_IN_PROGRESS, MEDIA_UPLOAD_AWAITING_RETRY) + .contains(it.uploadStatus) + } + // TODO(https://github.com/google/ground-android/issues/2120): + // Return [MediaMutations] instead once introduced. + .mapNotNull { it.submissionMutation } /** - * Returns all LOI and submission mutations in the local mutation queue relating to LOI with the - * specified id, sorted by creation timestamp (oldest first). + * Returns a [Flow] which emits the upload queue once and on each change, sorted in chronological + * order (FIFO). */ - suspend fun getMutations( - loiId: String, - vararg entitySyncStatus: MutationEntitySyncStatus, - ): List { - val loiMutations = - localLocationOfInterestStore - .findByLocationOfInterestId(loiId, *entitySyncStatus) - .map(LocationOfInterestMutationEntity::toModelObject) - val submissionMutations = - localSubmissionStore.findByLocationOfInterestId(loiId, *entitySyncStatus).map { - it.toSubmissionMutation() + private fun getUploadQueueFlow(): Flow> = + localLocationOfInterestStore.getAllMutationsFlow().combine( + localSubmissionStore.getAllMutationsFlow() + ) { loiMutations, submissionMutations -> + buildUploadQueue(loiMutations, submissionMutations) + } + + private suspend fun buildUploadQueue( + loiMutations: List, + submissionMutations: List, + ): List { + val user = authenticationManager.getAuthenticatedUser() + val loiMutationMap = loiMutations.filterByUser(user).associateBy { it.collectionId } + val submissionMutationMap = + submissionMutations.filterByUser(user).associateBy { it.collectionId } + val collectionIds: Set = loiMutationMap.keys + submissionMutationMap.keys + return collectionIds + .map { + val loiMutation = loiMutationMap[it] + val submissionMutation = submissionMutationMap[it] + val userId = submissionMutation?.userId ?: loiMutation!!.userId + val clientTimestamp = submissionMutation?.clientTimestamp ?: loiMutation!!.clientTimestamp + val syncStatus = submissionMutation?.syncStatus ?: loiMutation!!.syncStatus + UploadQueueEntry(userId, clientTimestamp, syncStatus, loiMutation, submissionMutation) } - return (loiMutations + submissionMutations).sortedBy { it.clientTimestamp } + .sortedBy { it.clientTimestamp } } - private suspend fun SubmissionMutationEntity.toSubmissionMutation(): SubmissionMutation = - toModelObject( - localSurveyStore.getSurveyById(surveyId) - ?: error("Survey missing $surveyId. Unable to fetch pending submission mutations.") - ) + private fun List.filterByUser(user: User): List { + val (validMutations, invalidMutations) = partition { it.userId == user.id } + if (invalidMutations.isNotEmpty()) { + Timber.e("Mutation(s) not deleted on sign-out") + } + return validMutations + } /** * Saves the provided list of mutations to local storage. Updates any locally stored, existing - * mutations to reflect the mutations in the list, and writes any new mutations. + * mutations to reflect the mutations in the list, creating new mutations as needed. */ - suspend fun saveMutationsLocally(mutations: List) { + private suspend fun saveMutationsLocally(mutations: List) { val loiMutations = mutations.filterIsInstance() localLocationOfInterestStore.updateAll(loiMutations) @@ -120,6 +147,8 @@ constructor( */ suspend fun finalizePendingMutationsForMediaUpload(mutations: List) { finalizeDeletions(mutations) + // TODO(https://github.com/google/ground-android/issues/2873): Only do this is there are + // actually photos to upload. markForMediaUpload(mutations) } @@ -141,6 +170,14 @@ constructor( saveMutationsLocally(mutations.updateMutationStatus(IN_PROGRESS)) } + suspend fun markAsMediaUploadInProgress(mutations: List) { + saveMutationsLocally(mutations.updateMutationStatus(MEDIA_UPLOAD_IN_PROGRESS)) + } + + suspend fun markAsComplete(mutations: List) { + saveMutationsLocally(mutations.updateMutationStatus(COMPLETED)) + } + suspend fun markAsFailed(mutations: List, error: Throwable) { saveMutationsLocally(mutations.updateMutationStatus(FAILED, error)) } @@ -149,6 +186,10 @@ constructor( saveMutationsLocally(mutations.updateMutationStatus(MEDIA_UPLOAD_PENDING)) } + suspend fun markAsFailedMediaUpload(mutations: List, error: Throwable) { + saveMutationsLocally(mutations.updateMutationStatus(MEDIA_UPLOAD_AWAITING_RETRY, error)) + } + private fun combineAndSortMutations( locationOfInterestMutations: List, submissionMutations: List, @@ -163,7 +204,7 @@ private fun List.updateMutationStatus( syncStatus: SyncStatus, error: Throwable? = null, ): List = map { - val hasSyncFailed = syncStatus == FAILED + val hasSyncFailed = syncStatus == FAILED || syncStatus == MEDIA_UPLOAD_AWAITING_RETRY val retryCount = if (hasSyncFailed) it.retryCount + 1 else it.retryCount val errorMessage = if (hasSyncFailed) error?.message ?: error.toString() else it.lastError diff --git a/ground/src/main/java/com/google/android/ground/repository/SubmissionRepository.kt b/ground/src/main/java/com/google/android/ground/repository/SubmissionRepository.kt index c348dca40b..89e5179436 100644 --- a/ground/src/main/java/com/google/android/ground/repository/SubmissionRepository.kt +++ b/ground/src/main/java/com/google/android/ground/repository/SubmissionRepository.kt @@ -97,7 +97,7 @@ constructor( private suspend fun applyAndEnqueue(mutation: SubmissionMutation) { localSubmissionStore.applyAndEnqueue(mutation) - mutationSyncWorkManager.enqueueSyncWorker(mutation.locationOfInterestId) + mutationSyncWorkManager.enqueueSyncWorker() } suspend fun getTotalSubmissionCount(loi: LocationOfInterest) = diff --git a/ground/src/main/java/com/google/android/ground/ui/home/HomeScreenViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/home/HomeScreenViewModel.kt index 89de4ed2aa..f782393b6e 100644 --- a/ground/src/main/java/com/google/android/ground/ui/home/HomeScreenViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/home/HomeScreenViewModel.kt @@ -18,7 +18,6 @@ package com.google.android.ground.ui.home import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData import androidx.lifecycle.viewModelScope -import com.google.android.ground.model.mutation.Mutation.SyncStatus.COMPLETED import com.google.android.ground.model.submission.DraftSubmission import com.google.android.ground.persistence.local.LocalValueStore import com.google.android.ground.persistence.sync.MediaUploadWorkManager @@ -71,12 +70,11 @@ internal constructor( * no-op. Workaround for https://github.com/google/ground-android/issues/2751. */ private suspend fun kickLocalMutationSyncWorkers() { - val mutations = mutationRepository.getAllMutationsFlow().first() - val incompleteLoiIds = - mutations.filter { it.syncStatus != COMPLETED }.map { it.locationOfInterestId }.toSet() - incompleteLoiIds.forEach { loiId -> - mutationSyncWorkManager.enqueueSyncWorker(loiId) - mediaUploadWorkManager.enqueueSyncWorker(loiId) + if (mutationRepository.getIncompleteUploads().isNotEmpty()) { + mutationSyncWorkManager.enqueueSyncWorker() + } + if (mutationRepository.getIncompleteMediaMutations().isNotEmpty()) { + mediaUploadWorkManager.enqueueSyncWorker() } } diff --git a/ground/src/main/java/com/google/android/ground/util/Debug.kt b/ground/src/main/java/com/google/android/ground/util/Debug.kt index fd6fd49e28..c2f44f8132 100644 --- a/ground/src/main/java/com/google/android/ground/util/Debug.kt +++ b/ground/src/main/java/com/google/android/ground/util/Debug.kt @@ -15,9 +15,11 @@ */ package com.google.android.ground.util +import android.util.Log import androidx.fragment.app.Fragment import androidx.fragment.app.FragmentActivity import com.google.android.ground.FirebaseCrashLogger +import com.google.firebase.FirebaseNetworkException import timber.log.Timber object Debug { @@ -40,3 +42,5 @@ object Debug { null } } + +fun Throwable.priority() = if (this is FirebaseNetworkException) Log.DEBUG else Log.ERROR diff --git a/ground/src/test/java/com/google/android/ground/persistence/sync/LocalMutationSyncWorkerTest.kt b/ground/src/test/java/com/google/android/ground/persistence/sync/LocalMutationSyncWorkerTest.kt index 1f3a8b06c1..f58836172e 100644 --- a/ground/src/test/java/com/google/android/ground/persistence/sync/LocalMutationSyncWorkerTest.kt +++ b/ground/src/test/java/com/google/android/ground/persistence/sync/LocalMutationSyncWorkerTest.kt @@ -23,22 +23,16 @@ import androidx.work.ListenableWorker.Result.success import androidx.work.WorkerFactory import androidx.work.WorkerParameters import androidx.work.testing.TestListenableWorkerBuilder -import androidx.work.workDataOf import com.google.android.ground.BaseHiltTest import com.google.android.ground.model.geometry.Point import com.google.android.ground.model.mutation.LocationOfInterestMutation import com.google.android.ground.model.mutation.Mutation +import com.google.android.ground.model.mutation.Mutation.SyncStatus.* import com.google.android.ground.model.mutation.SubmissionMutation -import com.google.android.ground.persistence.local.room.fields.MutationEntitySyncStatus.FAILED -import com.google.android.ground.persistence.local.room.fields.MutationEntitySyncStatus.IN_PROGRESS -import com.google.android.ground.persistence.local.room.fields.MutationEntitySyncStatus.MEDIA_UPLOAD_PENDING -import com.google.android.ground.persistence.local.room.fields.MutationEntitySyncStatus.PENDING -import com.google.android.ground.persistence.local.room.fields.MutationEntitySyncStatus.UNKNOWN import com.google.android.ground.persistence.local.stores.LocalLocationOfInterestStore import com.google.android.ground.persistence.local.stores.LocalSubmissionStore import com.google.android.ground.persistence.local.stores.LocalSurveyStore import com.google.android.ground.persistence.local.stores.LocalUserStore -import com.google.android.ground.persistence.sync.LocalMutationSyncWorker.Companion.LOCATION_OF_INTEREST_ID_PARAM_KEY import com.google.android.ground.repository.MutationRepository import com.google.android.ground.repository.UserRepository import com.google.common.truth.Truth.assertThat @@ -48,8 +42,8 @@ import com.sharedtest.persistence.remote.FakeRemoteDataStore import com.sharedtest.system.auth.FakeAuthenticationManager import dagger.hilt.android.testing.HiltAndroidTest import javax.inject.Inject +import kotlinx.coroutines.flow.first import kotlinx.coroutines.runBlocking -import org.junit.Assert.assertThrows import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -97,6 +91,8 @@ class LocalMutationSyncWorkerTest : BaseHiltTest() { ) } + private val testSurvey = FakeData.SURVEY.copy(id = TEST_SURVEY_ID) + @Before override fun setUp() { super.setUp() @@ -104,24 +100,17 @@ class LocalMutationSyncWorkerTest : BaseHiltTest() { runBlocking { fakeAuthenticationManager.setUser(FakeData.USER.copy(id = TEST_USER_ID)) localUserStore.insertOrUpdateUser(FakeData.USER.copy(id = TEST_USER_ID)) - localSurveyStore.insertOrUpdateSurvey(FakeData.SURVEY.copy(id = TEST_SURVEY_ID)) + localSurveyStore.insertOrUpdateSurvey(testSurvey) } } @Test - fun `Throws an NPE if LOI ID is null`() { - assertThrows(NullPointerException::class.java) { - runWithTestDispatcher { createAndDoWork(context, null) } - } - } - - @Test - fun `Ignores all mutations if the logged-in user is mismatching`() = runWithTestDispatcher { + fun `Ignores mutations not belonging to current user`() = runWithTestDispatcher { fakeAuthenticationManager.setUser(FakeData.USER.copy(id = "random user id")) addPendingMutations() - val result = createAndDoWork(context, TEST_LOI_ID) + val result = createAndDoWork(context) assertThat(result).isEqualTo(success()) assertMutationsState(pending = 2) @@ -135,7 +124,7 @@ class LocalMutationSyncWorkerTest : BaseHiltTest() { localUserStore.insertOrUpdateUser(FakeData.USER.copy(id = TEST_USER_ID)) localSubmissionStore.applyAndEnqueue(createSubmissionMutation(TEST_USER_ID)) - val result = createAndDoWork(context, TEST_LOI_ID) + val result = createAndDoWork(context) assertThat(result).isEqualTo(success()) assertMutationsState(pending = 1, complete = 1) @@ -143,7 +132,7 @@ class LocalMutationSyncWorkerTest : BaseHiltTest() { @Test fun `Succeeds if there are 0 pending mutations`() = runWithTestDispatcher { - val result = createAndDoWork(context, TEST_LOI_ID) + val result = createAndDoWork(context) assertThat(result).isEqualTo(success()) } @@ -152,7 +141,7 @@ class LocalMutationSyncWorkerTest : BaseHiltTest() { fun `Succeeds if there are non-zero pending mutations`() = runWithTestDispatcher { addPendingMutations() - val result = createAndDoWork(context, TEST_LOI_ID) + val result = createAndDoWork(context) assertThat(result).isEqualTo(success()) assertMutationsState(complete = 2) @@ -164,7 +153,7 @@ class LocalMutationSyncWorkerTest : BaseHiltTest() { fakeRemoteDataStore.applyMutationError = Error(ERROR_MESSAGE) addPendingMutations() - val result = createAndDoWork(context, TEST_LOI_ID) + val result = createAndDoWork(context) assertThat(result).isEqualTo(retry()) assertMutationsState( @@ -179,7 +168,7 @@ class LocalMutationSyncWorkerTest : BaseHiltTest() { fakeRemoteDataStore.applyMutationError = Error(ERROR_MESSAGE) addPendingMutations() - var result = createAndDoWork(context, TEST_LOI_ID) + var result = createAndDoWork(context) assertThat(result).isEqualTo(retry()) assertMutationsState( failed = 2, @@ -189,7 +178,7 @@ class LocalMutationSyncWorkerTest : BaseHiltTest() { for (i in 1..10) { // Worker should retry N times. - result = createAndDoWork(context, TEST_LOI_ID) + result = createAndDoWork(context) assertThat(result).isEqualTo(retry()) // Verify that the retryCount has incremented. assertMutationsState( @@ -200,6 +189,11 @@ class LocalMutationSyncWorkerTest : BaseHiltTest() { } } + private suspend fun getMutations(syncStatus: Mutation.SyncStatus): List = + (localLocationOfInterestStore.getAllMutationsFlow().first() + + localSubmissionStore.getAllMutationsFlow().first()) + .filter { it.syncStatus == syncStatus } + private suspend fun assertMutationsState( pending: Int = 0, inProgress: Int = 0, @@ -208,20 +202,18 @@ class LocalMutationSyncWorkerTest : BaseHiltTest() { retryCount: List = listOf(), lastErrors: List = listOf(), ) { - assertWithMessage("Unknown mutations count incorrect") - .that(mutationRepository.getMutations(TEST_LOI_ID, UNKNOWN)) - .hasSize(0) + assertWithMessage("Unknown mutations count incorrect").that(getMutations(UNKNOWN)).hasSize(0) assertWithMessage("Pending mutations count incorrect") - .that(mutationRepository.getMutations(TEST_LOI_ID, PENDING)) + .that(getMutations(PENDING)) .hasSize(pending) assertWithMessage("In Progress mutations count incorrect") - .that(mutationRepository.getMutations(TEST_LOI_ID, IN_PROGRESS)) + .that(getMutations(IN_PROGRESS)) .hasSize(inProgress) assertWithMessage("Completed mutations count incorrect") - .that(mutationRepository.getMutations(TEST_LOI_ID, MEDIA_UPLOAD_PENDING)) + .that(getMutations(MEDIA_UPLOAD_PENDING)) .hasSize(complete) - val failedMutations = mutationRepository.getMutations(TEST_LOI_ID, FAILED) + val failedMutations = getMutations(FAILED) assertWithMessage("Failed mutations count incorrect").that(failedMutations).hasSize(failed) assertThat(failedMutations.map { it.retryCount.toInt() }).containsExactlyElementsIn(retryCount) assertThat(failedMutations.map { it.lastError }).containsExactlyElementsIn(lastErrors) @@ -254,11 +246,8 @@ class LocalMutationSyncWorkerTest : BaseHiltTest() { collectionId = "collectionId", ) - private suspend fun createAndDoWork(context: Context, loiId: String?): ListenableWorker.Result = - TestListenableWorkerBuilder( - context, - inputData = workDataOf(Pair(LOCATION_OF_INTEREST_ID_PARAM_KEY, loiId)), - ) + private suspend fun createAndDoWork(context: Context): ListenableWorker.Result = + TestListenableWorkerBuilder(context) .setWorkerFactory(factory) .build() .doWork() diff --git a/ground/src/test/java/com/google/android/ground/persistence/sync/MediaUploadWorkerTest.kt b/ground/src/test/java/com/google/android/ground/persistence/sync/MediaUploadWorkerTest.kt index 15df16a234..4e4a11087f 100644 --- a/ground/src/test/java/com/google/android/ground/persistence/sync/MediaUploadWorkerTest.kt +++ b/ground/src/test/java/com/google/android/ground/persistence/sync/MediaUploadWorkerTest.kt @@ -24,18 +24,19 @@ import androidx.work.WorkerParameters import androidx.work.testing.TestListenableWorkerBuilder import com.google.android.ground.BaseHiltTest import com.google.android.ground.model.mutation.Mutation +import com.google.android.ground.model.mutation.Mutation.SyncStatus.* import com.google.android.ground.model.mutation.SubmissionMutation import com.google.android.ground.model.submission.PhotoTaskData import com.google.android.ground.model.submission.ValueDelta import com.google.android.ground.model.task.Task -import com.google.android.ground.persistence.local.room.fields.MutationEntitySyncStatus +import com.google.android.ground.model.task.Task.Type.PHOTO import com.google.android.ground.persistence.local.stores.LocalLocationOfInterestStore import com.google.android.ground.persistence.local.stores.LocalSubmissionStore import com.google.android.ground.persistence.local.stores.LocalSurveyStore import com.google.android.ground.persistence.local.stores.LocalUserStore import com.google.android.ground.repository.MutationRepository import com.google.android.ground.repository.UserMediaRepository -import com.google.common.truth.Truth.assertThat +import com.google.common.truth.Truth.assertWithMessage import com.google.firebase.crashlytics.FirebaseCrashlytics import com.sharedtest.FakeData import com.sharedtest.persistence.remote.FakeRemoteDataStore @@ -43,7 +44,7 @@ import com.sharedtest.persistence.remote.FakeRemoteStorageManager import dagger.hilt.android.testing.BindValue import dagger.hilt.android.testing.HiltAndroidTest import javax.inject.Inject -import org.junit.Assert.assertThrows +import kotlinx.coroutines.flow.first import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -86,23 +87,16 @@ class MediaUploadWorkerTest : BaseHiltTest() { context = ApplicationProvider.getApplicationContext() } - @Test - fun doWork_throwsOnEmptyLoiId() { - assertThrows(IllegalStateException::class.java) { - runWithTestDispatcher { createAndDoWork(context, "") } - } - } - @Test fun doWork_succeedsOnExistingPhoto() = runWithTestDispatcher { localUserStore.insertOrUpdateUser(FakeData.USER) localSurveyStore.insertOrUpdateSurvey(TEST_SURVEY) localLocationOfInterestStore.insertOrUpdate(TEST_LOI) localSubmissionStore.applyAndEnqueue( - createSubmissionMutation().copy(syncStatus = Mutation.SyncStatus.MEDIA_UPLOAD_PENDING) + createSubmissionMutation().copy(syncStatus = MEDIA_UPLOAD_PENDING) ) - createAndDoWork(context, FakeData.LOCATION_OF_INTEREST.id) - assertThatMutationCountEquals(MutationEntitySyncStatus.COMPLETED, 1) + createAndDoWork(context) + assertThatMutationCountEquals(COMPLETED, 1) } @Test @@ -111,11 +105,10 @@ class MediaUploadWorkerTest : BaseHiltTest() { localSurveyStore.insertOrUpdateSurvey(TEST_SURVEY) localLocationOfInterestStore.insertOrUpdate(TEST_LOI) localSubmissionStore.applyAndEnqueue( - createSubmissionMutation("does_not_exist.jpg") - .copy(syncStatus = Mutation.SyncStatus.MEDIA_UPLOAD_PENDING) + createSubmissionMutation("does_not_exist.jpg").copy(syncStatus = MEDIA_UPLOAD_PENDING) ) - createAndDoWork(context, FakeData.LOCATION_OF_INTEREST.id) - assertThatMutationCountEquals(MutationEntitySyncStatus.FAILED, 1) + createAndDoWork(context) + assertThatMutationCountEquals(MEDIA_UPLOAD_AWAITING_RETRY, 1) } @Test @@ -123,12 +116,12 @@ class MediaUploadWorkerTest : BaseHiltTest() { val mutation = createSubmissionMutation() // a valid mutation val delta = buildList { addAll(mutation.deltas) - add(ValueDelta(PHOTO_TASK_ID, Task.Type.PHOTO, PhotoTaskData("some/path/does_not_exist.jpg"))) + add(ValueDelta(PHOTO_TASK_ID, PHOTO, PhotoTaskData("some/path/does_not_exist.jpg"))) } val updatedMutation = mutation.copy( deltas = delta, - syncStatus = Mutation.SyncStatus.MEDIA_UPLOAD_PENDING, + syncStatus = MEDIA_UPLOAD_PENDING, ) // add an additional non-existent photo to the mutation localUserStore.insertOrUpdateUser(FakeData.USER) @@ -136,11 +129,12 @@ class MediaUploadWorkerTest : BaseHiltTest() { localLocationOfInterestStore.insertOrUpdate(TEST_LOI) localSubmissionStore.applyAndEnqueue(updatedMutation) - createAndDoWork(context, FakeData.LOCATION_OF_INTEREST.id) - assertThatMutationCountEquals(MutationEntitySyncStatus.FAILED, 1) - assertThatMutationCountEquals(MutationEntitySyncStatus.MEDIA_UPLOAD_PENDING, 0) - assertThatMutationCountEquals(MutationEntitySyncStatus.MEDIA_UPLOAD_IN_PROGRESS, 0) - assertThatMutationCountEquals(MutationEntitySyncStatus.COMPLETED, 0) + createAndDoWork(context) + + assertThatMutationCountEquals(MEDIA_UPLOAD_AWAITING_RETRY, 1) + assertThatMutationCountEquals(MEDIA_UPLOAD_PENDING, 0) + assertThatMutationCountEquals(MEDIA_UPLOAD_IN_PROGRESS, 0) + assertThatMutationCountEquals(COMPLETED, 0) } @Test @@ -148,36 +142,43 @@ class MediaUploadWorkerTest : BaseHiltTest() { localUserStore.insertOrUpdateUser(FakeData.USER) localSurveyStore.insertOrUpdateSurvey(TEST_SURVEY) localLocationOfInterestStore.insertOrUpdate(TEST_LOI) - addSubmissionMutationToLocalStorage(Mutation.SyncStatus.PENDING) - addSubmissionMutationToLocalStorage(Mutation.SyncStatus.FAILED) - addSubmissionMutationToLocalStorage(Mutation.SyncStatus.IN_PROGRESS) - addSubmissionMutationToLocalStorage(Mutation.SyncStatus.COMPLETED) - addSubmissionMutationToLocalStorage(Mutation.SyncStatus.UNKNOWN) - - createAndDoWork(context, FakeData.LOCATION_OF_INTEREST.id) - assertThatMutationCountEquals(MutationEntitySyncStatus.FAILED, 1) - assertThatMutationCountEquals(MutationEntitySyncStatus.PENDING, 1) - assertThatMutationCountEquals(MutationEntitySyncStatus.COMPLETED, 1) - assertThatMutationCountEquals(MutationEntitySyncStatus.IN_PROGRESS, 1) - assertThatMutationCountEquals(MutationEntitySyncStatus.UNKNOWN, 1) - assertThatMutationCountEquals(MutationEntitySyncStatus.MEDIA_UPLOAD_PENDING, 0) - assertThatMutationCountEquals(MutationEntitySyncStatus.MEDIA_UPLOAD_IN_PROGRESS, 0) + addSubmissionMutationToLocalStorage(PENDING) + addSubmissionMutationToLocalStorage(FAILED) + addSubmissionMutationToLocalStorage(IN_PROGRESS) + addSubmissionMutationToLocalStorage(COMPLETED) + addSubmissionMutationToLocalStorage(UNKNOWN) + + createAndDoWork(context) + + assertThatMutationCountEquals(PENDING, 1) + assertThatMutationCountEquals(FAILED, 1) + assertThatMutationCountEquals(IN_PROGRESS, 1) + assertThatMutationCountEquals(COMPLETED, 1) + assertThatMutationCountEquals(UNKNOWN, 1) + assertThatMutationCountEquals(MEDIA_UPLOAD_AWAITING_RETRY, 0) + assertThatMutationCountEquals(MEDIA_UPLOAD_PENDING, 0) + assertThatMutationCountEquals(MEDIA_UPLOAD_IN_PROGRESS, 0) } // Initiates and runs the MediaUploadWorker - private suspend fun createAndDoWork(context: Context, loiId: String) { - TestListenableWorkerBuilder( - context, - MediaUploadWorker.createInputData(loiId), - ) + private suspend fun createAndDoWork(context: Context) { + TestListenableWorkerBuilder(context) .setWorkerFactory(factory) .build() .doWork() } - // Assert a given number of mutations of the specified status exist. - private suspend fun assertThatMutationCountEquals(status: MutationEntitySyncStatus, count: Int) { - assertThat(mutationRepository.getMutations(FakeData.LOCATION_OF_INTEREST.id, status)) + private suspend fun getMutations(syncStatus: Mutation.SyncStatus): List = + (localLocationOfInterestStore.getAllMutationsFlow().first() + + localSubmissionStore.getAllMutationsFlow().first()) + .filter { it.syncStatus == syncStatus } + + /** + * Asserts that the specified number of mutations with the specified status exist in the local db. + */ + private suspend fun assertThatMutationCountEquals(status: Mutation.SyncStatus, count: Int) { + assertWithMessage("Expect $count mutations with status $status") + .that(getMutations(status)) .hasSize(count) } @@ -193,8 +194,7 @@ class MediaUploadWorkerTest : BaseHiltTest() { return SUBMISSION_MUTATION.copy( job = TEST_JOB, - deltas = - listOf(ValueDelta(PHOTO_TASK_ID, Task.Type.PHOTO, PhotoTaskData(photoName ?: photo.name))), + deltas = listOf(ValueDelta(PHOTO_TASK_ID, PHOTO, PhotoTaskData(photoName ?: photo.name))), ) } @@ -202,13 +202,7 @@ class MediaUploadWorkerTest : BaseHiltTest() { companion object { private const val PHOTO_TASK_ID = "photo_task_id" private val TEST_PHOTO_TASK: Task = - Task( - id = "photo_task_id", - index = 1, - isRequired = true, - type = Task.Type.PHOTO, - label = "photo_task", - ) + Task(id = "photo_task_id", index = 1, isRequired = true, type = PHOTO, label = "photo_task") private val TEST_JOB = FakeData.JOB.copy(tasks = mapOf(PHOTO_TASK_ID to TEST_PHOTO_TASK)) @@ -218,16 +212,13 @@ class MediaUploadWorkerTest : BaseHiltTest() { val SUBMISSION_MUTATION = SubmissionMutation( type = Mutation.Type.CREATE, - syncStatus = Mutation.SyncStatus.PENDING, + syncStatus = PENDING, locationOfInterestId = FakeData.LOCATION_OF_INTEREST.id, userId = FakeData.USER.id, job = FakeData.JOB, surveyId = FakeData.SURVEY.id, collectionId = "collectionId", - deltas = - listOf( - ValueDelta(PHOTO_TASK_ID, Task.Type.PHOTO, PhotoTaskData("foo/$PHOTO_TASK_ID.jpg")) - ), + deltas = listOf(ValueDelta(PHOTO_TASK_ID, PHOTO, PhotoTaskData("foo/$PHOTO_TASK_ID.jpg"))), ) } } 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 17e9edff4c..034eea3066 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 @@ -38,7 +38,6 @@ import kotlinx.coroutines.test.advanceUntilIdle import org.junit.Before import org.junit.Test import org.junit.runner.RunWith -import org.mockito.ArgumentMatchers.anyString import org.mockito.Mock import org.mockito.Mockito.`when` import org.mockito.kotlin.times @@ -103,12 +102,12 @@ class LocationOfInterestRepositoryTest : BaseHiltTest() { fun testApplyAndEnqueue_enqueuesWorker() = runWithTestDispatcher { locationOfInterestRepository.applyAndEnqueue(mutation) - verify(mockWorkManager).enqueueSyncWorker(LOCATION_OF_INTEREST.id) + verify(mockWorkManager).enqueueSyncWorker() } @Test fun testApplyAndEnqueue_returnsErrorOnWorkerSyncFailure() = runWithTestDispatcher { - `when`(mockWorkManager.enqueueSyncWorker(anyString())).thenThrow(Error()) + `when`(mockWorkManager.enqueueSyncWorker()).thenThrow(Error()) assertFailsWith { locationOfInterestRepository.applyAndEnqueue( @@ -116,7 +115,7 @@ class LocationOfInterestRepositoryTest : BaseHiltTest() { ) } - verify(mockWorkManager, times(1)).enqueueSyncWorker(LOCATION_OF_INTEREST.id) + verify(mockWorkManager, times(1)).enqueueSyncWorker() } // TODO(#1373): Add tests for new LOI sync once implemented (create, update, delete, error).