Merge pull request #9593 from keithjgrant/7506-yaml-json-eval-fix-2

Don't unnecessarily expand YAML expressions

SUMMARY
Prevents variables fields from expanding YAML expressions when possible:

In the detail view, the user may toggle to JSON (seeing the data structure fully expanded), but toggling back to YAML will continue to display the original un-expanded value with expressions intact
In edit mode, this works the same way, UNLESS the user edits the value while in JSON mode.

Addresses #7506
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

UI

ADDITIONAL INFORMATION

Reviewed-by: Jake McDermott <yo@jakemcdermott.me>
Reviewed-by: Chris Meyers <None>

awx/ui_next/src/components/CodeEditor/VariablesDetail.jsx

@@ -1,5 +1,5 @@
 import 'styled-components/macro';
-import React, { useState, useEffect } from 'react';
+import React, { useState } from 'react';
 import { node, number, oneOfType, shape, string, arrayOf } from 'prop-types';
 import { withI18n } from '@lingui/react';
 import { t } from '@lingui/macro';
@@ -23,39 +23,42 @@ import {
 import CodeEditor from './CodeEditor';
 import { JSON_MODE, YAML_MODE } from './constants';
 
-function getValueAsMode(value, mode) {
+function VariablesDetail({
-  if (!value) {
+  dataCy,
-    if (mode === JSON_MODE) {
+  helpText,
-      return '{}';
+  value,
-    }
+  label,
-    return '---';
+  rows,
-  }
+  fullHeight,
-  const modeMatches = isJsonString(value) === (mode === JSON_MODE);
+  i18n,
-  if (modeMatches) {
+}) {
-    return value;
-  }
-  return mode === YAML_MODE ? jsonToYaml(value) : yamlToJson(value);
-}
-
-function VariablesDetail({ dataCy, helpText, value, label, rows, i18n }) {
   const [mode, setMode] = useState(
     isJsonObject(value) || isJsonString(value) ? JSON_MODE : YAML_MODE
   );
-  const [currentValue, setCurrentValue] = useState(
-    isJsonObject(value) ? JSON.stringify(value, null, 2) : value || '---'
-  );
   const [isExpanded, setIsExpanded] = useState(false);
-  const [error, setError] = useState(null);
 
-  useEffect(() => {
+  let currentValue = value;
-    setCurrentValue(
+  let error;
-      getValueAsMode(
+
-        isJsonObject(value) ? JSON.stringify(value, null, 2) : value,
+  const getValueInCurrentMode = () => {
-        mode
+    if (!value) {
-      )
+      if (mode === JSON_MODE) {
-    );
+        return '{}';
-    /* eslint-disable-next-line react-hooks/exhaustive-deps */
+      }
-  }, [value]);
+      return '---';
+    }
+    const modeMatches = isJsonString(value) === (mode === JSON_MODE);
+    if (modeMatches) {
+      return value;
+    }
+    return mode === YAML_MODE ? jsonToYaml(value) : yamlToJson(value);
+  };
+
+  try {
+    currentValue = getValueInCurrentMode();
+  } catch (err) {
+    error = err;
+  }
 
   const labelCy = dataCy ? `${dataCy}-label` : null;
   const valueCy = dataCy ? `${dataCy}-value` : null;
@@ -76,8 +79,6 @@ function VariablesDetail({ dataCy, helpText, value, label, rows, i18n }) {
           mode={mode}
           setMode={setMode}
           currentValue={currentValue}
-          setCurrentValue={setCurrentValue}
-          setError={setError}
           onExpand={() => setIsExpanded(true)}
           i18n={i18n}
         />
@@ -93,6 +94,7 @@ function VariablesDetail({ dataCy, helpText, value, label, rows, i18n }) {
           value={currentValue}
           readOnly
           rows={rows}
+          fullHeight={fullHeight}
           css="margin-top: 10px"
         />
         {error && (
@@ -129,8 +131,6 @@ function VariablesDetail({ dataCy, helpText, value, label, rows, i18n }) {
             mode={mode}
             setMode={setMode}
             currentValue={currentValue}
-            setCurrentValue={setCurrentValue}
-            setError={setError}
             i18n={i18n}
           />
           <CodeEditor
@@ -163,11 +163,8 @@ function ModeToggle({
   label,
   helpText,
   dataCy,
-  currentValue,
-  setCurrentValue,
   mode,
   setMode,
-  setError,
   onExpand,
   i18n,
 }) {
@@ -196,12 +193,7 @@ function ModeToggle({
               ]}
               value={mode}
               onChange={newMode => {
-                try {
+                setMode(newMode);
-                  setCurrentValue(getValueAsMode(currentValue, newMode));
-                  setMode(newMode);
-                } catch (err) {
-                  setError(err);
-                }
               }}
             />
           </SplitItem>

awx/ui_next/src/components/CodeEditor/VariablesDetail.test.jsx

@@ -39,7 +39,7 @@ describe('<VariablesDetail>', () => {
     wrapper.find('MultiButtonToggle').invoke('onChange')('yaml');
     const input2 = wrapper.find('VariablesDetail___StyledCodeEditor');
     expect(input2.prop('mode')).toEqual('yaml');
-    expect(input2.prop('value')).toEqual('foo: bar\n');
+    expect(input2.prop('value')).toEqual('---foo: bar');
   });
 
   test('should render label and value= --- when there are no values', () => {

awx/ui_next/src/components/CodeEditor/VariablesField.jsx

@@ -73,10 +73,43 @@ function VariablesField({
     },
     [shouldValidate, validate] // eslint-disable-line react-hooks/exhaustive-deps
   );
+  const [lastYamlValue, setLastYamlValue] = useState(
+    mode === YAML_MODE ? field.value : null
+  );
+  const [isJsonEdited, setIsJsonEdited] = useState(false);
   const [isExpanded, setIsExpanded] = useState(false);
 
+  const handleModeChange = newMode => {
+    if (newMode === YAML_MODE && !isJsonEdited && lastYamlValue !== null) {
+      helpers.setValue(lastYamlValue, false);
+      setMode(newMode);
+      return;
+    }
+
+    try {
+      const newVal =
+        newMode === YAML_MODE
+          ? jsonToYaml(field.value)
+          : yamlToJson(field.value);
+      helpers.setValue(newVal, false);
+      setMode(newMode);
+    } catch (err) {
+      helpers.setError(err.message);
+    }
+  };
+
+  const handleChange = newVal => {
+    helpers.setValue(newVal);
+    if (mode === JSON_MODE) {
+      setIsJsonEdited(true);
+    } else {
+      setLastYamlValue(newVal);
+      setIsJsonEdited(false);
+    }
+  };
+
   return (
-    <>
+    <div>
       <VariablesFieldInternals
         i18n={i18n}
         id={id}
@@ -87,8 +120,9 @@ function VariablesField({
         tooltip={tooltip}
         onExpand={() => setIsExpanded(true)}
         mode={mode}
-        setMode={setMode}
+        setMode={handleModeChange}
         setShouldValidate={setShouldValidate}
+        handleChange={handleChange}
       />
       <Modal
         variant="xlarge"
@@ -118,8 +152,9 @@ function VariablesField({
             tooltip={tooltip}
             fullHeight
             mode={mode}
-            setMode={setMode}
+            setMode={handleModeChange}
             setShouldValidate={setShouldValidate}
+            handleChange={handleChange}
           />
         </div>
       </Modal>
@@ -128,7 +163,7 @@ function VariablesField({
           {meta.error}
         </div>
       ) : null}
-    </>
+    </div>
   );
 }
 VariablesField.propTypes = {
@@ -156,8 +191,9 @@ function VariablesFieldInternals({
   setMode,
   onExpand,
   setShouldValidate,
+  handleChange,
 }) {
-  const [field, meta, helpers] = useField(name);
+  const [field, meta] = useField(name);
 
   return (
     <div className="pf-c-form__group">
@@ -176,18 +212,7 @@ function VariablesFieldInternals({
                 [JSON_MODE, 'JSON'],
               ]}
               value={mode}
-              onChange={newMode => {
+              onChange={setMode}
-                try {
-                  const newVal =
-                    newMode === YAML_MODE
-                      ? jsonToYaml(field.value)
-                      : yamlToJson(field.value);
-                  helpers.setValue(newVal);
-                  setMode(newMode);
-                } catch (err) {
-                  helpers.setError(err.message);
-                }
-              }}
             />
           </SplitItem>
         </Split>
@@ -213,9 +238,7 @@ function VariablesFieldInternals({
         mode={mode}
         readOnly={readOnly}
         {...field}
-        onChange={newVal => {
+        onChange={handleChange}
-          helpers.setValue(newVal);
-        }}
         fullHeight={fullHeight}
         onFocus={() => setShouldValidate(false)}
         onBlur={() => setShouldValidate(true)}

awx/ui_next/src/components/CodeEditor/VariablesField.test.jsx

@@ -51,11 +51,90 @@ describe('VariablesField', () => {
     });
     wrapper.update();
     expect(wrapper.find('CodeEditor').prop('mode')).toEqual('yaml');
+    expect(wrapper.find('CodeEditor').prop('value')).toEqual(
+      '---\nfoo: bar\nbaz: 3'
+    );
+  });
+
+  it('should retain non-expanded yaml if JSON value not edited', async () => {
+    const value = '---\na: &aa [a,b,c]\nb: *aa';
+    const wrapper = mountWithContexts(
+      <Formik initialValues={{ variables: value }}>
+        {() => (
+          <VariablesField id="the-field" name="variables" label="Variables" />
+        )}
+      </Formik>
+    );
+    const jsButton = wrapper.find('Button.toggle-button-javascript');
+    await act(async () => {
+      jsButton.simulate('click');
+    });
+    wrapper.update();
+    const yamlButton = wrapper.find('Button.toggle-button-yaml');
+    await act(async () => {
+      yamlButton.simulate('click');
+    });
+    wrapper.update();
+    expect(wrapper.find('CodeEditor').prop('mode')).toEqual('yaml');
+    expect(wrapper.find('CodeEditor').prop('value')).toEqual(value);
+  });
+
+  it('should retain expanded yaml if JSON value is edited', async () => {
+    const value = '---\na: &aa [a,b,c]\nb: *aa';
+    const wrapper = mountWithContexts(
+      <Formik initialValues={{ variables: value }}>
+        {() => (
+          <VariablesField id="the-field" name="variables" label="Variables" />
+        )}
+      </Formik>
+    );
+    const jsButton = wrapper.find('Button.toggle-button-javascript');
+    await act(async () => {
+      jsButton.simulate('click');
+    });
+    wrapper.update();
+    wrapper.find('CodeEditor').invoke('onChange')(
+      '{\n  "foo": "bar",\n  "baz": 3\n}'
+    );
+    const yamlButton = wrapper.find('Button.toggle-button-yaml');
+    await act(async () => {
+      yamlButton.simulate('click');
+    });
+    wrapper.update();
+    expect(wrapper.find('CodeEditor').prop('mode')).toEqual('yaml');
     expect(wrapper.find('CodeEditor').prop('value')).toEqual(
       'foo: bar\nbaz: 3\n'
     );
   });
 
+  it('should retain non-expanded yaml if YAML value is edited', async () => {
+    const value = '---\na: &aa [a,b,c]\nb: *aa';
+    const wrapper = mountWithContexts(
+      <Formik initialValues={{ variables: value }}>
+        {() => (
+          <VariablesField id="the-field" name="variables" label="Variables" />
+        )}
+      </Formik>
+    );
+    wrapper.find('CodeEditor').invoke('onChange')(
+      '---\na: &aa [a,b,c]\nb: *aa\n'
+    );
+    const buttons = wrapper.find('Button');
+    await act(async () => {
+      buttons.at(1).simulate('click');
+    });
+    wrapper.update();
+    const buttons2 = wrapper.find('Button');
+    await act(async () => {
+      buttons2.at(0).simulate('click');
+    });
+    wrapper.update();
+    expect(wrapper.find('CodeEditor').prop('mode')).toEqual('yaml');
+    expect(wrapper.find('CodeEditor').prop('value')).toEqual(
+      '---\na: &aa [a,b,c]\nb: *aa\n'
+    );
+  });
+
   it('should set Formik error if yaml is invalid', async () => {
     const value = '---\nfoo bar\n';
     const wrapper = mountWithContexts(

awx/ui_next/src/components/MultiButtonToggle/MultiButtonToggle.jsx

@@ -25,6 +25,7 @@ function MultiButtonToggle({ buttons, value, onChange }) {
           <SmallButton
             aria-label={buttonLabel}
             key={buttonLabel}
+            className={`toggle-button-${buttonValue}`}
             onClick={() => setValue(buttonValue)}
             variant={buttonValue === value ? 'primary' : 'secondary'}
           >