From 400ea88c1b757e44817cd681fd6699ff765ffbd9 Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Fri, 9 Aug 2024 19:11:39 +0530 Subject: [PATCH] Fix value for multiple choice tasks with "other" option selected (#2621) --- .../submission/MultipleChoiceTaskData.kt | 22 +++++--- .../firebase/protobuf/ModelToProtoExt.kt | 2 +- .../datacollection/DataCollectionViewModel.kt | 2 +- .../MultipleChoiceTaskViewModel.kt | 6 +-- .../schema/SubmissionMutationConverterTest.kt | 50 ++++++++++++++++++- 5 files changed, 69 insertions(+), 13 deletions(-) diff --git a/ground/src/main/java/com/google/android/ground/model/submission/MultipleChoiceTaskData.kt b/ground/src/main/java/com/google/android/ground/model/submission/MultipleChoiceTaskData.kt index 6f954abffc..8fa2e686d4 100644 --- a/ground/src/main/java/com/google/android/ground/model/submission/MultipleChoiceTaskData.kt +++ b/ground/src/main/java/com/google/android/ground/model/submission/MultipleChoiceTaskData.kt @@ -17,6 +17,7 @@ package com.google.android.ground.model.submission import com.google.android.ground.model.task.MultipleChoice +import com.google.android.ground.ui.datacollection.tasks.multiplechoice.MultipleChoiceTaskViewModel import kotlinx.serialization.Serializable /** User responses to a select-one (radio) or select-multiple (checkbox) field. */ @@ -26,13 +27,18 @@ class MultipleChoiceTaskData( val selectedOptionIds: List, ) : TaskData { - // TODO: Reuse the key value here and in the view model - fun hasOtherText(): Boolean = selectedOptionIds.contains("OTHER_ID") + fun getSelectedOptionsIdsExceptOther(): List = + selectedOptionIds.filterNot { it.isOtherText() } - fun getOtherText(): String { - val otherId = selectedOptionIds.firstOrNull { it == "OTHER_ID" } ?: return "" - return multipleChoice?.getOptionById(otherId)?.label ?: "" - } + fun hasOtherText(): Boolean = selectedOptionIds.any { it.isOtherText() } + + fun getOtherText(): String = + selectedOptionIds + .firstOrNull { it.isOtherText() } + ?.removeSurrounding( + prefix = MultipleChoiceTaskViewModel.OTHER_PREFIX, + suffix = MultipleChoiceTaskViewModel.OTHER_SUFFIX, + ) ?: "" // TODO: Make these inner classes non-static and access Task directly. override fun getDetailsText(): String = @@ -55,6 +61,10 @@ class MultipleChoiceTaskData( override fun toString(): String = selectedOptionIds.sorted().joinToString() + private fun String.isOtherText(): Boolean = + startsWith(MultipleChoiceTaskViewModel.OTHER_PREFIX) && + endsWith(MultipleChoiceTaskViewModel.OTHER_SUFFIX) + companion object { fun fromList(multipleChoice: MultipleChoice?, ids: List): TaskData? = if (ids.isEmpty()) { 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..a409b1e6ba 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 @@ -148,7 +148,7 @@ private fun ValueDelta.toMessage() = taskData { dateTime = timestamp { seconds = (newTaskData as TimeTaskData).time.time / 1000 } } Task.Type.MULTIPLE_CHOICE -> multipleChoiceResponses = multipleChoiceResponses { - (newTaskData as MultipleChoiceTaskData).selectedOptionIds.forEach { + (newTaskData as MultipleChoiceTaskData).getSelectedOptionsIdsExceptOther().forEach { selectedOptionIds.add(it) } if (newTaskData.hasOtherText()) { diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionViewModel.kt index 2e7a1778e3..7759c783c1 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionViewModel.kt @@ -240,7 +240,7 @@ internal constructor( return } - data[taskViewModel.task] = taskViewModel.taskTaskData.firstOrNull() + data[taskViewModel.task] = taskViewModel.taskTaskData.value if (!isLastPosition()) { step(1) diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskViewModel.kt index 1822a574eb..1d02caef40 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskViewModel.kt @@ -148,8 +148,8 @@ class MultipleChoiceTaskViewModel @Inject constructor(resources: Resources) : } companion object { - private const val OTHER_ID: String = "OTHER_ID" - private const val OTHER_PREFIX: String = "[ " - private const val OTHER_SUFFIX: String = " ]" + const val OTHER_ID: String = "OTHER_ID" + const val OTHER_PREFIX: String = "[ " + const val OTHER_SUFFIX: String = " ]" } } diff --git a/ground/src/test/java/com/google/android/ground/persistence/remote/firebase/schema/SubmissionMutationConverterTest.kt b/ground/src/test/java/com/google/android/ground/persistence/remote/firebase/schema/SubmissionMutationConverterTest.kt index e48b23e7bc..9809a288a3 100644 --- a/ground/src/test/java/com/google/android/ground/persistence/remote/firebase/schema/SubmissionMutationConverterTest.kt +++ b/ground/src/test/java/com/google/android/ground/persistence/remote/firebase/schema/SubmissionMutationConverterTest.kt @@ -60,6 +60,7 @@ import com.google.android.ground.proto.TaskData.DRAW_GEOMETRY_RESULT_FIELD_NUMBE import com.google.android.ground.proto.TaskData.DateTimeResponse.DATE_TIME_FIELD_NUMBER import com.google.android.ground.proto.TaskData.DrawGeometryResult.GEOMETRY_FIELD_NUMBER import com.google.android.ground.proto.TaskData.MULTIPLE_CHOICE_RESPONSES_FIELD_NUMBER +import com.google.android.ground.proto.TaskData.MultipleChoiceResponses.OTHER_TEXT_FIELD_NUMBER import com.google.android.ground.proto.TaskData.MultipleChoiceResponses.SELECTED_OPTION_IDS_FIELD_NUMBER import com.google.android.ground.proto.TaskData.NUMBER_RESPONSE_FIELD_NUMBER import com.google.android.ground.proto.TaskData.NumberResponse.NUMBER_FIELD_NUMBER @@ -86,7 +87,6 @@ class SubmissionMutationConverterTest { private val textTaskData = TextTaskData.fromString("some data") - // TODO: Add test coverage for "other" value private val singleChoiceResponse = MultipleChoiceTaskData.fromList( MultipleChoice( @@ -99,7 +99,18 @@ class SubmissionMutationConverterTest { ids = listOf("option id 1"), ) - // TODO: Add test coverage for "other" value + private val singleChoiceResponseOther = + MultipleChoiceTaskData.fromList( + MultipleChoice( + persistentListOf( + Option("option id 1", "code1", "Option 1"), + Option("option id 2", "code2", "Option 2"), + ), + MultipleChoice.Cardinality.SELECT_ONE, + ), + ids = listOf("[ other value ]"), + ) + private val multipleChoiceTaskData = MultipleChoiceTaskData.fromList( MultipleChoice( @@ -112,6 +123,18 @@ class SubmissionMutationConverterTest { ids = listOf("option id 1", "option id 2"), ) + private val multipleChoiceTaskDataOther = + MultipleChoiceTaskData.fromList( + MultipleChoice( + persistentListOf( + Option("option id 1", "code1", "Option 1"), + Option("option id 2", "code2", "Option 2"), + ), + MultipleChoice.Cardinality.SELECT_MULTIPLE, + ), + ids = listOf("option id 1", "option id 2", "[ other value ]"), + ) + private val numberTaskData = NumberTaskData.fromNumber("123") private val dropPinTaskResult = DropPinTaskData(Point(Coordinates(10.0, 20.0))) @@ -159,11 +182,21 @@ class SubmissionMutationConverterTest { taskType = Task.Type.MULTIPLE_CHOICE, newTaskData = singleChoiceResponse, ), + ValueDelta( + taskId = "single_choice_task_other", + taskType = Task.Type.MULTIPLE_CHOICE, + newTaskData = singleChoiceResponseOther, + ), ValueDelta( taskId = "multiple_choice_task", taskType = Task.Type.MULTIPLE_CHOICE, newTaskData = multipleChoiceTaskData, ), + ValueDelta( + taskId = "multiple_choice_task_other", + taskType = Task.Type.MULTIPLE_CHOICE, + newTaskData = multipleChoiceTaskDataOther, + ), ValueDelta( taskId = "number_task", taskType = Task.Type.NUMBER, @@ -200,6 +233,11 @@ class SubmissionMutationConverterTest { mapOf(SELECTED_OPTION_IDS_FIELD_NUMBER.toString() to listOf("option id 1")), TASK_ID_FIELD_NUMBER.toString() to "single_choice_task", ), + mapOf( + MULTIPLE_CHOICE_RESPONSES_FIELD_NUMBER.toString() to + mapOf(OTHER_TEXT_FIELD_NUMBER.toString() to "other value"), + TASK_ID_FIELD_NUMBER.toString() to "single_choice_task_other", + ), mapOf( MULTIPLE_CHOICE_RESPONSES_FIELD_NUMBER.toString() to mapOf( @@ -207,6 +245,14 @@ class SubmissionMutationConverterTest { ), TASK_ID_FIELD_NUMBER.toString() to "multiple_choice_task", ), + mapOf( + MULTIPLE_CHOICE_RESPONSES_FIELD_NUMBER.toString() to + mapOf( + SELECTED_OPTION_IDS_FIELD_NUMBER.toString() to listOf("option id 1", "option id 2"), + OTHER_TEXT_FIELD_NUMBER.toString() to "other value", + ), + TASK_ID_FIELD_NUMBER.toString() to "multiple_choice_task_other", + ), mapOf( NUMBER_RESPONSE_FIELD_NUMBER.toString() to mapOf(NUMBER_FIELD_NUMBER.toString() to 123.0), TASK_ID_FIELD_NUMBER.toString() to "number_task",