Use first repeat instance(s) for jr:choice-name choices #758
+214
−114
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lognaturel
commented
Apr 12, 2024
| @@ -16,6 +16,25 @@ | |||
|
|
|||
| package org.javarosa.core.model; | |||
|
|
|||
| import static java.util.Collections.emptyList; | |||
There was a problem hiding this comment.
I think I'm likely the one who messed up this order previously. 😬 My IDE should now enforce the order that's mostly consistent across existing files.
| @@ -117,4 +119,75 @@ public class ChoiceNameTest { | |||
| scenario.answer("/data/city", "grenoble"); | |||
| assertThat(scenario.answerOf("/data/city_name"), is(stringAnswer("Grenoble"))); | |||
| } | |||
|
|
|||
| @Test public void choiceNameCallWithIndexedRepeatAndStaticChoices_worksWithMultipleRepeats() throws IOException, XFormParser.ParseException { | |||
There was a problem hiding this comment.
This is not affected by the code change. It's there to document behavior that has always worked.
seadowg
requested changes
May 20, 2024
| // SurveyCTO: We need a absolute "ref" to populate the dynamic choices, | ||
| // like we do when we populate those at FormEntryPrompt (line 251). | ||
| // The "ref" here is ambiguous, so we need to make it concrete first. | ||
| // ref, the reference used to specify where choices are defined, could be an absolute |
There was a problem hiding this comment.
This IFunctionHandler implementation should be moved out to another file now we're touching it again.
seadowg
approved these changes
May 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #757
What has been done to verify that this works as intended?
Added tests
Why is this the best possible solution? Were any other approaches considered?
My initial instinct was that using a reference to a repeat nodeset rather than a specific repeat instance to specify where choices should be taken from is a form design issue. But then I realized that a standards-compliant XPath engine would use the first repeat instance(s) when given a nodeset reference where it needs a single node reference.
This doesn't work well with choice filters but I think that's ok because it also wouldn't have been possible before. My priority here is to ensure that XLSForms that used to work continue to work without modification.
My recommendation for writing these kinds of
choice-namecalls would be to make them relative from within the repeat instance rather than making it from outside.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
This only affects
choice-namecalls given a reference parameter that is a nodeset reference. I think the case in the test is the only one in which this kind of reference would make sense. There's also the analogous nested case and that's why there's a for loop to qualify every unboud reference level.Do we need any specific form for testing your changes? If so, please attach one.
https://docs.google.com/spreadsheets/d/1MZvF9ynsOdGMWeZF8YsjuoUK9E0t83LZMi5-RsBOEiI/edit#gid=1068911091
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.