Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix local (de)serialization of photo tasks #2617

Merged
merged 7 commits into from
Aug 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ground/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 ->
Expand Down Expand Up @@ -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))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand All @@ -176,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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ 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
Expand Down Expand Up @@ -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")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,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 }
Expand Down Expand Up @@ -118,14 +120,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 }
Expand Down Expand Up @@ -175,14 +179,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 }
Expand Down Expand Up @@ -233,14 +239,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 }
Expand Down Expand Up @@ -291,7 +299,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 }
Expand Down