From 8b35642b085b0b31a2cc221d8506bac9704137ef Mon Sep 17 00:00:00 2001 From: Alex Corey Date: Thu, 1 Aug 2019 14:25:41 -0400 Subject: [PATCH] Fixes failing test and addresses PR issues --- awx/ui_next/src/api/Base.js | 6 +-- awx/ui_next/src/api/models/JobTemplates.js | 16 ++++-- .../components/MultiSelect/MultiSelect.jsx | 50 +++++++++---------- .../MultiSelect/MultiSelect.test.jsx | 15 ++++-- .../src/components/MultiSelect/index.js | 5 +- .../JobTemplateEdit/JobTemplateEdit.jsx | 21 ++++---- .../JobTemplateEdit/JobTemplateEdit.test.jsx | 19 ++++++- .../Template/shared/JobTemplateForm.jsx | 32 +++++++----- .../Template/shared/JobTemplateForm.test.jsx | 26 ++++------ 9 files changed, 111 insertions(+), 79 deletions(-) diff --git a/awx/ui_next/src/api/Base.js b/awx/ui_next/src/api/Base.js index c821bb21e0..33e3626994 100644 --- a/awx/ui_next/src/api/Base.js +++ b/awx/ui_next/src/api/Base.js @@ -1,8 +1,6 @@ import axios from 'axios'; -import { - encodeQueryString -} from '@util/qs'; +import { encodeQueryString } from '@util/qs'; const defaultHttp = axios.create({ xsrfCookieName: 'csrftoken', @@ -28,7 +26,7 @@ class Base { read(params) { return this.http.get(this.baseUrl, { - params + params, }); } diff --git a/awx/ui_next/src/api/models/JobTemplates.js b/awx/ui_next/src/api/models/JobTemplates.js index bdf330df09..44eef5529b 100644 --- a/awx/ui_next/src/api/models/JobTemplates.js +++ b/awx/ui_next/src/api/models/JobTemplates.js @@ -8,7 +8,9 @@ class JobTemplates extends InstanceGroupsMixin(Base) { this.launch = this.launch.bind(this); this.readLaunch = this.readLaunch.bind(this); - this.updateLabels = this.updateLabels.bind(this); + this.associateLabel = this.associateLabel.bind(this); + this.disassociateLabel = this.disassociateLabel.bind(this); + this.generateLabel = this.generateLabel.bind(this); } launch(id, data) { @@ -19,8 +21,16 @@ class JobTemplates extends InstanceGroupsMixin(Base) { return this.http.get(`${this.baseUrl}${id}/launch/`); } - updateLabels(id, data) { - return this.http.post(`${this.baseUrl}${id}/labels/`, data) + associateLabel(id, label) { + return this.http.post(`${this.baseUrl}${id}/labels/`, label); + } + + disassociateLabel(id, label) { + return this.http.post(`${this.baseUrl}${id}/labels/`, label); + } + + generateLabel(orgId, label) { + return this.http.post(`${this.baseUrl}${orgId}/labels/`, label); } } diff --git a/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx b/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx index 9bc8ad2a86..ea5a701ccd 100644 --- a/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx +++ b/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx @@ -51,10 +51,9 @@ class MultiSelect extends Component { constructor(props) { super(props); - this.myRef = React.createRef(); this.state = { input: '', - chipItems: [], + chipItems: this.getInitialChipItems(), isExpanded: false, }; this.handleAddItem = this.handleAddItem.bind(this); @@ -65,57 +64,58 @@ class MultiSelect extends Component { } componentDidMount() { - this.renderChips(); document.addEventListener('mousedown', this.handleClick, false); } + componentWillUnmount() { + document.removeEventListener('mousedown', this.handleClick, false); + } + + getInitialChipItems() { + const { associatedItems } = this.props; + return associatedItems.map(item => ({ + name: item.name, + id: item.id, + organization: item.organization, + })); + } + handleClick(e, option) { if (this.node && this.node.contains(e.target)) { if (option) { this.handleSelection(e, option); } - this.setState({ isExpanded: true }); } else { this.setState({ isExpanded: false }); } } - renderChips() { - const { associatedItems } = this.props; - const items = associatedItems.map(item => ({ - name: item.name, - id: item.id, - organization: item.organization, - })); - this.setState({ - chipItems: items, - }); - } - handleSelection(e, item) { const { chipItems } = this.state; const { onAddNewItem } = this.props; + e.preventDefault(); this.setState({ chipItems: chipItems.concat({ name: item.name, id: item.id }), + isExpanded: false, }); onAddNewItem(item); - e.preventDefault(); } handleAddItem(event) { const { input, chipItems } = this.state; const { onAddNewItem } = this.props; const newChip = { name: input, id: Math.random() }; - if (event.key === 'Tab') { - this.setState({ - chipItems: chipItems.concat(newChip), - isExpanded: false, - input: '', - }); - - onAddNewItem(input); + if (event.key !== 'Tab') { + return; } + this.setState({ + chipItems: chipItems.concat(newChip), + isExpanded: false, + input: '', + }); + + onAddNewItem(input); } handleInputChange(e) { diff --git a/awx/ui_next/src/components/MultiSelect/MultiSelect.test.jsx b/awx/ui_next/src/components/MultiSelect/MultiSelect.test.jsx index 368f5ca27f..422baacb84 100644 --- a/awx/ui_next/src/components/MultiSelect/MultiSelect.test.jsx +++ b/awx/ui_next/src/components/MultiSelect/MultiSelect.test.jsx @@ -1,4 +1,5 @@ import React from 'react'; +import { sleep } from '@testUtils/testUtils'; import MultiSelect, { _MultiSelect } from './MultiSelect'; import { mountWithContexts } from '../../../testUtils/enzymeHelpers'; @@ -10,7 +11,10 @@ describe('', () => { const options = [{ name: 'Angry', id: 3 }, { name: 'Potato', id: 4 }]; test('Initially render successfully', () => { - const renderChips = jest.spyOn(_MultiSelect.prototype, 'renderChips'); + const getInitialChipItems = jest.spyOn( + _MultiSelect.prototype, + 'getInitialChipItems' + ); const wrapper = mountWithContexts( ', () => { ); const component = wrapper.find('MultiSelect'); - expect(renderChips).toBeCalled(); + expect(getInitialChipItems).toBeCalled(); expect(component.state().chipItems.length).toBe(2); }); test('handleSelection add item to chipItems', async () => { @@ -33,9 +37,12 @@ describe('', () => { options={options} /> ); - const event = { preventDefault: () => {} }; const component = wrapper.find('MultiSelect'); - component.instance().handleSelection(event, { name: 'Apollo', id: 5 }); + component + .find('input[aria-label="labels"]') + .simulate('keydown', { key: 'Tab' }); + component.update(); + await sleep(1); expect(component.state().chipItems.length).toBe(3); }); test('handleAddItem adds a chip only when Tab is pressed', () => { diff --git a/awx/ui_next/src/components/MultiSelect/index.js b/awx/ui_next/src/components/MultiSelect/index.js index 95d32907ef..8cda42c7cb 100644 --- a/awx/ui_next/src/components/MultiSelect/index.js +++ b/awx/ui_next/src/components/MultiSelect/index.js @@ -1,4 +1 @@ -export { - default -} -from './MultiSelect'; +export { default } from './MultiSelect'; diff --git a/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx b/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx index db7e50e18c..655ececfe5 100644 --- a/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx +++ b/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx @@ -21,28 +21,27 @@ class JobTemplateEdit extends Component { this.handleSubmit = this.handleSubmit.bind(this); } - async handleSubmit(values, newLabels, removedLabels) { + async handleSubmit(values, newLabels = [], removedLabels = []) { const { template: { id, type }, history, } = this.props; - const disassociatedLabels = removedLabels - ? removedLabels.forEach(removedLabel => - JobTemplatesAPI.updateLabels(id, removedLabel) - ) - : null; + const disassociatedLabels = removedLabels.forEach(removedLabel => + JobTemplatesAPI.disassociateLabel(id, removedLabel) + ); const associatedLabels = newLabels - ? newLabels.forEach(newLabel => - JobTemplatesAPI.updateLabels(id, newLabel) - ) - : null; - + .filter(newLabel => !newLabel.organization) + .forEach(newLabel => JobTemplatesAPI.associateLabel(id, newLabel)); + const generatedLabels = newLabels + .filter(newLabel => newLabel.organization) + .forEach(newLabel => JobTemplatesAPI.generateLabel(id, newLabel)); try { await Promise.all([ JobTemplatesAPI.update(id, { ...values }), disassociatedLabels, associatedLabels, + generatedLabels, ]); history.push(`/templates/${type}/${id}/details`); } catch (error) { 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 9912f0e678..1448887e95 100644 --- a/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.test.jsx +++ b/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.test.jsx @@ -36,9 +36,26 @@ describe('', () => { description: 'new description', job_type: 'check', }; + const newLabels = [ + { associate: true, id: 3 }, + { associate: true, id: 3 }, + { name: 'Mapel', organization: 1 }, + { name: 'Tree', organization: 1 }, + ]; + const removedLabels = [ + { disassociate: true, id: 1 }, + { disassociate: true, id: 2 }, + ]; - wrapper.find('JobTemplateForm').prop('handleSubmit')(updatedTemplateData); + wrapper.find('JobTemplateForm').prop('handleSubmit')( + updatedTemplateData, + newLabels, + removedLabels + ); expect(JobTemplatesAPI.update).toHaveBeenCalledWith(1, updatedTemplateData); + expect(JobTemplatesAPI.disassociateLabel).toHaveBeenCalledTimes(2); + expect(JobTemplatesAPI.associateLabel).toHaveBeenCalledTimes(2); + expect(JobTemplatesAPI.generateLabel).toHaveBeenCalledTimes(2); }); test('should navigate to job template detail when cancel is clicked', () => { diff --git a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx index c1a041d4eb..fc2ff53556 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx @@ -67,13 +67,18 @@ class JobTemplateForm extends Component { }; this.handleNewLabel = this.handleNewLabel.bind(this); this.loadLabels = this.loadLabels.bind(this); - this.disassociateLabel = this.disassociateLabel.bind(this); + this.removeLabel = this.removeLabel.bind(this); } componentDidMount() { this.loadLabels(QSConfig); } + // The function below 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. + async loadLabels(QueryConfig) { this.setState({ contentError: null, hasContentLoading: true }); let loadedLabels; @@ -127,23 +132,26 @@ class JobTemplateForm extends Component { } } - disassociateLabel(label) { - const { removedLabels, loadedLabels, newLabels } = this.state; - const isNewCreatedLabel = loadedLabels.some( - loadedLabel => loadedLabel.name !== label.name + removeLabel(label) { + const { removedLabels, newLabels } = this.state; + const { template } = this.props; + + const isAssociatedLabel = template.summary_fields.labels.results.some( + tempLabel => tempLabel.id === label.id ); - if (isNewCreatedLabel) { - const filteredLabels = newLabels.filter( - newLabel => newLabel.name !== label.name - ); - this.setState({ newLabels: filteredLabels }); - } else { + + if (isAssociatedLabel) { this.setState({ removedLabels: removedLabels.concat({ disassociate: true, id: label.id, }), }); + } else { + const filteredLabels = newLabels.filter( + newLabel => newLabel.name !== label.name + ); + this.setState({ newLabels: filteredLabels }); } } @@ -285,7 +293,7 @@ class JobTemplateForm extends Component { 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 b0833f8168..4fb2223d7f 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx @@ -36,14 +36,16 @@ describe('', () => { }); test('initially renders successfully', () => { - mountWithContexts( + const wrapper = mountWithContexts( ); + const component = wrapper.find('ChipGroup'); expect(LabelsAPI.read).toHaveBeenCalled(); + expect(component.find('span#pf-random-id-1').text()).toEqual('Sushi'); }); test('should update form values on input changes', async () => { @@ -131,8 +133,7 @@ describe('', () => { wrapper.setState({ newLabels: [], loadedLabels: [], removedLabels: [] }); multiSelect.setState({ input: 'Foo' }); - - wrapper.find('input[aria-label="labels"]').prop('onKeyDown')(event); + component.find('input[aria-label="labels"]').prop('onKeyDown')(event); expect(handleNewLabel).toHaveBeenCalledWith('Foo'); component.instance().handleNewLabel({ name: 'Bar', id: 2 }); @@ -149,21 +150,16 @@ describe('', () => { handleCancel={jest.fn()} /> ); - const multiSelect = wrapper.find('MultiSelect'); const component = wrapper.find('JobTemplateForm'); - - component.setState({ - newLabels: [{ name: 'Foo', id: 1 }], - loadedLabels: [{ name: 'Bar', id: 3 }], - removedLabels: [], - }); - component.update(); - multiSelect.setState({ input: 'Wowza' }); - component.instance().disassociateLabel({ name: 'Foo', id: 1 }); + // 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); - - component.instance().disassociateLabel({ name: 'Bar', id: 3 }); + // 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); });