Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions webpack/JobWizard/JobWizard.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,12 @@ export const JobWizard = ({ rerunData }) => {
hostGroups: [],
});
const [hostsSearchQuery, setHostsSearchQuery] = useState('');
const [selectedBookmark, setSelectedBookmark] = useState(null);
const [fills, setFills] = useState(
rerunData
? {
search: rerunData?.targeting?.search_query,
bookmark_id: rerunData?.targeting?.bookmark_id,
...rerunData.inputs,
...routerSearch,
}
Expand Down Expand Up @@ -251,6 +253,7 @@ export const JobWizard = ({ rerunData }) => {
setFills,
setSelectedTargets,
setHostsSearchQuery,
setSelectedBookmark,
setJobTemplateID,
setTemplateValues,
setAdvancedValues,
Expand Down Expand Up @@ -298,6 +301,8 @@ export const JobWizard = ({ rerunData }) => {
setSelected={setSelectedTargets}
hostsSearchQuery={hostsSearchQuery}
setHostsSearchQuery={setHostsSearchQuery}
selectedBookmark={selectedBookmark}
setSelectedBookmark={setSelectedBookmark}
/>
),
canJumpTo: isTemplate,
Expand Down Expand Up @@ -474,6 +479,7 @@ export const JobWizard = ({ rerunData }) => {
dispatch,
selectedTargets,
hostsSearchQuery,
selectedBookmark,
location,
organization,
feature,
Expand Down Expand Up @@ -507,6 +513,7 @@ JobWizard.propTypes = {
job_category: PropTypes.string,
targeting: PropTypes.shape({
search_query: PropTypes.string,
bookmark_id: PropTypes.number,
targeting_type: PropTypes.string,
randomized_ordering: PropTypes.bool,
}),
Expand Down
5 changes: 5 additions & 0 deletions webpack/JobWizard/JobWizardConstants.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { translate as __ } from 'foremanReact/common/I18n';
import { foremanUrl } from 'foremanReact/common/helpers';
import { getControllerSearchProps } from 'foremanReact/constants';

export const JOB_TEMPLATES = 'JOB_TEMPLATES';
export const JOB_CATEGORIES = 'JOB_CATEGORIES';
Expand Down Expand Up @@ -61,6 +62,10 @@ export const hostMethods = {

export const hostQuerySearchID = 'mainHostQuery';
export const hostsController = 'hosts';
export const hostsSearchProps = getControllerSearchProps(
hostsController,
hostQuerySearchID
);

export const dataName = {
[HOSTS]: 'hosts',
Expand Down
8 changes: 8 additions & 0 deletions webpack/JobWizard/JobWizardSelectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import {
} from 'foremanReact/redux/API/APISelectors';
import { STATUS } from 'foremanReact/constants';
import { selectRouterLocation } from 'foremanReact/routes/RouterSelector';
import { BOOKMARKS } from 'foremanReact/components/PF4/Bookmarks/BookmarksConstants';
import { selectBookmarksResults } from 'foremanReact/components/PF4/Bookmarks/BookmarksSelectors';

import {
JOB_TEMPLATES,
Expand Down Expand Up @@ -134,3 +136,9 @@ export const selectRouterSearch = state => {
const { search } = selectRouterLocation(state) || {};
return URI.parseQuery(search);
};

const HOSTS_CONTROLLER = 'hosts';
const BOOKMARKS_HOSTS_KEY = `${BOOKMARKS}_${HOSTS_CONTROLLER.toUpperCase()}`;

export const selectHostBookmarks = state =>
selectBookmarksResults(state, BOOKMARKS_HOSTS_KEY, HOSTS_CONTROLLER);
62 changes: 53 additions & 9 deletions webpack/JobWizard/__tests__/JobWizardPageRerun.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,19 @@ import { Provider } from 'react-redux';
import { render, fireEvent, screen, act } from '@testing-library/react';
import { MockedProvider } from '@apollo/client/testing';

import * as APIHooks from 'foremanReact/common/hooks/API/APIHooks';
import * as api from 'foremanReact/redux/API';
import JobWizardPageRerun from '../JobWizardPageRerun';
import * as selectors from '../JobWizardSelectors';
import { testSetup, mockApi, gqlMock, jobInvocation } from './fixtures';
import {
testSetup,
mockApi,
gqlMock,
jobInvocation,
bookmarksList,
} from './fixtures';

const store = testSetup(selectors, api);
mockApi(api);
jest.spyOn(APIHooks, 'useAPI');
APIHooks.useAPI.mockImplementation((action, url) => {
if (url === '/ui_job_wizard/job_invocation?id=57') {
return { response: jobInvocation, status: 'RESOLVED' };
}
return {};
});

describe('Job wizard fill', () => {
it('fill defaults into fields', async () => {
Expand Down Expand Up @@ -76,4 +74,50 @@ describe('Job wizard fill', () => {
}).value
).toBe('6');
});

it('fills bookmark on rerun when job used a bookmark', async () => {
const bookmark = bookmarksList[0];
const jobWithBookmark = {
...jobInvocation,
job: {
...jobInvocation.job,
targeting: {
...jobInvocation.job.targeting,
bookmark_id: bookmark.id,
search_query: null,
},
},
};

selectors.selectRerunJobInvocationResponse.mockImplementation(
() => jobWithBookmark
);

render(
<MockedProvider mocks={gqlMock} addTypename={false}>
<Provider store={store}>
<JobWizardPageRerun
match={{
params: { id: '99' },
}}
/>
</Provider>
</MockedProvider>
);

await act(async () => {
fireEvent.click(screen.getByText('Target hosts and inputs'));
});

const hostMethodSelect = screen.getByRole('button', {
name: 'host method',
});
expect(hostMethodSelect.textContent).toContain('Search query');

expect(screen.queryAllByText(bookmark.query)).toHaveLength(1);

selectors.selectRerunJobInvocationResponse.mockImplementation(
() => jobInvocation
);
});
});
24 changes: 24 additions & 0 deletions webpack/JobWizard/__tests__/fixtures.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,22 @@ export const jobTemplateResponse = {
],
};

export const bookmarksList = [
{ id: 19, name: 'my hosts', query: 'name ~ myhost', controller: 'hosts' },
{
id: 23,
name: 'active hosts',
query: 'last_report > "1 hour ago"',
controller: 'hosts',
},
{
id: 31,
name: 'dashboard default',
query: 'os = centos',
controller: 'dashboard',
},
];

export const jobCategories = ['Services', 'Ansible Commands', 'Puppet'];

export const testSetup = (selectors, api) => {
Expand Down Expand Up @@ -176,6 +192,14 @@ export const testSetup = (selectors, api) => {
subtotal: 3,
},
},
bookmarksPF4: {
hosts: {
results: bookmarksList,
},
},
API: {
BOOKMARKS_HOSTS: { status: 'RESOLVED', results: [] },
},
});
return store;
};
Expand Down
61 changes: 57 additions & 4 deletions webpack/JobWizard/autofill.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,43 @@
import { useEffect } from 'react';
import { useDispatch } from 'react-redux';
import { useEffect, useState } from 'react';
import { useDispatch, useSelector } from 'react-redux';
import { get } from 'foremanReact/redux/API';
import { HOST_IDS, REX_FEATURE } from './JobWizardConstants';
import { getBookmarks } from 'foremanReact/components/PF4/Bookmarks/BookmarksActions';
import {
HOST_IDS,
REX_FEATURE,
hostsController,
hostsSearchProps,
} from './JobWizardConstants';
import { selectHostBookmarks } from './JobWizardSelectors';
import './JobWizard.scss';

export const useAutoFill = ({
fills,
setFills,
setSelectedTargets,
setHostsSearchQuery,
setSelectedBookmark,
setJobTemplateID,
setTemplateValues,
setAdvancedValues,
}) => {
const dispatch = useDispatch();
const bookmarks = useSelector(selectHostBookmarks);
const [pendingBookmarkId, setPendingBookmarkId] = useState(null);

useEffect(() => {
if (pendingBookmarkId === null || bookmarks.length === 0) return;
const bookmark = bookmarks.find(bm => bm.id === pendingBookmarkId);
if (bookmark) {
setSelectedBookmark({
id: bookmark.id,
name: bookmark.name,
query: bookmark.query,
});
setHostsSearchQuery(bookmark.query);
}
setPendingBookmarkId(null);
}, [bookmarks, pendingBookmarkId, setSelectedBookmark, setHostsSearchQuery]);

useEffect(() => {
if (Object.keys(fills).length) {
Expand All @@ -22,10 +46,12 @@ export const useAutoFill = ({
search,
feature,
template_id: templateID,
bookmark_id: bookmarkId,
...rest
} = { ...fills };
setFills({});
if (hostIds) {
setSelectedBookmark(null);
const hostSearch = Array.isArray(hostIds)
? `id = ${hostIds.join(' or id = ')}`
: `id = ${hostIds}`;
Expand All @@ -52,9 +78,34 @@ export const useAutoFill = ({
})
);
}
if ((search || search === '') && !hostIds?.length) {
if (bookmarkId) {
setSelectedTargets({
hosts: [],
hostCollections: [],
hostGroups: [],
});
const numericId = Number(bookmarkId);
if (bookmarks.length > 0) {
const bookmark = bookmarks.find(bm => bm.id === numericId);
if (bookmark) {
setSelectedBookmark({
id: bookmark.id,
name: bookmark.name,
query: bookmark.query,
});
setHostsSearchQuery(bookmark.query);
}
} else {
setPendingBookmarkId(numericId);
dispatch(
getBookmarks(hostsSearchProps.bookmarks.url, hostsController)
);
}
Comment on lines +88 to +103

@coderabbitai coderabbitai Bot May 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fetch bookmarks when the requested bookmark ID is missing from cached results.

The current logic only dispatches getBookmarks when the cache is empty. If cache is non-empty but doesn’t contain the rerun bookmark_id, autofill never resolves the bookmark.

Suggested fix
       if (bookmarkId) {
         setSelectedTargets({
           hosts: [],
           hostCollections: [],
           hostGroups: [],
         });
         const numericId = Number(bookmarkId);
-        if (bookmarks.length > 0) {
-          const bookmark = bookmarks.find(bm => bm.id === numericId);
-          if (bookmark) {
-            setSelectedBookmark({
-              id: bookmark.id,
-              name: bookmark.name,
-              query: bookmark.query,
-            });
-            setHostsSearchQuery(bookmark.query);
-          }
-        } else {
+        const bookmark = bookmarks.find(bm => bm.id === numericId);
+        if (bookmark) {
+          setSelectedBookmark({
+            id: bookmark.id,
+            name: bookmark.name,
+            query: bookmark.query,
+          });
+          setHostsSearchQuery(bookmark.query);
+        } else {
           setPendingBookmarkId(numericId);
           dispatch(
             getBookmarks(hostsSearchProps.bookmarks.url, hostsController)
           );
         }
       } else if ((search || search === '') && !hostIds?.length) {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@webpack/JobWizard/autofill.js` around lines 88 - 103, The current branch only
dispatches getBookmarks when bookmarks.length === 0, so if bookmarks is
non-empty but missing the requested numericId the autofill never resolves;
update the logic in autofill.js around the bookmarks lookup (where bookmarks,
numericId, setSelectedBookmark, setHostsSearchQuery are used) to detect when no
bookmark is found and in that case call setPendingBookmarkId(numericId) and
dispatch(getBookmarks(hostsSearchProps.bookmarks.url, hostsController)) so the
missing bookmark is fetched and the pending id can be resolved when results
arrive.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think this behaviour makes sense

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@MariaAga, understood — if bookmarks are already loaded in the store, a missing ID would indicate the bookmark no longer exists rather than being a cache miss, so skipping the fetch in that case is a reasonable assumption. I'll defer to your judgment here.

} else if ((search || search === '') && !hostIds?.length) {
// replace an empty string search with a dummy search query to match all hosts
// but only if search query was entered (based on presence of :search parameter)

setSelectedBookmark(null);
const hostSearch = search === '' ? "name != ''" : search;
setHostsSearchQuery(hostSearch);
}
Expand Down Expand Up @@ -100,9 +151,11 @@ export const useAutoFill = ({
setFills,
setSelectedTargets,
setHostsSearchQuery,
setSelectedBookmark,
setJobTemplateID,
setTemplateValues,
setAdvancedValues,
dispatch,
bookmarks,
]);
};
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ describe('AdvancedFields', () => {
const resourceSelectField = screen.getByLabelText(
'adv resource select toggle'
);
const searchField = screen.getByPlaceholderText('Filter...');
const searchField = screen.getByPlaceholderText('Search');
const dateField = screen.getByLabelText('adv date datepicker');
const timeField = screen.getByLabelText('adv date timepicker');

Expand Down Expand Up @@ -403,6 +403,11 @@ describe('AdvancedFields', () => {

jest.advanceTimersByTime(10000);
});
expect(newStore.getActions()).toMatchSnapshot('resource search');
const actions = newStore.getActions();
const resourceSearchAction = actions.filter(
action => action.key === 'ForemanTasksTask'
);
expect(resourceSearchAction).toHaveLength(2);
expect(String(resourceSearchAction[1].url)).toContain('name=some+search');
});
});
Loading
Loading