From f8b197b40d7ee574ebadbab54068edef12f8ce68 Mon Sep 17 00:00:00 2001 From: Jed Date: Mon, 25 Nov 2024 12:16:45 +0100 Subject: [PATCH 01/43] reworks schema convert mehod to make it more understandable --- modules/@apostrophecms/schema/index.js | 70 ++++++++++++-------------- 1 file changed, 31 insertions(+), 39 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index 66cec3f1d7..392f3d453d 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -632,6 +632,15 @@ module.exports = { } ); } catch (error) { + const isVisible = isParentVisible === false + ? false + : await self.isVisible(req, schema, destination, error.path); + + if (!isVisible) { + setDefaultValues(schema, field.name); + continue; + } + if (Array.isArray(error)) { const invalid = self.apos.error('invalid', { errors: error @@ -646,56 +655,39 @@ module.exports = { } } - 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 (isParentVisible === false) { - continue; - } - } - if (!Array.isArray(error) && typeof error !== 'string') { self.apos.util.error(error + '\n\n' + error.stack); } - errorsList.push(error); } - if (errorsList.length) { - throw errorsList; + if (errors.length) { + throw errors; + } + + function setDefaultValues(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); + } } }, // Determine whether the given field is visible // based on `if` conditions of all fields - async isVisible(req, schema, object, name) { const conditionalFields = {}; const errors = {}; From 288f4600c6429551773f4dd027d34cc7289fda7f Mon Sep 17 00:00:00 2001 From: Jed Date: Tue, 26 Nov 2024 10:31:53 +0100 Subject: [PATCH 02/43] wip --- .../components/AposMediaManagerEditor.vue | 2 + .../ui/apos/components/AposInputArray.vue | 1 + .../schema/ui/apos/logic/AposInputSelect.js | 4 ++ .../schema/ui/apos/logic/AposSchema.js | 44 +++++++++++-------- .../ui/apos/mixins/AposInputChoicesMixin.js | 6 +-- .../schema/ui/apos/mixins/AposInputMixin.js | 21 ++++++--- 6 files changed, 48 insertions(+), 30 deletions(-) diff --git a/modules/@apostrophecms/image/ui/apos/components/AposMediaManagerEditor.vue b/modules/@apostrophecms/image/ui/apos/components/AposMediaManagerEditor.vue index 86449f32b5..f2c882cd22 100644 --- a/modules/@apostrophecms/image/ui/apos/components/AposMediaManagerEditor.vue +++ b/modules/@apostrophecms/image/ui/apos/components/AposMediaManagerEditor.vue @@ -60,6 +60,7 @@ /> + diff --git a/modules/@apostrophecms/schema/ui/apos/logic/AposInputSelect.js b/modules/@apostrophecms/schema/ui/apos/logic/AposInputSelect.js index 5a501f0e18..530705334d 100644 --- a/modules/@apostrophecms/schema/ui/apos/logic/AposInputSelect.js +++ b/modules/@apostrophecms/schema/ui/apos/logic/AposInputSelect.js @@ -27,6 +27,10 @@ export default { return 'required'; } + if (this.field.name === 'linkType') { + console.log('value', value); + console.log('this.choices', this.choices); + } if (value && !this.choices.find(choice => choice.value === value)) { return 'invalid'; } diff --git a/modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js b/modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js index caadf492ab..53e59ad346 100644 --- a/modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js +++ b/modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js @@ -99,7 +99,7 @@ export default { ], data() { return { - schemaReady: false, + /* schemaReady: false, */ next: { hasErrors: false, data: {}, @@ -164,6 +164,8 @@ export default { } }, watch: { + // Deep watch update emit when fieldState changes, could we do without this? + // Check where fieldState is updated fieldState: { deep: true, handler() { @@ -228,8 +230,9 @@ export default { } return options; }, + // This method instantiate default value to fieldState, might trigger watcher populateDocData() { - this.schemaReady = false; + /* this.schemaReady = false; */ const next = { hasErrors: false, data: {} @@ -263,15 +266,15 @@ export default { // updating. This is only really a concern in editors that can swap // the active doc/object without unmounting AposSchema. this.$nextTick(() => { - this.schemaReady = true; + /* this.schemaReady = true; */ // Signal that the schema data is ready to be tracked. this.$emit('reset'); }); }, updateNextAndEmit() { - if (!this.schemaReady) { - return; - } + /* if (!this.schemaReady) { */ + /* return; */ + /* } */ const oldHasErrors = this.next.hasErrors; // destructure these for non-linked comparison const oldFieldState = { ...this.next.fieldState }; @@ -282,20 +285,23 @@ export default { this.next.hasErrors = false; this.next.fieldState = { ...this.fieldState }; - this.schema.filter(field => this.displayComponent(field)).forEach(field => { - if (this.fieldState[field.name].error) { - this.next.hasErrors = true; - } - if ( - this.fieldState[field.name].data !== undefined && + this.schema + .filter(field => this.displayComponent(field)) + .forEach(field => { + if (this.fieldState[field.name].error) { + this.next.hasErrors = true; + } + // This simply check if a field has changed since it has been instantiated + if ( + this.fieldState[field.name].data !== undefined && detectFieldChange(field, this.next.data[field.name], this.fieldState[field.name].data) - ) { - changeFound = true; - this.next.data[field.name] = this.fieldState[field.name].data; - } else { - this.next.data[field.name] = this.modelValue.data[field.name]; - } - }); + ) { + changeFound = true; + this.next.data[field.name] = this.fieldState[field.name].data; + } else { + this.next.data[field.name] = this.modelValue.data[field.name]; + } + }); if ( oldHasErrors !== this.next.hasErrors || oldFieldState !== newFieldState diff --git a/modules/@apostrophecms/schema/ui/apos/mixins/AposInputChoicesMixin.js b/modules/@apostrophecms/schema/ui/apos/mixins/AposInputChoicesMixin.js index b74fb2ca90..b03721e37d 100644 --- a/modules/@apostrophecms/schema/ui/apos/mixins/AposInputChoicesMixin.js +++ b/modules/@apostrophecms/schema/ui/apos/mixins/AposInputChoicesMixin.js @@ -11,7 +11,7 @@ export default { data() { return { choices: [], - enableValidate: false + fieldReady: false }; }, @@ -20,6 +20,7 @@ export default { onSuccess: this.updateChoices }); await this.debouncedUpdateChoices.skipDelay(); + this.fieldReady = true; }, watch: { @@ -34,7 +35,6 @@ export default { methods: { async getChoices() { - this.enableValidate = false; if (typeof this.field.choices === 'string') { const action = this.options.action; const response = await apos.http.post( @@ -60,12 +60,10 @@ export default { } }, updateChoices(choices) { - this.enableValidate = true; this.choices = choices; if (this.field.type === 'select') { this.prependEmptyChoice(); } - this.validate(this.next); }, prependEmptyChoice() { // Using `hasOwn` here, not simply checking if `field.def` is truthy diff --git a/modules/@apostrophecms/schema/ui/apos/mixins/AposInputMixin.js b/modules/@apostrophecms/schema/ui/apos/mixins/AposInputMixin.js index 2ee7702682..7a5ad85558 100644 --- a/modules/@apostrophecms/schema/ui/apos/mixins/AposInputMixin.js +++ b/modules/@apostrophecms/schema/ui/apos/mixins/AposInputMixin.js @@ -2,7 +2,7 @@ import { klona } from 'klona'; export default { // Implements v-model pattern - emits: [ 'update:modelValue' ], + emits: [ 'update:modelValue', 'field-ready' ], props: { // The value passed in from the parent component through the v-model // directive. @@ -70,12 +70,14 @@ export default { // in the UI between id attributes uid: Math.random(), // Automatically updated for you, can be watched - focus: false + focus: false, + fieldReady: true }; }, mounted () { this.$el.addEventListener('focusin', this.focusInListener); this.$el.addEventListener('focusout', this.focusOutListener); + this.setFieldReady(); }, unmounted () { this.$el.removeEventListener('focusin', this.focusInListener); @@ -136,16 +138,17 @@ export default { // You must supply the validate method. It receives the // internal representation used for editing (a string, for instance) validateAndEmit () { - if (this.enableValidate === false) { - return; - } // If the field is conditional and isn't shown, disregard any errors. - const error = this.conditionMet === false ? false : this.validate(this.next); + // If field isn't ready we don't want to validate its value + const error = this.conditionMet === false || !this.fieldReady + ? false + : this.validate(this.next); this.$emit('update:modelValue', { data: error ? this.next : this.convert(this.next), error, - ranValidation: this.conditionMet === false ? this.modelValue.ranValidation : true + ranValidation: this.conditionMet === false ? this.modelValue.ranValidation : true, + ready: this.fieldReady }); }, // Allows replacing the current component value externally, e.g. via @@ -206,6 +209,10 @@ export default { } return fieldMeta; + }, + + setFieldReady() { + this.$emit('field-ready', this.field.name); } } }; From 6625d0f0409616f1b1deaa91b5d36306ee930893 Mon Sep 17 00:00:00 2001 From: Jed Date: Tue, 26 Nov 2024 15:43:23 +0100 Subject: [PATCH 03/43] rollbacks code --- .../schema/ui/apos/components/AposInputArray.vue | 1 - .../@apostrophecms/schema/ui/apos/logic/AposInputSelect.js | 4 ---- .../schema/ui/apos/mixins/AposInputChoicesMixin.js | 1 + 3 files changed, 1 insertion(+), 5 deletions(-) diff --git a/modules/@apostrophecms/schema/ui/apos/components/AposInputArray.vue b/modules/@apostrophecms/schema/ui/apos/components/AposInputArray.vue index 705029b0a6..f70d340b7d 100644 --- a/modules/@apostrophecms/schema/ui/apos/components/AposInputArray.vue +++ b/modules/@apostrophecms/schema/ui/apos/components/AposInputArray.vue @@ -274,7 +274,6 @@ @validate="emitValidate()" @keydown.space="$event.stopImmediatePropagation()" @keydown.enter="$event.stopImmediatePropagation()" - @field-ready="setFieldReady" /> diff --git a/modules/@apostrophecms/schema/ui/apos/logic/AposInputSelect.js b/modules/@apostrophecms/schema/ui/apos/logic/AposInputSelect.js index 530705334d..5a501f0e18 100644 --- a/modules/@apostrophecms/schema/ui/apos/logic/AposInputSelect.js +++ b/modules/@apostrophecms/schema/ui/apos/logic/AposInputSelect.js @@ -27,10 +27,6 @@ export default { return 'required'; } - if (this.field.name === 'linkType') { - console.log('value', value); - console.log('this.choices', this.choices); - } if (value && !this.choices.find(choice => choice.value === value)) { return 'invalid'; } diff --git a/modules/@apostrophecms/schema/ui/apos/mixins/AposInputChoicesMixin.js b/modules/@apostrophecms/schema/ui/apos/mixins/AposInputChoicesMixin.js index b03721e37d..68f9dcb3f8 100644 --- a/modules/@apostrophecms/schema/ui/apos/mixins/AposInputChoicesMixin.js +++ b/modules/@apostrophecms/schema/ui/apos/mixins/AposInputChoicesMixin.js @@ -64,6 +64,7 @@ export default { if (this.field.type === 'select') { this.prependEmptyChoice(); } + this.validate(this.next); }, prependEmptyChoice() { // Using `hasOwn` here, not simply checking if `field.def` is truthy From ab34c763e09fe6e8b7c38abd3d1f767704dee828 Mon Sep 17 00:00:00 2001 From: Jed Date: Tue, 26 Nov 2024 17:59:49 +0100 Subject: [PATCH 04/43] fix backend convert method --- modules/@apostrophecms/schema/index.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index 392f3d453d..e2d3bf6ab5 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -634,10 +634,10 @@ module.exports = { } catch (error) { const isVisible = isParentVisible === false ? false - : await self.isVisible(req, schema, destination, error.path); + : await self.isVisible(req, schema, destination, field.name); if (!isVisible) { - setDefaultValues(schema, field.name); + setDefaultToInvisibleField(destination, schema, field.name); continue; } @@ -645,10 +645,8 @@ module.exports = { const invalid = self.apos.error('invalid', { errors: error }); - invalid.path = field.name; errors.push(invalid); } else { - error.path = field.name; errors.push(error); } } @@ -665,7 +663,7 @@ module.exports = { throw errors; } - function setDefaultValues(schema, fieldName) { + 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 From bead68d1fa468f70cf75ab49aaa85a3905ebbfc3 Mon Sep 17 00:00:00 2001 From: Jed Date: Tue, 26 Nov 2024 18:18:07 +0100 Subject: [PATCH 05/43] do not run validateAndEmit if field isn't ready --- .../schema/ui/apos/mixins/AposInputMixin.js | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/modules/@apostrophecms/schema/ui/apos/mixins/AposInputMixin.js b/modules/@apostrophecms/schema/ui/apos/mixins/AposInputMixin.js index 7a5ad85558..ee5e1523e8 100644 --- a/modules/@apostrophecms/schema/ui/apos/mixins/AposInputMixin.js +++ b/modules/@apostrophecms/schema/ui/apos/mixins/AposInputMixin.js @@ -71,13 +71,13 @@ export default { uid: Math.random(), // Automatically updated for you, can be watched focus: false, + // Can be overriden at input component level to handle async field preparation fieldReady: true }; }, mounted () { this.$el.addEventListener('focusin', this.focusInListener); this.$el.addEventListener('focusout', this.focusOutListener); - this.setFieldReady(); }, unmounted () { this.$el.removeEventListener('focusin', this.focusInListener); @@ -138,17 +138,19 @@ export default { // You must supply the validate method. It receives the // internal representation used for editing (a string, for instance) validateAndEmit () { + if (!this.fieldReady) { + return; + } // If the field is conditional and isn't shown, disregard any errors. // If field isn't ready we don't want to validate its value - const error = this.conditionMet === false || !this.fieldReady + const error = this.conditionMet === false ? false : this.validate(this.next); this.$emit('update:modelValue', { data: error ? this.next : this.convert(this.next), error, - ranValidation: this.conditionMet === false ? this.modelValue.ranValidation : true, - ready: this.fieldReady + ranValidation: this.conditionMet === false ? this.modelValue.ranValidation : true }); }, // Allows replacing the current component value externally, e.g. via @@ -209,10 +211,6 @@ export default { } return fieldMeta; - }, - - setFieldReady() { - this.$emit('field-ready', this.field.name); } } }; From 893a48eedee72787b70314262ece1a5041d8c426 Mon Sep 17 00:00:00 2001 From: Jed Date: Tue, 26 Nov 2024 18:18:14 +0100 Subject: [PATCH 06/43] removes comments --- .../image/ui/apos/components/AposMediaManagerEditor.vue | 2 -- 1 file changed, 2 deletions(-) diff --git a/modules/@apostrophecms/image/ui/apos/components/AposMediaManagerEditor.vue b/modules/@apostrophecms/image/ui/apos/components/AposMediaManagerEditor.vue index f2c882cd22..86449f32b5 100644 --- a/modules/@apostrophecms/image/ui/apos/components/AposMediaManagerEditor.vue +++ b/modules/@apostrophecms/image/ui/apos/components/AposMediaManagerEditor.vue @@ -60,7 +60,6 @@ /> - Date: Tue, 26 Nov 2024 18:19:19 +0100 Subject: [PATCH 07/43] rollbacks aposschema --- .../schema/ui/apos/logic/AposSchema.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js b/modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js index 53e59ad346..b33bb6b715 100644 --- a/modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js +++ b/modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js @@ -99,7 +99,7 @@ export default { ], data() { return { - /* schemaReady: false, */ + schemaReady: false, next: { hasErrors: false, data: {}, @@ -166,6 +166,7 @@ export default { watch: { // Deep watch update emit when fieldState changes, could we do without this? // Check where fieldState is updated + // Try updating updateNextAndEmit to pass old and new values instead of storing twice fieldState fieldState: { deep: true, handler() { @@ -232,7 +233,7 @@ export default { }, // This method instantiate default value to fieldState, might trigger watcher populateDocData() { - /* this.schemaReady = false; */ + this.schemaReady = false; const next = { hasErrors: false, data: {} @@ -266,15 +267,16 @@ export default { // updating. This is only really a concern in editors that can swap // the active doc/object without unmounting AposSchema. this.$nextTick(() => { - /* this.schemaReady = true; */ + this.schemaReady = true; // Signal that the schema data is ready to be tracked. this.$emit('reset'); }); }, + // Use Old and New values instead of storing fieldState twice.. (at root and inside next) updateNextAndEmit() { - /* if (!this.schemaReady) { */ - /* return; */ - /* } */ + if (!this.schemaReady) { + return; + } const oldHasErrors = this.next.hasErrors; // destructure these for non-linked comparison const oldFieldState = { ...this.next.fieldState }; From a8a923118e54811757d26d954b2644f75855f0e4 Mon Sep 17 00:00:00 2001 From: Jed Date: Tue, 26 Nov 2024 18:20:20 +0100 Subject: [PATCH 08/43] removes emit input mixin --- modules/@apostrophecms/schema/ui/apos/mixins/AposInputMixin.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/@apostrophecms/schema/ui/apos/mixins/AposInputMixin.js b/modules/@apostrophecms/schema/ui/apos/mixins/AposInputMixin.js index ee5e1523e8..f0398df66b 100644 --- a/modules/@apostrophecms/schema/ui/apos/mixins/AposInputMixin.js +++ b/modules/@apostrophecms/schema/ui/apos/mixins/AposInputMixin.js @@ -2,7 +2,7 @@ import { klona } from 'klona'; export default { // Implements v-model pattern - emits: [ 'update:modelValue', 'field-ready' ], + emits: [ 'update:modelValue' ], props: { // The value passed in from the parent component through the v-model // directive. From e7316ed3f0094a2ef9af8a713ed3a4b07e9942c2 Mon Sep 17 00:00:00 2001 From: Jed Date: Tue, 26 Nov 2024 18:24:40 +0100 Subject: [PATCH 09/43] updates logicn in choices mixin --- .../schema/ui/apos/mixins/AposInputChoicesMixin.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/modules/@apostrophecms/schema/ui/apos/mixins/AposInputChoicesMixin.js b/modules/@apostrophecms/schema/ui/apos/mixins/AposInputChoicesMixin.js index 68f9dcb3f8..f15f603943 100644 --- a/modules/@apostrophecms/schema/ui/apos/mixins/AposInputChoicesMixin.js +++ b/modules/@apostrophecms/schema/ui/apos/mixins/AposInputChoicesMixin.js @@ -35,6 +35,7 @@ export default { methods: { async getChoices() { + this.fieldReady = false; if (typeof this.field.choices === 'string') { const action = this.options.action; const response = await apos.http.post( @@ -61,6 +62,7 @@ export default { }, updateChoices(choices) { this.choices = choices; + this.fieldReady = true; if (this.field.type === 'select') { this.prependEmptyChoice(); } From 11b774e5de3bac8b08ba56c953d1d18175228819 Mon Sep 17 00:00:00 2001 From: Jed Date: Tue, 26 Nov 2024 18:27:06 +0100 Subject: [PATCH 10/43] updateNextAndEmit when fieldState is udpated through v-model instead of deep watching it --- .../schema/ui/apos/components/AposSchema.vue | 1 + .../@apostrophecms/schema/ui/apos/logic/AposSchema.js | 9 --------- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/modules/@apostrophecms/schema/ui/apos/components/AposSchema.vue b/modules/@apostrophecms/schema/ui/apos/components/AposSchema.vue index 39c8d6db34..733237c61a 100644 --- a/modules/@apostrophecms/schema/ui/apos/components/AposSchema.vue +++ b/modules/@apostrophecms/schema/ui/apos/components/AposSchema.vue @@ -52,6 +52,7 @@ :server-error="fields[field.name].serverError" :doc-id="docId" :generation="generation" + @update:model-value="updateNextAndEmit" @update-doc-data="onUpdateDocData" @validate="emitValidate()" /> diff --git a/modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js b/modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js index b33bb6b715..a8dde10fe3 100644 --- a/modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js +++ b/modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js @@ -164,15 +164,6 @@ export default { } }, watch: { - // Deep watch update emit when fieldState changes, could we do without this? - // Check where fieldState is updated - // Try updating updateNextAndEmit to pass old and new values instead of storing twice fieldState - fieldState: { - deep: true, - handler() { - this.updateNextAndEmit(); - } - }, schema() { this.populateDocData(); }, From 67f072c68fc55232904228b36db5afabb49ee562 Mon Sep 17 00:00:00 2001 From: Jed Date: Tue, 26 Nov 2024 18:32:42 +0100 Subject: [PATCH 11/43] adds constant to validateAndEmit to make code more readable --- .../schema/ui/apos/mixins/AposInputMixin.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/modules/@apostrophecms/schema/ui/apos/mixins/AposInputMixin.js b/modules/@apostrophecms/schema/ui/apos/mixins/AposInputMixin.js index f0398df66b..a9c6f1a124 100644 --- a/modules/@apostrophecms/schema/ui/apos/mixins/AposInputMixin.js +++ b/modules/@apostrophecms/schema/ui/apos/mixins/AposInputMixin.js @@ -143,14 +143,15 @@ export default { } // If the field is conditional and isn't shown, disregard any errors. // If field isn't ready we don't want to validate its value - const error = this.conditionMet === false - ? false - : this.validate(this.next); + const shouldValidate = this.conditionMet !== false; + const error = shouldValidate + ? this.validate(this.next) + : false; this.$emit('update:modelValue', { data: error ? this.next : this.convert(this.next), error, - ranValidation: this.conditionMet === false ? this.modelValue.ranValidation : true + ranValidation: shouldValidate ? true : this.modelValue.ranValidation }); }, // Allows replacing the current component value externally, e.g. via From 906335dcea88ca4283522fc772763c5198c58ace Mon Sep 17 00:00:00 2001 From: Jed Date: Wed, 27 Nov 2024 10:32:48 +0100 Subject: [PATCH 12/43] fix convert method, use data instead of destination inside isVisible method --- modules/@apostrophecms/schema/index.js | 69 +++++++++++++------------- 1 file changed, 35 insertions(+), 34 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index e2d3bf6ab5..cf34207b2c 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -612,43 +612,44 @@ module.exports = { const { convert } = self.fieldTypes[field.type]; - 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) { - const isVisible = isParentVisible === false - ? false - : await self.isVisible(req, schema, destination, field.name); + if (!convert) { + continue; + } - if (!isVisible) { - setDefaultToInvisibleField(destination, schema, field.name); - continue; - } + const isCurrentVisible = isParentVisible === false + ? false + : await self.isVisible(req, schema, data, field.name); - if (Array.isArray(error)) { - const invalid = self.apos.error('invalid', { - errors: error - }); - errors.push(invalid); - } else { - errors.push(error); + try { + const isRequired = await self.isFieldRequired(req, field, destination); + await convert( + req, + { + ...field, + required: isRequired + }, + data, + destination, + { + ...options, + isParentVisible: isCurrentVisible } + ); + } catch (error) { + if (!isCurrentVisible) { + setDefaultToInvisibleField(destination, schema, field.name); + continue; + } + + 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); } } } From c030360fc90b2b26d69caa046805a3d3551e6f18 Mon Sep 17 00:00:00 2001 From: Jed Date: Wed, 27 Nov 2024 11:07:18 +0100 Subject: [PATCH 13/43] add changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bae0d1812..3f7e1fdfaf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,7 @@ did not actually use any noncompliant cookie names or values, so there was no vu rendered output. * Search bar will perform the search even if the bar is empty allowing to reset a search. * Fixes Color picker being hidden in an inline array schema field, also fixes rgba inputs going off the modal. +* Simplifies the `convert` method from schema module. ### Adds @@ -56,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 From 2393e96aa40d782ddb70fa64b94dcc25bde2d56a Mon Sep 17 00:00:00 2001 From: Jed Date: Wed, 27 Nov 2024 14:21:18 +0100 Subject: [PATCH 14/43] removes comments --- modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js b/modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js index a8dde10fe3..076e69e4c0 100644 --- a/modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js +++ b/modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js @@ -222,7 +222,6 @@ export default { } return options; }, - // This method instantiate default value to fieldState, might trigger watcher populateDocData() { this.schemaReady = false; const next = { @@ -263,7 +262,6 @@ export default { this.$emit('reset'); }); }, - // Use Old and New values instead of storing fieldState twice.. (at root and inside next) updateNextAndEmit() { if (!this.schemaReady) { return; From d9a426f71f47ddeaa142303b5d97e93ff61f4df5 Mon Sep 17 00:00:00 2001 From: Jed Date: Tue, 3 Dec 2024 18:44:03 +0100 Subject: [PATCH 15/43] reworks to manager fields errors and visible when all converts have been ran --- modules/@apostrophecms/schema/index.js | 120 +++++++++++++----- .../schema/lib/addFieldTypes.js | 18 ++- 2 files changed, 102 insertions(+), 36 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index cf34207b2c..e686e17b4b 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -24,7 +24,6 @@ module.exports = { alias: 'schema' }, init(self) { - self.fieldTypes = {}; self.fieldsById = {}; self.arrayManagers = {}; @@ -585,20 +584,22 @@ module.exports = { { fetchRelationships = true, ancestors = [], - isParentVisible = true + rootConvert = true, + ancestorSchemas = {}, + ancestorPath = '' } = {} ) { const options = { fetchRelationships, ancestors, - isParentVisible + ancestorSchemas, + ancestorPath }; 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; @@ -611,15 +612,11 @@ module.exports = { } const { convert } = self.fieldTypes[field.type]; - if (!convert) { continue; } - const isCurrentVisible = isParentVisible === false - ? false - : await self.isVisible(req, schema, data, field.name); - + const fieldPath = ancestorPath ? `${ancestorPath}/${field.name}` : field.name; try { const isRequired = await self.isFieldRequired(req, field, destination); await convert( @@ -632,31 +629,48 @@ module.exports = { destination, { ...options, - isParentVisible: isCurrentVisible + rootConvert: false, + ancestorPath: fieldPath } ); } catch (error) { - if (!isCurrentVisible) { - setDefaultToInvisibleField(destination, schema, field.name); - continue; - } + error.name = field.name; + error.path = fieldPath; + convertErrors.push(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); - } + if (!rootConvert) { + if (convertErrors.length) { + throw self.apos.error('invalid', { errors: convertErrors }); } + + return; } - for (const error of errors) { - if (!Array.isArray(error) && typeof error !== 'string') { - self.apos.util.error(error + '\n\n' + error.stack); + const nonVisibleFields = new Set(); + const errors = []; + for (const error of convertErrors) { + if (!error.path) { + continue; + } + + for (const err of error.data?.errors || [ error ]) { + let curPath = ''; + const ancestors = error.path.split('/').reverse(); + for (const ancestor of ancestors) { + curPath = curPath ? `${curPath}/${ancestor}` : ancestor; + if (nonVisibleFields.has(curPath)) { + continue; + } + + const curSchema = self.getFieldSchema(schema, curPath); + // TODO: find schema lol + + await handleError(req, err, error.schema, destination, errors); + nonVisibleFields.add(curPath); + } + } } @@ -664,6 +678,25 @@ module.exports = { throw errors; } + async function handleError(req, error, schema, destination, errors) { + const isVisible = await self.isVisible( + req, + schema, + destination, + error.path + ); + + if (!isVisible) { + setDefaultToInvisibleField(destination, schema, error.path); + return; + } + + errors.push(error); + if (!Array.isArray(error) && typeof error !== 'string') { + self.apos.util.error(error + '\n\n' + error.stack); + } + } + function setDefaultToInvisibleField(destination, schema, fieldName) { // It is not reasonable to enforce required, // min, max or anything else for fields @@ -685,18 +718,45 @@ module.exports = { } }, + getFieldSchema(schema, fieldPath) { + let curSchema = schema; + const parts = fieldPath.split('/'); + for (const part of parts) { + const curField = curSchema.find(({ name }) => name === part); + curSchema = curField.schema; + if (!curSchema) { + return null; + } + } + + 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 = {}; + /* for (const [ ancestor, ancestorSchema ] of Object.entries(ancestorSchemas)) { */ + /* const ancestorVisible = await self.isVisible(req, ancestorSchema, destination, ancestor); */ + /* if (!ancestorVisible) { */ + /* return false; */ + /* } */ + /* } */ + while (true) { let change = false; 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; diff --git a/modules/@apostrophecms/schema/lib/addFieldTypes.js b/modules/@apostrophecms/schema/lib/addFieldTypes.js index 2c9e7d5c75..cf363cb312 100644 --- a/modules/@apostrophecms/schema/lib/addFieldTypes.js +++ b/modules/@apostrophecms/schema/lib/addFieldTypes.js @@ -751,7 +751,8 @@ module.exports = (self) => { { fetchRelationships = true, ancestors = [], - isParentVisible = true + rootConvert = true, + ancestorPath = '' } = {} ) { const schema = field.schema; @@ -777,7 +778,8 @@ module.exports = (self) => { const options = { fetchRelationships, ancestors: [ ...ancestors, destination ], - isParentVisible + rootConvert, + ancestorPath }; await self.convert(req, schema, datum, result, options); } catch (e) { @@ -867,7 +869,8 @@ module.exports = (self) => { { fetchRelationships = true, ancestors = {}, - isParentVisible = true, + rootConvert = true, + ancestorPath = '', doc = {} } = {} ) { @@ -881,7 +884,8 @@ module.exports = (self) => { const options = { fetchRelationships, ancestors: [ ...ancestors, destination ], - isParentVisible + rootConvert, + ancestorPath }; if (data == null || typeof data !== 'object' || Array.isArray(data)) { data = {}; @@ -978,12 +982,14 @@ module.exports = (self) => { destination, { fetchRelationships = true, - isParentVisible = true + rootConvert = true, + ancestorPath = '' } = {} ) { const options = { fetchRelationships, - isParentVisible + rootConvert, + ancestorPath }; const manager = self.apos.doc.getManager(field.withType); if (!manager) { From b7a4a90a00c101bd4bfb4fca8c6f05f50af94db0 Mon Sep 17 00:00:00 2001 From: Jed Date: Tue, 3 Dec 2024 18:44:24 +0100 Subject: [PATCH 16/43] adds new tests to check schema nesting isVisible detection --- test/schemas.js | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/test/schemas.js b/test/schemas.js index 1c106919d8..818bad91c6 100644 --- a/test/schemas.js +++ b/test/schemas.js @@ -3129,6 +3129,43 @@ describe('Schemas', function() { assert(!output.requiredProp); }); + it.only('should not error nested required property if parent is not visible', async function() { + const req = apos.task.getReq(); + const schema = apos.schema.compose({ + addFields: [ + { + name: 'object', + type: 'object', + if: { + showObject: true + }, + schema: [ + { + name: 'subfield', + type: 'string', + required: true + } + ] + }, + { + name: 'showObject', + type: 'boolean' + } + ] + }); + const output = {}; + + try { + await apos.schema.convert(req, schema, { + showObject: false + }, output); + assert(true); + } catch (err) { + console.log('err', err); + assert(!err); + } + }); + it('should error required property nested boolean', async function() { const schema = apos.schema.compose({ addFields: [ From 477d45366e93a34f4346171cd8571adc8fff8b50 Mon Sep 17 00:00:00 2001 From: Jed Date: Wed, 4 Dec 2024 12:07:03 +0100 Subject: [PATCH 17/43] mocha tests passing --- modules/@apostrophecms/schema/index.js | 60 ++++++++++++++++++-------- 1 file changed, 42 insertions(+), 18 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index e686e17b4b..b65eb82e29 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -634,8 +634,8 @@ module.exports = { } ); } catch (error) { - error.name = field.name; - error.path = fieldPath; + error.path = field.name; + error.fieldPath = fieldPath; convertErrors.push(error); } } @@ -648,29 +648,57 @@ module.exports = { return; } - const nonVisibleFields = new Set(); + // Do we need this, not sure a field specific error can appears twice? + // TODO: Put it in function and use a reduce + const handledFieldErrors = new Map(); const errors = []; for (const error of convertErrors) { - if (!error.path) { + if (!error.fieldPath) { continue; } for (const err of error.data?.errors || [ error ]) { + // We rebuild the path let curPath = ''; - const ancestors = error.path.split('/').reverse(); + + const ancestors = err.fieldPath.split('/'); + let ancestorsVisible = true; + for (const ancestor of ancestors) { + // Gets schema of the direct parent or root + const curSchema = self.getFieldSchema(schema, curPath); curPath = curPath ? `${curPath}/${ancestor}` : ancestor; - if (nonVisibleFields.has(curPath)) { + + // Case were field error has been handled already + const curFieldErrorHandled = handledFieldErrors.has(curPath); + if (curFieldErrorHandled) { + const curFieldVisible = handledFieldErrors.get(curPath); + if (!curFieldVisible) { + ancestorsVisible = false; + } continue; } - const curSchema = self.getFieldSchema(schema, curPath); - // TODO: find schema lol + // Case were this error field hasn't been treated + const isCurVisible = ancestorsVisible === false + ? false + : await self.isVisible(req, curSchema, destination, ancestor); + + console.log('curPath', curPath); + console.log('isCurVisible', isCurVisible); + console.log('curSchema', curSchema); + handledFieldErrors.set(curPath, isCurVisible); + if (!isCurVisible) { + setDefaultToInvisibleField(destination, curSchema, ancestor); + ancestorsVisible = false; + continue; + } - await handleError(req, err, error.schema, destination, errors); - nonVisibleFields.add(curPath); + if (!Array.isArray(error) && typeof error !== 'string') { + self.apos.util.error(error + '\n\n' + error.stack); + } + errors.push(err); } - } } @@ -719,6 +747,9 @@ module.exports = { }, getFieldSchema(schema, fieldPath) { + if (!fieldPath || fieldPath === '/') { + return schema; + } let curSchema = schema; const parts = fieldPath.split('/'); for (const part of parts) { @@ -738,13 +769,6 @@ module.exports = { const conditionalFields = {}; const errors = {}; - /* for (const [ ancestor, ancestorSchema ] of Object.entries(ancestorSchemas)) { */ - /* const ancestorVisible = await self.isVisible(req, ancestorSchema, destination, ancestor); */ - /* if (!ancestorVisible) { */ - /* return false; */ - /* } */ - /* } */ - while (true) { let change = false; for (const field of schema) { From 7dc5db53464033d22c5cda577245903b8a569cb4 Mon Sep 17 00:00:00 2001 From: Jed Date: Wed, 4 Dec 2024 18:53:36 +0100 Subject: [PATCH 18/43] wip trying to keep errors format that is nested --- modules/@apostrophecms/schema/index.js | 84 ++++++++++++++------------ 1 file changed, 46 insertions(+), 38 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index b65eb82e29..e784ed9fe4 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -616,7 +616,7 @@ module.exports = { continue; } - const fieldPath = ancestorPath ? `${ancestorPath}/${field.name}` : field.name; + const fieldPath = ancestorPath ? `${ancestorPath}.${field.name}` : field.name; try { const isRequired = await self.isFieldRequired(req, field, destination); await convert( @@ -633,41 +633,64 @@ module.exports = { ancestorPath: fieldPath } ); - } catch (error) { + } catch (err) { + const error = Array.isArray(err) + ? self.apos.error('invalid', { errors: err }) + : [ err ]; + error.path = field.name; - error.fieldPath = fieldPath; + error.schemaPath = fieldPath; convertErrors.push(error); } } if (!rootConvert) { if (convertErrors.length) { - throw self.apos.error('invalid', { errors: convertErrors }); + throw convertErrors; } return; } + const errors = await handleConvertErrors({ + req, + schema, + convertErrors, + destination + }); + // Do we need this, not sure a field specific error can appears twice? // TODO: Put it in function and use a reduce - const handledFieldErrors = new Map(); - const errors = []; - for (const error of convertErrors) { - if (!error.fieldPath) { - continue; - } + if (errors.length) { + throw errors; + } - for (const err of error.data?.errors || [ error ]) { + async function handleConvertErrors({ + req, + schema, + errors, + destination, + handledFieldErrors = new Map() + }) { + for (const error of convertErrors) { + if (!error.schemaPath) { + continue; + } + // // We rebuild the path let curPath = ''; + const ancestors = error.schemaPath.split('.'); + const errorField = ancestors.pop(); - const ancestors = err.fieldPath.split('/'); + console.log('ancestors', ancestors); + console.log('errorField', errorField); let ancestorsVisible = true; for (const ancestor of ancestors) { + /* const isErrorField = ancestors.length === index + 1; */ // Gets schema of the direct parent or root const curSchema = self.getFieldSchema(schema, curPath); - curPath = curPath ? `${curPath}/${ancestor}` : ancestor; + curPath = curPath ? `${curPath}.${ancestor}` : ancestor; // Case were field error has been handled already const curFieldErrorHandled = handledFieldErrors.has(curPath); @@ -684,11 +707,12 @@ module.exports = { ? false : await self.isVisible(req, curSchema, destination, ancestor); - console.log('curPath', curPath); - console.log('isCurVisible', isCurVisible); - console.log('curSchema', curSchema); handledFieldErrors.set(curPath, isCurVisible); + if (!isCurVisible) { + // Should we run this on schema field like object / array, as well as on subfields? + // Also this should run on the error field only, not on all ancestors? + // We just want to check visibility for parents setDefaultToInvisibleField(destination, curSchema, ancestor); ancestorsVisible = false; continue; @@ -697,34 +721,18 @@ module.exports = { if (!Array.isArray(error) && typeof error !== 'string') { self.apos.util.error(error + '\n\n' + error.stack); } - errors.push(err); - } - } - } - - if (errors.length) { - throw errors; - } - async function handleError(req, error, schema, destination, errors) { - const isVisible = await self.isVisible( - req, - schema, - destination, - error.path - ); + // TODO: Handle recursion to know if we keep all sub errors, and even the current one + // Maybe we can pass the concerned destination part to allow setting default value without reworking + // `setDefaultToInvisibleField` function - if (!isVisible) { - setDefaultToInvisibleField(destination, schema, error.path); - return; - } + errors.push(error); + } - errors.push(error); - if (!Array.isArray(error) && typeof error !== 'string') { - self.apos.util.error(error + '\n\n' + error.stack); } } + // TODO: function to get right destination level function setDefaultToInvisibleField(destination, schema, fieldName) { // It is not reasonable to enforce required, // min, max or anything else for fields From 3497f074525b86bd08c6697f520320e531582b53 Mon Sep 17 00:00:00 2001 From: Jed Date: Thu, 5 Dec 2024 11:12:41 +0100 Subject: [PATCH 19/43] Tests green, keep same error format --- modules/@apostrophecms/schema/index.js | 141 ++++++++++++++----------- 1 file changed, 82 insertions(+), 59 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index e784ed9fe4..38f68246dc 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -488,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); @@ -636,7 +644,7 @@ module.exports = { } catch (err) { const error = Array.isArray(err) ? self.apos.error('invalid', { errors: err }) - : [ err ]; + : err; error.path = field.name; error.schemaPath = fieldPath; @@ -652,12 +660,17 @@ module.exports = { return; } - const errors = await handleConvertErrors({ - req, - schema, - convertErrors, - destination - }); + let errors; + try { + errors = await handleConvertErrors({ + req, + schema, + convertErrors, + destination + }); + + } catch (err) { + } // Do we need this, not sure a field specific error can appears twice? // TODO: Put it in function and use a reduce @@ -668,68 +681,78 @@ module.exports = { async function handleConvertErrors({ req, schema, - errors, + convertErrors, destination, - handledFieldErrors = new Map() + checkedAncestors = new Map() }) { + const validErrors = []; for (const error of convertErrors) { - if (!error.schemaPath) { - continue; - } - // - // We rebuild the path - let curPath = ''; + const ancestors = error.schemaPath.split('.'); const errorField = ancestors.pop(); - console.log('ancestors', ancestors); - console.log('errorField', errorField); - let ancestorsVisible = true; - - for (const ancestor of ancestors) { - /* const isErrorField = ancestors.length === index + 1; */ - // Gets schema of the direct parent or root - const curSchema = self.getFieldSchema(schema, curPath); - curPath = curPath ? `${curPath}.${ancestor}` : ancestor; - - // Case were field error has been handled already - const curFieldErrorHandled = handledFieldErrors.has(curPath); - if (curFieldErrorHandled) { - const curFieldVisible = handledFieldErrors.get(curPath); - if (!curFieldVisible) { - ancestorsVisible = false; - } - continue; - } + const hasAncestorsVisible = await checkAncestorsVisible( + ancestors, + schema, + destination + ); - // Case were this error field hasn't been treated - const isCurVisible = ancestorsVisible === false - ? false - : await self.isVisible(req, curSchema, destination, ancestor); + const errorSchema = self.getFieldSchema(schema, error.schemaPath) || schema; + // Case were this error field hasn't been treated + const isFieldVisible = hasAncestorsVisible === false + ? false + : await self.isVisible(req, errorSchema, destination, errorField); - handledFieldErrors.set(curPath, isCurVisible); + // In this case do we need to check sub errors and set default values to them, + // we might not since we already set default value to parent to avoid errors? + if (!isFieldVisible) { + /* setDefaultToInvisibleField(destination, curSchema, ancestor); */ + continue; + } + + if (!Array.isArray(error) && typeof error !== 'string') { + self.apos.util.error(error + '\n\n' + error.stack); + } + + if (error.data?.errors) { + const subErrors = await handleConvertErrors({ + req, + schema, + convertErrors: error.data.errors, + destination + }); - if (!isCurVisible) { - // Should we run this on schema field like object / array, as well as on subfields? - // Also this should run on the error field only, not on all ancestors? - // We just want to check visibility for parents - setDefaultToInvisibleField(destination, curSchema, ancestor); - ancestorsVisible = false; + if (!subErrors.length) { continue; } + error.data.errors = subErrors; + } - if (!Array.isArray(error) && typeof error !== 'string') { - self.apos.util.error(error + '\n\n' + error.stack); - } + validErrors.push(error); + } - // TODO: Handle recursion to know if we keep all sub errors, and even the current one - // Maybe we can pass the concerned destination part to allow setting default value without reworking - // `setDefaultToInvisibleField` function + return validErrors; + } - errors.push(error); - } + // TODO: Handle checkedAncestors + async function checkAncestorsVisible(ancestors, schema, destination, fieldPath = '') { + if (!ancestors.length) { + return true; + } + + const ancestor = ancestors.shift(); + const ancestorPath = fieldPath ? `${fieldPath}.${ancestor}` : ancestor; + + /* if (checkedAncestors.has(ancestorPath)) */ + const curSchema = self.getFieldSchema(schema, fieldPath); + const ancestorVisible = await self.isVisible(req, curSchema, destination, ancestor); + + if (!ancestor.length || !ancestorVisible) { + return ancestorVisible; } + + return checkAncestorsVisible(ancestors, schema, destination, ancestorPath); } // TODO: function to get right destination level @@ -755,17 +778,17 @@ module.exports = { }, getFieldSchema(schema, fieldPath) { - if (!fieldPath || fieldPath === '/') { + if (!fieldPath || fieldPath === '.') { return schema; } let curSchema = schema; - const parts = fieldPath.split('/'); + const parts = fieldPath.split('.'); for (const part of parts) { const curField = curSchema.find(({ name }) => name === part); - curSchema = curField.schema; - if (!curSchema) { - return null; + if (!curField.schema) { + return curSchema; } + curSchema = curField.schema; } return curSchema; From 513257f7e6f8b09501b48d4bde6706e9b92e241d Mon Sep 17 00:00:00 2001 From: Jed Date: Thu, 5 Dec 2024 17:29:23 +0100 Subject: [PATCH 20/43] working with same errors format, checking for all fields if visible --- modules/@apostrophecms/schema/index.js | 148 +++++++++++++++---------- 1 file changed, 88 insertions(+), 60 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index 38f68246dc..5f6cf65258 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -660,58 +660,100 @@ module.exports = { return; } - let errors; - try { - errors = await handleConvertErrors({ - req, - schema, - convertErrors, - destination - }); + const nonVisibleFields = await setNonVisibleFields({ + req, + schema, + destination + }); - } catch (err) { - } + console.log('nonVisibleFields', nonVisibleFields); + + const errors = await handleConvertErrors({ + req, + schema, + convertErrors, + destination, + nonVisibleFields + }); + + console.dir(destination, { depth: 5 }); - // Do we need this, not sure a field specific error can appears twice? - // TODO: Put it in function and use a reduce if (errors.length) { throw errors; } + async function setNonVisibleFields({ + 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; + } + + if (field.type === 'array') { + for (const arrayItem of destination[field.name]) { + await setNonVisibleFields({ + req, + schema: field.schema, + destination: arrayItem, + nonVisibleFields, + fieldPath: `${curPath}.${arrayItem._id}` + }); + } + } else if (field.type === 'object') { + await setNonVisibleFields({ + req, + schema: field.schema, + destination: destination[field.name], + nonVisibleFields, + fieldPath: curPath + }); + } else if (field.type === 'relationship') { + // TODO + } + } + + return nonVisibleFields; + } + async function handleConvertErrors({ req, schema, convertErrors, + nonVisibleFields, destination, - checkedAncestors = new Map() + destinationPath = '', + hiddenAncestors = false }) { const validErrors = []; for (const error of convertErrors) { + const [ destId, destPath ] = error.path.includes('.') + ? error.path.split('.') + : [ null, error.path ]; - const ancestors = error.schemaPath.split('.'); - const errorField = ancestors.pop(); + const curDestination = destId + ? destination.find(({ _id }) => _id === destId) + : destination; - const hasAncestorsVisible = await checkAncestorsVisible( - ancestors, - schema, - destination - ); + const errorPath = destinationPath + ? `${destinationPath}/${error.path}` + : error.path; - const errorSchema = self.getFieldSchema(schema, error.schemaPath) || schema; // Case were this error field hasn't been treated - const isFieldVisible = hasAncestorsVisible === false - ? false - : await self.isVisible(req, errorSchema, destination, errorField); - - // In this case do we need to check sub errors and set default values to them, - // we might not since we already set default value to parent to avoid errors? - if (!isFieldVisible) { - /* setDefaultToInvisibleField(destination, curSchema, ancestor); */ - continue; - } + // Should check if path starts with, because parent can be invisible + const nonVisibleField = hiddenAncestors || nonVisibleFields.has(errorPath); - if (!Array.isArray(error) && typeof error !== 'string') { - self.apos.util.error(error + '\n\n' + error.stack); + if (nonVisibleField && !error.data?.errors) { + const curSchema = self.getFieldLevelSchema(schema, error.schemaPath); + // Only on final errors fields + setDefaultToInvisibleField(curDestination, curSchema, error.path); + continue; } if (error.data?.errors) { @@ -719,43 +761,28 @@ module.exports = { req, schema, convertErrors: error.data.errors, - destination + 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 + '\n\n' + error.stack); + } validErrors.push(error); } return validErrors; } - // TODO: Handle checkedAncestors - async function checkAncestorsVisible(ancestors, schema, destination, fieldPath = '') { - if (!ancestors.length) { - return true; - } - - const ancestor = ancestors.shift(); - const ancestorPath = fieldPath ? `${fieldPath}.${ancestor}` : ancestor; - - /* if (checkedAncestors.has(ancestorPath)) */ - - const curSchema = self.getFieldSchema(schema, fieldPath); - const ancestorVisible = await self.isVisible(req, curSchema, destination, ancestor); - - if (!ancestor.length || !ancestorVisible) { - return ancestorVisible; - } - - return checkAncestorsVisible(ancestors, schema, destination, ancestorPath); - } - - // TODO: function to get right destination level function setDefaultToInvisibleField(destination, schema, fieldName) { // It is not reasonable to enforce required, // min, max or anything else for fields @@ -777,17 +804,18 @@ module.exports = { } }, - getFieldSchema(schema, fieldPath) { + 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); - if (!curField.schema) { - return curSchema; - } + /* if (!curField.schema) { */ + /* return curSchema; */ + /* } */ curSchema = curField.schema; } From 3096067da2eecba96be558bbcd6c7dac936b8c3d Mon Sep 17 00:00:00 2001 From: Jed Date: Thu, 5 Dec 2024 17:29:40 +0100 Subject: [PATCH 21/43] wip tests --- modules/@apostrophecms/schema/index.js | 13 +-- test/schemas.js | 146 ++++++++++++++++++++++++- 2 files changed, 150 insertions(+), 9 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index 5f6cf65258..ef08f09a45 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -624,7 +624,7 @@ module.exports = { continue; } - const fieldPath = ancestorPath ? `${ancestorPath}.${field.name}` : field.name; + const fieldPath = ancestorPath ? `${ancestorPath}/${field.name}` : field.name; try { const isRequired = await self.isFieldRequired(req, field, destination); await convert( @@ -692,7 +692,7 @@ module.exports = { nonVisibleFields.add(curPath); continue; } - if (field.schema) { + if (!field.schema) { continue; } @@ -749,9 +749,9 @@ module.exports = { // 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); - // Only on final errors fields setDefaultToInvisibleField(curDestination, curSchema, error.path); continue; } @@ -805,17 +805,14 @@ module.exports = { }, getFieldLevelSchema(schema, fieldPath) { - if (!fieldPath || fieldPath === '.') { + if (!fieldPath || fieldPath === '/') { return schema; } let curSchema = schema; - const parts = fieldPath.split('.'); + const parts = fieldPath.split('/'); parts.pop(); for (const part of parts) { const curField = curSchema.find(({ name }) => name === part); - /* if (!curField.schema) { */ - /* return curSchema; */ - /* } */ curSchema = curField.schema; } diff --git a/test/schemas.js b/test/schemas.js index 818bad91c6..9ee9012d79 100644 --- a/test/schemas.js +++ b/test/schemas.js @@ -3129,7 +3129,8 @@ describe('Schemas', function() { assert(!output.requiredProp); }); - it.only('should not error nested required property if parent is not visible', async function() { + // HERE + it('should not error nested required property if parent is not visible', async function() { const req = apos.task.getReq(); const schema = apos.schema.compose({ addFields: [ @@ -3160,12 +3161,155 @@ describe('Schemas', function() { showObject: false }, output); assert(true); + } catch (err) { + assert(!err); + } + }); + + it.only('should not error complex nested required property if parents are not visible', async function() { + const req = apos.task.getReq(); + const schema = apos.schema.compose({ + addFields: [ + { + name: 'object', + type: 'object', + if: { + showObject: true + }, + schema: [ + { + name: 'objectString', + type: 'string', + required: true + }, + { + name: 'objectArray', + type: 'array', + required: true, + if: { + showObjectArray: true + }, + schema: [ + { + name: 'objectArrayString', + type: 'string', + required: true + } + ] + }, + { + name: 'showObjectArray', + type: 'boolean' + } + ] + }, + { + name: 'showObject', + type: 'boolean' + } + ] + }); + const output = {}; + + try { + await apos.schema.convert(req, schema, { + object: { + objectString: 'toto', + objectArray: [ + { + _id: 'tutu', + metaType: 'arrayItem' + } + ], + showObjectArray: false + }, + showObject: true + }, output); + assert(true); } catch (err) { console.log('err', err); assert(!err); } }); + it.skip('should not error complex nested arrays required property if parents are not visible', async function() { + const req = apos.task.getReq(); + const schema = apos.schema.compose({ + addFields: [ + { + name: 'root', + type: 'array', + if: { + showRoot: true + }, + schema: [ + { + name: 'rootString', + type: 'string', + required: true, + if: { + showrootArray: true + } + }, + { + name: 'rootArray', + type: 'array', + required: true, + schema: [ + { + name: 'rootArrayString', + type: 'string', + required: true, + if: { + showRootArray: true + } + }, + { + name: 'showRootArray', + type: 'boolean' + } + ] + } + ] + }, + { + name: 'showRoot', + type: 'boolean' + } + ] + }); + const output = {}; + + try { + await apos.schema.convert(req, schema, { + root: [ + { + _id: 'root_id', + metaType: 'arrayItem', + rootString: 'toto', + rootArray: [ + { + _id: 'root_array_id', + metaType: 'arrayItem', + rootArrayBool: true + }, + { + _id: 'root_array_id2', + metaType: 'arrayItem', + rootArrayBool: true, + showRootArray: true + } + ] + } + ], + showRoot: true + }, output); + assert(true); + } catch (err) { + assert(!err); + } + }); + it('should error required property nested boolean', async function() { const schema = apos.schema.compose({ addFields: [ From cdaa05e98dd466e33d7af246c8715367986ea350 Mon Sep 17 00:00:00 2001 From: Jed Date: Thu, 5 Dec 2024 18:13:19 +0100 Subject: [PATCH 22/43] add test --- test/schemas.js | 59 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 12 deletions(-) diff --git a/test/schemas.js b/test/schemas.js index 9ee9012d79..eac98e263b 100644 --- a/test/schemas.js +++ b/test/schemas.js @@ -3166,8 +3166,7 @@ describe('Schemas', function() { } }); - it.only('should not error complex nested required property if parents are not visible', async function() { - const req = apos.task.getReq(); + it('should not error complex nested required property if parents are not visible', async function() { const schema = apos.schema.compose({ addFields: [ { @@ -3211,28 +3210,64 @@ describe('Schemas', function() { }); const output = {}; - try { - await apos.schema.convert(req, schema, { + const [ success, error ] = await convert(schema, output); + + const expected = { + success: true, + error: false, + output: { object: { + _id: output.object._id, objectString: 'toto', objectArray: [ { _id: 'tutu', - metaType: 'arrayItem' + metaType: 'arrayItem', + scopedArrayName: undefined, + objectArrayString: '' } ], - showObjectArray: false + showObjectArray: false, + metaType: 'objectItem', + scopedObjectName: undefined }, showObject: true - }, output); - assert(true); - } catch (err) { - console.log('err', err); - assert(!err); + } + }; + + const actual = { + success, + error, + output + }; + + assert.deepEqual(expected, actual); + + async function convert(schema, output) { + const req = apos.task.getReq(); + try { + await apos.schema.convert(req, schema, { + object: { + objectString: 'toto', + objectArray: [ + { + _id: 'tutu', + metaType: 'arrayItem' + } + ], + showObjectArray: false + }, + showObject: true + }, output); + return [ true, false ]; + } catch (err) { + return [ false, true ]; + } } }); - it.skip('should not error complex nested arrays required property if parents are not visible', async function() { + // TODO: green + relationship test + it.only('should not error complex nested arrays required property if parents are not visible', async function() { const req = apos.task.getReq(); const schema = apos.schema.compose({ addFields: [ From 98173a4d6c73716fd53c0ab38ae84db49a5088dd Mon Sep 17 00:00:00 2001 From: Jed Date: Thu, 5 Dec 2024 18:16:03 +0100 Subject: [PATCH 23/43] rename methods --- modules/@apostrophecms/schema/index.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index ef08f09a45..23772e9196 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -660,7 +660,7 @@ module.exports = { return; } - const nonVisibleFields = await setNonVisibleFields({ + const nonVisibleFields = await getNonVisibleFields({ req, schema, destination @@ -682,7 +682,7 @@ module.exports = { throw errors; } - async function setNonVisibleFields({ + async function getNonVisibleFields({ req, schema, destination, nonVisibleFields = new Set(), fieldPath = '' }) { for (const field of schema) { @@ -698,7 +698,7 @@ module.exports = { if (field.type === 'array') { for (const arrayItem of destination[field.name]) { - await setNonVisibleFields({ + await getNonVisibleFields({ req, schema: field.schema, destination: arrayItem, @@ -707,7 +707,7 @@ module.exports = { }); } } else if (field.type === 'object') { - await setNonVisibleFields({ + await getNonVisibleFields({ req, schema: field.schema, destination: destination[field.name], From ad6f949c0ee5da3c2424065ce0f26ffee363553d Mon Sep 17 00:00:00 2001 From: Jed Date: Mon, 9 Dec 2024 14:30:45 +0100 Subject: [PATCH 24/43] fixes paths by using only dots --- modules/@apostrophecms/schema/index.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index 23772e9196..eb9156c67c 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -666,8 +666,6 @@ module.exports = { destination }); - console.log('nonVisibleFields', nonVisibleFields); - const errors = await handleConvertErrors({ req, schema, @@ -676,8 +674,6 @@ module.exports = { nonVisibleFields }); - console.dir(destination, { depth: 5 }); - if (errors.length) { throw errors; } @@ -686,7 +682,7 @@ module.exports = { req, schema, destination, nonVisibleFields = new Set(), fieldPath = '' }) { for (const field of schema) { - const curPath = fieldPath ? `${fieldPath}/${field.name}` : field.name; + const curPath = fieldPath ? `${fieldPath}.${field.name}` : field.name; const isVisible = await self.isVisible(req, schema, destination, field.name); if (!isVisible) { nonVisibleFields.add(curPath); @@ -742,7 +738,7 @@ module.exports = { : destination; const errorPath = destinationPath - ? `${destinationPath}/${error.path}` + ? `${destinationPath}.${error.path}` : error.path; // Case were this error field hasn't been treated @@ -771,6 +767,7 @@ module.exports = { if (!subErrors.length) { continue; } + error.data.errors = subErrors; } From 5244c98fe4156849f23b9f0672a1552b4c7f683d Mon Sep 17 00:00:00 2001 From: Jed Date: Mon, 9 Dec 2024 19:02:57 +0100 Subject: [PATCH 25/43] prepares support for relationships (no confitional fields currently) --- modules/@apostrophecms/schema/index.js | 9 ++++----- modules/@apostrophecms/schema/lib/addFieldTypes.js | 9 +++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index eb9156c67c..0ed3418870 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -692,8 +692,9 @@ module.exports = { continue; } - if (field.type === 'array') { - for (const arrayItem of destination[field.name]) { + // 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, @@ -710,8 +711,6 @@ module.exports = { nonVisibleFields, fieldPath: curPath }); - } else if (field.type === 'relationship') { - // TODO } } @@ -772,7 +771,7 @@ module.exports = { } if (typeof error !== 'string') { - self.apos.util.error(error + '\n\n' + error.stack); + self.apos.util.error(error.path + '\n' + error.stack); } validErrors.push(error); } diff --git a/modules/@apostrophecms/schema/lib/addFieldTypes.js b/modules/@apostrophecms/schema/lib/addFieldTypes.js index cf363cb312..2db1f24ae3 100644 --- a/modules/@apostrophecms/schema/lib/addFieldTypes.js +++ b/modules/@apostrophecms/schema/lib/addFieldTypes.js @@ -1073,15 +1073,16 @@ module.exports = (self) => { if (result) { actualDocs.push(result); } - } else if ((item && ((typeof item._id) === 'string'))) { + } else if ((item && (typeof item._id === 'string'))) { const result = results.find(doc => (doc._id === item._id)); if (result) { if (field.schema) { + const destItem = (Array.isArray(destination[field.name]) ? destination[field.name] : []) + .find((doc) => doc._id === item._id); result._fields = { - ...(destination[field.name] - ?.find?.(doc => doc._id === item._id) - ?._fields || {}) + ...destItem?._fields || {} }; + if (item && ((typeof item._fields === 'object'))) { await self.convert(req, field.schema, item._fields || {}, result._fields, options); } From 04eba944fd651597fc86cda2c12b0f3c8c382f1a Mon Sep 17 00:00:00 2001 From: Jed Date: Mon, 9 Dec 2024 19:03:17 +0100 Subject: [PATCH 26/43] test nested conditional fields --- test/schemas.js | 525 +++++++++++++++++++++++++++++++----------------- 1 file changed, 346 insertions(+), 179 deletions(-) diff --git a/test/schemas.js b/test/schemas.js index eac98e263b..5483e128af 100644 --- a/test/schemas.js +++ b/test/schemas.js @@ -3166,185 +3166,6 @@ describe('Schemas', function() { } }); - it('should not error complex nested required property if parents are not visible', async function() { - const schema = apos.schema.compose({ - addFields: [ - { - name: 'object', - type: 'object', - if: { - showObject: true - }, - schema: [ - { - name: 'objectString', - type: 'string', - required: true - }, - { - name: 'objectArray', - type: 'array', - required: true, - if: { - showObjectArray: true - }, - schema: [ - { - name: 'objectArrayString', - type: 'string', - required: true - } - ] - }, - { - name: 'showObjectArray', - type: 'boolean' - } - ] - }, - { - name: 'showObject', - type: 'boolean' - } - ] - }); - const output = {}; - - const [ success, error ] = await convert(schema, output); - - const expected = { - success: true, - error: false, - output: { - object: { - _id: output.object._id, - objectString: 'toto', - objectArray: [ - { - _id: 'tutu', - metaType: 'arrayItem', - scopedArrayName: undefined, - objectArrayString: '' - } - ], - showObjectArray: false, - metaType: 'objectItem', - scopedObjectName: undefined - }, - showObject: true - } - }; - - const actual = { - success, - error, - output - }; - - assert.deepEqual(expected, actual); - - async function convert(schema, output) { - const req = apos.task.getReq(); - try { - await apos.schema.convert(req, schema, { - object: { - objectString: 'toto', - objectArray: [ - { - _id: 'tutu', - metaType: 'arrayItem' - } - ], - showObjectArray: false - }, - showObject: true - }, output); - return [ true, false ]; - } catch (err) { - return [ false, true ]; - } - } - }); - - // TODO: green + relationship test - it.only('should not error complex nested arrays required property if parents are not visible', async function() { - const req = apos.task.getReq(); - const schema = apos.schema.compose({ - addFields: [ - { - name: 'root', - type: 'array', - if: { - showRoot: true - }, - schema: [ - { - name: 'rootString', - type: 'string', - required: true, - if: { - showrootArray: true - } - }, - { - name: 'rootArray', - type: 'array', - required: true, - schema: [ - { - name: 'rootArrayString', - type: 'string', - required: true, - if: { - showRootArray: true - } - }, - { - name: 'showRootArray', - type: 'boolean' - } - ] - } - ] - }, - { - name: 'showRoot', - type: 'boolean' - } - ] - }); - const output = {}; - - try { - await apos.schema.convert(req, schema, { - root: [ - { - _id: 'root_id', - metaType: 'arrayItem', - rootString: 'toto', - rootArray: [ - { - _id: 'root_array_id', - metaType: 'arrayItem', - rootArrayBool: true - }, - { - _id: 'root_array_id2', - metaType: 'arrayItem', - rootArrayBool: true, - showRootArray: true - } - ] - } - ], - showRoot: true - }, output); - assert(true); - } catch (err) { - assert(!err); - } - }); - it('should error required property nested boolean', async function() { const schema = apos.schema.compose({ addFields: [ @@ -5162,7 +4983,322 @@ describe('Schemas', function() { await testSchemaError(schema, {}, 'age', 'required'); }); + + it('should not error complex nested object required property if parents are not visible', async function() { + const schema = apos.schema.compose({ + addFields: [ + { + name: 'object', + type: 'object', + if: { + showObject: true + }, + schema: [ + { + name: 'objectString', + type: 'string', + required: true + }, + { + name: 'objectArray', + type: 'array', + required: true, + if: { + showObjectArray: true + }, + schema: [ + { + name: 'objectArrayString', + type: 'string', + required: true + } + ] + }, + { + name: 'showObjectArray', + type: 'boolean' + } + ] + }, + { + name: 'showObject', + type: 'boolean' + } + ] + }); + + const data = { + object: { + objectString: 'toto', + objectArray: [ + { + _id: 'tutu', + metaType: 'arrayItem' + } + ], + showObjectArray: false + }, + showObject: true + }; + + const output = {}; + const [ success, errors ] = await testConvert(apos, data, schema, output); + + const expected = { + success: true, + errors: [], + output: { + object: { + _id: output.object._id, + objectString: 'toto', + objectArray: [ + { + _id: 'tutu', + metaType: 'arrayItem', + scopedArrayName: undefined, + objectArrayString: '' + } + ], + showObjectArray: false, + metaType: 'objectItem', + scopedObjectName: undefined + }, + showObject: true + } + }; + + const actual = { + success, + errors, + output + }; + + assert.deepEqual(expected, actual); + + }); + + it('should not error complex nested arrays required property if parents are not visible', async function() { + const req = apos.task.getReq(); + const schema = apos.schema.compose({ + addFields: [ + { + name: 'root', + type: 'array', + if: { + showRoot: true + }, + schema: [ + { + name: 'rootString', + type: 'string', + required: true + }, + { + name: 'rootArray', + type: 'array', + required: true, + schema: [ + { + name: 'rootArrayString', + type: 'string', + required: true, + if: { + showRootArrayString: true + } + }, + { + name: 'showRootArrayString', + type: 'boolean' + } + ] + } + ] + }, + { + name: 'showRoot', + type: 'boolean' + } + ] + }); + + const data1 = { + root: [ + { + _id: 'root_id', + metaType: 'arrayItem', + rootString: 'toto', + rootArray: [ + { + _id: 'root_array_id', + metaType: 'arrayItem', + showRootArrayString: true + }, + { + _id: 'root_array_id2', + metaType: 'arrayItem', + rootArrayBool: true, + showRootArrayString: false + } + ] + } + ], + showRoot: true + }; + + const output1 = {}; + const [ success1, errors1 ] = await testConvert(apos, data1, schema, output1); + const foundError1 = findError(errors1, 'root_array_id.rootArrayString', 'required'); + + const data2 = { + root: [ + { + _id: 'root_id', + metaType: 'arrayItem', + rootString: 'toto', + rootArray: [ + { + _id: 'root_array_id', + metaType: 'arrayItem', + rootArrayString: 'Item 1', + showRootArrayString: true + }, + { + _id: 'root_array_id2', + metaType: 'arrayItem', + rootArrayBool: true, + showRootArrayString: false + } + ] + } + ], + showRoot: true + }; + + const output2 = {}; + const [ success2, errors2 ] = await testConvert(apos, data2, schema, output2); + + const expected = { + success1: false, + foundError1: true, + success2: true, + errors2: [], + output2: { + root: [ + { + _id: 'root_id', + metaType: 'arrayItem', + scopedArrayName: undefined, + rootString: 'toto', + rootArray: [ + { + _id: 'root_array_id', + metaType: 'arrayItem', + scopedArrayName: undefined, + rootArrayString: 'Item 1', + showRootArrayString: true + }, + { + _id: 'root_array_id2', + metaType: 'arrayItem', + scopedArrayName: undefined, + rootArrayString: '', + showRootArrayString: false + } + ] + } + ], + showRoot: true + } + }; + + const actual = { + success1, + foundError1, + success2, + errors2, + output2 + }; + + assert.deepEqual(expected, actual); + }); + + // TODO: update this test when support for conditional fields is added to relationships schemas + it('should not error complex nested relationships required property if parents are not visible', async function() { + const req = apos.task.getReq({ mode: 'draft' }); + const schema = apos.schema.compose({ + addFields: [ + { + name: 'title', + type: 'string', + required: true + }, + { + name: '_rel', + type: 'relationship', + withType: 'article', + schema: [ + { + name: 'relString', + type: 'string', + required: true, + if: { + showRelString: true + } + }, + { + name: 'showRelString', + type: 'boolean', + } + ] + } + ] + }); + + const article1 = await apos.article.insert(req, { + ...apos.article.newInstance(), + title: 'article 1' + }) + const article2 = await apos.article.insert(req, { + ...apos.article.newInstance(), + title: 'article 2' + }) + + article1._fields = { + showRelString: false + } + + article2._fields = { + relString: 'article 2 rel string', + showRelString: true + } + + const data = { + title: 'toto', + _rel: [ + article1, + article2 + ] + }; + + const errPath = `_rel.${article1._id}.relString` + const output = {}; + const [ success, errors ] = await testConvert(apos, data, schema, output); + const foundError = findError(errors, 'relString', 'required'); + + const expected = { + success: false, + foundError: true + } + + const actual = { + success, + foundError, + }; + + assert.deepEqual(expected, actual); + }); }); + async function testSchemaError(schema, input, path, name) { const req = apos.task.getReq(); const result = {}; @@ -5181,3 +5317,34 @@ describe('Schemas', function() { } } }); + +async function testConvert( + apos, + data, + schema, + output +) { + const req = apos.task.getReq({mode: 'draft'}); + try { + await apos.schema.convert(req, schema, data, output); + return [ true, [] ]; + } catch (err) { + return [ false, err ]; + } +} + +function findError(errors, fieldPath, errorName) { + if (!Array.isArray(errors)) { + return false + } + return errors.some((err) => { + if (err.data?.errors) { + const deepFound = findError(err.data.errors, fieldPath, errorName); + if (deepFound) { + return deepFound; + } + } + + return err.name === errorName && err.path === fieldPath; + }); +} From 84a532f77dc2ece994b2c2fdd3cd5cb610dd7f5f Mon Sep 17 00:00:00 2001 From: Jed Date: Tue, 10 Dec 2024 09:42:32 +0100 Subject: [PATCH 27/43] lint fixes --- modules/@apostrophecms/schema/index.js | 2 +- test/schemas.js | 20 +++++++++----------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index 0ed3418870..170a45379d 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -693,7 +693,7 @@ module.exports = { } // Relationship does not support conditional fields right now - if (['array' /*, 'relationship' */].includes(field.type) && field.schema) { + if ([ 'array' /*, 'relationship' */].includes(field.type) && field.schema) { for (const arrayItem of destination[field.name] || []) { await getNonVisibleFields({ req, diff --git a/test/schemas.js b/test/schemas.js index 5483e128af..f6e67f899e 100644 --- a/test/schemas.js +++ b/test/schemas.js @@ -5078,7 +5078,6 @@ describe('Schemas', function() { }); it('should not error complex nested arrays required property if parents are not visible', async function() { - const req = apos.task.getReq(); const schema = apos.schema.compose({ addFields: [ { @@ -5247,7 +5246,7 @@ describe('Schemas', function() { }, { name: 'showRelString', - type: 'boolean', + type: 'boolean' } ] } @@ -5257,20 +5256,20 @@ describe('Schemas', function() { const article1 = await apos.article.insert(req, { ...apos.article.newInstance(), title: 'article 1' - }) + }); const article2 = await apos.article.insert(req, { ...apos.article.newInstance(), title: 'article 2' - }) + }); article1._fields = { showRelString: false - } + }; article2._fields = { relString: 'article 2 rel string', showRelString: true - } + }; const data = { title: 'toto', @@ -5280,7 +5279,6 @@ describe('Schemas', function() { ] }; - const errPath = `_rel.${article1._id}.relString` const output = {}; const [ success, errors ] = await testConvert(apos, data, schema, output); const foundError = findError(errors, 'relString', 'required'); @@ -5288,11 +5286,11 @@ describe('Schemas', function() { const expected = { success: false, foundError: true - } + }; const actual = { success, - foundError, + foundError }; assert.deepEqual(expected, actual); @@ -5324,7 +5322,7 @@ async function testConvert( schema, output ) { - const req = apos.task.getReq({mode: 'draft'}); + const req = apos.task.getReq({ mode: 'draft' }); try { await apos.schema.convert(req, schema, data, output); return [ true, [] ]; @@ -5335,7 +5333,7 @@ async function testConvert( function findError(errors, fieldPath, errorName) { if (!Array.isArray(errors)) { - return false + return false; } return errors.some((err) => { if (err.data?.errors) { From ebeba68d6985a2ef1ace015453d2c4085e5ac130 Mon Sep 17 00:00:00 2001 From: Jed Date: Tue, 10 Dec 2024 10:14:49 +0100 Subject: [PATCH 28/43] updates changelog --- CHANGELOG.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f7e1fdfaf..3b79f9469a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,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. @@ -48,7 +47,6 @@ did not actually use any noncompliant cookie names or values, so there was no vu rendered output. * Search bar will perform the search even if the bar is empty allowing to reset a search. * Fixes Color picker being hidden in an inline array schema field, also fixes rgba inputs going off the modal. -* Simplifies the `convert` method from schema module. ### Adds From b52050db8149c9b46aa77c85e24db620983cd624 Mon Sep 17 00:00:00 2001 From: Jed Date: Thu, 19 Dec 2024 17:12:12 +0100 Subject: [PATCH 29/43] builds field schema path in validate instead of convert --- modules/@apostrophecms/schema/index.js | 23 +++++++++++-------- .../schema/lib/addFieldTypes.js | 16 ++++--------- 2 files changed, 18 insertions(+), 21 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index 170a45379d..477130e04b 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -593,15 +593,13 @@ module.exports = { fetchRelationships = true, ancestors = [], rootConvert = true, - ancestorSchemas = {}, - ancestorPath = '' + ancestorSchemas = {} } = {} ) { const options = { fetchRelationships, ancestors, - ancestorSchemas, - ancestorPath + ancestorSchemas }; if (Array.isArray(req)) { throw new Error('convert invoked without a req, do you have one in your context?'); @@ -624,7 +622,6 @@ module.exports = { continue; } - const fieldPath = ancestorPath ? `${ancestorPath}/${field.name}` : field.name; try { const isRequired = await self.isFieldRequired(req, field, destination); await convert( @@ -637,8 +634,7 @@ module.exports = { destination, { ...options, - rootConvert: false, - ancestorPath: fieldPath + rootConvert: false } ); } catch (err) { @@ -647,7 +643,7 @@ module.exports = { : err; error.path = field.name; - error.schemaPath = fieldPath; + error.schemaPath = field.aposPath; convertErrors.push(error); } } @@ -686,7 +682,6 @@ module.exports = { const isVisible = await self.isVisible(req, schema, destination, field.name); if (!isVisible) { nonVisibleFields.add(curPath); - continue; } if (!field.schema) { continue; @@ -1472,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.'); @@ -1501,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. diff --git a/modules/@apostrophecms/schema/lib/addFieldTypes.js b/modules/@apostrophecms/schema/lib/addFieldTypes.js index 2db1f24ae3..493827fa0e 100644 --- a/modules/@apostrophecms/schema/lib/addFieldTypes.js +++ b/modules/@apostrophecms/schema/lib/addFieldTypes.js @@ -751,8 +751,7 @@ module.exports = (self) => { { fetchRelationships = true, ancestors = [], - rootConvert = true, - ancestorPath = '' + rootConvert = true } = {} ) { const schema = field.schema; @@ -778,8 +777,7 @@ module.exports = (self) => { const options = { fetchRelationships, ancestors: [ ...ancestors, destination ], - rootConvert, - ancestorPath + rootConvert }; await self.convert(req, schema, datum, result, options); } catch (e) { @@ -870,7 +868,6 @@ module.exports = (self) => { fetchRelationships = true, ancestors = {}, rootConvert = true, - ancestorPath = '', doc = {} } = {} ) { @@ -884,8 +881,7 @@ module.exports = (self) => { const options = { fetchRelationships, ancestors: [ ...ancestors, destination ], - rootConvert, - ancestorPath + rootConvert }; if (data == null || typeof data !== 'object' || Array.isArray(data)) { data = {}; @@ -982,14 +978,12 @@ module.exports = (self) => { destination, { fetchRelationships = true, - rootConvert = true, - ancestorPath = '' + rootConvert = true } = {} ) { const options = { fetchRelationships, - rootConvert, - ancestorPath + rootConvert }; const manager = self.apos.doc.getManager(field.withType); if (!manager) { From 88938e753abbb1410909711117f303d4d4476c11 Mon Sep 17 00:00:00 2001 From: Jed Date: Thu, 19 Dec 2024 17:49:37 +0100 Subject: [PATCH 30/43] preps two more tests to write --- test/schemas.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/schemas.js b/test/schemas.js index f6e67f899e..01548ff5d8 100644 --- a/test/schemas.js +++ b/test/schemas.js @@ -5295,6 +5295,14 @@ describe('Schemas', function() { assert.deepEqual(expected, actual); }); + + it('should add proper aposPath to all fields when validation schema', async function () { + // TODO + }); + + it('should set default value to invisible fields that triggered convert errors', async function () { + // TODO + }); }); async function testSchemaError(schema, input, path, name) { From aa8c5355f96ae40961955c5a424f318ad7d9305b Mon Sep 17 00:00:00 2001 From: Jed Date: Mon, 30 Dec 2024 09:58:23 +0100 Subject: [PATCH 31/43] skip non visibles checks when current isn't visible and added to list --- modules/@apostrophecms/schema/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index 477130e04b..0bef62ace3 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -682,6 +682,7 @@ module.exports = { const isVisible = await self.isVisible(req, schema, destination, field.name); if (!isVisible) { nonVisibleFields.add(curPath); + continue; } if (!field.schema) { continue; From 92c012380012768f60e2e1ea21bbf7fcb383d605 Mon Sep 17 00:00:00 2001 From: Jed Date: Mon, 30 Dec 2024 09:58:29 +0100 Subject: [PATCH 32/43] changelog typo --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b79f9469a..8e25d96acd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,12 @@ ### Fixes +<<<<<<< HEAD * 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 +fields (or their parents) are visible and so related errors are discarded. +>>>>>>> e1fd5bd94 (changelog typo) ## 4.11.1 (2024-12-18) From 2d00a1c39355efdd5de539d6e55fc4f2ff1a3128 Mon Sep 17 00:00:00 2001 From: Jed Date: Mon, 30 Dec 2024 11:13:13 +0100 Subject: [PATCH 33/43] passes parent to validate for sub fields of relationship --- modules/@apostrophecms/schema/index.js | 14 +++++--------- modules/@apostrophecms/schema/lib/addFieldTypes.js | 2 +- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index 0bef62ace3..243918b935 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -1455,21 +1455,21 @@ 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); } }); }, // Validates a single schema field. See `validate`. validateField(field, options, parent = null) { - field.aposPath = options.ancestorPath - ? `${options.ancestorPath}/${field.name}` + field.aposPath = parent + ? `${parent.aposPath}/${field.name}` : field.name; const fieldType = self.fieldTypes[field.type]; @@ -1501,11 +1501,7 @@ module.exports = { warn(`editPermission or viewPermission must be defined on root fields only, provided on "${parent.name}.${field.name}"`); } if (fieldType.validate) { - const opts = { - ...options, - ancestorPath: field.aposPath - }; - fieldType.validate(field, opts, warn, fail); + fieldType.validate(field, options, warn, fail); } // Ancestors hoisting should happen AFTER the validation recursion, // so that ancestors are processed as well. diff --git a/modules/@apostrophecms/schema/lib/addFieldTypes.js b/modules/@apostrophecms/schema/lib/addFieldTypes.js index 493827fa0e..77ca432aee 100644 --- a/modules/@apostrophecms/schema/lib/addFieldTypes.js +++ b/modules/@apostrophecms/schema/lib/addFieldTypes.js @@ -1219,7 +1219,7 @@ module.exports = (self) => { self.validate(_field.schema, { type: 'relationship', subtype: _field.withType - }); + }, _field); if (!_field.fieldsStorage) { _field.fieldsStorage = _field.name.replace(/^_/, '') + 'Fields'; } From c41f8e4b124b2056af3ccaa62c08f54a5056678d Mon Sep 17 00:00:00 2001 From: Jed Date: Mon, 30 Dec 2024 11:32:25 +0100 Subject: [PATCH 34/43] test aposPath added to fields during validation --- test/schemas.js | 96 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 2 deletions(-) diff --git a/test/schemas.js b/test/schemas.js index 01548ff5d8..ac7bb66673 100644 --- a/test/schemas.js +++ b/test/schemas.js @@ -5296,8 +5296,100 @@ describe('Schemas', function() { assert.deepEqual(expected, actual); }); - it('should add proper aposPath to all fields when validation schema', async function () { - // TODO + it.only('should add proper aposPath to all fields when validation schema', async function () { + const schema = apos.schema.compose({ + addFields: [ + { + name: 'title', + type: 'string', + required: true + }, + { + name: '_rel', + type: 'relationship', + withType: 'article', + schema: [ + { + name: 'relString', + type: 'string' + } + ] + }, + { + name: 'array', + type: 'array', + schema: [ + { + name: 'arrayString', + type: 'string' + }, + { + name: 'arrayObject', + type: 'object', + schema: [ + { + name: 'arrayObjectString', + type: 'string' + } + ] + } + ] + }, + { + name: 'object', + type: 'object', + schema: [ + { + name: 'objectString', + type: 'string' + }, + { + name: 'objectArray', + type: 'array', + schema: [ + { + name: 'objectArrayString', + type: 'string' + } + ] + } + ] + } + ] + }); + + apos.schema.validate(schema, 'article'); + + const [ titleField, relField, arrayField, objectField ] = schema; + const expected = { + title: 'title', + rel: '_rel', + relString: '_rel/relString', + array: 'array', + arrayString: 'array/arrayString', + arrayObject: 'array/arrayObject', + arrayObjectString: 'array/arrayObject/arrayObjectString', + object: 'object', + objectString: 'object/objectString', + objectArray: 'object/objectArray', + objectArrayString: 'object/objectArray/objectArrayString' + }; + + const actual = { + title: titleField.aposPath, + rel: relField.aposPath, + relString: relField.schema[0].aposPath, + array: arrayField.aposPath, + arrayString: arrayField.schema[0].aposPath, + arrayObject: arrayField.schema[1].aposPath, + arrayObjectString: arrayField.schema[1].schema[0].aposPath, + object: objectField.aposPath, + objectString: objectField.schema[0].aposPath, + objectArray: objectField.schema[1].aposPath, + objectArrayString: objectField.schema[1].schema[0].aposPath + }; + + assert.deepEqual(actual, expected); }); it('should set default value to invisible fields that triggered convert errors', async function () { From a27d68d236eada1b0630cbcdd1cff945ca7c797f Mon Sep 17 00:00:00 2001 From: Jed Date: Mon, 30 Dec 2024 13:42:50 +0100 Subject: [PATCH 35/43] updates setDefaultToInvisibleFields to handle case where field path contains ID of containing object --- modules/@apostrophecms/schema/index.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index 243918b935..c9ae5e8bd4 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -775,7 +775,12 @@ module.exports = { return validErrors; } - function setDefaultToInvisibleField(destination, schema, fieldName) { + function 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 From 966ad3caa6718430120efcfdc0d5cbd105ab86d3 Mon Sep 17 00:00:00 2001 From: Jed Date: Mon, 30 Dec 2024 13:43:08 +0100 Subject: [PATCH 36/43] test setting default values to invisible fields --- test/schemas.js | 68 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/test/schemas.js b/test/schemas.js index ac7bb66673..0582926775 100644 --- a/test/schemas.js +++ b/test/schemas.js @@ -5296,7 +5296,7 @@ describe('Schemas', function() { assert.deepEqual(expected, actual); }); - it.only('should add proper aposPath to all fields when validation schema', async function () { + it('should add proper aposPath to all fields when validation schema', async function () { const schema = apos.schema.compose({ addFields: [ { @@ -5393,7 +5393,71 @@ describe('Schemas', function() { }); it('should set default value to invisible fields that triggered convert errors', async function () { - // TODO + const schema = apos.schema.compose({ + addFields: [ + { + name: 'array', + type: 'array', + if: { + showArray: true + }, + schema: [ + { + name: 'arrayString', + type: 'string', + pattern: '^cool-' + }, + { + name: 'arrayMin', + type: 'integer', + min: 5 + }, + { + name: 'arrayMax', + type: 'integer', + max: 10 + } + ] + }, + { + name: 'showArray', + type: 'boolean' + } + ] + }); + apos.schema.validate(schema, 'foo'); + + const input = { + showArray: false, + array: [ + { + arrayString: 'bad string', + arrayMin: 2, + arrayMax: 13 + } + ] + }; + + const req = apos.task.getReq(); + const result = {}; + await apos.schema.convert(req, schema, input, result); + + const expected = { + arrayString: '', + arrayMin: 5, + arrayMax: 10 + }; + + const { + arrayString, arrayMin, arrayMax + } = result.array[0]; + const actual = { + arrayString, + arrayMin, + arrayMax + }; + + assert.deepEqual(actual, expected); }); }); From f77638bac0acb9f49d346c2a8d4fa0b81b8bafb7 Mon Sep 17 00:00:00 2001 From: Jed Date: Mon, 30 Dec 2024 14:01:39 +0100 Subject: [PATCH 37/43] reinit validatedSchemas before each test to make them mor isolated.. --- test/schemas.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/schemas.js b/test/schemas.js index 0582926775..f094aa1351 100644 --- a/test/schemas.js +++ b/test/schemas.js @@ -241,6 +241,10 @@ describe('Schemas', function() { this.timeout(t.timeout); + beforeEach(async function () { + apos.schema.validatedSchemas = {}; + }); + before(async function() { apos = await t.create({ root: module, From 2ef3280a6abe39316ccb494a25bbe85b99c55e4f Mon Sep 17 00:00:00 2001 From: Jed Date: Mon, 30 Dec 2024 14:24:04 +0100 Subject: [PATCH 38/43] logs errors out of handleErrors method, moves methods to self --- modules/@apostrophecms/schema/index.js | 163 +++++++++++++------------ 1 file changed, 82 insertions(+), 81 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index c9ae5e8bd4..e24f03ff71 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -662,7 +662,7 @@ module.exports = { destination }); - const errors = await handleConvertErrors({ + const validErrors = await self.handleConvertErrors({ req, schema, convertErrors, @@ -670,8 +670,12 @@ module.exports = { nonVisibleFields }); - if (errors.length) { - throw errors; + for (const error of validErrors) { + self.apos.util.error(error.stack); + } + + if (validErrors.length) { + throw validErrors; } async function getNonVisibleFields({ @@ -713,91 +717,88 @@ module.exports = { return nonVisibleFields; } - async function 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); - 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 - }); + 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); + continue; + } - // If invalid error has no sub error, this one can be removed - if (!subErrors.length) { - continue; - } + if (error.data?.errors) { + const subErrors = await self.handleConvertErrors({ + req, + schema, + convertErrors: error.data.errors, + nonVisibleFields, + destination: curDestination[destPath], + destinationPath: errorPath, + hiddenAncestors: nonVisibleField + }); - error.data.errors = subErrors; + // If invalid error has no sub error, this one can be removed + if (!subErrors.length) { + continue; } - if (typeof error !== 'string') { - self.apos.util.error(error.path + '\n' + error.stack); - } - validErrors.push(error); + error.data.errors = subErrors; } + validErrors.push(error); + } - return validErrors; - } - - function 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); - } + 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); } }, From 8b14eed4f549857e1ba8ec0acac3a91ea9c588f1 Mon Sep 17 00:00:00 2001 From: Jed Date: Mon, 30 Dec 2024 17:25:58 +0100 Subject: [PATCH 39/43] updates changelog --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e25d96acd..a80ebdc9c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ fields (or their parents) are visible and so related errors are discarded. >>>>>>> e1fd5bd94 (changelog typo) +### 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. + ## 4.11.1 (2024-12-18) ### Fixes @@ -60,7 +64,6 @@ 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 From 5de55e2acf639cffc3ef85b03ca34c3aeed31228 Mon Sep 17 00:00:00 2001 From: Jed Date: Mon, 30 Dec 2024 17:27:34 +0100 Subject: [PATCH 40/43] removes useless ancestorSchemas param in convert --- modules/@apostrophecms/schema/index.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index e24f03ff71..5371f5989e 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -592,14 +592,12 @@ module.exports = { { fetchRelationships = true, ancestors = [], - rootConvert = true, - ancestorSchemas = {} + rootConvert = true } = {} ) { const options = { fetchRelationships, - ancestors, - ancestorSchemas + ancestors }; if (Array.isArray(req)) { throw new Error('convert invoked without a req, do you have one in your context?'); From 1508b0abab4fc310ed4fb46af3c464e40e5f32c7 Mon Sep 17 00:00:00 2001 From: Jed Date: Mon, 30 Dec 2024 18:20:08 +0100 Subject: [PATCH 41/43] moves getNonVisibleFields out of convert --- modules/@apostrophecms/schema/index.js | 61 +++++++++++++------------- 1 file changed, 30 insertions(+), 31 deletions(-) diff --git a/modules/@apostrophecms/schema/index.js b/modules/@apostrophecms/schema/index.js index 5371f5989e..2b0389b90c 100644 --- a/modules/@apostrophecms/schema/index.js +++ b/modules/@apostrophecms/schema/index.js @@ -654,7 +654,7 @@ module.exports = { return; } - const nonVisibleFields = await getNonVisibleFields({ + const nonVisibleFields = await self.getNonVisibleFields({ req, schema, destination @@ -675,46 +675,45 @@ module.exports = { if (validErrors.length) { throw validErrors; } + }, - 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) { - nonVisibleFields.add(curPath); - continue; - } - if (!field.schema) { - continue; - } + 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 getNonVisibleFields({ - req, - schema: field.schema, - destination: arrayItem, - nonVisibleFields, - fieldPath: `${curPath}.${arrayItem._id}` - }); - } - } else if (field.type === 'object') { - await getNonVisibleFields({ + // 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: destination[field.name], + destination: arrayItem, nonVisibleFields, - fieldPath: curPath + fieldPath: `${curPath}.${arrayItem._id}` }); } + } else if (field.type === 'object') { + await self.getNonVisibleFields({ + req, + schema: field.schema, + destination: destination[field.name], + nonVisibleFields, + fieldPath: curPath + }); } - - return nonVisibleFields; } + return nonVisibleFields; }, async handleConvertErrors({ From 63eef2a42b3a94b8cd282eba2a01947264cff3dd Mon Sep 17 00:00:00 2001 From: Jed Date: Mon, 30 Dec 2024 18:34:55 +0100 Subject: [PATCH 42/43] fix changelog --- CHANGELOG.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a80ebdc9c0..2a2b31ec81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,12 +4,9 @@ ### Fixes -<<<<<<< HEAD * 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 fields (or their parents) are visible and so related errors are discarded. ->>>>>>> e1fd5bd94 (changelog typo) ### Adds From f1f047ed66e1bce62eae100a2b34349b8fc27e61 Mon Sep 17 00:00:00 2001 From: Jed Date: Tue, 7 Jan 2025 14:24:04 +0100 Subject: [PATCH 43/43] changelog feedback --- CHANGELOG.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a2b31ec81..4c6fe75006 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,12 +5,11 @@ ### 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 -fields (or their parents) are visible and so related errors are discarded. +* Eliminated superfluous error messages. The convert method now waits for all recursive invocations to complete before attempting to determine if fields are visible. ### 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. +* Possibility to set a field not ready when performing async operations, when a field isn't ready, the validation and emit won't occur. ## 4.11.1 (2024-12-18)