-
Notifications
You must be signed in to change notification settings - Fork 899
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 group level and names for miq event #21569
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -35,13 +35,21 @@ class EventStream < ApplicationRecord | |||||||||
|
||||||||||
after_commit :emit_notifications, :on => :create | ||||||||||
|
||||||||||
# TODO: Consider moving since this is EmsEvent specific. group, group_level and group_name exposed as a virtual columns for reports/api. | ||||||||||
GROUP_LEVELS = %i(critical detail warning).freeze | ||||||||||
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 was mixing EmsEvent group levels with EventStream. Critical and warning were moved to EmsEvent, detail remains as a default here. |
||||||||||
DEFAULT_GROUP_NAME = :other | ||||||||||
DEFAULT_GROUP_LEVEL = :detail | ||||||||||
Comment on lines
+38
to
+39
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. Similarly, for i18n here, you'd do something like:
Suggested change
Then later or perhaps in another constant you'd do |
||||||||||
|
||||||||||
def self.description | ||||||||||
raise NotImplementedError, "Description must be implemented in a subclass" | ||||||||||
end | ||||||||||
|
||||||||||
def self.class_group_levels | ||||||||||
[] | ||||||||||
end | ||||||||||
|
||||||||||
def self.group_levels | ||||||||||
class_group_levels + [DEFAULT_GROUP_LEVEL] | ||||||||||
end | ||||||||||
|
||||||||||
def emit_notifications | ||||||||||
Notification.emit_for_event(self) | ||||||||||
rescue => err | ||||||||||
|
@@ -64,29 +72,29 @@ def self.event_groups | |||||||||
|
||||||||||
# TODO: Consider moving since this is EmsEvent specific. group, group_level and group_name exposed as a virtual columns for reports/api. | ||||||||||
def self.group_and_level(event_type) | ||||||||||
level = :detail # the level is detail as default | ||||||||||
level = DEFAULT_GROUP_LEVEL # the level is detail as default | ||||||||||
egroups = event_groups | ||||||||||
|
||||||||||
group = egroups.detect do |_, value| | ||||||||||
GROUP_LEVELS | ||||||||||
group_levels | ||||||||||
.detect { |lvl| value[lvl]&.any? { |typ| !typ.starts_with?("/") && typ == event_type } } | ||||||||||
.tap { |level_found| level = level_found || level } | ||||||||||
end&.first | ||||||||||
|
||||||||||
group ||= egroups.detect do |_, value| | ||||||||||
GROUP_LEVELS | ||||||||||
group_levels | ||||||||||
.detect { |lvl| value[lvl]&.any? { |typ| typ.starts_with?("/") && Regexp.new(typ[1..-2]).match?(event_type) } } | ||||||||||
.tap { |level_found| level = level_found || level } | ||||||||||
end&.first | ||||||||||
|
||||||||||
group ||= :other | ||||||||||
group ||= DEFAULT_GROUP_NAME | ||||||||||
return group, level | ||||||||||
end | ||||||||||
|
||||||||||
def self.group_name(group) | ||||||||||
return if group.nil? | ||||||||||
group = event_groups[group.to_sym] | ||||||||||
group.nil? ? 'Other' : group[:name] | ||||||||||
group.nil? ? DEFAULT_GROUP_NAME.to_s.capitalize : group[:name] | ||||||||||
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. Not sure if this will break I18N - we may have to figure that out in a follow up. 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. Yeah, I don't know how to test it. I can ask around and do it in a followup. |
||||||||||
end | ||||||||||
|
||||||||||
# TODO: Consider moving since this is EmsEvent specific. group, group_level and group_name exposed as a virtual columns for reports/api. | ||||||||||
|
@@ -112,6 +120,14 @@ def self.timeline_classes | |||||||||
EventStream.subclasses.select { |e| e.respond_to?(:group_names_and_levels) } | ||||||||||
end | ||||||||||
|
||||||||||
def self.default_group_names_and_levels | ||||||||||
{ | ||||||||||
:description => description, | ||||||||||
:group_names => {DEFAULT_GROUP_NAME => DEFAULT_GROUP_NAME.to_s.capitalize}, | ||||||||||
:group_levels => {} | ||||||||||
}.freeze | ||||||||||
end | ||||||||||
|
||||||||||
def self.timeline_options | ||||||||||
timeline_classes.map { |c| [c.name.to_sym, c.group_names_and_levels] }.to_h | ||||||||||
end | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,14 +20,26 @@ class MiqEvent < EventStream | |
ContainerReplicator, ContainerGroup, ContainerProject, | ||
ContainerNode, ContainerImage, PhysicalServer].freeze | ||
|
||
CLASS_GROUP_LEVELS = [MiqPolicy::CONDITION_SUCCESS, MiqPolicy::CONDITION_FAILURE].collect { |level| level.downcase.to_sym } | ||
|
||
def self.description | ||
_("Management Events") | ||
end | ||
|
||
def self.class_group_levels | ||
CLASS_GROUP_LEVELS | ||
end | ||
|
||
def self.description | ||
_("Policy Events") | ||
end | ||
|
||
def self.group_names_and_levels | ||
hash = {:description => description} | ||
hash[:group_names] = MiqEventDefinitionSet.all.pluck(:name, :description).to_h.symbolize_keys | ||
hash[:group_levels] = [MiqPolicy::CONDITION_SUCCESS, MiqPolicy::CONDITION_FAILURE].index_by { |c| c.downcase.to_sym } | ||
hash = default_group_names_and_levels | ||
hash[:group_names].merge!(MiqEventDefinitionSet.all.pluck(:name, :description).to_h.symbolize_keys) | ||
group_levels.each do |level| | ||
hash[:group_levels][level] ||= level.to_s.capitalize | ||
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. Similar here - this may cause i18n issues unless the target string is already in the .po. |
||
end | ||
hash | ||
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.
For i18n, I think the better way to do this might be to make this:
Then later, or perhaps in another constant, do
CLASS_GROUP_LEVELS.map { |l| l.downcase.to_sym }
This way, the strings are in the translation catalog, but the code can still use symbols.