Fixes #39253 - Implement "Not Found" error handling for invalid Job I… by pondrejk · Pull Request #1030 · theforeman/foreman_remote_execution

pondrejk · 2026-04-24T08:04:28Z

…nvocation IDs

SAT-44544

adamruzicka

Some nitpicks, seems to work well.

adamruzicka · 2026-04-27T11:19:40Z

+          <EmptyStateBody>
+            {sprintf(
+              __(
+                'There is no job invocation with id %s or there are access permissions needed. Please contact your administrator if this issue continues.'


From the top of my head, I could envision four error cases that can happen:

The user doesn't have permission to view job invocations at all (backend should give 403 unauthorized)

The user doesn't have permission to view this particular job invocation (backend should give 404)

The user has permission to view, but the job invocation doesn't exist (backend should give 404)

The backend failing to process the request (think 500 ISE)

Should we distinguish 1 vs 2+3 vs 4?

With that being said, the old page showed "Not found" for all cases, except 4 so feel free treat this as optional.

Would it make sense to include the backend message to the page? It could be more informative without us needing to introduce some parsing logic, wdyt?

Not sure if the backend says anything reasonable, but worth a try.

In some cases the backend adds a nice message like for case 3 »Resource job_invocation not found by id '88'«

adamruzicka · 2026-04-27T11:19:59Z

+          <EmptyStateBody>
+            {sprintf(
+              __(
+                'There is no job invocation with id %s or there are access permissions needed. Please contact your administrator if this issue continues.'


Since we know what the permissions required are, should we list them as we do on the unauthorized page?

ofedoren · 2026-04-27T14:47:23Z

+
+const jobInvocationsIndexPath = '/job_invocations';
+
+const JobInvocationEmptyState = ({ jobInvocationId }) => {


AFAIR we have some patterns in core:

EmptyState: https://github.com/theforeman/foreman/tree/develop/webpack/assets/javascripts/react_app/components/common/EmptyState

PermissionDenied: https://github.com/theforeman/foreman/tree/develop/webpack/assets/javascripts/react_app/components/PermissionDenied

Should we re-use them instead of implementing a brand new specific page for it? I mean if they are not feasible, that's okay, I'm just asking if it was considered? I might lag behind our UI changes, so not sure if those aren't marked for future removal.

I did consider them, but found them a bit too restrictive. Otoh, if its worth it I can also extend them based on what we agree upon here

What about them was too restrictive? might be worth to extend them as you said

pondrejk · 2026-05-12T13:45:03Z

@ofedoren @MariaAga @adamruzicka since the last time:

I changed thing so that the EmptyStatePattern from core is used
I added a note about needed perms
I display also the message from server

ofedoren

It's more a high level review, I'd rather leave the React stuff for someone else :)

One of the reasons why I wanted to re-use or enhance and re-use something from Foreman is because we're still trying to unify the UI across the pages at some degree.

Looking at the recent change to models page in Foreman and here I see a bunch of common things, essentially a lot of repetitions, which makes it harder to unify in the future if we'd need some changes. Anyway, at least now these two pages have at least different strings:

It just feels odd :/

We'd rather add tests, especially since we can use AI for that, just make sure it actually tests something.
There is yet another custom error formatter. And AFAIU there is an idea of creating a general formatter that can be re-used then in other places. Should we wait for that or at least mark somehow to revisit this one and replace after the general/common one is available.
Some inline comments.

ofedoren · 2026-05-14T12:30:25Z

+
+const jobInvocationsIndexPath = '/job_invocations';
+
+const JobInvocationEmptyState = ({ jobInvocationId, errorMessage = null }) => {


We already use defaultProps for that

Suggested change

const JobInvocationEmptyState = ({ jobInvocationId, errorMessage = null }) => {

const JobInvocationEmptyState = ({ jobInvocationId, errorMessage }) => {

pondrejk · 2026-05-15T09:32:46Z

Looking at the recent change to models page in Foreman and here I see a bunch of common things, essentially a lot of repetitions, which makes it harder to unify in the future if we'd need some changes.

Well I thought the point of these patterns was to declare the overall shape of things for the implementations to fill in their own content that could be site specific. To me they look fairly unified, I can extend the foreman pattern, but then we'd need to agree on which parts should be unified and which should stay modifiable

pondrejk · 2026-05-15T14:28:28Z

this is now dependent on theforeman/foreman#10986

adamruzicka

This will also require a required foreman version bump in engine.rb, ideally as a separate commit.

pondrejk · 2026-06-09T13:44:05Z

This will also require a required foreman version bump in engine.rb, ideally as a separate commit.

Hi, opened #1039

MariaAga · 2026-06-09T16:04:02Z

+        onClick: () => visit(foremanUrl(jobInvocationsIndexPath)),
+        ouiaId: 'job-invocation-empty-state-go-to-job-invocations-button',
+      }}
+      secondaryActions={[
+        {
+          label: __('Create a new job invocation'),
+          onClick: () => visit(foremanUrl('/job_invocations/new')),


It seems that ResourceLoadFailedEmptyState accepts urls, and uses react-router for the actions (which is faster than visit), could the onClicks be replaced with a url?

I tried it out but didn't work for me, maybe because /job_invocations is still a rails index page?

In what way did it not work? I tried it out locally and using

primaryAction={{ label: __('Go to job invocations'), url: foremanUrl(jobInvocationsIndexPath), ouiaId: 'job-invocation-empty-state-go-to-job-invocations-button', }} secondaryActions={[ { label: __('Create a new job invocation'), url: foremanUrl('/job_invocations/new'), ouiaId: 'job-invocation-empty-state-create-new-job-invocation-button', }, ]}

Works, and navigates

right, works for me to :) honestly, I don't know what was the issue last time I tried, sorry for the confusion

MariaAga · 2026-06-09T16:10:23Z

+  const { status: permissionsApiStatus } = useAPI(
+    'get',
+    currentPermissionsUrl,
+    {
+      key: CURRENT_PERMISSIONS,
+    }
+  );


I do think its good to check if the permissions api failed, but I think this logic should be moved to core (in a new PR, unrelated to this one)

I'm not sure how to go about this, since permissionsApiStatus is one of the things we use to find what errorMessage to send to the ResourceLoadFailedEmptyState. Can you suggest where should I put that logic in core and how I'd trigger it?

Sorry my bad, I rechecked, and the data from currentPermissionsUrl is already in useForemanPermissions in the empty state, so no need to call it at all.

Foreman ui meta data (which goes to useForemanPermissions)
permissions: (User.current.admin? ? Permission.all : User.current.permissions).pluck(:name),
https://github.com/theforeman/foreman/blob/07ce9438d216cf7d1cef3763de66b83f22669c16/app/helpers/application_helper.rb#L422

api call to currentPermissionsUrl
@current_permissions = @user.admin? ? Permission.all : @user.permissions
https://github.com/theforeman/foreman/blob/07ce9438d216cf7d1cef3763de66b83f22669c16/app/controllers/api/v2/permissions_controller.rb#L32

makes sense, so I removed those currentPermissionsUrl calls

MariaAga

Since the empty state unloads the job invocation page when the api is done, with an error, it causes some components (JobInvocationToolbarButtons) to unload to fast and cause console warnings. I think this could be fixed by doing something like toolbarButtons={ jobInvocationApiStatus === API_STATUS.SUCCESS && <JobInvocationToolbarButtons jobId={id} data={items} />}

pondrejk · 2026-06-23T11:29:42Z

Since the empty state unloads the job invocation page when the api is done, with an error, it causes some components (JobInvocationToolbarButtons) to unload to fast and cause console warnings. I think this could be fixed by doing something like toolbarButtons={ jobInvocationApiStatus === API_STATUS.SUCCESS && <JobInvocationToolbarButtons jobId={id} data={items} />}

thanks for noticing, I added your suggestion, but I had to also add some isMounted conditions to JobInvocationToolbarButtons for the console warning to stop

pondrejk · 2026-06-25T12:04:34Z

@MariaAga Hi, I think I've addressed all the comments, can you plrease take a look again?

MariaAga

I ran an AI review, and it caught that with the api permisisons removal, the toolbar and the actions, dont get to see the correct permissions, so it will be good to switch out the permissions checks in webpack/JobInvocationDetail/CheckboxesActions.js and webpack/JobInvocationDetail/JobInvocationToolbarButtons.js with the usePermissions hook. (this causes the buttons next to the search bar to always be disabled)

Also missing a test to make sure the empty state loads.

MariaAga · 2026-06-29T12:40:39Z

        breadcrumbOptions={breadcrumbOptions}
-        toolbarButtons={<JobInvocationToolbarButtons jobId={id} data={items} />}
+        toolbarButtons={
+          jobInvocationApiStatus === API_STATUS.SUCCESS && (


The STATUS constant from foremanReact/constants has keys PENDING, RESOLVED, and ERROR — there is no SUCCESS. This comparison always evaluates to false, so
JobInvocationToolbarButtons is never rendered regardless of API state. Should be API_STATUS.RESOLVED.

MariaAga · 2026-06-29T12:42:26Z

+    return null;
+  }, [jobInvocationApiStatus, jobInvocationErrorMessage]);
+
+  const httpStatus = jobInvocationHttpStatus;


Used once, no transformation. Pass jobInvocationHttpStatus directly to the prop.

MariaAga · 2026-06-29T12:42:40Z

+export const visit = jest.fn(url => {
+  global.window.location.href = url;
+});


MariaAga · 2026-06-29T12:44:53Z

+import { foremanUrl } from 'foremanReact/common/helpers';
+import ResourceLoadFailedEmptyState from 'foremanReact/components/common/EmptyState/ResourceLoadFailedEmptyState';
+
+const jobInvocationsIndexPath = '/job_invocations';


should live in JobInvocationConstants.js alongside jobInvocationDetailsUrl.

pondrejk · 2026-07-08T09:13:42Z

I ran an AI review, and it caught that with the api permisisons removal, the toolbar and the actions, dont get to see the correct permissions, so it will be good to switch out the permissions checks in webpack/JobInvocationDetail/CheckboxesActions.js and webpack/JobInvocationDetail/JobInvocationToolbarButtons.js with the usePermissions hook. (this causes the buttons next to the search bar to always be disabled)

Also missing a test to make sure the empty state loads.

Hi @MariaAga, I did the requested updates, please take a look when you have time

MariaAga

Thanks! PR logic works as expected, but needs some code changes

We prefer to test what the user sees, rather than relying on test ids, so screen.getByTestId( 'job-invocation-empty-state-go-to-job-invocations-button' ) should be changed to things like screen.getByRole('button', { name: 'Create a new job invocation' })

You can read more about testing guidelines here: https://github.com/theforeman/foreman/blob/develop/developer_docs/ui-testing-guidelines.asciidoc

MariaAga · 2026-07-08T13:24:11Z


+jest.mock('foremanReact/common/hooks/Permissions/permissionHooks', () => ({
+  usePermissions: jest.fn(() => true),
+}));


If the mock is the same as the mock file, no need to mock it again

MariaAga · 2026-07-08T13:24:16Z

+jest.mock('foremanReact/common/hooks/Permissions/permissionHooks', () => ({
+  usePermissions: jest.fn(() => true),
+}));


If the mock is the same as the mock file, no need to mock it again

MariaAga · 2026-07-08T13:28:06Z

+jest.mock(
+  'foremanReact/components/common/EmptyState/ResourceLoadFailedEmptyState',
+  () => ({
+    __esModule: true,
+    default: ({
+      resourceLabel,
+      resourceId,
+      errorMessage,
+      primaryAction,
+      secondaryActions,
+      ouiaIdPrefix,
+    }) => (
+      <div data-testid={ouiaIdPrefix}>
+        <h1>{`Unable to load ${resourceLabel}`}</h1>
+        <p>{`The ${resourceLabel} with id ${resourceId} could not be found.`}</p>
+        {errorMessage && <p>{`Server returned: ${errorMessage}`}</p>}
+        <button type="button" data-testid={primaryAction.ouiaId}>
+          {primaryAction.label}
+        </button>
+        {secondaryActions.map(action => (
+          <button key={action.ouiaId} type="button" data-testid={action.ouiaId}>
+            {action.label}
+          </button>
+        ))}
+      </div>
+    ),
+  })
+);


Prefer just relying on the original component, rather than mocking it.

MariaAga · 2026-07-08T13:29:05Z

+jest.mock(
+  'foremanReact/components/common/EmptyState/ResourceLoadFailedEmptyState',
+  () => ({
+    __esModule: true,
+    default: ({
+      resourceLabel,
+      resourceId,
+      errorMessage,
+      primaryAction,
+      ouiaIdPrefix,
+    }) => (
+      <div data-testid={ouiaIdPrefix}>
+        <h1>{`Unable to load ${resourceLabel}`}</h1>
+        <p>{`The ${resourceLabel} with id ${resourceId} could not be found.`}</p>
+        {errorMessage && <p>{`Server returned: ${errorMessage}`}</p>}
+        <button type="button" data-testid={primaryAction.ouiaId}>
+          {primaryAction.label}
+        </button>
+      </div>
+    ),
+  })
+);
+


Prefer just relying on the original component, rather than mocking it.

MariaAga · 2026-07-08T13:43:24Z

@@ -50,6 +58,33 @@ jest.mock('foremanReact/common/hooks/API/APIHooks', () => ({
  })),
 }));


useAPI is no longer used

MariaAga · 2026-07-08T13:51:17Z

  templateInvocationPageUrl,
 } from './JobInvocationConstants';
-import {
-  selectHasPermission,


Since this is no longer used, it can be removed from JobInvocationSelectors

MariaAga · 2026-07-08T13:52:25Z

-  CURRENT_PERMISSIONS,
  DATE_OPTIONS,
  JOB_INVOCATION_KEY,
  STATUS,
  STATUS_UPPERCASE,
-  currentPermissionsUrl,


Since this (currentPermissionsUrl,CURRENT_PERMISSIONS) is no longer used, it can be removed from JobInvocationConstants

MariaAga · 2026-07-08T13:53:17Z

+        url: foremanUrl(jobInvocationsIndexPath),
+        ouiaId: 'job-invocation-empty-state-go-to-job-invocations-button',
+      }}
+      secondaryActions={[
+        {
+          label: __('Create a new job invocation'),
+          url: foremanUrl('/job_invocations/new'),


both urls should be consistent, and the foremanUrl can be moved to the consts

MariaAga · 2026-07-09T09:20:13Z

 export const selectCurrentPermisions = state =>
  selectAPIResponse(state, CURRENT_PERMISSIONS);


can also be removed since its not used, and CURRENT_PERMISSIONS is undefined.

MariaAga · 2026-07-09T11:54:08Z

I started testing that the page still works, but now when a job is running the buttons (create report, rerun all) are flashing, and it didnt seem to happen without the pr

Screencast.From.2026-07-09.12-52-16.mp4

…nvocation IDs

pondrejk · 2026-07-10T13:41:11Z

I started testing that the page still works, but now when a job is running the buttons (create report, rerun all) are flashing, and it didnt seem to happen without the pr

great chatch! I did some update to fix that with ai assistance, I'm posting it as a separate commit for reviewability

MariaAga

Thanks! code lgtm, and I tested it

pr-processor Bot added the Not yet reviewed label Apr 24, 2026

pondrejk force-pushed the invocations-empty-page branch from 3cf4949 to 412d2fe Compare April 24, 2026 08:13

adamruzicka reviewed Apr 27, 2026

View reviewed changes

pr-processor Bot removed the Not yet reviewed label Apr 27, 2026

ofedoren reviewed Apr 27, 2026

View reviewed changes

MariaAga self-assigned this Apr 28, 2026

pondrejk force-pushed the invocations-empty-page branch 2 times, most recently from 236a8c6 to afbb4da Compare May 12, 2026 13:33

ofedoren reviewed May 14, 2026

View reviewed changes

pondrejk mentioned this pull request May 15, 2026

Fixes #39395 - reusable empty state page theforeman/foreman#10986

Merged

adamruzicka requested changes Jun 1, 2026

View reviewed changes

pr-processor Bot added the Waiting on contributor label Jun 1, 2026

pondrejk force-pushed the invocations-empty-page branch from 0a17018 to da1c2b0 Compare June 4, 2026 07:35

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

pondrejk force-pushed the invocations-empty-page branch from da1c2b0 to fa8441c Compare June 8, 2026 13:27

pondrejk mentioned this pull request Jun 9, 2026

Refs #39253 - Bump requires_foreman to 5.0 #1039

Merged

MariaAga requested changes Jun 9, 2026

View reviewed changes

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

MariaAga requested changes Jun 18, 2026

View reviewed changes

pondrejk force-pushed the invocations-empty-page branch from fa8441c to 8f0283f Compare June 23, 2026 11:26

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

pr-processor Bot removed the Needs re-review label Jun 23, 2026

pondrejk force-pushed the invocations-empty-page branch from 8f0283f to 5ce73bc Compare June 25, 2026 11:30

MariaAga requested changes Jun 29, 2026

View reviewed changes

pr-processor Bot added the Waiting on contributor label Jun 29, 2026

pondrejk force-pushed the invocations-empty-page branch from 5ce73bc to aaa3633 Compare July 1, 2026 12:48

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

MariaAga requested changes Jul 8, 2026

View reviewed changes

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

pondrejk force-pushed the invocations-empty-page branch from aaa3633 to 95d9efa Compare July 8, 2026 17:32

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

MariaAga reviewed Jul 9, 2026

View reviewed changes

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

pondrejk force-pushed the invocations-empty-page branch from 95d9efa to 4f79c5f Compare July 9, 2026 10:53

Lukshio mentioned this pull request Jul 9, 2026

Fixes #39494 - Fix create report button is enabled when state is 'running' #1041

Merged

pondrejk added 2 commits July 10, 2026 08:48

Fixes #39253 - Implement "Not Found" error handling for invalid Job I…

e7c3574

…nvocation IDs

Fixes #39253 - fix toolbar buttons remount

ec5e58e

pondrejk force-pushed the invocations-empty-page branch from 0a770c2 to ec5e58e Compare July 10, 2026 12:48

MariaAga approved these changes Jul 13, 2026

View reviewed changes

MariaAga merged commit 61739ea into theforeman:master Jul 13, 2026
17 checks passed


		const jobInvocationsIndexPath = '/job_invocations';

		const JobInvocationEmptyState = ({ jobInvocationId }) => {

		@@ -50,6 +58,33 @@ jest.mock('foremanReact/common/hooks/API/APIHooks', () => ({
		})),
		}));

		export const selectCurrentPermisions = state =>
		selectAPIResponse(state, CURRENT_PERMISSIONS);

Uh oh!

Conversation

pondrejk commented Apr 24, 2026

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!

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!

pondrejk commented May 12, 2026

Uh oh!

ofedoren left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pondrejk commented May 15, 2026

Uh oh!

pondrejk commented May 15, 2026

Uh oh!

adamruzicka left a comment

Choose a reason for hiding this comment

Uh oh!

pondrejk commented Jun 9, 2026

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!

pondrejk Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

pondrejk commented Jun 23, 2026

Uh oh!

pondrejk commented Jun 25, 2026

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!

pondrejk commented Jul 8, 2026

Uh oh!

MariaAga left a comment

Choose a reason for hiding this comment

Uh oh!

pondrejk Jun 23, 2026 •

edited

Loading