Fixes #22609 - customizable notifications per feature #318
Conversation
| class AddFeatureIdToJobInvocation < ActiveRecord::Migration[4.2] | ||
| def change | ||
| add_column :job_invocations, :remote_execution_feature_id, :integer, :index => true | ||
| add_foreign_key :job_invocations, :remote_execution_features, :column => :remote_execution_feature_id |
There was a problem hiding this comment.
Use the new Ruby 1.9 hash syntax.
Line is too long. [105/80]
| @@ -0,0 +1,6 @@ | |||
| class AddFeatureIdToJobInvocation < ActiveRecord::Migration[4.2] | |||
| def change | |||
| add_column :job_invocations, :remote_execution_feature_id, :integer, :index => true | |||
There was a problem hiding this comment.
Use the new Ruby 1.9 hash syntax.
Line is too long. [87/80]
| @@ -0,0 +1,6 @@ | |||
| class AddFeatureIdToJobInvocation < ActiveRecord::Migration[4.2] | |||
There was a problem hiding this comment.
Missing top-level class documentation comment.
Missing magic comment # frozen_string_literal: true.
| @@ -0,0 +1,5 @@ | |||
| class AddNotificationBuilderToRemoteExecutionFeature < ActiveRecord::Migration[4.2] | |||
There was a problem hiding this comment.
Missing top-level class documentation comment.
Missing magic comment # frozen_string_literal: true.
Line is too long. [83/80]
| attributes.delete(:host_action_button) | ||
| attrs = [ :host_action_button, :notification_builder ] | ||
| attrs.each do |attr| | ||
| unless self.attribute_names.include?(attr.to_s) |
There was a problem hiding this comment.
Favor modifier unless usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
Redundant self detected.
| @@ -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 | |||
| @@ -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 | |||
There was a problem hiding this comment.
Use %i or %I for an array of symbols.
Line is too long. [101/80]
| @@ -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] | |||
| @@ -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 | |||
There was a problem hiding this comment.
Line is too long. [157/80]
Use the new Ruby 1.9 hash syntax.
| @@ -233,6 +236,7 @@ def params | |||
| :targeting => targeting_params, | |||
| :triggering => {}, | |||
| :concurrency_control => {}, | |||
| :remote_execution_feature_id => @feature.id, | |||
app/models/job_invocation.rb
Outdated
| @@ -87,7 +89,17 @@ def notification_recipients_ids | |||
| end | |||
|
|
|||
| def build_notification | |||
| UINotifications::RemoteExecutionJobs::BaseJobFinish.new(self) | |||
| # self.remote_execution_feature | |||
There was a problem hiding this comment.
Was this left here intentionally?
There was a problem hiding this comment.
nope, I'll remove when you'll add more comments :-)
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
| }, | ||
| :targeting_type => 'static_query', | ||
| :search_query => 'some hosts', | ||
| :inputs => {input1.name => 'some_value'}} |
There was a problem hiding this comment.
Use the new Ruby 1.9 hash syntax.
Space inside { missing.
Space inside } missing.
| :time_span => time_span | ||
| }, | ||
| :targeting_type => 'static_query', | ||
| :search_query => 'some hosts', |
| :concurrency_level => level, | ||
| :time_span => time_span | ||
| }, | ||
| :targeting_type => 'static_query', |
| :remote_execution_feature_id => feature.id, | ||
| :concurrency_control => { | ||
| :concurrency_level => level, | ||
| :time_span => time_span |
| :job_template_id => trying_job_template_1.id, | ||
| :remote_execution_feature_id => feature.id, | ||
| :concurrency_control => { | ||
| :concurrency_level => level, |
| { :job_category => trying_job_template_1.job_category, | ||
| :job_template_id => trying_job_template_1.id, | ||
| :remote_execution_feature_id => feature.id, | ||
| :concurrency_control => { |
| 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, |
| let(:feature) { FacotryBot.create(:remote_execution_feature) } | ||
| let(:params) do | ||
| { :job_category => trying_job_template_1.job_category, | ||
| :job_template_id => trying_job_template_1.id, |
| context 'with rex feature defined' do | ||
| let(:feature) { FacotryBot.create(:remote_execution_feature) } | ||
| let(:params) do | ||
| { :job_category => trying_job_template_1.job_category, |
| }, | ||
| :targeting_type => 'static_query', | ||
| :search_query => 'some hosts', | ||
| :inputs => {input1.name => 'some_value'}} |
There was a problem hiding this comment.
Use the new Ruby 1.9 hash syntax.
Space inside { missing.
Space inside } missing.
| :time_span => time_span | ||
| }, | ||
| :targeting_type => 'static_query', | ||
| :search_query => 'some hosts', |
| :concurrency_level => level, | ||
| :time_span => time_span | ||
| }, | ||
| :targeting_type => 'static_query', |
| :remote_execution_feature_id => feature.id, | ||
| :concurrency_control => { | ||
| :concurrency_level => level, | ||
| :time_span => time_span |
| :job_template_id => trying_job_template_1.id, | ||
| :remote_execution_feature_id => feature.id, | ||
| :concurrency_control => { | ||
| :concurrency_level => level, |
| { :job_category => trying_job_template_1.job_category, | ||
| :job_template_id => trying_job_template_1.id, | ||
| :remote_execution_feature_id => feature.id, | ||
| :concurrency_control => { |
| 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, |
| let(:feature) { FacotryBot.create(:remote_execution_feature) } | ||
| let(:params) do | ||
| { :job_category => trying_job_template_1.job_category, | ||
| :job_template_id => trying_job_template_1.id, |
| context 'with rex feature defined' do | ||
| let(:feature) { FacotryBot.create(:remote_execution_feature) } | ||
| let(:params) do | ||
| { :job_category => trying_job_template_1.job_category, |
ares
force-pushed
the
fix/22609
branch
2 times, most recently
from
3d9bf8b
to
183f51d
Compare
| :remote_execution_feature_id => feature.id, | ||
| :targeting_type => 'static_query', | ||
| :search_query => 'some hosts', | ||
| :inputs => {input1.name => 'some_value'}} |
There was a problem hiding this comment.
Use the new Ruby 1.9 hash syntax.
Space inside { missing.
Space inside } missing.
| @@ -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? | |||
There was a problem hiding this comment.
Use safe navigation (&.) instead of checking if an object exists before calling the method.
| @@ -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? | |||
There was a problem hiding this comment.
Use safe navigation (&.) instead of checking if an object exists before calling the method.
|
cops issues resolved, all should be green this time |
|
Mr. J is happy with sqlite and postgresql, but mysql timed out. I'd say this is good to go |
|
I swear it was green before also on PG :-) |
|
Merged, thanks @ares ! |
This allows defining a custom notification builder for feature, if you want to see example usage, see theforeman/foreman_openscap#326
the fact we now store the feature makes it easy in future to add more customization, e.g. job invocation page, based on it