From c88865f74415ac1354dbd9d75aaa37ea0796da98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A9l=C3=A8ne=20Martin?= Date: Thu, 30 Sep 2021 10:27:49 -0700 Subject: [PATCH 1/2] Add failing test for selects in repeat case --- .../form/api/FormEntryPromptTest.java | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/src/test/java/org/javarosa/form/api/FormEntryPromptTest.java b/src/test/java/org/javarosa/form/api/FormEntryPromptTest.java index a23f84d6b..57cc8692f 100644 --- a/src/test/java/org/javarosa/form/api/FormEntryPromptTest.java +++ b/src/test/java/org/javarosa/form/api/FormEntryPromptTest.java @@ -26,6 +26,7 @@ import static org.javarosa.core.util.XFormsElement.item; import static org.javarosa.core.util.XFormsElement.mainInstance; import static org.javarosa.core.util.XFormsElement.model; +import static org.javarosa.core.util.XFormsElement.repeat; import static org.javarosa.core.util.XFormsElement.select1Dynamic; import static org.javarosa.core.util.XFormsElement.t; import static org.javarosa.core.util.XFormsElement.title; @@ -35,6 +36,7 @@ import org.junit.Test; public class FormEntryPromptTest { + //region Binding of select choice values to labels @Test public void getSelectItemText_onSelectionFromDynamicSelect_withoutTranslations_returnsLabelInnerText() throws IOException { Scenario scenario = Scenario.init("Select", html( @@ -108,4 +110,41 @@ public void getSelectItemText_onSelectionFromDynamicSelect_withTranslations_retu scenario.setLanguage("fr"); assertThat(questionPrompt.getAnswerText(), is("B (fr)")); } + + @Test + public void getSelectItemText_onSelectionsInRepeatInstances_returnsLabelInnerText() throws IOException { + Scenario scenario = Scenario.init("Select", html( + head( + title("Select"), + model( + mainInstance( + t("data id='select-repeat'", + t("repeat", + t("select", "a")), + t("repeat", + t("select", "a")))), + + instance("choices", + item("a", "A"), + item("aa", "AA"), + item("b", "B"), + item("bb", "BB")))), + body( + repeat("/data/repeat", + select1Dynamic("/data/repeat/select", "instance('choices')/root/item") + )))); + + scenario.next(); + scenario.next(); + FormEntryPrompt questionPrompt = scenario.getFormEntryPromptAtIndex(); + assertThat(questionPrompt.getAnswerText(), is("A")); + + // Prior to https://github.com/getodk/javarosa/issues/642 being addressed, the selected choice for a select in a + // repeat instance with the same choice list as the prior repeat instance's select would not be bound to its label + scenario.next(); + scenario.next(); + questionPrompt = scenario.getFormEntryPromptAtIndex(); + assertThat(questionPrompt.getAnswerText(), is("A")); + } + //endregion } From 416f18dd251cac9d26a35d2c9e0b9625c87202b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A9l=C3=A8ne=20Martin?= Date: Thu, 30 Sep 2021 12:23:16 -0700 Subject: [PATCH 2/2] Update the model when returning cached choices --- .../javarosa/core/model/ItemsetBinding.java | 37 ++++++++++++++++--- .../core/model/SelectOneChoiceFilterTest.java | 1 + 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/ItemsetBinding.java b/src/main/java/org/javarosa/core/model/ItemsetBinding.java index 97daa5a2e..dbf2122f9 100644 --- a/src/main/java/org/javarosa/core/model/ItemsetBinding.java +++ b/src/main/java/org/javarosa/core/model/ItemsetBinding.java @@ -92,6 +92,8 @@ public List getChoices(FormDef formDef, TreeReference curQRef) { // Return cached list if possible if (cachedFilteredChoiceList != null && allTriggerRefsBound && Objects.equals(currentTriggerValues, cachedTriggerValues) && Objects.equals(currentRandomizeSeed, cachedRandomizeSeed)) { + updateQuestionAnswerInModel(formDef, curQRef); + return randomize && cachedRandomizeSeed == null ? shuffle(cachedFilteredChoiceList) : cachedFilteredChoiceList; } @@ -116,10 +118,10 @@ public List getChoices(FormDef formDef, TreeReference curQRef) { Map selectChoicesForAnswer = initializeAnswerMap(formDef, curQRef); - List choices = new ArrayList<>(); + List filteredChoiceList = new ArrayList<>(); for (int i = 0; i < filteredItemReferences.size(); i++) { SelectChoice choice = getChoiceForTreeReference(formDef, formInstance, i, filteredItemReferences.get(i)); - choices.add(choice); + filteredChoiceList.add(choice); if (selectChoicesForAnswer != null && selectChoicesForAnswer.containsKey(choice.getValue())) { // Keys with values that don't get set here will have null values and must be filtered out of the answer. selectChoicesForAnswer.put(choice.getValue(), choice); @@ -128,13 +130,13 @@ public List getChoices(FormDef formDef, TreeReference curQRef) { updateQuestionAnswerInModel(formDef, curQRef, selectChoicesForAnswer); - cachedFilteredChoiceList = randomize ? shuffle(choices, currentRandomizeSeed) : choices; + cachedFilteredChoiceList = randomize ? shuffle(filteredChoiceList, currentRandomizeSeed) : filteredChoiceList; // TODO: write a test that fails if this is removed. It looks like a no-op because it's not accessing the shuffled collection. if (randomize) { // Match indices to new positions - for (int i = 0; i < choices.size(); i++) - choices.get(i).setIndex(i); + for (int i = 0; i < filteredChoiceList.size(); i++) + filteredChoiceList.get(i).setIndex(i); } //init localization @@ -202,7 +204,7 @@ private SelectChoice getChoiceForTreeReference(FormDef formDef, DataInstance for } /** - * Build a map with keys for each value in the current answer, each mapped to null. + * Builds a map with keys for each value in the current answer, each mapped to null. * * When we iterate over the new filtered choice list, we will update the values in this map. Keys with null values * after this process will be removed from the answer. We will also use this map to bind selection(s) in the IAnswerData @@ -226,6 +228,12 @@ private Map initializeAnswerMap(FormDef formDef, TreeRefer return selectChoicesForAnswer; } + /** + * Removes any answers that aren't in the filtered choice list. Binds the answer to its choice(s) so that the answer + * label can be retrieved. + * + * SIDE EFFECTS: mutates the instance node's value (TreeElement.value, type {@link SelectOneData} or {@link MultipleItemsData}) + */ private void updateQuestionAnswerInModel(FormDef formDef, TreeReference curQRef, Map selectChoicesForAnswer) { IAnswerData originalValue = formDef.getMainInstance().resolveReference(curQRef).getValue(); @@ -244,6 +252,23 @@ private void updateQuestionAnswerInModel(FormDef formDef, TreeReference curQRef, } } + /** + * Updates the model using the previously-cached choice list. + * + * @see #updateQuestionAnswerInModel(FormDef, TreeReference, Map) for details and side-effects + */ + private void updateQuestionAnswerInModel(FormDef formDef, TreeReference curQRef) { + Map selectChoicesForAnswer = initializeAnswerMap(formDef, curQRef); + if (selectChoicesForAnswer != null) { + for (SelectChoice choice : cachedFilteredChoiceList) { + if (selectChoicesForAnswer.containsKey(choice.getValue())) { + selectChoicesForAnswer.put(choice.getValue(), choice); + } + } + } + updateQuestionAnswerInModel(formDef, curQRef, selectChoicesForAnswer); + } + /** * @param selections an answer to a multiple selection question * @param selectChoicesForAnswer maps each value that could be in @{code selections} to a SelectChoice if it should be bound diff --git a/src/test/java/org/javarosa/core/model/SelectOneChoiceFilterTest.java b/src/test/java/org/javarosa/core/model/SelectOneChoiceFilterTest.java index 817ff0deb..f6556d72c 100644 --- a/src/test/java/org/javarosa/core/model/SelectOneChoiceFilterTest.java +++ b/src/test/java/org/javarosa/core/model/SelectOneChoiceFilterTest.java @@ -131,6 +131,7 @@ public void clearingValueAtLevel1_ShouldClearValuesAtLevels2And3() { // If we set level2 to "aa", form validation passes. Currently, clearing a choice only updates filter expressions // that directly depend on it. With this form, we could force clearing the third level when the first level is cleared // by making the level3 filter expression in the form definition reference level1 AND level2. + scenario.answer("/data/level1", "b"); scenario.answer("/data/level2", "bb"); validate = scenario.getValidationOutcome();