Merge pull request #646 from lognaturel/issue-642

Update the model even when a cached choice list can be used

src/main/java/org/javarosa/core/model/ItemsetBinding.java

@@ -92,6 +92,8 @@ public List<SelectChoice> 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<SelectChoice> getChoices(FormDef formDef, TreeReference curQRef) {
 
         Map<String, SelectChoice> selectChoicesForAnswer = initializeAnswerMap(formDef, curQRef);
 
-        List<SelectChoice> choices = new ArrayList<>();
+        List<SelectChoice> 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<SelectChoice> 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<String, SelectChoice> 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<String, SelectChoice> 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<String, SelectChoice> 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

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();

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
 }