Allow users to recover from xpath errors

[garethbowen]

When the xpath throws a validation error the web-forms UI displays the error in a dialog that is impossible to recover from without refreshing the form.

We can't necessarily show an error beside the field because it may be that it's not shown to the user, or the xpath expression may reference several fields. Instead show an error at the top of the form, and disable submit. That way the user is not blocked from fixing the error, and not interrupted if they're in the process of providing valid data.

Also consider if throwing errors is the correct approach or if the API should instead return error objects.

There may be a common approach that can be used for similar errors in the xforms-engine.

[garethbowen]

Consider putting a validation error on the field that the user is changing that triggered the error, because that's probably the value the user needs to change to fix it.

[garethbowen]

Revisit date formatting functions when working on this issue. To work around this bug we decided to return '' instead of throwing when parsing dates, but once a non-blocking error can be displayed this should be changed back to be more consistent with Collect.

[garethbowen]

I've spent some time working through this. The options I've looked in to are...

1. Modifying the xpath package to never throw. The package doesn't have enough context to know what to do if there's a failure so it would have to return a Result type containing the value if successful, or the error if not. This is a significant and breaking change.
2. Modifying the xforms-engine so errors thrown in setValue on the node are caught, and stored in the node. This was the least disruptive option, but it doesn't play nice with reactivity, as when an error is thrown solid freezes the graph. I never quite got it to unfreeze but it should be possible, but ultimately it's an antipattern that would be better to avoid. The main benefit of this solution is the Error is set on the question the user is updating, and the error can be shown there, rather than options 3 and 4 that show the error on the field with the failing calculation.
3. Modifying the xforms-engine so errors from xpath are caught immediately and converted to a Result type. The internal value of the node would then be a Result. Other xpath functions that resolve the erroring node would get null. The web-forms client does not know about the Result type either, and gets a null value. Update the node's ValidationState to reference the "exception" violation as well as required and constraint, so the client shows an error on visible questions that have an invalid calculation.
4. Same as 3 except: Modify the xpath package so it understands the Result type. Any expression that resolves a node with an error Result immediately returns an error Result.

My preference is for option 3, because unlike 2 it doesn't freeze the reactivity, and unlike 4 it's not a breaking change. While it would be nice to show errors on the user input, these types of errors should be edge cases.

[latin-panda]

@garethbowen
I had a look at the code, and I agree with option 3:

• When a calculate throws, it dies and the question keeps a stale value. So a dismissible dialog alone would be dangerous, because users could submit stale data. Catching inside the computation (option 3) fixes this. The calculate should stay "alive" and recover once the input is valid.
• Errors like bad value formats or unknown function names are form problems, and we should catch those. But engine bugs should stay loud, so people report them and we fix them fast. We could give form errors a shared base class in xpath, and only catch that. Then show them as violations as soon as they are detected (even on form load), don't wait for the question to be "touched" like constraints do.
• A group's relevant and a repeat's jr:count can throw too, but those nodes can't hold a violation (currently only value nodes can). I think violations could move from value nodes up to the DescendantNode, so we don't need a separate structure for errors, just violations. We should include these cases in the test plan!

[lognaturel]

Too bad that 2 didn't work as smoothly as I had hoped.

Update the node's ValidationState to reference the "exception" violation as well as required and constraint

And if there are issues with the required and constraint expressions themselves? Can those also be referenced from the node and displayed?

For the calculate case (field with calculate and no body node), the component gets 0 information about the failure, right? Can the engine do something like console log the error so there's some possibility to troubleshoot for advanced users?

A group's relevant and a repeat's jr:count can throw too

That's a good point. And readonly expressions everywhere too, right?

[garethbowen]

And if there are issues with the required and constraint expressions themselves? Can those also be referenced from the node and displayed?

Constraint (and required) currently have a two options, either valid or invalid. We could catch the error and return invalid, but the error message would be misleading, eg: "this field is required" rather than "this isn't a valid geopoint". So I think it would be better to introduce a third option of errored which allows for custom handling.

For the calculate case (field with calculate and no body node), the component gets 0 information about the failure, right?

Right, it would be shown at the top of the form, but not inline. I think we should aim to include the detail in the UI, though it may need different handling than the current error message which just says "x questions had errors". This is similar to how we show a detailed error at the top of the form when geolocation is denied.

And readonly expressions everywhere too, right?

Yes, true. I guess it should be a blocking error on readonly expressions as well because we simply don't know whether the node should be readonly or not...