Fixes #37630 - Display basic list of hosts on the new job_invocations page #913
Conversation
kmalyjur
force-pushed
the
job-inv-table
branch
2 times, most recently
from
257ee21
to
244eb88
Compare
kmalyjur
force-pushed
the
job-inv-table
branch
3 times, most recently
from
fd6ba44
to
2ba8883
Compare
kmalyjur
force-pushed
the
job-inv-table
branch
from
2ba8883
to
583e227
Compare
|
Going over the PR, in the old page there is a status "Awaiting start" and I dont see it in the table/pr |
| add_scoped_search_description_for(JobInvocation) | ||
| param :id, :identifier, :required => true | ||
| def hosts | ||
| Rails.logger.info "Params: #{params.inspect}" |
| export const columns = { | ||
| name: { | ||
| title: __('Name'), | ||
| wrapper: ({ name }) => <a href={`/new/hosts/${name}`}>{name}</a>, |
There was a problem hiding this comment.
Can the link to host be setting aware, example: theforeman/foreman_openscap#571
| groups: { | ||
| title: __('Host group'), | ||
| wrapper: ({ hostgroup_id, hostgroup_name }) => ( | ||
| <a href={`/hostgroups/${hostgroup_id}/edit`}>{hostgroup_name}</a> |
There was a problem hiding this comment.
If hostgroup is empty it shouldnt be a link
There was a problem hiding this comment.
Well it is shown as empty and without an error so the link comment might not be relevant
| <a href={`/operatingsystems/${operatingsystem_id}/edit`}> | ||
| {operatingsystem_name} | ||
| </a> | ||
| ), | ||
| weight: 3, | ||
| }, | ||
| smart_proxy: { | ||
| title: __('Smart proxy'), | ||
| wrapper: ({ smart_proxy_name, smart_proxy_id }) => ( | ||
| <a href={`/smart_proxies/${smart_proxy_id}`}>{smart_proxy_name}</a> |
There was a problem hiding this comment.
Same here, if the values are empty they shouldnt be links (Not sure its possible for os, but still)
|
|
||
| return ( | ||
| <TableIndexPage | ||
| apiUrl="" |
There was a problem hiding this comment.
Because it is not used in the TableIndexPage, I use the replacementResponse instead, however, feel free to correct me.
| itemCount={response?.subtotal} | ||
| results={response?.results} | ||
| url="" | ||
| refreshData={() => {}} |
There was a problem hiding this comment.
Because it isn't used either (but is required), it's only used in the Table component, but I pass my own Table to the TableIndexPage.
| {k === 'status' ? ( | ||
| <JobStatusIcon | ||
| status={columns[k].status({ | ||
| job_status: result?.job_status, | ||
| status_label: response?.status_label, | ||
| })} | ||
| > | ||
| {columns[k].tableTitle({ | ||
| job_status: result?.job_status, | ||
| status_label: response?.status_label, | ||
| })} | ||
| </JobStatusIcon> |
There was a problem hiding this comment.
Can this move to the wrapper in the consts?
| @@ -120,6 +121,7 @@ const JobInvocationDetailPage = ({ | |||
| /> | |||
| </Flex> | |||
| </Flex> | |||
| {items.id !== undefined && <JobInvocationHostTable id={id} />} | |||
There was a problem hiding this comment.
Not sure if its in the scope of the PR, but previously we had this:
<div class="alert alert-warning">
<%=
_("The dynamic query '%{query}' was not resolved yet. The list of hosts to which it would resolve now can be seen %{here}." %
{ :query => job_invocation.targeting.search_query,
:here => link_to(_('here'), hosts_url(:search => job_invocation.targeting.search_query))})
%>
</div>
if the table was empty
There was a problem hiding this comment.
The empty table in the old UI looks like this:
I made it to look like this now:
I don't know when the message from the code you sent appears. 🤔
There was a problem hiding this comment.
Looks good! the error is only when job_invocation.resolved? (I missed that)
There was a problem hiding this comment.
I'm not sure what you mean. This empty table happens when !isPending && results.length === 0 && !errorMessage.
There was a problem hiding this comment.
When a user starts a job in the future, and has a dynamic hosts list, there will be no hosts until the job starts.
So when that happens in the old page an alert will show up.
I assume job_invocation.resolved?, in the old page, is what checks if this is the case
kmalyjur
force-pushed
the
job-inv-table
branch
from
583e227
to
e4d6454
Compare
pr-processor
bot
added
Needs re-review
and removed
Waiting on contributor
Needs re-review
labels
kmalyjur
force-pushed
the
job-inv-table
branch
from
e4d6454
to
e608893
Compare
|
@MariaAga Thank you for your review! I incorporated everything except for If hostgroup is empty it shouldnt be a link, I think it's not necessary, but feel free to tell me otherwise. I also made Fixes #37805 - Add possibility to display message when table is empty on which it's dependent. |
| getColumnsStatus({ hostJobStatus: job_status }).title, | ||
| status: ({ job_status }) => | ||
| getColumnsStatus({ hostJobStatus: job_status }).status, | ||
| wrapper: ({ job_status }) => { | ||
| const { title, status } = getColumnsStatus({ |
There was a problem hiding this comment.
Can you move getColumnsStatus calls into one call just after const Columns = () => {?
Dependent on this PR.
The next PR will add sorting, filtering by status, and permission handling.
The changes in the
JobStatusIconalso affect the recent jobs card on the host detail page.