Skip to content

Commit

Permalink
web-forms (ui): Basic presentation of form load failures
Browse files Browse the repository at this point in the history
Where a “form load failure” is any Promise rejection produced by the engine’s `initializeForm` entrypoint. This is intentionally broad, and what I think is a relatively minimal first pass that might be useful to users (and to us).

“Relatively” minimal because I took the opportunity to lay a bit of groundwork for a few things either cued for near term work, or ripe for near term improvement.

Notable among those:

- Establish a more intentional approach to use of test fixtures in the `web-forms` package. This includes a dedicated fixtures directory (alongside those already used by `scenario`), and a narrower “test helper” (ugh) to access those. That access point also includes some additional breadcrumbs for ways we might improve fixture access in the future.

- `FormLoadFailure` begins laying a bit of groundwork for the incoming, much more comprehensive overhaul of error messaging.

- Actually tests a few different form load failure conditions. I believe these will be useful for catching potential regressions in that overhaul, as well in other future work around those specific failure modes.

UI/UX/presentation design note: I had a brief discussion with @alyblenkin about this being a minimal first step. We agreed that this seems like a reasonable first pass, and agreed there’s more to do in near-future iterations. Things which are currently lacking from a UX perspective, blocked by work with a larger scope:

- Better messaging of specific errors. We’ll be cataloguing known cases as part of #202, which will give us a much clearer picture of how to address this.
- Any kind of direct prompt for user action. Deferred for the same reasons.
- Ability to close the dialog or go back. Deferred because this would expand scope to navigation more generally, which we should look at more holistically.
  • Loading branch information
eyelidlessness committed Oct 4, 2024
1 parent 6d86e7a commit f137af5
Show file tree
Hide file tree
Showing 10 changed files with 432 additions and 26 deletions.
8 changes: 6 additions & 2 deletions packages/common/src/fixtures/REAMDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,13 @@ The fixtures are structured into a set of sub-directories. These sub-directories

Two of the sub-directories in particular serve an important purpose:

- [`test-javarosa`](./test-javarosa): Fixtures in this directory have been copied directly from JavaRosa, and are referenced by tests in [@getodk/scenario](../../../scenario/) integration test suites. These fixtures and resources should not be modified, except when their counterparts are updated upstream in JavaRosa.
- [`test-javarosa`](./test-javarosa/): Fixtures in this directory have been copied directly from JavaRosa, and are referenced by tests in [@getodk/scenario](../../../scenario/) integration test suites. These fixtures and resources should not be modified, except when their counterparts are updated upstream in JavaRosa.

- [`test-scenario`](./test-scenario): Fixtures in this directory are _derived from_ those in `test-javarosa`, with a naming convention appending `-alt` as a suffix to their base name. These fixtures are intended to have only minor modifications from their counterparts in the upstream `test-javarosa` directory. They should also not be modified, except to either reflect changes from JavaRosa upstream, or to demonstrate key behavioral differences in the tests referencing them.
- [`test-scenario`](./test-scenario/): Fixtures in this directory are _derived from_ those in `test-javarosa`, with a naming convention appending `-alt` as a suffix to their base name. These fixtures are intended to have only minor modifications from their counterparts in the upstream `test-javarosa` directory. They should also not be modified, except to either reflect changes from JavaRosa upstream, or to demonstrate key behavioral differences in the tests referencing them.

- [`test-web-forms`](./test-web-forms/): Fixtures in this directory are used for testing UI/UX behavior, as implemented by the [@getodk/web-forms](../../../web-forms/) package. These fixtures are intended to make it easy to manually inspect and interact with the same fixtures used under test.

**NOTE:** In the future, we may also consider ways to make UI access easier for test fixtures defined in tests inline, using the [XForms DSL](../test/fixtures/xform-dsl/README.md). In which case, we should consider inlining these fixtures as well.

- [`preview-service`](./preview-service/): Fixtures in this directory are used as real world Forms in the Web Form preview service. It contains XLSForms to be serve as downloadable resource from the service. XForms are generated using [pyxform](https://github.com/XLSForm/pyxform) / [xlsform-online](https://getodk.org/xlsform/) service and should not be manually modified. Note: XForm (xml) for WHO Verbal Autopsy is not placed in the `xform` subdirectory as it is already present in the [`smoketests`](./test-javarosa/resources//smoketests/)

Expand Down
25 changes: 25 additions & 0 deletions packages/common/src/fixtures/test-web-forms/simple-dag-cycle.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml"
xmlns:jr="http://openrosa.org/javarosa" xmlns:odk="http://www.opendatakit.org/xforms"
xmlns:orx="http://openrosa.org/xforms">
<h:head>
<h:title>Form design problem: computation cycle</h:title>
<model>
<instance>
<data id="simple-dag-cycle">
<group>
<a>1</a>
</group>
</data>
</instance>
<bind nodeset="/data/group/a" type="int" calculate="../a + 1" />
</model>
</h:head>
<h:body>
<group ref="/data/group">
<repeat nodeset="/data/group">
<input ref="/data/group/a" />
</repeat>
</group>
</h:body>
</h:html>
33 changes: 33 additions & 0 deletions packages/common/src/fixtures/test-web-forms/xpath-syntax-error.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms"
xmlns:ev="http://www.w3.org/2001/xml-events"
xmlns:h="http://www.w3.org/1999/xhtml"
xmlns:jr="http://openrosa.org/javarosa"
xmlns:orx="http://openrosa.org/xforms/"
xmlns:xsd="http://www.w3.org/2001/XMLSchema">
<h:head>
<h:title>Form design problem: syntax error</h:title>
<model>
<instance>
<root id="form-design-problem-syntax-error">
<first-question>1</first-question>
<second-question />
<meta>
<instanceID />
</meta>
</root>
</instance>
<bind nodeset="/root/first-question" />
<bind nodeset="/root/second-question" calculate="/root/first-question ? 2" />
<bind nodeset="/root/meta/instanceID" type="string" />
</model>
</h:head>
<h:body>
<input ref="/root/first-question">
<label>1. Default value is 1</label>
</input>
<input ref="/root/second-question">
<label>2. Fails to calculate (expression has invalid token: `?`)</label>
</input>
</h:body>
</h:html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms"
xmlns:ev="http://www.w3.org/2001/xml-events"
xmlns:h="http://www.w3.org/1999/xhtml"
xmlns:jr="http://openrosa.org/javarosa"
xmlns:orx="http://openrosa.org/xforms/"
xmlns:xsd="http://www.w3.org/2001/XMLSchema">
<h:head>
<h:title>Form design problem: unknown XPath function</h:title>
<model>
<instance>
<root id="xpath-unknown-function">
<first-question />
<second-question />
<meta>
<instanceID />
</meta>
</root>
</instance>
<bind nodeset="/root/first-question" calculate="nope(1)" />
<bind nodeset="/root/meta/instanceID" type="string" />
</model>
</h:head>
<h:body>
<input ref="/root/first-question">
<label>Calculate calls an XPath function `nope`, which does not exist!</label>
</input>
</h:body>
</h:html>
11 changes: 11 additions & 0 deletions packages/common/src/lib/runtime-types/shared-type-predicates.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export type UnknownObject = Readonly<Record<PropertyKey, unknown>>;

/**
* Checks whether a value's type is `'object'`, and the value is not `null`.
* This is useful as a precondition for more specific type-narrowing predicate
* and/or assertion functions, allowing inspection of arbitrary properties on
* {@link value} without verbose/error prone inline type assertions.
*/
export const isUnknownObject = (value: unknown): value is UnknownObject => {
return typeof value === 'object' && value != null;
};
88 changes: 88 additions & 0 deletions packages/web-forms/src/components/Form/FormLoadFailureDialog.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
<script setup lang="ts">
import Dialog from 'primevue/dialog';
import Message from 'primevue/message';
import { computed } from 'vue';
import type { InitializeFormFailure } from '../../lib/error/FormLoadFailure.ts';
/**
* @todo translations
*/
const FORM_LOAD_ERROR_TEXT = {
DIALOG_TITLE: 'An error occurred while loading this form',
DETAILS_SUMMARY_LABEL: 'Technical error details',
};
interface FormLoadErrorProps {
readonly error: InitializeFormFailure;
}
const props = defineProps<FormLoadErrorProps>();
interface FormLoadErrorDetail {
readonly stack: string | null;
readonly unknownCauseDetail: string | null;
}
const detail = computed((): FormLoadErrorDetail | null => {
const { stack = null, unknownCauseDetail } = props.error;
if (stack == null && unknownCauseDetail == null) {
return null;
}
return {
stack,
unknownCauseDetail,
};
});
</script>

<template>
<Dialog
class="form-load-failure-dialog"
:visible="detail != null"
:header="FORM_LOAD_ERROR_TEXT.DIALOG_TITLE"
:closable="false"
:draggable="false"
:keep-in-viewport="true"
>
<div class="content">
<Message severity="error" class="message" :closable="false">
{{ error.message }}
</Message>

<details v-if="detail != null" class="initialize-form-failure-details">
<summary>{{ FORM_LOAD_ERROR_TEXT.DETAILS_SUMMARY_LABEL }}</summary>

<pre v-if="detail.unknownCauseDetail != null">{{
detail.unknownCauseDetail
}}</pre>

<pre v-if="detail.stack != null">{{
detail.stack
}}</pre>
</details>
</div>
</Dialog>
</template>

<style scoped>
.form-load-failure-dialog .content {
width: calc(100dvw - 6rem) !important;
}
.form-load-failure-dialog .message {
margin-top: 0;
}
.initialize-form-failure-details {
display: block;
position: relative;
max-width: 100%;
overflow: auto;
}
.initialize-form-failure-details summary {
cursor: pointer;
}
</style>
50 changes: 33 additions & 17 deletions packages/web-forms/src/components/OdkWebForm.vue
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,19 @@ import Button from 'primevue/button';
import Card from 'primevue/card';
import PrimeMessage from 'primevue/message';
import { computed, provide, reactive, ref, watchEffect, type ComponentPublicInstance } from 'vue';
import { InitializeFormFailure } from '../lib/error/FormLoadFailure.ts';
import FormLoadFailureDialog from './Form/FormLoadFailureDialog.vue';
import FormHeader from './FormHeader.vue';
import QuestionList from './QuestionList.vue';
const webFormsVersion = __WEB_FORMS_VERSION__;
const props = defineProps<{ formXml: string }>();
const emit = defineEmits(['submit']);
const odkForm = ref<RootNode>();
const submitPressed = ref(false);
const emit = defineEmits(['submit']);
const initializeFormError = ref<InitializeFormFailure | null>();
initializeForm(props.formXml, {
config: {
Expand All @@ -26,9 +26,8 @@ initializeForm(props.formXml, {
.then((f) => {
odkForm.value = f;
})
.catch((error) => {
// eslint-disable-next-line no-console
console.error(error);
.catch((cause) => {
initializeFormError.value = new InitializeFormFailure(cause);
});
const handleSubmit = () => {
Expand All @@ -40,11 +39,11 @@ const handleSubmit = () => {
}
};
const errorMessagePopover = ref<ComponentPublicInstance | null>(null);
const validationErrorMessagePopover = ref<ComponentPublicInstance | null>(null);
provide('submitPressed', submitPressed);
const formErrorMessage = computed(() => {
const validationErrorMessage = computed(() => {
const violationLength = odkForm.value!.validationState.violations.length;
// TODO: translations
Expand All @@ -54,20 +53,37 @@ const formErrorMessage = computed(() => {
});
watchEffect(() => {
if (submitPressed.value && formErrorMessage.value) {
(errorMessagePopover.value?.$el as HTMLElement)?.showPopover?.();
if (submitPressed.value && validationErrorMessage.value) {
(validationErrorMessagePopover.value?.$el as HTMLElement)?.showPopover?.();
} else {
(errorMessagePopover.value?.$el as HTMLElement)?.hidePopover?.();
(validationErrorMessagePopover.value?.$el as HTMLElement)?.hidePopover?.();
}
});
</script>

<!--
TODO: consider handling all template control flow on `<template>` tags! While
`v-if` and similar Vue directives are available on HTML and component tags,
using `<template>` could help make it much more clear where control flow
exists. And it could help make it much more clear what props are actually
applicable for usage of a given tag.
-->
<template>
<div v-if="odkForm" class="odk-form" :class="{ 'submit-pressed': submitPressed }">
<template v-if="initializeFormError != null">
<FormLoadFailureDialog
severity="error"
:error="initializeFormError"
/>
</template>

<div
v-else-if="odkForm"
class="odk-form"
:class="{ 'submit-pressed': submitPressed }"
>
<div class="form-wrapper">
<div v-show="submitPressed && formErrorMessage" class="error-banner-placeholder" />
<PrimeMessage ref="errorMessagePopover" popover="manual" severity="error" icon="icon-error_outline" class="form-error-message" :closable="false">
{{ formErrorMessage }}
<div v-show="submitPressed && validationErrorMessage" class="error-banner-placeholder" />
<PrimeMessage ref="validationErrorMessagePopover" popover="manual" severity="error" icon="icon-error_outline" class="form-error-message" :closable="false">
{{ validationErrorMessage }}
</PrimeMessage>

<FormHeader :form="odkForm" />
Expand Down
Loading

0 comments on commit f137af5

Please sign in to comment.