603: Always generate secondary instance for selects

[lindsay-stevens]

Closes enketo/enketo#216

Interesting git history from 2012: a change to itemsets for everything, then reverted the next day for backwards compatibility.

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

Discussed in enketo/enketo#216 and references. Lots of changes required but generally explained in commit messages or new docstrings.

What are the regression risks?

Risk that some XForm consuming apps may rely on selects not always generating itemsets. Risk that since there are a lot of code changes there may be bugs, although tests were added / updated.

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

It's not a user-facing so probably not.

Before submitting this PR, please make sure you have:

• [x] included test cases for core behavior and edge cases in tests
• [x] run nosetests and verified all tests pass
• [x] run black pyxform tests to format code
• [x] verified that any code or assets from external sources are properly credited in comments

[lognaturel]

Interesting git history from 2012: a change to itemsets for everything, then reverted the next day for backwards compatibility.

There are three reasons we've discussed:

• Enketo's support for secondary instances was limited at the time (https://github.com/XLSForm/pyxform/issues/203#issuecomment-391832736). I can't think of a client or server that wouldn't support them fully now.
• Some folks have analysis pipelines that depend on internal choices. This might be painful but it's hard to estimate the impact.
• Injecting an other choice in a secondary instances to support or_other feels very weird. That said, it won't look any different from an XLSForm user's perspective. The longer-term goal is to support proper open selects (https://github.com/XLSForm/pyxform/issues/203#issuecomment-541936512)

Of those, I think the second point has the highest risk. We've had a post on the forum announcing this change for a year, though, so hopefully folks building downstream tools have had time to adapt.

Have you looked at or_other yet? I think for now it makes sense to inject an other choice. I haven't looked -- is that what the 2012 implementation did?

[tiritea]

sorry, I only just noticed this the other day (I get spammed by so many git updates I tend to tune them out... :-( Would it be possible to add a commandline option to continue to generate 'inline' select option when desired? I do actually currently rely on them because I havent completed parsing secondary instances in my (iOS) XForms parser yet. And given it is such a breaking change, it might still be nice to still support it (as deprecated), even if you have you now explicitly enable it via said option. Otherwise I guess I'll have to fork the current version and use that in the interim.

[lindsay-stevens]

@lognaturel there's a few more translations test to update, then the last implementation thing to look at should be or_other. The search() appearance now generates an itemset - unrelated to this PR (before and after), search() works in Collect but not Enketo.

[lognaturel]

More conversation with @tiritea on the forum.

The search() appearance now generates an itemset - unrelated to this PR (before and after)

Really. And not an itemset AND static choices, only an itemset? I could have sworn I checked that as part of my comment long ago. Well, that makes that easy at least.

search() works in Collect but not Enketo.

Yes, that's right.

[lindsay-stevens]

@lognaturel I meant:

1) The search() appearance now generates an itemset (due to this PR) 2) unrelated to this PR (before and after), search() works in Collect but not Enketo.

[lognaturel]

Got it, thanks!

[tiritea]

FWIW I dont support search() yet, either, so please dont do anything funky here on my account... (I'll deal with all that after I get secondary instances working).

[lindsay-stevens]

@lognaturel ready for review. It's covering the cases mentioned in enketo/enketo#216, and any tests that failed as a result of code changes. I haven't combed through every test case though, e.g. in case there's some way to avoid generating itemsets.

[lognaturel]

Finally spending some focused time on this after speaking to @lindsay-stevens about it at the ODK Summit. We would like to release this by mid-August at the latest.

My review checklist:

• [x] Verify impact on some real forms
  • WHO VA: 990kb before, 593kb after (va_before_after.zip). Tried on client, no obvious issues. Central doesn't flag any schema change. Skimmed diff which looks as expected.
  • All widgets: 44kb before, 35kb after. I quickly skimmed through the form in Collect and Enketo. In Enketo:

Thing	Before	After
Likert
Selects with media

Haven't looked into what's going on yet. Ideally pyxform output could be modified slightly to not get those differences but it might not be possible.

• [x] or_other
• [x] translations
  • [x] has code to generate translation names been removed? No, still relevant for media
• [x] labels that use values from form
  • An example. I don't think there's much reason to do this anymore now that we have selects from repeats but maybe people do things like get dynamic values from unrolled repeats or things like that. I verified that Collect and Enketo are happy with <output>s in itext.
• [x] "skip adding choice lists that aren't used in the xform to avoid extra unnecessary data. Maybe worth a warning or maybe not." in https://github.com/XLSForm/pyxform/pull/614/commits/1238bd6acaa10968b0881f174830878a88d16936, This is existing behavior. I've filed https://github.com/XLSForm/pyxform/issues/647 for consideration. @lindsay-stevens I'm interested in what you think.
• [x] media, long/short forms - Lots of tests with image and big-image on choices. No support for long/short forms yet.

[lognaturel]

Ok, @lindsay-stevens, I'm ready to merge once the couple of things above are addressed! The only real blocker for me is that I'm concerned dropping output_context won't work in every case. I haven't come up with a failing example yet but it'd be helpful to at least understand that chunk of code a bit better. I'd also like to at least try to get the Enketo formatting looking ok.

[lindsay-stevens]

@lognaturel thanks for the review! I have responded to the review comments.

About the Enketo issue, I don't see where the pyxform output formatting could cause this. There's no spacing added to the output e.g. near the jr://images/ values, or the coded values in the itemsets. Maybe there is a different code path in Enketo for looking up labels via jr:itext(itextId) + itemset vs. directly e.g. jr:itext('/widgets/grid_test/a:label')"? If Collect is not doing the spacing thing then that also points to an Enketo issue.

[lognaturel]

I have filed Enketo issues: https://github.com/enketo/enketo/issues/29, https://github.com/enketo/enketo/issues/28

The first is bad but I don't think it's bad enough to block merge. We can communicate the issues and encourage platforms that use pyxform and Enketo to update both at the same time.

We will release this as pyxform 2.0 because there is a compatibility/interface change.

[lognaturel]

We've identified a number of Enketo bugs as we have done quality assurance on this change. We're holding release until we can resolve those.

[lognaturel]

All Enketo bugs we know about are fixed! 🎉

But now a user has reported this strange error. I can reproduce it on master and can't reproduce on v1.12.1. @lindsay-stevens could you please take a look?

[lindsay-stevens]

@lognaturel Great news about Enketo! I have opened a new PR to fix this error, which occurs when using or_other + translations + group or repeat.