Add Monitoring main navigation section, with Alert sections #254

jeff-phillips-18 · 2017-01-26T14:29:04Z

This PR will provide viewing of alerts and the ability to assign, un-assign, acknowledge, un-acknowledge, and add comments to alerts.

Please see Jira stories for this contribution for UX designs:

https://patternfly.atlassian.net/browse/CFUX-117
https://patternfly.atlassian.net/browse/CFUX-118
https://patternfly.atlassian.net/browse/CFUX-121
https://patternfly.atlassian.net/browse/CFUX-153
https://patternfly.atlassian.net/browse/CFUX-164

This was ManageIQ/manageiq#11962 in the manageiq repo prior to the split. I did not move the PR due to significant changes due to API implementation changes.

UI Changes:

Todo:
Alert descriptions are not yet shown. This is being worked by ManageIQ/manageiq#13653
Add unit tests

@serenamarie125 @moolitayer @simon3z @martinpovolny @zgalor @abonas @dclarizio @cben

@miq-bot add_label wip, enhancement

simon3z · 2017-02-01T14:05:37Z

@jeff-phillips-18 @serenamarie125 @moolitayer what's the status here?

Is there something blocking it? (Still in WIP?) Do you need any information/reviews/etc?

jeff-phillips-18 · 2017-02-01T14:07:11Z

@simon3z This is waiting for ManageIQ/manageiq#13653 to be merged and I am finishing up the unit tests.

simon3z · 2017-02-01T14:10:50Z

@simon3z This is waiting for ManageIQ/manageiq#13653 to be merged and I am finishing up the unit tests.

@jeff-phillips-18 an nice! I didn't know you were trying to squeeze that in already (or maybe @moolitayer mentioned it but I didn't fully get we would have had the support in the first cycle).

jeff-phillips-18 · 2017-02-01T17:28:36Z

Unit tests are in
ManageIQ/manageiq#13653 merged.

Removing WIP

@miq-bot remove_label wip

jeff-phillips-18 · 2017-02-02T18:24:44Z

@serenamarie125 @martinpovolny @dclarizio This PR is ready for review.

dclarizio · 2017-02-02T18:44:39Z

@martinpovolny @himdel thinking this might need some eyes on it, can you please review? Thx, Dan

himdel · 2017-02-02T22:10:08Z

bower.json

@@ -42,6 +42,7 @@
    "manageiq-ui-components": "~0.0.9",
    "moment-strftime": "~0.2.0",
    "moment-timezone": "~0.4.1",
+    "moment-duration-format": "~1.3.0",


For now, you'll have to do the same change in manageiq/bower.json, we're in the process of moving it to ui-classic .. but .. right now, manageiq/bower.json is the one that ends up being used.

(But do keep it here too, will be easier to figure out if both are the same :)).

Added PR ManageIQ/manageiq#13772

himdel · 2017-02-02T22:15:35Z

app/controllers/application_controller.rb

@@ -2125,7 +2125,8 @@ def set_global_session_data
      case controller_name

      # These controllers don't use breadcrumbs, see above get method to store URL
-      when "dashboard", "report", "support", "alert", "jobs", "ui_jobs", "miq_ae_tools", "miq_policy", "miq_action", "miq_capacity", "chargeback", "service"
+      # when "dashboard", "report", "support", "alert", "alert_center", "jobs", "ui_jobs",


seems like this change doesn't belong here, does it?

Rubocop complains of an empty 'when' block. I commented it out rather than removing it for clarity of what sections are being ignored.

Oh I'd just ignore rubocop, but maybe it makes sense then.. :)

himdel · 2017-02-02T22:21:27Z

app/assets/javascripts/controllers/alerts/alerts_overview_controller.js

+    vm.loadingDone = false;
+
+    function setupInitialValues() {
+      angular.element(document.querySelector('#center_div')).addClass("miq-body");


Could you update this to document.getElementById("center_div").className += " miq-body"; please? Or change the other two to this, I don't care as long as they're the same.

himdel · 2017-02-02T22:23:07Z

So.. I'll give it a more thorough test in the morning but overall this looks really good, css is scoped, $scope is not used, API is.. :)

One more thing in addition to the above.. @martinpovolny can probably tell you more but each rails controller should have a menu_section :something (I guess :monitoring in your case) .. so that each controller knows under which primary menu item it is.

martinpovolny · 2017-02-03T17:29:51Z

app/assets/javascripts/controllers/alerts/edit_alert_dialog_controller.js

+angular.module('alertsCenter')
+  .controller('EditAlertDialogController',['$scope', 'vm',
+    function($scope, vm) {
+      $scope.vm = vm;


Please, use controllerAs instad of $scope for new code. Also, please, don't inject $scope when not needed.

martinpovolny · 2017-02-03T17:32:22Z

app/presenters/menu/default_menu.rb

+          'monitor_alerts_most_recent',
+          {:feature => 'monitor_alerts_most_recent', :any => true},
+          '/alerts_most_recent'
+        )


I would not say that this way of aligning the menu items is more readable than the way most of the file is alligned.

I did this to resolve rubocop issues with line lengths.

I understand that.

We should probably disable the long line check for the menu definition and disable the alignment check for the toolbars.

Both seem much more readable when we don't follow the rules. And readability is the goal, not following Rubocop in all details.

There seems to be a way to disable a check using comments in the source code -- see https://github.com/bbatsov/rubocop/blob/master/manual/configuration.md and of the page.

What do you think?

martinpovolny · 2017-02-07T09:27:42Z

@jeff-phillips-18, @himdel : please, see the above comment regarding rubocop rules exception. Shall I create a PR adding the exceptions for the menus and toolbars?

himdel · 2017-02-07T09:48:16Z

@martinpovolny agreed with the exception (or with removing that rule altogether :))

jeff-phillips-18 · 2017-02-07T12:24:17Z

I put back the original coding style and disabled line length checks for those two files.

This PR will provide viewing of alerts and the ability to assign/unassign, acknowledge/unacknowledge, or add comments to alerts.

himdel · 2017-02-14T13:36:02Z

app/assets/javascripts/services/alerts_center_service.js

+      prefix = 'os-';
+    }
+
+    var typeImage = path + prefix + imageName + suffix;


Nope nope nope, this will never work in production, sorry.

The full image path needs to come from the backend, and needs to pass through the image_path helper, otherwise, you'll get an invalid URL in production mode.

Yeah that's my fault...

@himdel do you have an example for an API returning images?
I could not find a proper way to implement it in ManageIQ

Aaah.. you're right, the API is probably not the place to serve images from...

Frankly we're not quite prepared for that, so I'm afraid every solution will have to be a bit hacky..

But.. maybe something simple like... add a method to the (non-api) controller that just gives you the list of images you need, and query that on load?

Something like...

def whatever_images render :json => { :this_vendor => image_path('svg/vendor-foo.svg'), :that_vendor => image_path('svg/vendor-bar.svg'), ... } end

WDYT?

This page works purely on REST AFAIK, it took some time to get the API ready and would definitely need improvements down the road. On next iteration I plan to add

URL for the show view of each entity (both alert targets and providers)

URL for image of each entity (both alert targets and providers)

I think you had some idea when I asked before (using an api decorator), and I also asked @abellotti about it, but I'm still not sure exactly how to do it and need some time to research it.

What ever solution we find for this now I will come back to solving this through the API.

The problem with API and icons is that we're actually going in the way of removing decorators from the API.

So I'm afraid the "give me the images" request can't go through the API.

I think at some point we may have to push the decorators to a JS library, but.. until we move to webpack, we can't have predictable image urls in production.

As for using the decorators, definitely for it, if there is a nice way :) (going to add support for class decorators soon, if it helps)

himdel · 2017-02-14T13:39:00Z

app/views/shared/views/_alerts_list.html.haml

+              %span.no-wrap{"tooltip" => "{{item.hostName}}",
+                            "tooltip-placement" => "bottom",
+                            "tooltip-popup-delay" => "1000"}
+                %img.list-view-type-img{"ng-src" => "/assets/100/container_node.png"}


Same problem here, this needs to be :src => image_path("100/container_node.png") (or ng-src, either)

himdel · 2017-02-14T13:39:49Z

spec/javascripts/services/alerts_center_service_spec.js

+      expect(alertsList.length).toBe(4);
+
+      expect(alertsList[0].objectType).toBe("Hawkular");
+      expect(alertsList[0].objectTypeImg).toBe("/assets/svg/vendor-hawkular.svg");


won't be, you may have more luck with something like .toMatch(/svg\/vendor-hawkular/)

himdel · 2017-02-14T13:40:53Z

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;


Another place that needs the image path to come from the server.

himdel · 2017-02-14T13:50:16Z

A minor styling issue.. the Display select is probably too narrow, "Nothing select" doesn't sound quite right ;)

himdel · 2017-02-14T13:51:23Z

Also, @jeff-phillips-18 is there any way to get some data here?

Hard to test with 0 alerts :)

moolitayer · 2017-02-14T14:27:58Z

@himdel my latest dump available at:
http://wikisend.com/download/876570/output.sql

It might have missing fields, let me know if it does

miq-bot · 2017-02-14T22:22:57Z

Checked commits jeff-phillips-18/manageiq-ui-classic@4a4099c~...347b071 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
9 files checked, 0 offenses detected
Everything looks good. 🍰

jeff-phillips-18 · 2017-02-14T22:23:48Z

Updated per @himdel comments.

Please advise on how to handle the images. Is there a good way currently to get the image path?

himdel · 2017-02-15T12:18:18Z

Is there a good way currently to get the image path?

@jeff-phillips-18 So.. if I understand it right, you're getting the type from the API, which is something like ManageIQ::Providers::Kubernetes::ContainerManager::ContainerNode. And in the general case it could be any model class, right? (Or is it always a ContainerNode descendant?)

I think the backend consensus was that decorators and icons are something that should live in the UI, therefore we can't expect to get that info from the API.

So.. there is really no other way than asking the UI for that info :). (Unless it can never be anything but one of a small number of classes that can never be expected to change in which case, you could just put them all in the view, and pick the right one when needed.)

Now, the prefferred way to get the right images is decorators, but for now, these depend on having an instance to decorate (should be fixed in the next sprint though, hopefully).

Another thing is that some decorators do give you a different icon based on some property of that instance, but I guess that's not the case here, right?

Yet another thing, we're slowly moving towards fonticons over images, so ideally your code should support both.

So.. you do need to add a method to the ruby non-api controller that gives you the right icon, and you do need to call it for every class you encounter - with the whole classname, no splits.

With class decorators, that method could be as simple as

def icon_for_class
  klass = params[:type].constantize
  render :json => {:image => ActionController::Base.helpers.image_path(klass.decorate.listicon_image),
                   :icon  => klass.decorate.fonticon}
end

(If you need it faster than we can get fonticon decorators, you'll probably need a custom case with every class and the right icon, and a FIXME comment :). Or alternatively, you can also send the id in addition to type and you can do klass.find_by(id).decorate.listicon_image)

EDIT: added image_path to the sample icon_for_class method

EDIT2: an optimization where you POST an array (sorted and uniqued) of class names to that method, and make it return an object keyed by the class name would make sense to me.

jeff-phillips-18 · 2017-02-15T12:30:22Z

There are two objects I need images for, the provider (which I get the type in the format above) and the host, which I currently only get ContainerNode (so we always use the container_node.png).

I don't have the type until I get data back from the API, using the ruby controller has no idea what types I will be getting back.

himdel · 2017-02-15T12:34:05Z

I don't have the type until I get data back from the API, using the ruby controller has no idea what types I will be getting back.

Sure, that's expected, but after you get that data back from the API, you can do the UI request to get the images.

I mean something like... (pseudocode-ish)

API.get('/api/alerts')
  .then(function(resources) {
    var klasses = _(resources).pluck('type').sort().uniq().values();
    
    $http.post('/monitoring/icon_for_class', klasses)
    .then(function(object) {
      resources.forEach(function(res) {
        res.image = object[res[type]];
      });
    })

  });

moolitayer · 2017-02-15T13:54:58Z

Update: @simon3z will be drafting images for sandbox delivery to openshift-ops tomorrow morning.
If this merges today we will be able to include it in this cycle
cc @jeff-phillips-18 @himdel @martinpovolny

moolitayer · 2017-02-22T13:38:36Z

@himdel @jeff-phillips-18 is there anything blocking us here besides ManageIQ/manageiq#13772 ?

jeff-phillips-18 · 2017-02-22T13:40:09Z

@moolitayer AFAIK, just a solution for the images

moolitayer · 2017-02-22T14:02:49Z

@jeff-phillips-18 @himdel
Can we use a link in the client for now (container nodes, container provider) image for now and improve it from the API going forward?

I don't think we should introduce any other mechanism since AFAIK we all agree that is the best solution in the long range.

cc @simon3z

himdel · 2017-02-22T14:13:15Z

Well, I thought I've already provided a workable solution for the images, even though it'd have to be a big case until #237 gets in.

And .. please note that there are currently no plans to add image support to the API.

himdel · 2017-02-22T14:13:36Z

But sorry, we can't merge as is, because this will not work in production.

jeff-phillips-18 · 2017-02-22T16:25:36Z

@moolitayer would you have time to work on the image solution provided by @himdel ? I have been re-prioritized and am knee deep in other issues there. Not sure when I can get back to this.

moolitayer · 2017-02-22T17:31:12Z

@himdel I guess it isn't that easy as I thought it is, thanks for helping resolve that

moolitayer · 2017-02-28T13:55:32Z

@jeff-phillips-18 @himdel I've added an icon endpoint in #507.
So I've sent a new PR based on this one. When review is finished I think It would be nice to merge the two in order, but I'm open to what ever way you want to work.

moolitayer · 2017-03-01T09:40:53Z

I had to rebase some commits here (mostly due to #204) in #507. So maybe now it would be better to merge that one instead (all @jeff-phillips-18 commits remain unchanged except for the rebase)
cc @himdel @jeff-phillips-18

himdel · 2017-03-01T12:28:54Z

Ok, added a Closes #254 to #507

miq-bot · 2017-03-07T11:32:59Z

This pull request is not mergeable. Please rebase and repush.

miq-bot added enhancement wip labels Jan 26, 2017

jeff-phillips-18 force-pushed the monitor branch from a378b07 to f8f29e9 Compare January 26, 2017 18:44

jeff-phillips-18 force-pushed the monitor branch from f8f29e9 to e61f3fd Compare February 1, 2017 15:56

jeff-phillips-18 changed the title ~~[WIP] Add Monitoring main navigation section, with Alert sections~~ Add Monitoring main navigation section, with Alert sections Feb 1, 2017

miq-bot removed the wip label Feb 1, 2017

dclarizio requested review from himdel and martinpovolny February 2, 2017 18:45

dclarizio added the euwe/no label Feb 2, 2017

himdel reviewed Feb 2, 2017

View reviewed changes

martinpovolny reviewed Feb 3, 2017

View reviewed changes

miq-bot added wip and removed wip labels Feb 3, 2017

martinpovolny reviewed Feb 3, 2017

View reviewed changes

jeff-phillips-18 mentioned this pull request Feb 6, 2017

Add moment-duration-format dependency (for upcoming alerts UI) ManageIQ/manageiq#13772

Merged

jeff-phillips-18 force-pushed the monitor branch from 2a1fc3f to 4b756dc Compare February 7, 2017 12:23

jeff-phillips-18 force-pushed the monitor branch from 4b756dc to b8a10e7 Compare February 7, 2017 12:32

Add Monitoring main navigation section, with Alert sections

4a4099c

This PR will provide viewing of alerts and the ability to assign/unassign, acknowledge/unacknowledge, or add comments to alerts.

himdel reviewed Feb 14, 2017

View reviewed changes

himdel added the pending core label Feb 14, 2017

Code simplification and minor width change for select components

347b071

moolitayer mentioned this pull request Feb 28, 2017

Add Monitoring main navigation section, with Alert sections #507

Merged

himdel closed this in #507 Mar 7, 2017

miq-bot added the unmergeable label Mar 7, 2017

himdel mentioned this pull request Mar 7, 2017

vim_performance_operating_range: can't dup NilClass ManageIQ/manageiq#13905

Closed

Add Monitoring main navigation section, with Alert sections #254

Add Monitoring main navigation section, with Alert sections #254

Conversation

jeff-phillips-18 commented Jan 26, 2017 • edited by himdel Loading

simon3z commented Feb 1, 2017

jeff-phillips-18 commented Feb 1, 2017

simon3z commented Feb 1, 2017

jeff-phillips-18 commented Feb 1, 2017

jeff-phillips-18 commented Feb 2, 2017

dclarizio commented Feb 2, 2017

himdel Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Feb 2, 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

himdel commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpovolny Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

martinpovolny commented Feb 7, 2017

himdel commented Feb 7, 2017

jeff-phillips-18 commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

moolitayer Feb 14, 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 14, 2017 • edited Loading

Choose a reason for hiding this comment

himdel Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Feb 14, 2017 • edited Loading

himdel commented Feb 14, 2017

moolitayer commented Feb 14, 2017

miq-bot commented Feb 14, 2017

jeff-phillips-18 commented Feb 14, 2017

himdel commented Feb 15, 2017 • edited Loading

jeff-phillips-18 commented Feb 15, 2017

himdel commented Feb 15, 2017 • edited Loading

moolitayer commented Feb 15, 2017

moolitayer commented Feb 22, 2017

jeff-phillips-18 commented Feb 22, 2017

moolitayer commented Feb 22, 2017

himdel commented Feb 22, 2017

himdel commented Feb 22, 2017

jeff-phillips-18 commented Feb 22, 2017

moolitayer commented Feb 22, 2017

moolitayer commented Feb 28, 2017

moolitayer commented Mar 1, 2017

himdel commented Mar 1, 2017

miq-bot commented Mar 7, 2017

jeff-phillips-18 commented Jan 26, 2017 •

edited by himdel

Loading

himdel Feb 2, 2017 •

edited

Loading

himdel Feb 2, 2017 •

edited

Loading

martinpovolny Feb 7, 2017 •

edited

Loading

himdel Feb 14, 2017 •

edited

Loading

moolitayer Feb 14, 2017 •

edited

Loading

himdel Feb 14, 2017 •

edited

Loading

himdel Feb 14, 2017 •

edited

Loading

himdel commented Feb 14, 2017 •

edited

Loading

himdel commented Feb 15, 2017 •

edited

Loading

himdel commented Feb 15, 2017 •

edited

Loading