Skip to content
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

Pro 6799 design default values invisible fields #4816

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
0d499c5
reworks schema convert mehod to make it more understandable
ValJed Nov 25, 2024
dbc28ab
wip
ValJed Nov 26, 2024
a1bb170
rollbacks code
ValJed Nov 26, 2024
471f75e
fix backend convert method
ValJed Nov 26, 2024
3bb052e
do not run validateAndEmit if field isn't ready
ValJed Nov 26, 2024
be2ec8b
removes comments
ValJed Nov 26, 2024
1e6bd19
rollbacks aposschema
ValJed Nov 26, 2024
082639b
removes emit input mixin
ValJed Nov 26, 2024
58dc331
updates logicn in choices mixin
ValJed Nov 26, 2024
ee2f0ec
updateNextAndEmit when fieldState is udpated through v-model instead …
ValJed Nov 26, 2024
2e2661b
adds constant to validateAndEmit to make code more readable
ValJed Nov 26, 2024
d137d38
fix convert method, use data instead of destination inside isVisible …
ValJed Nov 27, 2024
fc5666f
add changelog
ValJed Nov 27, 2024
65df650
removes comments
ValJed Nov 27, 2024
9ea5540
reworks to manager fields errors and visible when all converts have b…
ValJed Dec 3, 2024
96e21d5
adds new tests to check schema nesting isVisible detection
ValJed Dec 3, 2024
6dda768
mocha tests passing
ValJed Dec 4, 2024
c350d1c
wip trying to keep errors format that is nested
ValJed Dec 4, 2024
e7adaee
Tests green, keep same error format
ValJed Dec 5, 2024
505a73a
working with same errors format, checking for all fields if visible
ValJed Dec 5, 2024
647a792
wip tests
ValJed Dec 5, 2024
3a1fa44
add test
ValJed Dec 5, 2024
d83e1e6
rename methods
ValJed Dec 5, 2024
cbf794a
fixes paths by using only dots
ValJed Dec 9, 2024
db94f7c
prepares support for relationships (no confitional fields currently)
ValJed Dec 9, 2024
f8e73cd
test nested conditional fields
ValJed Dec 9, 2024
2341d53
lint fixes
ValJed Dec 10, 2024
8aae2c7
updates changelog
ValJed Dec 10, 2024
34b24cd
fixes typo
ValJed Dec 18, 2024
3668419
Merge remote-tracking branch 'origin/main' into pro-6799-design-defau…
ValJed Dec 19, 2024
7a33bc2
builds field schema path in validate instead of convert
ValJed Dec 19, 2024
214b44c
preps two more tests to write
ValJed Dec 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
# Changelog

## UNRELEASED

### Fixes

* In the schema convert method, we wait all sub convert to run, to have access to the final destination object in order to check if
fields (or their parents) are visible and so relates errors discarded.

## 4.11.1 (2024-12-18)

### Fixes
Expand Down Expand Up @@ -50,6 +57,7 @@ rendered output.
* Adds `bundleMarkup` to the data sent to the external front-end, containing all markup for injecting Apostrophe UI in the front-end.
* Warns users when two page types have the same field name, but a different field type. This may cause errors or other problems when an editor switches page types.
* The piece and page `GET` REST APIs now support `?render-areas=inline`. When this parameter is used, an HTML rendering of each widget is added to that specific widget in each area's `items` array as a new `_rendered` property. The existing `?render-areas=1` parameter is still supported to render the entire area as a single `_rendered` property. Note that this older option also causes `items` to be omitted from the response.
* Possibility to set a field not ready when performing async operations, when a field isn't ready, the validation and emit won't occur.

### Changes

Expand Down
274 changes: 202 additions & 72 deletions modules/@apostrophecms/schema/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ module.exports = {
alias: 'schema'
},
init(self) {

self.fieldTypes = {};
self.fieldsById = {};
self.arrayManagers = {};
Expand Down Expand Up @@ -489,7 +488,15 @@ module.exports = {
const destinationKey = _.get(destination, key);

if (key === '$or') {
const results = await Promise.all(val.map(clause => self.evaluateCondition(req, field, clause, destination, conditionalFields)));
const results = await Promise.all(
val.map(clause => self.evaluateCondition(
req,
field,
clause,
destination,
conditionalFields)
)
);
const testResults = _.isPlainObject(results?.[0])
? results.some(({ value }) => value)
: results.some((value) => value);
Expand Down Expand Up @@ -585,20 +592,20 @@ module.exports = {
{
fetchRelationships = true,
ancestors = [],
isParentVisible = true
rootConvert = true,
ancestorSchemas = {}
} = {}
) {
const options = {
fetchRelationships,
ancestors,
isParentVisible
ancestorSchemas
};
if (Array.isArray(req)) {
throw new Error('convert invoked without a req, do you have one in your context?');
}

const errors = [];

const convertErrors = [];
for (const field of schema) {
if (field.readOnly) {
continue;
Expand All @@ -611,92 +618,201 @@ module.exports = {
}

const { convert } = self.fieldTypes[field.type];
if (!convert) {
continue;
}

if (convert) {
try {
const isAllParentsVisible = isParentVisible === false
? false
: await self.isVisible(req, schema, destination, field.name);
const isRequired = await self.isFieldRequired(req, field, destination);
await convert(
req,
{
...field,
required: isRequired
},
data,
destination,
{
...options,
isParentVisible: isAllParentsVisible
}
);
} catch (error) {
if (Array.isArray(error)) {
const invalid = self.apos.error('invalid', {
errors: error
});
invalid.path = field.name;
errors.push(invalid);
} else {
error.path = field.name;
errors.push(error);
try {
const isRequired = await self.isFieldRequired(req, field, destination);
await convert(
req,
{
...field,
required: isRequired
},
data,
destination,
{
...options,
rootConvert: false
}
}
);
} catch (err) {
const error = Array.isArray(err)
? self.apos.error('invalid', { errors: err })
: err;

error.path = field.name;
error.schemaPath = field.aposPath;
convertErrors.push(error);
}
}

const errorsList = [];
if (!rootConvert) {
if (convertErrors.length) {
throw convertErrors;
}

for (const error of errors) {
if (error.path) {
// `self.isVisible` will only throw for required fields that have
// an external condition containing an unknown module or method:
const isVisible = isParentVisible === false
? false
: await self.isVisible(req, schema, destination, error.path);
return;
}

const nonVisibleFields = await getNonVisibleFields({
req,
schema,
destination
});

const errors = await handleConvertErrors({
req,
schema,
convertErrors,
destination,
nonVisibleFields
});

if (errors.length) {
throw errors;
}

async function getNonVisibleFields({
req, schema, destination, nonVisibleFields = new Set(), fieldPath = ''
}) {
for (const field of schema) {
const curPath = fieldPath ? `${fieldPath}.${field.name}` : field.name;
const isVisible = await self.isVisible(req, schema, destination, field.name);
if (!isVisible) {
// It is not reasonable to enforce required,
// min, max or anything else for fields
// hidden via "if" as the user cannot correct it
// and it will not be used. If the user changes
// the conditional field later then they won't
// be able to save until the erroneous field
// is corrected
const name = error.path;
const field = schema.find(field => field.name === name);
if (field) {
// To protect against security issues, an invalid value
// for a field that is not visible should be quietly discarded.
// We only worry about this if the value is not valid, as otherwise
// it's a kindness to save the work so the user can toggle back to it
destination[field.name] = klona((field.def !== undefined)
? field.def
: self.fieldTypes[field.type]?.def);
continue;
nonVisibleFields.add(curPath);
}
if (!field.schema) {
continue;
}

// Relationship does not support conditional fields right now
if ([ 'array' /*, 'relationship' */].includes(field.type) && field.schema) {
for (const arrayItem of destination[field.name] || []) {
await getNonVisibleFields({
req,
schema: field.schema,
destination: arrayItem,
nonVisibleFields,
fieldPath: `${curPath}.${arrayItem._id}`
});
}
} else if (field.type === 'object') {
await getNonVisibleFields({
req,
schema: field.schema,
destination: destination[field.name],
nonVisibleFields,
fieldPath: curPath
});
}
if (isParentVisible === false) {
}

return nonVisibleFields;
}

async function handleConvertErrors({
req,
schema,
convertErrors,
nonVisibleFields,
destination,
destinationPath = '',
hiddenAncestors = false
}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method might be moved somewhere else

const validErrors = [];
for (const error of convertErrors) {
const [ destId, destPath ] = error.path.includes('.')
? error.path.split('.')
: [ null, error.path ];

const curDestination = destId
? destination.find(({ _id }) => _id === destId)
: destination;

const errorPath = destinationPath
? `${destinationPath}.${error.path}`
: error.path;

// Case were this error field hasn't been treated
// Should check if path starts with, because parent can be invisible
const nonVisibleField = hiddenAncestors || nonVisibleFields.has(errorPath);

// We set default values only on final error fields
if (nonVisibleField && !error.data?.errors) {
const curSchema = self.getFieldLevelSchema(schema, error.schemaPath);
setDefaultToInvisibleField(curDestination, curSchema, error.path);
continue;
}

if (error.data?.errors) {
const subErrors = await handleConvertErrors({
req,
schema,
convertErrors: error.data.errors,
nonVisibleFields,
destination: curDestination[destPath],
destinationPath: errorPath,
hiddenAncestors: nonVisibleField
});

// If invalid error has no sub error, this one can be removed
if (!subErrors.length) {
continue;
}

error.data.errors = subErrors;
}

if (typeof error !== 'string') {
self.apos.util.error(error.path + '\n' + error.stack);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me to show what field failed, error path being the name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to log this detail later when logging the overall error. But I wouldn't die on that hill.

Will the error type ever be a string? I believe that went away in A3/A4...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's smart to check.

}
validErrors.push(error);
}

if (!Array.isArray(error) && typeof error !== 'string') {
self.apos.util.error(error + '\n\n' + error.stack);
return validErrors;
}

function setDefaultToInvisibleField(destination, schema, fieldName) {
// It is not reasonable to enforce required,
// min, max or anything else for fields
// hidden via "if" as the user cannot correct it
// and it will not be used. If the user changes
// the conditional field later then they won't
// be able to save until the erroneous field
// is corrected
const field = schema.find(field => field.name === fieldName);
if (field) {
// To protect against security issues, an invalid value
// for a field that is not visible should be quietly discarded.
// We only worry about this if the value is not valid, as otherwise
// it's a kindness to save the work so the user can toggle back to it
destination[field.name] = klona((field.def !== undefined)
? field.def
: self.fieldTypes[field.type]?.def);
}
errorsList.push(error);
}
},

if (errorsList.length) {
throw errorsList;
getFieldLevelSchema(schema, fieldPath) {
if (!fieldPath || fieldPath === '/') {
return schema;
}
let curSchema = schema;
const parts = fieldPath.split('/');
parts.pop();
for (const part of parts) {
const curField = curSchema.find(({ name }) => name === part);
curSchema = curField.schema;
}

return curSchema;
},

// Determine whether the given field is visible
// based on `if` conditions of all fields

async isVisible(req, schema, object, name) {
async isVisible(req, schema, destination, name) {
const conditionalFields = {};
const errors = {};

Expand All @@ -705,7 +821,13 @@ module.exports = {
for (const field of schema) {
if (field.if) {
try {
const result = await self.evaluateCondition(req, field, field.if, object, conditionalFields);
const result = await self.evaluateCondition(
req,
field,
field.if,
destination,
conditionalFields
);
const previous = conditionalFields[field.name];
if (previous !== result) {
change = true;
Expand Down Expand Up @@ -1345,6 +1467,10 @@ module.exports = {

// Validates a single schema field. See `validate`.
validateField(field, options, parent = null) {
field.aposPath = options.ancestorPath
? `${options.ancestorPath}/${field.name}`
: field.name;

const fieldType = self.fieldTypes[field.type];
if (!fieldType) {
fail('Unknown schema field type.');
Expand Down Expand Up @@ -1374,7 +1500,11 @@ module.exports = {
warn(`editPermission or viewPermission must be defined on root fields only, provided on "${parent.name}.${field.name}"`);
}
if (fieldType.validate) {
fieldType.validate(field, options, warn, fail);
const opts = {
...options,
ancestorPath: field.aposPath
};
fieldType.validate(field, opts, warn, fail);
}
// Ancestors hoisting should happen AFTER the validation recursion,
// so that ancestors are processed as well.
Expand Down
Loading
Loading