From 94565c4b0d2525ed9411f4fe7edc453e70d32b0a Mon Sep 17 00:00:00 2001 From: Gino Miceli Date: Wed, 7 Aug 2024 17:10:46 -0400 Subject: [PATCH 1/5] Fix photo task data serialization --- .../ground/model/submission/PhotoTaskData.kt | 22 ++++--------------- .../room/converter/ValueJsonConverter.kt | 11 +++++----- .../firebase/protobuf/ModelToProtoExt.kt | 3 ++- .../sync/LocalMutationSyncWorker.kt | 1 + .../tasks/photo/PhotoTaskViewModel.kt | 8 ++++--- 5 files changed, 18 insertions(+), 27 deletions(-) diff --git a/ground/src/main/java/com/google/android/ground/model/submission/PhotoTaskData.kt b/ground/src/main/java/com/google/android/ground/model/submission/PhotoTaskData.kt index 1946595e1c..ff6998d446 100644 --- a/ground/src/main/java/com/google/android/ground/model/submission/PhotoTaskData.kt +++ b/ground/src/main/java/com/google/android/ground/model/submission/PhotoTaskData.kt @@ -21,24 +21,10 @@ import kotlinx.serialization.Serializable /** Represents a single photo associated with a given [LocationOfInterest] and [Job]. */ @Serializable -class PhotoTaskData : TaskData { - val surveyId: String - val path: String - val filename: String - get() = path.split("/").last() +class PhotoTaskData(val remoteFilename: String) : TaskData { + override fun getDetailsText(): String = remoteFilename - constructor(path: String, surveyId: String) { - val filePattern = Regex("^[a-zA-Z0-9._ -]+\\.(png|jpg)$") - val filename = path.split("/").last() - require(filename.matches(filePattern)) { "Invalid photo file name" } + override fun isEmpty(): Boolean = remoteFilename.isEmpty() - this.path = path - this.surveyId = surveyId - } - - override fun getDetailsText(): String = path - - override fun isEmpty(): Boolean = path.isEmpty() - - override fun toString(): String = path + override fun toString(): String = remoteFilename } diff --git a/ground/src/main/java/com/google/android/ground/persistence/local/room/converter/ValueJsonConverter.kt b/ground/src/main/java/com/google/android/ground/persistence/local/room/converter/ValueJsonConverter.kt index 7dbb1ed264..24b71efe1c 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/local/room/converter/ValueJsonConverter.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/local/room/converter/ValueJsonConverter.kt @@ -30,7 +30,6 @@ import com.google.android.ground.model.submission.TextTaskData import com.google.android.ground.model.submission.TimeTaskData import com.google.android.ground.model.task.Task import com.google.android.ground.persistence.remote.DataStoreException -import com.google.android.ground.persistence.remote.firebase.FirebaseStorageManager import com.google.android.ground.persistence.remote.firebase.schema.CaptureLocationResultConverter.ACCURACY_KEY import com.google.android.ground.persistence.remote.firebase.schema.CaptureLocationResultConverter.ALTITUDE_KEY import com.google.android.ground.persistence.remote.firebase.schema.CaptureLocationResultConverter.GEOMETRY_KEY @@ -56,8 +55,7 @@ internal object ValueJsonConverter { is NumberTaskData -> taskData.value is DateTaskData -> dateToIsoString(taskData.date) is TimeTaskData -> dateToIsoString(taskData.time) - is PhotoTaskData -> - FirebaseStorageManager.getRemoteMediaPath(taskData.surveyId, taskData.filename) + is PhotoTaskData -> taskData.remoteFilename is DrawAreaTaskData -> GeometryWrapperTypeConverter.toString(taskData.geometry) is DropPinTaskData -> GeometryWrapperTypeConverter.toString(taskData.geometry) is CaptureLocationTaskData -> @@ -89,11 +87,14 @@ internal object ValueJsonConverter { return null } return when (task.type) { - Task.Type.TEXT, - Task.Type.PHOTO -> { + Task.Type.TEXT -> { DataStoreException.checkType(String::class.java, obj) TextTaskData.fromString(obj as String) } + Task.Type.PHOTO -> { + DataStoreException.checkType(String::class.java, obj) + PhotoTaskData(obj as String) + } Task.Type.MULTIPLE_CHOICE -> { DataStoreException.checkType(JSONArray::class.java, obj) MultipleChoiceTaskData.fromList(task.multipleChoice, toList(obj as JSONArray)) diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/protobuf/ModelToProtoExt.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/protobuf/ModelToProtoExt.kt index 84fcef2081..a667815ceb 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/protobuf/ModelToProtoExt.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/protobuf/ModelToProtoExt.kt @@ -167,7 +167,8 @@ private fun ValueDelta.toMessage() = taskData { // TODO: Add timestamp } Task.Type.PHOTO -> takePhotoResult = takePhotoResult { - photoPath = (newTaskData as PhotoTaskData).path + val data = newTaskData as PhotoTaskData + photoPath = data.remoteFilename } Task.Type.UNKNOWN -> error("Unknown task type") } 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 55bf3cb74d..2bc24ed7fd 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 @@ -121,6 +121,7 @@ constructor( } catch (t: Throwable) { // Mark all mutations as having failed since the remote datastore only commits when all // mutations have succeeded. + Timber.d(t, "Local mutation sync failed") mutationRepository.markAsFailed(mutations, t) false } 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 9ff665b17e..f95c1295a5 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 @@ -21,13 +21,14 @@ import androidx.lifecycle.LiveData import androidx.lifecycle.asLiveData import com.google.android.ground.model.submission.PhotoTaskData import com.google.android.ground.model.submission.isNotNullOrEmpty +import com.google.android.ground.persistence.remote.firebase.FirebaseStorageManager import com.google.android.ground.repository.UserMediaRepository import com.google.android.ground.ui.datacollection.tasks.AbstractTaskViewModel import com.google.android.ground.ui.util.BitmapUtil -import java.io.IOException -import javax.inject.Inject import kotlinx.coroutines.flow.map import timber.log.Timber +import java.io.IOException +import javax.inject.Inject class PhotoTaskViewModel @Inject @@ -61,7 +62,8 @@ constructor( val bitmap = bitmapUtil.fromUri(uri) val file = userMediaRepository.savePhoto(bitmap, currentTask) userMediaRepository.addImageToGallery(file.absolutePath, file.name) - setValue(PhotoTaskData(file.absolutePath, surveyId)) + val remoteFilename = FirebaseStorageManager.getRemoteMediaPath(surveyId, file.absolutePath) + setValue(PhotoTaskData(remoteFilename)) } catch (e: IOException) { Timber.e(e, "Error getting photo selected from storage") } From 9f4feede17f5570ca89324af67e28c2b1790e043 Mon Sep 17 00:00:00 2001 From: Gino Miceli Date: Wed, 7 Aug 2024 17:12:01 -0400 Subject: [PATCH 2/5] Update protos to latest --- ground/build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ground/build.gradle b/ground/build.gradle index 648192e903..c00e60b116 100644 --- a/ground/build.gradle +++ b/ground/build.gradle @@ -349,7 +349,7 @@ dependencies { api("com.google.protobuf:protobuf-kotlin-lite:4.26.1") // Pulls protodefs from the specified commit in the ground-platform repo. - groundProtoJar "com.github.google:ground-platform:3e9162a@jar" + groundProtoJar "com.github.google:ground-platform:581946f@jar" } protobuf { From 84e27814c29c7677fe09bf922926e4ab92c9e143 Mon Sep 17 00:00:00 2001 From: Gino Miceli Date: Wed, 7 Aug 2024 17:12:48 -0400 Subject: [PATCH 3/5] Add email to audit info --- .../persistence/remote/firebase/protobuf/ModelToProtoExt.kt | 1 + 1 file changed, 1 insertion(+) diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/protobuf/ModelToProtoExt.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/protobuf/ModelToProtoExt.kt index a667815ceb..fa398d653e 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/protobuf/ModelToProtoExt.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/protobuf/ModelToProtoExt.kt @@ -177,6 +177,7 @@ private fun ValueDelta.toMessage() = taskData { private fun createAuditInfoMessage(user: User, timestamp: Date) = auditInfo { userId = user.id displayName = user.displayName + emailAddress = user.email photoUrl = user.photoUrl ?: photoUrl clientTimestamp = timestamp.toMessage() serverTimestamp = timestamp.toMessage() From 18c8135e6f794ca654fd1c16d3e5fe0edbbdff1c Mon Sep 17 00:00:00 2001 From: Gino Miceli Date: Fri, 9 Aug 2024 12:35:11 -0400 Subject: [PATCH 4/5] Fix tests --- .../protobuf/ModelToProtoExtKtTest.kt | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/ground/src/test/java/com/google/android/ground/persistence/remote/firebase/protobuf/ModelToProtoExtKtTest.kt b/ground/src/test/java/com/google/android/ground/persistence/remote/firebase/protobuf/ModelToProtoExtKtTest.kt index e9e1b04318..c4d86603d7 100644 --- a/ground/src/test/java/com/google/android/ground/persistence/remote/firebase/protobuf/ModelToProtoExtKtTest.kt +++ b/ground/src/test/java/com/google/android/ground/persistence/remote/firebase/protobuf/ModelToProtoExtKtTest.kt @@ -67,14 +67,16 @@ class ModelToProtoExtKtTest { ownerId = "userId" created = auditInfo { userId = "userId" - displayName = "User" + displayName = user.displayName + emailAddress = user.email photoUrl = "" clientTimestamp = timestamp { seconds = 987654321L } serverTimestamp = timestamp { seconds = 987654321L } } lastModified = auditInfo { userId = "userId" - displayName = "User" + displayName = user.displayName + emailAddress = user.email photoUrl = "" clientTimestamp = timestamp { seconds = 987654321L } serverTimestamp = timestamp { seconds = 987654321L } @@ -116,14 +118,16 @@ class ModelToProtoExtKtTest { ownerId = "userId" created = auditInfo { userId = "userId" - displayName = "User" + displayName = user.displayName + emailAddress = user.email photoUrl = "" clientTimestamp = timestamp { seconds = 987654321L } serverTimestamp = timestamp { seconds = 987654321L } } lastModified = auditInfo { userId = "userId" - displayName = "User" + displayName = user.displayName + emailAddress = user.email photoUrl = "" clientTimestamp = timestamp { seconds = 987654321L } serverTimestamp = timestamp { seconds = 987654321L } @@ -172,14 +176,16 @@ class ModelToProtoExtKtTest { ownerId = "userId" created = auditInfo { userId = "userId" - displayName = "User" + displayName = user.displayName + emailAddress = user.email photoUrl = "" clientTimestamp = timestamp { seconds = 987654321L } serverTimestamp = timestamp { seconds = 987654321L } } lastModified = auditInfo { userId = "userId" - displayName = "User" + displayName = user.displayName + emailAddress = user.email photoUrl = "" clientTimestamp = timestamp { seconds = 987654321L } serverTimestamp = timestamp { seconds = 987654321L } @@ -229,14 +235,16 @@ class ModelToProtoExtKtTest { ownerId = "userId" created = auditInfo { userId = "userId" - displayName = "User" + displayName = user.displayName + emailAddress = user.email photoUrl = "" clientTimestamp = timestamp { seconds = 987654321L } serverTimestamp = timestamp { seconds = 987654321L } } lastModified = auditInfo { userId = "userId" - displayName = "User" + displayName = user.displayName + emailAddress = user.email photoUrl = "" clientTimestamp = timestamp { seconds = 987654321L } serverTimestamp = timestamp { seconds = 987654321L } @@ -286,7 +294,8 @@ class ModelToProtoExtKtTest { ownerId = "userId" lastModified = auditInfo { userId = "userId" - displayName = "User" + displayName = user.displayName + emailAddress = user.email photoUrl = "" clientTimestamp = timestamp { seconds = 987654321L } serverTimestamp = timestamp { seconds = 987654321L } From 04986e1ff14e1ca958c8303492374e8eb34d489c Mon Sep 17 00:00:00 2001 From: Gino Miceli Date: Fri, 9 Aug 2024 14:22:20 -0400 Subject: [PATCH 5/5] Format file --- .../ui/datacollection/tasks/photo/PhotoTaskViewModel.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 f95c1295a5..256e17451d 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 @@ -25,10 +25,10 @@ import com.google.android.ground.persistence.remote.firebase.FirebaseStorageMana import com.google.android.ground.repository.UserMediaRepository import com.google.android.ground.ui.datacollection.tasks.AbstractTaskViewModel import com.google.android.ground.ui.util.BitmapUtil -import kotlinx.coroutines.flow.map -import timber.log.Timber import java.io.IOException import javax.inject.Inject +import kotlinx.coroutines.flow.map +import timber.log.Timber class PhotoTaskViewModel @Inject