Fixes #37630 - Display basic list of hosts on the new job_invocations page #913

kmalyjur · 2024-07-04T17:44:33Z

Dependent on this PR.

The next PR will add sorting, filtering by status, and permission handling.
The changes in the JobStatusIcon also affect the recent jobs card on the host detail page.

MariaAga · 2024-09-02T13:45:51Z

Going over the PR, in the old page there is a status "Awaiting start" and I dont see it in the table/pr

MariaAga · 2024-09-02T13:57:57Z

app/controllers/api/v2/job_invocations_controller.rb

+      add_scoped_search_description_for(JobInvocation)
+      param :id, :identifier, :required => true
+      def hosts
+        Rails.logger.info "Params: #{params.inspect}"


Can the logger be removed?

MariaAga · 2024-09-02T15:41:02Z

webpack/JobInvocationDetail/JobInvocationConstants.js

+export const columns = {
+  name: {
+    title: __('Name'),
+    wrapper: ({ name }) => <a href={`/new/hosts/${name}`}>{name}</a>,


Can the link to host be setting aware, example: theforeman/foreman_openscap#571

MariaAga · 2024-09-02T15:44:17Z

webpack/JobInvocationDetail/JobInvocationConstants.js

+  groups: {
+    title: __('Host group'),
+    wrapper: ({ hostgroup_id, hostgroup_name }) => (
+      <a href={`/hostgroups/${hostgroup_id}/edit`}>{hostgroup_name}</a>


If hostgroup is empty it shouldnt be a link

Well it is shown as empty and without an error so the link comment might not be relevant

MariaAga · 2024-09-02T15:45:43Z

webpack/JobInvocationDetail/JobInvocationConstants.js

+      <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>


Same here, if the values are empty they shouldnt be links (Not sure its possible for os, but still)

MariaAga · 2024-09-02T15:48:52Z

webpack/JobInvocationDetail/JobInvocationHostTable.js

+
+  return (
+    <TableIndexPage
+      apiUrl=""


Why is it empty?

Because it is not used in the TableIndexPage, I use the replacementResponse instead, however, feel free to correct me.

MariaAga · 2024-09-02T15:49:31Z

webpack/JobInvocationDetail/JobInvocationHostTable.js

+        itemCount={response?.subtotal}
+        results={response?.results}
+        url=""
+        refreshData={() => {}}


Also why is this empty?

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.

MariaAga · 2024-09-02T15:50:34Z

webpack/JobInvocationDetail/JobInvocationHostTable.js

+                {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>


Can this move to the wrapper in the consts?

MariaAga · 2024-09-02T15:53:15Z

webpack/JobInvocationDetail/index.js

@@ -120,6 +121,7 @@ const JobInvocationDetailPage = ({
          />
        </Flex>
      </Flex>
+      {items.id !== undefined && <JobInvocationHostTable id={id} />}


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

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. 🤔

Looks good! the error is only when job_invocation.resolved? (I missed that)

I'm not sure what you mean. This empty table happens when !isPending && results.length === 0 && !errorMessage.

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

Thank you for explaining, @asteflova helped me with rewording it and I incorporated it like this:

Also, I made this PR, which is needed for that.

kmalyjur · 2024-09-11T13:35:43Z

@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.

MariaAga · 2024-09-12T17:13:25Z

webpack/JobInvocationDetail/JobInvocationConstants.js

+        getColumnsStatus({ hostJobStatus: job_status }).title,
+      status: ({ job_status }) =>
+        getColumnsStatus({ hostJobStatus: job_status }).status,
+      wrapper: ({ job_status }) => {
+        const { title, status } = getColumnsStatus({


Can you move getColumnsStatus calls into one call just after const Columns = () => {?

MariaAga · 2024-09-20T08:51:57Z

webpack/JobInvocationDetail/JobInvocationConstants.js

+      tableTitle: ({ job_status }) =>
+        getColumnsStatus({ hostJobStatus: job_status }).title,
+      status: ({ job_status }) =>
+        getColumnsStatus({ hostJobStatus: job_status }).status,
+      wrapper: ({ job_status }) => {
+        const { title, status } = getColumnsStatus({


Its still 3 calls instead of one, I think that it can call getColumnsStatus only once

MariaAga · 2024-09-20T08:59:34Z

webpack/JobInvocationDetail/JobInvocationHostTable.js

+      if (targeting?.targeting_type === 'dynamic_query') {
+        return {
+          action: {
+            title: 'View hosts',


Should be translated

MariaAga · 2024-09-20T09:07:28Z

webpack/JobInvocationDetail/JobInvocationHostTable.js

+        emptyAction={action}
+        emptyMessage={message}


I'm not sure an action with a button would be best here, since then the user cant open it in a new tab. instead of an action can there be a link in the message?

I can change it as you're proposing, but I'll have to use my own EmptyState in this file or make the emptyMessage accept an object instead of a string. Is it okay?

Yeah I think it should use its own EmptyState

MariaAga · 2024-09-20T09:53:50Z

webpack/JobInvocationDetail/JobInvocationDetail.scss

+.job-invocation-host-table-page-section {
+  padding-top: 0px;
+}


There is no element with this classname.
Also can the table sides have no padding like in the hosts page?

MariaAga · 2024-09-20T09:56:46Z

webpack/react_app/components/RecentJobsCard/JobStatusIcon.js

+        <span className="job-planned">
+          <BuildIcon {...props} /> {children}
+        </span>
+      );
+    case JOB_RUNNING_STATUS:
+      return (
+        <span className="job-running">
+          <RunningIcon {...props} /> {children}
+        </span>
+      );
+    case JOB_CANCELLED_STATUS:
+      return (
+        <span className="job-cancelled">
+          <ExclamationTriangleIcon {...props} /> {children}
+        </span>
+      );
+    case JOB_AWAITING_STATUS:
+      return <span className="job-awaiting_start">{children}</span>;
    default:
      return (
-        <span className="job-info">
+        <span className="job-unknown">


Only the icon should have the job* classname, so the text will always be black

Also, can you import webpack/react_app/components/RecentJobsCard/styles.scss in the page, just for clearity on where the colours come from

can you import webpack/react_app/components/RecentJobsCard/styles.scss in the page

What do you please mean by that? import './styles.scss'; in the JobStatusIcon.js isn't enough?

I missed that, nvm :)

MariaAga · 2024-10-02T12:15:21Z

webpack/JobInvocationDetail/JobInvocationHostTable.js

+                <>
+                  {__('The dynamic query is still being processed. You can ')}
+                  <a href={`/new/hosts?search=${targeting?.search_query}`}>
+                    {__('view the hosts')}
+                  </a>
+                  {__(' targeted by the query.')}


Please use formmatedMessage for this kind of translations, example:

import { FormattedMessage } from 'react-intl'; <FormattedMessage id="report-manual" defaultMessage={__( 'If you wish to configure Puppet to forward its reports to Foreman, please follow {settingUpPuppetReports} and {emailReporting}' )} values={{ settingUpPuppetReports: ( <a href={getManualURL('3.5.4PuppetReports')}> {__('setting up reporting')} </a> ), emailReporting: ( <a href={getWikiURL('Mail_Notifications')}> {__('e-mail reporting')} </a> ), }} /> ```

MariaAga

except translation UI code looks good, and rails look ok but im no expert there

kmalyjur · 2024-10-10T13:39:11Z

@MariaAga Could you please look at it again? I also incorporated some changes after @adamruzicka's review. Adam, could you please review the backend code?

adamruzicka · 2024-10-23T14:30:15Z

total (or subtotal) counts seem to be off once you start searching

adamruzicka · 2024-10-23T14:33:29Z

Once you set a search, you can't really unset it. It won't allow you to submit the search if the searchbar is empty, although I can't really put my finger on where it comes from

kmalyjur · 2024-10-31T15:38:02Z

@adamruzicka thank you, I think I somehow broke it in the process 😅 should be fixed now

MariaAga

Search clearing, subtotal, and showing the dynamic query message all work now

… page

adamruzicka · 2024-11-06T12:51:02Z

Thank you @kmalyjur & @MariaAga !

MariaAga · 2024-11-06T13:02:24Z

webpack/JobInvocationDetail/JobInvocationHostTable.js

+  const bottomPagination = (
+    <Pagination
+      ouiaId="table-hosts-bottom-pagination"
+      key="table-bottom-pagination"
+      page={params.page}
+      perPage={params.perPage}
+      itemCount={response?.subtotal}
+      onChange={onPagination}
+    />
+  );


This is the same code as the table pagination, since this pr is merged already can you remove it in a later pr?

pr-processor bot added the Not yet reviewed label Jul 4, 2024

kmalyjur force-pushed the job-inv-table branch 2 times, most recently from 257ee21 to 244eb88 Compare July 10, 2024 11:22

kmalyjur force-pushed the job-inv-table branch 3 times, most recently from fd6ba44 to 2ba8883 Compare August 28, 2024 08:36

kmalyjur marked this pull request as ready for review August 28, 2024 10:46

kmalyjur force-pushed the job-inv-table branch from 2ba8883 to 583e227 Compare August 28, 2024 11:56

MariaAga reviewed Sep 2, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Sep 2, 2024

MariaAga reviewed Sep 2, 2024

View reviewed changes

MariaAga suggested changes Sep 2, 2024

View reviewed changes

pr-processor bot added the Waiting on contributor label Sep 2, 2024

kmalyjur mentioned this pull request Sep 11, 2024

Fixes #37805 - Add possibility to display message when table is empty theforeman/foreman#10316

Merged

kmalyjur force-pushed the job-inv-table branch from 583e227 to e4d6454 Compare September 11, 2024 11:39

pr-processor bot added Needs re-review and removed Waiting on contributor Needs re-review labels Sep 11, 2024

kmalyjur force-pushed the job-inv-table branch from e4d6454 to e608893 Compare September 11, 2024 13:27

kmalyjur requested a review from MariaAga September 11, 2024 13:52

MariaAga reviewed Sep 12, 2024

View reviewed changes

kmalyjur mentioned this pull request Sep 18, 2024

Fixes #37836 - Add possibility to display action when table is empty theforeman/foreman#10327

Merged

kmalyjur force-pushed the job-inv-table branch from e608893 to b900ea7 Compare September 18, 2024 11:36

pr-processor bot added the Waiting on contributor label Sep 18, 2024

kmalyjur force-pushed the job-inv-table branch from b900ea7 to d7ff27f Compare September 18, 2024 11:37

pr-processor bot added Needs re-review and removed Waiting on contributor labels Sep 18, 2024

kmalyjur force-pushed the job-inv-table branch from d7ff27f to b5b0103 Compare September 18, 2024 11:44

MariaAga reviewed Sep 20, 2024

View reviewed changes

kmalyjur force-pushed the job-inv-table branch 2 times, most recently from 4108d90 to 76533c7 Compare September 30, 2024 10:40

kmalyjur requested a review from MariaAga September 30, 2024 11:13

MariaAga reviewed Oct 2, 2024

View reviewed changes

kmalyjur force-pushed the job-inv-table branch 4 times, most recently from 916d0f6 to b207ca1 Compare October 10, 2024 13:23

kmalyjur force-pushed the job-inv-table branch from b207ca1 to 595de12 Compare October 31, 2024 14:30

kmalyjur requested a review from MariaAga October 31, 2024 15:36

MariaAga approved these changes Oct 31, 2024

View reviewed changes

kmalyjur force-pushed the job-inv-table branch from 595de12 to 3d6a118 Compare November 4, 2024 12:52

Fixes #37630 - Display basic list of hosts on the new job_invocations…

069d59b

… page

kmalyjur force-pushed the job-inv-table branch from 3d6a118 to 069d59b Compare November 5, 2024 15:22

kmalyjur requested a review from MariaAga November 6, 2024 09:59

adamruzicka approved these changes Nov 6, 2024

View reviewed changes

adamruzicka merged commit 336f339 into theforeman:master Nov 6, 2024
23 checks passed

MariaAga reviewed Nov 6, 2024

View reviewed changes

ofedoren mentioned this pull request Nov 7, 2024

Fixes #37852 - Support Rails 7.0 Katello/katello#11155

Merged

Fixes #37630 - Display basic list of hosts on the new job_invocations page #913

Fixes #37630 - Display basic list of hosts on the new job_invocations page #913

Conversation

kmalyjur commented Jul 4, 2024 • edited Loading

MariaAga commented Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MariaAga Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

kmalyjur Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

kmalyjur commented Sep 11, 2024

MariaAga Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MariaAga left a comment

Choose a reason for hiding this comment

kmalyjur commented Oct 10, 2024 • edited Loading

adamruzicka commented Oct 23, 2024

adamruzicka commented Oct 23, 2024

kmalyjur commented Oct 31, 2024

MariaAga left a comment

Choose a reason for hiding this comment

adamruzicka commented Nov 6, 2024

Choose a reason for hiding this comment

kmalyjur commented Jul 4, 2024 •

edited

Loading

MariaAga Sep 11, 2024 •

edited

Loading

kmalyjur Sep 18, 2024 •

edited

Loading

MariaAga Sep 12, 2024 •

edited

Loading

kmalyjur commented Oct 10, 2024 •

edited

Loading