From 634c9892df640b79a5e72a5c46822aeea03bee3a Mon Sep 17 00:00:00 2001 From: "Keith J. Grant" Date: Thu, 11 Mar 2021 10:49:57 -0800 Subject: [PATCH 1/6] VariablesDetail: don't evaluate YAML expressions --- .../components/CodeEditor/VariablesDetail.jsx | 76 +++++++++---------- .../CodeEditor/VariablesDetail.test.jsx | 2 +- 2 files changed, 35 insertions(+), 43 deletions(-) diff --git a/awx/ui_next/src/components/CodeEditor/VariablesDetail.jsx b/awx/ui_next/src/components/CodeEditor/VariablesDetail.jsx index 797c10673d..5063ab033f 100644 --- a/awx/ui_next/src/components/CodeEditor/VariablesDetail.jsx +++ b/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) { - if (!value) { - if (mode === JSON_MODE) { - return '{}'; - } - return '---'; - } - const modeMatches = isJsonString(value) === (mode === JSON_MODE); - if (modeMatches) { - return value; - } - return mode === YAML_MODE ? jsonToYaml(value) : yamlToJson(value); -} - -function VariablesDetail({ dataCy, helpText, value, label, rows, i18n }) { +function VariablesDetail({ + dataCy, + helpText, + value, + label, + rows, + fullHeight, + 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(() => { - setCurrentValue( - getValueAsMode( - isJsonObject(value) ? JSON.stringify(value, null, 2) : value, - mode - ) - ); - /* eslint-disable-next-line react-hooks/exhaustive-deps */ - }, [value]); + let currentValue = value; + let error; + + const getValueInCurrentMode = () => { + if (!value) { + if (mode === JSON_MODE) { + return '{}'; + } + 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} /> { - try { - setCurrentValue(getValueAsMode(currentValue, newMode)); - setMode(newMode); - } catch (err) { - setError(err); - } + setMode(newMode); }} /> diff --git a/awx/ui_next/src/components/CodeEditor/VariablesDetail.test.jsx b/awx/ui_next/src/components/CodeEditor/VariablesDetail.test.jsx index 2705f2b2cf..e616c1beaf 100644 --- a/awx/ui_next/src/components/CodeEditor/VariablesDetail.test.jsx +++ b/awx/ui_next/src/components/CodeEditor/VariablesDetail.test.jsx @@ -39,7 +39,7 @@ describe('', () => { 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', () => { From 1bd71024e34538d9c9f571b4ff889277a9db3c8d Mon Sep 17 00:00:00 2001 From: "Keith J. Grant" Date: Mon, 15 Mar 2021 16:07:36 -0700 Subject: [PATCH 2/6] Don't unecessarily expand YAML expressions If the user toggles a VariablesField to JSON then back to YAML without editing the field content, revert to the initial YAML value to maintain any shorthand expressions --- .../components/CodeEditor/VariablesField.jsx | 42 ++++++++++----- .../CodeEditor/VariablesField.test.jsx | 53 +++++++++++++++++++ 2 files changed, 81 insertions(+), 14 deletions(-) diff --git a/awx/ui_next/src/components/CodeEditor/VariablesField.jsx b/awx/ui_next/src/components/CodeEditor/VariablesField.jsx index ed56f5376c..89260a6ea2 100644 --- a/awx/ui_next/src/components/CodeEditor/VariablesField.jsx +++ b/awx/ui_next/src/components/CodeEditor/VariablesField.jsx @@ -73,8 +73,29 @@ function VariablesField({ }, [shouldValidate, validate] // eslint-disable-line react-hooks/exhaustive-deps ); + const [initialYamlValue] = useState(mode === YAML_MODE ? field.value : null); + const [isEdited, setIsEdited] = useState(false); const [isExpanded, setIsExpanded] = useState(false); + const handleModeChange = newMode => { + if (newMode === YAML_MODE && !isEdited && initialYamlValue) { + helpers.setValue(initialYamlValue); + setMode(newMode); + return; + } + + try { + const newVal = + newMode === YAML_MODE + ? jsonToYaml(field.value) + : yamlToJson(field.value); + helpers.setValue(newVal); + setMode(newMode); + } catch (err) { + helpers.setError(err.message); + } + }; + return ( <> setIsExpanded(true)} mode={mode} - setMode={setMode} + setMode={handleModeChange} + setIsEdited={setIsEdited} setShouldValidate={setShouldValidate} /> @@ -155,6 +178,7 @@ function VariablesFieldInternals({ mode, setMode, onExpand, + setIsEdited, setShouldValidate, }) { const [field, meta, helpers] = useField(name); @@ -176,18 +200,7 @@ function VariablesFieldInternals({ [JSON_MODE, 'JSON'], ]} value={mode} - onChange={newMode => { - try { - const newVal = - newMode === YAML_MODE - ? jsonToYaml(field.value) - : yamlToJson(field.value); - helpers.setValue(newVal); - setMode(newMode); - } catch (err) { - helpers.setError(err.message); - } - }} + onChange={setMode} /> @@ -214,6 +227,7 @@ function VariablesFieldInternals({ readOnly={readOnly} {...field} onChange={newVal => { + setIsEdited(true); helpers.setValue(newVal); }} fullHeight={fullHeight} diff --git a/awx/ui_next/src/components/CodeEditor/VariablesField.test.jsx b/awx/ui_next/src/components/CodeEditor/VariablesField.test.jsx index 24c896069e..20efb1a2a6 100644 --- a/awx/ui_next/src/components/CodeEditor/VariablesField.test.jsx +++ b/awx/ui_next/src/components/CodeEditor/VariablesField.test.jsx @@ -51,6 +51,59 @@ 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 value not edited', async () => { + const value = '---\na: &aa [a,b,c]\nb: *aa'; + const wrapper = mountWithContexts( + + {() => ( + + )} + + ); + const buttons = wrapper.find('Button'); + expect(buttons).toHaveLength(2); + 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(value); + }); + + it('should retain expanded yaml if value is edited', async () => { + const value = '---\na: &aa [a,b,c]\nb: *aa'; + const wrapper = mountWithContexts( + + {() => ( + + )} + + ); + const buttons = wrapper.find('Button'); + expect(buttons).toHaveLength(2); + await act(async () => { + buttons.at(1).simulate('click'); + }); + wrapper.update(); + wrapper.find('CodeEditor').invoke('onChange')( + '{\n "foo": "bar",\n "baz": 3\n}' + ); + 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( 'foo: bar\nbaz: 3\n' ); From dff3103d96c705838c4cc1590121868dee829e81 Mon Sep 17 00:00:00 2001 From: "Keith J. Grant" Date: Tue, 16 Mar 2021 11:54:13 -0700 Subject: [PATCH 3/6] add more robust handling of JSON/YAML toggle to prevent expanding YAML expressions --- .../src/components/CodeEditor/CodeEditor.jsx | 4 +-- .../components/CodeEditor/VariablesField.jsx | 33 ++++++++++++------- .../CodeEditor/VariablesField.test.jsx | 32 ++++++++++++++++-- 3 files changed, 53 insertions(+), 16 deletions(-) diff --git a/awx/ui_next/src/components/CodeEditor/CodeEditor.jsx b/awx/ui_next/src/components/CodeEditor/CodeEditor.jsx index fac99894f4..c112ccc651 100644 --- a/awx/ui_next/src/components/CodeEditor/CodeEditor.jsx +++ b/awx/ui_next/src/components/CodeEditor/CodeEditor.jsx @@ -11,7 +11,6 @@ import 'ace-builds/src-noconflict/theme-github'; import { withI18n } from '@lingui/react'; import { t } from '@lingui/macro'; import styled from 'styled-components'; -import debounce from '../../util/debounce'; config.set('loadWorkerFromBlob', false); @@ -140,7 +139,8 @@ function CodeEditor({ mode={aceModes[mode] || 'text'} className={`pf-c-form-control ${className}`} theme="github" - onChange={debounce(onChange, 250)} + onChange={onChange} + debounceChangePeriod={250} value={value} onFocus={onFocus} onBlur={onBlur} diff --git a/awx/ui_next/src/components/CodeEditor/VariablesField.jsx b/awx/ui_next/src/components/CodeEditor/VariablesField.jsx index 89260a6ea2..3b1b1e528f 100644 --- a/awx/ui_next/src/components/CodeEditor/VariablesField.jsx +++ b/awx/ui_next/src/components/CodeEditor/VariablesField.jsx @@ -73,13 +73,15 @@ function VariablesField({ }, [shouldValidate, validate] // eslint-disable-line react-hooks/exhaustive-deps ); - const [initialYamlValue] = useState(mode === YAML_MODE ? field.value : null); - const [isEdited, setIsEdited] = useState(false); + 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 && !isEdited && initialYamlValue) { - helpers.setValue(initialYamlValue); + if (newMode === YAML_MODE && !isJsonEdited && lastYamlValue !== null) { + helpers.setValue(lastYamlValue); setMode(newMode); return; } @@ -96,6 +98,16 @@ function VariablesField({ } }; + const handleChange = newVal => { + helpers.setValue(newVal); + if (mode === JSON_MODE) { + setIsJsonEdited(true); + } else { + setLastYamlValue(newVal); + setIsJsonEdited(false); + } + }; + return ( <> setIsExpanded(true)} mode={mode} setMode={handleModeChange} - setIsEdited={setIsEdited} setShouldValidate={setShouldValidate} + handleChange={handleChange} /> @@ -178,10 +190,10 @@ function VariablesFieldInternals({ mode, setMode, onExpand, - setIsEdited, setShouldValidate, + handleChange, }) { - const [field, meta, helpers] = useField(name); + const [field, meta] = useField(name); return (
@@ -226,10 +238,7 @@ function VariablesFieldInternals({ mode={mode} readOnly={readOnly} {...field} - onChange={newVal => { - setIsEdited(true); - helpers.setValue(newVal); - }} + onChange={handleChange} fullHeight={fullHeight} onFocus={() => setShouldValidate(false)} onBlur={() => setShouldValidate(true)} diff --git a/awx/ui_next/src/components/CodeEditor/VariablesField.test.jsx b/awx/ui_next/src/components/CodeEditor/VariablesField.test.jsx index 20efb1a2a6..72326f06cf 100644 --- a/awx/ui_next/src/components/CodeEditor/VariablesField.test.jsx +++ b/awx/ui_next/src/components/CodeEditor/VariablesField.test.jsx @@ -56,7 +56,7 @@ describe('VariablesField', () => { ); }); - it('should retain non-expanded yaml if value not edited', async () => { + it('should retain non-expanded yaml if JSON value not edited', async () => { const value = '---\na: &aa [a,b,c]\nb: *aa'; const wrapper = mountWithContexts( @@ -80,7 +80,7 @@ describe('VariablesField', () => { expect(wrapper.find('CodeEditor').prop('value')).toEqual(value); }); - it('should retain expanded yaml if value is edited', async () => { + it('should retain expanded yaml if JSON value is edited', async () => { const value = '---\na: &aa [a,b,c]\nb: *aa'; const wrapper = mountWithContexts( @@ -109,6 +109,34 @@ describe('VariablesField', () => { ); }); + it('should retain non-expanded yaml if YAML value is edited', async () => { + const value = '---\na: &aa [a,b,c]\nb: *aa'; + const wrapper = mountWithContexts( + + {() => ( + + )} + + ); + 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( From 8dd4e6838552c881db6b218b74747acaf853a0b0 Mon Sep 17 00:00:00 2001 From: "Keith J. Grant" Date: Tue, 16 Mar 2021 12:04:11 -0700 Subject: [PATCH 4/6] change CodeEditor onChange back to our debounce implementation --- awx/ui_next/src/components/CodeEditor/CodeEditor.jsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/awx/ui_next/src/components/CodeEditor/CodeEditor.jsx b/awx/ui_next/src/components/CodeEditor/CodeEditor.jsx index c112ccc651..fac99894f4 100644 --- a/awx/ui_next/src/components/CodeEditor/CodeEditor.jsx +++ b/awx/ui_next/src/components/CodeEditor/CodeEditor.jsx @@ -11,6 +11,7 @@ import 'ace-builds/src-noconflict/theme-github'; import { withI18n } from '@lingui/react'; import { t } from '@lingui/macro'; import styled from 'styled-components'; +import debounce from '../../util/debounce'; config.set('loadWorkerFromBlob', false); @@ -139,8 +140,7 @@ function CodeEditor({ mode={aceModes[mode] || 'text'} className={`pf-c-form-control ${className}`} theme="github" - onChange={onChange} - debounceChangePeriod={250} + onChange={debounce(onChange, 250)} value={value} onFocus={onFocus} onBlur={onBlur} From 637b540a4df596d59ac32b8583fe2cfabbf669e4 Mon Sep 17 00:00:00 2001 From: "Keith J. Grant" Date: Wed, 7 Apr 2021 11:08:30 -0700 Subject: [PATCH 5/6] fix button selection in VariablesField tests --- .../CodeEditor/VariablesField.test.jsx | 18 ++++++++---------- .../MultiButtonToggle/MultiButtonToggle.jsx | 1 + 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/awx/ui_next/src/components/CodeEditor/VariablesField.test.jsx b/awx/ui_next/src/components/CodeEditor/VariablesField.test.jsx index 72326f06cf..92ead2400c 100644 --- a/awx/ui_next/src/components/CodeEditor/VariablesField.test.jsx +++ b/awx/ui_next/src/components/CodeEditor/VariablesField.test.jsx @@ -65,15 +65,14 @@ describe('VariablesField', () => { )} ); - const buttons = wrapper.find('Button'); - expect(buttons).toHaveLength(2); + const jsButton = wrapper.find('Button.toggle-button-javascript'); await act(async () => { - buttons.at(1).simulate('click'); + jsButton.simulate('click'); }); wrapper.update(); - const buttons2 = wrapper.find('Button'); + const yamlButton = wrapper.find('Button.toggle-button-yaml'); await act(async () => { - buttons2.at(0).simulate('click'); + yamlButton.simulate('click'); }); wrapper.update(); expect(wrapper.find('CodeEditor').prop('mode')).toEqual('yaml'); @@ -89,18 +88,17 @@ describe('VariablesField', () => { )} ); - const buttons = wrapper.find('Button'); - expect(buttons).toHaveLength(2); + const jsButton = wrapper.find('Button.toggle-button-javascript'); await act(async () => { - buttons.at(1).simulate('click'); + jsButton.simulate('click'); }); wrapper.update(); wrapper.find('CodeEditor').invoke('onChange')( '{\n "foo": "bar",\n "baz": 3\n}' ); - const buttons2 = wrapper.find('Button'); + const yamlButton = wrapper.find('Button.toggle-button-yaml'); await act(async () => { - buttons2.at(0).simulate('click'); + yamlButton.simulate('click'); }); wrapper.update(); expect(wrapper.find('CodeEditor').prop('mode')).toEqual('yaml'); diff --git a/awx/ui_next/src/components/MultiButtonToggle/MultiButtonToggle.jsx b/awx/ui_next/src/components/MultiButtonToggle/MultiButtonToggle.jsx index efc1cb3d92..8ce9038682 100644 --- a/awx/ui_next/src/components/MultiButtonToggle/MultiButtonToggle.jsx +++ b/awx/ui_next/src/components/MultiButtonToggle/MultiButtonToggle.jsx @@ -25,6 +25,7 @@ function MultiButtonToggle({ buttons, value, onChange }) { setValue(buttonValue)} variant={buttonValue === value ? 'primary' : 'secondary'} > From 8f2ef6ce019b99c39d89598ee836670d085808d0 Mon Sep 17 00:00:00 2001 From: "Keith J. Grant" Date: Wed, 14 Apr 2021 16:15:18 -0700 Subject: [PATCH 6/6] VariablesField: don't run Formik validation on mode change --- awx/ui_next/src/components/CodeEditor/VariablesField.jsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/awx/ui_next/src/components/CodeEditor/VariablesField.jsx b/awx/ui_next/src/components/CodeEditor/VariablesField.jsx index 3b1b1e528f..58e9960b82 100644 --- a/awx/ui_next/src/components/CodeEditor/VariablesField.jsx +++ b/awx/ui_next/src/components/CodeEditor/VariablesField.jsx @@ -81,7 +81,7 @@ function VariablesField({ const handleModeChange = newMode => { if (newMode === YAML_MODE && !isJsonEdited && lastYamlValue !== null) { - helpers.setValue(lastYamlValue); + helpers.setValue(lastYamlValue, false); setMode(newMode); return; } @@ -91,7 +91,7 @@ function VariablesField({ newMode === YAML_MODE ? jsonToYaml(field.value) : yamlToJson(field.value); - helpers.setValue(newVal); + helpers.setValue(newVal, false); setMode(newMode); } catch (err) { helpers.setError(err.message); @@ -109,7 +109,7 @@ function VariablesField({ }; return ( - <> +
) : null} - +
); } VariablesField.propTypes = {