From 1fd94bbf23110b2751e9f8bdc57d0dca6a721407 Mon Sep 17 00:00:00 2001 From: Andrei Lakatos Date: Wed, 20 May 2026 12:51:38 -0400 Subject: [PATCH 01/15] Fixes #39339 - use React component for TaskDetails page rendering --- .../foreman_tasks/api/tasks_controller.rb | 2 +- .../foreman_tasks/tasks_controller.rb | 29 ++- app/views/foreman_tasks/tasks/show.html.erb | 18 -- config/routes.rb | 5 +- test/controllers/tasks_controller_test.rb | 10 +- .../TaskDetails/Components/TaskInfo.js | 46 +---- .../Components/TaskDetails/TaskDetails.js | 2 +- .../Components/common/taskResultIcon.js | 52 +++++ .../Routes/ShowTaskDetails/TaskDetailsPage.js | 68 +++++++ .../__tests__/TaskDetailsPage.test.js | 183 ++++++++++++++++++ webpack/Routes/routes.js | 6 + webpack/Routes/routes.test.js | 39 +++- 12 files changed, 387 insertions(+), 73 deletions(-) delete mode 100644 app/views/foreman_tasks/tasks/show.html.erb create mode 100644 webpack/ForemanTasks/Components/common/taskResultIcon.js create mode 100644 webpack/ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage.js create mode 100644 webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js diff --git a/app/controllers/foreman_tasks/api/tasks_controller.rb b/app/controllers/foreman_tasks/api/tasks_controller.rb index 716780205..99122f138 100644 --- a/app/controllers/foreman_tasks/api/tasks_controller.rb +++ b/app/controllers/foreman_tasks/api/tasks_controller.rb @@ -324,7 +324,7 @@ def controller_permission def action_permission case params[:action] - when 'bulk_search', 'summary', 'details', 'sub_tasks' + when 'bulk_search', 'summary', 'show', 'details', 'sub_tasks' :view when 'bulk_resume', 'bulk_cancel', 'bulk_stop' :edit diff --git a/app/controllers/foreman_tasks/tasks_controller.rb b/app/controllers/foreman_tasks/tasks_controller.rb index 678495a8b..9a3e79348 100644 --- a/app/controllers/foreman_tasks/tasks_controller.rb +++ b/app/controllers/foreman_tasks/tasks_controller.rb @@ -7,8 +7,19 @@ class TasksController < ::ApplicationController before_action :find_dynflow_task, only: [:unlock, :force_unlock, :cancel, :abort, :cancel_step, :resume] def show - @task = resource_base.find(params[:id]) - render :layout => !request.xhr? + @task = task_detail_scope.find(params[:id]) + + respond_to do |format| + format.html do + response.headers['X-Request-Path'] = request.path + + render('react/index', :layout => 'layouts/react_application', :formats => [:html]) + end + + format.csv do + respond_with_tasks(Task.where(:id => @task.id)) + end + end end def index @@ -107,7 +118,7 @@ def controller_permission def action_permission case params[:action] - when 'sub_tasks', 'summary', 'summary_sub_tasks' + when 'show', 'sub_tasks', 'summary', 'summary_sub_tasks' :view when 'resume', 'unlock', 'force_unlock', 'cancel_step', 'cancel', 'abort' :edit @@ -131,5 +142,17 @@ def filter(scope, paginate: true) scope = scope.paginate(page: params[:page], per_page: params[:per_page]) if paginate scope.distinct end + + # Same scoped lookup as SPA entry (authorize + taxonomy + explicit search params). + def task_detail_scope + scope = ForemanTasks::Task.authorized('view_foreman_tasks') + sq = search_query + + if sq.blank? + scope + else + scope.search_for(sq) + end + end end end diff --git a/app/views/foreman_tasks/tasks/show.html.erb b/app/views/foreman_tasks/tasks/show.html.erb deleted file mode 100644 index ad454da05..000000000 --- a/app/views/foreman_tasks/tasks/show.html.erb +++ /dev/null @@ -1,18 +0,0 @@ -<% stylesheet 'foreman_tasks/foreman_tasks' %> -<% content_for(:javascripts) do %> - <%= webpacked_plugins_js_for :'foreman-tasks' %> -<% end %> -<% content_for(:stylesheets) do %> - <%= webpacked_plugins_css_for :'foreman-tasks' %> -<% end %> - -<% title _("Details of %s task") % @task.to_s %> - -<%= breadcrumbs( - items: breadcrumb_items, - name_field: 'action', - resource_url: foreman_tasks_api_tasks_path, - switcher_item_url: foreman_tasks_task_path(:id => ':id') -) %> - -<%= react_component('TaskDetails') %> diff --git a/config/routes.rb b/config/routes.rb index eb44fdc71..363f65e87 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -12,7 +12,7 @@ end end - resources :tasks, :only => [:show] do + resources :tasks, :only => [:index] do collection do get 'auto_complete_search' get '/summary/:recent_timeframe', action: 'summary' @@ -35,6 +35,7 @@ resources :tasks, :only => [:index], constraints: ->(req) { req.format == :csv } match '/tasks', to: '/react#index', via: :get + get '/tasks/:id', :to => 'tasks#show', :defaults => { :format => 'html' } match '/tasks/:id/sub_tasks', to: '/react#index', via: :get namespace :api do @@ -47,7 +48,7 @@ end end - resources :tasks, :only => [:show, :index] do + resources :tasks, :only => [:index, :show] do member do get :details end diff --git a/test/controllers/tasks_controller_test.rb b/test/controllers/tasks_controller_test.rb index dd3c684ba..237719fef 100644 --- a/test/controllers/tasks_controller_test.rb +++ b/test/controllers/tasks_controller_test.rb @@ -83,12 +83,20 @@ def in_taxonomy_scope(organization, location = nil) end describe 'show' do - it 'does not allow user without permissions to see task details' do + it 'does not allow user without permissions to see task details page' do setup_user('view', 'foreman_tasks', 'owner.id = current_user') get :show, params: { id: FactoryBot.create(:some_task).id }, session: set_session_user(User.current) assert_response :not_found end + + it 'serves react shell when user may view the task' do + task = FactoryBot.create(:some_task) + get :show, params: { id: task.id }, session: set_session_user + + assert_response :success + assert_includes @response.headers['X-Request-Path'].to_s, task.id.to_s + end end describe 'index' do diff --git a/webpack/ForemanTasks/Components/TaskDetails/Components/TaskInfo.js b/webpack/ForemanTasks/Components/TaskDetails/Components/TaskInfo.js index a206c1043..3c4a6145b 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/Components/TaskInfo.js +++ b/webpack/ForemanTasks/Components/TaskDetails/Components/TaskInfo.js @@ -5,17 +5,12 @@ import { GridItem, Progress, ProgressVariant, - Icon, } from '@patternfly/react-core'; -import { - CheckCircleIcon, - ExclamationCircleIcon, - ExclamationTriangleIcon, - QuestionCircleIcon, -} from '@patternfly/react-icons'; import { translate as __ } from 'foremanReact/common/I18n'; import RelativeDateTime from 'foremanReact/components/common/dates/RelativeDateTime'; +import { taskResultIconEl } from '../../common/taskResultIcon'; + const isDelayed = ({ startAt, startedAt }) => { if ( startAt == null || @@ -35,41 +30,6 @@ const isDelayed = ({ startAt, startedAt }) => { return a.getTime() !== b.getTime(); }; -const resultIconEl = (state, result) => { - if (state !== 'stopped') - return ( - - - - ); - switch (result) { - case 'success': - return ( - - - - ); - case 'error': - return ( - - - - ); - case 'warning': - return ( - - - - ); - default: - return ( - - - - ); - } -}; - const progressVariantForResult = result => { switch (result) { case 'error': @@ -117,7 +77,7 @@ const TaskInfo = props => { title: 'Result', value: ( - {resultIconEl(state, result)} {result} + {taskResultIconEl(state, result)} {result} ), }, diff --git a/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js b/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js index 7a14446bd..212a4a8ae 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js +++ b/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js @@ -72,7 +72,7 @@ const TaskDetails = ({ }; return ( -
+
{ + if (state !== 'stopped') { + return ( + + + + ); + } + + switch (result) { + case 'success': + return ( + + + + ); + case 'error': + return ( + + + + ); + case 'warning': + return ( + + + + ); + default: + return ( + + + + ); + } +}; diff --git a/webpack/ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage.js b/webpack/ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage.js new file mode 100644 index 000000000..2317b90b5 --- /dev/null +++ b/webpack/ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage.js @@ -0,0 +1,68 @@ +import React from 'react'; +import PropTypes from 'prop-types'; +import { useSelector } from 'react-redux'; +import { Flex, FlexItem, TextContent, Text } from '@patternfly/react-core'; +import PageLayout from 'foremanReact/routes/common/PageLayout/PageLayout'; +import { translate as __, sprintf } from 'foremanReact/common/I18n'; +import TaskDetails from '../../Components/TaskDetails'; +import { + selectAction, + selectResult, + selectState, +} from '../../Components/TaskDetails/TaskDetailsSelectors'; +import { taskResultIconEl } from '../../Components/common/taskResultIcon'; + +const TaskDetailsPage = props => { + const { id } = props.match.params; + const action = useSelector(selectAction); + const taskState = useSelector(selectState); + const taskResult = useSelector(selectResult); + const headerText = action + ? sprintf(__('Details of %s task'), action) + : __('Task details'); + + const header = ( + + + + {headerText} + + + + {taskResultIconEl(taskState, taskResult)} + + + ); + + return ( + + + + ); +}; + +TaskDetailsPage.propTypes = { + match: PropTypes.shape({ + params: PropTypes.shape({ + id: PropTypes.string.isRequired, + }).isRequired, + }).isRequired, +}; + +export default TaskDetailsPage; diff --git a/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js b/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js new file mode 100644 index 000000000..75f1367db --- /dev/null +++ b/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js @@ -0,0 +1,183 @@ +import React from 'react'; +import { render, screen, within } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { combineReducers, configureStore } from '@reduxjs/toolkit'; +import { IntlProvider } from 'react-intl'; +import { Provider } from 'react-redux'; + +import breadcrumbBarReducer from 'foremanReact/components/BreadcrumbBar/BreadcrumbBarReducer'; +import { STATUS } from 'foremanReact/constants'; +import intervalsReducer from 'foremanReact/redux/middlewares/IntervalMiddleware/IntervalReducer'; + +import { FOREMAN_TASK_DETAILS } from '../../../Components/TaskDetails/TaskDetailsConstants'; +import TaskDetailsPage from '../TaskDetailsPage'; + +const routerPropsBase = { + history: { push: jest.fn(), replace: jest.fn(), go: jest.fn() }, + location: { + pathname: '/foreman_tasks/tasks/task-route-id', + search: '', + hash: '', + state: undefined, + }, +}; + +const matchDefault = { + params: { id: 'task-route-id' }, + path: '/foreman_tasks/tasks/:id', + url: '/foreman_tasks/tasks/task-route-id', + isExact: true, +}; + +const baseTaskPayload = { + action: '', + input: [], + output: {}, + locks: [], + links: [], + depends_on: [], + blocks: [], + failed_steps: [], + running_steps: [], + execution_plan: {}, + state: 'running', + result: '', +}; + +const createStoreForTaskPayload = overrides => ({ + API: { + [FOREMAN_TASK_DETAILS]: { + response: { ...baseTaskPayload, ...overrides }, + status: STATUS.RESOLVED, + payload: {}, + }, + }, +}); + +const rootReducer = combineReducers({ + API: (state = {}, action) => state, + intervals: intervalsReducer, + breadcrumbBar: breadcrumbBarReducer, + foremanTasks: (state = {}, action) => state, +}); + +function renderPage(apiPayloadOverrides = {}, propsOverrides = {}) { + const store = configureStore({ + reducer: rootReducer, + preloadedState: createStoreForTaskPayload(apiPayloadOverrides), + middleware: getDefaultMiddleware => + getDefaultMiddleware({ + immutableCheck: false, + serializableCheck: false, + }), + }); + + window.history.pushState( + {}, + '', + `/foreman_tasks/tasks/${matchDefault.params.id}` + ); + + return render( + + + + + + ); +} + +function breadcrumbTitleHeadings() { + return screen.getAllByRole('heading', { level: 1 }).filter( + heading => heading.getAttribute('data-ouia-component-id') === 'breadcrumb_title' + ); +} + +/** Title strip (`customHeader`): `Flex` sets `data-ouia-component-id` (same pattern as Locks). */ +function taskDetailsTitleRegion(container) { + const el = container.querySelector( + '[data-ouia-component-id="foreman-tasks-task-details-title-row"]' + ); + + expect(el).toBeTruthy(); + + return el; +} + +describe('TaskDetailsPage', () => { + afterEach(() => { + window.history.pushState({}, '', '/'); + }); + + const expectToolbarHeadingText = substring => { + const headings = breadcrumbTitleHeadings(); + + expect(headings.length).toBeGreaterThan(0); + + headings.forEach(heading => { + expect(heading).toHaveTextContent(substring); + }); + }; + + it('shows generic title and breadcrumb from route id when action is unset', () => { + const page = renderPage({}); + + expectToolbarHeadingText('Task details'); + + expect(screen.getByRole('navigation', { name: 'Breadcrumb' })).toHaveTextContent( + 'Tasks' + ); + expect( + within(screen.getByRole('navigation', { name: 'Breadcrumb' })).getByText( + /task-route-id/ + ) + ).toBeInTheDocument(); + + const titleRegion = taskDetailsTitleRegion(page.container); + + expect( + within(titleRegion).getAllByRole('img', { hidden: true }).length + ).toBeGreaterThan(0); + expect(titleRegion.querySelector('[class*="danger"]')).toBeNull(); + }); + + it('uses task action for title and breadcrumb when loaded', () => { + renderPage({ action: 'Refresh hosts' }); + + expectToolbarHeadingText('Details of Refresh hosts task'); + + expect(screen.getByRole('link', { name: /^Tasks$/ })).toHaveAttribute( + 'href', + '/foreman_tasks/tasks' + ); + expect( + within(screen.getByRole('navigation', { name: 'Breadcrumb' })).getByText( + 'Refresh hosts' + ) + ).toBeInTheDocument(); + }); + + it('shows error status styling in the heading when task is stopped with error result', () => { + const page =renderPage({ + action: 'Some action', + state: 'stopped', + result: 'error', + }); + + expectToolbarHeadingText('Details of Some action task'); + + expect( + taskDetailsTitleRegion(page.container).querySelector('[class*="danger"]') + ).toBeTruthy(); + + expect( + within(screen.getByRole('navigation', { name: 'Breadcrumb' })).getByText( + 'Some action' + ) + ).toBeInTheDocument(); + }); +}); diff --git a/webpack/Routes/routes.js b/webpack/Routes/routes.js index 01c9f9301..c5e786412 100644 --- a/webpack/Routes/routes.js +++ b/webpack/Routes/routes.js @@ -1,5 +1,6 @@ import React from 'react'; import TasksTableIndexPage from '../ForemanTasks/Components/TasksTable/TasksIndexPage'; +import TaskDetailsPage from '../ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage'; const ForemanTasksRoutes = [ { @@ -7,6 +8,11 @@ const ForemanTasksRoutes = [ exact: true, render: props => , }, + { + path: '/foreman_tasks/tasks/:id', + exact: true, + render: props => , + }, { path: '/foreman_tasks/tasks/:id/sub_tasks', exact: true, diff --git a/webpack/Routes/routes.test.js b/webpack/Routes/routes.test.js index 44877a679..da6aa184c 100644 --- a/webpack/Routes/routes.test.js +++ b/webpack/Routes/routes.test.js @@ -12,6 +12,14 @@ jest.mock( } ); +jest.mock( + '../ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage', + () => + function TaskDetailsPageStub() { + return
; + } +); + const routerProps = { history: { push: jest.fn(), replace: jest.fn(), go: jest.fn() }, location: { @@ -34,6 +42,7 @@ describe('ForemanTasks routes', () => { ForemanTasksRoutes.map(({ path, exact }) => ({ path, exact })) ).toEqual([ { path: '/foreman_tasks/tasks', exact: true }, + { path: '/foreman_tasks/tasks/:id', exact: true }, { path: '/foreman_tasks/tasks/:id/sub_tasks', exact: true }, ]); }); @@ -53,9 +62,27 @@ describe('ForemanTasks routes', () => { ...routerProps, match: { ...routerProps.match, - params: { id: '7' }, - path: '/foreman_tasks/tasks/:id/sub_tasks', - url: '/foreman_tasks/tasks/7/sub_tasks', + params: { id: '99' }, + path: '/foreman_tasks/tasks/:id', + url: '/foreman_tasks/tasks/99', + }, + }, + { + ...routerProps, + match: { + ...routerProps.match, + params: { id: '99' }, + path: '/foreman_tasks/tasks/:id', + url: '/foreman_tasks/tasks/99', + }, + }, + { + ...routerProps, + match: { + ...routerProps.match, + params: { id: '42' }, + path: '/foreman_tasks/ex_tasks/:id', + url: '/foreman_tasks/ex_tasks/42', }, }, ]; @@ -63,7 +90,11 @@ describe('ForemanTasks routes', () => { ForemanTasksRoutes.forEach((route, index) => { const { unmount } = render(route.render(propsByIndex[index])); - if (index === 2) { + if (index === 1) { + expect( + screen.getByTestId('task-details-page-stub') + ).toBeInTheDocument(); + } else if (index === 3) { expect(screen.getByTestId('show-task-stub')).toBeInTheDocument(); } else { expect( From e12bba0b2a3414bebdbd68d769879eec1fc35cf1 Mon Sep 17 00:00:00 2001 From: Andrei Lakatos Date: Thu, 21 May 2026 06:08:27 -0400 Subject: [PATCH 02/15] Refs #39339 - revert ruby changes, remove unnecessary checks, fix tests --- .../foreman_tasks/api/tasks_controller.rb | 2 +- app/controllers/foreman_tasks/tasks_controller.rb | 12 ++---------- config/routes.rb | 2 +- test/controllers/tasks_controller_test.rb | 2 +- .../__tests__/TaskDetailsPage.test.js | 11 +++++++---- 5 files changed, 12 insertions(+), 17 deletions(-) diff --git a/app/controllers/foreman_tasks/api/tasks_controller.rb b/app/controllers/foreman_tasks/api/tasks_controller.rb index 99122f138..716780205 100644 --- a/app/controllers/foreman_tasks/api/tasks_controller.rb +++ b/app/controllers/foreman_tasks/api/tasks_controller.rb @@ -324,7 +324,7 @@ def controller_permission def action_permission case params[:action] - when 'bulk_search', 'summary', 'show', 'details', 'sub_tasks' + when 'bulk_search', 'summary', 'details', 'sub_tasks' :view when 'bulk_resume', 'bulk_cancel', 'bulk_stop' :edit diff --git a/app/controllers/foreman_tasks/tasks_controller.rb b/app/controllers/foreman_tasks/tasks_controller.rb index 9a3e79348..ebf23b107 100644 --- a/app/controllers/foreman_tasks/tasks_controller.rb +++ b/app/controllers/foreman_tasks/tasks_controller.rb @@ -11,8 +11,6 @@ def show respond_to do |format| format.html do - response.headers['X-Request-Path'] = request.path - render('react/index', :layout => 'layouts/react_application', :formats => [:html]) end @@ -145,14 +143,8 @@ def filter(scope, paginate: true) # Same scoped lookup as SPA entry (authorize + taxonomy + explicit search params). def task_detail_scope - scope = ForemanTasks::Task.authorized('view_foreman_tasks') - sq = search_query - - if sq.blank? - scope - else - scope.search_for(sq) - end + ForemanTasks::Task.authorized('view_foreman_tasks') + .search_for(search_query) end end end diff --git a/config/routes.rb b/config/routes.rb index 363f65e87..a670fcee1 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -48,7 +48,7 @@ end end - resources :tasks, :only => [:index, :show] do + resources :tasks, :only => [:show, :index] do member do get :details end diff --git a/test/controllers/tasks_controller_test.rb b/test/controllers/tasks_controller_test.rb index 237719fef..1e80e957e 100644 --- a/test/controllers/tasks_controller_test.rb +++ b/test/controllers/tasks_controller_test.rb @@ -95,7 +95,7 @@ def in_taxonomy_scope(organization, location = nil) get :show, params: { id: task.id }, session: set_session_user assert_response :success - assert_includes @response.headers['X-Request-Path'].to_s, task.id.to_s + assert_includes @response.body, '/webpack/' end end diff --git a/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js b/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js index 75f1367db..5015c257b 100644 --- a/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js +++ b/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js @@ -12,6 +12,8 @@ import intervalsReducer from 'foremanReact/redux/middlewares/IntervalMiddleware/ import { FOREMAN_TASK_DETAILS } from '../../../Components/TaskDetails/TaskDetailsConstants'; import TaskDetailsPage from '../TaskDetailsPage'; +const TASK_DETAILS_TITLE_ROW_OUIA_ID = 'foreman-tasks-task-details-title-row'; + const routerPropsBase = { history: { push: jest.fn(), replace: jest.fn(), go: jest.fn() }, location: { @@ -97,10 +99,12 @@ function breadcrumbTitleHeadings() { ); } -/** Title strip (`customHeader`): `Flex` sets `data-ouia-component-id` (same pattern as Locks). */ +/** + * Title row (`customHeader` root `Flex`): same OUIA pattern as `Locks.test.js`. + */ function taskDetailsTitleRegion(container) { const el = container.querySelector( - '[data-ouia-component-id="foreman-tasks-task-details-title-row"]' + `[data-ouia-component-id="${TASK_DETAILS_TITLE_ROW_OUIA_ID}"]` ); expect(el).toBeTruthy(); @@ -125,7 +129,6 @@ describe('TaskDetailsPage', () => { it('shows generic title and breadcrumb from route id when action is unset', () => { const page = renderPage({}); - expectToolbarHeadingText('Task details'); expect(screen.getByRole('navigation', { name: 'Breadcrumb' })).toHaveTextContent( @@ -162,7 +165,7 @@ describe('TaskDetailsPage', () => { }); it('shows error status styling in the heading when task is stopped with error result', () => { - const page =renderPage({ + const page = renderPage({ action: 'Some action', state: 'stopped', result: 'error', From 246969774bc76fff7736f460691c9ea8318c71be Mon Sep 17 00:00:00 2001 From: Andrei Lakatos Date: Thu, 21 May 2026 10:43:13 -0400 Subject: [PATCH 03/15] Refs #39339 - remove task_detail def, add show resource back, use resource_scope for find --- app/controllers/foreman_tasks/tasks_controller.rb | 14 ++------------ config/routes.rb | 2 +- 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/app/controllers/foreman_tasks/tasks_controller.rb b/app/controllers/foreman_tasks/tasks_controller.rb index ebf23b107..08b12f969 100644 --- a/app/controllers/foreman_tasks/tasks_controller.rb +++ b/app/controllers/foreman_tasks/tasks_controller.rb @@ -7,16 +7,12 @@ class TasksController < ::ApplicationController before_action :find_dynflow_task, only: [:unlock, :force_unlock, :cancel, :abort, :cancel_step, :resume] def show - @task = task_detail_scope.find(params[:id]) + @task = resource_scope.find(params[:id]) respond_to do |format| format.html do render('react/index', :layout => 'layouts/react_application', :formats => [:html]) end - - format.csv do - respond_with_tasks(Task.where(:id => @task.id)) - end end end @@ -116,7 +112,7 @@ def controller_permission def action_permission case params[:action] - when 'show', 'sub_tasks', 'summary', 'summary_sub_tasks' + when 'sub_tasks', 'summary', 'summary_sub_tasks' :view when 'resume', 'unlock', 'force_unlock', 'cancel_step', 'cancel', 'abort' :edit @@ -140,11 +136,5 @@ def filter(scope, paginate: true) scope = scope.paginate(page: params[:page], per_page: params[:per_page]) if paginate scope.distinct end - - # Same scoped lookup as SPA entry (authorize + taxonomy + explicit search params). - def task_detail_scope - ForemanTasks::Task.authorized('view_foreman_tasks') - .search_for(search_query) - end end end diff --git a/config/routes.rb b/config/routes.rb index a670fcee1..f011384fa 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -12,7 +12,7 @@ end end - resources :tasks, :only => [:index] do + resources :tasks, :only => [:show] do collection do get 'auto_complete_search' get '/summary/:recent_timeframe', action: 'summary' From 029de8fdf12ed792f61d0253cfc1ea6aaa91f9d5 Mon Sep 17 00:00:00 2001 From: Andrei Lakatos Date: Thu, 21 May 2026 12:36:04 -0400 Subject: [PATCH 04/15] Refs #39339 - use render react for task, simplify task route --- app/controllers/foreman_tasks/tasks_controller.rb | 6 +----- config/routes.rb | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/app/controllers/foreman_tasks/tasks_controller.rb b/app/controllers/foreman_tasks/tasks_controller.rb index 08b12f969..59c001304 100644 --- a/app/controllers/foreman_tasks/tasks_controller.rb +++ b/app/controllers/foreman_tasks/tasks_controller.rb @@ -9,11 +9,7 @@ class TasksController < ::ApplicationController def show @task = resource_scope.find(params[:id]) - respond_to do |format| - format.html do - render('react/index', :layout => 'layouts/react_application', :formats => [:html]) - end - end + render('react/index', :layout => 'layouts/react_application') end def index diff --git a/config/routes.rb b/config/routes.rb index f011384fa..9cbaf468e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -35,7 +35,7 @@ resources :tasks, :only => [:index], constraints: ->(req) { req.format == :csv } match '/tasks', to: '/react#index', via: :get - get '/tasks/:id', :to => 'tasks#show', :defaults => { :format => 'html' } + get '/tasks/:id', :to => 'tasks#show' match '/tasks/:id/sub_tasks', to: '/react#index', via: :get namespace :api do From c551a1fd7cf98f2e43243f13343f844131d41928 Mon Sep 17 00:00:00 2001 From: Andrei Lakatos Date: Tue, 26 May 2026 04:13:13 -0400 Subject: [PATCH 05/15] Refs #39339 - use react exclusively for the path, use Permitted to check permissions --- .../foreman_tasks/tasks_controller.rb | 6 -- config/routes.rb | 6 +- lib/foreman_tasks/engine.rb | 2 +- test/controllers/tasks_controller_test.rb | 17 ----- .../Routes/ShowTaskDetails/TaskDetailsPage.js | 29 ++++---- .../__tests__/TaskDetailsPage.test.js | 68 +++++++++++++++++++ 6 files changed, 88 insertions(+), 40 deletions(-) diff --git a/app/controllers/foreman_tasks/tasks_controller.rb b/app/controllers/foreman_tasks/tasks_controller.rb index 59c001304..3f21aeac1 100644 --- a/app/controllers/foreman_tasks/tasks_controller.rb +++ b/app/controllers/foreman_tasks/tasks_controller.rb @@ -6,12 +6,6 @@ class TasksController < ::ApplicationController before_action :find_dynflow_task, only: [:unlock, :force_unlock, :cancel, :abort, :cancel_step, :resume] - def show - @task = resource_scope.find(params[:id]) - - render('react/index', :layout => 'layouts/react_application') - end - def index params[:order] ||= 'started_at DESC' respond_with_tasks resource_base diff --git a/config/routes.rb b/config/routes.rb index 9cbaf468e..dbac28a98 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -12,7 +12,7 @@ end end - resources :tasks, :only => [:show] do + resources :tasks, :only => [] do collection do get 'auto_complete_search' get '/summary/:recent_timeframe', action: 'summary' @@ -27,7 +27,7 @@ post :cancel_step end end - resources :tasks, :only => [:show], constraints: ->(req) { req.format == :csv } do + resources :tasks, :only => [], constraints: ->(req) { req.format == :csv } do member do get :sub_tasks end @@ -35,7 +35,7 @@ resources :tasks, :only => [:index], constraints: ->(req) { req.format == :csv } match '/tasks', to: '/react#index', via: :get - get '/tasks/:id', :to => 'tasks#show' + match '/tasks/:id', to: '/react#index', via: :get, :as => :task match '/tasks/:id/sub_tasks', to: '/react#index', via: :get namespace :api do diff --git a/lib/foreman_tasks/engine.rb b/lib/foreman_tasks/engine.rb index 07268cf36..aed71a91b 100644 --- a/lib/foreman_tasks/engine.rb +++ b/lib/foreman_tasks/engine.rb @@ -36,7 +36,7 @@ class Engine < ::Rails::Engine :last => true security_block :foreman_tasks do |_map| - permission :view_foreman_tasks, { :'foreman_tasks/tasks' => [:auto_complete_search, :sub_tasks, :index, :summary, :summary_sub_tasks, :show], + permission :view_foreman_tasks, { :'foreman_tasks/tasks' => [:auto_complete_search, :sub_tasks, :index, :summary, :summary_sub_tasks], :'foreman_tasks/api/tasks' => [:bulk_search, :show, :index, :summary, :summary_sub_tasks, :details, :sub_tasks] }, :resource_type => 'ForemanTasks::Task' permission :edit_foreman_tasks, { :'foreman_tasks/tasks' => [:resume, :unlock, :force_unlock, :cancel_step, :cancel, :abort], :'foreman_tasks/api/tasks' => [:bulk_resume, :bulk_cancel, :bulk_stop] }, :resource_type => 'ForemanTasks::Task' diff --git a/test/controllers/tasks_controller_test.rb b/test/controllers/tasks_controller_test.rb index 1e80e957e..a96c9d30d 100644 --- a/test/controllers/tasks_controller_test.rb +++ b/test/controllers/tasks_controller_test.rb @@ -82,23 +82,6 @@ def in_taxonomy_scope(organization, location = nil) assert_include response.body.lines[1], 'Some action' end - describe 'show' do - it 'does not allow user without permissions to see task details page' do - setup_user('view', 'foreman_tasks', 'owner.id = current_user') - get :show, params: { id: FactoryBot.create(:some_task).id }, - session: set_session_user(User.current) - assert_response :not_found - end - - it 'serves react shell when user may view the task' do - task = FactoryBot.create(:some_task) - get :show, params: { id: task.id }, session: set_session_user - - assert_response :success - assert_includes @response.body, '/webpack/' - end - end - describe 'index' do it 'shows duration column' do task = ForemanTasks::Task.select_duration.find(FactoryBot.create(:some_task).id) diff --git a/webpack/ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage.js b/webpack/ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage.js index 2317b90b5..76f815d5d 100644 --- a/webpack/ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage.js +++ b/webpack/ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage.js @@ -3,6 +3,7 @@ import PropTypes from 'prop-types'; import { useSelector } from 'react-redux'; import { Flex, FlexItem, TextContent, Text } from '@patternfly/react-core'; import PageLayout from 'foremanReact/routes/common/PageLayout/PageLayout'; +import Permitted from 'foremanReact/components/Permitted/Permitted'; import { translate as __, sprintf } from 'foremanReact/common/I18n'; import TaskDetails from '../../Components/TaskDetails'; import { @@ -41,19 +42,21 @@ const TaskDetailsPage = props => { ); return ( - - - + + + + + ); }; diff --git a/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js b/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js index 5015c257b..6d483c1a7 100644 --- a/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js +++ b/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js @@ -12,6 +12,15 @@ import intervalsReducer from 'foremanReact/redux/middlewares/IntervalMiddleware/ import { FOREMAN_TASK_DETAILS } from '../../../Components/TaskDetails/TaskDetailsConstants'; import TaskDetailsPage from '../TaskDetailsPage'; +const mockUseForemanPermissions = jest.fn( + () => new Set(['view_foreman_tasks']) +); + +jest.mock('foremanReact/Root/Context/ForemanContext', () => ({ + ...jest.requireActual('foremanReact/Root/Context/ForemanContext'), + useForemanPermissions: (...args) => mockUseForemanPermissions(...args), +})); + const TASK_DETAILS_TITLE_ROW_OUIA_ID = 'foreman-tasks-task-details-title-row'; const routerPropsBase = { @@ -115,8 +124,67 @@ function taskDetailsTitleRegion(container) { describe('TaskDetailsPage', () => { afterEach(() => { window.history.pushState({}, '', '/'); + mockUseForemanPermissions.mockImplementation(() => new Set(['view_foreman_tasks'])); + }); + + beforeEach(() => { + mockUseForemanPermissions.mockImplementation(() => new Set(['view_foreman_tasks'])); + }); + + describe('permissions', () => { + const VIEW_PERM = 'view_foreman_tasks'; + + it(`renders the task details chrome when ${VIEW_PERM} is present`, () => { + mockUseForemanPermissions.mockImplementation( + () => new Set(['edit_foreman_tasks', VIEW_PERM]) + ); + + renderPage({ action: 'Run job' }); + + expect( + screen.getByRole('navigation', { name: 'Breadcrumb' }) + ).toBeInTheDocument(); + expect( + screen.getByRole('heading', { + level: 1, + name: /Details of Run job task/, + }) + ).toBeInTheDocument(); + expect(screen.queryByText('Permission Denied')).not.toBeInTheDocument(); + expect(screen.queryAllByRole('heading', { name: 'Permission Denied' })).toHaveLength(0); + expect(mockUseForemanPermissions).toHaveBeenCalled(); + }); + + it(`shows PermissionDenied and lists ${VIEW_PERM} when it is absent`, () => { + mockUseForemanPermissions.mockImplementation(() => new Set()); + + renderPage({ action: 'Hidden task' }); + + expect(screen.getByText('Permission Denied')).toBeInTheDocument(); + expect(screen.getByRole('heading', { name: /Permission denied/i })).toBeInTheDocument(); + expect(screen.getByText(VIEW_PERM)).toBeInTheDocument(); + expect( + screen.queryByRole('navigation', { name: 'Breadcrumb' }) + ).not.toBeInTheDocument(); + expect(screen.queryAllByRole('heading', { name: /Hidden task/i })).toHaveLength(0); + }); + + it('denies access when user only has edit_foreman_tasks without view', () => { + mockUseForemanPermissions.mockImplementation( + () => new Set(['edit_foreman_tasks']) + ); + + renderPage({}); + + expect(screen.getByText('Permission Denied')).toBeInTheDocument(); + expect(screen.getByText(VIEW_PERM)).toBeInTheDocument(); + expect( + screen.queryByRole('navigation', { name: 'Breadcrumb' }) + ).not.toBeInTheDocument(); + }); }); + const expectToolbarHeadingText = substring => { const headings = breadcrumbTitleHeadings(); From 1289c0ce3672312731635d7a8ddf3440b9ea3b91 Mon Sep 17 00:00:00 2001 From: Andrei Lakatos Date: Wed, 27 May 2026 06:33:18 -0400 Subject: [PATCH 06/15] Refs #39339 - handle api error messages for the taskdetails page --- .../Components/TaskDetails/TaskDetails.js | 43 +++++++--- .../TaskDetails/TaskDetailsSelectors.js | 13 +++- .../TaskDetails/__tests__/TaskDetails.test.js | 78 ++++++++++++++++--- .../Components/TaskDetails/index.js | 8 +- 4 files changed, 117 insertions(+), 25 deletions(-) diff --git a/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js b/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js index 212a4a8ae..e5b9e1ab3 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js +++ b/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js @@ -1,9 +1,11 @@ import React, { useEffect, useState } from 'react'; import PropTypes from 'prop-types'; import { Tabs, Tab, TabTitleText } from '@patternfly/react-core'; -import { translate as __, sprintf } from 'foremanReact/common/I18n'; +import { ExclamationCircleIcon } from '@patternfly/react-icons'; +import { translate as __ } from 'foremanReact/common/I18n'; import { STATUS } from 'foremanReact/constants'; -import MessageBox from 'foremanReact/components/common/MessageBox'; +import EmptyState from 'foremanReact/components/common/EmptyState'; +import PermissionDenied from 'foremanReact/components/PermissionDenied'; import Task from './Components/Task'; import RunningSteps from './Components/RunningSteps'; import Errors from './Components/Errors'; @@ -26,7 +28,9 @@ const TaskDetails = ({ cancelStep, taskReloadStart, taskReloadStop, - APIerror, + apiStatus, + apiErrorMessage, + apiErrorCode, ...props }) => { const id = getTaskID(); @@ -48,15 +52,32 @@ const TaskDetails = ({ } }; - if (status === STATUS.ERROR) { + if (status === STATUS.ERROR || apiStatus === STATUS.ERROR) { + if (apiErrorCode === 403) { + return ; + } + + if (apiErrorCode === 404) { + return ( + } + header={__('Task not found')} + description={ + apiErrorMessage || __('The requested task could not be found.') + } + /> + ); + } + return ( - } + header={__('Error')} + description={apiErrorMessage || __('Could not receive task data.')} /> ); } + const resumable = executionPlan ? executionPlan.state === 'paused' : false; const cancellable = executionPlan ? executionPlan.cancellable : false; const lockRecords = locks.concat(links); @@ -160,7 +181,9 @@ TaskDetails.propTypes = { cancelStep: PropTypes.func.isRequired, taskReload: PropTypes.bool.isRequired, status: PropTypes.oneOf(Object.keys(STATUS)), - APIerror: PropTypes.object, + apiStatus: PropTypes.oneOf(Object.keys(STATUS)), + apiErrorMessage: PropTypes.string, + apiErrorCode: PropTypes.number, taskReloadStop: PropTypes.func.isRequired, taskReloadStart: PropTypes.func.isRequired, links: PropTypes.array, @@ -175,7 +198,7 @@ TaskDetails.propTypes = { TaskDetails.defaultProps = { label: '', runningSteps: [], - APIerror: null, + apiErrorMessage: '', status: STATUS.PENDING, links: [], dependsOn: [], diff --git a/webpack/ForemanTasks/Components/TaskDetails/TaskDetailsSelectors.js b/webpack/ForemanTasks/Components/TaskDetails/TaskDetailsSelectors.js index 8638f9a76..7f30fc018 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/TaskDetailsSelectors.js +++ b/webpack/ForemanTasks/Components/TaskDetails/TaskDetailsSelectors.js @@ -2,6 +2,9 @@ import { selectAPIResponse, selectAPIByKey, + selectAPIStatus as selectAPIStatusByKey, + selectAPIError as selectAPIErrorByKey, + selectAPIErrorMessage as selectErrorMessageByKey, } from 'foremanReact/redux/API/APISelectors'; import { selectDoesIntervalExist } from 'foremanReact/redux/middlewares/IntervalMiddleware/IntervalSelectors'; import { STATUS } from 'foremanReact/constants'; @@ -103,8 +106,14 @@ export const selectCanEdit = state => export const selectStatus = state => selectTaskDetailsResponse(state).status; -export const selectAPIError = state => - selectTaskDetailsResponse(state)?.APIerror; +export const selectAPIStatus = state => + selectAPIStatusByKey(state, FOREMAN_TASK_DETAILS); + +export const selectAPIErrorMessage = state => + selectErrorMessageByKey(state, FOREMAN_TASK_DETAILS); + +export const selectAPIErrorCode = state => + selectAPIErrorByKey(state, FOREMAN_TASK_DETAILS)?.response?.status; export const selectIsLoading = state => !!selectAPIByKey(state, FOREMAN_TASK_DETAILS).response && diff --git a/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.test.js b/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.test.js index 8e8cad961..658eceabb 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.test.js +++ b/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.test.js @@ -1,6 +1,8 @@ import React from 'react'; import { render, screen } from '@testing-library/react'; import '@testing-library/jest-dom'; +import { configureStore } from '@reduxjs/toolkit'; +import { Provider } from 'react-redux'; import TaskDetails from '../TaskDetails'; import { minProps } from './TaskDetails.fixtures'; @@ -10,29 +12,83 @@ window.location = new URL( 'https://foreman.com/foreman_tasks/tasks/a15dd820-32f1-4ced-9ab7-c0fab8234c47/' ); +const store = configureStore({ reducer: state => state || {} }); + +function renderTaskDetails(props = {}) { + return render( + + + + ); +} + describe('TaskDetails', () => { - it('shows error message when status is ERROR', () => { - render( - - ); + it('shows generic EmptyState when status is ERROR', () => { + renderTaskDetails({ + status: 'ERROR', + apiErrorMessage: 'some-error', + }); + + expect(screen.getByRole('heading', { name: /^error$/i })).toBeInTheDocument(); + expect(screen.getByText('some-error')).toBeInTheDocument(); + expect(screen.queryByRole('tab', { name: /^task$/i })).not.toBeInTheDocument(); + }); + + it('shows generic EmptyState when apiStatus is ERROR', () => { + renderTaskDetails({ + status: 'RESOLVED', + apiStatus: 'ERROR', + apiErrorMessage: 'api-error', + }); + + expect(screen.getByRole('heading', { name: /^error$/i })).toBeInTheDocument(); + expect(screen.getByText('api-error')).toBeInTheDocument(); + expect(screen.queryByRole('tab', { name: /^task$/i })).not.toBeInTheDocument(); + }); + + it('shows PermissionDenied when apiErrorCode is 403', () => { + renderTaskDetails({ + status: 'RESOLVED', + apiStatus: 'ERROR', + apiErrorCode: 403, + apiErrorMessage: 'Forbidden', + }); + + expect(screen.getByText('Permission Denied')).toBeInTheDocument(); + expect(screen.getByText('view_foreman_tasks')).toBeInTheDocument(); expect( - screen.getByText(/could not receive data: some-error/i) + screen.queryByRole('heading', { name: /^error$/i }) + ).not.toBeInTheDocument(); + expect(screen.queryByRole('tab', { name: /^task$/i })).not.toBeInTheDocument(); + }); + + it('shows EmptyState when apiErrorCode is 404', () => { + renderTaskDetails({ + status: 'RESOLVED', + apiStatus: 'ERROR', + apiErrorCode: 404, + apiErrorMessage: 'Task missing', + }); + + expect( + screen.getByRole('heading', { name: /task not found/i }) ).toBeInTheDocument(); + expect(screen.getByText('Task missing')).toBeInTheDocument(); + expect( + screen.queryByRole('heading', { name: /^error$/i }) + ).not.toBeInTheDocument(); + expect(screen.queryByRole('tab', { name: /^task$/i })).not.toBeInTheDocument(); }); it('shows skeleton while loading on the Task tab', () => { - const { container } = render(); + const { container } = renderTaskDetails({ isLoading: true }); expect( container.querySelector('.react-loading-skeleton') ).toBeInTheDocument(); }); it('renders six tabs with expected labels', () => { - render(); + renderTaskDetails(); expect(document.getElementById('task-details-tabs')).toBeInTheDocument(); expect(screen.getByRole('tab', { name: /^task$/i })).toBeInTheDocument(); expect( diff --git a/webpack/ForemanTasks/Components/TaskDetails/index.js b/webpack/ForemanTasks/Components/TaskDetails/index.js index dda6d4064..13ea67a5b 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/index.js +++ b/webpack/ForemanTasks/Components/TaskDetails/index.js @@ -34,7 +34,9 @@ import { selectDynflowEnableConsole, selectCanEdit, selectStatus, - selectAPIError, + selectAPIStatus, + selectAPIErrorMessage, + selectAPIErrorCode, selectIsLoading, selectDependsOn, selectBlocks, @@ -71,7 +73,9 @@ const mapStateToProps = state => ({ dynflowEnableConsole: selectDynflowEnableConsole(state), canEdit: selectCanEdit(state), status: selectStatus(state), - APIerror: selectAPIError(state), + apiStatus: selectAPIStatus(state), + apiErrorMessage: selectAPIErrorMessage(state), + apiErrorCode: selectAPIErrorCode(state), isLoading: selectIsLoading(state), dependsOn: selectDependsOn(state), blocks: selectBlocks(state), From 5c9b6cd84580aab1c5774eb5d30ce09fee9ed2c6 Mon Sep 17 00:00:00 2001 From: Andrei Lakatos Date: Wed, 27 May 2026 13:02:53 -0400 Subject: [PATCH 07/15] Refs #39339 - display the backend message for the task errors --- .../TaskDetails/TaskDetailsSelectors.js | 17 +++- .../__tests__/TaskDetailsSelectors.test.js | 81 +++++++++++++++++++ .../__tests__/TaskDetailsPage.test.js | 13 ++- 3 files changed, 100 insertions(+), 11 deletions(-) create mode 100644 webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetailsSelectors.test.js diff --git a/webpack/ForemanTasks/Components/TaskDetails/TaskDetailsSelectors.js b/webpack/ForemanTasks/Components/TaskDetails/TaskDetailsSelectors.js index 7f30fc018..d6ddbfc32 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/TaskDetailsSelectors.js +++ b/webpack/ForemanTasks/Components/TaskDetails/TaskDetailsSelectors.js @@ -4,7 +4,6 @@ import { selectAPIByKey, selectAPIStatus as selectAPIStatusByKey, selectAPIError as selectAPIErrorByKey, - selectAPIErrorMessage as selectErrorMessageByKey, } from 'foremanReact/redux/API/APISelectors'; import { selectDoesIntervalExist } from 'foremanReact/redux/middlewares/IntervalMiddleware/IntervalSelectors'; import { STATUS } from 'foremanReact/constants'; @@ -109,11 +108,21 @@ export const selectStatus = state => selectTaskDetailsResponse(state).status; export const selectAPIStatus = state => selectAPIStatusByKey(state, FOREMAN_TASK_DETAILS); -export const selectAPIErrorMessage = state => - selectErrorMessageByKey(state, FOREMAN_TASK_DETAILS); +export const selectAPIError = state => + selectAPIErrorByKey(state, FOREMAN_TASK_DETAILS); + +export const selectAPIErrorMessage = state => { + const apiError = selectAPIError(state); + + if (apiError?.response?.data?.error) { + return apiError.response.data.error.message; + } + + return apiError?.message; +}; export const selectAPIErrorCode = state => - selectAPIErrorByKey(state, FOREMAN_TASK_DETAILS)?.response?.status; + selectAPIError(state)?.response?.status; export const selectIsLoading = state => !!selectAPIByKey(state, FOREMAN_TASK_DETAILS).response && diff --git a/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetailsSelectors.test.js b/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetailsSelectors.test.js new file mode 100644 index 000000000..dd199f678 --- /dev/null +++ b/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetailsSelectors.test.js @@ -0,0 +1,81 @@ +import { STATUS } from 'foremanReact/constants'; + +import { FOREMAN_TASK_DETAILS } from '../TaskDetailsConstants'; +import { + selectAPIError, + selectAPIErrorCode, + selectAPIErrorMessage, +} from '../TaskDetailsSelectors'; + +const axiosErrorWithApiBody = { + message: 'Request failed with status code 404', + response: { + status: 404, + data: { + error: { + message: 'Task not found', + }, + }, + }, +}; + +const plainError = { + message: 'Network Error', +}; + +const forbiddenError = { + message: 'Request failed with status code 403', + response: { + status: 403, + data: { + error: { + message: 'Forbidden', + }, + }, + }, +}; + +const buildState = (response, status = STATUS.ERROR) => ({ + API: { + [FOREMAN_TASK_DETAILS]: { + payload: {}, + response, + status, + }, + }, +}); + +describe('TaskDetailsSelectors - API error selectors', () => { + it('selectAPIError returns null when API status is not ERROR', () => { + expect(selectAPIError(buildState({}, STATUS.RESOLVED))).toBeNull(); + }); + + it('selectAPIError returns the stored response when API status is ERROR', () => { + expect(selectAPIError(buildState(plainError))).toEqual(plainError); + }); + + it('selectAPIErrorMessage prefers the API error body message', () => { + expect( + selectAPIErrorMessage(buildState(axiosErrorWithApiBody)) + ).toBe('Task not found'); + }); + + it('selectAPIErrorMessage falls back to the top-level error message', () => { + expect(selectAPIErrorMessage(buildState(plainError))).toBe('Network Error'); + }); + + it('selectAPIErrorMessage returns undefined when there is no API error', () => { + expect( + selectAPIErrorMessage(buildState({}, STATUS.RESOLVED)) + ).toBeUndefined(); + }); + + it('selectAPIErrorCode returns the HTTP status from the error response', () => { + expect(selectAPIErrorCode(buildState(axiosErrorWithApiBody))).toBe(404); + expect(selectAPIErrorCode(buildState(forbiddenError))).toBe(403); + }); + + it('selectAPIErrorCode returns undefined when the error response has no status', () => { + expect(selectAPIErrorCode(buildState(plainError))).toBeUndefined(); + }); +}); diff --git a/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js b/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js index 6d483c1a7..1ecf3f0bd 100644 --- a/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js +++ b/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js @@ -72,7 +72,7 @@ const rootReducer = combineReducers({ foremanTasks: (state = {}, action) => state, }); -function renderPage(apiPayloadOverrides = {}, propsOverrides = {}) { +const renderPage = (apiPayloadOverrides = {}, propsOverrides = {}) => { const store = configureStore({ reducer: rootReducer, preloadedState: createStoreForTaskPayload(apiPayloadOverrides), @@ -100,18 +100,17 @@ function renderPage(apiPayloadOverrides = {}, propsOverrides = {}) { ); -} +}; -function breadcrumbTitleHeadings() { - return screen.getAllByRole('heading', { level: 1 }).filter( +const breadcrumbTitleHeadings = () => + screen.getAllByRole('heading', { level: 1 }).filter( heading => heading.getAttribute('data-ouia-component-id') === 'breadcrumb_title' ); -} /** * Title row (`customHeader` root `Flex`): same OUIA pattern as `Locks.test.js`. */ -function taskDetailsTitleRegion(container) { +const taskDetailsTitleRegion = container => { const el = container.querySelector( `[data-ouia-component-id="${TASK_DETAILS_TITLE_ROW_OUIA_ID}"]` ); @@ -119,7 +118,7 @@ function taskDetailsTitleRegion(container) { expect(el).toBeTruthy(); return el; -} +}; describe('TaskDetailsPage', () => { afterEach(() => { From de511cf75149bbbc41efecb3241a6259a6c20987 Mon Sep 17 00:00:00 2001 From: Andrei Lakatos Date: Thu, 28 May 2026 05:32:25 -0400 Subject: [PATCH 08/15] Refs #39339 - added integration test for the task not found error --- test/integration/tasks_test.rb | 18 ++++++++++++++++++ test/test_plugin_helper.rb | 8 ++++++++ .../Components/TaskDetails/TaskDetails.js | 9 +++++---- 3 files changed, 31 insertions(+), 4 deletions(-) create mode 100644 test/integration/tasks_test.rb create mode 100644 test/test_plugin_helper.rb diff --git a/test/integration/tasks_test.rb b/test/integration/tasks_test.rb new file mode 100644 index 000000000..3dbfbfa29 --- /dev/null +++ b/test/integration/tasks_test.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +require_relative '../test_plugin_helper' +require 'integration_test_helper' + +class TasksIntegrationTest < IntegrationTestWithJavascript + test 'does not allow user without permissions to see task details' do + setup_user('view', 'foreman_tasks', 'owner.id = current_user') + task = FactoryBot.create(:some_task) + set_request_user(User.current) + + visit "/foreman_tasks/tasks/#{task.id}" + wait_for_ajax + + assert_selector 'h5', text: /Task not found/i + assert_no_selector '#task-details-tabs' + end +end diff --git a/test/test_plugin_helper.rb b/test/test_plugin_helper.rb new file mode 100644 index 000000000..5bd928eac --- /dev/null +++ b/test/test_plugin_helper.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +# This calls the main test_helper in Foreman-core +require 'test_helper' + +# Add plugin to FactoryBot's paths +FactoryBot.definition_file_paths << File.join(File.dirname(__FILE__), 'factories') +FactoryBot.reload diff --git a/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js b/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js index e5b9e1ab3..8bdaff919 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js +++ b/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js @@ -17,6 +17,9 @@ import { TaskSkeleton } from './Components/TaskSkeleton'; import './TaskDetails.scss'; +const NOT_FOUND_ERR = __('The requested task could not be found.'); +const GENERIC_ERR = __('Could not receive task data.'); + const TaskDetails = ({ executionPlan, failedSteps, @@ -62,9 +65,7 @@ const TaskDetails = ({ } header={__('Task not found')} - description={ - apiErrorMessage || __('The requested task could not be found.') - } + description={apiErrorMessage || NOT_FOUND_ERR} /> ); } @@ -73,7 +74,7 @@ const TaskDetails = ({ } header={__('Error')} - description={apiErrorMessage || __('Could not receive task data.')} + description={apiErrorMessage || GENERIC_ERR} /> ); } From 4074018cb4bd1de7dbdcaf062eced0da78c9168c Mon Sep 17 00:00:00 2001 From: Andrei Lakatos Date: Thu, 4 Jun 2026 07:33:22 -0400 Subject: [PATCH 09/15] Refs #39339 - use reusable empty state to handle api errors on task details page --- test/integration/tasks_test.rb | 2 +- .../Components/TaskDetails/TaskDetails.js | 38 +++++------- .../TaskDetails/__tests__/TaskDetails.test.js | 59 +++++++++++++------ 3 files changed, 57 insertions(+), 42 deletions(-) diff --git a/test/integration/tasks_test.rb b/test/integration/tasks_test.rb index 3dbfbfa29..c4959f24b 100644 --- a/test/integration/tasks_test.rb +++ b/test/integration/tasks_test.rb @@ -12,7 +12,7 @@ class TasksIntegrationTest < IntegrationTestWithJavascript visit "/foreman_tasks/tasks/#{task.id}" wait_for_ajax - assert_selector 'h5', text: /Task not found/i + assert_selector 'h5', text: /Unable to load task/i assert_no_selector '#task-details-tabs' end end diff --git a/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js b/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js index 8bdaff919..5693a37c9 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js +++ b/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js @@ -1,11 +1,9 @@ import React, { useEffect, useState } from 'react'; import PropTypes from 'prop-types'; import { Tabs, Tab, TabTitleText } from '@patternfly/react-core'; -import { ExclamationCircleIcon } from '@patternfly/react-icons'; import { translate as __ } from 'foremanReact/common/I18n'; import { STATUS } from 'foremanReact/constants'; -import EmptyState from 'foremanReact/components/common/EmptyState'; -import PermissionDenied from 'foremanReact/components/PermissionDenied'; +import { ResourceLoadFailedEmptyState } from 'foremanReact/components/common/EmptyState'; import Task from './Components/Task'; import RunningSteps from './Components/RunningSteps'; import Errors from './Components/Errors'; @@ -17,8 +15,7 @@ import { TaskSkeleton } from './Components/TaskSkeleton'; import './TaskDetails.scss'; -const NOT_FOUND_ERR = __('The requested task could not be found.'); -const GENERIC_ERR = __('Could not receive task data.'); +const TASKS_PATH = '/foreman_tasks/tasks'; const TaskDetails = ({ executionPlan, @@ -56,25 +53,20 @@ const TaskDetails = ({ }; if (status === STATUS.ERROR || apiStatus === STATUS.ERROR) { - if (apiErrorCode === 403) { - return ; - } - - if (apiErrorCode === 404) { - return ( - } - header={__('Task not found')} - description={apiErrorMessage || NOT_FOUND_ERR} - /> - ); - } - return ( - } - header={__('Error')} - description={apiErrorMessage || GENERIC_ERR} + ); } diff --git a/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.test.js b/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.test.js index 658eceabb..63994b4e7 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.test.js +++ b/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.test.js @@ -3,10 +3,17 @@ import { render, screen } from '@testing-library/react'; import '@testing-library/jest-dom'; import { configureStore } from '@reduxjs/toolkit'; import { Provider } from 'react-redux'; +import { createMemoryHistory } from 'history'; +import { Router } from 'react-router-dom'; import TaskDetails from '../TaskDetails'; import { minProps } from './TaskDetails.fixtures'; +jest.mock('foremanReact/Root/Context/ForemanContext', () => ({ + ...jest.requireActual('foremanReact/Root/Context/ForemanContext'), + useForemanPermissions: () => new Set(['view_foreman_tasks']), +})); + delete window.location; window.location = new URL( 'https://foreman.com/foreman_tasks/tasks/a15dd820-32f1-4ced-9ab7-c0fab8234c47/' @@ -15,38 +22,50 @@ window.location = new URL( const store = configureStore({ reducer: state => state || {} }); function renderTaskDetails(props = {}) { + const history = createMemoryHistory(); + return render( - - - + + + + + ); } describe('TaskDetails', () => { - it('shows generic EmptyState when status is ERROR', () => { + it('shows ResourceLoadFailedEmptyState when status is ERROR', () => { renderTaskDetails({ status: 'ERROR', apiErrorMessage: 'some-error', }); - expect(screen.getByRole('heading', { name: /^error$/i })).toBeInTheDocument(); - expect(screen.getByText('some-error')).toBeInTheDocument(); + expect( + screen.getByRole('heading', { name: /unable to load task/i }) + ).toBeInTheDocument(); + expect( + screen.getByText('Server returned: some-error') + ).toBeInTheDocument(); expect(screen.queryByRole('tab', { name: /^task$/i })).not.toBeInTheDocument(); }); - it('shows generic EmptyState when apiStatus is ERROR', () => { + it('shows ResourceLoadFailedEmptyState when apiStatus is ERROR', () => { renderTaskDetails({ status: 'RESOLVED', apiStatus: 'ERROR', apiErrorMessage: 'api-error', }); - expect(screen.getByRole('heading', { name: /^error$/i })).toBeInTheDocument(); - expect(screen.getByText('api-error')).toBeInTheDocument(); + expect( + screen.getByRole('heading', { name: /unable to load task/i }) + ).toBeInTheDocument(); + expect( + screen.getByText('Server returned: api-error') + ).toBeInTheDocument(); expect(screen.queryByRole('tab', { name: /^task$/i })).not.toBeInTheDocument(); }); - it('shows PermissionDenied when apiErrorCode is 403', () => { + it('shows permission denied when apiErrorCode is 403', () => { renderTaskDetails({ status: 'RESOLVED', apiStatus: 'ERROR', @@ -54,15 +73,17 @@ describe('TaskDetails', () => { apiErrorMessage: 'Forbidden', }); - expect(screen.getByText('Permission Denied')).toBeInTheDocument(); + expect( + screen.getByRole('heading', { name: /permission denied/i }) + ).toBeInTheDocument(); expect(screen.getByText('view_foreman_tasks')).toBeInTheDocument(); expect( - screen.queryByRole('heading', { name: /^error$/i }) - ).not.toBeInTheDocument(); + screen.getByText('Server returned: Forbidden') + ).toBeInTheDocument(); expect(screen.queryByRole('tab', { name: /^task$/i })).not.toBeInTheDocument(); }); - it('shows EmptyState when apiErrorCode is 404', () => { + it('shows not found messaging when apiErrorCode is 404', () => { renderTaskDetails({ status: 'RESOLVED', apiStatus: 'ERROR', @@ -71,12 +92,14 @@ describe('TaskDetails', () => { }); expect( - screen.getByRole('heading', { name: /task not found/i }) + screen.getByRole('heading', { name: /unable to load task/i }) + ).toBeInTheDocument(); + expect( + screen.getByText(/could not be found/i) ).toBeInTheDocument(); - expect(screen.getByText('Task missing')).toBeInTheDocument(); expect( - screen.queryByRole('heading', { name: /^error$/i }) - ).not.toBeInTheDocument(); + screen.getByText('Server returned: Task missing') + ).toBeInTheDocument(); expect(screen.queryByRole('tab', { name: /^task$/i })).not.toBeInTheDocument(); }); From acc0470e67088b374f6cb83636d17e14d6d131db Mon Sep 17 00:00:00 2001 From: Andrei Lakatos Date: Mon, 15 Jun 2026 07:36:37 -0400 Subject: [PATCH 10/15] Refs #39339 - update foreman core version to 5.0 --- lib/foreman_tasks/engine.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/foreman_tasks/engine.rb b/lib/foreman_tasks/engine.rb index aed71a91b..6b8b7a608 100644 --- a/lib/foreman_tasks/engine.rb +++ b/lib/foreman_tasks/engine.rb @@ -20,7 +20,7 @@ class Engine < ::Rails::Engine require 'foreman/cron' Foreman::Plugin.register :"foreman-tasks" do - requires_foreman '>= 3.19' + requires_foreman '>= 5.0' register_global_js_file 'global' divider :top_menu, :parent => :monitor_menu, :last => true, :caption => N_('Foreman Tasks') menu :top_menu, :tasks, From 7721d86c8a7ae188d09f31f840318de4e6346daf Mon Sep 17 00:00:00 2001 From: Andrei Lakatos Date: Mon, 15 Jun 2026 08:49:30 -0400 Subject: [PATCH 11/15] Refs #39339 - fix test attempt --- test/foreman_tasks_test_helper.rb | 4 ++-- test/integration/tasks_test.rb | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/test/foreman_tasks_test_helper.rb b/test/foreman_tasks_test_helper.rb index bbd2526c0..7e75a2a28 100644 --- a/test/foreman_tasks_test_helper.rb +++ b/test/foreman_tasks_test_helper.rb @@ -9,8 +9,8 @@ require 'dynflow/testing' require 'foreman_tasks/test_helpers' -FactoryBot.definition_file_paths = ["#{ForemanTasks::Engine.root}/test/factories"] -FactoryBot.find_definitions +FactoryBot.definition_file_paths << "#{ForemanTasks::Engine.root}/test/factories" +FactoryBot.reload ForemanTasks.dynflow.require! ForemanTasks.dynflow.config.disable_active_record_actions = true diff --git a/test/integration/tasks_test.rb b/test/integration/tasks_test.rb index c4959f24b..8d21af2fa 100644 --- a/test/integration/tasks_test.rb +++ b/test/integration/tasks_test.rb @@ -10,7 +10,6 @@ class TasksIntegrationTest < IntegrationTestWithJavascript set_request_user(User.current) visit "/foreman_tasks/tasks/#{task.id}" - wait_for_ajax assert_selector 'h5', text: /Unable to load task/i assert_no_selector '#task-details-tabs' From d8340ab0aa0cc71aeea04c310d23b4acb6c30348 Mon Sep 17 00:00:00 2001 From: Andrei Lakatos Date: Tue, 16 Jun 2026 14:23:27 -0400 Subject: [PATCH 12/15] Refs #39339 - add title to icons, remove redundant status prop, use empty state instead of Permitted, add switcher to breadcrumb --- .../Components/__tests__/Task.test.js | 2 - .../Components/__tests__/TaskButtons.test.js | 2 - .../Components/TaskDetails/TaskDetails.js | 10 +-- .../TaskDetails/TaskDetailsConstants.js | 3 +- .../TaskDetails/TaskDetailsSelectors.js | 6 +- .../__tests__/TaskDetails.fixtures.js | 2 +- .../TaskDetails/__tests__/TaskDetails.test.js | 22 +------ .../Components/TaskDetails/index.js | 2 - .../Components/common/taskResultIcon.js | 11 ++-- .../Routes/ShowTaskDetails/TaskDetailsPage.js | 61 +++++++++++++------ .../__tests__/TaskDetailsPage.test.js | 50 ++++++++++----- webpack/Routes/routes.test.js | 9 --- 12 files changed, 93 insertions(+), 87 deletions(-) diff --git a/webpack/ForemanTasks/Components/TaskDetails/Components/__tests__/Task.test.js b/webpack/ForemanTasks/Components/TaskDetails/Components/__tests__/Task.test.js index 4ef7727d2..2ca7caf8a 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/Components/__tests__/Task.test.js +++ b/webpack/ForemanTasks/Components/TaskDetails/Components/__tests__/Task.test.js @@ -1,7 +1,6 @@ import React from 'react'; import { render, screen } from '@testing-library/react'; import '@testing-library/jest-dom'; -import { STATUS } from 'foremanReact/constants'; import Task from '../Task'; @@ -29,7 +28,6 @@ describe('Task', () => { parentTask="parent-id" taskReload canEdit - status={STATUS.RESOLVED} taskProgressToggle={jest.fn()} taskReloadStart={jest.fn()} /> diff --git a/webpack/ForemanTasks/Components/TaskDetails/Components/__tests__/TaskButtons.test.js b/webpack/ForemanTasks/Components/TaskDetails/Components/__tests__/TaskButtons.test.js index ad3acd5d3..b0ecd89d8 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/Components/__tests__/TaskButtons.test.js +++ b/webpack/ForemanTasks/Components/TaskDetails/Components/__tests__/TaskButtons.test.js @@ -1,7 +1,6 @@ import React from 'react'; import { render, screen, fireEvent } from '@testing-library/react'; import '@testing-library/jest-dom'; -import { STATUS } from 'foremanReact/constants'; import { TaskButtons } from '../TaskButtons'; const setUnlockModalOpen = jest.fn(); @@ -169,7 +168,6 @@ describe('TaskButtons', () => { resumeTaskRequest, taskProgressToggle, taskReloadStart, - status: STATUS.RESOLVED, canEdit: true, resumable: true, cancellable: true, diff --git a/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js b/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js index 5693a37c9..078a91caa 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js +++ b/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js @@ -10,13 +10,12 @@ import Errors from './Components/Errors'; import Locks from './Components/Locks'; import Raw from './Components/Raw'; import Dependencies from './Components/Dependencies'; +import { TASKS_PATH } from './TaskDetailsConstants'; import { getTaskID } from './TasksDetailsHelper'; import { TaskSkeleton } from './Components/TaskSkeleton'; import './TaskDetails.scss'; -const TASKS_PATH = '/foreman_tasks/tasks'; - const TaskDetails = ({ executionPlan, failedSteps, @@ -34,7 +33,7 @@ const TaskDetails = ({ ...props }) => { const id = getTaskID(); - const { taskReload, status, isLoading, result } = props; + const { taskReload, isLoading, result } = props; const [activeTabKey, setActiveTabKey] = useState(1); useEffect(() => { @@ -52,7 +51,7 @@ const TaskDetails = ({ } }; - if (status === STATUS.ERROR || apiStatus === STATUS.ERROR) { + if (apiStatus === STATUS.ERROR) { return ( export const selectCanEdit = state => selectTaskDetailsResponse(state).can_edit || false; -export const selectStatus = state => selectTaskDetailsResponse(state).status; - export const selectAPIStatus = state => selectAPIStatusByKey(state, FOREMAN_TASK_DETAILS); @@ -125,8 +122,7 @@ export const selectAPIErrorCode = state => selectAPIError(state)?.response?.status; export const selectIsLoading = state => - !!selectAPIByKey(state, FOREMAN_TASK_DETAILS).response && - selectStatus(state) === STATUS.PENDING; + selectAPIStatus(state) === STATUS.PENDING; export const selectDependsOn = state => selectTaskDetailsResponse(state).depends_on || []; diff --git a/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.fixtures.js b/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.fixtures.js index 84bb4c07b..209bfcccc 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.fixtures.js +++ b/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.fixtures.js @@ -4,5 +4,5 @@ export const minProps = { taskReloadStop: jest.fn(), taskReloadStart: jest.fn(), taskProgressToggle: jest.fn(), - status: 'RESOLVED', + apiStatus: 'RESOLVED', }; diff --git a/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.test.js b/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.test.js index 63994b4e7..cc09c0265 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.test.js +++ b/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.test.js @@ -34,40 +34,23 @@ function renderTaskDetails(props = {}) { } describe('TaskDetails', () => { - it('shows ResourceLoadFailedEmptyState when status is ERROR', () => { - renderTaskDetails({ - status: 'ERROR', - apiErrorMessage: 'some-error', - }); - - expect( - screen.getByRole('heading', { name: /unable to load task/i }) - ).toBeInTheDocument(); - expect( - screen.getByText('Server returned: some-error') - ).toBeInTheDocument(); - expect(screen.queryByRole('tab', { name: /^task$/i })).not.toBeInTheDocument(); - }); - it('shows ResourceLoadFailedEmptyState when apiStatus is ERROR', () => { renderTaskDetails({ - status: 'RESOLVED', apiStatus: 'ERROR', - apiErrorMessage: 'api-error', + apiErrorMessage: 'some-error', }); expect( screen.getByRole('heading', { name: /unable to load task/i }) ).toBeInTheDocument(); expect( - screen.getByText('Server returned: api-error') + screen.getByText('Server returned: some-error') ).toBeInTheDocument(); expect(screen.queryByRole('tab', { name: /^task$/i })).not.toBeInTheDocument(); }); it('shows permission denied when apiErrorCode is 403', () => { renderTaskDetails({ - status: 'RESOLVED', apiStatus: 'ERROR', apiErrorCode: 403, apiErrorMessage: 'Forbidden', @@ -85,7 +68,6 @@ describe('TaskDetails', () => { it('shows not found messaging when apiErrorCode is 404', () => { renderTaskDetails({ - status: 'RESOLVED', apiStatus: 'ERROR', apiErrorCode: 404, apiErrorMessage: 'Task missing', diff --git a/webpack/ForemanTasks/Components/TaskDetails/index.js b/webpack/ForemanTasks/Components/TaskDetails/index.js index 13ea67a5b..58c0ed7c1 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/index.js +++ b/webpack/ForemanTasks/Components/TaskDetails/index.js @@ -33,7 +33,6 @@ import { selectExternalId, selectDynflowEnableConsole, selectCanEdit, - selectStatus, selectAPIStatus, selectAPIErrorMessage, selectAPIErrorCode, @@ -72,7 +71,6 @@ const mapStateToProps = state => ({ externalId: selectExternalId(state), dynflowEnableConsole: selectDynflowEnableConsole(state), canEdit: selectCanEdit(state), - status: selectStatus(state), apiStatus: selectAPIStatus(state), apiErrorMessage: selectAPIErrorMessage(state), apiErrorCode: selectAPIErrorCode(state), diff --git a/webpack/ForemanTasks/Components/common/taskResultIcon.js b/webpack/ForemanTasks/Components/common/taskResultIcon.js index 6698160cc..9dc376835 100644 --- a/webpack/ForemanTasks/Components/common/taskResultIcon.js +++ b/webpack/ForemanTasks/Components/common/taskResultIcon.js @@ -6,6 +6,7 @@ import { ExclamationTriangleIcon, QuestionCircleIcon, } from '@patternfly/react-icons'; +import { translate as __ } from 'foremanReact/common/I18n'; /** * Icon reflecting task state/result (aligned with TaskInfo / tasks table). @@ -17,7 +18,7 @@ import { export const taskResultIconEl = (state, result) => { if (state !== 'stopped') { return ( - + ); @@ -26,25 +27,25 @@ export const taskResultIconEl = (state, result) => { switch (result) { case 'success': return ( - + ); case 'error': return ( - + ); case 'warning': return ( - + ); default: return ( - + ); diff --git a/webpack/ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage.js b/webpack/ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage.js index 76f815d5d..ab80a0a01 100644 --- a/webpack/ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage.js +++ b/webpack/ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage.js @@ -3,14 +3,19 @@ import PropTypes from 'prop-types'; import { useSelector } from 'react-redux'; import { Flex, FlexItem, TextContent, Text } from '@patternfly/react-core'; import PageLayout from 'foremanReact/routes/common/PageLayout/PageLayout'; -import Permitted from 'foremanReact/components/Permitted/Permitted'; +import { ResourceLoadFailedEmptyState } from 'foremanReact/components/common/EmptyState'; import { translate as __, sprintf } from 'foremanReact/common/I18n'; +import { usePermissions } from 'foremanReact/common/hooks/Permissions/permissionHooks'; import TaskDetails from '../../Components/TaskDetails'; import { selectAction, selectResult, selectState, } from '../../Components/TaskDetails/TaskDetailsSelectors'; +import { + TASKS_PATH, + VIEW_FOREMAN_TASKS, +} from '../../Components/TaskDetails/TaskDetailsConstants'; import { taskResultIconEl } from '../../Components/common/taskResultIcon'; const TaskDetailsPage = props => { @@ -18,9 +23,10 @@ const TaskDetailsPage = props => { const action = useSelector(selectAction); const taskState = useSelector(selectState); const taskResult = useSelector(selectResult); + const hasViewPermission = usePermissions([VIEW_FOREMAN_TASKS]); const headerText = action ? sprintf(__('Details of %s task'), action) - : __('Task details'); + : __('Task Details'); const header = ( { ); - return ( - - - - - + /> + ); + } + + return ( + + + ); }; diff --git a/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js b/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js index 1ecf3f0bd..798d7e8ce 100644 --- a/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js +++ b/webpack/ForemanTasks/Routes/ShowTaskDetails/__tests__/TaskDetailsPage.test.js @@ -2,6 +2,8 @@ import React from 'react'; import { render, screen, within } from '@testing-library/react'; import '@testing-library/jest-dom'; import { combineReducers, configureStore } from '@reduxjs/toolkit'; +import { createMemoryHistory } from 'history'; +import { Router } from 'react-router-dom'; import { IntlProvider } from 'react-intl'; import { Provider } from 'react-redux'; @@ -73,6 +75,9 @@ const rootReducer = combineReducers({ }); const renderPage = (apiPayloadOverrides = {}, propsOverrides = {}) => { + const history = createMemoryHistory({ + initialEntries: [`/foreman_tasks/tasks/${matchDefault.params.id}`], + }); const store = configureStore({ reducer: rootReducer, preloadedState: createStoreForTaskPayload(apiPayloadOverrides), @@ -90,15 +95,18 @@ const renderPage = (apiPayloadOverrides = {}, propsOverrides = {}) => { ); return render( - - - - - + + + + + + + ); }; @@ -149,19 +157,27 @@ describe('TaskDetailsPage', () => { name: /Details of Run job task/, }) ).toBeInTheDocument(); - expect(screen.queryByText('Permission Denied')).not.toBeInTheDocument(); - expect(screen.queryAllByRole('heading', { name: 'Permission Denied' })).toHaveLength(0); + expect(screen.queryByRole('heading', { name: /Permission denied/i })).not.toBeInTheDocument(); expect(mockUseForemanPermissions).toHaveBeenCalled(); }); - it(`shows PermissionDenied and lists ${VIEW_PERM} when it is absent`, () => { + it(`shows ResourceLoadFailedEmptyState and lists ${VIEW_PERM} when it is absent`, () => { mockUseForemanPermissions.mockImplementation(() => new Set()); renderPage({ action: 'Hidden task' }); - expect(screen.getByText('Permission Denied')).toBeInTheDocument(); - expect(screen.getByRole('heading', { name: /Permission denied/i })).toBeInTheDocument(); + expect( + screen.getByRole('heading', { name: /Permission denied/i }) + ).toBeInTheDocument(); + expect( + screen.getByText( + /You do not have permission to view the task with id task-route-id/ + ) + ).toBeInTheDocument(); expect(screen.getByText(VIEW_PERM)).toBeInTheDocument(); + expect( + screen.getByRole('button', { name: /Back to tasks/i }) + ).toBeInTheDocument(); expect( screen.queryByRole('navigation', { name: 'Breadcrumb' }) ).not.toBeInTheDocument(); @@ -175,7 +191,9 @@ describe('TaskDetailsPage', () => { renderPage({}); - expect(screen.getByText('Permission Denied')).toBeInTheDocument(); + expect( + screen.getByRole('heading', { name: /Permission denied/i }) + ).toBeInTheDocument(); expect(screen.getByText(VIEW_PERM)).toBeInTheDocument(); expect( screen.queryByRole('navigation', { name: 'Breadcrumb' }) @@ -196,7 +214,7 @@ describe('TaskDetailsPage', () => { it('shows generic title and breadcrumb from route id when action is unset', () => { const page = renderPage({}); - expectToolbarHeadingText('Task details'); + expectToolbarHeadingText('Task Details'); expect(screen.getByRole('navigation', { name: 'Breadcrumb' })).toHaveTextContent( 'Tasks' diff --git a/webpack/Routes/routes.test.js b/webpack/Routes/routes.test.js index da6aa184c..b0f314490 100644 --- a/webpack/Routes/routes.test.js +++ b/webpack/Routes/routes.test.js @@ -67,15 +67,6 @@ describe('ForemanTasks routes', () => { url: '/foreman_tasks/tasks/99', }, }, - { - ...routerProps, - match: { - ...routerProps.match, - params: { id: '99' }, - path: '/foreman_tasks/tasks/:id', - url: '/foreman_tasks/tasks/99', - }, - }, { ...routerProps, match: { From 3040958c99311e3b10e1e2f1b515e7dfaf3b7d51 Mon Sep 17 00:00:00 2001 From: Andrei Lakatos Date: Wed, 17 Jun 2026 10:39:43 -0400 Subject: [PATCH 13/15] Refs #39339 - fix isLoading logic to be true only when loading and no data --- .../ForemanTasks/Components/TaskDetails/TaskDetailsSelectors.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/webpack/ForemanTasks/Components/TaskDetails/TaskDetailsSelectors.js b/webpack/ForemanTasks/Components/TaskDetails/TaskDetailsSelectors.js index 8cb2b1ce9..eed21ba0f 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/TaskDetailsSelectors.js +++ b/webpack/ForemanTasks/Components/TaskDetails/TaskDetailsSelectors.js @@ -1,6 +1,7 @@ /* eslint-disable camelcase */ import { selectAPIResponse, + selectAPIByKey, selectAPIStatus as selectAPIStatusByKey, selectAPIError as selectAPIErrorByKey, } from 'foremanReact/redux/API/APISelectors'; @@ -122,6 +123,7 @@ export const selectAPIErrorCode = state => selectAPIError(state)?.response?.status; export const selectIsLoading = state => + !selectAPIByKey(state, FOREMAN_TASK_DETAILS).response && selectAPIStatus(state) === STATUS.PENDING; export const selectDependsOn = state => From 38c4d163b122ce693054fe663f829bddcf155fa5 Mon Sep 17 00:00:00 2001 From: Andrei Lakatos Date: Wed, 17 Jun 2026 12:51:05 -0400 Subject: [PATCH 14/15] Refs #39339 - use ResourceLoadFailedEmptyState once, cleanup tests, constants --- .../Components/TaskDetails/TaskDetails.js | 6 ++-- .../TaskDetails/TaskDetailsConstants.js | 2 +- .../TaskDetails/TaskDetailsSelectors.js | 3 +- .../TaskDetails/__tests__/TaskDetails.test.js | 28 ++++++++++++++++- .../Routes/ShowTaskDetails/TaskDetailsPage.js | 24 --------------- .../__tests__/TaskDetailsPage.test.js | 30 ++++++++----------- webpack/Routes/routes.test.js | 11 ------- 7 files changed, 45 insertions(+), 59 deletions(-) diff --git a/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js b/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js index 078a91caa..cab955487 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js +++ b/webpack/ForemanTasks/Components/TaskDetails/TaskDetails.js @@ -3,6 +3,7 @@ import PropTypes from 'prop-types'; import { Tabs, Tab, TabTitleText } from '@patternfly/react-core'; import { translate as __ } from 'foremanReact/common/I18n'; import { STATUS } from 'foremanReact/constants'; +import { usePermissions } from 'foremanReact/common/hooks/Permissions/permissionHooks'; import { ResourceLoadFailedEmptyState } from 'foremanReact/components/common/EmptyState'; import Task from './Components/Task'; import RunningSteps from './Components/RunningSteps'; @@ -10,7 +11,7 @@ import Errors from './Components/Errors'; import Locks from './Components/Locks'; import Raw from './Components/Raw'; import Dependencies from './Components/Dependencies'; -import { TASKS_PATH } from './TaskDetailsConstants'; +import { TASKS_PATH, VIEW_FOREMAN_TASKS } from './TaskDetailsConstants'; import { getTaskID } from './TasksDetailsHelper'; import { TaskSkeleton } from './Components/TaskSkeleton'; @@ -35,6 +36,7 @@ const TaskDetails = ({ const id = getTaskID(); const { taskReload, isLoading, result } = props; const [activeTabKey, setActiveTabKey] = useState(1); + const hasViewPermission = usePermissions([VIEW_FOREMAN_TASKS]); useEffect(() => { taskReloadStart(id); @@ -51,7 +53,7 @@ const TaskDetails = ({ } }; - if (apiStatus === STATUS.ERROR) { + if (apiStatus === STATUS.ERROR || !hasViewPermission) { return ( selectAPIError(state)?.response?.status; export const selectIsLoading = state => - !selectAPIByKey(state, FOREMAN_TASK_DETAILS).response && + !Object.keys(selectTaskDetailsResponse(state) ?? {}).length && selectAPIStatus(state) === STATUS.PENDING; export const selectDependsOn = state => diff --git a/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.test.js b/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.test.js index cc09c0265..412f6787a 100644 --- a/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.test.js +++ b/webpack/ForemanTasks/Components/TaskDetails/__tests__/TaskDetails.test.js @@ -8,10 +8,15 @@ import { Router } from 'react-router-dom'; import TaskDetails from '../TaskDetails'; import { minProps } from './TaskDetails.fixtures'; +import { VIEW_FOREMAN_TASKS } from '../TaskDetailsConstants'; + +const mockUseForemanPermissions = jest.fn( + () => new Set(['view_foreman_tasks']) +); jest.mock('foremanReact/Root/Context/ForemanContext', () => ({ ...jest.requireActual('foremanReact/Root/Context/ForemanContext'), - useForemanPermissions: () => new Set(['view_foreman_tasks']), + useForemanPermissions: (...args) => mockUseForemanPermissions(...args), })); delete window.location; @@ -34,6 +39,27 @@ function renderTaskDetails(props = {}) { } describe('TaskDetails', () => { + beforeEach(() => { + mockUseForemanPermissions.mockImplementation( + () => new Set(['view_foreman_tasks']) + ); + }); + + it(`shows ResourceLoadFailedEmptyState when ${VIEW_FOREMAN_TASKS} is absent`, () => { + mockUseForemanPermissions.mockImplementation(() => new Set()); + + renderTaskDetails(); + + expect( + screen.getByRole('heading', { name: /permission denied/i }) + ).toBeInTheDocument(); + expect(screen.getByText(VIEW_FOREMAN_TASKS)).toBeInTheDocument(); + expect( + screen.getByRole('button', { name: /back to tasks/i }) + ).toBeInTheDocument(); + expect(screen.queryByRole('tab', { name: /^task$/i })).not.toBeInTheDocument(); + }); + it('shows ResourceLoadFailedEmptyState when apiStatus is ERROR', () => { renderTaskDetails({ apiStatus: 'ERROR', diff --git a/webpack/ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage.js b/webpack/ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage.js index ab80a0a01..31187a2e6 100644 --- a/webpack/ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage.js +++ b/webpack/ForemanTasks/Routes/ShowTaskDetails/TaskDetailsPage.js @@ -3,19 +3,13 @@ import PropTypes from 'prop-types'; import { useSelector } from 'react-redux'; import { Flex, FlexItem, TextContent, Text } from '@patternfly/react-core'; import PageLayout from 'foremanReact/routes/common/PageLayout/PageLayout'; -import { ResourceLoadFailedEmptyState } from 'foremanReact/components/common/EmptyState'; import { translate as __, sprintf } from 'foremanReact/common/I18n'; -import { usePermissions } from 'foremanReact/common/hooks/Permissions/permissionHooks'; import TaskDetails from '../../Components/TaskDetails'; import { selectAction, selectResult, selectState, } from '../../Components/TaskDetails/TaskDetailsSelectors'; -import { - TASKS_PATH, - VIEW_FOREMAN_TASKS, -} from '../../Components/TaskDetails/TaskDetailsConstants'; import { taskResultIconEl } from '../../Components/common/taskResultIcon'; const TaskDetailsPage = props => { @@ -23,7 +17,6 @@ const TaskDetailsPage = props => { const action = useSelector(selectAction); const taskState = useSelector(selectState); const taskResult = useSelector(selectResult); - const hasViewPermission = usePermissions([VIEW_FOREMAN_TASKS]); const headerText = action ? sprintf(__('Details of %s task'), action) : __('Task Details'); @@ -47,23 +40,6 @@ const TaskDetailsPage = props => { ); - if (!hasViewPermission) { - return ( - - ); - } - return ( { }; describe('TaskDetailsPage', () => { - afterEach(() => { - window.history.pushState({}, '', '/'); - mockUseForemanPermissions.mockImplementation(() => new Set(['view_foreman_tasks'])); - }); - beforeEach(() => { mockUseForemanPermissions.mockImplementation(() => new Set(['view_foreman_tasks'])); }); describe('permissions', () => { - const VIEW_PERM = 'view_foreman_tasks'; - - it(`renders the task details chrome when ${VIEW_PERM} is present`, () => { + it(`renders the task details chrome when ${VIEW_FOREMAN_TASKS} is present`, () => { mockUseForemanPermissions.mockImplementation( - () => new Set(['edit_foreman_tasks', VIEW_PERM]) + () => new Set(['edit_foreman_tasks', VIEW_FOREMAN_TASKS]) ); renderPage({ action: 'Run job' }); @@ -161,7 +154,7 @@ describe('TaskDetailsPage', () => { expect(mockUseForemanPermissions).toHaveBeenCalled(); }); - it(`shows ResourceLoadFailedEmptyState and lists ${VIEW_PERM} when it is absent`, () => { + it(`shows ResourceLoadFailedEmptyState and lists ${VIEW_FOREMAN_TASKS} when it is absent`, () => { mockUseForemanPermissions.mockImplementation(() => new Set()); renderPage({ action: 'Hidden task' }); @@ -174,14 +167,14 @@ describe('TaskDetailsPage', () => { /You do not have permission to view the task with id task-route-id/ ) ).toBeInTheDocument(); - expect(screen.getByText(VIEW_PERM)).toBeInTheDocument(); + expect(screen.getByText(VIEW_FOREMAN_TASKS)).toBeInTheDocument(); expect( screen.getByRole('button', { name: /Back to tasks/i }) ).toBeInTheDocument(); expect( - screen.queryByRole('navigation', { name: 'Breadcrumb' }) - ).not.toBeInTheDocument(); - expect(screen.queryAllByRole('heading', { name: /Hidden task/i })).toHaveLength(0); + screen.getByRole('navigation', { name: 'Breadcrumb' }) + ).toBeInTheDocument(); + expect(screen.queryByRole('tab', { name: /^task$/i })).not.toBeInTheDocument(); }); it('denies access when user only has edit_foreman_tasks without view', () => { @@ -194,10 +187,11 @@ describe('TaskDetailsPage', () => { expect( screen.getByRole('heading', { name: /Permission denied/i }) ).toBeInTheDocument(); - expect(screen.getByText(VIEW_PERM)).toBeInTheDocument(); + expect(screen.getByText(VIEW_FOREMAN_TASKS)).toBeInTheDocument(); expect( - screen.queryByRole('navigation', { name: 'Breadcrumb' }) - ).not.toBeInTheDocument(); + screen.getByRole('navigation', { name: 'Breadcrumb' }) + ).toBeInTheDocument(); + expect(screen.queryByRole('tab', { name: /^task$/i })).not.toBeInTheDocument(); }); }); diff --git a/webpack/Routes/routes.test.js b/webpack/Routes/routes.test.js index b0f314490..62553363d 100644 --- a/webpack/Routes/routes.test.js +++ b/webpack/Routes/routes.test.js @@ -67,15 +67,6 @@ describe('ForemanTasks routes', () => { url: '/foreman_tasks/tasks/99', }, }, - { - ...routerProps, - match: { - ...routerProps.match, - params: { id: '42' }, - path: '/foreman_tasks/ex_tasks/:id', - url: '/foreman_tasks/ex_tasks/42', - }, - }, ]; ForemanTasksRoutes.forEach((route, index) => { @@ -85,8 +76,6 @@ describe('ForemanTasks routes', () => { expect( screen.getByTestId('task-details-page-stub') ).toBeInTheDocument(); - } else if (index === 3) { - expect(screen.getByTestId('show-task-stub')).toBeInTheDocument(); } else { expect( screen.getByTestId('tasks-table-index-stub') From 88c9087893b32c9a69252036c01e960d2914fc90 Mon Sep 17 00:00:00 2001 From: Andrei Lakatos Date: Thu, 18 Jun 2026 10:16:49 -0400 Subject: [PATCH 15/15] Refs #39339 - fix icon title if no state --- webpack/ForemanTasks/Components/common/taskResultIcon.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webpack/ForemanTasks/Components/common/taskResultIcon.js b/webpack/ForemanTasks/Components/common/taskResultIcon.js index 9dc376835..2af4b8185 100644 --- a/webpack/ForemanTasks/Components/common/taskResultIcon.js +++ b/webpack/ForemanTasks/Components/common/taskResultIcon.js @@ -16,7 +16,7 @@ import { translate as __ } from 'foremanReact/common/I18n'; * @returns {React.ReactElement} */ export const taskResultIconEl = (state, result) => { - if (state !== 'stopped') { + if (state && state !== 'stopped') { return (