Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #38123 - add template invocation info to new job details #938

Merged
merged 1 commit into from
Jan 28, 2025
Merged

Fixes #38123 - add template invocation info to new job details #938

merged 1 commit into from
Jan 28, 2025

Conversation

MariaAga
Copy link
Member

@MariaAga MariaAga commented Jan 6, 2025

No description provided.

@MariaAga
Copy link
Member Author

MariaAga commented Jan 6, 2025

Needs either a foreman table pr, or to change to use pf4 table

@MariaAga
Copy link
Member Author

MariaAga commented Jan 6, 2025

@adamruzicka Could you review the back end? (the comment about the foreman table - the ui is working just a bit squished without it)

adamruzicka
adamruzicka reviewed Jan 14, 2025
View reviewed changes
Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

Oh, looks like I forgot to submit this, sorry about the delay

app/controllers/api/v2/template_invocations_controller.rb Outdated Show resolved Hide resolved
app/controllers/api/v2/template_invocations_controller.rb Outdated Show resolved Hide resolved
app/controllers/api/v2/template_invocations_controller.rb Outdated Show resolved Hide resolved
app/controllers/api/v2/template_invocations_controller.rb Outdated Show resolved Hide resolved
app/controllers/api/v2/template_invocations_controller.rb Outdated Show resolved Hide resolved
app/controllers/api/v2/template_invocations_controller.rb Outdated Show resolved Hide resolved
@MariaAga MariaAga force-pushed the job-details-expend-host branch from cfc8ce7 to 79f9833 Compare January 20, 2025 15:36
@MariaAga MariaAga marked this pull request as ready for review January 21, 2025 12:38
@MariaAga
Copy link
Member Author

Created theforeman/foreman#10419 for this PR

MariaAga
MariaAga commented Jan 22, 2025
View reviewed changes
webpack/JobInvocationDetail/TemplateInvocationComponents/TemplateActionButtons.js
import { addToast } from 'foremanReact/components/ToastsList';
import { translate as __ } from 'foremanReact/common/I18n';

export const TemplateActionButtons = ({
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: Move action buttons to kebab in the table, and keep as buttons in the page

Copy link
Contributor

@kmalyjur kmalyjur Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Wouldn't it be enough to have them in the kebab? It's taking up a lot of space in the expandable row. 🤔
EDIT: oh you probably mean to keep them on the page which is opened in a new tab, sorry (/job_invocations_detail/:id:/host_invocation/:id:)

@adamruzicka
Copy link
Contributor

image

The headers seem to be shifted. Maybe the expandable part is treated as a table cell?

@MariaAga
Copy link
Member Author

Depends on : theforeman/foreman#10419

@MariaAga MariaAga force-pushed the job-details-expend-host branch from 79f9833 to d0952ff Compare January 23, 2025 17:20
@MariaAga
Copy link
Member Author

Added kebabs to the table, need to still address some comments

kmalyjur
kmalyjur approved these changes Jan 23, 2025
View reviewed changes
Copy link
Contributor

@kmalyjur kmalyjur left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you very much for such a big PR. It works as it should. After resolving the tests, backend conversation, and your to-do comments, it's ready to be merged.

webpack/JobInvocationDetail/JobInvocationSelectors.js Outdated Show resolved Hide resolved
webpack/JobInvocationDetail/JobInvocationHostTable.js Show resolved Hide resolved
@adamruzicka
Copy link
Contributor

The dependency in foreman was merged, could you please bump the required foreman version?

@MariaAga MariaAga force-pushed the job-details-expend-host branch from d0952ff to 5e3ce65 Compare January 24, 2025 16:47
@MariaAga MariaAga force-pushed the job-details-expend-host branch from 5e3ce65 to 895c444 Compare January 24, 2025 16:53
@MariaAga MariaAga force-pushed the job-details-expend-host branch from 895c444 to 4dc0a88 Compare January 28, 2025 09:40
@MariaAga MariaAga force-pushed the job-details-expend-host branch from 4dc0a88 to 359c252 Compare January 28, 2025 10:56
@MariaAga MariaAga force-pushed the job-details-expend-host branch from 359c252 to fa2acbd Compare January 28, 2025 12:27
@adamruzicka adamruzicka merged commit 45bd8b8 into theforeman:master Jan 28, 2025
14 checks passed
@adamruzicka
Copy link
Contributor

Thank you @MariaAga & @kmalyjur !

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

@kmalyjur kmalyjur kmalyjur approved these changes

@adamruzicka adamruzicka adamruzicka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MariaAga @adamruzicka @kmalyjur