Bug 1867824: Fix dynamic form field ordering logic #6329

TheRealJon · 2020-08-13T17:33:14Z

Update form field ordering logic so that behavior matches documentation. Falls back to ordering by descriptor order.
Move some common dynamic form type definitions into types.ts to prevent dependency cycles.

cc @tlwu2013

Update form field ordering logic so that behavior matches documentation. Falls back to ordering by descriptor order. Move some common dynamic form type definitions into types.ts to prevent dependency cycles.

openshift-ci-robot · 2020-08-13T17:33:22Z

@TheRealJon: This pull request references Bugzilla bug 1867824, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1867824: Fix dynamic form field ordering logic

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-08-13T17:35:28Z

@TheRealJon: This pull request references Bugzilla bug 1867824, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1867824: Fix dynamic form field ordering logic

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

TheRealJon · 2020-08-13T17:41:18Z

/assign @spadgett

TheRealJon · 2020-08-13T17:46:01Z

frontend/packages/console-shared/src/components/dynamic-form/types.ts

+
+export type DynamicFormFieldOptionsList = {
+  label: string;
+  value: string;
+}[];
+
+export type DynamicFormFieldDependency = {
+  controlFieldPath: string;
+  controlFieldValue: string;
+  controlFieldName: string;
+};
+
+export type UiSchemaOptionsWithDependency = {
+  dependency?: DynamicFormFieldDependency;
+};
+
+export type DynamicFormSchemaError = {
+  title: string;
+  message: string;
+};


I was having issues with dependency cycles while trying to improve the types for the sort order function in utils, so I moved these reusable types here to prevent future issues.

spadgett

/lgtm

Thanks @TheRealJon

spadgett · 2020-08-13T17:45:01Z

frontend/packages/console-shared/src/components/dynamic-form/types.ts

+  controlFieldName: string;
+};
+
+export type UiSchemaOptionsWithDependency = {


nit: console naming conventions would make this capital UI

Suggested change

export type UiSchemaOptionsWithDependency = {

export type UISchemaOptionsWithDependency = {

openshift-ci-robot · 2020-08-13T17:48:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: spadgett, TheRealJon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/packages/console-shared/OWNERS~~ [spadgett]
~~frontend/packages/operator-lifecycle-manager/OWNERS~~ [TheRealJon,spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

TheRealJon · 2020-08-13T17:50:31Z

frontend/packages/console-shared/src/components/dynamic-form/templates.tsx

-    const { dependency } = getUiOptions(uiSchema ?? {}) as DependencyUIOption; // Type defs for this function are awful
+    const { dependency } = getUiOptions(uiSchema ?? {}) as UiSchemaOptionsWithDependency; // Type defs for this function are awful
    if (dependency) {
      setDependencyMet(
-        dependency.value ===
-          _.get(formContext.formData ?? {}, ['spec', ...(dependency.path ?? [])], '').toString(),
+        dependency?.controlFieldValue ===
+          _.get(
+            formContext.formData ?? {},
+            ['spec', ...(dependency?.controlFieldPath ?? [])],
+            '',
+          ).toString(),


Updated the uiSchema dependency option to include the control field name (the end of the path) to make sorting easier, and updated property naming to be more descriptive while I was at it.

TheRealJon · 2020-08-13T17:52:07Z

frontend/packages/console-shared/src/components/dynamic-form/types.ts

@@ -1,4 +1,4 @@
-export enum SchemaType {
+export enum JSONSchemaType {


More descriptive name since 'schema' could mean UI schema or JSON schema in the context of the DynamicForm component.

TheRealJon · 2020-08-13T17:54:49Z

frontend/packages/console-shared/src/components/dynamic-form/utils.ts

+/**
+ * Give a property name a sort wieght based on whether it has ui schema, is required, or is a
+ * control field for a property with a field dependency. A lower weight means higher sort order.
+ * Fields are weighted according to the following tiers:
+ *  Tier 1 (negative 10^9 - 10^6 magnitude):  Required fields with ui schema
+ *  Tier 2 (negative 10^9 magnitude):         Required fields without ui schema
+ *  Tier 3 (negative 10^6 magnitude):         Optional fields with ui schema
+ *  Tier 4 (positive 10^3 maginitude):        Control fields that don't fit any above
+ *  Tier 5 (Infinity):                        All other fields
+ *
+ * Within each of the above tiers, fields are further weighted based on field dependency and ui
+ * schema defined sort order:
+ *   - Fields without dependency - base weight + ui schema sort order
+ *   - Control field - base weight + ui schema sort order + (nth control field) * 10000
+ *   - Dependent field - control field weight + ui schema sort order + 1
+ *
+ * These weight numbers are arbitrary, but spaced far enough apart to prevent collisions.
+ */
+const getJSONSchemaPropertySortWeight = (
+  property: string,
+  jsonSchema: JSONSchema6,
+  uiSchema: UiSchema,
+): number => {
+  const isRequired = (jsonSchema?.required ?? []).includes(property);
+  const propertyUISchema = uiSchema?.[property];
+
+  // Any sibling has a dependency with this as the control field.
+  const isControlField = _.some(
+    uiSchema,
+    ({ 'ui:dependency': dependency }) => dependency?.controlFieldName === property,
+  );
+
+  // Property sort order from ui schema (adjusted to 1-based origin). Use propertyNames.length
+  // when no uiSchema sortOrder exists. Ensures properties without uiSchema sort order  have a
+  // higher weight than those with.
+  const propertySortOrder = Number(
+    propertyUISchema?.['ui:sortOrder'] ?? _.keys(jsonSchema?.properties).length,
+  );
+
+  // This property's control field name, if it exists
+  const controlFieldName = propertyUISchema?.['ui:dependency']?.controlFieldName;
+
+  // A small offset that is added to the base weight so that control fields get sorted
+  // below other fields in the same 'tier', and allows for depenendt fields to be sorted
+  // directly after their control field.
+  const controlFieldOffset = isControlField ? propertySortOrder * THOUSAND : 0;
+
+  // Total offset to be added to base tier
+  const offset = controlFieldOffset + propertySortOrder;
+
+  // If this property is a dependent, it's weight is based on it's control field
+  if (controlFieldName) {
+    return getJSONSchemaPropertySortWeight(controlFieldName, jsonSchema, uiSchema) + offset + 1;
+  }
+
+  // Tier 1 = -1001000000 (negative one billion one million) + offset
+  // Tier 2 = -1000000000 (negagive one billion) + offset
+  // Tier 3 = -1000000 (negative one million) + offset
+  // Tier 4 = 0 + offset
+  // Tier 5 = Infinity
+  return (
+    // Doesn't meet any sorting criteria, set to infinity
+    (!isRequired && !propertyUISchema && !controlFieldOffset ? Infinity : 0) -
+    (isRequired ? BILLION : 0) -
+    (propertyUISchema ? MILLION : 0) +
+    offset
+  );
+};


Pulled this function out of getJSONSchemaOrder scope.

I tried to improve the comments and logic to make this a little bit easier to understand.

TheRealJon · 2020-08-13T18:00:56Z

frontend/packages/console-shared/src/components/dynamic-form/utils.ts

+  // Property sort order from ui schema (adjusted to 1-based origin). Use propertyNames.length
+  // when no uiSchema sortOrder exists. Ensures properties without uiSchema sort order  have a
+  // higher weight than those with.
+  const propertySortOrder = Number(
+    propertyUISchema?.['ui:sortOrder'] ?? _.keys(jsonSchema?.properties).length,
+  );
+


This is the main bug fix. I added logic to include the original descriptor index as a sort weight modifier.

TheRealJon · 2020-08-13T18:05:55Z

frontend/packages/operator-lifecycle-manager/src/components/operand/utils.ts

-  const [, path, value] = capability.match(REGEXP_FIELD_DEPENDENCY_PATH_VALUE) ?? [];
-  if (!!path && !!value) {
-    return { 'ui:dependency': { path: descriptorPathToUISchemaPath(path), value } };
-  }
-  return {};
+  const [, path, controlFieldValue] = capability.match(REGEXP_FIELD_DEPENDENCY_PATH_VALUE) ?? [];
+  const controlFieldPath = descriptorPathToUISchemaPath(path);
+  const controlFieldName = _.last(controlFieldPath);
+  return {
+    ...(path &&
+      controlFieldValue && {
+        'ui:dependency': {
+          controlFieldPath,
+          controlFieldValue,
+          controlFieldName,
+        },
+      }),


Update property names to be more descriptive and add control field name for easier reference when sorting dependent/control fields.

TheRealJon · 2020-08-13T18:07:10Z

frontend/packages/operator-lifecycle-manager/src/components/operand/utils.ts

@@ -166,6 +175,7 @@ export const descriptorsToUISchema = (
            ...(description && { 'ui:description': description }),
            ...(displayName && { 'ui:title': displayName }),
            ...capabilitiesUISchema,
+            'ui:sortOrder': index + 1,


Add descriptor index to uiSchema to be used in sorting logic.

openshift-bot · 2020-08-14T04:07:35Z

/retest