From a34698371e12d7df402c98d2962bce540b569cd5 Mon Sep 17 00:00:00 2001 From: James Date: Thu, 2 Jul 2020 00:46:03 -0400 Subject: [PATCH] implements a safer way to revert to original data if update policies fail --- demo/collections/AllFields.js | 4 +++- .../components/forms/Form/flattenFilters.js | 10 -------- src/client/components/forms/Form/index.js | 3 +++ .../forms/field-types/Flexible/index.js | 4 ++-- .../forms/field-types/Repeater/index.js | 4 ++-- .../forms/field-types/Upload/index.js | 6 +++-- src/collections/operations/update.js | 4 ++-- src/fields/performFieldOperations.js | 24 ++++++++++++------- 8 files changed, 31 insertions(+), 28 deletions(-) delete mode 100644 src/client/components/forms/Form/flattenFilters.js diff --git a/demo/collections/AllFields.js b/demo/collections/AllFields.js index 7f0b2cc322..005c602749 100644 --- a/demo/collections/AllFields.js +++ b/demo/collections/AllFields.js @@ -168,7 +168,9 @@ const AllFields = { required: true, policies: { read: ({ req: { user } }) => Boolean(user), - update: () => false, + update: ({ req: { user } }) => { + return checkRole(['admin'], user); + }, }, }, ], diff --git a/src/client/components/forms/Form/flattenFilters.js b/src/client/components/forms/Form/flattenFilters.js deleted file mode 100644 index bfd3972c5b..0000000000 --- a/src/client/components/forms/Form/flattenFilters.js +++ /dev/null @@ -1,10 +0,0 @@ -const flattenFilters = [{ - test: (_, value) => { - const hasValidProperty = Object.prototype.hasOwnProperty.call(value, 'valid'); - const hasValueProperty = Object.prototype.hasOwnProperty.call(value, 'value'); - - return (hasValidProperty && hasValueProperty); - }, -}]; - -export default flattenFilters; diff --git a/src/client/components/forms/Form/index.js b/src/client/components/forms/Form/index.js index fc492ebbba..642d860138 100644 --- a/src/client/components/forms/Form/index.js +++ b/src/client/components/forms/Form/index.js @@ -254,6 +254,9 @@ const Form = (props) => { contextValue.validateForm = () => { return !Object.values(fields).some((field) => { + if (field.valid === false) { + console.log(field, ' is not valid'); + } return field.valid === false; }); }; diff --git a/src/client/components/forms/field-types/Flexible/index.js b/src/client/components/forms/field-types/Flexible/index.js index df27ef8dde..f10f3b5ec1 100644 --- a/src/client/components/forms/field-types/Flexible/index.js +++ b/src/client/components/forms/field-types/Flexible/index.js @@ -8,7 +8,7 @@ import { v4 as uuidv4 } from 'uuid'; import withCondition from '../../withCondition'; import Button from '../../../elements/Button'; import reducer from '../rowReducer'; -import useForm from '../../Form/useForm'; +import useFormFields from '../../Form/useFormFields'; import DraggableSection from '../../DraggableSection'; import { useRenderedFields } from '../../RenderFields'; import Error from '../../Error'; @@ -67,7 +67,7 @@ const Flexible = (props) => { const dataToInitialize = initialData || defaultValue; const [rows, dispatchRows] = useReducer(reducer, []); const { customComponentsPath } = useRenderedFields(); - const { getDataByPath } = useForm(); + const { getDataByPath } = useFormFields(); const addRow = (index, blockType) => { const data = getDataByPath(path); diff --git a/src/client/components/forms/field-types/Repeater/index.js b/src/client/components/forms/field-types/Repeater/index.js index f4464950f6..29ccb69d69 100644 --- a/src/client/components/forms/field-types/Repeater/index.js +++ b/src/client/components/forms/field-types/Repeater/index.js @@ -8,7 +8,7 @@ import Button from '../../../elements/Button'; import DraggableSection from '../../DraggableSection'; import reducer from '../rowReducer'; import { useRenderedFields } from '../../RenderFields'; -import useForm from '../../Form/useForm'; +import useFormFields from '../../Form/useFormFields'; import useFieldType from '../../useFieldType'; import Error from '../../Error'; import { repeater } from '../../../../../fields/validations'; @@ -39,7 +39,7 @@ const Repeater = (props) => { const dataToInitialize = initialData || defaultValue; const [rows, dispatchRows] = useReducer(reducer, []); const { customComponentsPath } = useRenderedFields(); - const { getDataByPath } = useForm(); + const { getDataByPath } = useFormFields(); const path = pathFromProps || name; diff --git a/src/client/components/forms/field-types/Upload/index.js b/src/client/components/forms/field-types/Upload/index.js index dca24615e3..79dca1c3dd 100644 --- a/src/client/components/forms/field-types/Upload/index.js +++ b/src/client/components/forms/field-types/Upload/index.js @@ -46,7 +46,7 @@ const Upload = (props) => { const fieldType = useFieldType({ path, required, - initialData, + initialData: initialData?.id, defaultValue, validate, }); @@ -166,7 +166,9 @@ Upload.propTypes = { required: PropTypes.bool, readOnly: PropTypes.bool, defaultValue: PropTypes.string, - initialData: PropTypes.shape({}), + initialData: PropTypes.shape({ + id: PropTypes.string, + }), validate: PropTypes.func, width: PropTypes.string, style: PropTypes.shape({}), diff --git a/src/collections/operations/update.js b/src/collections/operations/update.js index 81054ed81d..f2188ec117 100644 --- a/src/collections/operations/update.js +++ b/src/collections/operations/update.js @@ -51,7 +51,7 @@ const update = async (args) => { doc.setLocale(locale, fallbackLocale); } - const docJSON = doc.toJSON({ virtuals: true }); + options.originalDoc = doc.toJSON({ virtuals: true }); // ///////////////////////////////////// // 2. Execute before update hook @@ -67,7 +67,7 @@ const update = async (args) => { // 3. Merge updates into existing data // ///////////////////////////////////// - options.data = deepmerge(docJSON, options.data, { arrayMerge: overwriteMerge }); + options.data = deepmerge(options.originalDoc, options.data, { arrayMerge: overwriteMerge }); // ///////////////////////////////////// // 4. Execute field-level hooks, policies, and validation diff --git a/src/fields/performFieldOperations.js b/src/fields/performFieldOperations.js index 798f1c9c96..e6ba168c1d 100644 --- a/src/fields/performFieldOperations.js +++ b/src/fields/performFieldOperations.js @@ -3,13 +3,14 @@ const { ValidationError } = require('../errors'); module.exports = async (config, operation) => { const { data: fullData, + originalDoc: fullOriginalDoc, operationName, hook, } = operation; // Maintain a top-level list of promises // so that all async field policies / validations / hooks - // can run in tandem + // can run in parallel const validationPromises = []; const policyPromises = []; const hookPromises = []; @@ -28,15 +29,19 @@ module.exports = async (config, operation) => { } }; - const createPolicyPromise = async (data, field) => { + const createPolicyPromise = async (data, originalDoc, field) => { const resultingData = data; if (field.policies && field.policies[operationName]) { const result = await field.policies[operationName](operation); - if (!result) { + if (!result && operationName === 'create') { delete resultingData[field.name]; } + + if (!result && operationName === 'update' && originalDoc[field.name] !== undefined) { + resultingData[field.name] = originalDoc[field.name]; + } } }; @@ -48,7 +53,7 @@ module.exports = async (config, operation) => { } }; - const traverseFields = (fields, data = {}, path) => { + const traverseFields = (fields, data = {}, originalDoc = {}, path) => { fields.forEach((field) => { const dataCopy = data; @@ -63,20 +68,21 @@ module.exports = async (config, operation) => { if (data[field.name] === 'false') dataCopy[field.name] = false; } - policyPromises.push(createPolicyPromise(data, field)); + policyPromises.push(createPolicyPromise(data, originalDoc, field)); hookPromises.push(createHookPromise(data, field)); if (field.fields) { if (field.name === undefined) { - traverseFields(field.fields, data, path); + traverseFields(field.fields, data, originalDoc, path); } else if (field.type === 'repeater' || field.type === 'flexible') { if (Array.isArray(data[field.name])) { data[field.name].forEach((rowData, i) => { - traverseFields(field.fields, rowData, `${path}${field.name}.${i}.`); + const originalDocRow = originalDoc && originalDoc[field.name] && originalDoc[field.name][i]; + traverseFields(field.fields, rowData, originalDocRow || undefined, `${path}${field.name}.${i}.`); }); } } else { - traverseFields(field.fields, data[field.name], `${path}${field.name}.`); + traverseFields(field.fields, data[field.name], originalDoc[field.name], `${path}${field.name}.`); } } @@ -98,7 +104,7 @@ module.exports = async (config, operation) => { // ////////////////////////////////////////// try { - traverseFields(config.fields, fullData, ''); + traverseFields(config.fields, fullData, fullOriginalDoc, ''); await Promise.all(validationPromises); if (errors.length > 0) {