Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #38090 - Add filtering to job invocation hosts table #934

Merged
merged 6 commits into from
Jan 28, 2025

Conversation

kmalyjur
Copy link
Contributor

@kmalyjur kmalyjur commented Dec 12, 2024

Sorting and filtering were added to the host table of the new job invocation page.
(Ordering by status is not permitted on purpose.)
Needs Fixes #38134 - Add custom pagination to table

Copy link
Member

@MariaAga MariaAga left a comment

Choose a reason for hiding this comment

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

This works almost perfectly, and its nice that the chart also searchs the table, only a few comments?

SUCCESS: { id: 'success', title: __('Succeeded') },
FAILED: { id: 'failed', title: __('Failed') },
PENDING: { id: 'pending', title: __('In Progress') },
CANCELLED: { id: 'cancelled', title: __('Canceled') },
Copy link
Member

Choose a reason for hiding this comment

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

Canceled should have 2 Ls -> Cancelled (both right, but we use the 2 L one in REX)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch 😄

webpack/JobInvocationDetail/JobInvocationHostTable.js Outdated Show resolved Hide resolved
});
setAPIOptions({
...apiOptions,
params: { search: searchQuery },
Copy link
Member

Choose a reason for hiding this comment

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

Should include the filter search as well, for example if you select a status and then search the status is not included in the search but still shown as selected

@kmalyjur
Copy link
Contributor Author

I also removed the custom pagination from the Table as @MariaAga found out the usage of setParams is enough.

Comment on lines 155 to 160
setParamsAndAPI({
...params,
...newPagination,
search: urlSearchQuery,
});
};

Copy link
Member

Choose a reason for hiding this comment

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

this creates a second api call, best to use the new one :)

Copy link
Member

@MariaAga MariaAga Jan 21, 2025

Choose a reason for hiding this comment

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

nope my bad, something else is causing the second api call, to reproduce: select a status, edit the search, on search submit there are 2 api calls and one is wrong

@adamruzicka
Copy link
Contributor

image

image

It is a bit unfortunate to map "running" to "Pending" and "pending" to "pending" with lowercase p and a different icon. Does the design say anything about this?

@kmalyjur
Copy link
Contributor Author

kmalyjur commented Jan 25, 2025

@adamruzicka Actually, the design doesn't mention it. You're right, it looks bad. I'll change it.

@kmalyjur
Copy link
Contributor Author

  • I added the code @MariaAga wrote for fixing the second unneeded API call
  • @adamruzicka running and pending are both mapped to "Pending" now

The PR should be ready to be merged

@MariaAga
Copy link
Member

Running jobs dont show hosts, so merging should wait until we see if its the pr or just my env

@MariaAga
Copy link
Member

job_invocation.id=ID shows 0 results if the job is running

@kmalyjur
Copy link
Contributor Author

kmalyjur commented Jan 27, 2025

@MariaAga I can see them 🤔
EDIT: I now see you mean the scheduled / awaiting start hosts

@adamruzicka
Copy link
Contributor

Sadly, yes, the per-host tasks are not created until the job starts running, which makes it a little bit tricky.

kmalyjur and others added 5 commits January 28, 2025 15:14
Searching inside scheduled jobs doesn't really work, but at least that's
consistent with the old implementation.
@adamruzicka
Copy link
Contributor

js tests are red :/

@MariaAga
Copy link
Member

js test error make no sense, and dont happen locally, maybe re run them?

@kmalyjur
Copy link
Contributor Author

It ain't much, but it's honest work

@adamruzicka adamruzicka merged commit 2293c94 into theforeman:master Jan 28, 2025
13 of 14 checks passed
@adamruzicka
Copy link
Contributor

Thank you @kmalyjur & @MariaAga !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants