Fixes #38165 - Add checkboxes and related actions: open all, rerun, open all failed by kmalyjur · Pull Request #944 · theforeman/foreman_remote_execution

kmalyjur · 2025-01-28T14:31:48Z

Adds checkboxes to the table
Modifies the button and modal for opening selected hosts in a new tab
Adds a button for rerunning selected hosts
Adds a kebab dropdown for opening all failed hosts in a new tab

Canceling and aborting + tests for the table functionality will be added in a new PR.

MariaAga · 2025-01-28T14:39:54Z

 collection @hosts

-attribute :name, :operatingsystem_id, :operatingsystem_name, :hostgroup_id, :hostgroup_name, :id
+attribute :id, :name, :operatingsystem_id, :operatingsystem_name, :hostgroup_id, :hostgroup_name


is this needed?

Nope, just wrong rebase, sorry

MariaAga · 2025-01-28T14:41:36Z

+                `Task for the host ${
+                  action === 'cancel' ? 'cancelled' : 'aborted'
+                } successfully`


please ues 2 different strings so we wont have to do difficult translations

adamruzicka

Selecting the rows one by one and clicking rerun works.

Selecting all, then unchecking one and clicking rerun runs it only on the one that was unselected.

Selecting all and clicking rerun doesn't do anything, even though the button appears active.

MariaAga · 2025-01-28T14:47:48Z

          <OpenAlInvocations
            setShowAlert={setShowAlert}
            results={results}
            id={id}
-          />
-        }
+          />,
+          <CheckboxesActions
+            jobID={id}
+            bulkParams={bulkParams}
+            failedHosts={failedHosts}
+            selectedCount={selectedCount}
+            currentPermissions={currentPermissions}
+            permissionsStatus={permissionsStatus}
+          />,
+        ]}
+        controller="hosts"
+        creatable={false}
+        replacementResponse={combinedResponse}
+        selectionToolbar={selectionToolbar}
+        updateSearchQuery={updateSearchQueryBulk}


need key props

MariaAga · 2025-01-28T14:48:23Z

  targeting: PropTypes.object.isRequired,
  finished: PropTypes.bool.isRequired,
  autoRefresh: PropTypes.bool.isRequired,
+  initialFilter: PropTypes.string.isRequired,


The prop `initialFilter` is marked as required in `JobInvocationHostTable`, but its value is `undefined`.

MariaAga · 2025-01-28T14:50:11Z

+
+  return (
+    <>
+      <RerunButton />


can you please space between rerun and open all in new tabs?

Of course, sorry, I haven't even looked at it after rebasing

MariaAga · 2025-01-28T14:58:07Z

+      onClick={() => {
+        if (failedHosts.length <= 3) {
+          failedHosts.forEach(id =>
+            openLink(`/job_invocations/${id}`, () => {})


This should open /show_template_invocation_by_host/:host_id/job_invocation/:id

MariaAga

Thanks for the addition! I did an initial review but will need to do another one once its all addressed

MariaAga · 2025-03-18T12:29:33Z

  section:nth-child(1) {
    padding: 0;
  }
+  .open-all-button {


Can you make it more centered in the line height middle?

MariaAga · 2025-03-18T12:58:17Z

+    <ToolbarItem key="selectAll">
+      <SelectAllCheckbox


Also can you make the selectall be aligned with the search?

This one might be on me in foreman core

theforeman/foreman#10485

MariaAga · 2025-03-18T15:33:48Z

    <Modal
      ouiaId="template-invocation-new-tab-modal"
-      title={__('Open all invocations in new tabs')}
+      title={__('Open all selected invocations in new tabs')}


The modal is a bit too small for the new title, could you make the modal bigger? (or add some wrap css)

MariaAga · 2025-03-18T15:41:50Z

          onClick={() => {
-            results.forEach(result => {
+            idsArray.forEach(result => {
              openLink(templateInvocationPageUrl(result.id, id), '_blank');


Should also be openLink(templateInvocationPageUrl(result, id), '_blank'); as the previous one

Might be good to test that the window.open mock function has been called on click, with the right url

MariaAga · 2025-03-18T16:34:01Z

  const OpenAllButton = () => (
    <Button
      variant="link"
+      isDisabled={idsArray.length === 0}


Clicking on the select all checkbox doesnt enable the button

Only if the page has a search *

kmalyjur · 2025-05-14T18:02:55Z

Refactored a good bit - updated the PR description accordingly.

kmalyjur · 2025-06-11T10:45:23Z

I added a fix for the bottom pagination and also made some changes to the filter functionality.
There is one small issue with the table when changing the page directly in the URL, which I'll resolve in foreman/.../TableIndexPage.js.

MariaAga

I tried to open all failed and the array for hostsToOpen was empty but the modal said The number of failed invocations is: 97

Also if I select all the open in new tab button is disabled

MariaAga · 2025-05-22T10:29:52Z

+  const [isOpenFailed, setIsOpenFailed] = useState(false);
+  const idsArray = getIdsArray(bulkParams, allHostsIds, isAllSelected);
+  const permissions = useSelector(
+    state => selectTemplateInvocation(state)?.permissions


Need to make sure that #968 isnt clashing with this

MariaAga · 2025-06-13T10:34:37Z

  margin-bottom: -10px;
 }
+
+.pf-v5-c-toolbar__group.pf-m-button-group {


This needs to be under the page classname

MariaAga · 2025-06-13T10:38:03Z

    },
    actions: {
-      title: '',
+      title: <span className="pf-u-screen-reader">{__('Actions')}</span>,


For some reason its not being aligned

MariaAga · 2025-06-13T10:49:06Z

  [DYNAMIC_TYPE]: __('Dynamic Query'),
 };
+
+export const getIdsArray = (bulkParams, allHostsIds, isAllSelected) => {


Not sure, but can you just use inclusionSet, exclusionSet, I think useBulkSelect should send them as well

MariaAga · 2025-06-13T11:00:33Z

 export const GET_REPORT_TEMPLATE_INPUTS = 'GET_REPORT_TEMPLATE_INPUTS';
 export const JOB_INVOCATION_HOSTS = 'JOB_INVOCATION_HOSTS';
+export const GET_TEMPLATE_INVOCATION = 'GET_TEMPLATE_INVOCATION';
+export const MAX_HOSTS_API_SIZE = 'MAX_HOSTS_API_SIZE';


This should probably be a number since hostsToOpen.slice(0, MAX_HOSTS_API_SIZE) doesnt work for a string

ekohl

I'm thinking out loud here. Background is that https://issues.redhat.com/browse/SAT-26318 was opened and after looking at the new job detail page the only feedback was a request to see all output. Not just the failed ones. This PR looks like the closest we have, so I'm commenting here.

If there's a better place then I'll happily hear about that.

Should open be for opening in a new tab and use expand to open on the same page?
Does it make sense to special case the failed jobs? If you add an action to open/expand all then the user can filter the statuses to only failed.
Could it be a bulk action on the selected items?
Is it clear enough this opens new tabs? The "open" wording may not be clear and it could open 100 of tabs.

ekohl · 2025-06-25T14:24:09Z

+      href={foremanUrl(
+        `/job_invocations/${jobID}/rerun?${idsArray
+          .map(id => `host_ids[]=${id}`)
+          .join('&')}`


Does it make sense to use URLSearchParams to construct the query string? I think generally speaking it's a good practice. Here it may not matter that much, but it should deal well with escaping. [] should be escaped.

ekohl · 2025-06-25T14:39:14Z

+      foremanUrl(`/api/job_invocations/${jobID}/hosts`),
+      {
+        params: {
+          per_page: MAX_HOSTS_API_SIZE,


There is no pagination. What if there are more than 100 failed hosts? Will the UI give the user the impression it will open more than 100? How do you expect the user to open the ones beyond 100? I'm not sure if all is supported here or even a good idea.

Can we (or maybe we already do) add an "open selected" button, then users can choose what to open.
I dont think anyone want to open more than 100 tabs.
And anyway users can open in a new tab indevidually.
We warn them about the 100 in the modal "{__('This will open a new tab for each invocation. The maximum is 100.')}"

It's why I liked the suggestion to expand items in bulk on the page. That's a bit easier to manage than tabs.

We can add it in another pr, but the open all failed in tabs was requested specifically

Do you know if there is an issue for it now? I'd like to include it somewhere in the larger effort but it doesn't have to be this PR.

@ekohl @MariaAga thank you for your comments!

Regarding SAT-26318 - I closed this one and I've just created SAT-35515.

Should open be for opening in a new tab and use expand to open on the same page?

Yeah I think so and I refer to it like that

We can add it in another pr, but the open all failed in tabs was requested specifically

I agree

Do you know if there is an issue for it now? I'd like to include it somewhere in the larger effort but it doesn't have to be this PR.

You mean for the failed tabs or for expanding the table?

Regarding SAT-26318 - I closed this one and I've just created SAT-35515.

Excellent, thanks.

SAT-26318

Expanding the table, which you just created an issue for.

ekohl · 2025-06-25T14:39:47Z

+
+  const handleOpenHosts = async (type = 'all') => {
+    if (type === 'failed') {
+      if (failedCount <= 3) {


Why 3 here?

if its less than 3 we just open it in new tabs, if its more than we ask the user if they are sure they want to open X amount of tabs just so no one gets 10 new tabs by accidentally clicking the button

My programming professor in university always insistent on constants with a descriptive name. Could we do that?

I usually remember him in my reviews 🙃, not enough

kmalyjur · 2025-07-07T12:07:35Z

The latest push incorporates all of your suggestions. Could you please look at it again?

MariaAga · 2025-07-09T15:26:44Z

+import { OutlinedWindowRestoreIcon } from '@patternfly/react-icons';
+import EllipsisVIcon from '@patternfly/react-icons/dist/js/icons/ellipsis-v-icon';


Please combine them

MariaAga · 2025-07-09T15:44:56Z

+
+  const fetchFailedHosts = async () => {
+    const { data } = await axios.get(
+      foremanUrl(`/api/job_invocations/${jobID}/hosts`),


Sorry this is coming late in the review, took me sometime to find that this is called on tabs open and not on modal open (and I didnt see extra requests until I opened the tabs).
I think instead of axios.get on button click, it should use the Useapi, and then the request wont be repeated and the failed host list will be available to all with redux

MariaAga · 2025-07-15T15:10:36Z

+        `Are you sure you want to open all ${modalText} invocations in new tabs?`
+      )}
+      <br />
+      {__('This will open a new tab for each invocation. The maximum is 100.')}
+      <br />
+      {__(`The number of ${modalText} invocations is:`)}{' '}


should be in sptrinf https://projects.theforeman.org/projects/foreman/wiki/Translating

MariaAga · 2025-07-16T11:45:37Z

+        onClick={() => handleOpenHosts('failed')}
+        isDisabled={failedCount === 0}
+      >
+        {__(`Open all failed runs (${failedCount})`)}


also needs sprintf instead

MariaAga · 2025-07-16T11:46:00Z

+      ouiaId="template-invocation-new-tab-button"
+      variant="link"
+    >
+      <Tooltip content="Open selected in new tab">


needs to be translated

…pen all failed

adamruzicka · 2025-07-16T16:19:57Z

Thank you @kmalyjur & @MariaAga !

pr-processor Bot added the Not yet reviewed label Jan 28, 2025

kmalyjur mentioned this pull request Jan 28, 2025

Add checkboxes actions to job invocation page #943

Closed

1 task

MariaAga reviewed Jan 28, 2025

View reviewed changes

pr-processor Bot removed the Not yet reviewed label Jan 28, 2025

adamruzicka requested changes Jan 28, 2025

View reviewed changes

pr-processor Bot added the Waiting on contributor label Jan 28, 2025

MariaAga reviewed Jan 28, 2025

View reviewed changes

kmalyjur force-pushed the job-inv-table-checkboxes-clean branch from fe71baf to 121976e Compare February 12, 2025 10:39

pr-processor Bot added the Needs re-review label Feb 12, 2025

kmalyjur force-pushed the job-inv-table-checkboxes-clean branch from 121976e to 6c1ce3a Compare March 10, 2025 11:38

pr-processor Bot removed the Waiting on contributor label Mar 10, 2025

kmalyjur changed the title ~~Fixes #38165 - Add checkboxes actions to job invocation page~~ Fixes #38165 - Add checkboxes to job invocation page Mar 10, 2025

kmalyjur marked this pull request as ready for review March 10, 2025 12:55

kmalyjur force-pushed the job-inv-table-checkboxes-clean branch from 6c1ce3a to ccdd86d Compare March 10, 2025 14:21

kmalyjur requested review from MariaAga and adamruzicka March 10, 2025 14:28

MariaAga requested changes Mar 18, 2025

View reviewed changes

pr-processor Bot added Waiting on contributor and removed Needs re-review labels Mar 18, 2025

kmalyjur force-pushed the job-inv-table-checkboxes-clean branch from ccdd86d to 1bfeae8 Compare May 14, 2025 17:37

pr-processor Bot added Needs re-review and removed Waiting on contributor labels May 14, 2025

kmalyjur changed the title ~~Fixes #38165 - Add checkboxes to job invocation page~~ Fixes #38165 - Add checkboxes and related actions: open all, rerun, open all failed May 14, 2025

kmalyjur requested a review from MariaAga May 19, 2025 07:53

MariaAga reviewed May 19, 2025

View reviewed changes

Comment thread webpack/JobInvocationDetail/JobInvocationHostTable.js Outdated

pr-processor Bot removed the Needs re-review label May 19, 2025

pr-processor Bot added the Needs re-review label Jun 10, 2025

kmalyjur force-pushed the job-inv-table-checkboxes-clean branch 2 times, most recently from 9012e1f to 2cd2e8b Compare June 11, 2025 10:38

MariaAga requested changes Jun 13, 2025

View reviewed changes

pr-processor Bot added Waiting on contributor and removed Needs re-review labels Jun 13, 2025

MariaAga reviewed Jun 13, 2025

View reviewed changes

ekohl reviewed Jun 25, 2025

View reviewed changes

kmalyjur force-pushed the job-inv-table-checkboxes-clean branch from 2cd2e8b to d2b187b Compare July 7, 2025 11:10

pr-processor Bot added Needs re-review and removed Waiting on contributor labels Jul 7, 2025

kmalyjur force-pushed the job-inv-table-checkboxes-clean branch from d2b187b to 986687a Compare July 7, 2025 12:01

kmalyjur force-pushed the job-inv-table-checkboxes-clean branch from 986687a to ae8b5d9 Compare July 8, 2025 10:03

kmalyjur requested a review from MariaAga July 9, 2025 11:34

MariaAga reviewed Jul 9, 2025

View reviewed changes

pr-processor Bot removed the Needs re-review label Jul 9, 2025

MariaAga reviewed Jul 9, 2025

View reviewed changes

kmalyjur force-pushed the job-inv-table-checkboxes-clean branch 2 times, most recently from bedde35 to aebbac4 Compare July 14, 2025 17:44

MariaAga reviewed Jul 15, 2025

View reviewed changes

MariaAga reviewed Jul 16, 2025

View reviewed changes

kmalyjur force-pushed the job-inv-table-checkboxes-clean branch from aebbac4 to 5e3883e Compare July 16, 2025 15:03

Fixes #38165 - Add checkboxes and related actions: open all, rerun, o…

a0ad547

…pen all failed

kmalyjur force-pushed the job-inv-table-checkboxes-clean branch from 5e3883e to a0ad547 Compare July 16, 2025 15:35

MariaAga approved these changes Jul 16, 2025

View reviewed changes

adamruzicka merged commit 9be3621 into theforeman:master Jul 16, 2025
13 of 16 checks passed

		import { OutlinedWindowRestoreIcon } from '@patternfly/react-icons';
		import EllipsisVIcon from '@patternfly/react-icons/dist/js/icons/ellipsis-v-icon';

Uh oh!

Conversation

kmalyjur commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamruzicka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MariaAga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmalyjur commented May 14, 2025

Uh oh!

Uh oh!

kmalyjur commented Jun 11, 2025

Uh oh!

MariaAga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmalyjur commented Jan 28, 2025 •

edited

Loading

MariaAga Jul 9, 2025 •

edited

Loading