Change the way that network device details are displayed #3398

skovic · 2018-02-12T19:48:36Z

This PR changes the way that network device details are displayed on the physical server summary page. The existing Network Devices table was removed, and a new row named Network Devices was added to the Properties table. In this new row, the number of network devices a server has is displayed, and this row can be clicked on to go to a page displaying all network devices. Clicking on an entry on the network devices page takes the user to the network device summary page for that device.

This PR depends on changes made in ManageIQ/manageiq#16996

skovic · 2018-02-12T19:48:55Z

@miq-bot add_label wip

skovic · 2018-02-12T20:14:22Z

@AparnaKarve In this PR, I have created a controller, etc. for guest_devices so that network device details can be displayed. Physical server network device details are currently stored in the guest_devices database table with a device type of ethernet. When you click on the Guest Devices row in the UI, it will take you to a page displaying all guest devices. A couple of questions: 1. Is there a way to only display guest devices with a type of "ethernet" when clicking on this link? Maybe somehow specify a filter in the link? 2. Is there a way to dynamically change the "Guest Devices" heading for the table to "Network Devices" when navigating to this page from the link? Please see screenshots above. Thanks

AparnaKarve · 2018-02-13T00:17:52Z

Is there a way to only display guest devices with a type of "ethernet" when clicking on this link? Maybe somehow specify a filter in the link?

@skovic You might have to use something called named_scope to display the filtered list.

This is a broader picture of how named_scope is implemented -

The model should have a class method that would look something like this -

def self.with_ethernet_type
  where(:device_type => "ethernet")
end

And in the controller, you would have to override show_list implementation -

-include Mixins::GenericListMixin


+def show_list
+ options = {:model => "GuestDevice", :named_scope => [:with_ethernet_type]}
+ process_show_list(options)
+end

Also look up some existing named_scope examples to get a better idea.

Is there a way to dynamically change the "Guest Devices" heading for the table to "Network Devices" when navigating to this page from the link?

Add this line to manageiq/locale/en.yml

GuestDevice:  Network Device

skovic · 2018-02-13T18:51:11Z

@AparnaKarve I was able to get the page to have a "Network Devices" heading and display only the ethernet devices using your suggestions. Thanks

…ist view

skovic · 2018-02-14T21:38:58Z

@AparnaKarve Is there a way to only display the guest devices on the Guest Devices page that are associated with a given server? For example, if you click on the Network Devices row from the physical server summary page of a particular server, the Guest Devices page should show the guest devices that are associated with that server instead of all guest devices. Thanks

AparnaKarve · 2018-02-14T22:29:17Z

app/helpers/physical_server_helper/textual_summary.rb

+    end
+    device
+  end
+


You will need a nested display here, as shown below -
(please make adjustments to the below method as necessary)

def textual_network_devices hardware_nics = @record.hardware.nics.count h = {:label => _("Network Devices"), :icon => "ff ff-network-card", :value => hardware_nics} if hardware_nics > 0 h[:link] = "/physical_server/#{@record.id}?display=guest_devices" end h end

Note that "/physical_server/#{@record.id}?display=guest_devices" will work if there is an association between a physical server and a guest device.
Is it possible to add that association in the model if it does not already exist?

@AparnaKarve I created an association between physical server and guest device, but "/physical_server/#{@record.id}?display=guest_devices" doesn't seem to be working. When I click on the link, I get a page with an error that says "The page you were looking for doesn't exist" and in development.log, I get these errors:

[----] I, [2018-02-14T06:43:08.764242 #6493:2abc084e2ea8] INFO -- : Started GET "/physical_server/2?display=guest_devices" for 127.0.0.1 at 2018-02-14 06:43:08 -0500 [----] F, [2018-02-14T06:43:08.790984 #6493:2abc084e2ea8] FATAL -- : [----] F, [2018-02-14T06:43:08.791066 #6493:2abc084e2ea8] FATAL -- : ActionController::RoutingError (No route matches [GET] "/physical_server/2"): [----] F, [2018-02-14T06:43:08.791093 #6493:2abc084e2ea8] FATAL -- : [----] F, [2018-02-14T06:43:08.791116 #6493:2abc084e2ea8] FATAL -- : actionpack (5.0.6) lib/action_dispatch/middleware/debug_exceptions.rb:53:in `call' actionpack (5.0.6) lib/action_dispatch/middleware/show_exceptions.rb:31:in `call' railties (5.0.6) lib/rails/rack/logger.rb:36:in `call_app' railties (5.0.6) lib/rails/rack/logger.rb:26:in `call' sprockets-rails (3.2.1) lib/sprockets/rails/quiet_assets.rb:13:in `call' actionpack (5.0.6) lib/action_dispatch/middleware/request_id.rb:24:in `call' rack (2.0.4) lib/rack/method_override.rb:22:in `call' rack (2.0.4) lib/rack/runtime.rb:22:in `call' activesupport (5.0.6) lib/active_support/cache/strategy/local_cache_middleware.rb:28:in `call' actionpack (5.0.6) lib/action_dispatch/middleware/executor.rb:12:in `call' actionpack (5.0.6) lib/action_dispatch/middleware/static.rb:136:in `call' rack (2.0.4) lib/rack/sendfile.rb:111:in `call' railties (5.0.6) lib/rails/engine.rb:522:in `call' puma (3.7.1) lib/puma/configuration.rb:232:in `call' puma (3.7.1) lib/puma/server.rb:578:in `handle_request' puma (3.7.1) lib/puma/server.rb:415:in `process_client' puma (3.7.1) lib/puma/server.rb:275:in `block in run' puma (3.7.1) lib/puma/thread_pool.rb:120:in `block in spawn_thread'

@AparnaKarve Is there another change that I need to make somewhere to get this to work? Thanks

@skovic Looks like I missed a show action in the above url
It should have been -

"/physical_server/show/#{@record.id}?display=guest_devices"

I think you are just missing this method in the physical server controller -

def self.display_methods %w(guest_devices) end

@AparnaKarve Thanks. The page loads now, but it doesn't load a table with guest device details -- it only has the heading. I think I have the guest device / physical server relation set up correctly; I am able to access @record.guest_devices from the physical server textual_summary.rb file -- it returns a list of guest devices associated with the server. What are some other things I could check? Please see the screenshot below:

I am able to access @record.guest_devices from the physical server textual_summary.rb file -- it returns a list of guest devices associated with the server.

Ok, that is good.

So now you need the actual display method in your Physical Server controller -

def display_guest_devices nested_list(GuestDevice, {:named_scope => :with_ethernet_type}) end

Note that the nested list implementation does not use guest_device_controller - so if it's not being used for any other use case, you can remove it

@AparnaKarve I added the display_guest_devices method, but the page still doesn't display the table of devices. It still looks like the screenshot above. There doesn't seem to be any associated errors in the logs. How can I further debug this? Thanks

@skovic Can you push your latest code - that way I can troubleshoot it better.
Thanks.

AparnaKarve · 2018-02-14T22:31:08Z

app/helpers/physical_server_helper/textual_summary.rb

@@ -2,7 +2,7 @@ module PhysicalServerHelper::TextualSummary
  def textual_group_properties
    TextualGroup.new(
      _("Properties"),
-      %i(name model product_name manufacturer machine_type serial_number ems_ref capacity memory cores health_state loc_led_state)
+      %i(name model product_name manufacturer machine_type serial_number ems_ref capacity memory cores network_devices health_state loc_led_state)


Specify network_devices in a new Textual Group - Relationships

def textual_group_relationships TextualGroup.new(_("Relationships"), %i(network_devices)) end

Actually, on second thoughts, displaying it as 'Properties' (the way you have it currently), should be fine.

AparnaKarve · 2018-02-14T22:36:23Z

@skovic It looks like you need a nested display list here.
I have made some inline comments for the necessary changes.

skovic · 2018-02-20T19:34:55Z

@AparnaKarve The latest code has been pushed. Thanks

AparnaKarve · 2018-02-21T01:26:22Z

@skovic I could not test your exact requirement since I did not have the corresponding model changes. Although it looks like, to display the nested list, you will need this in your show.haml -

 #main_div
-  - if %w(physical_servers).include?(@display)
+  - if %w(guest_devices physical_servers).include?(@display)
     = render :partial => "layouts/gtl", :locals => {:action_url => "show/#{@record.id}"}
   - else
     - case @showtype

skovic · 2018-02-21T16:00:32Z

@AparnaKarve The change to show.haml fixed the problem. The page now displays the table of devices correctly. Thank you for looking into this.

skovic · 2018-02-21T21:07:47Z

@miq-bot add_label compute/physical infrastructure

skovic · 2018-02-21T21:13:53Z

@miq-bot remove_label wip

skovic · 2018-02-21T21:20:50Z

@agrare Also this PR. Thanks

AparnaKarve · 2018-02-21T21:38:38Z

@skovic Thanks for the update!

AparnaKarve · 2018-02-21T21:43:03Z

app/helpers/physical_server_helper/textual_summary.rb

@@ -163,6 +155,15 @@ def textual_health_state
    {:label => _("Health State"), :value => @record.health_state}
  end

+  def textual_network_devices
+    hardware_nics_count = @record.hardware.nics.count


@record.guest_devices would be the same as @record.hardware.nics, right?

Do you want to use @record.guest_devices.count here in order to be consistent with the nested display list?
i.e. both the link and the actual list rely on the PhysicalServer/GuestDevice association

@AparnaKarve @record.hardware.nics is different because it returns guest devices that have a device_type of 'ethernet' which is what we want here. @record.guest_devices would return all guest devices including storage adapters, ports, etc. which we don't want included in the count.

Ok, thanks for that clarification.
@record.hardware.nics makes sense to get the count for the 'ethernet' guest devices

agrare · 2018-02-26T16:26:51Z

Core PR has been merged

skovic · 2018-02-27T16:02:40Z

@AparnaKarve @agrare Any other changes needed in this PR? Thanks

AparnaKarve · 2018-02-27T17:19:09Z

app/controllers/guest_device_controller.rb

+  def show_list
+    options = {:model => "GuestDevice", :named_scope => [:with_ethernet_type]}
+    process_show_list(options)
+  end


@skovic I think you can remove the show_list override from here since we are not using it anymore.

@AparnaKarve I removed the override for show_list, and named_scope still seems to work. How is it still working without this override? Just curious. Thanks

The nested display list is using named_scope, and that's all we need to make named_scope work.

Note that the nested display list is an action that is called on physical_server_controller (the parent controller )and not on guest_device_controller.
In other words, GuestDeviceController#show_list is not being used at all -- hence you can remove that method from the controller and even routes.rb.

@AparnaKarve OK, got it. Thank you for the explanation. I'll remove the override.

AparnaKarve · 2018-02-27T17:32:20Z

@skovic Would you be able to add a specs for the following -

for the show method in physical_server_controller that displays the nested display list of guest devices?
A show_list spec for the new controller - guest_device_controller

skovic · 2018-02-28T19:02:23Z

@AparnaKarve I added tests for the controllers as requested. I was able to run the tests successfully locally, so I'm not sure why the Travis run failed.

miq-bot · 2018-02-28T19:42:53Z

Checked commits skovic/manageiq-ui-classic@10e0773~...f6043e3 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
13 files checked, 0 offenses detected
Everything looks fine. 🏆

AparnaKarve · 2018-03-01T22:24:38Z

@skovic LGTM

@dclarizio Good to go when green

skovic · 2018-03-02T18:13:43Z

@dclarizio Tests are passing now

miq-bot changed the title ~~Change the way network device details are displayed~~ [WIP] Change the way network device details are displayed Feb 12, 2018

miq-bot added the wip label Feb 12, 2018

skovic changed the title ~~[WIP] Change the way network device details are displayed~~ [WIP] Change the way that network device details are displayed Feb 12, 2018

skovic mentioned this pull request Feb 13, 2018

Rename new guest device pages ManageIQ/manageiq#16996

Merged

skovic added 3 commits February 14, 2018 15:38

Change the way network device details are displayed

10e0773

Update GuestDeviceController to only display network devices on the l…

826e5b4

…ist view

Create network device summary page

0ed1f69

Update code so only the network devices of a specific server are shown

024ab54

AparnaKarve reviewed Feb 14, 2018

View reviewed changes

skovic force-pushed the network-device-ui-redesign branch from 10f63d8 to 024ab54 Compare February 20, 2018 19:26

skovic added 4 commits February 20, 2018 18:01

Fix network devices table not showing up

f50291f

Remove device type from table

46e6508

Rework textual_network_devices method based on review feedback

c8052fc

Address rubocop issues

786ad89

miq-bot added the compute/physical infrastructure label Feb 21, 2018

miq-bot changed the title ~~[WIP] Change the way that network device details are displayed~~ Change the way that network device details are displayed Feb 21, 2018

miq-bot removed the wip label Feb 21, 2018

AparnaKarve reviewed Feb 21, 2018

View reviewed changes

AparnaKarve reviewed Feb 27, 2018

View reviewed changes

dclarizio added enhancement gaprindashvili/no labels Feb 27, 2018

dclarizio self-assigned this Feb 27, 2018

skovic added 2 commits February 27, 2018 14:09

Remove show_list override from the guest device controller

3be4f62

Add spec tests for network device pages

f6043e3

skovic closed this Mar 1, 2018

skovic reopened this Mar 1, 2018

skovic closed this Mar 2, 2018

skovic reopened this Mar 2, 2018

dclarizio merged commit 85ef2d8 into ManageIQ:master Mar 2, 2018

dclarizio added this to the Sprint 81 Ending Mar 12, 2018 milestone Mar 2, 2018

AparnaKarve mentioned this pull request Mar 13, 2018

TextualSummaries: part of client side rendering with React. #3420

Merged

martinpovolny mentioned this pull request Mar 16, 2018

TextualSummary: remove unneeded physical infra partials #3638

Merged

Change the way that network device details are displayed #3398

Change the way that network device details are displayed #3398

Conversation

skovic commented Feb 12, 2018 • edited Loading

skovic commented Feb 12, 2018

skovic commented Feb 12, 2018

AparnaKarve commented Feb 13, 2018

skovic commented Feb 13, 2018

skovic commented Feb 14, 2018

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

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

AparnaKarve commented Feb 14, 2018

skovic commented Feb 20, 2018

AparnaKarve commented Feb 21, 2018

skovic commented Feb 21, 2018

skovic commented Feb 21, 2018 • edited Loading

skovic commented Feb 21, 2018

skovic commented Feb 21, 2018

AparnaKarve commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare commented Feb 26, 2018

skovic commented Feb 27, 2018

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

AparnaKarve commented Feb 27, 2018

skovic commented Feb 28, 2018

miq-bot commented Feb 28, 2018

AparnaKarve commented Mar 1, 2018

skovic commented Mar 2, 2018

skovic commented Feb 12, 2018 •

edited

Loading

skovic commented Feb 21, 2018 •

edited

Loading