157 missing translations warning

[lindsay-stevens]

Closes #157

Adds a warning if it looks like a XLSForm is missing a translation on one or more translatable columns.

The originating issue was a test case concerning only survey label and media::image. This PR covers all translatable columns (per the Reference Table. On the survey sheet: label, hint, guidance_hint, media::image, media::audio, media::video, constraint_message, and required_message. On the choices sheet: label, media::image, media::audio, and media::video.

The warning message specifies the columns and languages which appear to be missing. The (unwrapped) template is:

"Missing translation column(s): there is no default_language set, and a "
"translation column was not found for the {cols_msg}. "
"To avoid unexpected form behaviour, specify a default_language in the "
"settings sheet, or add the missing translation columns."

Where {cols_msg} lists the columns (for either or both of survey and choices) separated by a semicolon and the languages separated by a comma, e.g. 'hint': 'English', 'French'; 'label': 'English'. It can be a bit difficult to read when there are a lot of missing columns / languages but I'm not sure of a better alternative, such as using list notation (square brackets) etc. However, presenting the warning like this seems preferable to emitting a separate warning for each column. Example:

Missing translation column(s): there is no default_language set, and a translation column was not found for the survey column(s) and language(s): 'constraint_message': 'default'; 'guidance_hint': 'default'; 'hint': 'default'; 'media::audio': 'default'; 'media::image': 'default'; 'media::video': 'default'; 'required_message': 'default' and choices column(s) and language(s): 'media::audio': 'default'; 'media::image': 'default'; 'media::video': 'default'. To avoid unexpected form behaviour, specify a default_language in the settings sheet, or add the missing translation columns.

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

It would have been a good outcome if there was clearly a way that pyxform could produce an XForm that avoided the issue that the warning is for. However there were not many tests for translations previously. This made it difficult to tell whether aspects of and differences in XLSForm input and XForm output were symptoms of the originating issue, or normal behaviour, or undiscovered bugs.

This PR includes a raft of relatively pedantic XPath tests for translations behaviour. There are probably still more permutations that don't have tests but this should be a good boost to test coverage. In test_translations.py there are some TODO comments which should be reviewed and converted to tickets if they represent genuine bugs, or removed if they note normal behaviour. Once the TODO's are resolved, maybe it would be possible for pyxform to emit an XForm such that the warning is not required.

What are the regression risks?

Some users may expect no warning for certain XLSForms with missing translation columns. Depending on how warnings are handled it may or may not be a problem. The changes in this PR are otherwise additive.

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

The guidance_hint column was missing from the XLSForm Reference Table but I've submitted a PR for that already.

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

[codecov-commenter]

Codecov Report

Merging #571 (eb6f0fe) into master (bb2497d) will increase coverage by 0.19%. The diff coverage is 87.32%.

@@            Coverage Diff             @@
##           master     #571      +/-   ##
==========================================
+ Coverage   83.59%   83.79%   +0.19%     
==========================================
  Files          25       26       +1     
  Lines        3761     3825      +64     
  Branches      930      952      +22     
==========================================
+ Hits         3144     3205      +61     
  Misses        474      474              
- Partials      143      146       +3     

Impacted Files	Coverage Δ
pyxform/xls2json.py	79.82% <80.00%> (+0.35%)	:arrow_up:
...m/validators/pyxform/missing_translations_check.py	86.20% <86.20%> (ø)
pyxform/aliases.py	100.00% <100.00%> (ø)
pyxform/constants.py	100.00% <100.00%> (ø)
pyxform/section.py	96.58% <100.00%> (+0.02%)	:arrow_up:
pyxform/survey.py	94.37% <0.00%> (+0.58%)	:arrow_up:

---

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 bb2497d...eb6f0fe. Read the comment docs.

[KeynesYouDigIt]

Pretty clean approach! I am still worried that _add_empty_translations in survey.py must be doing essentially the same work, but this leaves the separation of concerns much cleaner (maybe _add_empty_translations could be done here somehow and removed from survey.py?)

The added testing is pretty helpful!

[lindsay-stevens]

Hi @KeynesYouDigIt thanks for your review! Please let me know if any changes or further clarification are needed.

[KeynesYouDigIt]

@lindsay-stevens no im good, this all seems fine! I'm afraid I don't have official review permissions so someone else will have to do the official review.

[lindsay-stevens]

Hi @lognaturel would you be able to review?

[lognaturel]

Will do!

I believe dashes are filled in for labels because Validate requires a label for every language.

[lindsay-stevens]

Hi @lognaturel just a reminder about this - no worries if it's til the new year. Thanks!

[lindsay-stevens]

Hi @lognaturel happy new year! Just a reminder about this PR ready for review.

[lindsay-stevens]

Thanks for reviewing @lognaturel! I've replied to each comment, please let me know if any changes are needed at this stage (or via separate tickets).

[lindsay-stevens]

@lognaturel thanks for the feedback / discussion above. The outstanding items should all now be addressed, either by new commits or new tickets for follow up.