-
Notifications
You must be signed in to change notification settings - Fork 994
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.
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
Fixes #36867 - Add host delete & create to new host overview #9878
Fixes #36867 - Add host delete & create to new host overview #9878
Conversation
3cc493c
to
a879797
Compare
c19b11c
to
d6a8e46
Compare
92a964c
to
1898c26
Compare
Companion Katello PR: Katello/katello#10782 |
7062fdf
to
e204351
Compare
bfbe768
to
ba67f69
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Over all happy with the approach and output.
I just had a couple of questions relating to overloading controller (which is not a requirement but ideal.)
const dispatch = useDispatch(); | ||
const { destroyVmOnHostDelete } = useForemanSettings(); | ||
const deleteHostHandler = ({ hostName, computeId }) => | ||
dispatch(deleteHost(hostName, computeId, destroyVmOnHostDelete)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This per row delete operation seems to return me to /hosts/
while bulk delete returns me to /new/hosts
after deleting. Do you think this should be fixed in #9879
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will look into it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I reused the single delete action from host details, which actually appears to have the same (wrong) behavior. It's here:
foreman/webpack/assets/javascripts/react_app/components/HostDetails/ActionsBar/actions.js
Line 48 in 38aa4bb
handleSuccess: () => visit(foremanUrl('/hosts')), |
I think #9879 is indeed the best place to fix it. In addition to the Ruby changes there, should do an audit in JS files of wherever we have foremanUrl('/hosts')
(I only found 4). In those places we can useForemanSettings()
to get the setting and set the url accordingly.
a18c0ad
to
c77cdce
Compare
@parthaa updated: Moved endpoint to new controller & fixed rubocop. 👍 |
c77cdce
to
2999007
Compare
7f4544e
to
8537e5a
Compare
Add bulk modal with bulk params add register/create buttons; fix links address UX comments Remove icon from delete action in the toolbar’s kebab In the delete modal as a primary button use just “Delete” (not delete host) To the top part: Add a kebab with legacy UI button Ensure the loading screen doesn't say 'No Results' support foreman_remote_execution slot Refs #36867 - move action to new controller move bulk hosts extension test to Foreman
8537e5a
to
90fce22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
APJ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a quick look, looks fine to me, acking based on @parthaa 's testing.
Adds the following features to the new All Hosts page:
If you're using Katello, requires Katello/katello#10782