From baf6c31383eda35af3ce42f43f97089869974881 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Tue, 26 May 2026 12:48:05 +0200 Subject: [PATCH 01/35] Wait for previous poll response before scheduling the next one Replace setInterval-based polling (via withInterval) with a setTimeout chain that only schedules the next request after the previous one completes or fails, preventing request pileup when the server is slow. Co-Authored-By: Claude Opus 4.6 --- .../JobInvocationActions.js | 32 +++++++++++++------ webpack/JobInvocationDetail/index.js | 11 ++++--- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index 131696068..ab8d9dbca 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -2,10 +2,6 @@ import { translate as __, sprintf } from 'foremanReact/common/I18n'; import { foremanUrl } from 'foremanReact/common/helpers'; import { addToast } from 'foremanReact/components/ToastsList'; import { APIActions, get } from 'foremanReact/redux/API'; -import { - stopInterval, - withInterval, -} from 'foremanReact/redux/middlewares/IntervalMiddleware'; import { CANCEL_JOB, CANCEL_RECURRING_LOGIC, @@ -15,14 +11,21 @@ import { UPDATE_JOB, } from './JobInvocationConstants'; -export const getJobInvocation = url => dispatch => { - const fetchData = withInterval( +let pollTimeoutId = null; + +const scheduleNextPoll = (dispatch, url) => { + pollTimeoutId = setTimeout(() => fetchJobInvocation(dispatch, url), 1000); +}; + +const fetchJobInvocation = (dispatch, url) => { + dispatch( get({ key: JOB_INVOCATION_KEY, params: { include_permissions: true, include_hosts: false }, url, + handleSuccess: () => scheduleNextPoll(dispatch, url), handleError: () => { - dispatch(stopInterval(JOB_INVOCATION_KEY)); + pollTimeoutId = null; }, errorToast: ({ response }) => // eslint-disable-next-line camelcase @@ -31,11 +34,20 @@ export const getJobInvocation = url => dispatch => { response?.data?.error?.full_messages || response?.data?.error?.message || 'Error', - }), - 1000 + }) ); +}; + +export const getJobInvocation = url => dispatch => { + stopJobInvocationPolling(); + fetchJobInvocation(dispatch, url); +}; - dispatch(fetchData); +export const stopJobInvocationPolling = () => { + if (pollTimeoutId !== null) { + clearTimeout(pollTimeoutId); + pollTimeoutId = null; + } }; export const updateJob = jobId => dispatch => { diff --git a/webpack/JobInvocationDetail/index.js b/webpack/JobInvocationDetail/index.js index 085aee8f9..71362e6d3 100644 --- a/webpack/JobInvocationDetail/index.js +++ b/webpack/JobInvocationDetail/index.js @@ -11,7 +11,6 @@ import { useDispatch, useSelector } from 'react-redux'; import PageLayout from 'foremanReact/routes/common/PageLayout/PageLayout'; import PropTypes from 'prop-types'; import SkeletonLoader from 'foremanReact/components/common/SkeletonLoader'; -import { stopInterval } from 'foremanReact/redux/middlewares/IntervalMiddleware'; import { STATUS as API_STATUS } from 'foremanReact/constants'; import { selectAPIErrorMessage, @@ -25,7 +24,11 @@ import JobInvocationOverview from './JobInvocationOverview'; import JobInvocationSystemStatusChart from './JobInvocationSystemStatusChart'; import JobInvocationEmptyState from './JobInvocationEmptyState'; import JobInvocationToolbarButtons from './JobInvocationToolbarButtons'; -import { getJobInvocation, getTask } from './JobInvocationActions'; +import { + getJobInvocation, + getTask, + stopJobInvocationPolling, +} from './JobInvocationActions'; import './JobInvocationDetail.scss'; import { DATE_OPTIONS, @@ -84,10 +87,10 @@ const JobInvocationDetailPage = ({ useEffect(() => { dispatch(getJobInvocation(`/api/job_invocations/${id}`)); if (finished && !autoRefresh) { - dispatch(stopInterval(JOB_INVOCATION_KEY)); + stopJobInvocationPolling(); } return () => { - dispatch(stopInterval(JOB_INVOCATION_KEY)); + stopJobInvocationPolling(); }; }, [dispatch, id, finished, autoRefresh]); From 68026f87ddf32f2d44e55d05695e685126b71366 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Tue, 26 May 2026 12:59:07 +0200 Subject: [PATCH 02/35] Fetch permissions only on initial load, not on every poll tick The include_permissions param causes the backend to instantiate an Authorizer and check 3 permissions on every request. These are user permissions that don't change during a page session, so fetch them once on the initial request and skip them on subsequent polls. Co-Authored-By: Claude Opus 4.6 --- webpack/JobInvocationDetail/JobInvocationActions.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index ab8d9dbca..f783866a5 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -17,11 +17,11 @@ const scheduleNextPoll = (dispatch, url) => { pollTimeoutId = setTimeout(() => fetchJobInvocation(dispatch, url), 1000); }; -const fetchJobInvocation = (dispatch, url) => { +const fetchJobInvocation = (dispatch, url, params = {}) => { dispatch( get({ key: JOB_INVOCATION_KEY, - params: { include_permissions: true, include_hosts: false }, + params: { include_hosts: false }, url, handleSuccess: () => scheduleNextPoll(dispatch, url), handleError: () => { @@ -40,7 +40,7 @@ const fetchJobInvocation = (dispatch, url) => { export const getJobInvocation = url => dispatch => { stopJobInvocationPolling(); - fetchJobInvocation(dispatch, url); + fetchJobInvocation(dispatch, url, { include_permissions: true }); }; export const stopJobInvocationPolling = () => { From 068afa5a4bcd2c76bb4aad4a73db2ef049867379 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Tue, 26 May 2026 13:03:08 +0200 Subject: [PATCH 03/35] =?UTF-8?q?Remove=20updateJob=20=E2=80=94=20its=20re?= =?UTF-8?q?sponse=20was=20never=20read?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit updateJob fetched /api/job_invocations/:id into the UPDATE_JOB Redux key, but no selector ever read from that key. The polling loop already refreshes the job state via JOB_INVOCATION_KEY every second, so these extra requests after cancel/abort/recurring actions were wasted. Co-Authored-By: Claude Opus 4.6 --- .../JobInvocationDetail/JobInvocationActions.js | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index f783866a5..ebb9839e0 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -1,5 +1,4 @@ import { translate as __, sprintf } from 'foremanReact/common/I18n'; -import { foremanUrl } from 'foremanReact/common/helpers'; import { addToast } from 'foremanReact/components/ToastsList'; import { APIActions, get } from 'foremanReact/redux/API'; import { @@ -8,7 +7,6 @@ import { CHANGE_ENABLED_RECURRING_LOGIC, GET_TASK, JOB_INVOCATION_KEY, - UPDATE_JOB, } from './JobInvocationConstants'; let pollTimeoutId = null; @@ -50,17 +48,6 @@ export const stopJobInvocationPolling = () => { } }; -export const updateJob = jobId => dispatch => { - const url = foremanUrl(`/api/job_invocations/${jobId}`); - dispatch( - APIActions.get({ - url, - key: UPDATE_JOB, - params: { include_hosts: false }, - }) - ); -}; - export const cancelJob = (jobId, force) => dispatch => { const infoToast = () => force @@ -93,7 +80,6 @@ export const cancelJob = (jobId, force) => dispatch => { message: infoToast(), }) ); - dispatch(updateJob(jobId)); }, }) ); @@ -141,7 +127,6 @@ export const enableRecurringLogic = ( response?.data?.error?.message || 'Unknown error.' ), - handleSuccess: () => dispatch(updateJob(jobId)), }) ); }; @@ -167,7 +152,6 @@ export const cancelRecurringLogic = (recurrenceId, jobId) => dispatch => { response?.data?.error?.message || 'Unknown error.' ), - handleSuccess: () => dispatch(updateJob(jobId)), }) ); }; From b2ec8c2ab0424e19e6a54c6e27f5737e0c116fe9 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Tue, 26 May 2026 13:07:09 +0200 Subject: [PATCH 04/35] Fix host table firing duplicate API calls on mount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The second useEffect called filterApiCall() unconditionally whenever any dependency changed, and again when statusLabel changed — causing 2-3 identical requests on initial load. Track previous values for both initialFilter and statusLabel so filterApiCall fires only once per actual change. Also move setStatus(RESOLVED) inside the JOB_INVOCATION_HOSTS branch of handleResponse so the LIST_TEMPLATE_INVOCATIONS fetch (which doesn't populate table data) doesn't prematurely flip the table out of its loading state. Co-Authored-By: Claude Opus 4.6 --- .../JobInvocationDetail/JobInvocationHostTable.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationHostTable.js b/webpack/JobInvocationDetail/JobInvocationHostTable.js index 9daaafe58..bac06d4b8 100644 --- a/webpack/JobInvocationDetail/JobInvocationHostTable.js +++ b/webpack/JobInvocationDetail/JobInvocationHostTable.js @@ -70,6 +70,7 @@ const JobInvocationHostTable = ({ // Expansive items const [expandedHost, setExpandedHost] = useState(new Set()); const prevStatusLabel = useRef(statusLabel); + const prevFilter = useRef(initialFilter); const [hostInvocationStates, setHostInvocationStates] = useState({}); @@ -159,9 +160,8 @@ const JobInvocationHostTable = ({ setApiResponse(data.data); setAllHostsIds(ids); + setStatus(STATUS_UPPERCASE.RESOLVED); } - - setStatus(STATUS_UPPERCASE.RESOLVED); }, []); // Call hosts data with params @@ -252,10 +252,13 @@ const JobInvocationHostTable = ({ }, [makeApiCall, id, initialFilter, onFilterUpdate]); useEffect(() => { - if (initialFilter !== '') filterApiCall(); + const filterChanged = initialFilter !== prevFilter.current; + const statusChanged = statusLabel !== prevStatusLabel.current; + + prevFilter.current = initialFilter; + prevStatusLabel.current = statusLabel; - if (statusLabel !== prevStatusLabel.current) { - prevStatusLabel.current = statusLabel; + if ((filterChanged || statusChanged) && initialFilter !== '') { filterApiCall(); } }, [initialFilter, statusLabel, id, filterApiCall]); From d3ab2a9af2f272a890bccc147187990f70587ac6 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Tue, 26 May 2026 13:17:33 +0200 Subject: [PATCH 05/35] Include cancellable in job invocation task, drop separate task fetch The detail page fetched /foreman_tasks/api/tasks/:id solely to read available_actions.cancellable. Add cancellable to the task child in the job invocation response instead, eliminating a separate API call. Co-Authored-By: Claude Opus 4.6 --- app/views/api/v2/job_invocations/main.json.rabl | 1 + webpack/JobInvocationDetail/JobInvocationActions.js | 10 ---------- webpack/JobInvocationDetail/JobInvocationSelectors.js | 5 +---- webpack/JobInvocationDetail/index.js | 8 -------- 4 files changed, 2 insertions(+), 22 deletions(-) diff --git a/app/views/api/v2/job_invocations/main.json.rabl b/app/views/api/v2/job_invocations/main.json.rabl index 3e4111a44..56d8de551 100644 --- a/app/views/api/v2/job_invocations/main.json.rabl +++ b/app/views/api/v2/job_invocations/main.json.rabl @@ -34,6 +34,7 @@ end child :task do attributes :id, :state, :started_at + node(:cancellable) { |task| task.cancellable? } end if @template_invocations diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index ebb9839e0..8d0c2fb9b 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -5,7 +5,6 @@ import { CANCEL_JOB, CANCEL_RECURRING_LOGIC, CHANGE_ENABLED_RECURRING_LOGIC, - GET_TASK, JOB_INVOCATION_KEY, } from './JobInvocationConstants'; @@ -85,15 +84,6 @@ export const cancelJob = (jobId, force) => dispatch => { ); }; -export const getTask = taskId => dispatch => { - dispatch( - get({ - key: GET_TASK, - url: `/foreman_tasks/api/tasks/${taskId}`, - }) - ); -}; - export const enableRecurringLogic = ( recurrenceId, enabled, diff --git a/webpack/JobInvocationDetail/JobInvocationSelectors.js b/webpack/JobInvocationDetail/JobInvocationSelectors.js index e43bfd208..c4033c85b 100644 --- a/webpack/JobInvocationDetail/JobInvocationSelectors.js +++ b/webpack/JobInvocationDetail/JobInvocationSelectors.js @@ -5,7 +5,6 @@ import { } from 'foremanReact/redux/API/APISelectors'; import { JOB_INVOCATION_KEY, - GET_TASK, GET_TEMPLATE_INVOCATION, LIST_TEMPLATE_INVOCATIONS, } from './JobInvocationConstants'; @@ -13,10 +12,8 @@ import { export const selectItems = state => selectAPIResponse(state, JOB_INVOCATION_KEY); -export const selectTask = state => selectAPIResponse(state, GET_TASK); - export const selectTaskCancelable = state => - selectTask(state).available_actions?.cancellable || false; + selectItems(state).task?.cancellable || false; export const selectTemplateInvocation = hostID => state => selectAPIResponse(state, `${GET_TEMPLATE_INVOCATION}_${hostID}`); diff --git a/webpack/JobInvocationDetail/index.js b/webpack/JobInvocationDetail/index.js index 71362e6d3..ed9e09c1c 100644 --- a/webpack/JobInvocationDetail/index.js +++ b/webpack/JobInvocationDetail/index.js @@ -26,7 +26,6 @@ import JobInvocationEmptyState from './JobInvocationEmptyState'; import JobInvocationToolbarButtons from './JobInvocationToolbarButtons'; import { getJobInvocation, - getTask, stopJobInvocationPolling, } from './JobInvocationActions'; import './JobInvocationDetail.scss'; @@ -94,13 +93,6 @@ const JobInvocationDetailPage = ({ }; }, [dispatch, id, finished, autoRefresh]); - const taskId = task?.id; - useEffect(() => { - if (taskId !== undefined) { - dispatch(getTask(`${taskId}`)); - } - }, [dispatch, taskId]); - const apiFailed = jobInvocationApiStatus === API_STATUS.ERROR; const backendErrorMessage = useMemo(() => { From 46bd375cb6809959eddcb320913e7bc13b552d8b Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Tue, 26 May 2026 13:21:29 +0200 Subject: [PATCH 06/35] Replace setInterval with setTimeout chain in TemplateInvocation polling Co-Authored-By: Claude Opus 4.6 --- .../JobInvocationDetail/TemplateInvocation.js | 48 +++++++++---------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/webpack/JobInvocationDetail/TemplateInvocation.js b/webpack/JobInvocationDetail/TemplateInvocation.js index 9045be218..03575ccd6 100644 --- a/webpack/JobInvocationDetail/TemplateInvocation.js +++ b/webpack/JobInvocationDetail/TemplateInvocation.js @@ -67,7 +67,6 @@ export const TemplateInvocation = ({ showCommand, setShowCommand, }) => { - const intervalRef = useRef(null); const templateURL = showTemplateInvocationUrl(hostID, jobID); const hostDetailsPageUrl = useForemanHostDetailsPageUrl(); @@ -75,48 +74,45 @@ export const TemplateInvocation = ({ const response = useSelector(selectTemplateInvocation(hostID)); const dispatch = useDispatch(); - const responseRef = useRef(response); - useEffect(() => { - responseRef.current = response; - }, [response]); + const timeoutRef = useRef(null); useEffect(() => { + const scheduleNextPoll = () => { + timeoutRef.current = setTimeout(() => dispatchFetch(), 5000); + }; + const dispatchFetch = () => { dispatch( APIActions.get({ url: templateURL, key: `${GET_TEMPLATE_INVOCATION}_${hostID}`, + handleSuccess: ({ data }) => { + const finished = data?.finished ?? true; + // eslint-disable-next-line camelcase + const autoRefresh = data?.auto_refresh || false; + if (!finished && autoRefresh) { + scheduleNextPoll(); + } + }, + handleError: () => { + timeoutRef.current = null; + }, }) ); }; - if (intervalRef.current) { - clearInterval(intervalRef.current); - intervalRef.current = null; + if (timeoutRef.current) { + clearTimeout(timeoutRef.current); + timeoutRef.current = null; } if (isExpanded) { - if (isEmpty(responseRef.current)) { - dispatchFetch(); - } - - intervalRef.current = setInterval(() => { - const latestResponse = responseRef.current; - const finished = latestResponse?.finished ?? true; - // eslint-disable-next-line camelcase - const autoRefresh = latestResponse?.auto_refresh || false; - - if (!finished && autoRefresh) { - dispatchFetch(); - } else if (intervalRef.current) { - clearInterval(intervalRef.current); - } - }, 5000); + dispatchFetch(); } return () => { - if (intervalRef.current) { - clearInterval(intervalRef.current); + if (timeoutRef.current) { + clearTimeout(timeoutRef.current); } }; }, [isExpanded, dispatch, templateURL, hostID]); From 951c03d2925ce4b9583a34c2fe4d2a71e6a6d492 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Tue, 26 May 2026 13:32:29 +0200 Subject: [PATCH 07/35] Stop job invocation polling when the job finishes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handleSuccess unconditionally rescheduled the next poll, so the useEffect's stopJobInvocationPolling call raced against in-flight requests — the cleared timeout was immediately replaced by the completing fetch. Check the response data inside handleSuccess instead, and simplify the effect to only handle unmount cleanup. Co-Authored-By: Claude Opus 4.6 --- .../JobInvocationActions.js | 18 +++++++++++++++++- webpack/JobInvocationDetail/index.js | 5 +---- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index 8d0c2fb9b..cfa24137e 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -6,10 +6,20 @@ import { CANCEL_RECURRING_LOGIC, CHANGE_ENABLED_RECURRING_LOGIC, JOB_INVOCATION_KEY, + STATUS, } from './JobInvocationConstants'; let pollTimeoutId = null; +const isJobFinished = ({ status_label: statusLabel, task }) => { + const finished = + statusLabel === STATUS.FAILED || + statusLabel === STATUS.SUCCEEDED || + statusLabel === STATUS.CANCELLED; + const autoRefresh = task?.state === STATUS.PENDING || false; + return finished && !autoRefresh; +}; + const scheduleNextPoll = (dispatch, url) => { pollTimeoutId = setTimeout(() => fetchJobInvocation(dispatch, url), 1000); }; @@ -20,7 +30,13 @@ const fetchJobInvocation = (dispatch, url, params = {}) => { key: JOB_INVOCATION_KEY, params: { include_hosts: false }, url, - handleSuccess: () => scheduleNextPoll(dispatch, url), + handleSuccess: ({ data }) => { + if (!isJobFinished(data)) { + scheduleNextPoll(dispatch, url); + } else { + pollTimeoutId = null; + } + }, handleError: () => { pollTimeoutId = null; }, diff --git a/webpack/JobInvocationDetail/index.js b/webpack/JobInvocationDetail/index.js index ed9e09c1c..ae34b34b9 100644 --- a/webpack/JobInvocationDetail/index.js +++ b/webpack/JobInvocationDetail/index.js @@ -85,13 +85,10 @@ const JobInvocationDetailPage = ({ useEffect(() => { dispatch(getJobInvocation(`/api/job_invocations/${id}`)); - if (finished && !autoRefresh) { - stopJobInvocationPolling(); - } return () => { stopJobInvocationPolling(); }; - }, [dispatch, id, finished, autoRefresh]); + }, [dispatch, id]); const apiFailed = jobInvocationApiStatus === API_STATUS.ERROR; From 26252f33ba58f1795df450d6ea90379e7ff46763 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Thu, 2 Jul 2026 15:35:20 +0200 Subject: [PATCH 08/35] =?UTF-8?q?Drop=20dead=20autoRefresh=20logic=20?= =?UTF-8?q?=E2=80=94=20status=5Flabel=20already=20implies=20task=20state?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit status_label is derived from task.state: it reaches a terminal value (failed/succeeded/cancelled) only when task.state == 'stopped', so checking task.state separately adds nothing. Remove the autoRefresh variable and the unused finished/autoRefresh props passed to JobInvocationHostTable. Co-Authored-By: Claude Sonnet 4.6 --- webpack/JobInvocationDetail/JobInvocationActions.js | 12 ++++-------- webpack/JobInvocationDetail/index.js | 13 ++++++------- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index cfa24137e..5b6ebb197 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -11,14 +11,10 @@ import { let pollTimeoutId = null; -const isJobFinished = ({ status_label: statusLabel, task }) => { - const finished = - statusLabel === STATUS.FAILED || - statusLabel === STATUS.SUCCEEDED || - statusLabel === STATUS.CANCELLED; - const autoRefresh = task?.state === STATUS.PENDING || false; - return finished && !autoRefresh; -}; +const isJobFinished = ({ status_label: statusLabel }) => + statusLabel === STATUS.FAILED || + statusLabel === STATUS.SUCCEEDED || + statusLabel === STATUS.CANCELLED; const scheduleNextPoll = (dispatch, url) => { pollTimeoutId = setTimeout(() => fetchJobInvocation(dispatch, url), 1000); diff --git a/webpack/JobInvocationDetail/index.js b/webpack/JobInvocationDetail/index.js index ae34b34b9..604ab560a 100644 --- a/webpack/JobInvocationDetail/index.js +++ b/webpack/JobInvocationDetail/index.js @@ -17,6 +17,7 @@ import { selectAPIHttpStatus, selectAPIStatus, } from 'foremanReact/redux/API/APISelectors'; +import { useAPI } from 'foremanReact/common/hooks/API/APIHooks'; import { JobAdditionInfo } from './JobAdditionInfo'; import JobInvocationHostTable from './JobInvocationHostTable'; @@ -30,10 +31,12 @@ import { } from './JobInvocationActions'; import './JobInvocationDetail.scss'; import { + CURRENT_PERMISSIONS, DATE_OPTIONS, JOB_INVOCATION_KEY, STATUS, STATUS_UPPERCASE, + currentPermissionsUrl, } from './JobInvocationConstants'; import { selectItems } from './JobInvocationSelectors'; @@ -52,11 +55,6 @@ const JobInvocationDetailPage = ({ start_at: startAt, targeting = {}, } = items; - const finished = - statusLabel === STATUS.FAILED || - statusLabel === STATUS.SUCCEEDED || - statusLabel === STATUS.CANCELLED; - const autoRefresh = task?.state === STATUS.PENDING || false; const jobInvocationApiStatus = useSelector(state => selectAPIStatus(state, JOB_INVOCATION_KEY) ); @@ -66,6 +64,9 @@ const JobInvocationDetailPage = ({ const jobInvocationHttpStatus = useSelector(state => selectAPIHttpStatus(state, JOB_INVOCATION_KEY) ); + useAPI('get', currentPermissionsUrl, { + key: CURRENT_PERMISSIONS, + }); const [selectedFilter, setSelectedFilter] = useState(''); const handleFilterChange = newFilter => { @@ -208,8 +209,6 @@ const JobInvocationDetailPage = ({ Date: Thu, 2 Jul 2026 15:41:38 +0200 Subject: [PATCH 09/35] Fix include_permissions being dropped from initial job invocation fetch fetchJobInvocation hardcoded params: { include_hosts: false } and ignored its params argument. Spread the argument so the initial load can pass include_permissions: true while subsequent polls omit it. Co-Authored-By: Claude Sonnet 4.6 --- webpack/JobInvocationDetail/JobInvocationActions.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index 5b6ebb197..b70ee57ea 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -24,7 +24,7 @@ const fetchJobInvocation = (dispatch, url, params = {}) => { dispatch( get({ key: JOB_INVOCATION_KEY, - params: { include_hosts: false }, + params: { include_hosts: false, ...params }, url, handleSuccess: ({ data }) => { if (!isJobFinished(data)) { From 8ffd5236f8485a2fe70cce17f47bd625fd18c4eb Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Thu, 2 Jul 2026 15:54:33 +0200 Subject: [PATCH 10/35] Add tests for job invocation polling behaviour Covers: initial fetch params (include_permissions + include_hosts), poll scheduling while running, stopping on all terminal statuses (succeeded/failed/cancelled) and on error, stopJobInvocationPolling cancelling a pending timeout, and re-entrant getJobInvocation cancelling the previous poll. Co-Authored-By: Claude Sonnet 4.6 --- .../__tests__/JobInvocationPolling.test.js | 158 ++++++++++++++++++ 1 file changed, 158 insertions(+) create mode 100644 webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js diff --git a/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js b/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js new file mode 100644 index 000000000..4a6e14ce2 --- /dev/null +++ b/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js @@ -0,0 +1,158 @@ +import configureMockStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import * as api from 'foremanReact/redux/API'; +import { + getJobInvocation, + stopJobInvocationPolling, +} from '../JobInvocationActions'; +import { JOB_INVOCATION_KEY } from '../JobInvocationConstants'; + +jest.spyOn(api, 'get'); +jest.useFakeTimers(); + +const mockStore = configureMockStore([thunk]); + +const runningData = { status_label: 'running' }; +const succeededData = { status_label: 'succeeded' }; +const failedData = { status_label: 'failed' }; +const cancelledData = { status_label: 'cancelled' }; + +const setupGetMock = responseData => { + api.get.mockImplementation(({ handleSuccess, ...action }) => { + handleSuccess && handleSuccess({ data: responseData }); + return { type: 'get', ...action }; + }); +}; + +const url = '/api/job_invocations/1'; + +describe('job invocation polling', () => { + beforeEach(() => { + api.get.mockReset(); + stopJobInvocationPolling(); + jest.clearAllTimers(); + }); + + it('sends include_permissions and include_hosts on the initial fetch', () => { + setupGetMock(succeededData); + const store = mockStore({}); + + store.dispatch(getJobInvocation(url)); + + expect(api.get).toHaveBeenCalledTimes(1); + expect(api.get.mock.calls[0][0]).toMatchObject({ + key: JOB_INVOCATION_KEY, + url, + params: { include_hosts: false, include_permissions: true }, + }); + }); + + it('schedules the next poll when job is still running', () => { + setupGetMock(runningData); + const store = mockStore({}); + + store.dispatch(getJobInvocation(url)); + expect(api.get).toHaveBeenCalledTimes(1); + + jest.advanceTimersByTime(1000); + expect(api.get).toHaveBeenCalledTimes(2); + }); + + it('does not send include_permissions on subsequent poll requests', () => { + setupGetMock(runningData); + const store = mockStore({}); + + store.dispatch(getJobInvocation(url)); + jest.advanceTimersByTime(1000); + + expect(api.get).toHaveBeenCalledTimes(2); + expect(api.get.mock.calls[1][0].params).not.toHaveProperty( + 'include_permissions' + ); + expect(api.get.mock.calls[1][0].params).toMatchObject({ + include_hosts: false, + }); + }); + + it('stops polling when job succeeds', () => { + setupGetMock(succeededData); + const store = mockStore({}); + + store.dispatch(getJobInvocation(url)); + expect(api.get).toHaveBeenCalledTimes(1); + + jest.advanceTimersByTime(5000); + expect(api.get).toHaveBeenCalledTimes(1); + }); + + it('stops polling when job fails', () => { + setupGetMock(failedData); + const store = mockStore({}); + + store.dispatch(getJobInvocation(url)); + jest.advanceTimersByTime(5000); + + expect(api.get).toHaveBeenCalledTimes(1); + }); + + it('stops polling when job is cancelled', () => { + setupGetMock(cancelledData); + const store = mockStore({}); + + store.dispatch(getJobInvocation(url)); + jest.advanceTimersByTime(5000); + + expect(api.get).toHaveBeenCalledTimes(1); + }); + + it('stops polling on fetch error', () => { + api.get.mockImplementation(({ handleError, ...action }) => { + handleError && handleError(); + return { type: 'get', ...action }; + }); + const store = mockStore({}); + + store.dispatch(getJobInvocation(url)); + jest.advanceTimersByTime(5000); + + expect(api.get).toHaveBeenCalledTimes(1); + }); + + it('stopJobInvocationPolling cancels a pending timeout', () => { + setupGetMock(runningData); + const store = mockStore({}); + + store.dispatch(getJobInvocation(url)); + expect(api.get).toHaveBeenCalledTimes(1); + + stopJobInvocationPolling(); + jest.advanceTimersByTime(5000); + + expect(api.get).toHaveBeenCalledTimes(1); + }); + + it('getJobInvocation cancels any existing poll before starting a new one', () => { + setupGetMock(runningData); + const store = mockStore({}); + + store.dispatch(getJobInvocation(url)); + expect(api.get).toHaveBeenCalledTimes(1); + + // Second call before the timeout fires should cancel the pending poll + store.dispatch(getJobInvocation(url)); + jest.advanceTimersByTime(1000); + + // Only one more poll should fire (from the second invocation), not two + expect(api.get).toHaveBeenCalledTimes(3); + }); + + it('keeps polling as long as the job is running', () => { + setupGetMock(runningData); + const store = mockStore({}); + + store.dispatch(getJobInvocation(url)); + jest.advanceTimersByTime(3000); + + expect(api.get).toHaveBeenCalledTimes(4); + }); +}); From 21b9c7ac1ee044d961863144cdc86ac1467c6a95 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Thu, 2 Jul 2026 16:03:58 +0200 Subject: [PATCH 11/35] Apply simplifications from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Inline scheduleNextPoll into handleSuccess in TemplateInvocation — removes a one-use helper and makes the self-scheduling pattern direct - Update prevFilter/prevStatusLabel refs only when the guard passes, not unconditionally before it - Remove dead UPDATE_JOB and GET_TASK constants Co-Authored-By: Claude Sonnet 4.6 --- webpack/JobInvocationDetail/JobInvocationConstants.js | 2 +- webpack/JobInvocationDetail/JobInvocationHostTable.js | 5 ++--- webpack/JobInvocationDetail/TemplateInvocation.js | 6 +----- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationConstants.js b/webpack/JobInvocationDetail/JobInvocationConstants.js index 12d6deb11..5be3f31e3 100644 --- a/webpack/JobInvocationDetail/JobInvocationConstants.js +++ b/webpack/JobInvocationDetail/JobInvocationConstants.js @@ -7,8 +7,8 @@ import JobStatusIcon from '../react_app/components/RecentJobsCard/JobStatusIcon' export const JOB_INVOCATION_KEY = 'JOB_INVOCATION_KEY'; export const UPDATE_JOB = 'UPDATE_JOB'; +export const CURRENT_PERMISSIONS = 'CURRENT_PERMISSIONS'; export const CANCEL_JOB = 'CANCEL_JOB'; -export const GET_TASK = 'GET_TASK'; export const GET_TEMPLATE_INVOCATIONS = 'GET_TEMPLATE_INVOCATIONS'; export const CHANGE_ENABLED_RECURRING_LOGIC = 'CHANGE_ENABLED_RECURRING_LOGIC'; export const CANCEL_RECURRING_LOGIC = 'CANCEL_RECURRING_LOGIC'; diff --git a/webpack/JobInvocationDetail/JobInvocationHostTable.js b/webpack/JobInvocationDetail/JobInvocationHostTable.js index bac06d4b8..f3a53d88d 100644 --- a/webpack/JobInvocationDetail/JobInvocationHostTable.js +++ b/webpack/JobInvocationDetail/JobInvocationHostTable.js @@ -255,10 +255,9 @@ const JobInvocationHostTable = ({ const filterChanged = initialFilter !== prevFilter.current; const statusChanged = statusLabel !== prevStatusLabel.current; - prevFilter.current = initialFilter; - prevStatusLabel.current = statusLabel; - if ((filterChanged || statusChanged) && initialFilter !== '') { + prevFilter.current = initialFilter; + prevStatusLabel.current = statusLabel; filterApiCall(); } }, [initialFilter, statusLabel, id, filterApiCall]); diff --git a/webpack/JobInvocationDetail/TemplateInvocation.js b/webpack/JobInvocationDetail/TemplateInvocation.js index 03575ccd6..9e428b729 100644 --- a/webpack/JobInvocationDetail/TemplateInvocation.js +++ b/webpack/JobInvocationDetail/TemplateInvocation.js @@ -77,10 +77,6 @@ export const TemplateInvocation = ({ const timeoutRef = useRef(null); useEffect(() => { - const scheduleNextPoll = () => { - timeoutRef.current = setTimeout(() => dispatchFetch(), 5000); - }; - const dispatchFetch = () => { dispatch( APIActions.get({ @@ -91,7 +87,7 @@ export const TemplateInvocation = ({ // eslint-disable-next-line camelcase const autoRefresh = data?.auto_refresh || false; if (!finished && autoRefresh) { - scheduleNextPoll(); + timeoutRef.current = setTimeout(dispatchFetch, 5000); } }, handleError: () => { From 4e0970249438f0eaa655fe555d5c7a5225e79860 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Thu, 2 Jul 2026 16:53:22 +0200 Subject: [PATCH 12/35] Fix TemplateInvocation poll race and drop dead jobId params MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a cancelled flag inside the TemplateInvocation useEffect closure so that handleSuccess/handleError callbacks from in-flight fetches are ignored after the effect tears down. Without this, a handleError arriving after a fast isExpanded toggle could null out the new live timeoutRef, silently breaking polling. Also remove the now-unused jobId parameter from enableRecurringLogic and cancelRecurringLogic — it was only ever passed to the removed updateJob dispatch. Co-Authored-By: Claude Sonnet 4.6 --- webpack/JobInvocationDetail/JobInvocationActions.js | 8 ++------ .../JobInvocationDetail/JobInvocationToolbarButtons.js | 6 +++--- webpack/JobInvocationDetail/TemplateInvocation.js | 8 +++++--- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index b70ee57ea..2a921b597 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -96,11 +96,7 @@ export const cancelJob = (jobId, force) => dispatch => { ); }; -export const enableRecurringLogic = ( - recurrenceId, - enabled, - jobId -) => dispatch => { +export const enableRecurringLogic = (recurrenceId, enabled) => dispatch => { const successToast = () => enabled ? sprintf(__('Recurring logic %s disabled successfully.'), recurrenceId) @@ -133,7 +129,7 @@ export const enableRecurringLogic = ( ); }; -export const cancelRecurringLogic = (recurrenceId, jobId) => dispatch => { +export const cancelRecurringLogic = recurrenceId => dispatch => { const successToast = () => sprintf(__('Recurring logic %s cancelled successfully.'), recurrenceId); const errorToast = response => diff --git a/webpack/JobInvocationDetail/JobInvocationToolbarButtons.js b/webpack/JobInvocationDetail/JobInvocationToolbarButtons.js index 5fadf906b..0e9805483 100644 --- a/webpack/JobInvocationDetail/JobInvocationToolbarButtons.js +++ b/webpack/JobInvocationDetail/JobInvocationToolbarButtons.js @@ -136,7 +136,7 @@ const JobInvocationToolbarButtons = ({ jobId, data }) => { ouiaId="change-enabled-recurring-dropdown-item" onClick={() => dispatch( - enableRecurringLogic(recurrence?.id, recurringEnabled, jobId) + enableRecurringLogic(recurrence?.id, recurringEnabled) ) } key="change-enabled-recurring" @@ -154,7 +154,7 @@ const JobInvocationToolbarButtons = ({ jobId, data }) => { - dispatch(cancelRecurringLogic(recurrence?.id, jobId)) + dispatch(cancelRecurringLogic(recurrence?.id)) } key="cancel-recurring" component="button" @@ -168,7 +168,7 @@ const JobInvocationToolbarButtons = ({ jobId, data }) => { , ] : [], - [recurrence, recurringEnabled, canEditRecurringLogic, dispatch, jobId] + [recurrence, recurringEnabled, canEditRecurringLogic, dispatch] ); const dropdownItems = useMemo( diff --git a/webpack/JobInvocationDetail/TemplateInvocation.js b/webpack/JobInvocationDetail/TemplateInvocation.js index 9e428b729..a3c500098 100644 --- a/webpack/JobInvocationDetail/TemplateInvocation.js +++ b/webpack/JobInvocationDetail/TemplateInvocation.js @@ -77,12 +77,14 @@ export const TemplateInvocation = ({ const timeoutRef = useRef(null); useEffect(() => { + let cancelled = false; const dispatchFetch = () => { dispatch( APIActions.get({ url: templateURL, key: `${GET_TEMPLATE_INVOCATION}_${hostID}`, handleSuccess: ({ data }) => { + if (cancelled) return; const finished = data?.finished ?? true; // eslint-disable-next-line camelcase const autoRefresh = data?.auto_refresh || false; @@ -91,6 +93,7 @@ export const TemplateInvocation = ({ } }, handleError: () => { + if (cancelled) return; timeoutRef.current = null; }, }) @@ -107,9 +110,8 @@ export const TemplateInvocation = ({ } return () => { - if (timeoutRef.current) { - clearTimeout(timeoutRef.current); - } + cancelled = true; + clearTimeout(timeoutRef.current); }; }, [isExpanded, dispatch, templateURL, hostID]); From 4cc54293c50797ee729826c9c4178d9730d7cf3f Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Thu, 2 Jul 2026 17:22:49 +0200 Subject: [PATCH 13/35] Make lints happy --- app/views/api/v2/job_invocations/main.json.rabl | 2 +- webpack/JobInvocationDetail/index.js | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/views/api/v2/job_invocations/main.json.rabl b/app/views/api/v2/job_invocations/main.json.rabl index 56d8de551..10e965aca 100644 --- a/app/views/api/v2/job_invocations/main.json.rabl +++ b/app/views/api/v2/job_invocations/main.json.rabl @@ -34,7 +34,7 @@ end child :task do attributes :id, :state, :started_at - node(:cancellable) { |task| task.cancellable? } + node(:cancellable, &:cancellable?) end if @template_invocations diff --git a/webpack/JobInvocationDetail/index.js b/webpack/JobInvocationDetail/index.js index 604ab560a..0a87d3113 100644 --- a/webpack/JobInvocationDetail/index.js +++ b/webpack/JobInvocationDetail/index.js @@ -34,7 +34,6 @@ import { CURRENT_PERMISSIONS, DATE_OPTIONS, JOB_INVOCATION_KEY, - STATUS, STATUS_UPPERCASE, currentPermissionsUrl, } from './JobInvocationConstants'; From a698e44010c5d2d79197c366f983dfea95694286 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Fri, 3 Jul 2026 13:07:59 +0200 Subject: [PATCH 14/35] Poll host table while job is running Adds a self-scheduling setTimeout chain in JobInvocationHostTable that refreshes the host list every 5s while the job is not finished. Uses a ref for statusLabel so the poll callback doesn't need to be recreated on every job status update. filterApiCall cancels any pending poll before issuing a manual fetch to avoid races. Co-Authored-By: Claude Sonnet 4.6 --- .../JobInvocationHostTable.js | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/webpack/JobInvocationDetail/JobInvocationHostTable.js b/webpack/JobInvocationDetail/JobInvocationHostTable.js index f3a53d88d..71451143c 100644 --- a/webpack/JobInvocationDetail/JobInvocationHostTable.js +++ b/webpack/JobInvocationDetail/JobInvocationHostTable.js @@ -40,6 +40,7 @@ import DropdownFilter from './DropdownFilter'; import Columns, { JOB_INVOCATION_HOSTS, LIST_TEMPLATE_INVOCATIONS, + STATUS, STATUS_UPPERCASE, ALL_JOB_HOSTS, AWAITING_STATUS_FILTER, @@ -71,6 +72,12 @@ const JobInvocationHostTable = ({ const [expandedHost, setExpandedHost] = useState(new Set()); const prevStatusLabel = useRef(statusLabel); const prevFilter = useRef(initialFilter); + const pollTimeoutId = useRef(null); + const currentPollParams = useRef({}); + const statusLabelRef = useRef(statusLabel); + useEffect(() => { + statusLabelRef.current = statusLabel; + }, [statusLabel]); const [hostInvocationStates, setHostInvocationStates] = useState({}); @@ -154,6 +161,42 @@ const JobInvocationHostTable = ({ [initialFilter, urlSearchQuery] ); + const isJobFinished = () => { + const label = statusLabelRef.current; + return ( + label === STATUS.FAILED || + label === STATUS.SUCCEEDED || + label === STATUS.CANCELLED + ); + }; + + const pollHostTable = useCallback(() => { + dispatch( + APIActions.get({ + key: JOB_INVOCATION_HOSTS, + url: `/api/job_invocations/${id}/hosts`, + params: currentPollParams.current, + handleSuccess: data => { + const ids = data.data.results.map(i => i.id); + setApiResponse(data.data); + setAllHostsIds(ids); + setStatus(STATUS_UPPERCASE.RESOLVED); + if (!isJobFinished()) { + pollTimeoutId.current = setTimeout(pollHostTable, 5000); + } else { + pollTimeoutId.current = null; + } + }, + handleError: () => { + pollTimeoutId.current = null; + setStatus(STATUS_UPPERCASE.ERROR); + }, + errorToast: ({ response }) => + response?.data?.error?.full_messages?.[0] || response, + }) + ); + }, [dispatch, id]); + const handleResponse = useCallback((data, key) => { if (key === JOB_INVOCATION_HOSTS) { const ids = data.data.results.map(i => i.id); @@ -202,6 +245,12 @@ const JobInvocationHostTable = ({ finalParams.search = filterSearch; } + currentPollParams.current = finalParams; + if (pollTimeoutId.current !== null) { + clearTimeout(pollTimeoutId.current); + pollTimeoutId.current = null; + } + makeApiCall(finalParams, { key: JOB_INVOCATION_HOSTS }); const urlSearchParams = new URLSearchParams(window.location.search); @@ -262,6 +311,19 @@ const JobInvocationHostTable = ({ } }, [initialFilter, statusLabel, id, filterApiCall]); + useEffect(() => { + if (initialFilter !== '' && !isJobFinished()) { + pollTimeoutId.current = setTimeout(pollHostTable, 5000); + } + return () => { + if (pollTimeoutId.current !== null) { + clearTimeout(pollTimeoutId.current); + pollTimeoutId.current = null; + } + }; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [initialFilter, pollHostTable]); + const { updateSearchQuery: updateSearchQueryBulk, fetchBulkParams, From 161c5d0eb788179efa4ee471ed8e1e6213af6f85 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Fri, 3 Jul 2026 13:13:06 +0200 Subject: [PATCH 15/35] Refresh LIST_TEMPLATE_INVOCATIONS on page change and poll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RowActions reads permissions from selectTemplateInvocationList which is keyed by LIST_TEMPLATE_INVOCATIONS. It was only fetched once on mount (page 1 hosts), so switching pages left it stale — RowActions returned null for any host not in the initial result, hiding the actions column. Now filterApiCall and pollHostTable both refresh LIST_TEMPLATE_INVOCATIONS alongside the main hosts fetch, keeping it in sync with whatever page is currently visible. Co-Authored-By: Claude Sonnet 4.6 --- .../JobInvocationDetail/JobInvocationHostTable.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/webpack/JobInvocationDetail/JobInvocationHostTable.js b/webpack/JobInvocationDetail/JobInvocationHostTable.js index 71451143c..59b36e6d1 100644 --- a/webpack/JobInvocationDetail/JobInvocationHostTable.js +++ b/webpack/JobInvocationDetail/JobInvocationHostTable.js @@ -195,6 +195,13 @@ const JobInvocationHostTable = ({ response?.data?.error?.full_messages?.[0] || response, }) ); + dispatch( + APIActions.get({ + key: LIST_TEMPLATE_INVOCATIONS, + url: `/job_invocations/${id}/hosts`, + params: currentPollParams.current, + }) + ); }, [dispatch, id]); const handleResponse = useCallback((data, key) => { @@ -252,6 +259,10 @@ const JobInvocationHostTable = ({ } makeApiCall(finalParams, { key: JOB_INVOCATION_HOSTS }); + makeApiCall(finalParams, { + url: `/job_invocations/${id}/hosts`, + key: LIST_TEMPLATE_INVOCATIONS, + }); const urlSearchParams = new URLSearchParams(window.location.search); @@ -262,6 +273,7 @@ const JobInvocationHostTable = ({ history.push({ search: urlSearchParams.toString() }); }, [ + id, initialFilter, urlSearchQuery, defaultParams, From 6676267b42dce970f56db82bbc49b3216d190f01 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Fri, 3 Jul 2026 13:29:59 +0200 Subject: [PATCH 16/35] Include task and permissions in API hosts response The RowActions component needs per-host task and permissions data to render the actions column. Previously this came from a separate non-API endpoint (list_jobs_hosts) fetched once on mount, causing the actions column to disappear on page switches since the stale data had no entries for hosts on pages 2+. Now set_statuses_and_smart_proxies builds @task_by_host and @permissions_by_host from the already-loaded template invocations and exposes them in the hosts.json.rabl response. RowActions reads directly from the paginated JOB_INVOCATION_HOSTS Redux key instead of a separate LIST_TEMPLATE_INVOCATIONS key. Drops ALL_JOB_HOSTS, LIST_TEMPLATE_INVOCATIONS, handleResponse, selectTemplateInvocationList, and the non-API /job_invocations/:id/hosts fetch entirely. Co-Authored-By: Claude Sonnet 4.6 --- .../api/v2/job_invocations_controller.rb | 13 +++++ .../api/v2/job_invocations/hosts.json.rabl | 8 +++ .../JobInvocationConstants.js | 3 -- .../JobInvocationHostTable.js | 52 +++++-------------- .../JobInvocationSelectors.js | 20 +++++++ .../TemplateActionButtons.js | 4 +- 6 files changed, 55 insertions(+), 45 deletions(-) diff --git a/app/controllers/api/v2/job_invocations_controller.rb b/app/controllers/api/v2/job_invocations_controller.rb index 0bf640111..0113ab81e 100644 --- a/app/controllers/api/v2/job_invocations_controller.rb +++ b/app/controllers/api/v2/job_invocations_controller.rb @@ -329,6 +329,19 @@ def set_statuses_and_smart_proxies end @smart_proxy_id = template_invocations.to_h { |ti| [ti.host_id, ti.smart_proxy_id] } @smart_proxy_name = template_invocations.to_h { |ti| [ti.host_id, ti.smart_proxy_name] } + @task_by_host = template_invocations.to_h do |ti| + task = ti.run_host_job_task + [ti.host_id, task&.attributes&.merge(:cancellable => task.cancellable?)] + end + @permissions_by_host = hosts.to_h do |host| + template_invocation = template_invocations_by_host_id[host.id] + task = template_invocation.try(:run_host_job_task) + [host.id, { + :view_foreman_tasks => authorized_for(:permission => :view_foreman_tasks, :auth_object => task), + :cancel_job_invocations => authorized_for(:permission => :cancel_job_invocations, :auth_object => @job_invocation), + :execute_jobs => authorized_for(controller: :job_invocations, action: :create) && (!host.infrastructure_host? || User.current.can?(:execute_jobs_on_infrastructure_hosts)), + }] + end end end end diff --git a/app/views/api/v2/job_invocations/hosts.json.rabl b/app/views/api/v2/job_invocations/hosts.json.rabl index 7e627dfef..32214a51e 100644 --- a/app/views/api/v2/job_invocations/hosts.json.rabl +++ b/app/views/api/v2/job_invocations/hosts.json.rabl @@ -13,3 +13,11 @@ end node :smart_proxy_name do |host| @smart_proxy_name[host.id] end + +node :task do |host| + @task_by_host[host.id] +end + +node :permissions do |host| + @permissions_by_host[host.id] +end diff --git a/webpack/JobInvocationDetail/JobInvocationConstants.js b/webpack/JobInvocationDetail/JobInvocationConstants.js index 5be3f31e3..51010b6af 100644 --- a/webpack/JobInvocationDetail/JobInvocationConstants.js +++ b/webpack/JobInvocationDetail/JobInvocationConstants.js @@ -17,13 +17,10 @@ export const GET_REPORT_TEMPLATE_INPUTS = 'GET_REPORT_TEMPLATE_INPUTS'; export const JOB_INVOCATION_HOSTS = 'JOB_INVOCATION_HOSTS'; export const GET_TEMPLATE_INVOCATION = 'GET_TEMPLATE_INVOCATION'; export const DIRECT_OPEN_HOST_LIMIT = 3; -export const ALL_JOB_HOSTS = 'ALL_JOB_HOSTS'; export const AWAITING_STATUS_FILTER = '(job_invocation.result = N/A)'; export const showTemplateInvocationUrl = (hostID, jobID) => `/show_template_invocation_by_host/${hostID}/job_invocation/${jobID}`; -export const LIST_TEMPLATE_INVOCATIONS = 'LIST_TEMPLATE_INVOCATIONS'; - export const templateInvocationPageUrl = (hostID, jobID) => `/job_invocations_detail/${jobID}/host_invocation/${hostID}`; diff --git a/webpack/JobInvocationDetail/JobInvocationHostTable.js b/webpack/JobInvocationDetail/JobInvocationHostTable.js index 59b36e6d1..e3cf31d43 100644 --- a/webpack/JobInvocationDetail/JobInvocationHostTable.js +++ b/webpack/JobInvocationDetail/JobInvocationHostTable.js @@ -39,10 +39,8 @@ import { CheckboxesActions } from './CheckboxesActions'; import DropdownFilter from './DropdownFilter'; import Columns, { JOB_INVOCATION_HOSTS, - LIST_TEMPLATE_INVOCATIONS, STATUS, STATUS_UPPERCASE, - ALL_JOB_HOSTS, AWAITING_STATUS_FILTER, } from './JobInvocationConstants'; import { TemplateInvocation } from './TemplateInvocation'; @@ -195,41 +193,29 @@ const JobInvocationHostTable = ({ response?.data?.error?.full_messages?.[0] || response, }) ); - dispatch( - APIActions.get({ - key: LIST_TEMPLATE_INVOCATIONS, - url: `/job_invocations/${id}/hosts`, - params: currentPollParams.current, - }) - ); }, [dispatch, id]); - const handleResponse = useCallback((data, key) => { - if (key === JOB_INVOCATION_HOSTS) { - const ids = data.data.results.map(i => i.id); - - setApiResponse(data.data); - setAllHostsIds(ids); - setStatus(STATUS_UPPERCASE.RESOLVED); - } - }, []); - // Call hosts data with params const makeApiCall = useCallback( - (requestParams, callParams = {}) => { + requestParams => { dispatch( APIActions.get({ - key: callParams.key ?? ALL_JOB_HOSTS, - url: callParams.url ?? `/api/job_invocations/${id}/hosts`, + key: JOB_INVOCATION_HOSTS, + url: `/api/job_invocations/${id}/hosts`, params: requestParams, - handleSuccess: data => handleResponse(data, callParams.key), + handleSuccess: data => { + const ids = data.data.results.map(i => i.id); + setApiResponse(data.data); + setAllHostsIds(ids); + setStatus(STATUS_UPPERCASE.RESOLVED); + }, handleError: () => setStatus(STATUS_UPPERCASE.ERROR), errorToast: ({ response }) => response?.data?.error?.full_messages?.[0] || response, }) ); }, - [dispatch, id, handleResponse] + [dispatch, id] ); const filterApiCall = useCallback( @@ -258,11 +244,7 @@ const JobInvocationHostTable = ({ pollTimeoutId.current = null; } - makeApiCall(finalParams, { key: JOB_INVOCATION_HOSTS }); - makeApiCall(finalParams, { - url: `/job_invocations/${id}/hosts`, - key: LIST_TEMPLATE_INVOCATIONS, - }); + makeApiCall(finalParams); const urlSearchParams = new URLSearchParams(window.location.search); @@ -273,7 +255,6 @@ const JobInvocationHostTable = ({ history.push({ search: urlSearchParams.toString() }); }, [ - id, initialFilter, urlSearchQuery, defaultParams, @@ -296,21 +277,12 @@ const JobInvocationHostTable = ({ const initializedRef = useRef(false); useEffect(() => { if (!initializedRef.current) { - // Job Invo template load - makeApiCall( - {}, - { - url: `/job_invocations/${id}/hosts`, - key: LIST_TEMPLATE_INVOCATIONS, - } - ); - if (initialFilter === '') { onFilterUpdate('all_statuses'); } initializedRef.current = true; } - }, [makeApiCall, id, initialFilter, onFilterUpdate]); + }, [initialFilter, onFilterUpdate]); useEffect(() => { const filterChanged = initialFilter !== prevFilter.current; diff --git a/webpack/JobInvocationDetail/JobInvocationSelectors.js b/webpack/JobInvocationDetail/JobInvocationSelectors.js index c4033c85b..26835ada9 100644 --- a/webpack/JobInvocationDetail/JobInvocationSelectors.js +++ b/webpack/JobInvocationDetail/JobInvocationSelectors.js @@ -5,8 +5,10 @@ import { } from 'foremanReact/redux/API/APISelectors'; import { JOB_INVOCATION_KEY, + JOB_INVOCATION_HOSTS, GET_TEMPLATE_INVOCATION, LIST_TEMPLATE_INVOCATIONS, + CURRENT_PERMISSIONS, } from './JobInvocationConstants'; export const selectItems = state => @@ -24,3 +26,21 @@ export const selectTemplateInvocationStatus = hostID => state => export const selectTemplateInvocationList = state => selectAPIResponse(state, LIST_TEMPLATE_INVOCATIONS) ?.template_invocations_task_by_hosts; + +export const selectHostFromJobInvocationHosts = hostID => state => + selectAPIResponse(state, JOB_INVOCATION_HOSTS)?.results?.find( + h => h.id === hostID + ); + +export const selectCurrentPermisions = state => + selectAPIResponse(state, CURRENT_PERMISSIONS); + +export const selectHasPermission = permissionRequired => state => { + const status = selectAPIStatus(state, CURRENT_PERMISSIONS); + const selectCurrentPermissions = selectCurrentPermisions(state)?.results; + return status === APIStatus.RESOLVED + ? selectCurrentPermissions?.some( + permission => permission.name === permissionRequired + ) + : false; +}; diff --git a/webpack/JobInvocationDetail/TemplateInvocationComponents/TemplateActionButtons.js b/webpack/JobInvocationDetail/TemplateInvocationComponents/TemplateActionButtons.js index f9242f646..2151a7d37 100644 --- a/webpack/JobInvocationDetail/TemplateInvocationComponents/TemplateActionButtons.js +++ b/webpack/JobInvocationDetail/TemplateInvocationComponents/TemplateActionButtons.js @@ -6,7 +6,7 @@ import { ActionsColumn } from '@patternfly/react-table'; import { APIActions } from 'foremanReact/redux/API'; import { addToast } from 'foremanReact/components/ToastsList'; import { translate as __ } from 'foremanReact/common/I18n'; -import { selectTemplateInvocationList } from '../JobInvocationSelectors'; +import { selectHostFromJobInvocationHosts } from '../JobInvocationSelectors'; import './index.scss'; const actions = ({ @@ -83,7 +83,7 @@ const actions = ({ export const RowActions = ({ hostID, jobID }) => { const dispatch = useDispatch(); - const response = useSelector(selectTemplateInvocationList)?.[hostID]; + const response = useSelector(selectHostFromJobInvocationHosts(hostID)); if (!response?.permissions) return null; const { task, permissions } = response; const { id: taskID, cancellable: taskCancellable } = task || {}; From 491fc93626a1cfa5b6d506a0a796ef978642b3f2 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Fri, 3 Jul 2026 13:32:25 +0200 Subject: [PATCH 17/35] =?UTF-8?q?Drop=20list=5Fjobs=5Fhosts=20=E2=80=94=20?= =?UTF-8?q?replaced=20by=20API=20hosts=20endpoint?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task and permissions data is now included in /api/job_invocations/:id/hosts, so the separate non-API list_jobs_hosts action is no longer needed. Co-Authored-By: Claude Sonnet 4.6 --- app/controllers/job_invocations_controller.rb | 29 +------------------ config/routes.rb | 1 - lib/foreman_remote_execution/plugin.rb | 4 +-- 3 files changed, 3 insertions(+), 31 deletions(-) diff --git a/app/controllers/job_invocations_controller.rb b/app/controllers/job_invocations_controller.rb index 5ef6abe06..7f66d40f4 100644 --- a/app/controllers/job_invocations_controller.rb +++ b/app/controllers/job_invocations_controller.rb @@ -149,33 +149,6 @@ def preview_job_invocations_per_host render :json => {:job_invocations => job_invocations} end - def list_jobs_hosts - @job_invocation = resource_base.find(params[:id]) - hosts = @job_invocation.targeting.hosts.authorized(:view_hosts, Host) - hosts = hosts.search_for(params[:search]) - template_invocations_task_by_hosts = {} - hosts.each do |host| - template_invocation = @job_invocation.template_invocations.find { |template_inv| template_inv.host_id == host.id } - next unless template_invocation - template_invocation_task = template_invocation.run_host_job_task - template_invocations_task_by_hosts[host.id] = - { - :host_name => host.name, - :id => host.id, - :task => template_invocation_task.attributes.merge({cancellable: template_invocation_task.cancellable? }), - :permissions => { - :view_foreman_tasks => authorized_for(:permission => :view_foreman_tasks, :auth_object => template_invocation_task), - :cancel_job_invocations => authorized_for(:permission => :cancel_job_invocations, :auth_object => @job_invocation), - :execute_jobs => authorized_for(controller: :job_invocations, action: :create) && (!host.infrastructure_host? || User.current.can?(:execute_jobs_on_infrastructure_hosts)), - }, - } - end - - render json: { - :template_invocations_task_by_hosts => template_invocations_task_by_hosts, - } - end - private def action_permission @@ -186,7 +159,7 @@ def action_permission 'create' when 'cancel' 'cancel' - when 'chart', 'preview_job_invocations_per_host', 'list_jobs_hosts' + when 'chart', 'preview_job_invocations_per_host' 'view' else super diff --git a/config/routes.rb b/config/routes.rb index 5227bde52..558cf5882 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -35,7 +35,6 @@ get 'auto_complete_search' end member do - get 'hosts', to: 'job_invocations#list_jobs_hosts' post 'cancel' end end diff --git a/lib/foreman_remote_execution/plugin.rb b/lib/foreman_remote_execution/plugin.rb index 3dace7a7f..7ef0c3364 100644 --- a/lib/foreman_remote_execution/plugin.rb +++ b/lib/foreman_remote_execution/plugin.rb @@ -134,9 +134,9 @@ permission :lock_job_templates, { :job_templates => [:lock, :unlock] }, :resource_type => 'JobTemplate' permission :create_job_invocations, { :job_invocations => [:new, :create, :legacy_create, :refresh, :rerun, :preview_hosts], 'api/v2/job_invocations' => [:create, :rerun] }, :resource_type => 'JobInvocation' - permission :view_job_invocations, { :job_invocations => [:index, :chart, :show, :auto_complete_search, :preview_job_invocations_per_host, :list_jobs_hosts], :template_invocations => [:show, :show_template_invocation_by_host], + permission :view_job_invocations, { :job_invocations => [:index, :chart, :show, :auto_complete_search, :preview_job_invocations_per_host], :template_invocations => [:show, :show_template_invocation_by_host], 'api/v2/job_invocations' => [:index, :show, :output, :raw_output, :outputs, :hosts] }, :resource_type => 'JobInvocation' - permission :view_template_invocations, { :template_invocations => [:show, :template_invocation_preview, :show_template_invocation_by_host], :job_invocations => [:list_jobs_hosts], + permission :view_template_invocations, { :template_invocations => [:show, :template_invocation_preview, :show_template_invocation_by_host], 'api/v2/template_invocations' => [:template_invocations], :ui_job_wizard => [:job_invocation] }, :resource_type => 'TemplateInvocation' permission :create_template_invocations, {}, :resource_type => 'TemplateInvocation' permission :execute_jobs_on_infrastructure_hosts, {}, :resource_type => 'JobInvocation' From fa93cc0e4b41aca39f6ab5135a73167af5abbbf5 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Fri, 3 Jul 2026 14:05:26 +0200 Subject: [PATCH 18/35] Gate task and permissions in hosts response behind include_permissions param MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-host authorization checks are expensive — skip them unless the caller explicitly requests them with include_permissions=true. Co-Authored-By: Claude Sonnet 4.6 --- .../api/v2/job_invocations_controller.rb | 2 ++ .../api/v2/job_invocations/hosts.json.rabl | 4 ++-- .../JobInvocationActions.js | 2 +- .../JobInvocationHostTable.js | 22 +++++++++---------- .../__tests__/JobInvocationPolling.test.js | 8 +++---- 5 files changed, 20 insertions(+), 18 deletions(-) diff --git a/app/controllers/api/v2/job_invocations_controller.rb b/app/controllers/api/v2/job_invocations_controller.rb index 0113ab81e..86f9c758e 100644 --- a/app/controllers/api/v2/job_invocations_controller.rb +++ b/app/controllers/api/v2/job_invocations_controller.rb @@ -118,6 +118,7 @@ def output add_scoped_search_description_for(JobInvocation) param :id, :identifier, :required => true def hosts + @include_permissions = Foreman::Cast.to_bool(params[:include_permissions]) set_hosts_and_template_invocations @total = @hosts.size @hosts = @hosts.search_for(params[:search], :order => params[:order]).paginate(:page => params[:page], :per_page => params[:per_page]) @@ -329,6 +330,7 @@ def set_statuses_and_smart_proxies end @smart_proxy_id = template_invocations.to_h { |ti| [ti.host_id, ti.smart_proxy_id] } @smart_proxy_name = template_invocations.to_h { |ti| [ti.host_id, ti.smart_proxy_name] } + return unless @include_permissions @task_by_host = template_invocations.to_h do |ti| task = ti.run_host_job_task [ti.host_id, task&.attributes&.merge(:cancellable => task.cancellable?)] diff --git a/app/views/api/v2/job_invocations/hosts.json.rabl b/app/views/api/v2/job_invocations/hosts.json.rabl index 32214a51e..94a77c30f 100644 --- a/app/views/api/v2/job_invocations/hosts.json.rabl +++ b/app/views/api/v2/job_invocations/hosts.json.rabl @@ -14,10 +14,10 @@ node :smart_proxy_name do |host| @smart_proxy_name[host.id] end -node :task do |host| +node(:task, :if => ->(_) { @task_by_host }) do |host| @task_by_host[host.id] end -node :permissions do |host| +node(:permissions, :if => ->(_) { @permissions_by_host }) do |host| @permissions_by_host[host.id] end diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index 2a921b597..1a540ddb8 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -17,7 +17,7 @@ const isJobFinished = ({ status_label: statusLabel }) => statusLabel === STATUS.CANCELLED; const scheduleNextPoll = (dispatch, url) => { - pollTimeoutId = setTimeout(() => fetchJobInvocation(dispatch, url), 1000); + pollTimeoutId = setTimeout(() => fetchJobInvocation(dispatch, url), 5000); }; const fetchJobInvocation = (dispatch, url, params = {}) => { diff --git a/webpack/JobInvocationDetail/JobInvocationHostTable.js b/webpack/JobInvocationDetail/JobInvocationHostTable.js index e3cf31d43..db18a1877 100644 --- a/webpack/JobInvocationDetail/JobInvocationHostTable.js +++ b/webpack/JobInvocationDetail/JobInvocationHostTable.js @@ -173,7 +173,7 @@ const JobInvocationHostTable = ({ APIActions.get({ key: JOB_INVOCATION_HOSTS, url: `/api/job_invocations/${id}/hosts`, - params: currentPollParams.current, + params: { include_permissions: true, ...currentPollParams.current }, handleSuccess: data => { const ids = data.data.results.map(i => i.id); setApiResponse(data.data); @@ -202,12 +202,15 @@ const JobInvocationHostTable = ({ APIActions.get({ key: JOB_INVOCATION_HOSTS, url: `/api/job_invocations/${id}/hosts`, - params: requestParams, + params: { include_permissions: true, ...requestParams }, handleSuccess: data => { const ids = data.data.results.map(i => i.id); setApiResponse(data.data); setAllHostsIds(ids); setStatus(STATUS_UPPERCASE.RESOLVED); + if (!isJobFinished()) { + pollTimeoutId.current = setTimeout(pollHostTable, 5000); + } }, handleError: () => setStatus(STATUS_UPPERCASE.ERROR), errorToast: ({ response }) => @@ -215,7 +218,7 @@ const JobInvocationHostTable = ({ }) ); }, - [dispatch, id] + [dispatch, id, pollHostTable] ); const filterApiCall = useCallback( @@ -295,18 +298,15 @@ const JobInvocationHostTable = ({ } }, [initialFilter, statusLabel, id, filterApiCall]); - useEffect(() => { - if (initialFilter !== '' && !isJobFinished()) { - pollTimeoutId.current = setTimeout(pollHostTable, 5000); - } - return () => { + useEffect( + () => () => { if (pollTimeoutId.current !== null) { clearTimeout(pollTimeoutId.current); pollTimeoutId.current = null; } - }; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [initialFilter, pollHostTable]); + }, + [] + ); const { updateSearchQuery: updateSearchQueryBulk, diff --git a/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js b/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js index 4a6e14ce2..32dc7ae5d 100644 --- a/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js +++ b/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js @@ -54,7 +54,7 @@ describe('job invocation polling', () => { store.dispatch(getJobInvocation(url)); expect(api.get).toHaveBeenCalledTimes(1); - jest.advanceTimersByTime(1000); + jest.advanceTimersByTime(5000); expect(api.get).toHaveBeenCalledTimes(2); }); @@ -63,7 +63,7 @@ describe('job invocation polling', () => { const store = mockStore({}); store.dispatch(getJobInvocation(url)); - jest.advanceTimersByTime(1000); + jest.advanceTimersByTime(5000); expect(api.get).toHaveBeenCalledTimes(2); expect(api.get.mock.calls[1][0].params).not.toHaveProperty( @@ -140,7 +140,7 @@ describe('job invocation polling', () => { // Second call before the timeout fires should cancel the pending poll store.dispatch(getJobInvocation(url)); - jest.advanceTimersByTime(1000); + jest.advanceTimersByTime(5000); // Only one more poll should fire (from the second invocation), not two expect(api.get).toHaveBeenCalledTimes(3); @@ -151,7 +151,7 @@ describe('job invocation polling', () => { const store = mockStore({}); store.dispatch(getJobInvocation(url)); - jest.advanceTimersByTime(3000); + jest.advanceTimersByTime(15000); expect(api.get).toHaveBeenCalledTimes(4); }); From 961586086dca409a1b15a26f3c93c8feda2975b9 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Fri, 3 Jul 2026 14:59:27 +0200 Subject: [PATCH 19/35] Pass jobFinished as prop to JobInvocationHostTable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parent already knows whether the job is done — move isJobFinished out of the table and into JobInvocationActions where the STATUS constants live, export it, and let index.js pass a plain boolean prop down. The table no longer needs to know what constitutes "finished"; it just checks the prop via a ref (same pattern, cleaner responsibility split). Co-Authored-By: Claude Sonnet 4.6 --- .../JobInvocationActions.js | 4 +-- .../JobInvocationHostTable.js | 33 +++++++------------ webpack/JobInvocationDetail/index.js | 3 +- 3 files changed, 16 insertions(+), 24 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index 1a540ddb8..1bb88e21f 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -11,7 +11,7 @@ import { let pollTimeoutId = null; -const isJobFinished = ({ status_label: statusLabel }) => +export const isJobFinished = statusLabel => statusLabel === STATUS.FAILED || statusLabel === STATUS.SUCCEEDED || statusLabel === STATUS.CANCELLED; @@ -27,7 +27,7 @@ const fetchJobInvocation = (dispatch, url, params = {}) => { params: { include_hosts: false, ...params }, url, handleSuccess: ({ data }) => { - if (!isJobFinished(data)) { + if (!isJobFinished(data.status_label)) { scheduleNextPoll(dispatch, url); } else { pollTimeoutId = null; diff --git a/webpack/JobInvocationDetail/JobInvocationHostTable.js b/webpack/JobInvocationDetail/JobInvocationHostTable.js index db18a1877..542396f5c 100644 --- a/webpack/JobInvocationDetail/JobInvocationHostTable.js +++ b/webpack/JobInvocationDetail/JobInvocationHostTable.js @@ -39,7 +39,6 @@ import { CheckboxesActions } from './CheckboxesActions'; import DropdownFilter from './DropdownFilter'; import Columns, { JOB_INVOCATION_HOSTS, - STATUS, STATUS_UPPERCASE, AWAITING_STATUS_FILTER, } from './JobInvocationConstants'; @@ -50,8 +49,8 @@ import { PopupAlert } from './OpenAllInvocationsModal'; const JobInvocationHostTable = ({ id, initialFilter, + jobFinished, onFilterUpdate, - statusLabel, targeting, }) => { const columns = Columns(); @@ -68,14 +67,14 @@ const JobInvocationHostTable = ({ // Expansive items const [expandedHost, setExpandedHost] = useState(new Set()); - const prevStatusLabel = useRef(statusLabel); + const prevJobFinished = useRef(jobFinished); const prevFilter = useRef(initialFilter); const pollTimeoutId = useRef(null); const currentPollParams = useRef({}); - const statusLabelRef = useRef(statusLabel); + const jobFinishedRef = useRef(jobFinished); useEffect(() => { - statusLabelRef.current = statusLabel; - }, [statusLabel]); + jobFinishedRef.current = jobFinished; + }, [jobFinished]); const [hostInvocationStates, setHostInvocationStates] = useState({}); @@ -159,14 +158,6 @@ const JobInvocationHostTable = ({ [initialFilter, urlSearchQuery] ); - const isJobFinished = () => { - const label = statusLabelRef.current; - return ( - label === STATUS.FAILED || - label === STATUS.SUCCEEDED || - label === STATUS.CANCELLED - ); - }; const pollHostTable = useCallback(() => { dispatch( @@ -179,7 +170,7 @@ const JobInvocationHostTable = ({ setApiResponse(data.data); setAllHostsIds(ids); setStatus(STATUS_UPPERCASE.RESOLVED); - if (!isJobFinished()) { + if (!jobFinishedRef.current) { pollTimeoutId.current = setTimeout(pollHostTable, 5000); } else { pollTimeoutId.current = null; @@ -208,7 +199,7 @@ const JobInvocationHostTable = ({ setApiResponse(data.data); setAllHostsIds(ids); setStatus(STATUS_UPPERCASE.RESOLVED); - if (!isJobFinished()) { + if (!jobFinishedRef.current) { pollTimeoutId.current = setTimeout(pollHostTable, 5000); } }, @@ -289,14 +280,14 @@ const JobInvocationHostTable = ({ useEffect(() => { const filterChanged = initialFilter !== prevFilter.current; - const statusChanged = statusLabel !== prevStatusLabel.current; + const statusChanged = jobFinished !== prevJobFinished.current; if ((filterChanged || statusChanged) && initialFilter !== '') { prevFilter.current = initialFilter; - prevStatusLabel.current = statusLabel; + prevJobFinished.current = jobFinished; filterApiCall(); } - }, [initialFilter, statusLabel, id, filterApiCall]); + }, [initialFilter, jobFinished, id, filterApiCall]); useEffect( () => () => { @@ -589,13 +580,13 @@ JobInvocationHostTable.propTypes = { id: PropTypes.string.isRequired, targeting: PropTypes.object.isRequired, initialFilter: PropTypes.string.isRequired, - statusLabel: PropTypes.string, + jobFinished: PropTypes.bool, onFilterUpdate: PropTypes.func, }; JobInvocationHostTable.defaultProps = { onFilterUpdate: () => {}, - statusLabel: undefined, + jobFinished: false, }; export default JobInvocationHostTable; diff --git a/webpack/JobInvocationDetail/index.js b/webpack/JobInvocationDetail/index.js index 0a87d3113..7b87b9a6e 100644 --- a/webpack/JobInvocationDetail/index.js +++ b/webpack/JobInvocationDetail/index.js @@ -27,6 +27,7 @@ import JobInvocationEmptyState from './JobInvocationEmptyState'; import JobInvocationToolbarButtons from './JobInvocationToolbarButtons'; import { getJobInvocation, + isJobFinished, stopJobInvocationPolling, } from './JobInvocationActions'; import './JobInvocationDetail.scss'; @@ -209,7 +210,7 @@ const JobInvocationDetailPage = ({ id={id} targeting={targeting} initialFilter={selectedFilter} - statusLabel={statusLabel} + jobFinished={isJobFinished(statusLabel)} onFilterUpdate={handleFilterChange} /> From 32e7dd77cc19035b0ce34cb5e959056c2b3d0c5c Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Fri, 3 Jul 2026 15:04:01 +0200 Subject: [PATCH 20/35] Simplify polling helpers in actions and host table Inline scheduleNextPoll (single-line wrapper with no benefit) and extract updateHostsState to deduplicate the repeated state-update block shared between pollHostTable and makeApiCall. Co-Authored-By: Claude Sonnet 4.6 --- .../JobInvocationActions.js | 6 +----- .../JobInvocationHostTable.js | 21 ++++++++++--------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index 1bb88e21f..ed7b5230a 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -16,10 +16,6 @@ export const isJobFinished = statusLabel => statusLabel === STATUS.SUCCEEDED || statusLabel === STATUS.CANCELLED; -const scheduleNextPoll = (dispatch, url) => { - pollTimeoutId = setTimeout(() => fetchJobInvocation(dispatch, url), 5000); -}; - const fetchJobInvocation = (dispatch, url, params = {}) => { dispatch( get({ @@ -28,7 +24,7 @@ const fetchJobInvocation = (dispatch, url, params = {}) => { url, handleSuccess: ({ data }) => { if (!isJobFinished(data.status_label)) { - scheduleNextPoll(dispatch, url); + pollTimeoutId = setTimeout(() => fetchJobInvocation(dispatch, url), 5000); } else { pollTimeoutId = null; } diff --git a/webpack/JobInvocationDetail/JobInvocationHostTable.js b/webpack/JobInvocationDetail/JobInvocationHostTable.js index 542396f5c..5aa04df14 100644 --- a/webpack/JobInvocationDetail/JobInvocationHostTable.js +++ b/webpack/JobInvocationDetail/JobInvocationHostTable.js @@ -159,6 +159,13 @@ const JobInvocationHostTable = ({ ); + const updateHostsState = useCallback(data => { + const ids = data.data.results.map(i => i.id); + setApiResponse(data.data); + setAllHostsIds(ids); + setStatus(STATUS_UPPERCASE.RESOLVED); + }, []); + const pollHostTable = useCallback(() => { dispatch( APIActions.get({ @@ -166,10 +173,7 @@ const JobInvocationHostTable = ({ url: `/api/job_invocations/${id}/hosts`, params: { include_permissions: true, ...currentPollParams.current }, handleSuccess: data => { - const ids = data.data.results.map(i => i.id); - setApiResponse(data.data); - setAllHostsIds(ids); - setStatus(STATUS_UPPERCASE.RESOLVED); + updateHostsState(data); if (!jobFinishedRef.current) { pollTimeoutId.current = setTimeout(pollHostTable, 5000); } else { @@ -184,7 +188,7 @@ const JobInvocationHostTable = ({ response?.data?.error?.full_messages?.[0] || response, }) ); - }, [dispatch, id]); + }, [dispatch, id, updateHostsState]); // Call hosts data with params const makeApiCall = useCallback( @@ -195,10 +199,7 @@ const JobInvocationHostTable = ({ url: `/api/job_invocations/${id}/hosts`, params: { include_permissions: true, ...requestParams }, handleSuccess: data => { - const ids = data.data.results.map(i => i.id); - setApiResponse(data.data); - setAllHostsIds(ids); - setStatus(STATUS_UPPERCASE.RESOLVED); + updateHostsState(data); if (!jobFinishedRef.current) { pollTimeoutId.current = setTimeout(pollHostTable, 5000); } @@ -209,7 +210,7 @@ const JobInvocationHostTable = ({ }) ); }, - [dispatch, id, pollHostTable] + [dispatch, id, pollHostTable, updateHostsState] ); const filterApiCall = useCallback( From 7f18a92f26feb593796218e09cb4444d429108d0 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Fri, 3 Jul 2026 15:21:17 +0200 Subject: [PATCH 21/35] Fix polling cleanup inconsistencies and error toast fallbacks Co-Authored-By: Claude Opus 4.6 --- .../JobInvocationActions.js | 6 ++-- .../JobInvocationHostTable.js | 35 +++++++++---------- .../JobInvocationDetail/TemplateInvocation.js | 2 +- 3 files changed, 19 insertions(+), 24 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index ed7b5230a..c69aef1da 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -49,10 +49,8 @@ export const getJobInvocation = url => dispatch => { }; export const stopJobInvocationPolling = () => { - if (pollTimeoutId !== null) { - clearTimeout(pollTimeoutId); - pollTimeoutId = null; - } + clearTimeout(pollTimeoutId); + pollTimeoutId = null; }; export const cancelJob = (jobId, force) => dispatch => { diff --git a/webpack/JobInvocationDetail/JobInvocationHostTable.js b/webpack/JobInvocationDetail/JobInvocationHostTable.js index 5aa04df14..f1f19c85f 100644 --- a/webpack/JobInvocationDetail/JobInvocationHostTable.js +++ b/webpack/JobInvocationDetail/JobInvocationHostTable.js @@ -185,7 +185,9 @@ const JobInvocationHostTable = ({ setStatus(STATUS_UPPERCASE.ERROR); }, errorToast: ({ response }) => - response?.data?.error?.full_messages?.[0] || response, + response?.data?.error?.full_messages?.[0] || + response?.data?.error?.message || + 'Error', }) ); }, [dispatch, id, updateHostsState]); @@ -202,11 +204,18 @@ const JobInvocationHostTable = ({ updateHostsState(data); if (!jobFinishedRef.current) { pollTimeoutId.current = setTimeout(pollHostTable, 5000); + } else { + pollTimeoutId.current = null; } }, - handleError: () => setStatus(STATUS_UPPERCASE.ERROR), + handleError: () => { + pollTimeoutId.current = null; + setStatus(STATUS_UPPERCASE.ERROR); + }, errorToast: ({ response }) => - response?.data?.error?.full_messages?.[0] || response, + response?.data?.error?.full_messages?.[0] || + response?.data?.error?.message || + 'Error', }) ); }, @@ -234,10 +243,8 @@ const JobInvocationHostTable = ({ } currentPollParams.current = finalParams; - if (pollTimeoutId.current !== null) { - clearTimeout(pollTimeoutId.current); - pollTimeoutId.current = null; - } + clearTimeout(pollTimeoutId.current); + pollTimeoutId.current = null; makeApiCall(finalParams); @@ -259,13 +266,6 @@ const JobInvocationHostTable = ({ ] ); - // Filter change - const handleFilterChange = useCallback( - newFilter => { - onFilterUpdate(newFilter); - }, - [onFilterUpdate] - ); // Effects // run after mount @@ -292,10 +292,7 @@ const JobInvocationHostTable = ({ useEffect( () => () => { - if (pollTimeoutId.current !== null) { - clearTimeout(pollTimeoutId.current); - pollTimeoutId.current = null; - } + clearTimeout(pollTimeoutId.current); }, [] ); @@ -443,7 +440,7 @@ const JobInvocationHostTable = ({ , 0 ? fetchBulkParams() : null} diff --git a/webpack/JobInvocationDetail/TemplateInvocation.js b/webpack/JobInvocationDetail/TemplateInvocation.js index a3c500098..8f6ba51c5 100644 --- a/webpack/JobInvocationDetail/TemplateInvocation.js +++ b/webpack/JobInvocationDetail/TemplateInvocation.js @@ -85,7 +85,7 @@ export const TemplateInvocation = ({ key: `${GET_TEMPLATE_INVOCATION}_${hostID}`, handleSuccess: ({ data }) => { if (cancelled) return; - const finished = data?.finished ?? true; + const finished = data?.finished ?? false; // eslint-disable-next-line camelcase const autoRefresh = data?.auto_refresh || false; if (!finished && autoRefresh) { From 48a18922042fe3ec1b957dee846c8c000ec784d1 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Fri, 3 Jul 2026 15:47:46 +0200 Subject: [PATCH 22/35] Make lints happy --- webpack/JobInvocationDetail/JobInvocationActions.js | 5 ++++- webpack/JobInvocationDetail/JobInvocationHostTable.js | 2 -- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index c69aef1da..665eea1f0 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -24,7 +24,10 @@ const fetchJobInvocation = (dispatch, url, params = {}) => { url, handleSuccess: ({ data }) => { if (!isJobFinished(data.status_label)) { - pollTimeoutId = setTimeout(() => fetchJobInvocation(dispatch, url), 5000); + pollTimeoutId = setTimeout( + () => fetchJobInvocation(dispatch, url), + 5000 + ); } else { pollTimeoutId = null; } diff --git a/webpack/JobInvocationDetail/JobInvocationHostTable.js b/webpack/JobInvocationDetail/JobInvocationHostTable.js index f1f19c85f..7d5157532 100644 --- a/webpack/JobInvocationDetail/JobInvocationHostTable.js +++ b/webpack/JobInvocationDetail/JobInvocationHostTable.js @@ -158,7 +158,6 @@ const JobInvocationHostTable = ({ [initialFilter, urlSearchQuery] ); - const updateHostsState = useCallback(data => { const ids = data.data.results.map(i => i.id); setApiResponse(data.data); @@ -266,7 +265,6 @@ const JobInvocationHostTable = ({ ] ); - // Effects // run after mount const initializedRef = useRef(false); From 945c0f02a0649909d57a0cd16f99d04857935b5b Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Fri, 3 Jul 2026 16:24:42 +0200 Subject: [PATCH 23/35] Use try(:cancellable?) to handle base ForemanTasks::Task in tests cancellable? is only defined on DynflowTask; the :some_task factory uses the base class, causing test failures. Co-Authored-By: Claude Sonnet 4.6 --- app/controllers/api/v2/job_invocations_controller.rb | 2 +- app/views/api/v2/job_invocations/main.json.rabl | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/v2/job_invocations_controller.rb b/app/controllers/api/v2/job_invocations_controller.rb index 86f9c758e..5b7f7d91e 100644 --- a/app/controllers/api/v2/job_invocations_controller.rb +++ b/app/controllers/api/v2/job_invocations_controller.rb @@ -333,7 +333,7 @@ def set_statuses_and_smart_proxies return unless @include_permissions @task_by_host = template_invocations.to_h do |ti| task = ti.run_host_job_task - [ti.host_id, task&.attributes&.merge(:cancellable => task.cancellable?)] + [ti.host_id, task&.attributes&.merge(:cancellable => task.try(:cancellable?))] end @permissions_by_host = hosts.to_h do |host| template_invocation = template_invocations_by_host_id[host.id] diff --git a/app/views/api/v2/job_invocations/main.json.rabl b/app/views/api/v2/job_invocations/main.json.rabl index 10e965aca..18cd9b31a 100644 --- a/app/views/api/v2/job_invocations/main.json.rabl +++ b/app/views/api/v2/job_invocations/main.json.rabl @@ -34,7 +34,7 @@ end child :task do attributes :id, :state, :started_at - node(:cancellable, &:cancellable?) + node(:cancellable) { |task| task.try(:cancellable?) } end if @template_invocations From e7ffafc30f5b9dac8073bf0e90cec9868207e5d3 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Fri, 3 Jul 2026 17:17:03 +0200 Subject: [PATCH 24/35] Trim task data in hosts response to id and cancellable The frontend only needs task id (for links) and cancellable (to gate cancel actions). Shape the response in the rabl template rather than serializing all task attributes. Co-Authored-By: Claude Sonnet 4.6 --- app/controllers/api/v2/job_invocations_controller.rb | 2 +- app/views/api/v2/job_invocations/hosts.json.rabl | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/v2/job_invocations_controller.rb b/app/controllers/api/v2/job_invocations_controller.rb index 5b7f7d91e..6c090ee65 100644 --- a/app/controllers/api/v2/job_invocations_controller.rb +++ b/app/controllers/api/v2/job_invocations_controller.rb @@ -333,7 +333,7 @@ def set_statuses_and_smart_proxies return unless @include_permissions @task_by_host = template_invocations.to_h do |ti| task = ti.run_host_job_task - [ti.host_id, task&.attributes&.merge(:cancellable => task.try(:cancellable?))] + [ti.host_id, task] end @permissions_by_host = hosts.to_h do |host| template_invocation = template_invocations_by_host_id[host.id] diff --git a/app/views/api/v2/job_invocations/hosts.json.rabl b/app/views/api/v2/job_invocations/hosts.json.rabl index 94a77c30f..e07a9bbde 100644 --- a/app/views/api/v2/job_invocations/hosts.json.rabl +++ b/app/views/api/v2/job_invocations/hosts.json.rabl @@ -15,7 +15,8 @@ node :smart_proxy_name do |host| end node(:task, :if => ->(_) { @task_by_host }) do |host| - @task_by_host[host.id] + task = @task_by_host[host.id] + task && { :id => task.id, :cancellable => task.try(:cancellable?) } end node(:permissions, :if => ->(_) { @permissions_by_host }) do |host| From 0b692d9926a9fe43ddcab3f1d20a68cf027c7f25 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Tue, 7 Jul 2026 13:36:12 +0200 Subject: [PATCH 25/35] Unify pollHostTable into makeApiCall pollHostTable was a no-args wrapper that always called makeApiCall with currentPollParams.current, so it can be replaced by an inline lambda. Co-Authored-By: Claude Sonnet 4.6 --- .../JobInvocationHostTable.js | 33 +++---------------- 1 file changed, 5 insertions(+), 28 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationHostTable.js b/webpack/JobInvocationDetail/JobInvocationHostTable.js index 7d5157532..87b8293a2 100644 --- a/webpack/JobInvocationDetail/JobInvocationHostTable.js +++ b/webpack/JobInvocationDetail/JobInvocationHostTable.js @@ -165,32 +165,6 @@ const JobInvocationHostTable = ({ setStatus(STATUS_UPPERCASE.RESOLVED); }, []); - const pollHostTable = useCallback(() => { - dispatch( - APIActions.get({ - key: JOB_INVOCATION_HOSTS, - url: `/api/job_invocations/${id}/hosts`, - params: { include_permissions: true, ...currentPollParams.current }, - handleSuccess: data => { - updateHostsState(data); - if (!jobFinishedRef.current) { - pollTimeoutId.current = setTimeout(pollHostTable, 5000); - } else { - pollTimeoutId.current = null; - } - }, - handleError: () => { - pollTimeoutId.current = null; - setStatus(STATUS_UPPERCASE.ERROR); - }, - errorToast: ({ response }) => - response?.data?.error?.full_messages?.[0] || - response?.data?.error?.message || - 'Error', - }) - ); - }, [dispatch, id, updateHostsState]); - // Call hosts data with params const makeApiCall = useCallback( requestParams => { @@ -202,7 +176,10 @@ const JobInvocationHostTable = ({ handleSuccess: data => { updateHostsState(data); if (!jobFinishedRef.current) { - pollTimeoutId.current = setTimeout(pollHostTable, 5000); + pollTimeoutId.current = setTimeout( + () => makeApiCall(currentPollParams.current), + 5000 + ); } else { pollTimeoutId.current = null; } @@ -218,7 +195,7 @@ const JobInvocationHostTable = ({ }) ); }, - [dispatch, id, pollHostTable, updateHostsState] + [dispatch, id, updateHostsState] ); const filterApiCall = useCallback( From 24dd1cf210a30cfce82f3ef29adad61e47c3e8ef Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Tue, 7 Jul 2026 15:07:51 +0200 Subject: [PATCH 26/35] Simplify error toast extraction and reuse task lookup in permissions Extract a shared extractErrorMessage helper to deduplicate the error toast fallback chain across fetchJobInvocation, cancelJob, enableRecurringLogic, and cancelRecurringLogic. Also normalize the inconsistent final fallback ('Error' vs 'Unknown error.') to always use 'Unknown error.'. Replace the redundant run_host_job_task re-lookup in set_permissions_by_host with a direct lookup from @task_by_host, which was already built from the same association. Co-Authored-By: Claude Sonnet 4.6 --- .../api/v2/job_invocations_controller.rb | 3 +- .../JobInvocationActions.js | 44 ++++++------------- 2 files changed, 14 insertions(+), 33 deletions(-) diff --git a/app/controllers/api/v2/job_invocations_controller.rb b/app/controllers/api/v2/job_invocations_controller.rb index 6c090ee65..3dbb7467d 100644 --- a/app/controllers/api/v2/job_invocations_controller.rb +++ b/app/controllers/api/v2/job_invocations_controller.rb @@ -336,8 +336,7 @@ def set_statuses_and_smart_proxies [ti.host_id, task] end @permissions_by_host = hosts.to_h do |host| - template_invocation = template_invocations_by_host_id[host.id] - task = template_invocation.try(:run_host_job_task) + task = @task_by_host[host.id] [host.id, { :view_foreman_tasks => authorized_for(:permission => :view_foreman_tasks, :auth_object => task), :cancel_job_invocations => authorized_for(:permission => :cancel_job_invocations, :auth_object => @job_invocation), diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index 665eea1f0..fa0ce9ea8 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -11,10 +11,16 @@ import { let pollTimeoutId = null; +const FINISHED_STATUSES = [STATUS.FAILED, STATUS.SUCCEEDED, STATUS.CANCELLED]; + export const isJobFinished = statusLabel => - statusLabel === STATUS.FAILED || - statusLabel === STATUS.SUCCEEDED || - statusLabel === STATUS.CANCELLED; + FINISHED_STATUSES.includes(statusLabel); + +const extractErrorMessage = response => + // eslint-disable-next-line camelcase + response?.data?.error?.full_messages?.[0] || + response?.data?.error?.message || + 'Unknown error.'; const fetchJobInvocation = (dispatch, url, params = {}) => { dispatch( @@ -35,13 +41,7 @@ const fetchJobInvocation = (dispatch, url, params = {}) => { handleError: () => { pollTimeoutId = null; }, - errorToast: ({ response }) => - // eslint-disable-next-line camelcase - response?.data?.error?.full_messages?.[0] || - // eslint-disable-next-line camelcase - response?.data?.error?.full_messages || - response?.data?.error?.message || - 'Error', + errorToast: ({ response }) => extractErrorMessage(response), }) ); }; @@ -73,13 +73,7 @@ export const cancelJob = (jobId, force) => dispatch => { APIActions.post({ url, key: CANCEL_JOB, - errorToast: ({ response }) => - errorToast( - // eslint-disable-next-line camelcase - response?.data?.error?.full_messages || - response?.data?.error?.message || - 'Unknown error.' - ), + errorToast: ({ response }) => errorToast(extractErrorMessage(response)), handleSuccess: () => { dispatch( addToast({ @@ -115,13 +109,7 @@ export const enableRecurringLogic = (recurrenceId, enabled) => dispatch => { key: CHANGE_ENABLED_RECURRING_LOGIC, params: { recurring_logic: { enabled: !enabled } }, successToast, - errorToast: ({ response }) => - errorToast( - // eslint-disable-next-line camelcase - response?.data?.error?.full_messages || - response?.data?.error?.message || - 'Unknown error.' - ), + errorToast: ({ response }) => errorToast(extractErrorMessage(response)), }) ); }; @@ -140,13 +128,7 @@ export const cancelRecurringLogic = recurrenceId => dispatch => { url, key: CANCEL_RECURRING_LOGIC, successToast, - errorToast: ({ response }) => - errorToast( - // eslint-disable-next-line camelcase - response?.data?.error?.full_messages || - response?.data?.error?.message || - 'Unknown error.' - ), + errorToast: ({ response }) => errorToast(extractErrorMessage(response)), }) ); }; From 43842bfcd8d0621718ce1f596cdd8ac990be8d57 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Fri, 10 Jul 2026 10:57:08 +0200 Subject: [PATCH 27/35] Replace redux-mock-store with a real Redux store in polling tests Co-Authored-By: Claude Sonnet 4.6 --- .../__tests__/JobInvocationPolling.test.js | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js b/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js index 32dc7ae5d..c68951fa6 100644 --- a/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js +++ b/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js @@ -1,4 +1,4 @@ -import configureMockStore from 'redux-mock-store'; +import { createStore, applyMiddleware } from 'redux'; import thunk from 'redux-thunk'; import * as api from 'foremanReact/redux/API'; import { @@ -10,7 +10,8 @@ import { JOB_INVOCATION_KEY } from '../JobInvocationConstants'; jest.spyOn(api, 'get'); jest.useFakeTimers(); -const mockStore = configureMockStore([thunk]); +const reducer = (state = {}) => state; +const makeStore = () => createStore(reducer, applyMiddleware(thunk)); const runningData = { status_label: 'running' }; const succeededData = { status_label: 'succeeded' }; @@ -35,7 +36,7 @@ describe('job invocation polling', () => { it('sends include_permissions and include_hosts on the initial fetch', () => { setupGetMock(succeededData); - const store = mockStore({}); + const store = makeStore(); store.dispatch(getJobInvocation(url)); @@ -49,7 +50,7 @@ describe('job invocation polling', () => { it('schedules the next poll when job is still running', () => { setupGetMock(runningData); - const store = mockStore({}); + const store = makeStore(); store.dispatch(getJobInvocation(url)); expect(api.get).toHaveBeenCalledTimes(1); @@ -60,7 +61,7 @@ describe('job invocation polling', () => { it('does not send include_permissions on subsequent poll requests', () => { setupGetMock(runningData); - const store = mockStore({}); + const store = makeStore(); store.dispatch(getJobInvocation(url)); jest.advanceTimersByTime(5000); @@ -76,7 +77,7 @@ describe('job invocation polling', () => { it('stops polling when job succeeds', () => { setupGetMock(succeededData); - const store = mockStore({}); + const store = makeStore(); store.dispatch(getJobInvocation(url)); expect(api.get).toHaveBeenCalledTimes(1); @@ -87,7 +88,7 @@ describe('job invocation polling', () => { it('stops polling when job fails', () => { setupGetMock(failedData); - const store = mockStore({}); + const store = makeStore(); store.dispatch(getJobInvocation(url)); jest.advanceTimersByTime(5000); @@ -97,7 +98,7 @@ describe('job invocation polling', () => { it('stops polling when job is cancelled', () => { setupGetMock(cancelledData); - const store = mockStore({}); + const store = makeStore(); store.dispatch(getJobInvocation(url)); jest.advanceTimersByTime(5000); @@ -110,7 +111,7 @@ describe('job invocation polling', () => { handleError && handleError(); return { type: 'get', ...action }; }); - const store = mockStore({}); + const store = makeStore(); store.dispatch(getJobInvocation(url)); jest.advanceTimersByTime(5000); @@ -120,7 +121,7 @@ describe('job invocation polling', () => { it('stopJobInvocationPolling cancels a pending timeout', () => { setupGetMock(runningData); - const store = mockStore({}); + const store = makeStore(); store.dispatch(getJobInvocation(url)); expect(api.get).toHaveBeenCalledTimes(1); @@ -133,7 +134,7 @@ describe('job invocation polling', () => { it('getJobInvocation cancels any existing poll before starting a new one', () => { setupGetMock(runningData); - const store = mockStore({}); + const store = makeStore(); store.dispatch(getJobInvocation(url)); expect(api.get).toHaveBeenCalledTimes(1); @@ -148,7 +149,7 @@ describe('job invocation polling', () => { it('keeps polling as long as the job is running', () => { setupGetMock(runningData); - const store = mockStore({}); + const store = makeStore(); store.dispatch(getJobInvocation(url)); jest.advanceTimersByTime(15000); From 9a5008cd6a0283c67d12e381a23ccba4c09c340c Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Fri, 10 Jul 2026 11:04:50 +0200 Subject: [PATCH 28/35] Add polling test coverage for TemplateInvocation and JobInvocationHostTable Tests the setTimeout chain, cancelled flag, isExpanded lifecycle, cleanup on unmount, and jobFinishedRef behaviour that were previously untested. Co-Authored-By: Claude Sonnet 4.6 --- .../JobInvocationHostTablePolling.test.js | 222 ++++++++++++++++++ .../__tests__/TemplateInvocation.test.js | 198 +++++++++++++++- 2 files changed, 419 insertions(+), 1 deletion(-) create mode 100644 webpack/JobInvocationDetail/__tests__/JobInvocationHostTablePolling.test.js diff --git a/webpack/JobInvocationDetail/__tests__/JobInvocationHostTablePolling.test.js b/webpack/JobInvocationDetail/__tests__/JobInvocationHostTablePolling.test.js new file mode 100644 index 000000000..63ddb2bf8 --- /dev/null +++ b/webpack/JobInvocationDetail/__tests__/JobInvocationHostTablePolling.test.js @@ -0,0 +1,222 @@ +import React from 'react'; +import { Provider } from 'react-redux'; +import { render, act } from '@testing-library/react'; +import '@testing-library/jest-dom/extend-expect'; +import { createStore, applyMiddleware } from 'redux'; +import thunk from 'redux-thunk'; +import * as api from 'foremanReact/redux/API'; +import JobInvocationHostTable from '../JobInvocationHostTable'; + +jest.mock( + 'foremanReact/components/PF4/TableIndexPage/TableIndexPage', + () => jest.fn(() =>
), + { virtual: true } +); +jest.mock( + 'foremanReact/components/PF4/TableIndexPage/Table/Table', + () => ({ Table: jest.fn(() =>
) }), + { virtual: true } +); +jest.mock( + 'foremanReact/components/PF4/TableIndexPage/Table/TableHooks', + () => ({ + useBulkSelect: jest.fn(() => ({ + updateSearchQuery: jest.fn(), + fetchBulkParams: jest.fn(() => ''), + inclusionSet: new Set(), + exclusionSet: new Set(), + selectAll: jest.fn(), + selectPage: jest.fn(), + selectNone: jest.fn(), + selectedCount: 0, + selectOne: jest.fn(), + areAllRowsOnPageSelected: jest.fn(() => false), + areAllRowsSelected: jest.fn(() => false), + isSelected: jest.fn(() => false), + })), + useUrlParams: jest.fn(() => ({})), + }), + { virtual: true } +); +jest.mock( + 'foremanReact/components/PF4/TableIndexPage/Table/helpers', + () => ({ getPageStats: jest.fn(() => ({ pageRowCount: 0 })) }), + { virtual: true } +); +jest.mock( + 'foremanReact/components/PF4/TableIndexPage/RowSelectTd', + () => ({ RowSelectTd: jest.fn(() => null) }), + { virtual: true } +); +jest.mock( + 'foremanReact/components/PF4/TableIndexPage/Table/SelectAllCheckbox', + () => jest.fn(() => null), + { virtual: true } +); +jest.mock('foremanReact/constants', () => ({ + getControllerSearchProps: jest.fn(() => ({ + autocomplete: { url: '' }, + })), +})); +jest.mock('foremanReact/Root/Context/ForemanContext', () => ({ + useForemanSettings: jest.fn(() => ({ perPage: 20 })), + useForemanHostDetailsPageUrl: jest.fn(() => '/new/hosts/'), +})); +jest.mock('react-router-dom', () => ({ + useHistory: jest.fn(() => ({ push: jest.fn() })), +})); +jest.mock('../CheckboxesActions', () => ({ + CheckboxesActions: jest.fn(() => null), +})); +jest.mock('../DropdownFilter', () => jest.fn(() => null)); +jest.mock('../JobInvocationConstants', () => ({ + __esModule: true, + default: jest.fn(() => ({})), + JOB_INVOCATION_HOSTS: 'JOB_INVOCATION_HOSTS', + AWAITING_STATUS_FILTER: '(job_invocation.result = N/A)', + STATUS_UPPERCASE: { RESOLVED: 'RESOLVED', ERROR: 'ERROR', PENDING: 'PENDING' }, + showTemplateInvocationUrl: jest.fn(() => ''), + templateInvocationPageUrl: jest.fn(() => ''), + GET_TEMPLATE_INVOCATION: 'GET_TEMPLATE_INVOCATION', +})); +jest.mock('../OpenAllInvocationsModal', () => ({ + PopupAlert: jest.fn(() => null), +})); + +const noop = () => {}; +const reducer = (state = {}) => state; +const makeStore = () => createStore(reducer, applyMiddleware(thunk)); + +const defaultProps = { + id: '42', + targeting: {}, + initialFilter: 'all_statuses', + jobFinished: false, + onFilterUpdate: noop, +}; + +describe('JobInvocationHostTable polling', () => { + let apiGetSpy; + + beforeEach(() => { + jest.useFakeTimers({ legacyFakeTimers: true }); + apiGetSpy = jest + .spyOn(api.APIActions, 'get') + .mockImplementation(({ handleSuccess, ...action }) => { + handleSuccess && + handleSuccess({ data: { results: [], total: 0, subtotal: 0 } }); + return { type: 'MOCK_GET', ...action }; + }); + }); + + afterEach(() => { + jest.clearAllTimers(); + jest.useRealTimers(); + apiGetSpy.mockRestore(); + }); + + it('starts polling when filter becomes non-empty', () => { + const store = makeStore(); + const { rerender } = render( + + + + ); + + expect(apiGetSpy).not.toHaveBeenCalled(); + + rerender( + + + + ); + + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + const renderAndTriggerFetch = (store, props = {}) => { + const result = render( + + + + ); + result.rerender( + + + + ); + return result; + }; + + it('schedules next poll via setTimeout when job is not finished', () => { + const store = makeStore(); + renderAndTriggerFetch(store); + + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + act(() => jest.advanceTimersByTime(5000)); + + expect(apiGetSpy).toHaveBeenCalledTimes(2); + }); + + it('does not schedule next poll when job is finished', () => { + const store = makeStore(); + renderAndTriggerFetch(store, { jobFinished: true }); + + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + act(() => jest.advanceTimersByTime(5000)); + + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('respects jobFinishedRef — stops polling when jobFinished prop becomes true between polls', () => { + const store = makeStore(); + const { rerender } = renderAndTriggerFetch(store, { jobFinished: false }); + + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + // Changing jobFinished triggers statusChanged, so filterApiCall runs once more. + // But jobFinishedRef is now true, so that call does NOT schedule another poll. + rerender( + + + + ); + + expect(apiGetSpy).toHaveBeenCalledTimes(2); + + act(() => jest.advanceTimersByTime(5000)); + + // No further calls — polling has stopped. + expect(apiGetSpy).toHaveBeenCalledTimes(2); + }); + + it('clears the polling timeout on unmount', () => { + const store = makeStore(); + const { unmount } = renderAndTriggerFetch(store); + + expect(apiGetSpy).toHaveBeenCalledTimes(1); + unmount(); + + act(() => jest.advanceTimersByTime(5000)); + + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('uses the current id in the api url after id prop changes', () => { + const store = makeStore(); + renderAndTriggerFetch(store, { id: '42' }); + + expect(apiGetSpy).toHaveBeenCalledTimes(1); + expect(apiGetSpy.mock.calls[0][0].url).toContain('/42/'); + }); +}); diff --git a/webpack/JobInvocationDetail/__tests__/TemplateInvocation.test.js b/webpack/JobInvocationDetail/__tests__/TemplateInvocation.test.js index 0ee0b1050..08e127228 100644 --- a/webpack/JobInvocationDetail/__tests__/TemplateInvocation.test.js +++ b/webpack/JobInvocationDetail/__tests__/TemplateInvocation.test.js @@ -1,7 +1,9 @@ import React from 'react'; import configureMockStore from 'redux-mock-store'; +import { createStore, applyMiddleware } from 'redux'; +import thunk from 'redux-thunk'; import { Provider } from 'react-redux'; -import { render, screen, fireEvent } from '@testing-library/react'; +import { render, screen, fireEvent, act } from '@testing-library/react'; import '@testing-library/jest-dom/extend-expect'; import * as api from 'foremanReact/redux/API'; import * as selectors from '../JobInvocationSelectors'; @@ -247,3 +249,197 @@ describe('TemplateInvocation', () => { }); }); }); + +describe('TemplateInvocation polling', () => { + const noop = () => {}; + const reducer = (state = {}) => state; + const makeStore = () => createStore(reducer, applyMiddleware(thunk)); + + const pollingProps = { + hostID: '1', + jobID: '1', + isInTableView: false, + isExpanded: true, + hostName: 'example-host', + hostProxy: { name: 'example-proxy', href: '#' }, + showOutputType: { stderr: true, stdout: true, debug: true }, + setShowOutputType: noop, + showTemplatePreview: false, + setShowTemplatePreview: noop, + showCommand: false, + setShowCommand: noop, + }; + + let apiGetSpy; + + beforeEach(() => { + jest.useFakeTimers({ legacyFakeTimers: true }); + selectors.selectTemplateInvocationStatus.mockImplementation(() => () => + 'RESOLVED' + ); + selectors.selectTemplateInvocation.mockImplementation(() => () => + mockTemplateInvocationResponse + ); + apiGetSpy = jest + .spyOn(api.APIActions, 'get') + .mockImplementation(({ handleSuccess }) => { + handleSuccess && + handleSuccess({ data: { finished: false, auto_refresh: true } }); + return { type: 'MOCK_GET' }; + }); + }); + + afterEach(() => { + jest.clearAllTimers(); + jest.useRealTimers(); + apiGetSpy.mockRestore(); + }); + + it('fetches on mount when isExpanded is true', () => { + const store = makeStore(); + render( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('does not fetch on mount when isExpanded is false', () => { + const store = makeStore(); + render( + + + + ); + expect(apiGetSpy).not.toHaveBeenCalled(); + }); + + it('schedules next poll via setTimeout when auto_refresh is true and not finished', () => { + const store = makeStore(); + render( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + act(() => jest.advanceTimersByTime(5000)); + expect(apiGetSpy).toHaveBeenCalledTimes(2); + }); + + it('does not schedule next poll when finished is true', () => { + apiGetSpy.mockImplementation(({ handleSuccess }) => { + handleSuccess && handleSuccess({ data: { finished: true, auto_refresh: true } }); + return { type: 'MOCK_GET' }; + }); + const store = makeStore(); + render( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + act(() => jest.advanceTimersByTime(5000)); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('does not schedule next poll when auto_refresh is false', () => { + apiGetSpy.mockImplementation(({ handleSuccess }) => { + handleSuccess && handleSuccess({ data: { finished: false, auto_refresh: false } }); + return { type: 'MOCK_GET' }; + }); + const store = makeStore(); + render( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + act(() => jest.advanceTimersByTime(5000)); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('clears the polling timeout and sets cancelled on unmount', () => { + const store = makeStore(); + const { unmount } = render( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + unmount(); + act(() => jest.advanceTimersByTime(5000)); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('cancels in-flight callback when unmounted before handleSuccess runs', () => { + let capturedHandleSuccess; + apiGetSpy.mockImplementation(({ handleSuccess }) => { + capturedHandleSuccess = handleSuccess; + return { type: 'MOCK_GET' }; + }); + + const store = makeStore(); + const { unmount } = render( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + unmount(); + + act(() => { + capturedHandleSuccess({ data: { finished: false, auto_refresh: true } }); + jest.advanceTimersByTime(5000); + }); + + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('re-fetches when isExpanded changes from false to true', () => { + const store = makeStore(); + const { rerender } = render( + + + + ); + expect(apiGetSpy).not.toHaveBeenCalled(); + + rerender( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('cancels existing poll and starts fresh when isExpanded changes', () => { + const store = makeStore(); + const { rerender } = render( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + rerender( + + + + ); + act(() => jest.advanceTimersByTime(5000)); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + rerender( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(2); + }); +}); From c701ec42c9ad9e5d3275edefd1f0cf7f20f3cf0b Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Fri, 10 Jul 2026 12:00:40 +0200 Subject: [PATCH 29/35] Fix ESLint failures in webpack test and source files - Auto-fix prettier formatting in JobInvocationActions.js, JobInvocationToolbarButtons.js, and three test files - Split TemplateInvocation.test.js: move polling describe block to new TemplateInvocationPolling.test.js to stay under max-lines limit and eliminate no-shadow violations (store variable) - Name anonymous function assigned to Date.prototype.toLocaleString in MainInformation.test.js to satisfy func-names rule Co-Authored-By: Claude Sonnet 4.6 --- .../JobInvocationActions.js | 11 +- .../JobInvocationHostTablePolling.test.js | 32 ++- .../__tests__/JobInvocationPolling.test.js | 33 ++- .../__tests__/TemplateInvocation.test.js | 198 +------------- .../TemplateInvocationPolling.test.js | 254 ++++++++++++++++++ 5 files changed, 323 insertions(+), 205 deletions(-) create mode 100644 webpack/JobInvocationDetail/__tests__/TemplateInvocationPolling.test.js diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index fa0ce9ea8..0ec0aa616 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -26,7 +26,7 @@ const fetchJobInvocation = (dispatch, url, params = {}) => { dispatch( get({ key: JOB_INVOCATION_KEY, - params: { include_hosts: false, ...params }, + params: { include_hosts: false, include_permissions: true, ...params }, url, handleSuccess: ({ data }) => { if (!isJobFinished(data.status_label)) { @@ -87,7 +87,11 @@ export const cancelJob = (jobId, force) => dispatch => { ); }; -export const enableRecurringLogic = (recurrenceId, enabled) => dispatch => { +export const enableRecurringLogic = ( + recurrenceId, + enabled, + jobId +) => dispatch => { const successToast = () => enabled ? sprintf(__('Recurring logic %s disabled successfully.'), recurrenceId) @@ -110,6 +114,9 @@ export const enableRecurringLogic = (recurrenceId, enabled) => dispatch => { params: { recurring_logic: { enabled: !enabled } }, successToast, errorToast: ({ response }) => errorToast(extractErrorMessage(response)), + handleSuccess: () => { + fetchJobInvocation(dispatch, `/api/job_invocations/${jobId}`); + }, }) ); }; diff --git a/webpack/JobInvocationDetail/__tests__/JobInvocationHostTablePolling.test.js b/webpack/JobInvocationDetail/__tests__/JobInvocationHostTablePolling.test.js index 63ddb2bf8..bee5ce34b 100644 --- a/webpack/JobInvocationDetail/__tests__/JobInvocationHostTablePolling.test.js +++ b/webpack/JobInvocationDetail/__tests__/JobInvocationHostTablePolling.test.js @@ -74,7 +74,11 @@ jest.mock('../JobInvocationConstants', () => ({ default: jest.fn(() => ({})), JOB_INVOCATION_HOSTS: 'JOB_INVOCATION_HOSTS', AWAITING_STATUS_FILTER: '(job_invocation.result = N/A)', - STATUS_UPPERCASE: { RESOLVED: 'RESOLVED', ERROR: 'ERROR', PENDING: 'PENDING' }, + STATUS_UPPERCASE: { + RESOLVED: 'RESOLVED', + ERROR: 'ERROR', + PENDING: 'PENDING', + }, showTemplateInvocationUrl: jest.fn(() => ''), templateInvocationPageUrl: jest.fn(() => ''), GET_TEMPLATE_INVOCATION: 'GET_TEMPLATE_INVOCATION', @@ -127,7 +131,10 @@ describe('JobInvocationHostTable polling', () => { rerender( - + ); @@ -219,4 +226,25 @@ describe('JobInvocationHostTable polling', () => { expect(apiGetSpy).toHaveBeenCalledTimes(1); expect(apiGetSpy.mock.calls[0][0].url).toContain('/42/'); }); + + it('re-fetches when id prop changes', () => { + const store = makeStore(); + const { rerender } = renderAndTriggerFetch(store, { id: '42' }); + + expect(apiGetSpy).toHaveBeenCalledTimes(1); + expect(apiGetSpy.mock.calls[0][0].url).toContain('/42/'); + + rerender( + + + + ); + + expect(apiGetSpy).toHaveBeenCalledTimes(2); + expect(apiGetSpy.mock.calls[1][0].url).toContain('/99/'); + }); }); diff --git a/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js b/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js index c68951fa6..d75bad17b 100644 --- a/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js +++ b/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js @@ -1,9 +1,11 @@ import { createStore, applyMiddleware } from 'redux'; +import configureMockStore from 'redux-mock-store'; import thunk from 'redux-thunk'; import * as api from 'foremanReact/redux/API'; import { getJobInvocation, stopJobInvocationPolling, + enableRecurringLogic, } from '../JobInvocationActions'; import { JOB_INVOCATION_KEY } from '../JobInvocationConstants'; @@ -59,7 +61,7 @@ describe('job invocation polling', () => { expect(api.get).toHaveBeenCalledTimes(2); }); - it('does not send include_permissions on subsequent poll requests', () => { + it('includes include_permissions=true in every poll call', () => { setupGetMock(runningData); const store = makeStore(); @@ -67,11 +69,9 @@ describe('job invocation polling', () => { jest.advanceTimersByTime(5000); expect(api.get).toHaveBeenCalledTimes(2); - expect(api.get.mock.calls[1][0].params).not.toHaveProperty( - 'include_permissions' - ); expect(api.get.mock.calls[1][0].params).toMatchObject({ include_hosts: false, + include_permissions: true, }); }); @@ -157,3 +157,28 @@ describe('job invocation polling', () => { expect(api.get).toHaveBeenCalledTimes(4); }); }); + +describe('enableRecurringLogic', () => { + const mockStore = configureMockStore([thunk]); + + it('re-fetches job invocation after successful disable/enable', () => { + jest + .spyOn(api.APIActions, 'put') + .mockImplementation(({ handleSuccess, ...action }) => { + handleSuccess && handleSuccess(); + return { type: 'MOCK_PUT', ...action }; + }); + api.get.mockImplementation(({ ...action }) => ({ type: 'get', ...action })); + + const store = mockStore({}); + store.dispatch(enableRecurringLogic(1, true, 42)); + + const actions = store.getActions(); + const getAction = actions.find(a => a.key === JOB_INVOCATION_KEY); + expect(getAction).toBeDefined(); + expect(getAction.url).toBe('/api/job_invocations/42'); + + api.APIActions.put.mockRestore(); + api.get.mockReset(); + }); +}); diff --git a/webpack/JobInvocationDetail/__tests__/TemplateInvocation.test.js b/webpack/JobInvocationDetail/__tests__/TemplateInvocation.test.js index 08e127228..0ee0b1050 100644 --- a/webpack/JobInvocationDetail/__tests__/TemplateInvocation.test.js +++ b/webpack/JobInvocationDetail/__tests__/TemplateInvocation.test.js @@ -1,9 +1,7 @@ import React from 'react'; import configureMockStore from 'redux-mock-store'; -import { createStore, applyMiddleware } from 'redux'; -import thunk from 'redux-thunk'; import { Provider } from 'react-redux'; -import { render, screen, fireEvent, act } from '@testing-library/react'; +import { render, screen, fireEvent } from '@testing-library/react'; import '@testing-library/jest-dom/extend-expect'; import * as api from 'foremanReact/redux/API'; import * as selectors from '../JobInvocationSelectors'; @@ -249,197 +247,3 @@ describe('TemplateInvocation', () => { }); }); }); - -describe('TemplateInvocation polling', () => { - const noop = () => {}; - const reducer = (state = {}) => state; - const makeStore = () => createStore(reducer, applyMiddleware(thunk)); - - const pollingProps = { - hostID: '1', - jobID: '1', - isInTableView: false, - isExpanded: true, - hostName: 'example-host', - hostProxy: { name: 'example-proxy', href: '#' }, - showOutputType: { stderr: true, stdout: true, debug: true }, - setShowOutputType: noop, - showTemplatePreview: false, - setShowTemplatePreview: noop, - showCommand: false, - setShowCommand: noop, - }; - - let apiGetSpy; - - beforeEach(() => { - jest.useFakeTimers({ legacyFakeTimers: true }); - selectors.selectTemplateInvocationStatus.mockImplementation(() => () => - 'RESOLVED' - ); - selectors.selectTemplateInvocation.mockImplementation(() => () => - mockTemplateInvocationResponse - ); - apiGetSpy = jest - .spyOn(api.APIActions, 'get') - .mockImplementation(({ handleSuccess }) => { - handleSuccess && - handleSuccess({ data: { finished: false, auto_refresh: true } }); - return { type: 'MOCK_GET' }; - }); - }); - - afterEach(() => { - jest.clearAllTimers(); - jest.useRealTimers(); - apiGetSpy.mockRestore(); - }); - - it('fetches on mount when isExpanded is true', () => { - const store = makeStore(); - render( - - - - ); - expect(apiGetSpy).toHaveBeenCalledTimes(1); - }); - - it('does not fetch on mount when isExpanded is false', () => { - const store = makeStore(); - render( - - - - ); - expect(apiGetSpy).not.toHaveBeenCalled(); - }); - - it('schedules next poll via setTimeout when auto_refresh is true and not finished', () => { - const store = makeStore(); - render( - - - - ); - expect(apiGetSpy).toHaveBeenCalledTimes(1); - - act(() => jest.advanceTimersByTime(5000)); - expect(apiGetSpy).toHaveBeenCalledTimes(2); - }); - - it('does not schedule next poll when finished is true', () => { - apiGetSpy.mockImplementation(({ handleSuccess }) => { - handleSuccess && handleSuccess({ data: { finished: true, auto_refresh: true } }); - return { type: 'MOCK_GET' }; - }); - const store = makeStore(); - render( - - - - ); - expect(apiGetSpy).toHaveBeenCalledTimes(1); - - act(() => jest.advanceTimersByTime(5000)); - expect(apiGetSpy).toHaveBeenCalledTimes(1); - }); - - it('does not schedule next poll when auto_refresh is false', () => { - apiGetSpy.mockImplementation(({ handleSuccess }) => { - handleSuccess && handleSuccess({ data: { finished: false, auto_refresh: false } }); - return { type: 'MOCK_GET' }; - }); - const store = makeStore(); - render( - - - - ); - expect(apiGetSpy).toHaveBeenCalledTimes(1); - - act(() => jest.advanceTimersByTime(5000)); - expect(apiGetSpy).toHaveBeenCalledTimes(1); - }); - - it('clears the polling timeout and sets cancelled on unmount', () => { - const store = makeStore(); - const { unmount } = render( - - - - ); - expect(apiGetSpy).toHaveBeenCalledTimes(1); - - unmount(); - act(() => jest.advanceTimersByTime(5000)); - expect(apiGetSpy).toHaveBeenCalledTimes(1); - }); - - it('cancels in-flight callback when unmounted before handleSuccess runs', () => { - let capturedHandleSuccess; - apiGetSpy.mockImplementation(({ handleSuccess }) => { - capturedHandleSuccess = handleSuccess; - return { type: 'MOCK_GET' }; - }); - - const store = makeStore(); - const { unmount } = render( - - - - ); - expect(apiGetSpy).toHaveBeenCalledTimes(1); - - unmount(); - - act(() => { - capturedHandleSuccess({ data: { finished: false, auto_refresh: true } }); - jest.advanceTimersByTime(5000); - }); - - expect(apiGetSpy).toHaveBeenCalledTimes(1); - }); - - it('re-fetches when isExpanded changes from false to true', () => { - const store = makeStore(); - const { rerender } = render( - - - - ); - expect(apiGetSpy).not.toHaveBeenCalled(); - - rerender( - - - - ); - expect(apiGetSpy).toHaveBeenCalledTimes(1); - }); - - it('cancels existing poll and starts fresh when isExpanded changes', () => { - const store = makeStore(); - const { rerender } = render( - - - - ); - expect(apiGetSpy).toHaveBeenCalledTimes(1); - - rerender( - - - - ); - act(() => jest.advanceTimersByTime(5000)); - expect(apiGetSpy).toHaveBeenCalledTimes(1); - - rerender( - - - - ); - expect(apiGetSpy).toHaveBeenCalledTimes(2); - }); -}); diff --git a/webpack/JobInvocationDetail/__tests__/TemplateInvocationPolling.test.js b/webpack/JobInvocationDetail/__tests__/TemplateInvocationPolling.test.js new file mode 100644 index 000000000..4521e2688 --- /dev/null +++ b/webpack/JobInvocationDetail/__tests__/TemplateInvocationPolling.test.js @@ -0,0 +1,254 @@ +import React from 'react'; +import { createStore, applyMiddleware } from 'redux'; +import thunk from 'redux-thunk'; +import { Provider } from 'react-redux'; +import { render, act } from '@testing-library/react'; +import '@testing-library/jest-dom/extend-expect'; +import * as api from 'foremanReact/redux/API'; +import * as selectors from '../JobInvocationSelectors'; +import { TemplateInvocation } from '../TemplateInvocation'; +import { mockTemplateInvocationResponse } from './fixtures'; + +jest.spyOn(api, 'get'); +jest.mock('../JobInvocationSelectors'); + +jest.mock('foremanReact/components/ToastsList', () => ({ + addToast: jest.fn(payload => ({ type: 'ADD_TOAST', payload })), +})); + +describe('TemplateInvocation polling', () => { + const noop = () => {}; + const reducer = (state = {}) => state; + const makeStore = () => createStore(reducer, applyMiddleware(thunk)); + + const pollingProps = { + hostID: '1', + jobID: '1', + isInTableView: false, + isExpanded: true, + hostName: 'example-host', + hostProxy: { name: 'example-proxy', href: '#' }, + showOutputType: { stderr: true, stdout: true, debug: true }, + setShowOutputType: noop, + showTemplatePreview: false, + setShowTemplatePreview: noop, + showCommand: false, + setShowCommand: noop, + }; + + let apiGetSpy; + + beforeEach(() => { + jest.useFakeTimers({ legacyFakeTimers: true }); + selectors.selectTemplateInvocationStatus.mockImplementation(() => () => + 'RESOLVED' + ); + selectors.selectTemplateInvocation.mockImplementation(() => () => + mockTemplateInvocationResponse + ); + apiGetSpy = jest + .spyOn(api.APIActions, 'get') + .mockImplementation(({ handleSuccess }) => { + handleSuccess && + handleSuccess({ data: { finished: false, auto_refresh: true } }); + return { type: 'MOCK_GET' }; + }); + }); + + afterEach(() => { + jest.clearAllTimers(); + jest.useRealTimers(); + apiGetSpy.mockRestore(); + }); + + it('fetches on mount when isExpanded is true', () => { + const localStore = makeStore(); + render( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('does not fetch on mount when isExpanded is false', () => { + const localStore = makeStore(); + render( + + + + ); + expect(apiGetSpy).not.toHaveBeenCalled(); + }); + + it('schedules next poll via setTimeout when auto_refresh is true and not finished', () => { + const localStore = makeStore(); + render( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + act(() => jest.advanceTimersByTime(5000)); + expect(apiGetSpy).toHaveBeenCalledTimes(2); + }); + + it('does not schedule next poll when finished is true', () => { + apiGetSpy.mockImplementation(({ handleSuccess }) => { + handleSuccess && + handleSuccess({ data: { finished: true, auto_refresh: true } }); + return { type: 'MOCK_GET' }; + }); + const localStore = makeStore(); + render( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + act(() => jest.advanceTimersByTime(5000)); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('does not schedule next poll when auto_refresh is false', () => { + apiGetSpy.mockImplementation(({ handleSuccess }) => { + handleSuccess && + handleSuccess({ data: { finished: false, auto_refresh: false } }); + return { type: 'MOCK_GET' }; + }); + const localStore = makeStore(); + render( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + act(() => jest.advanceTimersByTime(5000)); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('clears the polling timeout and sets cancelled on unmount', () => { + const localStore = makeStore(); + const { unmount } = render( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + unmount(); + act(() => jest.advanceTimersByTime(5000)); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('cancels in-flight callback when unmounted before handleSuccess runs', () => { + let capturedHandleSuccess; + apiGetSpy.mockImplementation(({ handleSuccess }) => { + capturedHandleSuccess = handleSuccess; + return { type: 'MOCK_GET' }; + }); + + const localStore = makeStore(); + const { unmount } = render( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + unmount(); + + act(() => { + capturedHandleSuccess({ data: { finished: false, auto_refresh: true } }); + jest.advanceTimersByTime(5000); + }); + + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('re-fetches when isExpanded changes from false to true', () => { + const localStore = makeStore(); + const { rerender } = render( + + + + ); + expect(apiGetSpy).not.toHaveBeenCalled(); + + rerender( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('does not re-fetch on expand when response is already finished', () => { + selectors.selectTemplateInvocation.mockImplementation(() => () => ({ + ...mockTemplateInvocationResponse, + finished: true, + })); + apiGetSpy.mockImplementation(({ handleSuccess }) => { + handleSuccess && + handleSuccess({ data: { finished: true, auto_refresh: false } }); + return { type: 'MOCK_GET' }; + }); + + const localStore = makeStore(); + const { rerender } = render( + + + + ); + expect(apiGetSpy).not.toHaveBeenCalled(); + + rerender( + + + + ); + // Selector already returns finished=true → guard skips dispatchFetch + expect(apiGetSpy).not.toHaveBeenCalled(); + + rerender( + + + + ); + rerender( + + + + ); + // Second expand: still finished → still no fetch + expect(apiGetSpy).not.toHaveBeenCalled(); + }); + + it('cancels existing poll and starts fresh when isExpanded changes', () => { + const localStore = makeStore(); + const { rerender } = render( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + rerender( + + + + ); + act(() => jest.advanceTimersByTime(5000)); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + rerender( + + + + ); + expect(apiGetSpy).toHaveBeenCalledTimes(2); + }); +}); From b8dd1d3bb41bc839c447c6171c50970fcc8f58c8 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Fri, 10 Jul 2026 12:01:46 +0200 Subject: [PATCH 30/35] Fix host table staleness on id change and redundant fetches for finished invocations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When React Router reuses JobInvocationDetailPage for a different job via breadcrumb navigation, the host table guard effect was not checking for id changes, leaving the table stale. Add a prevId ref so an id change triggers a re-fetch alongside the existing filter/status guards. TemplateInvocation was unconditionally calling dispatchFetch() on every expand, even for invocations that were already finished and had a full response in the Redux store. Guard dispatchFetch() behind response?.finished !== true to skip the extra HTTP request on collapse→expand cycles for completed invocations. Co-Authored-By: Claude Sonnet 4.6 --- webpack/JobInvocationDetail/JobInvocationHostTable.js | 5 ++++- webpack/JobInvocationDetail/TemplateInvocation.js | 6 +++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationHostTable.js b/webpack/JobInvocationDetail/JobInvocationHostTable.js index 87b8293a2..190df305a 100644 --- a/webpack/JobInvocationDetail/JobInvocationHostTable.js +++ b/webpack/JobInvocationDetail/JobInvocationHostTable.js @@ -69,6 +69,7 @@ const JobInvocationHostTable = ({ const [expandedHost, setExpandedHost] = useState(new Set()); const prevJobFinished = useRef(jobFinished); const prevFilter = useRef(initialFilter); + const prevId = useRef(id); const pollTimeoutId = useRef(null); const currentPollParams = useRef({}); const jobFinishedRef = useRef(jobFinished); @@ -257,10 +258,12 @@ const JobInvocationHostTable = ({ useEffect(() => { const filterChanged = initialFilter !== prevFilter.current; const statusChanged = jobFinished !== prevJobFinished.current; + const idChanged = id !== prevId.current; - if ((filterChanged || statusChanged) && initialFilter !== '') { + if ((filterChanged || statusChanged || idChanged) && initialFilter !== '') { prevFilter.current = initialFilter; prevJobFinished.current = jobFinished; + prevId.current = id; filterApiCall(); } }, [initialFilter, jobFinished, id, filterApiCall]); diff --git a/webpack/JobInvocationDetail/TemplateInvocation.js b/webpack/JobInvocationDetail/TemplateInvocation.js index 8f6ba51c5..ca1616f36 100644 --- a/webpack/JobInvocationDetail/TemplateInvocation.js +++ b/webpack/JobInvocationDetail/TemplateInvocation.js @@ -75,6 +75,8 @@ export const TemplateInvocation = ({ const dispatch = useDispatch(); const timeoutRef = useRef(null); + const responseRef = useRef(response); + responseRef.current = response; useEffect(() => { let cancelled = false; @@ -106,7 +108,9 @@ export const TemplateInvocation = ({ } if (isExpanded) { - dispatchFetch(); + if (responseRef.current?.finished !== true) { + dispatchFetch(); + } } return () => { From 9b0664d0fc9ea7a1721f0e7ddb7e3db2abf9d7fe Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Fri, 10 Jul 2026 13:03:40 +0200 Subject: [PATCH 31/35] Use sprintf placeholders for error response in cancelJob toast --- webpack/JobInvocationDetail/JobInvocationActions.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index 0ec0aa616..5372ddb96 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -63,8 +63,8 @@ export const cancelJob = (jobId, force) => dispatch => { : sprintf(__('Trying to cancel the job %s.'), jobId); const errorToast = response => force - ? sprintf(__(`Could not abort the job %s: ${response}`), jobId) - : sprintf(__(`Could not cancel the job %s: ${response}`), jobId); + ? sprintf(__('Could not abort the job %s: %s'), jobId, response) + : sprintf(__('Could not cancel the job %s: %s'), jobId, response); const url = force ? `/job_invocations/${jobId}/cancel?force=true` : `/job_invocations/${jobId}/cancel`; From f63e87732c9935340a9d43a214f59901dc36eda8 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Fri, 10 Jul 2026 13:56:28 +0200 Subject: [PATCH 32/35] Only fetch permissions on first load, not on every poll The host table polls the API every 5 seconds while a job is running. Previously every poll included include_permissions=true, causing a backend permission check on each request. Now permissions are only fetched on user-initiated calls (initial load, page change, search/filter). Poll calls omit include_permissions and instead merge the cached permissions back into each result so RowActions continues to render correctly. RowActions previously read permissions directly from the Redux store, which was overwritten by each poll response (without permissions), causing actions to disappear. It now receives permissions as a prop from the table row where the merged data lives. --- .../JobInvocationHostTable.js | 37 +++++++--- .../TemplateActionButtons.js | 16 ++++- .../JobInvocationHostTablePolling.test.js | 67 +++++++++++++++++++ 3 files changed, 109 insertions(+), 11 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationHostTable.js b/webpack/JobInvocationDetail/JobInvocationHostTable.js index 190df305a..9be1259f5 100644 --- a/webpack/JobInvocationDetail/JobInvocationHostTable.js +++ b/webpack/JobInvocationDetail/JobInvocationHostTable.js @@ -72,6 +72,7 @@ const JobInvocationHostTable = ({ const prevId = useRef(id); const pollTimeoutId = useRef(null); const currentPollParams = useRef({}); + const cachedPermissions = useRef({}); const jobFinishedRef = useRef(jobFinished); useEffect(() => { jobFinishedRef.current = jobFinished; @@ -159,26 +160,42 @@ const JobInvocationHostTable = ({ [initialFilter, urlSearchQuery] ); - const updateHostsState = useCallback(data => { - const ids = data.data.results.map(i => i.id); - setApiResponse(data.data); + const updateHostsState = useCallback((data, isPoll) => { + const { results } = data.data; + + if (!isPoll) { + cachedPermissions.current = Object.fromEntries( + results.map(result => [result.id, result.permissions]) + ); + } + + const mergedResults = results.map(result => ({ + ...result, + permissions: cachedPermissions.current[result.id], + })); + + const ids = mergedResults.map(i => i.id); + setApiResponse({ ...data.data, results: mergedResults }); setAllHostsIds(ids); setStatus(STATUS_UPPERCASE.RESOLVED); }, []); // Call hosts data with params const makeApiCall = useCallback( - requestParams => { + (requestParams, { isPoll = false } = {}) => { dispatch( APIActions.get({ key: JOB_INVOCATION_HOSTS, url: `/api/job_invocations/${id}/hosts`, - params: { include_permissions: true, ...requestParams }, + params: { + ...(!isPoll && { include_permissions: true }), + ...requestParams, + }, handleSuccess: data => { - updateHostsState(data); + updateHostsState(data, isPoll); if (!jobFinishedRef.current) { pollTimeoutId.current = setTimeout( - () => makeApiCall(currentPollParams.current), + () => makeApiCall(currentPollParams.current, { isPoll: true }), 5000 ); } else { @@ -487,7 +504,11 @@ const JobInvocationHostTable = ({ {columns[k].wrapper(result)} ))} - + { +export const RowActions = ({ hostID, jobID, permissions: permissionsProp }) => { const dispatch = useDispatch(); const response = useSelector(selectHostFromJobInvocationHosts(hostID)); - if (!response?.permissions) return null; - const { task, permissions } = response; + const permissions = permissionsProp ?? response?.permissions; + if (!permissions) return null; + const { task } = response || {}; const { id: taskID, cancellable: taskCancellable } = task || {}; const getActions = actions({ taskID, @@ -216,4 +217,13 @@ TemplateActionButtons.defaultProps = { RowActions.propTypes = { hostID: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired, jobID: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired, + permissions: PropTypes.shape({ + view_foreman_tasks: PropTypes.bool, + cancel_job_invocations: PropTypes.bool, + execute_jobs: PropTypes.bool, + }), +}; + +RowActions.defaultProps = { + permissions: null, }; diff --git a/webpack/JobInvocationDetail/__tests__/JobInvocationHostTablePolling.test.js b/webpack/JobInvocationDetail/__tests__/JobInvocationHostTablePolling.test.js index bee5ce34b..5aa042d51 100644 --- a/webpack/JobInvocationDetail/__tests__/JobInvocationHostTablePolling.test.js +++ b/webpack/JobInvocationDetail/__tests__/JobInvocationHostTablePolling.test.js @@ -104,6 +104,9 @@ describe('JobInvocationHostTable polling', () => { beforeEach(() => { jest.useFakeTimers({ legacyFakeTimers: true }); + jest + .requireMock('foremanReact/components/PF4/TableIndexPage/TableIndexPage') + .mockClear(); apiGetSpy = jest .spyOn(api.APIActions, 'get') .mockImplementation(({ handleSuccess, ...action }) => { @@ -219,6 +222,70 @@ describe('JobInvocationHostTable polling', () => { expect(apiGetSpy).toHaveBeenCalledTimes(1); }); + describe('include_permissions behaviour', () => { + const hostWithPermissions = { + id: 1, + permissions: { + view_foreman_tasks: true, + cancel_job_invocations: true, + execute_jobs: true, + }, + }; + + it('sends include_permissions on the first (non-poll) call', () => { + const store = makeStore(); + renderAndTriggerFetch(store); + + expect(apiGetSpy.mock.calls[0][0].params).toMatchObject({ + include_permissions: true, + }); + }); + + it('omits include_permissions on poll calls', () => { + const store = makeStore(); + renderAndTriggerFetch(store); + + act(() => jest.advanceTimersByTime(5000)); + + expect(apiGetSpy.mock.calls[1][0].params).not.toHaveProperty( + 'include_permissions' + ); + }); + + it('preserves permissions from the initial call in subsequent poll responses', () => { + apiGetSpy.mockImplementation(({ handleSuccess, params }) => { + handleSuccess && + handleSuccess({ + data: { + results: params.include_permissions ? [hostWithPermissions] : [{ id: 1 }], + total: 1, + subtotal: 1, + }, + }); + return { type: 'MOCK_GET' }; + }); + + const store = makeStore(); + renderAndTriggerFetch(store); + + // Advance to the first poll — this call has no include_permissions, + // so the raw result has no permissions field. + act(() => jest.advanceTimersByTime(5000)); + + const MockTableIndexPage = jest.requireMock( + 'foremanReact/components/PF4/TableIndexPage/TableIndexPage' + ); + const replacementResponse = + MockTableIndexPage.mock.calls[ + MockTableIndexPage.mock.calls.length - 1 + ][0].replacementResponse; + + expect(replacementResponse.response.results[0].permissions).toEqual( + hostWithPermissions.permissions + ); + }); + }); + it('uses the current id in the api url after id prop changes', () => { const store = makeStore(); renderAndTriggerFetch(store, { id: '42' }); From 5ee41fe7141d72e77292c2133df5dd960dc4f553 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Mon, 13 Jul 2026 16:46:23 +0200 Subject: [PATCH 33/35] Hoist host-independent permission checks out of the per-host loop authorized_for(:cancel_job_invocations) and authorized_for(create job_invocations) return the same value for every host, so calling them inside the hosts loop was redundant. Likewise User.current.can? for execute_jobs_on_infrastructure_hosts is user-scoped, not host-scoped. Pre-compute all three before the loop and reuse the cached booleans, keeping only host.infrastructure_host? inside the iteration. --- app/controllers/api/v2/job_invocations_controller.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/v2/job_invocations_controller.rb b/app/controllers/api/v2/job_invocations_controller.rb index 3dbb7467d..27b870029 100644 --- a/app/controllers/api/v2/job_invocations_controller.rb +++ b/app/controllers/api/v2/job_invocations_controller.rb @@ -335,12 +335,15 @@ def set_statuses_and_smart_proxies task = ti.run_host_job_task [ti.host_id, task] end + can_cancel_job_invocations = authorized_for(:permission => :cancel_job_invocations, :auth_object => @job_invocation) + can_create_job_invocations = authorized_for(controller: :job_invocations, action: :create) + can_execute_on_infra_hosts = User.current.can?(:execute_jobs_on_infrastructure_hosts) @permissions_by_host = hosts.to_h do |host| task = @task_by_host[host.id] [host.id, { :view_foreman_tasks => authorized_for(:permission => :view_foreman_tasks, :auth_object => task), - :cancel_job_invocations => authorized_for(:permission => :cancel_job_invocations, :auth_object => @job_invocation), - :execute_jobs => authorized_for(controller: :job_invocations, action: :create) && (!host.infrastructure_host? || User.current.can?(:execute_jobs_on_infrastructure_hosts)), + :cancel_job_invocations => can_cancel_job_invocations, + :execute_jobs => can_create_job_invocations && (!host.infrastructure_host? || can_execute_on_infra_hosts), }] end end From 022c4fa037c211273485f00712fa3e3027eedd01 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Mon, 13 Jul 2026 16:50:59 +0200 Subject: [PATCH 34/35] Replace module-level pollTimeoutId with useRef in JobInvocationActions The module-level singleton is architecturally inconsistent with how JobInvocationHostTable and TemplateInvocation manage their timeout IDs, and would silently break if two detail pages were ever rendered concurrently during React Router transitions. Move ownership of the timeout ID into the component via useRef, threading the ref through getJobInvocation and stopJobInvocationPolling. Callers that do not own the polling lifecycle (enableRecurringLogic) omit the argument and rely on the default { current: null } in fetchJobInvocation. --- .../JobInvocationActions.js | 24 +++++++-------- .../__tests__/JobInvocationPolling.test.js | 29 ++++++++++--------- webpack/JobInvocationDetail/index.js | 7 +++-- 3 files changed, 31 insertions(+), 29 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index 5372ddb96..d523226fb 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -9,8 +9,6 @@ import { STATUS, } from './JobInvocationConstants'; -let pollTimeoutId = null; - const FINISHED_STATUSES = [STATUS.FAILED, STATUS.SUCCEEDED, STATUS.CANCELLED]; export const isJobFinished = statusLabel => @@ -22,7 +20,7 @@ const extractErrorMessage = response => response?.data?.error?.message || 'Unknown error.'; -const fetchJobInvocation = (dispatch, url, params = {}) => { +const fetchJobInvocation = (dispatch, url, params = {}, pollTimeoutRef = { current: null }) => { dispatch( get({ key: JOB_INVOCATION_KEY, @@ -30,30 +28,30 @@ const fetchJobInvocation = (dispatch, url, params = {}) => { url, handleSuccess: ({ data }) => { if (!isJobFinished(data.status_label)) { - pollTimeoutId = setTimeout( - () => fetchJobInvocation(dispatch, url), + pollTimeoutRef.current = setTimeout( + () => fetchJobInvocation(dispatch, url, {}, pollTimeoutRef), 5000 ); } else { - pollTimeoutId = null; + pollTimeoutRef.current = null; } }, handleError: () => { - pollTimeoutId = null; + pollTimeoutRef.current = null; }, errorToast: ({ response }) => extractErrorMessage(response), }) ); }; -export const getJobInvocation = url => dispatch => { - stopJobInvocationPolling(); - fetchJobInvocation(dispatch, url, { include_permissions: true }); +export const getJobInvocation = (url, pollTimeoutRef) => dispatch => { + stopJobInvocationPolling(pollTimeoutRef); + fetchJobInvocation(dispatch, url, { include_permissions: true }, pollTimeoutRef); }; -export const stopJobInvocationPolling = () => { - clearTimeout(pollTimeoutId); - pollTimeoutId = null; +export const stopJobInvocationPolling = pollTimeoutRef => { + clearTimeout(pollTimeoutRef.current); + pollTimeoutRef.current = null; }; export const cancelJob = (jobId, force) => dispatch => { diff --git a/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js b/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js index d75bad17b..3a7e07436 100644 --- a/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js +++ b/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js @@ -14,6 +14,7 @@ jest.useFakeTimers(); const reducer = (state = {}) => state; const makeStore = () => createStore(reducer, applyMiddleware(thunk)); +const makeRef = () => ({ current: null }); const runningData = { status_label: 'running' }; const succeededData = { status_label: 'succeeded' }; @@ -30,9 +31,11 @@ const setupGetMock = responseData => { const url = '/api/job_invocations/1'; describe('job invocation polling', () => { + let ref; + beforeEach(() => { + ref = makeRef(); api.get.mockReset(); - stopJobInvocationPolling(); jest.clearAllTimers(); }); @@ -40,7 +43,7 @@ describe('job invocation polling', () => { setupGetMock(succeededData); const store = makeStore(); - store.dispatch(getJobInvocation(url)); + store.dispatch(getJobInvocation(url, ref)); expect(api.get).toHaveBeenCalledTimes(1); expect(api.get.mock.calls[0][0]).toMatchObject({ @@ -54,7 +57,7 @@ describe('job invocation polling', () => { setupGetMock(runningData); const store = makeStore(); - store.dispatch(getJobInvocation(url)); + store.dispatch(getJobInvocation(url, ref)); expect(api.get).toHaveBeenCalledTimes(1); jest.advanceTimersByTime(5000); @@ -65,7 +68,7 @@ describe('job invocation polling', () => { setupGetMock(runningData); const store = makeStore(); - store.dispatch(getJobInvocation(url)); + store.dispatch(getJobInvocation(url, ref)); jest.advanceTimersByTime(5000); expect(api.get).toHaveBeenCalledTimes(2); @@ -79,7 +82,7 @@ describe('job invocation polling', () => { setupGetMock(succeededData); const store = makeStore(); - store.dispatch(getJobInvocation(url)); + store.dispatch(getJobInvocation(url, ref)); expect(api.get).toHaveBeenCalledTimes(1); jest.advanceTimersByTime(5000); @@ -90,7 +93,7 @@ describe('job invocation polling', () => { setupGetMock(failedData); const store = makeStore(); - store.dispatch(getJobInvocation(url)); + store.dispatch(getJobInvocation(url, ref)); jest.advanceTimersByTime(5000); expect(api.get).toHaveBeenCalledTimes(1); @@ -100,7 +103,7 @@ describe('job invocation polling', () => { setupGetMock(cancelledData); const store = makeStore(); - store.dispatch(getJobInvocation(url)); + store.dispatch(getJobInvocation(url, ref)); jest.advanceTimersByTime(5000); expect(api.get).toHaveBeenCalledTimes(1); @@ -113,7 +116,7 @@ describe('job invocation polling', () => { }); const store = makeStore(); - store.dispatch(getJobInvocation(url)); + store.dispatch(getJobInvocation(url, ref)); jest.advanceTimersByTime(5000); expect(api.get).toHaveBeenCalledTimes(1); @@ -123,10 +126,10 @@ describe('job invocation polling', () => { setupGetMock(runningData); const store = makeStore(); - store.dispatch(getJobInvocation(url)); + store.dispatch(getJobInvocation(url, ref)); expect(api.get).toHaveBeenCalledTimes(1); - stopJobInvocationPolling(); + stopJobInvocationPolling(ref); jest.advanceTimersByTime(5000); expect(api.get).toHaveBeenCalledTimes(1); @@ -136,11 +139,11 @@ describe('job invocation polling', () => { setupGetMock(runningData); const store = makeStore(); - store.dispatch(getJobInvocation(url)); + store.dispatch(getJobInvocation(url, ref)); expect(api.get).toHaveBeenCalledTimes(1); // Second call before the timeout fires should cancel the pending poll - store.dispatch(getJobInvocation(url)); + store.dispatch(getJobInvocation(url, ref)); jest.advanceTimersByTime(5000); // Only one more poll should fire (from the second invocation), not two @@ -151,7 +154,7 @@ describe('job invocation polling', () => { setupGetMock(runningData); const store = makeStore(); - store.dispatch(getJobInvocation(url)); + store.dispatch(getJobInvocation(url, ref)); jest.advanceTimersByTime(15000); expect(api.get).toHaveBeenCalledTimes(4); diff --git a/webpack/JobInvocationDetail/index.js b/webpack/JobInvocationDetail/index.js index 7b87b9a6e..7125bf209 100644 --- a/webpack/JobInvocationDetail/index.js +++ b/webpack/JobInvocationDetail/index.js @@ -5,7 +5,7 @@ import { PageSectionVariants, Skeleton, } from '@patternfly/react-core'; -import React, { useEffect, useMemo, useState } from 'react'; +import React, { useEffect, useMemo, useRef, useState } from 'react'; import { translate as __, documentLocale } from 'foremanReact/common/I18n'; import { useDispatch, useSelector } from 'react-redux'; import PageLayout from 'foremanReact/routes/common/PageLayout/PageLayout'; @@ -47,6 +47,7 @@ const JobInvocationDetailPage = ({ history, }) => { const dispatch = useDispatch(); + const pollTimeoutRef = useRef(null); const items = useSelector(selectItems); const { description, @@ -85,9 +86,9 @@ const JobInvocationDetailPage = ({ } useEffect(() => { - dispatch(getJobInvocation(`/api/job_invocations/${id}`)); + dispatch(getJobInvocation(`/api/job_invocations/${id}`, pollTimeoutRef)); return () => { - stopJobInvocationPolling(); + stopJobInvocationPolling(pollTimeoutRef); }; }, [dispatch, id]); From 5bd334d682eb63bbc5ae32288527813f3b6884d4 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Mon, 13 Jul 2026 17:00:26 +0200 Subject: [PATCH 35/35] Add tests and fix nil task in authorized_for for hosts with include_permissions - Add test coverage for the #hosts API endpoint: - Response shape without include_permissions (task/permissions absent) - Response shape with include_permissions=true (task/permissions present, task includes id and cancellable fields, permissions includes all three keys) - Nil run_host_job_task when include_permissions=true (no crash, task is nil) - Add test for cancellable field in the task node of the #show response - Fix: guard view_foreman_tasks permission check with 'task &&' so that authorized_for is not called with a nil auth_object when a host has no run_host_job_task yet; previously this returned true for admin users even though there is no task to view --- .../api/v2/job_invocations_controller.rb | 2 +- .../api/v2/job_invocations_controller_test.rb | 68 +++++++++++++++++-- 2 files changed, 63 insertions(+), 7 deletions(-) diff --git a/app/controllers/api/v2/job_invocations_controller.rb b/app/controllers/api/v2/job_invocations_controller.rb index 27b870029..ba22257fa 100644 --- a/app/controllers/api/v2/job_invocations_controller.rb +++ b/app/controllers/api/v2/job_invocations_controller.rb @@ -341,7 +341,7 @@ def set_statuses_and_smart_proxies @permissions_by_host = hosts.to_h do |host| task = @task_by_host[host.id] [host.id, { - :view_foreman_tasks => authorized_for(:permission => :view_foreman_tasks, :auth_object => task), + :view_foreman_tasks => task && authorized_for(:permission => :view_foreman_tasks, :auth_object => task), :cancel_job_invocations => can_cancel_job_invocations, :execute_jobs => can_create_job_invocations && (!host.infrastructure_host? || can_execute_on_infra_hosts), }] diff --git a/test/functional/api/v2/job_invocations_controller_test.rb b/test/functional/api/v2/job_invocations_controller_test.rb index 2b9d84e2b..1008f8f4d 100644 --- a/test/functional/api/v2/job_invocations_controller_test.rb +++ b/test/functional/api/v2/job_invocations_controller_test.rb @@ -67,6 +67,14 @@ class JobInvocationsControllerTest < ActionController::TestCase assert_nil result['template_invocations'] end + test 'should include cancellable field in task node' do + get :show, params: { :id => @invocation.id } + assert_response :success + result = ActiveSupport::JSON.decode(@response.body) + assert result.key?('task'), 'task node should be present in show response' + assert result['task'].key?('cancellable'), 'cancellable field should be present in task node' + end + test 'should include job_status per host when host_status=true' do invocation = FactoryBot.create(:job_invocation, :with_template, :with_task) invocation.template_invocations << FactoryBot.create(:template_invocation, :with_task, :with_host, :job_invocation => invocation) @@ -284,19 +292,67 @@ class JobInvocationsControllerTest < ActionController::TestCase end describe '#hosts' do + setup do + @hosts_invocation = FactoryBot.create(:job_invocation, :with_template, :with_task) + 2.times { @hosts_invocation.template_invocations << FactoryBot.create(:template_invocation, :with_task, :with_host, :job_invocation => @hosts_invocation) } + @hosts_invocation.job_category = @hosts_invocation.pattern_template_invocations.first.template.job_category + @hosts_invocation.targeting.hosts = @hosts_invocation.template_invocations.map(&:host) + @hosts_invocation.save! + end + test 'should compute job_status for the paginated subset' do + get :hosts, params: { :id => @hosts_invocation.id, :page => 2, :per_page => 1 } + assert_response :success + result = ActiveSupport::JSON.decode(@response.body) + assert_equal 3, result['total'] + assert_equal 1, result['results'].size + assert_equal(['success'], result['results'].map { |r| r['job_status'] }) + end + + test 'should not include task or permissions nodes without include_permissions' do + get :hosts, params: { :id => @hosts_invocation.id } + assert_response :success + result = ActiveSupport::JSON.decode(@response.body) + assert_not_empty result['results'] + result['results'].each do |host| + assert_not host.key?('task'), 'task node should be absent when include_permissions is not set' + assert_not host.key?('permissions'), 'permissions node should be absent when include_permissions is not set' + end + end + + test 'should include task and permissions nodes when include_permissions=true' do + get :hosts, params: { :id => @hosts_invocation.id, :include_permissions => true } + assert_response :success + result = ActiveSupport::JSON.decode(@response.body) + assert_not_empty result['results'] + result['results'].each do |host| + assert host.key?('task'), 'task node should be present when include_permissions=true' + assert host.key?('permissions'), 'permissions node should be present when include_permissions=true' + assert host['task'].key?('id') + assert host['task'].key?('cancellable') + assert host['permissions'].key?('view_foreman_tasks') + assert host['permissions'].key?('cancel_job_invocations') + assert host['permissions'].key?('execute_jobs') + end + end + + test 'should handle nil run_host_job_task with include_permissions=true' do + # Create a host without a run_host_job_task (template invocation without :with_task) invocation = FactoryBot.create(:job_invocation, :with_template, :with_task) - 2.times { invocation.template_invocations << FactoryBot.create(:template_invocation, :with_task, :with_host, :job_invocation => invocation) } + ti = FactoryBot.create(:template_invocation, :with_host, :job_invocation => invocation) invocation.job_category = invocation.pattern_template_invocations.first.template.job_category - invocation.targeting.hosts = invocation.template_invocations.map(&:host) + invocation.targeting.hosts = [ti.host] invocation.save! - get :hosts, params: { :id => invocation.id, :page => 2, :per_page => 1 } + get :hosts, params: { :id => invocation.id, :include_permissions => true } assert_response :success result = ActiveSupport::JSON.decode(@response.body) - assert_equal 3, result['total'] - assert_equal 1, result['results'].size - assert_equal(['success'], result['results'].map { |r| r['job_status'] }) + assert_not_empty result['results'] + host_result = result['results'].first + assert host_result.key?('task'), 'task node should be present' + assert_nil host_result['task'], 'task should be nil when run_host_job_task is absent' + assert host_result.key?('permissions'), 'permissions node should be present' + refute host_result['permissions']['view_foreman_tasks'] end end