From acfa6d056f1d45d446bd25c41c22daf11631ffbf Mon Sep 17 00:00:00 2001 From: Alex Corey Date: Fri, 28 Feb 2020 09:43:02 -0500 Subject: [PATCH] Adds error handling to submit labels, prevents uncessary api call The unecessary api call is for the webhook credential id. If there is no webhook service we do not want the api to make a call for get the webhook credential type id. --- .../CodeMirrorInput/VariablesField.jsx | 3 +- .../WorkflowJobTemplateAdd.jsx | 15 ++-- .../WorkflowJobTemplateEdit.jsx | 28 +++---- .../WorkflowJobTemplateEdit.test.jsx | 65 ++++++++++------ .../shared/WorkflowJobTemplateForm.jsx | 78 ++++++++++--------- 5 files changed, 100 insertions(+), 89 deletions(-) diff --git a/awx/ui_next/src/components/CodeMirrorInput/VariablesField.jsx b/awx/ui_next/src/components/CodeMirrorInput/VariablesField.jsx index 5a468bcccb..3c72e56084 100644 --- a/awx/ui_next/src/components/CodeMirrorInput/VariablesField.jsx +++ b/awx/ui_next/src/components/CodeMirrorInput/VariablesField.jsx @@ -5,9 +5,8 @@ import { t } from '@lingui/macro'; import { useField } from 'formik'; import styled from 'styled-components'; import { Split, SplitItem } from '@patternfly/react-core'; -import { CheckboxField } from '@components/FormField'; +import { CheckboxField, FieldTooltip } from '@components/FormField'; import { yamlToJson, jsonToYaml, isJson } from '@util/yaml'; -import { FieldTooltip } from '@components/FormField'; import CodeMirrorInput from './CodeMirrorInput'; import YamlJsonToggle from './YamlJsonToggle'; import { JSON_MODE, YAML_MODE } from './constants'; diff --git a/awx/ui_next/src/screens/Template/WorkflowJobTemplateAdd/WorkflowJobTemplateAdd.jsx b/awx/ui_next/src/screens/Template/WorkflowJobTemplateAdd/WorkflowJobTemplateAdd.jsx index 0cfead83c8..05d68a1b6e 100644 --- a/awx/ui_next/src/screens/Template/WorkflowJobTemplateAdd/WorkflowJobTemplateAdd.jsx +++ b/awx/ui_next/src/screens/Template/WorkflowJobTemplateAdd/WorkflowJobTemplateAdd.jsx @@ -17,33 +17,28 @@ function WorkflowJobTemplateAdd() { const { data: { id }, } = await WorkflowJobTemplatesAPI.create(remainingValues); - await Promise.all([submitLabels(id, labels, organizationId, values)]); + await submitLabels(id, labels, organizationId); history.push(`/templates/workflow_job_template/${id}/details`); } catch (err) { setFormSubmitError(err); } }; - const submitLabels = async ( - templateId, - labels = [], - organizationId, - values - ) => { - if (!organizationId && !values.organization) { + const submitLabels = async (templateId, labels = [], organizationId) => { + if (!organizationId) { try { const { data: { results }, } = await OrganizationsAPI.read(); organizationId = results[0].id; } catch (err) { - setFormSubmitError(err); + throw err; } } const associatePromises = labels.map(label => WorkflowJobTemplatesAPI.associateLabel(templateId, label, organizationId) ); - return Promise.all([...associatePromises]); + return [...associatePromises]; }; const handleCancel = () => { diff --git a/awx/ui_next/src/screens/Template/WorkflowJobTemplateEdit/WorkflowJobTemplateEdit.jsx b/awx/ui_next/src/screens/Template/WorkflowJobTemplateEdit/WorkflowJobTemplateEdit.jsx index 7b620044ed..6c9356e70d 100644 --- a/awx/ui_next/src/screens/Template/WorkflowJobTemplateEdit/WorkflowJobTemplateEdit.jsx +++ b/awx/ui_next/src/screens/Template/WorkflowJobTemplateEdit/WorkflowJobTemplateEdit.jsx @@ -13,46 +13,38 @@ function WorkflowJobTemplateEdit({ template, webhook_key }) { const handleSubmit = async values => { const { labels, ...remainingValues } = values; try { + await submitLabels(labels, values.organization, template.organization); await WorkflowJobTemplatesAPI.update(template.id, remainingValues); - await Promise.all([submitLabels(labels, values.organization)]); history.push(`/templates/workflow_job_template/${template.id}/details`); } catch (err) { setFormSubmitError(err); } }; - const submitLabels = async (labels = [], orgId) => { + const submitLabels = async (labels = [], formOrgId, templateOrgId) => { const { added, removed } = getAddedAndRemoved( template.summary_fields.labels.results, labels ); - if (!orgId && !template.organization) { + let orgId = formOrgId || templateOrgId; + if (!orgId) { try { const { data: { results }, } = await OrganizationsAPI.read(); orgId = results[0].id; } catch (err) { - setFormSubmitError(err); + throw err; } } - const disassociationPromises = removed.map(label => + const disassociationPromises = await removed.map(label => WorkflowJobTemplatesAPI.disassociateLabel(template.id, label) ); - - const associationPromises = added.map(label => { - return WorkflowJobTemplatesAPI.associateLabel( - template.id, - label, - orgId || template.organization - ); - }); - - const results = await Promise.all([ - ...disassociationPromises, - ...associationPromises, - ]); + const associationPromises = await added.map(label => + WorkflowJobTemplatesAPI.associateLabel(template.id, label, orgId) + ); + const results = [...disassociationPromises, ...associationPromises]; return results; }; diff --git a/awx/ui_next/src/screens/Template/WorkflowJobTemplateEdit/WorkflowJobTemplateEdit.test.jsx b/awx/ui_next/src/screens/Template/WorkflowJobTemplateEdit/WorkflowJobTemplateEdit.test.jsx index 0cd3979e35..f38fe2134b 100644 --- a/awx/ui_next/src/screens/Template/WorkflowJobTemplateEdit/WorkflowJobTemplateEdit.test.jsx +++ b/awx/ui_next/src/screens/Template/WorkflowJobTemplateEdit/WorkflowJobTemplateEdit.test.jsx @@ -1,12 +1,14 @@ import React from 'react'; import { Route } from 'react-router-dom'; import { act } from 'react-dom/test-utils'; -import { WorkflowJobTemplatesAPI } from '@api'; +import { WorkflowJobTemplatesAPI, OrganizationsAPI } from '@api'; import { mountWithContexts } from '@testUtils/enzymeHelpers'; import { createMemoryHistory } from 'history'; import WorkflowJobTemplateEdit from './WorkflowJobTemplateEdit'; -jest.mock('@api'); +jest.mock('@api/models/WorkflowJobTemplates'); +jest.mock('@api/models/Organizations'); + const mockTemplate = { id: 6, name: 'Foo', @@ -53,6 +55,7 @@ describe('', () => { afterEach(async () => { wrapper.unmount(); + jest.clearAllMocks(); }); test('renders successfully', () => { @@ -65,8 +68,8 @@ describe('', () => { id: 6, name: 'Alex', description: 'Apollo and Athena', - inventory: { id: 1, name: 'Inventory 1' }, - organization: 2, + inventory: 1, + organization: 1, labels: [{ name: 'Label 2', id: 2 }, { name: 'Generated Label' }], scm_branch: 'master', limit: '5000', @@ -78,8 +81,8 @@ describe('', () => { id: 6, name: 'Alex', description: 'Apollo and Athena', - inventory: { id: 1, name: 'Inventory 1' }, - organization: 2, + inventory: 1, + organization: 1, scm_branch: 'master', limit: '5000', variables: '---', @@ -94,9 +97,9 @@ describe('', () => { await expect(WorkflowJobTemplatesAPI.associateLabel).toBeCalledTimes(1); }); - test('handleCancel navigates the user to the /templates', async () => { - await act(async () => { - await wrapper.find('WorkflowJobTemplateForm').invoke('handleCancel')(); + test('handleCancel navigates the user to the /templates', () => { + act(() => { + wrapper.find('WorkflowJobTemplateForm').invoke('handleCancel')(); }); expect(history.location.pathname).toBe( '/templates/workflow_job_template/6/details' @@ -105,25 +108,41 @@ describe('', () => { test('throwing error renders FormSubmitError component', async () => { const error = new Error('oops'); - WorkflowJobTemplatesAPI.update.mockImplementation(() => - Promise.reject(error) - ); + OrganizationsAPI.read.mockResolvedValue({ results: [{ id: 1 }] }); + WorkflowJobTemplatesAPI.update.mockRejectedValue(error); await act(async () => { - wrapper.find('WorkflowJobTemplateForm').prop('handleSubmit')({ - id: 6, - name: 'Alex', - description: 'Apollo and Athena', - inventory: { id: 1, name: 'Inventory 1' }, - organization: 2, - scm_branch: 'master', - limit: '5000', - variables: '---', - }); + wrapper.find('Button[aria-label="Save"]').simulate('click'); }); - wrapper.update(); expect(WorkflowJobTemplatesAPI.update).toHaveBeenCalled(); + wrapper.update(); expect(wrapper.find('WorkflowJobTemplateForm').prop('submitError')).toEqual( error ); }); + + test('throwing error prevents form submission', () => { + OrganizationsAPI.read.mockRejectedValue(new Error('An error occurred')); + WorkflowJobTemplatesAPI.update.mockResolvedValue(); + + act(() => { + wrapper = mountWithContexts( + , + { + context: { + router: { + history, + }, + }, + } + ); + }); + wrapper.find('Button[aria-label="Save"]').simulate('click'); + + expect(wrapper.find('WorkflowJobTemplateForm').length).toBe(1); + expect(OrganizationsAPI.read).toBeCalled(); + expect(WorkflowJobTemplatesAPI.update).not.toBeCalled(); + expect(history.location.pathname).toBe( + '/templates/workflow_job_template/6/edit' + ); + }); }); diff --git a/awx/ui_next/src/screens/Template/shared/WorkflowJobTemplateForm.jsx b/awx/ui_next/src/screens/Template/shared/WorkflowJobTemplateForm.jsx index 19a8391f8c..7ebea06f9a 100644 --- a/awx/ui_next/src/screens/Template/shared/WorkflowJobTemplateForm.jsx +++ b/awx/ui_next/src/screens/Template/shared/WorkflowJobTemplateForm.jsx @@ -97,11 +97,14 @@ function WorkflowJobTemplateForm({ result: credTypeId, } = useRequest( useCallback(async () => { - const results = await CredentialTypesAPI.read({ - namespace: `${webhookService}_token`, - }); - // TODO: Consider how to handle the situation where the results returns - // and empty array, or any of the other values is undefined or null (data, results, id) + let results; + if (webhookService) { + results = await CredentialTypesAPI.read({ + namespace: `${webhookService}_token`, + }); + // TODO: Consider how to handle the situation where the results returns + // and empty array, or any of the other values is undefined or null (data, results, id) + } return results?.data?.results[0]?.id; }, [webhookService]) ); @@ -109,10 +112,10 @@ function WorkflowJobTemplateForm({ useEffect(() => { loadCredentialType(); }, [loadCredentialType]); - // TODO: Convert this function below to useRequest. Create a new webhookkey component - // that handles all of that api calls. Will also need to move this api call out of - // WorkflowJobTemplate.jsx and add it to workflowJobTemplateDetai.jsx - let initialWebhookKey = webhook_key; + + // TODO: Convert this function below to useRequest. Might want to create a new + // webhookkey component that handles all of that api calls. Will also need + // to move this api call out of WorkflowJobTemplate.jsx and add it to workflowJobTemplateDetai.jsx const changeWebhookKey = async () => { try { const { @@ -124,7 +127,9 @@ function WorkflowJobTemplateForm({ } }; + let initialWebhookKey = webhook_key; const initialWebhookCredential = template?.summary_fields?.webhook_credential; + const storeWebhookValues = (form, webhookServiceValue) => { if ( webhookServiceValue === form.initialValues.webhook_service || @@ -135,12 +140,17 @@ function WorkflowJobTemplateForm({ form.initialValues.webhook_credential ); setWebhookCredential(initialWebhookCredential); + form.setFieldValue('webhook_url', form.initialValues.webhook_url); + form.setFieldValue('webhook_service', form.initialValues.webhook_service); + setWebHookService(form.initialValues.webhook_service); + setWebHookKey(initialWebhookKey); } else { form.setFieldValue('webhook_credential', null); setWebhookCredential(null); + form.setFieldValue( 'webhook_url', `${urlOrigin}/api/v2/workflow_job_templates/${template.id}/${webhookServiceValue}/` @@ -232,36 +242,37 @@ function WorkflowJobTemplateForm({ {({ form }) => ( form.setFieldTouched('organization')} onChange={value => { form.setFieldValue('organization', value?.id || null); setOrganization(value); }} value={organization} - isValid={ - !(form.touched.organization || form.errors.organization) - } - touched={form.touched.organization} - error={form.errors.organization} + isValid={!form.errors.organization} /> )} {({ form }) => ( - { - form.setFieldValue('inventory', value?.id || null); - setInventory(value); - form.setFieldValue('organizationId', value?.organization); - }} - error={form.errors.inventory} - /> + + + { + form.setFieldValue('inventory', value?.id || null); + setInventory(value); + form.setFieldValue('organizationId', value?.organization); + }} + /> + )} @@ -473,7 +479,7 @@ function WorkflowJobTemplateForm({ )} )} - + {submitError && }