-
Notifications
You must be signed in to change notification settings - Fork 0
DAS-7055, DAS-7064 Improve feedback errors for missing fields in report forms #496
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
72f6cf7
906e5e6
76b2d1f
d291c79
4c8454f
3117693
4c282ee
3b84d3b
0f2bb79
312f671
d5a66a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,36 @@ | ||
import React, { memo } from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import Accordion from 'react-bootstrap/Accordion'; | ||
import Button from 'react-bootstrap/Button'; | ||
import Alert from 'react-bootstrap/Alert'; | ||
|
||
import { generateErrorListForApiResponseDetails } from '../utils/events'; | ||
|
||
import styles from './styles.module.scss'; | ||
|
||
const ReportFormErrorMessages = (props) => { | ||
const { errorData, onClose } = props; | ||
return <Alert onClose={onClose} dismissible={true} className={styles.saveErrorAlert} variant='danger'> | ||
<h5>Error saving report: {errorData.message}</h5> | ||
{<ul> | ||
{generateErrorListForApiResponseDetails(errorData).map(item => | ||
<li key={`${item.label} ${item.message}`}> | ||
<strong>{item.label}</strong>: <span>{item.message}</span> | ||
</li>)} | ||
</ul>} | ||
|
||
return <Alert onClose={onClose} dismissible={true} className={styles.saveErrorAlert} > | ||
<Accordion> | ||
<span>Error saving report.</span> | ||
<Accordion.Toggle as={Button} variant="link" eventKey="0" className={styles.saveErrorAlertLink}> | ||
See details | ||
</Accordion.Toggle> | ||
|
||
<Accordion.Collapse eventKey="0"> | ||
<ul> | ||
{errorData.map(item => | ||
<li key={`${item.label} ${item.message}`}> | ||
<strong>{item.label}</strong>: <span>{item.message}</span> | ||
</li>)} | ||
</ul> | ||
</Accordion.Collapse> | ||
</Accordion> | ||
</Alert>; | ||
}; | ||
|
||
export default memo(ReportFormErrorMessages); | ||
|
||
ReportFormErrorMessages.propTypes = { | ||
errorData: PropTypes.object.isRequired, | ||
errorData: PropTypes.array.isRequired, | ||
onClose: PropTypes.func, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,14 +8,16 @@ import styles from './styles.module.scss'; | |
|
||
const additionalMetaSchemas = [draft4JsonSchema]; | ||
|
||
const getLinearErrorPropTree = (errorProperty) => { | ||
return errorProperty.replace(/'|\.properties|\[|\]|\.enumNames|\.enum/g, '.') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can create a constant for the regex, using an explicit name that explains the purpose of it. This way it would be easier to know what we are trying to replace here. |
||
.split('.') | ||
.filter(p => !!p) | ||
.map(item => isNaN(item) ? item : parseFloat(item)); | ||
}; | ||
|
||
const filterOutEnumErrors = (errors, schema) => errors // filter out enum-based errors, as it's a type conflict between the property having type='string' when our API returns strings but expecting objects in the POSTs. | ||
.filter((error) => { | ||
const linearErrorPropTree = error.property | ||
.replace(/'|\.properties|\[|\]|\.enumNames|\.enum/g, '.') | ||
.split('.') | ||
.filter(p => !!p) | ||
.map(item => isNaN(item) ? item : parseFloat(item)); | ||
|
||
const linearErrorPropTree = getLinearErrorPropTree(error.property); | ||
amicavi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let match; | ||
|
||
if (linearErrorPropTree.length === 1) { | ||
|
@@ -30,7 +32,7 @@ const filterOutEnumErrors = (errors, schema) => errors // filter out enum-based | |
}, schema); | ||
} | ||
|
||
return !!match && !match.enum; | ||
return !!match || match?.enum?.length; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was causing for some required fields to not show an error |
||
}); | ||
|
||
const filterOutRequiredValueOnSchemaPropErrors = errors => errors.filter(err => !JSON.stringify(err).includes('required should be array')); | ||
|
@@ -41,7 +43,10 @@ const ReportFormBody = forwardRef((props, ref) => { // eslint-disable-line react | |
const transformErrors = useCallback((errors) => { | ||
const errs = filterOutRequiredValueOnSchemaPropErrors( | ||
filterOutEnumErrors(errors, schema)); | ||
return errs; | ||
return errs.map(err => ({ | ||
...err, | ||
linearProperty: getLinearErrorPropTree(err.property) | ||
})); | ||
}, [schema] | ||
); | ||
|
||
|
@@ -51,21 +56,15 @@ const ReportFormBody = forwardRef((props, ref) => { // eslint-disable-line react | |
className={styles.form} | ||
disabled={schema.readonly} | ||
formData={formData} | ||
liveValidate={true} | ||
onChange={onChange} | ||
formContext={ | ||
{ | ||
scrollContainer: formScrollContainer, | ||
} | ||
} | ||
formContext={{ scrollContainer: formScrollContainer }} | ||
onSubmit={onSubmit} | ||
ref={ref} | ||
schema={schema} | ||
ObjectFieldTemplate={ObjectFieldTemplate} | ||
transformErrors={transformErrors} | ||
uiSchema={uiSchema} | ||
{...rest} | ||
> | ||
{...rest}> | ||
{children} | ||
</Form>; | ||
}); | ||
|
Uh oh!
There was an error while loading. Please reload this page.