From f051c4d58a4de9eefe4d82783db69806dc14087c Mon Sep 17 00:00:00 2001 From: Alex Corey Date: Fri, 2 Oct 2020 11:17:22 -0400 Subject: [PATCH] fixes bug with disappearing modal and arguments field validation --- awx/ui_next/src/api/models/Groups.js | 12 +- .../AdHocCommands/AdHocCommands.jsx | 107 +++------ .../AdHocCommands/AdHocCommands.test.jsx | 179 ++------------ .../AdHocCommands/AdHocDetailsStep.jsx | 71 ++++-- .../AdHocCommands/AdHocDetailsStep.test.jsx | 5 +- .../InventoryGroupHostList.jsx | 80 ++++--- .../InventoryGroupHostList.test.jsx | 36 +-- .../InventoryGroups/InventoryGroupsList.jsx | 87 +++---- .../InventoryGroupsList.test.jsx | 24 +- .../InventoryHostGroupsList.jsx | 82 ++++--- .../InventoryHosts/InventoryHostList.jsx | 190 +++++++-------- .../InventoryHosts/InventoryHostList.test.jsx | 20 +- .../SmartInventoryHostList.jsx | 220 +++++++++--------- .../SmartInventoryHostList.test.jsx | 42 ++-- 14 files changed, 508 insertions(+), 647 deletions(-) diff --git a/awx/ui_next/src/api/models/Groups.js b/awx/ui_next/src/api/models/Groups.js index ea506cc303..0e76edca8c 100644 --- a/awx/ui_next/src/api/models/Groups.js +++ b/awx/ui_next/src/api/models/Groups.js @@ -12,7 +12,9 @@ class Groups extends Base { } associateHost(id, hostId) { - return this.http.post(`${this.baseUrl}${id}/hosts/`, { id: hostId }); + return this.http.post(`${this.baseUrl}${id}/hosts/`, { + id: hostId, + }); } createHost(id, data) { @@ -20,7 +22,9 @@ class Groups extends Base { } readAllHosts(id, params) { - return this.http.get(`${this.baseUrl}${id}/all_hosts/`, { params }); + return this.http.get(`${this.baseUrl}${id}/all_hosts/`, { + params, + }); } disassociateHost(id, host) { @@ -29,6 +33,10 @@ class Groups extends Base { disassociate: true, }); } + + readChildren(id, params) { + return this.http.get(`${this.baseUrl}${id}/children/`, params); + } } export default Groups; diff --git a/awx/ui_next/src/components/AdHocCommands/AdHocCommands.jsx b/awx/ui_next/src/components/AdHocCommands/AdHocCommands.jsx index a16ff73a38..927ad09f78 100644 --- a/awx/ui_next/src/components/AdHocCommands/AdHocCommands.jsx +++ b/awx/ui_next/src/components/AdHocCommands/AdHocCommands.jsx @@ -1,19 +1,25 @@ -import React, { useState, Fragment, useCallback, useEffect } from 'react'; +import React, { useCallback } from 'react'; import { useHistory } from 'react-router-dom'; import { withI18n } from '@lingui/react'; import { t } from '@lingui/macro'; import PropTypes from 'prop-types'; import useRequest, { useDismissableError } from '../../util/useRequest'; +import { InventoriesAPI } from '../../api'; + import AlertModal from '../AlertModal'; -import { CredentialTypesAPI } from '../../api'; import ErrorDetail from '../ErrorDetail'; import AdHocCommandsWizard from './AdHocCommandsWizard'; import ContentLoading from '../ContentLoading'; -import ContentError from '../ContentError'; -function AdHocCommands({ children, apiModule, adHocItems, itemId, i18n }) { - const [isWizardOpen, setIsWizardOpen] = useState(false); +function AdHocCommands({ + onClose, + adHocItems, + itemId, + i18n, + moduleOptions, + credentialTypeId, +}) { const history = useHistory(); const verbosityOptions = [ { value: '0', key: '0', label: i18n._(t`0 (Normal)`) }, @@ -22,59 +28,26 @@ function AdHocCommands({ children, apiModule, adHocItems, itemId, i18n }) { { value: '3', key: '3', label: i18n._(t`3 (Debug)`) }, { value: '4', key: '4', label: i18n._(t`4 (Connection Debug)`) }, ]; - const { - error: fetchError, - request: fetchModuleOptions, - result: { moduleOptions, credentialTypeId, isDisabled }, - } = useRequest( - useCallback(async () => { - const [choices, credId] = await Promise.all([ - apiModule.readAdHocOptions(itemId), - CredentialTypesAPI.read({ namespace: 'ssh' }), - ]); - const itemObject = (item, index) => { - return { - key: index, - value: item, - label: `${item}`, - isDisabled: false, - }; - }; - - const options = choices.data.actions.GET.module_name.choices.map( - (choice, index) => itemObject(choice[0], index) - ); - return { - moduleOptions: [itemObject('', -1), ...options], - credentialTypeId: credId.data.results[0].id, - isDisabled: !choices.data.actions.POST, - }; - }, [itemId, apiModule]), - { moduleOptions: [], isDisabled: true } - ); - - useEffect(() => { - fetchModuleOptions(); - }, [fetchModuleOptions]); const { isloading: isLaunchLoading, - error: launchError, + error, request: launchAdHocCommands, } = useRequest( useCallback( async values => { - const { data } = await apiModule.launchAdHocCommands(itemId, values); + const { data } = await InventoriesAPI.launchAdHocCommands( + itemId, + values + ); history.push(`/jobs/command/${data.id}/output`); }, - [apiModule, itemId, history] + [itemId, history] ) ); - const { error, dismissError } = useDismissableError( - launchError || fetchError - ); + const { dismissError } = useDismissableError(error); const handleSubmit = async values => { const { credential, ...remainingValues } = values; @@ -85,14 +58,13 @@ function AdHocCommands({ children, apiModule, adHocItems, itemId, i18n }) { ...remainingValues, }; await launchAdHocCommands(manipulatedValues); - setIsWizardOpen(false); }; if (isLaunchLoading) { return ; } - if (error && isWizardOpen) { + if (error) { return ( { dismissError(); - setIsWizardOpen(false); }} > - {launchError ? ( - <> - {i18n._(t`Failed to launch job.`)} - - - ) : ( - - )} + <> + {i18n._(t`Failed to launch job.`)} + + ); } return ( - - {children({ - openAdHocCommands: () => setIsWizardOpen(true), - isDisabled, - })} - - {isWizardOpen && ( - setIsWizardOpen(false)} - onLaunch={handleSubmit} - onDismissError={() => dismissError()} - /> - )} - + dismissError()} + /> ); } AdHocCommands.propTypes = { - children: PropTypes.func.isRequired, adHocItems: PropTypes.arrayOf(PropTypes.object).isRequired, itemId: PropTypes.number.isRequired, }; diff --git a/awx/ui_next/src/components/AdHocCommands/AdHocCommands.test.jsx b/awx/ui_next/src/components/AdHocCommands/AdHocCommands.test.jsx index 5d54fd1b1f..f83009b46a 100644 --- a/awx/ui_next/src/components/AdHocCommands/AdHocCommands.test.jsx +++ b/awx/ui_next/src/components/AdHocCommands/AdHocCommands.test.jsx @@ -18,6 +18,10 @@ const credentials = [ { id: 4, kind: 'Machine', name: 'Cred 4', url: 'www.google.com' }, { id: 5, kind: 'Machine', name: 'Cred 5', url: 'www.google.com' }, ]; +const moduleOptions = [ + ['command', 'command'], + ['shell', 'shell'], +]; const adHocItems = [ { name: 'Inventory 1 Org 0', @@ -25,14 +29,6 @@ const adHocItems = [ { name: 'Inventory 2 Org 0' }, ]; -const children = ({ openAdHocCommands, isDisabled }) => ( - - )} - + {i18n._(t`Run command`)} + ) @@ -279,6 +280,7 @@ function InventoryGroupHostList({ i18n }) { emptyStateControls={ canAdd && ( setIsModalOpen(true)} onAddNew={() => history.push(addFormUrl)} /> @@ -296,6 +298,16 @@ function InventoryGroupHostList({ i18n }) { title={i18n._(t`Select Hosts`)} /> )} + {isAdHocCommandsOpen && ( + setIsAdHocCommandsOpen(false)} + credentialTypeId={credentialTypeId} + moduleOptions={moduleOptions} + /> + )} {associateError && ( ', () => { }, }, }); + InventoriesAPI.readAdHocOptions.mockResolvedValue({ + data: { + actions: { + GET: { module_name: { choices: [['module']] } }, + POST: {}, + }, + }, + }); + CredentialTypesAPI.read.mockResolvedValue({ + data: { count: 1, results: [{ id: 1, name: 'cred' }] }, + }); await act(async () => { wrapper = mountWithContexts(); }); @@ -108,31 +119,8 @@ describe('', () => { }, }, }); - InventoriesAPI.readAdHocOptions.mockResolvedValue({ - data: { - actions: { - GET: { module_name: { choices: [['module']] } }, - POST: {}, - }, - }, - }); - CredentialTypesAPI.read.mockResolvedValue({ - data: { count: 1, results: [{ id: 1, name: 'cred' }] }, - }); await act(async () => { - wrapper = mountWithContexts( - - {({ openAdHocCommands, isDisabled }) => ( - - )} - + {i18n._(t`Run command`)} + @@ -321,6 +316,16 @@ function InventoryGroupsList({ i18n }) { ) } /> + {isAdHocCommandsOpen && ( + setIsAdHocCommandsOpen(false)} + credentialTypeId={credentialTypeId} + moduleOptions={moduleOptions} + /> + )} {deletionError && ( ', () => { await act(async () => { wrapper = mountWithContexts( - - {({ openAdHocCommands, isDisabled }) => ( - - )} - + {i18n._(t`Run command`)} + ) @@ -290,6 +288,16 @@ function InventoryHostGroupsList({ i18n }) { title={i18n._(t`Select Groups`)} /> )} + {isAdHocCommandsOpen && ( + setIsAdHocCommandsOpen(false)} + credentialTypeId={credentialTypeId} + moduleOptions={moduleOptions} + /> + )} {error && ( { - const params = parseQueryString(QS_CONFIG, queryString); - return InventoriesAPI.readHosts(hostId, params); - }; + const { + result: { + hosts, + hostCount, + actions, + relatedSearchableKeys, + searchableKeys, + moduleOptions, + credentialTypeId, + isAdHocDisabled, + }, + error: contentError, + isLoading, + request: fetchData, + } = useRequest( + useCallback(async () => { + const params = parseQueryString(QS_CONFIG, search); + const [response, hostOptions, adHocOptions, cred] = await Promise.all([ + InventoriesAPI.readHosts(id, params), + InventoriesAPI.readHostsOptions(id), + InventoriesAPI.readAdHocOptions(id), + CredentialTypesAPI.read({ namespace: 'ssh' }), + ]); + + return { + hosts: response.data.results, + hostCount: response.data.count, + actions: hostOptions.data.actions, + relatedSearchableKeys: ( + hostOptions?.data?.related_search_fields || [] + ).map(val => val.slice(0, -8)), + searchableKeys: Object.keys(hostOptions.data.actions?.GET || {}).filter( + key => hostOptions.data.actions?.GET[key].filterable + ), + moduleOptions: adHocOptions.data.actions.GET.module_name.choices, + credentialTypeId: cred.data.results[0].id, + isAdHocDisabled: !adHocOptions.data.actions.POST, + }; + }, [id, search]), + { + hosts: [], + hostCount: 0, + actions: {}, + relatedSearchableKeys: [], + searchableKeys: [], + moduleOptions: [], + isAdHocDisabled: true, + } + ); useEffect(() => { - async function fetchData() { - try { - const [ - { - data: { count, results }, - }, - { - data: { actions: optionActions }, - }, - ] = await Promise.all([ - fetchHosts(id, search), - InventoriesAPI.readOptions(), - ]); - - setHosts(results); - setHostCount(count); - setActions(optionActions); - } catch (error) { - setContentError(error); - } finally { - setIsLoading(false); - } - } - fetchData(); - }, [id, search]); + }, [fetchData]); const handleSelectAll = isSelected => { setSelected(isSelected ? [...hosts] : []); @@ -83,30 +99,17 @@ function InventoryHostList({ i18n }) { setSelected(selected.concat(row)); } }; - - const handleDelete = async () => { - setIsLoading(true); - - try { + const { + isLoading: isDeleteLoading, + deleteItems: deleteHosts, + deletionError, + clearDeletionError, + } = useDeleteItems( + useCallback(async () => { await Promise.all(selected.map(host => HostsAPI.destroy(host.id))); - } catch (error) { - setDeletionError(error); - } finally { - setSelected([]); - try { - const { - data: { count, results }, - } = await fetchHosts(id, search); - - setHosts(results); - setHostCount(count); - } catch (error) { - setContentError(error); - } finally { - setIsLoading(false); - } - } - }; + }, [selected]), + { qsConfig: QS_CONFIG, fetchItems: fetchData } + ); const canAdd = actions && Object.prototype.hasOwnProperty.call(actions, 'POST'); @@ -116,7 +119,7 @@ function InventoryHostList({ i18n }) { <> ( {({ isKebabified }) => isKebabified ? ( - setIsAdHocCommandsOpen(true)} + isDisabled={hostCount === 0 || isAdHocDisabled} + aria-label={i18n._(t`Run command`)} > - {({ openAdHocCommands, isDisabled }) => ( - - {i18n._(t`Run command`)} - - )} - + {i18n._(t`Run command`)} + ) : ( - setIsAdHocCommandsOpen(true)} + isDisabled={hostCount === 0 || isAdHocDisabled} > - {({ openAdHocCommands, isDisabled }) => ( - - )} - + {i18n._(t`Run command`)} + ) @@ -208,7 +198,7 @@ function InventoryHostList({ i18n }) { , , @@ -234,12 +224,22 @@ function InventoryHostList({ i18n }) { ) } /> + {isAdHocCommandsOpen && ( + setIsAdHocCommandsOpen(false)} + credentialTypeId={credentialTypeId} + moduleOptions={moduleOptions} + itemId={id} + /> + )} {deletionError && ( setDeletionError(null)} + onClose={clearDeletionError} > {i18n._(t`Failed to delete one or more hosts.`)} diff --git a/awx/ui_next/src/screens/Inventory/InventoryHosts/InventoryHostList.test.jsx b/awx/ui_next/src/screens/Inventory/InventoryHosts/InventoryHostList.test.jsx index 85a25f11bb..b71b0289ca 100644 --- a/awx/ui_next/src/screens/Inventory/InventoryHosts/InventoryHostList.test.jsx +++ b/awx/ui_next/src/screens/Inventory/InventoryHosts/InventoryHostList.test.jsx @@ -85,7 +85,7 @@ describe('', () => { results: mockHosts, }, }); - InventoriesAPI.readOptions.mockResolvedValue({ + InventoriesAPI.readHostsOptions.mockResolvedValue({ data: { actions: { GET: {}, @@ -276,8 +276,15 @@ describe('', () => { expect(wrapper.find('ToolbarAddButton').length).toBe(1); }); + test('should render enabled ad hoc commands button', async () => { + await waitForElement( + wrapper, + 'button[aria-label="Run command"]', + el => el.prop('disabled') === false + ); + }); test('should hide Add button for users without ability to POST', async () => { - InventoriesAPI.readOptions.mockResolvedValueOnce({ + InventoriesAPI.readHostsOptions.mockResolvedValueOnce({ data: { actions: { GET: {}, @@ -294,7 +301,7 @@ describe('', () => { }); test('should show content error when api throws error on initial render', async () => { - InventoriesAPI.readOptions.mockImplementation(() => + InventoriesAPI.readHostsOptions.mockImplementation(() => Promise.reject(new Error()) ); await act(async () => { @@ -304,11 +311,4 @@ describe('', () => { }); await waitForElement(wrapper, 'ContentError', el => el.length === 1); }); - test('should render enabled ad hoc commands button', async () => { - await waitForElement( - wrapper, - 'button[aria-label="Run command"]', - el => el.prop('disabled') === false - ); - }); }); diff --git a/awx/ui_next/src/screens/Inventory/SmartInventoryHosts/SmartInventoryHostList.jsx b/awx/ui_next/src/screens/Inventory/SmartInventoryHosts/SmartInventoryHostList.jsx index e5539647be..4322463741 100644 --- a/awx/ui_next/src/screens/Inventory/SmartInventoryHosts/SmartInventoryHostList.jsx +++ b/awx/ui_next/src/screens/Inventory/SmartInventoryHosts/SmartInventoryHostList.jsx @@ -1,4 +1,4 @@ -import React, { useEffect, useCallback } from 'react'; +import React, { useEffect, useCallback, useState } from 'react'; import { useLocation } from 'react-router-dom'; import { withI18n } from '@lingui/react'; import { t } from '@lingui/macro'; @@ -14,10 +14,10 @@ import SmartInventoryHostListItem from './SmartInventoryHostListItem'; import useRequest from '../../../util/useRequest'; import useSelected from '../../../util/useSelected'; import { getQSConfig, parseQueryString } from '../../../util/qs'; -import { InventoriesAPI } from '../../../api'; +import { InventoriesAPI, CredentialTypesAPI } from '../../../api'; import { Inventory } from '../../../types'; import { Kebabified } from '../../../contexts/Kebabified'; -import AdHocCommandsButton from '../../../components/AdHocCommands/AdHocCommands'; +import AdHocCommands from '../../../components/AdHocCommands/AdHocCommands'; const QS_CONFIG = getQSConfig('host', { page: 1, @@ -27,24 +27,35 @@ const QS_CONFIG = getQSConfig('host', { function SmartInventoryHostList({ i18n, inventory }) { const location = useLocation(); + const [isAdHocCommandsOpen, setIsAdHocCommandsOpen] = useState(false); const { - result: { hosts, count }, + result: { hosts, count, moduleOptions, credentialTypeId, isAdHocDisabled }, error: contentError, isLoading, request: fetchHosts, } = useRequest( useCallback(async () => { const params = parseQueryString(QS_CONFIG, location.search); - const { data } = await InventoriesAPI.readHosts(inventory.id, params); + const [hostResponse, adHocOptions, cred] = await Promise.all([ + InventoriesAPI.readHosts(inventory.id, params), + InventoriesAPI.readAdHocOptions(inventory.id), + CredentialTypesAPI.read({ namespace: 'ssh' }), + ]); + return { - hosts: data.results, - count: data.count, + hosts: hostResponse.data.results, + count: hostResponse.data.count, + moduleOptions: adHocOptions.data.actions.GET.module_name.choices, + credentialTypeId: cred.data.results[0].id, + isAdHocDisabled: !adHocOptions.data.actions.POST, }; }, [location.search, inventory.id]), { hosts: [], count: 0, + moduleOptions: [], + isAdHocDisabled: true, } ); @@ -57,109 +68,106 @@ function SmartInventoryHostList({ i18n, inventory }) { }, [fetchHosts]); return ( - ( - setSelected(isSelected ? [...hosts] : [])} - qsConfig={QS_CONFIG} - additionalControls={ - inventory?.summary_fields?.user_capabilities?.adhoc - ? [ - - {({ isKebabified }) => - isKebabified ? ( - - {({ openAdHocCommands, isDisabled }) => ( - - {i18n._(t`Run command`)} - - )} - - ) : ( - - + ( + + setSelected(isSelected ? [...hosts] : []) + } + qsConfig={QS_CONFIG} + additionalControls={ + inventory?.summary_fields?.user_capabilities?.adhoc + ? [ + + {({ isKebabified }) => + isKebabified ? ( + setIsAdHocCommandsOpen(true)} + isDisabled={count === 0 || isAdHocDisabled} > - - {({ openAdHocCommands, isDisabled }) => ( - + {i18n._(t`Run command`)} + + ) : ( + + - - - ) - } - , - ] - : [] - } + position="top" + key="adhoc" + > + + + + ) + } + , + ] + : [] + } + /> + )} + renderItem={host => ( + row.id === host.id)} + onSelect={() => handleSelect(host)} + /> + )} + /> + {isAdHocCommandsOpen && ( + setIsAdHocCommandsOpen(false)} + credentialTypeId={credentialTypeId} + moduleOptions={moduleOptions} /> )} - renderItem={host => ( - row.id === host.id)} - onSelect={() => handleSelect(host)} - /> - )} - /> + ); } diff --git a/awx/ui_next/src/screens/Inventory/SmartInventoryHosts/SmartInventoryHostList.test.jsx b/awx/ui_next/src/screens/Inventory/SmartInventoryHosts/SmartInventoryHostList.test.jsx index a1bb33f221..60fd23d75a 100644 --- a/awx/ui_next/src/screens/Inventory/SmartInventoryHosts/SmartInventoryHostList.test.jsx +++ b/awx/ui_next/src/screens/Inventory/SmartInventoryHosts/SmartInventoryHostList.test.jsx @@ -12,7 +12,6 @@ import mockHosts from '../shared/data.hosts.json'; jest.mock('../../../api'); describe('', () => { - // describe('User has adhoc permissions', () => { let wrapper; const clonedInventory = { ...mockInventory, @@ -41,17 +40,7 @@ describe('', () => { }); await act(async () => { wrapper = mountWithContexts( - - {({ openAdHocCommands, isDisabled }) => ( -