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
Fix the issue that user can't add alerts. #11718
Conversation
@@ -64,7 +64,7 @@ def self.import_from_hash(event, options = {}) | |||
|
|||
def etype | |||
set = memberof.first | |||
raise "unexpected error, no type found for event #{name}" if set.nil? | |||
_log.error("No type found for event #{name}") if set.nil? | |||
set |
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.
@lfu Are there any other callers to etype
that may to handling a nil
being returned?
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.
@gmcculloug Not that I can think of.
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.
Please review the two references to etype
in https://github.com/ManageIQ/manageiq/blob/master/app/controllers/miq_policy_controller/alerts.rb.
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.
@gmcculloug For the first reference, the handling of nil value of event.etype has been added.
For the second reference, it is called for an existing alert. The alert is created only when the based on event has valid event set. So the existing alert should have a valid event.etype.
Policy controller refers to event.etype as well. But that is for events with valid event set.
So in all these cases, only when adding a new alert, we need to check for the nil event.etype and skip those events if they don't have a valid event set. So the alerts can be created only based on valid events.
@@ -64,7 +64,7 @@ def self.import_from_hash(event, options = {}) | |||
|
|||
def etype | |||
set = memberof.first | |||
raise "unexpected error, no type found for event #{name}" if set.nil? | |||
_log.error("No type found for event #{name}") if set.nil? |
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.
Not needed but you could 1-line this: memberof.first.tap { |set| _log.error("No type found for event #{name}") if set.nil? }
@@ -279,6 +279,7 @@ def alert_build_edit_screen | |||
@edit[:events] = {} | |||
MiqEventDefinition.all_events.each do |e| |
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.
@lfu It would make more sense to move the filtering logic into the MiqEventDefinition
model and yield the valid event_definition object. The method could be something like all_alert_events
. I am open to a better name here.
def self.all_alert_events
all_events.each do |e|
next if e.name.ends_with?("compliance_check")
next if e.etype.nil?
yield(e)
end
end
Then this controller section would become:
MiqEventDefinition.all_alert_events.each do |e|
@edit[:events][e.id] = (e.etype.description + ": " + e.description)
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.
@gmcculloug Updated.
When seeded from the fixture file, default events in miq_event_definitions table always belong to certain event set. However there are some appliances from 5.6 fresh installation that have some deleted old events showed up in the DB and not belong to any event set. UI for Add a New Alert would not open due to these ghost and orphaned events. So far we can't find out why these deleted events still showed up in a fresh appliance. It is strange. This commit would skip these orphaned events so users can access the Add a New Alert page. https://bugzilla.redhat.com/show_bug.cgi?id=1379854
4884e07
to
2e19531
Compare
Checked commit lfu@2e19531 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 app/models/miq_event_definition.rb
|
Fix the issue that user can't add alerts. (cherry picked from commit 8b43521) https://bugzilla.redhat.com/show_bug.cgi?id=1379854
Euwe Backport details: $ git log -1
commit d88e9221901f0079a60ecf11fd00a37b875fa4dc
Author: Greg McCullough <gmccullo@redhat.com>
Date: Tue Oct 18 17:32:44 2016 +0200
Merge pull request #11718 from lfu/event_no_set_type_1379854
Fix the issue that user can't add alerts.
(cherry picked from commit 8b43521a41cf3a6fe9832badf9a2e78b4cbb8430)
https://bugzilla.redhat.com/show_bug.cgi?id=1379854 |
Fix the issue that user can't add alerts. (cherry picked from commit 8b43521) https://bugzilla.redhat.com/show_bug.cgi?id=1389790
Darga Backport details: $ git log -1
commit 40f82b792b2ff30269fb47f3d2a7ee1ad5e2868f
Author: Greg McCullough <gmccullo@redhat.com>
Date: Tue Oct 18 17:32:44 2016 +0200
Merge pull request #11718 from lfu/event_no_set_type_1379854
Fix the issue that user can't add alerts.
(cherry picked from commit 8b43521a41cf3a6fe9832badf9a2e78b4cbb8430)
https://bugzilla.redhat.com/show_bug.cgi?id=1389790 |
When seeded from the fixture file, default events in miq_event_definitions table always belong to certain event set. However there are some appliances from 5.6 fresh installation that have some deleted old events showed up in the DB and not belong to any event set. UI for Add a New Alert would not open due to these ghost and orphaned events.
So far we can't find out why these deleted events still showed up in a fresh appliance. It is strange.
This commit would skip these orphaned events so users can access the Add a New Alert page.
Links
https://bugzilla.redhat.com/show_bug.cgi?id=1379854
cc @gmcculloug @h-kataria