Fixes #39369 - Job Wizard should send bookmark_id#1037
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request introduces bookmark support to JobWizard for host targeting. The changes establish Redux selectors and constants for host bookmarks, extend the autofill hook to detect and apply bookmarks from rerun data, refactor HostSearch to match search input against bookmark queries, wire bookmark selection through the component hierarchy, and update the submit action to include bookmark IDs in the payload. Test fixtures, fixtures setup, and test cases validate the bookmark flow across rerun and submission scenarios. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webpack/JobWizard/steps/HostsAndInputs/index.js (1)
168-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear bookmark selection when leaving search-query targeting mode.
A previously matched bookmark can remain selected after method switch, and then submission still sends
bookmark_id, overriding current non-search targeting.Suggested fix
setValue={val => { setHostMethod(val); + if (val !== hostMethods.searchQuery) { + setSelectedBookmark(null); + } if (val === hostMethods.searchQuery) { setErrorText(__('Please enter a search query')); }🤖 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/steps/HostsAndInputs/index.js` around lines 168 - 193, When switching host targeting in the setValue handler for setHostMethod, clear any previously matched bookmark so it doesn't persist when leaving search-query mode; update the handler where setHostMethod(val) is called (the block that checks val === hostMethods.searchQuery / hostMethods.hosts / hostMethods.hostCollections / hostMethods.hostGroups) to call setSelectedBookmark(null) (or an empty string per app convention) whenever val !== hostMethods.searchQuery so bookmark_id is not sent for non-search targeting; keep the existing errorText logic unchanged.
🧹 Nitpick comments (1)
webpack/JobWizard/steps/AdvancedFields/__tests__/AdvancedFields.test.js (1)
406-411: ⚡ Quick winAvoid order-coupled assertion on the second dispatched action.
Using
resourceSearchAction[1]makes this test fragile if dispatch order changes. Assert that at least oneForemanTasksTaskaction includes the encoded query instead.Suggested test hardening
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'); +expect( + resourceSearchAction.some(action => + String(action.url).includes('name=some+search') + ) +).toBe(true);🤖 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/steps/AdvancedFields/__tests__/AdvancedFields.test.js` around lines 406 - 411, The test is fragile because it indexes the second dispatched ForemanTasksTask action (resourceSearchAction[1]); instead, filter newStore.getActions() for actions with key === 'ForemanTasksTask' (as done into resourceSearchAction) and assert that at least one of those actions' url contains the encoded query (e.g., check that some(action => String(action.url).includes('name=some+search'))). Keep the existing expect for total count if desired but remove the order-dependent indexing and use Array.prototype.some on resourceSearchAction to verify the encoded query appears in any matching action.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@webpack/JobWizard/autofill.js`:
- Around line 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.
---
Outside diff comments:
In `@webpack/JobWizard/steps/HostsAndInputs/index.js`:
- Around line 168-193: When switching host targeting in the setValue handler for
setHostMethod, clear any previously matched bookmark so it doesn't persist when
leaving search-query mode; update the handler where setHostMethod(val) is called
(the block that checks val === hostMethods.searchQuery / hostMethods.hosts /
hostMethods.hostCollections / hostMethods.hostGroups) to call
setSelectedBookmark(null) (or an empty string per app convention) whenever val
!== hostMethods.searchQuery so bookmark_id is not sent for non-search targeting;
keep the existing errorText logic unchanged.
---
Nitpick comments:
In `@webpack/JobWizard/steps/AdvancedFields/__tests__/AdvancedFields.test.js`:
- Around line 406-411: The test is fragile because it indexes the second
dispatched ForemanTasksTask action (resourceSearchAction[1]); instead, filter
newStore.getActions() for actions with key === 'ForemanTasksTask' (as done into
resourceSearchAction) and assert that at least one of those actions' url
contains the encoded query (e.g., check that some(action =>
String(action.url).includes('name=some+search'))). Keep the existing expect for
total count if desired but remove the order-dependent indexing and use
Array.prototype.some on resourceSearchAction to verify the encoded query appears
in any matching action.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 95a1cdb9-5450-4ecc-b7d7-37764726aa69
⛔ Files ignored due to path filters (2)
webpack/JobWizard/steps/AdvancedFields/__tests__/__snapshots__/AdvancedFields.test.js.snapis excluded by!**/*.snapwebpack/react_app/components/TargetingHosts/__tests__/__snapshots__/TargetingHostsPage.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
webpack/JobWizard/JobWizard.jswebpack/JobWizard/JobWizardConstants.jswebpack/JobWizard/JobWizardSelectors.jswebpack/JobWizard/__tests__/JobWizardPageRerun.test.jswebpack/JobWizard/__tests__/fixtures.jswebpack/JobWizard/autofill.jswebpack/JobWizard/steps/AdvancedFields/__tests__/AdvancedFields.test.jswebpack/JobWizard/steps/HostsAndInputs/HostSearch.jswebpack/JobWizard/steps/HostsAndInputs/__tests__/HostsAndInputs.test.jswebpack/JobWizard/steps/HostsAndInputs/index.jswebpack/JobWizard/steps/Schedule/__tests__/Schedule.test.jswebpack/JobWizard/submit.jswebpack/__mocks__/foremanReact/components/SearchBar.js
💤 Files with no reviewable changes (1)
- webpack/mocks/foremanReact/components/SearchBar.js
| 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) | ||
| ); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think this behaviour makes sense
There was a problem hiding this comment.
@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.
adamlazik1
left a comment
adamlazik1
left a comment
There was a problem hiding this comment.
I have few notes and questions:
-
When selecting a bookmark and then switching host method to something else, the bookmark search query is not cleared. For example, when the host method is switched to
Hostsand a different host than the one the bookmark resolves to is selected, the job invocation is executed on both hosts - from bookmark search query and fromHosts. Then under target hosts it displays only the search query from bookmarks but not the other resolved host fromHosts. Is this intended behavior? -
Typing a query identical to a bookmark implicitly sends bookmark_id instead of search_query. This might potentially cause subtle rerun mismatches. If the query was originally dynamic, the rerun is triggered with the static query from the bookmark instead.
-
The bookmark dropdown is being cut from the wizard view:
|
Thanks!
No, fixed - so when other items are selected, it will not send the bookmark id.
Right, fixed so if the search bar is typed, the bookmark id is not sent
Changed the position of the dropdown so it will be visible within the wizard |
|
I apologize for the noise, seems I don't know how to use the gh CLI feature. Can you please force push your branch in the current state once again to overwrite my mess? |
MariaAga
force-pushed
the
job-wizard-bookmarks
branch
from
e1045a7 to
47712ae
Compare
adamlazik1
left a comment
adamlazik1
left a comment
There was a problem hiding this comment.
LGTM. Tested on verified on packit build.
|
Thanks @MariaAga ! |
2 participants
bookmark_id is always null in jobs created by the job wizard, it should be the actually value of the bookmark used
Is this issue a regression from the legacy page.
To test:
Expected behavior:
"Bookmark BOOKMARK_NAME using static query", and the search query is the bookmark.
Also rerun job should apply the bookmark search query.