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.

Sign up for GitHub

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

Jump to bottom

Alerts style fixes #2047

Merged
merged 6 commits into from
Sep 1, 2017
Merged

Alerts style fixes #2047

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ae1b27e
Alerts: split alert_edit.
martinpovolny Aug 30, 2017
a95a05e
Alerts: limit branching in alert_edit_save_add.
martinpovolny Aug 30, 2017
e739b8d
Alerts: do not call MiqAlert.new or Alert.find 2x.
martinpovolny Aug 30, 2017
05291e0
Alerts: replace find_by_* calls.
martinpovolny Aug 30, 2017
a8d0c00
Alerts: decuplicate time hashes.
martinpovolny Aug 30, 2017
517ac5f
Alerts: make Rubocop happy.
martinpovolny Aug 30, 2017
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
116 changes: 60 additions & 56 deletions app/controllers/miq_policy_controller/alerts.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,49 +1,61 @@
module MiqPolicyController::Alerts
extend ActiveSupport::Concern

def alert_edit_cancel
@edit = nil
@alert = session[:edit][:alert_id] ? MiqAlert.find(session[:edit][:alert_id]) : MiqAlert.new
if @alert && @alert.id.blank?
add_flash(_("Add of new %{models} was cancelled by the user") % {:models => ui_lookup(:model => "MiqAlert")})
else
add_flash(_("Edit of %{model} \"%{name}\" was cancelled by the user") % {:model => ui_lookup(:model => "MiqAlert"), :name => @alert.description})
end
get_node_info(x_node)
replace_right_cell(:nodetype => @nodetype)
end

def alert_edit_save_add
id = params[:id] && params[:button] != "add" ? params[:id] : "new"
return unless load_edit("alert_edit__#{id}", "replace_cell__explorer")
alert = @alert = @edit[:alert_id] ? MiqAlert.find(@edit[:alert_id]) : MiqAlert.new
alert_set_record_vars(alert)

unless alert_valid_record?(alert) && alert.valid? && !@flash_array && alert.save
alert.errors.each do |field, msg|
add_flash("#{field.to_s.capitalize} #{msg}", :error)
end
replace_right_cell(:nodetype => "al")
return
end

AuditEvent.success(build_saved_audit(alert, params[:button] == "add"))
flash_key = params[:button] == 'save' ? _("Alert \"%{name}\" was saved") : _("Alert \"%{name}\" was added")
add_flash(flash_key % {:name => @edit[:new][:description]})
alert_get_info(MiqAlert.find(alert.id))
alert_sync_provider(@edit[:alert_id] ? :update : :new)
@edit = nil
@nodetype = "al"
@new_alert_node = "al-#{to_cid(alert.id)}"
replace_right_cell(:nodetype => "al", :replace_trees => [:alert_profile, :alert])
end

def alert_edit_reset
alert_build_edit_screen
@sb[:action] = "alert_edit"
if params[:button] == "reset"
add_flash(_("All changes have been reset"), :warning)
end
replace_right_cell(:nodetype => "al")
end

def alert_edit
assert_privileges(params[:pressed]) if params[:pressed]
case params[:button]
when "cancel"
@edit = nil
@alert = session[:edit][:alert_id] ? MiqAlert.find_by_id(session[:edit][:alert_id]) : MiqAlert.new
if @alert && @alert.id.blank?
add_flash(_("Add of new %{models} was cancelled by the user") % {:models => ui_lookup(:model => "MiqAlert")})
else
add_flash(_("Edit of %{model} \"%{name}\" was cancelled by the user") % {:model => ui_lookup(:model => "MiqAlert"), :name => @alert.description})
end
get_node_info(x_node)
replace_right_cell(:nodetype => @nodetype)
alert_edit_cancel
when "save", "add"
id = params[:id] && params[:button] != "add" ? params[:id] : "new"
return unless load_edit("alert_edit__#{id}", "replace_cell__explorer")
@alert = @edit[:alert_id] ? MiqAlert.find_by_id(@edit[:alert_id]) : MiqAlert.new
alert = @alert.id.blank? ? MiqAlert.new : MiqAlert.find(@alert.id) # Get new or existing record
alert_set_record_vars(alert)
if alert_valid_record?(alert) && alert.valid? && !@flash_array && alert.save
AuditEvent.success(build_saved_audit(alert, params[:button] == "add"))
flash_key = params[:button] == "save" ? _("%{model} \"%{name}\" was saved") :
_("%{model} \"%{name}\" was added")
add_flash(flash_key % {:model => ui_lookup(:model => "MiqAlert"), :name => @edit[:new][:description]})
alert_get_info(MiqAlert.find(alert.id))
alert_sync_provider(@edit[:alert_id] ? :update : :new)
@edit = nil
@nodetype = "al"
@new_alert_node = "al-#{to_cid(alert.id)}"
replace_right_cell(:nodetype => "al", :replace_trees => [:alert_profile, :alert])
else
alert.errors.each do |field, msg|
add_flash("#{field.to_s.capitalize} #{msg}", :error)
end
replace_right_cell(:nodetype => "al")
end
alert_edit_save_add
when "reset", nil # Reset or first time in
alert_build_edit_screen
@sb[:action] = "alert_edit"
if params[:button] == "reset"
add_flash(_("All changes have been reset"), :warning)
end
replace_right_cell(:nodetype => "al")
alert_edit_reset
end
end

Expand All @@ -52,10 +64,9 @@ def alert_delete
alerts = []
# showing 1 alert, delete it

if params[:id].nil? || MiqAlert.find_by_id(params[:id]).nil?
add_flash(_("%{models} no longer exists") % {:models => ui_lookup(:model => "MiqAlert")},
:error)
elsif MiqAlert.find_by_id(params[:id]).read_only
if params[:id].nil? || !MiqAlert.exists?(params[:id])
add_flash(_("%{models} no longer exists") % {:models => ui_lookup(:model => "MiqAlert")}, :error)
elsif MiqAlert.find(params[:id]).read_only
add_flash(_("%{models} can not be deleted") % {:models => ui_lookup(:model => "MiqAlert")}, :error)
else
alerts.push(params[:id])
Expand All @@ -71,7 +82,7 @@ def alert_delete

def alert_field_changed
return unless load_edit("alert_edit__#{params[:id]}", "replace_cell__explorer")
@alert = @edit[:alert_id] ? MiqAlert.find_by_id(@edit[:alert_id]) : MiqAlert.new
@alert = @edit[:alert_id] ? MiqAlert.find(@edit[:alert_id]) : MiqAlert.new

@edit[:new][:description] = params[:description].blank? ? nil : params[:description] if params[:description]
@edit[:new][:enabled] = params[:enabled_cb] == "1" if params.key?(:enabled_cb)
Expand Down Expand Up @@ -193,7 +204,7 @@ def alert_field_changed
# rebuild hash to hold user's email along with name if user record was found for display, defined as hash so only email id can be sent from form to be deleted from array above
@email_to = {}
@edit[:new][:email][:to].each_with_index do |e, _e_idx|
u = User.find_by_email(e)
u = User.find_by(:email => e)
@email_to[e] = u ? "#{u.name} (#{e})" : e
end
end
Expand Down Expand Up @@ -249,7 +260,7 @@ def alert_build_edit_screen
@email_to = {}
if @edit[:new][:email] && @edit[:new][:email][:to]
@edit[:new][:email][:to].each_with_index do |e, _e_idx|
u = User.find_by_email(e)
u = User.find_by(:email => e)
@email_to[e] = u ? "#{u.name} (#{e})" : e
end
end
Expand Down Expand Up @@ -479,20 +490,13 @@ def alert_build_pulldowns
}

# hourly_performance repeat times for Notify Every pull down
@sb[:alert][:hourly_repeat_times] ||= {
1.hour.to_i => _("1 Hour"), 2.hours.to_i => _("2 Hours"), 3.hours.to_i => _("3 Hours"),
4.hours.to_i => _("4 Hours"), 6.hours.to_i => _("6 Hours"), 12.hours.to_i => _("12 Hours"),
1.day.to_i => _("1 Day")
}
@sb[:alert][:hourly_repeat_times] ||= @sb[:alert][:hourly_time_thresholds]

# repeat times for Notify Every pull down
@sb[:alert][:repeat_times] ||= {
1.minutes.to_i => _("1 Minute"), 2.minutes.to_i => _("2 Minutes"), 3.minutes.to_i => _("3 Minutes"),
4.minutes.to_i => _("4 Minutes"), 5.minutes.to_i => _("5 Minutes"), 10.minutes.to_i => _("10 Minutes"),
15.minutes.to_i => _("15 Minutes"), 30.minutes.to_i => _("30 Minutes"), 1.hour.to_i => _("1 Hour"),
2.hours.to_i => _("2 Hours"), 3.hours.to_i => _("3 Hours"), 4.hours.to_i => _("4 Hours"),
3.hours.to_i => _("3 Hours"), 4.hours.to_i => _("4 Hours"),
6.hours.to_i => _("6 Hours"), 12.hours.to_i => _("12 Hours"), 1.day.to_i => _("1 Day")
}
}.merge(@sb[:alert][:rt_time_thresholds])

# repeat times for Notify Datawarehouse pull down
@sb[:alert][:repeat_times_dwh] ||= {
Expand Down Expand Up @@ -645,7 +649,7 @@ def alert_get_info(alert)
end
if @alert.options && @alert.options[:notifications] && @alert.options[:notifications][:email] && @alert.options[:notifications][:email][:to]
@alert.options[:notifications][:email][:to].each do |to|
user = User.find_by_email(to)
user = User.find_by(:email => to)
@email_to.push(user ? "#{user.name} (#{to})" : to)
end
end
Expand All @@ -660,7 +664,7 @@ def alert_get_info(alert)
end

if @alert.expression && !@alert.expression.kind_of?(MiqExpression) # Get the EMS if it's in the expression
@ems = ExtManagementSystem.find_by_id(@alert.expression[:options][:ems_id].to_i)
@ems = ExtManagementSystem.find(@alert.expression[:options][:ems_id].to_i)
Copy link
Member

Choose a reason for hiding this comment

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

This change is breaking the code, since [:ems_id] is not guaranteed to be present. So, find raises an exception because is trying to find something with id = 0

Copy link
Member Author

Choose a reason for hiding this comment

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

more likely there's a null going in and due to the .to_i it's converted to the 0

Would be nice to know more about the situation and the reasoning so that I could write a test for the regression.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, indeed, @alert.expression[:options][:ems_id] is nil, because [:ems_id] is not set.

I cannot speak broadly. At least, for MW-Servers the alerts are not tied to any infrastructure item nor any EMS and this is the reason why there is no ems_id in the options hash. The relationship is done through alert profiles.

The only kind of alert I could find that asks for a provider is a "VMware Alarm". Probably, this piece of code is specific to those alerts.

end
if @alert.expression.kind_of?(Hash) && @alert.expression[:eval_method]
MiqAlert.expression_options(@alert.expression[:eval_method]).each do |eo|
Expand Down