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.

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

Merged

Conversation

jeremylenz
Copy link
Contributor

@jeremylenz jeremylenz commented Oct 26, 2023

Adds the following features to the new All Hosts page:

  • Bulk host delete added to main kebab, with modal as per mockup
  • Link directly to the exact setting for VM host delete
  • Host Create and Register buttons, which link to their respective pages
  • Links to each host now use React (much faster)
  • Links to each host now use the host name, not database ID. This means you'll see 'rhel8.example.com' as your tab title, instead of, for example, '7.'
  • also fixed spacing of main kebab
  • also fixed loading screen so it doesn't say 'No Results'

image

image

If you're using Katello, requires Katello/katello#10782

@jeremylenz
Copy link
Contributor Author

Companion Katello PR: Katello/katello#10782

@jeremylenz jeremylenz marked this pull request as ready for review October 30, 2023 21:22
@ekohl ekohl changed the title Fixes #36867 - Add host delete & create Fixes #36867 - Add host delete & create to new host overview Oct 30, 2023
@jeremylenz jeremylenz force-pushed the 36867-add-delete-and-create branch 2 times, most recently from 7062fdf to e204351 Compare November 1, 2023 18:31
@jeremylenz jeremylenz force-pushed the 36867-add-delete-and-create branch 2 times, most recently from bfbe768 to ba67f69 Compare November 2, 2023 17:00
Copy link
Contributor

@parthaa parthaa left a 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.)

app/controllers/api/v2/hosts_controller.rb Outdated Show resolved Hide resolved
const dispatch = useDispatch();
const { destroyVmOnHostDelete } = useForemanSettings();
const deleteHostHandler = ({ hostName, computeId }) =>
dispatch(deleteHost(hostName, computeId, destroyVmOnHostDelete));
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will look into it

Copy link
Contributor Author

@jeremylenz jeremylenz Nov 3, 2023

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:

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.

@jeremylenz jeremylenz force-pushed the 36867-add-delete-and-create branch 2 times, most recently from a18c0ad to c77cdce Compare November 3, 2023 16:13
@jeremylenz
Copy link
Contributor Author

@parthaa updated: Moved endpoint to new controller & fixed rubocop. 👍

  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
Copy link
Contributor

@parthaa parthaa left a comment

Choose a reason for hiding this comment

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

APJ

Copy link
Contributor

@ianballou ianballou left a 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.

@jeremylenz jeremylenz merged commit f03e699 into theforeman:develop Nov 6, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants