Collect doesn't properly display fields after repeat group

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Fill out the attached sample form.
2. Save but don't finalize.
3. Go back in to edit.

What is the expected output? What do you see instead?

The final field of the form ("conclusion") should display in the field list. 
However, it is actually being hidden within the last element of the final 
repeat group. (If you drill down into the last element of the last repeat 
group, you will find it there.) This is a more general problem: if you have 
lots of fields after that last repeat group, they all appear inside the last 
element of the repeat group, instead of in the level above.

The same issue exists if you use the "go to prompt" interface to navigate 
within the form: fields after that last repeat group are hidden under the last 
element.

I have not tried to trace this problem. Perhaps it was introduced recently? I 
don't remember seeing it before.

Original issue reported on code.google.com by chrislro...@gmail.com on 15 May 2013 at 10:24

Attachments:

• SampleRostersForm3.xls

[GoogleCodeExporter]

What version of ODK Collect are you using?

This is a bad bug. Tagged for fixing before the next release

Original comment by mitchellsundt@gmail.com on 4 Jun 2013 at 8:11

• Added labels: Collect, Collect-1.3.next, Priority-High
• Removed labels: Priority-Medium

[GoogleCodeExporter]

Well, we discovered this in our branch, but our codebase is current as of 1-2 
weeks ago. I'm pretty sure that I also verified with the version of Collect 
currently in Google Play. Yes, well, I just re-verified in ODK Collect 1.3 
(1030).

Original comment by chrislro...@gmail.com on 4 Jun 2013 at 8:21

[GoogleCodeExporter]

You have any idea where/how to fix this? We can investigate more if necessary. 
Nobody has complained yet, but now with indexed-repeat() everybody is using 
lots of repeats, and so I figure it's inevitable that it comes up as a big 
issue for somebody.

Original comment by chrislro...@gmail.com on 19 Jun 2013 at 8:38

[GoogleCodeExporter]

(I have a note in our issue system saying that it's probably in 
FormHierarchyActivity.java. I guess that's as far as we've gotten in tracing 
it.)

Original comment by chrislro...@gmail.com on 19 Jun 2013 at 8:40

[GoogleCodeExporter]

The FormHierarchy is built by traversing the form and looking for events of 
type Question, Repeat, Group, etc.. If memory serves, it used to be that for 
each repeat you'd see the repeat event twice, once at the start and once at the 
end.  It appears that only the beginning Repeat event is happening now.  (it 
may be that we only ever tested forms that had a Repeat_Prompt event at the end 
of the repeating group rather than a pre-defined number of repeats and we 
missed this case).

The reason the last question (and probably any future questions until the next 
Repeat or End_of_form event) appear in that last repeat is that there isn't an 
event that 'closes' the repeat.  (this problem will also exist mid-form if you 
have a repeating group with a pre-defined number of repeats)

Not sure what a good fix woud be.  You could probably do something like once 
you hit a repeat, start checking the xpath reference of each question to make 
sure the string contains the full repeat path, and end the repeat if not.  
Though, ideally there'd be an event of some kind indicating the close of the 
repeat.

Original comment by carlhart...@gmail.com on 19 Jun 2013 at 2:09

[GoogleCodeExporter]

Carl, thanks for the guidance. I think that the issue was, as you suggested, 
repeat groups without a REPEAT_PROMPT event at the end. The fix was easy 
enough: as you suggested, simply checking to see if we're still inside the 
relevant repeat when processing new questions. Here is a clip from 
FormHierarchyActivity.java:

        event_search: while (event != FormEntryController.EVENT_END_OF_FORM) {
            switch (event) {
                case FormEntryController.EVENT_QUESTION:
                    // because we may not see an EVENT_PROMPT_NEW_REPEAT for all repeat groups,
                    // first see if we've exited the context of a repeat
                    String currentRef=formController.getFormIndex().getReference().toString(false);
                    if (!enclosingGroupRef.equalsIgnoreCase("") && !currentRef.startsWith(enclosingGroupRef)) {
                        // We were displaying a set of questions inside of a repeated group. We
                        // passed the end of that group, so we want to stop.
                        break event_search;
                    }
                    if (!repeatedGroupRef.equalsIgnoreCase("") && !currentRef.startsWith(repeatedGroupRef)) {
                        // We passed the end of the current repeating group, so we reset the
                        // repeatedGroupName variable
                        repeatedGroupRef = "";
                    }

                    if (!repeatedGroupRef.equalsIgnoreCase("")) {
                        // We're in a repeating group, so skip this question and move to the next
                        // index.
                        event =
                                formController.stepToNextEvent(FormController.STEP_INTO_GROUP);
                        continue;
                    }

                    FormEntryPrompt fp = formController.getQuestionPrompt();
                    String label = fp.getLongText();
                    if ( !fp.isReadOnly() || (label != null && label.length() > 0) ) {
                        // show the question if it is an editable field.
                        // or if it is read-only and the label is not blank.
                        formList.add(new HierarchyElement(fp.getLongText(), fp.getAnswerText(), null,
                                Color.WHITE, QUESTION, fp.getIndex()));
                    }
                    break;

The bits at the beginning are new, to check if we've left the current repeat 
context. Does that look like a good and safe fix to you? So far, it seems to 
work well, but I haven't put it through our full QA process yet.

Original comment by chrislro...@gmail.com on 20 Jun 2013 at 6:02

[GoogleCodeExporter]

It looks like there are still cases that are not handled properly. At least 
there is one that I have just discovered: if you have begin repeat, end repeat, 
then begin repeat and end repeat again, the second repeat is showing as INSIDE 
the first when you click on Go To Prompt. If you stick a normal field in 
between the two repeat groups, it's fine. But if the two repeat groups are 
adjoining, then the second shows as inside the first.

Original comment by chrislro...@gmail.com on 29 Aug 2013 at 5:40

[GoogleCodeExporter]

A more complete fix will be in the next release.

Original comment by mitchellsundt@gmail.com on 14 Mar 2014 at 12:21

• Added labels: Mar2014
• Removed labels: Collect-1.3.next

[GoogleCodeExporter]

Fix is in 1.4.3

Original comment by mitchellsundt@gmail.com on 19 May 2014 at 4:24

• Changed state: Fixed

[GoogleCodeExporter]

We actually merged your more complete fix and our latest release now has the 
problem that I mentioned in my Aug. 29 comment. We are going to revert to my 
original proposed fix, which seemed to handle all known cases. Alternatively, 
if we have a better fix for your fix, we will post it here.

Original comment by chrislro...@gmail.com on 1 Sep 2014 at 11:10

[GoogleCodeExporter]

OK. Thanks. Reopening.

If you have a test form you're using, please attach it. No ETA on when I will 
cycle back to this.

Original comment by mitchellsundt@gmail.com on 2 Sep 2014 at 3:56

• Changed state: Accepted

[GoogleCodeExporter]

Our nested roster form attached, which exhibits the problem because of 
adjoining/nesting repeats.

Original comment by chrislro...@gmail.com on 2 Sep 2014 at 5:30

Attachments:

• Sample-Rosters-NestedRepeats.xlsx

[GoogleCodeExporter]

Our proposed fix is to change lines 255-264 of FormHierarchyActivity from:

<code language="java">
        if (!currentRef.startsWith(curGroup)) {
            // We have left the current group
            if ( repeatGroupRef == null ) {
                // We are done.
                break event_search;
            } else {
                // exit the inner repeat group
                repeatGroupRef = null;
            }
        }
</code>

...to...

<code language="java">
        if (!currentRef.startsWith(curGroup)) {
            // We have left the current group
            if ( repeatGroupRef == null ) {
                // We are done.
                break event_search;
            } else {
                // exit the inner repeat group
                repeatGroupRef = null;

                if (!currentRef.startsWith(contextGroupRef)) {
                    break event_search;
                }
            }
        }
<code>

Original comment by m.margar...@synergo.gr on 15 Sep 2014 at 1:57