Pro 6799 design default values invisible fields #4816

ValJed · 2024-11-26T13:31:18Z

PRO-6528

Summary

Adds control over when fields are ready and can be validated, by handling a new fieldReady property for each field.
Must work with async fields like dynamic choices, being able to set a field not ready during fetching and ready when operation is finished.
Reworks schema level convert method. Check errors only when on root convert method to have access to the finale destination object to be able to check non visible fields, as well as non visible parents. No more order problem..
Adds aposPath to fields during validation process.
Adds tests.

What are the specific steps to test this change?

Cypress tests 🟢

What kind of change does this PR introduce?

Make sure the PR fulfills these requirements:

It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
The changelog is updated
Related documentation has been updated
Related tests have been updated

If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

linear · 2024-11-26T13:31:22Z

PRO-6799 Tech design and quick experiment if possible for setting default values for invisible fields

boutell

I like where you're going with this.

modules/@apostrophecms/schema/index.js

modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js

ValJed · 2024-11-27T10:12:38Z

modules/@apostrophecms/schema/index.js

-        const errorsList = [];
+          const isCurrentVisible = isParentVisible === false
+            ? false
+            : await self.isVisible(req, schema, data, field.name);


Notable difference here (previous versions might be buggy). We pass data instead of destination because destination is not fully built at this point.
I think isVisible should compare data as they have been set by the user and not the potential default values.
Tests are green with it but you might have some reasons to not allow this.
Let me know.

Data can be unvalidated stuff of unexpected types etc., it could be anything. Trusting it is problematic. You might get away with that in isVisible because you're not saving the values, but then isVisible must be written carefully to not assume the values are at all well-formed, for instance it must not assume string fields are actually strings yet, etc. because nothing has been validated.

data might contains obsolete values.
You can also have preventFieldsOnly set to true meaning destination will only contains partial fields.
We should think about these and if it has any impact on your implementation.

It now uses destination when eveything as been converted.

ValJed · 2024-11-27T13:19:27Z

modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js

-      handler() {
-        this.updateNextAndEmit();
-      }
-    },


We don't need a deep watch here, we can simply call the method when v-model updates fieldState, see above change.

ValJed · 2024-11-27T13:19:40Z

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"


With this, no more need for a deep watcher.

Why not on the compare meta version?

To keep the behavior consistent, fieldState was watched before, I just replaced the watch by this. compareMetaState wasn't.

ValJed · 2024-12-02T13:58:27Z

modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js

+          } else {
+            this.next.data[field.name] = this.modelValue.data[field.name];
+          }
+        });


Only indentation in this block

boutell

It is possible that changes are not necessary but see my note here about trusting data, isVisible must be written very defensively if it is going to see unvalidated user input.

boutell · 2024-12-02T14:52:28Z

modules/@apostrophecms/schema/index.js

-        const errorsList = [];
+          const isCurrentVisible = isParentVisible === false
+            ? false
+            : await self.isVisible(req, schema, data, field.name);


Data can be unvalidated stuff of unexpected types etc., it could be anything. Trusting it is problematic. You might get away with that in isVisible because you're not saving the values, but then isVisible must be written carefully to not assume the values are at all well-formed, for instance it must not assume string fields are actually strings yet, etc. because nothing has been validated.

haroun · 2024-12-02T14:14:13Z

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"


Why not on the compare meta version?

haroun · 2024-12-02T15:24:48Z

modules/@apostrophecms/schema/index.js

-        const errorsList = [];
+          const isCurrentVisible = isParentVisible === false
+            ? false
+            : await self.isVisible(req, schema, data, field.name);


data might contains obsolete values.
You can also have preventFieldsOnly set to true meaning destination will only contains partial fields.
We should think about these and if it has any impact on your implementation.

ValJed · 2024-12-10T09:21:39Z

@boutell @haroun Code could be cleaned but I would like your opinion on the general direction.

modules/@apostrophecms/schema/index.js

ValJed · 2024-12-10T09:23:14Z

modules/@apostrophecms/schema/index.js

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


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

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

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

I guess it's smart to check.

Idk, this was existing code, I removed it and moved the logs out of handleErrors function.
Also created a ticket to improve logs in convert.

boutell

I like the general strategy. Still think it makes more sense to set aposPath on each field just once in schema.validate and its related functions.

boutell · 2024-12-10T19:39:48Z

CHANGELOG.md

@@ -7,6 +7,8 @@
 * Focus properly Widget Editor modals when opened. Keep the previous active focus on the modal when closing the widget editor.
 * a11y improvements for context menus.
 * Fixes broken widget preview URL when the image is overridden (module improve) and external build module is registered.
+* In th schema convert method, we wait all sub convert to run, to have access to the final destination object in order to check if 


boutell · 2024-12-10T19:42:11Z

modules/@apostrophecms/schema/index.js

        } = {}
      ) {
        const options = {
          fetchRelationships,
          ancestors,
-          isParentVisible
+          ancestorSchemas,
+          ancestorPath


I think it is better to compute the aposPath of a field once, in the validate method of schema and its related functions, rather than pushing this boulder up the hill on each convert call 🪨

Yes good point. We agree that we are talking about the schema path and not the actual destination one.

Done, it's now computed during validate.

boutell · 2024-12-10T19:44:06Z

modules/@apostrophecms/schema/index.js

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


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

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

boutell · 2024-12-10T19:44:18Z

modules/@apostrophecms/schema/index.js

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


I guess it's smart to check.

ValJed · 2024-12-30T16:26:57Z

modules/@apostrophecms/schema/index.js

+                destination,
+                conditionalFields)
+              )
+            );


Just linting

modules/@apostrophecms/schema/index.js

ValJed · 2024-12-30T17:21:32Z

modules/@apostrophecms/schema/index.js

+
+        for (const error of validErrors) {
+          self.apos.util.error(error.stack);
+        }


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

ValJed · 2024-12-30T17:23:08Z

modules/@apostrophecms/schema/index.js

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


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

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

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

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

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

ValJed · 2024-12-30T17:24:03Z

modules/@apostrophecms/schema/index.js

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


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

ValJed · 2024-12-30T17:24:18Z

modules/@apostrophecms/schema/index.js

+                  field.if,
+                  destination,
+                  conditionalFields
+                );


Just linting

ValJed · 2024-12-30T17:24:47Z

modules/@apostrophecms/schema/index.js

          }
        });
      },

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


Build aposPath by using parent aposPath

ValJed · 2024-12-30T17:25:31Z

modules/@apostrophecms/schema/index.js

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


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

ValJed · 2024-12-30T17:25:53Z

modules/@apostrophecms/schema/lib/addFieldTypes.js

-                ...(destination[field.name]
-                  ?.find?.(doc => doc._id === item._id)
-                  ?._fields || {})
+                ...destItem?._fields || {}


Just make code more readable.

ValJed · 2024-12-30T17:27:27Z

modules/@apostrophecms/schema/lib/addFieldTypes.js

@@ -1218,7 +1219,7 @@ module.exports = (self) => {
        self.validate(_field.schema, {
          type: 'relationship',
          subtype: _field.withType
-        });
+        }, _field);


Passing the parent to validate. Should we modify object and array to use the same validate method instead of validateField. (the validate method keeps tracks of already validated fields). I think it would make sense to use the same method for relationship, object and array.

…ontains ID of containing object

boutell · 2025-01-06T14:53:37Z

CHANGELOG.md

@@ -5,6 +5,12 @@
 ### Fixes

 * Fixes a bug where images in Media manager are not selectable (click on an image does nothing) in both default and relationship mode.
+* In the schema convert method, we wait all sub convert to run, to have access to the final destination object. In order to check if 


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

boutell · 2025-01-06T14:54:06Z

CHANGELOG.md

+
+### 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.


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

modules/@apostrophecms/schema/index.js

boutell · 2025-01-06T16:40:45Z

modules/@apostrophecms/schema/index.js

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


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

ValJed self-assigned this Nov 26, 2024

ValJed marked this pull request as draft November 26, 2024 13:31

boutell reviewed Nov 26, 2024

View reviewed changes

modules/@apostrophecms/schema/index.js Outdated Show resolved Hide resolved

modules/@apostrophecms/schema/ui/apos/logic/AposSchema.js Outdated Show resolved Hide resolved

ValJed force-pushed the pro-6799-design-default-values-invisible-fields branch 2 times, most recently from 6de153c to 9bbcd21 Compare November 26, 2024 17:22

ValJed commented Nov 27, 2024

View reviewed changes

ValJed marked this pull request as ready for review November 27, 2024 13:17

ValJed requested review from boutell and haroun November 27, 2024 13:17

ValJed commented Nov 27, 2024

View reviewed changes

ValJed commented Dec 2, 2024

View reviewed changes

boutell requested changes Dec 2, 2024

View reviewed changes

haroun requested changes Dec 2, 2024

View reviewed changes

ValJed force-pushed the pro-6799-design-default-values-invisible-fields branch 5 times, most recently from 52e6290 to 2341d53 Compare December 10, 2024 09:02

ValJed requested review from boutell and haroun December 10, 2024 09:20

ValJed commented Dec 10, 2024

View reviewed changes

modules/@apostrophecms/schema/index.js Outdated Show resolved Hide resolved

ValJed commented Dec 10, 2024

View reviewed changes

boutell requested changes Dec 10, 2024

View reviewed changes

ValJed force-pushed the pro-6799-design-default-values-invisible-fields branch from a4969e5 to 2601619 Compare December 30, 2024 13:02

ValJed commented Dec 30, 2024

View reviewed changes

ValJed requested a review from boutell December 30, 2024 17:28

reworks schema convert mehod to make it more understandable

f8b197b

ValJed added 15 commits December 30, 2024 18:28

lint fixes

84a532f

updates changelog

ebeba68

builds field schema path in validate instead of convert

b52050d

preps two more tests to write

88938e7

skip non visibles checks when current isn't visible and added to list

aa8c535

changelog typo

92c0123

passes parent to validate for sub fields of relationship

2d00a1c

test aposPath added to fields during validation

c41f8e4

updates setDefaultToInvisibleFields to handle case where field path c…

a27d68d

…ontains ID of containing object

test setting default values to invisible fields

966ad3c

reinit validatedSchemas before each test to make them mor isolated..

f77638b

logs errors out of handleErrors method, moves methods to self

2ef3280

updates changelog

8b14eed

removes useless ancestorSchemas param in convert

5de55e2

moves getNonVisibleFields out of convert

1508b0a

ValJed force-pushed the pro-6799-design-default-values-invisible-fields branch from e3458a3 to 1508b0a Compare December 30, 2024 17:33

fix changelog

63eef2a

boutell requested changes Jan 6, 2025

View reviewed changes

changelog feedback

f1f047e

ValJed requested a review from boutell January 7, 2025 13:36

boutell approved these changes Jan 7, 2025

View reviewed changes

ValJed requested review from haroun and boutell and removed request for haroun January 7, 2025 14:13

boutell approved these changes Jan 7, 2025

View reviewed changes

haroun approved these changes Jan 9, 2025

View reviewed changes

ValJed merged commit 2dc106c into main Jan 9, 2025
9 checks passed

ValJed deleted the pro-6799-design-default-values-invisible-fields branch January 9, 2025 09:17


		### 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.

Pro 6799 design default values invisible fields #4816

Pro 6799 design default values invisible fields #4816

Conversation

ValJed commented Nov 26, 2024 • edited Loading

Summary

What are the specific steps to test this change?

What kind of change does this PR introduce?

Make sure the PR fulfills these requirements:

linear bot commented Nov 26, 2024

boutell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boutell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValJed commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValJed Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

boutell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValJed Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValJed Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValJed commented Nov 26, 2024 •

edited

Loading

ValJed Dec 31, 2024 •

edited

Loading

ValJed Dec 30, 2024 •

edited

Loading

ValJed Dec 30, 2024 •

edited

Loading