Always add a concrete repeat instance to nested repeats

[dimwight]

Addresses Collect #4059

What has been done to verify that this works as intended?

Verified in Collect as described in (closed) #4797. Tests FormNavigationTestCase and TriggerableDagTest have been updated to match the improvement.

Why is this the best possible solution? Were any other approaches considered?

Offers a straightforward resolution of the issue raised, there's no evident fix in Collect.

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?

Now behaves as a user would naturally expect; code changes have been deliberately isolated (see below) to avoid regression risk.

Do we need any specific form for testing your changes? If so, please attach one.

Defined in NestedRepeats.xlsx (derived from @seadowg form in #4059)

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No update needed?

The issue

Tracing the state of the form instance as repeats are added shows that the key code is in JR. The initial instance is created with nested repeat(s), but subsequent repeats are empty to start with.

Stepping through adding a repeat shows that in FormDef the template tree acquired in createNewRepeat has the appropriate members. However when this is passed to FormInstance.copyNode, TreeElement.deepCopy fails to include them because their multiplicities are all INDEX_TEMPLATE ie -2. Even if it did copy them, their multiplicities would stop them being indexable.

The fix

The obvious solution is to copy the whole template and set multiplicities to DEFAULT_MULTIPLICITY ie 0. This will be correct for all nested repeats and the appropriate value for the repeat root is set in copyNode.

Both copyNode and deepCopy have multiple call sites, so modifying either will most likely result in regression.

However in createNewRepeat we can pass to copyNode a clone of the template with adjusted multiplicities, created using an analogue in TreeElement of deepCopy. (The subsequent copying in copyNode will then be harmlessly redundant.)

This small change has the desired effect and should be safe from regression

[lognaturel]

There are 3 inner repeats so the count must reflect that. Now the counts look like they're always off by 1. Is there something I'm missing?

[dimwight]

Thanks @lognaturel for sparing the time to look at this.
I'm very aware of my limited understanding of how these results come about, but I'm guessing it has something to do with the creation under the updated code of nested repeats that are missing under the current code, which is what the original issue pointed out.

(On re-reading this it looks a bit sarcastic, but I'm genuinely uncertain what's going on.)

[dimwight]

To confess to my actual working method, I
- traced the code until it was clear where the original Collect test was failing
- played with the code until the test passed
- looked for JR tests that failed
- played with their code until they in turn passed

[lognaturel]

No worries, this is thorny stuff! Thanks again for taking a deep dive and I see my message came out more curt than I intended. The limitations of Github communication.

I hadn't carefully read the issue as my nonsense response about pyxform there indicates. I've read it again and now I'm thinking that it may not be a bug at all. I'm still on maternity leave with unpredictable availability but I'll keep this kicking around my brain and will hopefully have some thoughts to share soon.

[dimwight]

If not strictly a bug, then perhaps a quirk that contravenes the principle of least surprise? @seadowg's point has validity - the current behaviour is arguably inconsistent, even illogical. And my proposed change is so isolated as hardly to create a regression risk.