From 362339d89cd1c7d81a887d72d2f15d8e2782f9e1 Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Mon, 23 Sep 2019 10:59:19 -0700 Subject: [PATCH 01/11] add FieldTooltip component, some JobTemplateForm cleanup --- .../src/components/FormField/FieldTooltip.jsx | 26 +++ awx/ui_next/src/components/FormField/index.js | 1 + .../src/components/ListHeader/ListHeader.jsx | 1 + .../src/components/Lookup/InventoryLookup.jsx | 16 +- .../OrganizationNotifications.test.jsx.snap | 72 +++++++++ .../Template/shared/JobTemplateForm.jsx | 149 ++++++------------ 6 files changed, 152 insertions(+), 113 deletions(-) create mode 100644 awx/ui_next/src/components/FormField/FieldTooltip.jsx diff --git a/awx/ui_next/src/components/FormField/FieldTooltip.jsx b/awx/ui_next/src/components/FormField/FieldTooltip.jsx new file mode 100644 index 0000000000..59031bb365 --- /dev/null +++ b/awx/ui_next/src/components/FormField/FieldTooltip.jsx @@ -0,0 +1,26 @@ +import React from 'react'; +import { node } from 'prop-types'; +import { Tooltip } from '@patternfly/react-core'; +import { QuestionCircleIcon as PFQuestionCircleIcon } from '@patternfly/react-icons'; +import styled from 'styled-components'; + +const QuestionCircleIcon = styled(PFQuestionCircleIcon)` + margin-left: 10px; +`; + +function FieldTooltip({ content }) { + return ( + + + + ); +} +FieldTooltip.propTypes = { + content: node.isRequired, +}; + +export default FieldTooltip; diff --git a/awx/ui_next/src/components/FormField/index.js b/awx/ui_next/src/components/FormField/index.js index c592b7c800..4ce1944f17 100644 --- a/awx/ui_next/src/components/FormField/index.js +++ b/awx/ui_next/src/components/FormField/index.js @@ -1,2 +1,3 @@ export { default } from './FormField'; export { default as CheckboxField } from './CheckboxField'; +export { default as FieldTooltip } from './FieldTooltip'; diff --git a/awx/ui_next/src/components/ListHeader/ListHeader.jsx b/awx/ui_next/src/components/ListHeader/ListHeader.jsx index 4974c001c9..629a85b205 100644 --- a/awx/ui_next/src/components/ListHeader/ListHeader.jsx +++ b/awx/ui_next/src/components/ListHeader/ListHeader.jsx @@ -112,6 +112,7 @@ class ListHeader extends React.Component { columns, onSearch: this.handleSearch, onSort: this.handleSort, + qsConfig, })} InventoriesAPI.read(params); @@ -26,11 +20,7 @@ class InventoryLookup extends React.Component { isRequired={required} fieldId="inventory-lookup" > - {tooltip && ( - - - - )} + {tooltip && } initially renders succesfully 1`] = ` } onSearch={[Function]} onSort={[Function]} + qsConfig={ + Object { + "dateFields": Array [ + "modified", + "created", + ], + "defaultParams": Object { + "order_by": "name", + "page": 1, + "page_size": 5, + }, + "integerFields": Array [ + "page", + "page_size", + ], + "namespace": "notification", + } + } sortOrder="ascending" sortedColumnKey="name" > @@ -423,6 +441,24 @@ exports[` initially renders succesfully 1`] = ` onSearch={[Function]} onSelectAll={null} onSort={[Function]} + qsConfig={ + Object { + "dateFields": Array [ + "modified", + "created", + ], + "defaultParams": Object { + "order_by": "name", + "page": 1, + "page_size": 5, + }, + "integerFields": Array [ + "page", + "page_size", + ], + "namespace": "notification", + } + } showSelectAll={false} sortOrder="ascending" sortedColumnKey="name" @@ -590,6 +626,24 @@ exports[` initially renders succesfully 1`] = ` ] } onSearch={[Function]} + qsConfig={ + Object { + "dateFields": Array [ + "modified", + "created", + ], + "defaultParams": Object { + "order_by": "name", + "page": 1, + "page_size": 5, + }, + "integerFields": Array [ + "page", + "page_size", + ], + "namespace": "notification", + } + } sortedColumnKey="name" > initially renders succesfully 1`] = ` } i18n={"/i18n/"} onSearch={[Function]} + qsConfig={ + Object { + "dateFields": Array [ + "modified", + "created", + ], + "defaultParams": Object { + "order_by": "name", + "page": 1, + "page_size": 5, + }, + "integerFields": Array [ + "page", + "page_size", + ], + "namespace": "notification", + } + } sortedColumnKey="name" >
label { grid-column: 1 / -1; @@ -374,15 +368,12 @@ class JobTemplateForm extends Component { isValid={isValid} label={i18n._(t`Job Type`)} > - - - + the playbook. Select check to only check playbook syntax, + test environment setup, and report problems without + executing the playbook.`)} + /> - - - + /> - - - + /> - - - + produce as the playbook executes.`)} + /> - - - + Ansible tasks, where supported. This is equivalent + to Ansible’s --diff mode.`)} + />
- - - + playbook, and you want to run a specific part of a + play or task. Use commas to separate multiple tags. + Refer to Ansible Tower documentation for details on + the usage of tags.`)} + /> form.setFieldValue(field.name, value)} @@ -624,16 +600,13 @@ class JobTemplateForm extends Component { css="margin-top: 20px" fieldId="template-skip-tags" > - - - + /> form.setFieldValue(field.name, value)} @@ -661,16 +634,13 @@ class JobTemplateForm extends Component { {i18n._(t`Provisioning Callbacks`)}   - - - + /> } id="option-callbacks" @@ -735,50 +705,29 @@ class JobTemplateForm extends Component { const FormikApp = withFormik({ mapPropsToValues(props) { const { template = {} } = props; - const { - name = '', - description = '', - job_type = 'run', - inventory = '', - project = '', - playbook = '', - forks, - limit, - verbosity, - job_slice_count, - timeout, - diff_mode, - job_tags, - skip_tags, - become_enabled, - allow_callbacks, - allow_simultaneous, - use_fact_cache, - host_config_key, - summary_fields = { labels: { results: [] } }, - } = { ...template }; + const { summary_fields = { labels: { results: [] } } } = template; return { - name: name || '', - description: description || '', - job_type: job_type || '', - inventory: inventory || '', - project: project || '', - playbook: playbook || '', + name: template.name || '', + description: template.description || '', + job_type: template.job_type || 'run', + inventory: template.inventory || '', + project: template.project || '', + playbook: template.playbook || '', labels: summary_fields.labels.results, - forks: forks || 0, - limit: limit || '', - verbosity: verbosity || '0', - job_slice_count: job_slice_count || 1, - timeout: timeout || 0, - diff_mode: diff_mode || false, - job_tags: job_tags || '', - skip_tags: skip_tags || '', - become_enabled: become_enabled || false, - allow_callbacks: allow_callbacks || false, - allow_simultaneous: allow_simultaneous || false, - use_fact_cache: use_fact_cache || false, - host_config_key: host_config_key || '', + 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, + host_config_key: template.host_config_key || '', }; }, handleSubmit: (values, bag) => bag.props.handleSubmit(values), From 61f6e3c4d230f5c9ffb245b5c03c18abd3544934 Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Tue, 24 Sep 2019 08:22:11 -0700 Subject: [PATCH 02/11] Refactor to PlaybookSelect component --- .../Lookup}/ProjectLookup.jsx | 15 ++---- awx/ui_next/src/components/Lookup/index.js | 1 + .../Template/shared/JobTemplateForm.jsx | 54 +++---------------- .../Template/shared/PlaybookSelect.jsx | 51 ++++++++++++++++++ 4 files changed, 63 insertions(+), 58 deletions(-) rename awx/ui_next/src/{screens/Template/shared => components/Lookup}/ProjectLookup.jsx (78%) create mode 100644 awx/ui_next/src/screens/Template/shared/PlaybookSelect.jsx diff --git a/awx/ui_next/src/screens/Template/shared/ProjectLookup.jsx b/awx/ui_next/src/components/Lookup/ProjectLookup.jsx similarity index 78% rename from awx/ui_next/src/screens/Template/shared/ProjectLookup.jsx rename to awx/ui_next/src/components/Lookup/ProjectLookup.jsx index 0bcc42dd84..3493c2d531 100644 --- a/awx/ui_next/src/screens/Template/shared/ProjectLookup.jsx +++ b/awx/ui_next/src/components/Lookup/ProjectLookup.jsx @@ -2,16 +2,11 @@ import React from 'react'; import { string, func, bool } from 'prop-types'; import { withI18n } from '@lingui/react'; import { t } from '@lingui/macro'; -import { FormGroup, Tooltip } from '@patternfly/react-core'; -import { QuestionCircleIcon as PFQuestionCircleIcon } from '@patternfly/react-icons'; +import { FormGroup } from '@patternfly/react-core'; import { ProjectsAPI } from '@api'; import { Project } from '@types'; import Lookup from '@components/Lookup'; -import styled from 'styled-components'; - -const QuestionCircleIcon = styled(PFQuestionCircleIcon)` - margin-left: 10px; -`; +import { FieldTooltip } from '@components/FormField'; const loadProjects = async params => ProjectsAPI.read(params); @@ -36,11 +31,7 @@ class ProjectLookup extends React.Component { isValid={isValid} label={i18n._(t`Project`)} > - {tooltip && ( - - - - )} + {tooltip && } label { @@ -72,7 +76,6 @@ class JobTemplateForm extends Component { removedLabels: [], project: props.template.summary_fields.project, inventory: props.template.summary_fields.inventory, - relatedProjectPlaybooks: props.relatedProjectPlaybooks, relatedInstanceGroups: [], allowCallbacks: !!props.template.host_config_key, }; @@ -81,9 +84,6 @@ class JobTemplateForm extends Component { this.removeLabel = this.removeLabel.bind(this); this.handleProjectValidation = this.handleProjectValidation.bind(this); this.loadRelatedInstanceGroups = this.loadRelatedInstanceGroups.bind(this); - this.loadRelatedProjectPlaybooks = this.loadRelatedProjectPlaybooks.bind( - this - ); this.handleInstanceGroupsChange = this.handleInstanceGroupsChange.bind( this ); @@ -204,15 +204,6 @@ class JobTemplateForm extends Component { } } - async loadRelatedProjectPlaybooks(project) { - try { - const { data: playbooks = [] } = await ProjectsAPI.readPlaybooks(project); - this.setState({ relatedProjectPlaybooks: playbooks }); - } catch (contentError) { - this.setState({ contentError }); - } - } - handleProjectValidation() { const { i18n, touched } = this.props; const { project } = this.state; @@ -258,7 +249,6 @@ class JobTemplateForm extends Component { hasContentLoading, inventory, project, - relatedProjectPlaybooks = [], relatedInstanceGroups, allowCallbacks, } = this.state; @@ -284,28 +274,6 @@ class JobTemplateForm extends Component { isDisabled: false, }, ]; - const playbookOptions = relatedProjectPlaybooks - .map(playbook => { - return { - value: playbook, - key: playbook, - label: playbook, - isDisabled: false, - }; - }) - .reduce( - (arr, playbook) => { - return arr.concat(playbook); - }, - [ - { - value: '', - key: '', - label: i18n._(t`Choose a playbook`), - isDisabled: false, - }, - ] - ); const verbosityOptions = [ { value: '0', key: '0', label: i18n._(t`0 (Normal)`) }, @@ -412,7 +380,6 @@ class JobTemplateForm extends Component { tooltip={i18n._(t`Select the project containing the playbook you want this job to execute.`)} onChange={value => { - this.loadRelatedProjectPlaybooks(value.id); form.setFieldValue('project', value.id); this.setState({ project: value }); }} @@ -439,13 +406,8 @@ class JobTemplateForm extends Component { t`Select the playbook to be executed by this job.` )} /> - + ); }} diff --git a/awx/ui_next/src/screens/Template/shared/PlaybookSelect.jsx b/awx/ui_next/src/screens/Template/shared/PlaybookSelect.jsx new file mode 100644 index 0000000000..7b9c89f110 --- /dev/null +++ b/awx/ui_next/src/screens/Template/shared/PlaybookSelect.jsx @@ -0,0 +1,51 @@ +import React, { useState, useEffect } from 'react'; +import { number, string, oneOfType } from 'prop-types'; +import { withI18n } from '@lingui/react'; +import { t } from '@lingui/macro'; +import AnsibleSelect from '@components/AnsibleSelect'; +import { ProjectsAPI } from '@api'; + +function PlaybookSelect({ projectId, isValid, form, field, onError, i18n }) { + const [options, setOptions] = useState([]); + useEffect(() => { + (async () => { + try { + const { data } = await ProjectsAPI.readPlaybooks(projectId); + const opts = (data || []).map(playbook => ({ + value: playbook, + key: playbook, + label: playbook, + isDisabled: false, + })); + opts.unshift({ + value: '', + key: '', + label: i18n._(t`Choose a playbook`), + isDisabled: false, + }); + setOptions(opts); + } catch (contentError) { + onError(contentError); + } + })(); + }, [projectId]); + + return ( + + ); +} +PlaybookSelect.propTypes = { + projectId: oneOfType([number, string]), +}; +PlaybookSelect.defaultProps = { + projectId: null, +}; + +export { PlaybookSelect as _PlaybookSelect }; +export default withI18n()(PlaybookSelect); From 439727f1bd32fc2c000284e5bd3dde6c7ae7090f Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Wed, 25 Sep 2019 15:18:30 -0700 Subject: [PATCH 03/11] extract new LabelSelect component from JobTemplateForm --- .../Template/shared/JobTemplateForm.jsx | 177 +++++------------- .../screens/Template/shared/LabelSelect.jsx | 110 +++++++++++ .../Template/shared/PlaybookSelect.jsx | 3 + 3 files changed, 159 insertions(+), 131 deletions(-) create mode 100644 awx/ui_next/src/screens/Template/shared/LabelSelect.jsx diff --git a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx index a0de18a39f..9c1eaab9a2 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx @@ -15,7 +15,7 @@ import { import ContentError from '@components/ContentError'; import ContentLoading from '@components/ContentLoading'; import AnsibleSelect from '@components/AnsibleSelect'; -import MultiSelect, { TagMultiSelect } from '@components/MultiSelect'; +import { TagMultiSelect } from '@components/MultiSelect'; import FormActionGroup from '@components/FormActionGroup'; import FormField, { CheckboxField, FieldTooltip } from '@components/FormField'; import FormRow from '@components/FormRow'; @@ -28,7 +28,8 @@ import { InstanceGroupsLookup, ProjectLookup, } from '@components/Lookup'; -import { JobTemplatesAPI, LabelsAPI, ProjectsAPI } from '@api'; +import { JobTemplatesAPI } from '@api'; +import LabelSelect from './LabelSelect'; import PlaybookSelect from './PlaybookSelect'; const GridFormGroup = styled(FormGroup)` @@ -71,17 +72,11 @@ class JobTemplateForm extends Component { this.state = { hasContentLoading: true, contentError: false, - loadedLabels: [], - newLabels: [], - removedLabels: [], project: props.template.summary_fields.project, inventory: props.template.summary_fields.inventory, relatedInstanceGroups: [], allowCallbacks: !!props.template.host_config_key, }; - this.handleNewLabel = this.handleNewLabel.bind(this); - this.loadLabels = this.loadLabels.bind(this); - this.removeLabel = this.removeLabel.bind(this); this.handleProjectValidation = this.handleProjectValidation.bind(this); this.loadRelatedInstanceGroups = this.loadRelatedInstanceGroups.bind(this); this.handleInstanceGroupsChange = this.handleInstanceGroupsChange.bind( @@ -92,7 +87,8 @@ class JobTemplateForm extends Component { componentDidMount() { const { validateField } = this.props; this.setState({ contentError: null, hasContentLoading: true }); - Promise.all([this.loadLabels(), this.loadRelatedInstanceGroups()]).then( + // TODO: determine whene LabelSelect has finished loading labels? + Promise.all([this.loadRelatedInstanceGroups()]).then( () => { this.setState({ hasContentLoading: false }); validateField('project'); @@ -100,35 +96,6 @@ class JobTemplateForm extends Component { ); } - async loadLabels() { - // This function assumes that the user has no more than 400 - // labels. For the vast majority of users this will be more thans - // enough. This can be updated to allow more than 400 labels if we - // decide it is necessary. - let loadedLabels; - try { - const { data } = await LabelsAPI.read({ - page: 1, - page_size: 200, - order_by: 'name', - }); - loadedLabels = [...data.results]; - if (data.next && data.next.includes('page=2')) { - const { - data: { results }, - } = await LabelsAPI.read({ - page: 2, - page_size: 200, - order_by: 'name', - }); - loadedLabels = loadedLabels.concat(results); - } - this.setState({ loadedLabels }); - } catch (err) { - this.setState({ contentError: err }); - } - } - async loadRelatedInstanceGroups() { const { template } = this.props; if (!template.id) { @@ -145,65 +112,6 @@ class JobTemplateForm extends Component { } } - handleNewLabel(label) { - const { newLabels } = this.state; - const { template, setFieldValue } = this.props; - const isIncluded = newLabels.some(newLabel => newLabel.name === label.name); - if (isIncluded) { - const filteredLabels = newLabels.filter( - newLabel => newLabel.name !== label - ); - this.setState({ newLabels: filteredLabels }); - } else { - setFieldValue('newLabels', [ - ...newLabels, - { name: label.name, associate: true, id: label.id }, - ]); - this.setState({ - newLabels: [ - ...newLabels, - { - name: label.name, - associate: true, - id: label.id, - organization: template.summary_fields.inventory.organization_id, - }, - ], - }); - } - } - - removeLabel(label) { - const { removedLabels, newLabels } = this.state; - const { template, setFieldValue } = this.props; - - const isAssociatedLabel = template.summary_fields.labels.results.some( - tempLabel => tempLabel.id === label.id - ); - - if (isAssociatedLabel) { - setFieldValue( - 'removedLabels', - removedLabels.concat({ - disassociate: true, - id: label.id, - }) - ); - this.setState({ - removedLabels: removedLabels.concat({ - disassociate: true, - id: label.id, - }), - }); - } else { - const filteredLabels = newLabels.filter( - newLabel => newLabel.name !== label.name - ); - setFieldValue('newLabels', filteredLabels); - this.setState({ newLabels: filteredLabels }); - } - } - handleProjectValidation() { const { i18n, touched } = this.props; const { project } = this.state; @@ -244,7 +152,7 @@ class JobTemplateForm extends Component { render() { const { - loadedLabels, + // loadedLabels, contentError, hasContentLoading, inventory, @@ -256,6 +164,7 @@ class JobTemplateForm extends Component { handleCancel, handleSubmit, handleBlur, + setFieldValue, i18n, template, } = this.props; @@ -406,8 +315,13 @@ class JobTemplateForm extends Component { t`Select the playbook to be executed by this job.` )} /> - + this.setState({ contentError: err })} + /> ); }} @@ -416,15 +330,19 @@ class JobTemplateForm extends Component { - { + setFieldValue('newLabels', newLabels); + }} + onRemovedLabelsChange={removedLabels => { + setFieldValue('removedLabels', removedLabels); + }} + onError={err => this.setState({ contentError: err })} /> @@ -485,8 +403,8 @@ class JobTemplateForm extends Component { min="1" label={i18n._(t`Job Slicing`)} tooltip={i18n._(t`Divide the work done by this job template - into the specified number of job slices, each running the - same tasks against a portion of the inventory.`)} + into the specified number of job slices, each running the + same tasks against a portion of the inventory.`)} /> } @@ -615,19 +530,15 @@ class JobTemplateForm extends Component { id="option-concurrent" name="allow_simultaneous" label={i18n._(t`Concurrent Jobs`)} - tooltip={i18n._( - t`If enabled, simultaneous runs of this job template will - be allowed.` - )} + tooltip={i18n._(t`If enabled, simultaneous runs of this job + template will be allowed.`)} />
bag.props.handleSubmit(values), diff --git a/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx b/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx new file mode 100644 index 0000000000..4959a138e8 --- /dev/null +++ b/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx @@ -0,0 +1,110 @@ +import React, { useState, useEffect } from 'react'; +import { func, arrayOf, number, shape, string } from 'prop-types'; +import MultiSelect from '@components/MultiSelect'; +import { LabelsAPI } from '@api'; + +async function loadLabelOptions(setLabels, onError) { + let labels; + try { + const { data } = await LabelsAPI.read({ + page: 1, + page_size: 200, + order_by: 'name', + }); + labels = data.results; + setLabels(labels); + if (data.next && data.next.includes('page=2')) { + const { + data: { results }, + } = await LabelsAPI.read({ + page: 2, + page_size: 200, + order_by: 'name', + }); + labels = labels.concat(results); + } + setLabels(labels); + } catch (err) { + onError(err); + } +} + +function LabelSelect({ + initialValues, + organizationId, + onNewLabelsChange, + onRemovedLabelsChange, + onError, +}) { + const [options, setOptions] = useState([]); + // TODO: move newLabels into a prop? + const [newLabels, setNewLabels] = useState([]); + const [removedLabels, setRemovedLabels] = useState([]); + useEffect(() => { + loadLabelOptions(setOptions, onError); + }, []); + + const handleNewLabel = label => { + const isIncluded = newLabels.some(l => l.name === label.name); + if (isIncluded) { + const filteredLabels = newLabels.filter( + newLabel => newLabel.name !== label + ); + setNewLabels(filteredLabels); + } else { + const updatedNewLabels = newLabels.concat({ + name: label.name, + associate: true, + id: label.id, + // TODO: can this be null? what happens if inventory > org id changes? + // organization: organizationId, + }); + setNewLabels(updatedNewLabels); + onNewLabelsChange(updatedNewLabels); + } + }; + + const handleRemoveLabel = label => { + const isAssociatedLabel = initialValues.some( + l => l.id === label.id + ); + if (isAssociatedLabel) { + const updatedRemovedLabels = removedLabels.concat({ + id: label.id, + disassociate: true, + }); + setRemovedLabels(updatedRemovedLabels); + onRemovedLabelsChange(updatedRemovedLabels); + } else { + const filteredLabels = newLabels.filter(l => l.name !== label.name); + setNewLabels(filteredLabels); + onNewLabelsChange(filteredLabels); + } + }; + + return ( + + ); +} +LabelSelect.propTypes = { + initialValues: arrayOf( + shape({ + id: number.isRequired, + name: string.isRequired, + }) + ).isRequired, + organizationId: number, + onNewLabelsChange: func.isRequired, + onRemovedLabelsChange: func.isRequired, + onError: func.isRequired, +}; +LabelSelect.defaultProps = { + organizationId: null, +}; + +export default LabelSelect; diff --git a/awx/ui_next/src/screens/Template/shared/PlaybookSelect.jsx b/awx/ui_next/src/screens/Template/shared/PlaybookSelect.jsx index 7b9c89f110..3e7d03c37e 100644 --- a/awx/ui_next/src/screens/Template/shared/PlaybookSelect.jsx +++ b/awx/ui_next/src/screens/Template/shared/PlaybookSelect.jsx @@ -8,6 +8,9 @@ import { ProjectsAPI } from '@api'; function PlaybookSelect({ projectId, isValid, form, field, onError, i18n }) { const [options, setOptions] = useState([]); useEffect(() => { + if (!projectId) { + return; + } (async () => { try { const { data } = await ProjectsAPI.readPlaybooks(projectId); From 71511b66ac99f005a15d5de8ad459e14bccc81f9 Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Thu, 26 Sep 2019 10:01:34 -0700 Subject: [PATCH 04/11] fix JobTemplate tests --- .../JobTemplateAdd/JobTemplateAdd.test.jsx | 4 +-- .../Template/shared/JobTemplateForm.jsx | 1 - .../Template/shared/JobTemplateForm.test.jsx | 31 +++++------------- .../screens/Template/shared/LabelSelect.jsx | 5 --- .../Template/shared/LabelSelect.test.jsx | 9 ++++++ .../Template/shared/PlaybookSelect.test.jsx | 32 +++++++++++++++++++ 6 files changed, 50 insertions(+), 32 deletions(-) create mode 100644 awx/ui_next/src/screens/Template/shared/LabelSelect.test.jsx create mode 100644 awx/ui_next/src/screens/Template/shared/PlaybookSelect.test.jsx diff --git a/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.test.jsx b/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.test.jsx index f889f8fe97..039cb4cb8f 100644 --- a/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.test.jsx +++ b/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.test.jsx @@ -76,9 +76,7 @@ describe('', () => { ).toEqual(true); expect(wrapper.find('input#template-name').text()).toBe(defaultProps.name); - expect(wrapper.find('AnsibleSelect[name="playbook"]').text()).toBe( - 'Choose a playbook' - ); + expect(wrapper.find('PlaybookSelect')).toHaveLength(1); expect(wrapper.find('ProjectLookup').prop('value')).toBe(null); done(); }); diff --git a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx index 9c1eaab9a2..888835a611 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx @@ -152,7 +152,6 @@ class JobTemplateForm extends Component { render() { const { - // loadedLabels, contentError, hasContentLoading, inventory, 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 71a470112c..7973117a0c 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx @@ -2,7 +2,7 @@ import React from 'react'; import { mountWithContexts, waitForElement } from '@testUtils/enzymeHelpers'; import { sleep } from '@testUtils/testUtils'; import JobTemplateForm, { _JobTemplateForm } from './JobTemplateForm'; -import { LabelsAPI, JobTemplatesAPI } from '@api'; +import { LabelsAPI, JobTemplatesAPI, ProjectsAPI } from '@api'; jest.mock('@api'); @@ -61,6 +61,9 @@ describe('', () => { JobTemplatesAPI.readInstanceGroups.mockReturnValue({ data: { results: mockInstanceGroups }, }); + ProjectsAPI.readPlaybooks.mockReturnValue({ + data: ['debug.yml'], + }); }); afterEach(() => { @@ -156,27 +159,8 @@ describe('', () => { expect(handleCancel).toBeCalled(); }); - test('should call loadRelatedProjectPlaybooks when project value changes', async () => { - const loadRelatedProjectPlaybooks = jest.spyOn( - _JobTemplateForm.prototype, - 'loadRelatedProjectPlaybooks' - ); - const wrapper = mountWithContexts( - - ); - await waitForElement(wrapper, 'EmptyStateBody', el => el.length === 0); - wrapper.find('ProjectLookup').prop('onChange')({ - id: 10, - name: 'project', - }); - expect(loadRelatedProjectPlaybooks).toHaveBeenCalledWith(10); - }); - - test('handleNewLabel should arrange new labels properly', async () => { + // TODO Move this test to tests + test.skip('handleNewLabel should arrange new labels properly', async () => { const event = { key: 'Enter', preventDefault: () => {} }; const wrapper = mountWithContexts( ', () => { expect(newLabels[0].organization).toEqual(1); }); - test('disassociateLabel should arrange new labels properly', async () => { + // TODO Move this test to tests + test.skip('disassociateLabel should arrange new labels properly', async () => { const wrapper = mountWithContexts( ', () => { + test('should', () => { + wrapper = mount(); + }) +}); diff --git a/awx/ui_next/src/screens/Template/shared/PlaybookSelect.test.jsx b/awx/ui_next/src/screens/Template/shared/PlaybookSelect.test.jsx new file mode 100644 index 0000000000..a7cb28c491 --- /dev/null +++ b/awx/ui_next/src/screens/Template/shared/PlaybookSelect.test.jsx @@ -0,0 +1,32 @@ +import React from 'react'; +import { mountWithContexts } from '@testUtils/enzymeHelpers'; +import PlaybookSelect from './PlaybookSelect'; +import { ProjectsAPI } from '@api'; + +jest.mock('@api'); + +describe('', () => { + beforeEach(() => { + ProjectsAPI.readPlaybooks.mockReturnValue({ + data: ['debug.yml'], + }); + }); + + test('should reload playbooks when project value changes', () => { + const wrapper = mountWithContexts( + {}} + /> + ); + + expect(ProjectsAPI.readPlaybooks).toHaveBeenCalledWith(1); + wrapper.setProps({ projectId: 15 }); + + expect(ProjectsAPI.readPlaybooks).toHaveBeenCalledTimes(2); + expect(ProjectsAPI.readPlaybooks).toHaveBeenCalledWith(15); + }); +}); From 6e9804b713d67cc233bf0a6c37dbefe942a33884 Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Thu, 26 Sep 2019 13:27:54 -0700 Subject: [PATCH 05/11] Rework saving labels --- awx/ui_next/src/api/models/JobTemplates.js | 12 +++- .../components/MultiSelect/MultiSelect.jsx | 2 +- .../JobTemplateAdd/JobTemplateAdd.jsx | 31 +++++------ .../JobTemplateEdit/JobTemplateEdit.jsx | 27 +++++---- .../Template/shared/JobTemplateForm.jsx | 30 +++++----- .../Template/shared/JobTemplateForm.test.jsx | 2 +- .../screens/Template/shared/LabelSelect.jsx | 55 +++---------------- awx/ui_next/src/util/lists.js | 18 ++++++ awx/ui_next/src/util/lists.test.js | 51 +++++++++++++++++ 9 files changed, 131 insertions(+), 97 deletions(-) create mode 100644 awx/ui_next/src/util/lists.js create mode 100644 awx/ui_next/src/util/lists.test.js diff --git a/awx/ui_next/src/api/models/JobTemplates.js b/awx/ui_next/src/api/models/JobTemplates.js index 646f168df8..a6af575b44 100644 --- a/awx/ui_next/src/api/models/JobTemplates.js +++ b/awx/ui_next/src/api/models/JobTemplates.js @@ -27,11 +27,17 @@ class JobTemplates extends InstanceGroupsMixin(Base) { } disassociateLabel(id, label) { - return this.http.post(`${this.baseUrl}${id}/labels/`, label); + return this.http.post(`${this.baseUrl}${id}/labels/`, { + id: label.id, + disassociate: true, + }); } - generateLabel(orgId, label) { - return this.http.post(`${this.baseUrl}${orgId}/labels/`, label); + generateLabel(id, label, orgId) { + return this.http.post(`${this.baseUrl}${id}/labels/`, { + name: label.name, + organization: orgId, + }); } readCredentials(id, params) { diff --git a/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx b/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx index 930664334d..8f6752111f 100644 --- a/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx +++ b/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx @@ -144,8 +144,8 @@ class MultiSelect extends Component { const isNewItem = !match || !chipItems.find(item => item.id === match.id); if (event.key === 'Enter' && isNewItem) { event.preventDefault(); - const items = chipItems.concat({ name: input, id: input }); const newItem = match || this.createNewItem(input); + const items = chipItems.concat(newItem); this.setState({ chipItems: items, isExpanded: false, diff --git a/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.jsx b/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.jsx index 85bec5dfff..e0aab2f4e5 100644 --- a/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.jsx +++ b/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.jsx @@ -18,8 +18,8 @@ function JobTemplateAdd({ history, i18n }) { async function handleSubmit(values) { const { - newLabels, - removedLabels, + labels, + organizationId, addedInstanceGroups, removedInstanceGroups, ...remainingValues @@ -31,7 +31,7 @@ function JobTemplateAdd({ history, i18n }) { data: { id, type }, } = await JobTemplatesAPI.create(remainingValues); await Promise.all([ - submitLabels(id, newLabels, removedLabels), + submitLabels(id, labels, organizationId), submitInstanceGroups(id, addedInstanceGroups, removedInstanceGroups), ]); history.push(`/templates/${type}/${id}/details`); @@ -40,22 +40,17 @@ function JobTemplateAdd({ history, i18n }) { } } - function submitLabels(id, newLabels = [], removedLabels = []) { - const disassociationPromises = removedLabels.map(label => - JobTemplatesAPI.disassociateLabel(id, label) - ); - const associationPromises = newLabels - .filter(label => !label.organization) - .map(label => JobTemplatesAPI.associateLabel(id, label)); - const creationPromises = newLabels - .filter(label => label.organization) - .map(label => JobTemplatesAPI.generateLabel(id, label)); + function submitLabels(templateId, labels = [], organizationId) { + const associationPromises = labels + .filter(label => !label.isNew) + .map(label => JobTemplatesAPI.associateLabel(templateId, label)); + const creationPromises = labels + .filter(label => label.isNew) + .map(label => + JobTemplatesAPI.generateLabel(templateId, label, organizationId) + ); - return Promise.all([ - ...disassociationPromises, - ...associationPromises, - ...creationPromises, - ]); + return Promise.all([...associationPromises, ...creationPromises]); } function submitInstanceGroups(templateId, addedGroups = []) { diff --git a/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx b/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx index 8771b7c3bf..0aa128fb55 100644 --- a/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx +++ b/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx @@ -6,6 +6,7 @@ import ContentError from '@components/ContentError'; import ContentLoading from '@components/ContentLoading'; import { JobTemplatesAPI, ProjectsAPI } from '@api'; import { JobTemplate } from '@types'; +import { getAddedAndRemoved } from '@util/lists'; import JobTemplateForm from '../shared/JobTemplateForm'; class JobTemplateEdit extends Component { @@ -104,8 +105,8 @@ class JobTemplateEdit extends Component { async handleSubmit(values) { const { template, history } = this.props; const { - newLabels, - removedLabels, + labels, + organizationId, addedInstanceGroups, removedInstanceGroups, ...remainingValues @@ -115,7 +116,7 @@ class JobTemplateEdit extends Component { try { await JobTemplatesAPI.update(template.id, remainingValues); await Promise.all([ - this.submitLabels(newLabels, removedLabels), + this.submitLabels(labels, organizationId), this.submitInstanceGroups(addedInstanceGroups, removedInstanceGroups), ]); history.push(this.detailsUrl); @@ -124,17 +125,23 @@ class JobTemplateEdit extends Component { } } - async submitLabels(newLabels = [], removedLabels = []) { + async submitLabels(labels = [], organizationId) { const { template } = this.props; - const disassociationPromises = removedLabels.map(label => + const { added, removed } = getAddedAndRemoved( + template.summary_fields.labels.results, + labels + ); + const disassociationPromises = removed.map(label => JobTemplatesAPI.disassociateLabel(template.id, label) ); - const associationPromises = newLabels - .filter(label => !label.organization) + const associationPromises = added + .filter(label => !label.isNew) .map(label => JobTemplatesAPI.associateLabel(template.id, label)); - const creationPromises = newLabels - .filter(label => label.organization) - .map(label => JobTemplatesAPI.generateLabel(template.id, label)); + const creationPromises = added + .filter(label => label.isNew) + .map(label => + JobTemplatesAPI.generateLabel(template.id, label, organizationId) + ); const results = await Promise.all([ ...disassociationPromises, diff --git a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx index 888835a611..9a4e3d9230 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx @@ -87,13 +87,11 @@ class JobTemplateForm extends Component { componentDidMount() { const { validateField } = this.props; this.setState({ contentError: null, hasContentLoading: true }); - // TODO: determine whene LabelSelect has finished loading labels? - Promise.all([this.loadRelatedInstanceGroups()]).then( - () => { - this.setState({ hasContentLoading: false }); - validateField('project'); - } - ); + // TODO: determine whene LabelSelect has finished loading labels + Promise.all([this.loadRelatedInstanceGroups()]).then(() => { + this.setState({ hasContentLoading: false }); + validateField('project'); + }); } async loadRelatedInstanceGroups() { @@ -270,6 +268,7 @@ class JobTemplateForm extends Component { you want this job to manage.`)} onChange={value => { form.setFieldValue('inventory', value.id); + form.setFieldValue('organizationId', value.organization); this.setState({ inventory: value }); }} required @@ -335,12 +334,7 @@ class JobTemplateForm extends Component { /> { - setFieldValue('newLabels', newLabels); - }} - onRemovedLabelsChange={removedLabels => { - setFieldValue('removedLabels', removedLabels); - }} + onChange={labels => setFieldValue('labels', labels)} onError={err => this.setState({ contentError: err })} /> @@ -577,7 +571,10 @@ class JobTemplateForm extends Component { const FormikApp = withFormik({ mapPropsToValues(props) { const { template = {} } = props; - const { summary_fields = { labels: { results: [] } } } = template; + const { summary_fields = { + labels: { results: [] }, + inventory: { organization: null }, + } } = template; return { name: template.name || '', @@ -600,13 +597,12 @@ const FormikApp = withFormik({ allow_simultaneous: template.allow_simultaneous || false, use_fact_cache: template.use_fact_cache || false, host_config_key: template.host_config_key || '', + organizationId: summary_fields.inventory.organization_id || null, addedInstanceGroups: [], removedInstanceGroups: [], - newLabels: [], - removedLabels: [], }; }, - handleSubmit: (values, bag) => bag.props.handleSubmit(values), + handleSubmit: (values, { props }) => props.handleSubmit(values), })(JobTemplateForm); export { JobTemplateForm as _JobTemplateForm }; 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 7973117a0c..6825982d01 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx @@ -1,7 +1,7 @@ import React from 'react'; import { mountWithContexts, waitForElement } from '@testUtils/enzymeHelpers'; import { sleep } from '@testUtils/testUtils'; -import JobTemplateForm, { _JobTemplateForm } from './JobTemplateForm'; +import JobTemplateForm from './JobTemplateForm'; import { LabelsAPI, JobTemplatesAPI, ProjectsAPI } from '@api'; jest.mock('@api'); diff --git a/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx b/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx index 9b66634ab1..b050a4746c 100644 --- a/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx +++ b/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx @@ -30,63 +30,26 @@ async function loadLabelOptions(setLabels, onError) { } function LabelSelect({ - initialValues, - onNewLabelsChange, - onRemovedLabelsChange, + initialValues, // todo: change to value, controlled ? + onChange, onError, }) { const [options, setOptions] = useState([]); // TODO: move newLabels into a prop? - const [newLabels, setNewLabels] = useState([]); - const [removedLabels, setRemovedLabels] = useState([]); useEffect(() => { loadLabelOptions(setOptions, onError); }, []); - const handleNewLabel = label => { - const isIncluded = newLabels.some(l => l.name === label.name); - if (isIncluded) { - const filteredLabels = newLabels.filter( - newLabel => newLabel.name !== label - ); - setNewLabels(filteredLabels); - } else { - const updatedNewLabels = newLabels.concat({ - name: label.name, - associate: true, - id: label.id, - // TODO: can this be null? what happens if inventory > org id changes? - // organization: organizationId, - }); - setNewLabels(updatedNewLabels); - onNewLabelsChange(updatedNewLabels); - } - }; - - const handleRemoveLabel = label => { - const isAssociatedLabel = initialValues.some( - l => l.id === label.id - ); - if (isAssociatedLabel) { - const updatedRemovedLabels = removedLabels.concat({ - id: label.id, - disassociate: true, - }); - setRemovedLabels(updatedRemovedLabels); - onRemovedLabelsChange(updatedRemovedLabels); - } else { - const filteredLabels = newLabels.filter(l => l.name !== label.name); - setNewLabels(filteredLabels); - onNewLabelsChange(filteredLabels); - } - }; - return ( ({ + id: name, + name, + isNew: true, + })} /> ); } @@ -97,8 +60,6 @@ LabelSelect.propTypes = { name: string.isRequired, }) ).isRequired, - onNewLabelsChange: func.isRequired, - onRemovedLabelsChange: func.isRequired, onError: func.isRequired, }; diff --git a/awx/ui_next/src/util/lists.js b/awx/ui_next/src/util/lists.js new file mode 100644 index 0000000000..ad03cccde2 --- /dev/null +++ b/awx/ui_next/src/util/lists.js @@ -0,0 +1,18 @@ +/* eslint-disable import/prefer-default-export */ +export function getAddedAndRemoved (original, current) { + original = original || []; + current = current || []; + const added = []; + const removed = []; + original.forEach(orig => { + if (!current.find(cur => cur.id === orig.id)) { + removed.push(orig); + } + }); + current.forEach(cur => { + if (!original.find(orig => orig.id === cur.id)) { + added.push(cur); + } + }); + return { added, removed }; +} diff --git a/awx/ui_next/src/util/lists.test.js b/awx/ui_next/src/util/lists.test.js new file mode 100644 index 0000000000..b15fe009df --- /dev/null +++ b/awx/ui_next/src/util/lists.test.js @@ -0,0 +1,51 @@ +import {getAddedAndRemoved} from './lists'; + +const one = { id: 1 }; +const two = { id: 2 }; +const three = { id: 3 }; + +describe('getAddedAndRemoved', () => { + test('should handle no original list', () => { + const items = [one, two, three]; + expect(getAddedAndRemoved(null, items)).toEqual({ + added: items, + removed: [], + }); + }); + + test('should list added item', () => { + const original = [one, two]; + const current = [one, two, three]; + expect(getAddedAndRemoved(original, current)).toEqual({ + added: [three], + removed: [], + }); + }); + + test('should list removed item', () => { + const original = [one, two, three]; + const current = [one, three]; + expect(getAddedAndRemoved(original, current)).toEqual({ + added: [], + removed: [two], + }); + }); + + test('should handle both added and removed together', () => { + const original = [two]; + const current = [one, three]; + expect(getAddedAndRemoved(original, current)).toEqual({ + added: [one, three], + removed: [two], + }); + }); + + test('should handle different list order', () => { + const original = [three, two]; + const current = [one, two, three]; + expect(getAddedAndRemoved(original, current)).toEqual({ + added: [one], + removed: [], + }); + }); +}); From da149d931cace2dacbbe09f16eead96fa509c6d2 Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Fri, 27 Sep 2019 15:04:09 -0700 Subject: [PATCH 06/11] rework MultiSelect into controlled input; refactoring --- awx/ui_next/src/components/Chip/Chip.jsx | 1 + .../components/MultiSelect/MultiSelect.jsx | 156 ++++++++---------- .../MultiSelect/MultiSelect.test.jsx | 99 ++++++----- .../components/MultiSelect/TagMultiSelect.jsx | 2 +- .../Template/shared/JobTemplateForm.jsx | 29 ++-- .../Template/shared/JobTemplateForm.test.jsx | 53 ------ .../screens/Template/shared/LabelSelect.jsx | 11 +- 7 files changed, 139 insertions(+), 212 deletions(-) diff --git a/awx/ui_next/src/components/Chip/Chip.jsx b/awx/ui_next/src/components/Chip/Chip.jsx index f8837812f0..8c86115919 100644 --- a/awx/ui_next/src/components/Chip/Chip.jsx +++ b/awx/ui_next/src/components/Chip/Chip.jsx @@ -1,6 +1,7 @@ import { Chip } from '@patternfly/react-core'; import styled from 'styled-components'; +Chip.displayName = 'PFChip'; export default styled(Chip)` --pf-c-chip--m-read-only--PaddingTop: 3px; --pf-c-chip--m-read-only--PaddingRight: 8px; diff --git a/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx b/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx index 8f6752111f..b4676dc513 100644 --- a/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx +++ b/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx @@ -45,7 +45,7 @@ const Item = shape({ class MultiSelect extends Component { static propTypes = { - associatedItems: arrayOf(Item).isRequired, + value: arrayOf(Item).isRequired, options: arrayOf(Item), onAddNewItem: func, onRemoveItem: func, @@ -65,13 +65,11 @@ class MultiSelect extends Component { super(props); this.state = { input: '', - chipItems: this.getInitialChipItems(), isExpanded: false, }; - this.handleAddItem = this.handleAddItem.bind(this); + this.handleKeyDown = this.handleKeyDown.bind(this); this.handleInputChange = this.handleInputChange.bind(this); - this.handleSelection = this.handleSelection.bind(this); - this.removeChip = this.removeChip.bind(this); + this.removeItem = this.removeItem.bind(this); this.handleClick = this.handleClick.bind(this); this.createNewItem = this.createNewItem.bind(this); } @@ -84,33 +82,57 @@ class MultiSelect extends Component { document.removeEventListener('mousedown', this.handleClick, false); } - getInitialChipItems() { - const { associatedItems } = this.props; - return associatedItems.map(item => ({ ...item })); - } - handleClick(e, option) { if (this.node && this.node.contains(e.target)) { if (option) { - this.handleSelection(e, option); + e.preventDefault(); + this.addItem(option); } } else { this.setState({ input: '', isExpanded: false }); } } - handleSelection(e, item) { - const { chipItems } = this.state; - const { onAddNewItem, onChange } = this.props; - e.preventDefault(); - - const items = chipItems.concat({ name: item.name, id: item.id }); - this.setState({ - chipItems: items, - isExpanded: false, - }); + addItem(item) { + const { value, onAddNewItem, onChange } = this.props; + const items = value.concat(item); onAddNewItem(item); onChange(items); + this.close(); + } + + // TODO: UpArrow & DownArrow for menu navigation + handleKeyDown(event) { + const { value, options } = this.props; + const { input } = this.state; + if (event.key === 'Tab') { + this.close(); + return; + } + if (!input || event.key !== 'Enter') { + return; + } + + const isAlreadySelected = value.some(i => i.name === input); + if (isAlreadySelected) { + event.preventDefault(); + this.close(); + return; + } + + const match = options.find(item => item.name === input); + const isNewItem = !match || !value.find(item => item.id === match.id); + if (isNewItem) { + event.preventDefault(); + this.addItem(match || this.createNewItem(input)); + } + } + + close() { + this.setState({ + isExpanded: false, + input: '', + }); } createNewItem(name) { @@ -124,66 +146,28 @@ class MultiSelect extends Component { }; } - handleAddItem(event) { - const { input, chipItems } = this.state; - const { options, onAddNewItem, onChange } = this.props; - const match = options.find(item => item.name === input); - const isIncluded = chipItems.some(chipItem => chipItem.name === input); - - if (!input) { - return; - } - - if (isIncluded) { - // This event.preventDefault prevents the form from submitting - // if the user tries to create 2 chips of the same name - event.preventDefault(); - this.setState({ input: '', isExpanded: false }); - return; - } - const isNewItem = !match || !chipItems.find(item => item.id === match.id); - if (event.key === 'Enter' && isNewItem) { - event.preventDefault(); - const newItem = match || this.createNewItem(input); - const items = chipItems.concat(newItem); - this.setState({ - chipItems: items, - isExpanded: false, - input: '', - }); - onAddNewItem(newItem); - onChange(items); - } else if (!isNewItem || event.key === 'Tab') { - this.setState({ isExpanded: false, input: '' }); - } - } - handleInputChange(value) { this.setState({ input: value, isExpanded: true }); } - removeChip(e, item) { - const { onRemoveItem, onChange } = this.props; - const { chipItems } = this.state; - const chips = chipItems.filter(chip => chip.id !== item.id); + removeItem(item) { + const { value, onRemoveItem, onChange } = this.props; + const remainingItems = value.filter(chip => chip.id !== item.id); - this.setState({ chipItems: chips }); onRemoveItem(item); - onChange(chips); - - e.preventDefault(); + onChange(remainingItems); } render() { - const { options } = this.props; - const { chipItems, input, isExpanded } = this.state; + const { value, options } = this.props; + const { input, isExpanded } = this.state; - const list = options.map(option => ( + const dropdownOptions = options.map(option => ( {option.name.includes(input) ? ( item.id === option.id)} + isDisabled={value.some(item => item.id === option.id)} value={option.name} onClick={e => { this.handleClick(e, option); @@ -195,21 +179,6 @@ class MultiSelect extends Component { )); - const chips = ( - - {chipItems && - chipItems.map(item => ( - { - this.removeChip(e, item); - }} - > - {item.name} - - ))} - - ); return ( @@ -222,21 +191,32 @@ class MultiSelect extends Component { type="text" aria-label="labels" value={input} - onClick={() => this.setState({ isExpanded: true })} + onFocus={() => this.setState({ isExpanded: true })} onChange={this.handleInputChange} - onKeyDown={this.handleAddItem} + onKeyDown={this.handleKeyDown} /> Labels} - // Above is not rendered but is a required prop from Patternfly + // Above is not visible but is a required prop from Patternfly isOpen={isExpanded} - dropdownItems={list} + dropdownItems={dropdownOptions} />
-
{chips}
+
+ + {value.map(item => ( + { this.removeItem(item); }} + > + {item.name} + + ))} + +
); diff --git a/awx/ui_next/src/components/MultiSelect/MultiSelect.test.jsx b/awx/ui_next/src/components/MultiSelect/MultiSelect.test.jsx index e52e56b3c1..77c2beae1b 100644 --- a/awx/ui_next/src/components/MultiSelect/MultiSelect.test.jsx +++ b/awx/ui_next/src/components/MultiSelect/MultiSelect.test.jsx @@ -1,48 +1,51 @@ import React from 'react'; -import { mount } from 'enzyme'; -import { sleep } from '@testUtils/testUtils'; +import { mount, shallow } from 'enzyme'; import MultiSelect from './MultiSelect'; describe('', () => { - const associatedItems = [ + const value = [ { name: 'Foo', id: 1, organization: 1 }, { name: 'Bar', id: 2, organization: 1 }, ]; const options = [{ name: 'Angry', id: 3 }, { name: 'Potato', id: 4 }]; - test('Initially render successfully', () => { - const wrapper = mount( + test('should render successfully', () => { + const wrapper = shallow( + ); + + expect(wrapper.find('Chip')).toHaveLength(2); + }); + + test('should add item when typed', async () => { + const onChange = jest.fn(); + const onAdd = jest.fn(); + const wrapper = mount( + ); const component = wrapper.find('MultiSelect'); + const input = component.find('TextInput'); + input.invoke('onChange')('Flabadoo'); + input.simulate('keydown', { key: 'Enter' }); - expect(component.state().chipItems.length).toBe(2); + expect(onAdd.mock.calls[0][0].name).toEqual('Flabadoo'); + const newVal = onChange.mock.calls[0][0]; + expect(newVal).toHaveLength(3); + expect(newVal[2].name).toEqual('Flabadoo'); }); - test('handleSelection add item to chipItems', async () => { - const wrapper = mount( - - ); - const component = wrapper.find('MultiSelect'); - component - .find('input[aria-label="labels"]') - .simulate('keydown', { key: 'Enter' }); - component.update(); - await sleep(1); - expect(component.state().chipItems.length).toBe(2); - }); - - test('handleAddItem adds a chip only when Tab is pressed', () => { + test('should add item when clicked from menu', () => { const onAddNewItem = jest.fn(); const onChange = jest.fn(); const wrapper = mount( @@ -50,48 +53,44 @@ describe('', () => { onAddNewItem={onAddNewItem} onRemoveItem={jest.fn()} onChange={onChange} - associatedItems={associatedItems} + value={value} options={options} /> ); + + const input = wrapper.find('TextInput'); + input.simulate('focus'); + wrapper.update(); const event = { preventDefault: () => {}, - key: 'Enter', + target: wrapper.find('DropdownItem').at(1).getDOMNode(), }; - const component = wrapper.find('MultiSelect'); + wrapper.find('DropdownItem').at(1).invoke('onClick')(event); - component.setState({ input: 'newLabel' }); - component.update(); - component.instance().handleAddItem(event); - expect(component.state().chipItems.length).toBe(3); - expect(component.state().input.length).toBe(0); - expect(component.state().isExpanded).toBe(false); - expect(onAddNewItem).toBeCalled(); - expect(onChange).toBeCalled(); + expect(onAddNewItem).toHaveBeenCalledWith(options[1]); + const newVal = onChange.mock.calls[0][0]; + expect(newVal).toHaveLength(3); + expect(newVal[2]).toEqual(options[1]); }); - test('removeChip removes chip properly', () => { + test('should remove item', () => { const onRemoveItem = jest.fn(); const onChange = jest.fn(); - const wrapper = mount( ); - const event = { - preventDefault: () => {}, - }; - const component = wrapper.find('MultiSelect'); - component - .instance() - .removeChip(event, { name: 'Foo', id: 1, organization: 1 }); - expect(component.state().chipItems.length).toBe(1); - expect(onRemoveItem).toBeCalled(); - expect(onChange).toBeCalled(); + + wrapper.find('Chip').at(1).invoke('onClick')(); + + expect(onRemoveItem).toHaveBeenCalledWith(value[1]); + const newVal = onChange.mock.calls[0][0]; + expect(newVal).toHaveLength(1); + expect(newVal).toEqual([value[0]]); }); }); diff --git a/awx/ui_next/src/components/MultiSelect/TagMultiSelect.jsx b/awx/ui_next/src/components/MultiSelect/TagMultiSelect.jsx index 472327aaad..a398bc0311 100644 --- a/awx/ui_next/src/components/MultiSelect/TagMultiSelect.jsx +++ b/awx/ui_next/src/components/MultiSelect/TagMultiSelect.jsx @@ -33,7 +33,7 @@ function TagMultiSelect({ onChange, value }) { setOptions(options.concat(newItem)); } }} - associatedItems={stringToArray(value)} + value={stringToArray(value)} options={options} createNewItem={name => ({ id: name, name })} /> diff --git a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx index 9a4e3d9230..d7eb506975 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx @@ -326,18 +326,23 @@ class JobTemplateForm extends Component { /> - - - setFieldValue('labels', labels)} - onError={err => this.setState({ contentError: err })} - /> - + ( + + + setFieldValue('labels', labels)} + onError={err => this.setState({ contentError: err })} + /> + + )} + /> 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 6825982d01..0d53e4b405 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx @@ -158,57 +158,4 @@ describe('', () => { wrapper.find('button[aria-label="Cancel"]').prop('onClick')(); expect(handleCancel).toBeCalled(); }); - - // TODO Move this test to tests - test.skip('handleNewLabel should arrange new labels properly', async () => { - const event = { key: 'Enter', preventDefault: () => {} }; - const wrapper = mountWithContexts( - - ); - await waitForElement(wrapper, 'EmptyStateBody', el => el.length === 0); - const multiSelect = wrapper.find( - 'FormGroup[fieldId="template-labels"] MultiSelect' - ); - const component = wrapper.find('JobTemplateForm'); - - wrapper.setState({ newLabels: [], loadedLabels: [], removedLabels: [] }); - multiSelect.setState({ input: 'Foo' }); - component - .find('FormGroup[fieldId="template-labels"] input[aria-label="labels"]') - .prop('onKeyDown')(event); - - component.instance().handleNewLabel({ name: 'Bar', id: 2 }); - const newLabels = component.state('newLabels'); - expect(newLabels).toHaveLength(2); - expect(newLabels[0].name).toEqual('Foo'); - expect(newLabels[0].organization).toEqual(1); - }); - - // TODO Move this test to tests - test.skip('disassociateLabel should arrange new labels properly', async () => { - const wrapper = mountWithContexts( - - ); - await waitForElement(wrapper, 'EmptyStateBody', el => el.length === 0); - const component = wrapper.find('JobTemplateForm'); - // This asserts that the user generated a label or clicked - // on a label option, and then changed their mind and - // removed the label. - component.instance().removeLabel({ name: 'Alex', id: 17 }); - expect(component.state().newLabels.length).toBe(0); - expect(component.state().removedLabels.length).toBe(0); - // This asserts that the user removed a label that was associated - // with the template when the template loaded. - component.instance().removeLabel({ name: 'Sushi', id: 1 }); - expect(component.state().newLabels.length).toBe(0); - expect(component.state().removedLabels.length).toBe(1); - }); }); diff --git a/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx b/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx index b050a4746c..73f9b5278c 100644 --- a/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx +++ b/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx @@ -29,13 +29,8 @@ async function loadLabelOptions(setLabels, onError) { } } -function LabelSelect({ - initialValues, // todo: change to value, controlled ? - onChange, - onError, -}) { +function LabelSelect({ value, onChange, onError }) { const [options, setOptions] = useState([]); - // TODO: move newLabels into a prop? useEffect(() => { loadLabelOptions(setOptions, onError); }, []); @@ -43,7 +38,7 @@ function LabelSelect({ return ( ({ id: name, @@ -54,7 +49,7 @@ function LabelSelect({ ); } LabelSelect.propTypes = { - initialValues: arrayOf( + value: arrayOf( shape({ id: number.isRequired, name: string.isRequired, From 554a63d8fc2ee8b3bf325c22d5295bf0ac25673c Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Fri, 27 Sep 2019 15:30:42 -0700 Subject: [PATCH 07/11] write LabelSelect tests --- .../Template/shared/LabelSelect.test.jsx | 50 +++++++++++++++++-- .../Template/shared/PlaybookSelect.test.jsx | 4 ++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/awx/ui_next/src/screens/Template/shared/LabelSelect.test.jsx b/awx/ui_next/src/screens/Template/shared/LabelSelect.test.jsx index ebbaba2b4c..07bf0c0a62 100644 --- a/awx/ui_next/src/screens/Template/shared/LabelSelect.test.jsx +++ b/awx/ui_next/src/screens/Template/shared/LabelSelect.test.jsx @@ -1,9 +1,53 @@ import React from 'react'; import { mount } from 'enzyme'; +import { LabelsAPI } from '@api'; +import { sleep} from '@testUtils/testUtils'; import LabelSelect from './LabelSelect'; +jest.mock('@api'); + +const options = [ + { id: 1, name: 'one' }, + { id: 2, name: 'two' }, +]; + describe('', () => { - test('should', () => { - wrapper = mount(); - }) + afterEach(() => { + jest.resetAllMocks(); + }); + + test('should fetch labels', async () => { + LabelsAPI.read.mockReturnValue({ + data: { results: options }, + }); + const wrapper = mount(); + await sleep(1); + wrapper.update(); + + expect(LabelsAPI.read).toHaveBeenCalledTimes(1); + expect(wrapper.find('MultiSelect').prop('options')).toEqual(options); + }); + + test('should fetch two pages labels if present', async () => { + LabelsAPI.read.mockReturnValueOnce({ + data: { + results: options, + next: '/foo?page=2', + }, + }); + LabelsAPI.read.mockReturnValueOnce({ + data: { + results: options, + }, + }); + const wrapper = mount(); + await sleep(1); + wrapper.update(); + + expect(LabelsAPI.read).toHaveBeenCalledTimes(2); + expect(wrapper.find('MultiSelect').prop('options')).toEqual([ + ...options, + ...options + ]); + }); }); diff --git a/awx/ui_next/src/screens/Template/shared/PlaybookSelect.test.jsx b/awx/ui_next/src/screens/Template/shared/PlaybookSelect.test.jsx index a7cb28c491..0a2d979fff 100644 --- a/awx/ui_next/src/screens/Template/shared/PlaybookSelect.test.jsx +++ b/awx/ui_next/src/screens/Template/shared/PlaybookSelect.test.jsx @@ -12,6 +12,10 @@ describe('', () => { }); }); + afterEach(() => { + jest.resetAllMocks(); + }) + test('should reload playbooks when project value changes', () => { const wrapper = mountWithContexts( Date: Tue, 1 Oct 2019 11:03:36 -0700 Subject: [PATCH 08/11] update JT form tests --- .../OrganizationAccessItem.test.jsx.snap | 4 +- .../OrganizationNotifications.test.jsx.snap | 44 ------------------- .../JobTemplateAdd/JobTemplateAdd.test.jsx | 7 ++- .../JobTemplateEdit/JobTemplateEdit.test.jsx | 22 +++++----- .../Template/shared/JobTemplateForm.jsx | 2 +- .../Template/shared/JobTemplateForm.test.jsx | 12 ++--- 6 files changed, 25 insertions(+), 66 deletions(-) diff --git a/awx/ui_next/src/screens/Organization/OrganizationAccess/__snapshots__/OrganizationAccessItem.test.jsx.snap b/awx/ui_next/src/screens/Organization/OrganizationAccess/__snapshots__/OrganizationAccessItem.test.jsx.snap index 8e8c16a6dd..754d0fe75c 100644 --- a/awx/ui_next/src/screens/Organization/OrganizationAccess/__snapshots__/OrganizationAccessItem.test.jsx.snap +++ b/awx/ui_next/src/screens/Organization/OrganizationAccess/__snapshots__/OrganizationAccessItem.test.jsx.snap @@ -693,7 +693,7 @@ exports[` initially renders succesfully 1`] = ` isReadOnly={false} onClick={[Function]} > - initially renders succesfully 1`] = ` - + diff --git a/awx/ui_next/src/screens/Organization/OrganizationNotifications/__snapshots__/OrganizationNotifications.test.jsx.snap b/awx/ui_next/src/screens/Organization/OrganizationNotifications/__snapshots__/OrganizationNotifications.test.jsx.snap index e59d0e142a..080d13087e 100644 --- a/awx/ui_next/src/screens/Organization/OrganizationNotifications/__snapshots__/OrganizationNotifications.test.jsx.snap +++ b/awx/ui_next/src/screens/Organization/OrganizationNotifications/__snapshots__/OrganizationNotifications.test.jsx.snap @@ -385,10 +385,6 @@ exports[` initially renders succesfully 1`] = ` onSort={[Function]} qsConfig={ Object { - "dateFields": Array [ - "modified", - "created", - ], "defaultParams": Object { "order_by": "name", "page": 1, @@ -443,10 +439,6 @@ exports[` initially renders succesfully 1`] = ` onSort={[Function]} qsConfig={ Object { - "dateFields": Array [ - "modified", - "created", - ], "defaultParams": Object { "order_by": "name", "page": 1, @@ -626,24 +618,6 @@ exports[` initially renders succesfully 1`] = ` ] } onSearch={[Function]} - qsConfig={ - Object { - "dateFields": Array [ - "modified", - "created", - ], - "defaultParams": Object { - "order_by": "name", - "page": 1, - "page_size": 5, - }, - "integerFields": Array [ - "page", - "page_size", - ], - "namespace": "notification", - } - } sortedColumnKey="name" > initially renders succesfully 1`] = ` } i18n={"/i18n/"} onSearch={[Function]} - qsConfig={ - Object { - "dateFields": Array [ - "modified", - "created", - ], - "defaultParams": Object { - "order_by": "name", - "page": 1, - "page_size": 5, - }, - "integerFields": Array [ - "page", - "page_size", - ], - "namespace": "notification", - } - } sortedColumnKey="name" > ', () => { const defaultProps = { description: '', @@ -94,7 +96,10 @@ describe('', () => { const changeState = new Promise(resolve => { formik.setState( { - values: jobTemplateData, + values: { + ...jobTemplateData, + labels: [], + } }, () => resolve() ); diff --git a/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.test.jsx b/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.test.jsx index 3f43faf279..cd7ef7a141 100644 --- a/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.test.jsx +++ b/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.test.jsx @@ -34,6 +34,9 @@ const mockJobTemplate = { labels: { results: [{ name: 'Sushi', id: 1 }, { name: 'Major', id: 2 }], }, + inventory: { + organization_id: 1, + }, }, }; @@ -170,16 +173,12 @@ describe('', () => { description: 'new description', job_type: 'check', }; - const newLabels = [ - { associate: true, id: 3 }, - { associate: true, id: 3 }, - { name: 'Maple', organization: 1 }, - { name: 'Tree', organization: 1 }, - ]; - const removedLabels = [ - { disassociate: true, id: 1 }, - { disassociate: true, id: 2 }, - ]; + const labels = [ + { id: 3, name: 'Foo', isNew: true }, + { id: 4, name: 'Bar', isNew: true }, + { id: 5, name: 'Maple' }, + { id: 6, name: 'Tree' }, + ] JobTemplatesAPI.update.mockResolvedValue({ data: { ...updatedTemplateData }, }); @@ -190,8 +189,7 @@ describe('', () => { values: { ...mockJobTemplate, ...updatedTemplateData, - newLabels, - removedLabels, + labels, }, }, () => resolve() diff --git a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx index d7eb506975..a4e5227a2a 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx @@ -588,7 +588,7 @@ const FormikApp = withFormik({ inventory: template.inventory || '', project: template.project || '', playbook: template.playbook || '', - labels: summary_fields.labels.results, + labels: summary_fields.labels.results || [], forks: template.forks || 0, limit: template.limit || '', verbosity: template.verbosity || '0', 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 0d53e4b405..cf2da14304 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx @@ -70,7 +70,7 @@ describe('', () => { jest.clearAllMocks(); }); - test('should render labels MultiSelect', async () => { + test('should render LabelsSelect', async () => { const wrapper = mountWithContexts( ', () => { expect(LabelsAPI.read).toHaveBeenCalled(); expect(JobTemplatesAPI.readInstanceGroups).toHaveBeenCalled(); wrapper.update(); - expect( - wrapper - .find('FormGroup[fieldId="template-labels"] MultiSelect') - .prop('associatedItems') - ).toEqual(mockData.summary_fields.labels.results); + const select = wrapper.find('LabelSelect'); + expect(select).toHaveLength(1); + expect(select.prop('value')).toEqual( + mockData.summary_fields.labels.results + ); }); test('should update form values on input changes', async () => { From 77b68e0eb74a5b4df2c8950d8ea72797e862e191 Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Tue, 1 Oct 2019 14:37:42 -0700 Subject: [PATCH 09/11] use getAddedAndRemoved for saving instance groups --- .../JobTemplateAdd/JobTemplateAdd.jsx | 6 +- .../JobTemplateEdit/JobTemplateEdit.jsx | 13 +-- .../Template/shared/JobTemplateForm.jsx | 84 +++++++------------ 3 files changed, 40 insertions(+), 63 deletions(-) diff --git a/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.jsx b/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.jsx index e0aab2f4e5..0623ad7ab8 100644 --- a/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.jsx +++ b/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.jsx @@ -20,8 +20,8 @@ function JobTemplateAdd({ history, i18n }) { const { labels, organizationId, - addedInstanceGroups, - removedInstanceGroups, + instanceGroups, + initialInstanceGroups, ...remainingValues } = values; @@ -32,7 +32,7 @@ function JobTemplateAdd({ history, i18n }) { } = await JobTemplatesAPI.create(remainingValues); await Promise.all([ submitLabels(id, labels, organizationId), - submitInstanceGroups(id, addedInstanceGroups, removedInstanceGroups), + submitInstanceGroups(id, instanceGroups), ]); history.push(`/templates/${type}/${id}/details`); } catch (error) { diff --git a/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx b/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx index 0aa128fb55..a0cf904ab2 100644 --- a/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx +++ b/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx @@ -107,8 +107,8 @@ class JobTemplateEdit extends Component { const { labels, organizationId, - addedInstanceGroups, - removedInstanceGroups, + instanceGroups, + initialInstanceGroups, ...remainingValues } = values; @@ -117,7 +117,7 @@ class JobTemplateEdit extends Component { await JobTemplatesAPI.update(template.id, remainingValues); await Promise.all([ this.submitLabels(labels, organizationId), - this.submitInstanceGroups(addedInstanceGroups, removedInstanceGroups), + this.submitInstanceGroups(instanceGroups, initialInstanceGroups), ]); history.push(this.detailsUrl); } catch (formSubmitError) { @@ -151,12 +151,13 @@ class JobTemplateEdit extends Component { return results; } - async submitInstanceGroups(addedGroups, removedGroups) { + async submitInstanceGroups(groups, initialGroups) { const { template } = this.props; - const associatePromises = addedGroups.map(group => + const { added, removed } = getAddedAndRemoved(initialGroups, groups); + const associatePromises = added.map(group => JobTemplatesAPI.associateInstanceGroup(template.id, group.id) ); - const disassociatePromises = removedGroups.map(group => + const disassociatePromises = removed.map(group => JobTemplatesAPI.disassociateInstanceGroup(template.id, group.id) ); return Promise.all([...associatePromises, ...disassociatePromises]); diff --git a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx index a4e5227a2a..87cff2fa7e 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx @@ -74,20 +74,16 @@ class JobTemplateForm extends Component { contentError: false, project: props.template.summary_fields.project, inventory: props.template.summary_fields.inventory, - relatedInstanceGroups: [], allowCallbacks: !!props.template.host_config_key, }; this.handleProjectValidation = this.handleProjectValidation.bind(this); this.loadRelatedInstanceGroups = this.loadRelatedInstanceGroups.bind(this); - this.handleInstanceGroupsChange = this.handleInstanceGroupsChange.bind( - this - ); } componentDidMount() { const { validateField } = this.props; this.setState({ contentError: null, hasContentLoading: true }); - // TODO: determine whene LabelSelect has finished loading labels + // TODO: determine when LabelSelect has finished loading labels Promise.all([this.loadRelatedInstanceGroups()]).then(() => { this.setState({ hasContentLoading: false }); validateField('project'); @@ -95,16 +91,14 @@ class JobTemplateForm extends Component { } async loadRelatedInstanceGroups() { - const { template } = this.props; + const { setFieldValue, template } = this.props; if (!template.id) { return; } try { const { data } = await JobTemplatesAPI.readInstanceGroups(template.id); - this.setState({ - initialInstanceGroups: data.results, - relatedInstanceGroups: [...data.results], - }); + setFieldValue('initialInstanceGroups', data.results); + setFieldValue('instanceGroups', [...data.results]); } catch (err) { this.setState({ contentError: err }); } @@ -124,37 +118,12 @@ class JobTemplateForm extends Component { }; } - handleInstanceGroupsChange(relatedInstanceGroups) { - const { setFieldValue } = this.props; - const { initialInstanceGroups } = this.state; - let added = []; - const removed = []; - if (initialInstanceGroups) { - initialInstanceGroups.forEach(group => { - if (!relatedInstanceGroups.find(g => g.id === group.id)) { - removed.push(group); - } - }); - relatedInstanceGroups.forEach(group => { - if (!initialInstanceGroups.find(g => g.id === group.id)) { - added.push(group); - } - }); - } else { - added = relatedInstanceGroups; - } - setFieldValue('addedInstanceGroups', added); - setFieldValue('removedInstanceGroups', removed); - this.setState({ relatedInstanceGroups }); - } - render() { const { contentError, hasContentLoading, inventory, project, - relatedInstanceGroups, allowCallbacks, } = this.state; const { @@ -334,13 +303,13 @@ class JobTemplateForm extends Component { content={i18n._(t`Optional labels that describe this job template, such as 'dev' or 'test'. Labels can be used to group and filter job templates and completed jobs.`)} - /> - setFieldValue('labels', labels)} - onError={err => this.setState({ contentError: err })} - /> - + /> + setFieldValue('labels', labels)} + onError={err => this.setState({ contentError: err })} + /> + )} /> @@ -440,12 +409,17 @@ class JobTemplateForm extends Component { )} /> - ( + form.setFieldValue(field.name, value)} + tooltip={i18n._(t`Select the Instance Groups for this Organization + to run on.`)} + /> + )} /> props.handleSubmit(values), From 6d00d4327341507453f65aa7b9947b76cfc7f527 Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Tue, 1 Oct 2019 15:20:51 -0700 Subject: [PATCH 10/11] prettier --- .../src/components/MultiSelect/MultiSelect.jsx | 4 +++- .../components/MultiSelect/MultiSelect.test.jsx | 15 ++++++++++++--- .../JobTemplateAdd/JobTemplateAdd.test.jsx | 14 +++++--------- .../JobTemplateEdit/JobTemplateEdit.test.jsx | 2 +- .../screens/Template/shared/LabelSelect.test.jsx | 9 +++------ .../Template/shared/PlaybookSelect.test.jsx | 2 +- awx/ui_next/src/util/lists.js | 2 +- awx/ui_next/src/util/lists.test.js | 2 +- 8 files changed, 27 insertions(+), 23 deletions(-) diff --git a/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx b/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx index b4676dc513..1f00008859 100644 --- a/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx +++ b/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx @@ -210,7 +210,9 @@ class MultiSelect extends Component { {value.map(item => ( { this.removeItem(item); }} + onClick={() => { + this.removeItem(item); + }} > {item.name} diff --git a/awx/ui_next/src/components/MultiSelect/MultiSelect.test.jsx b/awx/ui_next/src/components/MultiSelect/MultiSelect.test.jsx index 77c2beae1b..e18ed64eff 100644 --- a/awx/ui_next/src/components/MultiSelect/MultiSelect.test.jsx +++ b/awx/ui_next/src/components/MultiSelect/MultiSelect.test.jsx @@ -63,9 +63,15 @@ describe('', () => { wrapper.update(); const event = { preventDefault: () => {}, - target: wrapper.find('DropdownItem').at(1).getDOMNode(), + target: wrapper + .find('DropdownItem') + .at(1) + .getDOMNode(), }; - wrapper.find('DropdownItem').at(1).invoke('onClick')(event); + wrapper + .find('DropdownItem') + .at(1) + .invoke('onClick')(event); expect(onAddNewItem).toHaveBeenCalledWith(options[1]); const newVal = onChange.mock.calls[0][0]; @@ -86,7 +92,10 @@ describe('', () => { /> ); - wrapper.find('Chip').at(1).invoke('onClick')(); + wrapper + .find('Chip') + .at(1) + .invoke('onClick')(); expect(onRemoveItem).toHaveBeenCalledWith(value[1]); const newVal = onChange.mock.calls[0][0]; diff --git a/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.test.jsx b/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.test.jsx index baf3792c67..41032535bf 100644 --- a/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.test.jsx +++ b/awx/ui_next/src/screens/Template/JobTemplateAdd/JobTemplateAdd.test.jsx @@ -57,7 +57,7 @@ describe('', () => { expect(wrapper.find('JobTemplateForm').length).toBe(1); }); - test('should render Job Template Form with default values', async done => { + test('should render Job Template Form with default values', async () => { const wrapper = mountWithContexts(); await waitForElement(wrapper, 'EmptyStateBody', el => el.length === 0); expect(wrapper.find('input#template-description').text()).toBe( @@ -80,10 +80,9 @@ describe('', () => { expect(wrapper.find('input#template-name').text()).toBe(defaultProps.name); expect(wrapper.find('PlaybookSelect')).toHaveLength(1); expect(wrapper.find('ProjectLookup').prop('value')).toBe(null); - done(); }); - test('handleSubmit should post to api', async done => { + test('handleSubmit should post to api', async () => { JobTemplatesAPI.create.mockResolvedValueOnce({ data: { id: 1, @@ -99,7 +98,7 @@ describe('', () => { values: { ...jobTemplateData, labels: [], - } + }, }, () => resolve() ); @@ -108,10 +107,9 @@ describe('', () => { wrapper.find('form').simulate('submit'); await sleep(1); expect(JobTemplatesAPI.create).toHaveBeenCalledWith(jobTemplateData); - done(); }); - test('should navigate to job template detail after form submission', async done => { + test('should navigate to job template detail after form submission', async () => { const history = { push: jest.fn(), }; @@ -133,10 +131,9 @@ describe('', () => { expect(history.push).toHaveBeenCalledWith( '/templates/job_template/1/details' ); - done(); }); - test('should navigate to templates list when cancel is clicked', async done => { + test('should navigate to templates list when cancel is clicked', async () => { const history = { push: jest.fn(), }; @@ -146,6 +143,5 @@ describe('', () => { await waitForElement(wrapper, 'EmptyStateBody', el => el.length === 0); wrapper.find('button[aria-label="Cancel"]').invoke('onClick')(); expect(history.push).toHaveBeenCalledWith('/templates'); - done(); }); }); diff --git a/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.test.jsx b/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.test.jsx index cd7ef7a141..363c1ca920 100644 --- a/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.test.jsx +++ b/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.test.jsx @@ -178,7 +178,7 @@ describe('', () => { { id: 4, name: 'Bar', isNew: true }, { id: 5, name: 'Maple' }, { id: 6, name: 'Tree' }, - ] + ]; JobTemplatesAPI.update.mockResolvedValue({ data: { ...updatedTemplateData }, }); diff --git a/awx/ui_next/src/screens/Template/shared/LabelSelect.test.jsx b/awx/ui_next/src/screens/Template/shared/LabelSelect.test.jsx index 07bf0c0a62..b03d4e1572 100644 --- a/awx/ui_next/src/screens/Template/shared/LabelSelect.test.jsx +++ b/awx/ui_next/src/screens/Template/shared/LabelSelect.test.jsx @@ -1,15 +1,12 @@ import React from 'react'; import { mount } from 'enzyme'; import { LabelsAPI } from '@api'; -import { sleep} from '@testUtils/testUtils'; +import { sleep } from '@testUtils/testUtils'; import LabelSelect from './LabelSelect'; jest.mock('@api'); -const options = [ - { id: 1, name: 'one' }, - { id: 2, name: 'two' }, -]; +const options = [{ id: 1, name: 'one' }, { id: 2, name: 'two' }]; describe('', () => { afterEach(() => { @@ -47,7 +44,7 @@ describe('', () => { expect(LabelsAPI.read).toHaveBeenCalledTimes(2); expect(wrapper.find('MultiSelect').prop('options')).toEqual([ ...options, - ...options + ...options, ]); }); }); diff --git a/awx/ui_next/src/screens/Template/shared/PlaybookSelect.test.jsx b/awx/ui_next/src/screens/Template/shared/PlaybookSelect.test.jsx index 0a2d979fff..3d1a26355a 100644 --- a/awx/ui_next/src/screens/Template/shared/PlaybookSelect.test.jsx +++ b/awx/ui_next/src/screens/Template/shared/PlaybookSelect.test.jsx @@ -14,7 +14,7 @@ describe('', () => { afterEach(() => { jest.resetAllMocks(); - }) + }); test('should reload playbooks when project value changes', () => { const wrapper = mountWithContexts( diff --git a/awx/ui_next/src/util/lists.js b/awx/ui_next/src/util/lists.js index ad03cccde2..561c306faf 100644 --- a/awx/ui_next/src/util/lists.js +++ b/awx/ui_next/src/util/lists.js @@ -1,5 +1,5 @@ /* eslint-disable import/prefer-default-export */ -export function getAddedAndRemoved (original, current) { +export function getAddedAndRemoved(original, current) { original = original || []; current = current || []; const added = []; diff --git a/awx/ui_next/src/util/lists.test.js b/awx/ui_next/src/util/lists.test.js index b15fe009df..126de4f07e 100644 --- a/awx/ui_next/src/util/lists.test.js +++ b/awx/ui_next/src/util/lists.test.js @@ -1,4 +1,4 @@ -import {getAddedAndRemoved} from './lists'; +import { getAddedAndRemoved } from './lists'; const one = { id: 1 }; const two = { id: 2 }; From 82064eb4dcecddd1125a7516a967aee3b428ed0e Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Wed, 2 Oct 2019 14:17:14 -0700 Subject: [PATCH 11/11] fix prop type error in LabelSelect --- awx/ui_next/src/screens/Template/shared/LabelSelect.jsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx b/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx index 73f9b5278c..3443aeb3b4 100644 --- a/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx +++ b/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx @@ -1,5 +1,5 @@ import React, { useState, useEffect } from 'react'; -import { func, arrayOf, number, shape, string } from 'prop-types'; +import { func, arrayOf, number, shape, string, oneOfType } from 'prop-types'; import MultiSelect from '@components/MultiSelect'; import { LabelsAPI } from '@api'; @@ -51,7 +51,7 @@ function LabelSelect({ value, onChange, onError }) { LabelSelect.propTypes = { value: arrayOf( shape({ - id: number.isRequired, + id: oneOfType([number, string]).isRequired, name: string.isRequired, }) ).isRequired,