From 357cece1ada2dd51ab115b2d2c4d1bc142f66b54 Mon Sep 17 00:00:00 2001 From: Florent Chehab Date: Sat, 6 Apr 2019 21:35:06 +0200 Subject: [PATCH] feat(form errors): * Form level errors now go down to the fields in an awesome manner ! * Improved type hints in Field and Form Fixes #70 --- frontend/src/components/editor/Editor.js | 20 +- frontend/src/components/form/Form.js | 229 ++++++++++++++---- .../src/components/form/FormLevelError.js | 41 ++++ frontend/src/components/form/fields/Field.js | 40 +-- .../editors/UniversitySemestersDatesEditor.js | 59 +++-- .../editors/common/ScholarshipForm.js | 32 +-- .../src/components/user/UserInfoEditor.js | 26 +- 7 files changed, 301 insertions(+), 146 deletions(-) create mode 100644 frontend/src/components/form/FormLevelError.js diff --git a/frontend/src/components/editor/Editor.js b/frontend/src/components/editor/Editor.js index de11464..952cefc 100644 --- a/frontend/src/components/editor/Editor.js +++ b/frontend/src/components/editor/Editor.js @@ -8,7 +8,6 @@ import IconButton from "@material-ui/core/IconButton"; import Typography from "@material-ui/core/Typography"; import CloseIcon from "@material-ui/icons/Close"; import Alert from "../common/Alert"; - // Form is imported only for type hints // eslint-disable-next-line no-unused-vars import Form from "../form/Form"; @@ -50,7 +49,7 @@ class Editor extends Component { * Extra attributes that should be retrieved in rawModelData when parsing * I.e. the fields that are not present in the form but still need to be there * - * @memberof Editor + * @type {Array.} */ extraFieldMappings = []; @@ -58,7 +57,6 @@ class Editor extends Component { * Creates an instance of Editor. and subscribes to the module wrapper so that it can access its functions. * * @param {object} props - * @memberof Editor */ constructor(props) { super(props); @@ -75,7 +73,6 @@ class Editor extends Component { * Plut the id. * * @returns - * @memberof Editor */ parseRawModelData() { let out = {}; @@ -97,7 +94,6 @@ class Editor extends Component { * Returns the form instance associated with the editor * * @returns {Form} - * @memberof Editor */ getForm() { return this.formRef.current; @@ -125,7 +121,6 @@ class Editor extends Component { * Function to handle save editor events, eg when clicking on the save button * This function is not trivial and checks are performed. * - * @memberof Editor */ handleSaveEditorRequest() { const getFormError = this.getFormError(); @@ -143,7 +138,7 @@ class Editor extends Component { } } else { - this.notifyFormHasErrors(getFormError.messages); + this.notifyFormHasErrors(); } } @@ -152,7 +147,6 @@ class Editor extends Component { * Function to save the `data` to the server. * * @param {Object} data - * @memberof Editor */ performSave(data) { this.notifyIsSaving(); @@ -168,7 +162,6 @@ class Editor extends Component { * This enables not to use weird save detection method. * * @param {object} newData object returned by the server - * @memberof Editor */ handleSaveRequestWasSuccessful(newData) { // We check if data was moderated @@ -194,7 +187,6 @@ class Editor extends Component { * Function to handle close editor request from the user. * It checks if there is data to save or not. * - * @memberof Editor */ handleCloseEditorRequest() { if (this.formHasChanges()) { @@ -208,7 +200,6 @@ class Editor extends Component { * Effectively close the editor window and notify if there was something new that was saved * * @param {Boolean} somethingWasSaved - * @memberof Editor */ closeEditor(somethingWasSaved) { this.props.closeFullScreenDialog(); @@ -222,7 +213,6 @@ class Editor extends Component { * - Detecting when there was a successful save * - etc. * - * @memberof Editor */ componentDidUpdate() { @@ -246,7 +236,6 @@ class Editor extends Component { * Basically something that extends the `Form` component * * @returns - * @memberof Editor */ renderForm() { throw new Error("The renderForm function must be extended in sub classes"); @@ -258,7 +247,6 @@ class Editor extends Component { * fullscreen app dialog through redux. * * @returns - * @memberof Editor */ renderEditor() { const {classes} = this.props; @@ -347,8 +335,8 @@ class Editor extends Component { this.props.enqueueSnackbar("Aucun changement n'a été repéré.", {variant: "info"}); } - notifyFormHasErrors(errors) { - this.props.enqueueSnackbar(`Le formulaire semble incohérent, merci de vérifier son contenu. ${errors}`, {variant: "error"}); + notifyFormHasErrors() { + this.props.enqueueSnackbar("Le formulaire semble incohérent, merci de vérifier son contenu.", {variant: "error"}); } notifyIsSaving() { diff --git a/frontend/src/components/form/Form.js b/frontend/src/components/form/Form.js index 7799072..4eee12b 100644 --- a/frontend/src/components/form/Form.js +++ b/frontend/src/components/form/Form.js @@ -1,23 +1,74 @@ -import { Component } from "react"; +import {Component} from "react"; import PropTypes from "prop-types"; import areSameObjects from "../../utils/areSameObjects"; import renderFieldsMixIn from "./renderFieldsMixIn"; import CustomError from "../common/CustomError"; +/** + * Function to build a chached version of the fields mapping to cascade update. + * + * @param {Array.} formLevelErrors + * @returns {Map} + */ +function buildFieldUpdateCache(formLevelErrors) { + let out = new Map(); + formLevelErrors.map(e => e.getFields()) + .forEach(fieldList => + fieldList.forEach(field => { + if (!out.has(field)) { + out.set(field, new Set()); + } + + fieldList + .filter(linkedField => linkedField !== field) + .forEach(linkedField => { + out.set(field, out.get(field).add(linkedField)); + }); + })); + + return out; +} + + /** * React component that should contain `Field` instances. - * Custom function have been implemented to ease form handling. + * + * It has a ton of custom behavior implemented in it. `Fields` and `Form` + * are depending on each other with some custom subscribing behavior. * * @class Form * @extends {React.Component} */ class Form extends Component { - // Store it as an object to make sure we don't have issues - // with resetting it for some reason + + /** + * + * + * handling of the interconnection between the fields and the form + * + * + */ + + + /** + * YOU SHOULDN'T OVERRIDE THIS + * + * We store it as an object to make sure we don't have issues + * with resetting it for some reason + * @type {object.} + */ fields = Object(); + /** + * Array containing the possible form level errors + * @abstract + * @type {Array.} + */ + formLevelErrors = []; /** + * YOU SHOULDN'T OVERRIDE THIS + * * This method MUST be used on all field inside a `Form` instance. * * Function that returns the value corresponding to `fieldMapping` and @@ -25,123 +76,196 @@ class Form extends Component { * * @param {string} fieldMapping * @param {function} [convertValue=v => v] Method applied to the value from the modelData to get "the value" - * @memberof Form */ getReferenceAndValue(fieldMapping, convertValue = v => v) { // using react ref was to complicated with ref forwarding not working with withStyles. - return { value: convertValue(this.props.modelData[fieldMapping]), form: this, fieldMapping }; + return {value: convertValue(this.props.modelData[fieldMapping]), form: this, fieldMapping}; } + + /** + * + * + * + * General Utils + * + * + * + */ + /** + * YOU SHOULDN'T OVERRIDE THIS + * * Function to be used in fields so that they subscribe to a form * * Using ref to field is not working with withStyles ref forwarding issues. * * @param {string} fieldMapping * @param {Field} field - * @memberof Form */ fieldSubscribe(fieldMapping, field) { this.fields[fieldMapping] = field; } /** + * YOU SHOULDN'T OVERRIDE THIS + * * Function that returns the fields contained in the form * as an array of {fieldMapping: string, field: Field} * * Works only if the `getReferenceAndValue` was used on the Field props and the field subscribed. * - * @returns {Array} - * @memberof Form + * @returns {{fieldMapping: string, field: Field}[]} */ getFields() { return Object.keys(this.fields) .map(fieldMapping => - ({ fieldMapping, field: this.fields[fieldMapping] }) + ({fieldMapping, field: this.fields[fieldMapping]}) ); } /** + * YOU SHOULDN'T OVERRIDE THIS + * + * Function to look if there has been changes compared to the data + * this is already saved. + * + * @returns {Boolean} + */ + hasChanges() { + const formData = this.getDataFromFields(), + modelData = this.props.modelData; + + return Object.keys(formData).some(fieldMapping => { + const cmp1 = formData[fieldMapping], + cmp2 = modelData[fieldMapping]; + + // we need to compare objects (ie JSON objects) differently + if (typeof cmp1 === "object") { + return !areSameObjects(cmp1, cmp2); + } else { + return cmp1 !== cmp2; + } + }); + } + + + /** + * + * + * + * + * Error handling + * + * + * + */ + + /** + * YOU SHOULDN'T OVERRIDE THIS + * * Returns an object containing with {fieldMapping: valueInField} * - * @returns {Object} - * @memberof Form + * @returns {object.} */ getDataFromFields() { return this.getFields() .reduce( - (acc, { field, fieldMapping }) => { + (acc, {field, fieldMapping}) => { acc[fieldMapping] = field.getValue(); return acc; }, Object()); } + /** + * YOU SHOULDN'T OVERRIDE THIS + * + * Returns the error object corresponding to the errors **from the form** for a field. + * + * @param {string} fieldMapping + * @returns {CustomError} + */ + getErrorForField(fieldMapping) { + const formData = this.getDataFromFields(); + const errorMessages = this.formLevelErrors + .filter(formLevelError => formLevelError.getFields().includes(fieldMapping)) + .filter(formLevelError => formLevelError.hasError(formData)) + .map(formLevelError => formLevelError.getMessage()); + return new CustomError(errorMessages); + } + + + /** + * YOU SHOULDN'T OVERRIDE THIS + * + * Function to call once a field has been updated, to trigger a rerendering of all the fields + * linked by a form level error handling. + * @param {string} fieldMapping + */ + fieldUpdated(fieldMapping) { + const fieldsToUpdate = this.getFieldsToUpdate(); + if (fieldsToUpdate.has(fieldMapping)) { + fieldsToUpdate + .get(fieldMapping) + .forEach(field => this.fields[field].forceUpdate()); + } + } + + + /** + * YOU SHOULDN'T OVERRIDE THIS + * + * Helper function to cache the map containin the mapping between a field(name) and the set of fields + * that should be updated when the previous one is updated. + * @returns {Map} + */ + getFieldsToUpdate() { + if (!this.fieldsToUpdate) { + this.fieldsToUpdate = buildFieldUpdateCache(this.formLevelErrors); + } + return this.fieldsToUpdate; + } + /** + * YOU SHOULDN'T OVERRIDE THIS * Function to build all the errors from the fields of the form. * - * @returns - * @memberof Form + * @returns {CustomError} */ - fieldsHaveError() { + getFieldsErrors() { return CustomError.superCombine( this.getFields() - .map(({ field }) => field.getError()) + .map(({field}) => field.getError()) ); } /** + * YOU SHOULDN'T OVERRIDE THIS * Function to know if the form has errors either at the field level * Or when running checks that combine fields * - * YOU SHOULDN'T OVERRIDE THIS - * - * @returns - * @memberof Form + * @returns {CustomError} */ getError() { - return this.fieldsHaveError() + return this.getFieldsErrors() .combine(this.hasFormLevelErrors()); } /** + * YOU SHOULDN'T OVERRIDE THIS * Function to check for errors in the form * - * Can be override in sub classes - * - * @virtual * @returns {CustomError} - * @memberof Form */ hasFormLevelErrors() { - return new CustomError([]); // default: no errors - } - - - /** - * Function to look if there has been changes compared to the data - * this is already saved. - * - * @returns {Boolean} - * @memberof Form - */ - hasChanges() { - const formData = this.getDataFromFields(), - modelData = this.props.modelData; - - return Object.keys(formData).some(fieldMapping => { - const cmp1 = formData[fieldMapping], - cmp2 = modelData[fieldMapping]; - - // we need to compare objects (ie JSON objects) differently - if (typeof cmp1 === "object") { - return !areSameObjects(cmp1, cmp2); - } else { - return cmp1 !== cmp2; - } - }); + const formData = this.getDataFromFields(); + const errors = this.formLevelErrors + .filter(formLevelError => formLevelError.hasError(formData)) + .map(formLevelError => formLevelError.getMessage()); + return new CustomError(errors); } } @@ -150,7 +274,6 @@ class Form extends Component { Object.assign(Form.prototype, renderFieldsMixIn); - Form.propTypes = { modelData: PropTypes.object.isRequired, outsideData: PropTypes.object, diff --git a/frontend/src/components/form/FormLevelError.js b/frontend/src/components/form/FormLevelError.js new file mode 100644 index 0000000..8186b4c --- /dev/null +++ b/frontend/src/components/form/FormLevelError.js @@ -0,0 +1,41 @@ +/** + * Class to handle easily form level errors + */ +export default class FormLevelError { + + /** + * @param {Array.} fields Array of the field concerned by the error. + * @param {function(**): boolean} check Function that performs the check of the fields. The values of the `fields` are passed as parameters and the function is expected to return a boolean. + * @param {string} message Message for the error in case the check doesn't pass. + */ + constructor(fields, check, message) { + this.fields = fields; + this.check = check; + this.message = message; + } + + /** + * Do we currently have an error ? + * @param {object} formData data from the form + * @returns {boolean} + */ + hasError(formData) { + return this.check(...this.fields.map(fieldMapping => formData[fieldMapping])); + } + + /** + * Get the message associated with the error + * @returns {string} + */ + getMessage() { + return this.message; + } + + /** + * Return an array of string corresponding to the field name. + * @returns {Array} + */ + getFields() { + return this.fields; + } +} diff --git a/frontend/src/components/form/fields/Field.js b/frontend/src/components/form/fields/Field.js index 1de56c0..256fa3a 100644 --- a/frontend/src/components/form/fields/Field.js +++ b/frontend/src/components/form/fields/Field.js @@ -1,4 +1,4 @@ -import React, { PureComponent } from "react"; +import React, {PureComponent} from "react"; import PropTypes from "prop-types"; import Form from "../Form"; import FieldWrapper from "./FieldWrapper"; @@ -18,18 +18,17 @@ class Field extends PureComponent { defaultNullValue = undefined; /** - *Creates an instance of Field. + * Creates an instance of Field. * @param {object} props * @param {object} [customStateAttrs={}] add custom state attributes on creation - * @memberof Field */ constructor(props, customStateAttrs = {}) { super(props); - // make sure to subscribe ! IMPORTANT + // make sure to subscribe to the form ! IMPORTANT props.form.fieldSubscribe(props.fieldMapping, this); - let { value } = props; + let {value} = props; this.state = { value: value, ...customStateAttrs, @@ -41,14 +40,17 @@ class Field extends PureComponent { * * @override * @param {object} newState - * @memberof Field */ setState(newState) { let state = Object.assign({}, newState); if (typeof this.defaultNullValue !== "undefined") { - Object.assign(state, { value: newState.value === this.defaultNullValue ? null : newState.value }); + Object.assign(state, {value: newState.value === this.defaultNullValue ? null : newState.value}); } - super.setState(newState); + const {form, fieldMapping} = this.props; + super.setState(newState, () => { + form.fieldUpdated(fieldMapping); + }); + } /** @@ -56,19 +58,28 @@ class Field extends PureComponent { * * @virtual * @returns {CustomError} - * @memberof Field */ getError() { throw Error("This methods has to be override in sub classes"); } + /** + * YOU SHOULDN'T OVERRIDE THIS + * + * Return the errors from the field and the errors from the form corresponding to the field. + * @returns {CustomError} + */ + getAllErrors() { + const {form, fieldMapping} = this.props; + return this.getError().combine(form.getErrorForField(fieldMapping)); + } + /** * function to get serialize the value from the field, to get it ready to send to server * You might need to override this for weird formats such as dates. * - * @returns - * @memberof Field + * @returns {string|object} */ serializeFromField() { return this.state.value; @@ -78,7 +89,6 @@ class Field extends PureComponent { * Returns the serialized value of the field * * @returns - * @memberof Field */ getValue() { return this.serializeFromField(); @@ -87,10 +97,9 @@ class Field extends PureComponent { /** * Function that should render the field itself * - * MUST BE OVERRIDE + * MUST BE OVERRIDEN * * @virtual - * @memberof Field */ renderField() { throw new Error("This method should be override in subclass of Field"); @@ -100,13 +109,12 @@ class Field extends PureComponent { * Default react render function * * @returns - * @memberof Field */ render() { return ( {this.renderField()} diff --git a/frontend/src/components/university/editors/UniversitySemestersDatesEditor.js b/frontend/src/components/university/editors/UniversitySemestersDatesEditor.js index 5940fe3..9855e40 100644 --- a/frontend/src/components/university/editors/UniversitySemestersDatesEditor.js +++ b/frontend/src/components/university/editors/UniversitySemestersDatesEditor.js @@ -1,7 +1,7 @@ import React from "react"; import withStyles from "@material-ui/core/styles/withStyles"; import compose from "recompose/compose"; -import { connect } from "react-redux"; +import {connect} from "react-redux"; import dateStrToDate from "../../../utils/dateStrToDate"; @@ -13,9 +13,9 @@ import editorStyle from "../../editor/editorStyle"; import getMapStateToPropsForEditor from "../../editor/getMapStateToPropsForEditor"; import getMapDispatchToPropsForEditor from "../../editor/getMapDispatchToPropsForEditor"; -import CustomError from "../../common/CustomError"; -import { withSnackbar } from "notistack"; +import {withSnackbar} from "notistack"; +import FormLevelError from "../../form/FormLevelError"; const styles = theme => ({ @@ -24,45 +24,44 @@ const styles = theme => ({ class UniversitySemestersDatesForm extends Form { - hasFormLevelErrors() { - let messages = Array(); - const formData = this.getDataFromFields(); - const { autumn_begin, autumn_end, spring_begin, spring_end } = formData; - if (onlyOneIsNull(autumn_begin, autumn_end)) { - messages.push("Si une date est saisie pour le semestre d'automne, l'autre doit l'être aussi."); - } - - if (onlyOneIsNull(spring_begin, spring_end)) { - messages.push("Si une date est saisie pour le semestre de printemps, l'autre doit l'être aussi."); - } - - if (autumn_begin && autumn_end && autumn_begin > autumn_end) { - messages.push("Le début du semestre d'automne doit être antérieur à sa fin..."); - } - - if (spring_begin && spring_end && spring_begin > spring_end) { - messages.push("Le début du semestre de printemps doit être antérieur à sa fin..."); - } - - return new CustomError(messages); - } + /** + * @override + */ + formLevelErrors = [ + new FormLevelError( + ["autumn_begin", "autumn_end"], + (begin, end) => onlyOneIsNull(begin, end), + "Si une date est saisie pour le semestre d'automne, l'autre doit l'être aussi."), + new FormLevelError( + ["spring_begin", "spring_end"], + (begin, end) => onlyOneIsNull(begin, end), + "Si une date est saisie pour le semestre de printemps, l'autre doit l'être aussi."), + new FormLevelError( + ["autumn_begin", "autumn_end"], + (begin, end) => begin && end && begin > end, + "Le début du semestre d'automne doit être antérieur à sa fin..."), + new FormLevelError( + ["spring_begin", "spring_end"], + (begin, end) => begin && end && begin > end, + "Le début du semestre de printemps doit être antérieur à sa fin..."), + ]; render() { return ( <> {this.renderObjModerationLevelField()} {this.renderCommentField()} {this.renderUsefulLinksField()} @@ -92,7 +91,7 @@ class UniversitySemestersDatesEditor extends Editor { export default compose( withSnackbar, - withStyles(styles, { withTheme: true }), + withStyles(styles, {withTheme: true}), connect( getMapStateToPropsForEditor("universitiesSemestersDates"), getMapDispatchToPropsForEditor("universitiesSemestersDates") diff --git a/frontend/src/components/university/editors/common/ScholarshipForm.js b/frontend/src/components/university/editors/common/ScholarshipForm.js index 2e2eb4c..c728c5e 100644 --- a/frontend/src/components/university/editors/common/ScholarshipForm.js +++ b/frontend/src/components/university/editors/common/ScholarshipForm.js @@ -2,28 +2,28 @@ import React from "react"; import PropTypes from "prop-types"; import Form from "../../../form/Form"; -import CustomError from "../../../common/CustomError"; +import FormLevelError from "../../../form/FormLevelError"; const frequencyOptions = [ - { value: "w", label: "Il s'agit du montant hebdomadaire" }, - { value: "m", label: "Il s'agit du montant mensuel" }, - { value: "s", label: "Il s'agit du montant semestriel" }, - { value: "y", label: "Il s'agit du montant annuel" }, - { value: "o", label: "Il s'agit d'un montant donné une seule fois" } + {value: "w", label: "Il s'agit du montant hebdomadaire"}, + {value: "m", label: "Il s'agit du montant mensuel"}, + {value: "s", label: "Il s'agit du montant semestriel"}, + {value: "y", label: "Il s'agit du montant annuel"}, + {value: "o", label: "Il s'agit d'un montant donné une seule fois"} ]; class ScholarshipForm extends Form { - hasFormLevelErrors() { - let messages = Array(); - const formData = this.getDataFromFields(), - { amount_min, amount_max } = formData; - if (amount_min !== null && amount_max !== null && amount_max < amount_min) { - messages.push("La logique voudrait que la borne supérieure de la bourse soit... supérieure à la borne inférieure."); - } - return new CustomError(messages); - } + /** + * @override + */ + formLevelErrors = [ + new FormLevelError( + ["amount_min", "amount_max"], + (amountMin, amountMax) => amountMin !== null && amountMax !== null && amountMax < amountMin, + "La logique voudrait que la borne supérieure de la bourse soit... supérieure à la borne inférieure."), + ]; render() { return ( @@ -37,7 +37,7 @@ class ScholarshipForm extends Form { maxLength: 200, fieldMapping: "short_description", })} - {this.renderCurrencyField({ label: "Devise", required: false, fieldMapping: "currency" })} + {this.renderCurrencyField({label: "Devise", required: false, fieldMapping: "currency"})} {this.renderNumberField({ label: "Borne inférieure du montant de la bourse", minValue: 0, diff --git a/frontend/src/components/user/UserInfoEditor.js b/frontend/src/components/user/UserInfoEditor.js index 91ceb24..a1c8ea2 100644 --- a/frontend/src/components/user/UserInfoEditor.js +++ b/frontend/src/components/user/UserInfoEditor.js @@ -11,8 +11,8 @@ import Editor from "../editor/Editor"; import getMapStateToPropsForEditor from "../editor/getMapStateToPropsForEditor"; import getMapDispatchToPropsForEditor from "../editor/getMapDispatchToPropsForEditor"; import BooleanField from "../form/fields/BooleanField"; -import CustomError from "../common/CustomError"; import {getLatestReadDataFromStore} from "../../redux/api/utils"; +import FormLevelError from "../form/FormLevelError"; const styles = theme => ({ @@ -24,21 +24,17 @@ class UserInfoForm extends Form { /** * @override - * @returns {CustomError} */ - hasFormLevelErrors() { - - let messages = Array(); - const formData = this.getDataFromFields(), - {allow_sharing_personal_info: sharingAllowed} = formData, - userData = getLatestReadDataFromStore("userDataOne"), - {owner_level: userLevel} = userData; - - if (userLevel > 0 && !sharingAllowed) { - messages.push("Les modérateurs, membres de la DRI ainsi que les administrateurs du site sont forcés d'avoir un profil publique."); - } - return new CustomError(messages); - } + formLevelErrors = [ + new FormLevelError( + ["allow_sharing_personal_info"], + (sharingAllowed) => { + const userData = getLatestReadDataFromStore("userDataOne"), + {owner_level: userLevel} = userData; + return userLevel > 0 && !sharingAllowed; + }, + "Les modérateurs, membres de la DRI ainsi que les administrateurs du site sont forcés d'avoir un profil publique.") + ]; render() { return ( -- GitLab