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

Merged
merged 43 commits into from
Jan 9, 2025
Merged
Show file tree
Hide file tree
Changes from 42 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
f8b197b
reworks schema convert mehod to make it more understandable
ValJed Nov 25, 2024
288f460
wip
ValJed Nov 26, 2024
6625d0f
rollbacks code
ValJed Nov 26, 2024
ab34c76
fix backend convert method
ValJed Nov 26, 2024
bead68d
do not run validateAndEmit if field isn't ready
ValJed Nov 26, 2024
893a48e
removes comments
ValJed Nov 26, 2024
f726703
rollbacks aposschema
ValJed Nov 26, 2024
a8a9231
removes emit input mixin
ValJed Nov 26, 2024
e7316ed
updates logicn in choices mixin
ValJed Nov 26, 2024
11b774e
updateNextAndEmit when fieldState is udpated through v-model instead …
ValJed Nov 26, 2024
67f072c
adds constant to validateAndEmit to make code more readable
ValJed Nov 26, 2024
906335d
fix convert method, use data instead of destination inside isVisible …
ValJed Nov 27, 2024
c030360
add changelog
ValJed Nov 27, 2024
2393e96
removes comments
ValJed Nov 27, 2024
d9a426f
reworks to manager fields errors and visible when all converts have b…
ValJed Dec 3, 2024
b7a4a90
adds new tests to check schema nesting isVisible detection
ValJed Dec 3, 2024
477d453
mocha tests passing
ValJed Dec 4, 2024
7dc5db5
wip trying to keep errors format that is nested
ValJed Dec 4, 2024
3497f07
Tests green, keep same error format
ValJed Dec 5, 2024
513257f
working with same errors format, checking for all fields if visible
ValJed Dec 5, 2024
3096067
wip tests
ValJed Dec 5, 2024
cdaa05e
add test
ValJed Dec 5, 2024
98173a4
rename methods
ValJed Dec 5, 2024
ad6f949
fixes paths by using only dots
ValJed Dec 9, 2024
5244c98
prepares support for relationships (no confitional fields currently)
ValJed Dec 9, 2024
04eba94
test nested conditional fields
ValJed Dec 9, 2024
84a532f
lint fixes
ValJed Dec 10, 2024
ebeba68
updates changelog
ValJed Dec 10, 2024
b52050d
builds field schema path in validate instead of convert
ValJed Dec 19, 2024
88938e7
preps two more tests to write
ValJed Dec 19, 2024
aa8c535
skip non visibles checks when current isn't visible and added to list
ValJed Dec 30, 2024
92c0123
changelog typo
ValJed Dec 30, 2024
2d00a1c
passes parent to validate for sub fields of relationship
ValJed Dec 30, 2024
c41f8e4
test aposPath added to fields during validation
ValJed Dec 30, 2024
a27d68d
updates setDefaultToInvisibleFields to handle case where field path c…
ValJed Dec 30, 2024
966ad3c
test setting default values to invisible fields
ValJed Dec 30, 2024
f77638b
reinit validatedSchemas before each test to make them mor isolated..
ValJed Dec 30, 2024
2ef3280
logs errors out of handleErrors method, moves methods to self
ValJed Dec 30, 2024
8b14eed
updates changelog
ValJed Dec 30, 2024
5de55e2
removes useless ancestorSchemas param in convert
ValJed Dec 30, 2024
1508b0a
moves getNonVisibleFields out of convert
ValJed Dec 30, 2024
63eef2a
fix changelog
ValJed Dec 30, 2024
f1f047e
changelog feedback
ValJed Jan 7, 2025
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
7 changes: 6 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@
### Fixes

* Fixes a bug where images in Media manager are not selectable (click on an image does nothing) in both default and relationship mode.
* 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
Copy link
Member

Choose a reason for hiding this comment

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

How about: "eliminated superfluous error messages. The convert method now waits for all recursive invocations to complete before attempting to determine if fields are visible."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

fields (or their parents) are visible and so related errors are discarded.

### Adds

* Possibility to set a field not ready when performing async operations, when a field isn't ready, the validation and emit won't occur.
Copy link
Member

Choose a reason for hiding this comment

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

Fields can now be marked as "not ready" when performing async operations. When a field isn't ready, the validation and emit won't occur.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


## 4.11.1 (2024-12-18)

Expand All @@ -22,7 +28,6 @@
* Adds option to disable `tabindex` on `AposToggle` component. A new prop `disableFocus` can be set to `false` to disable the focus on the toggle button. It's enabled by default.
* Adds support for event on `addContextOperation`, an option `type` can now be passed and can be `modal` (default) or `event`, in this case it does not try to open a modal but emit a bus event using the action as name.


### Fixes

* Focus properly Widget Editor modals when opened. Keep the previous active focus on the modal when closing the widget editor.
Expand Down
288 changes: 209 additions & 79 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)
)
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just linting

const testResults = _.isPlainObject(results?.[0])
? results.some(({ value }) => value)
: results.some((value) => value);
Expand Down Expand Up @@ -585,20 +592,18 @@ module.exports = {
{
fetchRelationships = true,
ancestors = [],
isParentVisible = true
rootConvert = true
} = {}
) {
const options = {
fetchRelationships,
ancestors,
isParentVisible
ancestors
};
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 +616,207 @@ 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;
boutell marked this conversation as resolved.
Show resolved Hide resolved
convertErrors.push(error);
}
}

const errorsList = [];

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);

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;
}
if (!rootConvert) {
if (convertErrors.length) {
throw convertErrors;
}

return;
}

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

const validErrors = await self.handleConvertErrors({
req,
schema,
convertErrors,
destination,
nonVisibleFields
});

for (const error of validErrors) {
self.apos.util.error(error.stack);
}
Copy link
Contributor Author

@ValJed ValJed Dec 30, 2024

Choose a reason for hiding this comment

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

Moved log of errors out of handleErrors method.
Created a ticket to improve globally message printed from convert errors.
Also, log stack only because it contains the error name and message also.


if (validErrors.length) {
throw validErrors;
}
},

async 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) {
nonVisibleFields.add(curPath);
continue;
}
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 self.getNonVisibleFields({
req,
schema: field.schema,
destination: arrayItem,
nonVisibleFields,
fieldPath: `${curPath}.${arrayItem._id}`
});
}
if (isParentVisible === false) {
} else if (field.type === 'object') {
await self.getNonVisibleFields({
req,
schema: field.schema,
destination: destination[field.name],
nonVisibleFields,
fieldPath: curPath
});
}
}

return nonVisibleFields;
},

async handleConvertErrors({
req,
schema,
convertErrors,
nonVisibleFields,
destination,
destinationPath = '',
hiddenAncestors = false
}) {
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);
self.setDefaultToInvisibleField(curDestination, curSchema, error.path);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this method is really useful.
I wrote a test for it and realized I saw it work only for a string that has a pattern.
In the case of in integer having a min or a max for example, the convert updates the value itself, it does not need this method.
Maybe there are some cases I didn't test.

Copy link
Member

Choose a reason for hiding this comment

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

If the field is not visible and never gets touched in the UI, doesn't the frontend submit def anyway? It should.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, maybe it does.
My question was more about in what cases this is useful except for invisible string fields having a pattern that is not respected in the existing data.
I'm asking you because this function simply wraps existing code from you.
Also because you evoke min and max in the comments but I realized the convert already fixed min and max doe invisible fields.

      setDefaultToInvisibleField(destination, schema, fieldPath) {
        // Field path might contain the ID of the object in which it is contained
        // We just want the field name here
        const [ _id, fieldName ] = fieldPath.includes('.')
          ? fieldPath.split('.')
          : [ null, fieldPath ];
        // 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);
        }
      }

So I'm just wondering how useful is it, is it worth keeping it etc. Also how to properly test it.

continue;
}

if (error.data?.errors) {
const subErrors = await self.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;
}
}

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

return validErrors;
},

setDefaultToInvisibleField(destination, schema, fieldPath) {
// Field path might contain the ID of the object in which it is contained
// We just want the field name here
const [ _id, fieldName ] = fieldPath.includes('.')
? fieldPath.split('.')
: [ null, fieldPath ];
// 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);
Copy link
Contributor Author

@ValJed ValJed Dec 30, 2024

Choose a reason for hiding this comment

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

As described above, not sure where this method is useful except for strings have a pattern property.

}
},

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 +825,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
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just linting

const previous = conditionalFields[field.name];
if (previous !== result) {
change = true;
Expand Down Expand Up @@ -1332,19 +1458,23 @@ module.exports = {
// reasonable values for certain properties, such as the `idsStorage` property
// of a `relationship` field, or the `label` property of anything.

validate(schema, options) {
validate(schema, options, parent = null) {
schema.forEach(field => {
// Infinite recursion prevention
const key = `${options.type}:${options.subtype}.${field.name}`;
if (!self.validatedSchemas[key]) {
self.validatedSchemas[key] = true;
self.validateField(field, options);
self.validateField(field, options, parent);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

relationship field uses validateu an not validateField like does array and object fields.
Maybe the three should use the validate method since we can now pass the parent?

}
});
},

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

Choose a reason for hiding this comment

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

Build aposPath by using parent aposPath


const fieldType = self.fieldTypes[field.type];
if (!fieldType) {
fail('Unknown schema field type.');
Expand Down
Loading
Loading