Fixes #38306 - dont load all outputs in job details

[MariaAga]

Created list_template_invocations_task_by_hosts so we can still see the kebab actions for all hosts without needed to expend anything

[adamruzicka]

As far as the functionality goes, I have no concerns, although it could use some polishing.

[adamruzicka]

This would find all the hosts which match the search, not just the hosts which are in the job. That could get out of hand very quickly

[adamruzicka]

We could be more specific and instead of asking "Can the user view tasks in general?" we could ask "Can the user view this host's task?".

Suggested change

	:view_foreman_tasks => User.current.allowed_to?(:view_foreman_tasks),
	:view_foreman_tasks => User.current.can?(:view_foreman_tasks, template_invocation_task,

Alternatively, in the old ui we use this

authorized_for(:permission => :view_foreman_tasks, :auth_object => task)

[adamruzicka]

Same thing as above, in the old ui we use

authorized_for(:permission => :cancel_job_invocations, :auth_object => job_invocation)

[adamruzicka]

disclaimer: I know there's a precedent for this kind of naming and I've approved the previous instance in the past, but back then we were under time pressure while now we have time.

Our api tries (and usually succeeds) in being REST-ful. Among other things, there are certain expectations about how routes should be formed. The rails routing guide[1] is a nice introduction.

In this case, template invocations cannot exist on their own so the path to get them shouldn't be top-level. They always have to belong to a job invocation, so endpoints for retrieving information about template invocations should be nested under a path which determines a job invocation, so something like /job_invocations/<JID>/template_invocations?search=.... If I'm reading it correctly then in here you're passing the job invocation id as a parameter, which also works, but still the pat-based approach would be preferable.

Additionally, I'm afraid users don't really care about template invocations, they care about "hosts in a job" and maybe it wouldn't hurt to indulge them in that regard. Maybe something like /job_invocations/<JID>/hosts?search=... would be a better fit? The answer would include all the task-level details and permissions and so on, without exposing our internals too much. Also, by mentioning "hosts" in the path, you set expectations about the search.

[1] - https://guides.rubyonrails.org/routing.html

[ofedoren]

Even though I agree with everything written here, is that necessary in this case? I guess you also agreed on a similar case before just because it's not an API endpoint? I mean, users still might use that, but it won't be explicitly "marketed" via API docs or something like a direct page, where you could copy/save the link from the browser.

On the other hand, if we want to explicitly expose this action to the users, I'd vote for a proper API endpoint, if not, this seems okay?..

[adamruzicka]

is that necessary in this case?

Was that meant to react to my whole comment or just parts of it? None of it is strictly necessary and the last paragraph probably falls into the nice-to-have bucket, but we should at least be consistent, even if just for our own sake.

I guess you also agreed on a similar case before just because it's not an API endpoint?

That's true, but I'd also say we can do better if we have time?

if we want to explicitly expose this action to the users, I'd vote for a proper API endpoint

I wasn't proposing to make this a "proper, user-facing and stable" api endpoint. But it is still an api endpoint of sorts, even if it is just frontend-facing and sort of private.

this seems okay?

Well, it works so there's that, but being a bit more consistent would be nice.

[ofedoren]

Nevermind me, somehow with all these workarounds for UI I forgot to follow Rails' how-tos

[MariaAga]

Thats for the explanations, is the issue just with the url or also with the method name list_template_invocations_task_by_hosts ?

[MariaAga]

re-remembering this pr:
/job_invocations/<JID>/hosts? already exists, and gives info about the host and some of its details, this endpoint only gives a list of hosts ids and their task ids

The answer would include all the task-level details and permissions and so on, without exposing our internals too much.

Could you explain what you mean here?

[adamruzicka]

is the issue just with the url or also with the method name list_template_invocations_task_by_hosts ?

I would say both. Usually the route and the corresponding method are named in a similar way so if we change one, we should also change the other.

/job_invocations/<JID>/hosts? already exists

Where? I'm aware of /api(/:apiv)/job_invocations/:id/hosts, but that's slightly different.

Could you explain what you mean here?

Something along the lines of this. Just don't explicitly call out template invocations in there

{
  "result": {
    "hosts": [
      { "hostname": "foo.example.com",
        "id": 7,
        "permissions": { "view_task": true, "cancel_job": true, "execute_job": true },
        "task": { "cancellable": true, id: "12345-..." }
      },
      ...
    ]
  }
}

[MariaAga]

@kmalyjur Can you review the ui part of this?

[kmalyjur]

The UI part is great, thank you.
@adamruzicka, I think it can be merged?

[kmalyjur]

Hi @adamruzicka @MariaAga, just checking in on this PR - any updates?

[adamruzicka]

If I expand multiple rows, only the last one that I expand keeps refreshing, the others just stop refreshing. I have a feeling that #978 might help with that. If that is the case, then I'd rather have the other pr merged first.

[Copilot]

Pull Request Overview

This PR refactors job details output loading by switching from the useAPI hook to redux selectors for handling template invocations and introduces a new API endpoint to list template invocations by host. Key changes include:

• Replacing direct API hook usage with redux selectors for template invocation data and status.
• Adding new selectors, constants, routes, and controller actions for listing template invocations.
• Updating tests and UI components to accommodate the new data flow.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
webpack/JobInvocationDetail/tests/TemplateInvocation.test.js	Replaced APIHooks mocking with redux selectors for controlling invocation status and data in tests.
webpack/JobInvocationDetail/TemplateInvocationPage.js	Updated the selector usage to accept hostID as a parameter.
webpack/JobInvocationDetail/TemplateInvocationComponents/TemplateActionButtons.js	Changed from the single invocation selector to a list selector and updated task structure extraction; minor spelling issues in toast messages.
webpack/JobInvocationDetail/TemplateInvocation.js	Removed useAPI in favor of redux selectors; added isExpanded prop and refactored task extraction, but potential issues with the intervalId variable and default value for finished.
webpack/JobInvocationDetail/JobInvocationSelectors.js	Updated the template invocation selector to be hostID parameterized and added a new selector for the list of template invocations.
webpack/JobInvocationDetail/JobInvocationHostTable.js	Introduced a new API call for hosts and passed isExpanded down to the TemplateInvocation component.
webpack/JobInvocationDetail/JobInvocationConstants.js	Added a new constant for listing template invocations.
lib/foreman_remote_execution/engine.rb	Updated permissions to support new endpoints for job invocations and template invocations.
config/routes.rb	Added a new route for listing job hosts.
app/controllers/template_invocations_controller.rb	Updated JSON render response to include the merged task attributes.
app/controllers/job_invocations_controller.rb	Introduced a new action to return hosts with associated template invocations.

Comments suppressed due to low confidence (3)

webpack/JobInvocationDetail/TemplateInvocationComponents/TemplateActionButtons.js:87

• There is a typo in the success toast message: 'succesfully' should be spelled 'successfully'.

successToast: () => __('Task for the host cancelled succesfully'),

webpack/JobInvocationDetail/TemplateInvocationComponents/TemplateActionButtons.js:105

• There is a typo in the abort toast message: 'succesfully' should be spelled 'successfully'.

successToast: () => __('task aborted succesfully'),

webpack/JobInvocationDetail/TemplateInvocation.js:87

• The variable 'intervalId' is used but not declared in the component scope; this may lead to unexpected errors. Consider declaring intervalId using a ref (e.g., useRef) to properly manage its lifecycle.

if (intervalId) clearInterval(intervalId);

[adamruzicka]

Needs a rebase

[MariaAga]

rebased, added a logic ((Object.keys(response).length === 0 || autoRefresh)) so it wont do an api call again if a user expands, un-expands and expand the same row again

[ofedoren]

Thanks, @MariaAga, just two ignorable nits, but otherwise LGTM and seems to work as expected.

@adamruzicka, any concerns?

[ofedoren]

Is it okay to define a function in useEffect hook?..

[MariaAga]

it is recommended

[adamruzicka]

Needs yet another rebase

[adamruzicka]

On my end it doesn't seem to be refreshing. When I expand the row, the data gets loaded once, but that's it. To get a fresh view I have to collapse the row and expand it again

[MariaAga]

rebased, @adamruzicka I think this is what youre describing? https://issues.redhat.com/browse/SAT-35992
Its out of scope for this pr probably

[adamruzicka]

I think this is what youre describing?

That doesn't sound like it. Furthermore, the issue I was having went away when I applied this suggestion

[MariaAga]

updated

[adamruzicka]

Thank you @MariaAga !