Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not allow user to attempt to delete a running job #9758

Merged
merged 1 commit into from
Apr 6, 2021
+84 −2
Merged

Do not allow user to attempt to delete a running job #9758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions awx/ui_next/src/components/JobList/JobList.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,8 @@ function JobList({ i18n, defaultParams, showTypeColumn = false }) {
}
};

const cannotDeleteItems = selected.filter(job => isJobRunning(job.status));

return (
<>
<Card>
Expand Down Expand Up @@ -238,6 +240,17 @@ function JobList({ i18n, defaultParams, showTypeColumn = false }) {
onDelete={handleJobDelete}
itemsToDelete={selected}
pluralizedItemName={i18n._(t`Jobs`)}
cannotDelete={item =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since pluralization can be complicated its best to use this syntax.

awx/awx/ui_next/src/screens/Inventory/InventoryList/InventoryList.jsx

Line 222 in 7058811

warningMessage={i18n._(

Once we upgrade Lingui we will adopt <Plural/>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexSCorey, I saw this syntax. And I attempted to use it. But the string reported during unit-tests was not sane. I will take a better look if I missed something.

isJobRunning(item.status) ||
!item.summary_fields.user_capabilities.delete
}
errorMessage={i18n.plural({
value: cannotDeleteItems.length,
one:
'The selected job cannot be deleted due to insufficient permission or a running job status',
other:
'The selected jobs cannot be deleted due to insufficient permissions or a running job status',
})}
/>,
<JobListCancelButton
key="cancel"
Expand Down
73 changes: 71 additions & 2 deletions awx/ui_next/src/components/JobList/JobList.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ const mockResults = [
];

UnifiedJobsAPI.read.mockResolvedValue({
data: { count: 3, results: mockResults },
data: { count: 6, results: mockResults },
});

UnifiedJobsAPI.readOptions.mockResolvedValue({
Expand All @@ -141,6 +141,9 @@ function waitForLoaded(wrapper) {
describe('<JobList />', () => {
let debug;
beforeEach(() => {
UnifiedJobsAPI.read.mockResolvedValue({
data: { count: 6, results: mockResults },
});
debug = global.console.debug; // eslint-disable-line prefer-destructuring
global.console.debug = () => {};
});
Expand Down Expand Up @@ -262,6 +265,72 @@ describe('<JobList />', () => {
jest.restoreAllMocks();
});

test('should display message about job running status', async () => {
UnifiedJobsAPI.read.mockResolvedValue({
data: {
count: 2,
results: [
{
id: 1,
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,
start: true,
},
},
},
{
id: 2,
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,
start: true,
},
},
},
],
},
});

let wrapper;
await act(async () => {
wrapper = mountWithContexts(<JobList />);
});
await waitForLoaded(wrapper);

act(() => {
wrapper.find('DataListToolbar').invoke('onSelectAll')(true);
});
wrapper.update();
wrapper.find('JobListItem');
expect(
wrapper.find('ToolbarDeleteButton').prop('itemsToDelete')
).toHaveLength(2);

wrapper.update();

const deleteButton = wrapper.find('ToolbarDeleteButton');
expect(deleteButton.find('Tooltip').prop('content').props.children).toEqual(
'The selected jobs cannot be deleted due to insufficient permissions or a running job status: job 1, job 2'
);

jest.restoreAllMocks();
});

test('error is shown when job not successfully deleted from api', async () => {
JobsAPI.destroy.mockImplementation(() => {
throw new Error({
Expand Down Expand Up @@ -298,7 +367,7 @@ describe('<JobList />', () => {
);
});

test('should send all corresponding delete API requests', async () => {
test('should send all corresponding cancel API requests', async () => {
JobsAPI.cancel = jest.fn();
let wrapper;
await act(async () => {
Expand Down