Fixes #36867 - Add host delete & create

  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

app/controllers/api/base_controller.rb

@@ -387,7 +387,7 @@ def action_permission
         'create'
       when 'edit', 'update'
         'edit'
-      when 'destroy'
+      when 'destroy', 'bulk_destroy'
         'destroy'
       when 'index', 'show', 'status'
         'view'

app/controllers/api/v2/hosts_bulk_actions_controller.rb

@@ -0,0 +1,35 @@
+module Api
+  module V2
+    class HostsBulkActionsController < V2::BaseController
+      include Api::Version2
+      include Api::V2::BulkHostsExtension
+
+      before_action :find_deletable_hosts, :only => [:bulk_destroy]
+
+      def_param_group :bulk_host_ids do
+        param :organization_id, :number, :required => true, :desc => N_("ID of the organization")
+        param :included, Hash, :desc => N_("Hosts to include in the action"), :required => true, :action_aware => true do
+          param :search, String, :required => false, :desc => N_("Search string describing which hosts to perform the action on")
+          param :ids, Array, :required => false, :desc => N_("List of host ids to perform the action on")
+        end
+        param :excluded, Hash, :desc => N_("Hosts to explicitly exclude in the action."\
+                                           " All other hosts will be included in the action,"\
+                                           " unless an included parameter is passed as well."), :required => true, :action_aware => true do
+          param :ids, Array, :required => false, :desc => N_("List of host ids to exclude and not perform the action on")
+        end
+      end
+
+      api :DELETE, "/hosts/bulk/", N_("Delete multiple hosts")
+      param_group :bulk_host_ids
+      def bulk_destroy
+        process_response @hosts.destroy_all
+      end
+
+      private
+
+      def find_deletable_hosts
+        find_bulk_hosts(:destroy_hosts, params)
+      end
+    end
+  end
+end

app/controllers/api/v2/hosts_controller.rb

@@ -2,6 +2,7 @@ module Api
   module V2
     class HostsController < V2::BaseController
       include Api::Version2
+      include Api::V2::BulkHostsExtension
       include ScopesPerAction
       include Foreman::Controller::SmartProxyAuth
       include Foreman::Controller::Parameters::Host

app/controllers/concerns/api/v2/bulk_hosts_extension.rb

@@ -0,0 +1,45 @@
+module Api::V2::BulkHostsExtension
+  extend ActiveSupport::Concern
+
+  def bulk_hosts_relation(permission, org)
+    relation = ::Host::Managed.authorized(permission)
+    relation = relation.where(organization: org) if org
+    relation
+  end
+
+  def find_bulk_hosts(permission, bulk_params, restrict_to = nil)
+    # works on a structure of param_group bulk_params and transforms it into a list of systems
+    bulk_params[:included] ||= {}
+    bulk_params[:excluded] ||= {}
+    search_param = bulk_params[:included][:search] || bulk_params[:search]
+
+    if !params[:install_all] && bulk_params[:included][:ids].blank? && search_param.nil?
+      render_error :custom_error, :status => :bad_request, :locals => { :message => _('No hosts have been specified') }
+    end
+
+    find_organization
+    @hosts = bulk_hosts_relation(permission, @organization)
+
+    if bulk_params[:included][:ids].present?
+      @hosts = @hosts.where(id: bulk_params[:included][:ids])
+    end
+
+    if search_param.present?
+      @hosts = @hosts.search_for(search_param)
+    end
+
+    @hosts = restrict_to.call(@hosts) if restrict_to
+
+    if bulk_params[:excluded][:ids].present?
+      @hosts = @hosts.where.not(id: bulk_params[:excluded][:ids])
+    end
+    if @hosts.empty?
+      render_error :custom_error, :status => :forbidden, :locals => { :message => _('No hosts matched search, or action unauthorized for selected hosts.') }
+    end
+    @hosts
+  end
+
+  def find_organization
+    @organization ||= Organization.find_by_id(params[:organization_id])
+  end
+end

app/registries/foreman/access_permissions.rb

@@ -275,6 +275,7 @@
                                   }
     map.permission :destroy_hosts, {:hosts => [:destroy, :multiple_actions, :reset_multiple, :multiple_destroy, :submit_multiple_destroy],
                                     :"api/v2/hosts" => [:destroy],
+                                    :"api/v2/hosts_bulk_actions" => [:bulk_destroy],
                                     :"api/v2/interfaces" => [:destroy],
                                   }
     map.permission :build_hosts,   {:hosts => [:setBuild, :cancelBuild, :multiple_build, :submit_multiple_build, :review_before_build,

config/routes/api/puppet/api/v2.rb

@@ -3,7 +3,7 @@
     # new v2 routes that point to v2
     scope "(:apiv)", :module => :v2, :defaults => {:apiv => 'v2'}, :apiv => /v2/, :constraints => ApiConstraints.new(:version => 2, :default => true) do
       constraints(:id => /[^\/]+/) do
-        resources :hosts, :except => [:new, :edit] do
+        resources :hosts, :only => [] do # only: [] to avoid adding other api/v2/hosts routes
           put :puppetrun, :on => :member, :to => 'puppet_hosts#puppetrun'
         end
       end

config/routes/api/v2.rb

@@ -3,6 +3,8 @@
   namespace :api, :defaults => {:format => 'json'} do
     # new v2 routes that point to v2
     scope "(:apiv)", :module => :v2, :defaults => {:apiv => 'v2'}, :apiv => /v2/, :constraints => ApiConstraints.new(:version => 2, :default => true) do
+      match 'hosts/bulk', :to => 'hosts_bulk_actions#bulk_destroy', :via => [:delete]
+
       resources :architectures, :except => [:new, :edit] do
         constraints(:id => /[^\/]+/) do
           resources :hosts, :except => [:new, :edit]

test/controllers/concerns/bulk_hosts_extension_test.rb

@@ -0,0 +1,168 @@
+require 'test_helper'
+
+class BulkHostsExtensionTestController < ::Api::BaseController
+  include Api::Version2
+  include ::Api::V2::BulkHostsExtension
+
+  def initialize(params = {})
+    @params = params
+  end
+
+  attr_reader :params
+end
+
+class BulkHostsExtensionTest < ActiveSupport::TestCase
+  def models
+    as_admin do
+      @organization = FactoryBot.create(:organization)
+      @host1 = FactoryBot.create(:host, :managed, :organization => @organization)
+      @host2 = FactoryBot.create(:host, :organization => @organization)
+      @host3 = FactoryBot.create(:host, :organization => @organization)
+      @host4 = FactoryBot.create(:host, :organization => @organization)
+      @host5 = FactoryBot.create(:host, :organization => @organization)
+      @host6 = FactoryBot.create(:host, :organization => @organization)
+      @host_ids = [@host1, @host2, @host3, @host4, @host5, @host6].map(&:id)
+    end
+  end
+
+  def permissions
+    @edit = :edit_hosts
+  end
+
+  def setup
+    # set_user
+    models
+    permissions
+    @controller = BulkHostsExtensionTestController.new(organization_id: @organization.id)
+  end
+
+  def test_search
+    bulk_params = {
+      :included => {
+        :search => "name = #{@host1.name}",
+      },
+    }
+    result = @controller.find_bulk_hosts(@edit, bulk_params)
+
+    assert_equal result, [@host1]
+  end
+
+  def test_search_restrict
+    bulk_params = {
+      :included => {
+        :search => "name ~ host",
+      },
+    }
+    restrict = ->(hosts) { hosts.where("id != #{@host2.id}") }
+    result = @controller.find_bulk_hosts(@edit, bulk_params, restrict)
+
+    assert_includes result, @host1
+    refute_includes result, @host2
+    assert_includes result, @host3
+  end
+
+  def test_search_exclude
+    bulk_params = {
+      :included => {
+        :search => "name ~ host",
+      },
+    :excluded => {
+      :ids => [@host1.id],
+    },
+    }
+    result = @controller.find_bulk_hosts(@edit, bulk_params)
+
+    refute_includes result, @host1
+    assert_includes result, @host2
+    assert_includes result, @host3
+  end
+
+  def test_no_hosts_specified
+    bulk_params = {
+      :included => {},
+    }
+    @controller.expects(:render_error).with(:custom_error, :status => :bad_request, :locals => { :message => _('No hosts have been specified') })
+    @controller.find_bulk_hosts(@edit, bulk_params)
+  end
+
+  def test_ids
+    bulk_params = {
+      :included => {
+        :ids => [@host1.id, @host2.id],
+      },
+    }
+    result = @controller.find_bulk_hosts(@edit, bulk_params)
+
+    assert_equal [@host1, @host2].sort, result.sort
+  end
+
+  def test_ids_excluded
+    bulk_params = {
+      :included => {
+        :ids => [@host1.id, @host2.id],
+      },
+      :excluded => {
+        :ids => [@host2.id],
+      },
+    }
+    result = @controller.find_bulk_hosts(@edit, bulk_params)
+
+    assert_equal result, [@host1]
+  end
+
+  def test_ids_restricted
+    bulk_params = {
+      :included => {
+        :ids => [@host1.id, @host2.id],
+      },
+    }
+    restrict = ->(hosts) { hosts.where("id != #{@host2.id}") }
+    result = @controller.find_bulk_hosts(@edit, bulk_params, restrict)
+
+    assert_equal result, [@host1]
+  end
+
+  def test_included_ids_with_nil_scoped_search
+    bulk_params = {
+      :included => {
+        :ids => [@host1.id, @host2.id],
+        :search => nil,
+      },
+    }
+
+    result = @controller.find_bulk_hosts(@edit, bulk_params)
+
+    assert_equal [@host1, @host2].sort, result.sort
+  end
+
+  def test_ids_with_scoped_search
+    bulk_params = {
+      :included => {
+        :ids => [@host1.id, @host2.id],
+        :search => "name != #{@host2.name}",
+      },
+    }
+
+    result = @controller.find_bulk_hosts(@edit, bulk_params)
+
+    assert_equal result, [@host1]
+  end
+
+  def test_forbidden
+    bulk_params = {
+      :included => {
+        :ids => [@host1.id],
+      },
+      :excluded => {
+        :ids => [@host1.id],
+      },
+    }
+    @controller.expects(:render_error).with(:custom_error, :status => :forbidden, :locals => { :message => _('No hosts matched search, or action unauthorized for selected hosts.') })
+    @controller.find_bulk_hosts(@edit, bulk_params)
+  end
+
+  def test_empty_params
+    @controller.expects(:render_error).with(:custom_error, :status => :bad_request, :locals => { :message => _('No hosts have been specified') })
+    @controller.find_bulk_hosts(@edit, {})
+  end
+end

test/integration/host_js_test.rb

@@ -112,7 +112,7 @@ class HostJSTest < IntegrationTestWithJavascript
       visit host_details_page_path(host)
       find('#hostdetails-kebab').click
       click_button 'Delete'
-      click_button 'Delete host'
+      find('button.pf-c-button.pf-m-danger').click # the red delete button, not the menu item
       assert_current_path hosts_path
       assert_raises(ActiveRecord::RecordNotFound) do
         Host.find(host.id)

test/test_helper.rb

@@ -129,6 +129,10 @@ def assert_sql_queries(num_of_queries, match = /SELECT/)
     ActiveSupport::Notifications.unsubscribe("sql.active_record")
     assert_equal num_of_queries, queries.size, "Expected #{num_of_queries} queries, but got #{queries.size}"
   end
+
+  def assert_equal_arrays(array1, array2)
+    assert_equal array1.sort, array2.sort
+  end
 end
 
 class ActionView::TestCase

webpack/assets/javascripts/react_app/components/ConfirmModal/index.js

@@ -1,11 +1,12 @@
-import React from 'react';
+import React, { useState } from 'react';
 import { useDispatch, useSelector } from 'react-redux';
-import { Modal, Button, ModalVariant } from '@patternfly/react-core';
+import { Modal, Button, ModalVariant, Checkbox } from '@patternfly/react-core';
 import { translate as __ } from '../../common/I18n';
 import { closeConfirmModal, selectConfirmModal } from './slice';
 
 const ConfirmModal = () => {
   const {
+    id,
     isOpen,
     title,
     message,
@@ -14,11 +15,16 @@ const ConfirmModal = () => {
     onCancel,
     modalProps,
     isWarning,
+    isDireWarning,
   } = useSelector(selectConfirmModal);
 
   const dispatch = useDispatch();
+  const [direWarningChecked, setDireWarningChecked] = useState(false);
 
-  const closeModal = () => dispatch(closeConfirmModal());
+  const closeModal = () => {
+    setDireWarningChecked(false);
+    return dispatch(closeConfirmModal());
+  };
 
   const handleCancel = () => {
     onCancel();
@@ -36,6 +42,7 @@ const ConfirmModal = () => {
       variant={isWarning ? 'danger' : 'primary'}
       onClick={handleConfirm}
       ouiaId="btn-modal-confirm"
+      isDisabled={isDireWarning && !direWarningChecked}
     >
       {confirmButtonText || __('Confirm')}
     </Button>,
@@ -49,12 +56,22 @@ const ConfirmModal = () => {
     </Button>,
   ];
 
+  const direWarningCheckbox = (
+    <Checkbox
+      id="dire-warning-checkbox"
+      ouiaId="dire-warning-checkbox"
+      label={__('I understand that this action cannot be undone.')}
+      isChecked={direWarningChecked}
+      onChange={val => setDireWarningChecked(val)}
+    />
+  );
+
   if (!isOpen) return null;
 
   return (
     <Modal
       ouiaId="app-confirm-modal"
-      id="app-confirm-modal"
+      id={id ?? 'app-confirm-modal'}
       aria-label="application confirm modal"
       variant={ModalVariant.small}
       title={title}
@@ -64,7 +81,10 @@ const ConfirmModal = () => {
       titleIconVariant={isWarning ? 'warning' : null}
       {...modalProps}
     >
-      {message}
+      <>
+        {message}
+        {isDireWarning && direWarningCheckbox}
+      </>
     </Modal>
   );
 };