Add Physical Server relationship for Host summary page #1440

gabrielsvinha · 2017-05-25T20:06:22Z

Adds physical server link to Host summary page in relationship label.

gabrielsvinha · 2017-05-25T20:13:13Z

@miq-bot add_labels compute/physical infrastructure, enhancement, textual summaries

AparnaKarve · 2017-05-25T21:45:15Z

app/helpers/host_helper/textual_summary.rb

@@ -235,6 +235,10 @@ def textual_cluster
    h
  end

+  def textual_physical_server
+    {:label => _("Physical Server"), :value => @record.physical_server.try(:name), :icon => "pficon pficon-server", :link => url_for(:controller => 'physical_server', :action => 'show', :id => @record.physical_server_id)}


@gabrielsvinha Not all hosts are expected to have a Physical Server associated with them, therefore, I recommend adding a conditional here.

{:label => _("Physical Server"), :value => @record.physical_server.try(:name), :icon => "pficon pficon-server", :link => url_for(:controller => 'physical_server', :action => 'show', :id => @record.physical_server_id)} if @record.physical_server_id

AparnaKarve · 2017-05-25T21:50:40Z

Could you also add the Physical server relationship in the left navigation view?

gabrielsvinha · 2017-05-25T22:13:12Z

Sure. But there is no point adding a count for physical servers in the listnav, their relationship is one-to-one. @AparnaKarve

AparnaKarve · 2017-05-25T23:30:57Z

Ok, makes sense.
Listnav is good as is.

AparnaKarve · 2017-05-26T21:47:31Z

LGTM.

@dclarizio Good to go.

dclarizio · 2017-05-30T17:43:22Z

@gabrielsvinha I'm pretty sure we try to make the listnav match the summary screen contents and have seen links to single relationships, such as Host to Provider, in the listnav. @AparnaKarve is checking to see what we would do in the listnav if there is no relationship to an entity.

AparnaKarve · 2017-05-30T19:13:02Z

@gabrielsvinha Let's revisit the discussion #1440 (comment) and #1440 (comment).

So, for 1:1 Relationships, the listnav should display the Relationship and the link to that should land on the Summary page of the item.
Meaning, let's display the Physical Server relationship in the Hosts listnav with a link pointing to the Physical Server show page (Summary page)

Please take a look at the screenshot below -

Two things to note here.

Notice how Providers:SCVMM (which has a 1:1 with the Host) is shown in the listnav, so we want something similar for Physical Server
Notice Cluster: None on the right.
Maybe we should take this approach too. i.e. display Physical Server: None when a host does not have a Physical server associated with it. And also display it in the listnav as purely an info item with no links (since a link is not possible in this case)

Let me know if I can assist in any way. Thanks.

gabrielsvinha · 2017-05-30T19:20:41Z

@AparnaKarve @dclarizio Got it, working on it right now.

gabrielsvinha · 2017-06-01T15:53:05Z

@AparnaKarve Done it:

AparnaKarve · 2017-06-01T18:17:59Z

Can you add Physical Server as the last entry in the listnav, that way we maintain the same order like in the Summary page?

AparnaKarve · 2017-06-01T18:27:21Z

app/helpers/host_helper/textual_summary.rb

+      {:label => _("Physical Server"), :value => "None", :icon => "pficon pficon-server"}
+    end
+  end
+


Flip the order here that way we do not have to use ! (negative conditional)
Also note the use of _("None") which takes care of i18n.

if @record.physical_server_id.nil? {:label => _("Physical Server"), :value => _("None"), :icon => "pficon pficon-server"} else {:label => _("Physical Server"), :value => @record.physical_server.try(:name), :icon => "pficon pficon-server", :link => url_for(:controller => 'physical_server', :action => 'show', :id => @record.physical_server_id)} end

AparnaKarve · 2017-06-01T18:32:54Z

app/views/layouts/listnav/_host.html.haml

@@ -77,6 +77,12 @@
              ems_infra_path(@record.ext_management_system),
              :title => _("Show this parent %{provider} for this %{host}") % {:host => host_title, :provider => ui_lookup(:table => "ems_infra")})

+        - if role_allows?(:feature => "physical_server_show") && [email protected]_server.nil?
+          %li
+            = link_to("#{ui_lookup({:table => "physical_servers"})}: #{@record.physical_server.name}",


Currently the text is being shown as plural Physical Servers... we need it to be singular.
I have created ManageIQ/manageiq#15275 with which ui_lookup should work correctly and show the singular text.

In addition to this, you will also need to consider a case when a host does not have a Physical Server associated with it and then display Physical Server: None in the listnav without a link.

- if role_allows?(:feature => "physical_server_show") && @record.physical_server.nil? = li_link(:if => false, :text => "#{ui_lookup({:table => "physical_servers"})}: #{_('None')}")

AparnaKarve · 2017-06-05T18:35:05Z

app/helpers/host_helper/textual_summary.rb

@@ -235,6 +235,14 @@ def textual_cluster
    h
  end

+  def textual_physical_server
+    if @record.physical_server_id.nil?
+      {:label => _("Physical Server"), :value => "None", :icon => "pficon pficon-server"}


Change "None" to _("None")

AparnaKarve · 2017-06-05T18:42:00Z

app/views/layouts/listnav/_host.html.haml

@@ -119,6 +119,15 @@
            :record_id     => @record.id,
            :title         => _("Show %{host} drift history") % {:host => host_title})

+        - if role_allows?(:feature => "physical_server_show") && [email protected]_server.nil?
+          %li
+            = link_to("#{ui_lookup(:table => "physical_servers")}: #{@record.physical_server.name}",


For this ui_lookup(:table => "physical_servers") (note the singular use of :table as opposed to :tables), I was expecting to see a singular Physical Server, but instead seeing a plural (Physical Servers)

@mzazrivec Any idea why?

I believe it's the :table => physical _servers, I tested :table => physical _server and it looked good.

Yes, I'm aware that :table => physical _server seems to work but the table name is physical_servers and not physical_server. Hence, it does not seem right to use physical_server

The other option would be to go with :model => 'PhysicalServer', which works perfectly well for singular and plural cases
:model => 'PhysicalServer' gives Physical Server
:models => 'PhysicalServer' gives Physical Servers

Let me know which one is the best.

I'm inclined to use :model => 'PhysicalServer'since it is working the way it's supposed to.

Got it. Doing it.

AparnaKarve · 2017-06-05T18:44:50Z

@gabrielsvinha Other than the few minor things above, everything else looks good.

AparnaKarve · 2017-06-05T23:25:53Z

In the next iteration can you address the rubocop issue?

gabrielsvinha · 2017-06-05T23:31:22Z

@AparnaKarve Any idea what is this rubocop issue about?

AparnaKarve · 2017-06-06T00:07:44Z

The haml-lint issue gets resolved if you write the link_to method like this:

%li
  - physical_server_show = {:controller => "physical_server", :action => 'show', :id => @record.physical_server.id}
  = link_to("#{ui_lookup(:table => "physical_servers")}: #{@record.physical_server.name}",
              physical_server_show,
              :title => _("Show Physical Server"))

On second thoughts, don't worry about fixing this lint error. Leave it as is. Thanks.

gabrielsvinha · 2017-06-06T00:14:12Z

Do not worry, it will not be a problem.

miq-bot · 2017-06-06T11:13:39Z

Checked commit gabrielsvinha@a605548 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

- Adds physical server to host summary page in relationships label

AparnaKarve · 2017-06-06T14:26:53Z

@dclarizio This can be merged now. Thanks.

miq-bot added compute/physical infrastructure enhancement textual summaries labels May 25, 2017

dclarizio assigned AparnaKarve May 25, 2017

dclarizio requested a review from AparnaKarve May 25, 2017 20:57

dclarizio assigned dclarizio and unassigned AparnaKarve May 25, 2017

dclarizio added the fine/no label May 25, 2017

AparnaKarve reviewed May 25, 2017

View reviewed changes

gabrielsvinha force-pushed the host_relationship branch 2 times, most recently from 76ddf16 to e9d1e51 Compare May 26, 2017 13:16

gabrielsvinha force-pushed the host_relationship branch from e9d1e51 to 9635cc5 Compare June 1, 2017 15:50

gabrielsvinha force-pushed the host_relationship branch from 9635cc5 to 3dc9eea Compare June 1, 2017 16:18

AparnaKarve reviewed Jun 1, 2017

View reviewed changes

gabrielsvinha force-pushed the host_relationship branch 5 times, most recently from 8bf5f58 to 33e51b5 Compare June 5, 2017 12:50

AparnaKarve reviewed Jun 5, 2017

View reviewed changes

gabrielsvinha force-pushed the host_relationship branch from 33e51b5 to 3bbf51e Compare June 5, 2017 21:32

gabrielsvinha force-pushed the host_relationship branch from 3bbf51e to a605548 Compare June 6, 2017 11:12

Add physical server relationship

a605548

- Adds physical server to host summary page in relationships label

dclarizio merged commit 03ea79c into ManageIQ:master Jun 6, 2017

dclarizio added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Physical Server relationship for Host summary page #1440

Add Physical Server relationship for Host summary page #1440

gabrielsvinha commented May 25, 2017

gabrielsvinha commented May 25, 2017

AparnaKarve May 25, 2017

gabrielsvinha May 26, 2017

AparnaKarve commented May 25, 2017

gabrielsvinha commented May 25, 2017

AparnaKarve commented May 25, 2017

AparnaKarve commented May 26, 2017

dclarizio commented May 30, 2017

AparnaKarve commented May 30, 2017

gabrielsvinha commented May 30, 2017

gabrielsvinha commented Jun 1, 2017

AparnaKarve commented Jun 1, 2017

AparnaKarve Jun 1, 2017

AparnaKarve Jun 1, 2017

AparnaKarve Jun 1, 2017 •

edited

Loading

gabrielsvinha Jun 5, 2017

AparnaKarve Jun 5, 2017

AparnaKarve Jun 5, 2017

gabrielsvinha Jun 5, 2017

gabrielsvinha Jun 5, 2017

AparnaKarve Jun 5, 2017

gabrielsvinha Jun 5, 2017

AparnaKarve Jun 5, 2017 •

edited

Loading

gabrielsvinha Jun 5, 2017

AparnaKarve commented Jun 5, 2017

AparnaKarve commented Jun 5, 2017

gabrielsvinha commented Jun 5, 2017

AparnaKarve commented Jun 6, 2017

gabrielsvinha commented Jun 6, 2017

miq-bot commented Jun 6, 2017

AparnaKarve commented Jun 6, 2017

Add Physical Server relationship for Host summary page #1440

Add Physical Server relationship for Host summary page #1440

Conversation

gabrielsvinha commented May 25, 2017

gabrielsvinha commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve commented May 25, 2017

gabrielsvinha commented May 25, 2017

AparnaKarve commented May 25, 2017

AparnaKarve commented May 26, 2017

dclarizio commented May 30, 2017

AparnaKarve commented May 30, 2017

gabrielsvinha commented May 30, 2017

gabrielsvinha commented Jun 1, 2017

AparnaKarve commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve Jun 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

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 Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve commented Jun 5, 2017

AparnaKarve commented Jun 5, 2017

gabrielsvinha commented Jun 5, 2017

AparnaKarve commented Jun 6, 2017

gabrielsvinha commented Jun 6, 2017

miq-bot commented Jun 6, 2017

AparnaKarve commented Jun 6, 2017

AparnaKarve Jun 1, 2017 •

edited

Loading

AparnaKarve Jun 5, 2017 •

edited

Loading