From 6e9804b713d67cc233bf0a6c37dbefe942a33884 Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Thu, 26 Sep 2019 13:27:54 -0700 Subject: [PATCH] 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: [], + }); + }); +});