461: Add value and label params to select_.._from_file

lindsay-stevens commented 2 years ago

Closes #461

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

xls2json.py: Added value, label to allowed params for select_from_file question types.
question.py: Updated to use params (if any) as the default itemset_ref value. Other logic around dynamic labels etc then layers on top.
test_external_instances_for_selects.py:
- Added tests for select_from_file question types (csv or xml) without params, with params, with choice_filter, and both params and choice filter.
- Refactored existing select_one_external tests to check the same scenarios as above, though they only work with choice_filter anyway. Also replaced an end-to-end test for itemsets.csv generation that had associated XLSX/XML/CSV fixture files with a new md_table_to_workbook method since openpyxl can write files.
- Added test for parameter value check (see below) for both select_from_file question types (csv or xml) and various good / bad param values.

ODK Validate rejects certain characters for the value and label parameters. Presumably the ref value is placed into a XPath, and characters that are XPath operators cause a problem. The error from ODK Validate looks like the below.

>> Something broke the parser. See above for a hint.
org.javarosa.xpath.XPathException: XPath evaluation: Parse error in XPath path: [val*].
Bad node: org.javarosa.xpath.parser.ast.ASTNodeAbstractExpr@63e2203c

Since this isn't super clear, the parameter values are validated by pyxform with the regex ^[a-zA-Z_][a-zA-Z0-9\-_\.]*$. The error message is similar to other design issues, e.g.

[row : 2] Parameter 'value' has a value which is not valid. Values must begin with a letter or underscore. Subsequent characters can include letters, numbers, dashes, underscores, and periods.

What are the regression risks?

None are anticipated.

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

The main XLSForm.org page should be reworded, to clarify that either the default names can be used, or custom names can be used by specifying the new parameters.
The Reference Table for select_one_from_file and select_multiple_from_file should mention the available parameters in the notes (as well as randomize and seed).
PR for the above here: https://github.com/XLSForm/xlsform.github.io/pull/223

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 commented 2 years ago

I've updated Validate manually to https://github.com/getodk/validate/releases/tag/v1.17.0 and run nosetests without the new test erroring.

~so I think it should work. I'm not as confident as I'd like to be because I wasn't able to run the automatic Validate update script (something has changed there?) and I'm having trouble running tests in my IDE.~

With a clear head I fixed my environment and was able to both run the Validate update script and run tests from my IDE. Hopefully you can do the update in a commit here and then move forward, @lindsay-stevens.

lognaturel commented 2 years ago

I've taken a first look through and this looks great to me and thoroughly tested. I'll take a deeper dive asap. @MartijnR perhaps you'll be interested in a peek as well? I think you were particularly interested in this change at some point.

MartijnR commented 2 years ago

Thanks! Very cool! I played around converting some forms and wasn't able to break it.

XLSForm / pyxform