Skip to content

Commit

Permalink
Fix LocationOfInterest display name (#2047)
Browse files Browse the repository at this point in the history
* Fix LocationOfInterest display name.

* Include properties field in Android Loi dataclasses.
* Propogate customId field to local database.
* Remove caption field.

* Make Loi properties non-null where possible

* Fix null values in tests

* Fix tests and lint errors

* Remove unused properties

* Fix LoiCardUtil logic

---------

Co-authored-by: Gino Miceli <[email protected]>
  • Loading branch information
rachaprince and gino-m authored Nov 7, 2023
1 parent 172107e commit be8105f
Show file tree
Hide file tree
Showing 18 changed files with 181 additions and 85 deletions.
2 changes: 1 addition & 1 deletion ground/src/main/java/com/google/android/ground/Config.kt
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ object Config {

// Local db settings.
// TODO(#128): Reset version to 1 before releasing.
const val DB_VERSION = 109
const val DB_VERSION = 110
const val DB_NAME = "ground.db"

// Firebase Cloud Firestore settings.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ 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

/** Alias for a map of properties with string names. */
typealias LoiProperties = Map<String, Any>

/** User-defined locations of interest (LOI) shown on the map. */
data class LocationOfInterest(
/** A system-defined ID for this LOI. */
Expand All @@ -31,9 +34,7 @@ data class LocationOfInterest(
/** The job associated with this LOI. */
val job: Job,
/** A user-specified ID for this location of interest. */
val customId: String? = null,
/** A human readable caption for this location of interest. */
val caption: String? = null,
val customId: String = "",
/** User and time audit info pertaining to the creation of this LOI. */
val created: AuditInfo,
/** User and time audit info pertaining to the last modification of this LOI. */
Expand All @@ -51,7 +52,9 @@ data class LocationOfInterest(
* Whether this LOI was created opportunistically by the user through the Suggest LOI flow, or
* false if the LOI was created by the survey organizer.
*/
val isOpportunistic: Boolean = false
val isOpportunistic: Boolean = false,
/** Custom map of properties for this LOI. Corresponds to the properties field in GeoJSON */
val properties: LoiProperties = mapOf(),
) {

/**
Expand All @@ -66,11 +69,12 @@ data class LocationOfInterest(
surveyId = surveyId,
locationOfInterestId = id,
userId = userId,
customId = customId,
clientTimestamp = lastModified.clientTimestamp,
geometry = geometry,
caption = caption,
submissionCount = submissionCount,
ownerEmail = ownerEmail,
isOpportunistic = isOpportunistic
isOpportunistic = isOpportunistic,
properties = properties
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package com.google.android.ground.model.mutation

import com.google.android.ground.model.geometry.Geometry
import com.google.android.ground.model.locationofinterest.LoiProperties
import java.util.Date

data class LocationOfInterestMutation(
Expand All @@ -29,9 +30,10 @@ data class LocationOfInterestMutation(
override val retryCount: Long = 0,
override val lastError: String = "",
val jobId: String = "",
val customId: String = "",
val geometry: Geometry? = null,
val caption: String? = null,
val submissionCount: Int = 0,
val ownerEmail: String? = null,
val isOpportunistic: Boolean = false
val isOpportunistic: Boolean = false,
val properties: LoiProperties = mapOf(),
) : Mutation()
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import com.google.android.ground.Config
import com.google.android.ground.persistence.local.room.converter.GeometryWrapperTypeConverter
import com.google.android.ground.persistence.local.room.converter.JsonArrayTypeConverter
import com.google.android.ground.persistence.local.room.converter.JsonObjectTypeConverter
import com.google.android.ground.persistence.local.room.converter.LoiPropertiesMapConverter
import com.google.android.ground.persistence.local.room.converter.StyleTypeConverter
import com.google.android.ground.persistence.local.room.dao.*
import com.google.android.ground.persistence.local.room.entity.*
Expand Down Expand Up @@ -64,7 +65,8 @@ import com.google.android.ground.persistence.local.room.fields.*
MutationEntitySyncStatus::class,
OfflineAreaEntityState::class,
StyleTypeConverter::class,
TileSetEntityState::class
TileSetEntityState::class,
LoiPropertiesMapConverter::class,
)
abstract class LocalDatabase : RoomDatabase() {
abstract fun locationOfInterestDao(): LocationOfInterestDao
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,13 +130,14 @@ fun LocationOfInterest.toLocalDataStoreObject() =
surveyId = surveyId,
jobId = job.id,
state = EntityState.DEFAULT,
caption = caption,
created = created.toLocalDataStoreObject(),
lastModified = lastModified.toLocalDataStoreObject(),
geometry = geometry.toLocalDataStoreObject(),
customId = customId,
submissionCount = submissionCount,
ownerEmail = ownerEmail,
isOpportunistic = isOpportunistic
isOpportunistic = isOpportunistic,
properties = properties,
)

fun LocationOfInterestEntity.toModelObject(survey: Survey): LocationOfInterest =
Expand All @@ -148,9 +149,10 @@ fun LocationOfInterestEntity.toModelObject(survey: Survey): LocationOfInterest =
surveyId = surveyId,
created = created.toModelObject(),
lastModified = lastModified.toModelObject(),
caption = caption,
customId = customId,
geometry = geometry.getGeometry(),
submissionCount = submissionCount,
properties = properties,
job = survey.getJob(jobId = jobId)
?: throw LocalDataConsistencyException(
"Unknown jobId ${this.jobId} in location of interest ${this.id}"
Expand All @@ -173,14 +175,15 @@ fun LocationOfInterestMutation.toLocalDataStoreObject(user: User): LocationOfInt
surveyId = surveyId,
jobId = jobId,
state = EntityState.DEFAULT,
caption = caption,
// TODO(#1562): Preserve creation audit info for UPDATE mutations.
created = auditInfo,
lastModified = auditInfo,
geometry = geometry?.toLocalDataStoreObject(),
customId = customId,
submissionCount = submissionCount,
ownerEmail = ownerEmail,
isOpportunistic = isOpportunistic
isOpportunistic = isOpportunistic,
properties = properties
)
}

Expand All @@ -191,13 +194,14 @@ fun LocationOfInterestMutation.toLocalDataStoreObject() =
jobId = jobId,
type = MutationEntityType.fromMutationType(type),
newGeometry = geometry?.toLocalDataStoreObject(),
caption = caption,
newCustomId = customId,
userId = userId,
locationOfInterestId = locationOfInterestId,
syncStatus = MutationEntitySyncStatus.fromMutationSyncStatus(syncStatus),
clientTimestamp = clientTimestamp.time,
lastError = lastError,
retryCount = retryCount
retryCount = retryCount,
newProperties = properties
)

fun LocationOfInterestMutationEntity.toModelObject() =
Expand All @@ -207,13 +211,14 @@ fun LocationOfInterestMutationEntity.toModelObject() =
jobId = jobId,
type = type.toMutationType(),
geometry = newGeometry?.getGeometry(),
caption = caption,
customId = newCustomId,
userId = userId,
locationOfInterestId = locationOfInterestId,
syncStatus = syncStatus.toMutationSyncStatus(),
clientTimestamp = Date(clientTimestamp),
lastError = lastError,
retryCount = retryCount,
properties = newProperties
)

fun MultipleChoiceEntity.toModelObject(optionEntities: List<OptionEntity>): MultipleChoice {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* 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.persistence.local.room.converter

import androidx.room.TypeConverter
import com.google.common.reflect.TypeToken
import com.google.gson.Gson
import com.google.gson.JsonSyntaxException
import kotlinx.collections.immutable.toPersistentMap
import org.json.JSONException
import org.json.JSONObject
import timber.log.Timber

object LoiPropertiesMapConverter {
@TypeConverter
fun toString(properties: Map<String, Any>): String {
var jsonString = ""
try {
jsonString = JSONObject(properties).toString()
} catch (e: JSONException) {
Timber.e(e, "Error building JSON")
}
return jsonString
}

@TypeConverter
fun fromString(jsonString: String): Map<String, Any> {
var map = mutableMapOf<String, Any>()
try {
val type = object : TypeToken<Map<String, Any>>() {}.type
map = Gson().fromJson(jsonString, type)
} catch (e: JsonSyntaxException) {
Timber.e(e, "Error parsing JSON string")
}
return map.toPersistentMap()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package com.google.android.ground.persistence.local.room.entity

import androidx.room.*
import com.google.android.ground.model.locationofinterest.LoiProperties
import com.google.android.ground.persistence.local.room.fields.EntityState

/**
Expand All @@ -28,11 +29,12 @@ data class LocationOfInterestEntity(
@ColumnInfo(name = "survey_id") val surveyId: String,
@ColumnInfo(name = "job_id") val jobId: String,
@ColumnInfo(name = "state") val state: EntityState, // TODO: Rename to DeletionState.
@ColumnInfo(name = "caption") val caption: String?,
@Embedded(prefix = "created_") val created: AuditInfoEntity,
@Embedded(prefix = "modified_") val lastModified: AuditInfoEntity,
val geometry: GeometryWrapper?,
val customId: String,
val submissionCount: Int,
val ownerEmail: String?,
val isOpportunistic: Boolean
val isOpportunistic: Boolean,
val properties: LoiProperties,
)
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package com.google.android.ground.persistence.local.room.entity

import androidx.room.*
import com.google.android.ground.model.locationofinterest.LoiProperties
import com.google.android.ground.persistence.local.room.fields.MutationEntitySyncStatus
import com.google.android.ground.persistence.local.room.fields.MutationEntityType

Expand Down Expand Up @@ -48,7 +49,8 @@ data class LocationOfInterestMutationEntity(
@ColumnInfo(name = "client_timestamp") val clientTimestamp: Long,
@ColumnInfo(name = "location_of_interest_id") val locationOfInterestId: String,
@ColumnInfo(name = "job_id") val jobId: String,
@ColumnInfo(name = "caption") val caption: String?,
/** Non-null if the LOI's geometry was updated, null if unchanged. */
val newGeometry: GeometryWrapper?,
val newProperties: LoiProperties,
val newCustomId: String
)
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,14 @@ object LoiConverter {
return LocationOfInterest(
id = loiId,
surveyId = survey.id,
customId = loiDoc.customId,
caption = loiDoc.caption,
customId = loiDoc.customId ?: "",
job = job,
created = AuditInfoConverter.toAuditInfo(created),
lastModified = AuditInfoConverter.toAuditInfo(lastModified),
// TODO(#929): Set geometry once LOI has been updated to use our own model.
geometry = geometry,
submissionCount = submissionCount
submissionCount = submissionCount,
properties = loiDoc.properties ?: mapOf()
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.google.android.ground.persistence.remote.firebase.schema

import com.google.android.ground.model.locationofinterest.LoiProperties
import com.google.firebase.firestore.GeoPoint
import com.google.firebase.firestore.IgnoreExtraProperties

Expand All @@ -24,11 +25,11 @@ import com.google.firebase.firestore.IgnoreExtraProperties
data class LoiDocument(
val jobId: String? = null,
val customId: String? = null,
val caption: String? = null,
val location: GeoPoint? = null,
val geoJson: String? = null,
val geometry: Map<String, Any>? = null,
val created: AuditInfoNestedObject? = null,
val lastModified: AuditInfoNestedObject? = null,
val submissionCount: Int? = null
val submissionCount: Int? = null,
val properties: LoiProperties? = null,
)
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ class LocationOfInterestHelper @Inject internal constructor(private val resource
locationOfInterest.map(::getLabel).orElse("")

fun getLabel(loi: LocationOfInterest): String {
val caption = loi.caption?.trim { it <= ' ' } ?: ""
// TODO(#2046): Reuse logic from card util to display LOI label
val caption = loi.customId?.trim { it <= ' ' } ?: ""
return caption.ifEmpty { getLocationOfInterestType(loi) }
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,16 @@ import com.google.android.ground.util.isNotNullOrEmpty
object LoiCardUtil {

fun getDisplayLoiName(context: Context, loi: LocationOfInterest): String {
val caption = loi.caption
val customId = loi.customId
val loiId =
if (loi.customId.isNotNullOrEmpty()) loi.customId else loi.properties?.get("id")?.toString()
val geometry = loi.geometry
return if (caption.isNotNullOrEmpty() && customId.isNotNullOrEmpty()) {
"$caption ($customId)"
} else if (caption.isNotNullOrEmpty()) {
"$caption"
} else if (customId.isNotNullOrEmpty()) {
"${geometry.toType(context)} ($customId)"
val name: String? = loi.properties?.get("name")?.toString()
return if (name.isNotNullOrEmpty() && loiId.isNotNullOrEmpty()) {
"$name ($loiId)"
} else if (name.isNotNullOrEmpty()) {
"$name"
} else if (loiId.isNotNullOrEmpty()) {
"${geometry.toType(context)} ($loiId)"
} else {
geometry.toDefaultName(context)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,13 @@ class LoiLocalDataStoreConverterTest {
mockLoiDocumentSnapshot(
"loi001",
LoiDocument(
/* jobId */
"job001", /* customId */
null, /* caption */
null, /* location */
null, /* geoJson */
null, /* geometry */
null, /* created */
AUDIT_INFO_1_NESTED_OBJECT, /* lastModified */
AUDIT_INFO_2_NESTED_OBJECT
/* jobId */ "job001",
/* customId */ null,
/* location */ null,
/* geoJson */ null,
/* geometry */ null,
/* created */ AUDIT_INFO_1_NESTED_OBJECT,
/* lastModified */ AUDIT_INFO_2_NESTED_OBJECT
)
)
assertThat(toLocationOfInterest().isFailure).isTrue()
Expand All @@ -93,7 +91,6 @@ class LoiLocalDataStoreConverterTest {
null,
null,
null,
null,
noVerticesGeometry,
AUDIT_INFO_1_NESTED_OBJECT,
AUDIT_INFO_2_NESTED_OBJECT
Expand Down
Loading

0 comments on commit be8105f

Please sign in to comment.