Fix group level and names for miq event #21569

jrafanie · 2021-11-19T20:26:09Z

I was noticing that ManageIQ/manageiq-api#1099 OPTIONS for event_streams was missing "other" and "detail" after #21552 was merged.

Part of ManageIQ/manageiq-api#1090

I realized this is because we had defaults for "other" and "detail" buried in code and didn't have defaults in the base class with overrides in the subclasses. This PR fixes some of this.

Here's an example of the JSON diff for OPTIONS on event_streams before and after this PR:

joerafaniello@Joes-MBP-2 manageiq % diff -u before.txt after.txt
--- before.txt	2021-11-19 15:17:47.000000000 -0500
+++ after.txt	2021-11-19 15:10:50.000000000 -0500
@@ -86,6 +86,7 @@
             "EmsEvent": {
                 "description": "Management Events",
                 "group_names": {
+                    "other": "Other",
                     "addition": "Creation/Addition",
                     "configuration": "Configuration/Reconfiguration",
                     "console": "Console Activity",
@@ -113,6 +114,7 @@
             "MiqEvent": {
                 "description": "Policy Events",
                 "group_names": {
+                    "other": "Other",
                     "ems_operations": "Provider Operation",
                     "host_operations": "Host Operation",
                     "compliance": "Compliance",
@@ -132,7 +134,8 @@
                 },
                 "group_levels": {
                     "success": "Success",
-                    "failure": "Failure"
+                    "failure": "Failure",
+                    "detail": "Detail"
                 }
             }
         }

jrafanie · 2021-11-19T20:32:06Z

app/models/event_stream.rb

@@ -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


☝️ This was mixing EmsEvent group levels with EventStream. Critical and warning were moved to EmsEvent, detail remains as a default here.

Push down the constants into the correct classes. We had lots of EmsEvent data hardcoded in the EventStream and that needs to be split apart.

miq-bot · 2021-11-19T21:06:11Z

Checked commits jrafanie/manageiq@660aeda~...c62c27d with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint
4 files checked, 0 offenses detected
Everything looks fine. 🍪

Fryguy · 2021-11-19T21:29:07Z

app/models/event_stream.rb

    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]


Not sure if this will break I18N - we may have to figure that out in a follow up.

Yeah, I don't know how to test it. I can ask around and do it in a followup.

Fryguy · 2021-11-19T21:31:04Z

app/models/miq_event.rb

+    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


Similar here - this may cause i18n issues unless the target string is already in the .po.

Fryguy · 2021-11-19T21:31:45Z

LGTM - my concerns are mostly around i18n but I'm not certain that was working prior to this, so don't want to hold it up...we can fix i18n afterwards.

Fryguy · 2021-11-22T15:03:38Z

app/models/ems_event.rb

@@ -10,14 +10,18 @@ class EmsEvent < EventStream
    'Rename_Task',
  ]

+  CLASS_GROUP_LEVELS = %i[critical warning].freeze


For i18n, I think the better way to do this might be to make this:

Suggested change

CLASS_GROUP_LEVELS = %i[critical warning].freeze

CLASS_GROUP_LEVELS = [

N_("Critical"),

N_("Warning")

].freeze

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.

Fryguy · 2021-11-22T15:04:49Z

app/models/event_stream.rb

+  DEFAULT_GROUP_NAME  = :other
+  DEFAULT_GROUP_LEVEL = :detail


Similarly, for i18n here, you'd do something like:

Suggested change

DEFAULT_GROUP_NAME = :other

DEFAULT_GROUP_LEVEL = :detail

DEFAULT_GROUP_NAME = N_("Other")

DEFAULT_GROUP_LEVEL = N_("Detail")

Then later or perhaps in another constant you'd do DEFAULT_GROUP_NAME.downcase.to_sym

Fryguy · 2021-11-22T15:09:49Z

Merging without the i18n stuff - we can fix that in a follow up

agrare · 2021-11-22T18:48:09Z

Looks like ui-classic is failing on missing EmsEvent::GROUP_LEVELS

Really weird that it doesn't show up in org code search: https://github.com/search?q=org%3AManageIQ+EmsEvent%3A%3AGROUP_LEVELS&type=code
But it is there: https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/application_controller/timelines/options.rb#L83-L85

Broken by ManageIQ/manageiq#21569 The constant was defined in the base class, but was actually an EmsEvent concern so this was moved to a method for subclasses to override.

jrafanie · 2021-11-22T19:01:44Z

Opened ManageIQ/manageiq-ui-classic#7972 to fix the "oops, didn't cross repo test with ui-classic"

jrafanie requested review from agrare, Fryguy and kbrock as code owners November 19, 2021 20:26

jrafanie added the bug label Nov 19, 2021

jrafanie commented Nov 19, 2021

View reviewed changes

This was referenced Nov 19, 2021

Api for timeline charts. ManageIQ/manageiq-api#1090

Open

Expose MiqEvent/EmsEvent group/levels in event stream OPTIONS ManageIQ/manageiq-api#1099

Merged

jrafanie force-pushed the fix_group_level_and_names_for_miq_event branch from 066954e to a0f93e2 Compare November 19, 2021 20:59

jrafanie added 3 commits November 19, 2021 16:00

Fix MiqEvent group_levels missing the EventStream default: detail.

660aeda

Push down the constants into the correct classes. We had lots of EmsEvent data hardcoded in the EventStream and that needs to be split apart.

Fix group_names missing :other => 'Other'

ccdddeb

Refactor common code into EventStream.default_group_names_and_levels

c62c27d

jrafanie force-pushed the fix_group_level_and_names_for_miq_event branch from a0f93e2 to c62c27d Compare November 19, 2021 21:00

Fryguy reviewed Nov 19, 2021

View reviewed changes

Fryguy reviewed Nov 22, 2021

View reviewed changes

Fryguy merged commit f7a98cb into ManageIQ:master Nov 22, 2021

jrafanie mentioned this pull request Nov 22, 2021

Use the group_levels method since the constant was removed ManageIQ/manageiq-ui-classic#7972

Merged

jrafanie deleted the fix_group_level_and_names_for_miq_event branch November 22, 2021 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix group level and names for miq event #21569

Fix group level and names for miq event #21569

jrafanie commented Nov 19, 2021 •

edited

Loading

jrafanie Nov 19, 2021

miq-bot commented Nov 19, 2021

Fryguy Nov 19, 2021

jrafanie Nov 19, 2021

Fryguy Nov 19, 2021

Fryguy commented Nov 19, 2021

Fryguy Nov 22, 2021

Fryguy Nov 22, 2021

Fryguy commented Nov 22, 2021

agrare commented Nov 22, 2021

jrafanie commented Nov 22, 2021

-  CLASS_GROUP_LEVELS = %i[critical warning].freeze
+  CLASS_GROUP_LEVELS = [
+    N_("Critical"),
+    N_("Warning")
+  ].freeze

Fix group level and names for miq event #21569

Fix group level and names for miq event #21569

Conversation

jrafanie commented Nov 19, 2021 • edited Loading

jrafanie Nov 19, 2021

Choose a reason for hiding this comment

miq-bot commented Nov 19, 2021

Fryguy Nov 19, 2021

Choose a reason for hiding this comment

jrafanie Nov 19, 2021

Choose a reason for hiding this comment

Fryguy Nov 19, 2021

Choose a reason for hiding this comment

Fryguy commented Nov 19, 2021

Fryguy Nov 22, 2021

Choose a reason for hiding this comment

Fryguy Nov 22, 2021

Choose a reason for hiding this comment

Fryguy commented Nov 22, 2021

agrare commented Nov 22, 2021

jrafanie commented Nov 22, 2021

jrafanie commented Nov 19, 2021 •

edited

Loading