From 9b2b2515fe7b00984b8471d1a040118d97833f2f Mon Sep 17 00:00:00 2001 From: MariaAga Date: Tue, 21 Jul 2026 15:44:59 +0100 Subject: [PATCH 1/5] drop: Fix failing new lint rules --- .../JobInvocationActions.js | 3 ++- .../JobInvocationConstants.js | 7 +++++++ .../JobInvocationDetail.scss | 8 ++++--- .../JobInvocationSystemStatusChart.js | 7 +++++-- .../JobInvocationDetail/TemplateInvocation.js | 11 ++++++++-- .../OutputCodeBlock.js | 5 ++++- .../TemplateInvocationComponents/index.scss | 1 - webpack/JobWizard/JobWizard.js | 6 +++++- webpack/JobWizard/JobWizard.scss | 21 +++++++++++++++++++ webpack/JobWizard/JobWizardConstants.js | 9 ++++++++ .../JobWizard/steps/Schedule/RepeatHour.js | 8 +++++-- .../JobWizard/steps/Schedule/RepeatWeek.js | 5 +++-- .../steps/Schedule/ScheduleRecurring.js | 4 ++-- .../JobWizard/steps/Schedule/ScheduleType.js | 5 ++++- .../JobWizard/steps/form/DateTimePicker.js | 18 ++++++++++------ webpack/JobWizard/steps/form/FormHelpers.js | 9 ++++---- .../components/FeaturesDropdown/index.scss | 2 ++ .../TargetingHosts/TargetingHostsConsts.js | 1 + .../TargetingHostsLabelsRow.scss | 8 +++++-- .../components/TargetingHosts/index.js | 7 +++++-- .../redux/actions/jobInvocations/index.js | 3 ++- webpack/react_app/redux/consts.js | 1 + 22 files changed, 116 insertions(+), 33 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index 131696068..e89bdeeb0 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -12,6 +12,7 @@ import { CHANGE_ENABLED_RECURRING_LOGIC, GET_TASK, JOB_INVOCATION_KEY, + POLLING_INTERVAL_MS, UPDATE_JOB, } from './JobInvocationConstants'; @@ -32,7 +33,7 @@ export const getJobInvocation = url => dispatch => { response?.data?.error?.message || 'Error', }), - 1000 + POLLING_INTERVAL_MS ); dispatch(fetchData); diff --git a/webpack/JobInvocationDetail/JobInvocationConstants.js b/webpack/JobInvocationDetail/JobInvocationConstants.js index 12453eca2..b7271ac78 100644 --- a/webpack/JobInvocationDetail/JobInvocationConstants.js +++ b/webpack/JobInvocationDetail/JobInvocationConstants.js @@ -20,6 +20,13 @@ 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 POLLING_INTERVAL_MS = 1000; +export const AUTO_REFRESH_INTERVAL_MS = 5000; +export const UNIX_TO_JS_TIMESTAMP_FACTOR = 1000; +export const DEFAULT_CHART_LEGEND_WIDTH = 270; +export const CLIPBOARD_COPIED_EXIT_DELAY_MS = 1500; +export const CLIPBOARD_DEFAULT_EXIT_DELAY_MS = 600; + export const showTemplateInvocationUrl = (hostID, jobID) => `/show_template_invocation_by_host/${hostID}/job_invocation/${jobID}`; export const LIST_TEMPLATE_INVOCATIONS = 'LIST_TEMPLATE_INVOCATIONS'; diff --git a/webpack/JobInvocationDetail/JobInvocationDetail.scss b/webpack/JobInvocationDetail/JobInvocationDetail.scss index 34e0d7e24..2742f0bc3 100644 --- a/webpack/JobInvocationDetail/JobInvocationDetail.scss +++ b/webpack/JobInvocationDetail/JobInvocationDetail.scss @@ -1,5 +1,6 @@ .job-invocation-detail-flex { $chart_size: 105px; + padding-top: 0px; padding-left: 10px; display: flex; @@ -92,16 +93,17 @@ section.job-additional-info { max-height: 25em; } } + div.invocation-output { display: block; padding: 9.5px; margin: 0 0 10px; font-size: 12px; word-break: break-all; - word-wrap: break-word; + overflow-wrap: break-word; color: rgba(255, 255, 255, 1); background-color: rgba(47, 47, 47, 1); - border: 1px solid #000000; + border: 1px solid #000; border-radius: 0px; font-family: Menlo, Monaco, Consolas, monospace; @@ -127,7 +129,7 @@ section.job-additional-info { } a { - color: #ffffff; + color: #fff; } a.scroll-link{ diff --git a/webpack/JobInvocationDetail/JobInvocationSystemStatusChart.js b/webpack/JobInvocationDetail/JobInvocationSystemStatusChart.js index f44f9b707..77a46e3c7 100644 --- a/webpack/JobInvocationDetail/JobInvocationSystemStatusChart.js +++ b/webpack/JobInvocationDetail/JobInvocationSystemStatusChart.js @@ -23,7 +23,10 @@ import { global_palette_blue_300 as inProgressColor, global_palette_green_500 as successedColor, } from '@patternfly/react-tokens'; -import { STATUS_TITLES } from './JobInvocationConstants'; +import { + STATUS_TITLES, + DEFAULT_CHART_LEGEND_WIDTH, +} from './JobInvocationConstants'; import './JobInvocationDetail.scss'; const JobInvocationSystemStatusChart = ({ @@ -52,7 +55,7 @@ const JobInvocationSystemStatusChart = ({ return '0'; }; const chartSize = 105; - const [legendWidth, setLegendWidth] = useState(270); + const [legendWidth, setLegendWidth] = useState(DEFAULT_CHART_LEGEND_WIDTH); // Calculates chart legend width based on its content useEffect(() => { diff --git a/webpack/JobInvocationDetail/TemplateInvocation.js b/webpack/JobInvocationDetail/TemplateInvocation.js index 9045be218..faad034f8 100644 --- a/webpack/JobInvocationDetail/TemplateInvocation.js +++ b/webpack/JobInvocationDetail/TemplateInvocation.js @@ -11,6 +11,9 @@ import { showTemplateInvocationUrl, templateInvocationPageUrl, GET_TEMPLATE_INVOCATION, + CLIPBOARD_COPIED_EXIT_DELAY_MS, + CLIPBOARD_DEFAULT_EXIT_DELAY_MS, + AUTO_REFRESH_INTERVAL_MS, } from './JobInvocationConstants'; import { selectTemplateInvocationStatus, @@ -41,7 +44,11 @@ const CopyToClipboard = ({ fullOutput }) => { textId="code-content" aria-label="Copy to clipboard" onClick={e => onClick(e, fullOutput)} - exitDelay={copied ? 1500 : 600} + exitDelay={ + copied + ? CLIPBOARD_COPIED_EXIT_DELAY_MS + : CLIPBOARD_DEFAULT_EXIT_DELAY_MS + } maxWidth="110px" variant="plain" onTooltipHidden={() => setCopied(false)} @@ -111,7 +118,7 @@ export const TemplateInvocation = ({ } else if (intervalRef.current) { clearInterval(intervalRef.current); } - }, 5000); + }, AUTO_REFRESH_INTERVAL_MS); } return () => { diff --git a/webpack/JobInvocationDetail/TemplateInvocationComponents/OutputCodeBlock.js b/webpack/JobInvocationDetail/TemplateInvocationComponents/OutputCodeBlock.js index 759b7b280..3a4a57644 100644 --- a/webpack/JobInvocationDetail/TemplateInvocationComponents/OutputCodeBlock.js +++ b/webpack/JobInvocationDetail/TemplateInvocationComponents/OutputCodeBlock.js @@ -7,6 +7,7 @@ import React, { import PropTypes from 'prop-types'; import { Button } from '@patternfly/react-core'; import { translate as __ } from 'foremanReact/common/I18n'; +import { UNIX_TO_JS_TIMESTAMP_FACTOR } from '../JobInvocationConstants'; export const OutputCodeBlock = ({ code, showOutputType, scrollElement }) => { let lineCounter = 0; @@ -122,7 +123,9 @@ export const OutputCodeBlock = ({ code, showOutputType, scrollElement }) => {
{lineCounter.toString().padStart(4, '\u00A0')}:{' '} diff --git a/webpack/JobInvocationDetail/TemplateInvocationComponents/index.scss b/webpack/JobInvocationDetail/TemplateInvocationComponents/index.scss index 893d701c3..550247e59 100644 --- a/webpack/JobInvocationDetail/TemplateInvocationComponents/index.scss +++ b/webpack/JobInvocationDetail/TemplateInvocationComponents/index.scss @@ -1,5 +1,4 @@ .jobs-table-action-item a { - color: inherit; text-decoration: none; diff --git a/webpack/JobWizard/JobWizard.js b/webpack/JobWizard/JobWizard.js index 68f2a820c..8e244c628 100644 --- a/webpack/JobWizard/JobWizard.js +++ b/webpack/JobWizard/JobWizard.js @@ -18,6 +18,7 @@ import { WIZARD_TITLES, SCHEDULE_TYPES, initialScheduleState, + STARTS_ERROR_CHECK_INTERVAL_MS, } from './JobWizardConstants'; import { selectTemplateError, @@ -237,7 +238,10 @@ export const JobWizard = ({ rerunData }) => { } }; updateStartsError(); - const interval = setInterval(updateStartsError, 5000); + const interval = setInterval( + updateStartsError, + STARTS_ERROR_CHECK_INTERVAL_MS + ); return () => { interval && clearInterval(interval); diff --git a/webpack/JobWizard/JobWizard.scss b/webpack/JobWizard/JobWizard.scss index 08e3afb0b..d8e638138 100644 --- a/webpack/JobWizard/JobWizard.scss +++ b/webpack/JobWizard/JobWizard.scss @@ -1,5 +1,6 @@ .job-wizard { font-size: var(--pf-v5-global--FontSize--md); + .wizard-title { margin-bottom: 25px; } @@ -15,6 +16,7 @@ var(--pf-v5-cwizard__toggle--ZIndex) + 1 ); // So the select box can be shown above the wizard footer and navigation toggle } + .pf-v5-c-wizard__main-body { @media (max-width: 600px) { max-width: 100%; @@ -41,6 +43,7 @@ .pf-v5-c-chip-group.pf-m-category { margin-bottom: 10px; } + .pf-v5-c-select__toggle-typeahead { border: 0px; } @@ -50,62 +53,76 @@ flex-wrap: nowrap; } } + .foreman-search-field { width: 100%; } } + input[type='radio'], input[type='checkbox'] { margin: 0; } + .schedule-tab { #repeat-on-weekly { display: grid; grid-template-columns: repeat(7, 1fr); } + .pf-v5-l-grid { gap: var(--pf-v5-cform--GridGap); } + #repeat-on-hourly { max-height: 300px; overflow: scroll; } + .schedule-radio label { width: 100%; } + .schedule-radio input { align-self: center; } + .schedule-radio-repeat-text { width: 100px; display: inline-block; margin-right: 5px; align-self: center; } + .schedule-radio-title { width: 80px; display: inline-block; align-self: center; } + .schedule-radio-occurences { display: inline-block; align-self: center; } + .schedule-radio-wrapper { display: flex; } } + .future-schedule-tab { .clear-datetime-button { margin-left: 10px; align-self: center; font-size: var(--pf-v5-global--FontSize--md); } + .pf-v5-c-form__group-control { display: flex; flex-wrap: wrap; } } + .pf-v5-c-date-picker { vertical-align: top; } @@ -119,10 +136,12 @@ // overwriting bootstrap/_forms.scss margin: 4px 0 0; margin: 0; } + textarea { min-height: 40px; min-width: 100px; } + .pf-v5-c-modal-box { width: auto; } @@ -132,9 +151,11 @@ margin-left: 10px; } } + .pf-v5-c-radio__body { font-size: var(--pf-v5-cradio__label--FontSize); } + .reset-default { padding-bottom: 0; } diff --git a/webpack/JobWizard/JobWizardConstants.js b/webpack/JobWizard/JobWizardConstants.js index ca1bfcdc0..a1db56bca 100644 --- a/webpack/JobWizard/JobWizardConstants.js +++ b/webpack/JobWizard/JobWizardConstants.js @@ -78,3 +78,12 @@ export const HOST_IDS = 'HOST_IDS'; export const REX_FEATURE = 'REX_FEATURE'; export const JOB_API_KEY = 'JOB_API_KEY'; + +export const MINUTES_IN_HOUR = 60; +export const DAYS_IN_WEEK = 7; +export const SUNDAY_BASE_YEAR = 2017; +export const DEFAULT_MINUTE_OPTIONS = [0, 15, 30, 45]; +export const MS_PER_MINUTE = 60000; +export const STARTS_ERROR_CHECK_INTERVAL_MS = 5000; +export const DATE_PADDING_SLICE = -2; +export const DEBOUNCE_INPUT_MS = 1000; diff --git a/webpack/JobWizard/steps/Schedule/RepeatHour.js b/webpack/JobWizard/steps/Schedule/RepeatHour.js index 28e8f588e..90a7ff0a6 100644 --- a/webpack/JobWizard/steps/Schedule/RepeatHour.js +++ b/webpack/JobWizard/steps/Schedule/RepeatHour.js @@ -13,12 +13,16 @@ import { } from '@patternfly/react-core/deprecated'; import { translate as __ } from 'foremanReact/common/I18n'; import { helpLabel } from '../form/FormHelpers'; +import { + MINUTES_IN_HOUR, + DEFAULT_MINUTE_OPTIONS, +} from '../../JobWizardConstants'; export const RepeatHour = ({ repeatData, setRepeatData }) => { const isValidMinute = newMinute => Number.isInteger(parseInt(newMinute, 10)) && newMinute >= 0 && - newMinute < 60; + newMinute < MINUTES_IN_HOUR; const { minute } = repeatData; useEffect(() => { @@ -27,7 +31,7 @@ export const RepeatHour = ({ repeatData, setRepeatData }) => { } }, [minute, setRepeatData]); const [minuteOpen, setMinuteOpen] = useState(false); - const [options, setOptions] = useState([0, 15, 30, 45]); + const [options, setOptions] = useState(DEFAULT_MINUTE_OPTIONS); const [isAlertOpen, setIsAlertOpen] = useState(false); return ( { const locale = documentLocale().replace(/-/g, '_'); - const baseDate = new Date(Date.UTC(2017, 0, 1)); // just a Sunday + const baseDate = new Date(Date.UTC(SUNDAY_BASE_YEAR, 0, 1)); // just a Sunday const weekDays = []; const formatOptions = { weekday: 'short', timeZone: 'UTC' }; - for (let i = 0; i < 7; i++) { + for (let i = 0; i < DAYS_IN_WEEK; i++) { try { weekDays.push(baseDate.toLocaleDateString(locale, formatOptions)); } catch { diff --git a/webpack/JobWizard/steps/Schedule/ScheduleRecurring.js b/webpack/JobWizard/steps/Schedule/ScheduleRecurring.js index 6668a9924..d2427b896 100644 --- a/webpack/JobWizard/steps/Schedule/ScheduleRecurring.js +++ b/webpack/JobWizard/steps/Schedule/ScheduleRecurring.js @@ -13,7 +13,7 @@ import { import { ExclamationCircleIcon } from '@patternfly/react-icons'; import { translate as __ } from 'foremanReact/common/I18n'; import { RepeatOn } from './RepeatOn'; -import { SCHEDULE_TYPES } from '../../JobWizardConstants'; +import { SCHEDULE_TYPES, MS_PER_MINUTE } from '../../JobWizardConstants'; import { PurposeField } from './PurposeField'; import { DateTimePicker } from '../form/DateTimePicker'; import { WizardTitle } from '../form/WizardTitle'; @@ -114,7 +114,7 @@ export const ScheduleRecurring = ({ setScheduleValue(current => ({ ...current, startsAt: new Date( - new Date().getTime() + 60000 + new Date().getTime() + MS_PER_MINUTE ).toISOString(), // 1 minute in the future isFuture: true, })) diff --git a/webpack/JobWizard/steps/Schedule/ScheduleType.js b/webpack/JobWizard/steps/Schedule/ScheduleType.js index 77862c1f4..07857fa40 100644 --- a/webpack/JobWizard/steps/Schedule/ScheduleType.js +++ b/webpack/JobWizard/steps/Schedule/ScheduleType.js @@ -6,6 +6,7 @@ import { WIZARD_TITLES, SCHEDULE_TYPES, repeatTypes, + MS_PER_MINUTE, } from '../../JobWizardConstants'; import { WizardTitle } from '../form/WizardTitle'; import { QueryType } from './QueryType'; @@ -49,7 +50,9 @@ export const ScheduleType = ({ onChange={() => { setScheduleValue(current => ({ ...current, - startsAt: new Date(new Date().getTime() + 60000).toISOString(), // 1 minute in the future + startsAt: new Date( + new Date().getTime() + MS_PER_MINUTE + ).toISOString(), // 1 minute in the future scheduleType: SCHEDULE_TYPES.FUTURE, repeatType: repeatTypes.noRepeat, })); diff --git a/webpack/JobWizard/steps/form/DateTimePicker.js b/webpack/JobWizard/steps/form/DateTimePicker.js index 4c12012b6..2917d968e 100644 --- a/webpack/JobWizard/steps/form/DateTimePicker.js +++ b/webpack/JobWizard/steps/form/DateTimePicker.js @@ -7,13 +7,19 @@ import { } from '@patternfly/react-core'; import { debounce } from 'lodash'; import { translate as __, documentLocale } from 'foremanReact/common/I18n'; +import { + DATE_PADDING_SLICE, + DEBOUNCE_INPUT_MS, +} from '../../JobWizardConstants'; const formatDateTime = d => `${d.getFullYear()}-${`0${d.getMonth() + 1}`.slice( - -2 - )}-${`0${d.getDate()}`.slice(-2)} ${`0${d.getHours()}`.slice( - -2 - )}:${`0${d.getMinutes()}`.slice(-2)}:${`0${d.getSeconds()}`.slice(-2)}`; + DATE_PADDING_SLICE + )}-${`0${d.getDate()}`.slice(DATE_PADDING_SLICE)} ${`0${d.getHours()}`.slice( + DATE_PADDING_SLICE + )}:${`0${d.getMinutes()}`.slice( + DATE_PADDING_SLICE + )}:${`0${d.getSeconds()}`.slice(DATE_PADDING_SLICE)}`; export const DateTimePicker = ({ dateTime, @@ -103,7 +109,7 @@ export const DateTimePicker = ({ aria-label={`${ariaLabel} datepicker`} value={formattedDate} placeholder="yyyy/mm/dd" - onChange={debounce(onDateChange, 1000, { + onChange={debounce(onDateChange, DEBOUNCE_INPUT_MS, { leading: false, trailing: true, })} @@ -123,7 +129,7 @@ export const DateTimePicker = ({ time={dateTime ? dateObject.toString() : ''} inputProps={dateTime ? {} : { value: '' }} placeholder={includeSeconds ? 'hh:mm:ss' : 'hh:mm'} - onChange={debounce(onTimeChange, 1000, { + onChange={debounce(onTimeChange, DEBOUNCE_INPUT_MS, { leading: false, trailing: true, })} diff --git a/webpack/JobWizard/steps/form/FormHelpers.js b/webpack/JobWizard/steps/form/FormHelpers.js index 29d0fb324..2209342c2 100644 --- a/webpack/JobWizard/steps/form/FormHelpers.js +++ b/webpack/JobWizard/steps/form/FormHelpers.js @@ -8,16 +8,17 @@ export const helpLabel = (text, id) => { if (!text) return null; return ( - + ); }; diff --git a/webpack/react_app/components/FeaturesDropdown/index.scss b/webpack/react_app/components/FeaturesDropdown/index.scss index e7e90c486..8f36f5465 100644 --- a/webpack/react_app/components/FeaturesDropdown/index.scss +++ b/webpack/react_app/components/FeaturesDropdown/index.scss @@ -1,7 +1,9 @@ #schedule-a-job-dropdown ul.pf-v5-c-dropdown__menu { padding-left: 0; + li { display: unset; + a { font-size: 16px; color: var(--pf-v5-cdropdown__menu-item--Color); diff --git a/webpack/react_app/components/TargetingHosts/TargetingHostsConsts.js b/webpack/react_app/components/TargetingHosts/TargetingHostsConsts.js index 5fd27d8c8..3642c0393 100644 --- a/webpack/react_app/components/TargetingHosts/TargetingHostsConsts.js +++ b/webpack/react_app/components/TargetingHosts/TargetingHostsConsts.js @@ -1,2 +1,3 @@ export const TARGETING_HOSTS = 'TARGETING_HOSTS'; export const TARGETING_HOSTS_AUTOCOMPLETE = 'targeting_hosts_search'; +export const TARGETING_HOSTS_POLLING_INTERVAL_MS = 1000; diff --git a/webpack/react_app/components/TargetingHosts/TargetingHostsLabelsRow.scss b/webpack/react_app/components/TargetingHosts/TargetingHostsLabelsRow.scss index a2c5713a0..b4b1139f8 100644 --- a/webpack/react_app/components/TargetingHosts/TargetingHostsLabelsRow.scss +++ b/webpack/react_app/components/TargetingHosts/TargetingHostsLabelsRow.scss @@ -3,24 +3,28 @@ .tasks-labels-row { margin: 0; padding: 10px; + .title { font-weight: 600; font-size: 13px; } + .label { font-size: 100%; - margin-left: 5px; margin-right: 5px; + a { padding-left: 10px; } } + .pficon-close { color: $color-pf-white; } + .compound-label-pf { - margin-left: 0; margin: 10px; + margin-left: 0; } } diff --git a/webpack/react_app/components/TargetingHosts/index.js b/webpack/react_app/components/TargetingHosts/index.js index 76f1ec728..415fb660b 100644 --- a/webpack/react_app/components/TargetingHosts/index.js +++ b/webpack/react_app/components/TargetingHosts/index.js @@ -17,7 +17,10 @@ import { selectStatusFilter, } from './TargetingHostsSelectors'; import { getApiUrl } from './TargetingHostsHelpers'; -import { TARGETING_HOSTS } from './TargetingHostsConsts'; +import { + TARGETING_HOSTS, + TARGETING_HOSTS_POLLING_INTERVAL_MS, +} from './TargetingHostsConsts'; import TargetingHostsPage from './TargetingHostsPage'; import { chartFilter } from '../../redux/actions/jobInvocations'; @@ -100,7 +103,7 @@ const WrappedTargetingHosts = () => { dispatch(stopInterval(TARGETING_HOSTS)); }, }), - 1000 + TARGETING_HOSTS_POLLING_INTERVAL_MS ), [dispatch] ); diff --git a/webpack/react_app/redux/actions/jobInvocations/index.js b/webpack/react_app/redux/actions/jobInvocations/index.js index 968d83923..885712656 100644 --- a/webpack/react_app/redux/actions/jobInvocations/index.js +++ b/webpack/react_app/redux/actions/jobInvocations/index.js @@ -5,6 +5,7 @@ import { JOB_INVOCATIONS_GET_JOB_INVOCATIONS, JOB_INVOCATIONS_POLLING_STARTED, JOB_INVOCATIONS_JOB_FINISHED, + HTTP_UNAUTHORIZED, } from '../../consts'; const defaultJobInvocationsPollingInterval = 1000; @@ -32,7 +33,7 @@ const getJobInvocations = url => async (dispatch, getState) => { } function onGetJobInvocationsFailed(error) { - if (error.response.status === 401) { + if (error.response.status === HTTP_UNAUTHORIZED) { window.location.replace('/users/login'); } } diff --git a/webpack/react_app/redux/consts.js b/webpack/react_app/redux/consts.js index 02c09fc35..1cdc74474 100644 --- a/webpack/react_app/redux/consts.js +++ b/webpack/react_app/redux/consts.js @@ -3,3 +3,4 @@ export const JOB_INVOCATIONS_POLLING_STARTED = export const JOB_INVOCATIONS_GET_JOB_INVOCATIONS = 'JOB_INVOCATIONS_GET_JOB_INVOCATIONS'; export const JOB_INVOCATIONS_JOB_FINISHED = 'JOB_INVOCATIONS_JOB_FINISHED'; +export const HTTP_UNAUTHORIZED = 401; From 440bc82252606803d96cc95726b8a89c142ff68c Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Tue, 21 Jul 2026 11:14:38 +0200 Subject: [PATCH 2/5] Refs #39253 - Add explicit polling to host table The host table previously relied on an accidental refresh cascade: parent polling updated Redux state, which changed callback identities, which triggered useEffect, which re-fetched hosts. This adds an explicit setTimeout-based polling chain (5s interval) that stops when the job finishes or on API error, and restarts when filters change. Co-Authored-By: Claude Opus 4.6 --- .../JobInvocationHostTable.js | 112 +++--- .../JobInvocationHostTablePolling.test.js | 336 ++++++++++++++++++ webpack/JobInvocationDetail/index.js | 4 +- 3 files changed, 407 insertions(+), 45 deletions(-) create mode 100644 webpack/JobInvocationDetail/__tests__/JobInvocationHostTablePolling.test.js diff --git a/webpack/JobInvocationDetail/JobInvocationHostTable.js b/webpack/JobInvocationDetail/JobInvocationHostTable.js index 9daaafe58..17aadfef5 100644 --- a/webpack/JobInvocationDetail/JobInvocationHostTable.js +++ b/webpack/JobInvocationDetail/JobInvocationHostTable.js @@ -41,8 +41,8 @@ import Columns, { JOB_INVOCATION_HOSTS, LIST_TEMPLATE_INVOCATIONS, STATUS_UPPERCASE, - ALL_JOB_HOSTS, AWAITING_STATUS_FILTER, + AUTO_REFRESH_INTERVAL_MS, } from './JobInvocationConstants'; import { TemplateInvocation } from './TemplateInvocation'; import { RowActions } from './TemplateInvocationComponents/TemplateActionButtons'; @@ -51,8 +51,8 @@ import { PopupAlert } from './OpenAllInvocationsModal'; const JobInvocationHostTable = ({ id, initialFilter, + jobFinished, onFilterUpdate, - statusLabel, targeting, }) => { const columns = Columns(); @@ -69,7 +69,16 @@ const JobInvocationHostTable = ({ // Expansive items const [expandedHost, setExpandedHost] = useState(new Set()); - const prevStatusLabel = useRef(statusLabel); + const prevJobFinished = useRef(jobFinished); + const prevFilter = useRef(''); + const prevId = useRef(id); + const pollTimeoutId = useRef(null); + const currentPollParams = useRef({}); + const mountedRef = useRef(true); + const jobFinishedRef = useRef(jobFinished); + useEffect(() => { + jobFinishedRef.current = jobFinished; + }, [jobFinished]); const [hostInvocationStates, setHostInvocationStates] = useState({}); @@ -153,33 +162,45 @@ const JobInvocationHostTable = ({ [initialFilter, urlSearchQuery] ); - const handleResponse = useCallback((data, key) => { - if (key === JOB_INVOCATION_HOSTS) { - const ids = data.data.results.map(i => i.id); - - setApiResponse(data.data); - setAllHostsIds(ids); - } - + const updateHostsState = useCallback(data => { + 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), - handleError: () => setStatus(STATUS_UPPERCASE.ERROR), + handleSuccess: data => { + if (!mountedRef.current) return; + updateHostsState(data); + if (!jobFinishedRef.current) { + pollTimeoutId.current = setTimeout( + () => makeApiCall(currentPollParams.current), + AUTO_REFRESH_INTERVAL_MS + ); + } else { + pollTimeoutId.current = null; + } + }, + 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 || + __('Failed to load host invocation data'), }) ); }, - [dispatch, id, handleResponse] + [dispatch, id, updateHostsState] ); const filterApiCall = useCallback( @@ -202,7 +223,11 @@ const JobInvocationHostTable = ({ finalParams.search = filterSearch; } - makeApiCall(finalParams, { key: JOB_INVOCATION_HOSTS }); + currentPollParams.current = finalParams; + clearTimeout(pollTimeoutId.current); + pollTimeoutId.current = null; + + makeApiCall(finalParams); const urlSearchParams = new URLSearchParams(window.location.search); @@ -222,26 +247,17 @@ const JobInvocationHostTable = ({ ] ); - // Filter change - const handleFilterChange = useCallback( - newFilter => { - onFilterUpdate(newFilter); - }, - [onFilterUpdate] - ); - // Effects // run after mount const initializedRef = useRef(false); useEffect(() => { if (!initializedRef.current) { - // Job Invo template load - makeApiCall( - {}, - { - url: `/job_invocations/${id}/hosts`, + dispatch( + APIActions.get({ key: LIST_TEMPLATE_INVOCATIONS, - } + url: `/job_invocations/${id}/hosts`, + params: {}, + }) ); if (initialFilter === '') { @@ -249,16 +265,28 @@ const JobInvocationHostTable = ({ } initializedRef.current = true; } - }, [makeApiCall, id, initialFilter, onFilterUpdate]); + }, [dispatch, id, initialFilter, onFilterUpdate]); useEffect(() => { - if (initialFilter !== '') filterApiCall(); - - if (statusLabel !== prevStatusLabel.current) { - prevStatusLabel.current = statusLabel; + const filterChanged = initialFilter !== prevFilter.current; + const statusChanged = jobFinished !== prevJobFinished.current; + const idChanged = id !== prevId.current; + + if ((filterChanged || statusChanged || idChanged) && initialFilter !== '') { + prevFilter.current = initialFilter; + prevJobFinished.current = jobFinished; + prevId.current = id; filterApiCall(); } - }, [initialFilter, statusLabel, id, filterApiCall]); + }, [initialFilter, jobFinished, id, filterApiCall]); + + useEffect( + () => () => { + mountedRef.current = false; + clearTimeout(pollTimeoutId.current); + }, + [] + ); const { updateSearchQuery: updateSearchQueryBulk, @@ -403,7 +431,7 @@ const JobInvocationHostTable = ({ , 0 ? fetchBulkParams() : null} @@ -541,13 +569,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/__tests__/JobInvocationHostTablePolling.test.js b/webpack/JobInvocationDetail/__tests__/JobInvocationHostTablePolling.test.js new file mode 100644 index 000000000..94140cfd7 --- /dev/null +++ b/webpack/JobInvocationDetail/__tests__/JobInvocationHostTablePolling.test.js @@ -0,0 +1,336 @@ +import React from 'react'; +import { render, act } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { Provider } from 'react-redux'; +import configureMockStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import { Router } from 'react-router-dom'; +import { createMemoryHistory } from 'history'; +import { APIActions } from 'foremanReact/redux/API'; +import { createForemanContextWrapper } from './foremanTestHelpers'; +import JobInvocationHostTable from '../JobInvocationHostTable'; +import { JOB_INVOCATION_HOSTS } from '../JobInvocationConstants'; + +jest.useFakeTimers(); + +jest.mock('foremanReact/redux/API/APISelectors', () => + jest.requireActual('foremanReact/redux/API/APISelectors') +); + +jest.mock('foremanReact/common/hooks/Permissions/permissionHooks', () => ({ + usePermissions: jest.fn(() => true), +})); + +jest.mock('../TemplateInvocation', () => ({ + TemplateInvocation: () =>
, +})); + +jest.mock('../TemplateInvocationComponents/TemplateActionButtons', () => ({ + RowActions: () =>
, +})); + +const mockStore = configureMockStore([thunk]); + +const hostsResponse = { + total: 1, + subtotal: 1, + page: 1, + per_page: 20, + results: [ + { + id: 1, + name: 'host1.example.com', + operatingsystem_id: 1, + operatingsystem_name: 'RHEL 9', + hostgroup_id: 1, + hostgroup_name: 'default', + job_status: 'success', + smart_proxy_id: 1, + smart_proxy_name: 'proxy1', + }, + ], +}; + +let apiGetSpy; +let hostsCalls; +let pendingCallbacks; + +const createStore = () => + mockStore({ + API: {}, + }); + +const flushPendingCallbacks = () => { + const batch = [...pendingCallbacks]; + pendingCallbacks = []; + batch.forEach(cb => cb()); +}; + +const renderTable = (props = {}) => { + const store = createStore(); + const history = createMemoryHistory(); + const Wrapper = createForemanContextWrapper(); + + const defaultProps = { + id: '42', + targeting: { targeting_type: 'static_query', search_query: '' }, + initialFilter: 'all_statuses', + jobFinished: false, + onFilterUpdate: jest.fn(), + ...props, + }; + + const result = render( + + + + + + + + ); + + return { ...result, store, history }; +}; + +const setupSuccessMock = () => { + apiGetSpy = jest + .spyOn(APIActions, 'get') + .mockImplementation(opts => dispatch => { + if (opts.key === JOB_INVOCATION_HOSTS) { + hostsCalls.push(opts); + if (opts.handleSuccess) { + pendingCallbacks.push(() => + opts.handleSuccess({ data: hostsResponse }) + ); + } + } + }); +}; + +const setupErrorMock = () => { + apiGetSpy = jest + .spyOn(APIActions, 'get') + .mockImplementation(opts => dispatch => { + if (opts.key === JOB_INVOCATION_HOSTS) { + hostsCalls.push(opts); + if (opts.handleError) { + pendingCallbacks.push(() => opts.handleError()); + } + } + }); +}; + +describe('JobInvocationHostTable polling', () => { + beforeEach(() => { + hostsCalls = []; + pendingCallbacks = []; + setupSuccessMock(); + }); + + afterEach(() => { + apiGetSpy.mockRestore(); + jest.clearAllTimers(); + }); + + it('schedules a poll after the initial fetch succeeds', () => { + renderTable(); + + expect(hostsCalls.length).toBe(1); + + act(() => { + flushPendingCallbacks(); + }); + + expect(hostsCalls.length).toBe(1); + + act(() => { + jest.advanceTimersByTime(5000); + }); + + act(() => { + flushPendingCallbacks(); + }); + + expect(hostsCalls.length).toBe(2); + + act(() => { + jest.advanceTimersByTime(5000); + }); + + act(() => { + flushPendingCallbacks(); + }); + + expect(hostsCalls.length).toBe(3); + }); + + it('does not schedule a poll when jobFinished is true', () => { + renderTable({ jobFinished: true }); + + expect(hostsCalls.length).toBe(1); + + act(() => { + flushPendingCallbacks(); + }); + + act(() => { + jest.advanceTimersByTime(10000); + }); + + act(() => { + flushPendingCallbacks(); + }); + + expect(hostsCalls.length).toBe(1); + }); + + it('stops polling when jobFinished transitions to true', () => { + const { rerender } = renderTable(); + + act(() => { + flushPendingCallbacks(); + }); + + expect(hostsCalls.length).toBe(1); + + const store = createStore(); + const history = createMemoryHistory(); + const Wrapper = createForemanContextWrapper(); + + act(() => { + rerender( + + + + + + + + ); + }); + + act(() => { + flushPendingCallbacks(); + }); + + const callsAtTransition = hostsCalls.length; + + act(() => { + jest.advanceTimersByTime(10000); + }); + + act(() => { + flushPendingCallbacks(); + }); + + expect(hostsCalls.length).toBe(callsAtTransition); + }); + + it('cleans up the poll timer on unmount', () => { + const { unmount } = renderTable(); + + act(() => { + flushPendingCallbacks(); + }); + + const callsBeforeUnmount = hostsCalls.length; + + unmount(); + + act(() => { + jest.advanceTimersByTime(10000); + }); + + act(() => { + flushPendingCallbacks(); + }); + + expect(hostsCalls.length).toBe(callsBeforeUnmount); + }); + + it('restarts polling when filter changes', () => { + const { rerender } = renderTable({ initialFilter: 'all_statuses' }); + + act(() => { + flushPendingCallbacks(); + }); + + const callsBeforeFilterChange = hostsCalls.length; + + const store = createStore(); + const history = createMemoryHistory(); + const Wrapper = createForemanContextWrapper(); + + act(() => { + rerender( + + + + + + + + ); + }); + + expect(hostsCalls.length).toBeGreaterThan(callsBeforeFilterChange); + + act(() => { + flushPendingCallbacks(); + }); + + const callsAfterFilterChange = hostsCalls.length; + + act(() => { + jest.advanceTimersByTime(5000); + }); + + act(() => { + flushPendingCallbacks(); + }); + + expect(hostsCalls.length).toBeGreaterThan(callsAfterFilterChange); + }); + + it('stops polling on API error', () => { + apiGetSpy.mockRestore(); + setupErrorMock(); + + renderTable(); + + expect(hostsCalls.length).toBe(1); + + act(() => { + flushPendingCallbacks(); + }); + + act(() => { + jest.advanceTimersByTime(10000); + }); + + act(() => { + flushPendingCallbacks(); + }); + + expect(hostsCalls.length).toBe(1); + }); +}); diff --git a/webpack/JobInvocationDetail/index.js b/webpack/JobInvocationDetail/index.js index 085aee8f9..a4260516c 100644 --- a/webpack/JobInvocationDetail/index.js +++ b/webpack/JobInvocationDetail/index.js @@ -216,10 +216,8 @@ const JobInvocationDetailPage = ({ From a0b8f8e8661c5467d9b6b0aa8a030b9437adc079 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Tue, 21 Jul 2026 14:02:54 +0200 Subject: [PATCH 3/5] Refs #39253 - Replace interval polling with setTimeout chains Replace withInterval/setInterval polling with recursive setTimeout chains for job invocation and template invocation detail pages. This prevents request pileup by waiting for the previous response before scheduling the next poll. - Remove updateJob action (stored under unused UPDATE_JOB key) - Remove getTask action, expose cancellable via task node in RABL - Add extractErrorMessage helper consolidating duplicated error handling - Fix sprintf injection in error toast messages - Stop template invocation polling on finished/error states Co-Authored-By: Claude Opus 4.6 --- .../api/v2/job_invocations/main.json.rabl | 1 + .../JobInvocationActions.js | 161 +++++------ .../JobInvocationConstants.js | 2 - .../JobInvocationSelectors.js | 5 +- .../JobInvocationToolbarButtons.js | 10 +- .../JobInvocationDetail/TemplateInvocation.js | 53 ++-- .../__tests__/JobInvocationPolling.test.js | 173 ++++++++++++ .../__tests__/MainInformation.test.js | 39 +-- .../TemplateInvocationPolling.test.js | 254 ++++++++++++++++++ .../JobInvocationDetail/__tests__/fixtures.js | 2 + webpack/JobInvocationDetail/index.js | 26 +- 11 files changed, 545 insertions(+), 181 deletions(-) create mode 100644 webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js create mode 100644 webpack/JobInvocationDetail/__tests__/TemplateInvocationPolling.test.js diff --git a/app/views/api/v2/job_invocations/main.json.rabl b/app/views/api/v2/job_invocations/main.json.rabl index 3e4111a44..18cd9b31a 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.try(:cancellable?) } end if @template_invocations diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index e89bdeeb0..0524c1431 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -1,53 +1,56 @@ 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 { APIActions } from 'foremanReact/redux/API'; import { CANCEL_JOB, CANCEL_RECURRING_LOGIC, CHANGE_ENABLED_RECURRING_LOGIC, - GET_TASK, JOB_INVOCATION_KEY, - POLLING_INTERVAL_MS, - UPDATE_JOB, + STATUS, } from './JobInvocationConstants'; -export const getJobInvocation = url => dispatch => { - const fetchData = withInterval( - get({ - key: JOB_INVOCATION_KEY, - params: { include_permissions: true, include_hosts: false }, - url, - handleError: () => { - dispatch(stopInterval(JOB_INVOCATION_KEY)); - }, - 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', - }), - POLLING_INTERVAL_MS - ); +const POLL_INTERVAL = 5000; + +const isJobFinished = statusLabel => + statusLabel === STATUS.FAILED || + statusLabel === STATUS.SUCCEEDED || + statusLabel === STATUS.CANCELLED; + +const extractErrorMessage = response => + // eslint-disable-next-line camelcase + response?.data?.error?.full_messages?.[0] || + response?.data?.error?.message || + 'Unknown error.'; - dispatch(fetchData); +export const getJobInvocation = (url, pollTimeoutRef) => dispatch => { + const poll = () => { + dispatch( + APIActions.get({ + key: JOB_INVOCATION_KEY, + params: { include_permissions: true, include_hosts: false }, + url, + handleSuccess: ({ data }) => { + // eslint-disable-next-line camelcase + if (!isJobFinished(data?.status_label)) { + pollTimeoutRef.current = setTimeout(poll, POLL_INTERVAL); + } else { + pollTimeoutRef.current = null; + } + }, + handleError: () => { + pollTimeoutRef.current = null; + }, + errorToast: ({ response }) => extractErrorMessage(response), + }) + ); + }; + + poll(); }; -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 stopJobInvocationPolling = pollTimeoutRef => { + clearTimeout(pollTimeoutRef.current); + pollTimeoutRef.current = null; }; export const cancelJob = (jobId, force) => dispatch => { @@ -55,10 +58,6 @@ export const cancelJob = (jobId, force) => dispatch => { force ? sprintf(__('Trying to abort the job %s.'), jobId) : 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); const url = force ? `/job_invocations/${jobId}/cancel?force=true` : `/job_invocations/${jobId}/cancel`; @@ -68,12 +67,17 @@ export const cancelJob = (jobId, force) => dispatch => { url, key: CANCEL_JOB, errorToast: ({ response }) => - errorToast( - // eslint-disable-next-line camelcase - response?.data?.error?.full_messages || - response?.data?.error?.message || - 'Unknown error.' - ), + force + ? sprintf( + __('Could not abort the job %s: %s'), + jobId, + extractErrorMessage(response) + ) + : sprintf( + __('Could not cancel the job %s: %s'), + jobId, + extractErrorMessage(response) + ), handleSuccess: () => { dispatch( addToast({ @@ -82,40 +86,16 @@ export const cancelJob = (jobId, force) => dispatch => { message: infoToast(), }) ); - dispatch(updateJob(jobId)); }, }) ); }; -export const getTask = taskId => dispatch => { - dispatch( - get({ - key: GET_TASK, - url: `/foreman_tasks/api/tasks/${taskId}`, - }) - ); -}; - -export const enableRecurringLogic = ( - recurrenceId, - enabled, - jobId -) => dispatch => { +export const enableRecurringLogic = (recurrenceId, enabled) => dispatch => { const successToast = () => enabled ? sprintf(__('Recurring logic %s disabled successfully.'), recurrenceId) : sprintf(__('Recurring logic %s enabled successfully.'), recurrenceId); - const errorToast = response => - enabled - ? sprintf( - __(`Could not disable recurring logic %s: ${response}`), - recurrenceId - ) - : sprintf( - __(`Could not enable recurring logic %s: ${response}`), - recurrenceId - ); const url = `/foreman_tasks/api/recurring_logics/${recurrenceId}`; dispatch( APIActions.put({ @@ -124,25 +104,24 @@ export const enableRecurringLogic = ( 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.' - ), - handleSuccess: () => dispatch(updateJob(jobId)), + enabled + ? sprintf( + __('Could not disable recurring logic %s: %s'), + recurrenceId, + extractErrorMessage(response) + ) + : sprintf( + __('Could not enable recurring logic %s: %s'), + recurrenceId, + extractErrorMessage(response) + ), }) ); }; -export const cancelRecurringLogic = (recurrenceId, jobId) => dispatch => { +export const cancelRecurringLogic = recurrenceId => dispatch => { const successToast = () => sprintf(__('Recurring logic %s cancelled successfully.'), recurrenceId); - const errorToast = response => - sprintf( - __(`Could not cancel recurring logic %s: ${response}`), - recurrenceId - ); const url = `/foreman_tasks/recurring_logics/${recurrenceId}/cancel`; dispatch( APIActions.post({ @@ -150,13 +129,11 @@ export const cancelRecurringLogic = (recurrenceId, jobId) => dispatch => { key: CANCEL_RECURRING_LOGIC, successToast, errorToast: ({ response }) => - errorToast( - // eslint-disable-next-line camelcase - response?.data?.error?.full_messages || - response?.data?.error?.message || - 'Unknown error.' + sprintf( + __('Could not cancel recurring logic %s: %s'), + recurrenceId, + extractErrorMessage(response) ), - handleSuccess: () => dispatch(updateJob(jobId)), }) ); }; diff --git a/webpack/JobInvocationDetail/JobInvocationConstants.js b/webpack/JobInvocationDetail/JobInvocationConstants.js index b7271ac78..fd2395359 100644 --- a/webpack/JobInvocationDetail/JobInvocationConstants.js +++ b/webpack/JobInvocationDetail/JobInvocationConstants.js @@ -6,9 +6,7 @@ import { useForemanHostDetailsPageUrl } from 'foremanReact/Root/Context/ForemanC import JobStatusIcon from '../react_app/components/RecentJobsCard/JobStatusIcon'; export const JOB_INVOCATION_KEY = 'JOB_INVOCATION_KEY'; -export const UPDATE_JOB = 'UPDATE_JOB'; 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/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/JobInvocationToolbarButtons.js b/webpack/JobInvocationDetail/JobInvocationToolbarButtons.js index 7869d331c..5bb6d1deb 100644 --- a/webpack/JobInvocationDetail/JobInvocationToolbarButtons.js +++ b/webpack/JobInvocationDetail/JobInvocationToolbarButtons.js @@ -141,9 +141,7 @@ const JobInvocationToolbarButtons = ({ jobId, data }) => { - dispatch( - enableRecurringLogic(recurrence?.id, recurringEnabled, jobId) - ) + dispatch(enableRecurringLogic(recurrence?.id, recurringEnabled)) } key="change-enabled-recurring" component="button" @@ -159,9 +157,7 @@ const JobInvocationToolbarButtons = ({ jobId, data }) => { , - dispatch(cancelRecurringLogic(recurrence?.id, jobId)) - } + onClick={() => dispatch(cancelRecurringLogic(recurrence?.id))} key="cancel-recurring" component="button" isDisabled={ @@ -174,7 +170,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 faad034f8..cd348e381 100644 --- a/webpack/JobInvocationDetail/TemplateInvocation.js +++ b/webpack/JobInvocationDetail/TemplateInvocation.js @@ -74,7 +74,7 @@ export const TemplateInvocation = ({ showCommand, setShowCommand, }) => { - const intervalRef = useRef(null); + const timeoutRef = useRef(null); const templateURL = showTemplateInvocationUrl(hostID, jobID); const hostDetailsPageUrl = useForemanHostDetailsPageUrl(); @@ -88,43 +88,46 @@ export const TemplateInvocation = ({ }, [response]); useEffect(() => { - const dispatchFetch = () => { + let cancelled = false; + + const schedulePoll = () => { dispatch( APIActions.get({ url: templateURL, key: `${GET_TEMPLATE_INVOCATION}_${hostID}`, + handleSuccess: ({ data }) => { + if (cancelled) return; + const isFinished = data?.finished ?? true; + // eslint-disable-next-line camelcase + const autoRefresh = data?.auto_refresh || false; + if (!isFinished && autoRefresh) { + timeoutRef.current = setTimeout( + schedulePoll, + AUTO_REFRESH_INTERVAL_MS + ); + } else { + timeoutRef.current = null; + } + }, + handleError: () => { + timeoutRef.current = null; + }, }) ); }; - if (intervalRef.current) { - clearInterval(intervalRef.current); - intervalRef.current = null; - } + 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); - } - }, AUTO_REFRESH_INTERVAL_MS); + if (responseRef.current?.finished) return undefined; + schedulePoll(); } return () => { - if (intervalRef.current) { - clearInterval(intervalRef.current); - } + cancelled = true; + clearTimeout(timeoutRef.current); + timeoutRef.current = null; }; }, [isExpanded, dispatch, templateURL, hostID]); diff --git a/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js b/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js new file mode 100644 index 000000000..c3681d733 --- /dev/null +++ b/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js @@ -0,0 +1,173 @@ +import { createStore, applyMiddleware } from 'redux'; +import thunk from 'redux-thunk'; +import { APIActions } from 'foremanReact/redux/API'; +import { + getJobInvocation, + stopJobInvocationPolling, +} from '../JobInvocationActions'; +import { JOB_INVOCATION_KEY } from '../JobInvocationConstants'; + +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' }; +const failedData = { status_label: 'failed' }; +const cancelledData = { status_label: 'cancelled' }; + +let apiGetSpy; + +const setupGetMock = responseData => { + apiGetSpy = jest + .spyOn(APIActions, 'get') + .mockImplementation(({ handleSuccess, ...action }) => dispatch => { + handleSuccess && handleSuccess({ data: responseData }); + return dispatch({ type: 'MOCK_GET', ...action }); + }); +}; + +const url = '/api/job_invocations/1'; + +describe('job invocation polling', () => { + let ref; + + beforeEach(() => { + ref = makeRef(); + jest.clearAllTimers(); + }); + + afterEach(() => { + if (apiGetSpy) { + apiGetSpy.mockRestore(); + } + }); + + it('sends include_permissions and include_hosts on the initial fetch', () => { + setupGetMock(succeededData); + const store = makeStore(); + + store.dispatch(getJobInvocation(url, ref)); + + expect(apiGetSpy).toHaveBeenCalledTimes(1); + expect(apiGetSpy.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 = makeStore(); + + store.dispatch(getJobInvocation(url, ref)); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + jest.advanceTimersByTime(5000); + expect(apiGetSpy).toHaveBeenCalledTimes(2); + }); + + it('includes include_permissions=true in every poll call', () => { + setupGetMock(runningData); + const store = makeStore(); + + store.dispatch(getJobInvocation(url, ref)); + jest.advanceTimersByTime(5000); + + expect(apiGetSpy).toHaveBeenCalledTimes(2); + expect(apiGetSpy.mock.calls[1][0].params).toMatchObject({ + include_hosts: false, + include_permissions: true, + }); + }); + + it('stops polling when job succeeds', () => { + setupGetMock(succeededData); + const store = makeStore(); + + store.dispatch(getJobInvocation(url, ref)); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + jest.advanceTimersByTime(5000); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('stops polling when job fails', () => { + setupGetMock(failedData); + const store = makeStore(); + + store.dispatch(getJobInvocation(url, ref)); + jest.advanceTimersByTime(5000); + + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('stops polling when job is cancelled', () => { + setupGetMock(cancelledData); + const store = makeStore(); + + store.dispatch(getJobInvocation(url, ref)); + jest.advanceTimersByTime(5000); + + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('stops polling on fetch error', () => { + apiGetSpy = jest + .spyOn(APIActions, 'get') + .mockImplementation(({ handleError, ...action }) => dispatch => { + handleError && handleError(); + return dispatch({ type: 'MOCK_GET', ...action }); + }); + const store = makeStore(); + + store.dispatch(getJobInvocation(url, ref)); + jest.advanceTimersByTime(5000); + + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('stopJobInvocationPolling cancels a pending timeout', () => { + setupGetMock(runningData); + const store = makeStore(); + + store.dispatch(getJobInvocation(url, ref)); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + stopJobInvocationPolling(ref); + jest.advanceTimersByTime(5000); + + expect(apiGetSpy).toHaveBeenCalledTimes(1); + }); + + it('allows multiple overlapping polls when getJobInvocation is called multiple times', () => { + setupGetMock(runningData); + const store = makeStore(); + + store.dispatch(getJobInvocation(url, ref)); + expect(apiGetSpy).toHaveBeenCalledTimes(1); + + // Second call before the timeout fires creates a second poll chain + store.dispatch(getJobInvocation(url, ref)); + expect(apiGetSpy).toHaveBeenCalledTimes(2); + + jest.advanceTimersByTime(5000); + + // Both poll chains fire their timeouts + expect(apiGetSpy).toHaveBeenCalledTimes(4); + }); + + it('keeps polling as long as the job is running', () => { + setupGetMock(runningData); + const store = makeStore(); + + store.dispatch(getJobInvocation(url, ref)); + jest.advanceTimersByTime(15000); + + expect(apiGetSpy).toHaveBeenCalledTimes(4); + }); +}); + diff --git a/webpack/JobInvocationDetail/__tests__/MainInformation.test.js b/webpack/JobInvocationDetail/__tests__/MainInformation.test.js index 76ce24b39..1b293bdc1 100644 --- a/webpack/JobInvocationDetail/__tests__/MainInformation.test.js +++ b/webpack/JobInvocationDetail/__tests__/MainInformation.test.js @@ -30,7 +30,6 @@ import { CHANGE_ENABLED_RECURRING_LOGIC, GET_REPORT_TEMPLATES, GET_REPORT_TEMPLATE_INPUTS, - GET_TASK, JOB_INVOCATION_KEY, } from '../JobInvocationConstants'; @@ -47,7 +46,7 @@ jest.mock('../JobInvocationActions', () => { return { ...actual, getJobInvocation: jest.fn(() => () => undefined), - getTask: jest.fn(() => () => undefined), + stopJobInvocationPolling: jest.fn(), }; }); @@ -109,7 +108,6 @@ const createJobInvocationDetailState = ({ jobInvocation = jobInvocationData, jobInvocationStatus = STATUS.RESOLVED, jobInvocationError = null, - includeTaskState = true, } = {}) => { const jobInvocationResponse = jobInvocationError || JSON.parse(JSON.stringify(jobInvocation)); @@ -125,15 +123,6 @@ const createJobInvocationDetailState = ({ response: jobInvocationResponse, status: jobInvocationStatus, }, - ...(includeTaskState && { - [GET_TASK]: { - response: { - available_actions: { cancellable: true }, - ...(jobInvocationResponse.task || {}), - }, - status: STATUS.RESOLVED, - }, - }), }, }; }; @@ -168,11 +157,8 @@ describe('JobInvocationDetailPage', () => { setupApiMocks(); usePermissions.mockReturnValue(true); - const { getJobInvocation, getTask } = jest.requireMock( - '../JobInvocationActions' - ); + const { getJobInvocation } = jest.requireMock('../JobInvocationActions'); getJobInvocation.mockImplementation(() => () => undefined); - getTask.mockImplementation(() => () => undefined); }); afterEach(() => { @@ -189,7 +175,6 @@ describe('JobInvocationDetailPage', () => { renderJobInvocationDetailPage( createJobInvocationDetailState({ jobInvocation: scheduledJobInvocation, - includeTaskState: false, }), { jobId: jobInvocationDataScheduled.id } ); @@ -324,15 +309,7 @@ describe('JobInvocationDetailPage', () => { }); it('should dispatch global actions', async () => { - const actualActions = jest.requireActual('../JobInvocationActions'); - const { getTask } = jest.requireMock('../JobInvocationActions'); - - getTask.mockImplementation(taskId => dispatch => - actualActions.getTask(taskId)(dispatch) - ); - const jobId = jobInvocationDataRecurring.id; - const taskId = jobInvocationDataRecurring.task.id; const recurrenceId = jobInvocationDataRecurring.recurrence.id; const { store } = renderJobInvocationDetailPage( @@ -348,12 +325,6 @@ describe('JobInvocationDetailPage', () => { url: '/api/report_templates', }) ); - expect(api.get).toHaveBeenCalledWith( - expect.objectContaining({ - key: GET_TASK, - url: `/foreman_tasks/api/tasks/${taskId}`, - }) - ); expect(api.get).toHaveBeenCalledWith( expect.objectContaining({ key: GET_REPORT_TEMPLATE_INPUTS, @@ -367,9 +338,9 @@ describe('JobInvocationDetailPage', () => { store.dispatch(cancelJob(jobId, false)); store.dispatch(cancelJob(jobId, true)); - store.dispatch(enableRecurringLogic(recurrenceId, true, jobId)); - store.dispatch(enableRecurringLogic(recurrenceId, false, jobId)); - store.dispatch(cancelRecurringLogic(recurrenceId, jobId)); + store.dispatch(enableRecurringLogic(recurrenceId, true)); + store.dispatch(enableRecurringLogic(recurrenceId, false)); + store.dispatch(cancelRecurringLogic(recurrenceId)); expect(APIActions.post).toHaveBeenNthCalledWith( 1, 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); + }); +}); diff --git a/webpack/JobInvocationDetail/__tests__/fixtures.js b/webpack/JobInvocationDetail/__tests__/fixtures.js index 9ad17a956..30523b33e 100644 --- a/webpack/JobInvocationDetail/__tests__/fixtures.js +++ b/webpack/JobInvocationDetail/__tests__/fixtures.js @@ -42,6 +42,7 @@ export const jobInvocationData = { id: '37ad5ead-51de-4798-bc73-a17687c4d5aa', state: 'stopped', started_at: '2024-01-01 12:34:56 +0100', + cancellable: true, }, template_invocations: [ { @@ -113,6 +114,7 @@ export const jobInvocationDataRecurring = { task: { id: '37ad5ead-51de-4798-bc73-a17687c4d5aa', state: 'scheduled', + cancellable: true, }, mode: 'recurring', recurrence: { diff --git a/webpack/JobInvocationDetail/index.js b/webpack/JobInvocationDetail/index.js index a4260516c..c96c424b7 100644 --- a/webpack/JobInvocationDetail/index.js +++ b/webpack/JobInvocationDetail/index.js @@ -5,13 +5,12 @@ 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'; 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,10 @@ import JobInvocationOverview from './JobInvocationOverview'; import JobInvocationSystemStatusChart from './JobInvocationSystemStatusChart'; import JobInvocationEmptyState from './JobInvocationEmptyState'; import JobInvocationToolbarButtons from './JobInvocationToolbarButtons'; -import { getJobInvocation, getTask } from './JobInvocationActions'; +import { + getJobInvocation, + stopJobInvocationPolling, +} from './JobInvocationActions'; import './JobInvocationDetail.scss'; import { DATE_OPTIONS, @@ -54,7 +56,7 @@ const JobInvocationDetailPage = ({ statusLabel === STATUS.FAILED || statusLabel === STATUS.SUCCEEDED || statusLabel === STATUS.CANCELLED; - const autoRefresh = task?.state === STATUS.PENDING || false; + const pollTimeoutRef = useRef(null); const jobInvocationApiStatus = useSelector(state => selectAPIStatus(state, JOB_INVOCATION_KEY) ); @@ -82,21 +84,11 @@ const JobInvocationDetailPage = ({ } useEffect(() => { - dispatch(getJobInvocation(`/api/job_invocations/${id}`)); - if (finished && !autoRefresh) { - dispatch(stopInterval(JOB_INVOCATION_KEY)); - } + dispatch(getJobInvocation(`/api/job_invocations/${id}`, pollTimeoutRef)); return () => { - dispatch(stopInterval(JOB_INVOCATION_KEY)); + stopJobInvocationPolling(pollTimeoutRef); }; - }, [dispatch, id, finished, autoRefresh]); - - const taskId = task?.id; - useEffect(() => { - if (taskId !== undefined) { - dispatch(getTask(`${taskId}`)); - } - }, [dispatch, taskId]); + }, [dispatch, id]); const apiFailed = jobInvocationApiStatus === API_STATUS.ERROR; From f2e46b0d16b671b83f8e5d67a1e89787b39e9b5f Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Wed, 29 Jul 2026 11:49:30 +0200 Subject: [PATCH 4/5] squash: Refs #39253 - Cancel pending timeout before starting new poll Adopt the cancel-first pattern from JobInvocationHostTable to prevent overlapping polls when getJobInvocation is called multiple times. This ensures that any pending timeout is cancelled before starting a new polling cycle, preventing race conditions and duplicate API calls. --- webpack/JobInvocationDetail/JobInvocationActions.js | 1 + .../__tests__/JobInvocationPolling.test.js | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/webpack/JobInvocationDetail/JobInvocationActions.js b/webpack/JobInvocationDetail/JobInvocationActions.js index 0524c1431..aea5d04bc 100644 --- a/webpack/JobInvocationDetail/JobInvocationActions.js +++ b/webpack/JobInvocationDetail/JobInvocationActions.js @@ -45,6 +45,7 @@ export const getJobInvocation = (url, pollTimeoutRef) => dispatch => { ); }; + stopJobInvocationPolling(pollTimeoutRef); poll(); }; diff --git a/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js b/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js index c3681d733..7188df4e5 100644 --- a/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js +++ b/webpack/JobInvocationDetail/__tests__/JobInvocationPolling.test.js @@ -143,21 +143,21 @@ describe('job invocation polling', () => { expect(apiGetSpy).toHaveBeenCalledTimes(1); }); - it('allows multiple overlapping polls when getJobInvocation is called multiple times', () => { + it('cancels pending timeout before starting a new poll when called multiple times', () => { setupGetMock(runningData); const store = makeStore(); store.dispatch(getJobInvocation(url, ref)); expect(apiGetSpy).toHaveBeenCalledTimes(1); - // Second call before the timeout fires creates a second poll chain + // Second call before the timeout fires cancels the first timeout and starts fresh store.dispatch(getJobInvocation(url, ref)); expect(apiGetSpy).toHaveBeenCalledTimes(2); jest.advanceTimersByTime(5000); - // Both poll chains fire their timeouts - expect(apiGetSpy).toHaveBeenCalledTimes(4); + // Only the second poll chain fires (first was cancelled) + expect(apiGetSpy).toHaveBeenCalledTimes(3); }); it('keeps polling as long as the job is running', () => { From 3efe3393f2645b01919c08e6a2bb5c88a4f8b8e6 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Tue, 21 Jul 2026 14:31:58 +0200 Subject: [PATCH 5/5] Refs #39253 - Consolidate host data into the API endpoint Merge per-host task and permissions data into the paginated /api/v2/job_invocations/:id/hosts endpoint, eliminating the separate list_jobs_hosts action that fetched ALL hosts unpaginated. The API endpoint now accepts include_permissions parameter to optionally return task cancellable state and per-host permissions, replacing the data previously served by the non-API endpoint. Co-Authored-By: Claude Opus 4.6 --- .../api/v2/job_invocations_controller.rb | 26 ++++++++- app/controllers/job_invocations_controller.rb | 29 +--------- .../api/v2/job_invocations/hosts.json.rabl | 8 +++ config/routes.rb | 1 - lib/foreman_remote_execution/plugin.rb | 4 +- .../api/v2/job_invocations_controller_test.rb | 53 ++++++++++++++++--- .../JobInvocationConstants.js | 2 - .../JobInvocationHostTable.js | 36 ++++++++----- .../JobInvocationSelectors.js | 5 -- .../TemplateActionButtons.js | 23 +++++--- 10 files changed, 122 insertions(+), 65 deletions(-) diff --git a/app/controllers/api/v2/job_invocations_controller.rb b/app/controllers/api/v2/job_invocations_controller.rb index 0bf640111..4965507e4 100644 --- a/app/controllers/api/v2/job_invocations_controller.rb +++ b/app/controllers/api/v2/job_invocations_controller.rb @@ -117,12 +117,14 @@ def output param_group :search_and_pagination, ::Api::V2::BaseController add_scoped_search_description_for(JobInvocation) param :id, :identifier, :required => true + param :include_permissions, :bool, :required => false, :desc => N_('Include per-host task and permission data in the response') def hosts 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]) @subtotal = @hosts.total_entries set_statuses_and_smart_proxies + set_tasks_and_permissions if Foreman::Cast.to_bool(params[:include_permissions]) if params[:awaiting] @hosts = @hosts.select { |host| @host_statuses[host.id] == 'N/A' } end @@ -321,15 +323,35 @@ def set_statuses_and_smart_proxies template_invocations = @template_invocations.where(host_id: @hosts.select(:id)) .includes(:run_host_job_task).to_a hosts = @hosts.to_a - template_invocations_by_host_id = template_invocations.index_by(&:host_id) + @template_invocations_by_host_id = template_invocations.index_by(&:host_id) @host_statuses = hosts.to_h do |host| - template_invocation = template_invocations_by_host_id[host.id] + template_invocation = @template_invocations_by_host_id[host.id] task = template_invocation.try(:run_host_job_task) [host.id, template_invocation_status(task, @job_invocation.task)] 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] } end + + def set_tasks_and_permissions + can_view_tasks = User.current.can?(:view_foreman_tasks) + can_cancel = authorized_for(:permission => :cancel_job_invocations, :auth_object => @job_invocation) + can_create = authorized_for(controller: :job_invocations, action: :create) + can_execute_on_infra = User.current.can?(:execute_jobs_on_infrastructure_hosts) + + hosts = @hosts.to_a + @task_by_host = hosts.to_h do |host| + task = @template_invocations_by_host_id[host.id].try(:run_host_job_task) + [host.id, task ? { :id => task.id, :cancellable => task.try(:cancellable?) || false } : nil] + end + @permissions_by_host = hosts.to_h do |host| + [host.id, { + :view_foreman_tasks => can_view_tasks, + :cancel_job_invocations => can_cancel, + :execute_jobs => can_create && (!host.infrastructure_host? || can_execute_on_infra), + }] + end + end end end end 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/app/views/api/v2/job_invocations/hosts.json.rabl b/app/views/api/v2/job_invocations/hosts.json.rabl index 7e627dfef..bcd6454f0 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, :if => ->(_host) { @task_by_host }) do |host| + @task_by_host[host.id] +end + +node(:permissions, :if => ->(_host) { @permissions_by_host }) do |host| + @permissions_by_host[host.id] +end 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 6dded4bb8..a53190859 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' diff --git a/test/functional/api/v2/job_invocations_controller_test.rb b/test/functional/api/v2/job_invocations_controller_test.rb index 2b9d84e2b..e5d7702d5 100644 --- a/test/functional/api/v2/job_invocations_controller_test.rb +++ b/test/functional/api/v2/job_invocations_controller_test.rb @@ -284,20 +284,59 @@ class JobInvocationsControllerTest < ActionController::TestCase end describe '#hosts' do - test 'should compute job_status for the paginated subset' do - 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) } - invocation.job_category = invocation.pattern_template_invocations.first.template.job_category - invocation.targeting.hosts = invocation.template_invocations.map(&:host) - invocation.save! + 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 - get :hosts, params: { :id => invocation.id, :page => 2, :per_page => 1 } + 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 and permissions without include_permissions' do + get :hosts, params: { :id => @hosts_invocation.id } + assert_response :success + result = ActiveSupport::JSON.decode(@response.body) + result['results'].each do |host| + assert_not host.key?('task'), 'Expected task to be absent' + assert_not host.key?('permissions'), 'Expected permissions to be absent' + end + end + + test 'should include task and permissions with include_permissions=true' do + get :hosts, params: { :id => @hosts_invocation.id, :include_permissions => true } + assert_response :success + result = ActiveSupport::JSON.decode(@response.body) + result['results'].each do |host| + assert host.key?('task'), 'Expected task to be present' + assert host.key?('permissions'), 'Expected permissions to be present' + permissions = host['permissions'] + assert permissions.key?('view_foreman_tasks') + assert permissions.key?('cancel_job_invocations') + assert permissions.key?('execute_jobs') + end + end + + test 'should handle hosts with nil task gracefully when include_permissions=true' do + invocation = FactoryBot.create(:job_invocation, :with_template, :with_task, :with_unplanned_host) + invocation.job_category = invocation.pattern_template_invocations.first.template.job_category + invocation.save! + + get :hosts, params: { :id => invocation.id, :include_permissions => true } + assert_response :success + result = ActiveSupport::JSON.decode(@response.body) + result['results'].each do |host| + assert host.key?('permissions'), 'Expected permissions to be present' + end + end end describe 'raw output' do diff --git a/webpack/JobInvocationDetail/JobInvocationConstants.js b/webpack/JobInvocationDetail/JobInvocationConstants.js index fd2395359..efc5de004 100644 --- a/webpack/JobInvocationDetail/JobInvocationConstants.js +++ b/webpack/JobInvocationDetail/JobInvocationConstants.js @@ -15,7 +15,6 @@ 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 POLLING_INTERVAL_MS = 1000; @@ -27,7 +26,6 @@ export const CLIPBOARD_DEFAULT_EXIT_DELAY_MS = 600; 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 17aadfef5..ef8e0ea4e 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, - LIST_TEMPLATE_INVOCATIONS, STATUS_UPPERCASE, AWAITING_STATUS_FILTER, AUTO_REFRESH_INTERVAL_MS, @@ -162,11 +161,24 @@ const JobInvocationHostTable = ({ [initialFilter, urlSearchQuery] ); + const [hostPermissions, setHostPermissions] = useState({}); + const updateHostsState = useCallback(data => { const ids = data.data.results.map(i => i.id); setApiResponse(data.data); setAllHostsIds(ids); setStatus(STATUS_UPPERCASE.RESOLVED); + + const resultsWithPermissions = data.data.results.filter(r => r.permissions); + if (resultsWithPermissions.length > 0) { + setHostPermissions(prev => { + const updated = { ...prev }; + resultsWithPermissions.forEach(r => { + updated[r.id] = { task: r.task, permissions: r.permissions }; + }); + return updated; + }); + } }, []); // Call hosts data with params @@ -223,7 +235,10 @@ const JobInvocationHostTable = ({ finalParams.search = filterSearch; } - currentPollParams.current = finalParams; + finalParams.include_permissions = true; + + currentPollParams.current = { ...finalParams }; + delete currentPollParams.current.include_permissions; clearTimeout(pollTimeoutId.current); pollTimeoutId.current = null; @@ -252,20 +267,12 @@ const JobInvocationHostTable = ({ const initializedRef = useRef(false); useEffect(() => { if (!initializedRef.current) { - dispatch( - APIActions.get({ - key: LIST_TEMPLATE_INVOCATIONS, - url: `/job_invocations/${id}/hosts`, - params: {}, - }) - ); - if (initialFilter === '') { onFilterUpdate('all_statuses'); } initializedRef.current = true; } - }, [dispatch, id, initialFilter, onFilterUpdate]); + }, [initialFilter, onFilterUpdate]); useEffect(() => { const filterChanged = initialFilter !== prevFilter.current; @@ -500,7 +507,12 @@ const JobInvocationHostTable = ({ {columns[k].wrapper(result)} ))} - + @@ -20,7 +19,3 @@ export const selectTemplateInvocation = hostID => state => export const selectTemplateInvocationStatus = hostID => state => selectAPIStatus(state, `${GET_TEMPLATE_INVOCATION}_${hostID}`); - -export const selectTemplateInvocationList = state => - selectAPIResponse(state, LIST_TEMPLATE_INVOCATIONS) - ?.template_invocations_task_by_hosts; diff --git a/webpack/JobInvocationDetail/TemplateInvocationComponents/TemplateActionButtons.js b/webpack/JobInvocationDetail/TemplateInvocationComponents/TemplateActionButtons.js index f9242f646..0eade9c3d 100644 --- a/webpack/JobInvocationDetail/TemplateInvocationComponents/TemplateActionButtons.js +++ b/webpack/JobInvocationDetail/TemplateInvocationComponents/TemplateActionButtons.js @@ -1,12 +1,11 @@ import React from 'react'; import PropTypes from 'prop-types'; -import { useDispatch, useSelector } from 'react-redux'; +import { useDispatch } from 'react-redux'; import { Flex, FlexItem, Button } from '@patternfly/react-core'; 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 './index.scss'; const actions = ({ @@ -81,11 +80,9 @@ const actions = ({ }, }); -export const RowActions = ({ hostID, jobID }) => { +export const RowActions = ({ hostID, jobID, task, permissions }) => { const dispatch = useDispatch(); - const response = useSelector(selectTemplateInvocationList)?.[hostID]; - if (!response?.permissions) return null; - const { task, permissions } = response; + if (!permissions) return null; const { id: taskID, cancellable: taskCancellable } = task || {}; const getActions = actions({ taskID, @@ -216,4 +213,18 @@ TemplateActionButtons.defaultProps = { RowActions.propTypes = { hostID: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired, jobID: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired, + task: PropTypes.shape({ + id: PropTypes.oneOfType([PropTypes.string, PropTypes.number]), + cancellable: PropTypes.bool, + }), + permissions: PropTypes.shape({ + view_foreman_tasks: PropTypes.bool, + cancel_job_invocations: PropTypes.bool, + execute_jobs: PropTypes.bool, + }), +}; + +RowActions.defaultProps = { + task: null, + permissions: null, };