Add Monitoring main navigation section, with Alert sections #507

moolitayer · 2017-02-28T13:51:51Z

Get resource icons from the server
Hide new monitor section by default
https://bugzilla.redhat.com/show_bug.cgi?id=1421173

Depends on [MERGED] ManageIQ/manageiq#14096 Adding the new setting

Closes #254

moolitayer · 2017-02-28T13:58:59Z

app/controllers/alerts_list_controller.rb

@@ -0,0 +1,38 @@
+class AlertsListController < ApplicationController
+  extend ActiveSupport::Concern
+  include UiServiceMixin


@himdel This probably shouldn't be here, should I create a service for it?

himdel · 2017-02-28T16:10:09Z

app/assets/javascripts/services/alerts_center_service.js

      .then(function() {
        return _this.getAlertsData(limit, offset, filters, sortField, sortAscending);
      });
  };

+  _this.updateHostIcons = function(response) {


I'd simplify this to just..

_this.updateHostIcons = function(response) { return $http.get('/alerts_list/class_icons') .then(function(response) { _this.hostIcons = response.data; return response; }); };

himdel · 2017-02-28T16:10:41Z

app/controllers/alerts_list_controller.rb

@@ -1,5 +1,6 @@
 class AlertsListController < ApplicationController
  extend ActiveSupport::Concern
+  include UiServiceMixin


Not sure this is being used here?

It is used for icons

Aaah... missed that.. in that case, are you sure that's the right place?

Because .. that's not a list of all available icons (or classes), that's just a list of things that are shown in one of the topology views.

Any chance the UI can ask for a list of classes instead?

himdel · 2017-02-28T16:12:35Z

app/controllers/alerts_list_controller.rb

+  def class_icons
+    res = {}
+    icons.each do |k, _val|
+      file_name = "100/#{k.to_s.underscore}.png"


this line would be better as k.decorate.listicon_image (assuming k is the class) .. but you'd need to add this method to the relevant decorators... so up to you.

But if you leave this like this, can you mention the list of classes that you're expecting to pass through here please? (So that it's easier to fix after.)

That would be nice and allow me to simplify code a bit, is decorate available for classes too or only instances? I can't seem to use it

It is available now :) (You need current master for both manageiq and ui-classic)

Then to use it, all you have to do is add a static method to the relevant decorator...

So you'd go to MiqPolicyDecorator for example, add something like def self.listicon_image; "100/miq_policy.png"; end .. aaand then MiqPolicy.decorate.listicon_image will work.

(Don't worry too much about having the method there twice (once class once instance), we were talking about doing something like defaulting to the class method unless the instance method needs to exist because the icon depends on some state, but for now... shrug :).)

uploaded a new version, still haven't convinced it to work tough...
I keep getting:

[----] F, [2017-02-28T20:19:32.671115 #5014:2f04e3c] FATAL -- : Error caught: [NoMethodError] undefined method `listicon_image' for #<Draper::CollectionDecorator:0x0000000b7d4280> /home/mtayer/dev/manageiq-ui-classic/app/controllers/alerts_list_controller.rb:22:in `block in class_icons'

I'll try to rebase the client code as well tomorrow

..for #<Draper::CollectionDecorator..

That's a dead giveaway that you have not updated ManageIQ ;)

himdel · 2017-02-28T16:14:09Z

@moolitayer nice work, completely agreed with the approach, just found a few details.. :)

moolitayer · 2017-02-28T17:06:26Z

Thanks for the review @himdel! addressed comments.

moolitayer · 2017-03-01T09:38:18Z

@himdel cool, working now, thanks.
Also I had to rebase @jeff-phillips-18's stuff, so maybe now it would be better to merge this one when we finish code review

moolitayer · 2017-03-01T09:49:46Z

@miq-bot add_label "pending core", enhancement, euwe/no

miq-bot · 2017-03-01T09:50:10Z

@moolitayer Cannot apply the following label because they are not recognized: "pending core"

simon3z · 2017-03-01T11:14:04Z

@moolitayer if this is supposed to replace #254 isn't the title of the PR misleading?

simon3z · 2017-03-01T11:15:10Z

@miq-bot assign moolitayer

moolitayer · 2017-03-01T11:49:36Z

@moolitayer if this is supposed to replace #254 isn't the title of the PR misleading?

Updated the title

himdel · 2017-03-01T12:41:16Z

app/decorators/container_node_decorator.rb

@@ -0,0 +1,5 @@
+class ContainerNodeDecorator < MiqDecorator
+  def self.listicon_image
+    ActionController::Base.helpers.image_path("100/container_node.png")


Please keep this in line with the rest.. listicon_image never calls image_path, only returns the string.

It's the caller's responsibility to call image_path.

sorry, fixed

himdel · 2017-03-01T12:41:55Z

app/controllers/alerts_list_controller.rb

@@ -16,6 +16,14 @@ def index
    redirect_to :action => 'show'
  end

+  def class_icons
+    res = {}
+    ['ManageIQ::Providers::Kubernetes::ContainerManager::ContainerNode'].each do |klass|


Wait, is this really always just this one class?

Or are we expecting more to be added?

himdel · 2017-03-01T12:43:53Z

app/assets/javascripts/services/alerts_center_service.js

+      deferred.resolve(response);
+    });
+    return deferred.promise;
+  };


... I guess you forgot to remove the original version? I'm seeing updateHostIcons twice now.

himdel · 2017-03-01T12:44:22Z

app/assets/javascripts/services/alerts_center_service.js

+    return $http.get('/alerts_list/class_icons')
+      .then(function(response) {
+        _this.hostIcons = response.data;
+         return response;


extra space ;) (one too many now)

jeff-phillips-18 · 2017-03-01T12:53:59Z

app/assets/javascripts/services/alerts_center_service.js

+            }
+
+            summaryItem.objectType = objectType.replace(/([a-z\d])([A-Z]+)/g, '$1_$2').replace(/[-\s]+/g, '_').toLowerCase();
+            summaryItem.objectTypeImg = path + 'vendor-' + summaryItem.objectType + suffix;


This path needs to come from the server as well I believe.

Agreed, same problem :)

moolitayer · 2017-03-05T11:45:32Z

@jeff-phillips-18 would it be possible for you to address the comments still outstanding from @himdel ?
I don't understand the topics. If not let me know I will try to figure them out (event part of them is good)

jeff-phillips-18 · 2017-03-06T15:06:00Z

@moolitayer I have put up a PR to your branch with fixes for the outstanding issues.

moolitayer · 2017-03-06T15:27:50Z

Thanks @jeff-phillips-18 ! I guess I could have done the same thing instead of creating this PR. Next time...

moolitayer · 2017-03-06T15:36:00Z

@himdel Ready for another review!

himdel · 2017-03-06T17:53:38Z

@moolitayer So.. how do I enable the feature? Adding :monitoring: true under :product as usual didn't work.

moolitayer · 2017-03-06T18:16:55Z

@himdel use

:prototype:
  :monitoring: true

(the name was discussed in ManageIQ/manageiq#14096)

himdel · 2017-03-07T10:58:22Z

Tested in the UI, looks good 👍, except for the sopLink link, which always has an empty href now. (href vs ng-href I expect).

Will merge when fixed :)

EDIT: aaand fixed :) ($parent.$parent.item is not great, but needs fixing on the patternfly side)
..waiting for green travis..

miq-bot · 2017-03-07T11:13:23Z

Checked commits moolitayer/manageiq-ui-classic@dc62746~...4e3dd03 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
11 files checked, 0 offenses detected
Everything looks good. 🏆

moolitayer · 2017-03-07T11:32:17Z

@himdel Thank you very much for the fast responses and vigilant review! 🍺

@jeff-phillips-18 🎉

himdel · 2017-03-07T11:35:05Z

🙇‍♂️ :)

moolitayer mentioned this pull request Feb 28, 2017

Add Monitoring main navigation section, with Alert sections #254

Closed

moolitayer changed the title ~~Icon endpoint~~ Add Icon endpoint and hide monitor section by default Feb 28, 2017

moolitayer commented Feb 28, 2017

View reviewed changes

moolitayer force-pushed the icon_endpoint branch 2 times, most recently from 4235661 to 92eedff Compare February 28, 2017 14:38

himdel reviewed Feb 28, 2017

View reviewed changes

moolitayer force-pushed the icon_endpoint branch 3 times, most recently from 625f0fa to d981fd7 Compare March 1, 2017 09:36

moolitayer force-pushed the icon_endpoint branch from d981fd7 to 74e2a53 Compare March 1, 2017 09:47

miq-bot added enhancement euwe/no labels Mar 1, 2017

miq-bot assigned moolitayer Mar 1, 2017

moolitayer changed the title ~~Add Icon endpoint and hide monitor section by default~~ Add Monitoring main navigation section, with Alert sections Mar 1, 2017

himdel added the pending core label Mar 1, 2017

himdel reviewed Mar 1, 2017

View reviewed changes

jeff-phillips-18 reviewed Mar 1, 2017

View reviewed changes

jeff-phillips-18 and others added 12 commits March 5, 2017 13:30

Use original coding style and ignore rubocop issues on line lengths.

82d3841

Add host name to alerts display info.

062f678

Add links to Hosts and Providers in list views

42488ad

Add link to alerts page from provider name in overview tiles

de9ddf0

Add link to SOP in alerts list expansion area

bcfe8ad

Add ability to filter by provider type in overview section

54e40e8

Update to not show components until initialization complete

6574f43

Keep display state for items after refresh

9eecec3

Change expected severity from danger to error

658dee7

Update list styling to show row borders

187329c

Get resource icons from the server

430dfc6

Hide new monitor section by default

ba9f755

moolitayer force-pushed the icon_endpoint branch from 62f0583 to ba9f755 Compare March 5, 2017 11:33

Fix SOP link

4e3dd03

moolitayer force-pushed the icon_endpoint branch from 9c87662 to 4e3dd03 Compare March 7, 2017 11:11

himdel added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 7, 2017

himdel merged commit d0e63ae into ManageIQ:master Mar 7, 2017

himdel mentioned this pull request May 4, 2017

Remove the obsolete AlertService as it has no references #1265

Merged

moolitayer mentioned this pull request Aug 21, 2017

Add monitoring menus ManageIQ/manageiq#15866

Merged

himdel mentioned this pull request Sep 7, 2018

Alerts - fix bug when severity null #4614

Merged

Add Monitoring main navigation section, with Alert sections #507

Add Monitoring main navigation section, with Alert sections #507

Conversation

moolitayer commented Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

himdel Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Feb 28, 2017

moolitayer commented Feb 28, 2017 • edited Loading

moolitayer commented Mar 1, 2017

moolitayer commented Mar 1, 2017

miq-bot commented Mar 1, 2017

simon3z commented Mar 1, 2017

simon3z commented Mar 1, 2017

moolitayer commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Mar 5, 2017

jeff-phillips-18 commented Mar 6, 2017

moolitayer commented Mar 6, 2017

moolitayer commented Mar 6, 2017

himdel commented Mar 6, 2017

moolitayer commented Mar 6, 2017 • edited Loading

himdel commented Mar 7, 2017 • edited Loading

miq-bot commented Mar 7, 2017

moolitayer commented Mar 7, 2017

himdel commented Mar 7, 2017

moolitayer commented Feb 28, 2017 •

edited

Loading

himdel Feb 28, 2017 •

edited

Loading

himdel Feb 28, 2017 •

edited

Loading

moolitayer commented Feb 28, 2017 •

edited

Loading

himdel Mar 1, 2017 •

edited

Loading

moolitayer commented Mar 6, 2017 •

edited

Loading

himdel commented Mar 7, 2017 •

edited

Loading