search

OpenClinica / enketo-oc

OpenClinica's fork of the Enketo web forms monorepo
Apache License 2.0
0 stars 1 forks source link

Invalid item required logic not caught by Validate #83

Open pbowen-oc opened 2 years ago

pbowen-oc commented 2 years ago

We have had several cases of mistypes for item required settings ("w6", "ues", etc). These do not generate errors from Validate on form upload. They also don't process in a helpful way when the form is opened, but they don't seem to generate errors.

Is there a way of detecting that configurations like this are not pointing to valid nodes and reject them with Validate? Is there a flexibility downside to doing this? I assume that if someone were trying to write arbitrary XPath in their logic, it would be hard to validate. However, wouldn't "w6" as item required logic refer to a child of that item (which would never exist in our use case as far as I know)? If so, could we prevent basic references like this to nodes that are not at least "../node"?

Potentially related to validation like in OpenClinica/enketo-oc#84.

MartijnR commented 2 years ago

related: https://github.com/enketo/enketo-validate/issues/105 https://github.com/enketo/enketo-validate/issues/106

MartijnR commented 2 years ago

I think this is a pragmatic solution: https://github.com/enketo/enketo-validate/issues/106#issuecomment-1092107205

@pbowen-oc could you please confirm that:

  1. OC always add the comment-status() clauses after the expressions added by the user?
  2. OC validates the XForm after it has been augmented by OC and not before (directly after pyxform transformation)
pbowen-oc commented 2 years ago

@MartijnR - We actually run Validate immediately after Pyxform, not after the OC modifications to the form. So, we'd run Validate on "w6", not "w6 and comment-status(...)".

Beyond that, we only add the comment-status() functions for non-strict constraint/required. We never add them for strict checks.

MartijnR commented 2 years ago

Thanks. That will keep it simpler!

MartijnR commented 2 years ago

The new detection feature has been added to master in enketo-validate, but it is not yet published.

If you are able to test this from master, that would be great. Otherwise, let me know if should publish first.

MartijnR commented 2 years ago
MartijnR commented 2 years ago

published enketo-validate (and updated the webapp)