From 491f4824b07e36cd5fca4e15204918aeda3977dd Mon Sep 17 00:00:00 2001 From: Alex Corey Date: Wed, 23 Oct 2019 09:59:23 -0400 Subject: [PATCH] Addresses PR Issues Improves credential ID variable in JT model. Removes unused prop from Lookup ComponentDidMount. Removed unused function call from Credentials ComponentDidMount. Streamlines toggleCredential function and moves it to JobTemplateForm. This was done because the JobTemplateForm should handle the credential values passed to the CredentialsLookup. Adds tests for JobTemplateForm to ensure toggleCredentialSelection function is putting proper values in state. Removed withRouter wrapper on CredentialsLookup export. Improved CredentialsLookup test to ensure that onChange is called when user removes a credential from the input. --- awx/ui_next/src/api/models/JobTemplates.js | 8 +-- .../components/Lookup/CredentialsLookup.jsx | 62 ++--------------- .../Lookup/CredentialsLookup.test.jsx | 43 +++++------- awx/ui_next/src/components/Lookup/Lookup.jsx | 7 +- .../JobTemplateEdit/JobTemplateEdit.jsx | 1 - .../Template/shared/JobTemplateForm.jsx | 37 +++++++++-- .../Template/shared/JobTemplateForm.test.jsx | 66 ++++++++++++++++++- 7 files changed, 124 insertions(+), 100 deletions(-) diff --git a/awx/ui_next/src/api/models/JobTemplates.js b/awx/ui_next/src/api/models/JobTemplates.js index e84df42ec1..1587f6c86b 100644 --- a/awx/ui_next/src/api/models/JobTemplates.js +++ b/awx/ui_next/src/api/models/JobTemplates.js @@ -45,15 +45,15 @@ class JobTemplates extends InstanceGroupsMixin(NotificationsMixin(Base)) { return this.http.get(`${this.baseUrl}${id}/credentials/`, { params }); } - associateCredentials(id, credential) { + associateCredentials(id, credentialId) { return this.http.post(`${this.baseUrl}${id}/credentials/`, { - id: credential, + id: credentialId, }); } - disassociateCredentials(id, credential) { + disassociateCredentials(id, credentialId) { return this.http.post(`${this.baseUrl}${id}/credentials/`, { - id: credential, + id: credentialId, disassociate: true, }); } diff --git a/awx/ui_next/src/components/Lookup/CredentialsLookup.jsx b/awx/ui_next/src/components/Lookup/CredentialsLookup.jsx index a57b3a76eb..77ae10570a 100644 --- a/awx/ui_next/src/components/Lookup/CredentialsLookup.jsx +++ b/awx/ui_next/src/components/Lookup/CredentialsLookup.jsx @@ -1,7 +1,6 @@ import React from 'react'; import PropTypes from 'prop-types'; import { withI18n } from '@lingui/react'; -import { withRouter } from 'react-router-dom'; import { t } from '@lingui/macro'; import { FormGroup, Tooltip } from '@patternfly/react-core'; import { QuestionCircleIcon as PFQuestionCircleIcon } from '@patternfly/react-icons'; @@ -19,31 +18,18 @@ class CredentialsLookup extends React.Component { super(props); this.state = { - credentials: props.credentials, selectedCredentialType: { label: 'Machine', id: 1, kind: 'ssh' }, credentialTypes: [], }; - - this.handleCredentialTypeSelect = this.handleCredentialTypeSelect.bind( - this - ); - this.loadCredentials = this.loadCredentials.bind(this); this.loadCredentialTypes = this.loadCredentialTypes.bind(this); - this.toggleCredential = this.toggleCredential.bind(this); + this.handleCredentialTypeSelect = this.handleCredentialTypeSelect.bind(this); + this.loadCredentials = this.loadCredentials.bind(this); } componentDidMount() { - this.loadCredentials({ page: 1, page_size: 5, order_by: 'name' }); this.loadCredentialTypes(); } - componentDidUpdate(prevState) { - const { selectedType } = this.state; - if (prevState.selectedType !== selectedType) { - Promise.all([this.loadCredentials()]); - } - } - async loadCredentialTypes() { const { onError } = this.props; try { @@ -57,6 +43,7 @@ class CredentialsLookup extends React.Component { // require different field values. cred = { id: cred.id, + key: cred.id, kind: cred.kind, type: cred.namespace, value: cred.name, @@ -85,43 +72,9 @@ class CredentialsLookup extends React.Component { this.setState({ selectedCredentialType: selectedType[0] }); } - toggleCredential(item) { - const { credentials: stateToUpdate, selectedCredentialType } = this.state; - const { onChange } = this.props; - const index = stateToUpdate.findIndex( - credential => credential.id === item.id - ); - if (index > -1) { - const newCredentialsList = stateToUpdate.filter( - cred => cred.id !== item.id - ); - this.setState({ credentials: newCredentialsList }); - onChange(newCredentialsList); - return; - } - - const credentialTypeOccupied = stateToUpdate.some( - cred => cred.kind === item.kind - ); - if (selectedCredentialType.value === 'Vault' || !credentialTypeOccupied) { - item.credentialType = selectedCredentialType; - this.setState({ credentials: [...stateToUpdate, item] }); - onChange([...stateToUpdate, item]); - } else { - const credsList = [...stateToUpdate]; - const occupyingCredIndex = stateToUpdate.findIndex( - occupyingCred => occupyingCred.kind === item.kind - ); - credsList.splice(occupyingCredIndex, 1, item); - this.setState({ credentials: credsList }); - onChange(credsList); - } - } - render() { - const { tooltip, i18n } = this.props; - const { credentials, selectedCredentialType, credentialTypes } = this.state; - + const { selectedCredentialType, credentialTypes } = this.state; + const { tooltip, i18n, credentials, onChange } = this.props; return ( {tooltip && ( @@ -134,7 +87,7 @@ class CredentialsLookup extends React.Component { selectCategoryOptions={credentialTypes} selectCategory={this.handleCredentialTypeSelect} selectedCategory={selectedCredentialType} - onToggleItem={this.toggleCredential} + onToggleItem={onChange} onloadCategories={this.loadCredentialTypes} id="org-credentials" lookupHeader={i18n._(t`Credentials`)} @@ -162,7 +115,6 @@ class CredentialsLookup extends React.Component { CredentialsLookup.propTypes = { tooltip: PropTypes.string, - onChange: PropTypes.func.isRequired, }; CredentialsLookup.defaultProps = { @@ -170,4 +122,4 @@ CredentialsLookup.defaultProps = { }; export { CredentialsLookup as _CredentialsLookup }; -export default withI18n()(withRouter(CredentialsLookup)); +export default withI18n()(CredentialsLookup); diff --git a/awx/ui_next/src/components/Lookup/CredentialsLookup.test.jsx b/awx/ui_next/src/components/Lookup/CredentialsLookup.test.jsx index c0cb0d19fb..9831565b94 100644 --- a/awx/ui_next/src/components/Lookup/CredentialsLookup.test.jsx +++ b/awx/ui_next/src/components/Lookup/CredentialsLookup.test.jsx @@ -1,6 +1,6 @@ import React from 'react'; import { mountWithContexts } from '@testUtils/enzymeHelpers'; -import CredentialsLookup, { _CredentialsLookup } from './CredentialsLookup'; +import CredentialsLookup from './CredentialsLookup'; import { CredentialsAPI, CredentialTypesAPI } from '@api'; jest.mock('@api'); @@ -9,6 +9,7 @@ describe('', () => { let wrapper; let lookup; let credLookup; + let onChange; const credentials = [ { id: 1, kind: 'cloud', name: 'Foo', url: 'www.google.com' }, @@ -42,12 +43,12 @@ describe('', () => { count: 3, }, }); - + onChange = jest.fn(); wrapper = mountWithContexts( {}} credentials={credentials} - onChange={() => {}} + onChange={onChange} tooltip="This is credentials look up" /> ); @@ -62,17 +63,22 @@ describe('', () => { test('CredentialsLookup renders properly', () => { expect(wrapper.find('CredentialsLookup')).toHaveLength(1); + expect(CredentialTypesAPI.read).toHaveBeenCalled(); }); - test('Removes credential from directly from input', () => { - const chip = wrapper.find('PFChip').at(0); - expect(chip).toHaveLength(1); - chip.find('ChipButton').invoke('onClick')(); - expect(wrapper.find('PFChip')).toHaveLength(1); + + test('onChange is called when you click to remove a credential from input', () => { + const chip = wrapper.find('PFChip'); + const button = chip.at(1).find('Button'); + expect(chip).toHaveLength(2); + button.prop('onClick')(); + expect(onChange).toBeCalledTimes(1); }); - test('can change credential types', async () => { + + test('can change credential types', () => { lookup.prop('selectCategory')({}, 'Vault'); expect(credLookup.state('selectedCredentialType')).toEqual({ id: 500, + key: 500, kind: 'vault', type: 'buzz', value: 'Vault', @@ -81,23 +87,4 @@ describe('', () => { }); expect(CredentialsAPI.read).toHaveBeenCalled(); }); - test('Toggle credentials properly adds credentials', async () => { - function callOnToggle(item, index) { - lookup.prop('onToggleItem')(item); - expect(credLookup.state('credentials')[index].name).toEqual( - `${item.name}` - ); - } - callOnToggle({ name: 'Gatsby', id: 8, kind: 'Machine' }, 2); - callOnToggle({ name: 'Party', id: 9, kind: 'Machine' }, 2); - callOnToggle({ name: 'Animal', id: 10, kind: 'Ansible' }, 3); - - lookup.prop('onToggleItem')({ - id: 1, - kind: 'cloud', - name: 'Foo', - url: 'www.google.com', - }); - expect(credLookup.state('credentials').length).toBe(3); - }); }); diff --git a/awx/ui_next/src/components/Lookup/Lookup.jsx b/awx/ui_next/src/components/Lookup/Lookup.jsx index 599a0576ae..1895fd8ac7 100644 --- a/awx/ui_next/src/components/Lookup/Lookup.jsx +++ b/awx/ui_next/src/components/Lookup/Lookup.jsx @@ -84,12 +84,7 @@ class Lookup extends React.Component { } componentDidMount() { - const { onLoadCredentialTypes } = this.props; - if (onLoadCredentialTypes) { - Promise.all([onLoadCredentialTypes(), this.getData()]); - } else { - this.getData(); - } + Promise.all([this.getData()]); } componentDidUpdate(prevProps) { diff --git a/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx b/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx index b6cfc65fd6..eb30892719 100644 --- a/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx +++ b/awx/ui_next/src/screens/Template/JobTemplateEdit/JobTemplateEdit.jsx @@ -110,7 +110,6 @@ class JobTemplateEdit extends Component { instanceGroups, initialInstanceGroups, credentials, - initialCredentials, ...remainingValues } = values; diff --git a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx index 310db6f5be..0c8b6d1225 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx @@ -77,9 +77,11 @@ class JobTemplateForm extends Component { project: props.template.summary_fields.project, inventory: props.template.summary_fields.inventory, allowCallbacks: !!props.template.host_config_key, + credentials: props.template.summary_fields.credentials, }; this.handleProjectValidation = this.handleProjectValidation.bind(this); this.loadRelatedInstanceGroups = this.loadRelatedInstanceGroups.bind(this); + this.toggleCredentialSelection = this.toggleCredentialSelection.bind(this); } componentDidMount() { @@ -106,6 +108,31 @@ class JobTemplateForm extends Component { } } + toggleCredentialSelection(newCredential) { + const { credentials: credentialsToUpdate } = this.state; + const { setFieldValue } = this.props; + + let newCredentialsList; + const isSelectedCredentialInState = + credentialsToUpdate.filter(cred => cred.id === newCredential.id).length > + 0; + + if (isSelectedCredentialInState) { + newCredentialsList = credentialsToUpdate.filter( + cred => cred.id !== newCredential.id + ); + } else { + newCredentialsList = credentialsToUpdate.filter( + credential => + credential.kind === 'vault' || credential.kind !== newCredential.kind + ); + newCredentialsList = [...newCredentialsList, newCredential]; + } + + setFieldValue('credentials', newCredentialsList); + this.setState({ credentials: newCredentialsList }); + } + handleProjectValidation() { const { i18n, touched } = this.props; const { project } = this.state; @@ -127,6 +154,7 @@ class JobTemplateForm extends Component { inventory, project, allowCallbacks, + credentials, } = this.state; const { handleCancel, @@ -324,11 +352,11 @@ class JobTemplateForm extends Component { ( + render={() => ( this.setState({ contentError: err })} - credentials={template.summary_fields.credentials} - onChange={value => form.setFieldValue('credentials', value)} tooltip={i18n._( t`Select credentials that allow Tower to access the nodes this job will be ran against. You can only select one credential of each type. For machine credentials (SSH), checking "Prompt on launch" without selecting credentials will require you to select a machine credential at run time. If you select credentials and check "Prompt on launch", the selected credential(s) become the defaults that can be updated at run time.` )} @@ -604,8 +632,7 @@ const FormikApp = withFormik({ organizationId: summary_fields.inventory.organization_id || null, initialInstanceGroups: [], instanceGroups: [], - initialCredentials: summary_fields.credentials || [], - credentials: [], + credentials: summary_fields.credentials || [], }; }, handleSubmit: (values, { props }) => props.handleSubmit(values), 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 2cac31bea9..c939e678d1 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx @@ -3,7 +3,13 @@ import { act } from 'react-dom/test-utils'; import { mountWithContexts, waitForElement } from '@testUtils/enzymeHelpers'; import { sleep } from '@testUtils/testUtils'; import JobTemplateForm from './JobTemplateForm'; -import { LabelsAPI, JobTemplatesAPI, ProjectsAPI } from '@api'; +import { + LabelsAPI, + JobTemplatesAPI, + ProjectsAPI, + CredentialTypesAPI, + CredentialsAPI, +} from '@api'; jest.mock('@api'); @@ -59,10 +65,21 @@ describe('', () => { policy_instance_list: [], }, ]; + const mockCredentials = [ + { id: 1, kind: 'cloud', name: 'Cred 1', url: 'www.google.com' }, + { id: 2, kind: 'ssh', name: 'Cred 2', url: 'www.google.com' }, + { id: 3, kind: 'Ansible', name: 'Cred 3', url: 'www.google.com' }, + { id: 4, kind: 'Machine', name: 'Cred 4', url: 'www.google.com' }, + { id: 5, kind: 'Machine', name: 'Cred 5', url: 'www.google.com' }, + ]; + beforeEach(() => { LabelsAPI.read.mockReturnValue({ data: mockData.summary_fields.labels, }); + CredentialsAPI.read.mockReturnValue({ + data: { results: mockCredentials }, + }); JobTemplatesAPI.readInstanceGroups.mockReturnValue({ data: { results: mockInstanceGroups }, }); @@ -138,6 +155,13 @@ describe('', () => { target: { value: 'new baz type', name: 'playbook' }, }); expect(form.state('values').playbook).toEqual('new baz type'); + wrapper + .find('CredentialChip') + .at(0) + .prop('onClick')(); + expect(form.state('values').credentials).toEqual([ + { id: 2, kind: 'ssh', name: 'Bar' }, + ]); }); test('should call handleSubmit when Submit button is clicked', async () => { @@ -176,4 +200,44 @@ describe('', () => { wrapper.find('button[aria-label="Cancel"]').invoke('onClick')(); expect(handleCancel).toBeCalled(); }); + test('toggleCredentialSelection should handle credential selection properly', async () => { + let wrapper; + await act(async () => { + wrapper = mountWithContexts( + + ); + }); + + function callToggleCredSelection(credential, formState) { + JobTempForm.instance().toggleCredentialSelection(credential); + + expect(form.state('values').credentials).toEqual(formState); + } + const form = wrapper.find('Formik'); + const JobTempForm = wrapper.find('JobTemplateForm'); + + callToggleCredSelection( + { id: 3, kind: 'vault', name: 'Vault Credential' }, + [ + { id: 1, kind: 'cloud', name: 'Foo' }, + { id: 2, kind: 'ssh', name: 'Bar' }, + { id: 3, kind: 'vault', name: 'Vault Credential' }, + ] + ); + callToggleCredSelection({ id: 4, kind: 'ssh', name: 'New Bar' }, [ + { id: 1, kind: 'cloud', name: 'Foo' }, + { id: 3, kind: 'vault', name: 'Vault Credential' }, + { id: 4, kind: 'ssh', name: 'New Bar' }, + ]); + callToggleCredSelection({ id: 5, kind: 'vault', name: 'New Vault' }, [ + { id: 1, kind: 'cloud', name: 'Foo' }, + { id: 3, kind: 'vault', name: 'Vault Credential' }, + { id: 4, kind: 'ssh', name: 'New Bar' }, + { id: 5, kind: 'vault', name: 'New Vault' }, + ]); + }); });