Update validation for new Enketo version

[jkuester]

The new version of Enketo being added to cht-core as a part of medic/cht-core#6345 includes several changes that it is important to account for here in cht-conf. Also, there is some additional validation which may be useful, but might not be a good fit for cht-conf. One option would be to include this validation in some kind of separate utility such as jkuester/cht-upgrade-helper (which already has proof-of-concept implementations of several of these validations).

(For each of these, it would be possible to implement them in some kind of separate utility helper, but it seems to make the most sense to include them in cht-conf wherever possible. So, I have noted my recommendation on each for whether it seems better to do it in the existing convert-*-forms action or perhaps in a new validate-forms action (which could possibly provide some kind of report to the user). I have also ordered these below according to my estimation of their importance/usefulness.)

(Also, to see an example of the output from the upgrade helper, see the reports on this gist for the default and standard config provided in cht-core.)

• A. Warn when non-required numbers used in calculations (validate)
  • Very hard to avoid false-positives since you would have to actually parse the calculation logic to know if there is actually a problem
• B. Warn when non-relevant questions with default values (validate)
  • Very hard to avoid false-positives since you would have to actually parse the calculation logic to know if there is actually a problem
• Error when invalid xPath paths in form #479
  • Pretty difficult to detect all cases (especially those involving complex xPaths)
  • Should be possible, though, to handle simple paths (by far the most common) without false positives
  • Seems like this should be happening in pyxform. Before going too far here, it seems worth looking into why https://github.com/medic/pyxform is so far behind https://github.com/XLSForm/pyxform since maybe this issue (and many others) have been fixed....
    • Upgrading pyxform does not seem to address this issue (though it might be indirectly fixed via https://github.com/enketo/enketo-validate or https://github.com/getodk/validate, but neither of those projects are feasible for us to use without significant modifications...
    • I have logged Update with upstream changes from XLSForm/pyxform pyxform#8 for eventually upgrading pyxform anyway
• D. Error when runtime error (convert/validate)
  • Requires depending on cht-conf-test-harness
  • Would be useful to run during convert but might not be worth the performance hit... (maybe could toggle with a flag?)
  • Might be a little dangerous to run by default during convert since a failure to load in the test harness might be a false-negative....
• Error when note fields are required in form #494
  • Not hard to detect all cases
• height of pulldown select too small enketo/enketo#275
  • Not hard to detect all cases
  • Depends on running with latest Enketo
  • Would also be nice to deprecate db-object appearance (and db:{{contact_type}} type) in favor of select-contact.
• [x] G. Do not warn when converting form that has choices with images but no labels
  • This is an upstream issue in pyxform that is still present in the latest version.
  • I have logged Warning given when converting a form with choices option with image but no label XLSForm/pyxform#610
  • Should try and expand the NO_LABEL logic we have to see if we can apply that here.
    • Logged: Do not warn when converting form that has choices with images but no labels pyxform#9
    • Not going to try to implement at this time

Details

A. Warn when non-required numbers used in calculations

The value used for unanswered number questions in calculations has changed. Previously 0 would be given to the calculation logic, but the new version of the CHT follows the ODK spec and returns NaN as the value of an unanswered number question. This behavior change can break form logic that expects 0.

B. Warn when non-relevant questions with default values

The behavior of default values for non-relevant fields has changed. Previously, if a question with a default value was never relevant, its default value would be used for calculations and other form logic. Now, however, the value from a non-relevant field will always be empty, regardless of the default value. (Note that because of this Enketo issue it can appear that the default value is being used while filling out the form. But, when the form it saved, the value will be cleared and all the dependent logic will be recalculated.) So, questions with default values that might be non-relevant, but are used in other form logic should be reviewed.

C. Error when invalid xPath paths

Currently, if you manually specify an xPath path in your XLSForm (instead of the ${...} variable notation) there is no validation that the path is valid (that it points to a real field). There is no error at runtime, either, but the behavior has slightly changed in the newer versions of Enketo. Previously an invalid xPath path considered equal to the empty string. Now it is not. Either way, it would be really nice to detect these invalid xPath paths and throw an error. It would make it a lot easier for app-developers to debug issues and maintain form integrity.

D. Error when runtime error

The syntax for concat has always been pretty simple: separate the parameters by commas. However, in previous versions of Enketo (when concat functionality was handled by native code) it was possible for a concat function to still work even if some comma's were missing. Now, config like that will cause an error when a user tried to open a form. It would be very helpful to be able to detect this and other runtime errors before deployment.

E. Error when note fields are required

Setting a note field to be required has always been bad data, but in previous versions of Enketo, this was just silently ignored. Now, this config will prevent the user from continuing to the next page and completing the form. cht-core has been updated, as a part of these changes: medic/cht-core#7256) to automatically remove the required designation from note fields, but best practice would still have us identify these improperly configured notes when compiling the form with cht-conf and produce an error so that the user knows to update the config. See this script as an example of regex for finding required notes and this xsl code which is how cht-core is removing the required designation.

F. Warn when using horizontal appearance instead of columns

The horizontal and horizontal-compact appearances are deprecated in favor of columns. It would be nice to provide a warning with a link to the columns documentation.

G. Do not warn when converting form that has choices with images but no labels

The new enketo supports the no-buttons appearance which allows for select fields for choices that have images, but no labels. (See the grid_test and grid_2_columns fields on the Enketo Widgets Demo Form for an example of what this looks like.) Annoyingly, pyxform will log a warning about any choice that does not have a label. It would be nice to see if we could find a way around this warning, perhaps by leveraging the NO_LABEL functionality that already exists in cht-conf.

[garethbowen]

It seems like C, E, and G are no brainers - do these first.

D: I feel like we should be able to detect this with regex (or regex + code a little code). For example, find the concat function, make sure all whitespace characters have an operator before or after them, and ignoring the contents of nested brackets. Am I missing something? If we can come up with a simpler version with no false positives and few false negatives this is doable too.

F is tricky because we don't want it to error for config going to cht-core v3.x. I'm not sure what to do here.

A and B are difficult because of the false positives and we should be cautious about incorporating those until we have an eslint style ignore feature.

Perhaps the solution is to keep A, B, and F in the script only so people can run it when they're looking to upgrade, and determine which are false positives, and then never have to run it again.

[jkuester]

Closing this since we have implemented all the validation that we wanted in cht-conf. The remaining functionality for A, B, and D exists in https://github.com/jkuester/cht-upgrade-helper and G needs to be solved in medic/pyxform#9