[Index Management] Improve UX of mappings editor (#152730)

Addresses #52708 ## Summary This PR implements the short-term solution proposed in #52708 (comment) - disabling the Update button of the mappings editor form when no changes have been made by the user and, when there are saveable changes, displaying some text guiding the user to review all of the field's settings and a tooltip that provides more context. This implementation uses the `useFormIsModified` hook from the form lib (https://docs.elastic.dev/form-lib/core/use-form-is-modified) which didn't work as expected for this use case so a fix to this hook is added as well. I verified that all other forms that use this hook (the field editor from Data Views and the Connector editor) are working fine with the added fix to the hook. Also, several mappings editor jest tests are modified to match the new scenario and a new one was added to test if the Update button is enabled/disabled as supposed. **How to test** 1. Go to Stack Management -> Index Management -> Index Templates 2. Create a new index template with some mappings fields. 3. Start editing the created index template and go to the Mappings step of the form wizard. 4. Click on the Edit button of any of the fields. 5. Verify that the Update button is disabled. 6. Verify that any change on the form enables the Update button and displays the "Review all settings before updating" text with a tooltip. https://user-images.githubusercontent.com/59341489/223154894-e3d94e6e-a315-4c75-b10f-d20e0cabcba2.mov <img width="698" alt="Screenshot 2023-03-06 at 15 20 38" src="https://user-images.githubusercontent.com/59341489/223154905-d5e40602-de7e-4780-8998-f12cf50281bd.png"> ### Checklist - [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [X] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [X] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) --------- Co-authored-by: Kibana Machine <[email protected]>
elastic · Mar 9, 2023 · ee5acfd · ee5acfd
1 parent 63da800
commit ee5acfd
Show file tree

Hide file tree

Showing 7 changed files with 167 additions and 58 deletions.
diff --git a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form_is_modified.ts b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form_is_modified.ts
@@ -5,7 +5,7 @@
  * in compliance with, at your election, the Elastic License 2.0 or the Server
  * Side Public License, v 1.
  */
-import { useMemo, useState } from 'react';
+import { useEffect, useMemo, useState } from 'react';
 import { get } from 'lodash';
 
 import { FieldHook, FormHook } from '../types';
@@ -53,7 +53,7 @@ export const useFormIsModified = ({
 
   const { getFields, __getFieldsRemoved, __getFormDefaultValue } = form;
 
-  const discardArrayToString = JSON.stringify(fieldPathsToDiscard);
+  const discardArrayToString = useMemo(() => JSON.stringify(fieldPathsToDiscard), []); // eslint-disable-line react-hooks/exhaustive-deps
 
   // Create a map of the fields to discard to optimize look up
   const fieldsToDiscard = useMemo(() => {
@@ -72,40 +72,48 @@ export const useFormIsModified = ({
 
   // We listen to all the form data change to trigger a re-render
   // and update our derived "isModified" state
-  useFormData({ form });
-
-  const isFieldIncluded = fieldsToDiscard
-    ? ([path]: [string, FieldHook]) => fieldsToDiscard[path] !== true
-    : () => true;
-
-  // Calculate next state value
-  // 1. Check if any field has been modified
-  let nextIsModified = Object.entries(getFields())
-    .filter(isFieldIncluded)
-    .some(([_, field]) => field.isModified);
-
-  if (!nextIsModified) {
-    // 2. Check if any field has been removed.
-    // If somme field has been removed **and** they were originaly present on the
-    // form "defaultValue" then the form has been modified.
-    const formDefaultValue = __getFormDefaultValue();
-    const fieldOnFormDefaultValue = (path: string) => Boolean(get(formDefaultValue, path));
-
-    const fieldsRemovedFromDOM: string[] = fieldsToDiscard
-      ? Object.keys(__getFieldsRemoved())
-          .filter((path) => fieldsToDiscard[path] !== true)
-          .filter(fieldOnFormDefaultValue)
-      : Object.keys(__getFieldsRemoved()).filter(fieldOnFormDefaultValue);
-
-    nextIsModified = fieldsRemovedFromDOM.length > 0;
-  }
+  const formData = useFormData({ form });
+
+  const isFieldIncluded = useMemo(
+    () =>
+      fieldsToDiscard
+        ? ([path]: [string, FieldHook]) => fieldsToDiscard[path] !== true
+        : () => true,
+    [fieldsToDiscard]
+  );
+
+  useEffect(() => {
+    // Calculate next state value
+    // 1. Check if any field has been modified
+    let nextIsModified = Object.entries(getFields())
+      .filter(isFieldIncluded)
+      .some(([_, field]) => field.isModified);
+
+    if (!nextIsModified) {
+      // 2. Check if any field has been removed.
+      // If somme field has been removed **and** they were originaly present on the
+      // form "defaultValue" then the form has been modified.
+      const formDefaultValue = __getFormDefaultValue();
+      const fieldOnFormDefaultValue = (path: string) => Boolean(get(formDefaultValue, path));
+
+      const fieldsRemovedFromDOM: string[] = fieldsToDiscard
+        ? Object.keys(__getFieldsRemoved())
+            .filter((path) => fieldsToDiscard[path] !== true)
+            .filter(fieldOnFormDefaultValue)
+        : Object.keys(__getFieldsRemoved()).filter(fieldOnFormDefaultValue);
+
+      nextIsModified = fieldsRemovedFromDOM.length > 0;
+    }
 
-  // Update the state **only** if it has changed to avoid creating an infinite re-render
-  if (nextIsModified && !isFormModified) {
-    setIsFormModified(true);
-  } else if (!nextIsModified && isFormModified) {
-    setIsFormModified(false);
-  }
+    setIsFormModified(nextIsModified);
+  }, [
+    __getFieldsRemoved,
+    __getFormDefaultValue,
+    fieldsToDiscard,
+    formData,
+    getFields,
+    isFieldIncluded,
+  ]);
 
   return isFormModified;
 };
diff --git a/.../components/mappings_editor/__jest__/client_integration/datatypes/point_datatype.test.tsx b/.../components/mappings_editor/__jest__/client_integration/datatypes/point_datatype.test.tsx
@@ -49,26 +49,33 @@ describe('Mappings editor: point datatype', () => {
       },
     };
 
-    const updatedMappings = { ...defaultMappings };
-
     await act(async () => {
       testBed = setup({ value: defaultMappings, onChange: onChangeHandler });
     });
     testBed.component.update();
 
     const {
       component,
-      actions: { startEditField, updateFieldAndCloseFlyout },
+      actions: { startEditField, updateFieldName, updateFieldAndCloseFlyout },
     } = testBed;
 
     // Open the flyout to edit the field
     await startEditField('myField');
 
+    // Update the name of the field
+    await updateFieldName('updatedField');
+
     // Save the field and close the flyout
     await updateFieldAndCloseFlyout();
 
-    // It should have the default parameters values added
-    updatedMappings.properties.myField = defaultPointParameters;
+    // It should have the default parameters values added for fields which are not set
+    const updatedMappings = {
+      properties: {
+        updatedField: {
+          ...defaultPointParameters,
+        },
+      },
+    };
 
     ({ data } = await getMappingsEditorData(component));
     expect(data).toEqual(updatedMappings);

diff --git a/.../components/mappings_editor/__jest__/client_integration/datatypes/shape_datatype.test.tsx b/.../components/mappings_editor/__jest__/client_integration/datatypes/shape_datatype.test.tsx
@@ -50,26 +50,33 @@ describe('Mappings editor: shape datatype', () => {
       },
     };
 
-    const updatedMappings = { ...defaultMappings };
-
     await act(async () => {
       testBed = setup({ value: defaultMappings, onChange: onChangeHandler });
     });
     testBed.component.update();
 
     const {
       component,
-      actions: { startEditField, updateFieldAndCloseFlyout },
+      actions: { startEditField, updateFieldName, updateFieldAndCloseFlyout },
     } = testBed;
 
     // Open the flyout to edit the field
     await startEditField('myField');
 
+    // Update the name of the field
+    await updateFieldName('updatedField');
+
     // Save the field and close the flyout
     await updateFieldAndCloseFlyout();
 
-    // It should have the default parameters values added
-    updatedMappings.properties.myField = defaultShapeParameters;
+    // It should have the default parameters values added for fields which are not set
+    const updatedMappings = {
+      properties: {
+        updatedField: {
+          ...defaultShapeParameters,
+        },
+      },
+    };
 
     ({ data } = await getMappingsEditorData(component));
     expect(data).toEqual(updatedMappings);

diff --git a/...n/components/mappings_editor/__jest__/client_integration/datatypes/text_datatype.test.tsx b/...n/components/mappings_editor/__jest__/client_integration/datatypes/text_datatype.test.tsx
@@ -55,8 +55,6 @@ describe('Mappings editor: text datatype', () => {
       },
     };
 
-    const updatedMappings = { ...defaultMappings };
-
     await act(async () => {
       testBed = setup({ value: defaultMappings, onChange: onChangeHandler });
     });
@@ -65,12 +63,15 @@ describe('Mappings editor: text datatype', () => {
     const {
       component,
       exists,
-      actions: { startEditField, getToggleValue, updateFieldAndCloseFlyout },
+      actions: { startEditField, updateFieldName, getToggleValue, updateFieldAndCloseFlyout },
     } = testBed;
 
     // Open the flyout to edit the field
     await startEditField('myField');
 
+    // Update the name of the field
+    await updateFieldName('updatedField');
+
     // It should have searchable ("index" param) active by default
     const indexFieldConfig = getFieldConfig('index');
     expect(getToggleValue('indexParameter.formRowToggle')).toBe(indexFieldConfig.defaultValue);
@@ -86,8 +87,12 @@ describe('Mappings editor: text datatype', () => {
     await updateFieldAndCloseFlyout();
 
     // It should have the default parameters values added
-    updatedMappings.properties.myField = {
-      ...defaultTextParameters,
+    const updatedMappings = {
+      properties: {
+        updatedField: {
+          ...defaultTextParameters,
+        },
+      },
     };
 
     ({ data } = await getMappingsEditorData(component));
@@ -120,16 +125,19 @@ describe('Mappings editor: text datatype', () => {
       form: { selectCheckBox, setSelectValue },
       actions: {
         startEditField,
+        updateFieldName,
         getCheckboxValue,
         showAdvancedSettings,
         updateFieldAndCloseFlyout,
       },
     } = testBed;
     const fieldToEdit = 'myField';
+    const newFieldName = 'updatedField';
 
-    // Start edit and immediately save to have all the default values
+    // Start edit, update the name only, and save to have all the default values
     await startEditField(fieldToEdit);
     await showAdvancedSettings();
+    await updateFieldName(newFieldName);
     await updateFieldAndCloseFlyout();
 
     expect(exists('mappingsEditorFieldEdit')).toBe(false);
@@ -139,7 +147,7 @@ describe('Mappings editor: text datatype', () => {
     let updatedMappings: any = {
       ...defaultMappings,
       properties: {
-        myField: {
+        updatedField: {
           ...defaultMappings.properties.myField,
           ...defaultTextParameters,
         },
@@ -148,7 +156,7 @@ describe('Mappings editor: text datatype', () => {
     expect(data).toEqual(updatedMappings);
 
     // Re-open the edit panel
-    await startEditField(fieldToEdit);
+    await startEditField(newFieldName);
     await showAdvancedSettings();
 
     // When no analyzer is defined, defaults to "Index default"
@@ -195,8 +203,8 @@ describe('Mappings editor: text datatype', () => {
     updatedMappings = {
       ...updatedMappings,
       properties: {
-        myField: {
-          ...updatedMappings.properties.myField,
+        updatedField: {
+          ...updatedMappings.properties.updatedField,
           analyzer: 'standard',
           search_analyzer: 'simple',
           search_quote_analyzer: 'whitespace',
@@ -208,7 +216,7 @@ describe('Mappings editor: text datatype', () => {
     expect(data).toEqual(updatedMappings);
 
     // Re-open the flyout and make sure the select have the correct updated value
-    await startEditField('myField');
+    await startEditField(newFieldName);
     await showAdvancedSettings();
 
     isUseSameAnalyzerForSearchChecked = getCheckboxValue('useSameAnalyzerForSearchCheckBox.input');

diff --git a/...ic/application/components/mappings_editor/__jest__/client_integration/edit_field.test.tsx b/...ic/application/components/mappings_editor/__jest__/client_integration/edit_field.test.tsx
@@ -121,4 +121,30 @@ describe('Mappings editor: edit field', () => {
 
     expect(data).toEqual(updatedMappings);
   });
+
+  test('should have Update button enabled only when changes are made', async () => {
+    const defaultMappings = {
+      properties: {
+        myField: {
+          type: 'text',
+        },
+      },
+    };
+
+    await act(async () => {
+      testBed = setup({ value: defaultMappings, onChange: onChangeHandler });
+    });
+    testBed.component.update();
+
+    await testBed.actions.expandAllFieldsAndReturnMetadata();
+
+    const {
+      actions: { startEditField, isUpdateButtonDisabled, updateFieldName },
+    } = testBed;
+    // Open the flyout to edit the field
+    await startEditField('myField');
+    expect(isUpdateButtonDisabled()).toBe(true);
+    await updateFieldName('updatedField');
+    expect(isUpdateButtonDisabled()).toBe(false);
+  });
 });
diff --git a/...omponents/mappings_editor/__jest__/client_integration/helpers/mappings_editor.helpers.tsx b/...omponents/mappings_editor/__jest__/client_integration/helpers/mappings_editor.helpers.tsx
@@ -141,6 +141,15 @@ const createActions = (testBed: TestBed<TestSubjects>) => {
     component.update();
   };
 
+  const updateFieldName = async (name: string) => {
+    await act(async () => {
+      form.setInputValue('nameParameterInput', name);
+      jest.advanceTimersByTime(0); // advance timers to allow the form to validate
+    });
+
+    component.update();
+  };
+
   const startEditField = async (path: string) => {
     const { testSubject } = getFieldAt(path);
     await act(async () => {
@@ -158,6 +167,9 @@ const createActions = (testBed: TestBed<TestSubjects>) => {
     component.update();
   };
 
+  const isUpdateButtonDisabled = () =>
+    find('mappingsEditorFieldEdit.editFieldUpdateButton').props().disabled;
+
   const showAdvancedSettings = async () => {
     if (find('mappingsEditorFieldEdit.advancedSettings').props().style.display === 'block') {
       // Already opened, nothing else to do
@@ -323,8 +335,10 @@ const createActions = (testBed: TestBed<TestSubjects>) => {
     getFieldAt,
     addField,
     expandAllFieldsAndReturnMetadata,
+    updateFieldName,
     startEditField,
     updateFieldAndCloseFlyout,
+    isUpdateButtonDisabled,
     showAdvancedSettings,
     updateJsonEditor,
     getJsonEditorValue,