From ea5e35910fa0ac58b3fe2b6306a37e850114aaef Mon Sep 17 00:00:00 2001 From: Marliana Lara Date: Wed, 20 May 2020 12:29:02 -0400 Subject: [PATCH] Fix project lookup autocomplete behavior * Refactor credential lookup to use useRequest hook * Update unit tests --- .../components/Lookup/CredentialLookup.jsx | 55 +++++------ .../src/components/Lookup/ProjectLookup.jsx | 13 ++- .../components/Lookup/ProjectLookup.test.jsx | 16 +-- .../Inventory/shared/InventorySourceForm.jsx | 2 +- .../InventorySourceSubForms/SCMSubForm.jsx | 17 +++- .../JobTemplateAdd/JobTemplateAdd.test.jsx | 18 +++- .../JobTemplateEdit/JobTemplateEdit.test.jsx | 19 +++- .../Template/shared/JobTemplateForm.jsx | 98 +++++++++---------- .../Template/shared/JobTemplateForm.test.jsx | 2 + 9 files changed, 141 insertions(+), 99 deletions(-) diff --git a/awx/ui_next/src/components/Lookup/CredentialLookup.jsx b/awx/ui_next/src/components/Lookup/CredentialLookup.jsx index 558b4fc039..32a418a3e7 100644 --- a/awx/ui_next/src/components/Lookup/CredentialLookup.jsx +++ b/awx/ui_next/src/components/Lookup/CredentialLookup.jsx @@ -1,4 +1,4 @@ -import React, { useEffect, useState, useRef } from 'react'; +import React, { useCallback, useEffect } from 'react'; import { bool, func, node, number, string, oneOfType } from 'prop-types'; import { withRouter } from 'react-router-dom'; import { withI18n } from '@lingui/react'; @@ -10,6 +10,7 @@ import { getQSConfig, parseQueryString, mergeParams } from '../../util/qs'; import { FieldTooltip } from '../FormField'; import Lookup from './Lookup'; import OptionsList from '../OptionsList'; +import useRequest from '../../util/useRequest'; import LookupErrorMessage from './shared/LookupErrorMessage'; const QS_CONFIG = getQSConfig('credentials', { @@ -32,20 +33,12 @@ function CredentialLookup({ i18n, tooltip, }) { - const [credentials, setCredentials] = useState([]); - const [count, setCount] = useState(0); - const [error, setError] = useState(null); - const isMounted = useRef(null); - - useEffect(() => { - isMounted.current = true; - return () => { - isMounted.current = false; - }; - }, []); - - useEffect(() => { - (async () => { + const { + result: { count, credentials }, + error, + request: fetchCredentials, + } = useRequest( + useCallback(async () => { const params = parseQueryString(QS_CONFIG, history.location.search); const typeIdParams = credentialTypeId ? { credential_type: credentialTypeId } @@ -54,21 +47,23 @@ function CredentialLookup({ ? { credential_type__kind: credentialTypeKind } : {}; - try { - const { data } = await CredentialsAPI.read( - mergeParams(params, { ...typeIdParams, ...typeKindParams }) - ); - if (isMounted.current) { - setCredentials(data.results); - setCount(data.count); - } - } catch (err) { - if (isMounted.current) { - setError(err); - } - } - })(); - }, [credentialTypeId, credentialTypeKind, history.location.search]); + const { data } = await CredentialsAPI.read( + mergeParams(params, { ...typeIdParams, ...typeKindParams }) + ); + return { + count: data.count, + credentials: data.results, + }; + }, [credentialTypeId, credentialTypeKind, history.location.search]), + { + count: 0, + credentials: [], + } + ); + + useEffect(() => { + fetchCredentials(); + }, [fetchCredentials]); // TODO: replace credential type search with REST-based grabbing of cred types diff --git a/awx/ui_next/src/components/Lookup/ProjectLookup.jsx b/awx/ui_next/src/components/Lookup/ProjectLookup.jsx index b0dfb8e969..2b1ee265ac 100644 --- a/awx/ui_next/src/components/Lookup/ProjectLookup.jsx +++ b/awx/ui_next/src/components/Lookup/ProjectLookup.jsx @@ -21,6 +21,7 @@ const QS_CONFIG = getQSConfig('project', { function ProjectLookup({ helperTextInvalid, + autocomplete, i18n, isValid, onChange, @@ -38,14 +39,14 @@ function ProjectLookup({ useCallback(async () => { const params = parseQueryString(QS_CONFIG, history.location.search); const { data } = await ProjectsAPI.read(params); - if (data.count === 1) { - onChange(data.results[0]); + if (data.count === 1 && autocomplete) { + autocomplete(data.results[0]); } return { count: data.count, projects: data.results, }; - }, [onChange, history.location.search]), + }, [history.location.search, autocomplete]), { count: 0, projects: [], @@ -131,22 +132,24 @@ function ProjectLookup({ } ProjectLookup.propTypes = { - value: Project, + autocomplete: func, helperTextInvalid: node, isValid: bool, onBlur: func, onChange: func.isRequired, required: bool, tooltip: string, + value: Project, }; ProjectLookup.defaultProps = { + autocomplete: () => {}, helperTextInvalid: '', isValid: true, + onBlur: () => {}, required: false, tooltip: '', value: null, - onBlur: () => {}, }; export { ProjectLookup as _ProjectLookup }; diff --git a/awx/ui_next/src/components/Lookup/ProjectLookup.test.jsx b/awx/ui_next/src/components/Lookup/ProjectLookup.test.jsx index b7be75a02e..09bb9e5741 100644 --- a/awx/ui_next/src/components/Lookup/ProjectLookup.test.jsx +++ b/awx/ui_next/src/components/Lookup/ProjectLookup.test.jsx @@ -15,12 +15,14 @@ describe('', () => { count: 1, }, }); - const onChange = jest.fn(); + const autocomplete = jest.fn(); await act(async () => { - mountWithContexts(); + mountWithContexts( + {}} /> + ); }); await sleep(0); - expect(onChange).toHaveBeenCalledWith({ id: 1 }); + expect(autocomplete).toHaveBeenCalledWith({ id: 1 }); }); test('should not auto-select project when multiple available', async () => { @@ -30,11 +32,13 @@ describe('', () => { count: 2, }, }); - const onChange = jest.fn(); + const autocomplete = jest.fn(); await act(async () => { - mountWithContexts(); + mountWithContexts( + {}} /> + ); }); await sleep(0); - expect(onChange).not.toHaveBeenCalled(); + expect(autocomplete).not.toHaveBeenCalled(); }); }); diff --git a/awx/ui_next/src/screens/Inventory/shared/InventorySourceForm.jsx b/awx/ui_next/src/screens/Inventory/shared/InventorySourceForm.jsx index 5dfe2656e6..a70cd2ee14 100644 --- a/awx/ui_next/src/screens/Inventory/shared/InventorySourceForm.jsx +++ b/awx/ui_next/src/screens/Inventory/shared/InventorySourceForm.jsx @@ -144,7 +144,7 @@ const InventorySourceForm = ({ overwrite: source?.overwrite || false, overwrite_vars: source?.overwrite_vars || false, source: source?.source || '', - source_path: source?.source_path || '', + source_path: source?.source_path === '' ? '/ (project root)' : '', source_project: source?.summary_fields?.source_project || null, source_vars: source?.source_vars || '---\n', update_cache_timeout: source?.update_cache_timeout || 0, diff --git a/awx/ui_next/src/screens/Inventory/shared/InventorySourceSubForms/SCMSubForm.jsx b/awx/ui_next/src/screens/Inventory/shared/InventorySourceSubForms/SCMSubForm.jsx index c771298deb..ebfe1dbc2e 100644 --- a/awx/ui_next/src/screens/Inventory/shared/InventorySourceSubForms/SCMSubForm.jsx +++ b/awx/ui_next/src/screens/Inventory/shared/InventorySourceSubForms/SCMSubForm.jsx @@ -37,10 +37,10 @@ const SCMSubForm = ({ i18n }) => { ); useEffect(() => { - if (projectField.value?.id) { - fetchSourcePath(projectField.value.id); + if (projectMeta.initialValue) { + fetchSourcePath(projectMeta.initialValue.id); } - }, [fetchSourcePath, projectField.value]); + }, [fetchSourcePath, projectMeta.initialValue]); const handleProjectUpdate = useCallback( value => { @@ -51,6 +51,16 @@ const SCMSubForm = ({ i18n }) => { [] // eslint-disable-line react-hooks/exhaustive-deps ); + const handleProjectAutocomplete = useCallback( + val => { + projectHelpers.setValue(val); + if (!projectMeta.initialValue) { + fetchSourcePath(val.id); + } + }, + [] // eslint-disable-line react-hooks/exhaustive-deps + ); + return ( <> { }} /> ', () => { beforeEach(() => { @@ -251,7 +264,7 @@ describe('', () => { const expected = { ...mockJobTemplate, - project: mockJobTemplate.project.id, + project: mockJobTemplate.project, ...updatedTemplateData, }; delete expected.summary_fields; diff --git a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx index a3a01c92c7..7ed7b8b116 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx @@ -46,27 +46,25 @@ const { origin } = document.location; function JobTemplateForm({ template, - validateField, handleCancel, handleSubmit, setFieldValue, submitError, i18n, }) { + const { values: formikValues } = useFormikContext(); + const [contentError, setContentError] = useState(false); - const [project, setProject] = useState(template?.summary_fields?.project); const [inventory, setInventory] = useState( template?.summary_fields?.inventory ); const [allowCallbacks, setAllowCallbacks] = useState( Boolean(template?.host_config_key) ); - const [enableWebhooks, setEnableWebhooks] = useState( Boolean(template.webhook_service) ); - const { values: formikValues } = useFormikContext(); const [jobTypeField, jobTypeMeta, jobTypeHelpers] = useField({ name: 'job_type', validate: required(null, i18n), @@ -74,16 +72,13 @@ function JobTemplateForm({ const [, inventoryMeta, inventoryHelpers] = useField('inventory'); const [projectField, projectMeta, projectHelpers] = useField({ name: 'project', - validate: () => handleProjectValidation(), + validate: project => handleProjectValidation(project), }); - const [scmField, , scmHelpers] = useField('scm_branch'); - const [playbookField, playbookMeta, playbookHelpers] = useField({ name: 'playbook', validate: required(i18n._(t`Select a value for this field`), i18n), }); - const [credentialField, , credentialHelpers] = useField('credentials'); const [labelsField, , labelsHelpers] = useField('labels'); const [limitField, limitMeta] = useField('limit'); @@ -101,13 +96,10 @@ function JobTemplateForm({ contentLoading: hasProjectLoading, } = useRequest( useCallback(async () => { - let projectData; if (template?.project) { - projectData = await ProjectsAPI.readDetail(template?.project); - validateField('project'); - setProject(projectData.data); + await ProjectsAPI.readDetail(template?.project); } - }, [template, validateField]) + }, [template]) ); const { @@ -133,26 +125,28 @@ function JobTemplateForm({ loadRelatedInstanceGroups(); }, [loadRelatedInstanceGroups]); - const handleProjectValidation = () => { + const handleProjectValidation = project => { if (!project && projectMeta.touched) { return i18n._(t`Select a value for this field`); } - if (project && project.status === 'never updated') { + if (project?.value?.status === 'never updated') { return i18n._(t`This project needs to be updated`); } return undefined; }; const handleProjectUpdate = useCallback( - newProject => { - if (project?.id !== newProject?.id) { - // Clear the selected playbook value when a different project is selected or - // when the project is deselected. - playbookHelpers.setValue(0); - } - setProject(newProject); - projectHelpers.setValue(newProject); + value => { + playbookHelpers.setValue(0); scmHelpers.setValue(''); + projectHelpers.setValue(value); + }, + [] // eslint-disable-line react-hooks/exhaustive-deps + ); + + const handleProjectAutocomplete = useCallback( + val => { + projectHelpers.setValue(val); }, [] // eslint-disable-line react-hooks/exhaustive-deps ); @@ -189,8 +183,12 @@ function JobTemplateForm({ return ; } - if (instanceGroupError || projectContentError) { - return ; + if (contentError || instanceGroupError || projectContentError) { + return ( + + ); } return ( @@ -261,18 +259,18 @@ function JobTemplateForm({ )} - projectHelpers.setTouched()} tooltip={i18n._(t`Select the project containing the playbook you want this job to execute.`)} isValid={!projectMeta.touched || !projectMeta.error} helperTextInvalid={projectMeta.error} onChange={handleProjectUpdate} + autocomplete={handleProjectAutocomplete} required /> - {project?.allow_override && ( + {projectField.value?.allow_override && ( playbookHelpers.setTouched()} @@ -609,6 +607,8 @@ const FormikApp = withFormik({ } = template; return { + allow_callbacks: template.allow_callbacks || false, + allow_simultaneous: template.allow_simultaneous || false, ask_credential_on_launch: template.ask_credential_on_launch || false, ask_diff_mode_on_launch: template.ask_diff_mode_on_launch || false, ask_inventory_on_launch: template.ask_inventory_on_launch || false, @@ -619,31 +619,29 @@ const FormikApp = withFormik({ ask_tags_on_launch: template.ask_tags_on_launch || false, ask_variables_on_launch: template.ask_variables_on_launch || false, ask_verbosity_on_launch: template.ask_verbosity_on_launch || false, - name: template.name || '', - description: template.description || '', - job_type: template.job_type || 'run', - inventory: template.inventory || null, - project: template.project || null, - scm_branch: template.scm_branch || '', - playbook: template.playbook || '', - labels: summary_fields.labels.results || [], - forks: template.forks || 0, - limit: template.limit || '', - verbosity: template.verbosity || '0', - job_slice_count: template.job_slice_count || 1, - timeout: template.timeout || 0, - diff_mode: template.diff_mode || false, - job_tags: template.job_tags || '', - skip_tags: template.skip_tags || '', become_enabled: template.become_enabled || false, - allow_callbacks: template.allow_callbacks || false, - allow_simultaneous: template.allow_simultaneous || false, - use_fact_cache: template.use_fact_cache || false, + credentials: summary_fields.credentials || [], + description: template.description || '', + diff_mode: template.diff_mode || false, + extra_vars: template.extra_vars || '---\n', + forks: template.forks || 0, host_config_key: template.host_config_key || '', initialInstanceGroups: [], instanceGroups: [], - credentials: summary_fields.credentials || [], - extra_vars: template.extra_vars || '---\n', + inventory: template.inventory || null, + job_slice_count: template.job_slice_count || 1, + job_tags: template.job_tags || '', + job_type: template.job_type || 'run', + labels: summary_fields.labels.results || [], + limit: template.limit || '', + name: template.name || '', + playbook: template.playbook || '', + project: summary_fields?.project || null, + scm_branch: template.scm_branch || '', + skip_tags: template.skip_tags || '', + timeout: template.timeout || 0, + use_fact_cache: template.use_fact_cache || false, + verbosity: template.verbosity || '0', webhook_service: template.webhook_service || '', webhook_url: template?.related?.webhook_receiver ? `${origin}${template.related.webhook_receiver}` diff --git a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx index b4664cb2bf..aef6f61c18 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx @@ -13,6 +13,7 @@ import { JobTemplatesAPI, ProjectsAPI, CredentialsAPI, + CredentialTypesAPI, } from '../../../api'; jest.mock('../../../api'); @@ -99,6 +100,7 @@ describe('', () => { LabelsAPI.read.mockReturnValue({ data: mockData.summary_fields.labels, }); + CredentialTypesAPI.loadAllTypes.mockResolvedValue([]); CredentialsAPI.read.mockReturnValue({ data: { results: mockCredentials }, });