From 6889128571e521acb32558a749b8ae0d9100a631 Mon Sep 17 00:00:00 2001 From: mabashian Date: Tue, 25 Aug 2020 09:08:06 -0400 Subject: [PATCH 1/8] Peel axios instantiation out into it's own file so that it can be used in the RelatedAPI model --- awx/ui_next/src/api/AxiosHTTP.js | 13 +++++++++++ awx/ui_next/src/api/Base.js | 14 ++---------- awx/ui_next/src/api/models/Related.js | 31 +++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 12 deletions(-) create mode 100644 awx/ui_next/src/api/AxiosHTTP.js create mode 100644 awx/ui_next/src/api/models/Related.js diff --git a/awx/ui_next/src/api/AxiosHTTP.js b/awx/ui_next/src/api/AxiosHTTP.js new file mode 100644 index 0000000000..f87ee8d78d --- /dev/null +++ b/awx/ui_next/src/api/AxiosHTTP.js @@ -0,0 +1,13 @@ +import axios from 'axios'; + +import { encodeQueryString } from '../util/qs'; + +const AxiosHTTP = axios.create({ + xsrfCookieName: 'csrftoken', + xsrfHeaderName: 'X-CSRFToken', + paramsSerializer(params) { + return encodeQueryString(params); + }, +}); + +export default AxiosHTTP; diff --git a/awx/ui_next/src/api/Base.js b/awx/ui_next/src/api/Base.js index 56492715fb..22b39cc5b2 100644 --- a/awx/ui_next/src/api/Base.js +++ b/awx/ui_next/src/api/Base.js @@ -1,17 +1,7 @@ -import axios from 'axios'; - -import { encodeQueryString } from '../util/qs'; - -const defaultHttp = axios.create({ - xsrfCookieName: 'csrftoken', - xsrfHeaderName: 'X-CSRFToken', - paramsSerializer(params) { - return encodeQueryString(params); - }, -}); +import AxiosHTTP from './AxiosHTTP'; class Base { - constructor(http = defaultHttp, baseURL) { + constructor(http = AxiosHTTP, baseURL) { this.http = http; this.baseUrl = baseURL; } diff --git a/awx/ui_next/src/api/models/Related.js b/awx/ui_next/src/api/models/Related.js new file mode 100644 index 0000000000..8650fa9589 --- /dev/null +++ b/awx/ui_next/src/api/models/Related.js @@ -0,0 +1,31 @@ +import AxiosHTTP from '../AxiosHTTP'; + +class Related { + constructor(http = AxiosHTTP) { + this.http = http; + } + + delete(relatedEndpoint) { + return this.http.delete(relatedEndpoint); + } + + get(relatedEndpoint, params) { + return this.http.get(relatedEndpoint, { + params, + }); + } + + patch(relatedEndpoint, data) { + return this.http.patch(relatedEndpoint, data); + } + + post(relatedEndpoint, data) { + return this.http.post(relatedEndpoint, data); + } + + put(relatedEndpoint, data) { + return this.http.put(relatedEndpoint, data); + } +} + +export default Related; From f27b541396e2d456f0665dbf8d163e52d37e1360 Mon Sep 17 00:00:00 2001 From: mabashian Date: Tue, 25 Aug 2020 10:05:44 -0400 Subject: [PATCH 2/8] Adds cancel button to jobs list toolbar --- .../src/components/JobList/JobList.jsx | 73 +++++++-- .../src/components/JobList/JobList.test.jsx | 102 +++++++++++++ .../JobList/JobListCancelButton.jsx | 141 ++++++++++++++++++ .../JobList/JobListCancelButton.test.jsx | 110 ++++++++++++++ 4 files changed, 415 insertions(+), 11 deletions(-) create mode 100644 awx/ui_next/src/components/JobList/JobListCancelButton.jsx create mode 100644 awx/ui_next/src/components/JobList/JobListCancelButton.test.jsx diff --git a/awx/ui_next/src/components/JobList/JobList.jsx b/awx/ui_next/src/components/JobList/JobList.jsx index 502b4378c7..b4cb3c940c 100644 --- a/awx/ui_next/src/components/JobList/JobList.jsx +++ b/awx/ui_next/src/components/JobList/JobList.jsx @@ -8,15 +8,20 @@ import AlertModal from '../AlertModal'; import DatalistToolbar from '../DataListToolbar'; import ErrorDetail from '../ErrorDetail'; import PaginatedDataList, { ToolbarDeleteButton } from '../PaginatedDataList'; -import useRequest, { useDeleteItems } from '../../util/useRequest'; +import useRequest, { + useDeleteItems, + useDismissableError, +} from '../../util/useRequest'; import { getQSConfig, parseQueryString } from '../../util/qs'; import JobListItem from './JobListItem'; +import JobListCancelButton from './JobListCancelButton'; import useWsJobs from './useWsJobs'; import { AdHocCommandsAPI, InventoryUpdatesAPI, JobsAPI, ProjectUpdatesAPI, + RelatedAPI, SystemJobsAPI, UnifiedJobsAPI, WorkflowJobsAPI, @@ -88,6 +93,30 @@ function JobList({ i18n, defaultParams, showTypeColumn = false }) { const jobs = useWsJobs(results, fetchJobsById, QS_CONFIG); const isAllSelected = selected.length === jobs.length && selected.length > 0; + + const { + error: cancelJobsError, + isLoading: isCancelLoading, + request: cancelJobs, + } = useRequest( + useCallback(async () => { + return Promise.all( + selected.map(job => { + if (['new', 'pending', 'waiting', 'running'].includes(job.status)) { + return RelatedAPI.post(job.related.cancel); + } + return Promise.resolve(); + }) + ); + }, [selected]), + {} + ); + + const { + error: cancelError, + dismissError: dismissCancelError, + } = useDismissableError(cancelJobsError); + const { isLoading: isDeleteLoading, deleteItems: deleteJobs, @@ -123,6 +152,11 @@ function JobList({ i18n, defaultParams, showTypeColumn = false }) { } ); + const handleJobCancel = async () => { + await cancelJobs(); + setSelected([]); + }; + const handleJobDelete = async () => { await deleteJobs(); setSelected([]); @@ -145,7 +179,7 @@ function JobList({ i18n, defaultParams, showTypeColumn = false }) { , + , ]} /> )} @@ -256,15 +294,28 @@ function JobList({ i18n, defaultParams, showTypeColumn = false }) { )} /> - - {i18n._(t`Failed to delete one or more jobs.`)} - - + {deletionError && ( + + {i18n._(t`Failed to delete one or more jobs.`)} + + + )} + {cancelError && ( + + {i18n._(t`Failed to cancel one or more jobs.`)} + + + )} ); } diff --git a/awx/ui_next/src/components/JobList/JobList.test.jsx b/awx/ui_next/src/components/JobList/JobList.test.jsx index 767243c59b..ffa2aff960 100644 --- a/awx/ui_next/src/components/JobList/JobList.test.jsx +++ b/awx/ui_next/src/components/JobList/JobList.test.jsx @@ -9,6 +9,7 @@ import { InventoryUpdatesAPI, JobsAPI, ProjectUpdatesAPI, + RelatedAPI, SystemJobsAPI, UnifiedJobsAPI, WorkflowJobsAPI, @@ -23,6 +24,10 @@ const mockResults = [ url: '/api/v2/project_updates/1', name: 'job 1', type: 'project_update', + status: 'running', + related: { + cancel: '/api/v2/project_updates/1/cancel', + }, summary_fields: { user_capabilities: { delete: true, @@ -35,6 +40,10 @@ const mockResults = [ url: '/api/v2/jobs/2', name: 'job 2', type: 'job', + status: 'running', + related: { + cancel: '/api/v2/jobs/2/cancel', + }, summary_fields: { user_capabilities: { delete: true, @@ -47,6 +56,10 @@ const mockResults = [ url: '/api/v2/inventory_updates/3', name: 'job 3', type: 'inventory_update', + status: 'running', + related: { + cancel: '/api/v2/inventory_updates/3/cancel', + }, summary_fields: { user_capabilities: { delete: true, @@ -59,6 +72,10 @@ const mockResults = [ url: '/api/v2/workflow_jobs/4', name: 'job 4', type: 'workflow_job', + status: 'running', + related: { + cancel: '/api/v2/workflow_jobs/4/cancel', + }, summary_fields: { user_capabilities: { delete: true, @@ -71,6 +88,10 @@ const mockResults = [ url: '/api/v2/system_jobs/5', name: 'job 5', type: 'system_job', + status: 'running', + related: { + cancel: '/api/v2/system_jobs/5/cancel', + }, summary_fields: { user_capabilities: { delete: true, @@ -83,6 +104,10 @@ const mockResults = [ url: '/api/v2/ad_hoc_commands/6', name: 'job 6', type: 'ad_hoc_command', + status: 'running', + related: { + cancel: '/api/v2/ad_hoc_commands/6/cancel', + }, summary_fields: { user_capabilities: { delete: true, @@ -273,4 +298,81 @@ describe('', () => { el => el.props().isOpen === true && el.props().title === 'Error!' ); }); + + test('should send all corresponding delete API requests', async () => { + RelatedAPI.post = jest.fn(); + let wrapper; + await act(async () => { + wrapper = mountWithContexts(); + }); + await waitForLoaded(wrapper); + + act(() => { + wrapper.find('DataListToolbar').invoke('onSelectAll')(true); + }); + wrapper.update(); + wrapper.find('JobListItem'); + expect( + wrapper.find('JobListCancelButton').prop('jobsToCancel') + ).toHaveLength(6); + + await act(async () => { + wrapper.find('JobListCancelButton').invoke('onCancel')(); + }); + expect(RelatedAPI.post).toHaveBeenCalledTimes(6); + expect(RelatedAPI.post).toHaveBeenCalledWith( + '/api/v2/project_updates/1/cancel' + ); + expect(RelatedAPI.post).toHaveBeenCalledWith('/api/v2/jobs/2/cancel'); + expect(RelatedAPI.post).toHaveBeenCalledWith( + '/api/v2/inventory_updates/3/cancel' + ); + expect(RelatedAPI.post).toHaveBeenCalledWith( + '/api/v2/workflow_jobs/4/cancel' + ); + expect(RelatedAPI.post).toHaveBeenCalledWith( + '/api/v2/system_jobs/5/cancel' + ); + expect(RelatedAPI.post).toHaveBeenCalledWith( + '/api/v2/ad_hoc_commands/6/cancel' + ); + + jest.restoreAllMocks(); + }); + + test('error is shown when job not successfully cancelled', async () => { + RelatedAPI.post.mockImplementation(() => { + throw new Error({ + response: { + config: { + method: 'post', + url: '/api/v2/jobs/2/cancel', + }, + data: 'An error occurred', + }, + }); + }); + let wrapper; + await act(async () => { + wrapper = mountWithContexts(); + }); + await waitForLoaded(wrapper); + await act(async () => { + wrapper + .find('JobListItem') + .at(1) + .invoke('onSelect')(); + }); + wrapper.update(); + + await act(async () => { + wrapper.find('JobListCancelButton').invoke('onCancel')(); + }); + wrapper.update(); + await waitForElement( + wrapper, + 'Modal', + el => el.props().isOpen === true && el.props().title === 'Error!' + ); + }); }); diff --git a/awx/ui_next/src/components/JobList/JobListCancelButton.jsx b/awx/ui_next/src/components/JobList/JobListCancelButton.jsx new file mode 100644 index 0000000000..a178034479 --- /dev/null +++ b/awx/ui_next/src/components/JobList/JobListCancelButton.jsx @@ -0,0 +1,141 @@ +import React, { useState } from 'react'; +import { withI18n } from '@lingui/react'; +import { t } from '@lingui/macro'; +import { arrayOf, func } from 'prop-types'; +import { Button, DropdownItem, Tooltip } from '@patternfly/react-core'; +import { Kebabified } from '../../contexts/Kebabified'; +import AlertModal from '../AlertModal'; +import { Job } from '../../types'; + +function cannotCancel(job) { + return !job.summary_fields.user_capabilities.start; +} + +function JobListCancelButton({ i18n, jobsToCancel, onCancel }) { + const [isModalOpen, setIsModalOpen] = useState(false); + + const handleCancel = () => { + onCancel(); + setIsModalOpen(false); + }; + + const renderTooltip = () => { + const jobsUnableToCancel = jobsToCancel + .filter(cannotCancel) + .map(job => job.name) + .join(', '); + if (jobsToCancel.some(cannotCancel)) { + return ( +
+ {i18n.plural({ + value: jobsToCancel.length, + one: 'You do not have permission to cancel the following job: ', + other: 'You do not have permission to cancel the following jobs: ', + })} + {jobsUnableToCancel} +
+ ); + } + if (jobsToCancel.length) { + return i18n.plural({ + value: jobsToCancel.length, + one: 'Cancel selected job', + other: 'Cancel selected jobs', + }); + } + return i18n._(t`Select a job to cancel`); + }; + + const isDisabled = + jobsToCancel.length === 0 || jobsToCancel.some(cannotCancel); + + const cancelJobText = i18n.plural({ + value: jobsToCancel.length < 2, + one: 'Cancel job', + other: 'Cancel jobs', + }); + + return ( + + {({ isKebabified }) => ( + <> + {isKebabified ? ( + setIsModalOpen(true)} + > + {cancelJobText} + + ) : ( + +
+ +
+ + )} + {isModalOpen && ( + setIsModalOpen(false)} + actions={[ + , + , + ]} + > +
+ {i18n.plural({ + value: jobsToCancel.length, + one: 'This action will cancel the following job:', + other: 'This action will cancel the following jobs:', + })} +
+ {jobsToCancel.map(job => ( + + {job.name} +
+ + ))} + + )} + + )} + + ); +} + +JobListCancelButton.propTypes = { + jobsToCancel: arrayOf(Job), + onCancel: func, +}; + +JobListCancelButton.defaultProps = { + jobsToCancel: [], + onCancel: () => {}, +}; + +export default withI18n()(JobListCancelButton); diff --git a/awx/ui_next/src/components/JobList/JobListCancelButton.test.jsx b/awx/ui_next/src/components/JobList/JobListCancelButton.test.jsx new file mode 100644 index 0000000000..7f085615cf --- /dev/null +++ b/awx/ui_next/src/components/JobList/JobListCancelButton.test.jsx @@ -0,0 +1,110 @@ +import React from 'react'; +import { shallow } from 'enzyme'; +import { mountWithContexts } from '../../../testUtils/enzymeHelpers'; +import JobListCancelButton from './JobListCancelButton'; + +describe('', () => { + let wrapper; + + afterEach(() => { + wrapper.unmount(); + }); + test('should be disabled when no rows are selected', () => { + wrapper = mountWithContexts(); + expect(wrapper.find('JobListCancelButton button').props().disabled).toBe( + true + ); + expect(wrapper.find('Tooltip').props().content).toBe( + 'Select a job to cancel' + ); + }); + test('should be disabled when user does not have permissions to cancel selected job', () => { + wrapper = mountWithContexts( + + ); + expect(wrapper.find('JobListCancelButton button').props().disabled).toBe( + true + ); + const tooltipContents = wrapper.find('Tooltip').props().content; + const renderedTooltipContents = shallow(tooltipContents); + expect( + renderedTooltipContents.matchesElement( +
+ You do not have permission to cancel the following job: some job +
+ ) + ).toBe(true); + }); + test('should be enabled when user does have permission to cancel selected job', () => { + wrapper = mountWithContexts( + + ); + expect(wrapper.find('JobListCancelButton button').props().disabled).toBe( + false + ); + expect(wrapper.find('Tooltip').props().content).toBe('Cancel selected job'); + }); + test('modal functions as expected', () => { + const onCancel = jest.fn(); + wrapper = mountWithContexts( + + ); + expect(wrapper.find('AlertModal').length).toBe(0); + wrapper.find('JobListCancelButton button').simulate('click'); + wrapper.update(); + expect(wrapper.find('AlertModal').length).toBe(1); + wrapper.find('AlertModal button[aria-label="Return"]').simulate('click'); + wrapper.update(); + expect(onCancel).toHaveBeenCalledTimes(0); + expect(wrapper.find('AlertModal').length).toBe(0); + expect(wrapper.find('AlertModal').length).toBe(0); + wrapper.find('JobListCancelButton button').simulate('click'); + wrapper.update(); + expect(wrapper.find('AlertModal').length).toBe(1); + wrapper + .find('AlertModal button[aria-label="Cancel job"]') + .simulate('click'); + wrapper.update(); + expect(onCancel).toHaveBeenCalledTimes(1); + }); +}); From bae10718d570171cd6d1ae2a1658247bf84e0215 Mon Sep 17 00:00:00 2001 From: mabashian Date: Wed, 26 Aug 2020 16:03:24 -0400 Subject: [PATCH 3/8] Remove RelatedAPI model in favor of JobsAPI. Adds cancel method to JobsAPI and uses that on the jobs list. --- awx/ui_next/src/api/AxiosHTTP.js | 13 ------ awx/ui_next/src/api/Base.js | 14 ++++++- awx/ui_next/src/api/models/Jobs.js | 40 ++++++++++++++----- awx/ui_next/src/api/models/Related.js | 31 -------------- .../src/components/JobList/JobList.jsx | 3 +- .../src/components/JobList/JobList.test.jsx | 30 +++++--------- 6 files changed, 53 insertions(+), 78 deletions(-) delete mode 100644 awx/ui_next/src/api/AxiosHTTP.js delete mode 100644 awx/ui_next/src/api/models/Related.js diff --git a/awx/ui_next/src/api/AxiosHTTP.js b/awx/ui_next/src/api/AxiosHTTP.js deleted file mode 100644 index f87ee8d78d..0000000000 --- a/awx/ui_next/src/api/AxiosHTTP.js +++ /dev/null @@ -1,13 +0,0 @@ -import axios from 'axios'; - -import { encodeQueryString } from '../util/qs'; - -const AxiosHTTP = axios.create({ - xsrfCookieName: 'csrftoken', - xsrfHeaderName: 'X-CSRFToken', - paramsSerializer(params) { - return encodeQueryString(params); - }, -}); - -export default AxiosHTTP; diff --git a/awx/ui_next/src/api/Base.js b/awx/ui_next/src/api/Base.js index 22b39cc5b2..56492715fb 100644 --- a/awx/ui_next/src/api/Base.js +++ b/awx/ui_next/src/api/Base.js @@ -1,7 +1,17 @@ -import AxiosHTTP from './AxiosHTTP'; +import axios from 'axios'; + +import { encodeQueryString } from '../util/qs'; + +const defaultHttp = axios.create({ + xsrfCookieName: 'csrftoken', + xsrfHeaderName: 'X-CSRFToken', + paramsSerializer(params) { + return encodeQueryString(params); + }, +}); class Base { - constructor(http = AxiosHTTP, baseURL) { + constructor(http = defaultHttp, baseURL) { this.http = http; this.baseUrl = baseURL; } diff --git a/awx/ui_next/src/api/models/Jobs.js b/awx/ui_next/src/api/models/Jobs.js index 06b929c0ba..9c43509f9e 100644 --- a/awx/ui_next/src/api/models/Jobs.js +++ b/awx/ui_next/src/api/models/Jobs.js @@ -1,13 +1,29 @@ import Base from '../Base'; import RelaunchMixin from '../mixins/Relaunch.mixin'; -const BASE_URLS = { - playbook: '/jobs/', - project: '/project_updates/', - system: '/system_jobs/', - inventory: '/inventory_updates/', - command: '/ad_hoc_commands/', - workflow: '/workflow_jobs/', +const getBaseURL = type => { + switch (type) { + case 'playbook': + case 'job': + return '/jobs/'; + case 'project': + case 'project_update': + return '/project_updates/'; + case 'system': + case 'system_job': + return '/system_jobs/'; + case 'inventory': + case 'inventory_update': + return '/inventory_updates/'; + case 'command': + case 'ad_hoc_command': + return '/ad_hoc_commands/'; + case 'workflow': + case 'workflow_job': + return '/workflow_jobs/'; + default: + throw new Error('Unable to find matching job type'); + } }; class Jobs extends RelaunchMixin(Base) { @@ -16,16 +32,20 @@ class Jobs extends RelaunchMixin(Base) { this.baseUrl = '/api/v2/jobs/'; } + cancel(id, type) { + return this.http.post(`/api/v2${getBaseURL(type)}${id}/cancel/`); + } + readDetail(id, type) { - return this.http.get(`/api/v2${BASE_URLS[type]}${id}/`); + return this.http.get(`/api/v2${getBaseURL(type)}${id}/`); } readEvents(id, type = 'playbook', params = {}) { let endpoint; if (type === 'playbook') { - endpoint = `/api/v2${BASE_URLS[type]}${id}/job_events/`; + endpoint = `/api/v2${getBaseURL(type)}${id}/job_events/`; } else { - endpoint = `/api/v2${BASE_URLS[type]}${id}/events/`; + endpoint = `/api/v2${getBaseURL(type)}${id}/events/`; } return this.http.get(endpoint, { params }); } diff --git a/awx/ui_next/src/api/models/Related.js b/awx/ui_next/src/api/models/Related.js deleted file mode 100644 index 8650fa9589..0000000000 --- a/awx/ui_next/src/api/models/Related.js +++ /dev/null @@ -1,31 +0,0 @@ -import AxiosHTTP from '../AxiosHTTP'; - -class Related { - constructor(http = AxiosHTTP) { - this.http = http; - } - - delete(relatedEndpoint) { - return this.http.delete(relatedEndpoint); - } - - get(relatedEndpoint, params) { - return this.http.get(relatedEndpoint, { - params, - }); - } - - patch(relatedEndpoint, data) { - return this.http.patch(relatedEndpoint, data); - } - - post(relatedEndpoint, data) { - return this.http.post(relatedEndpoint, data); - } - - put(relatedEndpoint, data) { - return this.http.put(relatedEndpoint, data); - } -} - -export default Related; diff --git a/awx/ui_next/src/components/JobList/JobList.jsx b/awx/ui_next/src/components/JobList/JobList.jsx index b4cb3c940c..d02b8b7f60 100644 --- a/awx/ui_next/src/components/JobList/JobList.jsx +++ b/awx/ui_next/src/components/JobList/JobList.jsx @@ -21,7 +21,6 @@ import { InventoryUpdatesAPI, JobsAPI, ProjectUpdatesAPI, - RelatedAPI, SystemJobsAPI, UnifiedJobsAPI, WorkflowJobsAPI, @@ -103,7 +102,7 @@ function JobList({ i18n, defaultParams, showTypeColumn = false }) { return Promise.all( selected.map(job => { if (['new', 'pending', 'waiting', 'running'].includes(job.status)) { - return RelatedAPI.post(job.related.cancel); + return JobsAPI.cancel(job.id, job.type); } return Promise.resolve(); }) diff --git a/awx/ui_next/src/components/JobList/JobList.test.jsx b/awx/ui_next/src/components/JobList/JobList.test.jsx index ffa2aff960..87f74abfeb 100644 --- a/awx/ui_next/src/components/JobList/JobList.test.jsx +++ b/awx/ui_next/src/components/JobList/JobList.test.jsx @@ -9,7 +9,6 @@ import { InventoryUpdatesAPI, JobsAPI, ProjectUpdatesAPI, - RelatedAPI, SystemJobsAPI, UnifiedJobsAPI, WorkflowJobsAPI, @@ -300,7 +299,7 @@ describe('', () => { }); test('should send all corresponding delete API requests', async () => { - RelatedAPI.post = jest.fn(); + JobsAPI.cancel = jest.fn(); let wrapper; await act(async () => { wrapper = mountWithContexts(); @@ -319,29 +318,20 @@ describe('', () => { await act(async () => { wrapper.find('JobListCancelButton').invoke('onCancel')(); }); - expect(RelatedAPI.post).toHaveBeenCalledTimes(6); - expect(RelatedAPI.post).toHaveBeenCalledWith( - '/api/v2/project_updates/1/cancel' - ); - expect(RelatedAPI.post).toHaveBeenCalledWith('/api/v2/jobs/2/cancel'); - expect(RelatedAPI.post).toHaveBeenCalledWith( - '/api/v2/inventory_updates/3/cancel' - ); - expect(RelatedAPI.post).toHaveBeenCalledWith( - '/api/v2/workflow_jobs/4/cancel' - ); - expect(RelatedAPI.post).toHaveBeenCalledWith( - '/api/v2/system_jobs/5/cancel' - ); - expect(RelatedAPI.post).toHaveBeenCalledWith( - '/api/v2/ad_hoc_commands/6/cancel' - ); + + expect(JobsAPI.cancel).toHaveBeenCalledTimes(6); + expect(JobsAPI.cancel).toHaveBeenCalledWith(1, 'project_update'); + expect(JobsAPI.cancel).toHaveBeenCalledWith(2, 'job'); + expect(JobsAPI.cancel).toHaveBeenCalledWith(3, 'inventory_update'); + expect(JobsAPI.cancel).toHaveBeenCalledWith(4, 'workflow_job'); + expect(JobsAPI.cancel).toHaveBeenCalledWith(5, 'system_job'); + expect(JobsAPI.cancel).toHaveBeenCalledWith(6, 'ad_hoc_command'); jest.restoreAllMocks(); }); test('error is shown when job not successfully cancelled', async () => { - RelatedAPI.post.mockImplementation(() => { + JobsAPI.cancel.mockImplementation(() => { throw new Error({ response: { config: { From 225f57fefd61ec8da83a26164b40b7422912d1c9 Mon Sep 17 00:00:00 2001 From: mabashian Date: Thu, 27 Aug 2020 09:26:23 -0400 Subject: [PATCH 4/8] Remove use of i18n.plural in favor of low level i18n._. --- .../JobList/JobListCancelButton.jsx | 58 +++++++++++-------- .../JobList/JobListCancelButton.test.jsx | 17 +----- 2 files changed, 35 insertions(+), 40 deletions(-) diff --git a/awx/ui_next/src/components/JobList/JobListCancelButton.jsx b/awx/ui_next/src/components/JobList/JobListCancelButton.jsx index a178034479..a4fbc197b4 100644 --- a/awx/ui_next/src/components/JobList/JobListCancelButton.jsx +++ b/awx/ui_next/src/components/JobList/JobListCancelButton.jsx @@ -13,6 +13,8 @@ function cannotCancel(job) { function JobListCancelButton({ i18n, jobsToCancel, onCancel }) { const [isModalOpen, setIsModalOpen] = useState(false); + const numJobsToCancel = jobsToCancel.length; + const zeroOrOneJobSelected = numJobsToCancel < 2; const handleCancel = () => { onCancel(); @@ -22,26 +24,28 @@ function JobListCancelButton({ i18n, jobsToCancel, onCancel }) { const renderTooltip = () => { const jobsUnableToCancel = jobsToCancel .filter(cannotCancel) - .map(job => job.name) - .join(', '); - if (jobsToCancel.some(cannotCancel)) { + .map(job => job.name); + const numJobsUnableToCancel = jobsUnableToCancel.length; + if (numJobsUnableToCancel > 0) { return (
- {i18n.plural({ - value: jobsToCancel.length, - one: 'You do not have permission to cancel the following job: ', - other: 'You do not have permission to cancel the following jobs: ', - })} - {jobsUnableToCancel} + {i18n._( + '{numJobsUnableToCancel, plural, one {You do not have permission to cancel the following job:} other {You do not have permission to cancel the following jobs:}}', + { + numJobsUnableToCancel, + } + )} + {' '.concat(jobsUnableToCancel.join(', '))}
); } - if (jobsToCancel.length) { - return i18n.plural({ - value: jobsToCancel.length, - one: 'Cancel selected job', - other: 'Cancel selected jobs', - }); + if (numJobsToCancel > 0) { + return i18n._( + '{numJobsToCancel, plural, one {Cancel selected job} other {Cancel selected jobs}}', + { + numJobsToCancel, + } + ); } return i18n._(t`Select a job to cancel`); }; @@ -49,11 +53,12 @@ function JobListCancelButton({ i18n, jobsToCancel, onCancel }) { const isDisabled = jobsToCancel.length === 0 || jobsToCancel.some(cannotCancel); - const cancelJobText = i18n.plural({ - value: jobsToCancel.length < 2, - one: 'Cancel job', - other: 'Cancel jobs', - }); + const cancelJobText = i18n._( + '{zeroOrOneJobSelected, plural, one {Cancel job} other {Cancel jobs}}', + { + zeroOrOneJobSelected, + } + ); return ( @@ -90,6 +95,7 @@ function JobListCancelButton({ i18n, jobsToCancel, onCancel }) { onClose={() => setIsModalOpen(false)} actions={[ , @@ -108,7 +121,12 @@ function JobListCancelButton({ i18n, jobsToCancel, onCancel }) { key="cancel" variant="secondary" aria-label={i18n._(t`Return`)} - onClick={() => setIsModalOpen(false)} + onClick={() => { + if (isKebabified) { + onKebabModalChange(false); + } + setIsModalOpen(false); + }} > {i18n._(t`Return`)} , diff --git a/awx/ui_next/src/components/PaginatedDataList/ToolbarDeleteButton.jsx b/awx/ui_next/src/components/PaginatedDataList/ToolbarDeleteButton.jsx index 7be476dfc1..4e9b44ae70 100644 --- a/awx/ui_next/src/components/PaginatedDataList/ToolbarDeleteButton.jsx +++ b/awx/ui_next/src/components/PaginatedDataList/ToolbarDeleteButton.jsx @@ -140,14 +140,17 @@ class ToolbarDeleteButton extends React.Component { // See: https://github.com/patternfly/patternfly-react/issues/1894 return ( - {({ isKebabified }) => ( + {({ isKebabified, onKebabModalChange }) => ( {isKebabified ? ( { + onKebabModalChange(true); + this.handleConfirmDelete(); + }} > {i18n._(t`Delete`)} @@ -170,13 +173,23 @@ class ToolbarDeleteButton extends React.Component { variant="danger" title={modalTitle} isOpen={isModalOpen} - onClose={this.handleCancelDelete} + onClose={() => { + if (isKebabified) { + onKebabModalChange(false); + } + this.handleCancelDelete(); + }} actions={[ , @@ -184,7 +197,12 @@ class ToolbarDeleteButton extends React.Component { key="cancel" variant="secondary" aria-label={i18n._(t`cancel delete`)} - onClick={this.handleCancelDelete} + onClick={() => { + if (isKebabified) { + onKebabModalChange(false); + } + this.handleCancelDelete(); + }} > {i18n._(t`Cancel`)} , From c8a07309ee98b2411dd8aeb246e3412faf17c659 Mon Sep 17 00:00:00 2001 From: mabashian Date: Wed, 9 Sep 2020 17:27:09 -0400 Subject: [PATCH 6/8] Fix typo --- .../src/components/DataListToolbar/DataListToolbar.jsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/awx/ui_next/src/components/DataListToolbar/DataListToolbar.jsx b/awx/ui_next/src/components/DataListToolbar/DataListToolbar.jsx index d8c84233c7..73d61f7c60 100644 --- a/awx/ui_next/src/components/DataListToolbar/DataListToolbar.jsx +++ b/awx/ui_next/src/components/DataListToolbar/DataListToolbar.jsx @@ -43,7 +43,7 @@ function DataListToolbar({ }) { const showExpandCollapse = onCompact && onExpand; const [kebabIsOpen, setKebabIsOpen] = useState(false); - const [kebabModalIsOpen, setKebabkebabModalIsOpen] = useState(false); + const [kebabModalIsOpen, setKebabModalIsOpen] = useState(false); const [advancedSearchShown, setAdvancedSearchShown] = useState(false); const onShowAdvancedSearch = shown => { @@ -114,7 +114,7 @@ function DataListToolbar({ setKebabkebabModalIsOpen(isOpen), + onKebabModalChange: isOpen => setKebabModalIsOpen(isOpen), }} > Date: Thu, 10 Sep 2020 13:36:05 -0400 Subject: [PATCH 7/8] Refactor kebab modal tracking logic in delete/cancel buttons --- .../DataListToolbar/DataListToolbar.jsx | 21 +- .../JobList/JobListCancelButton.jsx | 166 ++++++------ .../PaginatedDataList/ToolbarDeleteButton.jsx | 240 ++++++++---------- 3 files changed, 187 insertions(+), 240 deletions(-) diff --git a/awx/ui_next/src/components/DataListToolbar/DataListToolbar.jsx b/awx/ui_next/src/components/DataListToolbar/DataListToolbar.jsx index 73d61f7c60..79e83bda77 100644 --- a/awx/ui_next/src/components/DataListToolbar/DataListToolbar.jsx +++ b/awx/ui_next/src/components/DataListToolbar/DataListToolbar.jsx @@ -1,4 +1,4 @@ -import React, { Fragment, useEffect, useState } from 'react'; +import React, { useState } from 'react'; import PropTypes from 'prop-types'; import { withI18n } from '@lingui/react'; import { t } from '@lingui/macro'; @@ -51,12 +51,6 @@ function DataListToolbar({ setKebabIsOpen(false); }; - useEffect(() => { - if (!kebabModalIsOpen) { - setKebabIsOpen(false); - } - }, [kebabModalIsOpen]); - return ( {showExpandCollapse && ( - + <> - + )} {advancedSearchShown && ( @@ -114,7 +108,12 @@ function DataListToolbar({ setKebabModalIsOpen(isOpen), + onKebabModalChange: isOpen => { + if (kebabIsOpen && kebabModalIsOpen && !isOpen) { + setKebabIsOpen(false); + } + setKebabModalIsOpen(isOpen); + }, }} > } - isOpen={kebabIsOpen} + isOpen={kebabIsOpen || kebabModalIsOpen} isPlain dropdownItems={additionalControls} /> diff --git a/awx/ui_next/src/components/JobList/JobListCancelButton.jsx b/awx/ui_next/src/components/JobList/JobListCancelButton.jsx index 1e04886de4..165b245638 100644 --- a/awx/ui_next/src/components/JobList/JobListCancelButton.jsx +++ b/awx/ui_next/src/components/JobList/JobListCancelButton.jsx @@ -1,9 +1,9 @@ -import React, { useState } from 'react'; +import React, { useContext, useState } from 'react'; import { withI18n } from '@lingui/react'; import { t } from '@lingui/macro'; import { arrayOf, func } from 'prop-types'; import { Button, DropdownItem, Tooltip } from '@patternfly/react-core'; -import { Kebabified } from '../../contexts/Kebabified'; +import { KebabifiedContext } from '../../contexts/Kebabified'; import AlertModal from '../AlertModal'; import { Job } from '../../types'; @@ -12,13 +12,21 @@ function cannotCancel(job) { } function JobListCancelButton({ i18n, jobsToCancel, onCancel }) { + const { isKebabified, onKebabModalChange } = useContext(KebabifiedContext); const [isModalOpen, setIsModalOpen] = useState(false); const numJobsToCancel = jobsToCancel.length; const zeroOrOneJobSelected = numJobsToCancel < 2; - const handleCancel = () => { + const handleCancelJob = () => { onCancel(); - setIsModalOpen(false); + toggleModal(); + }; + + const toggleModal = () => { + if (isKebabified) { + onKebabModalChange(!isModalOpen); + } + setIsModalOpen(!isModalOpen); }; const renderTooltip = () => { @@ -61,96 +69,74 @@ function JobListCancelButton({ i18n, jobsToCancel, onCancel }) { ); return ( - - {({ isKebabified, onKebabModalChange }) => ( - <> - {isKebabified ? ( - + {isKebabified ? ( + + {cancelJobText} + + ) : ( + +
+ -
- - )} - {isModalOpen && ( - { - if (isKebabified) { - onKebabModalChange(false); - } - setIsModalOpen(false); - }} - actions={[ - , - , - ]} - > -
- {i18n._( - '{numJobsToCancel, plural, one {This action will cancel the following job:} other {This action will cancel the following jobs:}}', - { - numJobsToCancel, - } - )} -
- {jobsToCancel.map(job => ( - - {job.name} -
- - ))} - - )} - + + + )} - + {isModalOpen && ( + + {cancelJobText} + , + , + ]} + > +
+ {i18n._( + '{numJobsToCancel, plural, one {This action will cancel the following job:} other {This action will cancel the following jobs:}}', + { + numJobsToCancel, + } + )} +
+ {jobsToCancel.map(job => ( + + {job.name} +
+ + ))} + + )} + ); } diff --git a/awx/ui_next/src/components/PaginatedDataList/ToolbarDeleteButton.jsx b/awx/ui_next/src/components/PaginatedDataList/ToolbarDeleteButton.jsx index 4e9b44ae70..db77aa6174 100644 --- a/awx/ui_next/src/components/PaginatedDataList/ToolbarDeleteButton.jsx +++ b/awx/ui_next/src/components/PaginatedDataList/ToolbarDeleteButton.jsx @@ -1,4 +1,4 @@ -import React, { Fragment } from 'react'; +import React, { useContext, useState } from 'react'; import { func, bool, @@ -12,7 +12,7 @@ import { Button, DropdownItem, Tooltip } from '@patternfly/react-core'; import { withI18n } from '@lingui/react'; import { t } from '@lingui/macro'; import AlertModal from '../AlertModal'; -import { Kebabified } from '../../contexts/Kebabified'; +import { KebabifiedContext } from '../../contexts/Kebabified'; const requireNameOrUsername = props => { const { name, username } = props; @@ -59,53 +59,29 @@ function cannotDelete(item) { return !item.summary_fields.user_capabilities.delete; } -class ToolbarDeleteButton extends React.Component { - static propTypes = { - onDelete: func.isRequired, - itemsToDelete: arrayOf(ItemToDelete).isRequired, - pluralizedItemName: string, - errorMessage: string, - }; +function ToolbarDeleteButton({ + itemsToDelete, + pluralizedItemName, + errorMessage, + onDelete, + i18n, +}) { + const { isKebabified, onKebabModalChange } = useContext(KebabifiedContext); + const [isModalOpen, setIsModalOpen] = useState(false); - static defaultProps = { - pluralizedItemName: 'Items', - errorMessage: '', - }; - - constructor(props) { - super(props); - - this.state = { - isModalOpen: false, - }; - - this.handleConfirmDelete = this.handleConfirmDelete.bind(this); - this.handleCancelDelete = this.handleCancelDelete.bind(this); - this.handleDelete = this.handleDelete.bind(this); - } - - handleConfirmDelete() { - this.setState({ isModalOpen: true }); - } - - handleCancelDelete() { - this.setState({ isModalOpen: false }); - } - - handleDelete() { - const { onDelete } = this.props; + const handleDelete = () => { onDelete(); - this.setState({ isModalOpen: false }); - } + toggleModal(); + }; - renderTooltip() { - const { - itemsToDelete, - pluralizedItemName, - errorMessage, - i18n, - } = this.props; + const toggleModal = () => { + if (isKebabified) { + onKebabModalChange(!isModalOpen); + } + setIsModalOpen(!isModalOpen); + }; + const renderTooltip = () => { const itemsUnableToDelete = itemsToDelete .filter(cannotDelete) .map(item => item.name) @@ -125,103 +101,89 @@ class ToolbarDeleteButton extends React.Component { return i18n._(t`Delete`); } return i18n._(t`Select a row to delete`); - } + }; - render() { - const { itemsToDelete, pluralizedItemName, i18n } = this.props; - const { isModalOpen } = this.state; - const modalTitle = i18n._(t`Delete ${pluralizedItemName}?`); + const modalTitle = i18n._(t`Delete ${pluralizedItemName}?`); - const isDisabled = - itemsToDelete.length === 0 || itemsToDelete.some(cannotDelete); + const isDisabled = + itemsToDelete.length === 0 || itemsToDelete.some(cannotDelete); - // NOTE: Once PF supports tooltips on disabled elements, - // we can delete the extra
around the below. - // See: https://github.com/patternfly/patternfly-react/issues/1894 - return ( - - {({ isKebabified, onKebabModalChange }) => ( - - {isKebabified ? ( - { - onKebabModalChange(true); - this.handleConfirmDelete(); - }} - > - {i18n._(t`Delete`)} - - ) : ( - -
- -
- - )} - {isModalOpen && ( - { - if (isKebabified) { - onKebabModalChange(false); - } - this.handleCancelDelete(); - }} - actions={[ - , - , - ]} - > -
{i18n._(t`This action will delete the following:`)}
- {itemsToDelete.map(item => ( - - {item.name || item.username} -
- - ))} - - )} - - )} - - ); - } + // NOTE: Once PF supports tooltips on disabled elements, + // we can delete the extra
around the below. + // See: https://github.com/patternfly/patternfly-react/issues/1894 + return ( + <> + {isKebabified ? ( + + {i18n._(t`Delete`)} + + ) : ( + +
+ +
+ + )} + {isModalOpen && ( + + {i18n._(t`Delete`)} + , + , + ]} + > +
{i18n._(t`This action will delete the following:`)}
+ {itemsToDelete.map(item => ( + + {item.name || item.username} +
+ + ))} + + )} + + ); } +ToolbarDeleteButton.propTypes = { + onDelete: func.isRequired, + itemsToDelete: arrayOf(ItemToDelete).isRequired, + pluralizedItemName: string, + errorMessage: string, +}; + +ToolbarDeleteButton.defaultProps = { + pluralizedItemName: 'Items', + errorMessage: '', +}; + export default withI18n()(ToolbarDeleteButton); From 130a43f5c47fb087a99b6b2c40b0b4bec8404106 Mon Sep 17 00:00:00 2001 From: mabashian Date: Thu, 10 Sep 2020 15:31:32 -0400 Subject: [PATCH 8/8] Simplify kebab modal open logic --- .../DataListToolbar/DataListToolbar.jsx | 37 ++++++++++--------- .../JobList/JobListCancelButton.jsx | 11 ++++-- .../PaginatedDataList/ToolbarDeleteButton.jsx | 11 ++++-- .../ToolbarDeleteButton.test.jsx | 9 ++--- .../TemplateList/TemplateList.test.jsx | 8 +++- 5 files changed, 43 insertions(+), 33 deletions(-) diff --git a/awx/ui_next/src/components/DataListToolbar/DataListToolbar.jsx b/awx/ui_next/src/components/DataListToolbar/DataListToolbar.jsx index 79e83bda77..f5137d905b 100644 --- a/awx/ui_next/src/components/DataListToolbar/DataListToolbar.jsx +++ b/awx/ui_next/src/components/DataListToolbar/DataListToolbar.jsx @@ -1,4 +1,4 @@ -import React, { useState } from 'react'; +import React, { useEffect, useState } from 'react'; import PropTypes from 'prop-types'; import { withI18n } from '@lingui/react'; import { t } from '@lingui/macro'; @@ -42,15 +42,21 @@ function DataListToolbar({ pagination, }) { const showExpandCollapse = onCompact && onExpand; - const [kebabIsOpen, setKebabIsOpen] = useState(false); - const [kebabModalIsOpen, setKebabModalIsOpen] = useState(false); - const [advancedSearchShown, setAdvancedSearchShown] = useState(false); + const [isKebabOpen, setIsKebabOpen] = useState(false); + const [isKebabModalOpen, setIsKebabModalOpen] = useState(false); + const [isAdvancedSearchShown, setIsAdvancedSearchShown] = useState(false); const onShowAdvancedSearch = shown => { - setAdvancedSearchShown(shown); - setKebabIsOpen(false); + setIsAdvancedSearchShown(shown); + setIsKebabOpen(false); }; + useEffect(() => { + if (!isKebabModalOpen) { + setIsKebabOpen(false); + } + }, [isKebabModalOpen]); + return ( )} - {advancedSearchShown && ( + {isAdvancedSearchShown && ( { - if (kebabIsOpen && kebabModalIsOpen && !isOpen) { - setKebabIsOpen(false); - } - setKebabModalIsOpen(isOpen); - }, + onKebabModalChange: setIsKebabModalOpen, }} > { - if (!kebabModalIsOpen) { - setKebabIsOpen(isOpen); + if (!isKebabModalOpen) { + setIsKebabOpen(isOpen); } }} /> } - isOpen={kebabIsOpen || kebabModalIsOpen} + isOpen={isKebabOpen} isPlain dropdownItems={additionalControls} /> )} - {!advancedSearchShown && ( + {!isAdvancedSearchShown && ( {additionalControls.map(control => ( {control} ))} )} - {!advancedSearchShown && pagination && itemCount > 0 && ( + {!isAdvancedSearchShown && pagination && itemCount > 0 && ( {pagination} )} diff --git a/awx/ui_next/src/components/JobList/JobListCancelButton.jsx b/awx/ui_next/src/components/JobList/JobListCancelButton.jsx index 165b245638..684d088c96 100644 --- a/awx/ui_next/src/components/JobList/JobListCancelButton.jsx +++ b/awx/ui_next/src/components/JobList/JobListCancelButton.jsx @@ -1,4 +1,4 @@ -import React, { useContext, useState } from 'react'; +import React, { useContext, useEffect, useState } from 'react'; import { withI18n } from '@lingui/react'; import { t } from '@lingui/macro'; import { arrayOf, func } from 'prop-types'; @@ -23,12 +23,15 @@ function JobListCancelButton({ i18n, jobsToCancel, onCancel }) { }; const toggleModal = () => { - if (isKebabified) { - onKebabModalChange(!isModalOpen); - } setIsModalOpen(!isModalOpen); }; + useEffect(() => { + if (isKebabified) { + onKebabModalChange(isModalOpen); + } + }, [isKebabified, isModalOpen, onKebabModalChange]); + const renderTooltip = () => { const jobsUnableToCancel = jobsToCancel .filter(cannotCancel) diff --git a/awx/ui_next/src/components/PaginatedDataList/ToolbarDeleteButton.jsx b/awx/ui_next/src/components/PaginatedDataList/ToolbarDeleteButton.jsx index db77aa6174..25a280d549 100644 --- a/awx/ui_next/src/components/PaginatedDataList/ToolbarDeleteButton.jsx +++ b/awx/ui_next/src/components/PaginatedDataList/ToolbarDeleteButton.jsx @@ -1,4 +1,4 @@ -import React, { useContext, useState } from 'react'; +import React, { useContext, useEffect, useState } from 'react'; import { func, bool, @@ -75,12 +75,15 @@ function ToolbarDeleteButton({ }; const toggleModal = () => { - if (isKebabified) { - onKebabModalChange(!isModalOpen); - } setIsModalOpen(!isModalOpen); }; + useEffect(() => { + if (isKebabified) { + onKebabModalChange(isModalOpen); + } + }, [isKebabified, isModalOpen, onKebabModalChange]); + const renderTooltip = () => { const itemsUnableToDelete = itemsToDelete .filter(cannotDelete) diff --git a/awx/ui_next/src/components/PaginatedDataList/ToolbarDeleteButton.test.jsx b/awx/ui_next/src/components/PaginatedDataList/ToolbarDeleteButton.test.jsx index 843ffa5b4a..c85730226c 100644 --- a/awx/ui_next/src/components/PaginatedDataList/ToolbarDeleteButton.test.jsx +++ b/awx/ui_next/src/components/PaginatedDataList/ToolbarDeleteButton.test.jsx @@ -26,8 +26,8 @@ describe('', () => { const wrapper = mountWithContexts( {}} itemsToDelete={[itemA]} /> ); + expect(wrapper.find('Modal')).toHaveLength(0); wrapper.find('button').simulate('click'); - expect(wrapper.find('ToolbarDeleteButton').state('isModalOpen')).toBe(true); wrapper.update(); expect(wrapper.find('Modal')).toHaveLength(1); }); @@ -37,15 +37,14 @@ describe('', () => { const wrapper = mountWithContexts( ); - wrapper.find('ToolbarDeleteButton').setState({ isModalOpen: true }); + wrapper.find('button').simulate('click'); wrapper.update(); wrapper .find('ModalBoxFooter button[aria-label="confirm delete"]') .simulate('click'); + wrapper.update(); expect(onDelete).toHaveBeenCalled(); - expect(wrapper.find('ToolbarDeleteButton').state('isModalOpen')).toBe( - false - ); + expect(wrapper.find('Modal')).toHaveLength(0); }); test('should disable button when no delete permissions', () => { diff --git a/awx/ui_next/src/screens/Template/TemplateList/TemplateList.test.jsx b/awx/ui_next/src/screens/Template/TemplateList/TemplateList.test.jsx index 377e87583e..2486ea5579 100644 --- a/awx/ui_next/src/screens/Template/TemplateList/TemplateList.test.jsx +++ b/awx/ui_next/src/screens/Template/TemplateList/TemplateList.test.jsx @@ -261,7 +261,9 @@ describe('', () => { }, }); - wrapper.find('button[aria-label="Delete"]').prop('onClick')(); + await act(async () => { + wrapper.find('button[aria-label="Delete"]').prop('onClick')(); + }); wrapper.update(); await act(async () => { await wrapper @@ -302,7 +304,9 @@ describe('', () => { summary_fields: { user_capabilities: { delete: true } }, }, }); - wrapper.find('button[aria-label="Delete"]').prop('onClick')(); + await act(async () => { + wrapper.find('button[aria-label="Delete"]').prop('onClick')(); + }); wrapper.update(); await act(async () => { await wrapper