repeat-groups are not respecting the jr:count setting after a change to a lower value

[mdudzinski]

Software versions

All JavaRosa versions

Problem description

It's reported in https://opendatakit.org/help/form-design/workarounds/ under "repeat-groups are not respecting the jr:count setting after a change".

Also reported in ODK Collect issues.

Steps to reproduce the problem

• Open a form that has a repeat group driven by the jr:count attribute which is a path to a node that holds the actual value.
• Go to the question referenced by jr:count attribute and put any value. For example 5.
• Go to the 5th repeat group instance.
• Go back to the question referenced by jr:count attribute and put any value less that previously entered. For example 3.
• Go back to the 4th or the 5th repeat group instance.

Expected behavior

• The 4th and 5th repeat group instances should be deleted.

Other information

It seems like repeat group instances are created inside FormEntryModel.createModelIfNecessary(FormIndex index) method. The following snippet

if (index.getTerminal().getInstanceIndex() < fullcount) {
   try {
     getForm().createNewRepeat(index);
    } catch (InvalidReferenceException ire) {
      ire.printStackTrace();
      throw new RuntimeException("Invalid Reference while creting new repeat!" + ire.getMessage());
    }
 }

checks if amount of repeat section instances is less than the jr:count value (fullcount). If it is less, a new repeat group instance is created.

The above code runs every time FormEntryController.jumpToIndex(FormIndex index) is called so mostly upon FormEntryController#stepToNextEvent() and FormEntryController.stepToPreviousEvent(). This means a new repeat instance is created every time a user navigates to it.

I think it should be quite easy to remove extraneous repeat group instances in similar manner. However, if the user jumps over the repeat group, this code will not be called. I think it can be fixed by an additional code that traverses the whole form instance before form instance is saved. I believe that the IDag.validate(FormEntryController formEntryControllerToBeValidated, boolean markCompleted) may do the trick.

[lognaturel]

@mdudzinski Thanks for this thorough analysis! A pull request would be fantastic. I haven't carefully thought through the proposed approach yet, did you want some feedback before implementing?

[mdudzinski]

@lognaturel actually I did the analysis to make it easier for somebody else because I didn't have enough time to work on this. However, if no one wants (or has no time) to work on this, I may take this soon. I'd appreciate any feedback I can get. Thanks!

[mdudzinski]

@lognaturel You can assign this to me. Thanks.

[lognaturel]

Awesome, thanks so much! I've got some time blocked off for JavaRosa today so I'll read through your approach and give you any feedback I may have.

[lognaturel]

A minimal form to verify this behavior: counting-repeats.txt

I think the big question to ask first is whether we do want repeats to disappear in this way. It could potentially mean significant data loss if someone accidentally changed the value to 1 instead of 10, for example. @yanokwa?

We definitely do need to think carefully about how repeats are shown to users. One idea that's been floating around is to change the interface so that users see a list of repeats and have a chance to add more or remove existing ones. That way they wouldn't have to deal with counts at all and could explicitly choose which repeats to delete and get a chance to confirm deletion. That's a bigger change but if we can line ourselves up to do that soon, then maybe we don't want to do this fix?

[mdudzinski]

I believe the idea will work better with unlimited repeat groups and can be achieved with no changes in JavaRosa core. Of course it's possible to display all repeat groups on a single screen regardless if they are limited or unlimited but the option to add/delete should be visible only for unlimited ones. Because you cannot actually delete a group instance from a limited repeat group. It will just wipe out the data but will leave an empty repeat group anyway.

If a limited group is driven by an answer then amount of its group instances should match the answer so I still think it should just delete the last rows when changed to a lower value. Ideally, there should be a confirmation dialog to avoid accidental data loss but this sounds like something that should be tackled on the UI side.

[lognaturel]

Sounds good, @mdudzinski, let's see how this works out.

[mdudzinski]

@lognaturel I opened a Pull Request for this issue (#61). I included two unit tests but we'll be testing this internally and I'll let you know once it passes our QA. In mean time, I'll be happy to hear any feedback on the PR. Thanks in advance!

[getodk-bot]

ERROR: This issue is in progress, but has no assignee.

[getodk-bot]

ERROR: This issue is in progress, but has no assignee.

[getodk-bot]

Hello @mdudzinski, you have been unassigned from this issue because you have not updated this issue or any referenced pull requests for over 10 days.

You can reclaim this issue or claim any other issue by commenting @opendatakit-bot claim on that issue.

Thanks for your contributions, and hope to see you again soon!