Skip to content

Fixes #39352 - Job inv page ignores remote_execution_job_invocation_report_template setting - #1045

Open
kmalyjur wants to merge 1 commit into
theforeman:masterfrom
kmalyjur:fix-job-invocation-report-template-setting
Open

Fixes #39352 - Job inv page ignores remote_execution_job_invocation_report_template setting#1045
kmalyjur wants to merge 1 commit into
theforeman:masterfrom
kmalyjur:fix-job-invocation-report-template-setting

Conversation

@kmalyjur

Copy link
Copy Markdown
Contributor

New job invocation page hardcoded 'Job - Invocation Report' instead of reading the remote_execution_job_invocation_report_template setting. This fixes it by fetching the setting value via /api/settings/remote_execution_job_invocation_report_template and using it to search for the template. Falls back to the default if the setting fetch fails.

@kmalyjur
kmalyjur force-pushed the fix-job-invocation-report-template-setting branch from 030d504 to 427cc0a Compare July 16, 2026 18:31
@kmalyjur
kmalyjur marked this pull request as draft July 16, 2026 18:33
@kmalyjur
kmalyjur force-pushed the fix-job-invocation-report-template-setting branch from 427cc0a to 77e71ac Compare July 17, 2026 14:12
@kmalyjur
kmalyjur force-pushed the fix-job-invocation-report-template-setting branch from 304e8d7 to 2c1c592 Compare July 17, 2026 14:54
@kmalyjur
kmalyjur marked this pull request as ready for review July 17, 2026 19:48
@adamruzicka

Copy link
Copy Markdown
Contributor

Could we leave the lint fixing to #1043 ? Right now both PRs try to fix that, but with subtle differences.

@kmalyjur
kmalyjur force-pushed the fix-job-invocation-report-template-setting branch from 2c1c592 to 77e71ac Compare July 22, 2026 12:22
);
}
key: GET_REPORT_TEMPLATE_SETTING,
url: '/api/settings/remote_execution_job_invocation_report_template',

@Lukshio Lukshio Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, but hopefully @adamruzicka will check that.

I think that user needs view_settings permission to access this endpoint, so basically all "normal" users won't be able to get the response. So we should change backend to return correct template so generate_report_templates permission will be sufficient

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that user needs view_settings permission to access this endpoint, so basically all "normal" users won't be able to get the response

That's right, good catch.

So we should change backend to return correct template

Return correct template from where?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant adding something like this to job_invocations/base.json.rabl

if params.key?(:include_permissions)
  node :report_template do |invocation|
    next unless User.current&.can?(:generate_report_templates)
    template = job_report_template
    next unless template
    job_id_input = template.template_inputs.find_by(name: 'job_id')
    next unless job_id_input
    {
      id: template.id,
      job_id_input_id: job_id_input.id,
    }
  end
end

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding what we need to a place where it would be convenient is tempting, but it doesn't really belong there.

What if we introduced a new api endpoint like /api/v2/job_invocations/:id/report which would under the hood render the right template from settings, without ever leaking the setting value to the frontend? Frontend would just check if the user has the generate_report_templates permission, clicking the button would call that endpoint. There would still be some slightly rougher edges, but probably still better than bypassing permissions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I think we can do it like that, do you agree @kmalyjur?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants