From 982f02c2c4bd203e47f51e654a4079b4bc258da0 Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Sun, 24 Nov 2024 11:30:41 +0530 Subject: [PATCH 1/5] Make other text required --- .../MultipleChoiceTaskViewModel.kt | 8 ++- .../MultipleChoiceTaskFragmentTest.kt | 56 ++++++++++++++----- 2 files changed, 49 insertions(+), 15 deletions(-) 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 bf2cc0ab2a..dea000a842 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 @@ -49,7 +49,7 @@ class MultipleChoiceTaskViewModel @Inject constructor() : AbstractTaskViewModel( ?.let { val selected = if (task.isRequired) { - otherText != "" + otherText.trim() != "" } else { true } @@ -92,6 +92,12 @@ class MultipleChoiceTaskViewModel @Inject constructor() : AbstractTaskViewModel( } fun updateResponse() { + // Check if "other" text is missing or not. + if (task.isRequired && selectedIds.contains(OTHER_ID) && otherText.trim().isEmpty()) { + clearResponse() + return + } + setValue( fromList( task.multipleChoice, diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskFragmentTest.kt index 9a82a42fd4..b3a9943606 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/tasks/multiplechoice/MultipleChoiceTaskFragmentTest.kt @@ -147,29 +147,21 @@ class MultipleChoiceTaskFragmentTest : } @Test - fun `saves other text when selected`() = runWithTestDispatcher { + fun `saves other text`() = runWithTestDispatcher { val multipleChoice = MultipleChoice(options, MultipleChoice.Cardinality.SELECT_MULTIPLE, true) - setupTaskFragment(job, task.copy(multipleChoice = multipleChoice)) + setupTaskFragment( + job, + task.copy(multipleChoice = multipleChoice, isRequired = true), + ) val userInput = "User inputted text" onView(withText("Other")).perform(click()) onView(allOf(isDisplayed(), withId(R.id.user_response_text))) .perform(CustomViewActions.forceTypeText(userInput)) - hasValue(MultipleChoiceTaskData(multipleChoice, listOf("[ $userInput ]"))) - runner().assertButtonIsEnabled("Next") - } - - @Test - fun `selects other option on text input`() = runWithTestDispatcher { - val multipleChoice = MultipleChoice(options, MultipleChoice.Cardinality.SELECT_MULTIPLE, true) - setupTaskFragment(job, task.copy(multipleChoice = multipleChoice)) - val userInput = "A" - onView(withText("Other")).perform(click()) - onView(allOf(isDisplayed(), withId(R.id.user_response_text))) - .perform(CustomViewActions.forceTypeText(userInput)) onView(withText("Other")).check(matches(isChecked())) hasValue(MultipleChoiceTaskData(multipleChoice, listOf("[ $userInput ]"))) + runner().assertButtonIsEnabled("Next") } @Test @@ -315,4 +307,40 @@ class MultipleChoiceTaskFragmentTest : .assertButtonIsDisabled("Next") .assertButtonIsEnabled("Skip") } + + @Test + fun `doesn't save response when other text is missing and task is required`() = + runWithTestDispatcher { + val multipleChoice = MultipleChoice(options, MultipleChoice.Cardinality.SELECT_ONE, true) + setupTaskFragment( + job, + task.copy(multipleChoice = multipleChoice, isRequired = true), + ) + + onView(withText("Other")).perform(click()) + onView(allOf(isDisplayed(), withId(R.id.user_response_text))) + .perform(CustomViewActions.forceTypeText("")) + + hasValue(null) + runner().assertButtonIsDisabled("Next") + } + + @Test + fun `doesn't save response when multiple options selected but other text is missing and task is required`() = + runWithTestDispatcher { + val multipleChoice = MultipleChoice(options, MultipleChoice.Cardinality.SELECT_MULTIPLE, true) + setupTaskFragment( + job, + task.copy(multipleChoice = multipleChoice, isRequired = true), + ) + + onView(withText("Option 1")).perform(click()) + onView(withText("Option 2")).perform(click()) + onView(withText("Other")).perform(click()) + onView(allOf(isDisplayed(), withId(R.id.user_response_text))) + .perform(CustomViewActions.forceTypeText("")) + + hasValue(null) + runner().assertButtonIsDisabled("Next") + } } From 227fdd58271c2b187bab41dc76942fd81c8b1327 Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Sun, 24 Nov 2024 12:09:45 +0530 Subject: [PATCH 2/5] Fix ktdoc comment block --- .../tasks/multiplechoice/MultipleChoiceTaskViewModel.kt | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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 dea000a842..7166407ce5 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 @@ -25,11 +25,11 @@ import com.google.android.ground.model.task.MultipleChoice.Cardinality.SELECT_MU import com.google.android.ground.model.task.Option import com.google.android.ground.model.task.Task import com.google.android.ground.ui.datacollection.tasks.AbstractTaskViewModel -import javax.inject.Inject import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.filterNotNull +import javax.inject.Inject class MultipleChoiceTaskViewModel @Inject constructor() : AbstractTaskViewModel() { @@ -109,7 +109,9 @@ class MultipleChoiceTaskViewModel @Inject constructor() : AbstractTaskViewModel( ) } - /* Reads the list of options in a multiple choice object and converts them to MultipleChoiceItems*/ + /** + * Reads the list of options in a multiple choice object and converts them to MultipleChoiceItems. + */ private fun updateMultipleChoiceItems() { val multipleChoice = checkNotNull(task.multipleChoice) val itemsFromOptions: MutableList = mutableListOf() @@ -136,7 +138,7 @@ class MultipleChoiceTaskViewModel @Inject constructor() : AbstractTaskViewModel( this._items.value = itemsFromOptions } - /* Reads the saved task value and adds selected items to the selected list*/ + /** Reads the saved task value and adds selected items to the selected list. */ private fun loadPendingSelections() { val selectedOptionIds = (taskTaskData.value as? MultipleChoiceTaskData)?.selectedOptionIds val multipleChoice = checkNotNull(task.multipleChoice) From 7158679e77a45984986edc7d076ca8a375fbaefa Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Sun, 24 Nov 2024 15:02:34 +0530 Subject: [PATCH 3/5] Fix import order --- .../tasks/multiplechoice/MultipleChoiceTaskViewModel.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 7166407ce5..07ca1f7a23 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 @@ -25,11 +25,11 @@ import com.google.android.ground.model.task.MultipleChoice.Cardinality.SELECT_MU import com.google.android.ground.model.task.Option import com.google.android.ground.model.task.Task import com.google.android.ground.ui.datacollection.tasks.AbstractTaskViewModel +import javax.inject.Inject import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.filterNotNull -import javax.inject.Inject class MultipleChoiceTaskViewModel @Inject constructor() : AbstractTaskViewModel() { From b1564975b811156e81b549faca13e6d2be4118db Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Wed, 27 Nov 2024 00:54:27 +0530 Subject: [PATCH 4/5] Apply suggested refactors --- .../MultipleChoiceTaskViewModel.kt | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) 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 07ca1f7a23..7fbfea2d37 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 @@ -47,13 +47,11 @@ class MultipleChoiceTaskViewModel @Inject constructor() : AbstractTaskViewModel( _items.value .firstOrNull { it.isOtherOption } ?.let { - val selected = - if (task.isRequired) { - otherText.trim() != "" - } else { - true - } - setItem(it, selected, it.cardinality == SELECT_MULTIPLE) + setItem( + item = it, + selection = isOtherTextValid(), + canSelectMultiple = it.cardinality == SELECT_MULTIPLE, + ) } updateResponse() } @@ -93,7 +91,7 @@ class MultipleChoiceTaskViewModel @Inject constructor() : AbstractTaskViewModel( fun updateResponse() { // Check if "other" text is missing or not. - if (task.isRequired && selectedIds.contains(OTHER_ID) && otherText.trim().isEmpty()) { + if (selectedIds.contains(OTHER_ID) && !isOtherTextValid()) { clearResponse() return } @@ -109,6 +107,16 @@ class MultipleChoiceTaskViewModel @Inject constructor() : AbstractTaskViewModel( ) } + /** Returns true if the value for "other" text is valid, otherwise false. */ + private fun isOtherTextValid(): Boolean { + if (!task.isRequired) { + // If task is not required, then the text value can be empty. Hence, it is always valid. + return true + } + + return otherText.isNotBlank() + } + /** * Reads the list of options in a multiple choice object and converts them to MultipleChoiceItems. */ From a090df9f480a9c12f59b9505c1ccb145fb3c3cef Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Wed, 27 Nov 2024 01:03:13 +0530 Subject: [PATCH 5/5] Inline cardinality check --- .../MultipleChoiceTaskViewModel.kt | 16 +++++----------- .../res/layout/multiple_choice_checkbox_item.xml | 2 +- .../layout/multiple_choice_radiobutton_item.xml | 2 +- 3 files changed, 7 insertions(+), 13 deletions(-) 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 7fbfea2d37..2d173c4558 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 @@ -46,13 +46,7 @@ class MultipleChoiceTaskViewModel @Inject constructor() : AbstractTaskViewModel( // Set the other option. _items.value .firstOrNull { it.isOtherOption } - ?.let { - setItem( - item = it, - selection = isOtherTextValid(), - canSelectMultiple = it.cardinality == SELECT_MULTIPLE, - ) - } + ?.let { setItem(item = it, selection = isOtherTextValid()) } updateResponse() } @@ -71,8 +65,8 @@ class MultipleChoiceTaskViewModel @Inject constructor() : AbstractTaskViewModel( updateMultipleChoiceItems() } - fun setItem(item: MultipleChoiceItem, selection: Boolean, canSelectMultiple: Boolean) { - if (!canSelectMultiple && selection) { + fun setItem(item: MultipleChoiceItem, selection: Boolean) { + if (item.cardinality != SELECT_MULTIPLE && selection) { selectedIds.clear() } if (selection) { @@ -84,9 +78,9 @@ class MultipleChoiceTaskViewModel @Inject constructor() : AbstractTaskViewModel( updateMultipleChoiceItems() } - fun toggleItem(item: MultipleChoiceItem, canSelectMultiple: Boolean) { + fun toggleItem(item: MultipleChoiceItem) { val wasSelected = selectedIds.contains(item.option.id) - setItem(item, !wasSelected, canSelectMultiple) + setItem(item, !wasSelected) } fun updateResponse() { diff --git a/ground/src/main/res/layout/multiple_choice_checkbox_item.xml b/ground/src/main/res/layout/multiple_choice_checkbox_item.xml index 786e35c400..d20736c954 100644 --- a/ground/src/main/res/layout/multiple_choice_checkbox_item.xml +++ b/ground/src/main/res/layout/multiple_choice_checkbox_item.xml @@ -42,7 +42,7 @@ android:layout_width="match_parent" android:layout_height="wrap_content" android:checked="@{item.isSelected}" - android:onClickListener="@{() -> viewModel.toggleItem(item, true)}" + android:onClickListener="@{() -> viewModel.toggleItem(item)}" android:padding="16dp" android:text="@{ item.isOtherOption ? @string/other : item.option.label }" app:layout_constraintStart_toStartOf="parent" diff --git a/ground/src/main/res/layout/multiple_choice_radiobutton_item.xml b/ground/src/main/res/layout/multiple_choice_radiobutton_item.xml index 2f24b9a93f..43370390be 100644 --- a/ground/src/main/res/layout/multiple_choice_radiobutton_item.xml +++ b/ground/src/main/res/layout/multiple_choice_radiobutton_item.xml @@ -41,7 +41,7 @@ android:layout_height="wrap_content" android:buttonTint="?attr/colorPrimary" android:checked="@{item.isSelected}" - android:onClickListener="@{() -> viewModel.toggleItem(item, false)}" + android:onClickListener="@{() -> viewModel.toggleItem(item)}" android:padding="16dp" android:text="@{ item.isOtherOption ? @string/other : item.option.label }" app:layout_constraintStart_toStartOf="parent"