Skip to content

fix: error when a 'required' value is used on a group or repeat - #877

Open
cpruijsen wants to merge 2 commits into
XLSForm:masterfrom
cpruijsen:fix/issue-856-0e157d6a
Open

cpruijsen wants to merge 2 commits into
XLSForm:masterfrom
cpruijsen:fix/issue-856-0e157d6a

Conversation

@cpruijsen

Copy link
Copy Markdown

Title: fix: error when a 'required' value is used on a group or repeat

Closes #856

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

Conversion now raises an error when a group, repeat or loop has a `required` value. Before, a `required` value on a `begin group` row was written as `<bind nodeset="/data/mygroup" required="true()"/>`, which Collect and Enketo ignore: unlike `relevant`, it does not apply to the questions in the group.

There are two checks:

- `workbook_to_json` in `pyxform/xls2json.py` raises the new `SURVEY_011` error, with the row number, for a `begin group`, `begin repeat` or `begin loop` row that has a `required` value.
- `Section.validate()` in `pyxform/section.py` rejects a `required` bind on a group, repeat or loop. This covers input that does not go through the XLSForm reader (`create_survey_element_from_dict`, `xform2json`).

An explicit false value (`no`, `false`, `false()` and the other false values in `aliases.yes_no`) is still accepted, because it did not produce a misleading bind. Any other value, including an XPath expression, is rejected.

Judgement calls:
- Repeats and loops are rejected too, because they produce the same ignored bind. Limiting this to groups is one condition in each check.
- If you prefer only the XLSForm check, the `section.py` change and `test_group_required_bind__error` can be removed on their own.
- `required_message` on a group is unchanged.

#### What are the regression risks?

Forms that set `required` on a group, repeat or loop to anything other than an explicit false value now fail conversion, as the issue asks. Other forms are unaffected, because the checks only reject input.

#### Does this change require updates to documentation? If so, please file an issue [here](https://github.com/XLSForm/xlsform.github.io) and include the link below.

No. The XLSForm documentation does not list `required` as a group setting.

#### Before submitting this PR, please make sure you have:
- [x] included test cases for core behavior and edge cases in `tests`
- [x] run `python -m unittest` and verified all tests pass
- [x] run `ruff format pyxform tests` and `ruff check pyxform tests` to lint code
- [x] verified that any code or assets from external sources are properly credited in comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant