-
Notifications
You must be signed in to change notification settings - Fork 71
Fixes #39532 - All hosts - add action - bulk add capsule #616
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ofedoren Could you take a look at the ruby here?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks more like a separate API endpoint instead of actual extension/override. This doesn't change an existing route/action/controller, but adds a brand new endpoint with it's own route, action, etc. I'd say this should go under regular API endpoints. I think I've seen somewhere similar addition, but from the top of my head, Katello does something like: https://github.com/Katello/katello/blob/master/app/controllers/katello/api/v2/hosts_bulk_actions_controller.rb Unfortunately I'm a bit swamped with something else to do a proper review, so I'll defer to @adamruzicka to steal a bit of his time 😈 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| module ForemanOpenscap | ||
| module Api | ||
| module V2 | ||
| module HostsBulkActionsControllerExtensions | ||
| extend ActiveSupport::Concern | ||
|
|
||
| included do | ||
| before_action :find_editable_hosts, only: [:change_openscap_proxy] | ||
| end | ||
|
|
||
| extend Apipie::DSL::Concern | ||
|
|
||
| api :PUT, "/hosts/bulk/change_openscap_proxy", N_("Assign OpenSCAP Proxy to multiple hosts") | ||
| 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 | ||
| param :openscap_proxy_id, :number, :required => true, :desc => N_("ID of the OpenSCAP Proxy to assign to the hosts") | ||
| def change_openscap_proxy | ||
| openscap_proxy_id = params[:openscap_proxy_id] | ||
|
|
||
| if openscap_proxy_id.blank? | ||
| return render json: { | ||
| error: { | ||
| message: _("No OpenSCAP Proxy selected."), | ||
| }, | ||
| }, status: :unprocessable_entity | ||
| end | ||
|
|
||
| smart_proxy = begin | ||
| ::SmartProxy.find(openscap_proxy_id) | ||
| rescue ActiveRecord::RecordNotFound | ||
| nil | ||
| end | ||
|
|
||
| if smart_proxy.nil? | ||
| return render json: { | ||
| error: { | ||
| message: _("OpenSCAP proxy with id %s not found") % openscap_proxy_id, | ||
| }, | ||
| }, status: :unprocessable_entity | ||
| end | ||
|
|
||
| unless smart_proxy.has_feature?('Openscap') | ||
| return render json: { | ||
| error: { | ||
| message: _("The selected proxy does not have the OpenSCAP feature enabled."), | ||
| }, | ||
| }, status: :unprocessable_entity | ||
| end | ||
|
|
||
| failed_hosts = [] | ||
| @hosts.each do |host| | ||
| host.openscap_proxy = smart_proxy | ||
| failed_hosts << host unless host.save | ||
| end | ||
|
|
||
| if failed_hosts.empty? | ||
| process_response(true, { | ||
| message: _("OpenSCAP Proxy set to %s") % smart_proxy.name, | ||
| }) | ||
| else | ||
| total_count = @hosts.size | ||
| failed_count = failed_hosts.size | ||
| success_count = total_count - failed_count | ||
|
|
||
| parts = [ | ||
| n_("Failed to assign OpenSCAP Proxy to %{failed} of %{total} host.", | ||
| "Failed to assign OpenSCAP Proxy to %{failed} of %{total} hosts.", | ||
| total_count) % { failed: failed_count, total: total_count }, | ||
| ] | ||
| if success_count > 0 | ||
| parts << n_("Successfully updated %{success} host.", | ||
| "Successfully updated %{success} hosts.", | ||
| success_count) % { success: success_count } | ||
| end | ||
|
|
||
| render_error(:bulk_hosts_error, status: :unprocessable_entity, | ||
| locals: { | ||
| message: parts.join(' '), | ||
| failed_host_ids: failed_hosts.map(&:id), | ||
| }) | ||
| end | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def action_permission | ||
| case params[:action] | ||
| when 'change_openscap_proxy' | ||
| 'edit' | ||
| else | ||
| super | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| require 'test_plugin_helper' | ||
|
|
||
| class Api::V2::HostsBulkActionsControllerTest < ActionController::TestCase | ||
| def setup | ||
| as_admin do | ||
| @organization = FactoryBot.create(:organization) | ||
| @location = FactoryBot.create(:location) | ||
| @proxy = FactoryBot.create(:openscap_proxy, | ||
| :organizations => [@organization], | ||
| :locations => [@location]) | ||
| @host1 = FactoryBot.create(:host, :managed, | ||
| :organization => @organization, | ||
| :location => @location) | ||
| @host2 = FactoryBot.create(:host, :managed, | ||
| :organization => @organization, | ||
| :location => @location) | ||
| @host_ids = [@host1.id, @host2.id] | ||
| end | ||
| end | ||
|
|
||
| def valid_bulk_params(host_ids = @host_ids) | ||
| { | ||
| :organization_id => @organization.id, | ||
| :location_id => @location.id, | ||
| :included => { | ||
| :ids => host_ids, | ||
| }, | ||
| :excluded => { | ||
| :ids => [], | ||
| }, | ||
| } | ||
| end | ||
|
|
||
| test "should assign openscap proxy to selected hosts" do | ||
| put :change_openscap_proxy, | ||
| params: valid_bulk_params.merge(:openscap_proxy_id => @proxy.id), | ||
| session: set_session_user | ||
|
|
||
| assert_response :success | ||
| response = ActiveSupport::JSON.decode(@response.body) | ||
| assert_match(/OpenSCAP Proxy set to/, response['message']) | ||
| assert_includes response['message'], @proxy.name | ||
|
|
||
| [@host1, @host2].each do |host| | ||
| host.reload | ||
| assert_equal @proxy.id, host.openscap_proxy_id | ||
| end | ||
| end | ||
|
|
||
| test "should require openscap_proxy_id" do | ||
| put :change_openscap_proxy, | ||
| params: valid_bulk_params, | ||
| session: set_session_user | ||
|
|
||
| assert_response :unprocessable_entity | ||
| response = ActiveSupport::JSON.decode(@response.body) | ||
| assert_match(/No OpenSCAP Proxy selected/, response['error']['message']) | ||
| end | ||
|
|
||
| test "should return error when proxy is not found" do | ||
| put :change_openscap_proxy, | ||
| params: valid_bulk_params.merge(:openscap_proxy_id => 0), | ||
| session: set_session_user | ||
|
|
||
| assert_response :unprocessable_entity | ||
| response = ActiveSupport::JSON.decode(@response.body) | ||
| assert_match(/not found/, response['error']['message']) | ||
| end | ||
|
|
||
| test "should return error when proxy lacks Openscap feature" do | ||
| other_proxy = FactoryBot.create(:smart_proxy, | ||
| :organizations => [@organization], | ||
| :locations => [@location]) | ||
| openscap_feature = Feature.find_by(:name => 'Openscap') | ||
| other_proxy.features.delete(openscap_feature) if openscap_feature | ||
| refute other_proxy.reload.has_feature?('Openscap') | ||
|
|
||
| put :change_openscap_proxy, | ||
| params: valid_bulk_params.merge(:openscap_proxy_id => other_proxy.id), | ||
| session: set_session_user | ||
|
|
||
| assert_response :unprocessable_entity | ||
| response = ActiveSupport::JSON.decode(@response.body) | ||
| assert_match(/OpenSCAP feature/, response['error']['message']) | ||
| end | ||
|
|
||
| test "should assign openscap proxy for a single host" do | ||
| put :change_openscap_proxy, | ||
| params: valid_bulk_params([@host1.id]).merge(:openscap_proxy_id => @proxy.id), | ||
| session: set_session_user | ||
|
|
||
| assert_response :success | ||
| @host1.reload | ||
| assert_equal @proxy.id, @host1.openscap_proxy_id | ||
| @host2.reload | ||
| assert_nil @host2.openscap_proxy_id | ||
| end | ||
|
|
||
| test "should report failed and successful counts on partial failure" do | ||
| Host.any_instance.stubs(:save).returns(false).then.returns(true) | ||
|
|
||
| put :change_openscap_proxy, | ||
| params: valid_bulk_params.merge(:openscap_proxy_id => @proxy.id), | ||
| session: set_session_user | ||
|
|
||
| assert_response :unprocessable_entity | ||
| response = ActiveSupport::JSON.decode(@response.body) | ||
| assert_match(/Failed to assign OpenSCAP Proxy to 1 of 2 hosts/, response['error']['message']) | ||
| assert_match(/Successfully updated 1 host/, response['error']['message']) | ||
| assert_equal 1, response['error']['failed_host_ids'].size | ||
| assert_includes @host_ids, response['error']['failed_host_ids'].first | ||
| end | ||
|
|
||
| test "should report only failures when all hosts fail" do | ||
| Host.any_instance.stubs(:save).returns(false) | ||
|
|
||
| put :change_openscap_proxy, | ||
| params: valid_bulk_params.merge(:openscap_proxy_id => @proxy.id), | ||
| session: set_session_user | ||
|
|
||
| assert_response :unprocessable_entity | ||
| response = ActiveSupport::JSON.decode(@response.body) | ||
| assert_match(/Failed to assign OpenSCAP Proxy to 2 of 2 hosts/, response['error']['message']) | ||
| refute_match(/Successfully updated/, response['error']['message']) | ||
| assert_equal @host_ids.sort, response['error']['failed_host_ids'].sort | ||
| end | ||
| end |
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.
Success message doesn't include host count (inconsistent with core bulk actions)
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.
I tried changing org and location but it does not show count either