From a1d7beca830615532b346004456efc8ffa1d7d95 Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Wed, 22 Jan 2020 15:42:28 -0800 Subject: [PATCH 1/3] update VariablesDetail properly if value prop changes (preserving current mode) --- .../CodeMirrorInput/VariablesDetail.jsx | 31 ++++++++++++++----- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/awx/ui_next/src/components/CodeMirrorInput/VariablesDetail.jsx b/awx/ui_next/src/components/CodeMirrorInput/VariablesDetail.jsx index 45b02ef216..c2e3fe2b2c 100644 --- a/awx/ui_next/src/components/CodeMirrorInput/VariablesDetail.jsx +++ b/awx/ui_next/src/components/CodeMirrorInput/VariablesDetail.jsx @@ -1,4 +1,4 @@ -import React, { useState } from 'react'; +import React, { useState, useEffect } from 'react'; import { string, number } from 'prop-types'; import { Split, SplitItem, TextListItemVariants } from '@patternfly/react-core'; import { DetailName, DetailValue } from '@components/DetailList'; @@ -7,11 +7,30 @@ import CodeMirrorInput from './CodeMirrorInput'; import YamlJsonToggle from './YamlJsonToggle'; import { JSON_MODE, YAML_MODE } from './constants'; +function getValueAsMode(value, mode) { + if (!value) { + if (mode === JSON_MODE) { + return '{}'; + } + return '---'; + } + const modeMatches = isJson(value) === (mode === JSON_MODE); + if (modeMatches) { + return value; + } + return mode === YAML_MODE ? jsonToYaml(value) : yamlToJson(value); +} + function VariablesDetail({ value, label, rows }) { const [mode, setMode] = useState(isJson(value) ? JSON_MODE : YAML_MODE); - const [currentValue, setCurrentValue] = useState(value); + const [currentValue, setCurrentValue] = useState(value || '---'); const [error, setError] = useState(null); + useEffect(() => { + setCurrentValue(getValueAsMode(value, mode)); + /* eslint-disable-next-line react-hooks/exhaustive-deps */ + }, [value]); + return ( <> { try { - const newVal = - newMode === YAML_MODE - ? jsonToYaml(currentValue) - : yamlToJson(currentValue); - setCurrentValue(newVal); + setCurrentValue(getValueAsMode(currentValue, newMode)); setMode(newMode); } catch (err) { setError(err); @@ -56,7 +71,7 @@ function VariablesDetail({ value, label, rows }) { > Date: Thu, 23 Jan 2020 11:02:29 -0800 Subject: [PATCH 2/3] prevent inventory updates after unmount --- .../CodeMirrorInput/VariablesDetail.test.jsx | 20 ++++++++++++++++++- .../src/screens/Inventory/Inventory.jsx | 1 + .../InventoryDetail/InventoryDetail.jsx | 14 +++++++++++-- .../Inventory/InventoryEdit/InventoryEdit.jsx | 14 +++++++++++-- 4 files changed, 44 insertions(+), 5 deletions(-) diff --git a/awx/ui_next/src/components/CodeMirrorInput/VariablesDetail.test.jsx b/awx/ui_next/src/components/CodeMirrorInput/VariablesDetail.test.jsx index ea13a366c9..b99fda9f16 100644 --- a/awx/ui_next/src/components/CodeMirrorInput/VariablesDetail.test.jsx +++ b/awx/ui_next/src/components/CodeMirrorInput/VariablesDetail.test.jsx @@ -1,5 +1,6 @@ import React from 'react'; -import { shallow } from 'enzyme'; +import { act } from 'react-dom/test-utils'; +import { shallow, mount } from 'enzyme'; import VariablesDetail from './VariablesDetail'; jest.mock('@api'); @@ -40,9 +41,26 @@ describe('', () => { expect(input2.prop('mode')).toEqual('yaml'); expect(input2.prop('value')).toEqual('foo: bar\n'); }); + test('should render label and value= --- when there are no values', () => { const wrapper = shallow(); expect(wrapper.find('Styled(CodeMirrorInput)').length).toBe(1); expect(wrapper.find('div.pf-c-form__label').text()).toBe('Variables'); }); + + test('should update value if prop changes', () => { + const wrapper = mount( + + ); + act(() => { + wrapper.find('YamlJsonToggle').invoke('onChange')('javascript'); + }); + wrapper.setProps({ + value: '---bar: baz', + }); + wrapper.update(); + const input = wrapper.find('Styled(CodeMirrorInput)'); + expect(input.prop('mode')).toEqual('javascript'); + expect(input.prop('value')).toEqual('{\n "bar": "baz"\n}'); + }); }); diff --git a/awx/ui_next/src/screens/Inventory/Inventory.jsx b/awx/ui_next/src/screens/Inventory/Inventory.jsx index 6d2ccedea6..66d4872d95 100644 --- a/awx/ui_next/src/screens/Inventory/Inventory.jsx +++ b/awx/ui_next/src/screens/Inventory/Inventory.jsx @@ -37,6 +37,7 @@ function Inventory({ i18n, setBreadcrumb }) { useEffect(() => { async function fetchData() { try { + setHasContentLoading(true); const { data } = await InventoriesAPI.readDetail(match.params.id); setBreadcrumb(data); setInventory(data); diff --git a/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx b/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx index d3bc8fe5fa..3788dc130f 100644 --- a/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx +++ b/awx/ui_next/src/screens/Inventory/InventoryDetail/InventoryDetail.jsx @@ -1,4 +1,4 @@ -import React, { useState, useEffect } from 'react'; +import React, { useState, useEffect, useRef } from 'react'; import { Link, useHistory } from 'react-router-dom'; import { withI18n } from '@lingui/react'; import { t } from '@lingui/macro'; @@ -18,19 +18,29 @@ function InventoryDetail({ inventory, i18n }) { const [hasContentLoading, setHasContentLoading] = useState(true); const [contentError, setContentError] = useState(null); const history = useHistory(); + const isMounted = useRef(null); useEffect(() => { + isMounted.current = true; (async () => { setHasContentLoading(true); try { const { data } = await InventoriesAPI.readInstanceGroups(inventory.id); + if (!isMounted.current) { + return; + } setInstanceGroups(data.results); } catch (err) { setContentError(err); } finally { - setHasContentLoading(false); + if (isMounted.current) { + setHasContentLoading(false); + } } })(); + return () => { + isMounted.current = false; + }; }, [inventory.id]); const deleteInventory = async () => { diff --git a/awx/ui_next/src/screens/Inventory/InventoryEdit/InventoryEdit.jsx b/awx/ui_next/src/screens/Inventory/InventoryEdit/InventoryEdit.jsx index 0dfa05ae2e..f0b82ae943 100644 --- a/awx/ui_next/src/screens/Inventory/InventoryEdit/InventoryEdit.jsx +++ b/awx/ui_next/src/screens/Inventory/InventoryEdit/InventoryEdit.jsx @@ -1,4 +1,4 @@ -import React, { useState, useEffect } from 'react'; +import React, { useState, useEffect, useRef } from 'react'; import { useHistory } from 'react-router-dom'; import { object } from 'prop-types'; @@ -15,8 +15,10 @@ function InventoryEdit({ inventory }) { const [contentLoading, setContentLoading] = useState(true); const [credentialTypeId, setCredentialTypeId] = useState(null); const history = useHistory(); + const isMounted = useRef(null); useEffect(() => { + isMounted.current = true; const loadData = async () => { try { const [ @@ -32,15 +34,23 @@ function InventoryEdit({ inventory }) { kind: 'insights', }), ]); + if (!isMounted.current) { + return; + } setInstanceGroups(loadedInstanceGroups); setCredentialTypeId(loadedCredentialTypeId[0].id); } catch (err) { setError(err); } finally { - setContentLoading(false); + if (isMounted.current) { + setContentLoading(false); + } } }; loadData(); + return () => { + isMounted.current = false; + }; }, [inventory.id, contentLoading, inventory, credentialTypeId]); const handleCancel = () => { From c6159a7c3e1755a813b19a9f37c433b6ea86a3b3 Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Thu, 23 Jan 2020 11:15:33 -0800 Subject: [PATCH 3/3] add more VariablesDetail tests --- .../CodeMirrorInput/VariablesDetail.test.jsx | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/awx/ui_next/src/components/CodeMirrorInput/VariablesDetail.test.jsx b/awx/ui_next/src/components/CodeMirrorInput/VariablesDetail.test.jsx index b99fda9f16..05548f8aa8 100644 --- a/awx/ui_next/src/components/CodeMirrorInput/VariablesDetail.test.jsx +++ b/awx/ui_next/src/components/CodeMirrorInput/VariablesDetail.test.jsx @@ -63,4 +63,20 @@ describe('', () => { expect(input.prop('mode')).toEqual('javascript'); expect(input.prop('value')).toEqual('{\n "bar": "baz"\n}'); }); + + test('should default yaml value to "---"', () => { + const wrapper = shallow(); + const input = wrapper.find('Styled(CodeMirrorInput)'); + expect(input.prop('value')).toEqual('---'); + }); + + test('should default empty json to "{}"', () => { + const wrapper = mount(); + act(() => { + wrapper.find('YamlJsonToggle').invoke('onChange')('javascript'); + }); + wrapper.setProps({ value: '' }); + const input = wrapper.find('Styled(CodeMirrorInput)'); + expect(input.prop('value')).toEqual('{}'); + }); });