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 #22609 - customizable notifications per feature #318

Merged
merged 1 commit into from
Feb 26, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -91,5 +91,14 @@ Naming/HeredocDelimiterNaming:
Style/RescueStandardError:
Enabled: false

Style/SafeNavigation:
Enabled: false

Lint/BooleanSymbol:
Enabled: false

Metrics/PerceivedComplexity:
Max: 8

Metrics/AbcSize:
Max: 45
13 changes: 12 additions & 1 deletion app/models/job_invocation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ class JobInvocation < ApplicationRecord
belongs_to :triggering, :class_name => 'ForemanTasks::Triggering'
has_one :recurring_logic, :through => :triggering, :class_name => 'ForemanTasks::RecurringLogic'

belongs_to :remote_execution_feature

scope :with_task, -> { references(:task) }

scoped_search :relation => :recurring_logic, :on => 'id', :rename => 'recurring_logic.id'
Expand Down Expand Up @@ -87,7 +89,16 @@ def notification_recipients_ids
end

def build_notification
UINotifications::RemoteExecutionJobs::BaseJobFinish.new(self)
klass = nil
if self.remote_execution_feature && self.remote_execution_feature.notification_builder.present?

Choose a reason for hiding this comment

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

Use safe navigation (&.) instead of checking if an object exists before calling the method.

Choose a reason for hiding this comment

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

Use safe navigation (&.) instead of checking if an object exists before calling the method.

begin
klass = remote_execution_feature.notification_builder.constantize
rescue NameError => e
logger.exception "REX feature defines unknown notification builder class", e
end
end
klass ||= UINotifications::RemoteExecutionJobs::BaseJobFinish
klass.new(self)
end

def status
Expand Down
7 changes: 6 additions & 1 deletion app/models/job_invocation_composer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ def params
:targeting => ui_params.fetch(:targeting, {}).merge(:user_id => User.current.id),
:triggering => triggering,
:host_ids => ui_params[:host_ids],
:remote_execution_feature_id => ui_params[:remote_execution_feature_id],
:description_format => job_invocation_base[:description_format],
:password => blank_to_nil(job_invocation_base[:password]),
:key_passphrase => blank_to_nil(job_invocation_base[:key_passphrase]),
Expand Down Expand Up @@ -94,6 +95,7 @@ def params
:targeting => targeting_params,
:triggering => triggering_params,
:description_format => api_params[:description_format],
:remote_execution_feature_id => api_params[:remote_execution_feature_id],
:concurrency_control => concurrency_control_params,
:execution_timeout_interval => api_params[:execution_timeout_interval] || template.execution_timeout_interval,
:template_invocations => template_invocations_params }.with_indifferent_access
Expand Down Expand Up @@ -178,6 +180,7 @@ def params
:description_format => job_invocation.description_format,
:concurrency_control => concurrency_control_params,
:execution_timeout_interval => job_invocation.execution_timeout_interval,
:remote_execution_feature_id => job_invocation.remote_execution_feature_id,
:template_invocations => template_invocations_params }.with_indifferent_access
end

Expand Down Expand Up @@ -233,6 +236,7 @@ def params
:targeting => targeting_params,
:triggering => {},
:concurrency_control => {},
:remote_execution_feature_id => @feature.id,

Choose a reason for hiding this comment

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

Use the new Ruby 1.9 hash syntax.

:template_invocations => template_invocations_params }.with_indifferent_access
end

Expand Down Expand Up @@ -281,7 +285,7 @@ def job_template
end

attr_accessor :params, :job_invocation, :host_ids, :search_query
delegate :job_category, :pattern_template_invocations, :template_invocations, :targeting, :triggering, :to => :job_invocation
delegate :job_category, :remote_execution_feature_id, :pattern_template_invocations, :template_invocations, :targeting, :triggering, :to => :job_invocation

Choose a reason for hiding this comment

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

Line is too long. [157/80]
Use the new Ruby 1.9 hash syntax.


def initialize(params, set_defaults = false)
@params = params
Expand Down Expand Up @@ -314,6 +318,7 @@ def self.for_feature(feature_label, hosts, provided_inputs = {})
def compose
job_invocation.job_category = validate_job_category(params[:job_category])
job_invocation.job_category ||= available_job_categories.first if @set_defaults
job_invocation.remote_execution_feature_id = params[:remote_execution_feature_id]

Choose a reason for hiding this comment

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

Line is too long. [85/80]

job_invocation.targeting = build_targeting
job_invocation.triggering = build_triggering
job_invocation.pattern_template_invocations = build_template_invocations
Expand Down
16 changes: 12 additions & 4 deletions app/models/remote_execution_feature.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
class RemoteExecutionFeature < ApplicationRecord
VALID_OPTIONS = [:provided_inputs, :description, :host_action_button].freeze
VALID_OPTIONS = [:provided_inputs, :description, :host_action_button, :notification_builder].freeze

Choose a reason for hiding this comment

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

Use %i or %I for an array of symbols.
Line is too long. [101/80]

validates :label, :name, :presence => true, :uniqueness => true

belongs_to :job_template
Expand Down Expand Up @@ -27,11 +27,19 @@ def self.register(label, name, options = {})
options[:host_action_button] = false unless options.key?(:host_action_button)

feature = self.find_by(label: label)
builder = options[:notification_builder] ? options[:notification_builder].to_s : nil

Choose a reason for hiding this comment

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

Line is too long. [88/80]


attributes = { :name => name, :provided_input_names => options[:provided_inputs], :description => options[:description], :host_action_button => options[:host_action_button] }
attributes = { :name => name,

Choose a reason for hiding this comment

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

Use the new Ruby 1.9 hash syntax.

:provided_input_names => options[:provided_inputs],

Choose a reason for hiding this comment

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

Use the new Ruby 1.9 hash syntax.

:description => options[:description],

Choose a reason for hiding this comment

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

Use the new Ruby 1.9 hash syntax.

:host_action_button => options[:host_action_button],

Choose a reason for hiding this comment

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

Use the new Ruby 1.9 hash syntax.

:notification_builder => builder }

Choose a reason for hiding this comment

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

Use the new Ruby 1.9 hash syntax.

# in case DB does not have the attribute created yet but plugin initializer registers the feature, we need to skip this attribute
unless self.attribute_names.include?('host_action_button')
attributes.delete(:host_action_button)
attrs = [ :host_action_button, :notification_builder ]

Choose a reason for hiding this comment

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

Use %i or %I for an array of symbols.
Space inside square brackets detected.

attrs.each do |attr|
unless self.attribute_names.include?(attr.to_s)

Choose a reason for hiding this comment

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

Favor modifier unless usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
Redundant self detected.

attributes.delete(attr)
end
end

if feature.nil?
Expand Down
1 change: 0 additions & 1 deletion app/models/setting/remote_execution.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ class Setting::RemoteExecution < Setting

::Setting::BLANK_ATTRS.concat %w{remote_execution_ssh_password remote_execution_ssh_key_passphrase}

# rubocop:disable AbcSize
# rubocop:disable Metrics/MethodLength
def self.load_defaults
# Check the table exists
Expand Down
1 change: 1 addition & 0 deletions app/views/job_invocations/_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
<%= form_for @composer.job_invocation, :html => {'data-refresh-url' => refresh_job_invocations_path, :id => 'job_invocation_form'} do |f| %>

<%= selectable_f f, :job_category, @composer.available_job_categories, {}, :label => _('Job category') %>
<%= f.hidden_field(:job_category, :value => @composer.remote_execution_feature_id) %>

<% selected_templates_per_provider = {} %>
<% @composer.displayed_provider_types.each do |provider_type| %>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddNotificationBuilderToRemoteExecutionFeature < ActiveRecord::Migration[4.2]

Choose a reason for hiding this comment

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

Missing top-level class documentation comment.
Missing magic comment # frozen_string_literal: true.
Line is too long. [83/80]

def change
add_column :remote_execution_features, :notification_builder, :string
end
end
6 changes: 6 additions & 0 deletions db/migrate/20180202123215_add_feature_id_to_job_invocation.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class AddFeatureIdToJobInvocation < ActiveRecord::Migration[4.2]

Choose a reason for hiding this comment

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

Missing top-level class documentation comment.
Missing magic comment # frozen_string_literal: true.

def change
add_column :job_invocations, :remote_execution_feature_id, :integer, :index => true

Choose a reason for hiding this comment

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

Use the new Ruby 1.9 hash syntax.
Line is too long. [87/80]

add_foreign_key :job_invocations, :remote_execution_features, :column => :remote_execution_feature_id

Choose a reason for hiding this comment

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

Use the new Ruby 1.9 hash syntax.
Line is too long. [105/80]

end
end
19 changes: 18 additions & 1 deletion test/unit/job_invocation_composer_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -655,7 +655,7 @@ class JobInvocationComposerTest < ActiveSupport::TestCase
},
:targeting_type => 'static_query',
:search_query => 'some hosts',
:inputs => {input1.name => 'some_value'}}
:inputs => { input1.name => 'some_value' } }
end

it 'sets the concurrency level and time span based on the input' do
Expand All @@ -665,6 +665,23 @@ class JobInvocationComposerTest < ActiveSupport::TestCase
end
end

context 'with rex feature defined' do
let(:feature) { FactoryBot.create(:remote_execution_feature) }
let(:params) do
{ :job_category => trying_job_template_1.job_category,
:job_template_id => trying_job_template_1.id,
:remote_execution_feature_id => feature.id,
:targeting_type => 'static_query',
:search_query => 'some hosts',
:inputs => { input1.name => 'some_value' } }
end

it 'sets the remote execution feature based on the input' do
assert composer.save!
composer.job_invocation.remote_execution_feature.must_equal feature
end
end

context 'with invalid targeting' do
let(:params) do
{ :job_category => trying_job_template_1.job_category,
Expand Down