From eb2b8929efd95db361ad976f6789fe542d242611 Mon Sep 17 00:00:00 2001 From: Bernhard Suttner Date: Thu, 25 Jun 2026 13:39:51 +0200 Subject: [PATCH] Fixes #39463 - Handle host bulk action errors consistently Introduce shared error handling for host bulk actions to support different API and network error formats. Added tests for generic 'bulkErrorToastParams' and 'buildBulkRequestBody' methods and BulkPowerStateModal Co-authored-by: Codex --- .../assignTaxonomy/BulkAssignTaxonomyModal.js | 18 +- .../buildHosts/BulkBuildHostModal.js | 10 +- .../changeOwner/BulkChangeOwnerModal.js | 13 +- .../disassociate/BulkDisassociateModal.js | 13 +- .../HostsIndex/BulkActions/helpers.js | 13 ++ .../HostsIndex/BulkActions/helpers.test.js | 142 +++++++++++++++ .../BulkManageNotificationsModal.js | 16 +- .../powerState/BulkPowerStateModal.js | 11 +- .../powerState/BulkPowerStateModal.test.js | 172 ++++++++++++++++++ .../BulkReassignHostgroupModal.js | 11 +- 10 files changed, 358 insertions(+), 61 deletions(-) create mode 100644 webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/helpers.test.js create mode 100644 webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/powerState/BulkPowerStateModal.test.js diff --git a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/assignTaxonomy/BulkAssignTaxonomyModal.js b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/assignTaxonomy/BulkAssignTaxonomyModal.js index 562d770e31..052b9df990 100644 --- a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/assignTaxonomy/BulkAssignTaxonomyModal.js +++ b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/assignTaxonomy/BulkAssignTaxonomyModal.js @@ -12,6 +12,7 @@ import { TreeView, } from '@patternfly/react-core'; import { addToast } from '../../../ToastsList/slice'; +import { buildBulkRequestBody, bulkErrorToastParams } from '../helpers'; import { translate as __ } from '../../../../common/I18n'; import { STATUS } from '../../../../constants'; import { @@ -32,7 +33,6 @@ import { MODAL_TYPES, } from './BulkAssignTaxonomyConstants'; import TaxonomySelect from './TaxonomySelect'; -import { buildBulkRequestBody } from '../helpers'; export const BulkAssignOrganizationModal = props => ( @@ -53,6 +53,9 @@ const BulkAssignTaxonomyModal = ({ onSuccess: onSuccessCallback, }) => { const org = modalType === MODAL_TYPES.ORGANIZATION; + const actionKey = org + ? BULK_ASSIGN_ORGANIZATION_KEY + : BULK_ASSIGN_LOCATION_KEY; const taxType = org ? 'organization' : 'location'; const dispatch = useDispatch(); const [taxId, setTaxId] = useState(''); @@ -69,9 +72,7 @@ const BulkAssignTaxonomyModal = ({ : selectAPIStatus(state, LOCATION_KEY) ); const hostUpdateStatus = useSelector(state => - org - ? selectAPIStatus(state, BULK_ASSIGN_ORGANIZATION_KEY) - : selectAPIStatus(state, BULK_ASSIGN_LOCATION_KEY) + selectAPIStatus(state, actionKey) ); const handleModalClose = () => { setTaxId(''); @@ -106,15 +107,8 @@ const BulkAssignTaxonomyModal = ({ taxonomy.results.find(t => t.id === id)?.name; const handleError = error => { - const { - response: { - data: { - error: { message }, - }, - }, - } = error; - dispatch(addToast({ type: 'danger', message })); handleModalClose(); + dispatch(addToast(bulkErrorToastParams(error, actionKey))); }; const handleSuccess = response => { diff --git a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/buildHosts/BulkBuildHostModal.js b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/buildHosts/BulkBuildHostModal.js index eea9df3c7e..e77df2e37d 100644 --- a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/buildHosts/BulkBuildHostModal.js +++ b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/buildHosts/BulkBuildHostModal.js @@ -12,7 +12,7 @@ import { } from '@patternfly/react-core'; import { addToast } from '../../../ToastsList/slice'; import { translate as __ } from '../../../../common/I18n'; -import { buildBulkRequestBody, failedHostsToastParams } from '../helpers'; +import { buildBulkRequestBody, bulkErrorToastParams } from '../helpers'; import { STATUS } from '../../../../constants'; import { selectAPIStatus } from '../../../../redux/API/APISelectors'; import { bulkBuildHosts, HOST_BUILD_KEY } from './actions'; @@ -37,13 +37,9 @@ const BulkBuildHostModal = ({ closeModal(); }; - const handleError = ({ response }) => { + const handleError = error => { handleModalClose(); - dispatch( - addToast( - failedHostsToastParams({ ...response.data.error, key: HOST_BUILD_KEY }) - ) - ); + dispatch(addToast(bulkErrorToastParams(error, HOST_BUILD_KEY))); }; const handleSave = () => { const requestBody = buildBulkRequestBody({ diff --git a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/changeOwner/BulkChangeOwnerModal.js b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/changeOwner/BulkChangeOwnerModal.js index d5f202cea9..e9698e9130 100644 --- a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/changeOwner/BulkChangeOwnerModal.js +++ b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/changeOwner/BulkChangeOwnerModal.js @@ -28,7 +28,7 @@ import { selectAPIStatus, selectAPIResponse, } from '../../../../redux/API/APISelectors'; -import { buildBulkRequestBody, failedHostsToastParams } from '../helpers'; +import { buildBulkRequestBody, bulkErrorToastParams } from '../helpers'; const BulkChangeOwnerModal = ({ isOpen, @@ -99,16 +99,9 @@ const BulkChangeOwnerModal = ({ closeModal(); }; - const handleError = response => { + const handleError = error => { handleModalClose(); - dispatch( - addToast( - failedHostsToastParams({ - ...response.data.error, - key: BULK_CHANGE_OWNER_KEY, - }) - ) - ); + dispatch(addToast(bulkErrorToastParams(error, BULK_CHANGE_OWNER_KEY))); }; const handleSuccess = response => { diff --git a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/disassociate/BulkDisassociateModal.js b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/disassociate/BulkDisassociateModal.js index cc29187049..4bab86de55 100644 --- a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/disassociate/BulkDisassociateModal.js +++ b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/disassociate/BulkDisassociateModal.js @@ -12,7 +12,7 @@ import { import { addToast } from '../../../ToastsList/slice'; import { translate as __ } from '../../../../common/I18n'; import { BULK_DISASSOCIATE_KEY, bulkDisassociate } from './actions'; -import { buildBulkRequestBody, failedHostsToastParams } from '../helpers'; +import { buildBulkRequestBody, bulkErrorToastParams } from '../helpers'; const BulkDisassociateModal = ({ isOpen, @@ -56,16 +56,9 @@ const BulkDisassociateModal = ({ }, ]; - const handleError = response => { + const handleError = error => { closeModal(); - dispatch( - addToast( - failedHostsToastParams({ - ...response.data.error, - key: BULK_DISASSOCIATE_KEY, - }) - ) - ); + dispatch(addToast(bulkErrorToastParams(error, BULK_DISASSOCIATE_KEY))); }; const handleSuccess = response => { diff --git a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/helpers.js b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/helpers.js index e51c1ba8cc..def937cff8 100644 --- a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/helpers.js +++ b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/helpers.js @@ -52,3 +52,16 @@ export const failedHostsToastParams = ({ return toastParams; }; + +export const bulkErrorToastParams = (error, key) => { + const fallback = error?.message || __('Unexpected error occurred.'); + const apiError = error?.response?.data?.error; + const isObject = apiError && typeof apiError === 'object'; + const message = isObject ? apiError.message : apiError; + + return failedHostsToastParams({ + message: message || fallback, + failed_host_ids: isObject ? apiError.failed_host_ids : undefined, + key, + }); +}; diff --git a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/helpers.test.js b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/helpers.test.js new file mode 100644 index 0000000000..50ec714f33 --- /dev/null +++ b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/helpers.test.js @@ -0,0 +1,142 @@ +import { buildBulkRequestBody, bulkErrorToastParams } from './helpers'; + +jest.mock('../../../common/I18n'); + +describe('BulkActions helpers', () => { + describe('bulkErrorToastParams', () => { + it('uses API error details from the response', () => { + const toast = bulkErrorToastParams( + { + response: { + data: { + error: { + message: 'Some hosts failed', + failed_host_ids: [1, 2], + }, + }, + }, + }, + 'BULK_ACTION_KEY' + ); + + expect(toast).toMatchObject({ + type: 'danger', + message: 'Some hosts failed', + }); + expect(toast.link.children).toBe('Failed hosts'); + expect(toast.link.href).toContain('/new/hosts'); + expect(toast.link.href).toContain('search='); + }); + + it('uses the error message when no response exists', () => { + const toast = bulkErrorToastParams( + { message: 'Network Error' }, + 'BULK_ACTION_KEY' + ); + + expect(toast).toMatchObject({ + type: 'danger', + message: 'Network Error', + }); + expect(toast.link).toBeUndefined(); + }); + + it('uses a response error string as the message', () => { + const toast = bulkErrorToastParams( + { + response: { + data: { + error: 'Bulk action failed', + }, + }, + }, + 'BULK_ACTION_KEY' + ); + + expect(toast).toMatchObject({ + type: 'danger', + message: 'Bulk action failed', + }); + expect(toast.link).toBeUndefined(); + }); + + it('falls back to a generic message when no usable message exists', () => { + const toast = bulkErrorToastParams({}, 'BULK_ACTION_KEY'); + + expect(toast).toMatchObject({ + type: 'danger', + message: 'Unexpected error occurred.', + }); + expect(toast.link).toBeUndefined(); + }); + + it('keeps failed hosts link when response error has no message', () => { + const toast = bulkErrorToastParams( + { + response: { + data: { + error: { + failed_host_ids: [1, 2], + }, + }, + }, + }, + 'BULK_ACTION_KEY' + ); + + expect(toast).toMatchObject({ + type: 'danger', + message: 'Unexpected error occurred.', + }); + expect(toast.link.children).toBe('Failed hosts'); + }); + }); + + describe('buildBulkRequestBody', () => { + it('builds an included search from fetchBulkParams', () => { + const body = buildBulkRequestBody({ + fetchBulkParams: () => 'id ^ (1,2,3)', + reboot: true, + }); + + expect(body).toEqual({ + included: { + search: 'id ^ (1,2,3)', + }, + reboot: true, + }); + }); + + it('adds taxonomy params when present', () => { + const body = buildBulkRequestBody({ + fetchBulkParams: () => 'id ^ (1,2,3)', + organizationId: 1, + locationId: 2, + }); + + expect(body).toMatchObject({ + included: { + search: 'id ^ (1,2,3)', + }, + organization_id: 1, + location_id: 2, + }); + }); + + it('uses includedSearch instead of calling fetchBulkParams', () => { + const fetchBulkParams = jest.fn(); + + const body = buildBulkRequestBody({ + fetchBulkParams, + includedSearch: 'name ~ test', + }); + + expect(body).toEqual({ + included: { + search: 'name ~ test', + }, + }); + expect(fetchBulkParams).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/manageNotifications/BulkManageNotificationsModal.js b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/manageNotifications/BulkManageNotificationsModal.js index 373fd54ea1..277af61163 100644 --- a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/manageNotifications/BulkManageNotificationsModal.js +++ b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/manageNotifications/BulkManageNotificationsModal.js @@ -15,7 +15,7 @@ import { FormattedMessage } from 'react-intl'; import { translate as __ } from '../../../../common/I18n'; import { bulkManageNotifications } from './actions'; import { BULK_MANAGE_NOTIFICATIONS_KEY } from './constants'; -import { failedHostsToastParams } from '../helpers'; +import { bulkErrorToastParams } from '../helpers'; import { addToast } from '../../../ToastsList/slice'; const BulkManageNotificationsModal = ({ @@ -48,17 +48,9 @@ const BulkManageNotificationsModal = ({ }; const handleError = error => { - const apiError = error?.response?.data?.error; - if (apiError) { - dispatch( - addToast( - failedHostsToastParams({ - ...apiError, - key: BULK_MANAGE_NOTIFICATIONS_KEY, - }) - ) - ); - } + dispatch( + addToast(bulkErrorToastParams(error, BULK_MANAGE_NOTIFICATIONS_KEY)) + ); handleModalClose(); }; diff --git a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/powerState/BulkPowerStateModal.js b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/powerState/BulkPowerStateModal.js index c038155e64..61e852f98c 100644 --- a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/powerState/BulkPowerStateModal.js +++ b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/powerState/BulkPowerStateModal.js @@ -19,7 +19,11 @@ import './BulkPowerStateModal.scss'; import { HostsPowerRefreshContext } from '../../HostsPowerRefreshContext'; import { POWER_STATES, BULK_POWER_STATE_KEY } from './constants'; import { bulkChangePowerState } from './actions'; -import { buildBulkRequestBody, failedHostsToastParams } from '../helpers'; +import { + buildBulkRequestBody, + failedHostsToastParams, + bulkErrorToastParams, +} from '../helpers'; import { addToast } from '../../../ToastsList/slice'; const BulkPowerStateModal = ({ @@ -61,8 +65,9 @@ const BulkPowerStateModal = ({ const handleError = error => { const apiError = error?.response?.data?.error; + const isObject = apiError && typeof apiError === 'object'; - if (apiError) { + if (isObject) { let enhancedError = apiError; if (apiError.failed_hosts && apiError.failed_hosts.length > 0) { @@ -86,6 +91,8 @@ const BulkPowerStateModal = ({ }) ) ); + } else { + dispatch(addToast(bulkErrorToastParams(error, BULK_POWER_STATE_KEY))); } cleanup(); diff --git a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/powerState/BulkPowerStateModal.test.js b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/powerState/BulkPowerStateModal.test.js new file mode 100644 index 0000000000..b615bbfd8e --- /dev/null +++ b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/powerState/BulkPowerStateModal.test.js @@ -0,0 +1,172 @@ +import React from 'react'; +import { render, screen, fireEvent, act } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { Provider } from 'react-redux'; +import configureMockStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import BulkPowerStateModal from './BulkPowerStateModal'; +import { HostsPowerRefreshContext } from '../../HostsPowerRefreshContext'; +import { bulkChangePowerState } from './actions'; + +jest.mock('../../../../common/I18n'); +jest.mock('./actions'); + +const mockStore = configureMockStore([thunk]); + +const defaultProps = { + selectedHostsCount: 3, + fetchBulkParams: jest.fn(() => 'id ^ (1,2,3)'), + isOpen: true, + closeModal: jest.fn(), +}; + +const renderModal = (props = {}) => { + const store = mockStore({ API: {} }); + render( + + + + + + ); + return store; +}; + +describe('BulkPowerStateModal', () => { + let capturedHandleError; + + beforeEach(() => { + jest.clearAllMocks(); + bulkChangePowerState.mockImplementation( + (_payload, _handleSuccess, handleError) => { + capturedHandleError = handleError; + return { type: 'MOCK_ACTION' }; + } + ); + }); + + const selectAndSubmit = async () => { + await act(async () => { + fireEvent.click(screen.getByText('Select power state')); + }); + await act(async () => { + fireEvent.click(screen.getByText('Start')); + }); + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: /apply/i })); + }); + }; + + it('renders modal with title and power state select', () => { + renderModal(); + expect(screen.getByText('Change power state')).toBeInTheDocument(); + expect(screen.getByText('Select power state')).toBeInTheDocument(); + }); + + it('has Apply disabled until a power state is selected', async () => { + renderModal(); + expect(screen.getByRole('button', { name: /apply/i })).toBeDisabled(); + await act(async () => { + fireEvent.click(screen.getByText('Select power state')); + }); + await act(async () => { + fireEvent.click(screen.getByText('Start')); + }); + expect(screen.getByRole('button', { name: /apply/i })).not.toBeDisabled(); + }); + + it('calls closeModal on Cancel', async () => { + const closeModal = jest.fn(); + renderModal({ closeModal }); + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: /cancel/i })); + }); + expect(closeModal).toHaveBeenCalled(); + }); + + describe('handleError', () => { + it('shows object error as danger toast with failed hosts link', async () => { + const store = renderModal(); + await selectAndSubmit(); + + await act(async () => { + capturedHandleError({ + response: { + data: { + error: { message: 'Some hosts failed', failed_host_ids: [1, 2] }, + }, + }, + }); + }); + + const toast = store + .getActions() + .find(a => a.type === 'toasts/addToast')?.payload.toast; + expect(toast).toMatchObject({ type: 'danger', message: 'Some hosts failed' }); + expect(toast.link).toBeDefined(); + }); + + it('enriches message with provider errors from failed_hosts', async () => { + const store = renderModal(); + await selectAndSubmit(); + + await act(async () => { + capturedHandleError({ + response: { + data: { + error: { + message: 'Partial failure.', + failed_hosts: [ + { id: 1, error: 'Provider A failed' }, + { id: 2, error: 'Provider B failed' }, + ], + }, + }, + }, + }); + }); + + const toast = store + .getActions() + .find(a => a.type === 'toasts/addToast')?.payload.toast; + expect(toast.type).toBe('danger'); + expect(toast.message).toContain('Partial failure.'); + expect(toast.message).toContain('Provider A failed'); + expect(toast.message).toContain('Provider B failed'); + }); + + it('handles a string apiError correctly without a link', async () => { + // regression: old code spread a string into failedHostsToastParams, + // losing the message entirely — the isObject guard fixes this + const store = renderModal(); + await selectAndSubmit(); + + await act(async () => { + capturedHandleError({ + response: { data: { error: 'Something went wrong' } }, + }); + }); + + const toast = store + .getActions() + .find(a => a.type === 'toasts/addToast')?.payload.toast; + expect(toast).toMatchObject({ type: 'danger', message: 'Something went wrong' }); + expect(toast.link).toBeUndefined(); + }); + + it('falls back to error.message for network errors without a response', async () => { + const store = renderModal(); + await selectAndSubmit(); + + await act(async () => { + capturedHandleError({ message: 'Network Error' }); + }); + + const toast = store + .getActions() + .find(a => a.type === 'toasts/addToast')?.payload.toast; + expect(toast).toMatchObject({ type: 'danger', message: 'Network Error' }); + expect(toast.link).toBeUndefined(); + }); + }); +}); diff --git a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/reassignHostGroup/BulkReassignHostgroupModal.js b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/reassignHostGroup/BulkReassignHostgroupModal.js index 42d1ee7895..7867564566 100644 --- a/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/reassignHostGroup/BulkReassignHostgroupModal.js +++ b/webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/reassignHostGroup/BulkReassignHostgroupModal.js @@ -11,7 +11,7 @@ import { } from '@patternfly/react-core'; import { addToast } from '../../../ToastsList/slice'; import { translate as __ } from '../../../../common/I18n'; -import { buildBulkRequestBody, failedHostsToastParams } from '../helpers'; +import { buildBulkRequestBody, bulkErrorToastParams } from '../helpers'; import { STATUS } from '../../../../constants'; import { selectAPIStatus, @@ -120,15 +120,10 @@ const BulkReassignHostgroupModal = ({ dispatch(fetchHostgroups()); }, [dispatch]); - const handleError = response => { + const handleError = error => { handleModalClose(); dispatch( - addToast( - failedHostsToastParams({ - ...response.data.error, - key: BULK_REASSIGN_HOSTGROUP_KEY, - }) - ) + addToast(bulkErrorToastParams(error, BULK_REASSIGN_HOSTGROUP_KEY)) ); };