Output empty `<label/>` instead of omitting it. #543

lindsay-stevens · 2021-07-05T09:32:15Z

as per Output empty <label/> instead of omitting it. #439
pyxform should always output a label node even if there is no label specified.
pyxform had a warning for no label but this is removed since there is no requirement for a label.
added tests to verify that a label is output and no warning raised
fixed existing tests that either expected a warning or no label

Closes #439

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

As discussed in the issue ticket, the XForms spec requires a label node. It would not be feasible to change the spec, so this solution brings pyxform output in to line with the spec.

What are the regression risks?

It's possible that existing users expect there to be a warning if no label is supplied but a hint is supplied. So the regression risk would be in other apps that use pyxform. In this case the other app could add a check for empty labels.

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

The xlsform.org site says that the survey sheet label is mandatory. However, the update here is specific to pyxform behaviour. It would not be accurate to say that the label is not mandatory, because it is. It's just that for pyxform, in the absence of a label it will emit one in order to comply with the spec.

Before submitting this PR, please make sure you have:

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

- as per pyxform/XLSForm#439, should always output a label node even if there is no label specified. - pyxform had a warning for no label but this is removed since there is no requirement for a label. - add tests to verify that a label is output and no warning raised - fix existing tests that either expected a warning or no label

lognaturel · 2021-07-07T20:28:18Z

From @MartijnR at #439 (comment):

If ODK Validate does not get fooled by an empty label (and still requires either a label or hint text content)

The PR description doesn't address this that I can see -- will Validate still throw an error if there's an empty label and no hint? That would be the desired behavior.

lognaturel · 2021-07-08T04:34:34Z

I've come back to try this, and I do confirm that there is still a pyxform error thrown if neither label nor hint is present which is what we want. 👍

I went straight to tests to try to see if that was covered and didn't see it because the forms aren't inline. Also, I now see that test_row_without_label_or_calculation_throws_error already existed and remains which is great.

pyxform/tests_v1/test_guidance_hint.py

pyxform/tests_v1/test_sheet_columns.py

lindsay-stevens · 2021-07-08T05:06:01Z

will Validate still throw an error if there's an empty label and no hint?

For this case there is an existing test: in test_sheet_columns.InvalidSurveyColumnsTests.test_missing_label:

pyxform/pyxform/tests_v1/test_sheet_columns.py

Line 36 in 6bb547b

def test_missing_label(self):

For reference, the two e2e spreadsheets look like this:
hint_with_no_label_column:

hint_with_no_label_value:

- some clients hide the guidance hint by default, in which case the user would see nothing / a small (i) symbol if a question had only a guidance hint without any label (or media) or hint - update test_guidance_hint.py tests accordingly - change newly added test from ss_structure to md-style - removed e2e tests and associated files; more or less the same as the test immediately above in test_sheet_columns.py

codecov-commenter · 2021-07-27T10:02:41Z

Codecov Report

Merging #543 (a0d3f7c) into master (cf7ba39) will decrease coverage by 0.03%.
The diff coverage is 81.25%.

@@            Coverage Diff             @@
##           master     #543      +/-   ##
==========================================
- Coverage   83.88%   83.85%   -0.04%     
==========================================
  Files          25       25              
  Lines        3736     3747      +11     
  Branches      872      875       +3     
==========================================
+ Hits         3134     3142       +8     
- Misses        455      457       +2     
- Partials      147      148       +1

Impacted Files	Coverage Δ
pyxform/survey_element.py	`94.07% <76.92%> (-1.00%)`	⬇️
pyxform/xls2json.py	`78.56% <100.00%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf7ba39...a0d3f7c. Read the comment docs.

pyxform/survey_element.py

lognaturel · 2021-07-27T22:05:57Z

pyxform/xls2json.py

-        if (
-            constants.LABEL not in row
-            and row.get(constants.MEDIA) is None
-            and question_type not in aliases.label_optional_types


Are there tests for this case and the two below? (I'm pretty sure there are but would be good to confirm that you identified them)

This was for warnings, not for errors. It is related to what @yanokwa was doing at #542 so we can leave it in here and then look at #542 separately.

Restored this but in a slightly different location. #542 can still add in the new check condition, and add the tests. Please see commit msg for 3e4a84e

- label check block applies to questions and other item types such as groups and repeats, so may be valid in other scenarios - for example this block is referenced in XLSForm#542 - will require further changes to exclude question items from the check since that is the intent in XLSForm#439

- as noted in the comment, the check doesn't apply to questions anymore since PR XLSForm#543 makes pyxform always emit a label, and the only case that is not permissible is a guidance_hint with no label or hint. - nesting this check inside the "control group" processing branch seems the only neat+easy way about this, since: - the context is a loop from which the various branches continue, so it can't check at the end, - each branch parses / determines the actual item type which is not apparent in the raw "type" data from the row dict, and - after the loop context exits, the data is in an array with nested dicts so it's no longer possible to determine the sheet row number for the error message to be as useful to users. - refactored an old test which covers a lot of question types, into the tests_v1 / md style since this test is affected by the changes and having md style is more transparent - added some tests to verify the assumption that for the label check, it's not going to encounter a None or empty value for "control_type".

lindsay-stevens · 2021-07-28T11:10:40Z

Hi @lognaturel, resolving this turned out to be not as simple as restoring the check code block. When it was restored, a couple of tests needed to be updated. In particular there is an old-style test with a large "warnings.xls" fixture, which is now refactored into markdown. This should make it clearer when the label warning will and will not be raised. From here, #542 could add in the new condition to exclude warnings for field-list groups, and add the new tests. More context / info in the 3e4a84e commit message.

lognaturel

Thank you!

- as noted in the comment, the check doesn't apply to questions anymore since PR XLSForm#543 makes pyxform always emit a label, and the only case that is not permissible is a guidance_hint with no label or hint. - nesting this check inside the "control group" processing branch seems the only neat+easy way about this, since: - the context is a loop from which the various branches continue, so it can't check at the end, - each branch parses / determines the actual item type which is not apparent in the raw "type" data from the row dict, and - after the loop context exits, the data is in an array with nested dicts so it's no longer possible to determine the sheet row number for the error message to be as useful to users. - refactored an old test which covers a lot of question types, into the tests_v1 / md style since this test is affected by the changes and having md style is more transparent - added some tests to verify the assumption that for the label check, it's not going to encounter a None or empty value for "control_type".

lindsay-stevens added 2 commits July 5, 2021 18:52

add: test case for ok label node output when no label column in xlsform

6bb547b

lindsay-stevens requested a review from yanokwa July 5, 2021 09:32

lognaturel reviewed Jul 8, 2021

View reviewed changes

pyxform/tests_v1/test_guidance_hint.py Show resolved Hide resolved

pyxform/tests_v1/test_sheet_columns.py Show resolved Hide resolved

lindsay-stevens added 2 commits July 27, 2021 19:12

Merge branch 'master' into pyxform-439

4123379

lognaturel reviewed Jul 27, 2021

View reviewed changes

pyxform/survey_element.py Show resolved Hide resolved

lognaturel reviewed Jul 27, 2021

View reviewed changes

lindsay-stevens added 4 commits July 28, 2021 17:33

dev: black formatting

e91a176

chg: clarify test names

a0d3f7c

lognaturel approved these changes Jul 28, 2021

View reviewed changes

lognaturel merged commit 2e631b1 into XLSForm:master Jul 28, 2021

lindsay-stevens deleted the pyxform-439 branch July 29, 2021 03:20

jkuester mentioned this pull request Jun 6, 2022

Update with upstream changes from XLSForm/pyxform medic/pyxform#8

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Output empty `<label/>` instead of omitting it. #543

Output empty `<label/>` instead of omitting it. #543

lindsay-stevens commented Jul 5, 2021 •

edited

Loading

lognaturel commented Jul 7, 2021

lognaturel commented Jul 8, 2021 •

edited

Loading

lindsay-stevens commented Jul 8, 2021

codecov-commenter commented Jul 27, 2021 •

edited

Loading

lognaturel Jul 27, 2021 •

edited

Loading

lognaturel Jul 27, 2021

lindsay-stevens Jul 28, 2021

lindsay-stevens commented Jul 28, 2021

lognaturel left a comment

Output empty <label/> instead of omitting it. #543

Output empty <label/> instead of omitting it. #543

Conversation

lindsay-stevens commented Jul 5, 2021 • edited Loading

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

What are the regression risks?

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

Before submitting this PR, please make sure you have:

lognaturel commented Jul 7, 2021

lognaturel commented Jul 8, 2021 • edited Loading

lindsay-stevens commented Jul 8, 2021

codecov-commenter commented Jul 27, 2021 • edited Loading

Codecov Report

lognaturel Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

lognaturel Jul 27, 2021

Choose a reason for hiding this comment

lindsay-stevens Jul 28, 2021

Choose a reason for hiding this comment

lindsay-stevens commented Jul 28, 2021

lognaturel left a comment

Choose a reason for hiding this comment

Output empty `<label/>` instead of omitting it. #543

Output empty `<label/>` instead of omitting it. #543

lindsay-stevens commented Jul 5, 2021 •

edited

Loading

lognaturel commented Jul 8, 2021 •

edited

Loading

codecov-commenter commented Jul 27, 2021 •

edited

Loading

lognaturel Jul 27, 2021 •

edited

Loading