Physical Server quadicon #1173

MaysaMacedo · 2017-04-26T21:07:06Z

This PR contains:

Quadicon for physical server

depends on:

#15129

skateman

Could you please provide a screenshot of the new quadicon in the PR's description?

skateman · 2017-04-27T12:02:49Z

app/helpers/quadicon_helper.rb

+        end
+      end
+    end
+    safe_join(output.collect(&:html_safe))


Is this html_safe really necessary when using safe_join?

No, it wasn't. I just removed it.

gabrielsvinha · 2017-04-27T15:33:18Z

app/helpers/quadicon_helper.rb

@@ -641,6 +699,15 @@ def render_single_quad_quadicon(item, options)
    output.collect(&:html_safe).join('').html_safe
  end

+  def img_for_health_state(item)
+    case item.health_state
+    when "Valid"    then "100/healthstate-normal.png"


@MaysaMacedo this is changed in this PR: #1166 . The new path to this image is: svg/healthstate-normal.svg

OK. Just changed it.

skateman · 2017-04-28T18:54:05Z

Just one more small thing, can you squash the two commits and make the commit message the same as the PR's title?

hayesr

I'm impressed you found your way through this code, it's a bit of a mess 😄 It mostly looks great, but it would be helpful if the methods that are specific to a physical server could be moved to a decorator. I'd like to refactor this code to be more OO and I'd like the quadicon builders to ask the objects where their icons are. Along those lines it would be super helpful if you could add some specs so that when I refactor I don't break your code 😁

hayesr · 2017-05-04T16:37:31Z

app/helpers/quadicon_helper.rb

@@ -151,6 +151,10 @@ def render_quadicon(item, options = {})
    end
  end

+  def img_for_physical_vendor(item)


Could this be moved to a decorator? (here's an example) We're trying to clean up this code and that would help :)

I've just moved this to a decorator.

hayesr · 2017-05-04T16:39:38Z

app/helpers/quadicon_helper.rb

@@ -641,6 +699,15 @@ def render_single_quad_quadicon(item, options)
    output.collect(&:html_safe).join('').html_safe
  end

+  def img_for_health_state(item)


It would be cool if this could be moved into the decorator as well.

I'm impressive with all ManageIQ architecture, then I think that refactoring which you proposed can be think after this PR be merged, because this feature it's taking more time than supposed to.

I'm fine with keeping this here (for now), but let's ask @himdel's opinion.

Agreed, better have it together with the 4 other methods here, than in an unexpected place by itself. 👍

hayesr · 2017-05-04T17:11:35Z

app/helpers/quadicon_helper.rb

+    if options[:typ] == :listnav
+      # Listnav, no href needed
+      output << content_tag(:div, :class => 'flobj') do
+        tag(:img, :src => ActionController::Base.helpers.image_path("layout/reflection.png"), :border => 0)


I know tag(:img) exists elsewhere in this code, but I don't think it's necessary. Should be able to use quadicon_reflection_img here. Plus, I think doing this will clear up the CodeClimate complaint.

ok, I just changed it.

hayesr

Thanks @MaysaMacedo !

@dclarizio I'm 👍 on this when green.

skateman · 2017-05-04T19:58:28Z

app/decorators/physical_server_decorator.rb

+    "svg/vendor-#{label_for_vendor.downcase}.svg"
+  end
+
+  def img_for_health_state


health_state_fileicon sounds better, but let's ask @epwinchell &| @himdel

Just following the MIQ pattern.(e. g.: img_for_compliance, img_for_vendor, img_for_host_vendor and so on...)

AFAIK decorators don't have this pattern yet, so I'd like to ask the guys from above to check this out to not have a bad pattern that gets copied to other places 😉

AFAIK decorators there isn't any method wich has a img for a specific property yet as you realized, so to follow(not copy) a previous pattern sounds good for me. But we'll wait for other reviewers. ;)

Yeah, this is weird. In fact I'd just keep it where it was, since we have no standard way of doing this in decorators yet.

I mean, decorators are really useful if you want to get an icon for something you know is a model, but not sure which one.

Any use of the decorators for a method that is not available in any other decorator is ..a bit suspect..

(I mean, what's the point of doing @record.decorate.img_for_health_state if you know the type of the record and can't ever call it on any other kind.)

But.. this is a tech debt either way, so.. not saying you have to move it out, just that there is no point in putting it there :).

But: if you're creating a new decorator, please make sure it has one or both of the fonticon and fileicon methods, which return the right thing for that entity.

I agree with also having fonticon and fileicon methods. But the point of moving these other methods closer to the model is that they are more related to PhysicalServer. It's not super clear whose responsibility an image path is, but it's more likely the model's than the QuadiconHelper module. I also agree that having to know the type @record isn't great, but most of this code is type-checks on @record anyway.

Just added fonticon and fileicon methods, and also fixed the typo.

epwinchell

The images are fine. We'll replace them later with font icons.

walteraa · 2017-05-04T20:29:30Z

app/decorators/physical_server_decorator.rb

+    "svg/vendor-#{label_for_vendor.downcase}.svg"
+  end
+
+  def img_for_health_state


AFAIK decorators there isn't any method wich has a img for a specific property yet as you realized, so to follow(not copy) a previous pattern sounds good for me. But we'll wait for other reviewers. ;)

himdel · 2017-05-05T13:23:38Z

app/helpers/quadicon_helper.rb

+  end
+
+  def quadicon_named_for_base_model?(item)
+    %w(VmOrTempalte PhysicalServer).include?(item.class.base_model.name)


typo.. tempalte != template

himdel · 2017-05-05T13:29:44Z

Except for the typo, and missing fonticon or fileicon in the new decorator, LGTM :)

walteraa · 2017-05-05T13:33:43Z

@himdel in the case that doen't exists a useful to physical server fonticon/fileicon yet, we just need return nil? I need this information to fix this issues, can you help me?

himdel · 2017-05-05T13:35:48Z

@walteraa Sure.. I would not recommend returning nil .. look at what icon is shown when you see a list of servers in the UI.. it will either be an image (in which case you need fileicon) or a font icon (fonticon :)).

skateman · 2017-05-05T13:52:06Z

@walteraa if the icons are the same for each item regardless of their state you should to define self.fonticon and self.fileicon instead.

walteraa · 2017-05-05T13:57:54Z

Thank you guys!! @skateman @himdel

skateman · 2017-05-09T07:39:34Z

app/decorators/physical_server_decorator.rb

+    "svg/vendor-#{vendor.downcase}.svg"
+  end
+
+  def fonticon


As this method contains a static string, it is better to declare it as self.fonticon.

himdel · 2017-05-09T11:36:00Z

Thanks @MaysaMacedo, I just noticed one more thing..

img_for_physical_vendor .. this is just a duplicate of the vendor's fileicon method - icons for other entities should be the responsibility of their decorators, not PhysicalServerDecorator's.

So.. any time you call @record.decorate.img_for_physical_vendor, you should be calling @record.ext_management_system.decorate.fileicon instead.

(In fileicon, I have no idea whether vendor or label_for_vendor should be used.. but seems either works for now.)

Second, sorry for the confusion, but I'll ask you to move the other method (img_for_health_state) back to quadicon_helper.rb. Every other quadicon has that logic in quadicon helper, regardless of whether that's right or wrong, and it's less confusing to add the new method to a place which already contains 4 others than to a place where it would be unprecedented, and surprising. Eric is right in that we will eventually have the quadicon info in the decorator, but let's solve that then, not now.

(Also, what @skateman said about self.fonticon ;).)

Added Physical Server quadicon

MaysaMacedo · 2017-05-30T17:54:23Z

Closing to run travis again.

MaysaMacedo · 2017-05-30T19:49:51Z

@himdel @skateman the spec failures were fixed, now I think this PR is ok.

rodneyhbrown7 · 2017-06-01T17:10:57Z

@dclarizio Is this PR ready for a merge? Or there any additional comments?

skateman · 2017-06-01T17:26:18Z

@rodneyhbrown7 @MaysaMacedo I'm fine with this PR, but I'd wait for an ACK from @himdel. He's off this week and will be back on monday ...

dclarizio · 2017-06-01T21:05:17Z

@himdel feel free to merge if you think this is ready

chargio · 2017-06-06T13:43:45Z

@himdel Can we merge this?

himdel · 2017-06-07T15:16:40Z

Sorry, doesn't work...

FATAL -- : Error caught: [NameError] undefined local variable or method `size' for #<#<Class:0x007fafec760338>:0x007faffc18b470>
/home/himdel/manageiq-ui-classic/app/helpers/quadicon_helper.rb:233:in `render_physical_server_quadicon'
/home/himdel/manageiq-ui-classic/app/helpers/quadicon_helper.rb:166:in `quadicon_builder_factory'
/home/himdel/manageiq-ui-classic/app/helpers/quadicon_helper.rb:150:in `block in render_quadicon'

himdel · 2017-06-07T15:17:02Z

app/helpers/quadicon_helper.rb

+      output << flobj_img_simple(img_for_health_state(item), "d72")
+      output << flobj_img_simple('100/shield.png', "g72") unless item.get_policies.empty?
+    else
+      output << flobj_img_simple(size)


this size is never defined ..and does not really make sense because flobj_img_simple expects an image as a first arg

EDIT: Seems like other similar branches use either flobj_img_simple without args or flobj_img_simple("layout/base-single.png") (which is identical to the no args version).

So.. just remove that (size)?

himdel · 2017-06-07T15:23:44Z

app/helpers/quadicon_helper.rb

+      output << flobj_img_simple('100/shield.png', "g72") unless item.get_policies.empty?
+    else
+      output << flobj_img_simple(size)
+      output << flobj_img_simple(width * 1.8, item.ext_management_system.decorate.fileicon, "e72")


And the first arg here is dead too.. output << flobj_img_simple(item.ext_management_system.decorate.fileicon, "e72") should do the trick.

himdel · 2017-06-07T15:34:18Z

Another thing missing here is the My Settings change to be able to actually enable those quadicons. @MaysaMacedo you're adding the default values for settings(:quadicon, :physical_server) which is nice, but it just means you have to create a new DB to test this, or do a db change manually...

The right place to expose this is My Settings > Visual > Grid/Tile Icons (/configuration/change_tab?tab=1) - app/views/configuration/_ui_1.html.haml is the place you're looking for, but you'll also need to update get_form_vars in ConfigurationController.

EDIT: this seems to do the trick...

diff --git a/app/controllers/configuration_controller.rb b/app/controllers/configuration_controller.rb
index 7e4e64592f..0f5e77d504 100644
--- a/app/controllers/configuration_controller.rb
+++ b/app/controllers/configuration_controller.rb
@@ -580,6 +580,7 @@ class ConfigurationController < ApplicationController
       @edit[:new][:quadicons][:ems_cloud] = params[:quadicons_ems_cloud] == "true" if params[:quadicons_ems_cloud]
       @edit[:new][:quadicons][:host] = params[:quadicons_host] == "true" if params[:quadicons_host]
       @edit[:new][:quadicons][:vm] = params[:quadicons_vm] == "true" if params[:quadicons_vm]
+      @edit[:new][:quadicons][:physical_server] = params[:quadicons_physical_server] == "true" if params[:quadicons_physical_server]
       @edit[:new][:quadicons][:miq_template] = params[:quadicons_miq_template] == "true" if params[:quadicons_miq_template]
       if ::Settings.product.proto # Hide behind proto setting - Sprint 34
         @edit[:new][:quadicons][:service] = params[:quadicons_service] == "true" if params[:quadicons_service]
diff --git a/app/helpers/quadicon_helper.rb b/app/helpers/quadicon_helper.rb
index 909a14b7a0..0281977af9 100644
--- a/app/helpers/quadicon_helper.rb
+++ b/app/helpers/quadicon_helper.rb
@@ -230,8 +230,8 @@ module QuadiconHelper
       output << flobj_img_simple(img_for_health_state(item), "d72")
       output << flobj_img_simple('100/shield.png', "g72") unless item.get_policies.empty?
     else
-      output << flobj_img_simple(size)
-      output << flobj_img_simple(width * 1.8, item.ext_management_system.decorate.fileicon, "e72")
+      output << flobj_img_simple
+      output << flobj_img_simple(item.ext_management_system.decorate.fileicon, "e72")
     end
 
     if options[:typ] == :listnav
diff --git a/app/views/configuration/_ui_1.html.haml b/app/views/configuration/_ui_1.html.haml
index 4756cf2113..28dd2339e9 100644
--- a/app/views/configuration/_ui_1.html.haml
+++ b/app/views/configuration/_ui_1.html.haml
@@ -18,6 +18,7 @@
              [role_allows?(:feature => "host_show_list"),      _("Host"),                                 "host"],
              [role_allows?(:feature => "storage_show_list"),   ui_lookup(:table => "storages"),           "storage"],
              [true,                                           _("VM"),                                   "vm"],
+             [true,                                           _("Physical Server"),                                   "physical_server"],
              [true,                                           _("Template"),                             "miq_template"]].each do |icons_checkbox|
             - if icons_checkbox[0]
               .form-group

himdel · 2017-06-07T15:37:55Z

app/helpers/quadicon_helper.rb

+      output << flobj_img_simple("layout/base.png")
+
+      output << flobj_p_simple("a72", (item.host ? 1 : 0))
+      output << flobj_img_simple("svg/currentstate-#{h(item.power_state.downcase)}.svg", "b72")


may want to use item.power_state.try(:downcase) - hard to tell without real data, but this crashes when a server does not have a power_state which is technically possible

himdel · 2017-06-07T15:40:53Z

app/helpers/quadicon_helper.rb

+    case item.health_state
+    when "Valid"    then "svg/healthstate-normal.svg"
+    when "Critical" then "svg/healthstate-critical.svg"
+    when "None"     then "svg/healthstate-unknown.svg"


Again hard to tell without real data, but when health_state is nil, you get an ugly base-single in the fourth quadrant, may want to make "None" the default case.

miq-bot · 2017-06-07T17:23:51Z

Checked commits MaysaMacedo/manageiq-ui-classic@1acec52~...5a1e210 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 0 offenses detected
Everything looks fine. 👍

MaysaMacedo · 2017-06-07T17:38:45Z

@himdel I've made the changes you asked for. Thanks for the heads up.

himdel · 2017-06-07T17:43:51Z

Thanks, verified in the UI, both branches now work 👍 .. merged :)

skateman suggested changes Apr 27, 2017

View reviewed changes

gabrielsvinha reviewed Apr 27, 2017

View reviewed changes

gabrielsvinha mentioned this pull request Apr 28, 2017

[WIP] Physical Server Pages #1169

Closed

MaysaMacedo force-pushed the physical_server_quadicon branch from 1a2c25b to 576d53b Compare May 2, 2017 12:44

dclarizio added compute/physical infrastructure enhancement fine/no graphics labels May 3, 2017

dclarizio requested review from epwinchell and hayesr May 3, 2017 22:03

dclarizio self-assigned this May 3, 2017

hayesr suggested changes May 4, 2017

View reviewed changes

hayesr reviewed May 4, 2017

View reviewed changes

hayesr approved these changes May 4, 2017

View reviewed changes

skateman reviewed May 4, 2017

View reviewed changes

epwinchell approved these changes May 4, 2017

View reviewed changes

walteraa approved these changes May 5, 2017

View reviewed changes

himdel reviewed May 5, 2017

View reviewed changes

gabrielsvinha approved these changes May 8, 2017

View reviewed changes

MaysaMacedo force-pushed the physical_server_quadicon branch from 18cfa2a to 39a8a8a Compare May 8, 2017 20:03

skateman reviewed May 9, 2017

View reviewed changes

MaysaMacedo force-pushed the physical_server_quadicon branch from 58d8586 to 25b7a34 Compare May 9, 2017 20:02

MaysaMacedo force-pushed the physical_server_quadicon branch from fc0d428 to 25b7a34 Compare May 17, 2017 12:50

MaysaMacedo mentioned this pull request May 30, 2017

Added Lenovo vendor to physical server factory ManageIQ/manageiq#15129

Merged

MaysaMacedo added 7 commits May 30, 2017 14:31

Added quadicon

1acec52

Added Physical Server quadicon

086ad9d

Added Physical Server quadicon

Added physicalserver resource type to display quadicon

94dd5fc

Added physical server decorator

d376d23

Added fileicon and fonticon

6b998c9

Refactored physical server decorator

34a428c

Refactored access to physical server fileicon

055ad7b

MaysaMacedo force-pushed the physical_server_quadicon branch from d3c0cbb to e56ae4d Compare May 30, 2017 17:32

MaysaMacedo closed this May 30, 2017

MaysaMacedo reopened this May 30, 2017

himdel reviewed Jun 7, 2017

View reviewed changes

Fixed spec failure

5a1e210

MaysaMacedo force-pushed the physical_server_quadicon branch from e56ae4d to 5a1e210 Compare June 7, 2017 17:20

himdel merged commit ba8bd2e into ManageIQ:master Jun 7, 2017

himdel added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 7, 2017

Physical Server quadicon #1173

Physical Server quadicon #1173

Conversation

MaysaMacedo commented Apr 26, 2017 • edited

skateman left a comment • edited

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

skateman commented Apr 28, 2017

hayesr left a comment • edited

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

walteraa May 4, 2017 • edited

Choose a reason for hiding this comment

skateman May 17, 2017 • edited

Choose a reason for hiding this comment

himdel May 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hayesr left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaysaMacedo May 8, 2017 • edited

Choose a reason for hiding this comment

epwinchell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented May 5, 2017

walteraa commented May 5, 2017

himdel commented May 5, 2017

skateman commented May 5, 2017

walteraa commented May 5, 2017

Choose a reason for hiding this comment

himdel commented May 9, 2017 • edited

MaysaMacedo commented May 30, 2017

MaysaMacedo commented May 30, 2017

rodneyhbrown7 commented Jun 1, 2017

skateman commented Jun 1, 2017

dclarizio commented Jun 1, 2017

chargio commented Jun 6, 2017

himdel commented Jun 7, 2017

himdel Jun 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Jun 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jun 7, 2017

MaysaMacedo commented Jun 7, 2017

himdel commented Jun 7, 2017

MaysaMacedo commented Apr 26, 2017 •

edited

skateman left a comment •

edited

hayesr left a comment •

edited

walteraa May 4, 2017 •

edited

skateman May 17, 2017 •

edited

himdel May 17, 2017 •

edited

MaysaMacedo May 8, 2017 •

edited

himdel commented May 9, 2017 •

edited

himdel Jun 7, 2017 •

edited

himdel commented Jun 7, 2017 •

edited